linux-leds.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/3] LED triggers for USB host and device
@ 2014-09-17  7:21 Michal Sojka
  2014-09-17  7:21 ` [PATCH v5 1/3] usb: gadget: Refactor request completion Michal Sojka
                   ` (2 more replies)
  0 siblings, 3 replies; 34+ messages in thread
From: Michal Sojka @ 2014-09-17  7:21 UTC (permalink / raw)
  To: linux-usb
  Cc: Michal Sojka, Alan Stern, Bryan Wu, Felipe Balbi,
	Greg Kroah-Hartman, Linux LED Subsystem, linux-kernel,
	michal.vokac

(this is resend of a patch series from about three weeks ago)

This adds LED triggers for USB host and device. First patch refactors
UDC drivers as requested by Felipe Balbi, second is a preparation for
the third, which adds the LED triggers.

Changes from v4:
- Added performance numbers to the commit message of the last patch
  (greg k-h).
- Replaced BUG_ON with pr_err (Alan Stern, greg k-h).
- Used proper coding style for switch statement (greg k-h).
- Added comment about NULL argument (greg k-h).
- EXPORT_SYMBOL changed to EXPORT_SYMBOL_GPL (greg k-h).
- Both triggers are now registerd even if host or gagdet subsystem
  is not enabled (Bryan Wu, greg k-h).

Changes from v3:
- usb_gadget_giveback_request() moved outside of CONFIG_HAS_DMA
  conditioned block.
- Added kernel-doc for usb_gadget_giveback_request() (Felipe Balbi).
- Removed outdated comment (Alan Stern).
- req->complete == NULL is now a bug. Previously, this was ignored
  (Alan Stern).
- File rename moved to a separate commit (greg k-h).

Changes from v2:
- Host/gadget triggers merged to a single file in usb/common/ (Felipe
  Balbi).
- UDC drivers refactored so that LED trigger works for all of them.

Changes from v1:
- Moved from drivers/leds/ to drivers/usb/.
- Improved Kconfig help.
- Linked with other modules rather than being standalone modules.

Michal Sojka (3):
  usb: gadget: Refactor request completion
  usb: Rename usb-common.c
  usb: Add LED triggers for USB activity

 drivers/usb/Kconfig                           | 10 +++++
 drivers/usb/chipidea/udc.c                    |  6 +--
 drivers/usb/common/Makefile                   |  5 ++-
 drivers/usb/common/{usb-common.c => common.c} |  0
 drivers/usb/common/led.c                      | 57 +++++++++++++++++++++++++++
 drivers/usb/core/hcd.c                        |  2 +
 drivers/usb/dwc2/gadget.c                     |  6 +--
 drivers/usb/dwc3/gadget.c                     |  2 +-
 drivers/usb/gadget/udc/amd5536udc.c           |  2 +-
 drivers/usb/gadget/udc/at91_udc.c             |  2 +-
 drivers/usb/gadget/udc/atmel_usba_udc.c       |  4 +-
 drivers/usb/gadget/udc/bcm63xx_udc.c          |  2 +-
 drivers/usb/gadget/udc/dummy_hcd.c            | 10 ++---
 drivers/usb/gadget/udc/fotg210-udc.c          |  2 +-
 drivers/usb/gadget/udc/fsl_qe_udc.c           |  6 +--
 drivers/usb/gadget/udc/fsl_udc_core.c         |  6 +--
 drivers/usb/gadget/udc/fusb300_udc.c          |  2 +-
 drivers/usb/gadget/udc/goku_udc.c             |  2 +-
 drivers/usb/gadget/udc/gr_udc.c               |  2 +-
 drivers/usb/gadget/udc/lpc32xx_udc.c          |  2 +-
 drivers/usb/gadget/udc/m66592-udc.c           |  2 +-
 drivers/usb/gadget/udc/mv_u3d_core.c          |  8 +---
 drivers/usb/gadget/udc/mv_udc_core.c          |  8 +---
 drivers/usb/gadget/udc/net2272.c              |  2 +-
 drivers/usb/gadget/udc/net2280.c              |  2 +-
 drivers/usb/gadget/udc/omap_udc.c             |  2 +-
 drivers/usb/gadget/udc/pch_udc.c              |  2 +-
 drivers/usb/gadget/udc/pxa25x_udc.c           |  2 +-
 drivers/usb/gadget/udc/pxa27x_udc.c           |  2 +-
 drivers/usb/gadget/udc/r8a66597-udc.c         |  2 +-
 drivers/usb/gadget/udc/s3c-hsudc.c            |  3 +-
 drivers/usb/gadget/udc/s3c2410_udc.c          |  2 +-
 drivers/usb/gadget/udc/udc-core.c             | 23 +++++++++++
 drivers/usb/musb/musb_gadget.c                |  2 +-
 drivers/usb/renesas_usbhs/mod_gadget.c        |  2 +-
 include/linux/usb.h                           | 12 ++++++
 include/linux/usb/gadget.h                    |  8 ++++
 37 files changed, 157 insertions(+), 57 deletions(-)
 rename drivers/usb/common/{usb-common.c => common.c} (100%)
 create mode 100644 drivers/usb/common/led.c

