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

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

[Jean-Jacques Hiblot]

Hi,

Resending this series with only a minor modification in the binding
example after the comments from Sascha Hauer.

Thanks,
JJ


Original v3 message:

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 v2->v3, only minor changes:
 - rephrased the Kconfig descritpion
 - make the sysfs interface of underlying LEDs read-only only if the probe
   is successful.
 - sanitize the header files
 - removed the useless call to dev_set_drvdata()
 - use dev_fwnode() to get the fwnode to the 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  |  64 ++++++++
 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, 238 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

[RESEND PATCH v3 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

[RESEND PATCH v3 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

[RESEND PATCH v3 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/leds/leds-group-multicolor.yaml  | 64 +++++++++++++++++++
 1 file changed, 64 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..8ed059a5a724
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
@@ -0,0 +1,64 @@
+# 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>;
+            color = <LED_COLOR_ID_RED>;
+        };
+
+        led1: led-1 {
+            gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
+            color = <LED_COLOR_ID_GREEN>;
+        };
+
+        led2: led-2 {
+            gpios = <&mcu_pio 2 GPIO_ACTIVE_HIGH>;
+            color = <LED_COLOR_ID_BLUE>;
+        };
+    };
+
+    multi-led {
+        compatible = "leds-group-multicolor";
+        color = <LED_COLOR_ID_RGB>;
+        function = LED_FUNCTION_INDICATOR;
+        leds = <&led0>, <&led1>, <&led2>;
+    };
+
+...
-- 
2.25.1

[RESEND PATCH v3 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..c2610c47a82d 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..61f9e1954fc4
--- /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/leds.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.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;
+
+	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", count);
+		}
+		count++;
+
+		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;
+		/* configure the LED intensity to its maximum */
+		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 = dev_fwnode(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.\n",
+			cdev->name);
+
+	ret = led_mcg_set(cdev, cdev->brightness);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to set led value for %s.",
+				     cdev->name);
+
+	for (i = 0; i < count; i++) {
+		struct led_classdev *led_cdev = priv->monochromatics[i];
+
+		/* Make the sysfs of the monochromatic LED read-only */
+		led_cdev->flags |= LED_SYSFS_DISABLE;
+	}
+
+	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: [RESEND PATCH v3 1/4] leds: class: simplify the implementation of devm_of_led_get()

[Andy Shevchenko]

On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> Use the devm_add_action_or_reset() helper.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> 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
>


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> Hi,
>
> Resending this series with only a minor modification in the binding
> example after the comments from Sascha Hauer.

I would suggest to Cc a new version (if required a new version) to
Lee. It was a discussion about advancing the LED subsystem patch queue
with his help.

> Original v3 message:
>
> 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 v2->v3, only minor changes:
>  - rephrased the Kconfig descritpion
>  - make the sysfs interface of underlying LEDs read-only only if the probe
>    is successful.
>  - sanitize the header files
>  - removed the useless call to dev_set_drvdata()
>  - use dev_fwnode() to get the fwnode to the 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  |  64 ++++++++
>  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, 238 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
>


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Sep 17, 2022 at 11:14 AM 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.

...

> +config LEDS_GRP_MULTICOLOR
> +       tristate "Multi-color LED grouping support"
> +       help
> +         This option enables support for monochrome LEDs that are
> +         grouped into multicolor LEDs.

What will be the module name in case of "m" choice?

...

> +       struct led_mcg_priv *priv =
> +               container_of(mc_cdev, struct led_mcg_priv, mc_cdev);

One line?

...

> +               /*
> +                * Scale the intensity according the max brightness of the
> +                * monochromatic LED

Usually we put a grammar period at the end of sentences in multi-line comments.

> +                */

...

> +               actual_led_brightness = DIV_ROUND_CLOSEST(
> +                       mono->max_brightness * mc_cdev->subled_info[i].brightness,
> +                       mc_cdev->led_cdev.max_brightness);

Can you fix an indentation, so it won't leave the line ending by open
parenthesis? I believe with the help of a temporary variable it can be
easily achieved.

...

