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

[Dan Murphy <dmurphy@ti.com>]

Jacek

Thanks for the reply!

All

Happy New Year!

On 1/1/19 8:42 AM, Jacek Anaszewski wrote:
> On 12/31/18 8:15 PM, Dan Murphy wrote:
>> On 12/31/18 9:47 AM, Jacek Anaszewski wrote:
>>> On 12/31/18 4:43 PM, Jacek Anaszewski wrote:
>>>> On 12/30/18 6:35 PM, Pavel Machek wrote:
>>>>> On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote:
>>>>>> On 12/29/18 8:07 PM, Pavel Machek wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>>>>> With the "color" sysfs file it will make more sense to allow for user
>>>>>>>>>> defined color palettes.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think defining these values in the device tree or acpi severely limits the devices
>>>>>>>>> capabilities.  Especially in development phases.  If the knobs were exposed then the user space
>>>>>>>>> can create new experiences.  The color definition should be an absolute color defined in the dt and
>>>>>>>>> either the framework or user space needs to mix these appropriately.  IMO user space should set the policy
>>>>>>>>> of the user experience and the dt/acpi needs to set the capabilities.
>>>>>>>>>
>>>>>>>>> I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group.
>>>>>>>>>
>>>>>>>>> Maybe the framework could take these groups and combine/group them into a single node with the groups colors.
>>>>>>>>
>>>>>>>> There is still HSV approach [0] in store. One problem with proposed
>>>>>>>> implementation is fixed algorithm of RGB <-> HSV color space conversion.
>>>>>>>> Maybe allowing for some board specific adjustments in DT would add
>>>>>>>> more flexibility.
>>>>>>>>
>>>>>>>> [0] https://lkml.org/lkml/2017/8/31/255
>>>>>>>
>>>>>>> Yes we could do HSV. Problem is that that we do not really have RGB
>>>>>>> available. We do have integers for red, green and blue, but they do
>>>>>>> not correspond to RGB colorspace.
>>>>>>
>>>>>> OK, so conversion from HSV to RGB would only increase the aberration.
>>>>>> So, let's stick to RGB - we've got to have some stable ground and this
>>>>>> is something that the hardware at least pretends to be compliant
>>>>>> with.
>>>>>
>>>>> I'm not saying that we should stick to RGB. I'm just saying that
>>>>> problem is complex.
>>>>>
>>>>> And no, hardware does not even pretend to be compliant with RGB color
>>>>> model ( https://en.wikipedia.org/wiki/RGB_color_model ). In
>>>>> particular, in RGB there is non-linear brightness curve.
>>>>
>>>> Quotation from the wiki page you referred to:
>>>>
>>>> "RGB is a device-dependent color model: different devices detect or
>>>> reproduce a given RGB value differently, since the color elements (such
>>>> as phosphors or dyes) and their response to the individual R, G, and B
>>>> levels vary from manufacturer to manufacturer, or even in the same
>>>> device over time. Thus an RGB value does not define the same color
>>>> across devices without some kind of color management"
>>>>
>>>> This claim alone leaves much room for the manufacturers to pretend that
>>>> their devices are compliant with RGB model.
>>>>
>>>> And the documentation of the hardware the discussed driver is for
>>>> also refers to RGB model in many places - e.g. see Table 1, page 15
>>>> in the document [0], where mapping of output triplets to an RGB module
>>>> is shown.
>>>>
>>>> One thing that I missed is that the discussed hardware provides
>>>> LEDn_BRIGHTNESS registers for each RGB LED module, that can be
>>>> configured to set color intensity in linear or logarithmic fashion.
>>>>
>>>> Actually this stands in contradiction with RGB model, since
>>>> change of "color intensity" means change of all RGB components.
>>>>
>>>> We could use brightness file as for monochrome LEDs for that,
>>>
>>> Here I mean brightness file in addition to the previously proposed
>>> red, green and blue files.
>>>
>>>> but we'd need to come up with consistent interface semantics
>>>> for all devices, also those which don't have corresponding
>>>> functionality. Probably this is the place where we could apply
>>>> some RGB<->HSV conversion, as color intensity feels something
>>>> more of HSV's saturation and value.
>>>>
>>>> It would be good to hear from Dan how that looks in reality
>>>> in case of lp5024 device.
>>
>> Sorry for the non-response I have had a passing in my family and have not been at my
>> computer for some time.
> 
> Sorry to hear that. In fact there was no delay, since I wrote that
> yesterday.
> 
>> I am not seeing how HSV will fit into this device.  Not sure what the V is in HSV.
> I meant RGB<->HSV conversion as a fallback for RGB LED
> controllers that don't have the functionality similar to
> LEDn_BRIGHTNESS. For lp5024 we'd use just what hardware offers.
> 
> You can get the flavor of the relationship between RGB and HSV using
> e.g. GIMP color editor. After that you could share a feedback if
> changing LEDn_BRIGHTNESS feels like changing saturation and value of
> HSV.
> 
> Remember that we're still talking about generic approach to the problem.
> 
>> I am still not a fan of defining colors in the kernel.  I think the user space needs to do this based
>> on information it is given.  When I look at Android the user space sets all the policies of the hardware
>> the kernel just provides the vehicle to hardware.  I think defining any set colors in the kernel for devices
>> that have a full color spectrum palette is very restricting.  The kernel should indicate the absolute colors
>> available and not the colors that are allowed.  So in this case we indicate that a Red, green and blue LED are
>> available or that the palette is variable.  Or in the case of a white LED driver we just say white.
> 
> HSV is in no way restrictive. But I'm not pushing for that.
> Vesa in his recent message mentions some difficulties in mapping all RGB
> combinations to HSV.
> 
> We just need something generic and don't want ledn_mix and
> ctrl_bank*mix sysfs files, which are device specific.

OK that makes sense.  I will have to think about how to map HSV to this device.  It becomes complex because
of the way this device groups the LEDs and has a single brightness control.  

If we had a version of the HSV->RGB framework that looks to be set for application then I can present a new version with that FW being used
on top of that patch?

Is that on linux-leds-next?


> 
>> In the case of this device there are RGB outputs that are grouped in clusters and controlled by the LEDn_BRIGHTNESS
>> register.  This is what the brightness file is mapped to.  Within that cluster the individual intensity of the RGB can
>> be modified via the OUTn_color register.  Not knowing what color LED is on what output means the sysfs node has to be left generic.
>>
>> So as Pavel pointed out white would need to be achieved through the RGB individual LEDs being set to certain values and
>> the difuser disfusing the light to achieve the color.  This was done on the original DROID device with a RGB and we
>> were able to get a "white" color but had to set the RGB LEDs to different values.  For this device, once the color is
>> achieved there may be no reason to adjust the color so adjusting the overall brightness of the LEDs without adjusting the
>> individual color can be done with a single write and look seamless to the user.
>> Or other colors can be tuned by setting the unneeded colors in the OUTn_color to 0.
>>
>> These RGB LED clusters can also be grouped into LED banks as well so that all LEDs of the same color within the group will have
>> the same color gradient and brightness.  This is achieved with the BANK_X_COLOR and BRIGHTNESS registers.
>>
>> Again I am not sure how the HSV would work for this device since there is no reason to create a node for each LED output.
>> As the overall brightness of the cluster or bank is controlled by a single brightness file.
> 
> I think that you misunderstood me. I didn't mention creating
> separate nodes for each LED output anywhere.
> 

No this was not something you indicated it was an earlier comment by Pavel, and it is which the LED file nodes are expected
as we have discussed in the past.

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