[v3,1/9] leds: multicolor: Add sysfs interface definition

Message ID 20190523190820.29375-2-dmurphy@ti.com
State New
Headers show
Series
  • [v3,1/9] leds: multicolor: Add sysfs interface definition
Related show

Commit Message

Dan Murphy May 23, 2019, 7:08 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    | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor

-- 
2.21.0.5.gaeb582a983

Comments

Pavel Machek May 27, 2019, 10:33 a.m. | #1
On Thu 2019-05-23 14:08:12, 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    | 57 +++++++++++++++++++

>  1 file changed, 57 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..2f102ede258b

> --- /dev/null

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

> @@ -0,0 +1,57 @@

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

> +Date:		April 2019


I believe I suggested more reasonable interface. Why not use that?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Dan Murphy May 28, 2019, 11:34 a.m. | #2
Pavel

On 5/27/19 7:45 PM, Dan Murphy wrote:
> Pavel

>

> On 5/27/19 5:33 AM, Pavel Machek wrote:

>> On Thu 2019-05-23 14:08:12, 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    | 57 

>>> +++++++++++++++++++

>>>   1 file changed, 57 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..2f102ede258b

>>> --- /dev/null

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

>>> @@ -0,0 +1,57 @@

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

>>> +Date:        April 2019

>> I believe I suggested more reasonable interface. Why not use that?

>>

>

> Can you please provide the reference to your interface?

>

I think I found the suggestion [0].  Assuming that was the suggestion it 
violates the kernel 1 value/file and there was agreement that this 
interface had value. In testing the interface, it made sense to be able 
to setup a color after writing each color brightness file and then 
syncing the color LEDs with the new brightness levels.  It also provides 
flexibility to be able to set a single LED color without writing sync.  
The decision to do either or should really be a user space decision.   
If I have referenced the wrong thread please let me know which thread 
you are referring to.

[0] https://lore.kernel.org/patchwork/patch/1057044/

<snip>

Dan
Jacek Anaszewski May 28, 2019, 5:44 p.m. | #3
Dan,

On 5/28/19 7:32 PM, Dan Murphy wrote:
> Jacek

> 

> On 5/27/19 3:00 PM, Jacek Anaszewski wrote:

>> Hi Dan,

>>

>> Thank you for the update.

>>

>> One thing is missing here - we need to document how legacy brightness

>> levels map to the sub-LED color levels, i.e. what you do in

>> multicolor_set_brightness().

> 

> 

> Ok so i will need to document the algorithm that is used to determine 

> the color LED brightness.


Right, and please send just an update of that single patch.

-- 
Best regards,
Jacek Anaszewski
Dan Murphy May 28, 2019, 6:19 p.m. | #4
Jacek

On 5/28/19 12:44 PM, Jacek Anaszewski wrote:
> Dan,

>

> On 5/28/19 7:32 PM, Dan Murphy wrote:

>> Jacek

>>

>> On 5/27/19 3:00 PM, Jacek Anaszewski wrote:

>>> Hi Dan,

>>>

>>> Thank you for the update.

>>>

>>> One thing is missing here - we need to document how legacy brightness

>>> levels map to the sub-LED color levels, i.e. what you do in

>>> multicolor_set_brightness().

>>

>>

>> Ok so i will need to document the algorithm that is used to determine 

>> the color LED brightness.

>

> Right, and please send just an update of that single patch.

>


So you are asking for v4 with only this patch updated.


Dan
Jacek Anaszewski May 28, 2019, 6:29 p.m. | #5
On 5/28/19 8:19 PM, Dan Murphy wrote:
> Jacek

> 

> On 5/28/19 12:44 PM, Jacek Anaszewski wrote:

>> Dan,

>>

>> On 5/28/19 7:32 PM, Dan Murphy wrote:

>>> Jacek

>>>

>>> On 5/27/19 3:00 PM, Jacek Anaszewski wrote:

>>>> Hi Dan,

>>>>

>>>> Thank you for the update.

>>>>

>>>> One thing is missing here - we need to document how legacy brightness

>>>> levels map to the sub-LED color levels, i.e. what you do in

>>>> multicolor_set_brightness().

>>>

>>>

>>> Ok so i will need to document the algorithm that is used to determine 

>>> the color LED brightness.

>>

>> Right, and please send just an update of that single patch.

>>

> 

> So you are asking for v4 with only this patch updated.


Not exactly - I am asking for v4 of this patch. We don't need
to spam the lists with the rest of unaltered patches from the series.

The interface seems to be the most vital part of this patch set,
and it is possible that it will undergo at least slight modifications.

We will move to discussing the code once we achieve a total consensus.

-- 
Best regards,
Jacek Anaszewski
Dan Murphy May 30, 2019, 2:30 p.m. | #6
Jacek

On 5/28/19 1:29 PM, Jacek Anaszewski wrote:
> On 5/28/19 8:19 PM, Dan Murphy wrote:

>> Jacek

>>

>> On 5/28/19 12:44 PM, Jacek Anaszewski wrote:

>>> Dan,

>>>

>>> On 5/28/19 7:32 PM, Dan Murphy wrote:

>>>> Jacek

