Re: [PATCH v2 2/3] usb: Add LED trigger for USB host activity

2014-08-25 Thread Bryan Wu 
On Sat, Aug 23, 2014 at 2:52 AM, Michal Sojka so...@merica.cz wrote:
 Hi Bryan,

 thanks for the review. See some comments below.

 On Sat, Aug 23 2014, Bryan Wu wrote:
 On Fri, Aug 22, 2014 at 5:08 PM, Michal Sojka so...@merica.cz wrote:
 With this patch, USB host activity can be signaled by blinking a LED.

 This should work with all host controllers. Tested only with musb.

 Signed-off-by: Michal Sojka so...@merica.cz
 ---
  drivers/usb/core/Kconfig  |  9 +
  drivers/usb/core/Makefile |  1 +
  drivers/usb/core/hcd.c|  2 ++
  drivers/usb/core/led.c| 38 ++
  include/linux/usb/hcd.h   |  6 ++
  5 files changed, 56 insertions(+)
  create mode 100644 drivers/usb/core/led.c

 diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
 index 1060657..8295f65 100644
 --- a/drivers/usb/core/Kconfig
 +++ b/drivers/usb/core/Kconfig
 @@ -90,3 +90,12 @@ config USB_OTG_FSM
   Implements OTG Finite State Machine as specified in On-The-Go
   and Embedded Host Supplement to the USB Revision 2.0 
 Specification.

 +config USB_HOST_LED
 +   bool USB Host LED Trigger
 +   depends on LEDS_CLASS
 +   select LEDS_TRIGGERS
 +   help
 + This option adds a LED trigger for USB host controllers.
 +
 + Say Y here if you are working on a system with led-class supported
 + LEDs and you want to use them as USB host activity indicators.
 diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
 index 2f6f932..324c8c9 100644
 --- a/drivers/usb/core/Makefile
 +++ b/drivers/usb/core/Makefile
 @@ -9,5 +9,6 @@ usbcore-y += port.o

  usbcore-$(CONFIG_PCI)  += hcd-pci.o
  usbcore-$(CONFIG_ACPI) += usb-acpi.o
 +usbcore-$(CONFIG_USB_HOST_LED) += led.o

  obj-$(CONFIG_USB)  += usbcore.o
 diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
 index 487abcf..46d9f3a 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 (status == 0)
 +   usb_hcd_led_activity();

 /* pass ownership to the completion handler */
 urb-status = status;
 diff --git a/drivers/usb/core/led.c b/drivers/usb/core/led.c
 new file mode 100644
 index 000..49ff76c
 --- /dev/null
 +++ b/drivers/usb/core/led.c
 @@ -0,0 +1,38 @@
 +/*
 + * LED Trigger for USB Host Activity
 + *
 + * Copyright 2014 Michal Sojka so...@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/hcd.h
 +
 +#define BLINK_DELAY 30
 +
 +DEFINE_LED_TRIGGER(ledtrig_usb_hcd);

 Add one more trigger named ledtrig_usb_gadget

 +static unsigned long usb_hcd_blink_delay = BLINK_DELAY;
 +
 +void usb_hcd_led_activity(void)

 Give an input parameter like emum usb_led_event.
 USB_LED_EVENT_HOST = 0
 USB_LED_EVENT_GADGET = 1


 +{

 Add case for USB_LED_EVENT_HOST:
 +   led_trigger_blink_oneshot(ledtrig_usb_hcd,
 + usb_hcd_blink_delay, 
 usb_hcd_blink_delay, 0);

 Add case for USB_LED_EVENT_GADGET:
  led_trigger_blink_oneshot(ledtrig_usb_gadget,
  usb_gadget_blink_delay,
 usb_gadget_blink_delay, 0);

 +}
 +
 +int __init ledtrig_usb_hcd_init(void)
 +{
 +   led_trigger_register_simple(usb-host, ledtrig_usb_hcd);
 register one more trigger for gadget.

 This way, the code will be full of #ifdefs. Is the following really what
 you want? If you want to have it without #ifdefs, then I don't think it
 is a good idea to offer users the usb-gadget trigger on systems without
 usb gadget support.


Why do we need #ifdef here?
We can always define the 2 triggers for both USB host and gadget and
provide API like usb_led_activity().

If USB gadget stack is disabled in kernel, there is no users of this
usb_led_activity(, USB_LED_EVENT_GADGET). We don't need to change
anything in our driver but just waste one trigger instance.

Thanks,
-Bryan

 #define BLINK_DELAY 30

 static unsigned long usb_blink_delay = BLINK_DELAY;

 enum usb_led_event {
 USB_LED_EVENT_HOST = 0,
 USB_LED_EVENT_GADGET = 1,
 };

 #ifdef CONFIG_USB_GADGET_LED
 DEFINE_LED_TRIGGER(ledtrig_usbgadget);
 #endif
 #ifdef CONFIG_USB_HOST_LED
 DEFINE_LED_TRIGGER(ledtrig_usb_hcd);
 #endif

 void usb_led_activity(enum usb_led_event ev)
 {
 struct led_trigger *trig;

 switch (ev) {
 #ifdef CONFIG_USB_GADGET_LED
 case USB_LED_EVENT_GADGET: trig = ledtrig_usb_gadget; break;
 #endif
 #ifdef CONFIG_USB_HOST_LED
 case USB_LED_EVENT_HOST:   trig = ledtrig_usb_hcd; break;

Re: [PATCH v2 2/3] usb: Add LED trigger for USB host activity

2014-08-23 Thread Michal Sojka 
Hi Bryan,

thanks for the review. See some comments below.

On Sat, Aug 23 2014, Bryan Wu wrote:
 On Fri, Aug 22, 2014 at 5:08 PM, Michal Sojka so...@merica.cz wrote:
 With this patch, USB host activity can be signaled by blinking a LED.

 This should work with all host controllers. Tested only with musb.

 Signed-off-by: Michal Sojka so...@merica.cz
 ---
  drivers/usb/core/Kconfig  |  9 +
  drivers/usb/core/Makefile |  1 +
  drivers/usb/core/hcd.c|  2 ++
  drivers/usb/core/led.c| 38 ++
  include/linux/usb/hcd.h   |  6 ++
  5 files changed, 56 insertions(+)
  create mode 100644 drivers/usb/core/led.c

 diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
 index 1060657..8295f65 100644
 --- a/drivers/usb/core/Kconfig
 +++ b/drivers/usb/core/Kconfig
 @@ -90,3 +90,12 @@ config USB_OTG_FSM
   Implements OTG Finite State Machine as specified in On-The-Go
   and Embedded Host Supplement to the USB Revision 2.0 Specification.

 +config USB_HOST_LED
 +   bool USB Host LED Trigger
 +   depends on LEDS_CLASS
 +   select LEDS_TRIGGERS
 +   help
 + This option adds a LED trigger for USB host controllers.
 +
 + Say Y here if you are working on a system with led-class supported
 + LEDs and you want to use them as USB host activity indicators.
 diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
 index 2f6f932..324c8c9 100644
 --- a/drivers/usb/core/Makefile
 +++ b/drivers/usb/core/Makefile
 @@ -9,5 +9,6 @@ usbcore-y += port.o

  usbcore-$(CONFIG_PCI)  += hcd-pci.o
  usbcore-$(CONFIG_ACPI) += usb-acpi.o
 +usbcore-$(CONFIG_USB_HOST_LED) += led.o

  obj-$(CONFIG_USB)  += usbcore.o
 diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
 index 487abcf..46d9f3a 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 (status == 0)
 +   usb_hcd_led_activity();

 /* pass ownership to the completion handler */
 urb-status = status;
 diff --git a/drivers/usb/core/led.c b/drivers/usb/core/led.c
 new file mode 100644
 index 000..49ff76c
 --- /dev/null
 +++ b/drivers/usb/core/led.c
 @@ -0,0 +1,38 @@
 +/*
 + * LED Trigger for USB Host Activity
 + *
 + * Copyright 2014 Michal Sojka so...@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/hcd.h
 +
 +#define BLINK_DELAY 30
 +
 +DEFINE_LED_TRIGGER(ledtrig_usb_hcd);

 Add one more trigger named ledtrig_usb_gadget

 +static unsigned long usb_hcd_blink_delay = BLINK_DELAY;
 +
 +void usb_hcd_led_activity(void)

 Give an input parameter like emum usb_led_event.
 USB_LED_EVENT_HOST = 0
 USB_LED_EVENT_GADGET = 1


 +{

 Add case for USB_LED_EVENT_HOST:
 +   led_trigger_blink_oneshot(ledtrig_usb_hcd,
 + usb_hcd_blink_delay, 
 usb_hcd_blink_delay, 0);

 Add case for USB_LED_EVENT_GADGET:
  led_trigger_blink_oneshot(ledtrig_usb_gadget,
  usb_gadget_blink_delay,
 usb_gadget_blink_delay, 0);

 +}
 +
 +int __init ledtrig_usb_hcd_init(void)
 +{
 +   led_trigger_register_simple(usb-host, ledtrig_usb_hcd);
 register one more trigger for gadget.

This way, the code will be full of #ifdefs. Is the following really what
you want? If you want to have it without #ifdefs, then I don't think it
is a good idea to offer users the usb-gadget trigger on systems without
usb gadget support.

#define BLINK_DELAY 30

static unsigned long usb_blink_delay = BLINK_DELAY;

enum usb_led_event {
USB_LED_EVENT_HOST = 0,
USB_LED_EVENT_GADGET = 1,
};

#ifdef CONFIG_USB_GADGET_LED
DEFINE_LED_TRIGGER(ledtrig_usbgadget);
#endif
#ifdef CONFIG_USB_HOST_LED
DEFINE_LED_TRIGGER(ledtrig_usb_hcd);
#endif

void usb_led_activity(enum usb_led_event ev)
{
struct led_trigger *trig;

switch (ev) {
#ifdef CONFIG_USB_GADGET_LED
case USB_LED_EVENT_GADGET: trig = ledtrig_usb_gadget; break;
#endif
#ifdef CONFIG_USB_HOST_LED
case USB_LED_EVENT_HOST:   trig = ledtrig_usb_hcd; break;
#endif
default:;
}
led_trigger_blink_oneshot(trig, usb_blink_delay, usb_blink_delay, 0);
}
EXPORT_SYMBOL(usb_led_activity);


int __init ledtrig_usb_init(void)
{
#ifdef CONFIG_USB_GADGET_LED
led_trigger_register_simple(usb-gadget, ledtrig_usbgadget);
#endif
#ifdef CONFIG_USB_HOST_LED
led_trigger_register_simple(usb-host, ledtrig_usb_hcd);
#endif
return 0;
}

void __exit ledtrig_usb_exit(void)
{

Re: [PATCH v2 2/3] usb: Add LED trigger for USB host activity

2014-08-22 Thread Bryan Wu 
On Fri, Aug 22, 2014 at 5:08 PM, Michal Sojka so...@merica.cz wrote:
 With this patch, USB host activity can be signaled by blinking a LED.

 This should work with all host controllers. Tested only with musb.

 Signed-off-by: Michal Sojka so...@merica.cz
 ---
  drivers/usb/core/Kconfig  |  9 +
  drivers/usb/core/Makefile |  1 +
  drivers/usb/core/hcd.c|  2 ++
  drivers/usb/core/led.c| 38 ++
  include/linux/usb/hcd.h   |  6 ++
  5 files changed, 56 insertions(+)
  create mode 100644 drivers/usb/core/led.c

 diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
 index 1060657..8295f65 100644
 --- a/drivers/usb/core/Kconfig
 +++ b/drivers/usb/core/Kconfig
 @@ -90,3 +90,12 @@ config USB_OTG_FSM
   Implements OTG Finite State Machine as specified in On-The-Go
   and Embedded Host Supplement to the USB Revision 2.0 Specification.

 +config USB_HOST_LED
 +   bool USB Host LED Trigger
 +   depends on LEDS_CLASS
 +   select LEDS_TRIGGERS
 +   help
 + This option adds a LED trigger for USB host controllers.
 +
 + Say Y here if you are working on a system with led-class supported
 + LEDs and you want to use them as USB host activity indicators.
 diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
 index 2f6f932..324c8c9 100644
 --- a/drivers/usb/core/Makefile
 +++ b/drivers/usb/core/Makefile
 @@ -9,5 +9,6 @@ usbcore-y += port.o

  usbcore-$(CONFIG_PCI)  += hcd-pci.o
  usbcore-$(CONFIG_ACPI) += usb-acpi.o
 +usbcore-$(CONFIG_USB_HOST_LED) += led.o

  obj-$(CONFIG_USB)  += usbcore.o
 diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
 index 487abcf..46d9f3a 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 (status == 0)
 +   usb_hcd_led_activity();

 /* pass ownership to the completion handler */
 urb-status = status;
 diff --git a/drivers/usb/core/led.c b/drivers/usb/core/led.c
 new file mode 100644
 index 000..49ff76c
 --- /dev/null
 +++ b/drivers/usb/core/led.c
 @@ -0,0 +1,38 @@
 +/*
 + * LED Trigger for USB Host Activity
 + *
 + * Copyright 2014 Michal Sojka so...@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/hcd.h
 +
 +#define BLINK_DELAY 30
 +
 +DEFINE_LED_TRIGGER(ledtrig_usb_hcd);

Add one more trigger named ledtrig_usb_gadget

 +static unsigned long usb_hcd_blink_delay = BLINK_DELAY;
 +
 +void usb_hcd_led_activity(void)

Give an input parameter like emum usb_led_event.
USB_LED_EVENT_HOST = 0
USB_LED_EVENT_GADGET = 1


 +{

Add case for USB_LED_EVENT_HOST:
 +   led_trigger_blink_oneshot(ledtrig_usb_hcd,
 + usb_hcd_blink_delay, usb_hcd_blink_delay, 
 0);

Add case for USB_LED_EVENT_GADGET:
 led_trigger_blink_oneshot(ledtrig_usb_gadget,
 usb_gadget_blink_delay,
usb_gadget_blink_delay, 0);

 +}
 +
 +int __init ledtrig_usb_hcd_init(void)
 +{
 +   led_trigger_register_simple(usb-host, ledtrig_usb_hcd);
register one more trigger for gadget.

 +   return 0;
 +}
 +
 +void __exit ledtrig_usb_hcd_exit(void)
 +{
 +   led_trigger_unregister_simple(ledtrig_usb_hcd);
unregister one more trigger for gadget.

 +}
 diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
 index b43f0fe..eb5fa0f 100644
 --- a/include/linux/usb/hcd.h
 +++ b/include/linux/usb/hcd.h
 @@ -700,6 +700,12 @@ extern struct rw_semaphore ehci_cf_port_reset_rwsem;
  #define USB_EHCI_LOADED2
  extern unsigned long usb_hcds_loaded;

 +#ifdef CONFIG_USB_HOST_LED
 +extern void usb_hcd_led_activity(void);

User can call usb_led_activity(USB_LED_EVENT_HOST);
or usb_led_activity(USB_LED_EVENT_GADGET);

 +#else
 +static inline void usb_hcd_led_activity(void) {}
 +#endif
 +
  #endif /* __KERNEL__ */

  #endif /* __USB_CORE_HCD_H */
 --
 2.1.0

--
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