leds: core: add support for RGB LED's

PATCH Superseded Heiner Kallweit 3 participants 6 messages Mar 1, 2016 – Mar 30, 2016 LED

lore

Share & tools

lore

Heiner Kallweit

author

2016-03-01 21:37 UTC #0 lore raw

PATCH v5 4/4 leds: core: add support for RGB LED's

Export a function to convert HSV color values to RGB. It's intended to be called by drivers for RGB LEDs.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- move hsv -> rgb conversion to separate file
- remove flag LED_DEV_CAP_RGB
v3:
- call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set This is needed in cases when we have monochrome and color LEDs as well in a system.
v4:
- Export led_hsv_to_rgb and let the device driver call it instead of doing the conversion in the core
v5:
- don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB is set but warn

---
 drivers/leds/led-class.c    |  7 +++++++
 drivers/leds/led-rgb-core.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/leds.h        |  8 ++++++++
 3 files changed, 51 insertions(+)

drivers/leds/led-class.c +7 -0

index 8a3748a..a4b144e 100644
@@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	char name[64];
 	int ret;
 
+	/*
+	 * for now reading back the color is not supported as multiple
+	 * HSV -> RGB -> HSV conversions may distort the color due to
+	 * rounding issues in the conversion algorithm
+	 */
+	WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
+
 	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
 	if (ret < 0)
 		return ret;

drivers/leds/led-rgb-core.c +36 -0

index f6591f1..2b18d4c 100644
@@ -38,3 +38,39 @@ enum led_brightness led_confine_brightness(struct led_classdev *led_cdev,
 	return brightness |
 	       min(value & LED_BRIGHTNESS_MASK, led_cdev->max_brightness);
 }
+
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv)
+{
+	int h = min_t(int, (hsv >> 16) & 0xff, 251);
+	int s = (hsv >> 8) & 0xff;
+	int v = hsv & 0xff;
+	int f, p, q, t, r, g, b;
+
+	if (!v)
+		return 0;
+	if (!s)
+		return (v << 16) + (v << 8) + v;
+
+	f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
+	p = v - DIV_ROUND_CLOSEST(s * v, 255);
+	q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
+	t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
+
+	switch (h / 42) {
+	case 0:
+		r = v; g = t; b = p; break;
+	case 1:
+		r = q; g = v; b = p; break;
+	case 2:
+		r = p; g = v; b = t; break;
+	case 3:
+		r = p; g = q; b = v; break;
+	case 4:
+		r = t; g = p; b = v; break;
+	case 5:
+		r = v; g = p; b = q; break;
+	}
+
+	return (r << 16) + (g << 8) + b;
+}
+EXPORT_SYMBOL_GPL(led_hsv_to_rgb);

include/linux/leds.h +8 -0

index bbf24bb..82b3477 100644
@@ -226,6 +226,14 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
 	return led_cdev->flags & LED_SYSFS_DISABLE;
 }
 
+/**
+ * led_hsv_to_rgb - convert a hsv color value to rgb color model
+ * @hsv: the hsv value to convert
+ *
+ * Returns: the resulting rgb value
+ */
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
+
 /*
  * LED Triggers
  */

Signature · 1 line

2.7.1

Jacek Anaszewski

2016-03-04 09:05 UTC #1 lore raw

Quoted 29 lines · Heiner Kallweit

Export a function to convert HSV color values to RGB. It's intended to be called by drivers for RGB LEDs.
Signed-off-by Heiner Kallweit <[email protected]>
---
v2:
- move hsv -> rgb conversion to separate file
- remove flag LED_DEV_CAP_RGB
v3:
- call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set This is needed in cases when we have monochrome and color LEDs as well in a system.
v4:
- Export led_hsv_to_rgb and let the device driver call it instead of doing the conversion in the core
v5:
- don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB is set but warn
---
  drivers/leds/led-class.c    |  7 +++++++
  drivers/leds/led-rgb-core.c | 36 ++++++++++++++++++++++++++++++++++++
  include/linux/leds.h        |  8 ++++++++
  3 files changed, 51 insertions(+)
