[PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend

[PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend

[Aren Moynihan]

If none of the managed leds enable LED_CORE_SUSPENDRESUME, then we
shouldn't need to set it here. This makes it possible to use multicolor
groups with gpio leds that enable retain-state-suspended in the device
tree.

Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---

Changes in v2:
 - make sure count gets initialized
 - send the patch to (hopefully) all the correct people this time

 drivers/leds/rgb/leds-group-multicolor.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
index 39f58be32af5..b6c7679015fd 100644
--- a/drivers/leds/rgb/leds-group-multicolor.c
+++ b/drivers/leds/rgb/leds-group-multicolor.c
@@ -69,7 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
 	struct mc_subled *subled;
 	struct leds_multicolor *priv;
 	unsigned int max_brightness = 0;
-	int i, ret, count = 0;
+	int i, ret, count = 0, common_flags = 0;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -91,6 +91,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
 		if (!priv->monochromatics)
 			return -ENOMEM;
 
+		common_flags |= led_cdev->flags;
 		priv->monochromatics[count] = led_cdev;
 
 		max_brightness = max(max_brightness, led_cdev->max_brightness);
@@ -114,12 +115,15 @@ static int leds_gmc_probe(struct platform_device *pdev)
 
 	/* Initialise the multicolor's LED class device */
 	cdev = &priv->mc_cdev.led_cdev;
-	cdev->flags = LED_CORE_SUSPENDRESUME;
 	cdev->brightness_set_blocking = leds_gmc_set;
 	cdev->max_brightness = max_brightness;
 	cdev->color = LED_COLOR_ID_MULTI;
 	priv->mc_cdev.num_colors = count;
 
+	/* we only need suspend/resume if a sub-led requests it */
+	if (common_flags & LED_CORE_SUSPENDRESUME)
+		cdev->flags = LED_CORE_SUSPENDRESUME;
+
 	init_data.fwnode = dev_fwnode(dev);
 	ret = devm_led_classdev_multicolor_register_ext(dev, &priv->mc_cdev, &init_data);
 	if (ret)
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] arm64: dts: sun50i-a64-pinephone: Retain leds state in suspend

[Aren Moynihan]

From: Miles Alan <m@milesalan.com>

Allows user to set a led before entering suspend to know that
the phone is still on (or could be used for notifications etc.)

Signed-off-by: Miles Alan <m@milesalan.com>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---

(no changes since v1)

 arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
index 87847116ab6d..ad2476ee01e4 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -43,18 +43,21 @@ led-0 {
 			function = LED_FUNCTION_INDICATOR;
 			color = <LED_COLOR_ID_BLUE>;
 			gpios = <&pio 3 20 GPIO_ACTIVE_HIGH>; /* PD20 */
+			retain-state-suspended;
 		};
 
 		led-1 {
 			function = LED_FUNCTION_INDICATOR;
 			color = <LED_COLOR_ID_GREEN>;
 			gpios = <&pio 3 18 GPIO_ACTIVE_HIGH>; /* PD18 */
+			retain-state-suspended;
 		};
 
 		led-2 {
 			function = LED_FUNCTION_INDICATOR;
 			color = <LED_COLOR_ID_RED>;
 			gpios = <&pio 3 19 GPIO_ACTIVE_HIGH>; /* PD19 */
+			retain-state-suspended;
 		};
 	};
 
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Aren Moynihan]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ondřej Jirman]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Aren]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ondřej Jirman]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] arm64: dts: sun50i-a64-pinephone: change led type to status

[Aren Moynihan]

The status function is described in the documentation as being a rgb led
used for system notifications on phones[1][2]. This is exactly what this
led is used for on the PinePhone, so using status is probably more
accurate than indicator.

1: Documentation/leds/well-known-leds.txt
2: include/dt-bindings/leds/common.h

Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
I can't find any documentation describing the indicator function, so
it's definitely less specific than status, but besides that I'm not sure
how it compares. Please ignore this patch if it's not useful.

(no changes since v1)

 arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
index e53e0d4579a7..6d327266e6cc 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -61,7 +61,7 @@ led2: led-2 {
 	multi-led {
 		compatible = "leds-group-multicolor";
 		color = <LED_COLOR_ID_RGB>;
-		function = LED_FUNCTION_INDICATOR;
+		function = LED_FUNCTION_STATUS;
 		leds = <&led0>, <&led1>, <&led2>;
 	};
 
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] arm64: dts: sun50i-a64-pinephone: change led type to status

[Jernej Škrabec]

Dne torek, 06. februar 2024 ob 19:13:20 CET je Aren Moynihan napisal(a):
> The status function is described in the documentation as being a rgb led
> used for system notifications on phones[1][2]. This is exactly what this
> led is used for on the PinePhone, so using status is probably more
> accurate than indicator.
> 
> 1: Documentation/leds/well-known-leds.txt
> 2: include/dt-bindings/leds/common.h
> 
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>

Sorry for late review.

Please update subject in patches 2-3. Instead of "sun50i-a64-pinephone:"
use "allwinner: pinephone:" (check commit history of sun50i-a64-pinephone.dtsi).
Also rgb -> RGB, led -> LED. Last, please reword commit message to exclude
links and just say DT bindings documentation.

Note that I'll merge patches 2-3 once patch 1 is merged.

Best regards,
Jernej

> ---
> I can't find any documentation describing the indicator function, so
> it's definitely less specific than status, but besides that I'm not sure
> how it compares. Please ignore this patch if it's not useful.
> 
> (no changes since v1)
> 
>  arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> index e53e0d4579a7..6d327266e6cc 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> @@ -61,7 +61,7 @@ led2: led-2 {
>  	multi-led {
>  		compatible = "leds-group-multicolor";
>  		color = <LED_COLOR_ID_RGB>;
> -		function = LED_FUNCTION_INDICATOR;
> +		function = LED_FUNCTION_STATUS;
>  		leds = <&led0>, <&led1>, <&led2>;
>  	};
>  
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] arm64: dts: sun50i-a64-pinephone: change led type to status

[Lee Jones]

On Thu, 22 Feb 2024, Jernej Škrabec wrote:

> Dne torek, 06. februar 2024 ob 19:13:20 CET je Aren Moynihan napisal(a):
> > The status function is described in the documentation as being a rgb led
> > used for system notifications on phones[1][2]. This is exactly what this
> > led is used for on the PinePhone, so using status is probably more
> > accurate than indicator.
> > 
> > 1: Documentation/leds/well-known-leds.txt
> > 2: include/dt-bindings/leds/common.h
> > 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> 
> Sorry for late review.
> 
> Please update subject in patches 2-3. Instead of "sun50i-a64-pinephone:"
> use "allwinner: pinephone:" (check commit history of sun50i-a64-pinephone.dtsi).
> Also rgb -> RGB, led -> LED. Last, please reword commit message to exclude
> links and just say DT bindings documentation.
> 
> Note that I'll merge patches 2-3 once patch 1 is merged.

Works for me - I'll go apply it now.

-- 
Lee Jones [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] arm64: dts: sun50i-a64-pinephone: change led type to status

[Aren]

On Thu, Feb 22, 2024 at 09:57:00PM +0100, Jernej Škrabec wrote:
> Dne torek, 06. februar 2024 ob 19:13:20 CET je Aren Moynihan napisal(a):
> > The status function is described in the documentation as being a rgb led
> > used for system notifications on phones[1][2]. This is exactly what this
> > led is used for on the PinePhone, so using status is probably more
> > accurate than indicator.
> > 
> > 1: Documentation/leds/well-known-leds.txt
> > 2: include/dt-bindings/leds/common.h
> > 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> 
> Sorry for late review.
> 
> Please update subject in patches 2-3. Instead of "sun50i-a64-pinephone:"
> use "allwinner: pinephone:" (check commit history of sun50i-a64-pinephone.dtsi).
> Also rgb -> RGB, led -> LED. Last, please reword commit message to exclude
> links and just say DT bindings documentation.
> 
> Note that I'll merge patches 2-3 once patch 1 is merged.

Would you like me to reword and resend the patches, or is it quicker
for you to just do it when you apply them?

Thanks for taking a look at this,
 - Aren

> Best regards,
> Jernej
> 
> > ---
> > I can't find any documentation describing the indicator function, so
> > it's definitely less specific than status, but besides that I'm not sure
> > how it compares. Please ignore this patch if it's not useful.
> > 
> > (no changes since v1)
> > 
> >  arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > index e53e0d4579a7..6d327266e6cc 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > @@ -61,7 +61,7 @@ led2: led-2 {
> >  	multi-led {
> >  		compatible = "leds-group-multicolor";
> >  		color = <LED_COLOR_ID_RGB>;
> > -		function = LED_FUNCTION_INDICATOR;
> > +		function = LED_FUNCTION_STATUS;
> >  		leds = <&led0>, <&led1>, <&led2>;
> >  	};
> >  
> > 
> 
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] arm64: dts: sun50i-a64-pinephone: change led type to status

[Jernej Škrabec]

Dne petek, 23. februar 2024 ob 17:30:00 CET je Aren napisal(a):
> On Thu, Feb 22, 2024 at 09:57:00PM +0100, Jernej Škrabec wrote:
> > Dne torek, 06. februar 2024 ob 19:13:20 CET je Aren Moynihan napisal(a):
> > > The status function is described in the documentation as being a rgb led
> > > used for system notifications on phones[1][2]. This is exactly what this
> > > led is used for on the PinePhone, so using status is probably more
> > > accurate than indicator.
> > > 
> > > 1: Documentation/leds/well-known-leds.txt
> > > 2: include/dt-bindings/leds/common.h
> > > 
> > > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> > 
> > Sorry for late review.
> > 
> > Please update subject in patches 2-3. Instead of "sun50i-a64-pinephone:"
> > use "allwinner: pinephone:" (check commit history of sun50i-a64-pinephone.dtsi).
> > Also rgb -> RGB, led -> LED. Last, please reword commit message to exclude
> > links and just say DT bindings documentation.
> > 
> > Note that I'll merge patches 2-3 once patch 1 is merged.
> 
> Would you like me to reword and resend the patches, or is it quicker
> for you to just do it when you apply them?

