Re: [PATCH] USB:gadget:introduce isochronous source sink configuration driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Saturday 07 March 2009, tom.leiming@xxxxxxxxx wrote:
> From: Ming Lei <tom.leiming@xxxxxxxxx>
> 
> This patch introduces isochronous source sink configuration driver
> into zero composite driver. The driver is based on David Brownell's
> f_sourcesink.c

I like the idea, but this needs a few updates.

So ... NAK on this for now.


The biggest behavioral change would be to always use at least
double buffering ... there's no general way to ensure that the
FIFO gets reloaded quickly enough for the next ISO packet, so
it's best to avoid that issue.  The way you have it now might
work if the hardware has working double-buffering support, and
the peripheral isn't too busy; but for high bandwidth, it will
get fairly dicy.

There should also be a minor structural change to the config:
ISO endpoints may not reside in altsetting zero.  So you've
got to define another altsetting ... zero with no endpoints,
and the ones you have here would be alt #1, with endpoints.


> It can  be used to evalute isochronous transfer performance of usb
> host controller or usb host controller driver, also can be used to
> troubleshoot usb host controller driver(usually some embedded hcd)
> handily.
> 
> I have verified the isochronous configuration driver in omap3-based
> beagle board.

Which is an exceptionally functional bit of hardware.  ;)

This should work on a few more devices before it gets into
mainline.  Both high speed and full speed.  I can tell this
won't work on some common hardware already.


> 
> Signed-off-by: Ming Lei <tom.leiming@xxxxxxxxx>
> ---
>  drivers/usb/gadget/f_iso.c |  479 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/zero.c  |   25 ++-
>  2 files changed, 501 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/usb/gadget/f_iso.c
> 
> diff --git a/drivers/usb/gadget/f_iso.c b/drivers/usb/gadget/f_iso.c
> new file mode 100644
> index 0000000..18e7429
> --- /dev/null
> +++ b/drivers/usb/gadget/f_iso.c
> @@ -0,0 +1,479 @@
> +/*
> + * f_iso.c - USB peripheral isochronous source/sink configuration driver
> + * This driver is based on David Brownell's f_sourcesink.c.

You should add a "Copyright (C) 2009 Tom Leiming" here.
Without a copyright, the license can't apply, because it's
a grant from the copyright holder.  :)


> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +/* #define VERBOSE_DEBUG */
> +
> +#include <linux/kernel.h>
> +#include <linux/utsname.h>
> +#include <linux/device.h>
> +
> +#include "g_zero.h"
> +#include "gadget_chips.h"
> +
> +
> +#define  MULTI       (0<<11)

Please make the packet parameters be module parameters.

For high speed, a 3KB "packet" would imply MULTI=2 (3 packets
per uframe) and maxpacket=(3KB/3)=1024.  Not all high speed
UDCs support high bandwidth like that though.

For full speed, lots of hardware has odd restrictions.
Like maxpacket of 256 bytes (possibly configurable) or
lack of ISO support...

To some extent this will be a replacement for the gadgetfs
based test program ... which supports both packet size
controls and transfer rate parameters, sufficient to drive
24 MByte/sec for high bandwidth transfers.

ISTR it also supports the same for interrupt transfers...