> +       for (;;) {
> +               struct led_classdev *led_cdev;

> +               led_cdev = devm_of_led_get(dev, count);

Why _of_ variant? Please, make this OF independent since it's
pretending to cover not only OF-based systems.


> +               if (IS_ERR(led_cdev)) {

> +                       /* Reached the end of the list ? */
> +                       if (PTR_ERR(led_cdev) == -ENOENT)
> +                               break;

Looks like the above needs an _optional() variant

> +                       return dev_err_probe(dev, PTR_ERR(led_cdev),
> +                                            "Unable to get led #%d", count);
> +               }

...

> +               priv->monochromatics = devm_krealloc(dev, priv->monochromatics,
> +                                       count * sizeof(*priv->monochromatics),
> +                                       GFP_KERNEL);

This needs at minimum to use one of the helpers from overflow.h,
ideally you may implement devm_krealloc_array() as a suitable wrapper
for that.

> +               if (!priv->monochromatics)
> +                       return -ENOMEM;

...

> +       subled = devm_kzalloc(dev, count * sizeof(*subled), GFP_KERNEL);

NIH devm_kcalloc()

> +       if (!subled)
> +               return -ENOMEM;

--
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> This information might be useful for more than only deriving the led's

...

> +                       if (fwnode_property_present(init_data->fwnode, "color"))
> +                               fwnode_property_read_u32(init_data->fwnode, "color",
> +                                                        &led_cdev->color);

Is it already described in the schema?

...

>         unsigned int brightness;
>         unsigned int max_brightness;
> +       unsigned int color;

The above two are exposed via sysfs, do you need to expose a new one
as well? (Just a question, I am not taking any side here, want to hear
explanation of the either choice)

-- 
With Best Regards,
Andy Shevchenko

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

[Jacek Anaszewski]

Hi Jean,

On 9/17/22 10:13, Jean-Jacques Hiblot wrote:
> 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;

We have color_index in struct mc_subled. What would this serve for?

>   	int			 flags;
>   
>   	/* Lower 16 bits reflect status */

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

On 9/18/22 13:25, Jacek Anaszewski wrote:
> Hi Jean,
> 
> On 9/17/22 10:13, Jean-Jacques Hiblot wrote:
>> 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;
> 
> We have color_index in struct mc_subled. What would this serve for?

OK, I missed that you're using it leds-group-multicolor.c.

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

Hi Jean,

General question to this driver: since it seems that it is allowed to
have duplicate LEDs of the same color, then it will be impossible to
distinguish which of the two same colors in multi_index file will refer
to which physical LED. Are you aware of this shortcoming?

Besides that I have two remarks below.

On 9/17/22 10:13, Jean-Jacques Hiblot 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.
> 
> 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..c2610c47a82d 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..61f9e1954fc4
> --- /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/leds.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.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
s/according the/according to the/

> +		 */
> +		actual_led_brightness = DIV_ROUND_CLOSEST(
> +			mono->max_brightness * mc_cdev->subled_info[i].brightness,
> +			mc_cdev->led_cdev.max_brightness);

How about dropping above usage of led_mc_calc_color_components()
helper in favour of doing all required calculations here?
It would be easier for the reader to grasp the idea then.

> +
> +		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;
> +
> +	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", count);
> +		}
> +		count++;
> +
> +		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;
> +		/* configure the LED intensity to its maximum */
> +		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 = dev_fwnode(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.\n",
> +			cdev->name);
> +
> +	ret = led_mcg_set(cdev, cdev->brightness);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to set led value for %s.",
> +				     cdev->name);
> +
> +	for (i = 0; i < count; i++) {
> +		struct led_classdev *led_cdev = priv->monochromatics[i];
> +
> +		/* Make the sysfs of the monochromatic LED read-only */
> +		led_cdev->flags |= LED_SYSFS_DISABLE;
> +	}
> +
> +	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");

-- 
Best regards,
Jacek Anaszewski

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

[Jean-Jacques Hiblot]

On 17/09/2022 10:40, Andy Shevchenko wrote:
> On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> This information might be useful for more than only deriving the led's
> ...
>
>> +                       if (fwnode_property_present(init_data->fwnode, "color"))
>> +                               fwnode_property_read_u32(init_data->fwnode, "color",
>> +                                                        &led_cdev->color);
> Is it already described in the schema?

Hello Andy,


thanks for the reviews on the patch, and sorry for the delay in my 
responses.

