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

[Dan Murphy]

On 1/10/19 3:02 PM, Jacek Anaszewski wrote:
> On 1/10/19 8:58 PM, Dan Murphy wrote:
>> On 1/10/19 1:23 PM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 1/10/19 1:46 PM, Dan Murphy wrote:
>>>> Jacek
>>>>
>>>> On 1/8/19 3:25 PM, Dan Murphy wrote:
>>>>> Jacek
>>>>>
>>>>> On 1/8/19 3:18 PM, Jacek Anaszewski wrote:
>>>>>> Hi Dan,
>>>>>>
>>>>>> On 1/7/19 10:14 PM, Dan Murphy wrote:
>>>>>>> Jacek
>>>>>>>
>>>>>>> On 1/7/19 2:59 PM, Jacek Anaszewski wrote:
>>>>>>>> Dan,
>>>>>>>>
>>>>>>>> On 1/7/19 8:36 PM, Dan Murphy wrote:
>>>>>>>>> Jacek
>>>>>>>>>
>>>>>>>>> On 1/7/19 1:13 PM, Jacek Anaszewski wrote:
>>>>>>>>>> On 1/6/19 4:52 PM, Jacek Anaszewski wrote:
>>>>>>>>>>> Hi Pavel,
>>>>>>>>>>>
>>>>>>>>>>> On 1/5/19 11:12 PM, Pavel Machek wrote:
>>>>>>>>>>>> Hi!
>>>>>>>>>>>>
>>>>>>>>>>>>>> Grab yourself an RGB LED and play with it; you'll see what the
>>>>>>>>>>>>>> problems are. It is hard to explain colors over email...
>>>>>>>>>>>>>
>>>>>>>>>>>>> Video [0] gives some overview of lp5024 capabilities.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don't see any problems in exposing separate red,green,blue
>>>>>>>>>>>>> files and brightness for the devices with hardware support for
>>>>>>>>>>>>> that.
>>>>>>>>>>>>
>>>>>>>>>>>> Well, that's what we do today, as three separate LEDs, right?
>>>>>>>>>>>
>>>>>>>>>>> No. It doesn't allow for setting color intensity by having
>>>>>>>>>>> the color fixed beforehand. Below is relevant excerpt from
>>>>>>>>>>> the lp5024 documentation. This is not something that can be
>>>>>>>>>>> mapped to RGB color space, but rather to HSV/HSL, with the
>>>>>>>>>>> reservation that the hardware implementation uses PWM
>>>>>>>>>>> for setting color intensity.
>>>>>>>>>>>
>>>>>>>>>>> <quote>
>>>>>>>>>>> 8.3.1.2 Independent Intensity Control Per RGB LED Module
>>>>>>>>>>>
>>>>>>>>>>> When color is fixed, the independent intensity-control is used to
>>>>>>>>>>> achieve accurate and flexible dimming control for every RGB LED module.
>>>>>>>>>>>
>>>>>>>>>>> 8.3.1.2.1 Intensity-Control Register Configuration
>>>>>>>>>>>
>>>>>>>>>>> Every three consecutive output channels are assigned to their respective
>>>>>>>>>>> intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1,
>>>>>>>>>>> and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to
>>>>>>>>>>> connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx
>>>>>>>>>>> device allows 256-step intensity control for each RGB LED module, which
>>>>>>>>>>> helps achieve a smooth dimming effect.
>>>>>>>>>>> </quote>
>>>>>>>>>>>
>>>>>>>>>>>> I don't have problem with that, either; other drivers already do
>>>>>>>>>>>> that. He's free to use existing same interface.
>>>>>>>>>>>>
>>>>>>>>>>>> But that is insufficient, as it does not allow simple stuff, such as
>>>>>>>>>>>> turning led "white".
>>>>>>>>>>>>
>>>>>>>>>>>> So... perhaps we should agree on requirements, first, and then we can
>>>>>>>>>>>> discuss solutions?
>>>>>>>>>>>>
>>>>>>>>>>>> Requirements for RGB LED interface:
>>>>>>>>>>>>
>>>>>>>>>>>> 1) Userspace should be able to set the white color
>>>>>>>>>>>>
>>>>>>>>>>>> 2) Userspace should be able to arbitrary color from well known list
>>>>>>>>>>>> and it should approximately match what would CRT, LCD or OLED monitor display
>>>>>>>>>>>
>>>>>>>>>>> The difference is that monitor display driver is pre-calibrated
>>>>>>>>>>> for given display by the manufacturer. With the LED controllers the
>>>>>>>>>>> manufacturer has no control over what LEDs will be connected to the
>>>>>>>>>>> iouts. Therefore it should be not surprising that colors produced
>>>>>>>>>>> by custom LEDs are not as user would expect when comparing to
>>>>>>>>>>> the RGB color displayed on the monitor display.
>>>>>>>>>>>
>>>>>>>>>>> TI provides "Various LED Ring Lighting Patterns Reference Design" [0]
>>>>>>>>>>> that show how to produce various patterns with use of the reference
>>>>>>>>>>> board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1].
>>>>>>>>>>>
>>>>>>>>>>> Document [0] mentions also specific "Design considerations" in the
>>>>>>>>>>> chapter 2.2:
>>>>>>>>>>>
>>>>>>>>>>> <quote>
>>>>>>>>>>> Several considerations are taken into account for this particular design:
>>>>>>>>>>> • LED map (ring) for meeting the requirement of popular human-machine interaction style.
>>>>>>>>>>> • LED size, numbers and the diffuse design for meeting lighting pattern uniformity.
>>>>>>>>>>> • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern.
>>>>>>>>>>>
>>>>>>>>>>> These considerations apply to most human-machine interaction end equipment with day and night vision
>>>>>>>>>>> designs in some way, but the designer must decide the particular considerations to take into account for a
>>>>>>>>>>> specific design.
>>>>>>>>>>> </quote>
>>>>>>>>>>>
>>>>>>>>>>> This renders your requirement 2) infeasible with use of custom LEDs
>>>>>>>>>>> any fixed algorithm, since the final effect will always heavily depend
>>>>>>>>>>
>>>>>>>>>> Typo here: s/any fixed/and fixed/
>>>>>>>>>>
>>>>>>>>>>> on the LED circuit design.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>          2a) LEDs probably slightly change color as they age. That's out of
>>>>>>>>>>>>          scope, unless the variation is much greater than on monitors.
>>>>>>>>>>>>
>>>>>>>>>>>>          2b) Manufacturing differences cause small color variation. Again,
>>>>>>>>>>>>          that's out of scope, unless the variation is much greater than on
>>>>>>>>>>>>          monitors.
>>>>>>>>>>>>
>>>>>>>>>>>> Nice to have features:
>>>>>>>>>>>>
>>>>>>>>>>>> 3) Full range of available colors/intensities should be available to
>>>>>>>>>>>> userspace
>>>>>>>>>>>>
>>>>>>>>>>>> 4) Interface should work well with existing triggers
>>>>>>>>>>>>
>>>>>>>>>>>> 5) It would be nice if userland knew how many lumens are produced at
>>>>>>>>>>>> each wavelength for each setting (again, minus aging and manufacturing
>>>>>>>>>>>> variations).
>>>>>>>>>>>>
>>>>>>>>>>>> 6) Complexity of math in kernel should be low, and preferably it
>>>>>>>>>>>> should be integer or fixed point
>>>>>>>>>>>>
>>>>>>>>>>>> Problems:
>>>>>>>>>>>>
>>>>>>>>>>>> a) RGB LEDs are usually not balanced. Setting 100% PWM on
>>>>>>>>>>>> red/green/blue channels will result in nothing close to white
>>>>>>>>>>>> light. In fact, to get white light on N900, blue and green channel's
>>>>>>>>>>>> PWM needs to be set pretty low, as in 5%.
>>>>>>>>>>>>
>>>>>>>>>>>> b) LED class does not define any relation between "brightness" in
>>>>>>>>>>>> sysfs and ammount of light in lumens. Some drivers use close to linear
>>>>>>>>>>>> relation, some use exponential relation. Human eyes percieve logarithm
>>>>>>>>>>>> of lumens. RGB color model uses even more complex function.
>>>>>>>>>>>>
>>>>>>>>>>>> c) Except for white LEDs, LEDs are basically sources of single
>>>>>>>>>>>> wavelength of light, while CRTs and LCDs produce broader
>>>>>>>>>>>> spectrums.
>>>>>>>>>>>>
>>>>>>>>>>>> d) RG, RGBW and probably other LED combinations exist.
>>>>>>>>>>>>
>>>>>>>>>>>> e) Not all "red" LEDs will produce same wavelength. Similar
>>>>>>>>>>>> differences will exist for other colors.
>>>>>>>>>>>>
>>>>>>>>>>>> f) We have existing RGB LEDs represented as three separate
>>>>>>>>>>>> monochromatic LEDs in sysfs.
>>>>>>>>>>>
>>>>>>>>>>> One general question: do you have any solutions in store?
>>>>>>>>>>>
>>>>>>>>>>> [0] http://www.ti.com/lit/ug/tiduee6/tiduee6.pdf
>>>>>>>>>>> [1] http://www.everlight.com/file/ProductFile/19-337-R6GHBHC-A01-2T.pdf
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I just wanted to point out that there are 4 total devices right now that use the same mapping
>>>>>>>>>
>>>>>>>>> LP5018, LP5024, LP5030 and the LP5036.
>>>>>>>>>
>>>>>>>>> I can implement what ever we would like to I just need to know what to design against.
>>>>>>>>
>>>>>>>> As you can see from the discussion in this thread it may take some
>>>>>>>> time to work out the interface satisfying everyone. I made some design
>>>>>>>> proposal, but Pavel had no warm word for it. It would be easier if
>>>>>>>> we had more opinions.
>>>>>>>
>>>>>>> I got it from the threads and just the time invested in the FW and HSV.
>>>>>>>
>>>>>>>>
>>>>>>>> How do you feel about using brightness file for setting LEDn_BRIGHTNESS?
>>>>>>>
>>>>>>> I am using that now.  The brightness file will adjust the overall brightness of the LED group
>>>>>>> or bank pending on how the LEDs are grouped in the DT file.
>>>>>>>
>>>>>>>>
>>>>>>>> Does increasing LEDn_BRIGHTNESS value (i.e. color intensity) feel like
>>>>>>>> increasing color lightness (i.e. the pattern presented in the video [0]
>>>>>>>> starting from 1:22)?
>>>>>>>
>>>>>>> No unfortunately this is why I introduced the new files to control the individual RGB intensities
>>>>>>> so that the designers can set, tune, create color variations or patterns like the video.
>>>>>>>
>>>>>>> The RGB group brightness would be independent based on lighting conditions, enclosures and diffusers.
>>>>>>> So you could technically be changing color and overall brightness virtually simultaneously
>>>>>>
>>>>>> Oh, so this is surprising. Now it gets even more obscure to me.
>>>>>>
>>>>>> It would be really helpful if we could see a video showing
>>>>>> the LED effects with regard to the applied settings.
>>>>>
>>>>> Well I am doing a test off the command line to ensure the user space can interface with the RGB LED.
>>>>>
>>>>> I can ping someone in product development to see the application of this device if that would help.
>>>>> We did give them a test driver to work on their features but told them the driver is not final until it
>>>>> is in the mainline kernel
>>>>>
>>>>> Dan
>>>>>
>>>>>>
>>>>>>>>> But keep in mind I still need to invest my time in the other TI-lmu patches on my list to complete.
>>>>>>>>
>>>>>>>> Do what you deem most suitable for you. We are here only to help
>>>>>>>> merging the patches, but keeping in mind that kernel interface once
>>>>>>>> introduced must be preserved forever. Therefore we need to do our
>>>>>>>> best to make the best possible design decisions.
>>>>>>>>
>>>>>>>> [0] https://www.youtube.com/watch?v=qdt-alh8i6E
>>>>>>>>
>>>>>>>
>>>>>>> I understand.  Maybe I can make the files generic to use for either control or individual control.
>>>>>>>
>>>>>>> We can probably define new ABI's until either HSV or DT frameworks get going.  And them make the file presentation
>>>>>>> configurable and default to the new files.
>>>>>>
>>>>>> I am leaning towards it. Just commented on your patches.
>>>>>>
>>>>>
>>>>>
>>>>
>>>> Asked the HW team for videos this is what they sent
>>>> https://training.ti.com/lp50x-led-drivers-achieve-optimal-color-brightness-zero-audible-noise
>>>> https://training.ti.com/how-configuring-led-brightness-color-and-patterns-lp50x-gui-tool
>>>>
>>>> Not sure how helpful these would be
>>>
>>> Thank you for the videos - they are helpful, and they confirm my first
>>> impression regarding the LEDn_BRIGHTNESS effect. In is shown in the
>>> second video starting from 3:37.
>>>
>>> This is the same what I was asking about previously (video [0] from
>>> previous message starting from 1:22). For me this looks similarly
>>> to increasing V of HSV or L of HSL [1].
>>>
>>> You denied, so either we interpret colors differently, or there was
>>> some misunderstanding.
>>>
>>> [1] http://hslpicker.com
>>>
>>
>> Well I did indicate later that I could work on implementing against the HSV/RGB framework and fit that into
> 
> I just wanted to make sure that brightness file semantics fits
> for this particular LEDn_BRIGHTNESS feature.
> Now it is clear to me - yes it does.
> 
> For RGB LED modules brightness file would map to LEDn_BRIGHTNESS and
> red,green,blue files would map to OUTn_COLOR registers.
> 
> For banks brightness file would map to BANK_BRIGHTNESS and
> red,green,blue files would map to BANK_n_COLOR.
> 
> I wouldn't bother with cases when someone connects LEDs of
> other colors - he is on his own then.
> 
> If you want to have the driver merged quickly then you can
> use this interface. We will convert it to LED RGB class once
> it is ready. I presume that the interface I outlined will be
> supported by one of the brightness-mode's - I like the name
> proposed by Vesa.
> 
> Effectively - no need to implement HSV algorithm in your driver.
> 

Not quickly merged.  I would prefer it to be correct.

So you are saying I need to present brightness, red, blue and green files to the user space?

That would work for me.

There were a few proposals in a very lengthy chain so I want to be clear on the interfaces.

Dan

>> the LP5024 driver.  Only issue I see is mapping of the LED color to the correct output.  I know what is recommended
>> in the data sheet but that does not mean that is what the developers will do.  We cannot always guarantee that
>> the red LEDs will be on OUT0, OUT3, OUT6 or on BANK A
>>
>> But it is not part of the LED class yet I would have to pull in the patch to enable it.
>>
>> What is the thought on this?  Would the HSV/RGB class be pulled in as a part of the LP5024 driver?
>> I don't want to put to much effort into code that will have to wait.
>>
>> Dan
>>
> 


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