> +
> +/*
> + * ISOCHRONOUS SOURCE/SINK FUNCTION ... a primary testing vehicle for
> + * USB peripheral controller drivers, it can also be used to evalute
> + * performance of usb host controller or usb host controller diver,
> + * and can be used to troubleshoot usb host controller driver handily.
> + *
> + * This just sinks isochronous packets OUT to the peripheral and sources
> + * them IN to the host. As such it supports basic functionality and
> + * load tests.
> + *
> + * In terms of control messaging, this supports all the standard requests
> + *
> + * This is currently packaged as a configuration driver, which can't be
> + * combined with other functions to make composite devices.  However, it
> + * can be combined with other independent configurations.
> + */
> +struct f_iso_sourcesink {
> +	struct usb_function	function;
> +
> +	struct usb_ep		*in_ep;
> +	struct usb_ep		*out_ep;
> +};
> +
> +static inline struct f_iso_sourcesink *func_to_iso_ss(struct usb_function *f)
> +{
> +	return container_of(f, struct f_iso_sourcesink, function);
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static struct usb_interface_descriptor iso_source_sink_intf = {
> +	.bLength =		sizeof iso_source_sink_intf,
> +	.bDescriptorType =	USB_DT_INTERFACE,
> +
> +	.bNumEndpoints =	2,

No ... altsetting zero must not have any ISO endpoints.

You need to have two altsettings:  #0 has no endpoints,
and #1 would have the endpoints you list here.

You could have *more* altsettings, at different data
rates, but for now I'd say "don't bother".


> +	.bInterfaceClass =	USB_CLASS_VENDOR_SPEC,
> +	/* .iInterface = DYNAMIC */
> +};
> +
> +/* full speed support: */
> +
> +static struct usb_endpoint_descriptor fs_iso_source_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT,
> +
> +	.bEndpointAddress =	USB_DIR_IN,
> +	.bmAttributes =		USB_ENDPOINT_XFER_ISOC,
> +	.wMaxPacketSize =	__constant_cpu_to_le16(1023) ,
> +	.bInterval	=	1,

Full speed packet size (1..1023) and interval should be module
parameters.  For full speed I'd use the same values for both IN
and OUT transfers, but not high speed (see below).


> +};
> +
> +static struct usb_endpoint_descriptor fs_iso_sink_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT,
> +
> +	.bEndpointAddress =	USB_DIR_OUT,
> +	.bmAttributes =		USB_ENDPOINT_XFER_ISOC,
> +	.wMaxPacketSize =	__constant_cpu_to_le16(1023) ,
> +	.bInterval	=	1,
> +};
> +
> +static struct usb_descriptor_header *fs_iso_source_sink_descs[] = {
> +	(struct usb_descriptor_header *) &iso_source_sink_intf,

Two different altsettings required ...


> +	(struct usb_descriptor_header *) &fs_iso_sink_desc,
> +	(struct usb_descriptor_header *) &fs_iso_source_desc,
> +	NULL,
> +};
> +
> +/* high speed support: */
> +static struct usb_endpoint_descriptor hs_iso_source_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT,
> +	.bEndpointAddress =	USB_DIR_IN,
> +
> +	.bmAttributes =		USB_ENDPOINT_XFER_ISOC,
> +	.wMaxPacketSize =	cpu_to_le16(1024|MULTI) ,
> +	.bInterval	=	1,

Likewise highspeed packet size (1..3KB) and interval
should be module parameters.  In this case it'd be
important to let them work at different rates.

For example, on the Beagle you used to test with, the
patch supporting high bandwidth ISO only configures
one endpoint with enough buffer space (4KB) to handle
a 24 MByte/sec data rate.


> +};
> +
> +static struct usb_endpoint_descriptor hs_iso_sink_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT,
> +	.bEndpointAddress =	USB_DIR_OUT,
> +
> +	.bmAttributes =		USB_ENDPOINT_XFER_ISOC,
> +	.wMaxPacketSize =	cpu_to_le16(1024|MULTI),
> +	.bInterval	=	1,
> +};
> +
> +static struct usb_descriptor_header *hs_iso_source_sink_descs[] = {
> +	(struct usb_descriptor_header *) &iso_source_sink_intf,

Two different altsettings required ...


> +	(struct usb_descriptor_header *) &hs_iso_source_desc,
> +	(struct usb_descriptor_header *) &hs_iso_sink_desc,
> +	NULL,
> +};
> +
> +/* function-specific strings: */
> +static struct usb_string strings_iso_sourcesink[] = {
> +	[0].s = "iso source and sink data",
> +	{  }			/* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_iso_sourcesink = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_iso_sourcesink,
> +};
> +
> +static struct usb_gadget_strings *iso_sourcesink_strings[] = {
> +	&stringtab_iso_sourcesink,
> +	NULL,
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +static int __init
> +iso_sourcesink_bind(struct usb_configuration *c, struct usb_function *f)
> +{
> +	struct usb_composite_dev *cdev = c->cdev;
> +	struct f_iso_sourcesink	*ss = func_to_iso_ss(f);
> +	int	id;
> +
> +	/* allocate interface ID(s) */
> +	id = usb_interface_id(c, f);
> +	if (id < 0)
> +		return id;
> +	iso_source_sink_intf.bInterfaceNumber = id;

You'll have to set that for both interfaces.


> +
> +	/* allocate endpoints */
> +	ss->in_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_source_desc);

