> On Mon, Apr 29, 2013 at 09:02:24PM -0500, Alexandre Peixoto Ferreita wrote:
>> These patches implement a modification of the USBTMC protocol to allow
>> operation with Rigol equipment. Rigol requires that a single TMC request
>> to receive any buffer size and bulk requests to get the data. The
>> original algorithm sends a TMC request for each subset of the data (a
>> single USB transaction). The modification is only active for Rigol
>> equipment, vendor and product set is contained in the array usbtmc_id_quirk.
>>
>> The patches are tested with Rigol equipment but need more validation
>> with other equipment. The last patch implement an ioctl to dynamically
>> toggle the algorithm allowing udev to change the behavior without
>> recompiling the driver.
>
> Can't you trigger the quirk based on the vendor/product id of your
> device? We can't accept a patch that adds an ioctl just to turn a quirk
> on.
The patch already does that, the ioctl was only to present a way for
udev to set it instead of requiring recompilation for new devices. The
ioctl modification is optional, do you have a recommendation of a way to
set/reset it from user space? module parameter can be an option.
>
>> >From 87c5741be97d98a8f7424d3d35e1dbf758289b7b Mon Sep 17 00:00:00 2001
>> From: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
>> Date: Sun, 14 Apr 2013 20:21:31 -0500
>> Subject: [PATCH 1/2] Allow usbtmc_read to interface with Rigol version of
>> USBTMC but preserve the original operating behavior for other equipments
>
> You need to make your Subject: shorter, and put more information (like
> what you wrote above, in the changelog part of the patch here.
>
The patches are reformatted in smaller and progressive pieces, I hope it
is manageable now. I can try to split the third one in more pieces (a
little more complicated) if is still too big. The new patches are
attached to this email.
> Also, you might want to break this into 3 patches, or at least something
> that is a bit more reviewable, as it is, this one is tough to follow.
>
> thanks,
>From 4742ec04f9aceaec787825f7fe3d267759428dd2 Mon Sep 17 00:00:00 2001
From: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
Date: Mon, 29 Apr 2013 22:59:12 -0500
Subject: [PATCH 1/6] Create and modify the necessary variables
These patches implement a modification of the USBTMC
protocol to allow operation with Rigol equipment. Rigol requires that a
single TMC request to receive any buffer size and bulk requests to get the
data. The original algorithm sends a TMC request for each subset of the data
(a single USB transaction). The modification is only active for Rigol
equipment, vendor and product set is contained in the array usbtmc_id_quirk.
This patch creates the rigol_quirk variable and the arrays for the
idvendor and idproduct.
Signed-off-by: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
---
drivers/usb/class/usbtmc.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 4c5506a..c450b04 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -31,6 +31,8 @@
#include <linux/usb/tmc.h>
+#define RIGOL 1
+#define USBTMC_HEADER_SIZE 12
#define USBTMC_MINOR_BASE 176
/*
@@ -84,6 +86,8 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+ u8 rigol_quirk;
+
/* attributes from the USB TMC spec for this device */
u8 TermChar;
bool TermCharEnabled;
@@ -97,6 +101,16 @@ struct usbtmc_device_data {
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
+struct usbtmc_ID_rigol_quirk {
+ __u16 idVendor;
+ __u16 idProduct;
+};
+
+static const struct usbtmc_ID_rigol_quirk usbtmc_id_quirk[] = {
+ { 0x1ab1, 0x0588 },
+ { 0, 0 }
+};
+
/* Forward declarations */
static struct usb_driver usbtmc_driver;
--
1.8.2.1
>From 892a673c0f369b124087f04286d24868856cb18f Mon Sep 17 00:00:00 2001
From: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
Date: Mon, 29 Apr 2013 23:08:53 -0500
Subject: [PATCH 2/6] Separate the TMC sending from usbtmc_read
These patches implement a modification of the USBTMC
protocol to allow operation with Rigol equipment. The TMC request portion of
the code in function usbtmc_read is segregated to a function
send_request_dev_dep_msg_in as implemented by tommie in
https://github.com/tommie/linux/blob/usbtmc-rigol/drivers/usb/class/usbtmc.c
allowing the reuse later.
Signed-off-by: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
---
drivers/usb/class/usbtmc.c | 85 +++++++++++++++++++++++++++++-----------------
1 file changed, 54 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index c450b04..9c37042 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -375,6 +375,59 @@ exit:
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.
+ *
+ * See the USBTMC specification, Table 4.
+ *
+ * Also updates bTag_last_write.
+ */
+static int send_request_dev_dep_msg_in(struct usbtmc_device_data *data, size_t transfer_size)
+{
+ int retval;
+ u8 buffer[USBTMC_HEADER_SIZE];
+ int actual;
+
+ /* Setup IO buffer for REQUEST_DEV_DEP_MSG_IN message
+ * Refer to class specs for details
+ */
+ buffer[0] = 2;
+ buffer[1] = data->bTag;
+ buffer[2] = ~(data->bTag);
+ buffer[3] = 0; /* Reserved */
+ buffer[4] = (transfer_size) & 255;
+ buffer[5] = ((transfer_size) >> 8) & 255;
+ buffer[6] = ((transfer_size) >> 16) & 255;
+ buffer[7] = ((transfer_size) >> 24) & 255;
+ buffer[8] = data->TermCharEnabled * 2;
+ /* Use term character? */
+ buffer[9] = data->TermChar;
+ buffer[10] = 0; /* Reserved */
+ buffer[11] = 0; /* Reserved */
+
+ /* Send bulk URB */
+ retval = usb_bulk_msg(data->usb_dev,
+ usb_sndbulkpipe(data->usb_dev,
+ data->bulk_out),
+ buffer, USBTMC_HEADER_SIZE, &actual, USBTMC_TIMEOUT);
+
+ /* Store bTag (in case we need to abort) */
+ data->bTag_last_write = data->bTag;
+
+ /* Increment bTag -- and increment again if zero */
+ data->bTag++;
+ if (!data->bTag)
+ (data->bTag)++;
+
+ if (retval < 0) {
+ dev_err(&data->intf->dev, "usb_bulk_msg in send_request_dev_dep_msg_in() returned %d\n", retval);
+ return retval;
+ }
+
+ return 0;
+}
+
static ssize_t usbtmc_read(struct file *filp, char __user *buf,
size_t count, loff_t *f_pos)
{
@@ -411,37 +464,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf,
else
this_part = remaining;
- /* Setup IO buffer for DEV_DEP_MSG_IN message
- * Refer to class specs for details
- */
- buffer[0] = 2;
- buffer[1] = data->bTag;
- buffer[2] = ~(data->bTag);
- buffer[3] = 0; /* Reserved */
- buffer[4] = (this_part) & 255;
- buffer[5] = ((this_part) >> 8) & 255;
- buffer[6] = ((this_part) >> 16) & 255;
- buffer[7] = ((this_part) >> 24) & 255;
- buffer[8] = data->TermCharEnabled * 2;
- /* Use term character? */
- buffer[9] = data->TermChar;
- buffer[10] = 0; /* Reserved */
- buffer[11] = 0; /* Reserved */
-
- /* Send bulk URB */
- retval = usb_bulk_msg(data->usb_dev,
- usb_sndbulkpipe(data->usb_dev,
- data->bulk_out),
- buffer, 12, &actual, USBTMC_TIMEOUT);
-
- /* Store bTag (in case we need to abort) */
- data->bTag_last_write = data->bTag;
-
- /* Increment bTag -- and increment again if zero */
- data->bTag++;
- if (!data->bTag)
- (data->bTag)++;
-
+ retval = send_request_dev_dep_msg_in(data, this_part);
if (retval < 0) {
dev_err(dev, "usb_bulk_msg returned %d\n", retval);
if (data->auto_abort)
--
1.8.2.1
>From 5476e995cf1f14b9c61ef9c3735f0b5161602f17 Mon Sep 17 00:00:00 2001
From: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
Date: Mon, 29 Apr 2013 23:15:48 -0500
Subject: [PATCH 3/6] Determine if the equipment has the quirk
These patches implement a modification of the USBTMC
protocol to allow operation with Rigol equipment. It an idVendor and
idProduct is found on the usbtmc_id_quirk array, the rigol_quirk is set for
this device.
Signed-off-by: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
---
drivers/usb/class/usbtmc.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 9c37042..fd2f90d 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -1040,6 +1040,20 @@ static int usbtmc_probe(struct usb_interface *intf,
mutex_init(&data->io_mutex);
data->zombie = 0;
+ /* Determine if it is a Rigol or not */
+ data->rigol_quirk = 0;
+ dev_dbg(&intf->dev, "Trying to find if device Vendor 0x%04X Product 0x%04X has the RIGOL quirk\n",
+ data->usb_dev->descriptor.idVendor,
+ data->usb_dev->descriptor.idProduct);
+ for(n = 0; usbtmc_id_quirk[n].idVendor > 0; n++) {
+ if ((usbtmc_id_quirk[n].idVendor == data->usb_dev->descriptor.idVendor) &&
+ (usbtmc_id_quirk[n].idProduct == data->usb_dev->descriptor.idProduct)) {
+ dev_dbg(&intf->dev, "Setting this device as having the RIGOL quirk\n");
+ data->rigol_quirk = 1;
+ break;
+ }
+ }
+
/* Initialize USBTMC bTag and other fields */
data->bTag = 1;
data->TermCharEnabled = 0;
--
1.8.2.1
>From 8b021d721b62e534431afdda0d4506a00557074f Mon Sep 17 00:00:00 2001
From: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
Date: Mon, 29 Apr 2013 23:24:56 -0500
Subject: [PATCH 4/6] Main modificaton of the usbtmc_read
These patches implement a modification of the USBTMC
protocol to allow operation with Rigol equipment. The usbtmc_read function is
modified so if the quirk is active, the TMC header is sent with the size of
the data as the whole size of the request. If the quirk is inactive, the TMC
request is sent once per bulk transfer and with size limited to the bulk
transfer size. In the case of the quirk, only the first response contains the
TMC header and the others are just data.
Signed-off-by: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
---
drivers/usb/class/usbtmc.c | 152 +++++++++++++++++++++++++++++++++------------
1 file changed, 114 insertions(+), 38 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index fd2f90d..8ebd06b 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -455,21 +455,39 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf,
goto exit;
}
+ if (data->rigol_quirk) {
+ dev_dbg(dev, "usb_bulk_msg_in: count(%zu)\n", count);
+
+ retval = send_request_dev_dep_msg_in(data, count);
+
+ if (retval < 0) {
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_out(data);
+ goto exit;
+ }
+ }
+
+ /* Loop until we have fetched everything we requested */
remaining = count;
+ this_part = remaining;
done = 0;
while (remaining > 0) {
- if (remaining > USBTMC_SIZE_IOBUFFER - 12 - 3)
- this_part = USBTMC_SIZE_IOBUFFER - 12 - 3;
- else
- this_part = remaining;
+ if (!(data->rigol_quirk)) {
+ dev_dbg(dev, "usb_bulk_msg_in: remaining(%zu), count(%zu)\n", remaining, count);
+
+ if (remaining > USBTMC_SIZE_IOBUFFER - USBTMC_HEADER_SIZE - 3)
+ this_part = USBTMC_SIZE_IOBUFFER - USBTMC_HEADER_SIZE - 3;
+ else
+ this_part = remaining;
retval = send_request_dev_dep_msg_in(data, this_part);
- if (retval < 0) {
+ if (retval < 0) {
dev_err(dev, "usb_bulk_msg returned %d\n", retval);
- if (data->auto_abort)
- usbtmc_ioctl_abort_bulk_out(data);
- goto exit;
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_out(data);
+ goto exit;
+ }
}
/* Send bulk URB */
@@ -479,51 +497,109 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf,
buffer, USBTMC_SIZE_IOBUFFER, &actual,
USBTMC_TIMEOUT);
+ dev_dbg(dev, "usb_bulk_msg: retval(%u), done(%zu), remaining(%zu), actual(%d)\n", retval, done, remaining, actual);
+
/* Store bTag (in case we need to abort) */
data->bTag_last_read = data->bTag;
if (retval < 0) {
- dev_err(dev, "Unable to read data, error %d\n", retval);
+ dev_dbg(dev, "Unable to read data, error %d\n", retval);
if (data->auto_abort)
usbtmc_ioctl_abort_bulk_in(data);
goto exit;
}
- /* How many characters did the instrument send? */
- n_characters = buffer[4] +
- (buffer[5] << 8) +
- (buffer[6] << 16) +
- (buffer[7] << 24);
+ /* Parse header in first packet */
+ if ((done == 0) || (!(data->rigol_quirk))) {
+ /* Sanity checks for the header */
+ if (actual < USBTMC_HEADER_SIZE) {
+ dev_err(dev, "Device sent too small first packet: %u < %u\n", actual, USBTMC_HEADER_SIZE);
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_in(data);
+ goto exit;
+ }
- /* Ensure the instrument doesn't lie about it */
- if(n_characters > actual - 12) {
- dev_err(dev, "Device lies about message size: %u > %d\n", n_characters, actual - 12);
- n_characters = actual - 12;
- }
+ if (buffer[0] != 2) {
+ dev_err(dev, "Device sent reply with wrong MsgID: %u != 2\n", buffer[0]);
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_in(data);
+ goto exit;
+ }
- /* Ensure the instrument doesn't send more back than requested */
- if(n_characters > this_part) {
- dev_err(dev, "Device returns more than requested: %zu > %zu\n", done + n_characters, done + this_part);
- n_characters = this_part;
- }
+ if (buffer[1] != data->bTag_last_write) {
+ dev_err(dev, "Device sent reply with wrong bTag: %u != %u\n", buffer[1], data->bTag_last_write);
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_in(data);
+ goto exit;
+ }
- /* Bound amount of data received by amount of data requested */
- if (n_characters > this_part)
- n_characters = this_part;
+ /* How many characters did the instrument send? */
+ n_characters = buffer[4] +
+ (buffer[5] << 8) +
+ (buffer[6] << 16) +
+ (buffer[7] << 24);
- /* Copy buffer to user space */
- if (copy_to_user(buf + done, &buffer[12], n_characters)) {
- /* There must have been an addressing problem */
- retval = -EFAULT;
- goto exit;
+ if (n_characters > this_part) {
+ dev_err(dev, "Device wants to return more data than requested: %u > %zu\n", n_characters, count);
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_in(data);
+ goto exit;
+ }
+
+ /* Remove the USBTMC header */
+ actual -= USBTMC_HEADER_SIZE;
+
+ /* Check if the message is smaller than requested */
+ if (data->rigol_quirk) {
+ if (remaining > n_characters)
+ remaining = n_characters;
+ /* Remove padding if it exists */
+ if (actual > remaining)
+ actual = remaining;
+ }
+ else {
+ if (this_part > n_characters)
+ this_part = n_characters;
+ /* Remove padding if it exists */
+ if (actual > this_part)
+ actual = this_part;
+ }
+
+ dev_dbg(dev, "Bulk-IN header: N_characters(%u), bTransAttr(%u)\n", n_characters, buffer[8]);
+
+ remaining -= actual;
+
+ /* Terminate if end-of-message bit received from device */
+ if ((buffer[8] & 0x01) && (actual >= n_characters))
+ remaining = 0;
+
+ dev_dbg(dev, "Bulk-IN header: remaining(%zu), buf(%p), buffer(%p) done(%zu)\n", remaining,buf,buffer,done);
+
+
+ /* Copy buffer to user space */
+ if (copy_to_user(buf + done, &buffer[USBTMC_HEADER_SIZE], actual)) {
+ /* There must have been an addressing problem */
+ retval = -EFAULT;
+ goto exit;
+ }
+ done += actual;
}
+ else {
+ if (actual > remaining)
+ actual = remaining;
- done += n_characters;
- /* Terminate if end-of-message bit received from device */
- if ((buffer[8] & 0x01) && (actual >= n_characters + 12))
- remaining = 0;
- else
- remaining -= n_characters;
+ remaining -= actual;
+
+ dev_dbg(dev, "Bulk-IN header cont: actual(%u), done(%zu), remaining(%zu), buf(%p), buffer(%p)\n", actual, done, remaining,buf,buffer);
+
+ /* Copy buffer to user space */
+ if (copy_to_user(buf + done, buffer, actual)) {
+ /* There must have been an addressing problem */
+ retval = -EFAULT;
+ goto exit;
+ }
+ done += actual;
+ }
}
/* Update file position value */
--
1.8.2.1
>From 5cdd103229a511a5077ce0b7f838c0277bbd3a2f Mon Sep 17 00:00:00 2001
From: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
Date: Mon, 29 Apr 2013 23:32:27 -0500
Subject: [PATCH 5/6] Change 12 to USBTMC_HEADER_SIZE when appropriate
These patches implement a modification of the USBTMC
protocol to allow operation with Rigol equipment. Cosmetic change to show
that 12 is the USBTMC header size.
Signed-off-by: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
---
drivers/usb/class/usbtmc.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 8ebd06b..609dbc2 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -640,8 +640,8 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
done = 0;
while (remaining > 0) {
- if (remaining > USBTMC_SIZE_IOBUFFER - 12) {
- this_part = USBTMC_SIZE_IOBUFFER - 12;
+ if (remaining > USBTMC_SIZE_IOBUFFER - USBTMC_HEADER_SIZE) {
+ this_part = USBTMC_SIZE_IOBUFFER - USBTMC_HEADER_SIZE;
buffer[8] = 0;
} else {
this_part = remaining;
@@ -662,13 +662,13 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
buffer[10] = 0; /* Reserved */
buffer[11] = 0; /* Reserved */
- if (copy_from_user(&buffer[12], buf + done, this_part)) {
+ if (copy_from_user(&buffer[USBTMC_HEADER_SIZE], buf + done, this_part)) {
retval = -EFAULT;
goto exit;
}
- n_bytes = roundup(12 + this_part, 4);
- memset(buffer + 12 + this_part, 0, n_bytes - (12 + this_part));
+ n_bytes = roundup(USBTMC_HEADER_SIZE + this_part, 4);
+ memset(buffer + USBTMC_HEADER_SIZE + this_part, 0, n_bytes - (USBTMC_HEADER_SIZE + this_part));
do {
retval = usb_bulk_msg(data->usb_dev,
--
1.8.2.1
>From 536260a9541a53b564e7f7700c6864c5a6b7f351 Mon Sep 17 00:00:00 2001
From: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
Date: Mon, 29 Apr 2013 23:36:06 -0500
Subject: [PATCH 6/6] Create the IOCTL to toggle the quirk (optional).
These patches implement a modification of the USBTMC
protocol to allow operation with Rigol equipment. Ioctl change allowing test
and toggling of the status of the rigol_quirk bit.
Signed-off-by: Alexandre Peixoto Ferreira <alexandref75@xxxxxxxxx>
---
drivers/usb/class/usbtmc.c | 25 +++++++++++++++++++++++++
include/uapi/linux/usb/tmc.h | 1 +
2 files changed, 26 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 609dbc2..20c0b88 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -154,6 +154,28 @@ static int usbtmc_release(struct inode *inode, struct file *file)
return 0;
}
+static int usbtmc_ioctl_rigol_quirk(struct usbtmc_device_data *data,unsigned long arg)
+{
+ struct device *dev;
+
+ dev = &data->intf->dev;
+
+ switch(arg) {
+ case 0 : /* RESET RIGOL QUIRK MODE IF SET */
+ data->rigol_quirk = 0;
+ return 0;
+ case 1 : /* RESET RIGOL QUIRK MODE IF SET */
+ data->rigol_quirk = 1;
+ return 0;
+ case 2 : /* Check it it is set */
+ if (data->rigol_quirk)
+ return 0;
+ else
+ return -EINVAL;
+ }
+ return 0;
+}
+
static int usbtmc_ioctl_abort_bulk_in(struct usbtmc_device_data *data)
{
u8 *buffer;
@@ -1067,6 +1089,9 @@ 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 USBTMC_IOCTL_RIGOL_QUIRK:
+ retval = usbtmc_ioctl_rigol_quirk(data,arg);
+ break;
}
skip_io_on_zombie:
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index c045ae1..1787460 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -39,5 +39,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 USBTMC_IOCTL_RIGOL_QUIRK _IO(USBTMC_IOC_NR, 8)
#endif
--
1.8.2.1
