From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753269AbcGTIGg (ORCPT ); Wed, 20 Jul 2016 04:06:36 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:35634 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753004AbcGTIGZ convert rfc822-to-8bit (ORCPT ); Wed, 20 Jul 2016 04:06:25 -0400 MIME-Version: 1.0 In-Reply-To: <20160720010222.GA21679@rob-hp-laptop> References: <1468239883-22695-1-git-send-email-zajec5@gmail.com> <1468617060-29830-1-git-send-email-zajec5@gmail.com> <20160720010222.GA21679@rob-hp-laptop> From: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= Date: Wed, 20 Jul 2016 10:06:23 +0200 Message-ID: Subject: Re: [PATCH V2] leds: trigger: Introduce an USB port trigger To: Rob Herring Cc: Richard Purdie , Jacek Anaszewski , Felipe Balbi , Peter Chen , "linux-usb@vger.kernel.org" , Mark Rutland , Jonathan Corbet , Sakari Ailus , Ezequiel Garcia , Boris Brezillon , Pavel Machek , Geert Uytterhoeven , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list , "open list:DOCUMENTATION" , "open list:LED SUBSYSTEM" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20 July 2016 at 03:02, Rob Herring 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 >> --- >> 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 = '? 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ł