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