[PATCH/RESEND 3/5 v4] usb: Configure endpoint according to gadget speed.

[PATCH/RESEND 3/5 v4] usb: Configure endpoint according to gadget speed.

[Tatyana Brokhman]

Add config_ep_by_speed() to configure the endpoint according to the gadget
speed. Using this function will spare the FDs from handling the endpoint
chosen descriptor.

Signed-off-by: Tatyana Brokhman <tlinder@codeaurora.org>

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index c2251c4..fb7e488 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -74,6 +74,82 @@ MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");
 static char composite_manufacturer[50];
 
 /*-------------------------------------------------------------------------*/
+/**
+ * next_ep_desc() - advance to the next EP descriptor
+ * @t: currect pointer within descriptor array
+ *
+ * Return: next EP descriptor or NULL
+ *
+ * Iterate over @t until either EP descriptor found or
+ * NULL (that indicates end of list) encountered
+ */
+static struct usb_descriptor_header**
+next_ep_desc(struct usb_descriptor_header **t)
+{
+	for (; *t; t++) {
+		if ((*t)->bDescriptorType == USB_DT_ENDPOINT)
+			return t;
+	}
+	return NULL;
+}
+
+/**
+ * config_ep_by_speed() - configures the given endpoint
+ * according to gadget speed.
+ * @g: pointer to the gadget
+ * @f: usb function
+ * @_ep: the endpoint to configure
+ *
+ * Return: error code, 0 on success
+ *
+ * This function chooses the right descriptors for a given
+ * endpoint according to gadget speed and saves in in the
+ * endpoint desc field. If the endpoint already has a descriptor
+ * assigned to it - overwrites it with currently corresponding
+ * descriptor. The endpoint maxpacket field is updated according
+ * to the choosen descriptor.
+ * Note: the supplied function should hold all the descriptors
+ * for supported speeds
+ */
+int config_ep_by_speed(struct usb_gadget *g,
+			struct usb_function *f,
+			struct usb_ep *_ep)
+{
+	struct usb_endpoint_descriptor *chosen_desc = NULL;
+	struct usb_descriptor_header **speed_desc = NULL;
+
+	struct usb_descriptor_header **d_spd; /* cursor for speed desc */
+
+	if (!g || !f || !_ep)
+		return -EIO;
+
+	/* select desired speed */
+	switch (g->speed) {
+	case USB_SPEED_HIGH:
+		if (gadget_is_dualspeed(g)) {
+			speed_desc = f->hs_descriptors;
+			break;
+		}
+		/* else: fall through */
+	default:
+		speed_desc = f->descriptors;
+	}
+	/* find descriptors */
+	for (d_spd = next_ep_desc(speed_desc); d_spd;
+	      d_spd = next_ep_desc(d_spd+1)) {
+		chosen_desc = (struct usb_endpoint_descriptor *)*d_spd;
+		if (chosen_desc->bEndpointAddress == _ep->bEndpointAddress)
+			goto ep_found;
+	}
+	return -EIO;
+
+ep_found:
+	/* commit results */
+	_ep->maxpacket = le16_to_cpu(chosen_desc->wMaxPacketSize);
+	_ep->desc = chosen_desc;
+
+	return 0;
+}
 
 /**
  * usb_add_function() - add a function to a configuration
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 9b7360f..de3461a 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -191,6 +191,7 @@ ep_matches (
 			size = 64;
 		desc->wMaxPacketSize = cpu_to_le16(size);
 	}
+	ep->bEndpointAddress = desc->bEndpointAddress;
 	return 1;
 }
 
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 3d29a7d..b0c78cd 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -138,6 +138,27 @@ int usb_function_activate(struct usb_function *);
 int usb_interface_id(struct usb_configuration *, struct usb_function *);
 
 /**
+ * config_ep_by_speed() - configures the given endpoint
+ * according to gadget speed.
+ * @g: pointer to the gadget
+ * @f: usb function
+ * @_ep: the endpoint to configure
+ *
+ * Return: error code, 0 on success
+ *
+ * This function chooses the right descriptors for a given
+ * endpoint according to gadget speed and saves in in the
+ * endpoint desc field. If the endpoint already has a descriptor
+ * assigned to it - overwrites it with currently corresponding
+ * descriptor. The endpoint maxpacket field is updated according
+ * to the choosen descriptor.
+ * Note: the supplied function should hold all the descriptors
+ * for supported speeds
+ */
+int config_ep_by_speed(struct usb_gadget *g, struct usb_function *f,
+			struct usb_ep *_ep);
+
+/**
  * ep_choose - select descriptor endpoint at current device speed
  * @g: gadget, connected and running at some speed
  * @hs: descriptor to use for high speed operation
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 500ae91..0387aba 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -132,6 +132,8 @@ struct usb_ep_ops {
  *	value can sometimes be reduced (hardware allowing), according to
  *      the endpoint descriptor used to configure the endpoint.
  * @driver_data:for use by the gadget driver.
+ * @bEndpointAddress: used to identify the endpoint when finding
+ *	descriptor that matches connection speed
  * @desc: endpoint descriptor.  This pointer is set before the endpoint is
  *	enabled and remains valid until the endpoint is disabled.
  *
@@ -146,6 +148,7 @@ struct usb_ep {
 	const struct usb_ep_ops	*ops;
 	struct list_head	ep_list;
 	unsigned		maxpacket:16;
+	u8				bEndpointAddress;
 	const struct usb_endpoint_descriptor	*desc;
 };
 
-- 
1.7.3.3

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH/RESEND 3/5 v4] usb: Configure endpoint according to gadget speed.

[Sebastian Andrzej Siewior]

* Tatyana Brokhman | 2011-03-23 10:04:04 [+0200]:

>diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>index c2251c4..fb7e488 100644
>--- a/drivers/usb/gadget/composite.c
>+++ b/drivers/usb/gadget/composite.c
>@@ -74,6 +74,82 @@ MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");
> static char composite_manufacturer[50];
> 
> /*-------------------------------------------------------------------------*/
>+/**
>+ * next_ep_desc() - advance to the next EP descriptor
>+ * @t: currect pointer within descriptor array
>+ *
>+ * Return: next EP descriptor or NULL
>+ *
>+ * Iterate over @t until either EP descriptor found or
>+ * NULL (that indicates end of list) encountered
>+ */
>+static struct usb_descriptor_header**
>+next_ep_desc(struct usb_descriptor_header **t)
>+{
>+	for (; *t; t++) {
>+		if ((*t)->bDescriptorType == USB_DT_ENDPOINT)
>+			return t;
>+	}
>+	return NULL;
>+}
>+
>+/**
>+ * config_ep_by_speed() - configures the given endpoint
>+ * according to gadget speed.
>+ * @g: pointer to the gadget
>+ * @f: usb function
>+ * @_ep: the endpoint to configure
>+ *
>+ * Return: error code, 0 on success
>+ *
>+ * This function chooses the right descriptors for a given
>+ * endpoint according to gadget speed and saves in in the
                                                 ^it

>+ * endpoint desc field. If the endpoint already has a descriptor
>+ * assigned to it - overwrites it with currently corresponding
>+ * descriptor. The endpoint maxpacket field is updated according
>+ * to the choosen descriptor.
>+ * Note: the supplied function should hold all the descriptors
>+ * for supported speeds
>+ */
>+int config_ep_by_speed(struct usb_gadget *g,
>+			struct usb_function *f,
>+			struct usb_ep *_ep)
>+{
>+	struct usb_endpoint_descriptor *chosen_desc = NULL;
>+	struct usb_descriptor_header **speed_desc = NULL;
>+
>+	struct usb_descriptor_header **d_spd; /* cursor for speed desc */
>+
>+	if (!g || !f || !_ep)
>+		return -EIO;
>+
>+	/* select desired speed */
>+	switch (g->speed) {
>+	case USB_SPEED_HIGH:
>+		if (gadget_is_dualspeed(g)) {
>+			speed_desc = f->hs_descriptors;
>+			break;
>+		}
>+		/* else: fall through */
>+	default:
>+		speed_desc = f->descriptors;
>+	}
>+	/* find descriptors */
>+	for (d_spd = next_ep_desc(speed_desc); d_spd;
>+	      d_spd = next_ep_desc(d_spd+1)) {

this might look better if you would something like the list_for_each()
macro.

>+		chosen_desc = (struct usb_endpoint_descriptor *)*d_spd;
>+		if (chosen_desc->bEndpointAddress == _ep->bEndpointAddress)
>+			goto ep_found;
>+	}
>+	return -EIO;
>+
>+ep_found:
>+	/* commit results */
>+	_ep->maxpacket = le16_to_cpu(chosen_desc->wMaxPacketSize);
>+	_ep->desc = chosen_desc;
>+
>+	return 0;
>+}
> 
> /**
>  * usb_add_function() - add a function to a configuration
>diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
>index 3d29a7d..b0c78cd 100644
>--- a/include/linux/usb/composite.h
>+++ b/include/linux/usb/composite.h
>@@ -138,6 +138,27 @@ int usb_function_activate(struct usb_function *);
> int usb_interface_id(struct usb_configuration *, struct usb_function *);
> 
> /**
>+ * config_ep_by_speed() - configures the given endpoint
>+ * according to gadget speed.
>+ * @g: pointer to the gadget
>+ * @f: usb function
>+ * @_ep: the endpoint to configure
>+ *
>+ * Return: error code, 0 on success
>+ *
>+ * This function chooses the right descriptors for a given
>+ * endpoint according to gadget speed and saves in in the
>+ * endpoint desc field. If the endpoint already has a descriptor
>+ * assigned to it - overwrites it with currently corresponding
>+ * descriptor. The endpoint maxpacket field is updated according
>+ * to the choosen descriptor.
>+ * Note: the supplied function should hold all the descriptors
>+ * for supported speeds
>+ */

