lkml.org 
[lkml]   [2019]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition
From
Date
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

\
 
 \ /
  Last update: 2019-05-30 22:44    [W:0.127 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site