Re: [PATCH v5 4/4] leds: core: add support for RGB LED's
On 03/29/2016 10:42 PM, Heiner Kallweit wrote: Am 29.03.2016 um 12:05 schrieb Pavel Machek: On Tue 2016-03-01 22:36:12, Heiner Kallweit wrote: 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 --- 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(+) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 8a3748a..a4b144e 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -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. 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. 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 -- Best regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] leds: core: add support for RGB LED's
Am 29.03.2016 um 12:05 schrieb Pavel Machek: > On Tue 2016-03-01 22:36:12, Heiner Kallweit wrote: >> 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 >> --- >> 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(+) >> >> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >> index 8a3748a..a4b144e 100644 >> --- a/drivers/leds/led-class.c >> +++ b/drivers/leds/led-class.c >> @@ -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. > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] leds: core: add support for RGB LED's
On Tue 2016-03-01 22:36:12, Heiner Kallweit wrote: > 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 > --- > 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(+) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 8a3748a..a4b144e 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -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 -- (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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] leds: core: add support for RGB LED's
Am 04.03.2016 um 10:05 schrieb Jacek Anaszewski: > On 03/01/2016 10:36 PM, Heiner Kallweit wrote: >> 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 >> --- >> 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(+) >> >> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >> index 8a3748a..a4b144e 100644 >> --- a/drivers/leds/led-class.c >> +++ b/drivers/leds/led-class.c >> @@ -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? > 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". >> +WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get); >> + > > Could you move this warning to the line 216? > >> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); >> if (ret < 0) >> return ret; >> diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c >> index f6591f1..2b18d4c 100644 >> --- a/drivers/leds/led-rgb-core.c >> +++ b/drivers/leds/led-rgb-core.c >> @@ -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); >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index bbf24bb..82b3477 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -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 >>*/ >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] leds: core: add support for RGB LED's
On 03/01/2016 10:36 PM, Heiner Kallweit wrote: 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 --- 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(+) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 8a3748a..a4b144e 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -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? + WARN_ON(led_cdev->flags & LED_DEV_CAP_RGB && led_cdev->brightness_get); + Could you move this warning to the line 216? ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); if (ret < 0) return ret; diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c index f6591f1..2b18d4c 100644 --- a/drivers/leds/led-rgb-core.c +++ b/drivers/leds/led-rgb-core.c @@ -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); diff --git a/include/linux/leds.h b/include/linux/leds.h index bbf24bb..82b3477 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -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 */ -- Best regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 --- 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(+) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 8a3748a..a4b144e 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -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; diff --git a/drivers/leds/led-rgb-core.c b/drivers/leds/led-rgb-core.c index f6591f1..2b18d4c 100644 --- a/drivers/leds/led-rgb-core.c +++ b/drivers/leds/led-rgb-core.c @@ -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); diff --git a/include/linux/leds.h b/include/linux/leds.h index bbf24bb..82b3477 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -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 */ -- 2.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html