-- 
2.1.0

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v4 3/3] usb: Add LED triggers for USB activity
@ 2014-08-29 12:57 Michal Sojka
  2014-08-29 13:07 ` [PATCH v5 0/3] LED triggers for USB host and device Michal Sojka
  0 siblings, 1 reply; 34+ messages in thread
From: Michal Sojka @ 2014-08-29 12:57 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-usb, Alan Stern, Bryan Wu, Felipe Balbi,
	Linux LED Subsystem, linux-kernel, michal.vokac

On Wed, Aug 27 2014, Greg Kroah-Hartman wrote:
> On Wed, Aug 27, 2014 at 10:58:00PM +0200, Michal Sojka wrote:
>> With this patch, USB activity can be signaled by blinking a LED. There
>> are two triggers, one for activity on USB host and one for USB gadget.
>> 
>> Both trigger should work with all host/device controllers. Tested only
>> with musb.
>> 
>> Signed-off-by: Michal Sojka <sojka@merica.cz>
>> ---
>>  drivers/usb/Kconfig               | 11 ++++++++
>>  drivers/usb/common/Makefile       |  1 +
>>  drivers/usb/common/led.c          | 56 +++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/core/hcd.c            |  2 ++
>>  drivers/usb/gadget/udc/udc-core.c |  4 +++
>>  include/linux/usb.h               | 12 +++++++++
>>  6 files changed, 86 insertions(+)
>>  create mode 100644 drivers/usb/common/led.c
>> 
>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>> index e0cad441..fc90cda 100644
>> --- a/drivers/usb/Kconfig
>> +++ b/drivers/usb/Kconfig
>> @@ -147,4 +147,15 @@ source "drivers/usb/phy/Kconfig"
>>  
>>  source "drivers/usb/gadget/Kconfig"
>>  
>> +config USB_LED_TRIG
>> +	bool "USB LED Triggers"
>> +	depends on LEDS_CLASS && USB_COMMON
>> +	select LEDS_TRIGGERS
>
> I hate select lines, can't we just depend on LEDS_TRIGGERS as well as
> LEDS_CLASS?
>
>
>> +	help
>> +	  This option adds LED triggers for USB host and/or gadget activity.
>> +
>> +	  Say Y here if you are working on a system with led-class supported
>> +	  LEDs and you want to use them as activity indicators for USB host or
>> +	  gadget.
>> +
>>  endif # USB_SUPPORT
>> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
>> index 052c120..ca2f8bd 100644
>> --- a/drivers/usb/common/Makefile
>> +++ b/drivers/usb/common/Makefile
>> @@ -4,5 +4,6 @@
>>  
>>  obj-$(CONFIG_USB_COMMON)	  += usb-common.o
>>  usb-common-y			  += common.o
>> +usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>>  
>>  obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
>> diff --git a/drivers/usb/common/led.c b/drivers/usb/common/led.c
>> new file mode 100644
>> index 0000000..af3ce2c
>> --- /dev/null
>> +++ b/drivers/usb/common/led.c
>> @@ -0,0 +1,56 @@
>> +/*
>> + * LED Triggers for USB Activity
>> + *
>> + * Copyright 2014 Michal Sojka <sojka@merica.cz>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/leds.h>
>> +#include <linux/usb.h>
>> +
>> +#define BLINK_DELAY 30
>> +
>> +static unsigned long usb_blink_delay = BLINK_DELAY;
>> +
>> +DEFINE_LED_TRIGGER(ledtrig_usb_gadget);
>> +DEFINE_LED_TRIGGER(ledtrig_usb_host);
>> +
>> +void usb_led_activity(enum usb_led_event ev)
>> +{
>> +	struct led_trigger *trig = NULL;
>> +
>> +	switch (ev) {
>> +	case USB_LED_EVENT_GADGET: trig = ledtrig_usb_gadget; break;
>> +	case USB_LED_EVENT_HOST:   trig = ledtrig_usb_host;    break;
>> +	}
>
> Very odd formatting, please use the correct one and don't try to put
> case expressions all on one line.
>
>> +	led_trigger_blink_oneshot(trig, &usb_blink_delay, &usb_blink_delay, 0);
>
> What happens if trig is NULL?

This will work correctly - I added a comment about it in v5.

>> +}
>> +EXPORT_SYMBOL(usb_led_activity);
>
> EXPORT_SYMBOL_GPL() please.
>
>> +static int __init ledtrig_usb_init(void)
>> +{
>> +#ifdef CONFIG_USB_GADGET
>> +	led_trigger_register_simple("usb-gadget", &ledtrig_usb_gadget);
>> +#endif
>> +#ifdef CONFIG_USB
>> +	led_trigger_register_simple("usb-host", &ledtrig_usb_host);
>> +#endif
>
> Why not just always register both?

I didn't want to offer users a trigger that doesn't do anything useful
on their system. Since you are the second person suggesting registering
both, I did just that in v5.

>> +	return 0;
>> +}
>> +
>> +static void __exit ledtrig_usb_exit(void)
>> +{
>> +	led_trigger_unregister_simple(ledtrig_usb_gadget);
>> +	led_trigger_unregister_simple(ledtrig_usb_host);
>
> So you can unregister things that you never registered with no issues?

Yes, but it doesn't matter anymore in v5.

>> +}
>> +
>> +module_init(ledtrig_usb_init);
>> +module_exit(ledtrig_usb_exit);
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 487abcf..409cb95 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -1664,6 +1664,8 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>>  	usbmon_urb_complete(&hcd->self, urb, status);
>>  	usb_anchor_suspend_wakeups(anchor);
>>  	usb_unanchor_urb(urb);
>> +	if (likely(status == 0))
>> +		usb_led_activity(USB_LED_EVENT_HOST);
>
> That's a _really_ hot code path, how much is this going to cause in
> overhead?

I measured the overheads on ARM Cortex-A8 (TI AM335x) running on 600 MHz.

Duration of usb_led_activity():
- with no LED attached to the trigger:        2 ± 1 µs
- with one GPIO LED attached to the trigger:  2 ± 1 µs or 8 ± 2 µs (two peaks in histogram)

Duration of functions calling usb_led_activity()
(with my patches applied and no led attached to the trigger):
- __usb_hcd_giveback_urb():    10 - 25 µs
- usb_gadget_giveback_request(): 2 - 6 µs

With no LED attached, usb_led_activity() consists basically of
read_lock() and read_unlock().

I'll send v5 in a while.

Thanks,
-Michal

^ permalink raw reply	[flat|nested] 34+ messages in thread
end of thread, other threads:[~2014-09-29 14:05 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-17  7:21 [PATCH v5 0/3] LED triggers for USB host and device Michal Sojka
2014-09-17  7:21 ` [PATCH v5 1/3] usb: gadget: Refactor request completion Michal Sojka
2014-09-17 15:28   ` Felipe Balbi
2014-09-23  8:09     ` Michal Sojka
2014-09-24 14:48       ` Felipe Balbi
2014-09-24 15:08       ` Alan Stern
2014-09-24 20:43         ` [PATCH v6 0/4] LED triggers for USB host and device Michal Sojka
     [not found]           ` <1411591401-5874-1-git-send-email-sojka-Knnw/vAvyUalVyrhU4qvOw@public.gmane.org>
