[2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver
Commit Message
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
Comments
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
@@ -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
@@ -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
new file mode 100644
@@ -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");