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

[Jacek Anaszewski]

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.
>
> > > Our problem is how to set the color atomically. With HSV approach we
> > > were to obviate the problem by mapping brightness file to the "V"
> > > component of that color space, and write all H,S and V values to the
> > > hardware only on write to brightness file.
> >
> > I'm not sure how realistic the "atomic color" problem is. Computers
> > are way faster than human vision.
>
> With LEDn_BRIGHTNESS registers of lp5024 it seems that we need the
> ability for grouping LEDs in triplets and be able to set their intensity
> with a single write operation.
>
> > I believe problem to start with is the "white" problem. Setting
> > R=G=B=255 will _not_ result in anything close to white light on
> > hardware I have.
>
> RGBW LED controllers solve this problem. For the devices without
> white/amber we cannot do more than the hardware allows for.
>
> [0] http://www.ti.com/lit/ds/symlink/lp5024.pdf

--
Best regards,
Jacek Anaszewski