[PATCH 1/3] input: add support for the USB IR Toy and USB IR Droid

[PATCH 1/3] input: add support for the USB IR Toy and USB IR Droid

[Sean Young]

Add new serio ID for the USB IR Toy and IR Droid IR transceivers.

More information about the hardware:

http://dangerousprototypes.com/docs/USB_Infrared_Toy
https://www.irdroid.com/irdroid-usb-ir-transceiver/

Signed-off-by: Sean Young <sean@mess.org>
---
 include/uapi/linux/serio.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
index ed2a96f43ce4f..00c0be2546e93 100644
--- a/include/uapi/linux/serio.h
+++ b/include/uapi/linux/serio.h
@@ -83,5 +83,6 @@
 #define SERIO_PULSE8_CEC	0x40
 #define SERIO_RAINSHADOW_CEC	0x41
 #define SERIO_FSIA6B	0x42
+#define SERIO_IRTOY	0x43
 
 #endif /* _UAPI_SERIO_H */
-- 
2.26.2

[PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Sean Young]

serio drivers can only send one byte at a time. If the underlying tty
is a usb serial port, then each byte will be put into separate usb
urbs, which is not efficient.

Additionally, the Infrared Toy device refuses to transmit IR if the
IR data is sent one byte at a time. IR data is formatted in u16 values,
and the firmware expects complete u16 values in the packet.

https://github.com/DangerousPrototypes/USB_IR_Toy/blob/master/Firmware-main/IRs.c#L240

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/input/serio/serport.c |  9 +++++++++
 include/linux/serio.h         | 23 ++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
index 8ac970a423de6..887801691dddc 100644
--- a/drivers/input/serio/serport.c
+++ b/drivers/input/serio/serport.c
@@ -47,6 +47,14 @@ static int serport_serio_write(struct serio *serio, unsigned char data)
 	return -(serport->tty->ops->write(serport->tty, &data, 1) != 1);
 }
 
