| Message ID | 20240206185400.596979-3-aren@peacevolution.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend | expand |
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@peacevolution.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? 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 >
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@peacevolution.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. 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 > >
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@peacevolution.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 > > >
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@peacevolution.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. 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 > > > >
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; + 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";
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@peacevolution.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(-)