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

[Heiner Kallweit <hkallweit1@gmail.com>]

Am 25.02.2016 um 13:40 schrieb Jacek Anaszewski:
> On 02/23/2016 09:27 PM, Heiner Kallweit wrote:
>> Am 19.02.2016 um 14:59 schrieb Jacek Anaszewski:
>>> Hi Heiner,
>>>
>>> On 02/18/2016 10:31 PM, Heiner Kallweit wrote:
>>>> Add support for RGB LED's. Flag LED_DEV_CAP_HSV is used to instruct
>>>> the core to convert HSV to RGB on output.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> 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.
>>>> ---
>>>>    drivers/leds/led-color-core.c | 35 +++++++++++++++++++++++++++++++++++
>>>>    drivers/leds/led-core.c       | 17 ++++++++++++++++-
>>>>    drivers/leds/leds.h           |  1 +
>>>>    3 files changed, 52 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/leds/led-color-core.c b/drivers/leds/led-color-core.c
>>>> index b101f73..467beeb 100644
>>>> --- a/drivers/leds/led-color-core.c
>>>> +++ b/drivers/leds/led-color-core.c
>>>> @@ -31,3 +31,38 @@ 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;
>>>> +}
>>>
>>> Do you plan to add a driver using Color LEDs Support?
>>> If so, it would be good to add it to this patch set,
>>> which would make testing this code easier.
>>>
>> I own a Thingm Blink(1) dual RGB LED USB device which I use for testing.
>> With the RGB extension in the kernel the driver could be significantly simplified
>> so that the changes are more or less a rewrite of the driver.
>> Because the USB device uses a HID interface the driver currently is located
>> under drivers/hid. IMHO that's questionable because the driver implements
>> LED functionality only. Moving this driver to drivers/leds could be an option.
>> When sending the patches for this driver to you for testing I'll set the maintainer
>> of the HID subsystem on cc.
> 
> But it will be still USB driver. Please target the driver for usb
> subsystem and add myself on cc.
> 
I'm afraid the maintainer of the HID subsystem will reject the driver patches because
he can't even apply them as the RGB LED extension is not yet in his tree.
Would it make sense that I send the driver patches to you first for testing the
RGB extension (not as official patches)?
If everything goes well and the RGB extension shows up in the linux-next tree eventually
then the driver changes can be submitted officially to the HID maintainer.

>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>>> index 525d566..255bdd7 100644
>>>> --- a/drivers/leds/led-core.c
>>>> +++ b/drivers/leds/led-core.c
>>>> @@ -31,6 +31,11 @@ static int __led_set_brightness(struct led_classdev *led_cdev,
>>>>        if (!led_cdev->brightness_set)
>>>>            return -ENOTSUPP;
>>>>
>>>> +#if IS_ENABLED(CONFIG_LEDS_COLOR)
>>>> +    if (led_cdev->flags & LED_DEV_CAP_HSV)
>>>> +        value = led_hsv_to_rgb(value);
>>>> +#endif
>>>> +
>>>
>>> Please drop this changes. led_hsv_to_rgb() can be used internally
>>> by RGB LED class drivers.
>>>
>> OK
>>
>>>>        led_cdev->brightness_set(led_cdev, value);
>>>>
>>>>        return 0;
>>>> @@ -42,6 +47,11 @@ static int __led_set_brightness_blocking(struct led_classdev *led_cdev,
>>>>        if (!led_cdev->brightness_set_blocking)
>>>>            return -ENOTSUPP;
>>>>
>>>> +#if IS_ENABLED(CONFIG_LEDS_COLOR)
>>>> +    if (led_cdev->flags & LED_DEV_CAP_HSV)
>>>> +        value = led_hsv_to_rgb(value);
>>>> +#endif
>>>> +
>>>
>>> Ditto.
>>>
>>>>        return led_cdev->brightness_set_blocking(led_cdev, value);
>>>>    }
>>>>
>>>> @@ -294,7 +304,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_HSV)) {
>>>>            ret = led_cdev->brightness_get(led_cdev);
>>>>            if (ret >= 0) {
>>>>                led_cdev->brightness = ret;
>>>> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
>>>> index 094707f..763f19a 100644
>>>> --- a/drivers/leds/leds.h
>>>> +++ b/drivers/leds/leds.h
>>>> @@ -37,6 +37,7 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev,
>>>>    #if IS_ENABLED(CONFIG_LEDS_COLOR)
>>>>    enum led_brightness led_confine_brightness(struct led_classdev *led_cdev,
>>>>                            enum led_brightness value);
>>>> +enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
>>>>    #else
>>>>    static inline enum led_brightness led_confine_brightness(
>>>>            struct led_classdev *led_cdev, enum led_brightness value)
>>>>
>>>
>>>
>>
>>
>>
> 
>