Re: [PATCH v8 1/2] leds: core: Introduce LED pattern trigger

[Jacek Anaszewski]

Pavel,

On 09/10/2018 11:19 PM, Pavel Machek wrote:
> On Fri 2018-09-07 22:02:08, Bjorn Andersson wrote:
>> On Tue 04 Sep 04:01 PDT 2018, Baolin Wang wrote:
>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> [..]
>>> +What:		/sys/class/leds/<led>/hw_pattern
>>> +Date:		September 2018
>>> +KernelVersion:	4.20
>>> +Description:
>>> +		Specify a hardware pattern for the LED, for LED hardware that
>>> +		supports autonomously controlling brightness over time, according
>>> +		to some preprogrammed hardware patterns.
>>> +
>>> +		Since different LED hardware can have different semantics of
>>> +		hardware patterns, each driver is expected to provide its own
>>> +		description for the hardware patterns in their ABI documentation
>>> +		file.
>>> +
>>
>> So, after a full circle we're back at drivers with support for hardware
>> patterns should have their own ABI for setting that pattern.
>>
>> The controls for my hardware is:
>> * a list of brightness values
>> * the rate of the pattern
>> * a flag to indicate that the pattern should be played from start
>>   to end, end to start or start to end to start
>> * a boolean indicating if the pattern should be played once or repeated
>>   indefinitely.
> 
> No, we are not back to full circle.
> 
> Or at least we should not be.
> 
> Yes, hw_pattern can have some limitation pattern does not, but if you
> take values from hw_pattern file and put them into pattern file, you
> should get the same pattern (with more power being consumed). And that
> property is kind of important for me, because it should keep the ABI
> reasonable.

If you looked at what we agreed on with Baolin, you'd realize
that this property is in no way preserved.
This is what the whole story is about - we're introducing hw_pattern
because of difficulties in describing breathing pattern by a series
of [brightness delta_t] tuples.

And Bjorn presented another example. I'm inclined to leave the
definition of hw_pattern semantics to the LED class drivers,
and allow them to create related sysfs files.

>>> +What:		/sys/class/leds/<led>/repeat
>>> +Date:		September 2018
>>> +KernelVersion:	4.20
>>> +Description:
>>> +		Specify a pattern repeat number. 0 means repeat indefinitely.
>>> +
>>> +		This file will always return the originally written repeat
>>> +		number.
>>
>> I'm still convinced that this will confuse our users and to me it would
>> be more logical if this denotes the number of times the pattern should
>> be repeated, with e.g. negative numbers denoting infinite.
>>
>> In particular I expect to have to explain why my driver expects that you
>> write 0 in the file named "repeat" to make it repeat and 1 to make it
>> not repeat.
> 
> Ok, -1 works for me :-).
> 
> Thanks,
> 									Pavel
> 

-- 
Best regards,
Jacek Anaszewski