Hmm, I seem to recall expecting that high speed endpoints
would get configured first here.  For ISO, it can matter
a lot ... needing 2KB or 3KB of buffer space (for high
bandwidth) will be awkward.


> +	if (!ss->in_ep) {
> +autoconf_fail:
> +		ERROR(cdev, "%s: can't autoconfigure on %s\n",
> +			f->name, cdev->gadget->name);
> +		return -ENODEV;
> +	}
> +	ss->in_ep->driver_data = cdev;	/* claim */
> +
> +	ss->out_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_sink_desc);
> +	if (!ss->out_ep)
> +		goto autoconf_fail;
> +	ss->out_ep->driver_data = cdev;	/* claim */
> +
> +	/* support high speed hardware */
> +	if (gadget_is_dualspeed(c->cdev->gadget)) {
> +		hs_iso_source_desc.bEndpointAddress =
> +				fs_iso_source_desc.bEndpointAddress;
> +		hs_iso_sink_desc.bEndpointAddress =
> +				fs_iso_sink_desc.bEndpointAddress;
> +		f->hs_descriptors = hs_iso_source_sink_descs;
> +	}
> +
> +	DBG(cdev, "%s speed %s: IN/%s, OUT/%s\n",
> +			gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full",
> +			f->name, ss->in_ep->name, ss->out_ep->name);

For ISO I'd also dump the data rate info:  interval and data size
for each supported speed and direction.


