* [PATCH 2/2] leds: core: add support for RGB LED's @ 2016-02-07 10:45 Heiner Kallweit 2016-02-12 16:12 ` Jacek Anaszewski 0 siblings, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2016-02-07 10:45 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: linux-leds Add support for RGB LED's. Flag LED_DEV_CAP_RGB is used to instruct the core to convert HSV to RGB on output. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/leds/led-class.c | 3 +++ drivers/leds/led-core.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- include/linux/leds.h | 1 + 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 18a4558..f04efd2 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -193,6 +193,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) char name[64]; int ret; + if (led_cdev->flags & LED_DEV_CAP_RGB) + led_cdev->flags |= LED_DEV_CAP_COLOR; + ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); if (ret < 0) return ret; diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index 798e31e..6e1adc5 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -45,12 +45,50 @@ static inline enum led_brightness to_hsv(struct led_classdev *cdev, return ret | min(value & LED_BRIGHTNESS_MASK, cdev->max_brightness); } +static enum led_brightness 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; +} + static int led_set_output(struct led_classdev *cdev, enum led_brightness value) { if (!cdev->brightness_set) return -ENOTSUPP; + if (cdev->flags & LED_DEV_CAP_RGB) + value = hsv_to_rgb(value); + cdev->brightness_set(cdev, value); return 0; @@ -62,6 +100,9 @@ static int led_set_output_blocking(struct led_classdev *cdev, if (!cdev->brightness_set_blocking) return -ENOTSUPP; + if (cdev->flags & LED_DEV_CAP_RGB) + value = hsv_to_rgb(value); + return cdev->brightness_set_blocking(cdev, value); } @@ -320,7 +361,12 @@ int led_update_brightness(struct led_classdev *led_cdev) { int ret = 0; - if (led_cdev->brightness_get) { + /* + * 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 + */ + if (led_cdev->brightness_get && !(led_cdev->flags & LED_DEV_CAP_RGB)) { ret = led_cdev->brightness_get(led_cdev); if (ret >= 0) { led_cdev->brightness = ret; diff --git a/include/linux/leds.h b/include/linux/leds.h index 8e7db72..24eae4b 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -51,6 +51,7 @@ struct led_classdev { #define LED_DEV_CAP_FLASH (1 << 23) #define LED_HW_PLUGGABLE (1 << 24) #define LED_DEV_CAP_COLOR (1 << 25) +#define LED_DEV_CAP_RGB (1 << 26) /* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers -- 2.7.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] leds: core: add support for RGB LED's 2016-02-07 10:45 [PATCH 2/2] leds: core: add support for RGB LED's Heiner Kallweit @ 2016-02-12 16:12 ` Jacek Anaszewski 2016-02-13 18:31 ` Heiner Kallweit 0 siblings, 1 reply; 6+ messages in thread From: Jacek Anaszewski @ 2016-02-12 16:12 UTC (permalink / raw) To: Heiner Kallweit; +Cc: linux-leds On 02/07/2016 11:45 AM, Heiner Kallweit wrote: > Add support for RGB LED's. Flag LED_DEV_CAP_RGB is used to instruct > the core to convert HSV to RGB on output. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/leds/led-class.c | 3 +++ > drivers/leds/led-core.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- > include/linux/leds.h | 1 + > 3 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 18a4558..f04efd2 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -193,6 +193,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) > char name[64]; > int ret; > > + if (led_cdev->flags & LED_DEV_CAP_RGB) > + led_cdev->flags |= LED_DEV_CAP_COLOR; > + > ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); > if (ret < 0) > return ret; > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index 798e31e..6e1adc5 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -45,12 +45,50 @@ static inline enum led_brightness to_hsv(struct led_classdev *cdev, > return ret | min(value & LED_BRIGHTNESS_MASK, cdev->max_brightness); > } > > +static enum led_brightness 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; > +} > + This can be moved to led-hvs-core.c. Could you also share a reference to the algorithm of hsv -> rgb conversion you implemented here? > static int led_set_output(struct led_classdev *cdev, > enum led_brightness value) > { > if (!cdev->brightness_set) > return -ENOTSUPP; > > + if (cdev->flags & LED_DEV_CAP_RGB) > + value = hsv_to_rgb(value); > + > cdev->brightness_set(cdev, value); > > return 0; > @@ -62,6 +100,9 @@ static int led_set_output_blocking(struct led_classdev *cdev, > if (!cdev->brightness_set_blocking) > return -ENOTSUPP; > > + if (cdev->flags & LED_DEV_CAP_RGB) > + value = hsv_to_rgb(value); > + > return cdev->brightness_set_blocking(cdev, value); > } > > @@ -320,7 +361,12 @@ int led_update_brightness(struct led_classdev *led_cdev) > { > int ret = 0; > > - if (led_cdev->brightness_get) { > + /* > + * 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 > + */ > + if (led_cdev->brightness_get && !(led_cdev->flags & LED_DEV_CAP_RGB)) { > ret = led_cdev->brightness_get(led_cdev); > if (ret >= 0) { > led_cdev->brightness = ret; > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 8e7db72..24eae4b 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -51,6 +51,7 @@ struct led_classdev { > #define LED_DEV_CAP_FLASH (1 << 23) > #define LED_HW_PLUGGABLE (1 << 24) > #define LED_DEV_CAP_COLOR (1 << 25) > +#define LED_DEV_CAP_RGB (1 << 26) Why two flags - COLOR and RGB? Their meaning is ambiguous. > > /* Set LED brightness level > * Must not sleep. Use brightness_set_blocking for drivers > -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] leds: core: add support for RGB LED's 2016-02-12 16:12 ` Jacek Anaszewski @ 2016-02-13 18:31 ` Heiner Kallweit 2016-02-15 10:19 ` Jacek Anaszewski 0 siblings, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2016-02-13 18:31 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: linux-leds Am 12.02.2016 um 17:12 schrieb Jacek Anaszewski: > On 02/07/2016 11:45 AM, Heiner Kallweit wrote: >> Add support for RGB LED's. Flag LED_DEV_CAP_RGB is used to instruct >> the core to convert HSV to RGB on output. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/leds/led-class.c | 3 +++ >> drivers/leds/led-core.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- >> include/linux/leds.h | 1 + >> 3 files changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >> index 18a4558..f04efd2 100644 >> --- a/drivers/leds/led-class.c >> +++ b/drivers/leds/led-class.c >> @@ -193,6 +193,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) >> char name[64]; >> int ret; >> >> + if (led_cdev->flags & LED_DEV_CAP_RGB) >> + led_cdev->flags |= LED_DEV_CAP_COLOR; >> + >> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); >> if (ret < 0) >> return ret; >> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >> index 798e31e..6e1adc5 100644 >> --- a/drivers/leds/led-core.c >> +++ b/drivers/leds/led-core.c >> @@ -45,12 +45,50 @@ static inline enum led_brightness to_hsv(struct led_classdev *cdev, >> return ret | min(value & LED_BRIGHTNESS_MASK, cdev->max_brightness); >> } >> >> +static enum led_brightness 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; >> +} >> + > > This can be moved to led-hvs-core.c. Could you also share a reference > to the algorithm of hsv -> rgb conversion you implemented here? > OK, will move it. The HSV -> RGB algorithm is the one from Gonzalez / Woods and I took it from the German wikipedia. https://de.wikipedia.org/wiki/HSV-Farbraum#Umrechnung_HSV_in_RGB The calculation was slightly adjusted to deal with intervals 0-255 instead of [0,1] and for hue 0-251 instead of [0, 360) >> static int led_set_output(struct led_classdev *cdev, >> enum led_brightness value) >> { >> if (!cdev->brightness_set) >> return -ENOTSUPP; >> >> + if (cdev->flags & LED_DEV_CAP_RGB) >> + value = hsv_to_rgb(value); >> + >> cdev->brightness_set(cdev, value); >> >> return 0; >> @@ -62,6 +100,9 @@ static int led_set_output_blocking(struct led_classdev *cdev, >> if (!cdev->brightness_set_blocking) >> return -ENOTSUPP; >> >> + if (cdev->flags & LED_DEV_CAP_RGB) >> + value = hsv_to_rgb(value); >> + >> return cdev->brightness_set_blocking(cdev, value); >> } >> >> @@ -320,7 +361,12 @@ int led_update_brightness(struct led_classdev *led_cdev) >> { >> int ret = 0; >> >> - if (led_cdev->brightness_get) { >> + /* >> + * 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 >> + */ >> + if (led_cdev->brightness_get && !(led_cdev->flags & LED_DEV_CAP_RGB)) { >> ret = led_cdev->brightness_get(led_cdev); >> if (ret >= 0) { >> led_cdev->brightness = ret; >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index 8e7db72..24eae4b 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -51,6 +51,7 @@ struct led_classdev { >> #define LED_DEV_CAP_FLASH (1 << 23) >> #define LED_HW_PLUGGABLE (1 << 24) >> #define LED_DEV_CAP_COLOR (1 << 25) >> +#define LED_DEV_CAP_RGB (1 << 26) > > Why two flags - COLOR and RGB? Their meaning is ambiguous. > There might be color LED's with HSV interface and setting the COLOR flag but not the RGB flag would support such LED's w/o additional code. Not sure whether such devices exist, but one additional flag might be a small enough price for decoupling internal color handling and RGB output conversion. >> >> /* Set LED brightness level >> * Must not sleep. Use brightness_set_blocking for drivers >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] leds: core: add support for RGB LED's 2016-02-13 18:31 ` Heiner Kallweit @ 2016-02-15 10:19 ` Jacek Anaszewski 2016-02-15 20:40 ` Heiner Kallweit 0 siblings, 1 reply; 6+ messages in thread From: Jacek Anaszewski @ 2016-02-15 10:19 UTC (permalink / raw) To: Heiner Kallweit; +Cc: linux-leds On 02/13/2016 07:31 PM, Heiner Kallweit wrote: > Am 12.02.2016 um 17:12 schrieb Jacek Anaszewski: >> On 02/07/2016 11:45 AM, Heiner Kallweit wrote: >>> Add support for RGB LED's. Flag LED_DEV_CAP_RGB is used to instruct >>> the core to convert HSV to RGB on output. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> drivers/leds/led-class.c | 3 +++ >>> drivers/leds/led-core.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- >>> include/linux/leds.h | 1 + >>> 3 files changed, 51 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >>> index 18a4558..f04efd2 100644 >>> --- a/drivers/leds/led-class.c >>> +++ b/drivers/leds/led-class.c >>> @@ -193,6 +193,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) >>> char name[64]; >>> int ret; >>> >>> + if (led_cdev->flags & LED_DEV_CAP_RGB) >>> + led_cdev->flags |= LED_DEV_CAP_COLOR; >>> + >>> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); >>> if (ret < 0) >>> return ret; >>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>> index 798e31e..6e1adc5 100644 >>> --- a/drivers/leds/led-core.c >>> +++ b/drivers/leds/led-core.c >>> @@ -45,12 +45,50 @@ static inline enum led_brightness to_hsv(struct led_classdev *cdev, >>> return ret | min(value & LED_BRIGHTNESS_MASK, cdev->max_brightness); >>> } >>> >>> +static enum led_brightness 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; >>> +} >>> + >> >> This can be moved to led-hvs-core.c. Could you also share a reference >> to the algorithm of hsv -> rgb conversion you implemented here? >> > OK, will move it. The HSV -> RGB algorithm is the one from Gonzalez / Woods > and I took it from the German wikipedia. > https://de.wikipedia.org/wiki/HSV-Farbraum#Umrechnung_HSV_in_RGB > The calculation was slightly adjusted to deal with intervals > 0-255 instead of [0,1] and for hue 0-251 instead of [0, 360) Thanks for the link. It is possible that I'll have some comments to this piece of code after getting acquainted with the subject, but it may take a while. >>> static int led_set_output(struct led_classdev *cdev, >>> enum led_brightness value) >>> { >>> if (!cdev->brightness_set) >>> return -ENOTSUPP; >>> >>> + if (cdev->flags & LED_DEV_CAP_RGB) >>> + value = hsv_to_rgb(value); >>> + >>> cdev->brightness_set(cdev, value); >>> >>> return 0; >>> @@ -62,6 +100,9 @@ static int led_set_output_blocking(struct led_classdev *cdev, >>> if (!cdev->brightness_set_blocking) >>> return -ENOTSUPP; >>> >>> + if (cdev->flags & LED_DEV_CAP_RGB) >>> + value = hsv_to_rgb(value); >>> + >>> return cdev->brightness_set_blocking(cdev, value); >>> } >>> >>> @@ -320,7 +361,12 @@ int led_update_brightness(struct led_classdev *led_cdev) >>> { >>> int ret = 0; >>> >>> - if (led_cdev->brightness_get) { >>> + /* >>> + * 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 >>> + */ >>> + if (led_cdev->brightness_get && !(led_cdev->flags & LED_DEV_CAP_RGB)) { >>> ret = led_cdev->brightness_get(led_cdev); >>> if (ret >= 0) { >>> led_cdev->brightness = ret; >>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>> index 8e7db72..24eae4b 100644 >>> --- a/include/linux/leds.h >>> +++ b/include/linux/leds.h >>> @@ -51,6 +51,7 @@ struct led_classdev { >>> #define LED_DEV_CAP_FLASH (1 << 23) >>> #define LED_HW_PLUGGABLE (1 << 24) >>> #define LED_DEV_CAP_COLOR (1 << 25) >>> +#define LED_DEV_CAP_RGB (1 << 26) >> >> Why two flags - COLOR and RGB? Their meaning is ambiguous. >> > There might be color LED's with HSV interface and setting the COLOR flag > but not the RGB flag would support such LED's w/o additional code. > Not sure whether such devices exist, but one additional flag might be a > small enough price for decoupling internal color handling and RGB output > conversion. For now let's keep the code as simple as possible. We can always add the flag when needed. Please also rename LED_DEV_CAP_COLOR to LED_DEV_CAP_HSV. We'll also have to change the type of brightness and max_brightness properties of struct led_classdev to u32. By default it is signed int type, but using -fshort-enums GCC option causes the smallest possible type to be used [1]. >>> >>> /* Set LED brightness level >>> * Must not sleep. Use brightness_set_blocking for drivers >>> >> >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-leds" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > [1] http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Enumerations -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] leds: core: add support for RGB LED's 2016-02-15 10:19 ` Jacek Anaszewski @ 2016-02-15 20:40 ` Heiner Kallweit 2016-02-16 9:09 ` Jacek Anaszewski 0 siblings, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2016-02-15 20:40 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: linux-leds Am 15.02.2016 um 11:19 schrieb Jacek Anaszewski: > On 02/13/2016 07:31 PM, Heiner Kallweit wrote: >> Am 12.02.2016 um 17:12 schrieb Jacek Anaszewski: >>> On 02/07/2016 11:45 AM, Heiner Kallweit wrote: >>>> Add support for RGB LED's. Flag LED_DEV_CAP_RGB is used to instruct >>>> the core to convert HSV to RGB on output. >>>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>> --- >>>> drivers/leds/led-class.c | 3 +++ >>>> drivers/leds/led-core.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- >>>> include/linux/leds.h | 1 + >>>> 3 files changed, 51 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >>>> index 18a4558..f04efd2 100644 >>>> --- a/drivers/leds/led-class.c >>>> +++ b/drivers/leds/led-class.c >>>> @@ -193,6 +193,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) >>>> char name[64]; >>>> int ret; >>>> >>>> + if (led_cdev->flags & LED_DEV_CAP_RGB) >>>> + led_cdev->flags |= LED_DEV_CAP_COLOR; >>>> + >>>> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); >>>> if (ret < 0) >>>> return ret; >>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>> index 798e31e..6e1adc5 100644 >>>> --- a/drivers/leds/led-core.c >>>> +++ b/drivers/leds/led-core.c >>>> @@ -45,12 +45,50 @@ static inline enum led_brightness to_hsv(struct led_classdev *cdev, >>>> return ret | min(value & LED_BRIGHTNESS_MASK, cdev->max_brightness); >>>> } >>>> >>>> +static enum led_brightness 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; >>>> +} >>>> + >>> >>> This can be moved to led-hvs-core.c. Could you also share a reference >>> to the algorithm of hsv -> rgb conversion you implemented here? >>> >> OK, will move it. The HSV -> RGB algorithm is the one from Gonzalez / Woods >> and I took it from the German wikipedia. >> https://de.wikipedia.org/wiki/HSV-Farbraum#Umrechnung_HSV_in_RGB >> The calculation was slightly adjusted to deal with intervals >> 0-255 instead of [0,1] and for hue 0-251 instead of [0, 360) > > Thanks for the link. It is possible that I'll have some comments to this > piece of code after getting acquainted with the subject, but it > may take a while. > >>>> static int led_set_output(struct led_classdev *cdev, >>>> enum led_brightness value) >>>> { >>>> if (!cdev->brightness_set) >>>> return -ENOTSUPP; >>>> >>>> + if (cdev->flags & LED_DEV_CAP_RGB) >>>> + value = hsv_to_rgb(value); >>>> + >>>> cdev->brightness_set(cdev, value); >>>> >>>> return 0; >>>> @@ -62,6 +100,9 @@ static int led_set_output_blocking(struct led_classdev *cdev, >>>> if (!cdev->brightness_set_blocking) >>>> return -ENOTSUPP; >>>> >>>> + if (cdev->flags & LED_DEV_CAP_RGB) >>>> + value = hsv_to_rgb(value); >>>> + >>>> return cdev->brightness_set_blocking(cdev, value); >>>> } >>>> >>>> @@ -320,7 +361,12 @@ int led_update_brightness(struct led_classdev *led_cdev) >>>> { >>>> int ret = 0; >>>> >>>> - if (led_cdev->brightness_get) { >>>> + /* >>>> + * 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 >>>> + */ >>>> + if (led_cdev->brightness_get && !(led_cdev->flags & LED_DEV_CAP_RGB)) { >>>> ret = led_cdev->brightness_get(led_cdev); >>>> if (ret >= 0) { >>>> led_cdev->brightness = ret; >>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>> index 8e7db72..24eae4b 100644 >>>> --- a/include/linux/leds.h >>>> +++ b/include/linux/leds.h >>>> @@ -51,6 +51,7 @@ struct led_classdev { >>>> #define LED_DEV_CAP_FLASH (1 << 23) >>>> #define LED_HW_PLUGGABLE (1 << 24) >>>> #define LED_DEV_CAP_COLOR (1 << 25) >>>> +#define LED_DEV_CAP_RGB (1 << 26) >>> >>> Why two flags - COLOR and RGB? Their meaning is ambiguous. >>> >> There might be color LED's with HSV interface and setting the COLOR flag >> but not the RGB flag would support such LED's w/o additional code. >> Not sure whether such devices exist, but one additional flag might be a >> small enough price for decoupling internal color handling and RGB output >> conversion. > > For now let's keep the code as simple as possible. We can always add > the flag when needed. > > Please also rename LED_DEV_CAP_COLOR to LED_DEV_CAP_HSV. > > We'll also have to change the type of brightness and max_brightness > properties of struct led_classdev to u32. By default it is signed int > type, but using -fshort-enums GCC option causes the smallest possible > type to be used [1]. > Interesting. I think we want to avoid having to change existing drivers, therefore the callbacks in struct led_classdev will have to take enum led_brightness arguments also further on. But we need it to be at least 32 bit for the hsv extension. To ensure this (even if short-enums is used) we could add a dummy value of 0x70000000 to the enum. What do you think? > >>>> >>>> /* Set LED brightness level >>>> * Must not sleep. Use brightness_set_blocking for drivers >>>> >>> >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-leds" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > > [1] http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Enumerations > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] leds: core: add support for RGB LED's 2016-02-15 20:40 ` Heiner Kallweit @ 2016-02-16 9:09 ` Jacek Anaszewski 0 siblings, 0 replies; 6+ messages in thread From: Jacek Anaszewski @ 2016-02-16 9:09 UTC (permalink / raw) To: Heiner Kallweit; +Cc: linux-leds On 02/15/2016 09:40 PM, Heiner Kallweit wrote: > Am 15.02.2016 um 11:19 schrieb Jacek Anaszewski: >> On 02/13/2016 07:31 PM, Heiner Kallweit wrote: >>> Am 12.02.2016 um 17:12 schrieb Jacek Anaszewski: >>>> On 02/07/2016 11:45 AM, Heiner Kallweit wrote: >>>>> Add support for RGB LED's. Flag LED_DEV_CAP_RGB is used to instruct >>>>> the core to convert HSV to RGB on output. >>>>> >>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>>> --- >>>>> drivers/leds/led-class.c | 3 +++ >>>>> drivers/leds/led-core.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- >>>>> include/linux/leds.h | 1 + >>>>> 3 files changed, 51 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >>>>> index 18a4558..f04efd2 100644 >>>>> --- a/drivers/leds/led-class.c >>>>> +++ b/drivers/leds/led-class.c >>>>> @@ -193,6 +193,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) >>>>> char name[64]; >>>>> int ret; >>>>> >>>>> + if (led_cdev->flags & LED_DEV_CAP_RGB) >>>>> + led_cdev->flags |= LED_DEV_CAP_COLOR; >>>>> + >>>>> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); >>>>> if (ret < 0) >>>>> return ret; >>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>> index 798e31e..6e1adc5 100644 >>>>> --- a/drivers/leds/led-core.c >>>>> +++ b/drivers/leds/led-core.c >>>>> @@ -45,12 +45,50 @@ static inline enum led_brightness to_hsv(struct led_classdev *cdev, >>>>> return ret | min(value & LED_BRIGHTNESS_MASK, cdev->max_brightness); >>>>> } >>>>> >>>>> +static enum led_brightness 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; >>>>> +} >>>>> + >>>> >>>> This can be moved to led-hvs-core.c. Could you also share a reference >>>> to the algorithm of hsv -> rgb conversion you implemented here? >>>> >>> OK, will move it. The HSV -> RGB algorithm is the one from Gonzalez / Woods >>> and I took it from the German wikipedia. >>> https://de.wikipedia.org/wiki/HSV-Farbraum#Umrechnung_HSV_in_RGB >>> The calculation was slightly adjusted to deal with intervals >>> 0-255 instead of [0,1] and for hue 0-251 instead of [0, 360) >> >> Thanks for the link. It is possible that I'll have some comments to this >> piece of code after getting acquainted with the subject, but it >> may take a while. >> >>>>> static int led_set_output(struct led_classdev *cdev, >>>>> enum led_brightness value) >>>>> { >>>>> if (!cdev->brightness_set) >>>>> return -ENOTSUPP; >>>>> >>>>> + if (cdev->flags & LED_DEV_CAP_RGB) >>>>> + value = hsv_to_rgb(value); >>>>> + >>>>> cdev->brightness_set(cdev, value); >>>>> >>>>> return 0; >>>>> @@ -62,6 +100,9 @@ static int led_set_output_blocking(struct led_classdev *cdev, >>>>> if (!cdev->brightness_set_blocking) >>>>> return -ENOTSUPP; >>>>> >>>>> + if (cdev->flags & LED_DEV_CAP_RGB) >>>>> + value = hsv_to_rgb(value); >>>>> + >>>>> return cdev->brightness_set_blocking(cdev, value); >>>>> } >>>>> >>>>> @@ -320,7 +361,12 @@ int led_update_brightness(struct led_classdev *led_cdev) >>>>> { >>>>> int ret = 0; >>>>> >>>>> - if (led_cdev->brightness_get) { >>>>> + /* >>>>> + * 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 >>>>> + */ >>>>> + if (led_cdev->brightness_get && !(led_cdev->flags & LED_DEV_CAP_RGB)) { >>>>> ret = led_cdev->brightness_get(led_cdev); >>>>> if (ret >= 0) { >>>>> led_cdev->brightness = ret; >>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>>> index 8e7db72..24eae4b 100644 >>>>> --- a/include/linux/leds.h >>>>> +++ b/include/linux/leds.h >>>>> @@ -51,6 +51,7 @@ struct led_classdev { >>>>> #define LED_DEV_CAP_FLASH (1 << 23) >>>>> #define LED_HW_PLUGGABLE (1 << 24) >>>>> #define LED_DEV_CAP_COLOR (1 << 25) >>>>> +#define LED_DEV_CAP_RGB (1 << 26) >>>> >>>> Why two flags - COLOR and RGB? Their meaning is ambiguous. >>>> >>> There might be color LED's with HSV interface and setting the COLOR flag >>> but not the RGB flag would support such LED's w/o additional code. >>> Not sure whether such devices exist, but one additional flag might be a >>> small enough price for decoupling internal color handling and RGB output >>> conversion. >> >> For now let's keep the code as simple as possible. We can always add >> the flag when needed. >> >> Please also rename LED_DEV_CAP_COLOR to LED_DEV_CAP_HSV. >> >> We'll also have to change the type of brightness and max_brightness >> properties of struct led_classdev to u32. By default it is signed int >> type, but using -fshort-enums GCC option causes the smallest possible >> type to be used [1]. >> > Interesting. I think we want to avoid having to change existing drivers, > therefore the callbacks in struct led_classdev will have to take enum > led_brightness arguments also further on. > But we need it to be at least 32 bit for the hsv extension. To ensure this > (even if short-enums is used) we could add a dummy value of 0x70000000 > to the enum. What do you think? Originally the type was int. It was changed to enum led_brightness with patch [1], but it wasn't a good move, which turns out now. With -fshort-enums and current definition of enum led_brightness gcc can choose char for brightness type. This is clear bug because default maximum allowed brightness value can be overridden with max_brightness property. With -fshort-enums we end up with truncated brightness for levels over 255. Brightness value can be truncated also when passing to LED API. I agree that adding an enum value requiring four bytes seems to be the best solution. I'd prefer that the value was 0xffffffff, and a suitable comment should be placed next to it, explaining why LED_FULL is not the last value in the enum. [1] http://www.spinics.net/lists/linux-leds/msg02070.html -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-16 9:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-07 10:45 [PATCH 2/2] leds: core: add support for RGB LED's Heiner Kallweit 2016-02-12 16:12 ` Jacek Anaszewski 2016-02-13 18:31 ` Heiner Kallweit 2016-02-15 10:19 ` Jacek Anaszewski 2016-02-15 20:40 ` Heiner Kallweit 2016-02-16 9:09 ` Jacek Anaszewski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).