[U-Boot] [PATCH V5 03/18] usb: echi-hcd: add usb_lowlevel_init_device

Marek Vasut
2013-09-27T20:41:07Z

Dear Troy Kisky,

> On 9/27/2013 10:54 AM, Marek Vasut wrote:
> > Dear Troy Kisky,
> >
> >> Use this function so that we can verify the OTG_ID
> >> pin is high and device mode should be activated.
> >>
> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> >>
> >> ---
> >> v5: new patch
> >> ---
> >>
> >> drivers/usb/gadget/mv_udc.c | 2 +-
> >> drivers/usb/host/ehci-hcd.c | 13 +++++++++++++
> >> include/usb.h | 1 +
> >> 3 files changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/gadget/mv_udc.c b/drivers/usb/gadget/mv_udc.c
> >> index b87119c..ecd11d7 100644
> >> --- a/drivers/usb/gadget/mv_udc.c
> >> +++ b/drivers/usb/gadget/mv_udc.c
> >> @@ -684,7 +684,7 @@ int usb_gadget_register_driver(struct
> >> usb_gadget_driver *driver) if (driver->speed != USB_SPEED_FULL &&
> >> driver->speed != USB_SPEED_HIGH) return -EINVAL;
> >>
> >> - ret = usb_lowlevel_init(0, (void **)&controller.ctrl);
> >> + ret = usb_lowlevel_init_device(0, (void **)&controller.ctrl);
> >
> > You're crafting a new undocumented API here :-(
> >
> >> if (ret)
> >>
> >> return ret;
> >>
> >> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> >> index fdad739..dbea756 100644
> >> --- a/drivers/usb/host/ehci-hcd.c
> >> +++ b/drivers/usb/host/ehci-hcd.c
> >> @@ -1004,6 +1004,19 @@ int usb_lowlevel_init(int index, void
> >> **controller)
> >>
> >> return 0;
> >>
> >> }
> >>
> >> +int usb_lowlevel_init_device(int index, void **controller)
> >> +{
> >> + int rc = ehci_hcd_init(index, &ehcic[index].hccr, &ehcic[index].hcor);
> >> +
> >> + /* rc == 0 means host mode, failure for us */
> >
> > This looks pretty error-prone. A much better idea would be to use
> > usb_lowlevel_init() and add a flag to init the controller in either
> > device or host mode. That way you'd not even have to craft this strange
> > new API.
>
> How would you like the last parameter?
>
> Like "[PATCH v4] usb: new board-specific USB init interface"
>
> enum board_usb_init_type {
> USB_INIT_HOST,
> USB_INIT_DEVICE
> };
>
> Or just a plain int device_mode ?
>
> Should I wait for you to apply the above referenced patch before resending
> ?

Which one? I applied and pushed all I could.

> Should I add the same parameter to ehci_hcd_init ?

Makes sense, no ?

> And just to be perfectly clear, you want me to add the parameter to all 19
> current occurrences of usb_lowlevel_init ?

Yes, it does make sense, does it not? But look at the last take of this patch
here:

http://patchwork.ozlabs.org/patch/273911/

I'm sure some cooperation on this API would be in place now
[...]

Best regards,
Marek Vasut