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

login
register
mail settings
Submitter Troy Kisky
Date Sept. 26, 2013, 1:41 a.m.
Message ID <1380159678-26416-4-git-send-email-troy.kisky@boundarydevices.com>
Download mbox | patch
Permalink /patch/278063/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Troy Kisky - Sept. 26, 2013, 1:41 a.m.
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(-)
Marek Vasut - Sept. 27, 2013, 5:54 p.m.
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.

> +	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
Troy Kisky - Sept. 27, 2013, 8:03 p.m.
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
>
Marek Vasut - Sept. 27, 2013, 8:41 p.m.
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

Patch

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);
 	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 */
+	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,