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

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 5304 bytes --]

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
> 
> 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
@@ -25,11 +25,15 @@ patternProperties:
     description: Represents the LEDs that are to be grouped.
     properties:
       color:
-        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.
 
+	  For LEDs that can display arbitrary color (RGB, RGBW, etc), this
+	  property should be defined as LED_COLOR_ID_RGB.
+
     $ref: "common.yaml#"
 
     required:
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 846248a0693d..a6dce01dbd5e 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -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);
 
diff --git a/drivers/leds/leds-lp55xx-common.c 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
@@ -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
@@ -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 */
 	}
 
 	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 */
 }
 
 /*
diff --git a/include/dt-bindings/leds/common.h 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
@@ -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. */


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]