[04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar

[04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar

[Greg Kroah-Hartman]

On Thu, May 17, 2018 at 07:03:28PM +0200, Guido Kiener wrote:
> - add USBTMC488_IOCTL_TRIGGER to send TRIGGER Bulk-OUT header
>   according to Subclass USB488 Specification
> 
>   The usbtmc trigger command is equivalent to the IEEE 488 GET (Group
>   Execute Trigger) action. While the "*TRG" command can be sent as
>   data to perform the same operation, in some situations an instrument
>   will be busy and unable to process the data immediately in which
>   case the USBTMC488_IOCTL_TRIGGER can be used to trigger the
>   instrument with lower latency.
> 
> - add USBTMC_IOCTL_EOM_ENABLE to specify EOM bit for next write()
>   call. Sets Bit 0 of field 'bmTransferAttributes' of DEV_DEP_MSG_OUT
>   Bulk-OUT Header.
> 
>   Allows fine grained control over end of message handling on a
>   per file descriptor basis.
> 
> - add USBTMC_IOCTL_CONFIG_TERMCHAR to control TermChar handling
>   for next read(). Controls field 'TermChar' and Bit 1 of field
>   'bmTransferAttributes' of REQUEST_DEV_DEP_MSG_IN BULK-OUT header.
> 
>   Allows enabling/disabling of terminating a read on reception of
>   term_char individually for each read request.

Why isn't this 3 patches?  Please only do "one thing per patch".

thanks,

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

[04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar

[Guido Kiener]

- add USBTMC488_IOCTL_TRIGGER to send TRIGGER Bulk-OUT header
  according to Subclass USB488 Specification

  The usbtmc trigger command is equivalent to the IEEE 488 GET (Group
  Execute Trigger) action. While the "*TRG" command can be sent as
  data to perform the same operation, in some situations an instrument
  will be busy and unable to process the data immediately in which
  case the USBTMC488_IOCTL_TRIGGER can be used to trigger the
  instrument with lower latency.

- add USBTMC_IOCTL_EOM_ENABLE to specify EOM bit for next write()
  call. Sets Bit 0 of field 'bmTransferAttributes' of DEV_DEP_MSG_OUT
  Bulk-OUT Header.

  Allows fine grained control over end of message handling on a
  per file descriptor basis.

- add USBTMC_IOCTL_CONFIG_TERMCHAR to control TermChar handling
  for next read(). Controls field 'TermChar' and Bit 1 of field
  'bmTransferAttributes' of REQUEST_DEV_DEP_MSG_IN BULK-OUT header.

  Allows enabling/disabling of terminating a read on reception of
  term_char individually for each read request.

Reviewed-by: Steve Bayless <steve_bayless@keysight.com>
Signed-off-by: Dave Penkler <dpenkler@gmail.com>
Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
---
 drivers/usb/class/usbtmc.c   | 111 +++++++++++++++++++++++++++++++++--
 include/uapi/linux/usb/tmc.h |  11 ++++
 2 files changed, 116 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index ad7872003420..152e2daa9644 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -126,6 +126,7 @@ struct usbtmc_file_data {
 	u8             TermChar;
 	bool           TermCharEnabled;
 	bool           auto_abort;
+	u8             eom_val;
 };
 
 /* Forward declarations */
@@ -170,6 +171,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp)
 	file_data->TermChar = data->TermChar;
 	file_data->TermCharEnabled = data->TermCharEnabled;
 	file_data->auto_abort = data->auto_abort;
+	file_data->eom_val = 1;
 
 	INIT_LIST_HEAD(&file_data->file_elem);
 	spin_lock_irq(&data->dev_lock);
@@ -577,6 +579,51 @@ static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
 	return rv;
 }
 
+/*
+ * Sends a TRIGGER Bulk-OUT command message
+ * See the USBTMC-USB488 specification, Table 2.
+ *
+ * Also updates bTag_last_write.
+ */
+static int usbtmc488_ioctl_trigger(struct usbtmc_file_data *file_data)
+{
+	struct usbtmc_device_data *data = file_data->data;
+	int retval;
+	u8 *buffer;
+	int actual;
+
+	buffer = kzalloc(USBTMC_HEADER_SIZE, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	buffer[0] = 128;
+	buffer[1] = data->bTag;
+	buffer[2] = ~data->bTag;
+
+	retval = usb_bulk_msg(data->usb_dev,
+			      usb_sndbulkpipe(data->usb_dev,
+					      data->bulk_out),
+			      buffer, USBTMC_HEADER_SIZE,
+			      &actual, file_data->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++;
+
+	kfree(buffer);
+	if (retval < 0) {
+		dev_err(&data->intf->dev, "%s returned %d\n",
+			__func__, retval);
+		return retval;
+	}
+
+	return 0;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-OUT endpoint.
  * @transfer_size: number of bytes to request from the device.
@@ -829,7 +876,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
 			buffer[8] = 0;
 		} else {
 			this_part = remaining;
-			buffer[8] = 1;
+			buffer[8] = file_data->eom_val;
 		}
 
 		/* Setup IO buffer for DEV_DEP_MSG_OUT message */
@@ -1251,6 +1298,47 @@ static int usbtmc_ioctl_set_timeout(struct usbtmc_file_data *file_data,
 	return 0;
 }
 
+/*
+ * enables/disables sending EOM on write
+ */
+static int usbtmc_ioctl_eom_enable(struct usbtmc_file_data *file_data,
+				void __user *arg)
+{
+	__u8 eom_enable;
+
+	if (copy_from_user(&eom_enable, arg, sizeof(eom_enable)))
+		return -EFAULT;
+
+	if (eom_enable > 1)
+		return -EINVAL;
+
+	file_data->eom_val = eom_enable;
+
+	return 0;
+}
+
+/*
+ * Configure TermChar and TermCharEnable
+ */
+static int usbtmc_ioctl_config_termc(struct usbtmc_file_data *file_data,
+				void __user *arg)
+{
+	struct usbtmc_termchar termc;
+
+	if (copy_from_user(&termc, arg, sizeof(termc)))
+		return -EFAULT;
+
+	if ((termc.term_char_enabled > 1) ||
+		(termc.term_char_enabled &&
+		!(file_data->data->capabilities.device_capabilities & 1)))
+		return -EINVAL;
+
+	file_data->TermChar = termc.term_char;
+	file_data->TermCharEnabled = termc.term_char_enabled;
+
+	return 0;
+}
+
 static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct usbtmc_file_data *file_data;
@@ -1301,12 +1389,19 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 						  (void __user *)arg);
 		break;
 
+	case USBTMC_IOCTL_EOM_ENABLE:
+		retval = usbtmc_ioctl_eom_enable(file_data,
+						 (void __user *)arg);
+		break;
+
+	case USBTMC_IOCTL_CONFIG_TERMCHAR:
+		retval = usbtmc_ioctl_config_termc(file_data,
+						   (void __user *)arg);
+		break;
+
 	case USBTMC488_IOCTL_GET_CAPS:
-		retval = copy_to_user((void __user *)arg,
-				&data->usb488_caps,
-				sizeof(data->usb488_caps));
-		if (retval)
-			retval = -EFAULT;
+		retval = put_user(data->usb488_caps,
+				  (unsigned char __user *)arg);
 		break;
 
 	case USBTMC488_IOCTL_READ_STB:
@@ -1328,6 +1423,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		retval = usbtmc488_ioctl_simple(data, (void __user *)arg,
 						USBTMC488_REQUEST_LOCAL_LOCKOUT);
 		break;
+
+	case USBTMC488_IOCTL_TRIGGER:
+		retval = usbtmc488_ioctl_trigger(file_data);
+		break;
 	}
 
 skip_io_on_zombie:
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index d6de192150f4..60369825691c 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -16,6 +16,8 @@
 #ifndef __LINUX_USB_TMC_H
 #define __LINUX_USB_TMC_H
 
+#include <linux/types.h>   /* __u8 etc */
+
 /* USB TMC status values */
 #define USBTMC_STATUS_SUCCESS				0x01
 #define USBTMC_STATUS_PENDING				0x02
@@ -38,6 +40,11 @@
 #define USBTMC488_REQUEST_GOTO_LOCAL			161
 #define USBTMC488_REQUEST_LOCAL_LOCKOUT			162
 
+struct usbtmc_termchar {
+	__u8 term_char;
+	__u8 term_char_enabled; // bool
+} __attribute__ ((packed));
+
 /* Request values for USBTMC driver's ioctl entry point */
 #define USBTMC_IOC_NR			91
 #define USBTMC_IOCTL_INDICATOR_PULSE	_IO(USBTMC_IOC_NR, 1)
@@ -48,11 +55,15 @@
 #define USBTMC_IOCTL_CLEAR_IN_HALT	_IO(USBTMC_IOC_NR, 7)
 #define USBTMC_IOCTL_GET_TIMEOUT	_IOR(USBTMC_IOC_NR, 9, unsigned int)
 #define USBTMC_IOCTL_SET_TIMEOUT	_IOW(USBTMC_IOC_NR, 10, unsigned int)
+#define USBTMC_IOCTL_EOM_ENABLE	        _IOW(USBTMC_IOC_NR, 11, __u8)
+#define USBTMC_IOCTL_CONFIG_TERMCHAR	_IOW(USBTMC_IOC_NR, 12, struct usbtmc_termchar)
+
 #define USBTMC488_IOCTL_GET_CAPS	_IOR(USBTMC_IOC_NR, 17, unsigned char)
 #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)
+#define USBTMC488_IOCTL_TRIGGER		_IO(USBTMC_IOC_NR, 22)
 
 /* Driver encoded usb488 capabilities */
 #define USBTMC488_CAPABILITY_TRIGGER         1

[04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar

[Guido Kiener]

Zitat von Greg KH <gregkh@linuxfoundation.org>:

> On Thu, May 17, 2018 at 07:03:28PM +0200, Guido Kiener wrote:
>> - add USBTMC488_IOCTL_TRIGGER to send TRIGGER Bulk-OUT header
>>   according to Subclass USB488 Specification
>>
>>   The usbtmc trigger command is equivalent to the IEEE 488 GET (Group
>>   Execute Trigger) action. While the "*TRG" command can be sent as
>>   data to perform the same operation, in some situations an instrument
>>   will be busy and unable to process the data immediately in which
>>   case the USBTMC488_IOCTL_TRIGGER can be used to trigger the
>>   instrument with lower latency.
>>
>> - add USBTMC_IOCTL_EOM_ENABLE to specify EOM bit for next write()
>>   call. Sets Bit 0 of field 'bmTransferAttributes' of DEV_DEP_MSG_OUT
>>   Bulk-OUT Header.
>>
>>   Allows fine grained control over end of message handling on a
>>   per file descriptor basis.
>>
>> - add USBTMC_IOCTL_CONFIG_TERMCHAR to control TermChar handling
>>   for next read(). Controls field 'TermChar' and Bit 1 of field
>>   'bmTransferAttributes' of REQUEST_DEV_DEP_MSG_IN BULK-OUT header.
>>
>>   Allows enabling/disabling of terminating a read on reception of
>>   term_char individually for each read request.
>
> Why isn't this 3 patches?  Please only do "one thing per patch".

I just thought it is clearly arranged. Please let me know when you
want to see a breakup of other patches as well.
However I would be happy when I could save some extra work here.
In the middle of next week I start my holiday.

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

[04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar

[Greg Kroah-Hartman]

On Fri, May 18, 2018 at 12:08:16PM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <gregkh@linuxfoundation.org>:
> 
> > On Thu, May 17, 2018 at 07:03:28PM +0200, Guido Kiener wrote:
> > > - add USBTMC488_IOCTL_TRIGGER to send TRIGGER Bulk-OUT header
> > >   according to Subclass USB488 Specification
> > > 
> > >   The usbtmc trigger command is equivalent to the IEEE 488 GET (Group
> > >   Execute Trigger) action. While the "*TRG" command can be sent as
> > >   data to perform the same operation, in some situations an instrument
> > >   will be busy and unable to process the data immediately in which
> > >   case the USBTMC488_IOCTL_TRIGGER can be used to trigger the
> > >   instrument with lower latency.
> > > 
> > > - add USBTMC_IOCTL_EOM_ENABLE to specify EOM bit for next write()
> > >   call. Sets Bit 0 of field 'bmTransferAttributes' of DEV_DEP_MSG_OUT
> > >   Bulk-OUT Header.
> > > 
> > >   Allows fine grained control over end of message handling on a
> > >   per file descriptor basis.
> > > 
> > > - add USBTMC_IOCTL_CONFIG_TERMCHAR to control TermChar handling
> > >   for next read(). Controls field 'TermChar' and Bit 1 of field
> > >   'bmTransferAttributes' of REQUEST_DEV_DEP_MSG_IN BULK-OUT header.
> > > 
> > >   Allows enabling/disabling of terminating a read on reception of
> > >   term_char individually for each read request.
> > 
> > Why isn't this 3 patches?  Please only do "one thing per patch".
> 
> I just thought it is clearly arranged. Please let me know when you
> want to see a breakup of other patches as well.

I only got this far in the patch series and I stopped.  As you know this
has to be broken up, please do that and we can iterate from there.

Worse case, I take the first few patches and you redo the later ones,
that's a very common process.

> However I would be happy when I could save some extra work here.
> In the middle of next week I start my holiday.

No rush on my end, there's no deadlines in kernel development, send them
when you have them ready :)

thanks,

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

[04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar

[Oliver Neukum]

Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
> +       retval = usb_bulk_msg(data->usb_dev,
> +                             usb_sndbulkpipe(data->usb_dev,
> +                                             data->bulk_out),
> +                             buffer, USBTMC_HEADER_SIZE,
> +                             &actual, file_data->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++;
> +

Independent of whether this needs to be split up, do you really
want to do this regardless of usb_bulk_msg() returning an error?

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

[04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar

[Guido Kiener]

Zitat von Oliver Neukum <oneukum@suse.com>:

> Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
>> +       retval = usb_bulk_msg(data->usb_dev,
>> +                             usb_sndbulkpipe(data->usb_dev,
>> +                                             data->bulk_out),
>> +                             buffer, USBTMC_HEADER_SIZE,
>> +                             &actual, file_data->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++;
>> +
>
> Independent of whether this needs to be split up, do you really
> want to do this regardless of usb_bulk_msg() returning an error?
>
> 	Regards
> 		Oliver


I think it is ok. Our devices do not care much about the
right order of sequence numbers. It is just a hint to abort the last
messages. And the client should know the last message numbers itself.

The current implementation does it the same way.
https://elixir.bootlin.com/linux/v4.17-rc6/source/drivers/usb/class/usbtmc.c#L835

Regards

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