Re: Clarification regarding multicolor leds

[Luca Weiss <luca@z3ntu.xyz>]

Hi Alex,

On Samstag, 24. Oktober 2020 08:42:38 CEST Alexander Dahl wrote:
> Hello Luca,
> 
> On Sat, Oct 24, 2020 at 12:48:42AM +0200, Luca Weiss wrote:
> > I'm currently experimenting with the qcom lpg[0] which is a driver for the
> > rgb notification led found on e.g. Snapdragon 801 devices (and many
> > more), specifically my example is about the Fairphone 2
> > (msm8974-fairphone-fp2).
> Great to hear someone is interested in mainline support for Fairphone.
> I just bought a used FP2 on ebay. :-)
> 
> > [0]
> > https://lore.kernel.org/lkml/20201021201224.3430546-1-bjorn.andersson@lin
> > aro.org/> 
> > My dts is looking like the following (abbreviated):
> >     [in lpg node]
> >     multi-led {
> >     
> >         color = <LED_COLOR_ID_MULTI>;
> >         function = LED_FUNCTION_STATUS;
> >         ....
> >     
> >     };
> > 
> > I'm comparing this to the PinePhone where the leds are defined as follows:
> >     [in gpio-leds node]
> >     blue {
> >     
> >         function = LED_FUNCTION_INDICATOR;
> >         color = <LED_COLOR_ID_BLUE>;
> >     
> >     };
> >     
> >     green {
> >     
> >         function = LED_FUNCTION_INDICATOR;
> >         color = <LED_COLOR_ID_GREEN>;
> >     
> >     };
> >     
> >     red {
> >     
> >         function = LED_FUNCTION_INDICATOR;
> >         color = <LED_COLOR_ID_RED>;
> >     
> >     };
> > 
> > (sidenote: the LED_FUNCTION_INDICATOR should probably also be
> > LED_FUNCTION_STATUS there; the dts was made before the better descriptions
> > for the defines have been added)
> > 
> > This gets the following directories created in /sys/class/leds/:
> >     blue:indicator
> >     green:indicator
> >     red:indicator
> 
> That's right.  From Linux point of view these behave like three
> independent LEDs.  It's fully up to userspace to handle this.

Exactly, that I understand well.

> 
> > But with the multicolor led on the Fairphone 2 only a directory with the
> > name of "multi-led" gets created which I would have expected to be
> > "multicolor:status" instead.
> 
> Obviously it's named after the node label.  If I read
> Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
> correctly, that's how it is supposed to be named?
> 

That's from the i read from the documentation as well.

How is user space supposed to get function and/or color from the led? I don't 
see it exposed in user space - apart from the directory name (label) in the 
leds-gpio example.

This is what I get in sysfs for the multicolor led with the lpg driver:

    brightness
    device -> ../../../fc4cf000.spmi:pm8941@1:lpg
    max_brightness
    multi_index
    multi_intensity
    power
    subsystem -> ../../../../../../../../../class/leds
    trigger
    uevent


> > What's further confusing me is that include/dt-bindings/leds/common.h
> > states (reformatted for clarity):
> > 
> > /* For multicolor LEDs */
> > #define LED_COLOR_ID_MULTI	8
> > 
> > /* For multicolor LEDs that can do arbitrary color, so this would include
> > RGBW and similar */
> > #define LED_COLOR_ID_RGB	9
> > 
> > It sounds like these comments are the wrong way around as it says that RGB
> > does RGBW & others while MULTI is normal RGB?
> > 

Based on Pavel's reply my RGB LED should be _ID_RGB.
It would be great if this comment would be clarified because honestly still 
don't know what "arbitrary color" should mean; and add an example to the 
_ID_MULTI one that it only should be used for "strange stuff like Blue-UV 
combination".

> > I have also found this commit[1] while browsing through lkml which seems
> > to
> > validate my suspicions that _ID_RGB should be used normally? This commit
> > seems have been applied early September but hasn't been merged in the
> > 5.10 merge window?
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/
> > commit/?h=for-next&id=3d93edc77515c6f51fa9bbbe2185e2ec32bad024
> > 
> > But drivers/leds/led-core.c also states "We want to label LEDs that can
> > produce full range of colors as RGB, not multicolor" - not sure what "full
> > range" means here.
> > 
> > Thanks for reading through my long email and I'd appreciate any
> > clarification on the situation!
> 
> I just read
> https://www.kernel.org/doc/html/latest/leds/leds-class-multicolor.html
> and apart from formatting and inter-doc-link issues due to not used
> markup features, I can understand how it's supposed to be used from
> userspace.
> 

Even in the doc the directory name is "multicolor:status" exposing color & 
function..

Curiously when setting the (deprecated) property "label" to "rgb:status" the 
directory name in sysfs becomes rgb:status - so is it a bug that this doesn't 
happen automatically? Feels like it at least.

I've looked into the code yesterday a bit and found that "struct led_init_data 
*init_data" in led_classdev_register_ext is always passed as NULL for 
multicolor leds from devm_led_classdev_multicolor_register which seems to 
cause this to happen.

> However, multicolor is still quite new, maybe drivers/leds/TODO gives
> some hints?  It seems to me some open issues are already known. ;-)
> 
> HTH & Greets
> Alex

Regards
Luca