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

[Alan Stern]

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.

Alan Stern