linux-kernel - Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d8f9d69d-df6c-f24e-dd78-30501c9fc1b6@gmail.com>
Date:   Thu, 29 Sep 2016 19:13:57 +0200
From:   Florian Vaussard <florian.vaussard@...il.com>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Jacek Anaszewski <j.anaszewski@...sung.com>
Cc:     devicetree@...r.kernel.org, Richard Purdie <rpurdie@...ys.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Pavel Machek <pavel@....cz>, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Florian Vaussard <florian.vaussard@...g-vd.ch>
Subject: Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED
 driver

Replying to my self after thinking twice...

Le 29. 09. 16 à 18:18, Florian Vaussard a écrit :
> Hi Jacek,
> 
> Thank you for your comments!
> 
> Le 18. 09. 16 à 20:20, Jacek Anaszewski a écrit :
>> Hi Florian,
>>
>> Thanks for the updated patch set. I have few comments below.
>>
>> On 09/16/2016 01:34 PM, Florian Vaussard wrote:
>>> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled
>>> through I2C. The PWM of each channel can be independently set with 32
>>> distinct levels. In addition, the intensity of the current source can be
>>> globally set using an external bias resistor fixing the reference
>>> current (Iref) and a dedicated register (ILED), following the
>>> relationship:
>>>
>>> I = 2400*Iref/(31-ILED)
>>>
>>> with Iref = Vref/Rbias, and Vref = 0.6V.
>>>
>>> Signed-off-by: Florian Vaussard <florian.vaussard@...g-vd.ch>
>>> ---
>>>  drivers/leds/Kconfig        |  11 +++
>>>  drivers/leds/Makefile       |   1 +
>>>  drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 246 insertions(+)
>>>  create mode 100644 drivers/leds/leds-ncp5623.c
>>>

[...]

>>> +static int ncp5623_configure(struct device *dev,
>>> +                 struct ncp5623_priv *priv)
>>> +{
>>> +    unsigned int i;
>>> +    unsigned int n;
>>> +    struct ncp5623_led *led;
>>> +    int effective_current;
>>> +    int err;
>>
>> Below way of calculating max_brightness is not clear to me.
>> Let's analyze it below, using values from your DT example.
>>
>>> +
>>> +    /* Setup the internal current source, round down */
>>> +    n = 2400 * priv->led_iref / priv->leds_max_current + 1;
>>
>> n = 2400 * 10 / 20000 + 1 = 2
>>
>>> +    if (n > NCP5623_MAX_CURRENT)
>>> +        n = NCP5623_MAX_CURRENT;
>>> +
>>> +    effective_current = 2400 * priv->led_iref / n;
>>
>> effective_current = 2400 * 10 / 2 = 12000
>>
>>> +    dev_dbg(dev, "setting maximum current to %u uA\n", effective_current);
>>> +
>>> +    err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n);
>>> +    if (err < 0) {
>>> +        dev_err(dev, "cannot set the current\n");
>>> +        return err;
>>> +    }
>>> +
>>> +    /* Setup each individual LED */
>>> +    for (i = 0; i < NCP5623_MAX_LEDS; i++) {
>>> +        led = &priv->leds[i];
>>> +
>>> +        if (led->led_no < 0)
>>> +            continue;
>>> +
>>> +        led->priv = priv;
>>> +        led->ldev.brightness_set_blocking = ncp5623_brightness_set;
>>> +
>>> +        led->ldev.max_brightness = led->led_max_current *
>>> +            NCP5623_MAX_STEPS / effective_current;
>>
>> led->ldev.max_brightness = 20000 * 31 / 12000 = 51
>>
>> This is not intuitive, and I'm not even sure if the result is in line
>> with what you intended.
>>
> 
> There is indeed a problem in the case the allowed current on the LED is greater
> than the effective current provided by the current source, as in your example.
> Here I should put something like:
> 
> 	led->ldev.max_brightness =
> 		min(NCP5623_MAX_STEPS, x * NCP5623_MAX_STEPS / y);
> 
>> Instead I propose the following:
>>
>> n_iled_max =
>>      31 - (priv->led_iref * 2400 / priv->leds_max_current +
>>            !!(priv->led_iref * 2400 % priv->leds_max_current))
>>
>> (n_iled_max =
>>      31 - (24000 / 20000 + !!(24000 % 20000)) = 31 - (1 + 1) = 29)
>>
>> ncp5623_send_cmd(priv, CMD_ILED, n_iled_max)
>>
> 
> This is a good proposition, especially with the DIV_ROUND_UP proposed by Pavel.
> I simulated both and I noticed a problem in both cases for very low currents, as
> we would have negative values for the register setting (see attached figure). I
> will fix this in the next version.
> 

In fact my original solution does not have this problem because of the (n >
NCP5623_MAX_CURRENT) check and clipping before computing the effective current.
This was not included in my simulation, here is the updated graph. So I will
enhance your solution to avoid this exact problem.

Best,
Florian

Download attachment "current-comparison.pdf" of type "application/pdf" (4320 bytes)