Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver

[Dan Murphy <dmurphy@ti.com>]

Pavel

Thanks for trimming.

On 12/19/2018 04:27 PM, Pavel Machek wrote:
> Hi!
> 
> [cc list trimmed]
> 
>>>>> I don't think a user needs nor would want to have 24 different LED nodes with 24 different brightness files.
>>>>> Or with the LP5036 that would have 36 LED nodes.
>>>>>
>>>>> Table 1 in the data sheet shows how the outputs map to the control banks to the LED registers.
>>>>
>>>> Some time ago we had discussion with Vesa Jääskeläinen about possible
>>>> approaches to RGB LEDs [0]. What seemed to be the most suitable
>>>> variation of the discussed out-of-tree approach was the "color" property
>>>> and array of color triplets defined in Device Tree per each color.
>>>>
>>>> Please refer to [0] for the details.
>>>>
>>>> [0] https://lkml.org/lkml/2018/11/9/938
>>>
>>> Yes, plus I also have the set of HSV patches somewhere... and they
>>> work, but I found out that HSV->RGB conversion results in loss of precision.
>>>
>>> We may want to do something like that.
>>>
>>> But we need to do it once, in a driver core. We obviously don't want
>>> each driver having different version of RGB support.
>>
>> True.  But the RGB driver really should not be defining the color palette.
>>
>> Maybe a "color capability" reported to user space so that the user space can make the decision.
>>
>> It can report
>>
>> For GPIO or constant current situations
>> static red
>> static blue
>> static green 
>>
>> For adjustable configurations it can report:
>> variable red
>> variable blue
>> variable green
>>
>> or even simpler "static" or "dynamic" as a return to report the RGB configuration.
>> The LED driver would just need to set the variable.
> 
> I'm not sure what static/variable is.

My suggestion is really based in opinion.  It is my opinion that the user space sets the policy
of the RGB.  The DT nor the driver should limit the capability of what the user would want to do.

But instead should let the user know what the driver and HW can do.  It seems that the suggestion made in the
email thread [0] https://lkml.org/lkml/2018/11/9/938 limits that capability via the DT by defining a specific color.

If I have a configurable current RGB combo then the palette is endless.  But in a GPIO fixed current situation the palette is
limited.  So I don't think I agree with the DT side of it.

There needs to be a way that the kernel can indicate to the user space "heres what I can do you need to let me know".
As each kernel driver just needs to interact with the HW and not set policy.

> 
> I have RGB leds here that can set any channel to 0-255, at
> runtime. They seem to be quite common at phones.

Yes I agree.  Android specifically likes to set these values in the lighting HAL.

But alternatively once the color is set the brightness can be controlled by PWM or an ALS.

> 
> Anyway, if your 36 channels can be set independently, I believe you
> just want them to export as 36 LEDs.

I am not sure that is what the "customers" would want to have to set 36 different nodes or even declare those 36 nodes.
That is 36 independent user space to kernel space calls that are very expensive just to set a LED color and brightness.

Now that is very unrealistic that there would be 36 calls happening.  But for this part it
would cut the calls to set the brightness to the kernel in runtime after init to 1/3.  Set the color and then only one call for the brightness.

Dan

> 
> Thanks,
> 									Pavel
> 


-- 
------------------
Dan Murphy