>>>>

>>>> On 5/27/19 3:00 PM, Jacek Anaszewski wrote:

>>>>> Hi Dan,

>>>>>

>>>>> Thank you for the update.

>>>>>

>>>>> One thing is missing here - we need to document how legacy brightness

>>>>> levels map to the sub-LED color levels, i.e. what you do in

>>>>> multicolor_set_brightness().

>>>>

>>>>

>>>> Ok so i will need to document the algorithm that is used to 

>>>> determine the color LED brightness.

>>>

>>> Right, and please send just an update of that single patch.

>>>

>>

>> So you are asking for v4 with only this patch updated.

>

> Not exactly - I am asking for v4 of this patch. We don't need

> to spam the lists with the rest of unaltered patches from the series.

>

> The interface seems to be the most vital part of this patch set,

> and it is possible that it will undergo at least slight modifications.

>

> We will move to discussing the code once we achieve a total consensus.

>


Sorry for the late reply.  OK I have updated the sysfs documentation but 
in doing so I am also going to send in v4 of the sysfs documentation as 
it explains the interfaces in more detail. Also I will be adding more 
technical information into that sysfs doc on what is done and what to 
expect and it's usage.


Dan
Dan Murphy May 30, 2019, 8:43 p.m. | #7
Pavel

On 5/30/19 2:40 PM, Pavel Machek wrote:
> On Tue 2019-05-28 06:34:47, Dan Murphy wrote:

>> Pavel

>>

>> On 5/27/19 7:45 PM, Dan Murphy wrote:

>>> Pavel

>>>

>>> On 5/27/19 5:33 AM, Pavel Machek wrote:

>>>> On Thu 2019-05-23 14:08:12, 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    | 57

>>>>> +++++++++++++++++++

>>>>>    1 file changed, 57 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..2f102ede258b

>>>>> --- /dev/null

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

>>>>> @@ -0,0 +1,57 @@

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

>>>>> +Date:        April 2019

>>>> I believe I suggested more reasonable interface. Why not use that?

>>>>

>>> Can you please provide the reference to your interface?

>>>

>> I think I found the suggestion [0].  Assuming that was the suggestion it

>> violates the kernel 1 value/file and there was agreement that this interface

>> had value. In testing the interface, it made sense to be able to

> 1 value/file is actually slightly more complex rule:

>

> 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.

>

> See sysfs.txt. Proposed "sync_enable" is ugly enough, and the values

> really are array of values of same type, so we should be ok with nicer

> interface.


sync_enable is a binary input 0 or 1, but I could change that to ASCII 
"enabled/disabled".  And the sole purpose of this file is to enable or 
disable the synchronization of the grouped monochrome color LED 
brightness settings within that given directory.  So not sure why the 
sync_enable is ugly since it is meant to enable a feature of the class.  
Honestly I could drop the whole sync feature but I have found value in 
this interface through my testing both with a pulsing script I wrote and 
the notint.py you wrote.  This feature is meant for slower processors or 
processes with low priority that could be interrupted or switched out 
during writing the LED and cause LED color flicker.  Priming the colors 
and then updating all the available LEDs in the group in the kernel 
space with a single write to the sync file is a little more intuitive.  
I could change both the sync and sync_enable to module_params instead.

After mulling over the proposed solution of passing in the array of 
values I came to the conclusion this is actually more complicated than 
it needs to be and this implementation is not flexible.

There would need to be additional files that need to present information 
to the user space on what LEDs available, how many LEDs are available 
and what the LED array order is.  The user interface cannot always 
assume that the array is RGB.

There could be RGBW, RGBA, RGBWA, GBW or WIR or any combination of 
varying LED colors.  The interface that is proposed would need to have a 
varying array as the input.

I have posted v4 of the documentation of this patchset with updated 
sysfs documentation and ABI description.

Dan

>

> Best regards,

> 									Pavel

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..2f102ede258b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
@@ -0,0 +1,57 @@ 
+What:		/sys/class/leds/<led>/colors/sync_enable
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	read/write
+		Writing a 1 to this file will enable the synchronization of all
+		the defined color LEDs within the LED node.  Brightness values
+		for each LED will be stored and written when sync is set to 1.
+		Writing a 0 to this file will disable syncing and allow
+		individual control of the LEDs brightness settings.
+
+What:		/sys/class/leds/<led>/colors/sync
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	write only
+		Writing a 1 to this file while sync_enable is set to 1 will
+		write the current brightness values to 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>/colors/<led_color>/brightness
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	read/write
+		The led_color directory is dynamically created based on the
+		colors defined by the registrar of the class.
+		The led_color can be but not limited to red, green, blue,
+		white, amber, yellow and violet.  Drivers can also declare a
+		LED color for presentation.  There is one directory per color
+		presented.  The brightness file is created under each
+		led_color directory and controls the individual LED color
+		setting.
+
+		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>/colors/<led_color>/max_brightness.
+
+What:		/sys/class/leds/<led>/colors/<led_color>/max_brightness
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	read only
+		Maximum brightness level for the LED color, default is
+		255 (LED_FULL).
+
+		If the LED does not support different brightness levels, this
+		should be 1.