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

[Marek Vasut]

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 at 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.

> +	if (!rc)
> +		return -EINVAL;
> +	if (rc != -ENODEV)
> +		return rc;
> +	*controller = &ehcic[index];
> +	return 0;
> +}
> +
>  int
>  submit_bulk_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
>  		int length)
> diff --git a/include/usb.h b/include/usb.h
> index 60db897..4bc50cc 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -141,6 +141,7 @@ struct usb_device {
>  	defined(CONFIG_USB_MUSB_OMAP2PLUS)
> 
>  int usb_lowlevel_init(int index, void **controller);
> +int usb_lowlevel_init_device(int index, void **controller);
>  int usb_lowlevel_stop(int index);
> 
>  int submit_bulk_msg(struct usb_device *dev, unsigned long pipe,

Best regards,
Marek Vasut