drivers/leds/led-class.c +0 -0
index 8a3748a..a4b144e 100644

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 200

Quoted 8 lines

drivers/leds/led-class.c +5 -0

@@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
  	char name[64];
  	int ret;
 
+	/*
+	 * for now reading back the color is not supported as multiple
+	 * HSV -> RGB -> HSV conversions may distort the color due to
+	 * rounding issues in the conversion algorithm
+	 */

Does the "for now" mean that you plan on supporting it in the future?

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 202

Quoted 2 lines

drivers/leds/led-class.c +2 -0

+	WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
+

Could you move this warning to the line 216?

Quoted 67 lines

drivers/leds/led-class.c +0 -0

  	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
  	if (ret < 0)
  		return ret;

drivers/leds/led-rgb-core.c +36 -0

index f6591f1..2b18d4c 100644
@@ -38,3 +38,39 @@ enum led_brightness led_confine_brightness(struct led_classdev *led_cdev,
  	return brightness |
  	       min(value & LED_BRIGHTNESS_MASK, led_cdev->max_brightness);
  }
+
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv)
+{
+	int h = min_t(int, (hsv >> 16) & 0xff, 251);
+	int s = (hsv >> 8) & 0xff;
+	int v = hsv & 0xff;
+	int f, p, q, t, r, g, b;
+
+	if (!v)
+		return 0;
+	if (!s)
+		return (v << 16) + (v << 8) + v;
+
+	f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
+	p = v - DIV_ROUND_CLOSEST(s * v, 255);
+	q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
+	t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
+
+	switch (h / 42) {
+	case 0:
+		r = v; g = t; b = p; break;
+	case 1:
+		r = q; g = v; b = p; break;
+	case 2:
+		r = p; g = v; b = t; break;
+	case 3:
+		r = p; g = q; b = v; break;
+	case 4:
+		r = t; g = p; b = v; break;
+	case 5:
+		r = v; g = p; b = q; break;
+	}
+
+	return (r << 16) + (g << 8) + b;
+}
+EXPORT_SYMBOL_GPL(led_hsv_to_rgb);

include/linux/leds.h +8 -0

index bbf24bb..82b3477 100644
@@ -226,6 +226,14 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
  	return led_cdev->flags & LED_SYSFS_DISABLE;
  }
 
+/**
+ * led_hsv_to_rgb - convert a hsv color value to rgb color model
+ * @hsv: the hsv value to convert
+ *
+ * Returns: the resulting rgb value
+ */
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
+
  /*
   * LED Triggers
   */

Signature · 2 lines

Best regards,
Jacek Anaszewski

Heiner Kallweit

author

2016-03-04 21:23 UTC #2 lore raw

Am 04.03.2016 um 10:05 schrieb Jacek Anaszewski:

Quoted 33 lines · Heiner Kallweit

Quoted 32 lines
Export a function to convert HSV color values to RGB. It's intended to be called by drivers for RGB LEDs.
Signed-off-by Heiner Kallweit <[email protected]>
---
v2:
- move hsv -> rgb conversion to separate file
- remove flag LED_DEV_CAP_RGB
v3:
- call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set This is needed in cases when we have monochrome and color LEDs as well in a system.
v4:
- Export led_hsv_to_rgb and let the device driver call it instead of doing the conversion in the core
v5:
- don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB is set but warn
---
  drivers/leds/led-class.c    |  7 +++++++
  drivers/leds/led-rgb-core.c | 36 ++++++++++++++++++++++++++++++++++++
  include/linux/leds.h        |  8 ++++++++
  3 files changed, 51 insertions(+)
drivers/leds/led-class.c +0 -0
index 8a3748a..a4b144e 100644
@@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
      char name[64];
      int ret;
 

Quoted 8 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 200
Quoted 5 lines
drivers/leds/led-class.c +5 -0
+    /*
+     * for now reading back the color is not supported as multiple
+     * HSV -> RGB -> HSV conversions may distort the color due to
+     * rounding issues in the conversion algorithm
+     */
Does the "for now" mean that you plan on supporting it in the future?

