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 <[email protected]> >>> --- >>> 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
current-comparison.pdf
Description: Adobe PDF document
