[PATCH v2 0/4] Add a multicolor LED driver for groups of monochromatic LEDs

[PATCH v2 0/4] Add a multicolor LED driver for groups of monochromatic LEDs

[Jean-Jacques Hiblot]

Some HW design implement multicolor LEDs with several monochromatic LEDs.
Grouping the monochromatic LEDs allows to configure them in sync and use
the triggers.
The PWM multicolor LED driver implements such grouping but only for
PWM-based LEDs. As this feature is also desirable for the other types of
LEDs, this series implements it for any kind of LED device.

changes v1->v2:
 - Followed Rob Herrings's suggestion to make the dt binding much simpler.
 - Added a patch to store the color property of a LED in its class
   structure (struct led_classdev).

Jean-Jacques Hiblot (4):
  leds: class: simplify the implementation of devm_of_led_get()
  leds: class: store the color index in struct led_classdev
  dt-bindings: leds: Add binding for a multicolor group of LEDs
  leds: Add a multicolor LED driver to group monochromatic LEDs

 .../bindings/leds/leds-group-multicolor.yaml  |  61 +++++++
 drivers/leds/led-class.c                      |  27 ++--
 drivers/leds/rgb/Kconfig                      |   6 +
 drivers/leds/rgb/Makefile                     |   1 +
 drivers/leds/rgb/leds-group-multicolor.c      | 153 ++++++++++++++++++
 include/linux/leds.h                          |   1 +
 6 files changed, 235 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
 create mode 100644 drivers/leds/rgb/leds-group-multicolor.c

-- 
2.25.1

[PATCH v2 1/4] leds: class: simplify the implementation of devm_of_led_get()

[Jean-Jacques Hiblot]

Use the devm_add_action_or_reset() helper.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/leds/led-class.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 6a8ea94834fa..2c0d979d0c8a 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -258,11 +258,9 @@ void led_put(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_put);
 
-static void devm_led_release(struct device *dev, void *res)
+static void devm_led_release(void *cdev)
 {
-	struct led_classdev **p = res;
-
-	led_put(*p);
+	led_put(cdev);
 }
 
 /**
@@ -280,7 +278,7 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 						  int index)
 {
 	struct led_classdev *led;
-	struct led_classdev **dr;
+	int ret;
 
 	if (!dev)
 		return ERR_PTR(-EINVAL);
@@ -289,15 +287,9 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 	if (IS_ERR(led))
 		return led;
 
-	dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *),
-			  GFP_KERNEL);
-	if (!dr) {
-		led_put(led);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	*dr = led;
-	devres_add(dev, dr);
+	ret = devm_add_action_or_reset(dev, devm_led_release, led);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return led;
 }
-- 
2.25.1

[PATCH v2 2/4] leds: class: store the color index in struct led_classdev

[Jean-Jacques Hiblot]

This information might be useful for more than only deriving the led's
name.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/leds/led-class.c | 7 +++++++
 include/linux/leds.h     | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 2c0d979d0c8a..537379f09801 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -350,6 +350,10 @@ int led_classdev_register_ext(struct device *parent,
 			if (fwnode_property_present(init_data->fwnode,
 						    "retain-state-shutdown"))
 				led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
+
+			if (fwnode_property_present(init_data->fwnode, "color"))
+				fwnode_property_read_u32(init_data->fwnode, "color",
+							 &led_cdev->color);
 		}
 	} else {
 		proposed_name = led_cdev->name;
@@ -359,6 +363,9 @@ int led_classdev_register_ext(struct device *parent,
 	if (ret < 0)
 		return ret;
 
+	if (led_cdev->color >= LED_COLOR_ID_MAX)
+		dev_warn(parent, "LED %s color identifier out of range\n", final_name);
+
 	mutex_init(&led_cdev->led_access);
 	mutex_lock(&led_cdev->led_access);
 	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..fe6346604e36 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -71,6 +71,7 @@ struct led_classdev {
 	const char		*name;
 	unsigned int brightness;
 	unsigned int max_brightness;
+	unsigned int color;
 	int			 flags;
 
 	/* Lower 16 bits reflect status */
-- 
2.25.1

[PATCH v2 3/4] dt-bindings: leds: Add binding for a multicolor group of LEDs

[Jean-Jacques Hiblot]

