Re: [PATCH 2/2] leds: core: add support for RGB LED's

[Heiner Kallweit <hkallweit1@gmail.com>]

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
>