Skip to contentSkip to conversation

usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec

PATCH Superseded Dave Penkler 2 participants 11 messages Nov 15, 2015 – Nov 18, 2015 USB

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v4 cover usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec

Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations.

Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (4/5)

Add convenience ioctl to return all device capabilities (5/5)

PATCH Changelog:
v4 - Remove excess newlines and parens
- Simplify some expressions
- Remove redundant initialisation

v3 - Split into multiple patches as per gregkh request

V2 - Fix V1 bug: not waking sleepers on disconnect.
- Correct sparse warnings.

V1 - Original patch

Testing:
All functions tested ok on an USBTMC-USB488 compliant oscilloscope

Dave Penkler (5):
Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE
operation.
Add support for USBTMC USB488 SRQ notification with fasync Add support for receiving USBTMC USB488 SRQ notifications via
poll/select
Add ioctl to retrieve USBTMC-USB488 capabilities
Add ioctls to enable and disable local controls on an instrument

 drivers/usb/class/usbtmc.c   | 354 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |  29 +++-
 2 files changed, 380 insertions(+), 3 deletions(-)
Signature · 1 line
2.5.1
#1 lore raw

PATCH v4 1/5 Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Background:
When performing a read on an instrument that is executing a function that runs longer than the USB timeout the instrument may hang and require a device reset to recover. The READ_STATUS_BYTE operation always returns even when the instrument is busy permitting to poll for the appropriate condition. This capability is referred to in instrument application notes on synchronizing acquisitions for other platforms.

---
 drivers/usb/class/usbtmc.c   | 219 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |   2 +
 2 files changed, 221 insertions(+)
drivers/usb/class/usbtmc.c +219 -0
index 7a11a82..72ef7f0 100644
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
 	u8 bTag_last_write;	/* needed for abort */
 	u8 bTag_last_read;	/* needed for abort */
 
+	/* data for interrupt in endpoint handling */
+	u8             bNotify1;
+	u8             bNotify2;
+	u16            ifnum;
+	u8             iin_bTag;
+	u8            *iin_buffer;
+	atomic_t       iin_data_valid;
+	unsigned int   iin_ep;
+	int            iin_ep_present;
+	int            iin_interval;
+	struct urb    *iin_urb;
+	u16            iin_wMaxPacketSize;
+
 	u8 rigol_quirk;
 
 	/* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
 	struct usbtmc_dev_capabilities	capabilities;
 	struct kref kref;
 	struct mutex io_mutex;	/* only one i/o function running at a time */
+	wait_queue_head_t waitq;
 };
 #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
 
@@ -373,6 +387,88 @@ exit:
 	return rv;
 }
 
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+				unsigned long arg)
+{
+	u8 *buffer;
+	struct device *dev;
+	int rv;
+	u8 tag, stb;
+
+	dev = &data->intf->dev;
+
+	dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+		data->iin_ep_present);
+
+	buffer = kmalloc(8, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	atomic_set(&data->iin_data_valid, 0);
+
+	rv = usb_control_msg(data->usb_dev,
+			usb_rcvctrlpipe(data->usb_dev, 0),
+			USBTMC488_REQUEST_READ_STATUS_BYTE,
+			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			data->iin_bTag,
+			data->ifnum,
+			buffer, 0x03, USBTMC_TIMEOUT);
+
+	if (rv < 0) {
+		dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+		goto exit;
+	}
+
+	if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+		dev_err(dev, "control status returned %x\n", buffer[0]);
+		rv = -EIO;
+		goto exit;
+	}
+
+	if (data->iin_ep_present) {
+
+		rv = wait_event_interruptible_timeout(
+			data->waitq,
+			atomic_read(&data->iin_data_valid) != 0,
+			USBTMC_TIMEOUT);
+
+		if (rv < 0) {
+			dev_dbg(dev, "wait interrupted %d\n", rv);
+			goto exit;
+		}
+
+		if (rv == 0) {
+			dev_dbg(dev, "wait timed out\n");
+			rv = -ETIME;
+			goto exit;
+		}
+
+		tag = data->bNotify1 & 0x7f;
+
+		if (tag != data->iin_bTag) {
+			dev_err(dev, "expected bTag %x got %x\n",
+				data->iin_bTag, tag);
+		}
+
+		stb = data->bNotify2;
+	} else {
+		stb = buffer[2];
+	}
+
+	rv = copy_to_user((void __user *)arg, &stb, sizeof(stb));
+	if (rv)
+		rv = -EFAULT;
+
+ exit:
+	/* bump interrupt bTag */
+	data->iin_bTag += 1;
+	if (data->iin_bTag > 127)
+		data->iin_bTag = 2;
+
+	kfree(buffer);
+	return rv;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
  * @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1165,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case USBTMC_IOCTL_ABORT_BULK_IN:
 		retval = usbtmc_ioctl_abort_bulk_in(data);
 		break;
+
+	case USBTMC488_IOCTL_READ_STB:
+		retval = usbtmc488_ioctl_read_stb(data, arg);
+		break;
+
 	}
 
 skip_io_on_zombie:
@@ -1092,6 +1193,69 @@ static struct usb_class_driver usbtmc_class = {
 	.minor_base =	USBTMC_MINOR_BASE,
 };
 
