Re: [Patch V3 2/2] usbtest: Add interrupt EP testcases

2014-05-27 Thread Amit Virdi

On 5/23/2014 8:14 PM, Daniele Forsi wrote:

2014-05-23 8:32 GMT+02:00 Amit Virdi:


@@ -124,6 +130,9 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface 
*intf)
 switch (usb_endpoint_type(&e->desc)) {
 case USB_ENDPOINT_XFER_BULK:
 break;
+   case USB_ENDPOINT_XFER_INT:
+   if (dev->info->intr)
+   goto try_intr;
 case USB_ENDPOINT_XFER_ISOC:
 if (dev->info->iso)
 goto try_iso;
@@ -139,6 +148,15 @@ get_endpoints(struct usbtest_dev *dev, struct 
usb_interface *intf)


I don't think you really mean to fall through to case
USB_ENDPOINT_XFER_ISOC if the test is false, but the logic of that


I do mean to fall through to check whether the EP is ISOC type if it 
isn't INTERRUPT.



for-loop is becoming harder to follow

in pseudo code, the switch statement is like this?
case USB_ENDPOINT_XFER_BULK:
   set in or out;
   break;
case USB_ENDPOINT_XFER_INT:
   set int_in or int_out;
   break;
case USB_ENDPOINT_XFER_ISOC:
   set iso_in or iso_out;
   break;
default:
  do nothing;

it would be easier to follow even if it adds and indentation level



I do agree that the goto statements don't look visually pleasing here, 
but the approach you suggested would end up in upto 7 levels of 
indentation and that would make the code look even more ugly (see below 
sample code snippet)


---
for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
unsigned ep;
.
.
.

for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
struct usb_host_endpoint*e;
e = alt->endpoint + ep;
switch (usb_endpoint_type(&e->desc)) {
.
.
.

  case USB_ENDPOINT_XFER_INT:
 if (dev->info->intr) {
 if usb_endpoint_dir_in(&e->desc) {
 if (!int_in)
 int_in = e;
 }
 else {
 if (!int_out)
 int_out = e;
 }
 }
 break;
.
.
.
.

---
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Patch V3 2/2] usbtest: Add interrupt EP testcases

2014-05-23 Thread Daniele Forsi
2014-05-23 8:32 GMT+02:00 Amit Virdi:

> @@ -124,6 +130,9 @@ get_endpoints(struct usbtest_dev *dev, struct 
> usb_interface *intf)
> switch (usb_endpoint_type(&e->desc)) {
> case USB_ENDPOINT_XFER_BULK:
> break;
> +   case USB_ENDPOINT_XFER_INT:
> +   if (dev->info->intr)
> +   goto try_intr;
> case USB_ENDPOINT_XFER_ISOC:
> if (dev->info->iso)
> goto try_iso;
> @@ -139,6 +148,15 @@ get_endpoints(struct usbtest_dev *dev, struct 
> usb_interface *intf)

I don't think you really mean to fall through to case
USB_ENDPOINT_XFER_ISOC if the test is false, but the logic of that
for-loop is becoming harder to follow

in pseudo code, the switch statement is like this?
case USB_ENDPOINT_XFER_BULK:
  set in or out;
  break;
case USB_ENDPOINT_XFER_INT:
  set int_in or int_out;
  break;
case USB_ENDPOINT_XFER_ISOC:
  set iso_in or iso_out;
  break;
default:
 do nothing;

it would be easier to follow even if it adds and indentation level
-- 
Daniele Forsi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Patch V3 2/2] usbtest: Add interrupt EP testcases

2014-05-22 Thread Amit Virdi
Two simple test cases for interrupt endpoints are added to the usbtest.c file.
These are simple non-queued interrupt IN and interrupt OUT transfers. Currently,
only gadget zero is capable of executing the interrupt EP test cases. However,
extending the same to other gadgets is extremely simple and can be done
on-demand.

This code has been tested only with gadget zero and care has been taken so as to
not break the existing functionality. However, if anyone can test with other
gadgets then that would be great!

Signed-off-by: Amit Virdi 
---
 drivers/usb/misc/usbtest.c | 113 +++--
 1 file changed, 98 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index f6568b5..5c6baaa 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -54,6 +54,7 @@ struct usbtest_info {
unsignedautoconf:1;
unsignedctrl_out:1;
unsignediso:1;  /* try iso in/out */
+   unsignedintr:1; /* try interrupt in/out */
int alt;
 };
 
@@ -70,7 +71,10 @@ struct usbtest_dev {
int out_pipe;
int in_iso_pipe;
int out_iso_pipe;
+   int in_int_pipe;
+   int out_int_pipe;
struct usb_endpoint_descriptor  *iso_in, *iso_out;
+   struct usb_endpoint_descriptor  *int_in, *int_out;
struct mutexlock;
 
 #define TBUF_SIZE  256
@@ -101,6 +105,7 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface 
*intf)
struct usb_host_interface   *alt;
struct usb_host_endpoint*in, *out;
struct usb_host_endpoint*iso_in, *iso_out;
+   struct usb_host_endpoint*int_in, *int_out;
struct usb_device   *udev;
 
for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
@@ -108,6 +113,7 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface 
*intf)
 
in = out = NULL;
iso_in = iso_out = NULL;
+   int_in = int_out = NULL;
alt = intf->altsetting + tmp;
 
if (override_alt >= 0 &&
@@ -124,6 +130,9 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface 
*intf)
switch (usb_endpoint_type(&e->desc)) {
case USB_ENDPOINT_XFER_BULK:
break;
+   case USB_ENDPOINT_XFER_INT:
+   if (dev->info->intr)
+   goto try_intr;
case USB_ENDPOINT_XFER_ISOC:
if (dev->info->iso)
goto try_iso;
@@ -139,6 +148,15 @@ get_endpoints(struct usbtest_dev *dev, struct 
usb_interface *intf)
out = e;
}
continue;
+try_intr:
+   if (usb_endpoint_dir_in(&e->desc)) {
+   if (!int_in)
+   int_in = e;
+   } else {
+   if (!int_out)
+   int_out = e;
+   }
+   continue;
 try_iso:
if (usb_endpoint_dir_in(&e->desc)) {
if (!iso_in)
@@ -148,7 +166,7 @@ try_iso:
iso_out = e;
}
}
-   if ((in && out)  ||  iso_in || iso_out)
+   if ((in && out)  ||  iso_in || iso_out || int_in || int_out)
goto found;
}
return -EINVAL;
@@ -183,6 +201,20 @@ found:
iso_out->desc.bEndpointAddress
& USB_ENDPOINT_NUMBER_MASK);
}
+
+   if (int_in) {
+   dev->int_in = &int_in->desc;
+   dev->in_int_pipe = usb_rcvintpipe(udev,
+   int_in->desc.bEndpointAddress
+   & USB_ENDPOINT_NUMBER_MASK);
+   }
+
+   if (int_out) {
+   dev->int_out = &int_out->desc;
+   dev->out_int_pipe = usb_sndintpipe(udev,
+   int_out->desc.bEndpointAddress
+   & USB_ENDPOINT_NUMBER_MASK);
+   }
return 0;
 }
 
@@ -205,14 +237,22 @@ static struct urb *usbtest_alloc_urb(
int pipe,
unsigned long   bytes,
unsignedtransfer_flags,
-   unsignedoffset)
+   unsignedoffset,
+   u8  bInterval)
 {
struct urb  *urb;
 
urb = usb_alloc_urb(0, GFP_KER