| Message ID | 20230406060825.103187-3-andreas@kemnade.info |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: Add a driver for the BD2606MVV | expand |
Hi Andreas, Overall a nice and clean driver with a good explanation of shared brightness. Just some minor things. On 4/6/23 09:08, Andreas Kemnade wrote: > The device provides 6 channels which can be individually > turned off and on but groups of two channels share a common brightness > register. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/leds/Kconfig | 11 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-bd2606mvv.c | 145 ++++++++++++++++++++++++++++++++++ > 3 files changed, 157 insertions(+) > create mode 100644 drivers/leds/leds-bd2606mvv.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 9dbce09eabacf..cc4eadbb2542e 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -551,6 +551,17 @@ config LEDS_REGULATOR > help > This option enables support for regulator driven LEDs. > > +config LEDS_BD2606MVV > + tristate "LED driver for BD2606MVV" > + depends on LEDS_CLASS > + depends on I2C > + select REGMAP_I2C > + help > + This option enables support for BD2606MVV LED driver chips > + accessed via the I2C bus. It supports setting brightness, with > + the limitiation that there are groups of two channels sharing > + a brightness setting, but not the on/off setting. > + > config LEDS_BD2802 > tristate "LED driver for BD2802 RGB LED" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index d30395d11fd84..c07d1512c745a 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o > obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o > obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o > +obj-$(CONFIG_LEDS_BD2606MVV) += leds-bd2606mvv.o > obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o > obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o > obj-$(CONFIG_LEDS_CLEVO_MAIL) += leds-clevo-mail.o > diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c > new file mode 100644 > index 0000000000000..47ddd016bae3f > --- /dev/null > +++ b/drivers/leds/leds-bd2606mvv.c > @@ -0,0 +1,145 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2023 Andreas Kemnade Could add a link to data-sheet here. > + */ > + > +#include <linux/i2c.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +#define BD2606_MAX_LEDS 6 > +#define BD2606_MAX_BRIGHTNESS 63 > +#define BD2606_REG_PWRCNT 3 > +#define ldev_to_led(c) container_of(c, struct bd2606mvv_led, ldev) > + > +struct bd2606mvv_led { > + bool active; I didn't spot where this 'active' was used? > + unsigned int led_no; > + struct led_classdev ldev; > + struct bd2606mvv_priv *priv; > +}; > + > +struct bd2606mvv_priv { > + struct bd2606mvv_led leds[BD2606_MAX_LEDS]; > + struct regmap *regmap; > +}; > + > +static int > +bd2606mvv_brightness_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + struct bd2606mvv_led *led = ldev_to_led(led_cdev); > + struct bd2606mvv_priv *priv = led->priv; > + int err; > + > + if (brightness == 0) { > + return regmap_update_bits(priv->regmap, > + BD2606_REG_PWRCNT, > + 1 << led->led_no, > + 0); > + } could drop the brackets. > + > + /* shared brightness register */ > + err = regmap_write(priv->regmap, led->led_no / 2, > + brightness); > + if (err) > + return err; > + > + return regmap_update_bits(priv->regmap, > + BD2606_REG_PWRCNT, > + 1 << led->led_no, > + 1 << led->led_no); > +} > + > +static const struct regmap_config bd2606mvv_regmap = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0x3, > +}; > + > +static int bd2606mvv_probe(struct i2c_client *client) > +{ > + struct device_node *np, *child; Is it possible to only use the fwnode? I think the more generic fwnode_* is preferred over of_*. > + struct device *dev = &client->dev; > + struct bd2606mvv_priv *priv; > + int err, count, reg; > + > + np = dev_of_node(dev); > + if (!np) > + return -ENODEV; > + > + count = of_get_available_child_count(np); > + if (!count || count > BD2606_MAX_LEDS) > + return -EINVAL; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->regmap = devm_regmap_init_i2c(client, &bd2606mvv_regmap); > + if (IS_ERR(priv->regmap)) { > + err = PTR_ERR(priv->regmap); > + dev_err(dev, "Failed to allocate register map: %d\n", err); > + return err; > + } > + > + i2c_set_clientdata(client, priv); > + The IC seems to have an enable pin. I think you might add the enable-gpio in dt-bindings and try to (optionally) get and enable it here. > + for_each_available_child_of_node(np, child) { > + struct bd2606mvv_led *led; > + struct led_init_data init_data = {}; > + > + init_data.fwnode = of_fwnode_handle(child); > + > + err = of_property_read_u32(child, "reg", ®); > + if (err) { > + of_node_put(child); > + return err; > + } > + if (reg < 0 || reg >= BD2606_MAX_LEDS || > + priv->leds[reg].active) { > + of_node_put(child); > + return -EINVAL; > + } > + led = &priv->leds[reg]; > + > + led->active = true; > + led->priv = priv; > + led->led_no = reg; > + led->ldev.brightness_set_blocking = bd2606mvv_brightness_set; > + led->ldev.max_brightness = BD2606_MAX_BRIGHTNESS; > + err = devm_led_classdev_register_ext(dev, &led->ldev, > + &init_data); > + if (err < 0) { > + of_node_put(child); > + return dev_err_probe(dev, err, > + "couldn't register LED %s\n", > + led->ldev.name); > + } > + } > + return 0; > +} > + > +static const struct of_device_id __maybe_unused of_bd2606mvv_leds_match[] = { > + { .compatible = "rohm,bd2606mvv", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_bd2606mvv_leds_match); > + > +static struct i2c_driver bd2606mvv_driver = { > + .driver = { > + .name = "leds-bd2606mvv", > + .of_match_table = of_match_ptr(of_bd2606mvv_leds_match), > + }, > + .probe_new = bd2606mvv_probe, > +}; > + > +module_i2c_driver(bd2606mvv_driver); > + > +MODULE_AUTHOR("Andreas Kemnade <andreas@kemnade.info>"); > +MODULE_DESCRIPTION("BD2606 LED driver"); > +MODULE_LICENSE("GPL"); Yours, -- Matti
Hi Matti, thanks for the review: On Thu, 6 Apr 2023 11:57:15 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > + priv->regmap = devm_regmap_init_i2c(client, &bd2606mvv_regmap); > > + if (IS_ERR(priv->regmap)) { > > + err = PTR_ERR(priv->regmap); > > + dev_err(dev, "Failed to allocate register map: %d\n", err); > > + return err; > > + } > > + > > + i2c_set_clientdata(client, priv); > > + > > The IC seems to have an enable pin. I think you might add the > enable-gpio in dt-bindings and try to (optionally) get and enable it here. It has an enable pin. I would prefer to just have the binding as complete as possible and have it added later in the driver by someone needing it since I cannot test that. Regards, Andreas
On 4/6/23 14:43, Andreas Kemnade wrote: > Hi Matti, > > thanks for the review: > > On Thu, 6 Apr 2023 11:57:15 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >>> + priv->regmap = devm_regmap_init_i2c(client, &bd2606mvv_regmap); >>> + if (IS_ERR(priv->regmap)) { >>> + err = PTR_ERR(priv->regmap); >>> + dev_err(dev, "Failed to allocate register map: %d\n", err); >>> + return err; >>> + } >>> + >>> + i2c_set_clientdata(client, priv); >>> + >> >> The IC seems to have an enable pin. I think you might add the >> enable-gpio in dt-bindings and try to (optionally) get and enable it here. > > It has an enable pin. I would prefer to just have the binding as complete as > possible and have it added later in the driver by someone needing it > since I cannot test that. Fair enough. Thanks for working with this! Yours, -- Matti
On Thu, 6 Apr 2023 11:57:15 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: [...] > > > + */ > > + > > +#include <linux/i2c.h> > > +#include <linux/leds.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > + > > +#define BD2606_MAX_LEDS 6 > > +#define BD2606_MAX_BRIGHTNESS 63 > > +#define BD2606_REG_PWRCNT 3 > > +#define ldev_to_led(c) container_of(c, struct bd2606mvv_led, ldev) > > + > > +struct bd2606mvv_led { > > + bool active; > > I didn't spot where this 'active' was used? > [..] > > + if (reg < 0 || reg >= BD2606_MAX_LEDS || > > + priv->leds[reg].active) { here > > + of_node_put(child); > > + return -EINVAL; > > + } > > + led = &priv->leds[reg]; > > + > > + led->active = true; and here Regards, Andreas
On 4/6/23 22:45, Andreas Kemnade wrote: > On Thu, 6 Apr 2023 11:57:15 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > [...] > >> >>> + */ >>> + >>> +#include <linux/i2c.h> >>> +#include <linux/leds.h> >>> +#include <linux/module.h> >>> +#include <linux/of.h> >>> +#include <linux/of_device.h> >>> +#include <linux/regmap.h> >>> +#include <linux/slab.h> >>> + >>> +#define BD2606_MAX_LEDS 6 >>> +#define BD2606_MAX_BRIGHTNESS 63 >>> +#define BD2606_REG_PWRCNT 3 >>> +#define ldev_to_led(c) container_of(c, struct bd2606mvv_led, ldev) >>> + >>> +struct bd2606mvv_led { >>> + bool active; >> >> I didn't spot where this 'active' was used? >> > [..] > >>> + if (reg < 0 || reg >= BD2606_MAX_LEDS || >>> + priv->leds[reg].active) { > > here > >>> + of_node_put(child); >>> + return -EINVAL; >>> + } >>> + led = &priv->leds[reg]; >>> + >>> + led->active = true; > > and here Oh, right. So, if I read this correctly, "active" is only used in the probe for checking if same 'reg' is given for mone than one LEDs. If the 'active' is not used after probe then I'd prefer limiting the life-time to probe. Perhaps drop this from the allocated private data and just take it from the stack and let it go when probe is done? This is a minor thing but if there will be other reason(s) to re-spin, then this might be changed? Yours, -- Matti
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 9dbce09eabacf..cc4eadbb2542e 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -551,6 +551,17 @@ config LEDS_REGULATOR help This option enables support for regulator driven LEDs. +config LEDS_BD2606MVV + tristate "LED driver for BD2606MVV" + depends on LEDS_CLASS + depends on I2C + select REGMAP_I2C + help + This option enables support for BD2606MVV LED driver chips + accessed via the I2C bus. It supports setting brightness, with + the limitiation that there are groups of two channels sharing + a brightness setting, but not the on/off setting. + config LEDS_BD2802 tristate "LED driver for BD2802 RGB LED" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index d30395d11fd84..c07d1512c745a 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o +obj-$(CONFIG_LEDS_BD2606MVV) += leds-bd2606mvv.o obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o obj-$(CONFIG_LEDS_CLEVO_MAIL) += leds-clevo-mail.o diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c new file mode 100644 index 0000000000000..47ddd016bae3f --- /dev/null +++ b/drivers/leds/leds-bd2606mvv.c @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2023 Andreas Kemnade + */ + +#include <linux/i2c.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#define BD2606_MAX_LEDS 6 +#define BD2606_MAX_BRIGHTNESS 63 +#define BD2606_REG_PWRCNT 3 +#define ldev_to_led(c) container_of(c, struct bd2606mvv_led, ldev) + +struct bd2606mvv_led { + bool active; + unsigned int led_no; + struct led_classdev ldev; + struct bd2606mvv_priv *priv; +}; + +struct bd2606mvv_priv { + struct bd2606mvv_led leds[BD2606_MAX_LEDS]; + struct regmap *regmap; +}; + +static int +bd2606mvv_brightness_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct bd2606mvv_led *led = ldev_to_led(led_cdev); + struct bd2606mvv_priv *priv = led->priv; + int err; + + if (brightness == 0) { + return regmap_update_bits(priv->regmap, + BD2606_REG_PWRCNT, + 1 << led->led_no, + 0); + } + + /* shared brightness register */ + err = regmap_write(priv->regmap, led->led_no / 2, + brightness); + if (err) + return err; + + return regmap_update_bits(priv->regmap, + BD2606_REG_PWRCNT, + 1 << led->led_no, + 1 << led->led_no); +} + +static const struct regmap_config bd2606mvv_regmap = { + .reg_bits = 8, + .val_bits = 8, + .max_register = 0x3, +}; + +static int bd2606mvv_probe(struct i2c_client *client) +{ + struct device_node *np, *child; + struct device *dev = &client->dev; + struct bd2606mvv_priv *priv; + int err, count, reg; + + np = dev_of_node(dev); + if (!np) + return -ENODEV; + + count = of_get_available_child_count(np); + if (!count || count > BD2606_MAX_LEDS) + return -EINVAL; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->regmap = devm_regmap_init_i2c(client, &bd2606mvv_regmap); + if (IS_ERR(priv->regmap)) { + err = PTR_ERR(priv->regmap); + dev_err(dev, "Failed to allocate register map: %d\n", err); + return err; + } + + i2c_set_clientdata(client, priv); + + for_each_available_child_of_node(np, child) { + struct bd2606mvv_led *led; + struct led_init_data init_data = {}; + + init_data.fwnode = of_fwnode_handle(child); + + err = of_property_read_u32(child, "reg", ®); + if (err) { + of_node_put(child); + return err; + } + if (reg < 0 || reg >= BD2606_MAX_LEDS || + priv->leds[reg].active) { + of_node_put(child); + return -EINVAL; + } + led = &priv->leds[reg]; + + led->active = true; + led->priv = priv; + led->led_no = reg; + led->ldev.brightness_set_blocking = bd2606mvv_brightness_set; + led->ldev.max_brightness = BD2606_MAX_BRIGHTNESS; + err = devm_led_classdev_register_ext(dev, &led->ldev, + &init_data); + if (err < 0) { + of_node_put(child); + return dev_err_probe(dev, err, + "couldn't register LED %s\n", + led->ldev.name); + } + } + return 0; +} + +static const struct of_device_id __maybe_unused of_bd2606mvv_leds_match[] = { + { .compatible = "rohm,bd2606mvv", }, + {}, +}; +MODULE_DEVICE_TABLE(of, of_bd2606mvv_leds_match); + +static struct i2c_driver bd2606mvv_driver = { + .driver = { + .name = "leds-bd2606mvv", + .of_match_table = of_match_ptr(of_bd2606mvv_leds_match), + }, + .probe_new = bd2606mvv_probe, +}; + +module_i2c_driver(bd2606mvv_driver); + +MODULE_AUTHOR("Andreas Kemnade <andreas@kemnade.info>"); +MODULE_DESCRIPTION("BD2606 LED driver"); +MODULE_LICENSE("GPL");
The device provides 6 channels which can be individually turned off and on but groups of two channels share a common brightness register. Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- drivers/leds/Kconfig | 11 +++ drivers/leds/Makefile | 1 + drivers/leds/leds-bd2606mvv.c | 145 ++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 drivers/leds/leds-bd2606mvv.c