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

[Marek Vasut <marex@denx.de>]

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