linux-kernel - Re: [PATCH 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]
Date:	Wed, 22 Jun 2016 09:26:45 +0200
From:	Jacek Anaszewski <j.anaszewski@...sung.com>
To:	Florian Vaussard <florian.vaussard@...g-vd.ch>
Cc:	Florian Vaussard <florian.vaussard@...il.com>,
	devicetree@...r.kernel.org, Richard Purdie <rpurdie@...ys.net>,
	Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

Hi Florian,

On 06/22/2016 08:08 AM, Florian Vaussard wrote:
> Hi Jacek,
>
> Le 21. 06. 16 à 17:29, Jacek Anaszewski a écrit :
>> Hi Florian,
>>
>> Thanks for the patch. Please refer to my comments in the code.
>>
>
> Thanks for your review. I completely agree with you, I will address your
> comments in V2. I just have one small question below.
>
>> On 06/21/2016 09:29 AM, 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 | 265 ++++++++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 277 insertions(+)
>>>    create mode 100644 drivers/leds/leds-ncp5623.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 5ae2834..6d3e44d 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -588,6 +588,17 @@ config LEDS_BLINKM
>>>          This option enables support for the BlinkM RGB LED connected
>>>          through I2C. Say Y to enable support for the BlinkM LED.
>>>
>>> +config LEDS_NCP5623
>>> +    tristate "LED Support for NCP5623 I2C chip"
>>> +    depends on LEDS_CLASS
>>> +    depends on I2C
>>> +    help
>>> +      This option enables support for LEDs connected to NCP5623
>>> +      LED driver chips accessed via the I2C bus.
>>> +      Driver supports brightness control.
>>> +
>>> +      Say Y to enable this driver.
>>> +
>>>    config LEDS_POWERNV
>>>        tristate "LED support for PowerNV Platform"
>>>        depends on LEDS_CLASS
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index cb2013d..a2f0e10 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)        += leds-ktd2692.o
>>>    obj-$(CONFIG_LEDS_POWERNV)        += leds-powernv.o
>>>    obj-$(CONFIG_LEDS_SEAD3)        += leds-sead3.o
>>>    obj-$(CONFIG_LEDS_IS31FL32XX)        += leds-is31fl32xx.o
>>> +obj-$(CONFIG_LEDS_NCP5623)        += leds-ncp5623.o
>>>
>>>    # LED SPI Drivers
>>>    obj-$(CONFIG_LEDS_DAC124S085)        += leds-dac124s085.o
>>> diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c
>>> new file mode 100644
>>> index 0000000..a341e4a
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-ncp5623.c
>>> @@ -0,0 +1,265 @@
>>> +/*
>>> + * Copyright 2016 Florian Vaussard <florian.vaussard@...g-vd.ch>
>>> + *
>>> + * Based on leds-tlc591xx.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; version 2 of the License.
>>> + */
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#define NCP5623_MAX_LEDS    3
>>> +#define NCP5623_MAX_STEPS    32
>>> +#define NCP5623_MAX_CURRENT    31
>>> +#define NCP5623_MAX_CURRENT_UA    30000
>>> +
>>> +#define NCP5623_CMD_SHIFT    5
>>> +#define CMD_SHUTDOWN        (0x00 << NCP5623_CMD_SHIFT)
>>> +#define CMD_ILED        (0x01 << NCP5623_CMD_SHIFT)
>>> +#define CMD_PWM1        (0x02 << NCP5623_CMD_SHIFT)
>>> +#define CMD_PWM2        (0x03 << NCP5623_CMD_SHIFT)
>>> +#define CMD_PWM3        (0x04 << NCP5623_CMD_SHIFT)
>>> +#define CMD_UPWARD_DIM        (0x05 << NCP5623_CMD_SHIFT)
>>> +#define CMD_DOWNWARD_DIM    (0x06 << NCP5623_CMD_SHIFT)
>>> +#define CMD_DIM_STEP        (0x07 << NCP5623_CMD_SHIFT)
>>> +
>>> +#define NCP5623_DATA_MASK    GENMASK(NCP5623_CMD_SHIFT - 1, 0)
>>> +
>>> +#define NCP5623_CMD(cmd, data)    (cmd | (data & NCP5623_DATA_MASK))
>>> +
>
> [snip]
>
>>> +
>>> +static int ncp5623_configure(struct device *dev,
>>> +                 struct ncp5623_priv *priv)
>>> +{
>>> +    unsigned int i;
>>> +    unsigned int n;
>>> +    struct ncp5623_led *led;
>>> +    int err;
>>> +
>>> +    /* Compute the value of ILED register to honor led_max_current */
>>> +    n = 2400 * priv->led_iref / priv->led_max_current + 1;
>>> +    if (n > NCP5623_MAX_CURRENT)
>>> +        n = NCP5623_MAX_CURRENT;
>>> +    n = NCP5623_MAX_CURRENT - n;
>>> +
>>> +    dev_dbg(dev, "setting maximum current to %u uA\n",
>>> +        2400 * priv->led_iref / (NCP5623_MAX_CURRENT - n));
>>> +
>>> +    err = ncp5623_send_cmd(priv, CMD_ILED, 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->active)
>>> +            continue;
>>> +
>>> +        led->priv = priv;
>>> +        led->led_no = i;
>>> +        led->ldev.brightness_set = ncp5623_brightness_set;
>>> +        led->ldev.max_brightness = NCP5623_MAX_STEPS - 1;
>>
>> max_brightness should be asserted according to led-max-microamp
>> property.
>>
>
> We can set two parameters on this chip:
>
> - The intensity of the current source (common to all LEDs) from 0 to
> NCP5623_MAX_CURRENT (31)
> - The PWM for each LED independently from 0 to NCP5623_MAX_STEPS-1 (31)
>
> My understanding was to use the led-max-microamp property to set up the current
> source so that even with a duty cycle of 100% (PWM=31) the LED cannot be
> damaged. In this case, I think that is unnecessary to assert max_brightness, as
> we are guaranteed by design that led-max-microamp will not be surpassed for any
> value of the PWM. Would you suggest another approach?

Yes, if step 31 reflects led-max-microamp value, then max_brightness
should be 31, regardless of led-max-microamp value.

>>> +        INIT_WORK(&led->work, ncp5623_led_work);
>>
>> Please remove it and use brightness_set_blocking op.
>>
>>> +        err = led_classdev_register(dev, &led->ldev);
>>
>> Use devm prefixed version.
>>
>>> +        if (err < 0) {
>>> +            dev_err(dev, "couldn't register LED %s\n",
>>> +                led->ldev.name);
>>> +            goto exit;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +exit:
>>> +    ncp5623_destroy_devices(priv);
>>> +    return err;
>>> +}
>>> +
>
>


-- 
Best regards,
Jacek Anaszewski