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

[PATCH v4 0/6] 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 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 (6):
  devres: provide devm_krealloc_array()
  leds: class: simplify the implementation of devm_of_led_get()
  leds: provide devm_of_led_get_optional()
  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

 Documentation/ABI/testing/sysfs-class-led     |   9 ++
 .../bindings/leds/leds-group-multicolor.yaml  |  64 ++++++++
 drivers/leds/led-class.c                      |  65 ++++++--
 drivers/leds/rgb/Kconfig                      |  10 ++
 drivers/leds/rgb/Makefile                     |   1 +
 drivers/leds/rgb/leds-group-multicolor.c      | 152 ++++++++++++++++++
 include/linux/device.h                        |  13 ++
 include/linux/leds.h                          |   3 +
 8 files changed, 303 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 v4 1/6] devres: provide devm_krealloc_array()

[Jean-Jacques Hiblot]

Implement the managed variant of krealloc_array().
This internally uses devm_krealloc() and as such is usable with all memory
allocated by devm_kmalloc() (or devres functions using it implicitly like
devm_kmemdup(), devm_kstrdup() etc.)

Managed realloc'ed chunks can be manually released with devm_kfree().

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 include/linux/device.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 424b55df0272..3b472df6c6cd 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -223,6 +223,19 @@ static inline void *devm_kcalloc(struct device *dev,
 {
 	return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
 }
+static inline void *devm_krealloc_array(struct device *dev,
+					void *p,
+					size_t new_n,
+					size_t new_size,
+					gfp_t flags)
+{
+	size_t bytes;
+
+	if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
+		return NULL;
+
+	return devm_krealloc(dev, p, bytes, flags);
+}
 void devm_kfree(struct device *dev, const void *p);
 char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
 const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp);
-- 
2.25.1

[PATCH v4 2/6] 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v4 3/6] leds: provide devm_of_led_get_optional()

[Jean-Jacques Hiblot]

This version of devm_of_led_get() doesn't fail if a LED is not found.
Instead it returns a NULL pointer.

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

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 2c0d979d0c8a..2fea79a2300d 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -295,6 +295,31 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_of_led_get);
 
