Re: RGB LED class Re: [PATCH v2 2/2] leds: lp50xx: Add the LP50XX family of the RGB LED driver

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Pavel,

On 1/18/19 1:02 AM, Pavel Machek wrote:
> Hi!
> 
> 
>>> I am willing to work with you on the HSV and adapting the LP50xx part to this framework.
>>> Or any RGB framework for that matter.  I still don't agree with the kernel needing to declare colors
>>>   maybe color capabilities but not specific colors.
>>
>> Dan, if you have a bandwidth for LED RGB class implementation
>> then please go ahead. It would be good to compare colors produced
>> by software HSV->RGB algorithm to what can be achieved with
>> LEDn_BRIGHTNESS feature.
> 
> Don't get me wrong, I'd like to see LED RGB class implementation. But
> it will delay merge of this driver.
> 
> If we want to do that, we should first discuss the requirements, and
> then come up with interface.. and only then we can talk about the
> driver code.
> 
> That's why I believe preferable way would be to merge the driver using
> the existing interface.
> 
> Of course, first designing RGB LED class and then merging the
> driver.. is okay with me. But lets not rush the class because there's
> driver waiting for it.
> 
>> The requirements for LED RGB class as I would see it:
>>
>> sysfs interface:
>>
>> brightness-model: space separated list of available options:
>> - rgb (default):
>>    - creates color file with "red green blue" decimal values
>>    - creates brightness file
>> 	a) for devices with hardware support for adjusting color
>>             intensity it maps to corresponding register
>>          b) for the rest writing any value greater than 0 will result
>>             in setting all color registers to max
>> - hsv:
>>    - creates color file with "h s v" values - it shall
>>      use software HSV->RGB algorithm for setting color registers
>>
>> - any other custom color ranges defined in DT, but it can be covered
>>    later
>> - other options?
> 
> First, I think we want to decide if RGB LED should be presented as
> 3 LEDs or as 1 LED... and what to do with existing RGB leds being
> presented as 3 LEDs.
> 
> I don't think we want to support both RGB and HSV in the kernel. It is
> math, and not a nice one.
> 
> Yes, both have advantages and disadvantages, but having _both_ in
> kernel has disadvantages of both.
> 
> One way I could imagine the interface:
> 
>      RGB LED presented as one LED.
> 
>      brightness -- controls brightness of whole RGB module.

What algorithm would be used for mapping brightness levels to RGB values
in case of devices without hardware support for that?

>      pwm_channels -- "1000 240 300" -- "red part should be full on, green
>      should be pwm controlled to 240/1000, blue should be 300/1000"
> 
>      pwm_white -- "1000 500 400" -- tells userspace what to write to PWM
>      channels to get approximately white color.
> 
>      This would assume that RGB LEDs are always pwm controlled. That
>      seems to be true for hardware I seen.

Why pwm in the file names? I don't see any gain and only possible
problems. Many LED controllers use current level and not PWM
for driving LEDs.

Even mainline RGB LED driver: drivers/leds/leds-lp3952.c [0].

s/pwm/color/

Besides white also other color presets could be defined in DT.

>      + no complex math in kernel
> 
>      + userspace knows enough to display arbitrary colors
> 
>      + userspace can use full range of available PWM intensities
> 
>      + existing triggers will work nicely
> 
>      - userland needs to do non-trivial math to get colors it wants
> 
>      - not sure how to migrate existing devices
> 
> Thoughts? Other possible interfaces?


[0] https://www.mouser.com/ds/2/348/bd2802gu-e-210449.pdf

-- 
Best regards,
Jacek Anaszewski