| 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: Wed, 31 Jul 2019 13:46:17 -0500
From: Dan Murphy <dmurphy@...com>
To: Jacek Anaszewski <jacek.anaszewski@...il.com>, <pavel@....cz>
CC: <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 8/9] leds: lp50xx: Add the LP50XX family of the RGB LED
driver
Jacek
Thanks for looking at the patch
On 7/30/19 4:24 PM, Jacek Anaszewski wrote:
> Hi Dan,
>
> Thank you for the patch I see few things to improve.
>
> On 7/25/19 8:28 PM, Dan Murphy wrote:
>> Introduce the LP5036/30/24/18 RGB LED driver.
>> The difference in these parts are the number of
>> LED outputs where the:
>>
>> LP5036 can control 36 LEDs
>> LP5030 can control 30 LEDs
>> LP5024 can control 24 LEDs
>> LP5018 can control 18 LEDs
>>
>> The device has the ability to group LED output into control banks
>> so that multiple LED banks can be controlled with the same mixing and
>> brightness. Inversely the LEDs can also be controlled independently.
>>
>> Signed-off-by: Dan Murphy <dmurphy@...com>
>> ---
>> drivers/leds/Kconfig | 7 +
>> drivers/leds/Makefile | 1 +
>> drivers/leds/leds-lp50xx.c | 789 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 797 insertions(+)
>> create mode 100644 drivers/leds/leds-lp50xx.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 7f780d5b8490..69c037020f6b 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -350,6 +350,13 @@ config LEDS_LP3952
>> To compile this driver as a module, choose M here: the
>> module will be called leds-lp3952.
>>
>> +config LEDS_LP50XX
>> + tristate "LED Support for TI LP5036/30/24/18 LED driver chip"
>> + depends on LEDS_CLASS && REGMAP_I2C
>> + help
>> + If you say yes here you get support for the Texas Instruments
>> + LP5036, LP5030, LP5024 and LP5018 LED driver.
>> +
>> config LEDS_LP55XX_COMMON
>> tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
>> depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 897b810257dd..438a5499f3ee 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o
>> obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o
>> obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o
>> obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o
>> +obj-$(CONFIG_LEDS_LP50XX) += leds-lp50xx.o
>> obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o
>> obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o
>> obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o
>> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
>> new file mode 100644
>> index 000000000000..5a32410e4f34
> [...]
>> +static int lp50xx_probe_dt(struct lp50xx *priv)
>> +{
>> + u32 led_strings[LP5036_MAX_LED_MODULES];
>> + struct fwnode_handle *child = NULL;
>> + struct fwnode_handle *led_node = NULL;
>> + struct led_init_data init_data;
>> + struct lp50xx_led *led;
>> + int num_colors;
>> + u32 color_id;
>> + int led_number;
>> + size_t i = 0;
>> + int ret;
>> +
>> + priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev,
>> + "enable", GPIOD_OUT_LOW);
>> + if (IS_ERR(priv->enable_gpio)) {
>> + ret = PTR_ERR(priv->enable_gpio);
>> + dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + priv->regulator = devm_regulator_get(&priv->client->dev, "vled");
>> + if (IS_ERR(priv->regulator))
>> + priv->regulator = NULL;
>> +
>> + if (priv->model_id == LP5009)
>> + priv->max_leds = LP5009_MAX_LED_MODULES;
>> + else if (priv->model_id == LP5012)
>> + priv->max_leds = LP5012_MAX_LED_MODULES;
>> + else if (priv->model_id == LP5018)
>> + priv->max_leds = LP5018_MAX_LED_MODULES;
>> + else if (priv->model_id == LP5024)
>> + priv->max_leds = LP5024_MAX_LED_MODULES;
>> + else if (priv->model_id == LP5030)
>> + priv->max_leds = LP5030_MAX_LED_MODULES;
>> + else
>> + priv->max_leds = LP5036_MAX_LED_MODULES;
> You could simplify that by initializing data property of
> lp50xx_leds_match array elements and then call of_match_device.
> You can compare e.g. drivers/leds/leds-pca9532.c
Well I am not using of_ calls in driver anymore.
I would prefer to use device_get_match_data(&client->dev);
>> + device_for_each_child_node(&priv->client->dev, child) {
>> + led = &priv->leds[i];
>> + if (fwnode_property_present(child, "ti,led-bank")) {
>> + ret = fwnode_property_read_u32_array(child,
>> + "ti,led-bank",
>> + NULL, 0);
>> + ret = fwnode_property_read_u32_array(child,
>> + "ti,led-bank",
>> + led_strings,
>> + ret);
>> +
>> + priv->num_of_banked_leds = ARRAY_SIZE(led_strings);
>> +
>> + ret = lp50xx_set_banks(priv, led_strings);
>> + if (ret) {
>> + dev_err(&priv->client->dev,
>> + "Cannot setup banked LEDs\n");
>> + fwnode_handle_put(child);
>> + goto child_out;
>> + }
>> + led->ctrl_bank_enabled = 1;
>> +
>> + } else {
>> + ret = fwnode_property_read_u32(child, "reg",
>> + &led_number);
>> +
>> + led->led_number = led_number;
>> + }
>> + if (ret) {
>> + dev_err(&priv->client->dev,
>> + "led sourcing property missing\n");
>> + fwnode_handle_put(child);
>> + goto child_out;
>> + }
>> +
>> + if (led_number > priv->max_leds) {
>> + dev_err(&priv->client->dev,
>> + "led-sources property is invalid\n");
>> + ret = -EINVAL;
>> + fwnode_handle_put(child);
>> + goto child_out;
>> + }
>> +
>> + init_data.fwnode = child;
>> + init_data.devicename = priv->client->name;
> We'd rather not have devicename in the name of this LED class device
> according to the new LED naming convention. It's not a hot-pluggable
> device, so for properly designed DT we should not encounter name
> clash.
Ok I had this code from the previous led label patchset.
I will fix it
>
>> + init_data.default_label = ":";
> default_label should not be set for new drivers. See the comment in
> the include/linux/leds.h:
>
> /*
> * string to be used for devicename section of LED class device
> * either for label based LED name composition path or for fwnode
> * based when devname_mandatory is true
> */
>
> This is new driver and we should not add any convenience fallbacks
> for label based DT bindings. Label DT property has been marked
> deprecated, so for new drivers it should be deemed non-existent.
>
Same as above
>> +
>> + fwnode_property_read_string(child, "linux,default-trigger",
>> + &led->led_dev.default_trigger);
>> + num_colors = 0;
>> +
>> + fwnode_for_each_child_node(child, led_node) {
>> + ret = fwnode_property_read_u32(led_node, "color",
>> + &color_id);
>> + if (ret)
>> + dev_err(&priv->client->dev,
>> + "Cannot read color\n");
>> +
>> + led->mc_cdev.available_colors |= (1 << color_id);
>> + num_colors++;
>> +
>> + }
>> +
>> + led->priv = priv;
>> + led->mc_cdev.ops = &lp50xx_mc_ops;
>> + led->mc_cdev.num_leds = num_colors;
>> + led->mc_cdev.led_cdev = &led->led_dev;
>> + led->led_dev.brightness_set_blocking = lp50xx_brightness_set;
>> + led->led_dev.brightness_get = lp50xx_brightness_get;
>> + ret = led_classdev_multicolor_register_ext(&priv->client->dev,
>> + &led->mc_cdev,
>> + &init_data);
> Why not devm_* prefixed version?
I did create a devm_ version like you did for flash I will change it over
Dan
>
> [...]
>
Powered by blists - more mailing lists