On 20 July 2016 at 03:02, Rob Herring <r...@kernel.org> 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 <zaj...@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>, <ðmac0>;
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>, <ðmac0>;
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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