It was meant as "until somebody has an idea how to implement the conversion HSV -> RGB -> HSV in a way that always results in the original values". I'll remove the "for now".

Quoted 74 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 202

Quoted 2 lines

drivers/leds/led-class.c +2 -0

+    WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
+

Could you move this warning to the line 216?

Quoted 67 lines

drivers/leds/led-class.c +0 -0

      ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
      if (ret < 0)
          return ret;

drivers/leds/led-rgb-core.c +36 -0

index f6591f1..2b18d4c 100644
@@ -38,3 +38,39 @@ enum led_brightness led_confine_brightness(struct led_classdev *led_cdev,
      return brightness |
             min(value & LED_BRIGHTNESS_MASK, led_cdev->max_brightness);
  }
+
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv)
+{
+    int h = min_t(int, (hsv >> 16) & 0xff, 251);
+    int s = (hsv >> 8) & 0xff;
+    int v = hsv & 0xff;
+    int f, p, q, t, r, g, b;
+
+    if (!v)
+        return 0;
+    if (!s)
+        return (v << 16) + (v << 8) + v;
+
+    f = DIV_ROUND_CLOSEST((h % 42) * 255, 42);
+    p = v - DIV_ROUND_CLOSEST(s * v, 255);
+    q = v - DIV_ROUND_CLOSEST(f * s * v, 255 * 255);
+    t = v - DIV_ROUND_CLOSEST((255 - f) * s * v, 255 * 255);
+
+    switch (h / 42) {
+    case 0:
+        r = v; g = t; b = p; break;
+    case 1:
+        r = q; g = v; b = p; break;
+    case 2:
+        r = p; g = v; b = t; break;
+    case 3:
+        r = p; g = q; b = v; break;
+    case 4:
+        r = t; g = p; b = v; break;
+    case 5:
+        r = v; g = p; b = q; break;
+    }
+
+    return (r << 16) + (g << 8) + b;
+}
+EXPORT_SYMBOL_GPL(led_hsv_to_rgb);

include/linux/leds.h +8 -0

index bbf24bb..82b3477 100644
@@ -226,6 +226,14 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
      return led_cdev->flags & LED_SYSFS_DISABLE;
  }
 
+/**
+ * led_hsv_to_rgb - convert a hsv color value to rgb color model
+ * @hsv: the hsv value to convert
+ *
+ * Returns: the resulting rgb value
+ */
+enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
+
  /*
   * LED Triggers
   */

Pavel Machek

2016-03-29 10:05 UTC #3 lore raw

In reply to Heiner Kallweit

Quoted 29 lines · Heiner Kallweit

Export a function to convert HSV color values to RGB. It's intended to be called by drivers for RGB LEDs.
Signed-off-by Heiner Kallweit <[email protected]>
---
v2:
- move hsv -> rgb conversion to separate file
- remove flag LED_DEV_CAP_RGB
v3:
- call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set This is needed in cases when we have monochrome and color LEDs as well in a system.
v4:
- Export led_hsv_to_rgb and let the device driver call it instead of doing the conversion in the core
v5:
- don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB is set but warn
---
 drivers/leds/led-class.c    |  7 +++++++
 drivers/leds/led-rgb-core.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/leds.h        |  8 ++++++++
 3 files changed, 51 insertions(+)
drivers/leds/led-class.c +0 -0
index 8a3748a..a4b144e 100644

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 202

Quoted 10 lines

drivers/leds/led-class.c +7 -0

@@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	char name[64];
 	int ret;
 
+	/*
+	 * for now reading back the color is not supported as multiple
+	 * HSV -> RGB -> HSV conversions may distort the color due to
+	 * rounding issues in the conversion algorithm
+	 */
+	WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
+

Backtrace is useless here, you may want to add some ()s and you don't really want user to be causing messages in syslog this easily.
Pavel

Signature · 6 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Heiner Kallweit

author

2016-03-29 20:43 UTC #4 lore raw

