2023-06-16 10:18:08

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH v9 0/5] Add a multicolor LED driver for groups of monochromatic LEDs

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 v8->v9:
- rebased on top of lee-leds/for-leds-next
- updated kernel version and date for /sys/class/leds/<led>/color in
Documentation/ABI/testing/sysfs-class-led
- dropped patch "leds: class: simplify the implementation of
devm_of_led_get()" because __devm_led_get() is now used by
devm_led_get()

changes v7->v8:
- consistently use "LEDs group multicolor" throughout the code.
- rename some variables with more explicit names.
- improve comments.
- use the 100-characters per line limit.

changes v6->v7:
- in led_mcg_probe() increment the counter at the end of the loop for
clarity.

changes v5->v6:
- restore sysfs access to the leds when the device is removed

changes v4->v5:
- Use "depends on COMPILE_TEST || OF" in Kconfig to indicate that OF
is a functional requirement, not just a requirement for the
compilation.
- in led_mcg_probe() check if devm_of_led_get_optional() returns an
error before testing for the end of the list.
- use sysfs_emit() instead of sprintf() in color_show().
- some grammar fixes in the comments and the commit logs.

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 (5):
devres: provide devm_krealloc_array()
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 | 45 +++++
drivers/leds/rgb/Kconfig | 13 ++
drivers/leds/rgb/Makefile | 1 +
drivers/leds/rgb/leds-group-multicolor.c | 164 ++++++++++++++++++
include/linux/device.h | 13 ++
include/linux/leds.h | 3 +
8 files changed, 312 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
create mode 100644 drivers/leds/rgb/leds-group-multicolor.c

--
2.34.1



2023-06-16 10:24:18

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH v9 2/5] leds: provide devm_of_led_get_optional()

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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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 6dae56b914fe..eb1a8494dc5b 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -402,6 +402,31 @@ void led_remove_lookup(struct led_lookup_data *led_lookup)
}
EXPORT_SYMBOL_GPL(led_remove_lookup);

+/**
+ * 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 parsed to find the requested 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 50b2f8f153fb..95311c70d95c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -270,6 +270,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.34.1


2023-06-16 10:27:22

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH v9 5/5] leds: Add a multicolor LED driver to group monochromatic LEDs

Grouping multiple monochrome LEDs into a multicolor LED device has a few
benefits over handling the group in user-space:
- The state of the LEDs relative to each other is consistent. In other
words, if 2 threads competes to set the LED to green and red, the
end-result cannot be black or yellow.
- The multicolor LED as a whole can be driven through the sysfs LED
interface.

Signed-off-by: Jean-Jacques Hiblot <[email protected]>
---
drivers/leds/rgb/Kconfig | 13 ++
drivers/leds/rgb/Makefile | 1 +
drivers/leds/rgb/leds-group-multicolor.c | 164 +++++++++++++++++++++++
3 files changed, 178 insertions(+)
create mode 100644 drivers/leds/rgb/leds-group-multicolor.c

diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index 360c8679c6e2..fae9aeaf2ce1 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -2,6 +2,19 @@

if LEDS_CLASS_MULTICOLOR

+config LEDS_GROUP_MULTICOLOR
+ tristate "LEDs group multi-color support"
+ depends on OF || COMPILE_TEST
+ help
+ This option enables support for monochrome LEDs that are
+ grouped into multicolor LEDs.
+ This useful in the case where LEDs of different colors are
+ physically grouped in a single multi-color LED and driven
+ by a controller that doesn't have multi-color support.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-group-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 8c01daf63f61..c11cc56384e7 100644
--- a/drivers/leds/rgb/Makefile
+++ b/drivers/leds/rgb/Makefile
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0

+obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o
obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o
obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o
obj-$(CONFIG_LEDS_MT6370_RGB) += leds-mt6370-rgb.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..27c80c84de88
--- /dev/null
+++ b/drivers/leds/rgb/leds-group-multicolor.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * multi-color LED built with monochromatic LED devices
+ *
+ * This driver groups several monochromatic LED devices in a single multicolor LED device.
+ * Compared to handling this grouping in the user-space, the benefits are:
+ * - The state of the monochromatic LED relative to each other is always consistent.
+ * - the sysfs interface of the LEDs can be used for the group as a whole.
+ *
+ * Copyright 2023 Jean-Jacques Hiblot <[email protected]>
+ */
+
+#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 leds_multicolor {
+ 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 leds_multicolor *priv = container_of(mc_cdev, struct leds_multicolor, mc_cdev);
+ const unsigned int group_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_brightness = mono->max_brightness;
+ unsigned int intensity = mc_cdev->subled_info[i].intensity;
+ int mono_brightness;
+
+ /*
+ * Scale the brightness according to relative intensity of the
+ * color AND the max brightness of the monochromatic LED.
+ */
+ mono_brightness = DIV_ROUND_CLOSEST(brightness * intensity * mono_max_brightness,
+ group_max_brightness * group_max_brightness);
+
+ led_set_brightness(mono, mono_brightness);
+ }
+
+ return 0;
+}
+
+static void restore_sysfs_access(void *data)
+{
+ struct led_classdev *led_cdev = data;
+
+ mutex_lock(&led_cdev->led_access);
+ led_sysfs_enable(led_cdev);
+ mutex_unlock(&led_cdev->led_access);
+}
+
+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 leds_multicolor *priv;
+ unsigned int max_brightness = 0;
+ int i, ret, count = 0;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ for (;;) {
+ struct led_classdev *led_cdev;
+
+ led_cdev = devm_of_led_get_optional(dev, count);
+ if (IS_ERR(led_cdev))
+ return dev_err_probe(dev, PTR_ERR(led_cdev), "Unable to get LED #%d",
+ count);
+ if (!led_cdev)
+ break;
+
+ priv->monochromatics = devm_krealloc_array(dev, priv->monochromatics,
+ count + 1, sizeof(*priv->monochromatics),
+ GFP_KERNEL);
+ if (!priv->monochromatics)
+ return -ENOMEM;
+
+ priv->monochromatics[count] = led_cdev;
+
+ max_brightness = max(max_brightness, led_cdev->max_brightness);
+
+ count++;
+ }
+
+ 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;
+ }
+
+ /* Initialise 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 */
+ mutex_lock(&led_cdev->led_access);
+ led_sysfs_disable(led_cdev);
+ mutex_unlock(&led_cdev->led_access);
+
+ /* Restore sysfs access when the multicolor LED is released */
+ devm_add_action_or_reset(dev, restore_sysfs_access, led_cdev);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id of_led_group_multicolor_match[] = {
+ { .compatible = "leds-group-multicolor" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_led_group_multicolor_match);
+
+static struct platform_driver led_group_multicolor_driver = {
+ .probe = led_mcg_probe,
+ .driver = {
+ .name = "leds_group_multicolor",
+ .of_match_table = of_led_group_multicolor_match,
+ }
+};
+module_platform_driver(led_group_multicolor_driver);
+
+MODULE_AUTHOR("Jean-Jacques Hiblot <[email protected]>");
+MODULE_DESCRIPTION("LEDs group multicolor driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-group-multicolor");
--
2.34.1


2023-06-16 10:27:50

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH v9 3/5] leds: class: store the color index in struct led_classdev

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 <[email protected]>
---
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..1509e71fcde1 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: June 2023
+KernelVersion: 6.5
+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", "multicolor").
+
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 eb1a8494dc5b..6cca21b227dd 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -76,6 +76,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 sysfs_emit(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[] = {
@@ -90,6 +102,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,
};

@@ -482,6 +495,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;
@@ -491,6 +508,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 95311c70d95c..487d00dac4de 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -100,6 +100,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.34.1


2023-06-16 11:22:58

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH v9 4/5] dt-bindings: leds: Add binding for a multicolor group of LEDs

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

Signed-off-by: Jean-Jacques Hiblot <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../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 <[email protected]>
+
+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.34.1


2023-06-16 13:04:32

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH v9 1/5] devres: provide devm_krealloc_array()

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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
include/linux/device.h | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 472dd24d4823..e45dc78068b7 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.34.1


2023-06-21 20:17:18

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v9 5/5] leds: Add a multicolor LED driver to group monochromatic LEDs

On Fri, 16 Jun 2023, Jean-Jacques Hiblot wrote:

> Grouping multiple monochrome LEDs into a multicolor LED device has a few
> benefits over handling the group in user-space:
> - The state of the LEDs relative to each other is consistent. In other
> words, if 2 threads competes to set the LED to green and red, the
> end-result cannot be black or yellow.
> - The multicolor LED as a whole can be driven through the sysfs LED
> interface.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> ---
> drivers/leds/rgb/Kconfig | 13 ++
> drivers/leds/rgb/Makefile | 1 +
> drivers/leds/rgb/leds-group-multicolor.c | 164 +++++++++++++++++++++++
> 3 files changed, 178 insertions(+)
> create mode 100644 drivers/leds/rgb/leds-group-multicolor.c

Sorry to do this too you, but there are too many nits that I can't pass by.

> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> index 360c8679c6e2..fae9aeaf2ce1 100644
> --- a/drivers/leds/rgb/Kconfig
> +++ b/drivers/leds/rgb/Kconfig
> @@ -2,6 +2,19 @@
>
> if LEDS_CLASS_MULTICOLOR
>
> +config LEDS_GROUP_MULTICOLOR
> + tristate "LEDs group multi-color support"
> + depends on OF || COMPILE_TEST
> + help
> + This option enables support for monochrome LEDs that are
> + grouped into multicolor LEDs.
> + This useful in the case where LEDs of different colors are

"This is useful". Or better yet, take it up to the previous line and
continue the sentence with "which is".

> + physically grouped in a single multi-color LED and driven
> + by a controller that doesn't have multi-color support.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called leds-group-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 8c01daf63f61..c11cc56384e7 100644
> --- a/drivers/leds/rgb/Makefile
> +++ b/drivers/leds/rgb/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
>
> +obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o
> obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o
> obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o
> obj-$(CONFIG_LEDS_MT6370_RGB) += leds-mt6370-rgb.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..27c80c84de88
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-group-multicolor.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * multi-color LED built with monochromatic LED devices

"Multi-color"

> + * This driver groups several monochromatic LED devices in a single multicolor LED device.
> + * Compared to handling this grouping in the user-space, the benefits are:

Drop the "the" in "the user-space".

'\n' here.

> + * - The state of the monochromatic LED relative to each other is always consistent.
> + * - the sysfs interface of the LEDs can be used for the group as a whole.

"The"

"The LED's sysfs interface ..."

> + *
> + * Copyright 2023 Jean-Jacques Hiblot <[email protected]>
> + */
> +
> +#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 leds_multicolor {
> + struct led_classdev_mc mc_cdev;
> + struct led_classdev **monochromatics;
> +};
> +
> +static int led_mcg_set(struct led_classdev *cdev, enum led_brightness brightness)

"group multi-color" = gmc.

> +{
> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> + struct leds_multicolor *priv = container_of(mc_cdev, struct leds_multicolor, mc_cdev);
> + const unsigned int group_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_brightness = mono->max_brightness;
> + unsigned int intensity = mc_cdev->subled_info[i].intensity;
> + int mono_brightness;
> +
> + /*
> + * Scale the brightness according to relative intensity of the
> + * color AND the max brightness of the monochromatic LED.
> + */
> + mono_brightness = DIV_ROUND_CLOSEST(brightness * intensity * mono_max_brightness,
> + group_max_brightness * group_max_brightness);
> +
> + led_set_brightness(mono, mono_brightness);
> + }
> +
> + return 0;
> +}
> +
> +static void restore_sysfs_access(void *data)

..._write_access?

> +{
> + struct led_classdev *led_cdev = data;
> +
> + mutex_lock(&led_cdev->led_access);
> + led_sysfs_enable(led_cdev);
> + mutex_unlock(&led_cdev->led_access);
> +}
> +
> +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 leds_multicolor *priv;
> + unsigned int max_brightness = 0;
> + int i, ret, count = 0;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + for (;;) {
> + struct led_classdev *led_cdev;
> +
> + led_cdev = devm_of_led_get_optional(dev, count);
> + if (IS_ERR(led_cdev))
> + return dev_err_probe(dev, PTR_ERR(led_cdev), "Unable to get LED #%d",
> + count);
> + if (!led_cdev)
> + break;
> +
> + priv->monochromatics = devm_krealloc_array(dev, priv->monochromatics,
> + count + 1, sizeof(*priv->monochromatics),
> + GFP_KERNEL);
> + if (!priv->monochromatics)
> + return -ENOMEM;
> +
> + priv->monochromatics[count] = led_cdev;
> +
> + max_brightness = max(max_brightness, led_cdev->max_brightness);
> +
> + count++;
> + }
> +
> + 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;
> + }
> +
> + /* Initialise 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 */

"Make monochromatic LED sysfs interface read-only"

> + mutex_lock(&led_cdev->led_access);
> + led_sysfs_disable(led_cdev);
> + mutex_unlock(&led_cdev->led_access);
> +
> + /* Restore sysfs access when the multicolor LED is released */

Out of interest, why are we doing this?

> + devm_add_action_or_reset(dev, restore_sysfs_access, led_cdev);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id of_led_group_multicolor_match[] = {

"leds"

> + { .compatible = "leds-group-multicolor" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_led_group_multicolor_match);
> +
> +static struct platform_driver led_group_multicolor_driver = {

"leds"

> + .probe = led_mcg_probe,
> + .driver = {
> + .name = "leds_group_multicolor",
> + .of_match_table = of_led_group_multicolor_match,
> + }
> +};
> +module_platform_driver(led_group_multicolor_driver);
> +
> +MODULE_AUTHOR("Jean-Jacques Hiblot <[email protected]>");
> +MODULE_DESCRIPTION("LEDs group multicolor driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-group-multicolor");
> --
> 2.34.1
>

--
Lee Jones [李琼斯]

2023-06-22 18:37:27

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: Re: [PATCH v9 5/5] leds: Add a multicolor LED driver to group monochromatic LEDs



On 21/06/2023 21:33, Lee Jones wrote:
> On Fri, 16 Jun 2023, Jean-Jacques Hiblot wrote:
>
>> create mode 100644 drivers/leds/rgb/leds-group-multicolor.c
>
> Sorry to do this too you, but there are too many nits that I can't pass by.
That's alright. I'll fix them in the next (hopefully last) round
>
>> + /* Restore sysfs access when the multicolor LED is released */
>

> Out of interest, why are we doing this?
To prevent the user to play with the individual LEDs of the group, their
sysfs interface is disabled (read-only). The interface must be
re-enabled when the group is destroyed.
>
>> + devm_add_action_or_reset(dev, restore_sysfs_access, led_cdev);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id of_led_group_multicolor_match[] = {
>
> "leds"
>
>> + { .compatible = "leds-group-multicolor" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, of_led_group_multicolor_match);
>> +
>> +static struct platform_driver led_group_multicolor_driver = {
>
> "leds"
>
>> + .probe = led_mcg_probe,
>> + .driver = {
>> + .name = "leds_group_multicolor",
>> + .of_match_table = of_led_group_multicolor_match,
>> + }
>> +};
>> +module_platform_driver(led_group_multicolor_driver);
>> +
>> +MODULE_AUTHOR("Jean-Jacques Hiblot <[email protected]>");
>> +MODULE_DESCRIPTION("LEDs group multicolor driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:leds-group-multicolor");
>> --
>> 2.34.1
>>
>

2023-06-26 16:29:42

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v9 5/5] leds: Add a multicolor LED driver to group monochromatic LEDs

On Thu, 22 Jun 2023, Jean-Jacques Hiblot wrote:

>
>
> On 21/06/2023 21:33, Lee Jones wrote:
> > On Fri, 16 Jun 2023, Jean-Jacques Hiblot wrote:
> >
> > > create mode 100644 drivers/leds/rgb/leds-group-multicolor.c
> >
> > Sorry to do this too you, but there are too many nits that I can't pass by.
> That's alright. I'll fix them in the next (hopefully last) round
> >
> > > + /* Restore sysfs access when the multicolor LED is released */
> >
>
> > Out of interest, why are we doing this?
> To prevent the user to play with the individual LEDs of the group, their
> sysfs interface is disabled (read-only). The interface must be re-enabled
> when the group is destroyed.

Please make that very clear in the code.

--
Lee Jones [李琼斯]