Since Ondřej raised concerns, let's finish that discussion first. It's possible
that this patch will be rejected. That would also mean new revision of patches.

Sadly, this means DT patches will miss v6.9 window.

Best regards,
Jernej

> 
> Thanks for taking a look at this,
>  - Aren
> 
> > Best regards,
> > Jernej
> > 
> > > ---
> > > I can't find any documentation describing the indicator function, so
> > > it's definitely less specific than status, but besides that I'm not sure
> > > how it compares. Please ignore this patch if it's not useful.
> > > 
> > > (no changes since v1)
> > > 
> > >  arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > index e53e0d4579a7..6d327266e6cc 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > @@ -61,7 +61,7 @@ led2: led-2 {
> > >  	multi-led {
> > >  		compatible = "leds-group-multicolor";
> > >  		color = <LED_COLOR_ID_RGB>;
> > > -		function = LED_FUNCTION_INDICATOR;
> > > +		function = LED_FUNCTION_STATUS;
> > >  		leds = <&led0>, <&led1>, <&led2>;
> > >  	};
> > >  
> > > 
> > 
> > 
> > 
> > 
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 697 bytes --]

Hi!

> If none of the managed leds enable LED_CORE_SUSPENDRESUME, then we
> shouldn't need to set it here. This makes it possible to use multicolor
> groups with gpio leds that enable retain-state-suspended in the device
> tree.
> 
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>

Thanks for the series.

Acked-by: Pavel Machek <pavel@ucw.cz>

Note this will change userland API and maybe break the LED for code
expecting old setup and hardcoding paths. I guess we should not
backport this to stable. But we should do this, because it is really
one LED and not three.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend

[Lee Jones]

On Thu, 22 Feb 2024, Pavel Machek wrote:

> Hi!
> 
> > If none of the managed leds enable LED_CORE_SUSPENDRESUME, then we
> > shouldn't need to set it here. This makes it possible to use multicolor
> > groups with gpio leds that enable retain-state-suspended in the device
> > tree.
> > 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> 
> Thanks for the series.
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> Note this will change userland API and maybe break the LED for code
> expecting old setup and hardcoding paths. I guess we should not
> backport this to stable. But we should do this, because it is really
> one LED and not three.

Thanks Pavel.

Is this tied to the other patches in the set?

Will thing break if this is applied on its own?

-- 
Lee Jones [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend

[Lee Jones]

On Tue, 06 Feb 2024 13:13:17 -0500, Aren Moynihan wrote:
> If none of the managed leds enable LED_CORE_SUSPENDRESUME, then we
> shouldn't need to set it here. This makes it possible to use multicolor
> groups with gpio leds that enable retain-state-suspended in the device
> tree.
> 
> 

Applied, thanks!

[1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend
      commit: 68552911e71d59e62dd5e50e9ac56ebfc32f0c71

--
Lee Jones [李琼斯]


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend

[Lee Jones]

On Fri, 23 Feb 2024, Lee Jones wrote:

> On Tue, 06 Feb 2024 13:13:17 -0500, Aren Moynihan wrote:
> > If none of the managed leds enable LED_CORE_SUSPENDRESUME, then we
> > shouldn't need to set it here. This makes it possible to use multicolor
> > groups with gpio leds that enable retain-state-suspended in the device
> > tree.
> > 
> > 
> 
> Applied, thanks!
> 
> [1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend
>       commit: 68552911e71d59e62dd5e50e9ac56ebfc32f0c71

Note that I changed a bunch of grammatical issues.

led  => LED
gpio => GPIO

Capitalised the first word of the comment, etc.

-- 
Lee Jones [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend

[Aren]

On Fri, Feb 23, 2024 at 10:35:37AM +0000, Lee Jones wrote:
> On Fri, 23 Feb 2024, Lee Jones wrote:
> 
> > On Tue, 06 Feb 2024 13:13:17 -0500, Aren Moynihan wrote:
> > > If none of the managed leds enable LED_CORE_SUSPENDRESUME, then we
> > > shouldn't need to set it here. This makes it possible to use multicolor
> > > groups with gpio leds that enable retain-state-suspended in the device
> > > tree.
> > > 
> > > 
> > 
> > Applied, thanks!
> > 
> > [1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend
> >       commit: 68552911e71d59e62dd5e50e9ac56ebfc32f0c71
> 
> Note that I changed a bunch of grammatical issues.
> 
> led  => LED
> gpio => GPIO
> 
> Capitalised the first word of the comment, etc.

Awesome, thank you
 - Aren

> -- 
> Lee Jones [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel