Messages in this thread | | | Date | Thu, 23 Jun 2016 09:23:09 +0200 | From | Jacek Anaszewski <> | Subject | Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation |
| |
On 06/22/2016 04:25 PM, Florian Vaussard wrote: > Hi Jacek, > > Le 22. 06. 16 à 10:51, Jacek Anaszewski a écrit : >> Hi Florian, >> >> On 06/22/2016 08:08 AM, Florian Vaussard wrote: >>> Hi Jacek, >>> >>> Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit : >>>> Hi Florian, >>>> >>>> Thanks for the patch. I have two remarks below. >>>> >>>> On 06/21/2016 09:29 AM, Florian Vaussard wrote: >>>>> Add device tree binding documentation for On Semiconductor NCP5623 I2C >>>>> LED driver. The driver can independently control the PWM of the 3 >>>>> channels with 32 levels of intensity. >>>>> >>>>> The current delivered by the current source can be controlled using the >>>>> led-max-microamp property. In order to control this value, it is also >>>>> necessary to know the current on the Iref pin, hence the >>>>> onnn,led-iref-microamp property. It is usually set using an external >>>>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V. >>>>> >>>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch> >>>>> --- >>>>> .../devicetree/bindings/leds/leds-ncp5623.txt | 44 >>>>> ++++++++++++++++++++++ >>>>> 1 file changed, 44 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt >>>>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt >>>>> new file mode 100644 >>>>> index 0000000..0dc8345 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt >>>>> @@ -0,0 +1,44 @@ >>>>> +* ON Semiconductor - NCP5623 3-Channel LED Driver >>>>> + >>>>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each >>>>> +channel can be independently set using 32 levels. Each LED is represented >>>>> +as a sub-node of the device. >>>>> + >>>>> +Required properties: >>>>> + - compatible: Should be "onnn,ncp5623" >>>>> + - reg: I2C slave address (fixed to 0x38) >>>>> + - #address-cells: must be 1 >>>>> + - #size-cells: must be 0 >>>>> + - onnn,led-iref-microamp: Current on the Iref pin in microampere >>>> >>>> I think that you don't need this property. Just provide the formula for >>>> calculating led-max-microamp value, similarly as you're doing that in >>>> the commit message. >>>> >>> >>> I am not completely sure to understand your suggestion. So at the end, I have to >>> compute the value of the register (let call it 'ILED') that I need to send to >>> chip to configure the current source. The formula is: >>> >>> ILED = 31 - 2400*Iref/led-max-microamp >> >> led-max-microamp is the maximum current value for given LED. >> According to the documentation it can be calculated as follows: >> >> ILEDmax = Iref * 2400 / (31 - n) >> >> Since this is global setting for all LEDs, then I'd always set n to 30, >> and calculate max_brightness value for each LED separately, basing on >> led-max-microamp property value. Effectively, I'm revoking my previous >> statement about setting max_brightness to fixed level. >> > > Ok your proposal simplifies a bit the handling. Thus ILEDmax of the current > source would be always equal to Iref * 2400 and we use the PWM to limit the > current inside the LED. The only downside of this approach is a reduced number > of possible PWM steps, thus a limited number of RGB colors.
Yes, but by max_brightness being always 31, lowering led-max-microamp results in decreasing the amount of current per brightness level. Effectively, a human ability to notice perceived brightness level change also decreases then.
In the approach I proposed this limitation is reflected in reduced amount of available brightness levels.
> Regarding the DT binding, this would mean something like this: > > ncp5623@38 { > #address-cells = <1>; > #size-cells = <0>; > compatible = "onnn,ncp5623"; > reg = <0x38>; > led-max-microamp = <30000>;
Please drop it from here. It doesn't need to be configurable. You can hard code this in the driver.
> > ledr@0 { > label = "ncp:power:red"; > reg = <0>; > linux,default-trigger = "default-on"; > led-max-microamp = <5000>;
Is 5mA the maximum allowed current value for the LEDs on the board you're using? Is brightness level change easily noticeable by max current set to 5mA and max_brightness set to 31? It would be good to empirically check this configuration.
> }; > > ledb@1 { > label = "ncp:power:blue"; > reg = <1>; > led-max-microamp = <5000>; > }; > > ledg@2 { > label = "ncp:power:green"; > reg = <2>; > led-max-microamp = <5000>; > }; > }; > > The led-max-microamp property of the root node is used to infer Iref, and the > led-max-microamp property inside each LED node is used to compute the maximum > allowed PWM ratio (thus max_brightness). > > Would it be fine like this? > >> You can compare drivers/leds/leds-aat1290.c and its bindings, as it >> uses similar approach. >> > > Thanks for the pointer, interesting reading. In this case the flash-max-microamp > property is implicitly used to get the value of Rset, and led-max-microamp is > used to compute the flash/movie-mode ratio. Indeed similar but not exactly the > same, as the NCP5623 allows a finer control on the current using one register to > configure the current source and one register for the PWM.
Right, but it shows how led-max-microamp can be used to infer max_brightness level. This is quite new DT property with not too many users, because previously LED class drivers had been defining max_brightness directly in a Device Tree. Nonetheless brightness level was eventually considered not suitable unit for describing hardware property.
-- Best regards, Jacek Anaszewski
| |