[RFC] leds: multicolor: Add sysfs interface definition

Message ID 20190130183005.833-1-dmurphy@ti.com
State New
Headers show
Series
  • [RFC] leds: multicolor: Add sysfs interface definition
Related show

Commit Message

Dan Murphy Jan. 30, 2019, 6:30 p.m.
Add a documentation of LED Multicolor LED class specific
sysfs attributes.

Signed-off-by: Dan Murphy <dmurphy@ti.com>

---
 .../ABI/testing/sysfs-class-led-multicolor    | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor

-- 
2.20.1.98.gecbdaf0899

Comments

Jacek Anaszewski Jan. 30, 2019, 7:37 p.m. | #1
Hi Dan,

Thank you for the RFC.

One vital thing is missing - documentation of brightness file must
be updated to define its semantics for LED multi color class.

Either we need brightness-model file returning only "onoff" option
available, or, for time being, fix the max_brightness for LED multi
color class to 1 (which will map to max intensity level for each color).

Best regards,
Jacek Anaszewski

On 1/30/19 7:30 PM, Dan Murphy wrote:
> Add a documentation of LED Multicolor LED class specific

> sysfs attributes.

> 

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

>   .../ABI/testing/sysfs-class-led-multicolor    | 38 +++++++++++++++++++

>   1 file changed, 38 insertions(+)

>   create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor

> 

> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor

> new file mode 100644

> index 000000000000..19f8da9b150e

> --- /dev/null

> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor

> @@ -0,0 +1,38 @@

> +What:		/sys/class/leds/<led>/color/sync_enable

> +Date:		January 2019

> +KernelVersion:	5.0

> +Contact:	Dan Murphy <dmurphy@ti.com>

> +Description:	read/write

> +		Writing a 1 to this file will enable the sychronization of all

> +		the defined color LEDs within the LED node.  Writing a 0 to

> +		this file will disable syncing.

> +

> +What:		/sys/class/leds/<led>/color/sync

> +Date:		January 2019

> +KernelVersion:	5.0

> +Contact:	Dan Murphy <dmurphy@ti.com>

> +Description:	write only

> +		Writing a 1 to this file while sync_enable is set to 1 will

> +		synchronize all defined LEDs within the LED node.  All LEDs

> +		defined will be configured based on the brightness that has

> +		been requested.

> +

> +		If sync_enable is set to 0 then writing a 1 to sync has no

> +		affect on the LEDs.

> +

> +What:		/sys/class/leds/<led>/color/<led color>

> +Date:		January 2019

> +KernelVersion:	5.0

> +Contact:	Dan Murphy <dmurphy@ti.com>

> +Description:	read/write

> +		These files are dynamically created based on the colors defined

> +		by the registrar of the class.

> +		The led color(s) can be but not limited to red, green, blue,

> +		white, amber and violet.  If sync is enabled then writing the

> +		brightness value of the LED is deferred until a 1 is

> +		written to /sys/class/leds/<led>/color/sync.  If syncing is

> +		disabled then the LED brightness value will be written

> +		immediately to the LED driver.

> +

> +		The value of the color is from 0 to

> +		/sys/class/leds/<led>/max_brightness.

>
Dan Murphy Jan. 30, 2019, 7:59 p.m. | #2
Jacek

On 1/30/19 1:37 PM, Jacek Anaszewski wrote:
> Hi Dan,

> 

> Thank you for the RFC.

> 

> One vital thing is missing - documentation of brightness file must

> be updated to define its semantics for LED multi color class.

> 

> Either we need brightness-model file returning only "onoff" option

> available, or, for time being, fix the max_brightness for LED multi

> color class to 1 (which will map to max intensity level for each color).

> 


I can make max_brightness default to 1 if not set by the LED driver.

But the LP50xx has brightness controls so setting max_brightness from the driver should over ride
the max of 1 in the upper level.  

For devices that do not support brightness as a separate control we can create a file called
max_brightness_<color> that defines the max that a specific color can be set to.  If max_brightness
is set to 1 then create max_brightness_<color>.  If max_brightness > 1 then do not create the files.

I don't think we have fully vetted the brightness-model yet so I prefer to omit
it and possibly introduce that later.

Dan

> Best regards,

> Jacek Anaszewski

> 

> On 1/30/19 7:30 PM, Dan Murphy wrote:

>> Add a documentation of LED Multicolor LED class specific

>> sysfs attributes.

>>

>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>> ---

>>   .../ABI/testing/sysfs-class-led-multicolor    | 38 +++++++++++++++++++

>>   1 file changed, 38 insertions(+)

>>   create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor

>>

>> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor

>> new file mode 100644

>> index 000000000000..19f8da9b150e

>> --- /dev/null

>> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor

>> @@ -0,0 +1,38 @@

>> +What:        /sys/class/leds/<led>/color/sync_enable

>> +Date:        January 2019

>> +KernelVersion:    5.0

>> +Contact:    Dan Murphy <dmurphy@ti.com>

>> +Description:    read/write

>> +        Writing a 1 to this file will enable the sychronization of all

>> +        the defined color LEDs within the LED node.  Writing a 0 to

>> +        this file will disable syncing.

>> +

>> +What:        /sys/class/leds/<led>/color/sync

>> +Date:        January 2019

>> +KernelVersion:    5.0

>> +Contact:    Dan Murphy <dmurphy@ti.com>

>> +Description:    write only

>> +        Writing a 1 to this file while sync_enable is set to 1 will

>> +        synchronize all defined LEDs within the LED node.  All LEDs

>> +        defined will be configured based on the brightness that has

>> +        been requested.

>> +

>> +        If sync_enable is set to 0 then writing a 1 to sync has no

>> +        affect on the LEDs.

>> +

>> +What:        /sys/class/leds/<led>/color/<led color>

>> +Date:        January 2019

>> +KernelVersion:    5.0

>> +Contact:    Dan Murphy <dmurphy@ti.com>

>> +Description:    read/write

>> +        These files are dynamically created based on the colors defined

>> +        by the registrar of the class.

>> +        The led color(s) can be but not limited to red, green, blue,

>> +        white, amber and violet.  If sync is enabled then writing the

>> +        brightness value of the LED is deferred until a 1 is

>> +        written to /sys/class/leds/<led>/color/sync.  If syncing is

>> +        disabled then the LED brightness value will be written

>> +        immediately to the LED driver.

>> +

>> +        The value of the color is from 0 to

>> +        /sys/class/leds/<led>/max_brightness.

>>

> 



-- 
------------------
Dan Murphy
Jacek Anaszewski Jan. 30, 2019, 8:20 p.m. | #3
Dan,

On 1/30/19 8:59 PM, Dan Murphy wrote:
> Jacek

> 

> On 1/30/19 1:37 PM, Jacek Anaszewski wrote:

>> Hi Dan,

>>

>> Thank you for the RFC.

>>

>> One vital thing is missing - documentation of brightness file must

>> be updated to define its semantics for LED multi color class.

>>

>> Either we need brightness-model file returning only "onoff" option

>> available, or, for time being, fix the max_brightness for LED multi

>> color class to 1 (which will map to max intensity level for each color).

>>

> 

> I can make max_brightness default to 1 if not set by the LED driver.

> 

> But the LP50xx has brightness controls so setting max_brightness from the driver should over ride

> the max of 1 in the upper level.


