Hi David,
On Friday 19 June 2009 14:28:14 David Brownell wrote:
> On Friday 08 May 2009, Laurent Pinchart wrote:
> > Hi everybody,
> >
> > the USB composite gadget framework currently handles EP0 control requests
> > sent to endpoints (bRequestType[4..0] == USB_RECIP_ENDPOINT) down to the
> > current configuration EP0 request handler. Composite drivers thus need to
> > parse the request and call the appropriate function handlers, making
> > functions not self- contained.
> >
> > It should be possible to maintain an association between functions and
> > endpoints for each configuration, much like we already maintain an
> > association between functions and interfaces.
>
> Ideally something less pricey than an array of function pointers,
> indexed by endpoint ... :)
>
> A per-function bitmap would suffice, and it's not like those calls
> are on critical paths. Though I suppose the code to walk those
> bitmaps is likely to take as much space as the array-of-pointers. ;)
As the USB specification limits the number of endpoints to 32 (16 IN and
16 OUT), this would cost 128 bytes on a 32 bit architecture (256 bytes on a
64 bit architecture). The bitmap would take 4 (8) bytes, but would need to be
stored in the function instead of the configuration. If we assume the device
has two functions, function pointers have a data overhead of 120 (240) bytes.
Given the amount of code required to walk the bitmaps, this might indeed be an
overkill, but here you are :-) Could you please review the following patch
(based on 2.6.31-rc1) ?
diff --git a/drivers/usb/gadget/audio.c b/drivers/usb/gadget/audio.c
index ad7ed55..bdad613 100644
--- a/drivers/usb/gadget/audio.c
+++ b/drivers/usb/gadget/audio.c
@@ -89,120 +89,6 @@ static const struct usb_descriptor_header *otg_desc[] = {
/*-------------------------------------------------------------------------*/
-/**
- * Handle USB audio endpoint set/get command in setup class request
- */
-
-static int audio_set_endpoint_req(struct usb_configuration *c,
- const struct usb_ctrlrequest *ctrl)
-{
- struct usb_composite_dev *cdev = c->cdev;
- int value = -EOPNOTSUPP;
- u16 ep = le16_to_cpu(ctrl->wIndex);
- u16 len = le16_to_cpu(ctrl->wLength);
- u16 w_value = le16_to_cpu(ctrl->wValue);
-
- DBG(cdev, "bRequest 0x%x, w_value 0x%04x, len %d, endpoint %d\n",
- ctrl->bRequest, w_value, len, ep);
-
- switch (ctrl->bRequest) {
- case UAC_SET_CUR:
- value = 0;
- break;
-
- case UAC_SET_MIN:
- break;
-
- case UAC_SET_MAX:
- break;
-
- case UAC_SET_RES:
- break;
-
- case UAC_SET_MEM:
- break;
-
- default:
- break;
- }
-
- return value;
-}
-
-static int audio_get_endpoint_req(struct usb_configuration *c,
- const struct usb_ctrlrequest *ctrl)
-{
- struct usb_composite_dev *cdev = c->cdev;
- int value = -EOPNOTSUPP;
- u8 ep = ((le16_to_cpu(ctrl->wIndex) >> 8) & 0xFF);
- u16 len = le16_to_cpu(ctrl->wLength);
- u16 w_value = le16_to_cpu(ctrl->wValue);
-
- DBG(cdev, "bRequest 0x%x, w_value 0x%04x, len %d, endpoint %d\n",
- ctrl->bRequest, w_value, len, ep);
-
- switch (ctrl->bRequest) {
- case UAC_GET_CUR:
- case UAC_GET_MIN:
- case UAC_GET_MAX:
- case UAC_GET_RES:
- value = 3;
- break;
- case UAC_GET_MEM:
- break;
- default:
- break;
- }
-
- return value;
-}
-
-static int
-audio_setup(struct usb_configuration *c, const struct usb_ctrlrequest *ctrl)
-{
- struct usb_composite_dev *cdev = c->cdev;
- struct usb_request *req = cdev->req;
- int value = -EOPNOTSUPP;
- u16 w_index = le16_to_cpu(ctrl->wIndex);
- u16 w_value = le16_to_cpu(ctrl->wValue);
- u16 w_length = le16_to_cpu(ctrl->wLength);
-
- /* composite driver infrastructure handles everything except
- * Audio class messages; interface activation uses set_alt().
- */
- switch (ctrl->bRequestType) {
- case USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_ENDPOINT:
- value = audio_set_endpoint_req(c, ctrl);
- break;
-
- case USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT:
- value = audio_get_endpoint_req(c, ctrl);
- break;
-
- default:
- ERROR(cdev, "Invalid control req%02x.%02x v%04x i%04x l%d\n",
- ctrl->bRequestType, ctrl->bRequest,
- w_value, w_index, w_length);
- }
-
- /* respond with data transfer or status phase? */
- if (value >= 0) {
- DBG(cdev, "Audio req%02x.%02x v%04x i%04x l%d\n",
- ctrl->bRequestType, ctrl->bRequest,
- w_value, w_index, w_length);
- req->zero = 0;
- req->length = value;
- value = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
- if (value < 0)
- ERROR(cdev, "Audio response on err %d\n", value);
- }
-
- /* device either stalls (value < 0) or reports success */
- return value;
-}
-
-/*-------------------------------------------------------------------------*/
-
static int __init audio_do_config(struct usb_configuration *c)
{
/* FIXME alloc iConfiguration string, set it in c->strings */
@@ -220,7 +106,6 @@ static int __init audio_do_config(struct usb_configuration *c)
static struct usb_configuration audio_config_driver = {
.label = DRIVER_DESC,
.bind = audio_do_config,
- .setup = audio_setup,
.bConfigurationValue = 1,
/* .iConfiguration = DYNAMIC */
.bmAttributes = USB_CONFIG_ATT_SELFPOWER,
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 59e8523..897c2ed 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -688,6 +688,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
u16 w_value = le16_to_cpu(ctrl->wValue);
u16 w_length = le16_to_cpu(ctrl->wLength);
struct usb_function *f = NULL;
+ u8 endp;
/* partial re-init of the response message; the function or the
* gadget might need to intercept e.g. a control-OUT completion
@@ -800,23 +801,33 @@ unknown:
ctrl->bRequestType, ctrl->bRequest,
w_value, w_index, w_length);
- /* functions always handle their interfaces ... punt other
- * recipients (endpoint, other, WUSB, ...) to the current
+ /* functions always handle their interfaces and endpoints...
+ * punt other recipients (other, WUSB, ...) to the current
* configuration code.
*
* REVISIT it could make sense to let the composite device
* take such requests too, if that's ever needed: to work
* in config 0, etc.
*/
- if ((ctrl->bRequestType & USB_RECIP_MASK)
- == USB_RECIP_INTERFACE) {
+ switch (ctrl->bRequestType & USB_RECIP_MASK) {
+ case USB_RECIP_INTERFACE:
f = cdev->config->interface[intf];
- if (f && f->setup)
- value = f->setup(f, ctrl);
- else
+ break;
+
+ case USB_RECIP_ENDPOINT:
+ endp = ((w_index & 0x80) >> 3) | (w_index & 0x0f);
+ list_for_each_entry(f, &cdev->config->functions, list) {
+ if (test_bit(endp, f->endpoints))
+ break;
+ }
+ if (&f->list == &cdev->config->functions)
f = NULL;
+ break;
}
- if (value < 0 && !f) {
+
+ if (f && f->setup)
+ value = f->setup(f, ctrl);
+ else {
struct usb_configuration *c;
c = cdev->config;
diff --git a/drivers/usb/gadget/f_audio.c b/drivers/usb/gadget/f_audio.c
index 98e9bb9..6ebca2a 100644
--- a/drivers/usb/gadget/f_audio.c
+++ b/drivers/usb/gadget/f_audio.c
@@ -445,6 +445,70 @@ static int audio_get_intf_req(struct usb_function *f,
return len;
}
+static int audio_set_endpoint_req(struct usb_function *f,
+ const struct usb_ctrlrequest *ctrl)
+{
+ struct usb_composite_dev *cdev = f->config->cdev;
+ int value = -EOPNOTSUPP;
+ u16 ep = le16_to_cpu(ctrl->wIndex);
+ u16 len = le16_to_cpu(ctrl->wLength);
+ u16 w_value = le16_to_cpu(ctrl->wValue);
+
+ DBG(cdev, "bRequest 0x%x, w_value 0x%04x, len %d, endpoint %d\n",
+ ctrl->bRequest, w_value, len, ep);
+
+ switch (ctrl->bRequest) {
+ case UAC_SET_CUR:
+ value = 0;
+ break;
+
+ case UAC_SET_MIN:
+ break;
+
+ case UAC_SET_MAX:
+ break;
+
+ case UAC_SET_RES:
+ break;
+
+ case UAC_SET_MEM:
+ break;
+
+ default:
+ break;
+ }
+
+ return value;
+}
+
+static int audio_get_endpoint_req(struct usb_function *f,
+ const struct usb_ctrlrequest *ctrl)
+{
+ struct usb_composite_dev *cdev = f->config->cdev;
+ int value = -EOPNOTSUPP;
+ u8 ep = ((le16_to_cpu(ctrl->wIndex) >> 8) & 0xFF);
+ u16 len = le16_to_cpu(ctrl->wLength);
+ u16 w_value = le16_to_cpu(ctrl->wValue);
+
+ DBG(cdev, "bRequest 0x%x, w_value 0x%04x, len %d, endpoint %d\n",
+ ctrl->bRequest, w_value, len, ep);
+
+ switch (ctrl->bRequest) {
+ case UAC_GET_CUR:
+ case UAC_GET_MIN:
+ case UAC_GET_MAX:
+ case UAC_GET_RES:
+ value = 3;
+ break;
+ case UAC_GET_MEM:
+ break;
+ default:
+ break;
+ }
+
+ return value;
+}
+
static int
f_audio_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
@@ -455,7 +519,7 @@ f_audio_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
u16 w_value = le16_to_cpu(ctrl->wValue);
u16 w_length = le16_to_cpu(ctrl->wLength);
- /* composite driver infrastructure handles everything except
+ /* composite driver infrastructure handles everything;
* Audio class messages; interface activation uses set_alt().
*/
switch (ctrl->bRequestType) {
@@ -467,6 +531,14 @@ f_audio_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
value = audio_get_intf_req(f, ctrl);
break;
+ case USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_ENDPOINT:
+ value = audio_set_endpoint_req(f, ctrl);
+ break;
+
+ case USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT:
+ value = audio_get_endpoint_req(f, ctrl);
+ break;
+
default:
ERROR(cdev, "invalid control req%02x.%02x v%04x i%04x l%d\n",
ctrl->bRequestType, ctrl->bRequest,
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 4f6bb3d..738ea1a 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -127,6 +127,7 @@ struct usb_function {
/* private: */
/* internals */
struct list_head list;
+ DECLARE_BITMAP(endpoints, 32);
};
int usb_add_function(struct usb_configuration *, struct usb_function *);
Best regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
