Re: [PATCH V5] leds: trigger: Introduce a USB port trigger

[Jacek Anaszewski]

On 09/15/2016 03:33 PM, RafaÅ MiÅecki wrote:
> On 15 September 2016 at 14:56, Pavel Machek <pavel@xxxxxx> wrote:
> > On Fri 2016-09-09 13:31:10, RafaÅ MiÅecki wrote:
> > > On 9 September 2016 at 13:05, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > On Fri, Sep 09, 2016 at 05:34:40PM +0800, Peter Chen wrote:
> > > > > On Thu, Sep 08, 2016 at 06:08:24PM +0200, RafaÅ MiÅecki wrote:
> > > > > > From: RafaÅ MiÅecki <rafal@xxxxxxxxxx>
> > > > > >
> > > > > > This commit adds a new trigger responsible for turning on LED when USB
> > > > > > device gets connected to the selected USB port. This can can useful for
> > > > > > various home routers that have USB port(s) and a proper LED telling user
> > > > > > a device is connected.
> > > > > >
> > > > > > The trigger gets its documentation file but basically it just requires
> > > > > > enabling it and selecting USB ports (e.g. echo 1 > ports/usb1-1).
> > > > > >
> > > > > > There was a long discussion on design of this driver. Its current state
> > > > > > is a result of picking them most adjustable solution as others couldn't
> > > > > > handle all cases.
> > > > > >
> > > > > > 1) It wasn't possible for the driver to register separated trigger for
> > > > > >    each USB port. Some physical USB ports are handled by more than one
> > > > > >    controller and so by more than one USB port. E.g. USB 2.0 physical
> > > > > >    port may be handled by OHCI's port and EHCI's port.
> > > > > >    It's also not possible to assign more than 1 trigger to a single LED
> > > > > >    and implementing such feature would be tricky due to syncing triggers
> > > > > >    and sysfs conflicts with old triggers.
> > > > > >
> > > > > > 2) Another idea was to register trigger per USB hub. This wouldn't allow
> > > > > >    handling devices with multiple USB LEDs and controllers (hubs)
> > > > > >    controlling more than 1 physical port. It's common for hubs to have
> > > > > >    few ports and each may have its own LED.
> > > > > >
> > > > > > This final trigger is highly flexible. It allows selecting any USB ports
> > > > > > for any LED. It was also modified (compared to the initial version) to
> > > > > > allow choosing ports rather than having user /guess/ proper names. It
> > > > > > was successfully tested on SmartRG SR400ac which has 3 USB LEDs,
> > > > > > 2 physical ports and 3 controllers.
> > > > > >
> > > > > > Another planned feature is support for LED reacting to the USB activity.
> > > > > > This can be implemented with another sysfs file for setting mode. The
> > > > > > default mode wouldn't change so there won't be ABI breakage and such
> > > > > > feature can be safely implemented later.
> > > > >
> > > > > It has such driver at: drivers/usb/common/led.c
> > > >
> > > > Ugh, I thought I had seen something like this before...
> > > >
> > > > RafaÅ, can you just use this in-kernel code instead?
> > >
> > > I really don't think I can because of all the reasons I carefully
> > > listed in the commit message.
> > >
> > > Have you took a look at that simple driver? It does nothing I need.
> > > Its design doesn't allow implementing features I clearly listed in the
> > > commit message.
> >
> > In any case, the new driver should probably go near the old one, at
> > the very least.
>
> I can do that. Anyone objects?

It's OK with me.

--
Best regards,
Jacek Anaszewski