+static int serport_serio_write_buf(struct serio *serio, unsigned char *data,
+				   uint count)
+{
+	struct serport *serport = serio->port_data;
+
+	return -(serport->tty->ops->write(serport->tty, data, count) != count);
+}
+
 static int serport_serio_open(struct serio *serio)
 {
 	struct serport *serport = serio->port_data;
@@ -173,6 +181,7 @@ static ssize_t serport_ldisc_read(struct tty_struct * tty, struct file * file, u
 	serio->id = serport->id;
 	serio->id.type = SERIO_RS232;
 	serio->write = serport_serio_write;
+	serio->write_buf = serport_serio_write_buf;
 	serio->open = serport_serio_open;
 	serio->close = serport_serio_close;
 	serio->port_data = serport;
diff --git a/include/linux/serio.h b/include/linux/serio.h
index 6c27d413da921..3918e56aec51c 100644
--- a/include/linux/serio.h
+++ b/include/linux/serio.h
@@ -32,6 +32,7 @@ struct serio {
 	spinlock_t lock;
 
 	int (*write)(struct serio *, unsigned char);
+	int (*write_buf)(struct serio *serio, unsigned char *buf, uint size);
 	int (*open)(struct serio *);
 	void (*close)(struct serio *);
 	int (*start)(struct serio *);
@@ -121,12 +122,32 @@ void serio_unregister_driver(struct serio_driver *drv);
 
 static inline int serio_write(struct serio *serio, unsigned char data)
 {
-	if (serio->write)
+	if (serio->write_buf)
+		return serio->write_buf(serio, &data, 1);
+	else if (serio->write)
 		return serio->write(serio, data);
 	else
 		return -1;
 }
 
+static inline int serio_write_buf(struct serio *serio, unsigned char *data,
+				  uint size)
+{
+	if (serio->write_buf) {
+		return serio->write_buf(serio, data, size);
+	} else if (serio->write) {
+		int ret;
+
+		do {
+			ret = serio->write(serio, *data++);
+		} while (ret == 0 && --size);
+
+		return ret;
+	} else {
+		return -1;
+	}
+}
+
 static inline void serio_drv_write_wakeup(struct serio *serio)
 {
 	if (serio->drv && serio->drv->write_wakeup)
-- 
2.26.2

[PATCH 3/3] media: rc: add support for Infrared Toy and IR Droid devices

[Sean Young]

These devices are connected via an usb serial port, and need to be
attached using "inputattach -irtoy /dev/ttyACM[0-9]" (patches will
be sent once this is merged).

http://dangerousprototypes.com/docs/USB_Infrared_Toy
https://www.irdroid.com/irdroid-usb-ir-transceiver/

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/Kconfig  |  15 ++
 drivers/media/rc/Makefile |   1 +
 drivers/media/rc/ir_toy.c | 448 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 464 insertions(+)
 create mode 100644 drivers/media/rc/ir_toy.c

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index c18dee6482536..aa690ba6a981a 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -530,6 +530,21 @@ config IR_ZX
 	   To compile this driver as a module, choose M here: the
 	   module will be called zx-irdec.
 
+config IR_TOY
+	tristate "Infrared Toy and IR Droid"
+	depends on RC_CORE
+	select USB
+	select USB_ACM
+	select SERIO
+	select SERIO_SERPORT
+	help
+	   Say Y here if you want to use the Infrared Toy or IR Droid. This
+	   is a serio driver which needs to be attached to the usb serial
+	   port using inputattach.
+
+	   To compile this driver as a module, choose M here: the module will be
+	   called ir_toy.
+
 endif #RC_DEVICES
 
 endif #RC_CORE
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 48d23433b3c06..5bb2932ab1195 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -50,3 +50,4 @@ obj-$(CONFIG_IR_MTK) += mtk-cir.o
 obj-$(CONFIG_IR_ZX) += zx-irdec.o
 obj-$(CONFIG_IR_TANGO) += tango-ir.o
 obj-$(CONFIG_RC_XBOX_DVD) += xbox_remote.o
+obj-$(CONFIG_IR_TOY) += ir_toy.o
diff --git a/drivers/media/rc/ir_toy.c b/drivers/media/rc/ir_toy.c
new file mode 100644
index 0000000000000..8c9dffa002f44
--- /dev/null
+++ b/drivers/media/rc/ir_toy.c
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Infrared Toy and IR Droid RC core driver
+ *
+ * Copyright (C) 2020 Sean Young <sean@mess.org>
+
+ * This driver is based on the lirc driver which can be found here:
+ * https://sourceforge.net/p/lirc/git/ci/master/tree/plugins/irtoy.c
+ * Copyright (C) 2011 Peter Kooiman <pkooiman@gmail.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/serio.h>
+#include <linux/slab.h>
+
+#include <media/rc-core.h>
+
+static u8 COMMAND_VERSION[] = { 'v' };
+// End transmit and repeat reset command so we exit sump mode
+static u8 COMMAND_RESET[] = { 0xff, 0xff, 0, 0, 0, 0, 0 };
+static u8 COMMAND_SMODE_ENTER[] = { 's' };
+static u8 COMMAND_TXSTART[] = { 0x26, 0x24, 0x25, 0x03 };
+
+#define COMMAND_IO_WRITE 0x30
+#define COMMAND_IO_DIRECTION 0x31
+
+#define REPLY_XMITCOUNT 't'
+#define REPLY_XMITSUCCESS 'C'
+#define REPLY_VERSION 'V'
+#define REPLY_SAMPLEMODEPROTO 'S'
+
+#define TIMEOUT_READYFORDATA 1000000
+#define TIMEOUT_FLUSH 20000
+#define TIMEOUT_SMODE_ENTER 500000
+#define TIMEOUT_VERSION 500000
+
+#define LEN_XMITRES 4
+#define LEN_VERSION 4
+#define LEN_SAMPLEMODEPROTO 3
+
+#define MAX_MSG_SIZE 6
+
+#define MIN_FW_SUPPORTING_SETPIN 22
+#define MIN_FW_VERSION 20
+#define UNIT_NS 21333
+#define MAX_TIMEOUT_NS (UNIT_NS * U16_MAX)
+
+#define OPEN_PIN	5	// RA5
+#define RECEIVE_PIN	3	// RA3
+#define SENDING_PIN	4	// RA4
+
+struct irtoy {
+	struct device *dev;
+	struct rc_dev *rc;
+	struct serio *serio;
+
+	u8 rx[MAX_MSG_SIZE];
+	u8 rx_len;
+	u8 rx_needed;
+	bool pulse;
+	struct completion rx_done;
+
+	uint hw_version;
+	uint sw_version;
+	uint proto_version;
+};
+
+static irqreturn_t irtoy_interrupt(struct serio *serio, unsigned char data,
+				   unsigned int flags)
+{
+	struct irtoy *irtoy = serio_get_drvdata(serio);
+
+	// add byte to buffer
+	if (irtoy->rx_len < MAX_MSG_SIZE)
+		irtoy->rx[irtoy->rx_len++] = data;
+
+	if (irtoy->rx_needed == 0 && irtoy->rx_len == 2) {
+		u32 v = be16_to_cpup((__be16 *)irtoy->rx);
+
+		if (v != 0xffff) {
+			struct ir_raw_event rawir = {
+				.pulse = irtoy->pulse,
+				.duration = v * UNIT_NS,
+			};
+
+			ir_raw_event_store_with_timeout(irtoy->rc, &rawir);
+			ir_raw_event_handle(irtoy->rc);
+			irtoy->pulse = !irtoy->pulse;
+		} else {
+			irtoy->pulse = true;
+		}
+
+		irtoy->rx_len = 0;
+	} else if (irtoy->rx_len == irtoy->rx_needed) {
+		complete(&irtoy->rx_done);
+	}
+
+	return IRQ_HANDLED;
+}
+
+// Send a command and wait for a response of rx_len bytes, for at most
+// timeout microseconds. If rxlen is 0, then wait for timeout microseconds
+// and read all response bytes within that period
+static int irtoy_send_and_recv(struct irtoy *irtoy, u8 *cmd, int cmd_len,
+			       int rx_len, int timeout)
+{
+	int err;
+
+	serio_pause_rx(irtoy->serio);
+	init_completion(&irtoy->rx_done);
+	irtoy->rx_len = 0;
+	irtoy->rx_needed = rx_len;
+	serio_continue_rx(irtoy->serio);
+
+	err = serio_write_buf(irtoy->serio, cmd, cmd_len);
+	if (err)
+		return err;
+
+	if (!wait_for_completion_timeout(&irtoy->rx_done,
+					 usecs_to_jiffies(timeout)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+// Wait for timeout for rx_len bytes to arrive
+static int irtoy_recv(struct irtoy *irtoy, int rx_len, int timeout)
+{
+	bool done;
+
+	serio_pause_rx(irtoy->serio);
+	if (irtoy->rx_len >= rx_len) {
+		done = true;
+	} else {
+		init_completion(&irtoy->rx_done);
+		irtoy->rx_needed = rx_len;
+		done = false;
+	}
+	serio_continue_rx(irtoy->serio);
+
+	if (!done && !wait_for_completion_timeout(&irtoy->rx_done,
+						  usecs_to_jiffies(timeout)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int irtoy_send_tx(struct irtoy *irtoy, void *buf, uint size)
+{
+	uint buf_len, buf_size, count, emitted;
+	int err;
+
+	err = irtoy_send_and_recv(irtoy, COMMAND_TXSTART,
+				  sizeof(COMMAND_TXSTART),
+				  1, TIMEOUT_READYFORDATA);
+	if (err) {
+		dev_err(irtoy->dev, "failed to send tx start command: %d\n",
+			err);
+		return err;
+	}
+
+	buf_len = irtoy->rx[0];
+	count = size;
+
+	while (count) {
+		dev_dbg(irtoy->dev, "ready to receive: 0x%02x\n", buf_len);
+
+		if (buf_len == 0) {
+			dev_err(irtoy->dev, "not enough buffer space\n");
+			return -EINVAL;
+		}
+
+		buf_size = min(buf_len, count);
+
+		err = irtoy_send_and_recv(irtoy, buf, buf_size, 1,
+					  TIMEOUT_READYFORDATA);
+		if (err) {
+			dev_err(irtoy->dev, "failed to send tx buffer: %d\n",
+				err);
+			return err;
+		}
+
+		buf_len = irtoy->rx[0];
+
+		buf += buf_size;
+		count -= buf_size;
+	}
+
+	err = irtoy_recv(irtoy, 1 + LEN_XMITRES, TIMEOUT_READYFORDATA);
+	if (err) {
+		dev_err(irtoy->dev, "failed to receive tx result: %d\n", err);
+		return err;
+	}
+
+	dev_dbg(irtoy->dev, "tx result: %*phN", LEN_XMITRES, irtoy->rx + 1);
+
+	if (irtoy->rx[1] != REPLY_XMITCOUNT) {
+		dev_err(irtoy->dev, "invalid byte count indicator\n");
+		return -EINVAL;
+	}
+
+	emitted = be16_to_cpup((__be16 *)(irtoy->rx + 2));
+
+	if (size != emitted) {
+		dev_err(irtoy->dev, "expected %u emitted, got %u\n", count,
+			emitted);
+		return -EINVAL;
+	}
+
+	if (irtoy->rx[4] != REPLY_XMITSUCCESS) {
+		dev_err(irtoy->dev, "invalid byte count indicator\n");
+		return -EINVAL;
+	}
+
+	// switch to raw IR mode
+	irtoy->rx_needed = 0;
+	irtoy->rx_len = 0;
+
+	return 0;
+}
+
+static int irtoy_tx(struct rc_dev *rc, uint *txbuf, uint count)
+{
+	struct irtoy *irtoy = rc->priv;
+	unsigned int i;
+	size_t size;
+	__be16 *buf;
+	int err;
+
+	size = sizeof(u16) * (count + 1);
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		u16 v = DIV_ROUND_CLOSEST(US_TO_NS(txbuf[i]), UNIT_NS);
+
+		if (!v)
+			v = 1;
+		buf[i] = cpu_to_be16(v);
+	}
+
+	buf[i] = cpu_to_be16(0xffff);
+
+	err = irtoy_send_tx(irtoy, buf, size);
+	kfree(buf);
+
+	return err == 0 ? count : err;
+}
+
+static int irtoy_reset(struct irtoy *irtoy)
+{
+	int err;
+
+	err = serio_write_buf(irtoy->serio, COMMAND_RESET,
+			      sizeof(COMMAND_RESET));
+	if (err != 0) {
+		dev_err(irtoy->dev, "could not write reset command: %d\n",
+			err);
+		return err;
+	}
+
+	usleep_range(50, 50);
+
+	return 0;
+}
+
+static int irtoy_setup(struct irtoy *irtoy)
+{
+	uint version;
+	int err;
+
+	// reset device
+	err = irtoy_reset(irtoy);
+	if (err)
+		return err;
+
+	// get version
+	err = irtoy_send_and_recv(irtoy, COMMAND_VERSION,
+				  sizeof(COMMAND_VERSION),
+				  LEN_VERSION, TIMEOUT_VERSION);
+	if (err) {
+		dev_err(irtoy->dev, "could not write version command: %d\n",
+			err);
+		return err;
+	}
+
+	irtoy->rx[LEN_VERSION] = 0;
+
+	if (irtoy->rx[0] != REPLY_VERSION ||
+	    kstrtouint(irtoy->rx + 1, 10, &version)) {
+		dev_err(irtoy->dev, "invalid version %*phN. Please make sure you are using firmware v20 or higher",
+			LEN_VERSION, irtoy->rx);
+		return -ENODEV;
+	}
+
+	dev_dbg(irtoy->dev, "version %s\n", irtoy->rx);
+
+	irtoy->hw_version = version / 100;
+	irtoy->sw_version = version % 100;
+
+	// enter sample mode
+	err = irtoy_send_and_recv(irtoy, COMMAND_SMODE_ENTER,
+				  sizeof(COMMAND_SMODE_ENTER),
+				  LEN_SAMPLEMODEPROTO, TIMEOUT_SMODE_ENTER);
+	if (err) {
+		dev_err(irtoy->dev, "could not write version command: %d\n",
+			err);
+		return err;
+	}
+
+	irtoy->rx[LEN_SAMPLEMODEPROTO] = 0;
+
+	if (irtoy->rx[0] != REPLY_SAMPLEMODEPROTO ||
+	    kstrtouint(irtoy->rx + 1, 10, &version)) {
+		dev_err(irtoy->dev, "invalid sample mode response %*phN",
+			LEN_SAMPLEMODEPROTO, irtoy->rx);
+		return -ENODEV;
+	}
+
+	dev_dbg(irtoy->dev, "protocol %s\n", irtoy->rx);
+
+	irtoy->proto_version = version;
+
+	return 0;
+}
+
+static int irtoy_connect(struct serio *serio, struct serio_driver *drv)
+{
+	struct irtoy *irtoy;
+	struct rc_dev *rc;
+	int err;
+
+	irtoy = kzalloc(sizeof(*irtoy), GFP_KERNEL);
+	if (!irtoy)
+		return -ENOMEM;
+
+	rc = rc_allocate_device(RC_DRIVER_IR_RAW);
+	if (!rc) {
+		err = -ENOMEM;
+		goto free_irtoy;
+	}
+
+	irtoy->serio = serio;
+	irtoy->dev = &serio->dev;
+	irtoy->rc = rc;
+	irtoy->pulse = true;
+
+	serio_set_drvdata(serio, irtoy);
+
+	err = serio_open(serio, drv);
+	if (err)
+		goto free_device;
+
+	err = irtoy_setup(irtoy);
+	if (err)
+		goto close_serio;
+
+	dev_info(irtoy->dev, "version hardware %u, firmware %u, protocol %u",
+		 irtoy->hw_version, irtoy->sw_version, irtoy->proto_version);
+
+	if (irtoy->sw_version < MIN_FW_VERSION) {
+		dev_err(irtoy->dev, "need firmware V%02u or higher",
+			MIN_FW_VERSION);
+		err = -ENODEV;
+		goto close_serio;
+	}
+
+	rc->device_name = dev_name(&serio->dev);
+	rc->driver_name = KBUILD_MODNAME;
+	rc->input_phys = serio->phys;
+	rc->priv = irtoy;
+	rc->dev.parent = &serio->dev;
+	rc->tx_ir = irtoy_tx;
+	rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
+	rc->map_name = RC_MAP_RC6_MCE;
+	rc->rx_resolution = UNIT_NS;
+	rc->timeout = IR_DEFAULT_TIMEOUT;
+	rc->min_timeout = US_TO_NS(1);
+	rc->max_timeout = MAX_TIMEOUT_NS;
+
+	err = rc_register_device(rc);
+	if (err)
+		goto close_serio;
+
+	// switch to raw IR mode
+	irtoy->rx_needed = 0;
+	irtoy->rx_len = 0;
+
+	return 0;
+
+close_serio:
+	serio_close(serio);
+free_device:
+	serio_set_drvdata(serio, NULL);
+	rc_free_device(rc);
+free_irtoy:
+	kfree(irtoy);
+	return err;
+}
+
+static void irtoy_disconnect(struct serio *serio)
+{
+	struct irtoy *irtoy = serio_get_drvdata(serio);
+
+	rc_unregister_device(irtoy->rc);
+
+	irtoy->serio = NULL;
+
+	serio_close(serio);
+	serio_set_drvdata(serio, NULL);
+	kfree(irtoy);
+}
+
+static const struct serio_device_id irtoy_serio_ids[] = {
+	{
+		.type	= SERIO_RS232,
+		.proto	= SERIO_IRTOY,
+		.id	= SERIO_ANY,
+		.extra	= SERIO_ANY,
+	},
+	{ }
+};
+
+MODULE_DEVICE_TABLE(serio, irtoy_serio_ids);
+
+static struct serio_driver irtoy_drv = {
+	.driver		= {
+		.name	= "irtoy",
+	},
+	.description	= "Infrared Toy and IR Droid RC driver",
+	.id_table	= irtoy_serio_ids,
+	.interrupt	= irtoy_interrupt,
+	.connect	= irtoy_connect,
+	.disconnect	= irtoy_disconnect,
+};
+
+module_serio_driver(irtoy_drv);
+
+MODULE_AUTHOR("Sean Young <sean@mess.org>");
+MODULE_DESCRIPTION("Infrared Toy and IR Droid driver");
+MODULE_LICENSE("GPL");
-- 
2.26.2

Re: [PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Dmitry Torokhov]

On Thu, May 07, 2020 at 02:53:36PM +0100, Sean Young wrote:
> serio drivers can only send one byte at a time. If the underlying tty
> is a usb serial port, then each byte will be put into separate usb
> urbs, which is not efficient.
> 
> Additionally, the Infrared Toy device refuses to transmit IR if the
> IR data is sent one byte at a time. IR data is formatted in u16 values,
> and the firmware expects complete u16 values in the packet.
> 
> https://github.com/DangerousPrototypes/USB_IR_Toy/blob/master/Firmware-main/IRs.c#L240

Ummm, serial protocol data size is at most 9 bits so I have no earthly
idea how they expect to get 16.

serio is explicitly byte-oriented layer, of you need to exchange
larger data I'd recommend using USB natively maybe?

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Sean Young]

On Thu, May 07, 2020 at 01:25:46PM -0700, Dmitry Torokhov wrote:
> On Thu, May 07, 2020 at 02:53:36PM +0100, Sean Young wrote:
> > serio drivers can only send one byte at a time. If the underlying tty
> > is a usb serial port, then each byte will be put into separate usb
> > urbs, which is not efficient.
> > 
> > Additionally, the Infrared Toy device refuses to transmit IR if the
> > IR data is sent one byte at a time. IR data is formatted in u16 values,
> > and the firmware expects complete u16 values in the packet.
> > 
> > https://github.com/DangerousPrototypes/USB_IR_Toy/blob/master/Firmware-main/IRs.c#L240
> 
> Ummm, serial protocol data size is at most 9 bits so I have no earthly
> idea how they expect to get 16.

serio is a layer on top several serial protocols, including ttys. ttys allow
more than one byte to be written at a time, see struct tty_operations:

        int  (*write)(struct tty_struct * tty,
                      const unsigned char *buf, int count);

ttys would be very inefficient if you could only write one byte at a time,
and they are very serial.

This patch exposes the underlying tty write() functionality to serio. When
the underlying tty is a usb serial port this makes for far fewer usb packets
being used to send the same data, and fixes my driver problem, and it
would reduce the number of calls in a few other cases too.

I'm happy to rework the patch if there are comments on the style or
approach.

Thanks,

Sean

Re: [PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Greg KH]

On Thu, May 07, 2020 at 09:59:18PM +0100, Sean Young wrote:
> On Thu, May 07, 2020 at 01:25:46PM -0700, Dmitry Torokhov wrote:
> > On Thu, May 07, 2020 at 02:53:36PM +0100, Sean Young wrote:
> > > serio drivers can only send one byte at a time. If the underlying tty
> > > is a usb serial port, then each byte will be put into separate usb
> > > urbs, which is not efficient.
> > > 
> > > Additionally, the Infrared Toy device refuses to transmit IR if the
> > > IR data is sent one byte at a time. IR data is formatted in u16 values,
> > > and the firmware expects complete u16 values in the packet.
> > > 
> > > https://github.com/DangerousPrototypes/USB_IR_Toy/blob/master/Firmware-main/IRs.c#L240
> > 
> > Ummm, serial protocol data size is at most 9 bits so I have no earthly
> > idea how they expect to get 16.
> 
> serio is a layer on top several serial protocols, including ttys. ttys allow
> more than one byte to be written at a time, see struct tty_operations:
> 
>         int  (*write)(struct tty_struct * tty,
>                       const unsigned char *buf, int count);
> 
> ttys would be very inefficient if you could only write one byte at a time,
> and they are very serial.
> 
> This patch exposes the underlying tty write() functionality to serio. When
> the underlying tty is a usb serial port this makes for far fewer usb packets
> being used to send the same data, and fixes my driver problem, and it
> would reduce the number of calls in a few other cases too.
> 
> I'm happy to rework the patch if there are comments on the style or
> approach.

Why not just use the ir-usb.c driver for this device instead?

thanks,

greg k-h

Re: [PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Sean Young]

On Mon, May 11, 2020 at 08:51:18AM +0200, Greg KH wrote:
> On Thu, May 07, 2020 at 09:59:18PM +0100, Sean Young wrote:
> > On Thu, May 07, 2020 at 01:25:46PM -0700, Dmitry Torokhov wrote:
> > > On Thu, May 07, 2020 at 02:53:36PM +0100, Sean Young wrote:
> > > > serio drivers can only send one byte at a time. If the underlying tty
> > > > is a usb serial port, then each byte will be put into separate usb
> > > > urbs, which is not efficient.
> > > > 
> > > > Additionally, the Infrared Toy device refuses to transmit IR if the
> > > > IR data is sent one byte at a time. IR data is formatted in u16 values,
> > > > and the firmware expects complete u16 values in the packet.
> > > > 
> > > > https://github.com/DangerousPrototypes/USB_IR_Toy/blob/master/Firmware-main/IRs.c#L240
> > > 
> > > Ummm, serial protocol data size is at most 9 bits so I have no earthly
> > > idea how they expect to get 16.
> > 
> > serio is a layer on top several serial protocols, including ttys. ttys allow
> > more than one byte to be written at a time, see struct tty_operations:
> > 
> >         int  (*write)(struct tty_struct * tty,
> >                       const unsigned char *buf, int count);
> > 
> > ttys would be very inefficient if you could only write one byte at a time,
> > and they are very serial.
> > 
> > This patch exposes the underlying tty write() functionality to serio. When
> > the underlying tty is a usb serial port this makes for far fewer usb packets
> > being used to send the same data, and fixes my driver problem, and it
> > would reduce the number of calls in a few other cases too.
> > 
> > I'm happy to rework the patch if there are comments on the style or
> > approach.
> 
> Why not just use the ir-usb.c driver for this device instead?

So this device is the infrared kind which rc-core (in drivers/media/rc/)
supports, remotes and such things (not for serial IR). So by using a 
rc-core driver, it can use kernel IR decoding, BPF decoding, lirc chardev
and rc keymaps, etc.

This device is a PIC18F2550 type device, which is a usb serial port
microcontroller type with some firmware and IR diodes:
	http://dangerousprototypes.com/docs/USB_IR_Toy_v2

serio supports a whole bunch of usb serial devices which can be attached
via inputattach(1). Not all of these are input devices, for example there
are two cec devices too.

Now, in many of these drivers, multiple bytes need to be written to the
device in order to send it a command, for example in
drivers/input/touchscreen/elo.c:

        for (i = 0; i < ELO10_PACKET_LEN; i++) {
                csum += packet[i];
                if (serio_write(elo->serio, packet[i]))
                        goto out;
        }

So if serio had an interface for sending a buffer, that would be less
call overhead. In fact, if the underlying serio is a serial usb port,
that would much more efficient on the usb layer too (one usb roundtrips in
stead of ELO10_PACKET_LEN roundtrips), like so:

	serio_write_buf(elo->serio, packet, ELO10_PACKET_LEN);

So what I'm suggesting is extending the serio interface to allow sending
a buffer of bytes. Of course serio isn't just usb serial ports. There quite
a few instances of serio_register_port() in the kernel. Many of them
can be extended to support sending a buffer rather than a single byte,
if this makes sense. For example the ps2 serio port takes a mutex for every
byte, so this could be more efficient by reducing it to one mutex lock
per buffer.

Now it would be nice to have a discussion about this rather than being
dismissed with:

> > > Ummm, serial protocol data size is at most 9 bits so I have no earthly
> > > idea how they expect to get 16.

Which is just a tad insulting.


Sean

Re: [PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Dmitry Torokhov]

On Tue, May 12, 2020 at 10:07:24AM +0100, Sean Young wrote:
> On Mon, May 11, 2020 at 08:51:18AM +0200, Greg KH wrote:
> > On Thu, May 07, 2020 at 09:59:18PM +0100, Sean Young wrote:
> > > On Thu, May 07, 2020 at 01:25:46PM -0700, Dmitry Torokhov wrote:
> > > > On Thu, May 07, 2020 at 02:53:36PM +0100, Sean Young wrote:
> > > > > serio drivers can only send one byte at a time. If the underlying tty
> > > > > is a usb serial port, then each byte will be put into separate usb
> > > > > urbs, which is not efficient.
> > > > > 
> > > > > Additionally, the Infrared Toy device refuses to transmit IR if the
> > > > > IR data is sent one byte at a time. IR data is formatted in u16 values,
> > > > > and the firmware expects complete u16 values in the packet.
> > > > > 
> > > > > https://github.com/DangerousPrototypes/USB_IR_Toy/blob/master/Firmware-main/IRs.c#L240
> > > > 
> > > > Ummm, serial protocol data size is at most 9 bits so I have no earthly
> > > > idea how they expect to get 16.
> > > 
> > > serio is a layer on top several serial protocols, including ttys. ttys allow
> > > more than one byte to be written at a time, see struct tty_operations:
> > > 
> > >         int  (*write)(struct tty_struct * tty,
> > >                       const unsigned char *buf, int count);
> > > 
> > > ttys would be very inefficient if you could only write one byte at a time,
> > > and they are very serial.
> > > 
> > > This patch exposes the underlying tty write() functionality to serio. When
> > > the underlying tty is a usb serial port this makes for far fewer usb packets
> > > being used to send the same data, and fixes my driver problem, and it
> > > would reduce the number of calls in a few other cases too.
> > > 
> > > I'm happy to rework the patch if there are comments on the style or
> > > approach.
> > 
> > Why not just use the ir-usb.c driver for this device instead?
> 
> So this device is the infrared kind which rc-core (in drivers/media/rc/)
> supports, remotes and such things (not for serial IR). So by using a 
> rc-core driver, it can use kernel IR decoding, BPF decoding, lirc chardev
> and rc keymaps, etc.
> 
> This device is a PIC18F2550 type device, which is a usb serial port
> microcontroller type with some firmware and IR diodes:
> 	http://dangerousprototypes.com/docs/USB_IR_Toy_v2
> 
> serio supports a whole bunch of usb serial devices which can be attached
> via inputattach(1). Not all of these are input devices, for example there
> are two cec devices too.
> 
> Now, in many of these drivers, multiple bytes need to be written to the
> device in order to send it a command, for example in
> drivers/input/touchscreen/elo.c:
> 
>         for (i = 0; i < ELO10_PACKET_LEN; i++) {
>                 csum += packet[i];
>                 if (serio_write(elo->serio, packet[i]))
>                         goto out;
>         }
> 
> So if serio had an interface for sending a buffer, that would be less
> call overhead. In fact, if the underlying serio is a serial usb port,
> that would much more efficient on the usb layer too (one usb roundtrips in
> stead of ELO10_PACKET_LEN roundtrips), like so:
> 
> 	serio_write_buf(elo->serio, packet, ELO10_PACKET_LEN);
> 
> So what I'm suggesting is extending the serio interface to allow sending
> a buffer of bytes. Of course serio isn't just usb serial ports. There quite
> a few instances of serio_register_port() in the kernel. Many of them
> can be extended to support sending a buffer rather than a single byte,
> if this makes sense. For example the ps2 serio port takes a mutex for every
> byte, so this could be more efficient by reducing it to one mutex lock
> per buffer.
> 
> Now it would be nice to have a discussion about this rather than being
> dismissed with:
> 
> > > > Ummm, serial protocol data size is at most 9 bits so I have no earthly
> > > > idea how they expect to get 16.
> 
> Which is just a tad insulting.

That was not meant to be insulting, however serial protocol defines that
the data size is at most 9 bits, so expecting that one can transmit
anything more than that _atomically_ is wrong. If your device/firmware
requires 16 bits to be transferred as indivisible units, then serial
port abstraction is wrong one to be used.

Now serio is layer "above" serial ports (but does not have to have
an underlying serial port) that provides byte-oriented communication
that is expected to mostly flow into host. Its does not expect heavy
data flows coming from the host and into the device (if you look at all
the touchscreens, psmouse, etc, they all send initialization sequences
to the device, and then all the data flows into the host). Therefore
there is little benefit in optimizing serio writes.

You are using performance clams as a clutch for the requirement of
sending u16s, but as I mentioned it is wrong if you use serial ports
abstraction layer. Greg mentioned ir-usb. You can maybe enhance it, or
create a similar driver that connects USB to rc-core and ensures that
you can communicate with the device with exact format it needs.

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Greg KH]

On Tue, May 12, 2020 at 10:07:24AM +0100, Sean Young wrote:
> On Mon, May 11, 2020 at 08:51:18AM +0200, Greg KH wrote:
> > On Thu, May 07, 2020 at 09:59:18PM +0100, Sean Young wrote:
> > > On Thu, May 07, 2020 at 01:25:46PM -0700, Dmitry Torokhov wrote:
> > > > On Thu, May 07, 2020 at 02:53:36PM +0100, Sean Young wrote:
> > > > > serio drivers can only send one byte at a time. If the underlying tty
> > > > > is a usb serial port, then each byte will be put into separate usb
> > > > > urbs, which is not efficient.
> > > > > 
> > > > > Additionally, the Infrared Toy device refuses to transmit IR if the
> > > > > IR data is sent one byte at a time. IR data is formatted in u16 values,
> > > > > and the firmware expects complete u16 values in the packet.
> > > > > 
> > > > > https://github.com/DangerousPrototypes/USB_IR_Toy/blob/master/Firmware-main/IRs.c#L240
> > > > 
> > > > Ummm, serial protocol data size is at most 9 bits so I have no earthly
> > > > idea how they expect to get 16.
> > > 
> > > serio is a layer on top several serial protocols, including ttys. ttys allow
> > > more than one byte to be written at a time, see struct tty_operations:
> > > 
> > >         int  (*write)(struct tty_struct * tty,
> > >                       const unsigned char *buf, int count);
> > > 
> > > ttys would be very inefficient if you could only write one byte at a time,
> > > and they are very serial.
> > > 
> > > This patch exposes the underlying tty write() functionality to serio. When
> > > the underlying tty is a usb serial port this makes for far fewer usb packets
> > > being used to send the same data, and fixes my driver problem, and it
> > > would reduce the number of calls in a few other cases too.
> > > 
> > > I'm happy to rework the patch if there are comments on the style or
> > > approach.
> > 
> > Why not just use the ir-usb.c driver for this device instead?
> 
> So this device is the infrared kind which rc-core (in drivers/media/rc/)
> supports, remotes and such things (not for serial IR). So by using a 
> rc-core driver, it can use kernel IR decoding, BPF decoding, lirc chardev
> and rc keymaps, etc.

So why do you want to user serio for this?  serio should only be for
input devices with a serial protocol.

> This device is a PIC18F2550 type device, which is a usb serial port
> microcontroller type with some firmware and IR diodes:
> 	http://dangerousprototypes.com/docs/USB_IR_Toy_v2
> 
> serio supports a whole bunch of usb serial devices which can be attached
> via inputattach(1). Not all of these are input devices, for example there
> are two cec devices too.
> 
> Now, in many of these drivers, multiple bytes need to be written to the
> device in order to send it a command, for example in
> drivers/input/touchscreen/elo.c:
> 
>         for (i = 0; i < ELO10_PACKET_LEN; i++) {
>                 csum += packet[i];
>                 if (serio_write(elo->serio, packet[i]))
>                         goto out;
>         }
> 
> So if serio had an interface for sending a buffer, that would be less
> call overhead. In fact, if the underlying serio is a serial usb port,
> that would much more efficient on the usb layer too (one usb roundtrips in
> stead of ELO10_PACKET_LEN roundtrips), like so:
> 
> 	serio_write_buf(elo->serio, packet, ELO10_PACKET_LEN);
> 
> So what I'm suggesting is extending the serio interface to allow sending
> a buffer of bytes. Of course serio isn't just usb serial ports. There quite
> a few instances of serio_register_port() in the kernel. Many of them
> can be extended to support sending a buffer rather than a single byte,
> if this makes sense. For example the ps2 serio port takes a mutex for every
> byte, so this could be more efficient by reducing it to one mutex lock
> per buffer.
> 
> Now it would be nice to have a discussion about this rather than being
> dismissed with:

I think a custom usb driver that exposes the interfaces as input devices
is going to be the simplest thing for you to do here as you will have
full control over the packet size and format much easier.  Odds are it
will be less work overall for this.

thanks,

greg k-h

Re: [PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Sean Young]

On Wed, May 13, 2020 at 10:16:46AM +0200, Greg KH wrote:
> On Tue, May 12, 2020 at 10:07:24AM +0100, Sean Young wrote:
> > So this device is the infrared kind which rc-core (in drivers/media/rc/)
> > supports, remotes and such things (not for serial IR). So by using a 
> > rc-core driver, it can use kernel IR decoding, BPF decoding, lirc chardev
> > and rc keymaps, etc.
> 
> So why do you want to user serio for this?  serio should only be for
> input devices with a serial protocol.

Admittedly this is a bit tenuous.

What I'm trying to do is write a kernel driver which uses the usb serial
drivers, and not write a poor man's version of usb serial in the IR driver.

> I think a custom usb driver that exposes the interfaces as input devices
> is going to be the simplest thing for you to do here as you will have
> full control over the packet size and format much easier.  Odds are it
> will be less work overall for this.

Admittedly I don't think it will be much code, so maybe it won't be so
ugly. It's just the code duplication I was trying to avoid.

So, I'll go ahead and as you suggest.

Thank you for your time and thoughts on this.


Sean

Re: [PATCH 2/3] input: serio: allow more than one byte to be sent at once

[Sean Young]

On Tue, May 12, 2020 at 10:37:27AM -0700, Dmitry Torokhov wrote:
> On Tue, May 12, 2020 at 10:07:24AM +0100, Sean Young wrote:
> > Now it would be nice to have a discussion about this rather than being
> > dismissed with:
> > 
> > > > > Ummm, serial protocol data size is at most 9 bits so I have no earthly
> > > > > idea how they expect to get 16.
> > 
> > Which is just a tad insulting.
> 
> That was not meant to be insulting, however serial protocol defines that
> the data size is at most 9 bits, so expecting that one can transmit
> anything more than that _atomically_ is wrong. If your device/firmware
> requires 16 bits to be transferred as indivisible units, then serial
> port abstraction is wrong one to be used.

Honestly thank you for explaining that. I had no idea this was an abstract
point about the demarcations of serial port-ness.

There is no physical rs-232 cabling involved at all in this case.

> Now serio is layer "above" serial ports (but does not have to have
> an underlying serial port) that provides byte-oriented communication
> that is expected to mostly flow into host. Its does not expect heavy
> data flows coming from the host and into the device (if you look at all
> the touchscreens, psmouse, etc, they all send initialization sequences
> to the device, and then all the data flows into the host). Therefore
> there is little benefit in optimizing serio writes.

True, I didn't think this would make much of an measurable improvement,
but still, some.

> You are using performance clams as a clutch for the requirement of
> sending u16s, but as I mentioned it is wrong if you use serial ports
> abstraction layer. Greg mentioned ir-usb. You can maybe enhance it, or
> create a similar driver that connects USB to rc-core and ensures that
> you can communicate with the device with exact format it needs.

Yes, I'll go down this route.

Thank you for the discussion, it was very helpful.


Sean