2010/9/20 Martin Fuzzey <mfuzzey@xxxxxxxxx>:
> Hi,
Hi,
Great thanks for your test and comments.
>
> tom.leiming@xxxxxxxxx wrote:
>> From: Ming Lei <tom.leiming@xxxxxxxxx>
>>
>> This patch introduces isochronous source sink function driver
>> into zero composite driver. The driver is based on David Brownell's
>> f_sourcesink.c
>>
>
>> +/*-------------------------------------------------------------------------*/
>> +static int __init
>> +iso_sourcesink_bind(struct usb_configuration *c, struct usb_function *f)
>> + if (gadget_is_dualspeed(c->cdev->gadget))
>> + hs = 1;
>> + else
>> + hs = 0;
>> +
>> + /*use the ep desc with the strongest capability to configure hw ep*/
>> + if (hs) {
>> + ialt = ss->hs_alt;
>> + max_in = ialt[(int)ss->hs_in_maxp_alt].source_ep_desc;
>> + max_out = ialt[(int)ss->hs_out_maxp_alt].sink_ep_desc;
>> + } else {
>> + ialt = ss->fs_alt;
>> + max_in = ialt[(int)ss->fs_in_maxp_alt].source_ep_desc;
>> + max_out = ialt[(int)ss->fs_out_maxp_alt].sink_ep_desc;
>> + }
>> +
>>
> Have you tested this on dual speed gadget hardware connected to a full
> speed only host?
Yes, of cource. If the capability of all highspeed endpoints is stronger than
that of fullspeed endpoint(see the comments), there is no problem.
But seems the code snippet above is not very good.
> To get that configuration to work for me I had to force hs=0 in the
> above code.
No, you don't need to do it.
>
> Otherwise using a configuration like
> fs:alt0:alt1,in/512/1,out/512/1:hs:alt0:alt1,in/1024*3/1:alt2,in/1024/1
> (note fs has in and out whereas hs has in only)
> causes an oops in enable_iso_source_sink() because one of the endpoints
> is null.
Yes, there is a problem in your case, since the max packet size of highspeed
out endpoint is not stronger than that of fullspeed endpoint.
>
> The problem seems to be that iso_sourcesink_bind() sets up ss->in_ep and
> ss->out_ep based
> on the configuration for the gadget hardware regardless of the host
> capabilities.
The issue is same with above your case.
>
>
>> +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;
>> + if (ep)
>> + return 0;
>>
>>
> Shouldn"t this be
> if (!ep)
> return 0;
> ??
OOPS, it is really a bug.
>
> Otherwise no requests are actually submitted.
> As usbtest #16 doesn't actually check the length
> of data transfered the test is still OK but nothing
> is transferred...
>
>
> I also have another "strange" behaviour but that may be due to my
> misunderstanding:
> When I configure different max packet sizes for IN transfers and observe
> the data size
> on the bus (with a bus analyser) I see:
>
> config bus
> 256 256
> 512 512
> 248 256
> 1020 1024
> 1023 1024
How do you trigger the IN transfer in usb host? usb test #16?
If the usb device does response packets which size is larger than
the max packet size in endpoint descriptor, you may meet overflow
error. If you don't see this kind of error in usb hcd, seems the data
size from your bus analyser is not accurate.
Follows the patch, which can fix your two issues and is against v1.
diff --git a/drivers/usb/gadget/f_iso.c b/drivers/usb/gadget/f_iso.c
index 76830d6..5bfe91b 100644
--- a/drivers/usb/gadget/f_iso.c
+++ b/drivers/usb/gadget/f_iso.c
@@ -521,6 +521,8 @@ iso_sourcesink_bind(struct usb_configuration *c,
struct usb_function *f)
struct iso_alt *ialt;
struct usb_endpoint_descriptor *max_in;
struct usb_endpoint_descriptor *max_out;
+ struct usb_endpoint_descriptor *fs_max_in;
+ struct usb_endpoint_descriptor *fs_max_out;
const char *in_name, *out_name;
/* allocate interface ID(s) */
@@ -545,15 +547,16 @@ iso_sourcesink_bind(struct usb_configuration *c,
struct usb_function *f)
hs = 0;
/*use the ep desc with the strongest capability to configure hw ep*/
- if (hs) {
- ialt = ss->hs_alt;
- max_in = ialt[(int)ss->hs_in_maxp_alt].source_ep_desc;
- max_out = ialt[(int)ss->hs_out_maxp_alt].sink_ep_desc;
- } else {
- ialt = ss->fs_alt;
- max_in = ialt[(int)ss->fs_in_maxp_alt].source_ep_desc;
- max_out = ialt[(int)ss->fs_out_maxp_alt].sink_ep_desc;
- }
+ ialt = ss->hs_alt;
+ max_in = ialt[(int)ss->hs_in_maxp_alt].source_ep_desc;
+ max_out = ialt[(int)ss->hs_out_maxp_alt].sink_ep_desc;
+
+ ialt = ss->fs_alt;
+ fs_max_in = ialt[(int)ss->fs_in_maxp_alt].source_ep_desc;
+ fs_max_out = ialt[(int)ss->fs_out_maxp_alt].sink_ep_desc;
+
+ max_in = max_in ? : fs_max_in;
+ max_out = max_out ? : fs_max_out;
if (!max_in)
goto out;
@@ -648,7 +651,7 @@ static int iso_source_sink_start_ep(struct
f_iso_sourcesink *ss, bool is_in)
int status;
ep = is_in ? ss->in_ep : ss->out_ep;
- if (ep)
+ if (!ep)
return 0;
req = alloc_ep_req(ep);
thanks,
--
Lei Ming
--
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