+static void usbtmc_interrupt(struct urb *urb)
+{
+	struct usbtmc_device_data *data = urb->context;
+	int status = urb->status;
+	int rv;
+
+	dev_dbg(&data->intf->dev, "int status: %d len %d\n",
+		status, urb->actual_length);
+
+	switch (status) {
+	case 0: /* SUCCESS */
+
+		if (data->iin_buffer[0] & 0x80) {
+			/* check for valid STB notification */
+			if ((data->iin_buffer[0] & 0x7f) > 1) {
+				data->bNotify1 = data->iin_buffer[0];
+				data->bNotify2 = data->iin_buffer[1];
+				atomic_set(&data->iin_data_valid, 1);
+				wake_up_interruptible(&data->waitq);
+				goto exit;
+			}
+		}
+		dev_warn(&data->intf->dev, "invalid notification: %x\n",
+			data->iin_buffer[0]);
+		break;
+	case -EOVERFLOW:
+		dev_err(&data->intf->dev,
+			"%s - overflow with length %d, actual length is %d\n",
+			__func__, data->iin_wMaxPacketSize, urb->actual_length);
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+	case -EILSEQ:
+	case -ETIME:
+		/* urb terminated, clean up */
+		dev_dbg(&data->intf->dev,
+			"%s - urb terminated, status: %d\n",
+			__func__, status);
+		return;
+	default:
+		dev_err(&data->intf->dev,
+			"%s - unknown status received: %d\n",
+			__func__, status);
+	}
+exit:
+	rv = usb_submit_urb(urb, GFP_ATOMIC);
+	if (rv) {
+		dev_err(&data->intf->dev, "%s - usb_submit_urb failed: %d\n",
+			__func__, rv);
+	}
+}
+
+static void usbtmc_free_int(struct usbtmc_device_data *data)
+{
+	if (data->iin_ep_present) {
+		if (data->iin_urb) {
+			usb_kill_urb(data->iin_urb);
+			kfree(data->iin_buffer);
+			usb_free_urb(data->iin_urb);
+			kref_put(&data->kref, usbtmc_delete);
+		}
+	}
+}
 
 static int usbtmc_probe(struct usb_interface *intf,
 			const struct usb_device_id *id)
@@ -1114,6 +1278,8 @@ static int usbtmc_probe(struct usb_interface *intf,
 	usb_set_intfdata(intf, data);
 	kref_init(&data->kref);
 	mutex_init(&data->io_mutex);
+	init_waitqueue_head(&data->waitq);
+	atomic_set(&data->iin_data_valid, 0);
 	data->zombie = 0;
 
 	/* Determine if it is a Rigol or not */
@@ -1134,9 +1300,12 @@ static int usbtmc_probe(struct usb_interface *intf,
 	data->bTag	= 1;
 	data->TermCharEnabled = 0;
 	data->TermChar = '\n';
+	/*  2 <= bTag <= 127   USBTMC-USB488 subclass specification 4.3.1 */
+	data->iin_bTag = 2;
 
 	/* USBTMC devices have only one setting, so use that */
 	iface_desc = data->intf->cur_altsetting;
+	data->ifnum = iface_desc->desc.bInterfaceNumber;
 
 	/* Find bulk in endpoint */
 	for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
@@ -1161,6 +1330,20 @@ static int usbtmc_probe(struct usb_interface *intf,
 			break;
 		}
 	}
+	/* Find int endpoint */
+	for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
+		endpoint = &iface_desc->endpoint[n].desc;
+
+		if (usb_endpoint_is_int_in(endpoint)) {
+			data->iin_ep_present = 1;
+			data->iin_ep = endpoint->bEndpointAddress;
+			data->iin_wMaxPacketSize = usb_endpoint_maxp(endpoint);
+			data->iin_interval = endpoint->bInterval;
+			dev_dbg(&intf->dev, "Found Int in endpoint at %u\n",
+				data->iin_ep);
+			break;
+		}
+	}
 
 	retcode = get_capabilities(data);
 	if (retcode)
@@ -1169,6 +1352,40 @@ static int usbtmc_probe(struct usb_interface *intf,
 		retcode = sysfs_create_group(&intf->dev.kobj,
 					     &capability_attr_grp);
 
+
+	if (data->iin_ep_present) {
+		/* allocate int urb */
+		data->iin_urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!data->iin_urb) {
+			dev_err(&intf->dev, "Failed to allocate int urb\n");
+			goto error_register;
+		}
+
+		/* will reference data in int urb */
+		kref_get(&data->kref);
+
+		/* allocate buffer for interrupt in */
+		data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
+					GFP_KERNEL);
+		if (!data->iin_buffer) {
+			dev_err(&intf->dev, "Failed to allocate int buf\n");
+			goto error_register;
+		}
+
+		/* fill interrupt urb */
+		usb_fill_int_urb(data->iin_urb, data->usb_dev,
+				usb_rcvintpipe(data->usb_dev, data->iin_ep),
+				data->iin_buffer, data->iin_wMaxPacketSize,
+				usbtmc_interrupt,
+				data, data->iin_interval);
+
+		if (usb_submit_urb(data->iin_urb, GFP_KERNEL)) {
+			retcode = -EIO;
+			dev_err(&intf->dev, "Failed to submit iin_urb\n");
+			goto error_register;
+		}
+	}
+
 	retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);
 
 	retcode = usb_register_dev(intf, &usbtmc_class);
@@ -1185,6 +1402,7 @@ static int usbtmc_probe(struct usb_interface *intf,
 error_register:
 	sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
 	sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+	usbtmc_free_int(data);
 	kref_put(&data->kref, usbtmc_delete);
 	return retcode;
 }
@@ -1196,6 +1414,7 @@ static void usbtmc_disconnect(struct usb_interface *intf)
 	dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
 
 	data = usb_get_intfdata(intf);
+	usbtmc_free_int(data);
 	usb_deregister_dev(intf, &usbtmc_class);
 	sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
 	sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
include/uapi/linux/usb/tmc.h +2 -0
index c045ae1..7e5ced8 100644
@@ -30,6 +30,7 @@
 #define USBTMC_REQUEST_CHECK_CLEAR_STATUS		6
 #define USBTMC_REQUEST_GET_CAPABILITIES			7
 #define USBTMC_REQUEST_INDICATOR_PULSE			64