Yes. This is already part of the schema.
> ...
>
>>          unsigned int brightness;
>>          unsigned int max_brightness;
>> +       unsigned int color;
> The above two are exposed via sysfs, do you need to expose a new one
> as well? (Just a question, I am not taking any side here, want to hear
> explanation of the either choice)

I didn't really think about it because I didn't need it.

It probably doesn't hurt to expose t in the sysfs. I'll add this in the 
next round.

>

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

[Jean-Jacques Hiblot]

On 17/09/2022 10:37, Andy Shevchenko wrote:
> On Sat, Sep 17, 2022 at 11:14 AM 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.
> ...
>
>> +config LEDS_GRP_MULTICOLOR
>> +       tristate "Multi-color LED grouping support"
>> +       help
>> +         This option enables support for monochrome LEDs that are
>> +         grouped into multicolor LEDs.
> What will be the module name in case of "m" choice?
>
> ...
>
>> +       struct led_mcg_priv *priv =
>> +               container_of(mc_cdev, struct led_mcg_priv, mc_cdev);
> One line?
>
> ...
>
>> +               /*
>> +                * Scale the intensity according the max brightness of the
>> +                * monochromatic LED
> Usually we put a grammar period at the end of sentences in multi-line comments.
>
>> +                */
> ...
>
>> +               actual_led_brightness = DIV_ROUND_CLOSEST(
>> +                       mono->max_brightness * mc_cdev->subled_info[i].brightness,
>> +                       mc_cdev->led_cdev.max_brightness);
> Can you fix an indentation, so it won't leave the line ending by open
> parenthesis? I believe with the help of a temporary variable it can be
> easily achieved.
Could be done but I have no good name for the variable and I think it 
would just make things harder to understand. In that form I think it is 
clear that this is a cross-multiplication.
>
> ...
>
>> +       for (;;) {
>> +               struct led_classdev *led_cdev;
>> +               led_cdev = devm_of_led_get(dev, count);
> Why _of_ variant? Please, make this OF independent since it's
> pretending to cover not only OF-based systems.

This is not OF independent. It could be, but that will wait until 
someone needs it. I don't know much about ACPI and have no hardware to 
test it on.

I'll add the missing  dependency on OF in the Kconfig.

>
>
>> +               if (IS_ERR(led_cdev)) {
>> +                       /* Reached the end of the list ? */
>> +                       if (PTR_ERR(led_cdev) == -ENOENT)
>> +                               break;
> Looks like the above needs an _optional() variant
>
>> +                       return dev_err_probe(dev, PTR_ERR(led_cdev),
>> +                                            "Unable to get led #%d", count);
>> +               }
> ...
>
>> +               priv->monochromatics = devm_krealloc(dev, priv->monochromatics,
>> +                                       count * sizeof(*priv->monochromatics),
>> +                                       GFP_KERNEL);
> This needs at minimum to use one of the helpers from overflow.h,
> ideally you may implement devm_krealloc_array() as a suitable wrapper
> for that.
>
>> +               if (!priv->monochromatics)
>> +                       return -ENOMEM;
> ...
>
>> +       subled = devm_kzalloc(dev, count * sizeof(*subled), GFP_KERNEL);
> NIH devm_kcalloc()
>
>> +       if (!subled)
>> +               return -ENOMEM;
> --
> With Best Regards,
> Andy Shevchenko

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

[Jean-Jacques Hiblot]

On 18/09/2022 16:54, Jacek Anaszewski wrote:
> Hi Jean,
>
> General question to this driver: since it seems that it is allowed to
> have duplicate LEDs of the same color, then it will be impossible to
> distinguish which of the two same colors in multi_index file will refer
> to which physical LED. Are you aware of this shortcoming?

Hi Jacek,

yes I'm aware of this, but I don't think it can be a problem in a real 
environment. There will probably few cases were multiple leds of the 
same color are used and even fewer were those leds need to be controlled 
differently.

>
> Besides that I have two remarks below.
>
> On 9/17/22 10:13, Jean-Jacques Hiblot 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.
>>
>> 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..c2610c47a82d 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..61f9e1954fc4
>> --- /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/leds.h>
>> +#include <linux/led-class-multicolor.h>
>> +#include <linux/math.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.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
> s/according the/according to the/
>
>> +         */
>> +        actual_led_brightness = DIV_ROUND_CLOSEST(
>> +            mono->max_brightness * mc_cdev->subled_info[i].brightness,
>> +            mc_cdev->led_cdev.max_brightness);
>
> How about dropping above usage of led_mc_calc_color_components()
> helper in favour of doing all required calculations here?
> It would be easier for the reader to grasp the idea then.

>
>> +
>> +        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;
>> +
>> +    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", count);
>> +        }
>> +        count++;
>> +
>> +        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;
>> +        /* configure the LED intensity to its maximum */
>> +        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 = dev_fwnode(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.\n",
>> +            cdev->name);
>> +
>> +    ret = led_mcg_set(cdev, cdev->brightness);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret,
>> +                     "failed to set led value for %s.",
>> +                     cdev->name);
>> +
>> +    for (i = 0; i < count; i++) {
>> +        struct led_classdev *led_cdev = priv->monochromatics[i];
>> +
>> +        /* Make the sysfs of the monochromatic LED read-only */
>> +        led_cdev->flags |= LED_SYSFS_DISABLE;
>> +    }
>> +
>> +    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: [RESEND PATCH v3 4/4] leds: Add a multicolor LED driver to group monochromatic LEDs

