[PATCH 0/2] leds: Add a driver for the BD2606MVV

[PATCH 0/2] leds: Add a driver for the BD2606MVV

[Andreas Kemnade]

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

[PATCH 1/2] dt-bindings: leds: ROHM BD2606MVV LED driver

[Andreas Kemnade]

Document ROHM BD2606MVV LED driver devicetree bindings.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 .../bindings/leds/rohm,bd2606mvv.yaml         | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml

diff --git a/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml b/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
new file mode 100644
index 0000000000000..6d4ddd8d31162
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/rohm,bd2606mvv.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: BD2606MVV LED controller
+
+maintainers:
+  - Andreas Kemnade <andreas@kemnade.info>
+
+description:
+  The BD2606 MVV is a programmable LED controller connected via I2C that can
+  drive 6 separate lines. Each of them can be individually switched on and off
+  but the brightness setting is shared between two of them.
+
+properties:
+  compatible:
+    const: rohm,bd2606mvv
+
+  reg:
+    description: I2C slave address of the controller.
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-6]$":
+    type: object
+    $ref: common.yaml#
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 6
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/leds/common.h>
+
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@66 {
+            compatible = "rohm,bd2606mvv";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <0x66>;
+
+            led@0 {
+                color = <LED_COLOR_ID_RED>;
+                function = LED_FUNCTION_POWER;
+                reg = <0x0>;
+            };
+
+            led@2 {
+                color = <LED_COLOR_ID_WHITE>;
+                function = LED_FUNCTION_STATUS;
+                reg = <0x2>;
+            };
+        };
+    };
+
+...
-- 
2.39.2

[PATCH 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver

[Andreas Kemnade]

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
+ */
+
+#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", &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");
-- 
2.39.2

Re: [PATCH 1/2] dt-bindings: leds: ROHM BD2606MVV LED driver

[Matti Vaittinen]

Hi Andreas,

Thanks for the patch! Adding new support for devices is Much Appreciated!

On 4/6/23 09:08, Andreas Kemnade wrote:
> Document ROHM BD2606MVV LED driver devicetree bindings.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>   .../bindings/leds/rohm,bd2606mvv.yaml         | 76 +++++++++++++++++++
>   1 file changed, 76 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml b/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
> new file mode 100644
> index 0000000000000..6d4ddd8d31162
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/rohm,bd2606mvv.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: BD2606MVV LED controller
> +
> +maintainers:
> +  - Andreas Kemnade <andreas@kemnade.info>
> +
> +description:
> +  The BD2606 MVV is a programmable LED controller connected via I2C that can
> +  drive 6 separate lines. Each of them can be individually switched on and off
> +  but the brightness setting is shared between two of them.

Maybe add a link to data-sheet?
https://fscdn.rohm.com/en/products/databook/datasheet/ic/power/led_driver/bd2606mvv_1-e.pdf

> +
> +properties:
> +  compatible:
> +    const: rohm,bd2606mvv
> +
> +  reg:
> +    description: I2C slave address of the controller.
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^led@[0-6]$":
> +    type: object
> +    $ref: common.yaml#
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 6
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false

According to the data-sheet, BD2606 has an enable-pin. Should it be 
visible in the bindings?


Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver

[Matti Vaittinen]

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", &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! ~~

Re: [PATCH 1/2] dt-bindings: leds: ROHM BD2606MVV LED driver

[Andreas Kemnade]

Hi Matti,

On Thu, 6 Apr 2023 11:32:42 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Andreas,
> 
> Thanks for the patch! Adding new support for devices is Much Appreciated!
> 
> On 4/6/23 09:08, Andreas Kemnade wrote:
> > Document ROHM BD2606MVV LED driver devicetree bindings.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >   .../bindings/leds/rohm,bd2606mvv.yaml         | 76 +++++++++++++++++++
> >   1 file changed, 76 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml b/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
> > new file mode 100644
> > index 0000000000000..6d4ddd8d31162
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/rohm,bd2606mvv.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: BD2606MVV LED controller
> > +
> > +maintainers:
> > +  - Andreas Kemnade <andreas@kemnade.info>
> > +
> > +description:
> > +  The BD2606 MVV is a programmable LED controller connected via I2C that can
> > +  drive 6 separate lines. Each of them can be individually switched on and off
> > +  but the brightness setting is shared between two of them.  
> 
> Maybe add a link to data-sheet?
> https://fscdn.rohm.com/en/products/databook/datasheet/ic/power/led_driver/bd2606mvv_1-e.pdf
> 
Maybe also (because it has the register description):
https://fscdn.rohm.com/en/products/databook/applinote/ic/power/led_driver/bd2606mvv_tsb_001_ug-e.pdf

> > +
> > +properties:
> > +  compatible:
> > +    const: rohm,bd2606mvv
> > +
> > +  reg:
> > +    description: I2C slave address of the controller.
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^led@[0-6]$":
> > +    type: object
> > +    $ref: common.yaml#
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        minimum: 0
> > +        maximum: 6
> > +
> > +    required:
> > +      - reg
> > +
> > +additionalProperties: false  
> 
> According to the data-sheet, BD2606 has an enable-pin. Should it be 
> visible in the bindings?
> 
yes, it should.

Regards,
Andreas

Re: [PATCH 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver

[Andreas Kemnade]

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

Re: [PATCH 1/2] dt-bindings: leds: ROHM BD2606MVV LED driver

[Matti Vaittinen]

On 4/6/23 14:33, Andreas Kemnade wrote:
> Hi Matti,
> 
> On Thu, 6 Apr 2023 11:32:42 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi Andreas,
>>
>> Thanks for the patch! Adding new support for devices is Much Appreciated!
>>
>> On 4/6/23 09:08, Andreas Kemnade wrote:
>>> Document ROHM BD2606MVV LED driver devicetree bindings.
>>>
>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
>>> ---
>>>    .../bindings/leds/rohm,bd2606mvv.yaml         | 76 +++++++++++++++++++
>>>    1 file changed, 76 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml b/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
>>> new file mode 100644
>>> index 0000000000000..6d4ddd8d31162
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/rohm,bd2606mvv.yaml
>>> @@ -0,0 +1,76 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/leds/rohm,bd2606mvv.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: BD2606MVV LED controller
>>> +
>>> +maintainers:
>>> +  - Andreas Kemnade <andreas@kemnade.info>
>>> +
>>> +description:
>>> +  The BD2606 MVV is a programmable LED controller connected via I2C that can
>>> +  drive 6 separate lines. Each of them can be individually switched on and off
>>> +  but the brightness setting is shared between two of them.
>>
>> Maybe add a link to data-sheet?
>> https://fscdn.rohm.com/en/products/databook/datasheet/ic/power/led_driver/bd2606mvv_1-e.pdf
>>
> Maybe also (because it has the register description):
> https://fscdn.rohm.com/en/products/databook/applinote/ic/power/led_driver/bd2606mvv_tsb_001_ug-e.pdf

I think both documents do contain the register description. Well, the 
eval board user-guide seems to contain the IC information as well as the 
eval board information so I am fine with either of these.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver

[Matti Vaittinen]

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


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver

[Andreas Kemnade]

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

Re: [PATCH 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver

[Matti Vaittinen]

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

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~