+#define USBTMC488_REQUEST_READ_STATUS_BYTE		128
 
 /* Request values for USBTMC driver's ioctl entry point */
 #define USBTMC_IOC_NR			91
@@ -39,5 +40,6 @@
 #define USBTMC_IOCTL_ABORT_BULK_IN	_IO(USBTMC_IOC_NR, 4)
 #define USBTMC_IOCTL_CLEAR_OUT_HALT	_IO(USBTMC_IOC_NR, 6)
 #define USBTMC_IOCTL_CLEAR_IN_HALT	_IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_READ_STB	_IOR(USBTMC_IOC_NR, 18, unsigned char)
 
 #endif
Signature · 1 line
2.5.1
#2 lore raw

Re: [PATCH v4 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Quoted 9 lines · Dave Penkler

Background:
When performing a read on an instrument that is executing a function that runs longer than the USB timeout the instrument may hang and require a device reset to recover. The READ_STATUS_BYTE operation always returns even when the instrument is busy permitting to poll for the appropriate condition. This capability is referred to in instrument application notes on synchronizing acquisitions for other platforms.

Few nitpicks below.

Quoted 41 lines
---
 drivers/usb/class/usbtmc.c   | 219 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |   2 +
 2 files changed, 221 insertions(+)
drivers/usb/class/usbtmc.c +14 -0
index 7a11a82..72ef7f0 100644
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
        u8 bTag_last_write;     /* needed for abort */
        u8 bTag_last_read;      /* needed for abort */
 
+       /* data for interrupt in endpoint handling */
+       u8             bNotify1;
+       u8             bNotify2;
+       u16            ifnum;
+       u8             iin_bTag;
+       u8            *iin_buffer;
+       atomic_t       iin_data_valid;
+       unsigned int   iin_ep;
+       int            iin_ep_present;
+       int            iin_interval;
+       struct urb    *iin_urb;
+       u16            iin_wMaxPacketSize;
+
        u8 rigol_quirk;
 
        /* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
        struct usbtmc_dev_capabilities  capabilities;
        struct kref kref;
        struct mutex io_mutex;  /* only one i/o function running at a time */
+       wait_queue_head_t waitq;
 };
 #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
 
@@ -373,6 +387,88 @@ exit:
        return rv;
 }

drivers/usb/class/usbtmc.c exit: · line 398

Quoted 10 lines
drivers/usb/class/usbtmc.c +9 -0
 
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+                               unsigned long arg)
+{
+       u8 *buffer;
+       struct device *dev;
+       int rv;
+       u8 tag, stb;
+
+       dev = &data->intf->dev;

it could be struct device *dev = …;

Quoted 21 lines
drivers/usb/class/usbtmc.c +21 -0
+
+       dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+               data->iin_ep_present);
+
+       buffer = kmalloc(8, GFP_KERNEL);
+       if (!buffer)
+               return -ENOMEM;
+
+       atomic_set(&data->iin_data_valid, 0);
+
+       rv = usb_control_msg(data->usb_dev,
+                       usb_rcvctrlpipe(data->usb_dev, 0),
+                       USBTMC488_REQUEST_READ_STATUS_BYTE,
+                       USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+                       data->iin_bTag,
+                       data->ifnum,
+                       buffer, 0x03, USBTMC_TIMEOUT);
+
+       if (rv < 0) {
+               dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+               goto exit;

drivers/usb/class/usbtmc.c exit: · line 429

Quoted 10 lines
drivers/usb/class/usbtmc.c +10 -0
+       }
+
+       if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+               dev_err(dev, "control status returned %x\n", buffer[0]);
+               rv = -EIO;
+               goto exit;
+       }
+
+       if (data->iin_ep_present) {
+

Redundant empty line.

Quoted 28 lines
drivers/usb/class/usbtmc.c +28 -0
+               rv = wait_event_interruptible_timeout(
+                       data->waitq,
+                       atomic_read(&data->iin_data_valid) != 0,
+                       USBTMC_TIMEOUT);
+
+               if (rv < 0) {
+                       dev_dbg(dev, "wait interrupted %d\n", rv);
+                       goto exit;
+               }
+
+               if (rv == 0) {
+                       dev_dbg(dev, "wait timed out\n");
+                       rv = -ETIME;
+                       goto exit;
+               }
+
+               tag = data->bNotify1 & 0x7f;
+
+               if (tag != data->iin_bTag) {
+                       dev_err(dev, "expected bTag %x got %x\n",
+                               data->iin_bTag, tag);
+               }
+
+               stb = data->bNotify2;
+       } else {
+               stb = buffer[2];
+       }
+

drivers/usb/class/usbtmc.c exit: · line 466

Quoted 10 lines
drivers/usb/class/usbtmc.c +9 -0
+       rv = copy_to_user((void __user *)arg, &stb, sizeof(stb));
+       if (rv)
+               rv = -EFAULT;
+
+ exit:
+       /* bump interrupt bTag */
+       data->iin_bTag += 1;
+       if (data->iin_bTag > 127)
 
+               data->iin_bTag = 2;

Hmm… Why 2?
A-ha, below I found a comment. Something might be good to have here as well.

Quoted 9 lines
drivers/usb/class/usbtmc.c +5 -0
+
+       kfree(buffer);
+       return rv;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.

* @transfer_size: number of bytes to request from the device.

drivers/usb/class/usbtmc.c static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 1172

Quoted 8 lines
drivers/usb/class/usbtmc.c +5 -0
@@ -1069,6 +1165,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
        case USBTMC_IOCTL_ABORT_BULK_IN:
                retval = usbtmc_ioctl_abort_bulk_in(data);
                break;
+
+       case USBTMC488_IOCTL_READ_STB:
+               retval = usbtmc488_ioctl_read_stb(data, arg);
+               break;
+

Redundant empty line.

