Re: [PATCH v2 3/4] arm64: dts: sun50i-a64-pinephone: add multicolor led node

[Aren]

On Sat, Feb 24, 2024 at 12:28:36AM +0100, Ondřej Jirman wrote:
> Hello Aren,
> 
> On Fri, Feb 23, 2024 at 10:22:36AM -0500, Aren wrote:
> > On Fri, Feb 23, 2024 at 09:46:25AM +0100, Ondřej Jirman wrote:
> > > Hello Aren,
> > > 
> > > On Tue, Feb 06, 2024 at 01:13:19PM -0500, Aren Moynihan wrote:
> > > > The red, green, and blue leds currently in the device tree represent a
> > > > single rgb led on the front of the PinePhone.
> > > > 
> > > > Signed-off-by: Aren Moynihan <aren@xxxxxxxxxxxxxxxxx>
> > > > ---
> > > > 
> > > > Changes in v2:
> > > >  - remove function property from individual led nodes
> > > > 
> > > >  .../boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 16 ++++++++++------
> > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > > index ad2476ee01e4..e53e0d4579a7 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > > @@ -39,28 +39,32 @@ chosen {
> > > >  	leds {
> > > >  		compatible = "gpio-leds";
> > > >  
> > > > -		led-0 {
> > > > -			function = LED_FUNCTION_INDICATOR;
> > > 
> > > This looks like a needless change that will just break people's current scripts
> > > and setup. It does mine, and there sure are others that will be surprised, too.
> > > 
> > > This leads to a change in sysfs path from:
> > > 
> > >   /sys/class/leds/blue:indicator
> > > 
> > > to
> > > 
> > >   /sys/class/leds/blue:
> > > 
> > > which is 1) a weird name and 2) a backwards compatibility break for seemingly
> > > no apparent reason. Any reaons for the change?
> > 
> > Leds-group-multicolor will make those read-only, so that will break when
> > it's enabled either way. Removing the function property makes it less
> > likely that programs attempting to discover leds will use the wrong
> > path.
> 
> Allright. Then this breaks even more usecases then I thought. I quite depend on
> being able to apply various kernel based LED triggers to individual LEDs in the
> RGB LED. (The LED is not really multi-color in practice - you don't get a mix of
> colors. It's only technically a RGB LED, but if you turn on red and green, you
> see red and green, individually. I suspect it's a clear capped LED, not
> a diffused one and expects an external difuser - which Pinephone doesn't have.
> So this works out quite fine from the user perspective, because you see
> individual colors and can assign meanings to them individually)
> 
> So this change makes it that I'll not be able to map different triggers
> to individual colors now.
> 
> Eg. I have a power manager driver for Pinephone keyboard that exports LED triggers
> to indicate various charging states of the keyboard battery, so that I get
> warnings when the KB is discharged, or disconnected from control algorithm
> due to its tendency to shut off, and disappear from I2C bus, etc. AXP20x driver
> also exports a bunch of useful triggers.
> 
> ---
> 
> As a side note, this new multicolor abstraction seems a bit lacking and
> cumbersome, too.
> 
> I still have to control individual LEDs via multi_intensity file and have to
> parse multi_index to even know which number corresponds to what color. In addition
> I have to ensure correct value in brightness file.
> 
> If I want to change one of the LEDs, I now have to first read the whole status
> of multi-color LED and update it somehow in a non-racy way, which is
> impossible with this API without going through some userspace daemon that
> centralizes the LED control. 
> 
> If this is expected to be usable in device independent way, then there's another
> problem:  multi_intensity values don't seem to have any range defined in sysfs,
> so I can't know what is the max usable value. How do I know the range is 0-1 vs
> 0-255 or 0-200000? There's max_brightness but that seems to be related to
> possible values in brightness file. Seems like I can write pretty much anything
> to multi_intensity, even things like this:
> 
>   https://megous.com/dl/tmp/4c6db9376f148951.png
> 
> (looks like all LEDs are turned on even for negative intensity)
> 
> If multi_intensity is supposed to be some relative measure of brightenss of
> individual LEDs in RGB LED, then echo '1 1 1000' > multi_intensity 
> should pretty much result in only red LED being on. But all are on at full
> brightness. So the kernel doesn't even represent the desired color properly,
> based on HW limitations it knows about. There's no aproximation/rounding to the
> limits of control of individual LEDs. Even 1 millionth of relative intensity
> will result in the LED being turned fully on.
> 
> Discoverability of how to control the LEDs is also quite lacking. Individual
> directories under /sys/class/leds are still there for individual parts of RGB
> LED, and it's not clearly identifiable that you can't control them. brightness
> file still has -rw-r--r-- permissions. But writing to it produces EBUSY.
> As far as I can see, the only indication that something is amiss is that
> there's a device/consumer:platform:multi-led symlink under the individual LEDs.
> But that can have any name in practice, and how can one know that just because
> some device has a in-kernel consumer, that it's a kind of a consumer that blocks
> changes to the device?
> 
> If I have to discover that some LED is controlled by some other driver only
> by trying to light it up, then that's not a great API, I'd say.
> 
> End of side note. :)
> 
> ---
> 
> ^^^^ All this from the PoV of a (API) user. I can understand the device-tree
> appeal of representing the technically RGB LED as RGB node, but as a user who
> messes with sysfs, and likes the simplicity and flexibility of how things work
> now, with the upstream Pinephone DT, I really dislike the change, because it
> forces unpleasant userspace changes, incl. necessity for some userspace LED
> control daemon to get race-free LED control from multiple sources. And I'm not
> even sure what I'd do with the keyboard driver, and how to keep the triggers
> working.
> 
> Anyway, I and others can get around all this by:
> 
>   echo multi-led > /sys/class/leds/rgb:status/device/driver/unbind
> 
> at system startup and get back control via idndividual LED directories.
> But please don't change the names of directories that were there up to now. That
> will just break many people's scripts for no reason. So my only request is to
> make this fallback to previous way it worked easy.

Makes sense, I'll send v1 again with a revised commit message when I get
a chance (this was the only change to the device tree part of this
patchset between v1 and v2).

Thanks
 - Aren

> Kind regards,
> 	o.
> 
> > I left these in v1 of this patch, but was recommended to remove them.
> > https://lore.kernel.org/lkml/k26bellccok4tj3kz2nrtp2vth2rnsiea677e2kzm56m767wjx@pnkqiz5hmiyb/
> >
> > Thanks for taking a look at this
> >  - Aren
> > 
> > > People normally hardcode these paths in eg. /etc/tmpfiles.d to apply LED triggers
> > > to particular LEDs.
> > > 
> > > Kind regards,
> > > 	o.
> > > 
> > > > +		led0: led-0 {
> > > >  			color = <LED_COLOR_ID_BLUE>;
> > > >  			gpios = <&pio 3 20 GPIO_ACTIVE_HIGH>; /* PD20 */
> > > >  			retain-state-suspended;
> > > >  		};
> > > >  
> > > > -		led-1 {
> > > > -			function = LED_FUNCTION_INDICATOR;
> > > > +		led1: led-1 {
> > > >  			color = <LED_COLOR_ID_GREEN>;
> > > >  			gpios = <&pio 3 18 GPIO_ACTIVE_HIGH>; /* PD18 */
> > > >  			retain-state-suspended;
> > > >  		};
> > > >  
> > > > -		led-2 {
> > > > -			function = LED_FUNCTION_INDICATOR;
> > > > +		led2: led-2 {
> > > >  			color = <LED_COLOR_ID_RED>;
> > > >  			gpios = <&pio 3 19 GPIO_ACTIVE_HIGH>; /* PD19 */
> > > >  			retain-state-suspended;
> > > >  		};
> > > >  	};
> > > >  
> > > > +	multi-led {
> > > > +		compatible = "leds-group-multicolor";
> > > > +		color = <LED_COLOR_ID_RGB>;
> > > > +		function = LED_FUNCTION_INDICATOR;
> > > > +		leds = <&led0>, <&led1>, <&led2>;
> > > > +	};
> > > > +
> > > >  	reg_ps: ps-regulator {
> > > >  		compatible = "regulator-fixed";
> > > >  		regulator-name = "ps";
> > > > -- 
> > > > 2.43.0
> > > >