Am 29.03.2016 um 12:05 schrieb Pavel Machek:

Quoted 33 lines · Heiner Kallweit

Quoted 32 lines
Export a function to convert HSV color values to RGB. It's intended to be called by drivers for RGB LEDs.
Signed-off-by Heiner Kallweit <[email protected]>
---
v2:
- move hsv -> rgb conversion to separate file
- remove flag LED_DEV_CAP_RGB
v3:
- call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set This is needed in cases when we have monochrome and color LEDs as well in a system.
v4:
- Export led_hsv_to_rgb and let the device driver call it instead of doing the conversion in the core
v5:
- don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB is set but warn
---
 drivers/leds/led-class.c    |  7 +++++++
 drivers/leds/led-rgb-core.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/leds.h        |  8 ++++++++
 3 files changed, 51 insertions(+)
drivers/leds/led-class.c +0 -0
index 8a3748a..a4b144e 100644
@@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	char name[64];
 	int ret;
 

Quoted 11 lines

drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 202
Quoted 7 lines
drivers/leds/led-class.c +7 -0
+	/*
+	 * for now reading back the color is not supported as multiple
+	 * HSV -> RGB -> HSV conversions may distort the color due to
+	 * rounding issues in the conversion algorithm
+	 */
+	WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
+
Backtrace is useless here, you may want to add some ()s and you don't really want user to be causing messages in syslog this easily.

I agree that the backtrace doesn't provide a benefit here.

However I don't see how a user could create syslog entries.
The warn condition can be true only for drivers implementing the RGB extension in a not supported way (by setting flag LED_DEV_CAP_RGB and implementing brightness_get).

Pavel

Quoted 1 line

Jacek Anaszewski

2016-03-30 08:00 UTC #5 lore raw

Quoted 45 lines · Heiner Kallweit

Am 29.03.2016 um 12:05 schrieb Pavel Machek:
Quoted 33 lines · Heiner Kallweit
Quoted 32 lines
Export a function to convert HSV color values to RGB. It's intended to be called by drivers for RGB LEDs.
Signed-off-by Heiner Kallweit <[email protected]>
---
v2:
- move hsv -> rgb conversion to separate file
- remove flag LED_DEV_CAP_RGB
v3:
- call led_hsv_to_rgb only if LED_DEV_CAP_HSV is set This is needed in cases when we have monochrome and color LEDs as well in a system.
v4:
- Export led_hsv_to_rgb and let the device driver call it instead of doing the conversion in the core
v5:
- don't ignore led_cdev->brightness_get silently if LED_DEV_CAP_RGB is set but warn
---
  drivers/leds/led-class.c    |  7 +++++++
  drivers/leds/led-rgb-core.c | 36 ++++++++++++++++++++++++++++++++++++
  include/linux/leds.h        |  8 ++++++++
  3 files changed, 51 insertions(+)
drivers/leds/led-class.c +0 -0
index 8a3748a..a4b144e 100644
@@ -193,6 +193,13 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
  	char name[64];
  	int ret;
 
Quoted 11 lines
drivers/leds/led-class.c int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) · line 202
Quoted 7 lines
drivers/leds/led-class.c +7 -0
+	/*
+	 * for now reading back the color is not supported as multiple
+	 * HSV -> RGB -> HSV conversions may distort the color due to
+	 * rounding issues in the conversion algorithm
+	 */
+	WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get);
+
Backtrace is useless here, you may want to add some ()s and you don't really want user to be causing messages in syslog this easily.

Quoted 1 line

I agree that the backtrace doesn't provide a benefit here.

I requested it to provide a verbose notification for a LED RGB class driver developer, in case they try to implement a brightness_get op, which would be harmful in case of HSV interface for RGB LEDs.

Quoted 10 lines

However I don't see how a user could create syslog entries.
The warn condition can be true only for drivers implementing the RGB extension in a not supported way (by setting flag LED_DEV_CAP_RGB and implementing brightness_get).
Pavel
Quoted 1 line

Signature · 2 lines

Best regards,
Jacek Anaszewski