Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
From: Vesa JÃÃskelÃinen
Date: Thu Jan 03 2019 - 18:19:18 EST
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
$ 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.
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).
This is ad hoc design so it can have some logical flaws.
Best regards,
Jacek Anaszewski
Thanks,
Vesa JÃÃskelÃinen