From: Florian Vaussard <florian.vaussard@heig-vd.ch>
To: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: Florian Vaussard <florian.vaussard@gmail.com>,
devicetree@vger.kernel.org, Richard Purdie <rpurdie@rpsys.net>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
Vaussard Florian <florian.vaussard@heig-vd.ch>
Subject: Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation
Date: Thu, 23 Jun 2016 10:32:49 +0200 [thread overview]
Message-ID: <576B9EB1.5040601@heig-vd.ch> (raw)
In-Reply-To: <576B8E5D.90000@samsung.com>
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.
>>
>> 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.
>> };
>>
>> 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.
>
next prev parent reply other threads:[~2016-06-23 8:32 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-21 7:29 [PATCH 0/2] leds: Add driver for NCP5623 3-channel I2C LED driver Florian Vaussard
[not found] ` <1466494154-3786-1-git-send-email-florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org>
2016-06-21 7:29 ` [PATCH 1/2] leds: ncp5623: Add device tree binding documentation Florian Vaussard
2016-06-21 15:28 ` Jacek Anaszewski
2016-06-22 6:08 ` Florian Vaussard
2016-06-22 8:51 ` Jacek Anaszewski
[not found] ` <576A5191.4070402-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2016-06-22 14:25 ` Florian Vaussard
[not found] ` <c97adf5a-7c5b-8602-83c1-f85854bd64d6-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org>
2016-06-23 7:23 ` Jacek Anaszewski
2016-06-23 8:32 ` Florian Vaussard [this message]
2016-06-23 11:21 ` Jacek Anaszewski
2016-06-23 12:01 ` Florian Vaussard
2016-06-23 13:53 ` Jacek Anaszewski
2016-06-22 8:52 ` Jacek Anaszewski
2016-06-21 21:52 ` Rob Herring
2016-06-22 6:17 ` Florian Vaussard
2016-06-21 7:29 ` [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver Florian Vaussard
2016-06-21 15:29 ` Jacek Anaszewski
2016-06-22 6:08 ` Florian Vaussard
2016-06-22 7:26 ` Jacek Anaszewski
2016-06-26 21:49 ` Pavel Machek
2016-06-27 5:46 ` Florian Vaussard
2016-06-27 7:09 ` Jacek Anaszewski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=576B9EB1.5040601@heig-vd.ch \
--to=florian.vaussard@heig-vd.ch \
--cc=devicetree@vger.kernel.org \
--cc=florian.vaussard@gmail.com \
--cc=j.anaszewski@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=rpurdie@rpsys.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).