Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCH] please review mcs7830 (DeLOCK USB etherner) driver | Date | Mon, 7 Aug 2006 18:11:09 +0200 |
| |
On Monday 07 August 2006 16:54, David Hollis wrote: > These values are dupes of the MII_xxxx constants from linux/mii.h. It > would be clearer and more consistent to use those instead
ok
> These values are device specific so you would want to define them here. > Following the MII_xxxxx naming convention may be helpful.
ok
> > + > > +static DEFINE_MUTEX(mcs7830_phy_mutex); > > + > > Does this need to be global? Isn't it really just to prevent > simultaneous access to the adapters PHY? What if you have multiple > adapters installed?
It's very rarely held, so I don't expect this to be a bottleneck, even with a large number of adapters.
The implementation is slightly simpler this way, but I can move the mutex to struct usbnet instead if you prefer.
> > + dev->in = usb_rcvbulkpipe(dev->udev, 1); > > + dev->out = usb_sndbulkpipe(dev->udev, 2); > > Couldn't you use usbnet_getendpoints() here. It will also pick up the > status pipe.
Yes. usbnet_getendpoints() didn't work at first, but I think I found the problem with it now. I'll change that once I have the interrupts sorted out.
> use MII_BMSR here instead of the magic value '1'.
ok
Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |