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

[Jacek Anaszewski <j.anaszewski@samsung.com>]

On 06/23/2016 02:01 PM, Florian Vaussard wrote:
>
>
> On 06/23/2016 01:21 PM, Jacek Anaszewski wrote:
>> On 06/23/2016 10:32 AM, Florian Vaussard wrote:
>>> 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.
>>
>> OK, please keep here the property you initially introduced for that:
>>
>> onnn,led-iref-microamp
>>
>
> Ok.
>
>>>
>>>>>
>>>>>       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.
>>
>> It would be nice to have all 31 levels available per LED, but is it
>> feasible having that ILED register is global for all LEDs? It seems that
>> we couldn't set different maximum current for each LED then.
>> Am I right or I am missing something?
>>
>
> If led-max-microamp is different for each LED, we can select the highest one and
> use it to compute the ILED register. Then we limit the PWM ratio for the LEDs
> that have a lowest led-max-microamp. I guess that this is the best compromise.

Sounds good. Please proceed accordingly.

-- 
Best regards,
Jacek Anaszewski