Message-ID: <xhowm7pne2og7iyhoa7wi26qa5q23fvl2ptaxtwnp4upgunubo@q5unl6mzvluy>
Date: Sat, 24 Feb 2024 00:28:36 +0100
From: Ondřej Jirman <megi@....cz>
To: Aren <aren@...cevolution.org>
Cc: linux-kernel@...r.kernel.org, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org, 
	Jean-Jacques Hiblot <jjhiblot@...phandler.com>, Chen-Yu Tsai <wens@...e.org>, linux-sunxi@...ts.linux.dev, 
	Lee Jones <lee@...nel.org>, Pavel Machek <pavel@....cz>, 
	linux-arm-kernel@...ts.infradead.org, Jernej Skrabec <jernej.skrabec@...il.com>, 
	linux-leds@...r.kernel.org, Conor Dooley <conor+dt@...nel.org>, Miles Alan <m@...esalan.com>, 
	Samuel Holland <samuel@...lland.org>
Subject: Re: [PATCH v2 3/4] arm64: dts: sun50i-a64-pinephone: add multicolor
 led node

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@...cevolution.org>
> > > ---
> > > 
> > > 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.

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

Powered by blists - more mailing lists