[Andy Shevchenko]

On Fri, Oct 7, 2022 at 9:34 AM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> On 17/09/2022 10:37, Andy Shevchenko wrote:
> > On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
> > <jjhiblot@traphandler.com> wrote:

...

> >> +               led_cdev = devm_of_led_get(dev, count);
> > Why _of_ variant? Please, make this OF independent since it's
> > pretending to cover not only OF-based systems.
>
> This is not OF independent. It could be, but that will wait until
> someone needs it. I don't know much about ACPI and have no hardware to
> test it on.
>
> I'll add the missing  dependency on OF in the Kconfig.

No, please consider getting rid of OF-centric API usage.

...

I'm not sure why you left a lot of context in the reply without
commenting or replying to it.

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Jacques Hiblot]

On 07/10/2022 10:53, Andy Shevchenko wrote:
> On Fri, Oct 7, 2022 at 9:34 AM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> On 17/09/2022 10:37, Andy Shevchenko wrote:
>>> On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
>>> <jjhiblot@traphandler.com> wrote:
> ...
>
>>>> +               led_cdev = devm_of_led_get(dev, count);
>>> Why _of_ variant? Please, make this OF independent since it's
>>> pretending to cover not only OF-based systems.
>> This is not OF independent. It could be, but that will wait until
>> someone needs it. I don't know much about ACPI and have no hardware to
>> test it on.
>>
>> I'll add the missing  dependency on OF in the Kconfig.
> No, please consider getting rid of OF-centric API usage.

The trouble is that the OF-agnostic API for leds doesn't exist yet and I 
don't really want to add it without any way to test it.

>
> ...
>
> I'm not sure why you left a lot of context in the reply without
> commenting or replying to it.
>

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

[Andy Shevchenko]

On Fri, Oct 7, 2022 at 3:03 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> On 07/10/2022 10:53, Andy Shevchenko wrote:
> > On Fri, Oct 7, 2022 at 9:34 AM Jean-Jacques Hiblot
> > <jjhiblot@traphandler.com> wrote:
> >> On 17/09/2022 10:37, Andy Shevchenko wrote:
> >>> On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
> >>> <jjhiblot@traphandler.com> wrote:

...

> >>>> +               led_cdev = devm_of_led_get(dev, count);
> >>> Why _of_ variant? Please, make this OF independent since it's
> >>> pretending to cover not only OF-based systems.
> >> This is not OF independent. It could be, but that will wait until
> >> someone needs it. I don't know much about ACPI and have no hardware to
> >> test it on.
> >>
> >> I'll add the missing  dependency on OF in the Kconfig.
> > No, please consider getting rid of OF-centric API usage.
>
> The trouble is that the OF-agnostic API for leds doesn't exist yet and I
> don't really want to add it without any way to test it.

Yeah, that might be a problem due to unestablished descriptions
outside DT. Anyway, it seems harmless to call that function when there
is no OF dependency. In such cases it will fail with a deferred probe.

-- 
With Best Regards,
Andy Shevchenko