| Message ID | 20190401173400.14238-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | MultiColor LED framework Documentation | expand |
Hi, On 4/1/19 10:33 AM, Dan Murphy wrote: > Add the support documentation on the multicolor LED framework. > This document defines the directores and file generated by the directories and files > multicolor framework. It also documents usage. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > Documentation/leds/leds-class-multicolor.txt | 99 ++++++++++++++++++++ > 1 file changed, 99 insertions(+) > create mode 100644 Documentation/leds/leds-class-multicolor.txt > > diff --git a/Documentation/leds/leds-class-multicolor.txt b/Documentation/leds/leds-class-multicolor.txt > new file mode 100644 > index 000000000000..8112b99a7668 > --- /dev/null > +++ b/Documentation/leds/leds-class-multicolor.txt > @@ -0,0 +1,99 @@ > + > +Multi Color LED handling under Linux > +================================================= > + > +Authors: Dan Murphy <dmurphy@ti.com> or Author: > + > +Description > +----------- > +There are varying monochrome LED colors available for application. These > +LEDs can be used as a single use case LED or can be mixed with other color > +LEDs to produce the full spectrum of color. Color LEDs that are grouped > +can be presented under a single LED node with individual color control. > +The multicolor class groups these LEDs and allows dynamically setting the value > +of a single LED or setting the brightness values of the LEDs in the group and > +updating the LEDs virtually simultaneously. > + > +Multicolor Class Control > +------------------------- > +The multicolor class presents the LED groups under a directory called "colors". > +This directory is a child under the LED parent node created but the led_class created by > +framework. The led_class framework is documented in led-class.txt within this > +documentation directory. > + > +Each colored LED is given it's own directory. These directories can be but not its huh??????????? > +limited to red, green, blue, white, amber, yellow and violet. Under these > +directories the brightness and max_brightness files are presented for each LED. > + > +Under the "colors" directory there are two files created "sync" and created: > +"sync_enable". The sync_enable file controls whether the LED brightness > +value is set real time or if the LED brightness value setting is deferred until > +the "sync" file is written. If sync_enable is set then writing to each LED > +"brightness" file will store the brightness value. Once the "sync" file is > +written then each LED color defined in the node will write the brightness of > +the LED in the device driver. > + > +If "sync_enable" is not set then writing the brightness value of the LED to the > +device driver is done immediately. Writing the "sync" file has no affect. > + > +Directory Layout Example > +------------------------ > +root:/sys/class/leds/rgb:grouped_leds# ls -lR colors/ > +colors/: > +drwxr-xr-x 2 root root 0 Jun 28 20:21 blue > +drwxr-xr-x 2 root root 0 Jun 28 20:21 green > +drwxr-xr-x 2 root root 0 Jun 28 20:21 red > +--w------- 1 root root 4096 Jun 28 20:21 sync > +-rw------- 1 root root 4096 Jun 28 20:22 sync_enable > + > +colors/blue: > +-rw------- 1 root root 4096 Jun 28 20:21 brightness > +-r-------- 1 root root 4096 Jun 28 20:27 max_brightness > + > +colors/green: > +-rw------- 1 root root 4096 Jun 28 20:22 brightness > +-r-------- 1 root root 4096 Jun 28 20:27 max_brightness > + > +colors/red: > +-rw------- 1 root root 4096 Jun 28 20:21 brightness > +-r-------- 1 root root 4096 Jun 28 20:27 max_brightness > + > +Example of Writing LEDs with Sync Enabled > +----------------------------------------- > +Below the red, green and blue LEDs are set to corresponding values. These > +values are stored and not written until the sync file is written. > + > +cd /sys/class/leds/rgb:grouped_leds/colors > + > +echo 1 > sync_enable > + > +echo 100 > red/brightness > +echo 80 > green/brightness > +echo 180 > blue/brightness > + > +* LED device driver has not been updated and the LED states have not changed. > +* Writing the LED brightness files again will only change the stored value and > +* not the device driver value. > + > +echo 1 > sync > + > +* LED device driver has been updated the LEDs should present the brightness updated; the LEDs > +* levels that have been set. Since sync_enable is still enabled writing to the > +* LED brightness files will not change the current brightnesses. > + > +Example of Writing LEDs with Sync Disabled > +------------------------------------------ > +Below the values of each LED are written to the device driver immediately upon > +request. > + > +cd /sys/class/leds/rgb:grouped_leds/colors > + > +echo 0 > sync_enable > + > +echo 100 > red/brightness // Red LED should be on with the current brightness > +echo 80 > green/brightness // Green LED should be on with the current brightness > +echo 180 > blue/brightness // Blue LED should be on with the current brightness > +. > +. > +. > +echo 0 > green/brightness // Green LED should be off > cheers. -- ~Randy
Hi! > +Under the "colors" directory there are two files created "sync" and > +"sync_enable". The sync_enable file controls whether the LED brightness > +value is set real time or if the LED brightness value setting is deferred until > +the "sync" file is written. If sync_enable is set then writing to each LED > +"brightness" file will store the brightness value. Once the "sync" file is > +written then each LED color defined in the node will write the brightness of > +the LED in the device driver. > + > +If "sync_enable" is not set then writing the brightness value of the LED to the > +device driver is done immediately. Writing the "sync" file has no > affect. I believe better solutions exists for this problem. One was discussed before -- have single file which contains coefficients for r/g/b channels. Or at least... you should not really need separate sync and sync_enable files. One should do. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi! > .../bindings/leds/leds-class-multicolor.txt | 140 ++++++++++++++++++ > 1 file changed, 140 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt > > diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt > new file mode 100644 > index 000000000000..4b1a26104c79 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt > @@ -0,0 +1,140 @@ > +* Multicolor LED properties > + > +Multicolor LEDs can consist of a RGB, RGBW or a RGBA LED clusters. These devices > +can be grouped together and also provide a modeling mechanism so that the > +cluster LEDs can vary in hue and intensity to produce a wide range of colors. > + > +The nodes and properties defined in this document are unique to the multicolor > +LED class. Common LED nodes and properties are inherited from the common.txt > +within this documentation directory. > + > +Required LED Child properties: > + - color : This is the color ID of the LED. Definitions can be found > + in include/linux/leds/common.txt > + > +Optional LED Child properties: > + - available-brightness-models : This is the phandle to the brightness-model > + node(s) that this LED cluster can support. > + > +Required Brightness model properties > + - led-brightness-model : This flag alerts the device driver and class > + code that this node is a brightness model node > + and to process the properties differently. > + > +Required Brightness model child properties > + - model_name : This is the name of the model presented to the user. This > + should be a color that the LED cluster can produce for > + the device it is attached to. > + - layout : This is the LED layout for the levels. This layout will > + determine the color order of the levels. The layout and > + level-x properties array should be the same size. > + - level-x : These are the values for the LEDs to produce the color that > + is defined. These values are placed in the array according > + to the layout property. > +led-controller@30 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "ti,lp5024"; > + reg = <0x29>; > + > + lp5024_model_yellow: brightness-models { > + led-brightness-model; > + model@0 { > + model_name = "yellow"; > + layout = <LED_COLOR_ID_RED > + LED_COLOR_ID_GREEN > + LED_COLOR_ID_BLUE>; > + level-1 = <255 227 40>; > + level-2 = <255 240 136>; > + level-3 = <255 247 196>; > + }; > + }; I don't think this works. RGB LED can show millions of colors. Do you propose to have millions of entries in dts? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, 1 Apr 2019 23:27:10 +0200 Pavel Machek <pavel@ucw.cz> wrote: > One was discussed before -- have single file which contains > coefficients for r/g/b channels. That would somehow break the rule one file/one value. Although you could consider the whole color one value, that way it would not be broken...
Pavel On 4/1/19 4:29 PM, Pavel Machek wrote: > Hi! > > >> .../bindings/leds/leds-class-multicolor.txt | 140 ++++++++++++++++++ >> 1 file changed, 140 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt >> >> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt >> new file mode 100644 >> index 000000000000..4b1a26104c79 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt >> @@ -0,0 +1,140 @@ >> +* Multicolor LED properties >> + >> +Multicolor LEDs can consist of a RGB, RGBW or a RGBA LED clusters. These devices >> +can be grouped together and also provide a modeling mechanism so that the >> +cluster LEDs can vary in hue and intensity to produce a wide range of colors. >> + >> +The nodes and properties defined in this document are unique to the multicolor >> +LED class. Common LED nodes and properties are inherited from the common.txt >> +within this documentation directory. >> + >> +Required LED Child properties: >> + - color : This is the color ID of the LED. Definitions can be found >> + in include/linux/leds/common.txt >> + >> +Optional LED Child properties: >> + - available-brightness-models : This is the phandle to the brightness-model >> + node(s) that this LED cluster can support. >> + >> +Required Brightness model properties >> + - led-brightness-model : This flag alerts the device driver and class >> + code that this node is a brightness model node >> + and to process the properties differently. >> + >> +Required Brightness model child properties >> + - model_name : This is the name of the model presented to the user. This >> + should be a color that the LED cluster can produce for >> + the device it is attached to. >> + - layout : This is the LED layout for the levels. This layout will >> + determine the color order of the levels. The layout and >> + level-x properties array should be the same size. >> + - level-x : These are the values for the LEDs to produce the color that >> + is defined. These values are placed in the array according >> + to the layout property. > >> +led-controller@30 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "ti,lp5024"; >> + reg = <0x29>; >> + >> + lp5024_model_yellow: brightness-models { >> + led-brightness-model; >> + model@0 { >> + model_name = "yellow"; >> + layout = <LED_COLOR_ID_RED >> + LED_COLOR_ID_GREEN >> + LED_COLOR_ID_BLUE>; >> + level-1 = <255 227 40>; >> + level-2 = <255 240 136>; >> + level-3 = <255 247 196>; >> + }; >> + }; > > I don't think this works. RGB LED can show millions of colors. Do you > propose to have millions of entries in dts? > Pavel > I have had off line conversations with Jacek about this brightness model node. Your concern was actually one of my concerns as well. Not only millions of entries but also having a huge DT binary. We wanted to RFC this to get feedback. And this is why I have not added any support for this in the framework code. Dan -- ------------------ Dan Murphy
Pavel Thanks for the comments On 4/1/19 4:27 PM, Pavel Machek wrote: > Hi! > >> +Under the "colors" directory there are two files created "sync" and >> +"sync_enable". The sync_enable file controls whether the LED brightness >> +value is set real time or if the LED brightness value setting is deferred until >> +the "sync" file is written. If sync_enable is set then writing to each LED >> +"brightness" file will store the brightness value. Once the "sync" file is >> +written then each LED color defined in the node will write the brightness of >> +the LED in the device driver. >> + >> +If "sync_enable" is not set then writing the brightness value of the LED to the >> +device driver is done immediately. Writing the "sync" file has no >> affect. > > I believe better solutions exists for this problem. > I agree there are other solutions. Better not so sure. We keep kicking a solution down the road we have been talking about adding something to the kernel for almost 5 months now, but no one can decide what to do. How do we move a solution forward? Should we just forget the whole idea and keep developing against the current framework? > One was discussed before -- have single file which contains > coefficients for r/g/b channels. > That has been presented as well and the concept was not well received either. > Or at least... you should not really need separate sync and > sync_enable files. One should do. > The idea here was to be able to set the LED brightness immediately on a single LED with a single brightness write or setup the color brightness on all the LEDs and then sync the LED brightnesses. If you wanted to control a single LED for notifications the user space may have to do echo 0 > blue/brightness echo 0 > green/brightness echo 255 > red/brightness echo 1 > sync As opposed to just echo 255 > red/brightness But if that is not a concern then removing the sync_enable is fine with me from a code standpoint but not from a user space side. Dan > Pavel > -- ------------------ Dan Murphy
Hi On Tue, 2 Apr 2019 06:53:30 -0500 Dan Murphy <dmurphy@ti.com> wrote: > I agree there are other solutions. Better not so sure. > We keep kicking a solution down the road we have been talking about adding something to the kernel > for almost 5 months now, but no one can decide what to do. > > How do we move a solution forward? > Should we just forget the whole idea and keep developing against the current framework? > > > One was discussed before -- have single file which contains > > coefficients for r/g/b channels. > > > > That has been presented as well and the concept was not well received either. > > > Or at least... you should not really need separate sync and > > sync_enable files. One should do. > > > > The idea here was to be able to set the LED brightness immediately on a single LED > with a single brightness write or setup the color brightness on all the LEDs and then > sync the LED brightnesses. > > If you wanted to control a single LED for notifications the user space may have to do > > echo 0 > blue/brightness > echo 0 > green/brightness > echo 255 > red/brightness > echo 1 > sync > > As opposed to just > echo 255 > red/brightness > Maybe other kernel developers, or sysfs maintainers like Greg, schould also have a word about this. Another solution, although not usable from shell, could be this: if a process opens all brightness files of a singled multicolor LED and then does a special SYNC_ON_THIS ioctl on one of them, then the sync is done when this color is written. But that would also require ioctl for sysfs files, and I don't think Greg would like that. Dan's sync_enable API looks right to me from the one file/one value sysfs rule as well - you can enable this setting (sync_enable) via one file. What is the main reason you guys are concerned about this? That users will complain that they are writing to red/brightness and the LED does not change color (because they did not disable sync_enable)? Marek
On Tue, 2 Apr 2019 06:40:53 -0500 Dan Murphy <dmurphy@ti.com> wrote: > I have had off line conversations with Jacek about this brightness model node. > > Your concern was actually one of my concerns as well. Not only millions of entries but also having a huge > DT binary. > > We wanted to RFC this to get feedback. And this is why I have not added any support for this in the framework code. > > Dan > What if we just stuck to color spaces and defined channel curves in the device tree? The curve could then be defined via an array of integer pairs, which would be interpreted as points via which the curve passes and the curve would be approximated by linear segemts... led@0 { colorspace = <COLOR_SPACE_ID_RGB>; red-curve = <0 0>, <255 255>; green-curve = <0 0>, <255 128>; blue-curve = <0 0>, <255 128>; };
Marek On 4/2/19 11:17 AM, Marek Behun wrote: > On Tue, 2 Apr 2019 06:40:53 -0500 > Dan Murphy <dmurphy@ti.com> wrote: > >> I have had off line conversations with Jacek about this brightness model node. >> >> Your concern was actually one of my concerns as well. Not only millions of entries but also having a huge >> DT binary. >> >> We wanted to RFC this to get feedback. And this is why I have not added any support for this in the framework code. >> >> Dan >> > > What if we just stuck to color spaces and defined channel curves in the > device tree? > Vesa did a good summary of the current MC framework implementations that are being proposed The thread also documents Pavel's requirements https://lore.kernel.org/patchwork/patch/1037147/ And here is the series with a single brightness file and passing in multiple integers. I did not document this code as I was pretty certain it was not the way to go. https://patches.linaro.org/project/linux-leds/list/?series=18022 Dan > The curve could then be defined via an array of integer pairs, which > would be interpreted as points via which the curve passes and the curve > would be approximated by linear segemts... > > led@0 { > colorspace = <COLOR_SPACE_ID_RGB>; > red-curve = <0 0>, <255 255>; > green-curve = <0 0>, <255 128>; > blue-curve = <0 0>, <255 128>; > }; > -- ------------------ Dan Murphy