From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751594AbcFWHXR (ORCPT ); Thu, 23 Jun 2016 03:23:17 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:32716 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751287AbcFWHXO (ORCPT ); Thu, 23 Jun 2016 03:23:14 -0400 MIME-version: 1.0 Content-type: text/plain; charset=windows-1252; format=flowed X-AuditID: cbfec7f5-f792a6d000001302-a5-576b8e5f461e Content-transfer-encoding: 8BIT Message-id: <576B8E5D.90000@samsung.com> Date: Thu, 23 Jun 2016 09:23:09 +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> In-reply-to: X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrELMWRmVeSWpSXmKPExsVy+t/xa7rxfdnhBrcWcVjMP3KO1WLyocXs Fq+2W1tc3jWHzWLrm3WMFkuvX2SyaN17hN1i966nrA4cHmvmrWH02DnrLrvH03X9LB6bVnWy eeyZ/4PV4/MmuQC2KC6blNSczLLUIn27BK6Mpv0/WAs26Fb8WtzH3sB4WqWLkZNDQsBE4m3L IXYIW0ziwr31bF2MXBxCAksZJdYufc8EkuAVEJT4MfkeSxcjBwezgLzEkUvZIGFmAVuJBe/X sUDUP2OU+LrzCxtEvYZEx/VFzCA2i4CqxIvdC8DmsAkYSvx88RrMFhWIkPhzeh8riC0iYCQx ceJiZoihPxgllp7MArGFBXwlPr5fAhYXEtjKJNGw0hDkBk4BO4lvnfoTGAVmIbluFsJ1s5Bc t4CReRWjaGppckFxUnqukV5xYm5xaV66XnJ+7iZGSMh/3cG49JjVIUYBDkYlHt6MY1nhQqyJ ZcWVuYcYJTiYlUR4f3dnhwvxpiRWVqUW5ccXleakFh9ilOZgURLnnbnrfYiQQHpiSWp2ampB ahFMlomDU6qBUTgs3P7oh0XKMwrz9xYZ1P2M53i+PzJnd931NlH2xT+2LHzIOGlzx8kNm2Z3 yVywMpiyd88PLeHjqg+Cz+ZbLkov5xA/fNVxfbHa97Vdvlwf+LTk8777T49Yn7M243OUbvxZ mUtta3V+T+z+EHNiSopa/wT1GQeunGFc9a+27v29q52yp+Yt4ldiKc5INNRiLipOBAB48ZIK dQIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > 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