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

[Troy Kisky <troy.kisky@boundarydevices.com>]

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 ?

Should I add the same parameter to ehci_hcd_init ?

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



>
>> +	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
>