+/**
+ * devm_of_led_get_optional - Resource-managed request of an optional LED device
+ * @dev:	LED consumer
+ * @index:	index of the LED to obtain in the consumer
+ *
+ * The device node of the device is parse to find the request LED device.
+ * The LED device returned from this function is automatically released
+ * on driver detach.
+ *
+ * @return a pointer to a LED device, ERR_PTR(errno) on failure and NULL if the
+ * led was not found.
+ */
+struct led_classdev *__must_check devm_of_led_get_optional(struct device *dev,
+							int index)
+{
+	struct led_classdev *led;
+
+	led = devm_of_led_get(dev, index);
+	if (IS_ERR(led) && PTR_ERR(led) == -ENOENT)
+		return NULL;
+
+	return led;
+}
+EXPORT_SYMBOL_GPL(devm_of_led_get_optional);
+
 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..41df18f42d00 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -215,6 +215,8 @@ extern struct led_classdev *of_led_get(struct device_node *np, int index);
 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_of_led_get_optional(struct device *dev,
+						  int index);
 
 /**
  * led_blink_set - set blinking with software fallback
-- 
2.25.1

[PATCH v4 4/6] 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.
And since we have this information, we can expose it in the sysfs.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 Documentation/ABI/testing/sysfs-class-led |  9 +++++++++
 drivers/leds/led-class.c                  | 20 ++++++++++++++++++++
 include/linux/leds.h                      |  1 +
 3 files changed, 30 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 2e24ac3bd7ef..e3ae9111b70d 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -59,6 +59,15 @@ Description:
 		brightness. Reading this file when no hw brightness change
 		event has happened will return an ENODATA error.
 
+What:		/sys/class/leds/<led>/color
+Date:		October 2022
+KernelVersion:	6.1
+Description:
+		Color of the led.
+
+		This is a read-only file. Reading this file returns the color
+		of the led as a string (ex: "red", "green").
+
 What:		/sys/class/leds/<led>/trigger
 Date:		March 2006
 KernelVersion:	2.6.17
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 2fea79a2300d..fcc528f82f92 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,18 @@ static ssize_t max_brightness_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(max_brightness);
 
+static ssize_t color_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const char *color_text = "invalid";
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	if (led_cdev->color < LED_COLOR_ID_MAX)
+		color_text = led_colors[led_cdev->color];
+	return sprintf(buf, "%s\n", color_text);
+}
+static DEVICE_ATTR_RO(color);
+
 #ifdef CONFIG_LEDS_TRIGGERS
 static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write, 0);
 static struct bin_attribute *led_trigger_bin_attrs[] = {
@@ -88,6 +100,7 @@ static const struct attribute_group led_trigger_group = {
 static struct attribute *led_class_attrs[] = {
 	&dev_attr_brightness.attr,
 	&dev_attr_max_brightness.attr,
+	&dev_attr_color.attr,
 	NULL,
 };
 
@@ -375,6 +388,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;
@@ -384,6 +401,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 41df18f42d00..9d38cb90331d 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 v4 5/6] 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

[PATCH v4 6/6] 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                 |  10 ++
 drivers/leds/rgb/Makefile                |   1 +
 drivers/leds/rgb/leds-group-multicolor.c | 152 +++++++++++++++++++++++
 3 files changed, 163 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..5ba9b843464b 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -2,6 +2,16 @@
 
 if LEDS_CLASS_MULTICOLOR
 
+config LEDS_GRP_MULTICOLOR
+	tristate "Multi-color LED grouping support"
+	depends on OF
+	help
+	  This option enables support for monochrome LEDs that are
+	  grouped into multicolor LEDs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-grp-multicolor.
+
 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..50b812c16d00
--- /dev/null
+++ b/drivers/leds/rgb/leds-group-multicolor.c
@@ -0,0 +1,152 @@
+// 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);
+	const unsigned int mc_max_brightness = mc_cdev->led_cdev.max_brightness;
+	int i;
+
+	for (i = 0; i < mc_cdev->num_colors; i++) {
+		struct led_classdev *mono = priv->monochromatics[i];
+		const unsigned int mono_max = mono->max_brightness;
+		unsigned int rel_intensity = mc_cdev->subled_info[i].intensity;
+		int b;
+
+		/*
+		 * Scale the brightness according to relative intensity of the
+		 * color AND the max brightness of the monochromatic LED.
+		 */
+		b = DIV_ROUND_CLOSEST(brightness * rel_intensity * mono_max,
+			mc_max_brightness * mc_max_brightness);
+
+		led_set_brightness(mono, b);
+	}
+
+	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_optional(dev, count);
+		if (!led_cdev)
+			/* Reached the end of the list */
+			break;
+
+		if (IS_ERR(led_cdev))
+			return dev_err_probe(dev, PTR_ERR(led_cdev),
+					     "Unable to get led #%d", count);
+		count++;
+
+		priv->monochromatics = devm_krealloc_array(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_kcalloc(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: [PATCH v4 1/6] devres: provide devm_krealloc_array()

[Andy Shevchenko]

+Cc: Bart who learnt realloc() specifics hard way

Thanks for doing this!

On Fri, Oct 7, 2022 at 6:13 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> Implement the managed variant of krealloc_array().
> This internally uses devm_krealloc() and as such is usable with all memory
> allocated by devm_kmalloc() (or devres functions using it implicitly like
> devm_kmemdup(), devm_kstrdup() etc.)

Missed grammatical period at the end.

> Managed realloc'ed chunks can be manually released with devm_kfree().

...

>  {
>         return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
>  }

Missed blank line?

> +static inline void *devm_krealloc_array(struct device *dev,
> +                                       void *p,
> +                                       size_t new_n,
> +                                       size_t new_size,
> +                                       gfp_t flags)
> +{
> +       size_t bytes;
> +
> +       if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
> +               return NULL;

I'm not sure it is what we want, but I have read the man realloc and found this:

      ... reallocarray() fails safely in the case where the multiplication
      would overflow.  If such an overflow occurs, reallocarray() returns NULL,
      sets  errno  to  ENOMEM,  and leaves the original block of memory
      unchanged.

So, perhaps you can add that this behaviour mimics reallocarray()?

> +       return devm_krealloc(dev, p, bytes, flags);
> +}

...

All comments are minor, feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/6] leds: provide devm_of_led_get_optional()

[Andy Shevchenko]

On Fri, Oct 7, 2022 at 5:56 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> This version of devm_of_led_get() doesn't fail if a LED is not found.
> Instead it returns a NULL pointer.

Yep, thanks!

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

> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>  drivers/leds/led-class.c | 25 +++++++++++++++++++++++++
>  include/linux/leds.h     |  2 ++
>  2 files changed, 27 insertions(+)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 2c0d979d0c8a..2fea79a2300d 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -295,6 +295,31 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_of_led_get);
>
> +/**
> + * devm_of_led_get_optional - Resource-managed request of an optional LED device

I'm not sure we need to keep "_of" here in the name, but it's not
harmful anyway.

> + * @dev:       LED consumer
> + * @index:     index of the LED to obtain in the consumer
> + *
> + * The device node of the device is parse to find the request LED device.

I guess you copy'n'pasted this, but this has some spelling issues, I
would change

parse --> parsed
request --> requested

> + * The LED device returned from this function is automatically released
> + * on driver detach.
> + *
> + * @return a pointer to a LED device, ERR_PTR(errno) on failure and NULL if the
> + * led was not found.
> + */
> +struct led_classdev *__must_check devm_of_led_get_optional(struct device *dev,
> +                                                       int index)
> +{
> +       struct led_classdev *led;
> +
> +       led = devm_of_led_get(dev, index);
> +       if (IS_ERR(led) && PTR_ERR(led) == -ENOENT)
> +               return NULL;
> +
> +       return led;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_led_get_optional);
> +
>  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..41df18f42d00 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -215,6 +215,8 @@ extern struct led_classdev *of_led_get(struct device_node *np, int index);
>  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_of_led_get_optional(struct device *dev,
> +                                                 int index);
>
>  /**
>   * led_blink_set - set blinking with software fallback
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/6] leds: class: store the color index in struct led_classdev

[Andy Shevchenko]

On Fri, Oct 7, 2022 at 6:03 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> This information might be useful for more than only deriving the led's
> name.
> And since we have this information, we can expose it in the sysfs.

Not sure why you haven't continued a new sentence on the previous line.

...

> +static ssize_t color_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       const char *color_text = "invalid";
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +       if (led_cdev->color < LED_COLOR_ID_MAX)
> +               color_text = led_colors[led_cdev->color];

> +       return sprintf(buf, "%s\n", color_text);

According to the Documentation you must use sysfs_emit() here. It
might be good to update existing code as well (as a separate change).

> +}
> +static DEVICE_ATTR_RO(color);


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 7, 2022 at 5:56 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.

...

> +config LEDS_GRP_MULTICOLOR
> +       tristate "Multi-color LED grouping support"

> +       depends on OF

But there is no compilation requirement for that.
If you want to tell that this is functional requirement, you may use

    depends on COMPILE_TEST || OF

pattern

...

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

Perhaps keep it before previous line?

> +       count = 0;
> +       max_brightness = 0;
> +       for (;;) {
> +               struct led_classdev *led_cdev;
> +
> +               led_cdev = devm_of_led_get_optional(dev, count);

> +               if (!led_cdev)

> +                       /* Reached the end of the list */
> +                       break;

This will require {} according to the style guide. Maybe move the
comment on top of the if (!led_cdev) check?

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

I would check for an error first, this is a common pattern in the Linux kernel.

> +               count++;
> +
> +               priv->monochromatics = devm_krealloc_array(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_kcalloc(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;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Jacques Hiblot]

On 07/10/2022 18:31, Andy Shevchenko wrote:
> But there is no compilation requirement for that.
> If you want to tell that this is functional requirement, you may use
>
>      depends on COMPILE_TEST || OF
>
> pattern
Yes. that is what I meant by adding depends on OF. Thanks.
>
> ...
>
>> +       struct device *dev = &pdev->dev;
>> +       struct led_init_data init_data = {};
>> +       struct led_classdev *cdev;
>> +       struct mc_subled *subled;
>> +       struct led_mcg_priv *priv;

Re: [PATCH v4 1/6] devres: provide devm_krealloc_array()

[Jean-Jacques Hiblot]

On 07/10/2022 18:18, Andy Shevchenko wrote:
>>   {
>>          return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
>>   }
> Missed blank line?

There is no blank line after the definitions of devm_kzalloc(), 
devm_kmalloc_array() and devm_kcalloc() defined just above.

>
>> +static inline void *devm_krealloc_array(struct device *dev,
>> +                                       void *p,
>> +                                       size_t new_n,
>> +                                       size_t new_size,
>> +                                       gfp_t flags)
>> +{
>> +       size_t bytes;
>> +
>> +       if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
>> +               return NULL;
> I'm not sure it is what we want, but I have read the man realloc and found this:
>
>        ... reallocarray() fails safely in the case where the multiplication
>        would overflow.  If such an overflow occurs, reallocarray() returns NULL,
>        sets  errno  to  ENOMEM,  and leaves the original block of memory
>        unchanged.
>
> So, perhaps you can add that this behaviour mimics reallocarray()?

except for the errno part, that is what is does. I don't think we should 
use ERR_PTR in this case as the other allocation functions don't use it.

>
>> +       return devm_krealloc(dev, p, bytes, flags);
>> +}
> ...
>
> All comments are minor, feel free to add
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>