Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On Thu, Aug 25, 2016 at 08:39:24PM +0200, Bjørn Mork wrote: > Greg KHwrites: > > On Thu, Aug 25, 2016 at 07:14:52AM +0200, Rafał Miłecki wrote: > >> > >> Good question. I would like to extend this USB port trigger in the > >> future by reacting to USB activity. This involves playing with URBs > >> and I believe that at that point it'd be getting too much USB specific > >> to /rule them all/. > > > > Oh that's never going to work, sorry. USB "activity" happens deep in > > USB host controller hardware, not up at the kernel level at all. It's > > just too fast, and would take up too much CPU to do it otherwise. Heck, > > even old UHCI 1.1 USB controllers did this. > > > > USB "activity" happens all the time, look at a USB bus analyzer if you > > want to see what goes on. Teasing out what is "real data" and what is > > just "normal bus activity" is impossible at the kernel level, > > OK, I am going to play stupid again: Does this mean that usbmon is > impossible? Hah, nice try :) I guess when I think "USB activity" I think about the lower layers of frames and acks and naks on the bus. But maybe because I have just recently been doing a lot of work down at the EHCI frame timing layer for some strange hardware... But you are right, if all you care about is the data that the host sees, then yes, you could do something like usbmon to "tickle" a led if you want to. Odds are, someone's already done this for a raspberry pi :) thanks, greg k-h -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
Greg KHwrites: > On Thu, Aug 25, 2016 at 07:14:52AM +0200, Rafał Miłecki wrote: >> >> Good question. I would like to extend this USB port trigger in the >> future by reacting to USB activity. This involves playing with URBs >> and I believe that at that point it'd be getting too much USB specific >> to /rule them all/. > > Oh that's never going to work, sorry. USB "activity" happens deep in > USB host controller hardware, not up at the kernel level at all. It's > just too fast, and would take up too much CPU to do it otherwise. Heck, > even old UHCI 1.1 USB controllers did this. > > USB "activity" happens all the time, look at a USB bus analyzer if you > want to see what goes on. Teasing out what is "real data" and what is > just "normal bus activity" is impossible at the kernel level, OK, I am going to play stupid again: Does this mean that usbmon is impossible? > and really > makes no sense at all (your led would just be "on" all the time.) I don't see how this is any different from e.g. the network activity triggers. FWIW, on my system "cat /sys/kernel/debug/usb/usbmon/0u" can be silent for long periods, and show a rush of lines whenever there is USB activity. I imagine that a LED triggered by a hook in a similar place would be useful on systems with LEDs for that sort of stuff. Bjørn -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On Thu, Aug 25, 2016 at 07:14:52AM +0200, Rafał Miłecki wrote: > On 24 August 2016 at 23:04, Greg KHwrote: > > On Wed, Aug 24, 2016 at 11:29:51AM +0200, Rafał Miłecki wrote: > >> On 24 August 2016 at 11:22, Greg KH wrote: > >> > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote: > >> >> +static ssize_t ports_show(struct device *dev, struct device_attribute > >> >> *attr, > >> >> + char *buf) > >> >> +{ > >> >> + struct led_classdev *led_cdev = dev_get_drvdata(dev); > >> >> + struct usbport_trig_data *usbport_data = led_cdev->trigger_data; > >> >> + struct usbport_trig_port *port; > >> >> + ssize_t ret = 0; > >> >> + int len; > >> >> + > >> >> + list_for_each_entry(port, _data->ports, list) { > >> >> + len = sprintf(buf + ret, "%s\n", port->name); > >> >> + if (len >= 0) > >> >> + ret += len; > >> >> + } > >> >> + > >> >> + return ret; > >> >> +} > >> > > >> > sysfs is "one value per file", here you are listing a bunch of things in > >> > one sysfs file. Please don't do that. > >> > >> OK. What do you think about creating "ports" subdirectory and creating > >> file-per-port in it? Then I'd need to bring back something like > >> "new_port" and "remove_port". Does it sound OK? > > > > Maybe, I don't know. Why is "USB" somehow unique here? Why isn't this > > the same for PCI slots/ports? pccard? sdcard? thunderbolt? > > Good question. I would like to extend this USB port trigger in the > future by reacting to USB activity. This involves playing with URBs > and I believe that at that point it'd be getting too much USB specific > to /rule them all/. Oh that's never going to work, sorry. USB "activity" happens deep in USB host controller hardware, not up at the kernel level at all. It's just too fast, and would take up too much CPU to do it otherwise. Heck, even old UHCI 1.1 USB controllers did this. USB "activity" happens all the time, look at a USB bus analyzer if you want to see what goes on. Teasing out what is "real data" and what is just "normal bus activity" is impossible at the kernel level, and really makes no sense at all (your led would just be "on" all the time.) Are you sure you know how USB works? Writing things like this makes me really worry... greg k-h -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On 24 August 2016 at 23:04, Greg KHwrote: > On Wed, Aug 24, 2016 at 11:29:51AM +0200, Rafał Miłecki wrote: >> On 24 August 2016 at 11:22, Greg KH wrote: >> > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote: >> >> +static ssize_t ports_show(struct device *dev, struct device_attribute >> >> *attr, >> >> + char *buf) >> >> +{ >> >> + struct led_classdev *led_cdev = dev_get_drvdata(dev); >> >> + struct usbport_trig_data *usbport_data = led_cdev->trigger_data; >> >> + struct usbport_trig_port *port; >> >> + ssize_t ret = 0; >> >> + int len; >> >> + >> >> + list_for_each_entry(port, _data->ports, list) { >> >> + len = sprintf(buf + ret, "%s\n", port->name); >> >> + if (len >= 0) >> >> + ret += len; >> >> + } >> >> + >> >> + return ret; >> >> +} >> > >> > sysfs is "one value per file", here you are listing a bunch of things in >> > one sysfs file. Please don't do that. >> >> OK. What do you think about creating "ports" subdirectory and creating >> file-per-port in it? Then I'd need to bring back something like >> "new_port" and "remove_port". Does it sound OK? > > Maybe, I don't know. Why is "USB" somehow unique here? Why isn't this > the same for PCI slots/ports? pccard? sdcard? thunderbolt? Good question. I would like to extend this USB port trigger in the future by reacting to USB activity. This involves playing with URBs and I believe that at that point it'd be getting too much USB specific to /rule them all/. -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On Wed, Aug 24, 2016 at 11:29:51AM +0200, Rafał Miłecki wrote: > On 24 August 2016 at 11:22, Greg KHwrote: > > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote: > >> +static ssize_t ports_show(struct device *dev, struct device_attribute > >> *attr, > >> + char *buf) > >> +{ > >> + struct led_classdev *led_cdev = dev_get_drvdata(dev); > >> + struct usbport_trig_data *usbport_data = led_cdev->trigger_data; > >> + struct usbport_trig_port *port; > >> + ssize_t ret = 0; > >> + int len; > >> + > >> + list_for_each_entry(port, _data->ports, list) { > >> + len = sprintf(buf + ret, "%s\n", port->name); > >> + if (len >= 0) > >> + ret += len; > >> + } > >> + > >> + return ret; > >> +} > > > > sysfs is "one value per file", here you are listing a bunch of things in > > one sysfs file. Please don't do that. > > OK. What do you think about creating "ports" subdirectory and creating > file-per-port in it? Then I'd need to bring back something like > "new_port" and "remove_port". Does it sound OK? Maybe, I don't know. Why is "USB" somehow unique here? Why isn't this the same for PCI slots/ports? pccard? sdcard? thunderbolt? thanks, greg k-h -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On Wed, Aug 24, 2016 at 11:26:27AM +0200, Rafał Miłecki wrote: > On 24 August 2016 at 11:21, Greg KHwrote: > > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote: > >> From: Rafał Miłecki > >> > >> This commit adds a new trigger responsible for turning on LED when USB > >> device gets connected to the specified USB port. This can can useful for > >> various home routers that have USB port(s) and a proper LED telling user > >> a device is connected. > >> > >> The trigger gets its documentation file but basically it just requires > >> specifying USB port in a Linux format (e.g. echo 1-1 > new_port). > >> > >> During work on this trigger there was a plan to add DT bindings for it, > >> but there wasn't an agreement on the format yet. This can be worked on > >> later, a sysfs interface is needed anyway for platforms not using DT. > >> > >> Signed-off-by: Rafał Miłecki > >> --- > >> V2: Trying to add DT support, idea postponed as it will take more time > >> to discuss the bindings. > >> V3: Fix typos in commit and Documentation (thanks Jacek!) > >> Use "ports" sysfs file for adding and removing USB ports (thx Jacek) > >> Check if there is USB device connected after adding new USB port > >> Fix memory leak or two > >> > >> Felipe: I'd like to ask for your Ack before having this patch pushed. > >> --- > >> Documentation/leds/ledtrig-usbport.txt | 49 +++ > >> drivers/leds/trigger/Kconfig | 8 ++ > >> drivers/leds/trigger/Makefile | 1 + > >> drivers/leds/trigger/ledtrig-usbport.c | 253 > >> + > >> 4 files changed, 311 insertions(+) > >> create mode 100644 Documentation/leds/ledtrig-usbport.txt > >> create mode 100644 drivers/leds/trigger/ledtrig-usbport.c > > > > You are adding sysfs files without adding a Documentation/ABI/ entry, > > please never do that. > > This is the way all LED triggers are documented, I just blindly > assumed it's OK. Could you be a bit more helpful and help me to > understand further steps? Should I drop > Documentation/leds/ledtrig-usbport.txt and add proper entry in > Documentation/ABI/testing? Or should I keep both files? What about > current sysfs of other triggers? Should they be moved/documented in > ABI? In the end, all should be moved into Documentation/ABI/ But for you, might as well start documenting them in there for any new ones so you don't have to just move them later. thanks, greg k-h -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On 24/08/16 13:02, Rafał Miłecki wrote: On 24 August 2016 at 12:49, Matthias Bruggerwrote: On 24/08/16 00:03, Rafał Miłecki wrote: [...] +static int usbport_trig_notify(struct notifier_block *nb, unsigned long action, + void *data) +{ + struct usbport_trig_data *usbport_data = + container_of(nb, struct usbport_trig_data, nb); + struct led_classdev *led_cdev = usbport_data->led_cdev; + + switch (action) { + case USB_DEVICE_ADD: + if (usbport_trig_usb_dev_observed(usbport_data, data)) { Maybe we should switch this and fist see if the usbport is observed before evaluating the action. Also cast data to "struct usb_device *" to make that clear. I'm aware there is one duplicated line of code, I did to first evaluate very quick test (checking unsigned long value), then iterate over ports & keep only 1 switch block. I could move usbport_trig_usb_dev_observed call up, but it would be executed for other actions as well (currently just USB_BUS_ADD and USB_BUS_REMOVE). Ok. I'm a USB noop but from my understanding the notifier is only called when a device or a hub gets added/removed. So this shouldn't happen that much. Therefor it has no impact if we check if the usb device is in the observer list for all actions. + if (usbport_data->count++ == 0) I'm a bit puzzled. I think: if (++usbport_data->count > 0) makes this more consistent with the remove case. Your condition would be always true (as we don't use negative numbers). The point is to enable LED only if it was disabled before. So I need to increase counter unconditionally but enable LED only if initial value (before increasing it) was 0. Got it. My personal opinion is, that adding one line for incrementing/decrementing the counter would help to make this crystal-clear to everyone (at least to me :) Cheers, Matthias +module_init(usbport_trig_init); +module_exit(usbport_trig_exit); + +MODULE_AUTHOR("Rafał Miłecki "); Nit: ra...@milecki.pl Oops, thanks! -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On 24/08/16 00:03, Rafał Miłecki wrote: From: Rafał MiłeckiThis commit adds a new trigger responsible for turning on LED when USB device gets connected to the specified USB port. This can can useful for various home routers that have USB port(s) and a proper LED telling user a device is connected. The trigger gets its documentation file but basically it just requires specifying USB port in a Linux format (e.g. echo 1-1 > new_port). During work on this trigger there was a plan to add DT bindings for it, but there wasn't an agreement on the format yet. This can be worked on later, a sysfs interface is needed anyway for platforms not using DT. Signed-off-by: Rafał Miłecki --- V2: Trying to add DT support, idea postponed as it will take more time to discuss the bindings. V3: Fix typos in commit and Documentation (thanks Jacek!) Use "ports" sysfs file for adding and removing USB ports (thx Jacek) Check if there is USB device connected after adding new USB port Fix memory leak or two Felipe: I'd like to ask for your Ack before having this patch pushed. --- Documentation/leds/ledtrig-usbport.txt | 49 +++ drivers/leds/trigger/Kconfig | 8 ++ drivers/leds/trigger/Makefile | 1 + drivers/leds/trigger/ledtrig-usbport.c | 253 + 4 files changed, 311 insertions(+) create mode 100644 Documentation/leds/ledtrig-usbport.txt create mode 100644 drivers/leds/trigger/ledtrig-usbport.c diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt new file mode 100644 index 000..fa42227 --- /dev/null +++ b/Documentation/leds/ledtrig-usbport.txt @@ -0,0 +1,49 @@ +USB port LED trigger + + +This LED trigger can be used for signalling to the 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. Used format +matches Linux kernel format and consists of a root hub number and a hub port +separated by a dash (e.g. 3-1). + +It is also possible to handle devices with internal hubs (that are always +connected to the root hub). User can simply specify internal hub ports then +(e.g. 1-1.1, 1-1.2, etc.). + +Please note that this trigger allows assigning multiple USB ports to a single +LED. This can be useful in two cases: + +1) Device with single USB LED and few physical ports + +In such a case LED will be turned on as long as there is at least one connected +USB device. + +2) Device with a physical port handled by few controllers + +Some devices have e.g. one controller per PHY standard. E.g. USB 3.0 physical +port may be handled by ohci-platform, ehci-platform and xhci-hcd. If there is +only one LED user will most likely want to assign ports from all 3 hubs. + + +This trigger can be activated from user space on led class devices as shown +below: + + echo usbport > trigger + +This adds the following sysfs attributes to the LED: + + ports - Reading it lists all USB ports assigned to the trigger. Writing USB + port number to it will make this driver start observing it. It's also + possible to remove USB port from observable list by writing it with a + "-" prefix. + +Example use-case: + + echo usbport > trigger + echo 4-1 > ports + echo 2-1 > ports + echo -4-1 > ports + cat ports diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index 3f9ddb9..bdd6fd2 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC a different trigger. If unsure, say Y. +config LEDS_TRIGGER_USBPORT + tristate "USB port LED trigger" + depends on LEDS_TRIGGERS && USB + help + This allows LEDs to be controlled by USB events. Enabling this option + allows specifying list of USB ports that should turn on LED when some + USB device gets connected. + endif # LEDS_TRIGGERS diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile index a72c43c..56e1741 100644 --- a/drivers/leds/trigger/Makefile +++ b/drivers/leds/trigger/Makefile @@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o +obj-$(CONFIG_LEDS_TRIGGER_USBPORT) += ledtrig-usbport.o diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c new file mode 100644 index 000..7f5237c --- /dev/null +++ b/drivers/leds/trigger/ledtrig-usbport.c @@ -0,0 +1,253 @@ +/* + * USB port LED trigger + * + * Copyright (C) 2016 Rafał Miłecki + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On 24 August 2016 at 12:49, Matthias Bruggerwrote: > On 24/08/16 00:03, Rafał Miłecki wrote: >> >> From: Rafał Miłecki >> >> This commit adds a new trigger responsible for turning on LED when USB >> device gets connected to the specified USB port. This can can useful for >> various home routers that have USB port(s) and a proper LED telling user >> a device is connected. >> >> The trigger gets its documentation file but basically it just requires >> specifying USB port in a Linux format (e.g. echo 1-1 > new_port). >> >> During work on this trigger there was a plan to add DT bindings for it, >> but there wasn't an agreement on the format yet. This can be worked on >> later, a sysfs interface is needed anyway for platforms not using DT. >> >> Signed-off-by: Rafał Miłecki >> --- >> V2: Trying to add DT support, idea postponed as it will take more time >> to discuss the bindings. >> V3: Fix typos in commit and Documentation (thanks Jacek!) >> Use "ports" sysfs file for adding and removing USB ports (thx Jacek) >> Check if there is USB device connected after adding new USB port >> Fix memory leak or two >> >> Felipe: I'd like to ask for your Ack before having this patch pushed. >> --- >> Documentation/leds/ledtrig-usbport.txt | 49 +++ >> drivers/leds/trigger/Kconfig | 8 ++ >> drivers/leds/trigger/Makefile | 1 + >> drivers/leds/trigger/ledtrig-usbport.c | 253 >> + >> 4 files changed, 311 insertions(+) >> create mode 100644 Documentation/leds/ledtrig-usbport.txt >> create mode 100644 drivers/leds/trigger/ledtrig-usbport.c >> >> diff --git a/Documentation/leds/ledtrig-usbport.txt >> b/Documentation/leds/ledtrig-usbport.txt >> new file mode 100644 >> index 000..fa42227 >> --- /dev/null >> +++ b/Documentation/leds/ledtrig-usbport.txt >> @@ -0,0 +1,49 @@ >> +USB port LED trigger >> + >> + >> +This LED trigger can be used for signalling to the 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. Used >> format >> +matches Linux kernel format and consists of a root hub number and a hub >> port >> +separated by a dash (e.g. 3-1). >> + >> +It is also possible to handle devices with internal hubs (that are always >> +connected to the root hub). User can simply specify internal hub ports >> then >> +(e.g. 1-1.1, 1-1.2, etc.). >> + >> +Please note that this trigger allows assigning multiple USB ports to a >> single >> +LED. This can be useful in two cases: >> + >> +1) Device with single USB LED and few physical ports >> + >> +In such a case LED will be turned on as long as there is at least one >> connected >> +USB device. >> + >> +2) Device with a physical port handled by few controllers >> + >> +Some devices have e.g. one controller per PHY standard. E.g. USB 3.0 >> physical >> +port may be handled by ohci-platform, ehci-platform and xhci-hcd. If >> there is >> +only one LED user will most likely want to assign ports from all 3 hubs. >> + >> + >> +This trigger can be activated from user space on led class devices as >> shown >> +below: >> + >> + echo usbport > trigger >> + >> +This adds the following sysfs attributes to the LED: >> + >> + ports - Reading it lists all USB ports assigned to the trigger. Writing >> USB >> + port number to it will make this driver start observing it. It's >> also >> + possible to remove USB port from observable list by writing it >> with a >> + "-" prefix. >> + >> +Example use-case: >> + >> + echo usbport > trigger >> + echo 4-1 > ports >> + echo 2-1 > ports >> + echo -4-1 > ports >> + cat ports >> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig >> index 3f9ddb9..bdd6fd2 100644 >> --- a/drivers/leds/trigger/Kconfig >> +++ b/drivers/leds/trigger/Kconfig >> @@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC >> a different trigger. >> If unsure, say Y. >> >> +config LEDS_TRIGGER_USBPORT >> + tristate "USB port LED trigger" >> + depends on LEDS_TRIGGERS && USB >> + help >> + This allows LEDs to be controlled by USB events. Enabling this >> option >> + allows specifying list of USB ports that should turn on LED when >> some >> + USB device gets connected. >> + >> endif # LEDS_TRIGGERS >> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile >> index a72c43c..56e1741 100644 >> --- a/drivers/leds/trigger/Makefile >> +++ b/drivers/leds/trigger/Makefile >> @@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += >> ledtrig-default-on.o >> obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o >> obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o >> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o >>
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On 24 August 2016 at 11:22, Greg KHwrote: > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote: >> +static ssize_t ports_show(struct device *dev, struct device_attribute *attr, >> + char *buf) >> +{ >> + struct led_classdev *led_cdev = dev_get_drvdata(dev); >> + struct usbport_trig_data *usbport_data = led_cdev->trigger_data; >> + struct usbport_trig_port *port; >> + ssize_t ret = 0; >> + int len; >> + >> + list_for_each_entry(port, _data->ports, list) { >> + len = sprintf(buf + ret, "%s\n", port->name); >> + if (len >= 0) >> + ret += len; >> + } >> + >> + return ret; >> +} > > sysfs is "one value per file", here you are listing a bunch of things in > one sysfs file. Please don't do that. OK. What do you think about creating "ports" subdirectory and creating file-per-port in it? Then I'd need to bring back something like "new_port" and "remove_port". Does it sound OK? -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On 24 August 2016 at 11:21, Greg KHwrote: > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote: >> From: Rafał Miłecki >> >> This commit adds a new trigger responsible for turning on LED when USB >> device gets connected to the specified USB port. This can can useful for >> various home routers that have USB port(s) and a proper LED telling user >> a device is connected. >> >> The trigger gets its documentation file but basically it just requires >> specifying USB port in a Linux format (e.g. echo 1-1 > new_port). >> >> During work on this trigger there was a plan to add DT bindings for it, >> but there wasn't an agreement on the format yet. This can be worked on >> later, a sysfs interface is needed anyway for platforms not using DT. >> >> Signed-off-by: Rafał Miłecki >> --- >> V2: Trying to add DT support, idea postponed as it will take more time >> to discuss the bindings. >> V3: Fix typos in commit and Documentation (thanks Jacek!) >> Use "ports" sysfs file for adding and removing USB ports (thx Jacek) >> Check if there is USB device connected after adding new USB port >> Fix memory leak or two >> >> Felipe: I'd like to ask for your Ack before having this patch pushed. >> --- >> Documentation/leds/ledtrig-usbport.txt | 49 +++ >> drivers/leds/trigger/Kconfig | 8 ++ >> drivers/leds/trigger/Makefile | 1 + >> drivers/leds/trigger/ledtrig-usbport.c | 253 >> + >> 4 files changed, 311 insertions(+) >> create mode 100644 Documentation/leds/ledtrig-usbport.txt >> create mode 100644 drivers/leds/trigger/ledtrig-usbport.c > > You are adding sysfs files without adding a Documentation/ABI/ entry, > please never do that. This is the way all LED triggers are documented, I just blindly assumed it's OK. Could you be a bit more helpful and help me to understand further steps? Should I drop Documentation/leds/ledtrig-usbport.txt and add proper entry in Documentation/ABI/testing? Or should I keep both files? What about current sysfs of other triggers? Should they be moved/documented in ABI? -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote: > +static ssize_t ports_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct usbport_trig_data *usbport_data = led_cdev->trigger_data; > + struct usbport_trig_port *port; > + ssize_t ret = 0; > + int len; > + > + list_for_each_entry(port, _data->ports, list) { > + len = sprintf(buf + ret, "%s\n", port->name); > + if (len >= 0) > + ret += len; > + } > + > + return ret; > +} sysfs is "one value per file", here you are listing a bunch of things in one sysfs file. Please don't do that. greg k-h -- 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
Re: [PATCH V3] leds: trigger: Introduce an USB port trigger
On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote: > From: Rafał Miłecki> > This commit adds a new trigger responsible for turning on LED when USB > device gets connected to the specified USB port. This can can useful for > various home routers that have USB port(s) and a proper LED telling user > a device is connected. > > The trigger gets its documentation file but basically it just requires > specifying USB port in a Linux format (e.g. echo 1-1 > new_port). > > During work on this trigger there was a plan to add DT bindings for it, > but there wasn't an agreement on the format yet. This can be worked on > later, a sysfs interface is needed anyway for platforms not using DT. > > Signed-off-by: Rafał Miłecki > --- > V2: Trying to add DT support, idea postponed as it will take more time > to discuss the bindings. > V3: Fix typos in commit and Documentation (thanks Jacek!) > Use "ports" sysfs file for adding and removing USB ports (thx Jacek) > Check if there is USB device connected after adding new USB port > Fix memory leak or two > > Felipe: I'd like to ask for your Ack before having this patch pushed. > --- > Documentation/leds/ledtrig-usbport.txt | 49 +++ > drivers/leds/trigger/Kconfig | 8 ++ > drivers/leds/trigger/Makefile | 1 + > drivers/leds/trigger/ledtrig-usbport.c | 253 > + > 4 files changed, 311 insertions(+) > create mode 100644 Documentation/leds/ledtrig-usbport.txt > create mode 100644 drivers/leds/trigger/ledtrig-usbport.c You are adding sysfs files without adding a Documentation/ABI/ entry, please never do that. NAK. greg k-h -- 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
[PATCH V3] leds: trigger: Introduce an USB port trigger
From: Rafał MiłeckiThis commit adds a new trigger responsible for turning on LED when USB device gets connected to the specified USB port. This can can useful for various home routers that have USB port(s) and a proper LED telling user a device is connected. The trigger gets its documentation file but basically it just requires specifying USB port in a Linux format (e.g. echo 1-1 > new_port). During work on this trigger there was a plan to add DT bindings for it, but there wasn't an agreement on the format yet. This can be worked on later, a sysfs interface is needed anyway for platforms not using DT. Signed-off-by: Rafał Miłecki --- V2: Trying to add DT support, idea postponed as it will take more time to discuss the bindings. V3: Fix typos in commit and Documentation (thanks Jacek!) Use "ports" sysfs file for adding and removing USB ports (thx Jacek) Check if there is USB device connected after adding new USB port Fix memory leak or two Felipe: I'd like to ask for your Ack before having this patch pushed. --- Documentation/leds/ledtrig-usbport.txt | 49 +++ drivers/leds/trigger/Kconfig | 8 ++ drivers/leds/trigger/Makefile | 1 + drivers/leds/trigger/ledtrig-usbport.c | 253 + 4 files changed, 311 insertions(+) create mode 100644 Documentation/leds/ledtrig-usbport.txt create mode 100644 drivers/leds/trigger/ledtrig-usbport.c diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt new file mode 100644 index 000..fa42227 --- /dev/null +++ b/Documentation/leds/ledtrig-usbport.txt @@ -0,0 +1,49 @@ +USB port LED trigger + + +This LED trigger can be used for signalling to the 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. Used format +matches Linux kernel format and consists of a root hub number and a hub port +separated by a dash (e.g. 3-1). + +It is also possible to handle devices with internal hubs (that are always +connected to the root hub). User can simply specify internal hub ports then +(e.g. 1-1.1, 1-1.2, etc.). + +Please note that this trigger allows assigning multiple USB ports to a single +LED. This can be useful in two cases: + +1) Device with single USB LED and few physical ports + +In such a case LED will be turned on as long as there is at least one connected +USB device. + +2) Device with a physical port handled by few controllers + +Some devices have e.g. one controller per PHY standard. E.g. USB 3.0 physical +port may be handled by ohci-platform, ehci-platform and xhci-hcd. If there is +only one LED user will most likely want to assign ports from all 3 hubs. + + +This trigger can be activated from user space on led class devices as shown +below: + + echo usbport > trigger + +This adds the following sysfs attributes to the LED: + + ports - Reading it lists all USB ports assigned to the trigger. Writing USB + port number to it will make this driver start observing it. It's also + possible to remove USB port from observable list by writing it with a + "-" prefix. + +Example use-case: + + echo usbport > trigger + echo 4-1 > ports + echo 2-1 > ports + echo -4-1 > ports + cat ports diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index 3f9ddb9..bdd6fd2 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC a different trigger. If unsure, say Y. +config LEDS_TRIGGER_USBPORT + tristate "USB port LED trigger" + depends on LEDS_TRIGGERS && USB + help + This allows LEDs to be controlled by USB events. Enabling this option + allows specifying list of USB ports that should turn on LED when some + USB device gets connected. + endif # LEDS_TRIGGERS diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile index a72c43c..56e1741 100644 --- a/drivers/leds/trigger/Makefile +++ b/drivers/leds/trigger/Makefile @@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o +obj-$(CONFIG_LEDS_TRIGGER_USBPORT) += ledtrig-usbport.o diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c new file mode 100644 index 000..7f5237c --- /dev/null +++ b/drivers/leds/trigger/ledtrig-usbport.c @@ -0,0 +1,253 @@ +/* + * USB port LED trigger + * + * Copyright (C) 2016 Rafał Miłecki + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by +