2014-09-24 20:43             ` [PATCH v6 1/4] usb: gadget: Introduce usb_gadget_giveback_request() Michal Sojka
     [not found]               ` <1411591401-5874-2-git-send-email-sojka-Knnw/vAvyUalVyrhU4qvOw@public.gmane.org>
2014-09-24 21:00                 ` Felipe Balbi
2014-09-24 20:59             ` [PATCH v6 0/4] LED triggers for USB host and device Felipe Balbi
2014-09-24 21:41               ` Greg Kroah-Hartman
     [not found]                 ` <20140924214155.GA30689-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2014-09-24 22:18                   ` Felipe Balbi
2014-09-24 23:15                     ` Felipe Balbi
2014-09-25 10:36                       ` Greg Kroah-Hartman
2014-09-25 13:56                         ` Felipe Balbi
2014-09-25 14:56                           ` Greg Kroah-Hartman
2014-09-24 20:43           ` [PATCH v6 2/4] usb: gadget: Refactor request completion Michal Sojka
2014-09-24 23:14             ` Felipe Balbi
2014-09-29  8:50             ` Robert Baldyga
2014-09-29  9:13               ` Michal Sojka
2014-09-29 14:05               ` Felipe Balbi
2014-09-24 20:43           ` [PATCH v6 3/4] usb: Rename usb-common.c Michal Sojka
     [not found]             ` <1411591401-5874-4-git-send-email-sojka-Knnw/vAvyUalVyrhU4qvOw@public.gmane.org>
2014-09-24 23:15               ` Felipe Balbi
2014-09-25 15:03             ` Greg Kroah-Hartman
2014-09-25 15:48               ` project wide: git config entry for [diff] renames=true Joe Perches
2014-09-25 18:00                 ` Jeff King
2014-09-25 18:06                   ` Joe Perches
2014-09-25 18:43                     ` Junio C Hamano
     [not found]                   ` <20140925180005.GA11755-AdEPDUrAXsQ@public.gmane.org>
2014-09-25 18:53                     ` Junio C Hamano
2014-09-24 20:43           ` [PATCH v6 4/4] usb: Add LED triggers for USB activity Michal Sojka
2014-09-24 20:56             ` Felipe Balbi
2014-09-17  7:21 ` [PATCH v5 2/3] usb: Rename usb-common.c Michal Sojka
2014-09-17  7:21 ` [PATCH v5 3/3] usb: Add LED triggers for USB activity Michal Sojka
  -- strict thread matches above, loose matches on Subject: below --
2014-08-29 12:57 [PATCH v4 " Michal Sojka
2014-08-29 13:07 ` [PATCH v5 0/3] LED triggers for USB host and device Michal Sojka
2014-08-29 13:07   ` [PATCH v5 1/3] usb: gadget: Refactor request completion Michal Sojka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).