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

[Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Am 29.02.2016 um 10:57 schrieb Jacek Anaszewski:
> On 02/26/2016 11:36 PM, Heiner Kallweit wrote:
>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>> ---
>>>>>> 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.
> 
> USB maintainer could only ack it and I would merge it through the
> LED tree. I propose to cc the whole patch set also to linux-usb list,
> which would also have the benefit that more people could look at
> the whole idea of using hsv interface for RGB LED. The traffic on
> linux-leds isn't too heavy. Please cc the patch set also to linux-kernel
> next time.
> 
OK, I cc Jiri as HID USB maintainer and will send the next version of the patch series
also to linux-usb and linux-kernel.

>>>>>> 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)
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html