Quoted 12 lines
drivers/usb/class/usbtmc.c +5 -0
        }
 
 skip_io_on_zombie:
@@ -1092,6 +1193,69 @@ static struct usb_class_driver usbtmc_class = {
        .minor_base =   USBTMC_MINOR_BASE,
 };
 
+static void usbtmc_interrupt(struct urb *urb)
+{
+       struct usbtmc_device_data *data = urb->context;
+       int status = urb->status;
+       int rv;

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1210

Quoted 10 lines
drivers/usb/class/usbtmc.c +10 -0
+
+       dev_dbg(&data->intf->dev, "int status: %d len %d\n",
+               status, urb->actual_length);
+
+       switch (status) {
+       case 0: /* SUCCESS */
+
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {

It's the same as

     if (data->iin_buffer[0] & 0x7e) {
Quoted 12 lines
drivers/usb/class/usbtmc.c +12 -0
+                               data->bNotify1 = data->iin_buffer[0];
+                               data->bNotify2 = data->iin_buffer[1];
+                               atomic_set(&data->iin_data_valid, 1);
+                               wake_up_interruptible(&data->waitq);
+                               goto exit;
+                       }
+               }
+               dev_warn(&data->intf->dev, "invalid notification: %x\n",
+                       data->iin_buffer[0]);
+               break;
+       case -EOVERFLOW:
+               dev_err(&data->intf->dev,

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1232

Quoted 10 lines
drivers/usb/class/usbtmc.c +10 -0
+                       "%s - overflow with length %d, actual length is %d\n",
+                       __func__, data->iin_wMaxPacketSize, urb->actual_length);
+       case -ECONNRESET:
+       case -ENOENT:
+       case -ESHUTDOWN:
+       case -EILSEQ:
+       case -ETIME:
+               /* urb terminated, clean up */
+               dev_dbg(&data->intf->dev,
+                       "%s - urb terminated, status: %d\n",

I heard that dynamic debug adds function name.

Quoted 9 lines
drivers/usb/class/usbtmc.c +9 -0
+                       __func__, status);
+               return;
+       default:
+               dev_err(&data->intf->dev,
+                       "%s - unknown status received: %d\n",
+                       __func__, status);
+       }
+exit:
+       rv = usb_submit_urb(urb, GFP_ATOMIC);

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1251

Quoted 10 lines
drivers/usb/class/usbtmc.c +10 -0
+       if (rv) {
+               dev_err(&data->intf->dev, "%s - usb_submit_urb failed: %d\n",
+                       __func__, rv);
+       }
+}
+
+static void usbtmc_free_int(struct usbtmc_device_data *data)
+{
+       if (data->iin_ep_present) {
+               if (data->iin_urb) {

Why not

if (!data->iin_ep_present || !data->iin_urb)
return;

?

Quoted 127 lines
drivers/usb/class/usbtmc.c +62 -0
+                       usb_kill_urb(data->iin_urb);
+                       kfree(data->iin_buffer);
+                       usb_free_urb(data->iin_urb);
+                       kref_put(&data->kref, usbtmc_delete);
+               }
+       }
+}
 
 static int usbtmc_probe(struct usb_interface *intf,
                        const struct usb_device_id *id)
@@ -1114,6 +1278,8 @@ static int usbtmc_probe(struct usb_interface *intf,
        usb_set_intfdata(intf, data);
        kref_init(&data->kref);
        mutex_init(&data->io_mutex);
+       init_waitqueue_head(&data->waitq);
+       atomic_set(&data->iin_data_valid, 0);
        data->zombie = 0;
 
        /* Determine if it is a Rigol or not */
@@ -1134,9 +1300,12 @@ static int usbtmc_probe(struct usb_interface *intf,
        data->bTag      = 1;
        data->TermCharEnabled = 0;
        data->TermChar = '\n';
+       /*  2 <= bTag <= 127   USBTMC-USB488 subclass specification 4.3.1 */
+       data->iin_bTag = 2;
 
        /* USBTMC devices have only one setting, so use that */
        iface_desc = data->intf->cur_altsetting;
+       data->ifnum = iface_desc->desc.bInterfaceNumber;
 
        /* Find bulk in endpoint */
        for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
@@ -1161,6 +1330,20 @@ static int usbtmc_probe(struct usb_interface *intf,
                        break;
                }
        }
+       /* Find int endpoint */
+       for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
+               endpoint = &iface_desc->endpoint[n].desc;
+
+               if (usb_endpoint_is_int_in(endpoint)) {
+                       data->iin_ep_present = 1;
+                       data->iin_ep = endpoint->bEndpointAddress;
+                       data->iin_wMaxPacketSize = usb_endpoint_maxp(endpoint);
+                       data->iin_interval = endpoint->bInterval;
+                       dev_dbg(&intf->dev, "Found Int in endpoint at %u\n",
+                               data->iin_ep);
+                       break;
+               }
+       }
 
        retcode = get_capabilities(data);
        if (retcode)
@@ -1169,6 +1352,40 @@ static int usbtmc_probe(struct usb_interface *intf,
                retcode = sysfs_create_group(&intf->dev.kobj,
                                             &capability_attr_grp);
 
+
+       if (data->iin_ep_present) {
+               /* allocate int urb */
+               data->iin_urb = usb_alloc_urb(0, GFP_KERNEL);
+               if (!data->iin_urb) {
+                       dev_err(&intf->dev, "Failed to allocate int urb\n");
+                       goto error_register;
+               }
+
+               /* will reference data in int urb */
+               kref_get(&data->kref);
+
+               /* allocate buffer for interrupt in */
+               data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
+                                       GFP_KERNEL);
+               if (!data->iin_buffer) {
+                       dev_err(&intf->dev, "Failed to allocate int buf\n");
+                       goto error_register;
+               }
+
+               /* fill interrupt urb */
+               usb_fill_int_urb(data->iin_urb, data->usb_dev,
+                               usb_rcvintpipe(data->usb_dev, data->iin_ep),
+                               data->iin_buffer, data->iin_wMaxPacketSize,
+                               usbtmc_interrupt,
+                               data, data->iin_interval);
+
+               if (usb_submit_urb(data->iin_urb, GFP_KERNEL)) {
+                       retcode = -EIO;
+                       dev_err(&intf->dev, "Failed to submit iin_urb\n");
+                       goto error_register;
+               }
+       }
+
        retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);
 
        retcode = usb_register_dev(intf, &usbtmc_class);
@@ -1185,6 +1402,7 @@ static int usbtmc_probe(struct usb_interface *intf,
 error_register:
        sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
        sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+       usbtmc_free_int(data);
        kref_put(&data->kref, usbtmc_delete);
        return retcode;
 }
@@ -1196,6 +1414,7 @@ static void usbtmc_disconnect(struct usb_interface *intf)
        dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
 
        data = usb_get_intfdata(intf);
+       usbtmc_free_int(data);
        usb_deregister_dev(intf, &usbtmc_class);
        sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
        sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
include/uapi/linux/usb/tmc.h +2 -0
index c045ae1..7e5ced8 100644
@@ -30,6 +30,7 @@
 #define USBTMC_REQUEST_CHECK_CLEAR_STATUS              6
 #define USBTMC_REQUEST_GET_CAPABILITIES                        7
 #define USBTMC_REQUEST_INDICATOR_PULSE                 64
+#define USBTMC488_REQUEST_READ_STATUS_BYTE             128
 
 /* Request values for USBTMC driver's ioctl entry point */
 #define USBTMC_IOC_NR                  91
@@ -39,5 +40,6 @@
 #define USBTMC_IOCTL_ABORT_BULK_IN     _IO(USBTMC_IOC_NR, 4)
 #define USBTMC_IOCTL_CLEAR_OUT_HALT    _IO(USBTMC_IOC_NR, 6)
 #define USBTMC_IOCTL_CLEAR_IN_HALT     _IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_READ_STB       _IOR(USBTMC_IOC_NR, 18, unsigned char)
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#3 lore raw

Re: [PATCH v4 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Hi Andy,

Quoted 1 line · Andy Shevchenko

On Sun, Nov 15, 2015 at 8:39 PM, Dave Penkler <[email protected]> wrote:

snip

Quoted 4 lines
Quoted 1 line

+

Redundant empty line.

ok

Quoted 6 lines
Quoted 2 lines

+ data->iin_bTag = 2;

Hmm??? Why 2?
A-ha, below I found a comment. Something might be good to have here as well.

Added comment

Quoted 4 lines
Quoted 1 line

+

Redundant empty line.

ok

Quoted 8 lines

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1210

Quoted 4 lines
drivers/usb/class/usbtmc.c +4 -0
+
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {

It's the same as

     if (data->iin_buffer[0] & 0x7e) {

Yes but when reading the spec and the code it is more obvious that here we are testing for the value in bits D6..D0 to be a valid iin_bTag return. (See Table 7 in the USBTMC-USB488 spec.)

Quoted 6 lines

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1232

Quoted 2 lines
drivers/usb/class/usbtmc.c +2 -0
+               dev_dbg(&data->intf->dev,
+                       "%s - urb terminated, status: %d\n",

I heard that dynamic debug adds function name.

Quoted 1 line

+ __func__, status);

I checked in device.h and could not find any trace of it. I'm on 4.4.0-rc1.

Quoted 7 lines

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1251

Quoted 4 lines
drivers/usb/class/usbtmc.c +4 -0
+static void usbtmc_free_int(struct usbtmc_device_data *data)
+{
+       if (data->iin_ep_present) {
+               if (data->iin_urb) {

Why not

Quoted 5 lines

if (!data->iin_ep_present || !data->iin_urb)
return;

?

OK

Thanks,
-Dave

#4 lore raw

PATCH v4 2/5 Add support for USBTMC USB488 SRQ notification with fasync

In reply to Dave Penkler

Background:
By configuring an instrument's event status register various conditions can be reported via an SRQ notification. This complements the synchronous polling approach using the READ_STATUS_BYTE ioctl with an asynchronous notification.

---
 drivers/usb/class/usbtmc.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
drivers/usb/class/usbtmc.c +24 -0
index 72ef7f0..7430a52 100644
@@ -99,6 +99,7 @@ struct usbtmc_device_data {
 	int            iin_interval;
 	struct urb    *iin_urb;
 	u16            iin_wMaxPacketSize;
+	atomic_t       srq_asserted;
 
 	u8 rigol_quirk;
 
@@ -113,6 +114,7 @@ struct usbtmc_device_data {
 	struct kref kref;
 	struct mutex io_mutex;	/* only one i/o function running at a time */
 	wait_queue_head_t waitq;
+	struct fasync_struct *fasync;
 };
 #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
 
@@ -406,6 +408,9 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
 
 	atomic_set(&data->iin_data_valid, 0);
 
+	/* must issue read_stb before using poll or select */
+	atomic_set(&data->srq_asserted, 0);
+
 	rv = usb_control_msg(data->usb_dev,
 			usb_rcvctrlpipe(data->usb_dev, 0),
 			USBTMC488_REQUEST_READ_STATUS_BYTE,
@@ -1177,6 +1182,13 @@ skip_io_on_zombie:
 	return retval;
 }
 
+static int usbtmc_fasync(int fd, struct file *file, int on)
+{
+	struct usbtmc_device_data *data = file->private_data;
+
+	return fasync_helper(fd, file, on, &data->fasync);
+}
+
 static const struct file_operations fops = {
 	.owner		= THIS_MODULE,
 	.read		= usbtmc_read,
@@ -1184,6 +1196,7 @@ static const struct file_operations fops = {
 	.open		= usbtmc_open,
 	.release	= usbtmc_release,
 	.unlocked_ioctl	= usbtmc_ioctl,
+	.fasync         = usbtmc_fasync,
 	.llseek		= default_llseek,
 };
 
@@ -1214,6 +1227,16 @@ static void usbtmc_interrupt(struct urb *urb)
 				wake_up_interruptible(&data->waitq);
 				goto exit;
 			}
+			/* check for SRQ notification */
+			if ((data->iin_buffer[0] & 0x7f) == 1) {
+				if (data->fasync)
+					kill_fasync(&data->fasync,
+						SIGIO, POLL_IN);
+
+				atomic_set(&data->srq_asserted, 1);
+				wake_up_interruptible(&data->waitq);
+				goto exit;
+			}
 		}
 		dev_warn(&data->intf->dev, "invalid notification: %x\n",
 			data->iin_buffer[0]);
@@ -1280,6 +1303,7 @@ static int usbtmc_probe(struct usb_interface *intf,
 	mutex_init(&data->io_mutex);
 	init_waitqueue_head(&data->waitq);
 	atomic_set(&data->iin_data_valid, 0);
+	atomic_set(&data->srq_asserted, 0);
 	data->zombie = 0;
 
 	/* Determine if it is a Rigol or not */
Signature · 1 line
2.5.1
#5 lore raw

PATCH v4 3/5 Add support for receiving USBTMC USB488 SRQ notifications via poll/select

In reply to Dave Penkler

Background:
In many situations operations on multiple instruments need to be synchronized. poll/select provide a convenient way of waiting on a number of different instruments and other peripherals simultaneously.

---
 drivers/usb/class/usbtmc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
drivers/usb/class/usbtmc.c +23 -0
index 7430a52..6c0e1dc 100644
@@ -27,6 +27,7 @@
 #include <linux/uaccess.h>
 #include <linux/kref.h>
 #include <linux/slab.h>
+#include <linux/poll.h>
 #include <linux/mutex.h>
 #include <linux/usb.h>
 #include <linux/usb/tmc.h>
@@ -1189,6 +1190,27 @@ static int usbtmc_fasync(int fd, struct file *file, int on)
 	return fasync_helper(fd, file, on, &data->fasync);
 }
 
+static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
+{
+	struct usbtmc_device_data *data = file->private_data;
+	unsigned int mask;
+
+	mutex_lock(&data->io_mutex);
+
+	if (data->zombie) {
+		mask = POLLHUP | POLLERR;
+		goto no_poll;
+	}
+
+	poll_wait(file, &data->waitq, wait);
+
+	mask = (atomic_read(&data->srq_asserted)) ? POLLIN | POLLRDNORM : 0;
+
+no_poll:
+	mutex_unlock(&data->io_mutex);
+	return mask;
+}
+
 static const struct file_operations fops = {
 	.owner		= THIS_MODULE,
 	.read		= usbtmc_read,
@@ -1197,6 +1219,7 @@ static const struct file_operations fops = {
 	.release	= usbtmc_release,
 	.unlocked_ioctl	= usbtmc_ioctl,
 	.fasync         = usbtmc_fasync,
+	.poll           = usbtmc_poll,
 	.llseek		= default_llseek,
 };
 
Signature · 1 line
2.5.1
#6 lore raw

PATCH v4 4/5 Add ioctl to retrieve USBTMC-USB488 capabilities

In reply to Dave Penkler

This is a convenience function to obtain an instrument's capabilities from its file descriptor without having to access sysfs from the user program.

---
 drivers/usb/class/usbtmc.c   | 12 ++++++++++++
 include/uapi/linux/usb/tmc.h | 21 ++++++++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)
drivers/usb/class/usbtmc.c +12 -0
index 6c0e1dc..add0ce2 100644
@@ -102,6 +102,9 @@ struct usbtmc_device_data {
 	u16            iin_wMaxPacketSize;
 	atomic_t       srq_asserted;
 
+	/* coalesced usb488_caps from usbtmc_dev_capabilities */
+	u8 usb488_caps;
+
 	u8 rigol_quirk;
 
 	/* attributes from the USB TMC spec for this device */
@@ -997,6 +1000,7 @@ static int get_capabilities(struct usbtmc_device_data *data)
 	data->capabilities.device_capabilities = buffer[5];
 	data->capabilities.usb488_interface_capabilities = buffer[14];
 	data->capabilities.usb488_device_capabilities = buffer[15];
+	data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
 	rv = 0;
 
 err_out:
@@ -1172,6 +1176,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		retval = usbtmc_ioctl_abort_bulk_in(data);
 		break;
 
+	case USBTMC488_IOCTL_GET_CAPS:
+		retval = copy_to_user((void __user *)arg,
+				&data->usb488_caps,
+				sizeof(data->usb488_caps));
+		if (retval)
+			retval = -EFAULT;
+		break;
+
 	case USBTMC488_IOCTL_READ_STB:
 		retval = usbtmc488_ioctl_read_stb(data, arg);
 		break;
include/uapi/linux/usb/tmc.h +18 -3
index 7e5ced8..1dc3af1 100644
@@ -2,12 +2,14 @@
  * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
  * Copyright (C) 2008 Novell, Inc.
  * Copyright (C) 2008 Greg Kroah-Hartman <[email protected]>
+ * Copyright (C) 2015 Dave Penkler <[email protected]>
  *
  * This file holds USB constants defined by the USB Device Class
- * Definition for Test and Measurement devices published by the USB-IF.
+ * and USB488 Subclass Definitions for Test and Measurement devices
+ * published by the USB-IF.
  *
- * It also has the ioctl definitions for the usbtmc kernel driver that
- * userspace needs to know about.
+ * It also has the ioctl and capability definitions for the
+ * usbtmc kernel driver that userspace needs to know about.
  */
 
 #ifndef __LINUX_USB_TMC_H
@@ -40,6 +42,19 @@
 #define USBTMC_IOCTL_ABORT_BULK_IN	_IO(USBTMC_IOC_NR, 4)
 #define USBTMC_IOCTL_CLEAR_OUT_HALT	_IO(USBTMC_IOC_NR, 6)
 #define USBTMC_IOCTL_CLEAR_IN_HALT	_IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_GET_CAPS	_IO(USBTMC_IOC_NR, 17)
 #define USBTMC488_IOCTL_READ_STB	_IOR(USBTMC_IOC_NR, 18, unsigned char)
 
+/* Driver encoded usb488 capabilities */
+#define USBTMC488_CAPABILITY_TRIGGER         1
+#define USBTMC488_CAPABILITY_SIMPLE          2
+#define USBTMC488_CAPABILITY_REN_CONTROL     2
+#define USBTMC488_CAPABILITY_GOTO_LOCAL      2
+#define USBTMC488_CAPABILITY_LOCAL_LOCKOUT   2
+#define USBTMC488_CAPABILITY_488_DOT_2       4
+#define USBTMC488_CAPABILITY_DT1             16
+#define USBTMC488_CAPABILITY_RL1             32
+#define USBTMC488_CAPABILITY_SR1             64
+#define USBTMC488_CAPABILITY_FULL_SCPI       128
+
 #endif
Signature · 1 line
2.5.1
#7 lore raw

PATCH v4 5/5 Add ioctls to enable and disable local controls on an instrument

In reply to Dave Penkler

These ioctls provide support for the USBTMC-USB488 control requests for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT

---
 drivers/usb/class/usbtmc.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |  6 ++++
 2 files changed, 82 insertions(+)
drivers/usb/class/usbtmc.c +76 -0
index add0ce2..79caa78 100644
@@ -478,6 +478,68 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
 	return rv;
 }
 
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+				unsigned long arg,
+				unsigned int cmd)
+{
+	u8 *buffer;
+	struct device *dev;
+	int rv;
+	unsigned int val;
+	u16 wValue;
+
+	dev = &data->intf->dev;
+
+	if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+		return -EINVAL;
+
+	buffer = kmalloc(8, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+		rv = copy_from_user(&val, (void __user *)arg, sizeof(val));
+		if (rv) {
+			rv = -EFAULT;
+			goto exit;
+		}
+		wValue = (val) ? 1 : 0;
+	} else {
+		wValue = 0;
+	}
+
+	dev_warn(dev, "wValue is %d\n", wValue);
+
+	rv = usb_control_msg(data->usb_dev,
+			usb_rcvctrlpipe(data->usb_dev, 0),
+			cmd,
+			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			wValue,
+			data->ifnum,
+			buffer, 0x01, USBTMC_TIMEOUT);
+
+	if (rv < 0) {
+		dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+		goto exit;
+	} else if (rv != 1) {
+		dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+		rv = -EIO;
+		goto exit;
+	}
+
+	if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+		dev_err(dev, "simple control status returned %x\n", buffer[0]);
+		rv = -EIO;
+		goto exit;
+	} else {
+		rv = 0;
+	}
+
+ exit:
+	kfree(buffer);
+	return rv;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
  * @transfer_size: number of bytes to request from the device.
@@ -1188,6 +1250,20 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		retval = usbtmc488_ioctl_read_stb(data, arg);
 		break;
 
+	case USBTMC488_IOCTL_REN_CONTROL:
+		retval = usbtmc488_ioctl_simple(data, arg,
+						USBTMC488_REQUEST_REN_CONTROL);
+		break;
+
+	case USBTMC488_IOCTL_GOTO_LOCAL:
+		retval = usbtmc488_ioctl_simple(data, arg,
+						USBTMC488_REQUEST_GOTO_LOCAL);
+		break;
+
+	case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+		retval = usbtmc488_ioctl_simple(data, arg,
+						USBTMC488_REQUEST_LOCAL_LOCKOUT);
+		break;
 	}
 
 skip_io_on_zombie:
include/uapi/linux/usb/tmc.h +6 -0
index 1dc3af1..0d852c9 100644
@@ -33,6 +33,9 @@
 #define USBTMC_REQUEST_GET_CAPABILITIES			7
 #define USBTMC_REQUEST_INDICATOR_PULSE			64
 #define USBTMC488_REQUEST_READ_STATUS_BYTE		128
+#define USBTMC488_REQUEST_REN_CONTROL			160
+#define USBTMC488_REQUEST_GOTO_LOCAL			161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT			162
 
 /* Request values for USBTMC driver's ioctl entry point */
 #define USBTMC_IOC_NR			91
@@ -44,6 +47,9 @@
 #define USBTMC_IOCTL_CLEAR_IN_HALT	_IO(USBTMC_IOC_NR, 7)
 #define USBTMC488_IOCTL_GET_CAPS	_IO(USBTMC_IOC_NR, 17)
 #define USBTMC488_IOCTL_READ_STB	_IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL	_IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL	_IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT	_IO(USBTMC_IOC_NR, 21)
 
 /* Driver encoded usb488 capabilities */
 #define USBTMC488_CAPABILITY_TRIGGER         1
Signature · 1 line
2.5.1
#8 lore raw

Re: [PATCH v4 5/5] Add ioctls to enable and disable local controls on an instrument

Quoted 3 lines · Dave Penkler

These ioctls provide support for the USBTMC-USB488 control requests for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT

Nitpicks below.

Quoted 15 lines
---
 drivers/usb/class/usbtmc.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |  6 ++++
 2 files changed, 82 insertions(+)
drivers/usb/class/usbtmc.c +1 -0
index add0ce2..79caa78 100644
@@ -478,6 +478,68 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
        return rv;
 }
 
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 491

Quoted 10 lines
drivers/usb/class/usbtmc.c +10 -0
+                               unsigned long arg,
+                               unsigned int cmd)
+{
+       u8 *buffer;
+       struct device *dev;
+       int rv;
+       unsigned int val;
+       u16 wValue;
+
+       dev = &data->intf->dev;

Could be assigned above.

Quoted 5 lines
drivers/usb/class/usbtmc.c +5 -0
+
+       if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+               return -EINVAL;
+
+       buffer = kmalloc(8, GFP_KERNEL);

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 506

Quoted 10 lines
drivers/usb/class/usbtmc.c +10 -0
+       if (!buffer)
+               return -ENOMEM;
+
+       if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+               rv = copy_from_user(&val, (void __user *)arg, sizeof(val));
+               if (rv) {
+                       rv = -EFAULT;
+                       goto exit;
+               }
+               wValue = (val) ? 1 : 0;

Redundant parens.

Quoted 10 lines
drivers/usb/class/usbtmc.c +10 -0
+       } else {
+               wValue = 0;
+       }
+
+       dev_warn(dev, "wValue is %d\n", wValue);
+
+       rv = usb_control_msg(data->usb_dev,
+                       usb_rcvctrlpipe(data->usb_dev, 0),
+                       cmd,
+                       USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 526

Quoted 10 lines
drivers/usb/class/usbtmc.c +10 -0
+                       wValue,
+                       data->ifnum,
+                       buffer, 0x01, USBTMC_TIMEOUT);
+
+       if (rv < 0) {
+               dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+               goto exit;
+       } else if (rv != 1) {
+               dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+               rv = -EIO;

Hmm… Does usb_control_msg() return a proper error code? If it does, perhaps better to propagate it.

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 534

Quoted 9 lines
drivers/usb/class/usbtmc.c +8 -0
+               goto exit;
+       }
+
+       if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+               dev_err(dev, "simple control status returned %x\n", buffer[0]);
+               rv = -EIO;
+               goto exit;
 
+       } else {

Redundant else.

Quoted 64 lines
drivers/usb/class/usbtmc.c +8 -0
+               rv = 0;
+       }
 
 
+
+ exit:
+       kfree(buffer);
+       return rv;
+}
+

/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.

drivers/usb/class/usbtmc.c +14 -0
@@ -1188,6 +1250,20 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
                retval = usbtmc488_ioctl_read_stb(data, arg);
                break;
 
+       case USBTMC488_IOCTL_REN_CONTROL:
+               retval = usbtmc488_ioctl_simple(data, arg,
+                                               USBTMC488_REQUEST_REN_CONTROL);
+               break;
+
+       case USBTMC488_IOCTL_GOTO_LOCAL:
+               retval = usbtmc488_ioctl_simple(data, arg,
+                                               USBTMC488_REQUEST_GOTO_LOCAL);
+               break;
+
+       case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+               retval = usbtmc488_ioctl_simple(data, arg,
+                                               USBTMC488_REQUEST_LOCAL_LOCKOUT);
+               break;
        }
 
 skip_io_on_zombie:
include/uapi/linux/usb/tmc.h +6 -1
index 1dc3af1..0d852c9 100644
@@ -33,6 +33,9 @@
 #define USBTMC_REQUEST_GET_CAPABILITIES                        7
 #define USBTMC_REQUEST_INDICATOR_PULSE                 64
 #define USBTMC488_REQUEST_READ_STATUS_BYTE             128
+#define USBTMC488_REQUEST_REN_CONTROL                  160
+#define USBTMC488_REQUEST_GOTO_LOCAL                   161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT                        162
 
 /* Request values for USBTMC driver's ioctl entry point */
 #define USBTMC_IOC_NR                  91
@@ -44,6 +47,9 @@
 #define USBTMC_IOCTL_CLEAR_IN_HALT     _IO(USBTMC_IOC_NR, 7)
 #define USBTMC488_IOCTL_GET_CAPS       _IO(USBTMC_IOC_NR, 17)
 #define USBTMC488_IOCTL_READ_STB       _IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL    _IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL     _IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT  _IO(USBTMC_IOC_NR, 21)
 
 /* Driver encoded usb488 capabilities */
 #define USBTMC488_CAPABILITY_TRIGGER         1
--

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

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#9 lore raw

Re: [PATCH v4 5/5] Add ioctls to enable and disable local controls on an instrument

Quoted 9 lines · Andy Shevchenko

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 491

Quoted 5 lines · Dave Penkler

These ioctls provide support for the USBTMC-USB488 control requests for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT

drivers/usb/class/usbtmc.c +2 -0
+
+       dev = &data->intf->dev;

Could be assigned above.

ok

Quoted 4 lines
Quoted 1 line

+ wValue = (val) ? 1 : 0;

Redundant parens.

ok

Quoted 5 lines
Quoted 1 line

+ rv = -EIO;

Hmm??? Does usb_control_msg() return a proper error code? If it does, perhaps better to propagate it.

You are right.

Quoted 7 lines

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 534

Quoted 4 lines
drivers/usb/class/usbtmc.c +3 -0
+               rv = -EIO;
+               goto exit;
 
+       } else {

Redundant else.

OK

Quoted 1 line

Thanks,
-Dave

#10 lore raw
In reply to Dave Penkler
Quoted 5 lines · Dave Penkler

Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations.

Quoted 7 lines

Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (4/5)

Add convenience ioctl to return all device capabilities (5/5)

PATCH Changelog:
v4 - Remove excess newlines and parens

Seems you missed few extra empty lines at least. Please, recheck again.

Quoted 2 lines

- Simplify some expressions
- Remove redundant initialisation

Signature · 2 lines
With Best Regards,
Andy Shevchenko