Yes, so the max_brightness should be updated basing on current
brightness-model. For LEDn_BRIGHTNESS of LP50xx we could have
"hw" or maybe even better just "lp50xx-linear" and "lp50xx-logarithmic"
- I just forgot about that capability of the device.

> For devices that do not support brightness as a separate control we can create a file called

> max_brightness_<color> that defines the max that a specific color can be set to.  If max_brightness

> is set to 1 then create max_brightness_<color>.  If max_brightness > 1 then do not create the files.


Right. We will need dedicated max_brightness for each color.
They should be placed also in the colors directory, next to the color
files.

> I don't think we have fully vetted the brightness-model yet so I prefer to omit

> it and possibly introduce that later.


We need to start from something. It will give better overview of the
whole idea.

Best regards,
Jacek Anaszewski

> Dan

> 

>> Best regards,

>> Jacek Anaszewski

>>

>> On 1/30/19 7:30 PM, Dan Murphy wrote:

>>> Add a documentation of LED Multicolor LED class specific

>>> sysfs attributes.

>>>

>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>> ---

>>>    .../ABI/testing/sysfs-class-led-multicolor    | 38 +++++++++++++++++++

>>>    1 file changed, 38 insertions(+)

>>>    create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor

>>>

>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor

>>> new file mode 100644

>>> index 000000000000..19f8da9b150e

>>> --- /dev/null

>>> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor

>>> @@ -0,0 +1,38 @@

>>> +What:        /sys/class/leds/<led>/color/sync_enable

>>> +Date:        January 2019

>>> +KernelVersion:    5.0

>>> +Contact:    Dan Murphy <dmurphy@ti.com>

>>> +Description:    read/write

>>> +        Writing a 1 to this file will enable the sychronization of all

>>> +        the defined color LEDs within the LED node.  Writing a 0 to

>>> +        this file will disable syncing.

>>> +

>>> +What:        /sys/class/leds/<led>/color/sync

>>> +Date:        January 2019

>>> +KernelVersion:    5.0

>>> +Contact:    Dan Murphy <dmurphy@ti.com>

>>> +Description:    write only

>>> +        Writing a 1 to this file while sync_enable is set to 1 will

>>> +        synchronize all defined LEDs within the LED node.  All LEDs

>>> +        defined will be configured based on the brightness that has

>>> +        been requested.

>>> +

>>> +        If sync_enable is set to 0 then writing a 1 to sync has no

>>> +        affect on the LEDs.

>>> +

>>> +What:        /sys/class/leds/<led>/color/<led color>

>>> +Date:        January 2019

>>> +KernelVersion:    5.0

>>> +Contact:    Dan Murphy <dmurphy@ti.com>

>>> +Description:    read/write

>>> +        These files are dynamically created based on the colors defined

>>> +        by the registrar of the class.

>>> +        The led color(s) can be but not limited to red, green, blue,

>>> +        white, amber and violet.  If sync is enabled then writing the

>>> +        brightness value of the LED is deferred until a 1 is

>>> +        written to /sys/class/leds/<led>/color/sync.  If syncing is

>>> +        disabled then the LED brightness value will be written

>>> +        immediately to the LED driver.

>>> +

>>> +        The value of the color is from 0 to

>>> +        /sys/class/leds/<led>/max_brightness.

>>>

>>

> 

>
Dan Murphy Jan. 30, 2019, 9:07 p.m. | #4
Jacek

On 1/30/19 2:20 PM, Jacek Anaszewski wrote:
> Dan,

> 

> On 1/30/19 8:59 PM, Dan Murphy wrote:

>> Jacek

>>

>> On 1/30/19 1:37 PM, Jacek Anaszewski wrote:

>>> Hi Dan,

>>>

>>> Thank you for the RFC.

>>>

>>> One vital thing is missing - documentation of brightness file must

>>> be updated to define its semantics for LED multi color class.

>>>

>>> Either we need brightness-model file returning only "onoff" option

>>> available, or, for time being, fix the max_brightness for LED multi

>>> color class to 1 (which will map to max intensity level for each color).

>>>

>>

>> I can make max_brightness default to 1 if not set by the LED driver.

>>

>> But the LP50xx has brightness controls so setting max_brightness from the driver should over ride

>> the max of 1 in the upper level.

> 

> Yes, so the max_brightness should be updated basing on current

> brightness-model. For LEDn_BRIGHTNESS of LP50xx we could have

> "hw" or maybe even better just "lp50xx-linear" and "lp50xx-logarithmic"

> - I just forgot about that capability of the device.

> 


OK maybe "hw" would make sense as there may be other devices that have dedicated brightness controls
over color controls.

Probably need to create a model for non-modeled cases like "rgb-independent".  Dumb name but I could not
think of anything better.

>> For devices that do not support brightness as a separate control we can create a file called

>> max_brightness_<color> that defines the max that a specific color can be set to.  If max_brightness

>> is set to 1 then create max_brightness_<color>.  If max_brightness > 1 then do not create the files.

> 

> Right. We will need dedicated max_brightness for each color.

> They should be placed also in the colors directory, next to the color

> files.

> 


OK will document this.

>> I don't think we have fully vetted the brightness-model yet so I prefer to omit

>> it and possibly introduce that later.

> 

> We need to start from something. It will give better overview of the

> whole idea.

> 


OK.  Don't get me wrong I don't oppose this idea I am just trying to figure out how the user space would
know what models and brightness levels are available.

I mean we can read the brightness-models and present the available models but then how does the user know
what and how many levels are in each model?  And how do we govern putting them in the right order?

The DT file can get messed up, per the previous example
rgb-green = <0x277c33 0x37b048 0x47e45d>; 

This is assumed to be from dimmest to brightest.  But that is just an assumption

What if the entry looked like this?
rgb-green = <0x37b048 0x277c33 0x47e45d>; 

Then echo 1 > brightness is not really a lower intensity then echo 2 > brightness.
I know this is a product level issue but this can be misused and there is no way for maintainers
or reviewers to really catch this error in code reviews.

The driver can map the brightness to the appropriate level requested by the class but again not
sure how the user space can know what is available.  And there is nothing from stopping a
definition of up to 2^32 brightness combinations.  This I know is unrealistic but the capability is there

I am wondering if there should be some sort of coefficient that can be defined that is 
applied to each color (no complex math).  I can see this working in a linear device but logrithimic
maybe an issue.

Like

rgb-green = <0x277c33>;  //Coefficient used to set the dimmest allowed brightness for the color model.

echo 1 > brightness

red = 0x27
green = 0x7c
blue = 0x33

echo 2 > brightness

red = 0x28
green = 0x7d
blue = 0x34

echo 3 > brightness

red = 0x29
green = 0x7e
blue = 0x35

This example would give you 132 different brightness levels. green is the brightest defined color so the step calculation is 

255-124+1 = 132 (zero based) as 0 is off.

There can be a file called rgb_green_max which can be read to indicate how many brightness levels can be achieved.
If the user goes beyond the steps then throw -EINVAL at them.

These brightness models probably should be put into a separate directory to isolate and not clutter the colors directory.
And writing brightness to these models would be immediate no sync involved.

Dan

> Best regards,

> Jacek Anaszewski

> 

>> Dan

>>


<snip>-- 
------------------
Dan Murphy
Jacek Anaszewski Jan. 30, 2019, 10:14 p.m. | #5
Dan,

