Messages in this thread | | | Date | Thu, 6 Apr 2023 11:57:15 +0300 | From | Matti Vaittinen <> | Subject | Re: [PATCH 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver |
| |
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
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |