We have multicolor, but should we turn it into RGB?

Hi!

Multicolor is a bit too abstract. Yes, we can have Green-Magenta-Ultraviolet LED, but so far all the LEDs we support are RGB, and not even RGB-White or RGB-Yellow variants emerged.

Multicolor is not a good fit for RGB LED. It does not really know about LED color. In particular, there's no way to make LED "white".

Userspace is interested in knowing "this LED can produce arbitrary color", which not all multicolor LEDs can.

Proposal: let's add "rgb" to led_colors in drivers/leds/led-core.c, add corresponding device tree defines, and use that, instead of multicolor for RGB LEDs.

We really need to do that now; "white" stuff can wait.

RGB LEDs are quite common, and it would be good to be able to turn LED white and to turn it into any arbitrary color. It is essential that userspace is able to set arbitrary colors, and it might be good to have that ability from kernel, too... to allow full-color triggers.

Best regads,
Pavel

Quoted 14 lines

Multicolor is a bit too abstract. Yes, we can have Green-Magenta-Ultraviolet LED, but so far all the LEDs we support are RGB, and not even RGB-White or RGB-Yellow variants emerged.

Multicolor is not a good fit for RGB LED. It does not really know about LED color. In particular, there's no way to make LED "white".

Userspace is interested in knowing "this LED can produce arbitrary color", which not all multicolor LEDs can.

Proposal: let's add "rgb" to led_colors in drivers/leds/led-core.c, add corresponding device tree defines, and use that, instead of multicolor for RGB LEDs.

Quoted 6 lines

We really need to do that now; "white" stuff can wait.

RGB LEDs are quite common, and it would be good to be able to turn LED white and to turn it into any arbitrary color. It is essential that userspace is able to set arbitrary colors, and it might be good to have that ability from kernel, too... to allow full-color triggers.

I am not against adding RGB if you want to somehow teach the subsystem to mix arbitrary color (either by teaching it color curves or some other way). But I think we shouldn't remove multicolor, and here's the reason why:

Most of the time I have seen 2 LEDs per ethernet port, green and yellow, but some ports have 2 Bi-Color LEDs, each consisting of green and yellow. I think most of the time these are 2-terminal LEDs.

So basically here we have, instead of a RGB LED, a GY LED (GY for green/yellow).

So if we want to reasonably add support for this configuration of LEDs and to offer the user to configure these DUAL modes via the trigger API, I think these LEDs should be shown in the system as multicolor LEDs.

Quoted 21 lines

Multicolor is a bit too abstract. Yes, we can have Green-Magenta-Ultraviolet LED, but so far all the LEDs we support are RGB, and not even RGB-White or RGB-Yellow variants emerged.

Multicolor is not a good fit for RGB LED. It does not really know about LED color. In particular, there's no way to make LED "white".

Userspace is interested in knowing "this LED can produce arbitrary color", which not all multicolor LEDs can.

Proposal: let's add "rgb" to led_colors in drivers/leds/led-core.c, add corresponding device tree defines, and use that, instead of multicolor for RGB LEDs.

We really need to do that now; "white" stuff can wait.

RGB LEDs are quite common, and it would be good to be able to turn LED white and to turn it into any arbitrary color. It is essential that userspace is able to set arbitrary colors, and it might be good to have that ability from kernel, too... to allow full-color triggers.

Quoted 2 lines

Best regads,
Pavel

I am not against adding RGB if you want to somehow teach the subsystem to mix arbitrary color (either by teaching it color curves or some other way). But I think we shouldn't remove multicolor, and here's the reason why:

Hi!

Quoted 23 lines

Quoted 23 lines

Multicolor is a bit too abstract. Yes, we can have Green-Magenta-Ultraviolet LED, but so far all the LEDs we support are RGB, and not even RGB-White or RGB-Yellow variants emerged.

Multicolor is not a good fit for RGB LED. It does not really know about LED color. In particular, there's no way to make LED "white".

Userspace is interested in knowing "this LED can produce arbitrary color", which not all multicolor LEDs can.

Proposal: let's add "rgb" to led_colors in drivers/leds/led-core.c, add corresponding device tree defines, and use that, instead of multicolor for RGB LEDs.

We really need to do that now; "white" stuff can wait.

RGB LEDs are quite common, and it would be good to be able to turn LED white and to turn it into any arbitrary color. It is essential that userspace is able to set arbitrary colors, and it might be good to have that ability from kernel, too... to allow full-color triggers.

Quoted 8 lines

Quoted 3 lines

Best regads,
Pavel

I am not against adding RGB if you want to somehow teach the subsystem to mix arbitrary color (either by teaching it color curves or some other way). But I think we shouldn't remove multicolor, and here's the reason why:

Something like this?

diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml index b55e1f1308a4..e820040a09d9 100644 --- a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
+++
b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml

(quoted diff) +0 -0

@@ -25,11 +25,15 @@ patternProperties: description: Represents the

LEDs that are to be grouped. properties: color:

Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml +2 -2

-        const: 8  # LED_COLOR_ID_MULTI
+        minimum: 8  # LED_COLOR_ID_MULTI
+	maximum: 9  # LED_COLOR_ID_RGB
         description: |
-          For multicolor LED support this property should be defined

as
+ For non-RGB multicolor LEDs this property should be
defined as LED_COLOR_ID_MULTI which can be found in include/linux/leds/common.h.

