Add the binding description and the corresponding driver for
the BD2606.
Andreas Kemnade (2):
dt-bindings: leds: ROHM BD2606MVV LED driver
leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver
.../bindings/leds/rohm,bd2606mvv.yaml | 76 +++++++++
drivers/leds/Kconfig | 11 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-bd2606mvv.c | 145 ++++++++++++++++++
4 files changed, 233 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
create mode 100644 drivers/leds/leds-bd2606mvv.c
--
2.39.2
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 <[email protected]>
---
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
+ */
+
+#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 <[email protected]>");
+MODULE_DESCRIPTION("BD2606 LED driver");
+MODULE_LICENSE("GPL");
--
2.39.2
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 <[email protected]>
> ---
> 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 <[email protected]>");
> +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! ~~
Hi Matti,
thanks for the review:
On Thu, 6 Apr 2023 11:57:15 +0300
Matti Vaittinen <[email protected]> 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 <[email protected]> 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
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Thu, 6 Apr 2023 11:57:15 +0300
Matti Vaittinen <[email protected]> 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 <[email protected]> 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
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~