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

[Jacek Anaszewski <j.anaszewski@samsung.com>]

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.

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


-- 
Best regards,
Jacek Anaszewski