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

Troy Kisky
2013-09-27T20:03:30Z

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
>