Clarification regarding multicolor leds

Clarification regarding multicolor leds

[Luca Weiss]

Hi,
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).

[0] https://lore.kernel.org/lkml/20201021201224.3430546-1-bjorn.andersson@linaro.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

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.


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?

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!
Regards,
Luca

Re: Clarification regarding multicolor leds

[Alexander Dahl]

[-- Attachment #1: Type: text/plain, Size: 3840 bytes --]

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@linaro.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.

> 
> 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?

> 
> 
> 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?
> 
> 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.

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

-- 
/"\ ASCII RIBBON | »With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.«
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Clarification regarding multicolor leds

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

Hi!

> 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. :-)

You may want to cc phone-devel@vger.kernel.org.

And yes, I'd like to see usable phone running mainline. I currently
have Droid 4, which is close.

> > [0] https://lore.kernel.org/lkml/20201021201224.3430546-1-bjorn.andersson@linaro.org/
> > 
> > My dts is looking like the following (abbreviated):
> > 
> >     [in lpg node]
> >     multi-led {
> >         color = <LED_COLOR_ID_MULTI>;
> >         function = LED_FUNCTION_STATUS;
> >         ....
> >     };

Make it _ID_RGB.

> > (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.

Yes. Eventually we'll want to switch Pinephone to newer interface.

> > 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?

Multi is for strange stuff like Blue-UV combination. Avoid.

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Clarification regarding multicolor leds

[Luca Weiss]

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

Re: Clarification regarding multicolor leds

[Dan Murphy]

Luca

On 10/24/20 4:16 AM, Luca Weiss wrote:
> 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

If you refer to the ABI doc you see that by reading the multi_index file 
gives you the colors contained within the file.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-led-multicolor

Dan

Re: Clarification regarding multicolor leds

[Jacek Anaszewski]

Hi Luca,

On 10/24/20 12:48 AM, Luca Weiss wrote:
> Hi,
> 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).
> 
> [0] https://lore.kernel.org/lkml/20201021201224.3430546-1-bjorn.andersson@linaro.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
> 
> 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.

This is because the driver from patch set [0] does not use *ext()
multicolor registration API, but follows old-fashion LED name
initialization via 'name' property of struct led_classdev, which is
initialized to DT 'label' value or DT node name if the former is absent.

-- 
Best regards,
Jacek Anaszewski

Re: Clarification regarding multicolor leds

[Luca Weiss]

Hi Jacek

On Samstag, 24. Oktober 2020 21:34:38 CET Jacek Anaszewski wrote:
> Hi Luca,
> 
> On 10/24/20 12:48 AM, Luca Weiss wrote:
> > Hi,
> > 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).
> > 
> > [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
> > 
> > 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.
> 
> This is because the driver from patch set [0] does not use *ext()
> multicolor registration API, but follows old-fashion LED name
> initialization via 'name' property of struct led_classdev, which is
> initialized to DT 'label' value or DT node name if the former is absent.

Right, I remember now having done something similar with my sgm3140 flash led 
driver half a year ago, thanks for the reminder!
With the qcom-lpg driver using the _ext function it works as expected.

@Bjorn I've prepared a diff for your patches at
https://public.z3ntu.xyz/tmp/lpg.diff where 
1. LED_COLOR_ID_MULTI is replaced with LED_COLOR_ID_RGB based on these emails
2. The example in the documentation uses LED_FUNCTION_STATUS (see description 
for the constant in include/dt-bindings/leds/common.h)
3. The _ext registration api now gets used, see e.g. commit 5b4b723c483f

Thanks for your help, really appreciate it!
Regards
Luca

Re: Clarification regarding multicolor leds

[Bjorn Andersson]

On Sat 24 Oct 14:34 CDT 2020, Jacek Anaszewski wrote:

> Hi Luca,
> 
> On 10/24/20 12:48 AM, Luca Weiss wrote:
> > Hi,
> > 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).
> > 
> > [0] https://lore.kernel.org/lkml/20201021201224.3430546-1-bjorn.andersson@linaro.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
> > 
> > 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.
> 
> This is because the driver from patch set [0] does not use *ext()
> multicolor registration API, but follows old-fashion LED name
> initialization via 'name' property of struct led_classdev, which is
> initialized to DT 'label' value or DT node name if the former is absent.
> 

Sorry, I had missed this advancement. I will update the LPG patches to
add this and to switch to expect LED_COLOR_ID_RGB instead of multi.

Thanks,
Bjorn