* [PATCHv3 1/4] usb: Provide usb_device_speed_name() function
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
@ 2011-08-19 22:32 ` Michal Nazarewicz
2011-08-19 23:15 ` Felipe Balbi
2011-08-19 22:33 ` [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed" Michal Nazarewicz
` (2 subsequent siblings)
3 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:32 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
In a few places kernel wants to print a human-readable USB
device speed name (eg. "high") instead of a raw number
(eg. 3). Usually a switch is introduced in those places
leading to code repetition. To mitigate this issue, this
commit introduces usb_device_speed_name() function, which
returns a human-readable name of provided speed.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/Makefile | 3 +++
drivers/usb/common.c | 24 ++++++++++++++++++++++++
drivers/usb/gadget/udc-core.c | 19 ++-----------------
include/linux/usb/ch9.h | 8 ++++++++
4 files changed, 37 insertions(+), 17 deletions(-)
create mode 100644 drivers/usb/common.c
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 30ddf8d..a3167cc 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -51,3 +51,6 @@ obj-$(CONFIG_USB_MUSB_HDRC) += musb/
obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
obj-$(CONFIG_USB_OTG_UTILS) += otg/
obj-$(CONFIG_USB_GADGET) += gadget/
+
+obj-$(CONFIG_USB) += common.o
+obj-$(CONFIG_USB_GADGET) += common.o
diff --git a/drivers/usb/common.c b/drivers/usb/common.c
new file mode 100644
index 0000000..e9d7141
--- /dev/null
+++ b/drivers/usb/common.c
@@ -0,0 +1,24 @@
+/*
+ * Provides code common for host and device side USB.
+ */
+
+#include <linux/kernel.h> /* for ARRAY_SIZE() */
+#include <linux/module.h> /* for EXPORT_SYMBOL_GPL() */
+#include <linux/usb/ch9.h>
+
+const char *usb_device_speed_name(enum usb_device_speed speed)
+{
+ static const char *const names[] = {
+ "UNKNOWN",
+ "low-speed",
+ "full-speed",
+ "high-speed",
+ "wireless",
+ "super-speed",
+ };
+
+ if (speed < 0 || speed >= ARRAY_SIZE(names))
+ speed = USB_SPEED_UNKNOWN;
+ return names[speed];
+}
+EXPORT_SYMBOL_GPL(usb_device_speed_name);
diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index 05ba472..e1ecdbc 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -375,23 +375,8 @@ static ssize_t usb_udc_speed_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
- struct usb_gadget *gadget = udc->gadget;
-
- switch (gadget->speed) {
- case USB_SPEED_LOW:
- return snprintf(buf, PAGE_SIZE, "low-speed\n");
- case USB_SPEED_FULL:
- return snprintf(buf, PAGE_SIZE, "full-speed\n");
- case USB_SPEED_HIGH:
- return snprintf(buf, PAGE_SIZE, "high-speed\n");
- case USB_SPEED_WIRELESS:
- return snprintf(buf, PAGE_SIZE, "wireless\n");
- case USB_SPEED_SUPER:
- return snprintf(buf, PAGE_SIZE, "super-speed\n");
- case USB_SPEED_UNKNOWN: /* FALLTHROUGH */
- default:
- return snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
- }
+ return snprintf(buf, PAGE_SIZE, "%s\n",
+ usb_device_speed_name(udc->gadget->speed));
}
static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index 0fd3fbd..61c4c80 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -851,6 +851,14 @@ enum usb_device_speed {
USB_SPEED_SUPER, /* usb 3.0 */
};
+/**
+ * usb_device_speed_name() - Returns human readable-name of the speed.
+ * @speed: The speed to return human-readable name for. If it's not
+ * any of the speeds defined in usb_device_speed enum, string for
+ * USB_SPEED_UNKNOWN will be returned.
+ */
+extern const char *usb_device_speed_name(enum usb_device_speed speed);
+
enum usb_device_state {
/* NOTATTACHED isn't in the USB spec, and this state acts
* the same as ATTACHED ... but it's clearer this way.
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCHv3 1/4] usb: Provide usb_device_speed_name() function
2011-08-19 22:32 ` [PATCHv3 1/4] usb: Provide usb_device_speed_name() function Michal Nazarewicz
@ 2011-08-19 23:15 ` Felipe Balbi
2011-08-22 14:53 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-19 23:15 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3646 bytes --]
Hi,
On Sat, Aug 20, 2011 at 12:32:59AM +0200, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> In a few places kernel wants to print a human-readable USB
> device speed name (eg. "high") instead of a raw number
> (eg. 3). Usually a switch is introduced in those places
> leading to code repetition. To mitigate this issue, this
> commit introduces usb_device_speed_name() function, which
> returns a human-readable name of provided speed.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> drivers/usb/Makefile | 3 +++
> drivers/usb/common.c | 24 ++++++++++++++++++++++++
> drivers/usb/gadget/udc-core.c | 19 ++-----------------
> include/linux/usb/ch9.h | 8 ++++++++
> 4 files changed, 37 insertions(+), 17 deletions(-)
> create mode 100644 drivers/usb/common.c
>
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index 30ddf8d..a3167cc 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -51,3 +51,6 @@ obj-$(CONFIG_USB_MUSB_HDRC) += musb/
> obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
> obj-$(CONFIG_USB_OTG_UTILS) += otg/
> obj-$(CONFIG_USB_GADGET) += gadget/
> +
> +obj-$(CONFIG_USB) += common.o
> +obj-$(CONFIG_USB_GADGET) += common.o
> diff --git a/drivers/usb/common.c b/drivers/usb/common.c
> new file mode 100644
> index 0000000..e9d7141
> --- /dev/null
> +++ b/drivers/usb/common.c
> @@ -0,0 +1,24 @@
> +/*
> + * Provides code common for host and device side USB.
> + */
> +
> +#include <linux/kernel.h> /* for ARRAY_SIZE() */
> +#include <linux/module.h> /* for EXPORT_SYMBOL_GPL() */
> +#include <linux/usb/ch9.h>
> +
> +const char *usb_device_speed_name(enum usb_device_speed speed)
somehow usb_speed_string() sounds better to me.
> +{
> + static const char *const names[] = {
> + "UNKNOWN",
> + "low-speed",
> + "full-speed",
> + "high-speed",
> + "wireless",
> + "super-speed",
> + };
> +
> + if (speed < 0 || speed >= ARRAY_SIZE(names))
> + speed = USB_SPEED_UNKNOWN;
I generally dislike depending on a particular ordering of enumerations.
While's it's highly unlikely that anyone will change that, if someone
does change, then it'll take a long time to actually find the bug on the
print here :-)
I woud rather use a switch in this case.
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index 05ba472..e1ecdbc 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -375,23 +375,8 @@ static ssize_t usb_udc_speed_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> - struct usb_gadget *gadget = udc->gadget;
> -
> - switch (gadget->speed) {
> - case USB_SPEED_LOW:
> - return snprintf(buf, PAGE_SIZE, "low-speed\n");
> - case USB_SPEED_FULL:
> - return snprintf(buf, PAGE_SIZE, "full-speed\n");
> - case USB_SPEED_HIGH:
> - return snprintf(buf, PAGE_SIZE, "high-speed\n");
> - case USB_SPEED_WIRELESS:
> - return snprintf(buf, PAGE_SIZE, "wireless\n");
> - case USB_SPEED_SUPER:
> - return snprintf(buf, PAGE_SIZE, "super-speed\n");
> - case USB_SPEED_UNKNOWN: /* FALLTHROUGH */
> - default:
> - return snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
> - }
> + return snprintf(buf, PAGE_SIZE, "%s\n",
> + usb_device_speed_name(udc->gadget->speed));
> }
> static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
you said 'several locations', but there's only one ? No other driver is
using something similar ?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCHv3 1/4] usb: Provide usb_device_speed_name() function
2011-08-19 23:15 ` Felipe Balbi
@ 2011-08-22 14:53 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-22 14:53 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
On Sat, 20 Aug 2011 01:15:00 +0200, Felipe Balbi <balbi@ti.com> wrote:
> On Sat, Aug 20, 2011 at 12:32:59AM +0200, Michal Nazarewicz wrote:
>> From: Michal Nazarewicz <mina86@mina86.com>
>>
>> In a few places kernel wants to print a human-readable USB
>> device speed name (eg. "high") instead of a raw number
>> (eg. 3). Usually a switch is introduced in those places
>> leading to code repetition. To mitigate this issue, this
>> commit introduces usb_device_speed_name() function, which
>> returns a human-readable name of provided speed.
>>
>> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
>> ---
>> drivers/usb/Makefile | 3 +++
>> drivers/usb/common.c | 24 ++++++++++++++++++++++++
>> drivers/usb/gadget/udc-core.c | 19 ++-----------------
>> include/linux/usb/ch9.h | 8 ++++++++
>> 4 files changed, 37 insertions(+), 17 deletions(-)
>> create mode 100644 drivers/usb/common.c
>>
>> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
>> index 30ddf8d..a3167cc 100644
>> --- a/drivers/usb/Makefile
>> +++ b/drivers/usb/Makefile
>> @@ -51,3 +51,6 @@ obj-$(CONFIG_USB_MUSB_HDRC) += musb/
>> obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
>> obj-$(CONFIG_USB_OTG_UTILS) += otg/
>> obj-$(CONFIG_USB_GADGET) += gadget/
>> +
>> +obj-$(CONFIG_USB) += common.o
>> +obj-$(CONFIG_USB_GADGET) += common.o
>> diff --git a/drivers/usb/common.c b/drivers/usb/common.c
>> new file mode 100644
>> index 0000000..e9d7141
>> --- /dev/null
>> +++ b/drivers/usb/common.c
>> @@ -0,0 +1,24 @@
>> +/*
>> + * Provides code common for host and device side USB.
>> + */
>> +
>> +#include <linux/kernel.h> /* for ARRAY_SIZE() */
>> +#include <linux/module.h> /* for EXPORT_SYMBOL_GPL() */
>> +#include <linux/usb/ch9.h>
>> +
>> +const char *usb_device_speed_name(enum usb_device_speed speed)
>
> somehow usb_speed_string() sounds better to me.
Done.
>> +{
>> + static const char *const names[] = {
>> + "UNKNOWN",
>> + "low-speed",
>> + "full-speed",
>> + "high-speed",
>> + "wireless",
>> + "super-speed",
>> + };
>> +
>> + if (speed < 0 || speed >= ARRAY_SIZE(names))
>> + speed = USB_SPEED_UNKNOWN;
>
> I generally dislike depending on a particular ordering of enumerations.
> While's it's highly unlikely that anyone will change that, if someone
> does change, then it'll take a long time to actually find the bug on the
> print here :-)
>
> I woud rather use a switch in this case.
Would a C99 initialisation be fine by you?
I, myself, have this aversion to switch as being slower (which is
disputable) and too verbose.
>> diff --git a/drivers/usb/gadget/udc-core.c
>> b/drivers/usb/gadget/udc-core.c
>> index 05ba472..e1ecdbc 100644
>> --- a/drivers/usb/gadget/udc-core.c
>> +++ b/drivers/usb/gadget/udc-core.c
>> @@ -375,23 +375,8 @@ static ssize_t usb_udc_speed_show(struct device
>> *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
>> - struct usb_gadget *gadget = udc->gadget;
>> -
>> - switch (gadget->speed) {
>> - case USB_SPEED_LOW:
>> - return snprintf(buf, PAGE_SIZE, "low-speed\n");
>> - case USB_SPEED_FULL:
>> - return snprintf(buf, PAGE_SIZE, "full-speed\n");
>> - case USB_SPEED_HIGH:
>> - return snprintf(buf, PAGE_SIZE, "high-speed\n");
>> - case USB_SPEED_WIRELESS:
>> - return snprintf(buf, PAGE_SIZE, "wireless\n");
>> - case USB_SPEED_SUPER:
>> - return snprintf(buf, PAGE_SIZE, "super-speed\n");
>> - case USB_SPEED_UNKNOWN: /* FALLTHROUGH */
>> - default:
>> - return snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
>> - }
>> + return snprintf(buf, PAGE_SIZE, "%s\n",
>> + usb_device_speed_name(udc->gadget->speed));
>> }
>> static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
>
> you said 'several locations', but there's only one ? No other driver is
> using something similar ?
OK, you caught me on being lazy... ;) Hunted down a few more instances.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
2011-08-19 22:32 ` [PATCHv3 1/4] usb: Provide usb_device_speed_name() function Michal Nazarewicz
@ 2011-08-19 22:33 ` Michal Nazarewicz
2011-08-19 23:28 ` Felipe Balbi
2011-08-19 22:33 ` [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed Michal Nazarewicz
2011-08-19 22:33 ` [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED Michal Nazarewicz
3 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:33 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
This commit replaces usb_gadget's is_dualspeed field with
a max_speed field.
This change is made so that one will be able to check at
run-time if given gadget supports super speed.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/gadget/amd5536udc.c | 2 +-
drivers/usb/gadget/atmel_usba_udc.c | 2 +-
drivers/usb/gadget/ci13xxx_udc.c | 7 +++++--
drivers/usb/gadget/dummy_hcd.c | 2 +-
drivers/usb/gadget/epautoconf.c | 6 +++---
drivers/usb/gadget/fsl_udc_core.c | 2 +-
drivers/usb/gadget/fusb300_udc.c | 2 +-
drivers/usb/gadget/goku_udc.c | 1 +
drivers/usb/gadget/langwell_udc.c | 2 +-
drivers/usb/gadget/m66592-udc.c | 2 +-
drivers/usb/gadget/mv_udc_core.c | 2 +-
drivers/usb/gadget/net2272.c | 2 +-
drivers/usb/gadget/net2280.c | 2 +-
drivers/usb/gadget/omap_udc.c | 1 +
drivers/usb/gadget/pch_udc.c | 2 +-
drivers/usb/gadget/printer.c | 4 ++--
drivers/usb/gadget/r8a66597-udc.c | 2 +-
drivers/usb/gadget/s3c-hsotg.c | 2 +-
drivers/usb/gadget/s3c-hsudc.c | 2 +-
drivers/usb/gadget/udc-core.c | 26 ++++++++++++++++++++------
drivers/usb/musb/musb_gadget.c | 2 +-
drivers/usb/renesas_usbhs/mod_gadget.c | 2 +-
include/linux/usb/gadget.h | 10 +++++-----
23 files changed, 53 insertions(+), 34 deletions(-)
diff --git a/drivers/usb/gadget/amd5536udc.c b/drivers/usb/gadget/amd5536udc.c
index 70f2b37..80ae808 100644
--- a/drivers/usb/gadget/amd5536udc.c
+++ b/drivers/usb/gadget/amd5536udc.c
@@ -3363,7 +3363,7 @@ static int udc_probe(struct udc *dev)
dev_set_name(&dev->gadget.dev, "gadget");
dev->gadget.dev.release = gadget_release;
dev->gadget.name = name;
- dev->gadget.is_dualspeed = 1;
+ dev->gadget.max_speed = USB_SPEED_HIGH;
/* init registers, interrupts, ... */
startup_registers(dev);
diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 5b1665e..8837d92c 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1038,7 +1038,7 @@ static struct usba_udc the_udc = {
.gadget = {
.ops = &usba_udc_ops,
.ep_list = LIST_HEAD_INIT(the_udc.gadget.ep_list),
- .is_dualspeed = 1,
+ .max_speed = USB_SPEED_HIGH,
.name = "atmel_usba_udc",
.dev = {
.init_name = "gadget",
diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
index 1265a85..b877464 100644
--- a/drivers/usb/gadget/ci13xxx_udc.c
+++ b/drivers/usb/gadget/ci13xxx_udc.c
@@ -751,8 +751,11 @@ static ssize_t show_device(struct device *dev, struct device_attribute *attr,
n += scnprintf(buf + n, PAGE_SIZE - n, "speed = %d\n",
gadget->speed);
+ n += scnprintf(buf + n, PAGE_SIZE - n, "max_speed = %d\n",
+ gadget->max_speed);
+ /* Provide "is_dualspeed" for backward compatibility. */
n += scnprintf(buf + n, PAGE_SIZE - n, "is_dualspeed = %d\n",
- gadget->is_dualspeed);
+ gadget_is_dualspeed(gadget));
n += scnprintf(buf + n, PAGE_SIZE - n, "is_otg = %d\n",
gadget->is_otg);
n += scnprintf(buf + n, PAGE_SIZE - n, "is_a_peripheral = %d\n",
@@ -2866,7 +2869,7 @@ static int udc_probe(struct ci13xxx_udc_driver *driver, struct device *dev,
udc->gadget.ops = &usb_gadget_ops;
udc->gadget.speed = USB_SPEED_UNKNOWN;
- udc->gadget.is_dualspeed = 1;
+ udc->gadget.max_speed = USB_SPEED_HIGH;
udc->gadget.is_otg = 0;
udc->gadget.name = driver->name;
diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index e755a9d..fa60e9b 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -986,7 +986,7 @@ static int dummy_udc_probe (struct platform_device *pdev)
dum->gadget.name = gadget_name;
dum->gadget.ops = &dummy_ops;
- dum->gadget.is_dualspeed = 1;
+ dum->gadget.max_speed = USB_SPEED_SUPER;;
dev_set_name(&dum->gadget.dev, "gadget");
dum->gadget.dev.parent = &pdev->dev;
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 7a7e6b7..5b40f4b 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -162,7 +162,7 @@ ep_matches (
switch (type) {
case USB_ENDPOINT_XFER_INT:
/* INT: limit 64 bytes full speed, 1024 high/super speed */
- if (!gadget->is_dualspeed && max > 64)
+ if (!gadget_is_dualspeed(gadget) && max > 64)
return 0;
/* FALLTHROUGH */
@@ -170,12 +170,12 @@ ep_matches (
/* ISO: limit 1023 bytes full speed, 1024 high/super speed */
if (ep->maxpacket < max)
return 0;
- if (!gadget->is_dualspeed && max > 1023)
+ if (!gadget_is_dualspeed(gadget) && max > 1023)
return 0;
/* BOTH: "high bandwidth" works only at high speed */
if ((desc->wMaxPacketSize & cpu_to_le16(3<<11))) {
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
return 0;
/* configure your hardware with enough buffering!! */
}
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index de24a42..b16034c 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -2546,7 +2546,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
/* Setup gadget structure */
udc_controller->gadget.ops = &fsl_gadget_ops;
- udc_controller->gadget.is_dualspeed = 1;
+ udc_controller->gadget.max_speed = USB_SPEED_HIGH;
udc_controller->gadget.ep0 = &udc_controller->eps[0].ep;
INIT_LIST_HEAD(&udc_controller->gadget.ep_list);
udc_controller->gadget.speed = USB_SPEED_UNKNOWN;
diff --git a/drivers/usb/gadget/fusb300_udc.c b/drivers/usb/gadget/fusb300_udc.c
index 0e4489f..731675f 100644
--- a/drivers/usb/gadget/fusb300_udc.c
+++ b/drivers/usb/gadget/fusb300_udc.c
@@ -1473,7 +1473,7 @@ static int __init fusb300_probe(struct platform_device *pdev)
dev_set_name(&fusb300->gadget.dev, "gadget");
- fusb300->gadget.is_dualspeed = 1;
+ fusb300->gadget.max_speed = USB_SPEED_HIGH;
fusb300->gadget.dev.parent = &pdev->dev;
fusb300->gadget.dev.dma_mask = pdev->dev.dma_mask;
fusb300->gadget.dev.release = pdev->dev.release;
diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
index 7f87805..ab9c924 100644
--- a/drivers/usb/gadget/goku_udc.c
+++ b/drivers/usb/gadget/goku_udc.c
@@ -1796,6 +1796,7 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
spin_lock_init(&dev->lock);
dev->pdev = pdev;
dev->gadget.ops = &goku_ops;
+ dev->gadget.max_speed = USB_SPEED_FULL;
/* the "gadget" abstracts/virtualizes the controller */
dev_set_name(&dev->gadget.dev, "gadget");
diff --git a/drivers/usb/gadget/langwell_udc.c b/drivers/usb/gadget/langwell_udc.c
index a06e2c2..121a64b 100644
--- a/drivers/usb/gadget/langwell_udc.c
+++ b/drivers/usb/gadget/langwell_udc.c
@@ -3320,7 +3320,7 @@ static int langwell_udc_probe(struct pci_dev *pdev,
dev->gadget.ep0 = &dev->ep[0].ep; /* gadget ep0 */
INIT_LIST_HEAD(&dev->gadget.ep_list); /* ep_list */
dev->gadget.speed = USB_SPEED_UNKNOWN; /* speed */
- dev->gadget.is_dualspeed = 1; /* support dual speed */
+ dev->gadget.max_speed = USB_SPEED_HIGH; /* support dual speed */
#ifdef OTG_TRANSCEIVER
dev->gadget.is_otg = 1; /* support otg mode */
#endif
diff --git a/drivers/usb/gadget/m66592-udc.c b/drivers/usb/gadget/m66592-udc.c
index 491f825..fbdfca8 100644
--- a/drivers/usb/gadget/m66592-udc.c
+++ b/drivers/usb/gadget/m66592-udc.c
@@ -1663,7 +1663,7 @@ static int __init m66592_probe(struct platform_device *pdev)
m66592->gadget.ops = &m66592_gadget_ops;
device_initialize(&m66592->gadget.dev);
dev_set_name(&m66592->gadget.dev, "gadget");
- m66592->gadget.is_dualspeed = 1;
+ m66592->gadget.max_speed = USB_SPEED_HIGH;
m66592->gadget.dev.parent = &pdev->dev;
m66592->gadget.dev.dma_mask = pdev->dev.dma_mask;
m66592->gadget.dev.release = pdev->dev.release;
diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
index ce1ac2b..17e9c5d 100644
--- a/drivers/usb/gadget/mv_udc_core.c
+++ b/drivers/usb/gadget/mv_udc_core.c
@@ -2061,7 +2061,7 @@ int mv_udc_probe(struct platform_device *dev)
udc->gadget.ep0 = &udc->eps[0].ep; /* gadget ep0 */
INIT_LIST_HEAD(&udc->gadget.ep_list); /* ep_list */
udc->gadget.speed = USB_SPEED_UNKNOWN; /* speed */
- udc->gadget.is_dualspeed = 1; /* support dual speed */
+ udc->gadget.max_speed = USB_SPEED_HIGH; /* support dual speed */
/* the "gadget" abstracts/virtualizes the controller */
dev_set_name(&udc->gadget.dev, "gadget");
diff --git a/drivers/usb/gadget/net2272.c b/drivers/usb/gadget/net2272.c
index ab98ea9..e21ec48 100644
--- a/drivers/usb/gadget/net2272.c
+++ b/drivers/usb/gadget/net2272.c
@@ -2265,7 +2265,7 @@ net2272_probe_init(struct device *dev, unsigned int irq)
ret->irq = irq;
ret->dev = dev;
ret->gadget.ops = &net2272_ops;
- ret->gadget.is_dualspeed = 1;
+ ret->gadget.max_speed = USB_SPEED_HIGH;
/* the "gadget" abstracts/virtualizes the controller */
dev_set_name(&ret->gadget.dev, "gadget");
diff --git a/drivers/usb/gadget/net2280.c b/drivers/usb/gadget/net2280.c
index 3dd40b4..c3220a1 100644
--- a/drivers/usb/gadget/net2280.c
+++ b/drivers/usb/gadget/net2280.c
@@ -2743,7 +2743,7 @@ static int net2280_probe (struct pci_dev *pdev, const struct pci_device_id *id)
spin_lock_init (&dev->lock);
dev->pdev = pdev;
dev->gadget.ops = &net2280_ops;
- dev->gadget.is_dualspeed = 1;
+ dev->gadget.max_speed = USB_SPEED_HIGH;
/* the "gadget" abstracts/virtualizes the controller */
dev_set_name(&dev->gadget.dev, "gadget");
diff --git a/drivers/usb/gadget/omap_udc.c b/drivers/usb/gadget/omap_udc.c
index 740c7da..fff7e1c 100644
--- a/drivers/usb/gadget/omap_udc.c
+++ b/drivers/usb/gadget/omap_udc.c
@@ -2686,6 +2686,7 @@ omap_udc_setup(struct platform_device *odev, struct otg_transceiver *xceiv)
INIT_LIST_HEAD(&udc->gadget.ep_list);
INIT_LIST_HEAD(&udc->iso);
udc->gadget.speed = USB_SPEED_UNKNOWN;
+ udc->gadget.max_speed = USB_SPEED_FULL;
udc->gadget.name = driver_name;
device_initialize(&udc->gadget.dev);
diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
index f96615a..d781540 100644
--- a/drivers/usb/gadget/pch_udc.c
+++ b/drivers/usb/gadget/pch_udc.c
@@ -2949,7 +2949,7 @@ static int pch_udc_probe(struct pci_dev *pdev,
dev->gadget.dev.dma_mask = pdev->dev.dma_mask;
dev->gadget.dev.release = gadget_release;
dev->gadget.name = KBUILD_MODNAME;
- dev->gadget.is_dualspeed = 1;
+ dev->gadget.max_speed = USB_SPEED_HIGH;
retval = device_register(&dev->gadget.dev);
if (retval)
diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
index a341dde..2db4913 100644
--- a/drivers/usb/gadget/printer.c
+++ b/drivers/usb/gadget/printer.c
@@ -1158,7 +1158,7 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
break;
/*
* assumes ep0 uses the same value for both
@@ -1172,7 +1172,7 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
case USB_DT_OTHER_SPEED_CONFIG:
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
break;
/* FALLTHROUGH */
#endif /* CONFIG_USB_GADGET_DUALSPEED */
diff --git a/drivers/usb/gadget/r8a66597-udc.c b/drivers/usb/gadget/r8a66597-udc.c
index 50991e5..a8f7a05 100644
--- a/drivers/usb/gadget/r8a66597-udc.c
+++ b/drivers/usb/gadget/r8a66597-udc.c
@@ -1616,7 +1616,7 @@ static int __init r8a66597_probe(struct platform_device *pdev)
r8a66597->gadget.ops = &r8a66597_gadget_ops;
device_initialize(&r8a66597->gadget.dev);
dev_set_name(&r8a66597->gadget.dev, "gadget");
- r8a66597->gadget.is_dualspeed = 1;
+ r8a66597->gadget.max_speed = USB_SPEED_HIGH;
r8a66597->gadget.dev.parent = &pdev->dev;
r8a66597->gadget.dev.dma_mask = pdev->dev.dma_mask;
r8a66597->gadget.dev.release = pdev->dev.release;
diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index 8bdee67..88dd131 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -3368,7 +3368,7 @@ static int __devinit s3c_hsotg_probe(struct platform_device *pdev)
dev_set_name(&hsotg->gadget.dev, "gadget");
- hsotg->gadget.is_dualspeed = 1;
+ hsotg->gadget.max_speed = USB_SPEED_HIGH;
hsotg->gadget.ops = &s3c_hsotg_gadget_ops;
hsotg->gadget.name = dev_name(dev);
diff --git a/drivers/usb/gadget/s3c-hsudc.c b/drivers/usb/gadget/s3c-hsudc.c
index 3fa717c..3ac4241 100644
--- a/drivers/usb/gadget/s3c-hsudc.c
+++ b/drivers/usb/gadget/s3c-hsudc.c
@@ -1287,7 +1287,7 @@ static int s3c_hsudc_probe(struct platform_device *pdev)
device_initialize(&hsudc->gadget.dev);
dev_set_name(&hsudc->gadget.dev, "gadget");
- hsudc->gadget.is_dualspeed = 1;
+ hsudc->gadget.max_speed = USB_SPEED_HIGH;
hsudc->gadget.ops = &s3c_hsudc_gadget_ops;
hsudc->gadget.name = dev_name(dev);
hsudc->gadget.dev.parent = dev;
diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index e1ecdbc..25058b4 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
}
static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
-static ssize_t usb_udc_speed_show(struct device *dev,
+#define USB_UDC_SPEED_ATTR(name) \
+ssize_t usb_udc_##name##_show(struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
+ return snprintf(buf, PAGE_SIZE, "%s\n", \
+ usb_device_speed_name(udc->gadget->name)); \
+} \
+static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
+
+static USB_UDC_SPEED_ATTR(speed);
+static USB_UDC_SPEED_ATTR(max_speed);
+
+/* Provide "is_dualspeed" for backward compatibility. */
+static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
- return snprintf(buf, PAGE_SIZE, "%s\n",
- usb_device_speed_name(udc->gadget->speed));
+ struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
+ return snprintf(buf, PAGE_SIZE, "%d\n",
+ gadget_is_dualspeed(udc->gadget));
}
-static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
+static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show, NULL);
#define USB_UDC_ATTR(name) \
ssize_t usb_udc_##name##_show(struct device *dev, \
@@ -391,7 +405,6 @@ ssize_t usb_udc_##name##_show(struct device *dev, \
} \
static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
-static USB_UDC_ATTR(is_dualspeed);
static USB_UDC_ATTR(is_otg);
static USB_UDC_ATTR(is_a_peripheral);
static USB_UDC_ATTR(b_hnp_enable);
@@ -402,6 +415,7 @@ static struct attribute *usb_udc_attrs[] = {
&dev_attr_srp.attr,
&dev_attr_soft_connect.attr,
&dev_attr_speed.attr,
+ &dev_attr_max_speed.attr,
&dev_attr_is_dualspeed.attr,
&dev_attr_is_otg.attr,
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 8c41a2e..0b3fb20 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1827,7 +1827,7 @@ int __init musb_gadget_setup(struct musb *musb)
*/
musb->g.ops = &musb_gadget_operations;
- musb->g.is_dualspeed = 1;
+ musb->g.max_speed = USB_SPEED_HIGH;
musb->g.speed = USB_SPEED_UNKNOWN;
/* this "gadget" abstracts/virtualizes the controller */
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index cb2d451..a8ae54f 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -917,7 +917,7 @@ int __devinit usbhs_mod_gadget_probe(struct usbhs_priv *priv)
gpriv->gadget.dev.parent = dev;
gpriv->gadget.name = "renesas_usbhs_udc";
gpriv->gadget.ops = &usbhsg_gadget_ops;
- gpriv->gadget.is_dualspeed = 1;
+ gpriv->gadget.max_speed = USB_SPEED_HIGH;
INIT_LIST_HEAD(&gpriv->gadget.ep_list);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 087f4b9..7eb2ec3 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -477,8 +477,8 @@ struct usb_gadget_ops {
* driver setup() requests
* @ep_list: List of other endpoints supported by the device.
* @speed: Speed of current connection to USB host.
- * @is_dualspeed: True if the controller supports both high and full speed
- * operation. If it does, the gadget driver must also support both.
+ * @max_speed: Maximal speed the UDC can handle. UDC must support this
+ * and all slower speeds.
* @is_otg: True if the USB device port uses a Mini-AB jack, so that the
* gadget driver must provide a USB OTG descriptor.
* @is_a_peripheral: False unless is_otg, the "A" end of a USB cable
@@ -518,7 +518,7 @@ struct usb_gadget {
struct usb_ep *ep0;
struct list_head ep_list; /* of usb_ep */
enum usb_device_speed speed;
- unsigned is_dualspeed:1;
+ enum usb_device_speed max_speed;
unsigned is_otg:1;
unsigned is_a_peripheral:1;
unsigned b_hnp_enable:1;
@@ -549,7 +549,7 @@ static inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
static inline int gadget_is_dualspeed(struct usb_gadget *g)
{
#ifdef CONFIG_USB_GADGET_DUALSPEED
- /* runtime test would check "g->is_dualspeed" ... that might be
+ /* runtime test would check "g->max_speed" ... that might be
* useful to work around hardware bugs, but is mostly pointless
*/
return 1;
@@ -567,7 +567,7 @@ static inline int gadget_is_superspeed(struct usb_gadget *g)
{
#ifdef CONFIG_USB_GADGET_SUPERSPEED
/*
- * runtime test would check "g->is_superspeed" ... that might be
+ * runtime test would check "g->max_speed" ... that might be
* useful to work around hardware bugs, but is mostly pointless
*/
return 1;
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-19 22:33 ` [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed" Michal Nazarewicz
@ 2011-08-19 23:28 ` Felipe Balbi
2011-08-23 13:48 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-19 23:28 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]
Hi,
On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> This commit replaces usb_gadget's is_dualspeed field with
> a max_speed field.
>
> This change is made so that one will be able to check at
> run-time if given gadget supports super speed.
IMHO the logic is inverted here. It should start from the function
drivers. They should say which USB speeds they support, that would go up
to composite layer and composite would call e.g.
usb_gadget_set_speed(gadget, maximum_speed);
that function will set gadget->maximum_speed and call into the UDC
driver which would write maximum_speed into its internal registers, thus
preventing the core from working at speeds unsupported by the gadget
driver.
All of this should happen before we even connect to the host, so first
all UDCs should have their conversion to udc-core finished and start
using udc_start()/udc_stop(). The driver I just sent [1] illustrates
what udc_start()/udc_stop() should do.
[1] http://marc.info/?l=linux-usb&m=131376669022473&w=2
<snip>
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index e1ecdbc..25058b4 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
> }
> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
>
> -static ssize_t usb_udc_speed_show(struct device *dev,
> +#define USB_UDC_SPEED_ATTR(name) \
> +ssize_t usb_udc_##name##_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
> + return snprintf(buf, PAGE_SIZE, "%s\n", \
> + usb_device_speed_name(udc->gadget->name)); \
> +} \
> +static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
> +
> +static USB_UDC_SPEED_ATTR(speed);
how about "current_speed" ?
> +static USB_UDC_SPEED_ATTR(max_speed);
in this case, humans will be reading sysfs, so maximum_speed will look
nicer, IMHO.
> +/* Provide "is_dualspeed" for backward compatibility. */
> +static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> - return snprintf(buf, PAGE_SIZE, "%s\n",
> - usb_device_speed_name(udc->gadget->speed));
> + struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + gadget_is_dualspeed(udc->gadget));
> }
> -static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
> +static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show, NULL);
maybe deprecate this one on feature-removal-schedule ??
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-19 23:28 ` Felipe Balbi
@ 2011-08-23 13:48 ` Michal Nazarewicz
2011-08-23 13:58 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 13:48 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
Hi,
Sorry, I somehow missed this mail before.
> On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
>> This commit replaces usb_gadget's is_dualspeed field with
>> a max_speed field.
>>
>> This change is made so that one will be able to check at
>> run-time if given gadget supports super speed.
On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> IMHO the logic is inverted here. It should start from the function
> drivers. They should say which USB speeds they support, that would go up
> to composite layer and composite would call e.g.
> usb_gadget_set_speed(gadget, maximum_speed);
This is actually not how composite works at the moment. Currently,
a composite gadget can declare a maximum speed of say “high” even if
all the functions do not support that speed. Of course when host asks
about descriptors for given speed, only functions that support that
speed will be returned (and hence only configurations that have at
least one function supporting that speed).
Whether the behaviour should be changed is, in my opinion, issue separate
from the patchset that I'm sending.
>> diff --git a/drivers/usb/gadget/udc-core.c
>> b/drivers/usb/gadget/udc-core.c
>> index e1ecdbc..25058b4 100644
>> --- a/drivers/usb/gadget/udc-core.c
>> +++ b/drivers/usb/gadget/udc-core.c
>> @@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct
>> device *dev,
>> }
>> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,
>> usb_udc_softconn_store);
>>
>> -static ssize_t usb_udc_speed_show(struct device *dev,
>> +#define USB_UDC_SPEED_ATTR(name) \
>> +ssize_t usb_udc_##name##_show(struct device *dev, \
>> + struct device_attribute *attr, char *buf) \
>> +{ \
>> + struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
>> + return snprintf(buf, PAGE_SIZE, "%s\n", \
>> + usb_device_speed_name(udc->gadget->name)); \
>> +} \
>> +static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
>> +
>> +static USB_UDC_SPEED_ATTR(speed);
>
> how about "current_speed" ?
Is there a big advantage? That would change external interface and I don't
see reason to do so. Of course, udc class is quite recent so if you feel
we can
ignore this issue I can go forward with that change.
>> +static USB_UDC_SPEED_ATTR(max_speed);
>
> in this case, humans will be reading sysfs, so maximum_speed will look
> nicer, IMHO.
I don't care much. Will change.
>> +/* Provide "is_dualspeed" for backward compatibility. */
>> +static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> - struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
>> - return snprintf(buf, PAGE_SIZE, "%s\n",
>> - usb_device_speed_name(udc->gadget->speed));
>> + struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
>> + return snprintf(buf, PAGE_SIZE, "%d\n",
>> + gadget_is_dualspeed(udc->gadget));
>> }
>> -static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
>> +static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show,
>> NULL);
>
> maybe deprecate this one on feature-removal-schedule ??
Sure.
Also, if we decide to change “speed” to “current_speed”, we could just drop
this without the announcement.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 13:48 ` Michal Nazarewicz
@ 2011-08-23 13:58 ` Felipe Balbi
2011-08-23 14:15 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 13:58 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Alan Stern, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4763 bytes --]
Hi,
On Tue, Aug 23, 2011 at 03:48:30PM +0200, Michal Nazarewicz wrote:
> Sorry, I somehow missed this mail before.
np
> >On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
> >>This commit replaces usb_gadget's is_dualspeed field with
> >>a max_speed field.
> >>
> >>This change is made so that one will be able to check at
> >>run-time if given gadget supports super speed.
>
> On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >IMHO the logic is inverted here. It should start from the function
> >drivers. They should say which USB speeds they support, that would go up
> >to composite layer and composite would call e.g.
> >usb_gadget_set_speed(gadget, maximum_speed);
>
> This is actually not how composite works at the moment. Currently,
my suggestion was exactly to change that :-) Speed is something
functions support. composite.c shouldn't dictate which speed functions
should use, rather composite.c should use the maximum speed which all
functions support.
> a composite gadget can declare a maximum speed of say “high” even if
> all the functions do not support that speed. Of course when host asks
> about descriptors for given speed, only functions that support that
> speed will be returned (and hence only configurations that have at
> least one function supporting that speed).
>
> Whether the behaviour should be changed is, in my opinion, issue
> separate from the patchset that I'm sending.
I wouldn't say that, actually. Just replacing is_dualspeed with
max_speed isn't changing much and if you want to make that part of the
code better, why not doing The Right Thing(TM) ?
All of the speed negotiation between composite.c and f_*.c should happen
before even connecting to host (before attaching data pullups, enabling
IRQs, etc), that's exactly why me and Sebastian have decided (at that
time off list) to add udc_start()/udc_stop() methods. One of the reason
was that it would be a quite intrusive change to all UDC drivers, second
we wanted to give maintainers/authors of those UDC drivers some grace
period for the change, third when all UDCs are converted, it allow us
to do the speed negotiation before connecting to host.
> >>diff --git a/drivers/usb/gadget/udc-core.c
> >>b/drivers/usb/gadget/udc-core.c
> >>index e1ecdbc..25058b4 100644
> >>--- a/drivers/usb/gadget/udc-core.c
> >>+++ b/drivers/usb/gadget/udc-core.c
> >>@@ -371,14 +371,28 @@ static ssize_t
> >>usb_udc_softconn_store(struct device *dev,
> >> }
> >> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,
> >>usb_udc_softconn_store);
> >>
> >>-static ssize_t usb_udc_speed_show(struct device *dev,
> >>+#define USB_UDC_SPEED_ATTR(name) \
> >>+ssize_t usb_udc_##name##_show(struct device *dev, \
> >>+ struct device_attribute *attr, char *buf) \
> >>+{ \
> >>+ struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
> >>+ return snprintf(buf, PAGE_SIZE, "%s\n", \
> >>+ usb_device_speed_name(udc->gadget->name)); \
> >>+} \
> >>+static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
> >>+
> >>+static USB_UDC_SPEED_ATTR(speed);
> >
> >how about "current_speed" ?
>
> Is there a big advantage? That would change external interface and I don't
> see reason to do so. Of course, udc class is quite recent so if you
> feel we can
> ignore this issue I can go forward with that change.
you already maximum_speed (below) and speed alone looses some extra
hint of what kind of information will be there. I think it's better to
change this to current_speed and make a symbolic link called 'speed'
which we can keep for the next 5 years and remove it in e.g. Linux v5.0
> >>+/* Provide "is_dualspeed" for backward compatibility. */
> >>+static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
> >> struct device_attribute *attr, char *buf)
> >> {
> >>- struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> >>- return snprintf(buf, PAGE_SIZE, "%s\n",
> >>- usb_device_speed_name(udc->gadget->speed));
> >>+ struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> >>+ return snprintf(buf, PAGE_SIZE, "%d\n",
> >>+ gadget_is_dualspeed(udc->gadget));
> >> }
> >>-static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
> >>+static DEVICE_ATTR(is_dualspeed, S_IRUSR,
> >>usb_udc_is_dualspeed_show, NULL);
> >
> >maybe deprecate this one on feature-removal-schedule ??
>
> Sure.
>
> Also, if we decide to change “speed” to “current_speed”, we could just drop
> this without the announcement.
I rather have the announcement. See above that you can make a symbolic
link with the name 'speed' for the time being.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 13:58 ` Felipe Balbi
@ 2011-08-23 14:15 ` Michal Nazarewicz
2011-08-23 14:37 ` Alan Stern
2011-08-23 15:05 ` Felipe Balbi
0 siblings, 2 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 14:15 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
On Tue, 23 Aug 2011 15:58:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
>> On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
>>> IMHO the logic is inverted here. It should start from the function
>>> drivers. They should say which USB speeds they support, that would go
>>> up to composite layer and composite would call e.g.
>>> usb_gadget_set_speed(gadget, maximum_speed);
>>
>> This is actually not how composite works at the moment. Currently,
>
> my suggestion was exactly to change that :-) Speed is something
> functions support. composite.c shouldn't dictate which speed functions
> should use, rather composite.c should use the maximum speed which all
> functions support.
Strictly speaking, composite.c does not dictate anything. It just copies
what the composite gadget driver declared as maximum speed.
My understanding was that one could consciously create a composite gadget
such that not all of the functions support all of the speeds.
I would suggest leaving everything as is, expect if usb_composite_driver's
max_speed is set to USB_SPEED_UNKNOWN in which case composite would iterate
over all the functions and figure out the maximum speed that all of the
functions support.
>> a composite gadget can declare a maximum speed of say “high” even if
>> all the functions do not support that speed. Of course when host asks
>> about descriptors for given speed, only functions that support that
>> speed will be returned (and hence only configurations that have at
>> least one function supporting that speed).
>>
>> Whether the behaviour should be changed is, in my opinion, issue
>> separate from the patchset that I'm sending.
>
> I wouldn't say that, actually. Just replacing is_dualspeed with
> max_speed isn't changing much and if you want to make that part of the
> code better, why not doing The Right Thing(TM) ?
I'm just saying that the main reason for this patchset is to get rid of
the USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. So
the purpose of changing is_dualspeed with max_speed is to be able to
check if gadget is super speed at run-time so that gadget_is_superspeed()
can be implemented.
So I would like to just get this done and then figure out what to do with
composite.c. How does that sound?
> All of the speed negotiation between composite.c and f_*.c should happen
> before even connecting to host
Yep, obviously. The usb_gadget_probe_driver() is called at the very and
once all the functions and everything is added so composite.c can do all
the analysis it wants and figure out the maximum speed.
> (before attaching data pullups, enabling IRQs, etc), that's exactly why
> me and Sebastian have decided (at that time off list) to add
> udc_start()/udc_stop() methods.
I don't really follow why those would be needed...
> One of the reason was that it would be a quite intrusive change to
> all UDC drivers, second we wanted to give maintainers/authors of
> those UDC drivers some grace period for the change, third when
> all UDCs are converted, it allow us to do the speed negotiation
> before connecting to host.
Again, I don't follow. We can figure out the max_speed before calling
usb_gadget_probe_driver() just fine. We don't even have to have UDC
to figure that out (ie. gadget driver's max_speed does not change
depending on UDC, right?).
>>> how about "current_speed" ?
>>
>> Is there a big advantage? That would change external interface and I
>> don't see reason to do so. Of course, udc class is quite recent so if
>> you feel we can ignore this issue I can go forward with that change.
> you already maximum_speed (below) and speed alone looses some extra
> hint of what kind of information will be there. I think it's better to
> change this to current_speed and make a symbolic link called 'speed'
> which we can keep for the next 5 years and remove it in e.g. Linux v5.0
OK, I'll do that (as soon as I figure out/recall how to make symlinks that
is ;) ).
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:15 ` Michal Nazarewicz
@ 2011-08-23 14:37 ` Alan Stern
2011-08-23 14:58 ` Felipe Balbi
2011-08-23 15:05 ` Felipe Balbi
1 sibling, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-23 14:37 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
> My understanding was that one could consciously create a composite gadget
> such that not all of the functions support all of the speeds.
That's an important point. If somebody wants to build a composite
gadget that includes both midi and storage support, the storage driver
would be able to go all the way up to SuperSpeed (or rather, it will
once Felipe's patch is finished and merged) but the midi driver would
only support full speed. So what speed should the UDC connect at?
In cases like this there's really no way to know. Ultimately the best
approach would be to update the midi driver, but that's a long-term
sort of thing.
Usually when the kernel can't decide something, it asks the user for
help (by allowing the user to specify the speed via a module parameter,
for instance). In this case even that wouldn't work, because the
gadget's userspace isn't aware of which functions the host's user will
want to utilize.
At the moment I don't know the best answer. This is something we
should all think about.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:37 ` Alan Stern
@ 2011-08-23 14:58 ` Felipe Balbi
2011-08-23 15:07 ` Michal Nazarewicz
2011-08-23 15:43 ` Alan Stern
0 siblings, 2 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 14:58 UTC (permalink / raw)
To: Alan Stern
Cc: Michal Nazarewicz, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]
Hi,
On Tue, Aug 23, 2011 at 10:37:23AM -0400, Alan Stern wrote:
> On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
>
> > My understanding was that one could consciously create a composite gadget
> > such that not all of the functions support all of the speeds.
>
> That's an important point. If somebody wants to build a composite
> gadget that includes both midi and storage support, the storage driver
> would be able to go all the way up to SuperSpeed (or rather, it will
> once Felipe's patch is finished and merged) but the midi driver would
> only support full speed. So what speed should the UDC connect at?
if they are both part of the of the same configuration, full speed.
Otherwise Audio won't work. If they are part of different
configurations, then maximum_speed will be defined by the chosen config,
no ?
> In cases like this there's really no way to know. Ultimately the best
> approach would be to update the midi driver, but that's a long-term
> sort of thing.
yeah, I agree here.
> Usually when the kernel can't decide something, it asks the user for
> help (by allowing the user to specify the speed via a module parameter,
> for instance). In this case even that wouldn't work, because the
> gadget's userspace isn't aware of which functions the host's user will
> want to utilize.
>
> At the moment I don't know the best answer. This is something we
> should all think about.
I stand with the fact that if they are part of the same configuration,
there's no way around it, storage will have to come down to full speed.
Unless I'm missing something.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:58 ` Felipe Balbi
@ 2011-08-23 15:07 ` Michal Nazarewicz
2011-08-23 15:11 ` Felipe Balbi
2011-08-23 15:43 ` Alan Stern
1 sibling, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 15:07 UTC (permalink / raw)
To: Alan Stern, Felipe Balbi
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young, linux-usb,
linux-kernel
On Tue, 23 Aug 2011 16:58:01 +0200, Felipe Balbi <balbi@ti.com> wrote:
> I stand with the fact that if they are part of the same configuration,
> there's no way around it, storage will have to come down to full speed.
> Unless I'm missing something.
If you look at config_buf() in composite.c, you'll notice that composite
just skips functions that do not support current speed. This means, that
if a connection uses high speed but given function can handle only full
speed, it won't be included in the descriptors.
config_buf() is called from config_desc() which similarly, iterates over
configurations skipping the ones that do not support given speed.
Configuration is defined to support given speed if at least one function
that is added to that configuration supports that speed (see
usb_add_function()).
This also allows creating functions that support high speed but do not
support full speed (which I'm unsure whether it makes sense).
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:07 ` Michal Nazarewicz
@ 2011-08-23 15:11 ` Felipe Balbi
2011-08-23 15:26 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 15:11 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]
Hi,
On Tue, Aug 23, 2011 at 05:07:11PM +0200, Michal Nazarewicz wrote:
> On Tue, 23 Aug 2011 16:58:01 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >I stand with the fact that if they are part of the same configuration,
> >there's no way around it, storage will have to come down to full speed.
> >Unless I'm missing something.
>
> If you look at config_buf() in composite.c, you'll notice that composite
> just skips functions that do not support current speed. This means, that
> if a connection uses high speed but given function can handle only full
> speed, it won't be included in the descriptors.
>
> config_buf() is called from config_desc() which similarly, iterates over
> configurations skipping the ones that do not support given speed.
>
> Configuration is defined to support given speed if at least one function
> that is added to that configuration supports that speed (see
> usb_add_function()).
I'm trying to think if this is really what we want. This might mean
$current_config will be different depending if we connect it to SS, HS
or FS hubs. Do we need USB-IF's help here ? I guess I could drop them a
mail and ask for some clarification. Don't know how long it would take
to get an answer though :-s
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:11 ` Felipe Balbi
@ 2011-08-23 15:26 ` Michal Nazarewicz
2011-08-23 17:19 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 15:26 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
> On Tue, Aug 23, 2011 at 05:07:11PM +0200, Michal Nazarewicz wrote:
>> If you look at config_buf() in composite.c, you'll notice that composite
>> just skips functions that do not support current speed. This means,
>> that if a connection uses high speed but given function can handle only
>> full speed, it won't be included in the descriptors.
>>
>> config_buf() is called from config_desc() which similarly, iterates over
>> configurations skipping the ones that do not support given speed.
>>
>> Configuration is defined to support given speed if at least one function
>> that is added to that configuration supports that speed (see
>> usb_add_function()).
On Tue, 23 Aug 2011 17:11:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> I'm trying to think if this is really what we want. This might mean
> $current_config will be different depending if we connect it to SS, HS
> or FS hubs.
What do you mean by $current_config? The bConfigurationValue will be
the same for "the same" configuration regardless of speed but different
configurations may be available for different speeds and "the same"
configuration in various speeds might have different functions.
So just for the sake of having something concrete, let's say we have:
Configuration #1 with
function A supporting FS and HS and
function B supporting FS only.
Configuration #2 with
function B supporting FS only.
Configuration #3 with
function A supporting FS and HS and
function C supporting HS only (dunno if that makes sense).
When connected to FS hub, we will return the following:
Configuration #1: A, B
Configuration #2: B
Configuration #3: A
When connect to HS hub, we will return the following:
Configuration #1: A
Configuration #3: A, C
And similarly to bConfigurationValue, the interface IDs may have holes in
them.
Whether that's what we want, I don't know... But if I understand
composite.c correctly, that's how it is implemented now.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:26 ` Michal Nazarewicz
@ 2011-08-23 17:19 ` Felipe Balbi
2011-08-23 18:44 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 17:19 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Alan Stern, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2213 bytes --]
Hi,
On Tue, Aug 23, 2011 at 05:26:24PM +0200, Michal Nazarewicz wrote:
> >On Tue, Aug 23, 2011 at 05:07:11PM +0200, Michal Nazarewicz wrote:
> >>If you look at config_buf() in composite.c, you'll notice that composite
> >>just skips functions that do not support current speed. This
> >>means, that if a connection uses high speed but given function
> >>can handle only
> >>full speed, it won't be included in the descriptors.
> >>
> >>config_buf() is called from config_desc() which similarly, iterates over
> >>configurations skipping the ones that do not support given speed.
> >>
> >>Configuration is defined to support given speed if at least one function
> >>that is added to that configuration supports that speed (see
> >>usb_add_function()).
>
> On Tue, 23 Aug 2011 17:11:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >I'm trying to think if this is really what we want. This might mean
> >$current_config will be different depending if we connect it to SS, HS
> >or FS hubs.
>
> What do you mean by $current_config? The bConfigurationValue will be
the currently chosen configuration.
> the same for "the same" configuration regardless of speed but different
> configurations may be available for different speeds and "the same"
> configuration in various speeds might have different functions.
>
> So just for the sake of having something concrete, let's say we have:
>
> Configuration #1 with
> function A supporting FS and HS and
my question is what happens with the HS support on function A
(descriptors, endpoints, etc) when you connect it to a HS hub or to a FS
hub.
> function B supporting FS only.
> Configuration #2 with
> function B supporting FS only.
> Configuration #3 with
> function A supporting FS and HS and
> function C supporting HS only (dunno if that makes sense).
>
> When connected to FS hub, we will return the following:
>
> Configuration #1: A, B
> Configuration #2: B
> Configuration #3: A
>
> When connect to HS hub, we will return the following:
>
> Configuration #1: A
See, I'm not sure this is allowed by USB spec. But I could be wrong. (I
mean having a function disappear)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 17:19 ` Felipe Balbi
@ 2011-08-23 18:44 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 18:44 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
>> On Tue, 23 Aug 2011 17:11:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
>>> I'm trying to think if this is really what we want. This might mean
>>> $current_config will be different depending if we connect it to SS, HS
>>> or FS hubs.
> On Tue, Aug 23, 2011 at 05:26:24PM +0200, Michal Nazarewicz wrote:
>> What do you mean by $current_config? The bConfigurationValue will be
On Tue, 23 Aug 2011 19:19:43 +0200, Felipe Balbi <balbi@ti.com> wrote:
> the currently chosen configuration.
So yes, it potentially ma be different depending on the speed.
>> the same for "the same" configuration regardless of speed but different
>> configurations may be available for different speeds and "the same"
>> configuration in various speeds might have different functions.
>>
>> So just for the sake of having something concrete, let's say we have:
>>
>> Configuration #1 with
>> function A supporting FS and HS and
> my question is what happens with the HS support on function A
> (descriptors, endpoints, etc) when you connect it to a HS hub or to a FS
> hub.
When you connect it to HS hub you get it's HS descriptors (and FS
descriptors
if you request “other speed descriptors”). When you connect it to FS hub
you
get it's FS descriptors.
>> function B supporting FS only.
>> Configuration #2 with
>> function B supporting FS only.
>> Configuration #3 with
>> function A supporting FS and HS and
>> function C supporting HS only (dunno if that makes sense).
>>
>> When connected to FS hub, we will return the following:
>>
>> Configuration #1: A, B
>> Configuration #2: B
>> Configuration #3: A
>>
>> When connect to HS hub, we will return the following:
>>
>> Configuration #1: A
>
> See, I'm not sure this is allowed by USB spec. But I could be wrong. (I
> mean having a function disappear)
I'm hardly an expert so I cannot address that concern.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:58 ` Felipe Balbi
2011-08-23 15:07 ` Michal Nazarewicz
@ 2011-08-23 15:43 ` Alan Stern
2011-08-23 17:21 ` Felipe Balbi
1 sibling, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-23 15:43 UTC (permalink / raw)
To: Felipe Balbi
Cc: Michal Nazarewicz, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011, Felipe Balbi wrote:
> Hi,
>
> On Tue, Aug 23, 2011 at 10:37:23AM -0400, Alan Stern wrote:
> > On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
> >
> > > My understanding was that one could consciously create a composite gadget
> > > such that not all of the functions support all of the speeds.
> >
> > That's an important point. If somebody wants to build a composite
> > gadget that includes both midi and storage support, the storage driver
> > would be able to go all the way up to SuperSpeed (or rather, it will
> > once Felipe's patch is finished and merged) but the midi driver would
> > only support full speed. So what speed should the UDC connect at?
>
> if they are both part of the of the same configuration, full speed.
> Otherwise Audio won't work. If they are part of different
> configurations, then maximum_speed will be defined by the chosen config,
> no ?
No. The speed has to be chosen first, when the gadget connects to the
host. The config isn't chosen until later, after the host retrieves
the various descriptors and decides among them.
> > In cases like this there's really no way to know. Ultimately the best
> > approach would be to update the midi driver, but that's a long-term
> > sort of thing.
>
> yeah, I agree here.
>
> > Usually when the kernel can't decide something, it asks the user for
> > help (by allowing the user to specify the speed via a module parameter,
> > for instance). In this case even that wouldn't work, because the
> > gadget's userspace isn't aware of which functions the host's user will
> > want to utilize.
> >
> > At the moment I don't know the best answer. This is something we
> > should all think about.
>
> I stand with the fact that if they are part of the same configuration,
> there's no way around it, storage will have to come down to full speed.
> Unless I'm missing something.
What if they are in different configurations?
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:43 ` Alan Stern
@ 2011-08-23 17:21 ` Felipe Balbi
2011-08-23 18:00 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 17:21 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Michal Nazarewicz, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 823 bytes --]
Hi,
On Tue, Aug 23, 2011 at 11:43:44AM -0400, Alan Stern wrote:
> > if they are both part of the of the same configuration, full speed.
> > Otherwise Audio won't work. If they are part of different
> > configurations, then maximum_speed will be defined by the chosen config,
> > no ?
>
> No. The speed has to be chosen first, when the gadget connects to the
> host. The config isn't chosen until later, after the host retrieves
> the various descriptors and decides among them.
true
> > I stand with the fact that if they are part of the same configuration,
> > there's no way around it, storage will have to come down to full speed.
> > Unless I'm missing something.
>
> What if they are in different configurations?
then you use the fastest speed between both configurations?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 17:21 ` Felipe Balbi
@ 2011-08-23 18:00 ` Alan Stern
2011-08-23 19:05 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-23 18:00 UTC (permalink / raw)
To: Felipe Balbi
Cc: Michal Nazarewicz, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011, Felipe Balbi wrote:
> Hi,
>
> On Tue, Aug 23, 2011 at 11:43:44AM -0400, Alan Stern wrote:
> > > if they are both part of the of the same configuration, full speed.
> > > Otherwise Audio won't work. If they are part of different
> > > configurations, then maximum_speed will be defined by the chosen config,
> > > no ?
> >
> > No. The speed has to be chosen first, when the gadget connects to the
> > host. The config isn't chosen until later, after the host retrieves
> > the various descriptors and decides among them.
>
> true
>
> > > I stand with the fact that if they are part of the same configuration,
> > > there's no way around it, storage will have to come down to full speed.
> > > Unless I'm missing something.
> >
> > What if they are in different configurations?
>
> then you use the fastest speed between both configurations?
I simply don't know. It's a difficult problem.
For the time being, perhaps the best answer is to use the _lowest_
max_speed value among all the function drivers. It's the simplest
answer and most likely to be correct most of the time.
If anybody ever does want to put together a composite gadget that runs
faster than full speed but still provides a midi function ... well,
they'll just have to update the midi gadget driver.
(Actually that's a bogus example. It turns out that gmidi.c doesn't
support the composite framework. Oh well.)
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 18:00 ` Alan Stern
@ 2011-08-23 19:05 ` Michal Nazarewicz
2011-08-23 20:49 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 19:05 UTC (permalink / raw)
To: Felipe Balbi, Alan Stern
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young, linux-usb,
linux-kernel
On Tue, 23 Aug 2011 20:00:25 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> For the time being, perhaps the best answer is to use the _lowest_
> max_speed value among all the function drivers. It's the simplest
> answer and most likely to be correct most of the time.
In theory, one could create a composite function with only SS/HS
descriptors. So it would work for SS/HS but not for FS. I'm not
saying that it would make sense (or that it's not a bug) but that
adds to the discussion.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 19:05 ` Michal Nazarewicz
@ 2011-08-23 20:49 ` Alan Stern
2011-08-24 8:56 ` Felipe Balbi
2011-08-24 13:10 ` Michal Nazarewicz
0 siblings, 2 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-23 20:49 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
> On Tue, 23 Aug 2011 20:00:25 +0200, Alan Stern <stern@rowland.harvard.edu>
> wrote:
> > For the time being, perhaps the best answer is to use the _lowest_
> > max_speed value among all the function drivers. It's the simplest
> > answer and most likely to be correct most of the time.
>
> In theory, one could create a composite function with only SS/HS
> descriptors.
The only reason for doing that would be if the function was incapable
of carrying out its duties when running at full speed (insufficient
bandwidth, for example). Something like this is discussed briefly in
section 9.2.6.6 of the USB-2 spec.
> So it would work for SS/HS but not for FS. I'm not
> saying that it would make sense (or that it's not a bug) but that
> adds to the discussion.
If somebody writes a composite gadget containing two functions, one of
which supports only full speed and the other only high speed, they'll
get what they deserve. :-)
Still, maybe I was wrong. Maybe it would be better to use the fastest
speed supported by at least one of the function drivers. The user can
always force a SuperSpeed-capable device to run at high speed by using
a USB-2 cable to plug it in. I'm not sure whether the user could force
such a device to run at full speed, however.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 20:49 ` Alan Stern
@ 2011-08-24 8:56 ` Felipe Balbi
2011-08-24 13:10 ` Michal Nazarewicz
1 sibling, 0 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-24 8:56 UTC (permalink / raw)
To: Alan Stern
Cc: Michal Nazarewicz, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]
Hi,
On Tue, Aug 23, 2011 at 04:49:37PM -0400, Alan Stern wrote:
> On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
>
> > On Tue, 23 Aug 2011 20:00:25 +0200, Alan Stern <stern@rowland.harvard.edu>
> > wrote:
> > > For the time being, perhaps the best answer is to use the _lowest_
> > > max_speed value among all the function drivers. It's the simplest
> > > answer and most likely to be correct most of the time.
> >
> > In theory, one could create a composite function with only SS/HS
> > descriptors.
>
> The only reason for doing that would be if the function was incapable
> of carrying out its duties when running at full speed (insufficient
> bandwidth, for example). Something like this is discussed briefly in
> section 9.2.6.6 of the USB-2 spec.
>
> > So it would work for SS/HS but not for FS. I'm not
> > saying that it would make sense (or that it's not a bug) but that
> > adds to the discussion.
>
> If somebody writes a composite gadget containing two functions, one of
> which supports only full speed and the other only high speed, they'll
> get what they deserve. :-)
>
> Still, maybe I was wrong. Maybe it would be better to use the fastest
> speed supported by at least one of the function drivers. The user can
> always force a SuperSpeed-capable device to run at high speed by using
> a USB-2 cable to plug it in. I'm not sure whether the user could force
> such a device to run at full speed, however.
plug it behind a full speed hub, like those that usually come on USB
keyboards.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 20:49 ` Alan Stern
2011-08-24 8:56 ` Felipe Balbi
@ 2011-08-24 13:10 ` Michal Nazarewicz
2011-08-24 14:31 ` Alan Stern
1 sibling, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-24 13:10 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011 22:49:37 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> Still, maybe I was wrong. Maybe it would be better to use the fastest
> speed supported by at least one of the function drivers. The user can
> always force a SuperSpeed-capable device to run at high speed by using
> a USB-2 cable to plug it in. I'm not sure whether the user could force
> such a device to run at full speed, however.
I'm actually wondering whether we really need to worry about it.
composite_driver has the “max_speed” field and I would just leave it as
composite driver's author responsibility to put correct value there.
One thing that could be worth doing is iterate over all the configurations
and figure out if at least one supports the speed declared by “max_speed”
and if not, lower it.
Like I've said before, as additional functionality, composite.c could
check if it's USB_SPEED_UNKWONW in which case it would look for the lowest
speed that all the functions support.
Such configuration would allow composite driver authors set the speed to
USB_SPEED_SUPER when they mean “choose maximum speed at least one function
supports” and to “USB_SPEED_UNKWONW” when they mean “choose maximum speed
all the functions support”.
Does that make sense?
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 13:10 ` Michal Nazarewicz
@ 2011-08-24 14:31 ` Alan Stern
2011-08-24 14:53 ` Michal Nazarewicz
2011-08-24 22:57 ` Felipe Balbi
0 siblings, 2 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-24 14:31 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
> On Tue, 23 Aug 2011 22:49:37 +0200, Alan Stern <stern@rowland.harvard.edu>
> wrote:
> > Still, maybe I was wrong. Maybe it would be better to use the fastest
> > speed supported by at least one of the function drivers. The user can
> > always force a SuperSpeed-capable device to run at high speed by using
> > a USB-2 cable to plug it in. I'm not sure whether the user could force
> > such a device to run at full speed, however.
>
> I'm actually wondering whether we really need to worry about it.
> composite_driver has the “max_speed” field and I would just leave it as
> composite driver's author responsibility to put correct value there.
>
> One thing that could be worth doing is iterate over all the configurations
> and figure out if at least one supports the speed declared by “max_speed”
> and if not, lower it.
That's the same as what I said: Use the highest speed supported by any
function driver. There really isn't any point in allowing connections
faster than that. And as Felipe pointed out, the user always has the
capability to force the connection to be slower, by using the right
sorts of cables and hubs.
> Like I've said before, as additional functionality, composite.c could
> check if it's USB_SPEED_UNKWONW in which case it would look for the lowest
> speed that all the functions support.
>
> Such configuration would allow composite driver authors set the speed to
> USB_SPEED_SUPER when they mean “choose maximum speed at least one function
> supports” and to “USB_SPEED_UNKWONW” when they mean “choose maximum speed
> all the functions support”.
>
> Does that make sense?
It's reasonable. Just be sure to document it properly; otherwise
nobody will understand or remember it!
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 14:31 ` Alan Stern
@ 2011-08-24 14:53 ` Michal Nazarewicz
2011-08-24 15:15 ` Alan Stern
2011-08-24 22:57 ` Felipe Balbi
1 sibling, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-24 14:53 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
> On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
>> I'm actually wondering whether we really need to worry about it.
>> composite_driver has the “max_speed” field and I would just leave it as
>> composite driver's author responsibility to put correct value there.
>>
>> One thing that could be worth doing is iterate over all the
>> configurations and figure out if at least one supports the speed
>> declared by “max_speed” and if not, lower it.
On Wed, 24 Aug 2011 16:31:27 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> That's the same as what I said: Use the highest speed supported by any
> function driver.
Right.
>> Like I've said before, as additional functionality, composite.c could
>> check if it's USB_SPEED_UNKWONW in which case it would look for the
>> lowest speed that all the functions support.
> It's reasonable. Just be sure to document it properly; otherwise
> nobody will understand or remember it!
I've found where my reasoning was faulty. The usb_gadget_driver's
max_speed is set before all the functions get added so composite.c has
no way to figure those things in advance. That's why we need to relay
on usb_composite_driver's max_speed be set to a proper value.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 14:53 ` Michal Nazarewicz
@ 2011-08-24 15:15 ` Alan Stern
2011-08-24 15:25 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-24 15:15 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
> I've found where my reasoning was faulty. The usb_gadget_driver's
> max_speed is set before all the functions get added so composite.c has
> no way to figure those things in advance. That's why we need to relay
> on usb_composite_driver's max_speed be set to a proper value.
This is what Felipe was complaining about earlier. We shouldn't set
max_speed or allow the UDC to connect to the host until all the
functions have been added.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 15:15 ` Alan Stern
@ 2011-08-24 15:25 ` Michal Nazarewicz
2011-08-24 23:04 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-24 15:25 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Wed, 24 Aug 2011 17:15:29 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
>
>> I've found where my reasoning was faulty. The usb_gadget_driver's
>> max_speed is set before all the functions get added so composite.c has
>> no way to figure those things in advance. That's why we need to relay
>> on usb_composite_driver's max_speed be set to a proper value.
>
> This is what Felipe was complaining about earlier. We shouldn't set
> max_speed or allow the UDC to connect to the host until all the
> functions have been added.
We're not doing the latter. The functions are added in bind callback so
(hopefully) UDC will first run the bind callback and then try connecting
to host.
And the former would be fixed if we allowed gadget driver to set max_speed
in bind callback rather then prior to calling usb_gadget_probe_driver().
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 15:25 ` Michal Nazarewicz
@ 2011-08-24 23:04 ` Felipe Balbi
2011-08-25 12:46 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-24 23:04 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]
Hi,
On Wed, Aug 24, 2011 at 05:25:11PM +0200, Michal Nazarewicz wrote:
> On Wed, 24 Aug 2011 17:15:29 +0200, Alan Stern
> <stern@rowland.harvard.edu> wrote:
>
> >On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
> >
> >>I've found where my reasoning was faulty. The usb_gadget_driver's
> >>max_speed is set before all the functions get added so composite.c has
> >>no way to figure those things in advance. That's why we need to relay
> >>on usb_composite_driver's max_speed be set to a proper value.
> >
> >This is what Felipe was complaining about earlier. We shouldn't set
> >max_speed or allow the UDC to connect to the host until all the
> >functions have been added.
>
> We're not doing the latter. The functions are added in bind callback so
> (hopefully) UDC will first run the bind callback and then try connecting
> to host.
>
> And the former would be fixed if we allowed gadget driver to set max_speed
> in bind callback rather then prior to calling usb_gadget_probe_driver().
there's one catch. As of today, we always start UDCs with data pullups
connected, which means that we could connect to a host even before a
gadget driver is loaded. My point in moving to udc_start/udc_stop is
that the above would be take care of. See udc-core.c:
int usb_gadget_probe_driver(struct usb_gadget_driver *driver,
int (*bind)(struct usb_gadget *))
{
....
if (udc_is_newstyle(udc)) {
ret = bind(udc->gadget);
if (ret)
goto err1;
ret = usb_gadget_udc_start(udc->gadget, driver);
if (ret) {
driver->unbind(udc->gadget);
goto err1;
}
usb_gadget_connect(udc->gadget);
} else {
ret = usb_gadget_start(udc->gadget, driver, bind);
if (ret)
goto err1;
}
...
}
If all UDCs are converted to udc_start()/udc_stop() we get the guarantee
that they will only conect to host after gadget driver is fully loaded
for free. We can also, finally, properly use the
usb_function_deactivate/usb_function_activate properly. So for each
registered function, composite.c calls usb_function_deactivate() and
function is _required_ to call usb_function_activate when it's ready
(for f_obex.c that would be when userland OBEX stack has opened
/dev/ttyGS*, for g_mass_storage that would be on function bind(), and so
on).
Then, when on gadget driver's bind() we can take this kind of spee
decision and pass that on to UDC driver.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 23:04 ` Felipe Balbi
@ 2011-08-25 12:46 ` Michal Nazarewicz
2011-08-25 12:53 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-25 12:46 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
On Thu, 25 Aug 2011 01:04:19 +0200, Felipe Balbi <balbi@ti.com> wrote:
> there's one catch. As of today, we always start UDCs with data pullups
> connected, which means that we could connect to a host even before a
> gadget driver is loaded. My point in moving to udc_start/udc_stop is
> that the above would be take care of. See udc-core.c: [...]
Honestly I'm not quite sure why udc_start/udc_stop is needed here. Even
without those the UDC driver can start with all hw disabled and turn it
on only after the gadget driver's bind callback finishes.
> If all UDCs are converted to udc_start()/udc_stop() we get the guarantee
> that they will only conect to host after gadget driver is fully loaded
> for free.
> We can also, finally, properly use the usb_function_deactivate/
> usb_function_activate properly. So for each registered function,
> composite.c calls usb_function_deactivate() and function is _required_
> to call usb_function_activate when it's ready.
I'm not really sure why that would be beneficial. Also, it would still
require disconnect-connect cycle if some function decides to (de)activate
itself while gadget is connected.
> Then, when on gadget driver's bind() we can take this kind of speed
> decision and pass that on to UDC driver.
So can we leave things as they are for now and wait for UDCs to be
converted
and once this is done, do all kinds of magic we want in copomiset's bind
callback?
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-25 12:46 ` Michal Nazarewicz
@ 2011-08-25 12:53 ` Felipe Balbi
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-25 12:53 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Alan Stern, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]
Hi,
On Thu, Aug 25, 2011 at 02:46:55PM +0200, Michal Nazarewicz wrote:
> On Thu, 25 Aug 2011 01:04:19 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >there's one catch. As of today, we always start UDCs with data pullups
> >connected, which means that we could connect to a host even before a
> >gadget driver is loaded. My point in moving to udc_start/udc_stop is
> >that the above would be take care of. See udc-core.c: [...]
>
> Honestly I'm not quite sure why udc_start/udc_stop is needed here. Even
> without those the UDC driver can start with all hw disabled and turn it
> on only after the gadget driver's bind callback finishes.
>
> >If all UDCs are converted to udc_start()/udc_stop() we get the guarantee
> >that they will only conect to host after gadget driver is fully loaded
> >for free.
>
> >We can also, finally, properly use the usb_function_deactivate/
> >usb_function_activate properly. So for each registered function,
> >composite.c calls usb_function_deactivate() and function is _required_
> >to call usb_function_activate when it's ready.
>
> I'm not really sure why that would be beneficial. Also, it would still
> require disconnect-connect cycle if some function decides to (de)activate
> itself while gadget is connected.
>
> >Then, when on gadget driver's bind() we can take this kind of speed
> >decision and pass that on to UDC driver.
>
> So can we leave things as they are for now and wait for UDCs to be
> converted
> and once this is done, do all kinds of magic we want in copomiset's bind
> callback?
sure.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 14:31 ` Alan Stern
2011-08-24 14:53 ` Michal Nazarewicz
@ 2011-08-24 22:57 ` Felipe Balbi
1 sibling, 0 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-24 22:57 UTC (permalink / raw)
To: Alan Stern
Cc: Michal Nazarewicz, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]
On Wed, Aug 24, 2011 at 10:31:27AM -0400, Alan Stern wrote:
> On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
>
> > On Tue, 23 Aug 2011 22:49:37 +0200, Alan Stern <stern@rowland.harvard.edu>
> > wrote:
> > > Still, maybe I was wrong. Maybe it would be better to use the fastest
> > > speed supported by at least one of the function drivers. The user can
> > > always force a SuperSpeed-capable device to run at high speed by using
> > > a USB-2 cable to plug it in. I'm not sure whether the user could force
> > > such a device to run at full speed, however.
> >
> > I'm actually wondering whether we really need to worry about it.
> > composite_driver has the “max_speed” field and I would just leave it as
> > composite driver's author responsibility to put correct value there.
> >
> > One thing that could be worth doing is iterate over all the configurations
> > and figure out if at least one supports the speed declared by “max_speed”
> > and if not, lower it.
>
> That's the same as what I said: Use the highest speed supported by any
> function driver. There really isn't any point in allowing connections
> faster than that. And as Felipe pointed out, the user always has the
> capability to force the connection to be slower, by using the right
> sorts of cables and hubs.
>
> > Like I've said before, as additional functionality, composite.c could
> > check if it's USB_SPEED_UNKWONW in which case it would look for the lowest
> > speed that all the functions support.
> >
> > Such configuration would allow composite driver authors set the speed to
> > USB_SPEED_SUPER when they mean “choose maximum speed at least one function
> > supports” and to “USB_SPEED_UNKWONW” when they mean “choose maximum speed
> > all the functions support”.
> >
> > Does that make sense?
>
> It's reasonable. Just be sure to document it properly; otherwise
> nobody will understand or remember it!
good point. I agree with this approach.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:15 ` Michal Nazarewicz
2011-08-23 14:37 ` Alan Stern
@ 2011-08-23 15:05 ` Felipe Balbi
2011-08-23 15:30 ` Michal Nazarewicz
1 sibling, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 15:05 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Alan Stern, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5235 bytes --]
Hi,
On Tue, Aug 23, 2011 at 04:15:08PM +0200, Michal Nazarewicz wrote:
> On Tue, 23 Aug 2011 15:58:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
>
> >>On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >>>IMHO the logic is inverted here. It should start from the function
> >>>drivers. They should say which USB speeds they support, that
> >>>would go up to composite layer and composite would call e.g.
> >>>usb_gadget_set_speed(gadget, maximum_speed);
> >>
> >>This is actually not how composite works at the moment. Currently,
> >
> >my suggestion was exactly to change that :-) Speed is something
> >functions support. composite.c shouldn't dictate which speed functions
> >should use, rather composite.c should use the maximum speed which all
> >functions support.
>
> Strictly speaking, composite.c does not dictate anything. It just copies
> what the composite gadget driver declared as maximum speed.
true.
> My understanding was that one could consciously create a composite gadget
> such that not all of the functions support all of the speeds.
of course, but if he puts FS function and SS function on the same
configuration, SS function will have to come down to FS, no ?
> >>a composite gadget can declare a maximum speed of say “high” even if
> >>all the functions do not support that speed. Of course when host asks
> >>about descriptors for given speed, only functions that support that
> >>speed will be returned (and hence only configurations that have at
> >>least one function supporting that speed).
> >>
> >>Whether the behaviour should be changed is, in my opinion, issue
> >>separate from the patchset that I'm sending.
> >
> >I wouldn't say that, actually. Just replacing is_dualspeed with
> >max_speed isn't changing much and if you want to make that part of the
> >code better, why not doing The Right Thing(TM) ?
>
> I'm just saying that the main reason for this patchset is to get rid of
> the USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. So
yes, but for removing USB_GADGET_DUALSPEED there's no requirement, per
se, to remove is_dualspeed, am I right ? I could be missing something.
> the purpose of changing is_dualspeed with max_speed is to be able to
> check if gadget is super speed at run-time so that gadget_is_superspeed()
> can be implemented.
I understand. It does sound better than adding is_superspeed flag.
> So I would like to just get this done and then figure out what to do with
> composite.c. How does that sound?
When you put it that way, I guess I must agree ;-)
> >All of the speed negotiation between composite.c and f_*.c should happen
> >before even connecting to host
>
> Yep, obviously. The usb_gadget_probe_driver() is called at the very and
> once all the functions and everything is added so composite.c can do all
> the analysis it wants and figure out the maximum speed.
>
> >(before attaching data pullups, enabling IRQs, etc), that's exactly why
> >me and Sebastian have decided (at that time off list) to add
> >udc_start()/udc_stop() methods.
>
> I don't really follow why those would be needed...
Ok, I guess I need to give the full picture here, my bad.
Let's say you have a SuperSpeed controller, but you know that this
particular gadget driver can only support fullspeed, so why do you need
to go through RX detection, HS chirp sequence and whatnot if you can
decide the maximum_speed before kickstarting the UDC's state machine ?
most controllers (or at least the ones I have seen) have ways to set the
maximum_speed we are going to support. As of today, we always, blindly,
set the maximum supported by the controller, but that can change
overtime.
This might (and I never tested) mean we would be saving a few uA in the
sense that unused HW blocks wouldn't even be powered up (at the least
the more recent cores have quite a fine grain control over what powers
up and what doesn't).
> >One of the reason was that it would be a quite intrusive change to
> >all UDC drivers, second we wanted to give maintainers/authors of
> >those UDC drivers some grace period for the change, third when
> >all UDCs are converted, it allow us to do the speed negotiation
> >before connecting to host.
>
> Again, I don't follow. We can figure out the max_speed before calling
> usb_gadget_probe_driver() just fine. We don't even have to have UDC
> to figure that out (ie. gadget driver's max_speed does not change
> depending on UDC, right?).
it doesn't change depending on UDC, but UDC can take certain decisions
depending on the maximum_speed current gadget driver supports ;-)
> >you already maximum_speed (below) and speed alone looses some extra
> >hint of what kind of information will be there. I think it's better to
> >change this to current_speed and make a symbolic link called 'speed'
> >which we can keep for the next 5 years and remove it in e.g. Linux v5.0
>
> OK, I'll do that (as soon as I figure out/recall how to make symlinks that
> is ;) ).
yeah, I would have to go through the same re-education ;-)
(please add a note on feature-removal-schedule too)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:05 ` Felipe Balbi
@ 2011-08-23 15:30 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 15:30 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
>> On Tue, 23 Aug 2011 15:58:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
>>> All of the speed negotiation between composite.c and f_*.c should
>>> happen before even connecting to host
> On Tue, Aug 23, 2011 at 04:15:08PM +0200, Michal Nazarewicz wrote:
>> Yep, obviously. The usb_gadget_probe_driver() is called at the very and
>> once all the functions and everything is added so composite.c can do all
>> the analysis it wants and figure out the maximum speed.
>>
>> >(before attaching data pullups, enabling IRQs, etc), that's exactly why
>> >me and Sebastian have decided (at that time off list) to add
>> >udc_start()/udc_stop() methods.
>>
>> I don't really follow why those would be needed...
On Tue, 23 Aug 2011 17:05:48 +0200, Felipe Balbi <balbi@ti.com> wrote:
> Ok, I guess I need to give the full picture here, my bad.
>
> Let's say you have a SuperSpeed controller, but you know that this
> particular gadget driver can only support fullspeed, so why do you need
> to go through RX detection, HS chirp sequence and whatnot if you can
> decide the maximum_speed before kickstarting the UDC's state machine ?
But isn't that what's happening right now? The gadget_driver structure
has a speed field which is set to the maximum speed the gadget driver
can handle. Only after this is set, usb_gadget_probe_driver() is
called so at this point a SS UDC can figure out whether it needs to
turn pieces needed for SS support or not.
>>> you already maximum_speed (below) and speed alone looses some extra
>>> hint of what kind of information will be there. I think it's better to
>>> change this to current_speed and make a symbolic link called 'speed'
>>> which we can keep for the next 5 years and remove it in e.g. Linux v5.0
>>
>> OK, I'll do that (as soon as I figure out/recall how to make symlinks
>> that is ;) ).
> yeah, I would have to go through the same re-education ;-)
Adding another attribute with the same show function seems easy, but
that's probably not elegant. ;)
> (please add a note on feature-removal-schedule too)
Yep!
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
2011-08-19 22:32 ` [PATCHv3 1/4] usb: Provide usb_device_speed_name() function Michal Nazarewicz
2011-08-19 22:33 ` [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed" Michal Nazarewicz
@ 2011-08-19 22:33 ` Michal Nazarewicz
2011-08-19 23:31 ` Felipe Balbi
2011-08-19 22:33 ` [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED Michal Nazarewicz
3 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:33 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
This commit renames the “speed” field of the usb_gadget_driver
structure to “max_speed”. This is so that to make it more
apparent that the field represents the maximum speed gadget
driver can support.
This also make the field look more like fields with the same
name in usb_gadget and usb_composite_driver structures. All
of those represent the *maximal* speed given entity supports.
After this commit, there are the following fields in various
structures:
* usb_gadget::speed - the current connection speed,
* usb_gadget::max_speed - maximal speed UDC supports,
* usb_gadget_driver::max_speed - maximal speed gadget driver
supports, and
* usb_composite_driver::max_speed - maximal speed composite
gadget supports.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/gadget/amd5536udc.c | 2 +-
drivers/usb/gadget/ci13xxx_udc.c | 2 +-
drivers/usb/gadget/composite.c | 8 ++++----
drivers/usb/gadget/dbgp.c | 2 +-
drivers/usb/gadget/dummy_hcd.c | 13 ++++++-------
drivers/usb/gadget/file_storage.c | 4 ++--
drivers/usb/gadget/gmidi.c | 2 +-
drivers/usb/gadget/goku_udc.c | 2 +-
drivers/usb/gadget/inode.c | 6 +++---
drivers/usb/gadget/m66592-udc.c | 2 +-
drivers/usb/gadget/net2272.c | 2 +-
drivers/usb/gadget/net2280.c | 2 +-
drivers/usb/gadget/pch_udc.c | 2 +-
drivers/usb/gadget/printer.c | 2 +-
drivers/usb/gadget/r8a66597-udc.c | 2 +-
include/linux/usb/gadget.h | 4 ++--
16 files changed, 28 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/gadget/amd5536udc.c b/drivers/usb/gadget/amd5536udc.c
index 80ae808..7b48ed2 100644
--- a/drivers/usb/gadget/amd5536udc.c
+++ b/drivers/usb/gadget/amd5536udc.c
@@ -1968,7 +1968,7 @@ static int amd5536_start(struct usb_gadget_driver *driver,
u32 tmp;
if (!driver || !bind || !driver->setup
- || driver->speed != USB_SPEED_HIGH)
+ || driver->max_speed != USB_SPEED_HIGH)
return -EINVAL;
if (!dev)
return -ENODEV;
diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
index b877464..6c13174 100644
--- a/drivers/usb/gadget/ci13xxx_udc.c
+++ b/drivers/usb/gadget/ci13xxx_udc.c
@@ -798,7 +798,7 @@ static ssize_t show_driver(struct device *dev, struct device_attribute *attr,
n += scnprintf(buf + n, PAGE_SIZE - n, "function = %s\n",
(driver->function ? driver->function : ""));
n += scnprintf(buf + n, PAGE_SIZE - n, "max speed = %d\n",
- driver->speed);
+ driver->max_speed);
return n;
}
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5a3461e..851b5da 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1560,9 +1560,9 @@ composite_resume(struct usb_gadget *gadget)
static struct usb_gadget_driver composite_driver = {
#ifdef CONFIG_USB_GADGET_SUPERSPEED
- .speed = USB_SPEED_SUPER,
+ .max_speed = USB_SPEED_SUPER,
#else
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
#endif
.unbind = composite_unbind,
@@ -1609,8 +1609,8 @@ int usb_composite_probe(struct usb_composite_driver *driver,
driver->iProduct = driver->name;
composite_driver.function = (char *) driver->name;
composite_driver.driver.name = driver->name;
- composite_driver.speed = min((u8)composite_driver.speed,
- (u8)driver->max_speed);
+ composite_driver.max_speed = min((u8)composite_driver.max_speed,
+ (u8)driver->max_speed);
composite = driver;
composite_gadget_bind = bind;
diff --git a/drivers/usb/gadget/dbgp.c b/drivers/usb/gadget/dbgp.c
index e8737b5..4e6f8a2 100644
--- a/drivers/usb/gadget/dbgp.c
+++ b/drivers/usb/gadget/dbgp.c
@@ -405,7 +405,7 @@ fail:
static struct usb_gadget_driver dbgp_driver = {
.function = "dbgp",
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
.unbind = dbgp_unbind,
.setup = dbgp_setup,
.disconnect = dbgp_disconnect,
diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index fa60e9b..ccbe5ef 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -832,19 +832,18 @@ static int dummy_pullup (struct usb_gadget *_gadget, int value)
if (value && dum->driver) {
if (mod_data.is_super_speed)
- dum->gadget.speed = dum->driver->speed;
+ dum->gadget.speed = dum->driver->max_speed;
else if (mod_data.is_high_speed)
dum->gadget.speed = min_t(u8, USB_SPEED_HIGH,
- dum->driver->speed);
+ dum->driver->max_speed);
else
dum->gadget.speed = USB_SPEED_FULL;
dummy_udc_udpate_ep0(dum);
- if (dum->gadget.speed < dum->driver->speed)
+ if (dum->gadget.speed < dum->driver->max_speed)
dev_dbg(udc_dev(dum), "This device can perform faster"
- " if you connect it to a %s port...\n",
- (dum->driver->speed == USB_SPEED_SUPER ?
- "SuperSpeed" : "HighSpeed"));
+ " if you connect it to a %s port...\n",
+ usb_device_speed_name(dum->driver->max_speed));
}
dum_hcd = gadget_to_dummy_hcd(_gadget);
@@ -907,7 +906,7 @@ static int dummy_udc_start(struct usb_gadget *g,
struct dummy_hcd *dum_hcd = gadget_to_dummy_hcd(g);
struct dummy *dum = dum_hcd->dum;
- if (driver->speed == USB_SPEED_UNKNOWN)
+ if (driver->max_speed == USB_SPEED_UNKNOWN)
return -EINVAL;
/*
diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index cf875be..3cb083b 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3563,9 +3563,9 @@ static void fsg_resume(struct usb_gadget *gadget)
static struct usb_gadget_driver fsg_driver = {
#ifdef CONFIG_USB_GADGET_DUALSPEED
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
#else
- .speed = USB_SPEED_FULL,
+ .max_speed = USB_SPEED_FULL,
#endif
.function = (char *) fsg_string_product,
.unbind = fsg_unbind,
diff --git a/drivers/usb/gadget/gmidi.c b/drivers/usb/gadget/gmidi.c
index 305a8a8..23cab05 100644
--- a/drivers/usb/gadget/gmidi.c
+++ b/drivers/usb/gadget/gmidi.c
@@ -1292,7 +1292,7 @@ static void gmidi_resume(struct usb_gadget *gadget)
static struct usb_gadget_driver gmidi_driver = {
- .speed = USB_SPEED_FULL,
+ .max_speed = USB_SPEED_FULL,
.function = (char *)longname,
.unbind = gmidi_unbind,
diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
index ab9c924..5af70fcc 100644
--- a/drivers/usb/gadget/goku_udc.c
+++ b/drivers/usb/gadget/goku_udc.c
@@ -1357,7 +1357,7 @@ static int goku_start(struct usb_gadget_driver *driver,
int retval;
if (!driver
- || driver->speed < USB_SPEED_FULL
+ || driver->max_speed < USB_SPEED_FULL
|| !bind
|| !driver->disconnect
|| !driver->setup)
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index 1b24099..4bd48ea 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -1774,9 +1774,9 @@ gadgetfs_suspend (struct usb_gadget *gadget)
static struct usb_gadget_driver gadgetfs_driver = {
#ifdef CONFIG_USB_GADGET_DUALSPEED
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
#else
- .speed = USB_SPEED_FULL,
+ .max_speed = USB_SPEED_FULL,
#endif
.function = (char *) driver_desc,
.unbind = gadgetfs_unbind,
@@ -1800,7 +1800,7 @@ static int gadgetfs_probe (struct usb_gadget *gadget)
}
static struct usb_gadget_driver probe_driver = {
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
.unbind = gadgetfs_nop,
.setup = (void *)gadgetfs_nop,
.disconnect = gadgetfs_nop,
diff --git a/drivers/usb/gadget/m66592-udc.c b/drivers/usb/gadget/m66592-udc.c
index fbdfca8..72dc3f1 100644
--- a/drivers/usb/gadget/m66592-udc.c
+++ b/drivers/usb/gadget/m66592-udc.c
@@ -1482,7 +1482,7 @@ static int m66592_start(struct usb_gadget_driver *driver,
int retval;
if (!driver
- || driver->speed != USB_SPEED_HIGH
+ || driver->max_speed != USB_SPEED_HIGH
|| !bind
|| !driver->setup)
return -EINVAL;
diff --git a/drivers/usb/gadget/net2272.c b/drivers/usb/gadget/net2272.c
index e21ec48..c6c4934 100644
--- a/drivers/usb/gadget/net2272.c
+++ b/drivers/usb/gadget/net2272.c
@@ -1461,7 +1461,7 @@ static int net2272_start(struct usb_gadget_driver *driver,
unsigned i;
if (!driver || !bind || !driver->unbind || !driver->setup ||
- driver->speed != USB_SPEED_HIGH)
+ driver->max_speed != USB_SPEED_HIGH)
return -EINVAL;
if (!dev)
return -ENODEV;
diff --git a/drivers/usb/gadget/net2280.c b/drivers/usb/gadget/net2280.c
index c3220a1..1eb4774 100644
--- a/drivers/usb/gadget/net2280.c
+++ b/drivers/usb/gadget/net2280.c
@@ -1892,7 +1892,7 @@ static int net2280_start(struct usb_gadget_driver *driver,
* "must not be used in normal operation"
*/
if (!driver
- || driver->speed != USB_SPEED_HIGH
+ || driver->max_speed != USB_SPEED_HIGH
|| !bind || !driver->setup)
return -EINVAL;
if (!dev)
diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
index d781540..1db6d0e 100644
--- a/drivers/usb/gadget/pch_udc.c
+++ b/drivers/usb/gadget/pch_udc.c
@@ -2701,7 +2701,7 @@ static int pch_udc_start(struct usb_gadget_driver *driver,
struct pch_udc_dev *dev = pch_udc;
int retval;
- if (!driver || (driver->speed == USB_SPEED_UNKNOWN) || !bind ||
+ if (!driver || (driver->max_speed == USB_SPEED_UNKNOWN) || !bind ||
!driver->setup || !driver->unbind || !driver->disconnect) {
dev_err(&dev->pdev->dev,
"%s: invalid driver parameter\n", __func__);
diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
index 2db4913..104a18a 100644
--- a/drivers/usb/gadget/printer.c
+++ b/drivers/usb/gadget/printer.c
@@ -1552,7 +1552,7 @@ fail:
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver printer_driver = {
- .speed = DEVSPEED,
+ .max_speed = DEVSPEED,
.function = (char *) driver_desc,
.unbind = printer_unbind,
diff --git a/drivers/usb/gadget/r8a66597-udc.c b/drivers/usb/gadget/r8a66597-udc.c
index a8f7a05..df99690 100644
--- a/drivers/usb/gadget/r8a66597-udc.c
+++ b/drivers/usb/gadget/r8a66597-udc.c
@@ -1442,7 +1442,7 @@ static int r8a66597_start(struct usb_gadget_driver *driver,
int retval;
if (!driver
- || driver->speed != USB_SPEED_HIGH
+ || driver->max_speed != USB_SPEED_HIGH
|| !bind
|| !driver->setup)
return -EINVAL;
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 7eb2ec3..c8ea048 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -760,7 +760,7 @@ static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
/**
* struct usb_gadget_driver - driver for usb 'slave' devices
* @function: String describing the gadget's function
- * @speed: Highest speed the driver handles.
+ * @max_speed: Highest speed the driver handles.
* @setup: Invoked for ep0 control requests that aren't handled by
* the hardware level driver. Most calls must be handled by
* the gadget driver, including descriptor and configuration
@@ -824,7 +824,7 @@ static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
*/
struct usb_gadget_driver {
char *function;
- enum usb_device_speed speed;
+ enum usb_device_speed max_speed;
void (*unbind)(struct usb_gadget *);
int (*setup)(struct usb_gadget *,
const struct usb_ctrlrequest *);
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed
2011-08-19 22:33 ` [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed Michal Nazarewicz
@ 2011-08-19 23:31 ` Felipe Balbi
2011-08-20 2:34 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-19 23:31 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3588 bytes --]
On Sat, Aug 20, 2011 at 12:33:01AM +0200, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> This commit renames the “speed” field of the usb_gadget_driver
> structure to “max_speed”. This is so that to make it more
> apparent that the field represents the maximum speed gadget
> driver can support.
>
> This also make the field look more like fields with the same
> name in usb_gadget and usb_composite_driver structures. All
> of those represent the *maximal* speed given entity supports.
>
> After this commit, there are the following fields in various
> structures:
> * usb_gadget::speed - the current connection speed,
> * usb_gadget::max_speed - maximal speed UDC supports,
this will be handled inside the UDC itself, so why do you need to expose
it ?
> * usb_gadget_driver::max_speed - maximal speed gadget driver
> supports, and
> * usb_composite_driver::max_speed - maximal speed composite
> gadget supports.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> drivers/usb/gadget/amd5536udc.c | 2 +-
> drivers/usb/gadget/ci13xxx_udc.c | 2 +-
> drivers/usb/gadget/composite.c | 8 ++++----
> drivers/usb/gadget/dbgp.c | 2 +-
> drivers/usb/gadget/dummy_hcd.c | 13 ++++++-------
> drivers/usb/gadget/file_storage.c | 4 ++--
> drivers/usb/gadget/gmidi.c | 2 +-
> drivers/usb/gadget/goku_udc.c | 2 +-
> drivers/usb/gadget/inode.c | 6 +++---
> drivers/usb/gadget/m66592-udc.c | 2 +-
> drivers/usb/gadget/net2272.c | 2 +-
> drivers/usb/gadget/net2280.c | 2 +-
> drivers/usb/gadget/pch_udc.c | 2 +-
> drivers/usb/gadget/printer.c | 2 +-
> drivers/usb/gadget/r8a66597-udc.c | 2 +-
> include/linux/usb/gadget.h | 4 ++--
> 16 files changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/gadget/amd5536udc.c b/drivers/usb/gadget/amd5536udc.c
> index 80ae808..7b48ed2 100644
> --- a/drivers/usb/gadget/amd5536udc.c
> +++ b/drivers/usb/gadget/amd5536udc.c
> @@ -1968,7 +1968,7 @@ static int amd5536_start(struct usb_gadget_driver *driver,
> u32 tmp;
>
> if (!driver || !bind || !driver->setup
> - || driver->speed != USB_SPEED_HIGH)
> + || driver->max_speed != USB_SPEED_HIGH)
> return -EINVAL;
> if (!dev)
> return -ENODEV;
> diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
> index b877464..6c13174 100644
> --- a/drivers/usb/gadget/ci13xxx_udc.c
> +++ b/drivers/usb/gadget/ci13xxx_udc.c
> @@ -798,7 +798,7 @@ static ssize_t show_driver(struct device *dev, struct device_attribute *attr,
> n += scnprintf(buf + n, PAGE_SIZE - n, "function = %s\n",
> (driver->function ? driver->function : ""));
> n += scnprintf(buf + n, PAGE_SIZE - n, "max speed = %d\n",
> - driver->speed);
> + driver->max_speed);
>
> return n;
> }
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 5a3461e..851b5da 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1560,9 +1560,9 @@ composite_resume(struct usb_gadget *gadget)
>
> static struct usb_gadget_driver composite_driver = {
> #ifdef CONFIG_USB_GADGET_SUPERSPEED
> - .speed = USB_SPEED_SUPER,
> + .max_speed = USB_SPEED_SUPER,
> #else
> - .speed = USB_SPEED_HIGH,
> + .max_speed = USB_SPEED_HIGH,
by doing this based on what the function drivers tell you, you could
drop this ifdeferry.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed
2011-08-19 23:31 ` Felipe Balbi
@ 2011-08-20 2:34 ` Alan Stern
2011-08-22 10:42 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-20 2:34 UTC (permalink / raw)
To: Felipe Balbi
Cc: Michal Nazarewicz, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Sat, 20 Aug 2011, Felipe Balbi wrote:
> On Sat, Aug 20, 2011 at 12:33:01AM +0200, Michal Nazarewicz wrote:
> > From: Michal Nazarewicz <mina86@mina86.com>
> >
> > This commit renames the “speed” field of the usb_gadget_driver
> > structure to “max_speed”. This is so that to make it more
> > apparent that the field represents the maximum speed gadget
> > driver can support.
> >
> > This also make the field look more like fields with the same
> > name in usb_gadget and usb_composite_driver structures. All
> > of those represent the *maximal* speed given entity supports.
> >
> > After this commit, there are the following fields in various
> > structures:
> > * usb_gadget::speed - the current connection speed,
> > * usb_gadget::max_speed - maximal speed UDC supports,
>
> this will be handled inside the UDC itself, so why do you need to expose
> it ?
I wondered about this too. It turns out that sometimes the gadget
driver really does need to know about the maximum speed supported by
the UDC hardware and driver.
For example, suppose the gadget driver and the UDC both support high
speed, but the gadget is currently plugged into a USB-1.1 controller
and therefore running only at full speed. Then the gadget driver has
to respond to the Get-Device-Qualifier and Get-Other-Speed-Config
requests by sending the appropriate descriptors. But if the UDC
doesn't support high speed operation then the gadget driver has to
respond to those requests with a STALL.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed
2011-08-20 2:34 ` Alan Stern
@ 2011-08-22 10:42 ` Felipe Balbi
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-22 10:42 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Michal Nazarewicz, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]
Hi,
On Fri, Aug 19, 2011 at 10:34:29PM -0400, Alan Stern wrote:
> On Sat, 20 Aug 2011, Felipe Balbi wrote:
>
> > On Sat, Aug 20, 2011 at 12:33:01AM +0200, Michal Nazarewicz wrote:
> > > From: Michal Nazarewicz <mina86@mina86.com>
> > >
> > > This commit renames the “speed” field of the usb_gadget_driver
> > > structure to “max_speed”. This is so that to make it more
> > > apparent that the field represents the maximum speed gadget
> > > driver can support.
> > >
> > > This also make the field look more like fields with the same
> > > name in usb_gadget and usb_composite_driver structures. All
> > > of those represent the *maximal* speed given entity supports.
> > >
> > > After this commit, there are the following fields in various
> > > structures:
> > > * usb_gadget::speed - the current connection speed,
> > > * usb_gadget::max_speed - maximal speed UDC supports,
> >
> > this will be handled inside the UDC itself, so why do you need to expose
> > it ?
>
> I wondered about this too. It turns out that sometimes the gadget
> driver really does need to know about the maximum speed supported by
> the UDC hardware and driver.
>
> For example, suppose the gadget driver and the UDC both support high
> speed, but the gadget is currently plugged into a USB-1.1 controller
> and therefore running only at full speed. Then the gadget driver has
> to respond to the Get-Device-Qualifier and Get-Other-Speed-Config
> requests by sending the appropriate descriptors. But if the UDC
> doesn't support high speed operation then the gadget driver has to
> respond to those requests with a STALL.
aa, good point :-)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
` (2 preceding siblings ...)
2011-08-19 22:33 ` [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed Michal Nazarewicz
@ 2011-08-19 22:33 ` Michal Nazarewicz
2011-08-20 13:41 ` Alan Stern
3 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:33 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
This commit removes USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
Kconfig options. Since now kernel allows many UDC drivers to be
compiled, those options may turn to no longer be valid. For
instance, if someone decides to build UDC that supports super
speed and UDC that supports high speed only, the latter will be
"assumed" to support super speed since USB_GADGET_SUPERSPEED will
be selected by the former.
The test of whether CONFIG_USB_GADGET_*SPEED was defined was just
an optimisation which removed otherwise dead code (ie. if UDC is
not dual speed, there is no need to handle cases that can happen
if speed is high). This commit removes those checks.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/gadget/Kconfig | 30 ----------------------------
drivers/usb/gadget/composite.c | 9 +-------
drivers/usb/gadget/dummy_hcd.c | 2 +-
drivers/usb/gadget/file_storage.c | 4 ---
drivers/usb/gadget/inode.c | 10 ---------
drivers/usb/gadget/printer.c | 39 ++++--------------------------------
drivers/usb/gadget/u_ether.c | 7 ------
include/linux/usb/gadget.h | 19 +----------------
8 files changed, 9 insertions(+), 111 deletions(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 5a084b9..c60c167 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -133,7 +133,6 @@ config USB_AT91
config USB_ATMEL_USBA
tristate "Atmel USBA"
- select USB_GADGET_DUALSPEED
depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
help
USBA is the integrated high-speed USB Device controller on
@@ -142,7 +141,6 @@ config USB_ATMEL_USBA
config USB_FSL_USB2
tristate "Freescale Highspeed USB DR Peripheral Controller"
depends on FSL_SOC || ARCH_MXC
- select USB_GADGET_DUALSPEED
select USB_FSL_MPH_DR_OF if OF
help
Some of Freescale PowerPC processors have a High Speed
@@ -158,7 +156,6 @@ config USB_FSL_USB2
config USB_FUSB300
tristate "Faraday FUSB300 USB Peripheral Controller"
depends on !PHYS_ADDR_T_64BIT
- select USB_GADGET_DUALSPEED
help
Faraday usb device controller FUSB300 driver
@@ -206,7 +203,6 @@ config USB_PXA25X_SMALL
config USB_R8A66597
tristate "Renesas R8A66597 USB Peripheral Controller"
- select USB_GADGET_DUALSPEED
help
R8A66597 is a discrete USB host and peripheral controller chip that
supports both full and high speed USB 2.0 data transfers.
@@ -220,7 +216,6 @@ config USB_RENESAS_USBHS_UDC
tristate 'Renesas USBHS controller'
depends on SUPERH || ARCH_SHMOBILE
depends on USB_RENESAS_USBHS
- select USB_GADGET_DUALSPEED
help
Renesas USBHS is a discrete USB host and peripheral controller chip
that supports both full and high speed USB 2.0 data transfers.
@@ -249,7 +244,6 @@ config USB_S3C_HSOTG
tristate "S3C HS/OtG USB Device controller"
depends on S3C_DEV_USB_HSOTG
select USB_GADGET_S3C_HSOTG_PIO
- select USB_GADGET_DUALSPEED
help
The Samsung S3C64XX USB2.0 high-speed gadget controller
integrated into the S3C64XX series SoC.
@@ -287,7 +281,6 @@ config USB_S3C2410_DEBUG
config USB_S3C_HSUDC
tristate "S3C2416, S3C2443 and S3C2450 USB Device Controller"
depends on ARCH_S3C2410
- select USB_GADGET_DUALSPEED
help
Samsung's S3C2416, S3C2443 and S3C2450 is an ARM9 based SoC
integrated with dual speed USB 2.0 device controller. It has
@@ -298,7 +291,6 @@ config USB_S3C_HSUDC
config USB_PXA_U2O
tristate "PXA9xx Processor USB2.0 controller"
depends on ARCH_MMP
- select USB_GADGET_DUALSPEED
help
PXA9xx Processor series include a high speed USB2.0 device
controller, which support high speed and full speed USB peripheral.
@@ -311,14 +303,12 @@ config USB_PXA_U2O
config USB_GADGET_MUSB_HDRC
tristate "Inventra HDRC USB Peripheral (TI, ADI, ...)"
depends on USB_MUSB_HDRC
- select USB_GADGET_DUALSPEED
help
This OTG-capable silicon IP is used in dual designs including
the TI DaVinci, OMAP 243x, OMAP 343x, TUSB 6010, and ADI Blackfin
config USB_M66592
tristate "Renesas M66592 USB Peripheral Controller"
- select USB_GADGET_DUALSPEED
help
M66592 is a discrete USB peripheral controller chip that
supports both full and high speed USB 2.0 data transfers.
@@ -335,7 +325,6 @@ config USB_M66592
config USB_AMD5536UDC
tristate "AMD5536 UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
The AMD5536 UDC is part of the AMD Geode CS5536, an x86 southbridge.
It is a USB Highspeed DMA capable USB device controller. Beside ep0
@@ -363,7 +352,6 @@ config USB_FSL_QE
config USB_CI13XXX_PCI
tristate "MIPS USB CI13xxx PCI UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
MIPS USB IP core family device controller
Currently it only supports IP part number CI13412
@@ -374,7 +362,6 @@ config USB_CI13XXX_PCI
config USB_NET2272
tristate "PLX NET2272"
- select USB_GADGET_DUALSPEED
help
PLX NET2272 is a USB peripheral controller which supports
both full and high speed USB 2.0 data transfers.
@@ -398,7 +385,6 @@ config USB_NET2272_DMA
config USB_NET2280
tristate "NetChip 228x"
depends on PCI
- select USB_GADGET_DUALSPEED
help
NetChip 2280 / 2282 is a PCI based USB peripheral controller which
supports both full and high speed USB 2.0 data transfers.
@@ -429,7 +415,6 @@ config USB_LANGWELL
tristate "Intel Langwell USB Device Controller"
depends on PCI
depends on !PHYS_ADDR_T_64BIT
- select USB_GADGET_DUALSPEED
help
Intel Langwell USB Device Controller is a High-Speed USB
On-The-Go device controller.
@@ -444,7 +429,6 @@ config USB_LANGWELL
config USB_EG20T
tristate "Intel EG20T PCH/OKI SEMICONDUCTOR ML7213 IOH UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
This is a USB device driver for EG20T PCH.
EG20T PCH is the platform controller hub that is used in Intel's
@@ -466,7 +450,6 @@ config USB_EG20T
config USB_CI13XXX_MSM
tristate "MIPS USB CI13xxx for MSM"
depends on ARCH_MSM
- select USB_GADGET_DUALSPEED
select USB_MSM_OTG
help
MSM SoC has chipidea USB controller. This driver uses
@@ -487,8 +470,6 @@ config USB_CI13XXX_MSM
config USB_DUMMY_HCD
tristate "Dummy HCD (DEVELOPMENT)"
depends on USB=y || (USB=m && USB_GADGET=m)
- select USB_GADGET_DUALSPEED
- select USB_GADGET_SUPERSPEED
help
This host controller driver emulates USB, looping all data transfer
requests back to a USB "gadget driver" in the same host. The host
@@ -513,17 +494,6 @@ config USB_DUMMY_HCD
endchoice
-# Selected by UDC drivers that support high-speed operation.
-config USB_GADGET_DUALSPEED
- bool
- depends on USB_GADGET
-
-# Selected by UDC drivers that support super-speed opperation
-config USB_GADGET_SUPERSPEED
- bool
- depends on USB_GADGET
- depends on USB_GADGET_DUALSPEED
-
#
# USB Gadget Drivers
#
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 851b5da..c08edd9 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
- .max_speed = USB_SPEED_SUPER,
-#else
- .max_speed = USB_SPEED_HIGH,
-#endif
-
.unbind = composite_unbind,
.setup = composite_setup,
@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver *driver,
driver->iProduct = driver->name;
composite_driver.function = (char *) driver->name;
composite_driver.driver.name = driver->name;
- composite_driver.max_speed = min((u8)composite_driver.max_speed,
- (u8)driver->max_speed);
+ composite_driver.max_speed = driver->max_speed;
composite = driver;
composite_gadget_bind = bind;
diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index ccbe5ef..d3ce66c 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -985,7 +985,7 @@ static int dummy_udc_probe (struct platform_device *pdev)
dum->gadget.name = gadget_name;
dum->gadget.ops = &dummy_ops;
- dum->gadget.max_speed = USB_SPEED_SUPER;;
+ dum->gadget.max_speed = USB_SPEED_SUPER;
dev_set_name(&dum->gadget.dev, "gadget");
dum->gadget.dev.parent = &pdev->dev;
diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index 3cb083b..0b94981 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
.max_speed = USB_SPEED_HIGH,
-#else
- .max_speed = USB_SPEED_FULL,
-#endif
.function = (char *) fsg_string_product,
.unbind = fsg_unbind,
.disconnect = fsg_disconnect,
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index 4bd48ea..23c75d8 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -837,7 +837,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
if (value == 0)
data->state = STATE_EP_ENABLED;
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_SPEED_HIGH:
/* fails if caller didn't provide that descriptor... */
ep->desc = &data->hs_desc;
@@ -845,7 +844,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
if (value == 0)
data->state = STATE_EP_ENABLED;
break;
-#endif
default:
DBG(data->dev, "unconnected, %s init abandoned\n",
data->name);
@@ -1331,7 +1329,6 @@ static const struct file_operations ep0_io_operations = {
* Unrecognized ep0 requests may be handled in user space.
*/
-#ifdef CONFIG_USB_GADGET_DUALSPEED
static void make_qualifier (struct dev_data *dev)
{
struct usb_qualifier_descriptor qual;
@@ -1354,7 +1351,6 @@ static void make_qualifier (struct dev_data *dev)
memcpy (dev->rbuf, &qual, sizeof qual);
}
-#endif
static int
config_buf (struct dev_data *dev, u8 type, unsigned index)
@@ -1434,7 +1430,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
dev->dev->bMaxPacketSize0 = dev->gadget->ep0->maxpacket;
req->buf = dev->dev;
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
if (!dev->hs_config)
break;
@@ -1444,7 +1439,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
case USB_DT_OTHER_SPEED_CONFIG:
// FALLTHROUGH
-#endif
case USB_DT_CONFIG:
value = config_buf (dev,
w_value >> 8,
@@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
}
static struct usb_gadget_driver gadgetfs_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
.max_speed = USB_SPEED_HIGH,
-#else
- .max_speed = USB_SPEED_FULL,
-#endif
.function = (char *) driver_desc,
.unbind = gadgetfs_unbind,
.setup = gadgetfs_setup,
diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
index 104a18a..93f0b94 100644
--- a/drivers/usb/gadget/printer.c
+++ b/drivers/usb/gadget/printer.c
@@ -164,12 +164,6 @@ module_param(qlen, uint, S_IRUGO|S_IWUSR);
#define QLEN qlen
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-#define DEVSPEED USB_SPEED_HIGH
-#else /* full speed (low speed doesn't do bulk) */
-#define DEVSPEED USB_SPEED_FULL
-#endif
-
/*-------------------------------------------------------------------------*/
#define xprintk(d, level, fmt, args...) \
@@ -288,8 +282,6 @@ static const struct usb_descriptor_header *fs_printer_function [11] = {
NULL
};
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
/*
* usb 2.0 devices need to expose both high speed and full speed
* descriptors, unless they only run at full speed.
@@ -328,13 +320,6 @@ static const struct usb_descriptor_header *hs_printer_function [11] = {
/* maxpacket and other transfer characteristics vary by speed. */
#define ep_desc(g, hs, fs) (((g)->speed == USB_SPEED_HIGH)?(hs):(fs))
-#else
-
-/* if there's no high speed support, maxpacket doesn't change. */
-#define ep_desc(g, hs, fs) (((void)(g)), (fs))
-
-#endif /* !CONFIG_USB_GADGET_DUALSPEED */
-
/*-------------------------------------------------------------------------*/
/* descriptors that are built on-demand */
@@ -979,9 +964,7 @@ printer_set_config(struct printer_dev *dev, unsigned number)
switch (gadget->speed) {
case USB_SPEED_FULL: speed = "full"; break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_SPEED_HIGH: speed = "high"; break;
-#endif
default: speed = "?"; break;
}
@@ -998,23 +981,15 @@ config_buf(enum usb_device_speed speed, u8 *buf, u8 type, unsigned index,
{
int len;
const struct usb_descriptor_header **function;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
int hs = (speed == USB_SPEED_HIGH);
+ if (index >= device_desc.bNumConfigurations)
+ return -EINVAL;
+
if (type == USB_DT_OTHER_SPEED_CONFIG)
hs = !hs;
- if (hs) {
- function = hs_printer_function;
- } else {
- function = fs_printer_function;
- }
-#else
- function = fs_printer_function;
-#endif
-
- if (index >= device_desc.bNumConfigurations)
- return -EINVAL;
+ function = hs ? hs_printer_function : fs_printer_function;
/* for now, don't advertise srp-only devices */
if (!is_otg)
@@ -1156,7 +1131,6 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
value = min(wLength, (u16) sizeof device_desc);
memcpy(req->buf, &device_desc, value);
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
if (!gadget_is_dualspeed(gadget))
break;
@@ -1175,7 +1149,6 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
if (!gadget_is_dualspeed(gadget))
break;
/* FALLTHROUGH */
-#endif /* CONFIG_USB_GADGET_DUALSPEED */
case USB_DT_CONFIG:
value = config_buf(gadget->speed, req->buf,
wValue >> 8,
@@ -1460,11 +1433,9 @@ autoconf_fail:
goto autoconf_fail;
out_ep->driver_data = out_ep; /* claim */
-#ifdef CONFIG_USB_GADGET_DUALSPEED
/* assumes that all endpoints are dual-speed */
hs_ep_in_desc.bEndpointAddress = fs_ep_in_desc.bEndpointAddress;
hs_ep_out_desc.bEndpointAddress = fs_ep_out_desc.bEndpointAddress;
-#endif /* DUALSPEED */
usb_gadget_set_selfpowered(gadget);
@@ -1552,7 +1523,7 @@ fail:
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver printer_driver = {
- .max_speed = DEVSPEED,
+ .max_speed = USB_SPEED_HIGH,
.function = (char *) driver_desc,
.unbind = printer_unbind,
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index dfed4c1..8642abb 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -92,17 +92,10 @@ struct eth_dev {
#define DEFAULT_QLEN 2 /* double buffering by default */
-
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
static unsigned qmult = 5;
module_param(qmult, uint, S_IRUGO|S_IWUSR);
MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
-#else /* full speed (low speed doesn't do bulk) */
-#define qmult 1
-#endif
-
/* for dual-speed hardware, use deeper queues at high/super speed */
static inline int qlen(struct usb_gadget *gadget)
{
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index c8ea048..22404cf 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -548,14 +548,7 @@ static inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
*/
static inline int gadget_is_dualspeed(struct usb_gadget *g)
{
-#ifdef CONFIG_USB_GADGET_DUALSPEED
- /* runtime test would check "g->max_speed" ... that might be
- * useful to work around hardware bugs, but is mostly pointless
- */
- return 1;
-#else
- return 0;
-#endif
+ return g->max_speed >= USB_SPEED_HIGH;
}
/**
@@ -565,15 +558,7 @@ static inline int gadget_is_dualspeed(struct usb_gadget *g)
*/
static inline int gadget_is_superspeed(struct usb_gadget *g)
{
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
- /*
- * runtime test would check "g->max_speed" ... that might be
- * useful to work around hardware bugs, but is mostly pointless
- */
- return 1;
-#else
- return 0;
-#endif
+ return g->max_speed >= USB_SPEED_SUPER;
}
/**
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
2011-08-19 22:33 ` [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED Michal Nazarewicz
@ 2011-08-20 13:41 ` Alan Stern
2011-08-22 14:51 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-20 13:41 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
On Sat, 20 Aug 2011, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> This commit removes USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
> Kconfig options. Since now kernel allows many UDC drivers to be
> compiled, those options may turn to no longer be valid. For
> instance, if someone decides to build UDC that supports super
> speed and UDC that supports high speed only, the latter will be
> "assumed" to support super speed since USB_GADGET_SUPERSPEED will
> be selected by the former.
>
> The test of whether CONFIG_USB_GADGET_*SPEED was defined was just
> an optimisation which removed otherwise dead code (ie. if UDC is
> not dual speed, there is no need to handle cases that can happen
> if speed is high). This commit removes those checks.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
...
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -985,7 +985,7 @@ static int dummy_udc_probe (struct platform_device *pdev)
>
> dum->gadget.name = gadget_name;
> dum->gadget.ops = &dummy_ops;
> - dum->gadget.max_speed = USB_SPEED_SUPER;;
> + dum->gadget.max_speed = USB_SPEED_SUPER;
You might as well get this right in patch 2/4. Then this hunk
wouldn't be needed.
> --- a/drivers/usb/gadget/inode.c
> +++ b/drivers/usb/gadget/inode.c
...
> @@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
> }
>
> static struct usb_gadget_driver gadgetfs_driver = {
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> .max_speed = USB_SPEED_HIGH,
> -#else
> - .max_speed = USB_SPEED_FULL,
> -#endif
I'm not sure that a static definition is correct here. The actual
max_speed depends on what descriptors the userspace program sends.
Take a look at the ep_config() routine.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
2011-08-20 13:41 ` Alan Stern
@ 2011-08-22 14:51 ` Michal Nazarewicz
2011-08-22 15:03 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-22 14:51 UTC (permalink / raw)
To: Alan Stern
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
> On Sat, 20 Aug 2011, Michal Nazarewicz wrote:
>> --- a/drivers/usb/gadget/dummy_hcd.c
>> +++ b/drivers/usb/gadget/dummy_hcd.c
>> @@ -985,7 +985,7 @@ static int dummy_udc_probe (struct platform_device
>> *pdev)
>>
>> dum->gadget.name = gadget_name;
>> dum->gadget.ops = &dummy_ops;
>> - dum->gadget.max_speed = USB_SPEED_SUPER;;
>> + dum->gadget.max_speed = USB_SPEED_SUPER;
On Sat, 20 Aug 2011 15:41:17 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> You might as well get this right in patch 2/4. Then this hunk
> wouldn't be needed.
Sorry about that, I've fixuped the wrong commit
>> --- a/drivers/usb/gadget/inode.c
>> +++ b/drivers/usb/gadget/inode.c
> ...
>> @@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
>> }
>>
>> static struct usb_gadget_driver gadgetfs_driver = {
>> -#ifdef CONFIG_USB_GADGET_DUALSPEED
>> .max_speed = USB_SPEED_HIGH,
>> -#else
>> - .max_speed = USB_SPEED_FULL,
>> -#endif
> I'm not sure that a static definition is correct here. The actual
> max_speed depends on what descriptors the userspace program sends.
> Take a look at the ep_config() routine.
Good catch. I've changed it to:
if (dev->hs_config)
gadgetfs_driver.max_speed = USB_SPEED_HIGH;
else
gadgetfs_driver.max_speed = USB_SPEED_FULL;
just before usb_gadget_probe_driver(), which should do the trick.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
2011-08-22 14:51 ` Michal Nazarewicz
@ 2011-08-22 15:03 ` Alan Stern
0 siblings, 0 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-22 15:03 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
On Mon, 22 Aug 2011, Michal Nazarewicz wrote:
> >> --- a/drivers/usb/gadget/inode.c
> >> +++ b/drivers/usb/gadget/inode.c
> > ...
> >> @@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
> >> }
> >>
> >> static struct usb_gadget_driver gadgetfs_driver = {
> >> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> >> .max_speed = USB_SPEED_HIGH,
> >> -#else
> >> - .max_speed = USB_SPEED_FULL,
> >> -#endif
>
> > I'm not sure that a static definition is correct here. The actual
> > max_speed depends on what descriptors the userspace program sends.
> > Take a look at the ep_config() routine.
>
> Good catch. I've changed it to:
>
> if (dev->hs_config)
> gadgetfs_driver.max_speed = USB_SPEED_HIGH;
> else
> gadgetfs_driver.max_speed = USB_SPEED_FULL;
>
> just before usb_gadget_probe_driver(), which should do the trick.
Yes, that sounds like the right thing to do.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread