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

[Rob Herring <robh@kernel.org>]

On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
> This commit adds a new trigger that can turn on LED when USB device gets
> connected to the USB port. This can be useful for various home routers
> that have USB port and a proper LED telling user a device is connected.
> 
> Right now this trigger is usable with a proper DT only, there isn't a
> way to specify USB ports from user space. This may change in a future.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> V2: The first version got support for specifying list of USB ports from
>     user space only. There was a (big try &) discussion on adding DT
>     support. It led to a pretty simple solution of comparing of_node of
>     usb_device to of_node specified in usb-ports property.
>     Since it appeared DT support may be simpler and non-DT a bit more
>     complex, this version drops previous support for "ports" and
>     "new_port" and focuses on DT only. The plan is to see if this
>     solution with DT is OK, get it accepted and then work on non-DT.
> 
> Felipe: if there won't be any objections I'd like to ask for your Ack.
> ---
>  Documentation/devicetree/bindings/leds/common.txt |  11 ++
>  Documentation/leds/ledtrig-usbport.txt            |  19 ++
>  drivers/leds/trigger/Kconfig                      |   8 +
>  drivers/leds/trigger/Makefile                     |   1 +
>  drivers/leds/trigger/ledtrig-usbport.c            | 206 ++++++++++++++++++++++
>  5 files changed, 245 insertions(+)
>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index af10678..75536f7 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -50,6 +50,12 @@ property can be omitted.
>  For controllers that have no configurable timeout the flash-max-timeout-us
>  property can be omitted.
>  
> +Trigger specific properties for child nodes:
> +
> +usbport trigger:
> +- usb-ports : List of USB ports that usbport should observed for turning on a
> +	      given LED.

I think this should be more generic such that it could work for disk or 
network LEDs too. Perhaps 'led-triggers = <nodes>'? trigger is a bit of 
a Linux name, but I haven't thought of anything better. Really, I'd 
prefer the link in the other direction (e.g. port node have a 'leds" or 
'*-leds' property), but that's maybe harder to parse.

> +
>  Examples:
>  
>  system-status {
> @@ -58,6 +64,11 @@ system-status {
>  	...
>  };
>  
> +usb {
> +	label = "USB";

It's not really clear in the example this is an LED node as it is 
incomplete.

> +	usb-ports = <&ohci_port1>, <&ehci_port1>;
> +};
> +
>  camera-flash {
>  	label = "Flash";
>  	led-sources = <0>, <1>;
> diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
> new file mode 100644
> index 0000000..642c4cd
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-usbport.txt
> @@ -0,0 +1,19 @@
> +USB port LED trigger
> +====================
> +
> +This LED trigger can be used for signaling user a presence of USB device in a
> +given port. It simply turns on LED when device appears and turns it off when it
> +disappears.
> +
> +It requires specifying a list of USB ports that should be observed. This can be
> +done in DT by setting a proper property with list of a phandles. If more than
> +one port is specified, LED will be turned on as along as there is at least one
> +device connected to any of ports.
> +
> +This trigger can be activated from user space on led class devices as shown
> +below:
> +
> +  echo usbport > trigger

Why do I have to do this (by default)? I already specified in the DT 
what the connection is. It should come up working OOTB, or don't put it 
in DT.

> +Nevertheless, current there isn't a way to specify list of USB ports from user
> +space.

s/current/currently/

This is a problem since if it works by default and you switch to a 
different trigger, there's no way to get back to the default (unless 
you remember the ports).

Rob