[RESEND,v6,0/6] Add a multicolor LED driver for groups of monochromatic LEDs

Resending the v6 of the series.

I updated the date and kernel version in Documentation/ABI/testing/sysfs-class-led
to Dec 2022 and version 6.2.

Thanks,

JJ


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 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 (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      | 168 ++++++++++++++++++
 include/linux/device.h                        |  13 ++
 include/linux/leds.h                          |   3 +
 8 files changed, 319 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
 create mode 100644 drivers/leds/rgb/leds-group-multicolor.c

On Tue, Dec 27, 2022 at 12:07 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.

...

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

> +

Redundant blank line.

> +               if (IS_ERR(led_cdev))
> +                       return dev_err_probe(dev, PTR_ERR(led_cdev),
> +                                            "Unable to get led #%d", count);
> +
> +               /* Reached the end of the list ?*/
> +               if (!led_cdev)
> +                       break;

> +               count++;

If I understand the flow correctly, this can be moved...

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

...here either as a separate line or a part of the above assignment,
in either case the -1 wouldn't be needed.


> +               max_brightness = max(max_brightness, led_cdev->max_brightness);
> +       }

On Wed, Dec 28, 2022 at 2:41 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Dec 27, 2022 at 12:07 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.
>
> ...
>
> > +       count = 0;
> > +       max_brightness = 0;
> > +       for (;;) {
> > +               struct led_classdev *led_cdev;
> > +
> > +               led_cdev = devm_of_led_get_optional(dev, count);
>
> > +
>
> Redundant blank line.
>
> > +               if (IS_ERR(led_cdev))
> > +                       return dev_err_probe(dev, PTR_ERR(led_cdev),
> > +                                            "Unable to get led #%d", count);
> > +
> > +               /* Reached the end of the list ?*/
> > +               if (!led_cdev)
> > +                       break;
>
> > +               count++;
>
> If I understand the flow correctly, this can be moved...
>
> > +               priv->monochromatics = devm_krealloc_array(dev, priv->monochromatics,
> > +                                       count, sizeof(*priv->monochromatics),

Yes, here the + 1 will be needed instead. But I think it would be
better from the reader perspective, as we try to allocate memory for
existing amount + 1 as it would be clearly written.

> > +                                       GFP_KERNEL);
> > +               if (!priv->monochromatics)
> > +                       return -ENOMEM;
>
> > +               priv->monochromatics[count - 1] = led_cdev;
>
> ...here either as a separate line or a part of the above assignment,
> in either case the -1 wouldn't be needed.
>
>
> > +               max_brightness = max(max_brightness, led_cdev->max_brightness);
> > +       }

P.S. Bjorn's address is wrong.