On 1/30/19 10:07 PM, Dan Murphy wrote:
> Jacek

> 

> On 1/30/19 2:20 PM, Jacek Anaszewski wrote:

>> Dan,

>>

>> On 1/30/19 8:59 PM, Dan Murphy wrote:

>>> Jacek

>>>

>>> On 1/30/19 1:37 PM, Jacek Anaszewski wrote:

>>>> Hi Dan,

>>>>

>>>> Thank you for the RFC.

>>>>

>>>> One vital thing is missing - documentation of brightness file must

>>>> be updated to define its semantics for LED multi color class.

>>>>

>>>> Either we need brightness-model file returning only "onoff" option

>>>> available, or, for time being, fix the max_brightness for LED multi

>>>> color class to 1 (which will map to max intensity level for each color).

>>>>

>>>

>>> I can make max_brightness default to 1 if not set by the LED driver.

>>>

>>> But the LP50xx has brightness controls so setting max_brightness from the driver should over ride

>>> the max of 1 in the upper level.

>>

>> Yes, so the max_brightness should be updated basing on current

>> brightness-model. For LEDn_BRIGHTNESS of LP50xx we could have

>> "hw" or maybe even better just "lp50xx-linear" and "lp50xx-logarithmic"

>> - I just forgot about that capability of the device.

>>

> 

> OK maybe "hw" would make sense as there may be other devices that have dedicated brightness controls

> over color controls.


Single "hw" doesn't address both linear and logarithmic.
This is device specific, so I don't see anything wrong in
"lp50xx-*", provided that it will be documented.

> Probably need to create a model for non-modeled cases like "rgb-independent".  Dumb name but I could not

> think of anything better.


There is no point in having any rgb* brightness model since increasing
rgb in an arbitrary way will not give the impression of increasing color
intensity (lightness of the same hue). With DT defined hsl-<color>
ranges there is no way to verify that levels arrangement makes sense
with regard to preserving hue, this is a matter of trust. But we should
state that it is highly recommended to obey this constraint so as to
not mislead users.

>>> For devices that do not support brightness as a separate control we can create a file called

>>> max_brightness_<color> that defines the max that a specific color can be set to.  If max_brightness

>>> is set to 1 then create max_brightness_<color>.  If max_brightness > 1 then do not create the files.

>>

>> Right. We will need dedicated max_brightness for each color.

>> They should be placed also in the colors directory, next to the color

>> files.

>>

> 

> OK will document this.

> 

>>> I don't think we have fully vetted the brightness-model yet so I prefer to omit

>>> it and possibly introduce that later.

>>

>> We need to start from something. It will give better overview of the

>> whole idea.

>>

> 

> OK.  Don't get me wrong I don't oppose this idea I am just trying to figure out how the user space would

> know what models and brightness levels are available.


$ cat brightness-model
lp50xx-linear lp50xx-logarithmic onoff hsl-green hsl=blue

max_brightness will return available number of brightness levels
for each brightness model.

I'd not bother with presenting whole range of available color
presets. Userspace can verify the brightness->color mapping
by reading colors/<color> files after setting given brightness
level.

However, I'm not sure how useful it will be. This is a gist
of this whole discussion - we cannot be certain about exact
color effect produced with given settings.

> I mean we can read the brightness-models and present the available models but then how does the user know

> what and how many levels are in each model?  And how do we govern putting them in the right order?


`cat max_brightness` will return number of levels for the model
currently set. Regarding the order - we must rely on the DT array
arrangement. In case of hardware originated brightness model we
must trust hardware implementation.

> The DT file can get messed up, per the previous example

> rgb-green = <0x277c33 0x37b048 0x47e45d>;

> 

> This is assumed to be from dimmest to brightest.  But that is just an assumption

> 

> What if the entry looked like this?

> rgb-green = <0x37b048 0x277c33 0x47e45d>;


We can do nothing. It is just the cost of leaving some decisions to DT.

> Then echo 1 > brightness is not really a lower intensity then echo 2 > brightness.

> I know this is a product level issue but this can be misused and there is no way for maintainers

> or reviewers to really catch this error in code reviews.

> 

> The driver can map the brightness to the appropriate level requested by the class but again not

> sure how the user space can know what is available.  And there is nothing from stopping a

> definition of up to 2^32 brightness combinations.  This I know is unrealistic but the capability is there

> 

> I am wondering if there should be some sort of coefficient that can be defined that is

> applied to each color (no complex math).  I can see this working in a linear device but logrithimic

> maybe an issue.

> 

> Like

> 

> rgb-green = <0x277c33>;  //Coefficient used to set the dimmest allowed brightness for the color model.

> 

> echo 1 > brightness

> 

> red = 0x27

> green = 0x7c

> blue = 0x33

> 

> echo 2 > brightness

> 

> red = 0x28

> green = 0x7d

> blue = 0x34

> 

> echo 3 > brightness

> 

> red = 0x29

> green = 0x7e

> blue = 0x35

> 

> This example would give you 132 different brightness levels. green is the brightest defined color so the step calculation is

> 

> 255-124+1 = 132 (zero based) as 0 is off.

> 

> There can be a file called rgb_green_max which can be read to indicate how many brightness levels can be achieved.

> If the user goes beyond the steps then throw -EINVAL at them.

> 

> These brightness models probably should be put into a separate directory to isolate and not clutter the colors directory.

> And writing brightness to these models would be immediate no sync involved.


I intended that brightness-model location would be the main LED class
device directory.

And the whole concept is simple. We allow to set what we get from DT
or from the hardware. Without verification exceeding beyond
max_brightness values defined per iout.

-- 
Best regards,
Jacek Anaszewski
Pavel Machek Jan. 30, 2019, 10:35 p.m. | #6
On Wed 2019-01-30 12:30:05, Dan Murphy wrote:
> Add a documentation of LED Multicolor LED class specific

> sysfs attributes.


No, sorry. This does not most of the requirements.

								Pavel

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. Color list is for example here:

	 https://www.rapidtables.com/web/color/RGB_Color.html

    2a) LEDs probably slightly change color as they age. That's out of
    scope, unless the variation is much greater than on monitors.

    3a) 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.

	   https://en.wikipedia.org/wiki/Gamma_correction
	   #Windows, Mac, sRGB and TV/video standard gammas
	   https://en.wikipedia.org/wiki/SRGB#Specification_of_the_transformation

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.



> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

>  .../ABI/testing/sysfs-class-led-multicolor    | 38 +++++++++++++++++++

>  1 file changed, 38 insertions(+)

>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor

> 

> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor

> new file mode 100644

> index 000000000000..19f8da9b150e

> --- /dev/null

> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor

> @@ -0,0 +1,38 @@

> +What:		/sys/class/leds/<led>/color/sync_enable

> +Date:		January 2019

> +KernelVersion:	5.0

> +Contact:	Dan Murphy <dmurphy@ti.com>

> +Description:	read/write

> +		Writing a 1 to this file will enable the sychronization of all

> +		the defined color LEDs within the LED node.  Writing a 0 to

> +		this file will disable syncing.

> +

> +What:		/sys/class/leds/<led>/color/sync

> +Date:		January 2019

> +KernelVersion:	5.0

> +Contact:	Dan Murphy <dmurphy@ti.com>

> +Description:	write only

> +		Writing a 1 to this file while sync_enable is set to 1 will

> +		synchronize all defined LEDs within the LED node.  All LEDs

> +		defined will be configured based on the brightness that has

> +		been requested.

> +

> +		If sync_enable is set to 0 then writing a 1 to sync has no

> +		affect on the LEDs.

> +

> +What:		/sys/class/leds/<led>/color/<led color>

> +Date:		January 2019

> +KernelVersion:	5.0

> +Contact:	Dan Murphy <dmurphy@ti.com>

> +Description:	read/write

> +		These files are dynamically created based on the colors defined

> +		by the registrar of the class.

> +		The led color(s) can be but not limited to red, green, blue,

> +		white, amber and violet.  If sync is enabled then writing the

> +		brightness value of the LED is deferred until a 1 is

> +		written to /sys/class/leds/<led>/color/sync.  If syncing is

> +		disabled then the LED brightness value will be written

> +		immediately to the LED driver.

> +

> +		The value of the color is from 0 to

> +		/sys/class/leds/<led>/max_brightness.


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Dan Murphy Jan. 31, 2019, 1:48 p.m. | #7
Jacek

On 1/30/19 4:14 PM, Jacek Anaszewski wrote:
> Dan,

> 

> On 1/30/19 10:07 PM, Dan Murphy wrote:

>> Jacek

>>

>> On 1/30/19 2:20 PM, Jacek Anaszewski wrote:

>>> Dan,

>>>

>>> On 1/30/19 8:59 PM, Dan Murphy wrote:

>>>> Jacek

>>>>

>>>> On 1/30/19 1:37 PM, Jacek Anaszewski wrote:

>>>>> Hi Dan,

>>>>>

>>>>> Thank you for the RFC.

>>>>>

>>>>> One vital thing is missing - documentation of brightness file must

>>>>> be updated to define its semantics for LED multi color class.

>>>>>

>>>>> Either we need brightness-model file returning only "onoff" option

>>>>> available, or, for time being, fix the max_brightness for LED multi

>>>>> color class to 1 (which will map to max intensity level for each color).

>>>>>

>>>>

>>>> I can make max_brightness default to 1 if not set by the LED driver.

>>>>

>>>> But the LP50xx has brightness controls so setting max_brightness from the driver should over ride

>>>> the max of 1 in the upper level.

>>>

>>> Yes, so the max_brightness should be updated basing on current

>>> brightness-model. For LEDn_BRIGHTNESS of LP50xx we could have

>>> "hw" or maybe even better just "lp50xx-linear" and "lp50xx-logarithmic"

>>> - I just forgot about that capability of the device.

>>>

>>

>> OK maybe "hw" would make sense as there may be other devices that have dedicated brightness controls

>> over color controls.

> 

> Single "hw" doesn't address both linear and logarithmic.

> This is device specific, so I don't see anything wrong in

> "lp50xx-*", provided that it will be documented.

> 


OK.  This would need to be lp50xx specific.

>> Probably need to create a model for non-modeled cases like "rgb-independent".  Dumb name but I could not

>> think of anything better.

> 

> There is no point in having any rgb* brightness model since increasing

> rgb in an arbitrary way will not give the impression of increasing color

> intensity (lightness of the same hue). With DT defined hsl-<color>

> ranges there is no way to verify that levels arrangement makes sense

> with regard to preserving hue, this is a matter of trust. But we should

> state that it is highly recommended to obey this constraint so as to

> not mislead users.

> 


OK.  I will document this in v2 for discussion

>>>> For devices that do not support brightness as a separate control we can create a file called

>>>> max_brightness_<color> that defines the max that a specific color can be set to.  If max_brightness

>>>> is set to 1 then create max_brightness_<color>.  If max_brightness > 1 then do not create the files.

>>>

>>> Right. We will need dedicated max_brightness for each color.

>>> They should be placed also in the colors directory, next to the color

>>> files.

>>>

>>

>> OK will document this.

>>

>>>> I don't think we have fully vetted the brightness-model yet so I prefer to omit

>>>> it and possibly introduce that later.

>>>

>>> We need to start from something. It will give better overview of the

>>> whole idea.

>>>

>>

>> OK.  Don't get me wrong I don't oppose this idea I am just trying to figure out how the user space would

>> know what models and brightness levels are available.

> 

> $ cat brightness-model

> lp50xx-linear lp50xx-logarithmic onoff hsl-green hsl=blue

> 

> max_brightness will return available number of brightness levels

> for each brightness model.

> 


So max_brightness here would be dynamic?
Or could we read hsl-<color> and get the number of levels and not mess with the max_brightness definition?

Or should that be the current level?

> I'd not bother with presenting whole range of available color

> presets. Userspace can verify the brightness->color mapping

> by reading colors/<color> files after setting given brightness

> level.

> 


Userspace would have to have knowledge of the values set in order to verify the
color values.

> However, I'm not sure how useful it will be. This is a gist

> of this whole discussion - we cannot be certain about exact

> color effect produced with given settings.

> 

>> I mean we can read the brightness-models and present the available models but then how does the user know

>> what and how many levels are in each model?  And how do we govern putting them in the right order?

> 

> `cat max_brightness` will return number of levels for the model

> currently set. Regarding the order - we must rely on the DT array

> arrangement. In case of hardware originated brightness model we

> must trust hardware implementation.

> 


Another question on this is what is the proposed definition of the DT value?

is it supposed to be 0xrrggbb?  What about other colors like Amber?
This was demonstrated in one of the videos you sent.

Maybe it should be rgb-<color> and not hsl-<color> because these values are not hsl values
but instead RGB values.

>> The DT file can get messed up, per the previous example

>> rgb-green = <0x277c33 0x37b048 0x47e45d>;

>>

>> This is assumed to be from dimmest to brightest.  But that is just an assumption

>>

>> What if the entry looked like this?

>> rgb-green = <0x37b048 0x277c33 0x47e45d>;

> 

> We can do nothing. It is just the cost of leaving some decisions to DT.

> 


<snip>

> 

> I intended that brightness-model location would be the main LED class

> device directory.

> 

> And the whole concept is simple. We allow to set what we get from DT

> or from the hardware. Without verification exceeding beyond

> max_brightness values defined per iout.

> 


Yes this is simple will put some documentation around it for discussion

Dan

-- 
------------------
Dan Murphy
Jacek Anaszewski Jan. 31, 2019, 8:54 p.m. | #8
Hi Dan,

On 1/31/19 2:48 PM, Dan Murphy wrote:
> Jacek

> 

> On 1/30/19 4:14 PM, Jacek Anaszewski wrote:

>> Dan,

>>

>> On 1/30/19 10:07 PM, Dan Murphy wrote:

>>> Jacek

>>>

>>> On 1/30/19 2:20 PM, Jacek Anaszewski wrote:

>>>> Dan,

>>>>

>>>> On 1/30/19 8:59 PM, Dan Murphy wrote:

>>>>> Jacek

>>>>>

>>>>> On 1/30/19 1:37 PM, Jacek Anaszewski wrote:

>>>>>> Hi Dan,

>>>>>>

>>>>>> Thank you for the RFC.

>>>>>>

>>>>>> One vital thing is missing - documentation of brightness file must

>>>>>> be updated to define its semantics for LED multi color class.

>>>>>>

