On Tue, May 21, 2013 at 10:37:55AM -0400, Alan Stern wrote:
> On Tue, 21 May 2013, Heikki Krogerus wrote:
> > diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> > index 595d210..a5708d9 100644
> > --- a/drivers/usb/host/ehci-pci.c
> > +++ b/drivers/usb/host/ehci-pci.c
> > @@ -322,7 +322,8 @@ static bool usb_is_intel_switchable_ehci(struct pci_dev
> > *pdev)
> > (pdev->device == 0x1E26 ||
> > pdev->device == 0x8C2D ||
> > pdev->device == 0x8C26 ||
> > - pdev->device == 0x9C26);
> > + pdev->device == 0x9C26 ||
> > + pdev->device == 0x0F34);
>
> The entries should be kept sorted in numerical order. In fact, you
> might even interchange the 0x8C2D and 0x8C26 entries.
OK.
> > diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> > index 4c338ec..6062822 100644
> > --- a/drivers/usb/host/pci-quirks.c
> > +++ b/drivers/usb/host/pci-quirks.c
> > @@ -724,6 +724,7 @@ static int handshake(void __iomem *ptr, u32 mask, u32
> > done,
> >
> > #define PCI_DEVICE_ID_INTEL_LYNX_POINT_XHCI 0x8C31
> > #define PCI_DEVICE_ID_INTEL_LYNX_POINT_LP_XHCI 0x9C31
> > +#define PCI_DEVICE_ID_INTEL_BYT_XHCI 0x0F35
> >
> > bool usb_is_intel_ppt_switchable_xhci(struct pci_dev *pdev)
> > {
> > @@ -741,10 +742,19 @@ bool usb_is_intel_lpt_switchable_xhci(struct pci_dev
> > *pdev)
> > pdev->device == PCI_DEVICE_ID_INTEL_LYNX_POINT_LP_XHCI);
> > }
> >
> > +/* And so does the Intel BayTrail. */
> > +bool usb_is_intel_byt_switchable_xhci(struct pci_dev *pdev)
> > +{
> > + return pdev->class == PCI_CLASS_SERIAL_USB_XHCI &&
> > + pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > + pdev->device == PCI_DEVICE_ID_INTEL_BYT_XHCI;
> > +}
> > +
> > bool usb_is_intel_switchable_xhci(struct pci_dev *pdev)
> > {
> > return usb_is_intel_ppt_switchable_xhci(pdev) ||
> > - usb_is_intel_lpt_switchable_xhci(pdev);
> > + usb_is_intel_lpt_switchable_xhci(pdev) ||
> > + usb_is_intel_byt_switchable_xhci(pdev);
> > }
> > EXPORT_SYMBOL_GPL(usb_is_intel_switchable_xhci);
>
> This code cries out for refactoring. Why test pdev->class and
> pdev->vendor in the same way in three different places? Those two
> tests should be moved directly into usb_is_intel_switchable_xhci().
>
> The other helper functions then become simple comparisons. As far as
> I'm concerned, they also could be moved inline into
> usb_is_intel_switchable_xhci(). Sarah may disagree, however.
Sarah what do you think?
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
