| 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
| ||
|
Date: Mon, 27 Jun 2016 11:12:37 +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>,
Pavel Machek <pavel@....cz>, linux-leds@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] leds: Add driver for NCP5623 3-channel I2C LED
driver
On 06/27/2016 10:54 AM, Florian Vaussard wrote:
> Hi Jacek,
>
> Le 27. 06. 16 à 10:14, Jacek Anaszewski a écrit :
>> Hi Florian,
>>
>> Thanks for the update. I have few comments below.
>>
>> On 06/27/2016 09:03 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 | 269 ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 281 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..6e4c969
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-ncp5623.c
>>> @@ -0,0 +1,269 @@
>>> +/*
>>> + * 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>
>>> +
>>> +#define NCP5623_MAX_LEDS 3
>>> +#define NCP5623_MAX_STEPS 31
>>> +#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 LED_to_CMD(led) ((0x02 + led) << NCP5623_CMD_SHIFT)
>>
>> Please use only capital letters in the macro name.
>>
>>> +
>>> +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0)
>>> +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK))
>>> +
>>> +struct ncp5623_led {
>>> + bool active;
>>> + unsigned int led_no;
>>> + u32 led_max_current;
>>> + struct led_classdev ldev;
>>> + struct ncp5623_priv *priv;
>>> +};
>>> +
>>> +struct ncp5623_priv {
>>> + struct ncp5623_led leds[NCP5623_MAX_LEDS];
>>> + u32 led_iref;
>>> + u32 leds_max_current;
>>> + struct i2c_client *client;
>>> +};
>>> +
>>> +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev)
>>> +{
>>> + return container_of(ldev, struct ncp5623_led, ldev);
>>> +}
>>> +
>>> +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data)
>>> +{
>>> + char cmd_data[1] = { NCP5623_CMD(cmd, data) };
>>> + int err;
>>> +
>>> + err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data));
>>> +
>>> + return (err < 0 ? err : 0);
>>> +}
>>> +
>>> +static int ncp5623_set_pwm(struct ncp5623_led *led, u8 brightness)
>>> +{
>>> + struct ncp5623_priv *priv = led->priv;
>>> + u8 cmd;
>>
>> cmd is redundant here, you can pass the macro directly.
>>
>>> +
>>> + cmd = LED_to_CMD(led->led_no);
>>> +
>>> + return ncp5623_send_cmd(priv, cmd, brightness);
>>
>> How about just one line:
>>
>> return ncp5623_send_cmd(led->priv, LED_TO_CMD(led->led_no), brightness);
>>
>
> Ok why not. As it is used only by ncp5623_brightness_set(), does it make sense
> to keep this function after all?
Of course not. Let's remove it.
>>> +}
>>> +
>>> +static int ncp5623_brightness_set(struct led_classdev *led_cdev,
>>> + enum led_brightness brightness)
>>> +{
>>> + struct ncp5623_led *led = ldev_to_led(led_cdev);
>>> +
>>> + return ncp5623_set_pwm(led, brightness);
>>
>> return ncp5623_set_pwm(ldev_to_led(led_cdev), brightness);
>>
>
> Here I am a bit less enthusiast as I find the result less readable. But I am not
> strongly against it neither.
After removal of ncp5623_set_pwm the auxiliary variables may be
still required anyway,
>
>>> +}
>>> +
>>> +static void ncp5623_destroy_devices(struct ncp5623_priv *priv)
>>> +{
>>> + struct ncp5623_led *led;
>>> + int i;
>>> +
>>> + for (i = 0; i < NCP5623_MAX_LEDS; i++) {
>>> + led = &priv->leds[i];
>>> + if (led->active) {
>>> + led_classdev_unregister(&led->ldev);
>>> + led->active = false;
>>> + }
>>> + }
>>> +}
>>
>> This function is redundant when using devm_led_classdev_register().
>>
>
> Indeed.
>
>>> +
>>> +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;
>>> +
>>> + /* Setup the internal current source, round down */
>>> + n = 2400 * priv->led_iref / priv->leds_max_current + 1;
>>> + if (n > NCP5623_MAX_CURRENT)
>>> + n = NCP5623_MAX_CURRENT;
>>> +
>>> + effective_current = 2400 * priv->led_iref / n;
>>> + 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->active)
>>> + continue;
>>> +
>>> + led->priv = priv;
>>> + led->led_no = i;
>>> + led->ldev.brightness_set_blocking = ncp5623_brightness_set;
>>> +
>>> + led->ldev.max_brightness = led->led_max_current *
>>> + NCP5623_MAX_STEPS / effective_current;
>>> + if (led->ldev.max_brightness > NCP5623_MAX_STEPS)
>>> + led->ldev.max_brightness = NCP5623_MAX_STEPS;
>>> +
>>> + err = devm_led_classdev_register(dev, &led->ldev);
>>> + 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;
>>> +}
>>> +
>>> +static int ncp5623_parse_dt(struct ncp5623_priv *priv, struct device_node *np)
>>> +{
>>> + struct device_node *child;
>>> + struct ncp5623_led *led;
>>> + u32 reg;
>>> + int count;
>>> + int err;
>>> +
>>> + err = of_property_read_u32(np, "onnn,led-iref-microamp",
>>> + &priv->led_iref);
>>> + if (err)
>>> + return -EINVAL;
>>> +
>>> + priv->leds_max_current = 0;
>>
>> priv is allocated with kzalloc, so this is redundant.
>>
>>> +
>>> + count = of_get_child_count(np);
>>> + if (!count || count > NCP5623_MAX_LEDS)
>>> + return -EINVAL;
>>> +
>>> + for_each_child_of_node(np, child) {
>>> + err = of_property_read_u32(child, "reg", ®);
>>> + if (err)
>>> + return err;
>>
>> Empty line here please.
>>
>>> + if (reg < 0 || reg >= NCP5623_MAX_LEDS) {
>>> + err = -EINVAL;
>>> + goto dt_child_parse_error;
>>> + }
>>> +
>>> + led = &priv->leds[reg];
>>> + if (led->active) {
>>> + err = -EINVAL;
>>> + goto dt_child_parse_error;
>>> + }
>>> +
>>> + led->active = true;
>>
>> Is the 'active' property really needed? You could verify if the LED
>> has been already initialized e.g. by checking led->ldev.name for
>> being NULL.
>>
>
> This is also an option, but I find
>
> if (led->ldev.name != NULL) {
> /* error */
> ...
> }
>
> less clear about the purpose of the test.
I agree. It's up to you. You can also compare how other LED class
drivers handle similar issues. Maybe changing the driver design a little
bit would allow to get rid of the 'active' property.
>>> +
>>> + err = of_property_read_u32(child, "led-max-microamp",
>>> + &led->led_max_current);
>>> + if (err || led->led_max_current > NCP5623_MAX_CURRENT_UA)
>>> + return -EINVAL;
>>> + if (led->led_max_current > priv->leds_max_current)
>>> + priv->leds_max_current = led->led_max_current;
>>> +
>>> + led->ldev.name =
>>> + of_get_property(child, "label", NULL) ? : child->name;
>>> + led->ldev.default_trigger =
>>> + of_get_property(child, "linux,default-trigger", NULL);
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +dt_child_parse_error:
>>> + of_node_put(child);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static const struct of_device_id ncp5623_of_match[] = {
>>> + { .compatible = "onnn,ncp5623" },
>>> + { /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ncp5623_of_match);
>>> +
>>> +static int ncp5623_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct device *dev = &client->dev;
>>> + struct device_node *np = dev->of_node;
>>> + struct ncp5623_priv *priv;
>>> + int err;
>>> +
>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>> +
>>> + priv->client = client;
>>> + i2c_set_clientdata(client, priv);
>>> +
>>> + err = ncp5623_parse_dt(priv, np);
>>> + if (err)
>>> + return err;
>>> +
>>> + return ncp5623_configure(dev, priv);
>>> +}
>>> +
>>> +static int ncp5623_remove(struct i2c_client *client)
>>> +{
>>> + struct ncp5623_priv *priv = i2c_get_clientdata(client);
>>> +
>>> + ncp5623_destroy_devices(priv);
>>> +
>>> + return 0;
>>> +}
>>
>> remove callback is not needed with devm prefixed registration.
>>
>
> Thank you for the review!
>
> Best,
> Florian
>
>>> +
>>> +static const struct i2c_device_id ncp5623_id[] = {
>>> + { "ncp5623" },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ncp5623_id);
>>> +
>>> +static struct i2c_driver ncp5623_driver = {
>>> + .driver = {
>>> + .name = "ncp5623",
>>> + .of_match_table = of_match_ptr(ncp5623_of_match),
>>> + },
>>> + .probe = ncp5623_probe,
>>> + .remove = ncp5623_remove,
>>> + .id_table = ncp5623_id,
>>> +};
>>> +
>>> +module_i2c_driver(ncp5623_driver);
>>> +
>>> +MODULE_AUTHOR("Florian Vaussard <florian.vaussard@...g-vd.ch>");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_DESCRIPTION("NCP5623 LED driver");
>>>
>>
>>
>
>
--
Best regards,
Jacek Anaszewski
Powered by blists - more mailing lists