From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Murphy Subject: Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver Date: Thu, 10 Jan 2019 15:07:26 -0600 Message-ID: <2d9299d9-b557-0608-50ba-1069e27e3802@ti.com> References: <986b5105-2fdb-bd25-7c8a-ca8fd1ade821@gmail.com> <7f205102-e854-f1cb-cc03-1307d1cddc87@gmail.com> <20190104201256.GA2931@amd> <20190104220726.GA12395@amd> <4cf79414-578e-eea7-4f46-fc8789206988@gmail.com> <20190105123146.GA16354@amd> <8044cdd9-b9b3-fddd-6106-184b906859e2@gmail.com> <20190105221254.GA22322@amd> <70366f2a-aafd-174f-73ec-a8117133b7af@gmail.com> <2c3425b0-70af-1f38-35ee-0af857bf0539@gmail.com> <63713076-b111-316d-7e60-9b799ff22c0a@gmail.com> <0a163b0c-eabc-6cf0-c605-28622af3d31d@ti.com> <1594152a-4f57-e258-5c9c-e1b158c72932@gmail.com> <5972a548-89cd-18ab-4f62-dd26057cf444@ti.com> <155f0473-0be0-e8a2-7873-607e4e495a03@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <155f0473-0be0-e8a2-7873-607e4e495a03@gmail.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Jacek Anaszewski , Pavel Machek Cc: =?UTF-8?B?VmVzYSBKw6TDpHNrZWzDpGluZW4=?= , robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org List-Id: linux-leds@vger.kernel.org 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. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 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. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> 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: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 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. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 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