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

[PATCH 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.

Jean-Jacques Hiblot (4):
  leds: class: simplify the implementation of devm_of_led_get()
  led: class: Add devm_fwnode_led_get() to get a LED from a firmware
    node
  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  |  94 ++++++++++
 drivers/leds/led-class.c                      |  56 ++++--
 drivers/leds/rgb/Kconfig                      |   7 +
 drivers/leds/rgb/Makefile                     |   1 +
 drivers/leds/rgb/leds-group-multicolor.c      | 177 ++++++++++++++++++
 include/linux/leds.h                          |   4 +
 6 files changed, 325 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 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 | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 6a8ea94834fa..72fd6ee7af88 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -20,8 +20,10 @@
 #include <linux/timer.h>
 #include <uapi/linux/uleds.h>
 #include <linux/of.h>
+#include <linux/acpi.h>
 #include "leds.h"
 
+
 static struct class *leds_class;
 
 static ssize_t brightness_show(struct device *dev,
@@ -258,11 +260,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((struct led_classdev *) cdev);
 }
 
 /**
@@ -280,7 +280,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 +289,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

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

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 5:49 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> Use the devm_add_action_or_reset() helper.

...

> @@ -20,8 +20,10 @@
>  #include <linux/timer.h>
>  #include <uapi/linux/uleds.h>
>  #include <linux/of.h>
> +#include <linux/acpi.h>
>  #include "leds.h"
>
> +
>  static struct class *leds_class;
>
>  static ssize_t brightness_show(struct device *dev,

Stray changes.

...

> +       led_put((struct led_classdev *) cdev);

Casting from/to void * is redundant.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/4] led: class: Add devm_fwnode_led_get() to get a LED from a firmware node

[Jean-Jacques Hiblot]

Same functionality as devm_of_led_get(), but it takes a firmware node
as a parameter.
This mimic devm_fwnode_pwm_get() found in the PWM core.

The ACPI implementation is missing and the function returns -EOPNOTSUPP
when the firmware node is actually an ACPI node.

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

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 72fd6ee7af88..7faa953a3870 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -297,6 +297,40 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_of_led_get);
 
+/**
+ * devm_fwnode_led_get() - request a resource managed LED from firmware node
+ * @dev: device for LED consumer
+ * @fwnode: firmware node to get the LED from
+ * @index:	index of the LED to obtain in the consumer
+ *
+ * Returns the LED device parsed from the firmware node. See of_led_get().
+ *
+ * Returns: A pointer to the requested LED device or an ERR_PTR()-encoded
+ * error code on failure.
+ */
+struct led_classdev *__must_check devm_fwnode_led_get(struct device *dev,
+				       struct fwnode_handle *fwnode,
+				       int index)
+{
+	struct led_classdev *led = ERR_PTR(-ENODEV);
+	int ret;
+
+	if (is_of_node(fwnode))
+		led = of_led_get(to_of_node(fwnode), index);
+	else if (is_acpi_node(fwnode))
+		/* ACPI support is not yet implemented */
+		led = ERR_PTR(-EOPNOTSUPP);
+	if (IS_ERR(led))
+		return led;
+
+	ret = devm_add_action_or_reset(dev, devm_led_release, led);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return led;
+}
+EXPORT_SYMBOL_GPL(devm_fwnode_led_get);
+
 static int led_classdev_next_name(const char *init_name, char *name,
 				  size_t len)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..ace0130bfcd2 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -216,6 +216,10 @@ extern void led_put(struct led_classdev *led_cdev);
 struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 						  int index);
 
+struct led_classdev *__must_check devm_fwnode_led_get(struct device *dev,
+				       struct fwnode_handle *fwnode,
+				       int index);
+
 /**
  * led_blink_set - set blinking with software fallback
  * @led_cdev: the LED to start blinking
-- 
2.25.1

Re: [PATCH 2/4] led: class: Add devm_fwnode_led_get() to get a LED from a firmware node

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 5:49 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> Same functionality as devm_of_led_get(), but it takes a firmware node
> as a parameter.
> This mimic devm_fwnode_pwm_get() found in the PWM core.
>
> The ACPI implementation is missing and the function returns -EOPNOTSUPP
> when the firmware node is actually an ACPI node.

...

> +       if (is_of_node(fwnode))
> +               led = of_led_get(to_of_node(fwnode), index);
> +       else if (is_acpi_node(fwnode))
> +               /* ACPI support is not yet implemented */
> +               led = ERR_PTR(-EOPNOTSUPP);
> +       if (IS_ERR(led))
> +               return led;

