Am 25.01.2016 um 11:52 schrieb Jacek Anaszewski:
> On 01/25/2016 10:51 AM, Heiner Kallweit wrote:
>> On Mon, Jan 25, 2016 at 9:41 AM, Jacek Anaszewski
>> <j.anaszewski@xxxxxxxxxxx> wrote:
>>> Hi Heiner,
>>>
>>>
>>> On 01/24/2016 02:38 PM, Heiner Kallweit wrote:
>>>>
>>>> Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:
>>>>>
>>>>> On 01/15/2016 09:16 PM, Heiner Kallweit wrote:
>>>>>>
>>>>>> Am 14.01.2016 um 13:08 schrieb Jacek Anaszewski:
>>>>>>>
>>>>>>> On 01/10/2016 09:27 PM, Heiner Kallweit wrote:
>>>>>>>>
>>>>>>>> When playing with a ThingM Blink(1) USB RGB LED device I found that
>>>>>>>> there
>>>>>>>> are few drivers for RGB LED's spread across the kernel and each one
>>>>>>>> implements the RGB functionality in its own way.
>>>>>>>> Some examples:
>>>>>>>> - drivers/hid/hid-thingm.c
>>>>>>>> - drivers/usb/misc/usbled.c
>>>>>>>> - drivers/leds/leds-bd2802.c
>>>>>>>> - drivers/leds/leds-blinkm.c
>>>>>>>> ...
>>>>>>>> IMHO it would make sense to add generic RGB functionality to the LED
>>>>>>>> core.
>>>>>>>>
>>>>>>>> Things I didn't like too much in other driver implementations:
>>>>>>>> - one led_classdev per color or at least
>>>>>>>> - one sysfs attribute per color
>>>>>>>> Colors are not really independent therefore I'd prefer one
>>>>>>>> led_classdev
>>>>>>>> per RGB LED. Also userspace should be able to change the color with
>>>>>>>> one
>>>>>>>> call -> therefore only one sysfs attribute for the RGB values.
>>>>>>>>
>>>>>>>> Also the current brightness-related functionality should not be
>>>>>>>> effected
>>>>>>>> by the RGB extension.
>>>>>>>>
>>>>>>>> This patch is intended to demonstrate the idea of the extension. Most
>>>>>>>> likely
>>>>>>>> it's not ready yet for submission.
>>>>>>>>
>>>>>>>> Main idea is to base the effective RGB value on a combination of
>>>>>>>> brightness
>>>>>>>> and a scaled-to-max RGB value.
>>>>>>>> The RGB-related callbacks are basically the same as for brightness.
>>>>>>>> RGB functionally can be switched on with a new flag in the
>>>>>>>> led_classdev.flags
>>>>>>>> bitmap.
>>>>>>>>
>>>>>>>> Experimentally I changed the thingm driver to use this extension and
>>>>>>>> it works
>>>>>>>> quite well. As one result the driver could be very much simplified.
>>>>>>>>
>>>>>>>> Any feedback is appreciated.
>>>>>>>>
>>>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>>>>>> ---
>>>>>>>> drivers/leds/led-class.c | 62 +++++++++++++++++++++++++++
>>>>>>>> drivers/leds/led-core.c | 106
>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>> drivers/leds/leds.h | 12 ++++++
>>>>>>>> include/linux/leds.h | 13 ++++++
>>>>>>>> 4 files changed, 193 insertions(+)
>>>>>>>>
>>> [...]
>>>
>>>>>>>> +static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
>>>>>>>> +{
>>>>>>>> + u32 red_raw = (rgb >> 16) & 0xff;
>>>>>>>> + u32 green_raw = (rgb >> 8) & 0xff;
>>>>>>>> + u32 blue_raw = rgb & 0xff;
>>>>>>>> + u32 max_raw, red, green, blue;
>>>>>>>> +
>>>>>>>> + max_raw = max(red_raw, green_raw);
>>>>>>>> + if (blue_raw > max_raw)
>>>>>>>> + max_raw = blue_raw;
>>>>>>>> +
>>>>>>>> + if (!max_raw) {
>>>>>>>> + cdev->brightness = 0;
>>>>>>>> + cdev->rgb_val = 0;
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
>>>>>>>> + green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
>>>>>>>> + blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
>>>>>>>> +
>>>>>>>> + cdev->brightness = max_raw;
>>>>>>>> + cdev->rgb_val = (red << 16) + (green << 8) + blue;
>>>>>>>> +}
>>>>>>>
>>>>>>>
>>>>>>> I think that we shouldn't impose specific way of calculating brightness
>>>>>>> depending on the rgb value set. We should just pass value from
>>>>>>> userspace
>>>>>>> to the driver.
>>>>>>>
>>>>>> Right, setting brightness from userspace is no problem.
>>>>>> But how about led_update_brightness? It's supposed to update the
>>>>>> brightness
>>>>>> value in led_cdev with the actual value. And for a RGB LED we have only
>>>>>> the RGB values, so we need to calculate the brightness based on RGB
>>>>>> values.
>>>>>> Or do you see a better way?
>>>>>
>>>>>
>>>>> I thought that you had some device using both brightness and rgb
>>>>> components settings (no idea if this could be sane in any way).
>>>>> If there are only three color components, then why not just redefine
>>>>> brightness to store three components in case of the LEDs with
>>>>> LED_DEV_CAP_RGB flags set?
>>>>>
>>>> My devices just have the three color components (normal 8 bits per color).
>>>> Even in the new RGB mode I don't want to break the current
>>>> brightness-based API
>>>> but extend it with RGB functionality. Therefore I think it's best to store
>>>> brightness and color separately.
>>>> Just one argument: If we store the color components only then we'll
>>>> lose the color information if the brightness is set to 0.
>>>> And I would like to support e.g. software blink in whatever color.
>>>
>>>
>>> Currently blinking works properly and brightness value isn't being lost.
>>> We store it in the led_cdev->blink_brightness property. Could you
>>> present exact use case you'd like to support and which is not feasible
>>> with current LED core design?
>>>
>> I didn't have a closer look on how soft blinking works and you're right,
>> this was a bad example.
>> It's not about that something is wrong with the current LED core design
>> but that IMHO storing just the raw RGB values wouldn't be a good idea
>> and we should store brightness and color separately.
>>
>> So let's take another example:
>> I set a color via sysfs and a trigger is controling the LED. Once the LED
>> brightness is set to LED_OFF by the trigger and e.g. a sysfs read triggers
>> an update the stored RGB value is 0,0,0.
>> If the LED is switched on again, then which color to choose?
>
> The one stored in blink_brightness. I don't get how changing the
> interpretation of brightness value can affect anything here.
>
But blink_brightness is used in case of soft blink only.
If a trigger or userspace sets the brightness to zero then blink_brightness
isn't used.
I agreed that soft blink was a bad example.
>> That's why I'm saying we should store color and brightness separately to
>> avoid losing the color information information even if the brightness
>> is set to zero.
>>
>> Regards, Heiner
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
