mbox series

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

Message ID 20230328161541.599463-1-jjhiblot@traphandler.com
Headers show
Series Add a multicolor LED driver for groups of monochromatic LEDs | expand

Message

Jean-Jacques Hiblot March 28, 2023, 4:15 p.m. UTC 
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 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 (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                      |  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, 318 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
 create mode 100644 drivers/leds/rgb/leds-group-multicolor.c

Comments

Andy Shevchenko March 28, 2023, 5:15 p.m. UTC | #1 
On Tue, Mar 28, 2023 at 7:15 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.

...

> +Date:          March 2023
> +KernelVersion: 6.3

Outdated version.

...

> +               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").

There are no strict rules about colour and I don't think it's a good
idea. Why in such a case is it different to label? My proposal here at
least documenting that the colour must follow one of the existing
naming standards (like RGB in hex, HTML, or name in accordance with
chosen standard).

Yet, it won't technically prevent abusing that, but at least will show
the intention and allow pointing out to the bugs or develop user space
tooling based on existing parsers (if any).
Jean-Jacques Hiblot April 6, 2023, 6:52 a.m. UTC | #2 
On 28/03/2023 19:15, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 7:15 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.
> 
> ...
> 
>> +Date:          March 2023
>> +KernelVersion: 6.3
> 
> Outdated version.
> 
> ...
> 
>> +               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").
> 
> There are no strict rules about colour and I don't think it's a good
> idea. Why in such a case is it different to label? My proposal here at
> least documenting that the colour must follow one of the existing
> naming standards (like RGB in hex, HTML, or name in accordance with
> chosen standard).
Actually the colors are defined in an array: led_colors (led-core.c: 88)
So the color is one of the following: white, red, reen, blue, amber, 
violet, yellow, ir, multicolor, rgb

There is mention in the TODO file of changing the way RGB leds are 
handled and the RGB leds would probably show the hex RGB values here.

> 
> Yet, it won't technically prevent abusing that, but at least will show
> the intention and allow pointing out to the bugs or develop user space
> tooling based on existing parsers (if any).
>
Jean-Jacques Hiblot April 28, 2023, 12:39 p.m. UTC | #3 
On 28/03/2023 18:15, Jean-Jacques Hiblot wrote:
> 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 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.
> 
Hi Lee,

Are there any pending items regarding this series ?
If not, can you consider merging it ?


Thanks,
JJ
Pavel Machek April 28, 2023, 12:50 p.m. UTC | #4 
On Fri 2023-04-28 14:39:20, Jean-Jacques Hiblot wrote:
> On 28/03/2023 18:15, Jean-Jacques Hiblot wrote:
> > 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 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.
> > 
> Hi Lee,
> 
> Are there any pending items regarding this series ?
> If not, can you consider merging it ?

We should not be merging it in the middle of merge window... right?

BR,
								Pavel