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

[Jacek Anaszewski]

On 09/01/2016 04:36 PM, Alan Stern wrote:
> On Thu, 1 Sep 2016, Jacek Anaszewski wrote:
>
> > 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.
>
> Maybe it would make more sense, in this case, to allow only three
> possibilities for a USB port activity trigger.  Toggle the LED
> whenever:
>
> 	There is activity on the specified port, or
>
> 	There is any activity on any port on the specified hub, or
>
> 	There is any USB activity on any port.
>
> That ought to cover most of the normal use cases, and it would be
> simple enough to implement.

What would be the benefit of having a USB port activity trigger,
for which we would be specifying the port to observe, but in the same
time we would react on any activity on any port (cases 1 and 3)?

--
Best regards,
Jacek Anaszewski