You have the same comment (including the same typo) here and in the .c.
Please use only on (in the .c file) as they will go async.

>+int config_ep_by_speed(struct usb_gadget *g, struct usb_function *f,
>+			struct usb_ep *_ep);
>+
>+/**
>  * ep_choose - select descriptor endpoint at current device speed
>  * @g: gadget, connected and running at some speed
>  * @hs: descriptor to use for high speed operation
>diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>index 500ae91..0387aba 100644
>--- a/include/linux/usb/gadget.h
>+++ b/include/linux/usb/gadget.h
>@@ -132,6 +132,8 @@ struct usb_ep_ops {
>  *	value can sometimes be reduced (hardware allowing), according to
>  *      the endpoint descriptor used to configure the endpoint.
>  * @driver_data:for use by the gadget driver.
>+ * @bEndpointAddress: used to identify the endpoint when finding
>+ *	descriptor that matches connection speed
>  * @desc: endpoint descriptor.  This pointer is set before the endpoint is
>  *	enabled and remains valid until the endpoint is disabled.
>  *

Sebastian

RE: [PATCH/RESEND 3/5 v4] usb: Configure endpoint according to gadget speed.

[Tanya Brokhman]

> >+	/* find descriptors */
> >+	for (d_spd = next_ep_desc(speed_desc); d_spd;
> >+	      d_spd = next_ep_desc(d_spd+1)) {
> 
> this might look better if you would something like the list_for_each()
> macro.

The function next_ep_desc() goes over the endpoint descriptors (struct
usb_descriptor_header). It's not a list so I don't see how the
list_for_each() macro can be used here. Am I missing something?

> > /**
> >+ * config_ep_by_speed() - configures the given endpoint
> >+ * according to gadget speed.
> >+ * @g: pointer to the gadget
> >+ * @f: usb function
> >+ * @_ep: the endpoint to configure
> >+ *
> >+ * Return: error code, 0 on success
> >+ *
> >+ * This function chooses the right descriptors for a given
> >+ * endpoint according to gadget speed and saves in in the
> >+ * endpoint desc field. If the endpoint already has a descriptor
> >+ * assigned to it - overwrites it with currently corresponding
> >+ * descriptor. The endpoint maxpacket field is updated according
> >+ * to the choosen descriptor.
> >+ * Note: the supplied function should hold all the descriptors
> >+ * for supported speeds
> >+ */
> 
> You have the same comment (including the same typo) here and in the .c.
> Please use only on (in the .c file) as they will go async.

Thanks for noticing that! 
What do you mean by "they will go async"? Is there some sort of automation
that updates the comments for functions from c files to the headers? I'm not
familiar with such...


Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH/RESEND 3/5 v4] usb: Configure endpoint according to gadget speed.