This allows to group multiple monochromatic LEDs into a multicolor
LED, e.g. RGB LEDs.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 .../bindings/leds/leds-group-multicolor.yaml  | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
new file mode 100644
index 000000000000..79e5882a08e2
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-group-multicolor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Multi-color LED built with monochromatic LEDs
+
+maintainers:
+  - Jean-Jacques Hiblot <jjhiblot@traphandler.com>
+
+description: |
+  This driver combines several monochromatic LEDs into one multi-color
+  LED using the multicolor LED class.
+
+properties:
+  compatible:
+    const: leds-group-multicolor
+
+  leds:
+    description:
+      An aray of monochromatic leds
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+required:
+  - leds
+
+allOf:
+  - $ref: leds-class-multicolor.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    monochromatic-leds {
+        compatible = "gpio-leds";
+
+        led0: led-0 {
+            gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
+        };
+
+        led1: led-1 {
+            gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
+        };
+
+        led2: led-2 {
+            gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
+        };
+    };
+
+    multi-led {
+        compatible = "leds-group-multicolor";
+        color = <LED_COLOR_ID_RGB>;
+        function = LED_FUNCTION_INDICATOR;
+        leds = <&led0>, <&led1>, <&led2>;
+    };
+
+...
-- 
2.25.1

Re: [PATCH v2 3/4] dt-bindings: leds: Add binding for a multicolor group of LEDs

[Rob Herring]

On Tue, 19 Jul 2022 21:18:00 +0200, Jean-Jacques Hiblot wrote:
> This allows to group multiple monochromatic LEDs into a multicolor
> LED, e.g. RGB LEDs.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>  .../bindings/leds/leds-group-multicolor.yaml  | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-group-multicolor.example.dtb: multi-led: Unevaluated properties are not allowed ('color', 'function' were unexpected)
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v2 3/4] dt-bindings: leds: Add binding for a multicolor group of LEDs

[Rob Herring]

On Tue, Jul 19, 2022 at 03:25:13PM -0600, Rob Herring wrote:
> On Tue, 19 Jul 2022 21:18:00 +0200, Jean-Jacques Hiblot wrote:
> > This allows to group multiple monochromatic LEDs into a multicolor
> > LED, e.g. RGB LEDs.
> > 
> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> > ---
> >  .../bindings/leds/leds-group-multicolor.yaml  | 61 +++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-group-multicolor.example.dtb: multi-led: Unevaluated properties are not allowed ('color', 'function' were unexpected)
> 	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml

Looks like this is fixed by commit de40c8496ead ("dt-bindings: leds: 
class-multicolor: reference class directly in multi-led node") in next.

Rob

Re: [PATCH v2 3/4] dt-bindings: leds: Add binding for a multicolor group of LEDs

[Rob Herring]

On Tue, 19 Jul 2022 21:18:00 +0200, Jean-Jacques Hiblot wrote:
> This allows to group multiple monochromatic LEDs into a multicolor
> LED, e.g. RGB LEDs.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>  .../bindings/leds/leds-group-multicolor.yaml  | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 4/4] leds: Add a multicolor LED driver to group monochromatic LEDs

[Jean-Jacques Hiblot]

By allowing to group multiple monochrome LED into multicolor LEDs,
all involved LEDs can be controlled in-sync. This enables using effects
using triggers, etc.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/leds/rgb/Kconfig                 |   6 +
 drivers/leds/rgb/Makefile                |   1 +
 drivers/leds/rgb/leds-group-multicolor.c | 153 +++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/leds/rgb/leds-group-multicolor.c

diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index 204cf470beae..70b157d1fdca 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -2,6 +2,12 @@
 
 if LEDS_CLASS_MULTICOLOR
 
+config LEDS_GRP_MULTICOLOR
+	tristate "multi-color LED grouping Support"
+	help
+	  This option enables support for monochrome LEDs that are
+	  grouped into multicolor LEDs.
+
 config LEDS_PWM_MULTICOLOR
 	tristate "PWM driven multi-color LED Support"
 	depends on PWM
diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
index 0675bc0f6e18..4de087ad79bc 100644
--- a/drivers/leds/rgb/Makefile
+++ b/drivers/leds/rgb/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 
+obj-$(CONFIG_LEDS_GRP_MULTICOLOR)	+= leds-group-multicolor.o
 obj-$(CONFIG_LEDS_PWM_MULTICOLOR)	+= leds-pwm-multicolor.o
 obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg.o
diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
new file mode 100644
index 000000000000..be71b85edfb5
--- /dev/null
+++ b/drivers/leds/rgb/leds-group-multicolor.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * multi-color LED built with monochromatic LED devices
+ *
+ * Copyright 2022 Jean-Jacques Hiblot <jjhiblot@traphandler.com>
+ */
+
+#include <linux/err.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
+#include <linux/math.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+struct led_mcg_priv {
+	struct led_classdev_mc mc_cdev;
+	struct led_classdev **monochromatics;
+};
+
+static int led_mcg_set(struct led_classdev *cdev,
+			  enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct led_mcg_priv *priv =
+		container_of(mc_cdev, struct led_mcg_priv, mc_cdev);
+	int i;
+
+	led_mc_calc_color_components(mc_cdev, brightness);
+
+	for (i = 0; i < mc_cdev->num_colors; i++) {
+		struct led_classdev *mono = priv->monochromatics[i];
+		int actual_led_brightness;
+
+		/*
+		 * Scale the intensity according the max brightness of the
+		 * monochromatic LED
+		 */
+		actual_led_brightness = DIV_ROUND_CLOSEST(
+			mono->max_brightness * mc_cdev->subled_info[i].brightness,
+			mc_cdev->led_cdev.max_brightness);
+
+		led_set_brightness(mono, actual_led_brightness);
+	}
+	return 0;
+}
+
+static int led_mcg_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct led_init_data init_data = {};
+	struct led_classdev *cdev;
+	struct mc_subled *subled;
+	struct led_mcg_priv *priv;
+	int i, count, ret;
+	unsigned int max_brightness;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return  -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, priv);
+
+
+	count = 0;
+	max_brightness = 0;
+	for (;;) {
+		struct led_classdev *led_cdev;
+
+		led_cdev = devm_of_led_get(dev, count);
+		if (IS_ERR(led_cdev)) {
+			/* Reached the end of the list ? */
+			if (PTR_ERR(led_cdev) == -ENOENT)
+				break;
+			return dev_err_probe(dev, PTR_ERR(led_cdev),
+					     "Unable to get led #%d", i);
+		}
+		count++;
+
+		/* Make the sysfs of the monochromatic LED read-only */
+		led_cdev->flags |= LED_SYSFS_DISABLE;
+
+		priv->monochromatics = devm_krealloc(dev, priv->monochromatics,
+					count * sizeof(*priv->monochromatics),
+					GFP_KERNEL);
+		if (!priv->monochromatics)
+			return -ENOMEM;
+
+		priv->monochromatics[count - 1] = led_cdev;
+
+		max_brightness = max(max_brightness, led_cdev->max_brightness);
+	}
+
+	subled = devm_kzalloc(dev, count * sizeof(*subled), GFP_KERNEL);
+	if (!subled)
+		return -ENOMEM;
+	priv->mc_cdev.subled_info = subled;
+
+	for (i = 0; i < count; i++) {
+		struct led_classdev *led_cdev = priv->monochromatics[i];
+
+		subled[i].color_index = led_cdev->color;
+		/* by default all LEDs have full intensity */
+		subled[i].intensity = max_brightness;
+
+	}
+
+	/* init the multicolor's LED class device */
+	cdev = &priv->mc_cdev.led_cdev;
+	cdev->flags = LED_CORE_SUSPENDRESUME;
+	cdev->brightness_set_blocking = led_mcg_set;
+	cdev->max_brightness = max_brightness;
+	cdev->color = LED_COLOR_ID_MULTI;
+	priv->mc_cdev.num_colors = count;
+
+	init_data.fwnode = of_fwnode_handle(dev_of_node(dev));
+	ret = devm_led_classdev_multicolor_register_ext(dev, &priv->mc_cdev,
+							&init_data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+			"failed to register multicolor led for %s: %d\n",
+			cdev->name, ret);
+
+	ret = led_mcg_set(cdev, cdev->brightness);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to set led value for %s: %d",
+				     cdev->name, ret);
+
+	return 0;
+}
+
+static const struct of_device_id of_led_mcg_match[] = {
+	{ .compatible = "leds-group-multicolor" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_led_mcg_match);
+
+static struct platform_driver led_mcg_driver = {
+	.probe		= led_mcg_probe,
+	.driver		= {
+		.name	= "leds_group_multicolor",
+		.of_match_table = of_led_mcg_match,
+	}
+};
+module_platform_driver(led_mcg_driver);
+
+MODULE_AUTHOR("Jean-Jacques Hiblot <jjhiblot@traphandler.com>");
+MODULE_DESCRIPTION("multi-color LED group driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-group-multicolor");
-- 
2.25.1

Re: [PATCH v2 4/4] leds: Add a multicolor LED driver to group monochromatic LEDs

[Christophe JAILLET]

Hi,

a few nitpick below.


Le 19/07/2022 à 21:18, Jean-Jacques Hiblot a écrit :
> By allowing to group multiple monochrome LED into multicolor LEDs,
> all involved LEDs can be controlled in-sync. This enables using effects
> using triggers, etc.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>   drivers/leds/rgb/Kconfig                 |   6 +
>   drivers/leds/rgb/Makefile                |   1 +
>   drivers/leds/rgb/leds-group-multicolor.c | 153 +++++++++++++++++++++++
>   3 files changed, 160 insertions(+)
>   create mode 100644 drivers/leds/rgb/leds-group-multicolor.c
> 
> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> index 204cf470beae..70b157d1fdca 100644
> --- a/drivers/leds/rgb/Kconfig
> +++ b/drivers/leds/rgb/Kconfig
> @@ -2,6 +2,12 @@
>   
>   if LEDS_CLASS_MULTICOLOR
>   
> +config LEDS_GRP_MULTICOLOR
> +	tristate "multi-color LED grouping Support"

Why "Support" and not "support"?

> +	help
> +	  This option enables support for monochrome LEDs that are
> +	  grouped into multicolor LEDs.
> +
>   config LEDS_PWM_MULTICOLOR
>   	tristate "PWM driven multi-color LED Support"
>   	depends on PWM
> diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> index 0675bc0f6e18..4de087ad79bc 100644
> --- a/drivers/leds/rgb/Makefile
> +++ b/drivers/leds/rgb/Makefile
> @@ -1,4 +1,5 @@
>   # SPDX-License-Identifier: GPL-2.0
>   
> +obj-$(CONFIG_LEDS_GRP_MULTICOLOR)	+= leds-group-multicolor.o
>   obj-$(CONFIG_LEDS_PWM_MULTICOLOR)	+= leds-pwm-multicolor.o
>   obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg.o
> diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
> new file mode 100644
> index 000000000000..be71b85edfb5
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-group-multicolor.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * multi-color LED built with monochromatic LED devices
> + *
> + * Copyright 2022 Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/math.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +struct led_mcg_priv {
> +	struct led_classdev_mc mc_cdev;
> +	struct led_classdev **monochromatics;
> +};
> +
> +static int led_mcg_set(struct led_classdev *cdev,
> +			  enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct led_mcg_priv *priv =
> +		container_of(mc_cdev, struct led_mcg_priv, mc_cdev);
> +	int i;
> +
> +	led_mc_calc_color_components(mc_cdev, brightness);
> +
> +	for (i = 0; i < mc_cdev->num_colors; i++) {
> +		struct led_classdev *mono = priv->monochromatics[i];
> +		int actual_led_brightness;
> +
> +		/*
> +		 * Scale the intensity according the max brightness of the
> +		 * monochromatic LED
> +		 */
> +		actual_led_brightness = DIV_ROUND_CLOSEST(
> +			mono->max_brightness * mc_cdev->subled_info[i].brightness,
> +			mc_cdev->led_cdev.max_brightness);
> +
> +		led_set_brightness(mono, actual_led_brightness);
> +	}
> +	return 0;
> +}
> +
> +static int led_mcg_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct led_init_data init_data = {};
> +	struct led_classdev *cdev;
> +	struct mc_subled *subled;
> +	struct led_mcg_priv *priv;
> +	int i, count, ret;
> +	unsigned int max_brightness;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return  -ENOMEM;

Extra space between "return" and "-ENOMEM".

> +
> +	dev_set_drvdata(&pdev->dev, priv);

Is it needed?
(apparently, there is no dev_get_drvdata())

> +
> +

One empty line is enough.

> +	count = 0;
> +	max_brightness = 0;

Could be initialized when the variable are declared, as already done 
with "init_data". I guess it is a matter of taste.

> +	for (;;) {
> +		struct led_classdev *led_cdev;
> +
> +		led_cdev = devm_of_led_get(dev, count);
> +		if (IS_ERR(led_cdev)) {
> +			/* Reached the end of the list ? */
> +			if (PTR_ERR(led_cdev) == -ENOENT)
> +				break;
> +			return dev_err_probe(dev, PTR_ERR(led_cdev),
> +					     "Unable to get led #%d", i);

"i" is not used yet. "count"?

> +		}
> +		count++;
> +
> +		/* Make the sysfs of the monochromatic LED read-only */
> +		led_cdev->flags |= LED_SYSFS_DISABLE;
> +
> +		priv->monochromatics = devm_krealloc(dev, priv->monochromatics,
> +					count * sizeof(*priv->monochromatics),
> +					GFP_KERNEL);
> +		if (!priv->monochromatics)
> +			return -ENOMEM;
> +
> +		priv->monochromatics[count - 1] = led_cdev;
> +
> +		max_brightness = max(max_brightness, led_cdev->max_brightness);
> +	}
> +
> +	subled = devm_kzalloc(dev, count * sizeof(*subled), GFP_KERNEL);
> +	if (!subled)
> +		return -ENOMEM;
> +	priv->mc_cdev.subled_info = subled;
> +
> +	for (i = 0; i < count; i++) {
> +		struct led_classdev *led_cdev = priv->monochromatics[i];
> +
> +		subled[i].color_index = led_cdev->color;
> +		/* by default all LEDs have full intensity */
> +		subled[i].intensity = max_brightness;
> +

Uneeded empty line.

> +	}
> +
> +	/* init the multicolor's LED class device */
> +	cdev = &priv->mc_cdev.led_cdev;
> +	cdev->flags = LED_CORE_SUSPENDRESUME;
> +	cdev->brightness_set_blocking = led_mcg_set;
> +	cdev->max_brightness = max_brightness;
> +	cdev->color = LED_COLOR_ID_MULTI;
> +	priv->mc_cdev.num_colors = count;
> +
> +	init_data.fwnode = of_fwnode_handle(dev_of_node(dev));
> +	ret = devm_led_classdev_multicolor_register_ext(dev, &priv->mc_cdev,
> +							&init_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			"failed to register multicolor led for %s: %d\n",
> +			cdev->name, ret);

No need to add 'ret' in the message, dev_err_probe() already display it.

> +
> +	ret = led_mcg_set(cdev, cdev->brightness);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to set led value for %s: %d",
> +				     cdev->name, ret);