> +	return 0;
> +}
> +
> +static void
> +iso_sourcesink_unbind(struct usb_configuration *c, struct usb_function *f)
> +{
> +	kfree(func_to_iso_ss(f));
> +}
> +
> +static unsigned char iso_count;
> +static void iso_reinit_write_data(struct usb_ep *ep, struct usb_request *req)
> +{
> +	memset(req->buf, iso_count++, req->length);
> +}
> +
> +static void iso_source_sink_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_iso_sourcesink	*ss = ep->driver_data;
> +	struct usb_composite_dev *cdev = ss->function.config->cdev;
> +	int			status = req->status;
> +
> +	switch (status) {
> +
> +	case 0:				/* normal completion? */

You're not looking at the RX data here.  So there's no point
in the memset.  And the TX data isn't going to change either;
so there's no point in re-initializing it.

Likewise "usbtest" cases 15 (ISO read) and 16 (ISO write)
don't touch the ISO data (not entirely a feature) ... so
I'd just drop this if/else entirely.


> +		if (ep == ss->out_ep)
> +			memset(req->buf, 0x55, req->length);
> +		else
> +			iso_reinit_write_data(ep, req);
> +		break;
> +
> +	/* this endpoint is normally active while we're configured */
> +	case -ECONNABORTED:		/* hardware forced ep reset */
> +	case -ECONNRESET:		/* request dequeued */
> +	case -ESHUTDOWN:		/* disconnect from host */
> +		VDBG(cdev, "%s gone (%d), %d/%d\n", ep->name, status,
> +				req->actual, req->length);
> +		free_ep_req(ep, req);
> +		return;
> +
> +	case -EOVERFLOW:		/* buffer overrun on read means that
> +					 * we didn't provide a big enough
> +					 * buffer.
> +					 */
> +	default:
> +#if 1
> +		DBG(cdev, "%s complete --> %d, %d/%d\n", ep->name,
> +				status, req->actual, req->length);
> +#endif
> +	case -EREMOTEIO:		/* short read */
> +		break;
> +	}
> +
> +	status = usb_ep_queue(ep, req, GFP_ATOMIC);
> +	if (status) {
> +		ERROR(cdev, "kill %s:  resubmit %d bytes --> %d\n",
> +				ep->name, req->length, status);
> +		usb_ep_set_halt(ep);

ISO endpoints can't halt.  :)


> +		/* FIXME recover later ... somehow */
> +	}
> +}
> +
> +static int iso_source_sink_start_ep(struct f_iso_sourcesink *ss, bool is_in)
> +{
> +	struct usb_ep		*ep;
> +	struct usb_request	*req;
> +	int			status;
> +
> +	ep = is_in ? ss->in_ep : ss->out_ep;

This should loop twice ... hardware typically will support
double buffering (ping/pong) but not much more.


> +	req = alloc_ep_req(ep);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	req->complete = iso_source_sink_complete;
> +	if (is_in)
> +		iso_reinit_write_data(ep, req);
> +	else
> +		memset(req->buf, 0x55, req->length);

See above.  No point in that if/else for now.

> +
> +	status = usb_ep_queue(ep, req, GFP_ATOMIC);
> +	if (status) {
> +		struct usb_composite_dev	*cdev;
> +
> +		cdev = ss->function.config->cdev;
> +		ERROR(cdev, "start %s %s --> %d\n",
> +				is_in ? "IN" : "OUT",
> +				ep->name, status);
> +		free_ep_req(ep, req);
> +	}
> +
> +	return status;
> +}
> +
> +static void disable_iso_source_sink(struct f_iso_sourcesink *ss)
> +{
> +	struct usb_composite_dev	*cdev;
> +
> +	cdev = ss->function.config->cdev;
> +	disable_endpoints(cdev, ss->in_ep, ss->out_ep);
> +	VDBG(cdev, "%s disabled\n", ss->function.name);
> +}
> +
> +static int
> +enable_iso_source_sink(struct usb_composite_dev *cdev, \

No need for that backslash; it's just normal whitespace.


> +		       struct f_iso_sourcesink *ss)
> +{
> +	int					result = 0;
> +	const struct usb_endpoint_descriptor	*src, *sink;
> +	struct usb_ep				*ep;
> +
> +	src = ep_choose(cdev->gadget, &hs_iso_source_desc, &fs_iso_source_desc);
> +	sink = ep_choose(cdev->gadget, &hs_iso_sink_desc, &fs_iso_sink_desc);
> +
> +	/* one endpoint writes (sources) zeroes IN (to the host) */
> +	ep = ss->in_ep;
> +	result = usb_ep_enable(ep, src);
> +	if (result < 0)
> +		return result;
> +
> +	ep->driver_data = ss;
> +
> +	result = iso_source_sink_start_ep(ss, true);
> +	if (result < 0) {
> +fail:
> +		ep = ss->in_ep;
> +		usb_ep_disable(ep);
> +		ep->driver_data = NULL;
> +		return result;
> +	}
> +
> +	/* one endpoint reads (sinks) anything OUT (from the host) */
> +	ep = ss->out_ep;
> +	result = usb_ep_enable(ep, sink);
> +	if (result < 0)
> +		goto fail;
> +	ep->driver_data = ss;
> +
> +	result = iso_source_sink_start_ep(ss, false);
> +	if (result < 0) {
> +		usb_ep_disable(ep);
> +		ep->driver_data = NULL;
> +		goto fail;
> +	}
> +
> +	DBG(cdev, "%s enabled\n", ss->function.name);
> +	return result;
> +}
> +
> +static int iso_sourcesink_set_alt(struct usb_function *f,
> +		unsigned intf, unsigned alt)
> +{
> +	struct f_iso_sourcesink	*ss = func_to_iso_ss(f);
> +	struct usb_composite_dev *cdev = f->config->cdev;
> +
> +	/* we know alt is zero */
> +	if (ss->in_ep->driver_data)
> +		disable_iso_source_sink(ss);
> +	return enable_iso_source_sink(cdev, ss);

This is wrong.  See above ... as described in the USB 2.0 spec,
ISO endpoints don't belong in altsetting zero.

So instead:

	switch (alt) {
	case 0:
		return 0;
	case 1:
		return enable(...);
	default:
		return -EINVAL;
	}



> +}
> +
> +static void iso_sourcesink_disable(struct usb_function *f)
> +{
> +	struct f_iso_sourcesink	*ss = func_to_iso_ss(f);
> +
> +	disable_iso_source_sink(ss);
> +}
> +
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int __init iso_sourcesink_bind_config(struct usb_configuration *c)
> +{
> +	struct f_iso_sourcesink	*ss;
> +	int			status;
> +
> +	ss = kzalloc(sizeof *ss, GFP_KERNEL);
> +	if (!ss)
> +		return -ENOMEM;
> +
> +	ss->function.name = "iso source/sink";
> +	ss->function.descriptors = fs_iso_source_sink_descs;
> +	ss->function.bind = iso_sourcesink_bind;
> +	ss->function.unbind = iso_sourcesink_unbind;
> +	ss->function.set_alt = iso_sourcesink_set_alt;
> +	ss->function.disable = iso_sourcesink_disable;
> +
> +	status = usb_add_function(c, &ss->function);
> +	if (status)
> +		kfree(ss);
> +	return status;
> +}
> +
> +static int iso_sourcesink_setup(struct usb_configuration *c,
> +		const struct usb_ctrlrequest *ctrl)
> +{
> +	struct usb_request	*req = c->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
> +	 * the two control test requests.
> +	 */

I'd just strike this entire setup() routine ... it's enough
to have it in the bulk source/sink configuration.  No point
in cloning that code here, and possibly letting bugs grow.


> +	switch (ctrl->bRequest) {
> +
> +	/*
> +	 * These are the same vendor-specific requests supported by
> +	 * Intel's USB 2.0 compliance test devices.  We exceed that
> +	 * device spec by allowing multiple-packet requests.
> +	 *
> +	 * NOTE:  the Control-OUT data stays in req->buf ... better
> +	 * would be copying it into a scratch buffer, so that other
> +	 * requests may safely intervene.
> +	 */
> +	case 0x5b:	/* control WRITE test -- fill the buffer */
> +		if (ctrl->bRequestType != (USB_DIR_OUT|USB_TYPE_VENDOR))
> +			goto unknown;
> +		if (w_value || w_index)
> +			break;
> +		/* just read that many bytes into the buffer */
> +		if (w_length > req->length)
> +			break;
> +		value = w_length;
> +		break;
> +	case 0x5c:	/* control READ test -- return the buffer */
> +		if (ctrl->bRequestType != (USB_DIR_IN|USB_TYPE_VENDOR))
> +			goto unknown;
> +		if (w_value || w_index)
> +			break;
> +		/* expect those bytes are still in the buffer; send back */
> +		if (w_length > req->length)
> +			break;
> +		value = w_length;
> +		break;
> +
> +	default:
> +unknown:
> +		VDBG(c->cdev,
> +			"unknown 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) {
> +		VDBG(c->cdev, "source/sink 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(c->cdev->gadget->ep0, req, GFP_ATOMIC);
> +		if (value < 0)
> +			ERROR(c->cdev, "source/sinkc response, err %d\n",
> +					value);
> +	}
> +
> +	/* device either stalls (value < 0) or reports success */
> +	return value;
> +}
> +
> +static struct usb_configuration iso_sourcesink_driver = {
> +	.label		= "iso source/sink",
> +	.strings	= iso_sourcesink_strings,
> +	.bind		= iso_sourcesink_bind_config,
> +	.setup		= iso_sourcesink_setup,
> +	.bConfigurationValue = 4,
> +	.bmAttributes	= USB_CONFIG_ATT_SELFPOWER,
> +	/* .iConfiguration = DYNAMIC */
> +};
> +
> +/**
> + * sourcesink_add - add a source/sink testing configuration to a device
> + * @cdev: the device to support the configuration
> + */
> +int __init iso_sourcesink_add(struct usb_composite_dev *cdev)
> +{
> +	int id;
> +
> +	/* allocate string ID(s) */
> +	id = usb_string_id(cdev);
> +	if (id < 0)
> +		return id;
> +	strings_iso_sourcesink[0].id = id;
> +
> +	iso_source_sink_intf.iInterface = id;
> +	iso_sourcesink_driver.iConfiguration = id;
> +
> +	/* support OTG systems */
> +	if (gadget_is_otg(cdev->gadget)) {
> +		iso_sourcesink_driver.descriptors = otg_desc;
> +		iso_sourcesink_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;
> +	}
> +
> +	return usb_add_config(cdev, &iso_sourcesink_driver);
> +}
> diff --git a/drivers/usb/gadget/zero.c b/drivers/usb/gadget/zero.c
> index 20614dc..db2a7b5 100644
> --- a/drivers/usb/gadget/zero.c
> +++ b/drivers/usb/gadget/zero.c
> @@ -71,6 +71,7 @@
>  #include "config.c"
>  #include "epautoconf.c"
>  
> +#include "f_iso.c"
>  #include "f_sourcesink.c"
>  #include "f_loopback.c"
>  
> @@ -92,6 +93,14 @@ module_param(buflen, uint, 0);
>  static int loopdefault = 0;
>  module_param(loopdefault, bool, S_IRUGO|S_IWUSR);
>  
> +/*
> + * Normally the "iso" configuration is third (index 2) so
> + * it's not the default.  Here's where to change that order, to
> + * work better with hosts where config changes are problematic or
> + * controllers (like original superh) that only support one config.
> + */
> +static int isodefault;
> +module_param(isodefault, bool, S_IRUGO|S_IWUSR);

Please preserve the blank line here...


>  /*-------------------------------------------------------------------------*/
>  
>  /* Thanks to NetChip Technologies for donating this product ID.
> @@ -118,7 +127,7 @@ static struct usb_device_descriptor device_desc = {
>  
>  	.idVendor =		cpu_to_le16(DRIVER_VENDOR_NUM),
>  	.idProduct =		cpu_to_le16(DRIVER_PRODUCT_NUM),
> -	.bNumConfigurations =	2,
> +	.bNumConfigurations =	3,
>  };
>  
>  #ifdef CONFIG_USB_OTG
> @@ -244,12 +253,22 @@ static int __init zero_bind(struct usb_composite_dev *cdev)
>  	 */
>  	if (loopdefault) {
>  		loopback_add(cdev);
> -		if (!gadget_is_sh(gadget))
> +		if (!gadget_is_sh(gadget)) {
>  			sourcesink_add(cdev);
> +			iso_sourcesink_add(cdev);

You shouldn't assume all hardware can support ISO properly

In particular, if gadget_is_pxa() don't try to enable it,
and report an error if someone tries to set isodefault ...
it doesn't support altsettings other  than zero (sigh).
And may not support the configuration number you use...

For other controllers, the lack of support for ISO should
become apparent politely when iso_sourcesink_add() fails.

Although ... none of these calls actually check the return
values from those add() calls; sigh.


> +		}
> +	} else if (isodefault) {
> +		iso_sourcesink_add(cdev);
> +		if (!gadget_is_sh(gadget)) {
> +			sourcesink_add(cdev);
> +			loopback_add(cdev);
> +		}
>  	} else {
>  		sourcesink_add(cdev);
> -		if (!gadget_is_sh(gadget))
> +		if (!gadget_is_sh(gadget)) {
>  			loopback_add(cdev);
> +			iso_sourcesink_add(cdev);
> +		}
>  	}
>  
>  	gcnum = usb_gadget_controller_number(gadget);
> -- 
> 1.6.0.GIT
> 
> 


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

[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux