From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Vaussard Subject: Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation Date: Wed, 22 Jun 2016 08:08:21 +0200 Message-ID: <59143a9b-ab51-bd3b-e9db-93680415d205@heig-vd.ch> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <57695D02.2000109@samsung.com> Sender: linux-kernel-owner@vger.kernel.org To: Jacek Anaszewski , Florian Vaussard Cc: devicetree@vger.kernel.org, Richard Purdie , Rob Herring , Mark Rutland , linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-leds@vger.kernel.org Hi Jacek, Le 21. 06. 16 =E0 17:28, Jacek Anaszewski a =E9crit : > Hi Florian, >=20 > Thanks for the patch. I have two remarks below. >=20 > On 06/21/2016 09:29 AM, Florian Vaussard wrote: >> Add device tree binding documentation for On Semiconductor NCP5623 I= 2C >> 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 als= o >> 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 =3D Vref/Rbias with Vref=3D0.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-ncp= 5623.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 repre= sented >> +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 >=20 > I think that you don't need this property. Just provide the formula f= or > calculating led-max-microamp value, similarly as you're doing that in > the commit message. >=20 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 s= end to chip to configure the current source. The formula is: ILED =3D 31 - 2400*Iref/led-max-microamp I need two pieces of information that depends on the hardware setup; th= e current on the Iref pin (which is setup by a bias resistor Iref =3D 0.6V/Rbias)= , and the desired output current (usually limited due to the chosen LED or the re= quired brightness). On the other hand, I could also simply ask people to put the right valu= e of 'ILED' (between 0 - 31) directly in the device tree, but this is less u= ser-friendly. >> + - led-max-microamp: Desired maximum current for each LED in micro= ampere >> + (maximum 30000uA) >=20 > Please add instead of (maximum ...): >=20 > Valid values: min - max, step by N (rounded {up|down}) >=20 > E.g. >=20 > Valid values: 10000 - 30000, step by 1000 (rounded down) >=20 This is unfortunately not a linear relationship, led-max-microamp =3D 2400*Iref/(31-ILED) thus steps are not constant. This can be seen on figure 5 (p.9) of the = datasheet [1]. Thanks for your review, =46lorian [1] http://www.onsemi.com/pub/Collateral/NCP5623-D.PDF From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751858AbcFVGJa (ORCPT ); Wed, 22 Jun 2016 02:09:30 -0400 Received: from mailcl0.heig-vd.ch ([193.134.216.181]:34978 "EHLO heig-vd.ch" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751631AbcFVGIw (ORCPT ); Wed, 22 Jun 2016 02:08:52 -0400 Subject: Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation To: Jacek Anaszewski , Florian Vaussard 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> CC: , Richard Purdie , Rob Herring , Mark Rutland , , From: Florian Vaussard Message-ID: <59143a9b-ab51-bd3b-e9db-93680415d205@heig-vd.ch> Date: Wed, 22 Jun 2016 08:08:21 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <57695D02.2000109@samsung.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.192.93.248] X-ClientProxiedBy: EINTFEA.einet.ad.eivd.ch (10.192.41.59) To EINTMBXC.einet.ad.eivd.ch (10.192.41.65) X-MailCleaner-RDNS: invalid reverse DNS for 10.192.41.59 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 I need two pieces of information that depends on the hardware setup; the current on the Iref pin (which is setup by a bias resistor Iref = 0.6V/Rbias), and the desired output current (usually limited due to the chosen LED or the required brightness). On the other hand, I could also simply ask people to put the right value of 'ILED' (between 0 - 31) directly in the device tree, but this is less user-friendly. >> + - led-max-microamp: Desired maximum current for each LED in microampere >> + (maximum 30000uA) > > Please add instead of (maximum ...): > > Valid values: min - max, step by N (rounded {up|down}) > > E.g. > > Valid values: 10000 - 30000, step by 1000 (rounded down) > This is unfortunately not a linear relationship, led-max-microamp = 2400*Iref/(31-ILED) thus steps are not constant. This can be seen on figure 5 (p.9) of the datasheet [1]. Thanks for your review, Florian [1] http://www.onsemi.com/pub/Collateral/NCP5623-D.PDF