From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752060AbcFVIvh (ORCPT ); Wed, 22 Jun 2016 04:51:37 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:18597 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751604AbcFVIve (ORCPT ); Wed, 22 Jun 2016 04:51:34 -0400 MIME-version: 1.0 Content-type: text/plain; charset=windows-1252; format=flowed X-AuditID: cbfec7f5-f792a6d000001302-34-576a5192e70d Content-transfer-encoding: 8BIT Message-id: <576A5191.4070402@samsung.com> Date: Wed, 22 Jun 2016 10:51:29 +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> In-reply-to: <59143a9b-ab51-bd3b-e9db-93680415d205@heig-vd.ch> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrELMWRmVeSWpSXmKPExsVy+t/xa7qTArPCDS6uELeYf+Qcq8XkQ4vZ LV5tt7a4vGsOm8XWN+sYLZZev8hk0br3CLvF7l1PWR04PNbMW8PosXPWXXaPp+v6WTw2repk 89gz/werx+dNcgFsUVw2Kak5mWWpRfp2CVwZPRe+sBbcl69omcPfwLhfsouRk0NCwETiw/e5 jBC2mMSFe+vZuhi5OIQEljJKfJ7yhQ0kwSsgKPFj8j2WLkYODmYBeYkjl7JBwswCthIL3q9j gah/xihx6t4KqHotiY8nPrGD2CwCqhL7XhxmAbHZBAwlfr54zQRiiwpESPw5vY8VxBYRMJKY OHExM8TQH4wSS09mgdjCAr4SH98vAYsLCTxglOjeKAticwrYSbzpvsM8gVFgFpLzZiGcNwvJ eQsYmVcxiqaWJhcUJ6XnGukVJ+YWl+al6yXn525ihIT81x2MS49ZHWIU4GBU4uFl6MoMF2JN LCuuzD3EKMHBrCTCa+iTFS7Em5JYWZValB9fVJqTWnyIUZqDRUmcd+au9yFCAumJJanZqakF qUUwWSYOTqkGRoGzLo0N/1pl10fNESva1S6ytqjbfnNqzmv+su4D/SfYDgsXfxDVKwvakVLE puXzutfgZvR64a2TOatXr+9OcOw4qFaQET7RbMXbsymdKonytvu8tj0XmfetmenPXfbUxIrl 8V8cH6/MezPtUtVs3+fbHQXuxC1j9bCelDN7Xm4z8+r7Uj5JSizFGYmGWsxFxYkA0NnCaHUC AAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. You can compare drivers/leds/leds-aat1290.c and its bindings, as it uses similar approach. > > 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, OK, please define only max and min value then. > 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 > > -- Best regards, Jacek Anaszewski