Re: [RFC PATCH 0/2] Multicolor PWM LED support

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Sven,

On 1/25/22 10:22 AM, sven@svenschwermer.de wrote:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> Hi,
> 
> As previously discussed [1] on the linux-leds list I am missing
> multicolor PWM LED support. In the mean time I have put together a
> working prototype for such a driver. This is my first Linux driver
> so I'm hoping for some feedback. Here are some questions that came up
> while putting this thing together:
> 
>    1. Currently, the max-brightness property is expected as a property to
>       the multi-led node. That seems consistent with the existing
>       multicolor class code, but I'm wondering whether it would make
>       sense to have a max-brigthness for the individual LEDs as well?

For the proper mixed color calculation all sub-leds should have
the same max_brightness as the global max_brightness.

Look at how sub-led intensities are calculated in
led_mc_calc_color_components().

See also [0] and [1].

>    2. The current multi-led node definition calls for a node index which
>       would in turn require the reg property to be set within the node.
>       In this context, that doesn't seem to make sense. Should this
>       requirement be lifted from leds-class-multicolor.yaml?

reg is required for all DT nodes with address unit in the name.
If you skipped the address unit, then reg would be also not required.

>    3. I'm not currently reusing any leds-pwm code because there aren't
>       too many overlaps. Does anyone have suggestions what could be
>       factored out into a common source file?

I think that having a separate pwm driver for multicolor LEDs is a good
idea. leds-pwm.c is old and well tested driver, there's no need to
tinker at it for no vital reason. And there is not much code to share
as you've noticed.

> I would appreciate if anyone would test this code. It runs on my
> i.MX6ULL-based hardware.
> 
> Best regards,
> Sven
> 
> [1]: https://www.spinics.net/lists/linux-leds/msg19988.html
> 
> Sven Schwermer (2):
>    dt-bindings: leds: Add multicolor PWM LED bindings
>    leds: Add PWM multicolor driver
> 
>   .../bindings/leds/leds-pwm-multicolor.yaml    |  73 +++++++
>   drivers/leds/Kconfig                          |   8 +
>   drivers/leds/Makefile                         |   1 +
>   drivers/leds/leds-pwm-multicolor.c            | 184 ++++++++++++++++++
>   4 files changed, 266 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
>   create mode 100644 drivers/leds/leds-pwm-multicolor.c
> 

[0] Documentation/ABI/testing/sysfs-class-led-multicolor
[1] Documentation/leds/leds-class-multicolor.rst

-- 
Best regards,
Jacek Anaszewski