On Wed, 2010-12-22 at 21:18 -0500, Alan Stern wrote:
> On Wed, 22 Dec 2010, Anoop P.A wrote:
>
> > From: Anoop P A <anoop.pa@xxxxxxxxx>
> >
> > Adding chip specific code under quirk.
>
> NAK. See below.
>
> > Signed-off-by: Anoop P A <anoop.pa@xxxxxxxxx>
> > ---
> > drivers/usb/core/hub.c | 45 ++++++++++++++++++++++++++++++++++++++-----
> > drivers/usb/core/quirks.c | 3 ++
> > include/linux/usb/quirks.h | 3 ++
> > 3 files changed, 45 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 27115b4..4bff994 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3377,12 +3377,45 @@ static void hub_events(void)
> > }
> >
> > if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
> > - dev_err (hub_dev,
> > - "over-current change on port %d\n",
> > - i);
> > - clear_port_feature(hdev, i,
> > - USB_PORT_FEAT_C_OVER_CURRENT);
> > - hub_power_on(hub, true);
> > + usb_detect_quirks(hdev);
>
> This line is wrong. usb_detect_quirks() gets called only once per
> device, when the device is initialized. Besides, you probably want to
> use a hub-specific flag for this rather than a device-specific flag.
Can you point me to an example for the recommended way of doing the
hack. I don't have much exposure to USB subsystem.
>
> > + if (hdev->quirks & USB_QUIRK_MSP_OVERCURRENT) {
>
> Also, it would be better to put this code in a separate subroutine
> instead of indenting it so far.
>
> > + /* clear OCC bit */
> > + clear_port_feature(hdev, i,
> > + USB_PORT_FEAT_C_OVER_CURRENT);
> > +
> > + /* This step is required to toggle the
> > + * PP bit to 0 and 1 (by hub_power_on)
> > + * in order the CSC bit to be
> > + * transitioned properly for device
> > + * hotplug
> > + */
> > + /* clear PP bit */
> > + clear_port_feature(hdev, i,
> > + USB_PORT_FEAT_POWER);
> > +
> > + /* resume power */
> > + hub_power_on(hub, true);
> > +
> > + /* delay 100 usec */
> > + udelay(100);
> > +
> > + /* read OCA bit */
> > + if (portstatus &
> > + (1<<USB_PORT_FEAT_OVER_CURRENT)) {
> > + /* declare overcurrent */
> > + dev_err(hub_dev,
> > + "over-current change \
> > + on port %d\n", i);
> > + }
> > + } else {
> > + dev_err(hub_dev,
> > + "over-current change \
> > + on port %d\n", i);
> > + clear_port_feature(hdev, i,
> > + USB_PORT_FEAT_C_OVER_CURRENT);
> > + hub_power_on(hub, true);
> > + }
> > +
> > }
> >
> > if (portchange & USB_PORT_STAT_C_RESET) {
> > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> > index 25719da..59843b9 100644
> > --- a/drivers/usb/core/quirks.c
> > +++ b/drivers/usb/core/quirks.c
> > @@ -88,6 +88,9 @@ static const struct usb_device_id usb_quirk_list[] = {
> > /* INTEL VALUE SSD */
> > { USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
> >
> > + /* PMC MSP over current quirk */
> > + { USB_DEVICE(0x1d6b, 0x0002), .driver_info = USB_QUIRK_MSP_OVERCURRENT },
> > +
>
> This implementation is completely wrong. It applies to all USB-2.0
> root hubs in Linux, not just the PMC MSP.
>
> > { } /* terminating entry must be last */
> > };
> >
> > diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> > index 3e93de7..97ab168 100644
> > --- a/include/linux/usb/quirks.h
> > +++ b/include/linux/usb/quirks.h
> > @@ -30,4 +30,7 @@
> > descriptor */
> > #define USB_QUIRK_DELAY_INIT 0x00000040
> >
> > +/*MSP SoC onchip EHCI overcurrent issue */
> > +#define USB_QUIRK_MSP_OVERCURRENT 0x00000080
> > +
> > #endif /* __LINUX_USB_QUIRKS_H */
> >
>
Thanks
Anoop
--
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