>>>>>> Either we need brightness-model file returning only "onoff" option

>>>>>> available, or, for time being, fix the max_brightness for LED multi

>>>>>> color class to 1 (which will map to max intensity level for each color).

>>>>>>

>>>>>

>>>>> I can make max_brightness default to 1 if not set by the LED driver.

>>>>>

>>>>> But the LP50xx has brightness controls so setting max_brightness from the driver should over ride

>>>>> the max of 1 in the upper level.

>>>>

>>>> Yes, so the max_brightness should be updated basing on current

>>>> brightness-model. For LEDn_BRIGHTNESS of LP50xx we could have

>>>> "hw" or maybe even better just "lp50xx-linear" and "lp50xx-logarithmic"

>>>> - I just forgot about that capability of the device.

>>>>

>>>

>>> OK maybe "hw" would make sense as there may be other devices that have dedicated brightness controls

>>> over color controls.

>>

>> Single "hw" doesn't address both linear and logarithmic.

>> This is device specific, so I don't see anything wrong in

>> "lp50xx-*", provided that it will be documented.

>>

> 

> OK.  This would need to be lp50xx specific.


Thinking more of it - "hw_logarithmic" and "hw_linear" should also do.
It would be even better option as it is generic.

My only concern here is how to document them. I we chose lp50xx-*
prefixed names, then we could just refer to the data sheet.

For generic hw_{logarithmic|linear} brightness models we will need
precise description. Actually it will be needed also for DT provided
color levels. People must have good reference to know how to arrange DT
color arrays.

It would be best to refer to standards, e.g. CIECAM02 [0], description
of hue [1] and the overall relationship between "six technically defined
dimensions of color appearance: brightness (luminance), lightness,
colorfulness, chroma, saturation, and hue." I've found also interesting
presentation [2].

After we agree on the interface I can try to put these together.

>>> Probably need to create a model for non-modeled cases like "rgb-independent".  Dumb name but I could not

>>> think of anything better.

>>

>> There is no point in having any rgb* brightness model since increasing

>> rgb in an arbitrary way will not give the impression of increasing color

>> intensity (lightness of the same hue). With DT defined hsl-<color>

>> ranges there is no way to verify that levels arrangement makes sense

>> with regard to preserving hue, this is a matter of trust. But we should

>> state that it is highly recommended to obey this constraint so as to

>> not mislead users.

>>

> 

> OK.  I will document this in v2 for discussion

> 

>>>>> For devices that do not support brightness as a separate control we can create a file called

>>>>> max_brightness_<color> that defines the max that a specific color can be set to.  If max_brightness

>>>>> is set to 1 then create max_brightness_<color>.  If max_brightness > 1 then do not create the files.

>>>>

>>>> Right. We will need dedicated max_brightness for each color.

>>>> They should be placed also in the colors directory, next to the color

>>>> files.

>>>>

>>>

>>> OK will document this.

>>>

>>>>> I don't think we have fully vetted the brightness-model yet so I prefer to omit

>>>>> it and possibly introduce that later.

>>>>

>>>> We need to start from something. It will give better overview of the

>>>> whole idea.

>>>>

>>>

>>> OK.  Don't get me wrong I don't oppose this idea I am just trying to figure out how the user space would

>>> know what models and brightness levels are available.

>>

>> $ cat brightness-model

>> lp50xx-linear lp50xx-logarithmic onoff hsl-green hsl=blue

>>

>> max_brightness will return available number of brightness levels

>> for each brightness model.

>>

> 

> So max_brightness here would be dynamic?


Yes.

> Or could we read hsl-<color> and get the number of levels and not mess with the max_brightness definition?


For hw_{linear|logarithmic| it will return the brightness resolution
supported by the hardware.
For hsl-<color> (or whatever we will call it) it will return the number
of elements in the array the currently chosen brightness-model refers
to.

> Or should that be the current level?


Hmm? Current level will be available after executing "cat brightness".

>> I'd not bother with presenting whole range of available color

>> presets. Userspace can verify the brightness->color mapping

>> by reading colors/<color> files after setting given brightness

>> level.

>>

> 

> Userspace would have to have knowledge of the values set in order to verify the

> color values.


For the single element array:

some-color-model = <0xaabbcc>;

user will get this:

$ echo some-color-model > brightness-model
$ echo 1 > brightness
$ cat colors/red   // prints 0xaa
$ cat colors/green // prints 0xbb
$ cat colors/blue  // prints 0xcc

>> However, I'm not sure how useful it will be. This is a gist

>> of this whole discussion - we cannot be certain about exact

>> color effect produced with given settings.

>>

>>> I mean we can read the brightness-models and present the available models but then how does the user know

>>> what and how many levels are in each model?  And how do we govern putting them in the right order?

>>

>> `cat max_brightness` will return number of levels for the model

>> currently set. Regarding the order - we must rely on the DT array

>> arrangement. In case of hardware originated brightness model we

>> must trust hardware implementation.

>>

> 

> Another question on this is what is the proposed definition of the DT value?

> 

> is it supposed to be 0xrrggbb?  What about other colors like Amber?

> This was demonstrated in one of the videos you sent.

> 

> Maybe it should be rgb-<color> and not hsl-<color> because these values are not hsl values

> but instead RGB values.


Right, it should be possible to have even more than 4 LEDs
in the LED multi-color class device. We shouldn't also limit
the number of brightness levels for a single LED to one byte,
as current framework also doesn't do that.

For 5 LEDs in the cluster, the array containing three
brightness level would look like below (dummy values):

purple-linear-0 = <198 323 442 238 43 90>;
purple-linear-1 = <200 323 442 238 43 90>;
purple-linear-2 = <398 323 442 238 43 90>;

One more thing related to the brightness-model interface -
instead of a file it must be a directory with files named
after colors ranges defined in DT. There is PAGE_SIZE limit
imposed on the sysfs file size, and we already had a report
about crash caused by triggers file size that exceeded this
limit due to large number of cpus available on the platform,
that created equally large number of cpu triggers.

Also, if we will have those files, we will be able to report the number
of available brightness levels per each color range.

>>> The DT file can get messed up, per the previous example

>>> rgb-green = <0x277c33 0x37b048 0x47e45d>;

>>>

>>> This is assumed to be from dimmest to brightest.  But that is just an assumption

>>>

>>> What if the entry looked like this?

>>> rgb-green = <0x37b048 0x277c33 0x47e45d>;

>>

>> We can do nothing. It is just the cost of leaving some decisions to DT.

>>

> 

> <snip>

> 

>>

>> I intended that brightness-model location would be the main LED class

>> device directory.

>>

>> And the whole concept is simple. We allow to set what we get from DT

>> or from the hardware. Without verification exceeding beyond

>> max_brightness values defined per iout.

>>

> 

> Yes this is simple will put some documentation around it for discussion

> 

> Dan

> 


[0] https://en.wikipedia.org/wiki/CIECAM02
[1] https://en.wikipedia.org/wiki/Hue
[2] http://rit-mcsl.org/fairchild/PDFs/AppearanceLec.pdf

-- 
Best regards,
Jacek Anaszewski
Vesa Jääskeläinen Feb. 8, 2019, 4:55 a.m. | #9
Hi All,

On 31/01/2019 0.35, Pavel Machek wrote:
> On Wed 2019-01-30 12:30:05, Dan Murphy wrote:

>> Add a documentation of LED Multicolor LED class specific

>> sysfs attributes.

> 

> No, sorry. This does not most of the requirements.

> 

> 								Pavel

> 

> Requirements for RGB LED interface:


...

I have tried to capture relevant parts of ideas and requirements and 
usage in a wiki page in github:

https://github.com/vesajaaskelainen/linux-multicolor-leds/wiki

I believe the discussion is good to perform in mailing list -- I am 
happy to update or give you access to update the wiki so we could have 
easy to use summary/details source during discussions.

Ideas on the interface seem to be a bit drifting so I apologize if some 
of Your ideas were not captured.

There has been at least two direct proposals for userspace interface and 
I tried to provide usage example also for Dan's proposal. Feel free to 
correct me if I made a mistake.

I believe it is a good to create summary page as there seems to many 
aspects to be though out. What do you feel on this approach?

Thanks,
Vesa Jääskeläinen
Vesa Jääskeläinen Feb. 8, 2019, 5:09 a.m. | #10
Hi All,

On 08/02/2019 6.55, Vesa Jääskeläinen wrote:
> Hi All,

> 

> On 31/01/2019 0.35, Pavel Machek wrote:

>> On Wed 2019-01-30 12:30:05, Dan Murphy wrote:

>>> Add a documentation of LED Multicolor LED class specific

>>> sysfs attributes.

>>

>> No, sorry. This does not most of the requirements.

>>

>>                                 Pavel

>>

>> Requirements for RGB LED interface:

> 

> ...

> 

> I have tried to capture relevant parts of ideas and requirements and 

> usage in a wiki page in github:

> 

> https://github.com/vesajaaskelainen/linux-multicolor-leds/wiki

> 

> I believe the discussion is good to perform in mailing list -- I am 

> happy to update or give you access to update the wiki so we could have 

> easy to use summary/details source during discussions.

> 

> Ideas on the interface seem to be a bit drifting so I apologize if some 

> of Your ideas were not captured.

> 

> There has been at least two direct proposals for userspace interface and 

> I tried to provide usage example also for Dan's proposal. Feel free to 

> correct me if I made a mistake.

> 

> I believe it is a good to create summary page as there seems to many 

> aspects to be though out. What do you feel on this approach?


And should we perhaps move this discussion only to linux-leds mailing 
list for successive replies :) ? So we don't generate too broad traffic.

Thanks,
Vesa Jääskeläinen
Vesa Jääskeläinen Feb. 9, 2019, 9:11 a.m. | #11
Hi All,

Dropped kernel-ml from this email.

On 08/02/2019 18.55, Dan Murphy wrote:
> Veas

> 

> On 2/7/19 11:09 PM, Vesa Jääskeläinen wrote:

>> Hi All,

>>

>> On 08/02/2019 6.55, Vesa Jääskeläinen wrote:

>>> Hi All,

>>>

>>> On 31/01/2019 0.35, Pavel Machek wrote:

>>>> On Wed 2019-01-30 12:30:05, Dan Murphy wrote:

>>>>> Add a documentation of LED Multicolor LED class specific

>>>>> sysfs attributes.

>>>>

>>>> No, sorry. This does not most of the requirements.

>>>>

>>>>                                  Pavel

>>>>

>>>> Requirements for RGB LED interface:

>>>

>>> ...

>>>

>>> I have tried to capture relevant parts of ideas and requirements and usage in a wiki page in github:

>>>

>>> https://github.com/vesajaaskelainen/linux-multicolor-leds/wiki

>>>

>>> I believe the discussion is good to perform in mailing list -- I am happy to update or give you access to update the wiki so we could have easy to use summary/details source during discussions.

>>>

>>> Ideas on the interface seem to be a bit drifting so I apologize if some of Your ideas were not captured.

>>>

>>> There has been at least two direct proposals for userspace interface and I tried to provide usage example also for Dan's proposal. Feel free to correct me if I made a mistake.

>>>

>>> I believe it is a good to create summary page as there seems to many aspects to be though out. What do you feel on this approach?

>>

>> And should we perhaps move this discussion only to linux-leds mailing list for successive replies :) ? So we don't generate too broad traffic.

>>

> 

> Thank you for this.  I have posted updated documentation and posted updated code that includes a test file that does

> nothing but register a dummy node.  This was tested on raspberry pi 5.0-rc kernel.

> 

> Also the code is updated for the LP50xx and a HACK for Droid4.

> 

> brightness-model is still an open topic on the best way to implement this so we have not introduced the code yet.

> 

> Here is the ABI doc

> http://git.ti.com/gitweb/?p=ti-analog-linux-kernel/dmurphy-analog.git;a=blob;f=Documentation/ABI/testing/sysfs-class-led-multicolor;h=45629199791285200e3775fc0b4dde1dfebb130f;hb=ce08183aa24edc0d883550339eef93fd72b4ac45

> 

> Here is the class description

> http://git.ti.com/gitweb/?p=ti-analog-linux-kernel/dmurphy-analog.git;a=blob;f=Documentation/leds/leds-class-multicolor.txt;h=357f045a826aac71d65704891c18e7fb31e5cb9b;hb=refs/heads/multicolor_class


Updated the page and added links to those.

Also added section "Triggering uses maximum brightness".

***

I believe major difference between the models is how one changes 
individual color elements.

In Dan's model you have own sysfs node for each indivudal color element 
(red/brightness, green/brightness, ...) and in Vesa's you have one sysfs 
node (color) that is used to update all color elements in one go.

In addition Dan's model have configurable synchronization/atomic change 
configuration sysfs node and mechanism.

***

Pavel's point of needing to have ability to change color to "white" 
could be done with either model.

With Dan's model we would have user space writing the values to the 
kernel sysfs nodes but it requires to have some device specific 
translation table (could be ICC type of profile) from "white" to 
individual values.

With Vesa's model same ICC profile model could still be used. Model for 
color could be extended to also support color mapping table in 
devicetree then symbolic name could be written to "color" node (eg. echo 
white > color) but that would require preregistration for colors in 
devicetree.

As there is no resolution yet for color space support within kernel 
adjusting brightness for led color with led level "brightness" node is 
problematic.

If there is hardware support then hardware would be doing that but 
software approach needs agreeing what level support is on where.

Vesa proposed approach for linearity mapping problem to use ramp tables 
defined in devicetree similar to what is used with LCD backlight driver. 
I believe this could work with both Dan's and Vesa's model.

Vesa proposed that linearity mapping problem could be partially be 
responsibility for device developer so that they can adjust the mode 
with "britghtness_model" (or with devicetree). This in combination with 
ramp tables could make adjusting with numbers ranging from 0-MAX easier 
for user to grasp and could compensate also for different color element 
balances transparently from user.

I would like to keep user thinking about LED's color elements in linear 
mode in user space interface point of view. So that there would be read 
values ranging from 0 .. 255. This would make it easier for user to 
understand what is happening. I don't like to see that user needs to 
think in model that they are not common with (RGB and HSV/HSL I could 
categorize as known models). Eg. I do not want to see that they need to 
think in raw PWM values (->ramp table could solve this).

***

I tried to map both Dan's and Vesa's model to our usage:

Our devices currently have either PWM or GPIO controlled LEDs so our 
hardware models are simple. And we want to express only basic set of 
colors. But if we want to see orange blinking it should be blinking in 
orange and not with some transitional colors. PWM is mainly used to 
allow end user control for LED brightness. We basically have a status 
led that indicates status of the device. Status LED will have following 
color elements depending on device; red-green or red-green-blue. Other 
LEDs are automatically hardware controlled like Ethernet LEDs.

With Dan's in our use case one would always need to use synchronization 
mode enabled and updating the color in scripts/programs would require 
multiple commands to be executed or multiple writes to sysfs (with rgb 
led 4 x sysfs writes). Problem would come with Dan's model that we may 
need to have custom configuration per product as we have different 
amount of color elements in leds depending on product and we could not 
share the same code with all products unless there would be some user 
space helper library that would be used in the software adjusting the color.

With Vesa's model in our use case scripting would be a bit simpler but 
have same expressivity as Dan's model. Down side with the model is that 
you need to know beforehand what indices in color value array means 
which would create problem with different amount of color elements in 
leds. We could manage this internally probably by using color mapping 
table in devicetree and then set color with name.

In both models would need to have led level brightness control and then 
triggering would need to be fixed to support that as we need to have end 
user configurable dimmed leds. We do not need high accuracy color 
representation but it should be tunable so that when we expect to see 
orange we get the orange that is close enough. White we do not use 
currently.

As a device designer I would like to hide complexity of color mapping to 
devicetree so that user space usage could then be shared in multiple 
devices.

***

Now I have a question to our maintainers Pavel and Jacek. Do you think 
that we have enough input to formulate next level proposal? Are we 
missing some points? Please give guidance on how to proceed with the 
matter. I believe that once we have user space interface model next 
issues would be easier to solve.

Thanks,
Vesa Jääskeläinen
Jacek Anaszewski Feb. 10, 2019, 3:40 p.m. | #12
Hi Vesa,

On 2/9/19 10:11 AM, Vesa Jääskeläinen wrote:
> Hi All,

> 

> Dropped kernel-ml from this email.

> 

> On 08/02/2019 18.55, Dan Murphy wrote:

>> Veas

>>

>> On 2/7/19 11:09 PM, Vesa Jääskeläinen wrote:

>>> Hi All,

>>>

>>> On 08/02/2019 6.55, Vesa Jääskeläinen wrote:

>>>> Hi All,

>>>>

>>>> On 31/01/2019 0.35, Pavel Machek wrote:

>>>>> On Wed 2019-01-30 12:30:05, Dan Murphy wrote:

>>>>>> Add a documentation of LED Multicolor LED class specific

>>>>>> sysfs attributes.

>>>>>

>>>>> No, sorry. This does not most of the requirements.

>>>>>

>>>>>                                  Pavel

>>>>>

>>>>> Requirements for RGB LED interface:

>>>>

>>>> ...

>>>>

>>>> I have tried to capture relevant parts of ideas and requirements and 

>>>> usage in a wiki page in github:

>>>>

>>>> https://github.com/vesajaaskelainen/linux-multicolor-leds/wiki


Thank you for your initiative!

>>>>

>>>> I believe the discussion is good to perform in mailing list -- I am 

>>>> happy to update or give you access to update the wiki so we could 

>>>> have easy to use summary/details source during discussions.

>>>>

>>>> Ideas on the interface seem to be a bit drifting so I apologize if 

>>>> some of Your ideas were not captured.

>>>>

>>>> There has been at least two direct proposals for userspace interface 

>>>> and I tried to provide usage example also for Dan's proposal. Feel 

>>>> free to correct me if I made a mistake.

>>>>

>>>> I believe it is a good to create summary page as there seems to many 

>>>> aspects to be though out. What do you feel on this approach?

>>>

>>> And should we perhaps move this discussion only to linux-leds mailing 

>>> list for successive replies :) ? So we don't generate too broad traffic.

>>>

>>

>> Thank you for this.  I have posted updated documentation and posted 

>> updated code that includes a test file that does

>> nothing but register a dummy node.  This was tested on raspberry pi 

>> 5.0-rc kernel.

>>

>> Also the code is updated for the LP50xx and a HACK for Droid4.

>>

>> brightness-model is still an open topic on the best way to implement 

>> this so we have not introduced the code yet.

>>

>> Here is the ABI doc

>> http://git.ti.com/gitweb/?p=ti-analog-linux-kernel/dmurphy-analog.git;a=blob;f=Documentation/ABI/testing/sysfs-class-led-multicolor;h=45629199791285200e3775fc0b4dde1dfebb130f;hb=ce08183aa24edc0d883550339eef93fd72b4ac45 

>>

>>

>> Here is the class description

>> http://git.ti.com/gitweb/?p=ti-analog-linux-kernel/dmurphy-analog.git;a=blob;f=Documentation/leds/leds-class-multicolor.txt;h=357f045a826aac71d65704891c18e7fb31e5cb9b;hb=refs/heads/multicolor_class 

>>

> 

> Updated the page and added links to those.

> 

> Also added section "Triggering uses maximum brightness".

> 

> ***

> 

> I believe major difference between the models is how one changes 

> individual color elements.

> 

> In Dan's model you have own sysfs node for each indivudal color element 

> (red/brightness, green/brightness, ...) and in Vesa's you have one sysfs 

> node (color) that is used to update all color elements in one go.

> 

> In addition Dan's model have configurable synchronization/atomic change 

> configuration sysfs node and mechanism.

> 

> ***

> 

> Pavel's point of needing to have ability to change color to "white" 

> could be done with either model.


If we changed the definition of the new LED class from RGB to multi
color then this argument would become invalid. Especially because the
presence of all red, green and blue colors would not be guaranteed.

> With Dan's model we would have user space writing the values to the 

> kernel sysfs nodes but it requires to have some device specific 

> translation table (could be ICC type of profile) from "white" to 

> individual values.

> 

> With Vesa's model same ICC profile model could still be used. Model for 

> color could be extended to also support color mapping table in 

> devicetree then symbolic name could be written to "color" node (eg. echo 

> white > color) but that would require preregistration for colors in 

> devicetree.

> 

> As there is no resolution yet for color space support within kernel 

> adjusting brightness for led color with led level "brightness" node is 

> problematic.

> 

> If there is hardware support then hardware would be doing that but 

> software approach needs agreeing what level support is on where.

> 

> Vesa proposed approach for linearity mapping problem to use ramp tables 

> defined in devicetree similar to what is used with LCD backlight driver. 

> I believe this could work with both Dan's and Vesa's model.

> 

> Vesa proposed that linearity mapping problem could be partially be 

> responsibility for device developer so that they can adjust the mode 

> with "britghtness_model" (or with devicetree). This in combination with 

> ramp tables could make adjusting with numbers ranging from 0-MAX easier 

> for user to grasp and could compensate also for different color element 

> balances transparently from user.

> 

> I would like to keep user thinking about LED's color elements in linear 

> mode in user space interface point of view. So that there would be read 

> values ranging from 0 .. 255. This would make it easier for user to 

> understand what is happening. I don't like to see that user needs to 

> think in model that they are not common with (RGB and HSV/HSL I could 

> categorize as known models). Eg. I do not want to see that they need to 

> think in raw PWM values (->ramp table could solve this).


I propose to stick to the current notion of brightness level only.
With LED multi color class we would be just handling several iouts
under a single multi color node.

> ***

> 

> I tried to map both Dan's and Vesa's model to our usage:

> 

> Our devices currently have either PWM or GPIO controlled LEDs so our 

> hardware models are simple. And we want to express only basic set of 

> colors. But if we want to see orange blinking it should be blinking in 

> orange and not with some transitional colors. PWM is mainly used to 

> allow end user control for LED brightness. We basically have a status 

> led that indicates status of the device. Status LED will have following 

> color elements depending on device; red-green or red-green-blue. Other 

> LEDs are automatically hardware controlled like Ethernet LEDs.

> 

> With Dan's in our use case one would always need to use synchronization 

> mode enabled and updating the color in scripts/programs would require 

> multiple commands to be executed or multiple writes to sysfs (with rgb 

> led 4 x sysfs writes). Problem would come with Dan's model that we may 

> need to have custom configuration per product as we have different 

> amount of color elements in leds depending on product and we could not 

> share the same code with all products unless there would be some user 

> space helper library that would be used in the software adjusting the 

> color.

> 

> With Vesa's model in our use case scripting would be a bit simpler but 

> have same expressivity as Dan's model. Down side with the model is that 

> you need to know beforehand what indices in color value array means 

> which would create problem with different amount of color elements in 

> leds. We could manage this internally probably by using color mapping 

> table in devicetree and then set color with name.

> 

> In both models would need to have led level brightness control and then 

> triggering would need to be fixed to support that as we need to have end 

> user configurable dimmed leds. We do not need high accuracy color 

> representation but it should be tunable so that when we expect to see 

> orange we get the orange that is close enough. White we do not use 

> currently.

> 

> As a device designer I would like to hide complexity of color mapping to 

> devicetree so that user space usage could then be shared in multiple 

> devices.


Dan's proposal is going to be supplemented with brightness-models.
This way it will fulfill your needs and will reduce the number of
sysfs calls to set the needed color, when there are presets available.

> 

> ***

> 

> Now I have a question to our maintainers Pavel and Jacek. Do you think 

> that we have enough input to formulate next level proposal? Are we 

> missing some points? Please give guidance on how to proceed with the 

> matter. I believe that once we have user space interface model next 

> issues would be easier to solve.


We've been working on the framework with Dan through private channels
during last weeks. Until it is ready for posting to the list I am
submitting for discussion my DT design based on lp5024 device
example. Brightness models are also covered.


led-controller@0 {
     compatible = "ti,lp5024";

     led-module@0 {
         label = "rgb:led_mod0";
         reg = <0x0>; // this refers to RGB LED module
         available-brightness-models = <&lp5024-brightness-models>;

         led@0 {
             reg = <0x0>;        // this refers to iout
             color = LED_COLOR_ID_RED;
         };

         led@1 {
             reg = <0x1>;
             color = LED_COLOR_ID_GREEN;
         };

         led@2 {
             reg = <0x2>;
             color = LED_COLOR_ID_BLUE;
         };
     };

     led-module@1 {
         label = "rgb:led_mod1";
         reg = <0x0>; // this refers to RGB LED module
         available-brightness-models = <&lp5024-brightness-models>;

         led@0 {
             reg = <0x3>;
             color = LED_COLOR_ID_RED;
         };

         led@1 {
             reg = <0x4>;
             color = LED_COLOR_ID_GREEN;
         };

         led@2 {
             reg = <0x5>;
             color = LED_COLOR_ID_BLUE;
         };
     };


     lp5024-brightness-models: brightness-models {
         model@0 {
             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>;
         };

         model@1 {
             name = "orange";
             layout = <LED_COLOR_ID_RED
                       LED_COLOR_ID_GREEN
                       LED_COLOR_ID_BLUE>;
             level-1 = <236 140 16>;
             level-2 = <236 157 55>;
             level-3 = <236 183 115>;
         };
     };
};



-- 
Best regards,
Jacek Anaszewski
Pavel Machek March 2, 2019, 10:49 p.m. | #13
Hi!

> I believe major difference between the models is how one changes individual

> color elements.

> 

> In Dan's model you have own sysfs node for each indivudal color element

> (red/brightness, green/brightness, ...) and in Vesa's you have one sysfs

> node (color) that is used to update all color elements in one go.

> 

> In addition Dan's model have configurable synchronization/atomic change

> configuration sysfs node and mechanism.


I like your version more.

> ***

> 

> Pavel's point of needing to have ability to change color to "white" could be

> done with either model.

> 

> With Dan's model we would have user space writing the values to the kernel

> sysfs nodes but it requires to have some device specific translation table

> (could be ICC type of profile) from "white" to individual values.

> 

> With Vesa's model same ICC profile model could still be used. Model for

> color could be extended to also support color mapping table in devicetree

> then symbolic name could be written to "color" node (eg. echo white > color)

> but that would require preregistration for colors in devicetree.


No. If we decide to do color conversions in kernel, we should do it
for all colors, not just for selected list.

Color conversion in kernel is doable, but will likely mean that full
set of possible colors/brightnesses will not be available, as I
explained elsewhere.

> Vesa proposed approach for linearity mapping problem to use ramp tables

> defined in devicetree similar to what is used with LCD backlight driver. I

> believe this could work with both Dan's and Vesa's model.


Lets not do that. Replacing computation with table is not really
improvement, as it a) costs us memory, b) will be less precise, c)
people will not get the tables right, as they will be hard to measure.

> I would like to keep user thinking about LED's color elements in linear mode

> in user space interface point of view. So that there would be read values

> ranging from 0 .. 255. This would make it easier for user to understand what

> is happening. I don't like to see that user needs to think in model that

> they are not common with (RGB and HSV/HSL I could categorize as known

> models). Eg. I do not want to see that they need to think in raw PWM values

> (->ramp table could solve this).


We should really make it simple to implement in kernel.. and user
space can adapt. Yes, it probably means we should do shared library to
make it easy for "user".

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
new file mode 100644
index 000000000000..19f8da9b150e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
@@ -0,0 +1,38 @@ 
+What:		/sys/class/leds/<led>/color/sync_enable
+Date:		January 2019
+KernelVersion:	5.0
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	read/write
+		Writing a 1 to this file will enable the sychronization of all
+		the defined color LEDs within the LED node.  Writing a 0 to
+		this file will disable syncing.
+
+What:		/sys/class/leds/<led>/color/sync
+Date:		January 2019
+KernelVersion:	5.0
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	write only
+		Writing a 1 to this file while sync_enable is set to 1 will
+		synchronize all defined LEDs within the LED node.  All LEDs
+		defined will be configured based on the brightness that has
+		been requested.
+
+		If sync_enable is set to 0 then writing a 1 to sync has no
+		affect on the LEDs.
+
+What:		/sys/class/leds/<led>/color/<led color>
+Date:		January 2019
+KernelVersion:	5.0
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	read/write
+		These files are dynamically created based on the colors defined
+		by the registrar of the class.
+		The led color(s) can be but not limited to red, green, blue,
+		white, amber and violet.  If sync is enabled then writing the
+		brightness value of the LED is deferred until a 1 is
+		written to /sys/class/leds/<led>/color/sync.  If syncing is
+		disabled then the LED brightness value will be written
+		immediately to the LED driver.
+
+		The value of the color is from 0 to
+		/sys/class/leds/<led>/max_brightness.