From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751001AbcFWNxw (ORCPT ); Thu, 23 Jun 2016 09:53:52 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:33253 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750799AbcFWNxu (ORCPT ); Thu, 23 Jun 2016 09:53:50 -0400 MIME-version: 1.0 Content-type: text/plain; charset=windows-1252; format=flowed X-AuditID: cbfec7f5-f792a6d000001302-4d-576be9ead723 Content-transfer-encoding: 8BIT Message-id: <576BE9E9.7090807@samsung.com> Date: Thu, 23 Jun 2016 15:53:45 +0200 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 To: Florian Vaussard Cc: Florian Vaussard , devicetree@vger.kernel.org, Richard Purdie , Rob Herring , Mark Rutland , linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation References: <1466494154-3786-1-git-send-email-florian.vaussard@heig-vd.ch> <1466494154-3786-2-git-send-email-florian.vaussard@heig-vd.ch> <57695D02.2000109@samsung.com> <59143a9b-ab51-bd3b-e9db-93680415d205@heig-vd.ch> <576A5191.4070402@samsung.com> <576B8E5D.90000@samsung.com> <576B9EB1.5040601@heig-vd.ch> <576BC63D.1030504@samsung.com> <576BCF95.4010508@heig-vd.ch> In-reply-to: <576BCF95.4010508@heig-vd.ch> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrILMWRmVeSWpSXmKPExsVy+t/xq7qvXmaHG5w6qGUx/8g5VovJhxaz W7zabm1xedccNoutb9YxWiy9fpHJonXvEXaL3buesjpweKyZt4bRY+esu+weT9f1s3hsWtXJ 5rFn/g9Wj8+b5ALYorhsUlJzMstSi/TtErgyzn9OKnhnUbH7WXkD41/dLkZODgkBE4k7Gw+z QthiEhfurWfrYuTiEBJYyijx8fsBFpAEr4CgxI/J94BsDg5mAXmJI5eyQcLMArYSC96vY4Go f8Yo8e7jXWaQGl4BLYnedrCZLAKqElcXXmAGsdkEDCV+vnjNBGKLCkRI/Dm9D6xGRMBIYuLE xcwQM38wSiw9mQViCwv4Snx8v4QZYv5GZonTJw+C3cMJNH9C21PGCYwCs5CcNwvhvFlIzlvA yLyKUTS1NLmgOCk910ivODG3uDQvXS85P3cTIyTgv+5gXHrM6hCjAAejEg9vxrGscCHWxLLi ytxDjBIczEoivGpPs8OFeFMSK6tSi/Lji0pzUosPMUpzsCiJ887c9T5ESCA9sSQ1OzW1ILUI JsvEwSnVwJhgvvHpm+U2yj28Tw3Yw959vzzpRaWOZYLluapbkYKCd67rprz5Vrr3AltHphXD nndB9ZrRu5e9qF7kelehOWhnYkx9fn/HFHeJXF5vj8c/syO5mLeqnphrbPP3+c2r5eE2a3NL 7lyeVBytHXHNPdSaM+mHxDG3BUZqz3cfvdPp9viLd5uRqRJLcUaioRZzUXEiAI1n/v90AgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>>>>>> --- >>>>>>>>> .../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