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
This information might be useful for more than only deriving the led's
name.
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
---
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
Use the devm_add_action_or_reset() helper.
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
---
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
On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
<[email protected]> 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
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 | 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 <[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);
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("multi-color LED group driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-group-multicolor");
--
2.25.1
On Sat, Sep 17, 2022 at 11:14 AM 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"
> + 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
On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
<[email protected]> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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 <[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);
> + 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 <[email protected]>");
> +MODULE_DESCRIPTION("multi-color LED group driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-group-multicolor");
--
Best regards,
Jacek Anaszewski
On 17/09/2022 10:40, Andy Shevchenko wrote:
> On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
> <[email protected]> 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.
>
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 <[email protected]>
>> ---
>> 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 <[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);
>> + 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 <[email protected]>");
>> +MODULE_DESCRIPTION("multi-color LED group driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:leds-group-multicolor");
>
On 17/09/2022 10:37, Andy Shevchenko wrote:
> On Sat, Sep 17, 2022 at 11:14 AM 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"
>> + 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
On Fri, Oct 7, 2022 at 9:34 AM Jean-Jacques Hiblot
<[email protected]> wrote:
> On 17/09/2022 10:37, Andy Shevchenko wrote:
> > On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
> > <[email protected]> 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
On 07/10/2022 10:53, Andy Shevchenko wrote:
> On Fri, Oct 7, 2022 at 9:34 AM Jean-Jacques Hiblot
> <[email protected]> wrote:
>> On 17/09/2022 10:37, Andy Shevchenko wrote:
>>> On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
>>> <[email protected]> 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.
>
On Fri, Oct 7, 2022 at 3:03 PM Jean-Jacques Hiblot
<[email protected]> wrote:
> On 07/10/2022 10:53, Andy Shevchenko wrote:
> > On Fri, Oct 7, 2022 at 9:34 AM Jean-Jacques Hiblot
> > <[email protected]> wrote:
> >> On 17/09/2022 10:37, Andy Shevchenko wrote:
> >>> On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot
> >>> <[email protected]> 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