Yeah, acpi.h is needed here and it needs to be ordered, I guess should
be added somewhere at the top of the header block.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 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  | 94 +++++++++++++++++++
 1 file changed, 94 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..30a67985ae33
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
@@ -0,0 +1,94 @@
+# 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
+
+  multi-led:
+    type: object
+
+    patternProperties:
+      "^led-[0-9a-z]+$":
+        type: object
+        $ref: common.yaml#
+
+        additionalProperties: false
+
+        properties:
+          leds:
+            maxItems: 1
+
+          color: true
+
+        required:
+          - leds
+          - color
+
+required:
+  - compatible
+
+allOf:
+  - $ref: leds-class-multicolor.yaml#
+
+additionalProperties: 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>;
+        };
+    };
+
+    multicolor-led-group {
+        compatible = "leds-group-multicolor";
+
+        multi-led {
+          color = <LED_COLOR_ID_RGB>;
+          function = LED_FUNCTION_INDICATOR;
+          max-brightness = <256>;
+
+          led-red {
+              leds = <&led0>;
+              color = <LED_COLOR_ID_RED>;
+          };
+
+          led-green {
+              leds = <&led1>;
+              color = <LED_COLOR_ID_GREEN>;
+          };
+
+          led-blue {
+              leds = <&led2>;
+              color = <LED_COLOR_ID_BLUE>;
+          };
+        };
+    };
+
+...
-- 
2.25.1

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

[Rob Herring]

On Wed, Jun 15, 2022 at 05:49:17PM +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  | 94 +++++++++++++++++++
>  1 file changed, 94 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..30a67985ae33
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
> @@ -0,0 +1,94 @@
> +# 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
> +
> +  multi-led:
> +    type: object
> +
> +    patternProperties:
> +      "^led-[0-9a-z]+$":
> +        type: object
> +        $ref: common.yaml#
> +
> +        additionalProperties: false
> +
> +        properties:
> +          leds:

Not a standard property. What is the type?

Really, just do a GPIO multi-color LED binding similar to the PWM one 
rather than adding this layer. I suppose you could combine LEDs from all 
different controllers, but that seems somewhat unlikely to me.

Rob

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

[Jean-Jacques Hiblot]

On 28/06/2022 00:12, Rob Herring wrote:
> On Wed, Jun 15, 2022 at 05:49:17PM +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  | 94 +++++++++++++++++++
>>   1 file changed, 94 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..30a67985ae33
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
>> @@ -0,0 +1,94 @@
>> +# 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
>> +
>> +  multi-led:
>> +    type: object
>> +
>> +    patternProperties:
>> +      "^led-[0-9a-z]+$":
>> +        type: object
>> +        $ref: common.yaml#
>> +
>> +        additionalProperties: false
>> +
>> +        properties:
>> +          leds:
> Not a standard property. What is the type?
That would be a reference to the node of a LED
> Really, just do a GPIO multi-color LED binding similar to the PWM one
> rather than adding this layer. I suppose you could combine LEDs from all
> different controllers, but that seems somewhat unlikely to me.

I'm not using gpio leds, rather leds driven by two TLC5925.

I agree that combining from different model of controller is unlikely. 
However from 2 separate chips of the same model is not (ex: driving 5 
RGB LEDs with two 8-output chips)

In the case of the TLC5925, that is not really a problem because as long 
as the chips are on the same CS, they are considered as a single entity 
by the driver. But for I2C chips at least that would be a problem.


JJ

>
> Rob

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

[Rob Herring]