[Michal Nazarewicz]

On Wed, 13 Apr 2011 11:56:27 +0200, Tanya Brokhman  
<tlinder@codeaurora.org> wrote:

>> >+	/* find descriptors */
>> >+	for (d_spd = next_ep_desc(speed_desc); d_spd;
>> >+	      d_spd = next_ep_desc(d_spd+1)) {
>>
>> this might look better if you would something like the list_for_each()
>> macro.
>
> The function next_ep_desc() goes over the endpoint descriptors (struct
> usb_descriptor_header). It's not a list so I don't see how the
> list_for_each() macro can be used here. Am I missing something?

What op[1] meant was to create a for_each_ep_desc() macro, eg. (not  
tested):

#define for_each_ep_desc(it, start) \
         for (it = (start); (it = next_ep_desc(it)); ++it)

>> > /**
>> >+ * config_ep_by_speed() - configures the given endpoint
>> >+ * according to gadget speed.
>> >+ * @g: pointer to the gadget
>> >+ * @f: usb function
>> >+ * @_ep: the endpoint to configure
>> >+ *
>> >+ * Return: error code, 0 on success
>> >+ *
>> >+ * This function chooses the right descriptors for a given
>> >+ * endpoint according to gadget speed and saves in in the
>> >+ * endpoint desc field. If the endpoint already has a descriptor
>> >+ * assigned to it - overwrites it with currently corresponding
>> >+ * descriptor. The endpoint maxpacket field is updated according
>> >+ * to the choosen descriptor.
>> >+ * Note: the supplied function should hold all the descriptors
>> >+ * for supported speeds
>> >+ */
>>
>> You have the same comment (including the same typo) here and in the .c.
>> Please use only on (in the .c file) as they will go async.
>
> Thanks for noticing that!
> What do you mean by "they will go async"? Is there some sort of  
> automation
> that updates the comments for functions from c files to the headers?

There's none.  That's the problem.  Someone will modify comment in one of  
them
and will forget to modify it in the other.  That's why it's better to keep
comments in one place only.

[1] Also, please leave the attribution lines (the “On ... someone wrote:”
     lines) in replies.

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