| Message ID | 20190523190820.29375-2-dmurphy@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/9] leds: multicolor: Add sysfs interface definition | expand |
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
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
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
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
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
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
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
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.
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