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

[RafaÅ MiÅecki]

On 20 July 2016 at 03:02, Rob Herring <robh@xxxxxxxxxx> wrote:
> 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@xxxxxxxxx>
>> ---
>> 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?


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


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


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