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

[Jacek Anaszewski]

Hi Vesa,

On 1/4/19 12:19 AM, Vesa JÃÃskelÃinen wrote:
> Hi Jacek,
>
> Comments below.
>
> On 04/01/2019 0.05, Jacek Anaszewski wrote:
> > Hi Vesa,
> >
> > Thank you for sharing your ideas.
> >
> > Please find my comment below.
> >
> > On 1/1/19 2:45 PM, Vesa JÃÃskelÃinen wrote:
> > > Hi All,
> > >
> > > On 20/12/2018 14.40, Vesa JÃÃskelÃinen wrote:
> > > > Idea was to define preset colors in device tree as an example when 
> > > > you are dealing with multi-color LEDs without PWM. In that case you 
> > > > only have GPIOs to control and then have a problem what does those 
> > > > GPIO's mean.
> > > >
> > > > With preset definitions one can use color names to act as a shortcut 
> > > > to configure GPIO's to proper state for that particular color.
> > > >
> > > > For more flexible setups where you have PWM or such control you have 
> > > > larger space of available colors. In this case you need to somehow 
> > > > define also meaning of those controls.
> > > >
> > > > Also we may not have LED with only red, green and blue elements. 
> > > > There might in example be amber, ultraviolet, white elements.
> > > >
> > > > This is where device tree is concerned. It helps us craft the 
> > > > logical definition for LED so that we can control it from user space 
> > > > in common way.
> > > >
> > > > Now the next problem then is how does user space work then.
> > > >
> > > > For multi-color LEDs it it important to change the color atomically 
> > > > so that no wrong colors are being shown as user space got 
> > > > interrupted when controlling it.
> > > >
> > > > Also we have brightness setting that would be useful for PWM 
> > > > controlled LEDs.
> > > >
> > > > Setting color is easy when you use preset names then you only need 
> > > > to deal with brightness value (eg. RGB -> HSV * brightness -> RGB). 
> > > > Of course here additional problem is other color elements are they 
> > > > then scaled according to brightness value?.
> > > >
> > > > Setting color as "raw" values is then next problem. In order to do 
> > > > it atomically it needs to be one atomic activation and could be eg. 
> > > > one write to "color" sysfs entry with combination of all color 
> > > > elements and perhaps additionally also brightness value. Next 
> > > > question is then what is the format for such entry then? What are 
> > > > the value ranges? In here we can utilize device tree definition to 
> > > > help define what kind of LED we do have and what kind of 
> > > > capabilities it does have.
> > > >
> > > > Additional problem risen also in discussion was non-linearity of 
> > > > some control mechanisms vs. perceived color. So there might be a 
> > > > need for curve mapping similarly what is with backlight control and 
> > > > that would be defined either in device tree and possibly in user 
> > > > space if there is a need for that. I suppose golden curve definition 
> > > > in device tree should be good enough.
> > > >
> > > > Then there was additional discussion about possible animation 
> > > > support but I would leave that for future design as that would then 
> > > > be utilizing the same framework.
> > > >
> > > > I suppose color space handling and that kind of stuff should be in 
> > > > some led core functionality and then raw control should be part of 
> > > > physical led driver.
> > > >
> > > > I was planning to play with it during holiday season but lets see 
> > > > how it goes. Feel free to also experiment with the idea.
> > >
> > > I was playing with this and got some results with PWM LED driver. I 
> > > would like to get feedback now even thou it is not yet ready for 
> > > patch sending.
> > >
> > > They still need more work but the idea can be seen here:
> > > https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led
> > >
> > > This branch is now based on Linux kernel 4.20 release.
> > >
> > > Consider that branch as volatile as I will forcibly update it when 
> > > there are updates.
> > >
> > > ÂFrom there specifically in commits (while they last):
> > >
> > > drivers: leds: Add core support for multi color element LEDs
> > > https://github.com/vesajaaskelainen/linux/commit/55d553906d0a158591435bb6323a318462079d59 
> > >
> > >
> > > WIP: drivers: leds: leds-pwm: Add multi color element LED support.
> > > https://github.com/vesajaaskelainen/linux/commit/efccef08cbf3b2e1e49b95b69ff81cd380519fe3 
> > >
> > >
> > > What is there now:
> > >
> > > - led-core supports color elements
> > > - led-class supports users space configuration
> > > - both led-core and led-class are driver agnostic so they should be 
> > > treated as generic code.
> > > - leds-pwm: my testing code with PWM led.
> > > - no HSV support for brightness as there could be multiple color 
> > > elements out from traditional red-green-blue space or odd 
> > > combinations of colors and they are a bit hard to map to HSV formula 
> > > (and it needs fixed point math).
> > > - no color presets that could be optionally be selected
> > > - when I configure led trigger to heartbeat it actually blinks with 
> > > color specified -- thou trigger gets zeroed out with one sets new 
> > > color or brightness as that was previous functionality with brightness.
> > > - some documentation added
> > > - code should pass checkpatch
> > >
> > > What I was planning to do next:
> > >
> > > - cleanup PWM LED driver so that it works with and without 
> > > LED_MULTI_COLOR_LED being defined.
> > > - improve documentation
> > > - try out how my other device behaves which have dual color element 
> > > LED controlled with GPIO's and see how it would integrate to gpio-led 
> > > driver.
> > >
> > > I would like to get feedback on:
> > > - Device tree idea
> > > - Internal logic
> > > - Should the trigger be really reseted when one changes value of 
> > > brightness? I would think it should function like setting brightness 
> > > entry from sysfs would set current brightness for trigger when it is 
> > > lit. Setting color should change color and brightness and it should 
> > > be active from there one until trigger is disabled from trigger sysfs 
> > > node.
> > >
> > > My testing device has RGB LED with all color elements controlled with 
> > > individual PWM channels from TI's AM335x's integrated PWM controller.
> > >
> > > In device tree I have following:
> > >
> > > ÂÂÂÂÂmulti-color-leds {
> > > ÂÂÂÂÂÂÂÂ compatible = "pwm-leds";
> > >
> > > ÂÂÂÂÂÂÂÂ status-led {
> > > ÂÂÂÂÂÂÂÂÂÂÂÂ label = "status";
> > >
> > > ÂÂÂÂÂÂÂÂÂÂÂÂ element-red {
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pwms = <&ehrpwm0 0 100000 0>;
> > > ÂÂÂÂÂÂÂÂÂÂÂÂ };
> > > ÂÂÂÂÂÂÂÂÂÂÂÂ element-green {
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pwms = <&ehrpwm1 0 100000 0>;
> > > ÂÂÂÂÂÂÂÂÂÂÂÂ };
> > > ÂÂÂÂÂÂÂÂÂÂÂÂ element-blue {
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pwms = <&ehrpwm1 1 100000 0>;
> > > ÂÂÂÂÂÂÂÂÂÂÂÂ };
> > > ÂÂÂÂÂÂÂÂ };
> > > ÂÂÂÂÂ};
> > >
> > > For my second test device I was planning to replace "pwms" with 
> > > "gpios" or such entries.
> > >
> > > In user space one can use it like:
> > >
> > > # --- start of snippet ---
> > >
> > > hostname ~ # cd /sys/class/leds/
> > > hostname leds # ls
> > > status
> > > hostname leds # cd status
> > > hostname status # ls
> > > brightnessÂÂÂÂÂ colorÂÂÂÂÂÂÂÂÂÂ deviceÂÂÂÂÂÂÂÂÂ max_brightness 
> > > max_colorÂÂÂÂÂÂ powerÂÂÂÂÂÂÂÂÂÂ subsystemÂÂÂÂÂÂ triggerÂÂÂÂÂÂÂÂ uevent
> > > hostname status # cat color
> > > brightness=0 red=0 green=0 blue=0
> >
> > This breaks one-value-per-file sysfs rule.
>
> I believe you are referring to this text in:
> https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt
>
> "Attributes should be ASCII text files, preferably with only one value
> per file. It is noted that it may not be efficient to contain only one
> value per file, so it is socially acceptable to express an array of
> values of the same type."
>
> I suppose if one would just make it an array of values (separated by 
> space) and then one file with string array of color element names and on 
> file with maximum value array it could be within those words.
>
> The it would be something like:
>
> $ echo "23 54 32" > color

