Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Florian Vaussard <florian.vaussard@heig-vd.ch>]

Hi Jacek,

On 06/23/2016 09:23 AM, Jacek Anaszewski wrote:
> 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.
> 

It is not user configurable, but it is a hardware configuration imposed by the
bias resistor on the Iref pin (ILEDmax = 2400*Iref = 2400*0.6V/Rbias). So I
cannot hard code it as it can change from one design to another. And I need this
piece of information to compute the maximum allowable PWM ratio.

>>
>>     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.
> 

No the maximum is 20mA on our board, but limiting to 5mA is safer to avoid
blinding the user :) This RGB led is quite powerful...

Some experiments:
1) When setting the current source at 5mA, the PWM steps are easily noticeable
at low brightness (below 50%). Above the eye is not sensitive enough. Thus on
the 32768 possible colours, I agree that not all will be distinguishable.
2) When setting the current source at 20mA, the PWM steps are even more visible
at low brightness. As I have to keep the PWM ratio below 25% to satisfy the 5mA
limit, all the 7 steps (brightness = [0; 7]) are clearly noticeable. This also
means only 512 different colours. For sure in this case they are all
distinguishable :)

With your proposal, the hardware fix is probably to decrease Iref by increasing
the bias resistor. This way the PWM steps would be smaller and less noticeable.
But a hardware fix is not always possible.

>>     };
>>
>>     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.
>