On Mon, 5 Sep 2011, Felipe Balbi wrote: > > Now, locating resources is clearly platform-specific. Registering > > the hcd and defining the hc_driver structure are the only truly > > redundant parts. > > the EHCI-part is the same for all of them. Ok, I'll took ehci-atmel and > here's a sample patch removing all things which are generic. Of course > this won't work but it shows the amount of duplicated code on that file > alone: Apart from the hc_driver and various registration calls, the duplicated code is all stuff to do with locating and acquiring various resources: IRQ vectors and memory-mapped I/O regions. Clearly locating these resources will be vary, depending on the bus type. On the other hand, acquiring them could be moved into usbcore. (That's where it is already for PCI controllers.) There's no good reason to dump all this information into more platform resources. There already are fields for these things in the hcd structure. The code to populate those fields could be shared among all the ehci-<arch> drivers. > > > . ?hci-hcd stacks are PCI centric > > > > In what way? The only PCI-centric part of ehci-hcd is ehci-pci.c, > > $ git grep -e "msi" drivers/usb/host > $ git grep -e "pci\|PCI" drivers/usb/host/ | grep -v xhci-pci | \ > grep -v ehci-pci | grep -v ohci-pci | grep -v uhci-pci \ > | grep -v isp1760 | grep -v pci-quirks | grep -v oxu210 Let's take just the EHCI drivers as a representative example. Looking at drivers/usb/host/e*.[ch], and removing all the lines that are just comments or actually part of the PCI implementation, there's nothing left. > > True, xhci-hcd is currently very PCI-centric. That's because until > > very recently there was no non-PCI xHCI hardware available. > > Still, all other ?hci got implementations on embedded, so it would be at > least a good bet to make a driver which doesn't depend on PCI. Of course. I agree, xhci-hcd needs to be updated. > > > . ?hci-hcd PM handling is coupled with the underlying architecture > > > > Are you talking about the bus_suspend/bus_resume methods? The methods > > themselves are in the common code; only the method pointers are > > duplicated in the hc_driver structures. > > It comes down to the other comment I had: if it's common code, why do > all *-$arch.c need to pass it ? This is the same objection as before. When the hc_driver structure is no longer duplicated, its members (including bus_suspend and bus_resume) won't be duplicated either. > > > . lack of pm_runtime on ?hci-hcd (it could be used for e.g. > > > bus_suspend/bus_resume) > > > > No, the runtime PM support _is_ present, but it is in usbcore instead > > in the host controller drivers. > > We can use device hierarchy for that, no ? Instead of adding methods to > struct hc_driver, implement things on dev_pm_ops. I don't understand. We _are_ using the device hierarchy for this. The dev_pm_ops methods invoke the callbacks in hc_driver. I don't see any way around this, because the dev_pm_ops methods are part of usbcore, not duplicated in each host controller driver. > > Here's a different approach which addresses all the problems you listed > > above -- and does so without adding extra layers to the device model. > > It's a lot like what you're suggesting, but without a bunch of > > extraneous stuff. > > > > Make each ehci-<arch>.c into a standalone module. Remove all the > > duplicated hc_drivers structures from these files, put a single > > publicly-available ehci_hc_driver in ehci-hcd.c, and make ehci-hcd a > > separate module containing just the core routines. > > why making ehci_hc_driver publicly available ? It doesn't have to be. Because it's better to have one exported symbol than a whole bunch of useless platform device structures. > If it's publicly available you could allow (now, the following is really > farfetched) someone to write a ehci-bomb.c file which passes bogus > function pointers to ehci-stack. And it will get executed. You might as well complain about the fact that usb_submit_urb() is exported. Somebody could pass it an URB with a bogus function pointer for the completion handler. > If, on the other hand, ehci_hc_driver is static (and, if possible, > const) that will not happen. The point is, if it's > common/generic/standardized/whatever, why do you need to publicize it ? So that drivers like ehci-tegra can copy it and then override certain fields with their own entry points. The alternative, which would be less efficient, is to publicize a routine that calls usb_create_hcd() with the static ehci_hc_driver as an argument. > > To make this work, we have to find a way to handle all the individual > > differences in the various hc_driver structures. Most of the > > differences lie in the product_desc and reset fields. > > you can pass platform_data to ehci-stack core driver. Why bother, when the data you want to send will just end up in the hcd structure anyway? Why not put it there directly? 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