same here.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_led_mcg_match[] = {
> +	{ .compatible = "leds-group-multicolor" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_led_mcg_match);
> +
> +static struct platform_driver led_mcg_driver = {
> +	.probe		= led_mcg_probe,
> +	.driver		= {
> +		.name	= "leds_group_multicolor",
> +		.of_match_table = of_led_mcg_match,
> +	}
> +};
> +module_platform_driver(led_mcg_driver);
> +
> +MODULE_AUTHOR("Jean-Jacques Hiblot <jjhiblot@traphandler.com>");
> +MODULE_DESCRIPTION("multi-color LED group driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-group-multicolor");

Re: [PATCH v2 4/4] leds: Add a multicolor LED driver to group monochromatic LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

Hi!

> By allowing to group multiple monochrome LED into multicolor LEDs,
> all involved LEDs can be controlled in-sync. This enables using effects
> using triggers, etc.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>  drivers/leds/rgb/Kconfig                 |   6 +
>  drivers/leds/rgb/Makefile                |   1 +
>  drivers/leds/rgb/leds-group-multicolor.c | 153 +++++++++++++++++++++++
>  3 files changed, 160 insertions(+)
>  create mode 100644 drivers/leds/rgb/leds-group-multicolor.c
> 
> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> index 204cf470beae..70b157d1fdca 100644
> --- a/drivers/leds/rgb/Kconfig
> +++ b/drivers/leds/rgb/Kconfig
> @@ -2,6 +2,12 @@
>  
>  if LEDS_CLASS_MULTICOLOR
>  
> +config LEDS_GRP_MULTICOLOR
> +	tristate "multi-color LED grouping Support"

Make this
        tristate "Multi-color LED grouping support"

Others commented on other issues.

Thank you,
							Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]