| Message ID | 290ed068-2518-50ef-4d02-394bef8b7ee9@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5@gmail.com> wrote: > On 02/28/2017 10:38 PM, Jacek Anaszewski wrote: >> >> I think that it would be simpler if we could initially see >> a complete sample dts implementation containing all required DT >> nodes. The example could contain timer trigger as well as usb-port >> trigger specific bindings. > > > Please take a look at attached patch. I used it on Tenda AC9 with: I'm not sure about this extra level of indirection. I don't see the need. > usb_trigger: usb-trigger { > trigger-type = "usbport"; Why do we need to know the type? The trigger device knows what type it is. All we should need to know here is what device(s) controls an LED. The rest the kernel can figure out. > ports = <&ohci_port1>, <&ehci_port1>; > }; > > usb { > label = "bcm53xx:blue:usb"; > gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>; > triggers = <&usb_trigger>; > }; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/28/2017 11:12 PM, Rob Herring wrote: > On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote: >>> >>> I think that it would be simpler if we could initially see >>> a complete sample dts implementation containing all required DT >>> nodes. The example could contain timer trigger as well as usb-port >>> trigger specific bindings. >> >> >> Please take a look at attached patch. I used it on Tenda AC9 with: > > I'm not sure about this extra level of indirection. I don't see the need. > >> usb_trigger: usb-trigger { >> trigger-type = "usbport"; > > Why do we need to know the type? The trigger device knows what type it > is. All we should need to know here is what device(s) controls an LED. > The rest the kernel can figure out. The thing is that in the proposed approach the trigger is not necessary a device. The trigger node is here only a container with initialization data. We could have e,g, two such nodes with different set of ports (say usb1-trigger and usb2-trigger). Then if we had two LED nodes usb1 and usb2, the former could have its triggers property initialized to &usb1-trigger and the latter to &usb2-trigger. Thanks to that both LEDs after executing "echo usbport > triggers" would listen to events from different set of usb ports. Also, e.g. for the timer trigger we could define two separate DT nodes with different delay intervals. >> ports = <&ohci_port1>, <&ehci_port1>; >> }; >> >> usb { >> label = "bcm53xx:blue:usb"; >> gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>; >> triggers = <&usb_trigger>; >> }; >
On 02/28/2017 10:51 PM, Rafał Miłecki wrote: > On 02/28/2017 10:38 PM, Jacek Anaszewski wrote: >> I think that it would be simpler if we could initially see >> a complete sample dts implementation containing all required DT >> nodes. The example could contain timer trigger as well as usb-port >> trigger specific bindings. > > Please take a look at attached patch. I used it on Tenda AC9 with: > > usb_trigger: usb-trigger { > trigger-type = "usbport"; > ports = <&ohci_port1>, <&ehci_port1>; > }; > > usb { > label = "bcm53xx:blue:usb"; > gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>; > triggers = <&usb_trigger>; > }; OK, I got it, thanks. > >> I suppose that we should see DT nodes containing #list-cells >> properties that define the quantity of phandle arguments. >> >> It seems that this approach allows for defining a list of elements >> with variable number of arguments, i.e. what you were initially >> asking for. > > Are you sure we need #list-cells? Can't we simply use > of_count_phandle_with_args(np, "triggers", NULL); > ? I'm not sure, I just read the function documentation :-) I haven't verified nor have I used this API. > > From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <rafal@milecki.pl> > Date: Mon, 3 Oct 2016 16:49:05 +0200 > Subject: [PATCH] usb: core: read USB ports from DT in the usbport LED > trigger > driver > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../devicetree/bindings/leds/triggers-usbport.txt | 19 ++++++++ > drivers/leds/led-triggers.c | 3 +- > drivers/usb/core/ledtrig-usbport.c | 56 > ++++++++++++++++++++++ > 3 files changed, 77 insertions(+), 1 deletion(-) > create mode 100644 > Documentation/devicetree/bindings/leds/triggers-usbport.txt > > diff --git a/Documentation/devicetree/bindings/leds/triggers-usbport.txt > b/Documentation/devicetree/bindings/leds/triggers-usbport.txt > new file mode 100644 > index 000000000000..10e55122cded > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/triggers-usbport.txt > @@ -0,0 +1,19 @@ > +USB port LED trigger properties. > + > +USB port trigger can be used for signalling to the user a presence of USB > +device(s) in given ports. It's possible to specify USB ports in DT by > providing > +their references. > + > +Properties: > +- trigger-type : Must be "usbport". > +- ports : List of USB ports related to this LED. Some devices may have > one USB > + LED for all ports, other may have few of them (e.g. USB version > + specific). It's used by usbport trigger for reading a list of ports > + that should cause LED to turn on whenver device get connected. > + > +Examples: > + > +usbport-trigger { > + trigger-type = "usbport"; > + ports = <&ohci_port1>, <&ehci_port1>; > +}; > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c > index c53c20d676cd..1d8bda9755ca 100644 > --- a/drivers/leds/led-triggers.c > +++ b/drivers/leds/led-triggers.c > @@ -181,7 +181,8 @@ static void led_trigger_of_read_trigger(struct > led_classdev *led_cdev) > } > > /* Check if trigger specified in DT is supported */ > - if (strcmp(trigger_type, "timer")) > + if (strcmp(trigger_type, "timer") && > + strcmp(trigger_type, "usbport")) > goto err_node_put; > > led_cdev->default_trigger = trigger_type; > diff --git a/drivers/usb/core/ledtrig-usbport.c > b/drivers/usb/core/ledtrig-usbport.c > index 1713248ab15a..599f7e6b0ba8 100644 > --- a/drivers/usb/core/ledtrig-usbport.c > +++ b/drivers/usb/core/ledtrig-usbport.c > @@ -11,8 +11,10 @@ > #include <linux/device.h> > #include <linux/leds.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/slab.h> > #include <linux/usb.h> > +#include <linux/usb/of.h> > > struct usbport_trig_data { > struct led_classdev *led_cdev; > @@ -123,6 +125,58 @@ static const struct attribute_group ports_group = { > * Adding & removing ports > ***************************************/ > > +/** > + * usbport_trig_port_observed - Check if port should be observed > + * > + * Each LED may have list of related USB ports specified in a DT. This > function > + * reads them using ports property and sets a proper state. > + */ > +static bool usbport_trig_port_observed(struct usbport_trig_data > *usbport_data, > + struct usb_device *usb_dev, int port1) > +{ > + struct device_node *led_np = usbport_data->led_cdev->trigger_node; > + struct device *dev = usbport_data->led_cdev->dev; > + struct of_phandle_args args; > + struct device_node *port_np; > + int count, i; > + > + if (!led_np) > + return false; > + > + /* Get node of port being added */ > + port_np = usb_of_get_child_node(usb_dev->dev.of_node, port1); > + if (!port_np) > + return false; > + > + /* Amount of ports this LED references */ > + count = of_count_phandle_with_args(led_np, "ports", NULL); > + if (count < 0) { > + dev_warn(dev, "Failed to get USB ports for %s\n", > + led_np->full_name); > + return false; > + } > + > + /* Check if port is on this LED's list */ > + for (i = 0; i < count; i++) { > + int err; > + > + err = of_parse_phandle_with_args(led_np, "ports", NULL, i, > + &args); > + if (err) { > + dev_err(dev, "Failed to get USB port phandle at index %d: > %d\n", > + i, err); > + continue; > + } > + > + of_node_put(args.np); > + > + if (args.np == port_np) > + return true; > + } > + > + return false; > +} > + > static int usbport_trig_add_port(struct usbport_trig_data *usbport_data, > struct usb_device *usb_dev, > const char *hub_name, int portnum) > @@ -141,6 +195,7 @@ static int usbport_trig_add_port(struct > usbport_trig_data *usbport_data, > port->data = usbport_data; > port->hub = usb_dev; > port->portnum = portnum; > + port->observed = usbport_trig_port_observed(usbport_data, usb_dev, > portnum); > > len = strlen(hub_name) + 8; > port->port_name = kzalloc(len, GFP_KERNEL); > @@ -255,6 +310,7 @@ static void usbport_trig_activate(struct > led_classdev *led_cdev) > if (err) > goto err_free; > usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports); > + usbport_trig_update_count(usbport_data); > > /* Notifications */ > usbport_data->nb.notifier_call = usbport_trig_notify,
On 03/01/2017 10:04 PM, Jacek Anaszewski wrote: > On 02/28/2017 11:12 PM, Rob Herring wrote: >> On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5@gmail.com> wrote: >>> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote: >>>> >>>> I think that it would be simpler if we could initially see >>>> a complete sample dts implementation containing all required DT >>>> nodes. The example could contain timer trigger as well as usb-port >>>> trigger specific bindings. >>> >>> >>> Please take a look at attached patch. I used it on Tenda AC9 with: >> >> I'm not sure about this extra level of indirection. I don't see the need. >> >>> usb_trigger: usb-trigger { >>> trigger-type = "usbport"; >> >> Why do we need to know the type? The trigger device knows what type it >> is. All we should need to know here is what device(s) controls an LED. >> The rest the kernel can figure out. > > The thing is that in the proposed approach the trigger is not necessary > a device. The trigger node is here only a container with initialization > data. > > We could have e,g, two such nodes with different set of ports > (say usb1-trigger and usb2-trigger). Then if we had two LED nodes usb1 > and usb2, the former could have its triggers property initialized > to &usb1-trigger and the latter to &usb2-trigger. Thanks to that both > LEDs after executing "echo usbport > triggers" would listen to events > from different set of usb ports. > > Also, e.g. for the timer trigger we could define two separate DT nodes > with different delay intervals. Hi Rob, what do you think about this? Jacek is correct, we could have e.g. timer-trigger-1 { trigger-type = "timer"; delay-on = <200>; delay-off = <500>; }; timer-trigger-2 { trigger-type = "timer"; delay-on = <500>; delay-off = <1000>; }; or something like: usbport-2-0-trigger { trigger-type = "usbport"; ports = <&ohci_port1>, <&ehci_port1>; }; usbport-3-0-trigger { trigger-type = "usbport"; ports = <&xhci_port1>; }; Does it make more sense? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/01/2017 10:04 PM, Jacek Anaszewski wrote: > On 02/28/2017 10:51 PM, Rafał Miłecki wrote: >> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote: >>> I think that it would be simpler if we could initially see >>> a complete sample dts implementation containing all required DT >>> nodes. The example could contain timer trigger as well as usb-port >>> trigger specific bindings. >> >> Please take a look at attached patch. I used it on Tenda AC9 with: >> >> usb_trigger: usb-trigger { >> trigger-type = "usbport"; >> ports = <&ohci_port1>, <&ehci_port1>; >> }; >> >> usb { >> label = "bcm53xx:blue:usb"; >> gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>; >> triggers = <&usb_trigger>; >> }; > > OK, I got it, thanks. > >> >>> I suppose that we should see DT nodes containing #list-cells >>> properties that define the quantity of phandle arguments. >>> >>> It seems that this approach allows for defining a list of elements >>> with variable number of arguments, i.e. what you were initially >>> asking for. >> >> Are you sure we need #list-cells? Can't we simply use >> of_count_phandle_with_args(np, "triggers", NULL); >> ? > > I'm not sure, I just read the function documentation :-) > I haven't verified nor have I used this API. So we could use #list-cells to support something like: usb { label = "bcm53xx:blue:usb"; gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>; triggers = <&usb_trigger 1 2>, <&timer_trigger 3 4 5>; }; but I don't see any need for this. Just specifying triggers like: triggers = <&usb_trigger>, <&timer_trigger>, <&foo>; should be always enough, especially with the new trigger nodes you suggested. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/06/2017 07:16 AM, Rafał Miłecki wrote: > On 03/01/2017 10:04 PM, Jacek Anaszewski wrote: >> On 02/28/2017 10:51 PM, Rafał Miłecki wrote: >>> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote: >>>> I think that it would be simpler if we could initially see >>>> a complete sample dts implementation containing all required DT >>>> nodes. The example could contain timer trigger as well as usb-port >>>> trigger specific bindings. >>> >>> Please take a look at attached patch. I used it on Tenda AC9 with: >>> >>> usb_trigger: usb-trigger { >>> trigger-type = "usbport"; >>> ports = <&ohci_port1>, <&ehci_port1>; >>> }; >>> >>> usb { >>> label = "bcm53xx:blue:usb"; >>> gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>; >>> triggers = <&usb_trigger>; >>> }; >> >> OK, I got it, thanks. >> >>> >>>> I suppose that we should see DT nodes containing #list-cells >>>> properties that define the quantity of phandle arguments. >>>> >>>> It seems that this approach allows for defining a list of elements >>>> with variable number of arguments, i.e. what you were initially >>>> asking for. >>> >>> Are you sure we need #list-cells? Can't we simply use >>> of_count_phandle_with_args(np, "triggers", NULL); >>> ? >> >> I'm not sure, I just read the function documentation :-) >> I haven't verified nor have I used this API. > > So we could use #list-cells to support something like: > > usb { > label = "bcm53xx:blue:usb"; > gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>; > triggers = <&usb_trigger 1 2>, <&timer_trigger 3 4 5>; > }; > > but I don't see any need for this. Just specifying triggers like: > triggers = <&usb_trigger>, <&timer_trigger>, <&foo>; > should be always enough, especially with the new trigger nodes you > suggested. Indeed, it seems to be not needed for this approach. I'm OK with this design.
On Mon, Mar 06, 2017 at 07:06:32AM +0100, Rafał Miłecki wrote: > On 03/01/2017 10:04 PM, Jacek Anaszewski wrote: > > On 02/28/2017 11:12 PM, Rob Herring wrote: > > > On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5@gmail.com> wrote: > > > > On 02/28/2017 10:38 PM, Jacek Anaszewski wrote: > > > > > > > > > > I think that it would be simpler if we could initially see > > > > > a complete sample dts implementation containing all required DT > > > > > nodes. The example could contain timer trigger as well as usb-port > > > > > trigger specific bindings. > > > > > > > > > > > > Please take a look at attached patch. I used it on Tenda AC9 with: > > > > > > I'm not sure about this extra level of indirection. I don't see the need. > > > > > > > usb_trigger: usb-trigger { > > > > trigger-type = "usbport"; > > > > > > Why do we need to know the type? The trigger device knows what type it > > > is. All we should need to know here is what device(s) controls an LED. > > > The rest the kernel can figure out. > > > > The thing is that in the proposed approach the trigger is not necessary > > a device. The trigger node is here only a container with initialization > > data. > > > > We could have e,g, two such nodes with different set of ports > > (say usb1-trigger and usb2-trigger). Then if we had two LED nodes usb1 > > and usb2, the former could have its triggers property initialized > > to &usb1-trigger and the latter to &usb2-trigger. Thanks to that both > > LEDs after executing "echo usbport > triggers" would listen to events > > from different set of usb ports. > > > > Also, e.g. for the timer trigger we could define two separate DT nodes > > with different delay intervals. > > Hi Rob, what do you think about this? > > Jacek is correct, we could have e.g. > timer-trigger-1 { > trigger-type = "timer"; > delay-on = <200>; > delay-off = <500>; > }; > timer-trigger-2 { > trigger-type = "timer"; > delay-on = <500>; > delay-off = <1000>; > }; This is just letting the Linux driver structure define/influence the binding. This could all be described with a single LED property like "led-blink-pattern". The fact that it gets implemented by the timer trigger is irrelevant to the binding. Plus timer based triggering seems more like user controlled than platform or hardware defined. So trying to create a binding for all Linux triggers is a mistake. The only thing we need here is a way to associate an LED with a h/w device. > > or something like: > usbport-2-0-trigger { > trigger-type = "usbport"; > ports = <&ohci_port1>, <&ehci_port1>; > }; > usbport-3-0-trigger { > trigger-type = "usbport"; > ports = <&xhci_port1>; > }; > > Does it make more sense? No, for the reasons above. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/leds/triggers-usbport.txt b/Documentation/devicetree/bindings/leds/triggers-usbport.txt new file mode 100644 index 000000000000..10e55122cded --- /dev/null +++ b/Documentation/devicetree/bindings/leds/triggers-usbport.txt @@ -0,0 +1,19 @@ +USB port LED trigger properties. + +USB port trigger can be used for signalling to the user a presence of USB +device(s) in given ports. It's possible to specify USB ports in DT by providing +their references. + +Properties: +- trigger-type : Must be "usbport". +- ports : List of USB ports related to this LED. Some devices may have one USB + LED for all ports, other may have few of them (e.g. USB version + specific). It's used by usbport trigger for reading a list of ports + that should cause LED to turn on whenver device get connected. + +Examples: + +usbport-trigger { + trigger-type = "usbport"; + ports = <&ohci_port1>, <&ehci_port1>; +}; diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c index c53c20d676cd..1d8bda9755ca 100644 --- a/drivers/leds/led-triggers.c +++ b/drivers/leds/led-triggers.c @@ -181,7 +181,8 @@ static void led_trigger_of_read_trigger(struct led_classdev *led_cdev) } /* Check if trigger specified in DT is supported */ - if (strcmp(trigger_type, "timer")) + if (strcmp(trigger_type, "timer") && + strcmp(trigger_type, "usbport")) goto err_node_put; led_cdev->default_trigger = trigger_type; diff --git a/drivers/usb/core/ledtrig-usbport.c b/drivers/usb/core/ledtrig-usbport.c index 1713248ab15a..599f7e6b0ba8 100644 --- a/drivers/usb/core/ledtrig-usbport.c +++ b/drivers/usb/core/ledtrig-usbport.c @@ -11,8 +11,10 @@ #include <linux/device.h> #include <linux/leds.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/slab.h> #include <linux/usb.h> +#include <linux/usb/of.h> struct usbport_trig_data { struct led_classdev *led_cdev; @@ -123,6 +125,58 @@ static const struct attribute_group ports_group = { * Adding & removing ports ***************************************/ +/** + * usbport_trig_port_observed - Check if port should be observed + * + * Each LED may have list of related USB ports specified in a DT. This function + * reads them using ports property and sets a proper state. + */ +static bool usbport_trig_port_observed(struct usbport_trig_data *usbport_data, + struct usb_device *usb_dev, int port1) +{ + struct device_node *led_np = usbport_data->led_cdev->trigger_node; + struct device *dev = usbport_data->led_cdev->dev; + struct of_phandle_args args; + struct device_node *port_np; + int count, i; + + if (!led_np) + return false; + + /* Get node of port being added */ + port_np = usb_of_get_child_node(usb_dev->dev.of_node, port1); + if (!port_np) + return false; + + /* Amount of ports this LED references */ + count = of_count_phandle_with_args(led_np, "ports", NULL); + if (count < 0) { + dev_warn(dev, "Failed to get USB ports for %s\n", + led_np->full_name); + return false; + } + + /* Check if port is on this LED's list */ + for (i = 0; i < count; i++) { + int err; + + err = of_parse_phandle_with_args(led_np, "ports", NULL, i, + &args); + if (err) { + dev_err(dev, "Failed to get USB port phandle at index %d: %d\n", + i, err); + continue; + } + + of_node_put(args.np); + + if (args.np == port_np) + return true; + } + + return false; +} + static int usbport_trig_add_port(struct usbport_trig_data *usbport_data, struct usb_device *usb_dev, const char *hub_name, int portnum) @@ -141,6 +195,7 @@ static int usbport_trig_add_port(struct usbport_trig_data *usbport_data, port->data = usbport_data; port->hub = usb_dev; port->portnum = portnum; + port->observed = usbport_trig_port_observed(usbport_data, usb_dev, portnum); len = strlen(hub_name) + 8; port->port_name = kzalloc(len, GFP_KERNEL); @@ -255,6 +310,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev) if (err) goto err_free; usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports); + usbport_trig_update_count(usbport_data); /* Notifications */ usbport_data->nb.notifier_call = usbport_trig_notify,