On 20 July 2016 at 03:02, Rob Herring [off-list ref] wrote:

On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:

quoted

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.

I was already thinking on this as I plan to add "netdev" trigger at
some point in the future. I'd like to use "net-devices" for it (as a
equivalent or "usb-ports").

However there is a problem with your idea of sharing "led-triggers"
between triggers.. Consider device with generic purpose LED that
should be controllable by a user. I believe we should let user switch
it's state, e.g. with:
echo usbport > trigger
echo netdev > trigger
with a shared property I don't see how we couldn't handle it properly.
I don't think we can use anything like:
led-triggers = <&ohci_port1>, <&ehci_port1>, <&ethmac0>;
I'd get even more complicated if we ever need cells for any trigger.
AFAIK it's not allowed to mix handles with different cells like:
led-triggers = <&ohci_port1>, <&foo 5>, <&ethmac0>;

These problems made me use trigger-specific properies for specifying
LED triggers. Do you have any other idea for sharing a property &
avoiding above problems at the same time?

quoted

+
 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.

What do you mean by incomplete? Should I include e.g. ohci_port1 in
this example?

quoted

+     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.

Just specifying connection with "usb-ports" (or "led-triggers") won't
enable a given trigger and it shouldn't. There is already a way to
specify default trigger using property "linux,default-trigger". So you
can specify:
1) Default one with:
linux,default-trigger = "usbport";
2) On runtime:
echo usbport > trigger

quoted

+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).

There is no such problem. You can freely do:
echo none > trigger
(e.g. to disable LED during night or whatever)
and then:
echo usbport > trigger

This will make "usbport" use triggers specified in DT as expected.

-- 
Rafał