On Fri, Jul 01, 2022 at 11:33:22AM +0200, Jean-Jacques Hiblot wrote:
> 
> On 28/06/2022 00:12, Rob Herring wrote:
> > On Wed, Jun 15, 2022 at 05:49:17PM +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  | 94 +++++++++++++++++++
> > >   1 file changed, 94 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..30a67985ae33
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
> > > @@ -0,0 +1,94 @@
> > > +# 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
> > > +
> > > +  multi-led:
> > > +    type: object
> > > +
> > > +    patternProperties:
> > > +      "^led-[0-9a-z]+$":
> > > +        type: object
> > > +        $ref: common.yaml#
> > > +
> > > +        additionalProperties: false
> > > +
> > > +        properties:
> > > +          leds:
> > Not a standard property. What is the type?
> That would be a reference to the node of a LED
> > Really, just do a GPIO multi-color LED binding similar to the PWM one
> > rather than adding this layer. I suppose you could combine LEDs from all
> > different controllers, but that seems somewhat unlikely to me.
> 
> I'm not using gpio leds, rather leds driven by two TLC5925.
> 
> I agree that combining from different model of controller is unlikely.
> However from 2 separate chips of the same model is not (ex: driving 5 RGB
> LEDs with two 8-output chips)
> 
> In the case of the TLC5925, that is not really a problem because as long as
> the chips are on the same CS, they are considered as a single entity by the
> driver. But for I2C chips at least that would be a problem.

Okay.

I think the binding can be simplified a bit to just this:

multi-led {
    compatible = "leds-group-multicolor";
    color = <LED_COLOR_ID_RGB>;
    function = LED_FUNCTION_INDICATOR;
    
    leds = <&red_led>, <&green_led>, <&blue_led>;
};

The individual color should be defined in the parent LED node (e.g. 
red_led). You can either look up the color or the index in 'leds' 
defines the color.

Also, I don't think 'max-brightness' here makes sense. That's a property 
of the parent LED.

Rob

[PATCH 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                 |   7 +
 drivers/leds/rgb/Makefile                |   1 +
 drivers/leds/rgb/leds-group-multicolor.c | 177 +++++++++++++++++++++++
 3 files changed, 185 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..b50790385561 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -2,6 +2,13 @@
 
 if LEDS_CLASS_MULTICOLOR
 
+config LEDS_GRP_MULTICOLOR
+	tristate "multi-color LED grouping Support"
+	depends on PWM
+	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..872854aed6eb
--- /dev/null
+++ b/drivers/leds/rgb/leds-group-multicolor.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * multi-color LED built with monochromatic LED devices
+ *
+ * This driver is derived from the leds-pwm-multicolor driver
+ *
+ * Copyright 2022 Jean-Jacques Hiblot <jjhiblot@traphandler.com>
+ */
+
+#include <linux/err.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.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 iterate_subleds(struct device *dev, struct led_mcg_priv *priv,
+			   struct fwnode_handle *mcnode)
+{
+	struct mc_subled *subled = priv->mc_cdev.subled_info;
+	struct fwnode_handle *fwnode;
+	int ret;
+
+	/* iterate over the nodes inside the multi-led node */
+	fwnode_for_each_child_node(mcnode, fwnode) {
+		u32 color;
+		struct led_classdev *led_cdev;
+
+		led_cdev = devm_fwnode_led_get(dev, fwnode, 0);
+		if (IS_ERR(led_cdev)) {
+			ret = PTR_ERR(led_cdev);
+			dev_err(dev, "unable to request LED: %d\n", ret);
+			goto release_fwnode;
+		}
+		priv->monochromatics[priv->mc_cdev.num_colors] = led_cdev;
+
+		ret = fwnode_property_read_u32(fwnode, "color", &color);
+		if (ret) {
+			dev_err(dev, "cannot read color: %d\n", ret);
+			goto release_fwnode;
+		}
+		subled[priv->mc_cdev.num_colors].color_index = color;
+
+		/* Make the sysfs of the monochromatic LED read-only */
+		led_cdev->flags |= LED_SYSFS_DISABLE;
+
+		priv->mc_cdev.num_colors++;
+	}
+
+	return 0;
+
+release_fwnode:
+	fwnode_handle_put(fwnode);
+	return ret;
+}
+
+static int led_mcg_probe(struct platform_device *pdev)
+{
+	struct fwnode_handle *mcnode, *fwnode;
+	struct led_init_data init_data = {};
+	struct led_classdev *cdev;
+	struct mc_subled *subled;
+	struct led_mcg_priv *priv;
+	int count = 0;
+	int ret = 0;
+
+	mcnode = device_get_named_child_node(&pdev->dev, "multi-led");
+	if (!mcnode)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "expected multi-led node\n");
+
+	/* count the nodes inside the multi-led node */
+	fwnode_for_each_child_node(mcnode, fwnode)
+		count++;
+
+	priv = devm_kzalloc(&pdev->dev,
+			    struct_size(priv, monochromatics, count),
+			    GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto release_mcnode;
+	}
+
+	subled = devm_kcalloc(&pdev->dev, count, sizeof(*subled), GFP_KERNEL);
+	if (!subled) {
+		ret = -ENOMEM;
+		goto release_mcnode;
+	}
+	priv->mc_cdev.subled_info = subled;
+
+	/* init the multicolor's LED class device */
+	cdev = &priv->mc_cdev.led_cdev;
+	fwnode_property_read_u32(mcnode, "max-brightness",
+				 &cdev->max_brightness);
+	cdev->flags = LED_CORE_SUSPENDRESUME;
+	cdev->brightness_set_blocking = led_mcg_set;
+
+	ret = iterate_subleds(&pdev->dev, priv, mcnode);
+	if (ret)
+		goto release_mcnode;
+
+	init_data.fwnode = mcnode;
+	ret = devm_led_classdev_multicolor_register_ext(&pdev->dev,
+							&priv->mc_cdev,
+							&init_data);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register multicolor led for %s: %d\n",
+			cdev->name, ret);
+		goto release_mcnode;
+	}
+
+	ret = led_mcg_set(cdev, cdev->brightness);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to set led value for %s: %d",
+				     cdev->name, ret);
+
+	return 0;
+
+release_mcnode:
+	fwnode_handle_put(mcnode);
+	return ret;
+}
+
+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 4/4] leds: Add a multicolor LED driver to group monochromatic LEDs

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 5:49 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> 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.

