Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 09/01/2016 07:25 AM, RafaÅ MiÅecki wrote:
> On 31 August 2016 at 21:00, RafaÅ MiÅecki <zajec5@xxxxxxxxx> wrote:
> > On 31 August 2016 at 20:23, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > > On Tue, 30 Aug 2016, RafaÅ MiÅecki wrote:
> > > > Not really as it won't cover some pretty common use cases. Many home
> > > > routers have few USB ports (2-5) and only 1 USB LED. It has to be
> > > > possible to assign few USB ports to a single LED (trigger). That way
> > > > LED should be turned on (and kept on) if there is at least 1 USB
> > > > device connected. You obviously can't do:
> > > > echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger
> > > >
> > > > This was already brought up by Rob (who mentioned CPU trigger) and I
> > > > replied him pretty much the same way in:
> > > > https://lkml.org/lkml/2016/7/29/38
> > > > (reply starts with "Anyway, the serious limitation I see").
> > >
> > > The code for a bunch of triggers must already be written.  What would
> > > the user do if he wanted to flash a single LED in response to both
> > > CPU activity and MTD activity?  If not
> > >
> > >         echo "cpu mtd" >/sys/class/leds/foo/trigger
> > >
> > > then what?
> >
> > Well, it sounds like a new feature then. Shall we add an extra API
> > with a request function for turning LED on? It could internally count
> > how many requests were raised and keep LED on as long as there is at
> > least 1 left. I guess we should implement it in trigger "subsystem"
> > (if I can call it so). Does it sound like a good plan?
>
> I'm pretty sure noone ever planned to have more than 1 trigger
> assigned to a single LED. I just realized there will be a problem with
> proposed solution: sysfs files conflict.
>
> Consider 2 existing triggers for a moment:
> 1) oneshot: it creates following sysfs files:
> /sys/class/leds/foo/delay_on
> /sys/class/leds/foo/delay_off
> /sys/class/leds/foo/invert
> /sys/class/leds/foo/shot
> 2) timer: it creates following sysfs files:
> /sys/class/leds/foo/delay_on
> /sys/class/leds/foo/delay_off
>
> Activating both of them will probably cause a WARNING in sysfs. They
> can't coexist :(
>
> We should probably have per-trigger subdirs, e.g.:
> /sys/class/leds/foo/trigger-oneshot/delay_on
> /sys/class/leds/foo/trigger-oneshot/delay_off
> /sys/class/leds/foo/trigger-oneshot/invert
> /sys/class/leds/foo/trigger-oneshot/shot
> /sys/class/leds/foo/trigger-timer/delay_on
> /sys/class/leds/foo/trigger-timer/delay_off
> but implementing it now would break the ABI.
>
> One workaround I can see is doing triggers V2, they:
> 1) Would put sysfs files in /sys/class/leds/foo/trigger-bar/
> 2) Use new API for *requesting* LED to be on/off
> 3) There would be a counter of requests in V2 API
> 4) Multiple triggers V2 would be allowed to be used (assigned) at the same time

Currently we support only triggers dedicated to specific type of
devices. Even in case of triggers that don't expose custom sysfs
attributes, registered with led_trigger_register_simple(), device
drivers have to generate trigger event with dedicated function, e.g:

- ledtrig-cpu: void ledtrig_cpu(enum cpu_led_event ledevt)
- ledtrig-disk: void ledtrig_disk_activity(void)
- ledtrig-mtd: void ledtrig_mtd_activity(void)

If one wanted to have the LED notified by different type of devices,
then they would have to implement a trigger that would exposed all
required types of API.

Unfortunately, there are many possible combinations of
triggers and that doesn't sound sane to add a new one when someone
will find it useful. Of course it would entail adding a call to the
new trigger API in the drivers, which doesn't seem like something
acceptable in the mainline.

--
Best regards,
Jacek Anaszewski