(quoted diff) +1 -0

+	  For LEDs that can display arbitrary color (RGB, RGBW,

etc), this

Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml +2 -0

+	  property should be defined as LED_COLOR_ID_RGB.
+
     $ref: "common.yaml#"
 
     required:

drivers/leds/led-core.c +1 -0

index 846248a0693d..a6dce01dbd5e 100644
@@ -35,6 +35,7 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
 	[LED_COLOR_ID_YELLOW] = "yellow",
 	[LED_COLOR_ID_IR] = "ir",
 	[LED_COLOR_ID_MULTI] = "multicolor",
+	[LED_COLOR_ID_RGB] = "rgb",
 };
 EXPORT_SYMBOL_GPL(led_colors);
 

(quoted diff) +0 -0

b/drivers/leds/leds-lp55xx-common.c index af14e2b2d577..56210f4ad919 100644 --- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c

drivers/leds/leds-lp55xx-common.c +1 -1

@@ -638,7 +638,7 @@ static int lp55xx_parse_logical_led(struct
 device_node *np, if (ret)
 		return ret;
 
-	if (led_color == LED_COLOR_ID_MULTI)
+	if (led_color == LED_COLOR_ID_RGB)
 		return lp55xx_parse_multi_led(np, cfg, child_number);
 
 	ret =  lp55xx_parse_common_child(np, cfg, child_number,

&chan_nr); diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c index bb23d8e16614..5c360632170a 100644 --- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c

drivers/leds/leds-turris-omnia.c +4 -4

@@ -94,15 +94,15 @@ static int omnia_led_register(struct i2c_client
 *client, struct omnia_led *led, dev_warn(dev,
 			 "Node %pOF: must contain 'reg' property
 with values between 0 and %i\n", np, OMNIA_BOARD_LEDS - 1);
-		return 0;
+		return 0; /* FIXME: should return 0/errrno */
 	}
 
 	ret = of_property_read_u32(np, "color", &color);
-	if (ret || color != LED_COLOR_ID_MULTI) {
+	if (ret || color != LED_COLOR_ID_RGB) {
 		dev_warn(dev,
-			 "Node %pOF: must contain 'color' property
 with value LED_COLOR_ID_MULTI\n",
+			 "Node %pOF: must contain 'color' property
 with value LED_COLOR_ID_RGB\n", np);
-		return 0;
+		return 0; /* FIXME: should return 0/errrno */
 	}
 

drivers/leds/leds-turris-omnia.c +1 -1

 	led->subled_info[0].color_index = LED_COLOR_ID_RED;

@@ -145,7 +145,7 @@ static int omnia_led_register(struct i2c_client
 *client, struct omnia_led *led, return ret;
 	}
 
-	return 1;
+	return 1; /* FIXME: should return 0/errrno */
 }
 
 /*

(quoted diff) +0 -0

b/include/dt-bindings/leds/common.h index a463ce6a8794..52b619d44ba2 100644 --- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h

include/dt-bindings/leds/common.h +3 -2

@@ -30,8 +30,10 @@
 #define LED_COLOR_ID_VIOLET	5
 #define LED_COLOR_ID_YELLOW	6
 #define LED_COLOR_ID_IR		7
-#define LED_COLOR_ID_MULTI	8
-#define LED_COLOR_ID_MAX	9
+#define LED_COLOR_ID_MULTI	8	/* For multicolor LEDs */
+#define LED_COLOR_ID_RGB	9	/* For multicolor LEDs that
 can do arbitrary color,
+					   so this would include

RGBW and similar */ +#define LED_COLOR_ID_MAX 10

/* Standard LED functions */
/* Keyboard LEDs, usually it would be input4::capslock etc. */

Quoted 26 lines

+++ b/drivers/leds/leds-turris-omnia.c

drivers/leds/leds-turris-omnia.c +4 -4

@@ -94,15 +94,15 @@ static int omnia_led_register(struct i2c_client
 *client, struct omnia_led *led, dev_warn(dev,
 			 "Node %pOF: must contain 'reg' property
 with values between 0 and %i\n", np, OMNIA_BOARD_LEDS - 1);
-		return 0;
+		return 0; /* FIXME: should return 0/errrno */
 	}
 
 	ret = of_property_read_u32(np, "color", &color);
-	if (ret || color != LED_COLOR_ID_MULTI) {
+	if (ret || color != LED_COLOR_ID_RGB) {
 		dev_warn(dev,
-			 "Node %pOF: must contain 'color' property
 with value LED_COLOR_ID_MULTI\n",
+			 "Node %pOF: must contain 'color' property
 with value LED_COLOR_ID_RGB\n", np);
-		return 0;
+		return 0; /* FIXME: should return 0/errrno */
 	}
 

drivers/leds/leds-turris-omnia.c +0 -0

 	led->subled_info[0].color_index = LED_COLOR_ID_RED;

@@ -145,7 +145,7 @@ static int omnia_led_register(struct i2c_client
 *client, struct omnia_led *led, return ret;
 	}
 

drivers/leds/leds-turris-omnia.c static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, · line 151

Quoted 5 lines

drivers/leds/leds-turris-omnia.c +1 -1

-	return 1;
+	return 1; /* FIXME: should return 0/errrno */
 }
 
 /*

Yes, if you want to have RGB as a special case of multicolor so that in the future we can work on color curves or something, this could work