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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1119 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
-- 
(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 --]

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

[Marek Behún]

On Mon, 27 Jul 2020 10:45:00 +0200
Pavel Machek <pavel@ucw.cz> wrote:

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

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

Marvell PHYs support something they call Bi-Color LED Mixing. Normally
the LED can be either in ON or OFF state (in terms of LED API
max_brightness = 1), but with Bi-Color LED Mixing the 2-terminal
GY LED supports max_brightness = 8 for both green and yellow
channels.

Moreover Marvell PHYs support something called DUAL modes. This
basically means that this GY LED can be controlled by hardware in
several ways. For example one of these DUAL modes has following
behavior:

		LED[1]		LED[0]
1000 noact	Off		Solid On
1000   act	Off		Blink
 100 noact	Solid Mix	Solid Mix
 100   act	Blink Mix	Blink Mix
  10 noact	Solid On	Off
  10   act	Blink		Off
nolink		Off		Off

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.

Marek

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2201 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.
> 
> 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:

I'd not remove multicolor. It would be still there for non-RGB
uses. (Sorry if I was unclear).

But I may want to disable it for now, not to have ABI incompatibility
in future.

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

Yes, I guess multicolor may make sense there.

Best regards,
									Pavel
-- 
(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 --]

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

[Pavel Machek]

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

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

[Marek Behún]

On Mon, 27 Jul 2020 12:52:26 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> 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. */
> 
> 

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.

Marek

[PATCH] Add multicolor to the list.

[Pavel Machek]

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


commit ddfd8931c942a64c1ebdd7b93a4f18c84bd3b97b
Author: Pavel Machek <pavel@ucw.cz>
Date:   Mon Aug 3 13:20:06 2020 +0200

    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.
    
    Signed-off-by: Pavel Machek <pavel@ucw.cz>

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/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. */

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

[PATCH] leds: disallow /sys/class/leds/*:multi:* for now

[Pavel Machek]

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


commit 77dce3a22e8941552a15046d4113df9ce132fb3d
Author: Pavel Machek <pavel@ucw.cz>
Date:   Mon Aug 3 14:00:06 2020 +0200

    leds: disallow /sys/class/leds/*:multi:* for now
    
    All the LEDs in the queue are RGB, so they should not use multi for
    their color.
    
    Make sure we don't add such LED by mistake (and make it part of ABI).
    
    Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index a6dce01dbd5e..c4e780bdb385 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -425,6 +425,10 @@ int led_compose_name(struct device *dev, struct led_init_data *init_data,
 	struct fwnode_handle *fwnode = init_data->fwnode;
 	const char *devicename = init_data->devicename;
 
+	/* We want to label LEDs that can produce full range of colors
+	 * as RGB, not multicolor */
+	BUG_ON(props.color == LED_COLOR_ID_MULTI);
+
 	if (!led_classdev_name)
 		return -EINVAL;
 

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

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

turris-omnia: fixes needed was Re: We have multicolor, but should we turn it into RGB?

[Pavel Machek]

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

Hi!

> > +++ 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 */
> >  }
> >  
> >  /*

> 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

Ok, let's do that.

Could you review return value of omnia_led_register() [see the patch
above]? AFAICT it is buggy.

I'd like to push the driver to Linus in few days...

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

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