On Fri, 12 Jun 2009 22:55:16 +0800
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 12 Jun 2009, Alek Du wrote:
>
> > diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> > index d04d018..eb3dc98 100644
> > --- a/drivers/usb/host/ehci.h
> > +++ b/drivers/usb/host/ehci.h
>
> > @@ -538,10 +539,18 @@ struct ehci_fstn {
> > #define ehci_is_TDI(e) (ehci_to_hcd(e)->has_tt)
> >
> > /* Returns the speed of a device attached to a port on the root hub. */
> > -static inline unsigned int
> > +static unsigned int
> > ehci_port_speed(struct ehci_hcd *ehci, unsigned int portsc)
> > {
>
> Although this will build okay, it's not good form. Non-inline
> functions should not be defined in .h files. The function should be
> moved into ehci-hub.c.
>
> > if (ehci_is_TDI(ehci)) {
> > + if (ehci->has_hostpc) {
> > + if (portsc & (1<<25))
> > + return 1<<USB_PORT_FEAT_LOWSPEED;
> > + if (portsc & (1<<26))
> > + return 1<<USB_PORT_FEAT_HIGHSPEED;
> > + else
> > + return 0; /* Full speed */
> > + }
> > switch ((portsc>>26)&3) {
>
> Can't you unify these two paths? For example:
>
> unsigned bits;
>
> bits = (portsc >> (ehci->has_hostpc ? 25 : 26)) & 3;
> switch (bits) {
> ...
>
Ok, if I pack this well, I could still keep it as inline function, right?
I really do not want to move this function...
> By the way, why did your team decide to put the speed bits at positions
> 25 and 26 instead of 26 and 27, like everybody else? Just to make life
> more difficult for driver developers?
I guess someone who designed the chip would know the answer... But I'm sure
he won't show up in the list ...
>
> 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
