On Tue, 4 Oct 2011, Felipe Balbi wrote:
> Hi,
>
> On Tue, Oct 04, 2011 at 12:57:07PM -0400, Alan Stern wrote:
> > On Tue, 4 Oct 2011, Felipe Balbi wrote:
> >
> > > > Oddly enough, hcd_priv_size differs in a few cases; some of the drivers
> > > > add private data of their own. ehci-spear adds a clock, ehci-fsl adds
> > > > a PHY setting, and ehci-pxa168 adds both a clock and some PHY
> > > > resources. We're going to have to figure out a way to accomodate all
> > > > of them if they are to share the same hc_driver structure. Any ideas?
> > >
> > > just don't allocate priv_size yourself ? Something like
> > > dev->driver_data ?
> >
> > 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?...
Regardless, this is more of a change than I had in mind.
> 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.
> 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?
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.
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
