On Sun, 18 Jul 2010, Phil Dibowitz wrote:
> Alan - this is the first quirk I've added outside of usb-storage... did I
> miss anything? All I really did here was a compile test.
It would be a little easier to review if your patch was inline instead
of an attachment. Nevertheless...
> The Logitech Harmony 700 series needs a second of sleep before checking
> it's speed capabilities. This adds a quirk for that device and sleeps
Typo: The word "it's" is a contraction of "it is". You mean "its".
(Also the grammar is a little wonky, but never mind.)
> if that quirk is present.
>
> Signed-off-by: Phil Dibowitz <phil@xxxxxxxx>
>
> ---
>
> linux-2.6.35-rc5-phil/drivers/usb/core/hub.c | 5 +++++
> linux-2.6.35-rc5-phil/drivers/usb/core/quirks.c | 3 +++
> linux-2.6.35-rc5-phil/include/linux/usb/quirks.h | 3 +++
> 3 files changed, 11 insertions(+)
>
> diff -puN drivers/usb/core/quirks.c~usbcore-checkspeed-quirk drivers/usb/core/quirks.c
> --- linux-2.6.35-rc5/drivers/usb/core/quirks.c~usbcore-checkspeed-quirk 2010-07-18 16:00:15.000000000 +0200
> +++ linux-2.6.35-rc5-phil/drivers/usb/core/quirks.c 2010-07-18 16:10:11.000000000 +0200
> @@ -38,6 +38,9 @@ static const struct usb_device_id usb_qu
> /* Creative SB Audigy 2 NX */
> { USB_DEVICE(0x041e, 0x3020), .driver_info = USB_QUIRK_RESET_RESUME },
>
> + /* Logitech Harmony 700-series */
> + { USB_DEVICE(0x046d, 0xc122), .driver_info = USB_QUIRK_CHECK_SPEED },
> +
> /* Philips PSC805 audio device */
> { USB_DEVICE(0x0471, 0x0155), .driver_info = USB_QUIRK_RESET_RESUME },
>
> diff -puN drivers/usb/core/hub.c~usbcore-checkspeed-quirk drivers/usb/core/hub.c
> --- linux-2.6.35-rc5/drivers/usb/core/hub.c~usbcore-checkspeed-quirk 2010-07-18 16:00:15.000000000 +0200
> +++ linux-2.6.35-rc5-phil/drivers/usb/core/hub.c 2010-07-18 16:14:27.000000000 +0200
> @@ -20,11 +20,13 @@
> #include <linux/usb.h>
> #include <linux/usbdevice_fs.h>
> #include <linux/usb/hcd.h>
> +#include <linux/usb/quirks.h>
> #include <linux/kthread.h>
> #include <linux/mutex.h>
> #include <linux/freezer.h>
> #include <linux/pm_runtime.h>
>
> +
Why add an extra, unnecessary blank line?
> #include <asm/uaccess.h>
> #include <asm/byteorder.h>
>
> @@ -2894,6 +2896,9 @@ check_highspeed (struct usb_hub *hub, st
> struct usb_qualifier_descriptor *qual;
> int status;
>
> + if (udev->quirks & USB_QUIRK_CHECK_SPEED) {
> + msleep(1000);
> + }
> qual = kmalloc (sizeof *qual, GFP_KERNEL);
> if (qual == NULL)
> return;
> diff -puN include/linux/usb/quirks.h~usbcore-checkspeed-quirk include/linux/usb/quirks.h
> --- linux-2.6.35-rc5/include/linux/usb/quirks.h~usbcore-checkspeed-quirk 2010-07-18 16:00:15.000000000 +0200
> +++ linux-2.6.35-rc5-phil/include/linux/usb/quirks.h 2010-07-18 16:04:14.000000000 +0200
> @@ -26,4 +26,7 @@
> and can't handle talking to these interfaces */
> #define USB_QUIRK_HONOR_BNUMINTERFACES 0x00000020
>
> +/* device needs a pause before checking speed */
> +#define USB_QUIRK_CHECK_SPEED 0x00000040
This is not a good name at all. Firstly, CHECK_SPEED implies that
there's something strange about the speed check, which isn't true.
(In fact, the "check" doesn't really check the speed at all; it
retrieves the Device Qualifier descriptor.) Secondly, Stephen's
testing makes it clear that the need for the delay has nothing to do
with the speed check -- the delay is needed even if the speed check is
removed entirely. It turns out the delay has to occur after we read
the Device descriptor during device initialization. A better name
would be something like USB_QUIRK_DELAY_INIT.
Also, inside check_highspeed() is not a good place to put the delay.
It really belongs inside hub_port_connect_change(), just after the call
to hub_port_init() (that's the routine which reads the Device
descriptor).
Alan Stern
--
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