...

> +#include <linux/err.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>

Missed math.h

...

> +static int iterate_subleds(struct device *dev, struct led_mcg_priv *priv,
> +                          struct fwnode_handle *mcnode)

Use namespace even for static functions (think about tracing, for example).

led_mcg_iterave_subleds

> +{
> +       struct mc_subled *subled = priv->mc_cdev.subled_info;
> +       struct fwnode_handle *fwnode;
> +       int ret;
> +
> +       /* iterate over the nodes inside the multi-led node */
> +       fwnode_for_each_child_node(mcnode, fwnode) {
> +               u32 color;
> +               struct led_classdev *led_cdev;
> +
> +               led_cdev = devm_fwnode_led_get(dev, fwnode, 0);
> +               if (IS_ERR(led_cdev)) {

> +                       ret = PTR_ERR(led_cdev);
> +                       dev_err(dev, "unable to request LED: %d\n", ret);

ret = dev_err_probe(...);

> +                       goto release_fwnode;
> +               }
> +               priv->monochromatics[priv->mc_cdev.num_colors] = led_cdev;
> +
> +               ret = fwnode_property_read_u32(fwnode, "color", &color);
> +               if (ret) {
> +                       dev_err(dev, "cannot read color: %d\n", ret);
> +                       goto release_fwnode;
> +               }
> +               subled[priv->mc_cdev.num_colors].color_index = color;
> +
> +               /* Make the sysfs of the monochromatic LED read-only */
> +               led_cdev->flags |= LED_SYSFS_DISABLE;
> +
> +               priv->mc_cdev.num_colors++;
> +       }
> +
> +       return 0;
> +
> +release_fwnode:
> +       fwnode_handle_put(fwnode);
> +       return ret;
> +}

...

> +       /* count the nodes inside the multi-led node */
> +       fwnode_for_each_child_node(mcnode, fwnode)
> +               count++;

Don't we have a _count API? Hmm... Indeed, we have it only for a
device and not for fwnode...

...

> +       priv = devm_kzalloc(&pdev->dev,
> +                           struct_size(priv, monochromatics, count),
> +                           GFP_KERNEL);
> +       if (!priv) {
> +               ret = -ENOMEM;
> +               goto release_mcnode;

This is the wrong order. You shouldn't mix non-devm_ APIs with devm_
like this. devm_ calls always should be first. You have two options
(at least?): 1) drop devm_ and switch to plain error handling and
->remove(); 2) make devm_ wrappers for the certain calls.

> +       }

...

> +       if (ret) {
> +               dev_err(&pdev->dev,
> +                       "failed to register multicolor led for %s: %d\n",
> +                       cdev->name, ret);

Taking into account above,
return dev_err_probe(...);

> +               goto release_mcnode;
> +       }

-- 
With Best Regards,
Andy Shevchenko