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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Aug 23, 2014 at 2:52 AM, Michal Sojka <sojka@xxxxxxxxx> 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 <sojka@xxxxxxxxx> 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 <sojka@xxxxxxxxx>
>>> ---
>>>  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 0000000..49ff76c
>>> --- /dev/null
>>> +++ b/drivers/usb/core/led.c
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * LED Trigger for USB Host Activity
>>> + *
>>> + * Copyright 2014 Michal Sojka <sojka@xxxxxxxxx>
>>> + *
>>> + * 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;
> #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)
> {
> #ifdef CONFIG_USB_GADGET_LED
>         led_trigger_unregister_simple(ledtrig_usbgadget);
> #endif
> #ifdef CONFIG_USB_HOST_LED
>         led_trigger_unregister_simple(ledtrig_usb_hcd);
> #endif
> }
>
>
> -Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux