2022-10-07 15:18:28

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH v4 0/6] 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 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


2022-10-07 15:37:02

by Jean-Jacques Hiblot

[permalink] [raw]
Subject: [PATCH v4 5/6] 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.25.1

2022-10-07 15:38:09

by Jean-Jacques Hiblot

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

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 <[email protected]>
---
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 <[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 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 <[email protected]>");
+MODULE_DESCRIPTION("multi-color LED group driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-group-multicolor");
--
2.25.1

2022-10-07 17:14:39

by Andy Shevchenko

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

On Fri, Oct 7, 2022 at 5:56 PM Jean-Jacques Hiblot
<[email protected]> 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

2022-10-14 09:30:50

by Jean-Jacques Hiblot

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


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;