On Wed, Jul 20, 2016 at 10:06:23AM +0200, Rafał Miłecki wrote:

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

quoted

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.

Well, the userspace interface and DT bindings are independent things. 
You can't argue for what the DT binding should look like based on the 
userspace interface.

Perhaps we need a "device trigger" where you echo the device to 
be the trigger (similar to how bind works). If you have something to id 
the device, then you can lookup its struct device and then its of_node 
ptr.

The other problem with your userspace interface is it only works if 
the trigger is defined in DT. It doesn't allow for specifying which usb 
port or which netdev. Any trigger source should be assignable to any 
LED? I can assign the disk activity trigger to the numlock LED if I 
want to...

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>;

It is allowed, but you would have to have a '#led-trigger-cells' 
property in each phandle target.

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

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?

label and usb-ports alone in this node does not make an LED node.

quoted

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

IMO, these should be mutually exclusive. Either you specify a DT node as 
the trigger or you specify a linux specific trigger.

quoted

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.

I think the singular "usbport" trigger is problematic. Look at how cpu 
triggers are done. I can specify which cpu is the trigger. You should be 
able specify which port is the trigger, too.

In summary, I guess what I'm saying is don't push the problems of the 
current userspace interface down to DT.

Rob