Hi,
On Tue, Oct 04, 2011 at 01:59:59PM -0400, Alan Stern wrote:
> > > Right now dev->driver_data points to the usb_hcd structure, which has
> > > the private data allocated at the end. The allocation is done in
> > > usb_create_shared_hcd(), so that routine needs to know how big the
> > > private data should be. That's what hcd_priv_size is for -- it gives
> > > the size of the private data.
> >
> > I guess you misunderstood me. Why does usbcore need to allocate
> > priv_size ? Move that to HC drivers. So an HC driver would:
> >
> > struct my_hc_controller {
> > struct device *dev;
> > struct usb_hcd *hcd;
> > void __iomem *regs;
> >
> > ...
> > };
>
> Why would an HC driver have a new struct device in its private data
> structure?...
why not ? Coming back to my suggestion of splitting EHCI to its own
platform_driver/device, that might be necessary but you didn't like
about that part, unfortunately.
I still think that'll be the only reasonable way to have a half-way
generic ARM kernel with USB working.
I can't help considering the many different ways ARM SoCs will integrate
the same IP. You see, the EHCI controller on TI's OMAP, is a sourced IP
from Synopsys - now comes the speculation -, I'm pretty sure there are
other ehci-*.c in kernel, from other ARM SoCs which source the same IP
from Synopsys, the only change is the way the IP is integrated into the
SoC (clocks, IRQ numbers, Interconnect Bridges, etc). But that's ok,
we're all over that, I guess...
> Regardless, this is more of a change than I had in mind.
Not suitable for -rcs, for sure, but that kind of re-factor can easily
go into a merge window ;-)
> > on probe you would:
> >
> > static int __devinit my_hc_probe(struct platform_device *pdev)
> > {
> > struct my_hc_controller *my;
> > struct usb_hcd *hcd;
> >
> > my = kzalloc(sizeof(*my), GFP_KERNEL);
> > /* check */
> >
> > hcd = usb_alloc_hcd();
> > usb_hcd_set_priv_data(hcd, my);
> > my->hcd = hcd;
> >
> > usb_add_hcd(hcd);
> >
> > platform_set_drvdata(pdev, my);
> >
> > ...
> >
> > return 0;
> > }
> >
> > that's what I meant. Another option would be require HC drivers to have
> > an struct usb_hcd, rather than a pointer to it:
> >
> > struct my_hc_controller {
> > struct device *dev;
> > struct usb_hcd hcd;
> > void __iomem *regs;
> >
> > ...
> > };
>
> I'm not sure this would work with the reference counting we use for the
> usb_hcd structures.
good point.
> > on probe:
> >
> > static int __devinit my_hc_probe(struct platform_device *pdev)
> > {
> > struct my_hc_controller *my;
> >
> > my = kzalloc(sizeof(*my), GFP_KERNEL);
> > /* check */
> >
> > usb_hcd_set_priv_data(&hcd, my);
> > platform_set_drvdata(pdev, my);
> >
> > usb_add_hcd(&pdev->dev, &my->hcd);
> >
> > ...
> >
> > return 0;
> > }
> >
> > then, if you need to fetch struct my_hc_controller via a callback, you
> > can:
> >
> > #define hcd_to_my(hcd) container_of((hcd), struct my_hc_controller, hcd)
> >
> > static int my_hcd_reset(struct usb_hcd *hcd)
> > {
> > struct my_hc_controller *my = hcd_to_my(hcd);
> >
> > __my_hcd_reset(my);
> >
> > return 0;
> > }
> >
> > and so on...
>
> All this is lots more than I had in mind. Really, I was asking why one
> HC driver needs to have a clock pointer, another needs to have some PHY
> data, and the rest don't need to have anything. Shouldn't this be more
> uniform? For example, shouldn't the PHY stuff be handled in a separate
> layer?
maybe, but is it time to discuss that now ?
> What extra data is it reasonable to expect a driver to want? Whatever
> that is, we can add it directly into the usb_hcd structure.
I don't think we can come up with a truly generic solution for all hcs.
For all EHCIs, maybe, but not for all HCs.
--
balbi
Attachment:
signature.asc
Description: Digital signature
