Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface

From: Jacek Anaszewski
Date: Tue Jul 17 2018 - 16:26:16 EST


On 07/16/2018 11:56 PM, Pavel Machek wrote:
Hi!

echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere

s/somewhere/pattern/

pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.


I don't think this is the best way. For example, if you want more than one
LED to have the same pattern, then the patterns will not be synchronized
between the LEDs. The same things happens now with many of the existing
triggers. For example, if I have two LEDs side-by-side using the heartbeat
trigger, they may blink at the same time or they may not, which is not
very nice. I think we can make something better.

Yes, synchronization would be nice -- it is really a must for RGB LEDs
-- but I believe it is too much to ask from Baolin at the moment.

It is virtually impossible to enforce synchronous blinking for the
LEDs driven by different hardware due to:

- different hardware means via which brightness is set (MMIO, I2C, SPI,
PWM and other pulsed fashion based protocols),
- the need for deferring brightness setting to a workqueue task to
allow for setting LED brightness from atomic context,
- contention on locks

I disagree here. Yes, it would be hard to synchronize blinking down to
microseconds, but it would be easy to get synchronization right down
to miliseconds and humans will not be able to tell the difference.

There have been problems with blink interval stability close to
1ms, and thus there were some attempts of employing hr timers,
which in turn introduced a new class of issues related to
system performance etc.

For the LEDs driven by the same chip it would make more sense
to allow for synchronization, but it can be achieved on driver
level, with help of some subsystem level interface to indicate
which LEDs should be synchronized.

However, when we start to pretend that we can synchronize the
devices, we must answer how accurate we can be. The accuracy
will decrease as blink frequency rises. We'd need to define
reliability limit.

We don't need _that_ ammount of overengineering. We just need to
synchronize them well enough :-).

Well, it would be disappointing for the users to realize that
they don't get the effect advertised by the ABI documentation.

We've had few attempts of approaching the subject of synchronized
blinking but none of them proved to be good enough to be merged.

I'm sure interested person could do something like that in less than
two weeks fulltime... It is not rocket science, just a lot of work in
kernel...

But patterns are few years overdue and I believe we should not delay
them any further.

So... I guess I agree with Jacek in the end :-).

How about taking Baolin's patches as of v5? Later, provided that
the pattern trigger yet to be implemented will create pattern file
on activation, we'll need to initialize default-trigger DT property,
to keep the interface unchanged.

--
Best regards,
Jacek Anaszewski