On Thu, 11 Jun 2009 23:18:54 +0800
Greg KH <greg@xxxxxxxxx> wrote:
> On Thu, Jun 11, 2009 at 10:50:21PM +0800, Alek Du wrote:
> > Changes from v1:
> >
> >
> > +#ifdef CONFIG_USB_LANGWELL_OTG
> > +#include <linux/usb/otg.h>
> > +#include <linux/usb/langwell_otg.h>
> > +#endif
>
> Don't use ifdefs in .c code, just always include the header files
> please.
>
> > /*-------------------------------------------------------------------------*/
> >
> > /*
> > @@ -1046,6 +1051,9 @@ MODULE_LICENSE ("GPL");
> > #ifdef CONFIG_PCI
> > #include "ehci-pci.c"
> > #define PCI_DRIVER ehci_pci_driver
> > +#ifdef CONFIG_USB_LANGWELL_OTG
> > +#define LNW_OTG_HOST_DRIVER ehci_otg_driver
> > +#endif
>
> Why is this ifdef and define needed?
>
Greg, if you look into the original source file, you will find all the other drivers are hooked into ehci-hcd.c like this ...
If I use a new way ... will let this driver look different ...
> > #endif
> >
> > #ifdef CONFIG_USB_EHCI_FSL
> > @@ -1133,8 +1141,19 @@ static int __init ehci_hcd_init(void)
> > if (retval < 0)
> > goto clean3;
> > #endif
> > +
> > +#ifdef LNW_OTG_HOST_DRIVER
> > + retval = langwell_register_host(&LNW_OTG_HOST_DRIVER);
> > + if (retval < 0)
> > + goto clean4;
> > +#endif
>
> No #ifdef in .c code please, you should handle this by making
> langwell_register_host() a noop if you don't have the config option
> enabled.
>
> And what if you do have it enabled, what will happen if you don't have
> the hardware?
Here only register the type of HCD, if no hardware, then will not call the probe at all.
Here is the same as above, all the other vendor EHCI drivers are in such style ... I just made them look like same...
>
> > return retval;
> >
> > +#ifdef LNW_OTG_HOST_DRIVER
> > +clean4:
> > + langwell_unregister_host(&LNW_OTG_HOST_DRIVER);
> > +#endif
>
> Same as above. And for the rest of this patch. Please fix it up.
>
> thanks,
>
> greg k-h
--
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