Go ahead, but first convince Pavel, and then Greg :-)

I'd personally would not have much against, especially that the
list of values will not grow for that one like in case of old patch set
[0] where Pavel and Greg thwarted my similar attempt.

> $ cat max_color
> 255 255 255
>
> $ cat color_names
> red green blue
>
> In addition to this -- one could also export individual color element 
> files.
>
> > Regarding led_scale_color_elements() - I checked it in GIMP and
> > the results are not satisfactory when increasing brightness.
> > Even if we managed to fix it, the result would not be guaranteed
> > to be the same across all devices.
>
> No and they will never be the same. I was told by our hardware expert 
> that it is rather impossible to get linearly behaving LED control 
> without special curve fitting trimmed for particular hardware and LED 
> component in use. And if you go and change LED component/vendor it would 
> need to be "calibrated" again if such accuracy would be required. Also 
> LEDs age and that has also effect on this.
> > This is still the same problem.
> >
> > I have another proposal, being a mix of what has been discussed so far:
> >
> > ÂÂÂ RGB LED class will expose following files:
> > ÂÂÂ a) available by default:
> > ÂÂÂÂÂ - red, green, blue
> > ÂÂÂÂÂÂÂ Writing any of these file will result in writing corresponding
> > ÂÂÂÂÂÂÂ device register.
>
> Problem with this is that we are basically back at square one and one 
> cannot do "atomic" color change with this.
>
> In order to set or activate new values one would need "load values" file 
> or such that when writing to it would activate new values. However it 
> becomes quite clumsy interface at that point as you need to handle 
> multiple writes to multiple files and makes those operations rather slow.
>
> Then we have color presets left that could kinda solve the issue on 
> setting the color to fixed values atomically.

That's why I proposed "color_space" file that is also a part of my
proposed design below.

> Of course one direction is what happened with gpio driver was own device 
> node with ioctl's allowing more faster and more fancier control.
>
> > ÂÂÂÂÂ - color_space: it would accept color space, e,g. "hsv", that would
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ have to be supported by LED RGB core; setting color
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ space would create relevant files, e.g. for hsv
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ hue. saturation, brightness, and remove default ones
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ other "color spaces" could be defined in DT as
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ proposed by Vesa; reading this file would print
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ available color spaces
> >
> > ÂÂÂ b) available conditionally:
> > ÂÂÂÂÂ - brightness
> > ÂÂÂÂÂÂ It will be exposed by devices that have hardware support for
> > ÂÂÂÂÂÂ changing color brightness, like lp5024, or it will be made
> > ÂÂÂÂÂÂ available after setting relevant color space, like "hsv", or
> > ÂÂÂÂÂÂ other color presets defined in DT
> >
> > I think it will be flexible enough to meet everyone's needs.
> >
> > Current triggers would work only when brightness file is available.
>
> Or we could transition it in that case to simulated "on/off" type of 
> thing. As that is what triggers more or less use.
>
> When "on" LED would have its configured color and when "off" LED would 
> be turned off (eg. values of zero).

Yeah, it would be even better solution.

[0] https://www.spinics.net/lists/devicetree/msg69730.html

--
Best regards,
Jacek Anaszewski