[PATCH v3 0/3] IR driver for USB-UIRT device

[PATCH v3 0/3] IR driver for USB-UIRT device

[Sean Young]

This is a new rc-core driver for the USB-UIRT which you can see here
http://www.usbuirt.com/

This device is supported in lirc, via the usb serial kernel driver. This
driver is both for rc-core, which means it can use kernel/BPF decoding
ec. Also this implement is superior because it can:
 - support learning mode
 - setting transmit carrier
 - larger transmits using streaming tx command

Changes since v2:
 - Fixed race condition is disconnect
 - Removed superfluous kmalloc in short tx

Changes since v1:
 - Review comments from Oliver Neukum
 - Simplified wideband read function

Sean Young (3):
  USB: serial: move ftdi_sio.h into include directories
  media: rc: new driver for USB-UIRT device
  USB: serial: blacklist USB-UIRT when driver is selected

 drivers/media/rc/Kconfig                      |  11 +
 drivers/media/rc/Makefile                     |   1 +
 drivers/media/rc/uirt.c                       | 723 ++++++++++++++++++
 drivers/usb/serial/ftdi_sio.c                 |  10 +-
 .../serial => include/linux/usb}/ftdi_sio.h   |   0
 5 files changed, 744 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/rc/uirt.c
 rename {drivers/usb/serial => include/linux/usb}/ftdi_sio.h (100%)

-- 
2.31.1

[PATCH v3 1/3] USB: serial: move ftdi_sio.h into include directories

[Sean Young]

A following commit adds a new IR driver uirt which needs the ftdi defines;
the hardware uses an fdti usb serial port.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/usb/serial/ftdi_sio.c                        | 2 +-
 {drivers/usb/serial => include/linux/usb}/ftdi_sio.h | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename {drivers/usb/serial => include/linux/usb}/ftdi_sio.h (100%)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index c867592477c9..542073d2f0dd 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -41,7 +41,7 @@
 #include <linux/serial.h>
 #include <linux/gpio/driver.h>
 #include <linux/usb/serial.h>
-#include "ftdi_sio.h"
+#include <linux/usb/ftdi_sio.h>
 #include "ftdi_sio_ids.h"
 
 #define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@kroah.com>, Bill Ryder <bryder@sgi.com>, Kuba Ober <kuba@mareimbrium.org>, Andreas Mohr, Johan Hovold <jhovold@gmail.com>"
diff --git a/drivers/usb/serial/ftdi_sio.h b/include/linux/usb/ftdi_sio.h
similarity index 100%
rename from drivers/usb/serial/ftdi_sio.h
rename to include/linux/usb/ftdi_sio.h
-- 
2.31.1

Re: [PATCH v3 1/3] USB: serial: move ftdi_sio.h into include directories

[Johan Hovold]

On Thu, May 06, 2021 at 01:44:53PM +0100, Sean Young wrote:
> A following commit adds a new IR driver uirt which needs the ftdi defines;
> the hardware uses an fdti usb serial port.

You don't seem to use more than a couple of the FTDI defines in the
driver so please copy those to your implementation instead of moving the
header.

Johan

[PATCH v3 2/3] media: rc: new driver for USB-UIRT device

[Sean Young]

See http://www.usbuirt.com/

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

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index f016b35c2b17..dff85a5bfd1e 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -532,6 +532,17 @@ config IR_TOY
 	   To compile this driver as a module, choose M here: the module will be
 	   called ir_toy.
 
+config IR_UIRT
+	tristate "USB-UIRT"
+	depends on RC_CORE
+	depends on USB_ARCH_HAS_HCD
+	help
+	   Say Y here if you want to use the USB-UIRT. See
+	   http://www.usbuirt.com/
+
+	   To compile this driver as a module, choose M here: the module will be
+	   called uirt.
+
 endif #RC_DEVICES
 
 endif #RC_CORE
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 4927f585ebc2..17037915e742 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_TANGO) += tango-ir.o
 obj-$(CONFIG_RC_XBOX_DVD) += xbox_remote.o
 obj-$(CONFIG_IR_TOY) += ir_toy.o
+obj-$(CONFIG_IR_UIRT) += uirt.o
diff --git a/drivers/media/rc/uirt.c b/drivers/media/rc/uirt.c
new file mode 100644
index 000000000000..0e08187b0b05
--- /dev/null
+++ b/drivers/media/rc/uirt.c
@@ -0,0 +1,723 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * USB-UIRT
+ *
+ * Copyright (C) 2021 Sean Young <sean@mess.org>
+ *
+ * See http://www.usbuirt.com/USB-UIRT%20Command%20Protocol.doc
+ */
+
+#include <linux/completion.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/usb.h>
+#include <linux/slab.h>
+#include <linux/usb/input.h>
+#include <linux/usb/ftdi_sio.h>
+
+#include <media/rc-core.h>
+
+static const u8 CMD_GETVERSION[] = { 0x23, 0xdd };
+static const u8 CMD_SETMODERAW[] = { 0x21, 0xdf };
+static const u8 CMD_SETWIDEBAND[] = { 0x24, 0xdc };
+
+#define UNIT_US 50
+#define IR_TIMEOUT 12500
+#define MAX_PACKET 64
+
+enum cmd_state {
+	CMD_STATE_GETVERSION,
+	CMD_STATE_SETMODERAW,
+	CMD_STATE_SETMODEWIDEBAND,
+	CMD_STATE_IRDATA,
+	CMD_STATE_DOTXRAW,
+	CMD_STATE_STREAMING_TX,
+};
+
+enum rx_state {
+	RX_STATE_INTERSPACE_HIGH,
+	RX_STATE_INTERSPACE_LOW,
+	RX_STATE_ON_HIGH,
+	RX_STATE_ON_LOW,
+	RX_STATE_FREQ_HIGH,
+	RX_STATE_FREQ_LOW,
+	RX_STATE_OFF_HIGH,
+	RX_STATE_OFF_LOW,
+};
+
+struct uirt {
+	struct device *dev;
+	struct usb_device *usbdev;
+
+	struct rc_dev *rc;
+	struct urb *urb_in, *urb_out;
+
+	u8 *in;
+	u8 *out;
+	struct completion cmd_done;
+	u8 freq;
+	u8 high;
+	bool wideband;
+	u32 last_duration;
+
+	enum cmd_state cmd_state;
+	enum rx_state rx_state;
+
+	void *tx_buf;
+	uint tx_len;
+
+	char phys[64];
+};
+
+// read IR in raw mode
+static void uirt_raw_mode(struct uirt *uirt, u32 len)
+{
+	uint i, duration;
+
+	for (i = 2; i < len; i++) {
+		switch (uirt->rx_state) {
+		case RX_STATE_INTERSPACE_HIGH:
+			uirt->rx_state = RX_STATE_INTERSPACE_LOW;
+			break;
+		case RX_STATE_INTERSPACE_LOW:
+			uirt->rx_state = RX_STATE_ON_HIGH;
+			break;
+		case RX_STATE_ON_HIGH:
+			duration = uirt->in[i];
+			if (duration == 0)
+				duration = 1;
+
+			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+				.duration = duration * UNIT_US,
+				.pulse = true,
+			}));
+
+			uirt->rx_state = RX_STATE_OFF_HIGH;
+			break;
+		case RX_STATE_OFF_HIGH:
+			if (uirt->in[i] == 0xff) {
+				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+					.duration = IR_TIMEOUT,
+					.timeout = true,
+				}));
+				uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
+				break;
+			}
+
+			duration = uirt->in[i];
+			if (duration == 0)
+				duration = 1;
+
+			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+				.duration = duration * UNIT_US,
+				.pulse = false,
+			}));
+			uirt->rx_state = RX_STATE_ON_HIGH;
+			break;
+		default:
+			WARN(1, "unreachable state");
+			uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
+			break;
+		}
+	}
+
+	ir_raw_event_handle(uirt->rc);
+}
+
+// Read IR in wideband mode. The byte stream is delivered in packets,
+// and the values which come in two bytes may straddle two packets
+static void uirt_wideband(struct uirt *uirt, u32 len)
+{
+	uint i, duration, carrier, pulses;
+
+	for (i = 2; i < len; i++) {
+		switch (uirt->rx_state) {
+		case RX_STATE_INTERSPACE_HIGH:
+			uirt->rx_state = RX_STATE_INTERSPACE_LOW;
+			break;
+		case RX_STATE_INTERSPACE_LOW:
+			uirt->rx_state = RX_STATE_ON_HIGH;
+			break;
+		case RX_STATE_ON_HIGH:
+			uirt->high = uirt->in[i];
+			uirt->rx_state = RX_STATE_ON_LOW;
+			break;
+		case RX_STATE_ON_LOW:
+			// duration is in 400ns units
+			duration = (uirt->high << 8) | uirt->in[i];
+			uirt->last_duration = duration;
+			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
+				.pulse = true,
+			}));
+			uirt->rx_state = RX_STATE_FREQ_HIGH;
+			break;
+		case RX_STATE_FREQ_HIGH:
+			if (uirt->in[i] & 0x80) {
+				uirt->high = uirt->in[i] & 0x7f;
+				uirt->rx_state = RX_STATE_FREQ_LOW;
+				break;
+			}
+
+			uirt->high = 0;
+			fallthrough;
+		case RX_STATE_FREQ_LOW:
+			pulses = (uirt->high << 8) | uirt->in[i];
+			if (pulses) {
+				dev_dbg(uirt->dev, "carrier duration %u pulses %u",
+					uirt->last_duration, pulses);
+
+				// calculate the Hz of pulses in duration 400ns units
+				carrier = DIV_ROUND_CLOSEST_ULL(pulses * 10000000ull,
+								uirt->last_duration * 4);
+				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+					.carrier = carrier,
+					.carrier_report = true,
+				}));
+			}
+			uirt->rx_state = RX_STATE_OFF_HIGH;
+			break;
+		case RX_STATE_OFF_HIGH:
+			if (uirt->in[i] == 0xff) {
+				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+					.duration = IR_TIMEOUT,
+					.timeout = true,
+				}));
+				uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
+			} else {
+				uirt->high = uirt->in[i];
+				uirt->rx_state = RX_STATE_OFF_LOW;
+			}
+			break;
+		case RX_STATE_OFF_LOW:
+			// duration is in 400ns units
+			duration = (uirt->high << 8) | uirt->in[i];
+			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
+				.pulse = false,
+			}));
+			uirt->rx_state = RX_STATE_ON_HIGH;
+			break;
+		}
+	}
+
+	ir_raw_event_handle(uirt->rc);
+}
+
+static void uirt_response(struct uirt *uirt, u32 len)
+{
+	int i;
+
+	dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in);
+
+	// Do we have more IR to transmit
+	if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 &&
+	    uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) {
+		u32 len;
+		int err;
+
+		len = min_t(u32, uirt->tx_len, MAX_PACKET);
+
+		memcpy(uirt->out, uirt->tx_buf, len);
+		uirt->urb_out->transfer_buffer_length = len;
+
+		uirt->tx_len -= len;
+		uirt->tx_buf += len;
+
+		err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC);
+		if (err != 0)
+			dev_warn(uirt->dev,
+				 "failed to submit out urb: %d\n", err);
+	}
+
+	// if we only have two bytes, it just gives us the serial line status
+	if (len <= 2)
+		return;
+
+	switch (uirt->cmd_state) {
+	case CMD_STATE_GETVERSION:
+		if (len == 10) {
+			// check checksum
+			u8 checksum = 0;
+
+			for (i = 2; i < len; i++)
+				checksum += uirt->in[i];
+
+			if (checksum != 0) {
+				dev_err(uirt->dev, "checksum does not match: %*phN\n",
+					len, uirt->in);
+				return;
+			}
+
+			dev_info(uirt->dev,
+				 "USB-UIRT firmware v%u.%u protocol v%u.%u %02u-%02u-%04u",
+				 uirt->in[2], uirt->in[3], uirt->in[4],
+				 uirt->in[5], uirt->in[6], uirt->in[7],
+				 2000 + uirt->in[8]);
+
+			complete(&uirt->cmd_done);
+			uirt->cmd_state = CMD_STATE_IRDATA;
+			return;
+		}
+		break;
+	case CMD_STATE_DOTXRAW:
+	case CMD_STATE_STREAMING_TX:
+	case CMD_STATE_SETMODERAW:
+	case CMD_STATE_SETMODEWIDEBAND:
+		if (len == 3) {
+			switch (uirt->in[2]) {
+			case 0x20:
+				// 0x20 transmitting is expected during streaming tx
+				if (uirt->cmd_state == CMD_STATE_STREAMING_TX)
+					return;
+
+				if (uirt->cmd_state == CMD_STATE_DOTXRAW)
+					complete(&uirt->cmd_done);
+				else
+					dev_err(uirt->dev, "device transmitting");
+				break;
+			case 0x21:
+				if (uirt->tx_len) {
+					dev_err(uirt->dev, "tx completed with %u left to send",
+						uirt->tx_len);
+				} else {
+					if (uirt->cmd_state == CMD_STATE_SETMODERAW)
+						uirt->wideband = false;
+					if (uirt->cmd_state == CMD_STATE_SETMODEWIDEBAND)
+						uirt->wideband = true;
+
+					complete(&uirt->cmd_done);
+				}
+				break;
+			case 0x80:
+				dev_err(uirt->dev, "checksum error");
+				break;
+			case 0x81:
+				dev_err(uirt->dev, "timeout");
+				break;
+			case 0x82:
+				dev_err(uirt->dev, "command error");
+				break;
+			default:
+				dev_err(uirt->dev, "unknown response");
+			}
+
+			uirt->cmd_state = CMD_STATE_IRDATA;
+			return;
+		}
+	default:
+		break;
+	}
+
+	if (uirt->wideband)
+		uirt_wideband(uirt, len);
+	else
+		uirt_raw_mode(uirt, len);
+}
+
+static void uirt_out_callback(struct urb *urb)
+{
+	struct uirt *uirt = urb->context;
+
+	if (urb->status)
+		dev_warn(uirt->dev, "out urb status: %d\n", urb->status);
+}
+
+static void uirt_in_callback(struct urb *urb)
+{
+	struct uirt *uirt = urb->context;
+	int ret;
+
+	if (urb->status == 0)
+		uirt_response(uirt, urb->actual_length);
+	else
+		dev_dbg(uirt->dev, "in urb status: %d\n", urb->status);
+
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret && ret != -ENODEV)
+		dev_warn(uirt->dev, "failed to resubmit urb: %d\n", ret);
+}
+
+static int uirt_command(struct uirt *uirt, const u8 *cmd, int cmd_len,
+			enum cmd_state state)
+{
+	int err;
+
+	init_completion(&uirt->cmd_done);
+
+	uirt->cmd_state = state;
+
+	memcpy(uirt->out, cmd, cmd_len);
+	uirt->urb_out->transfer_buffer_length = cmd_len;
+
+	err = usb_submit_urb(uirt->urb_out, GFP_KERNEL);
+	if (err != 0) {
+		uirt->cmd_state = CMD_STATE_IRDATA;
+		return err;
+	}
+
+	if (!wait_for_completion_timeout(&uirt->cmd_done,
+					 msecs_to_jiffies(USB_CTRL_SET_TIMEOUT))) {
+		usb_kill_urb(uirt->urb_out);
+		uirt->cmd_state = CMD_STATE_IRDATA;
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int uirt_setup(struct uirt *uirt)
+{
+	int err;
+
+	err = uirt_command(uirt, CMD_SETMODERAW, sizeof(CMD_SETMODERAW),
+			   CMD_STATE_SETMODERAW);
+	if (err) {
+		dev_err(uirt->dev, "set mode raw command failed: %d\n",
+			err);
+		return err;
+	}
+
+	err = uirt_command(uirt, CMD_GETVERSION,
+			   sizeof(CMD_GETVERSION), CMD_STATE_GETVERSION);
+	if (err != 0)
+		dev_err(uirt->dev, "get version command failed: %d\n",
+			err);
+
+	return err;
+}
+
+// IR TX when the data can fit into a single packet (i.e 64 bytes).
+// count must be no larger than 24, else we might overflow the buffer.
+static int uirt_short_tx(struct rc_dev *rc, uint *txbuf, uint count)
+{
+	struct uirt *uirt = rc->priv;
+	u8 out[MAX_PACKET], checksum;
+	u32 i, dest, width, freq;
+	int err;
+
+	out[0] = 0x36; // DOTXRAW
+	out[2] = uirt->freq; // carrier frequency
+	out[3] = 1; // number of repeats
+
+	dest = 7;
+
+	freq = uirt->freq & 0x7f;
+
+	for (i = 0; i < count; i++) {
+		// width = (us / freq) * 2.5
+		width = DIV_ROUND_CLOSEST(txbuf[i] * 5, freq * 2);
+
+		if (width == 0)
+			width = 1;
+		else if (width > 127)
+			out[dest++] = (width >> 8) | 0x80;
+
+		out[dest++] = width;
+	}
+
+	// length of RAWSTRUCT + 1
+	out[1] = dest - 1;
+	// number of bytes in encoded pulse/space
+	out[6] = dest - 7;
+
+	// checksum
+	for (i = 0, checksum = 0; i < dest; i++)
+		checksum -= out[i];
+
+	out[dest++] = checksum;
+
+	uirt->tx_buf = NULL;
+	uirt->tx_len = 0;
+
+	err = uirt_command(uirt, out, dest, CMD_STATE_DOTXRAW);
+	if (err != 0)
+		return err;
+
+	return count;
+}
+
+static int uirt_tx(struct rc_dev *rc, uint *txbuf, uint count)
+{
+	struct uirt *uirt = rc->priv;
+	u8 *out;
+	u32 i, dest, unit_raw, freq, len;
+	int err;
+
+	// streaming tx does not work for short IR; use non-streaming
+	// tx for short IR
+	if (count <= 24)
+		return uirt_short_tx(rc, txbuf, count);
+
+	out = kmalloc(count * 2 + 3, GFP_KERNEL);
+	if (!out)
+		return -ENOMEM;
+
+	out[0] = 0x25; // Streaming Transmit
+	out[1] = 0xdb; // checksum over command (just the previous byte)
+	out[2] = uirt->freq; // carrier frequency
+
+	dest = 3;
+
+	freq = uirt->freq & 0x7f;
+
+	for (i = 0; i < count; i++) {
+		// width = (us / freq) * 2.5
+		unit_raw = DIV_ROUND_CLOSEST(txbuf[i] * 5, freq * 2);
+
+		if (unit_raw == 0)
+			unit_raw = 1;
+		else if (unit_raw > 127)
+			out[dest++] = (unit_raw >> 8) | 0x80;
+
+		out[dest++] = unit_raw;
+	}
+
+	len = min_t(u32, dest, MAX_PACKET);
+
+	uirt->tx_buf = out + len;
+	uirt->tx_len = dest - len;
+
+	err = uirt_command(uirt, out, len, CMD_STATE_STREAMING_TX);
+	kfree(out);
+	if (err != 0)
+		return err;
+
+	return count;
+}
+
+static int uirt_set_tx_carrier(struct rc_dev *dev, uint32_t carrier)
+{
+	struct uirt *uirt = dev->priv;
+
+	if (carrier == 0)
+		// bit 7 must be 1 for unmodulated, lower bits need to
+		// be something that makes sense for tx
+		uirt->freq = 0xc0;
+	else if (carrier >= 20000 && carrier <= 500000)
+		// bit 7 must be 0 for modulated
+		uirt->freq = 2500000 / carrier;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+
+static int uirt_set_rx_wideband(struct rc_dev *dev, int enable)
+{
+	struct uirt *uirt = dev->priv;
+	int err;
+
+	if (enable)
+		err = uirt_command(uirt, CMD_SETWIDEBAND,
+				   sizeof(CMD_SETWIDEBAND),
+				   CMD_STATE_SETMODEWIDEBAND);
+	else
+		err = uirt_command(uirt, CMD_SETMODERAW,
+				   sizeof(CMD_SETMODERAW),
+				   CMD_STATE_SETMODERAW);
+
+	if (err) {
+		dev_err(uirt->dev, "set mode command failed: %d\n",
+			err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int init_ftdi(struct usb_device *udev)
+{
+	int err;
+
+	// set the baud rate
+	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			      FTDI_SIO_SET_BAUDRATE_REQUEST,
+			      FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE,
+			      0x4009, 0x0001,
+			      NULL, 0, USB_CTRL_SET_TIMEOUT);
+	if (err)
+		return err;
+
+	// enabling rts/cts flow control
+	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			      FTDI_SIO_SET_FLOW_CTRL_REQUEST,
+			      FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
+			      0, FTDI_SIO_RTS_CTS_HS,
+			      NULL, 0, USB_CTRL_SET_TIMEOUT);
+	if (err)
+		return err;
+
+	// Set latency in milliseconds. The USB-UIRT will generate a
+	// urb every latency milliseconds (IR or not), so this should be
+	// set as high as possible.
+	//
+	// The USB-UIRT has a IR timeout of 14ms (i.e. it sends the 0xff
+	// byte after 14ms of IR silence. So if we set the timeout to a higher
+	// value, the 0xff just gets sent in a separate packet, and there
+	// is a small delay for the IR to be processed.
+	return usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
+			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
+			      50, 0,
+			      NULL, 0, USB_CTRL_SET_TIMEOUT);
+}
+
+static int uirt_probe(struct usb_interface *intf,
+		      const struct usb_device_id *id)
+{
+	struct usb_host_interface *idesc = intf->cur_altsetting;
+	struct usb_device *usbdev = interface_to_usbdev(intf);
+	struct usb_endpoint_descriptor *ep_in = NULL;
+	struct usb_endpoint_descriptor *ep_out = NULL;
+	struct usb_endpoint_descriptor *ep = NULL;
+	struct uirt *uirt;
+	struct rc_dev *rc;
+	struct urb *urb;
+	int i, pipe, err = -ENOMEM;
+
+	for (i = 0; i < idesc->desc.bNumEndpoints; i++) {
+		ep = &idesc->endpoint[i].desc;
+
+		if (!ep_in && usb_endpoint_is_bulk_in(ep) &&
+		    usb_endpoint_maxp(ep) == MAX_PACKET)
+			ep_in = ep;
+
+		if (!ep_out && usb_endpoint_is_bulk_out(ep) &&
+		    usb_endpoint_maxp(ep) == MAX_PACKET)
+			ep_out = ep;
+	}
+
+	if (!ep_in || !ep_out) {
+		dev_err(&intf->dev, "required endpoints not found\n");
+		return -ENODEV;
+	}
+
+	uirt = kzalloc(sizeof(*uirt), GFP_KERNEL);
+	if (!uirt)
+		return -ENOMEM;
+
+	uirt->in = kmalloc(MAX_PACKET, GFP_KERNEL);
+	if (!uirt->in)
+		goto free_uirt;
+
+	uirt->out = kmalloc(MAX_PACKET, GFP_KERNEL);
+	if (!uirt->out)
+		goto free_uirt;
+
+	rc = rc_allocate_device(RC_DRIVER_IR_RAW);
+	if (!rc)
+		goto free_uirt;
+
+	urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!urb)
+		goto free_rcdev;
+
+	pipe = usb_rcvbulkpipe(usbdev, ep_in->bEndpointAddress);
+	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->in, MAX_PACKET,
+			  uirt_in_callback, uirt);
+	uirt->urb_in = urb;
+
+	urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!urb)
+		goto free_rcdev;
+
+	pipe = usb_sndbulkpipe(usbdev, ep_out->bEndpointAddress);
+	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->out, MAX_PACKET,
+			  uirt_out_callback, uirt);
+
+	uirt->dev = &intf->dev;
+	uirt->usbdev = usbdev;
+	uirt->rc = rc;
+	uirt->urb_out = urb;
+	uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
+
+	err = usb_submit_urb(uirt->urb_in, GFP_KERNEL);
+	if (err != 0) {
+		dev_err(uirt->dev, "failed to submit read urb: %d\n", err);
+		goto free_rcdev;
+	}
+
+	err = init_ftdi(usbdev);
+	if (err) {
+		dev_err(uirt->dev, "failed to setup ftdi: %d\n", err);
+		goto free_rcdev;
+	}
+
+	err = uirt_setup(uirt);
+	if (err)
+		goto free_rcdev;
+
+	usb_make_path(usbdev, uirt->phys, sizeof(uirt->phys));
+
+	rc->device_name = "USB-UIRT";
+	rc->driver_name = KBUILD_MODNAME;
+	rc->input_phys = uirt->phys;
+	usb_to_input_id(usbdev, &rc->input_id);
+	rc->dev.parent = &intf->dev;
+	rc->priv = uirt;
+	rc->tx_ir = uirt_tx;
+	rc->s_tx_carrier = uirt_set_tx_carrier;
+	rc->s_learning_mode = uirt_set_rx_wideband;
+	rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
+	rc->map_name = RC_MAP_RC6_MCE;
+	rc->rx_resolution = UNIT_US;
+	rc->timeout = IR_TIMEOUT;
+
+	uirt_set_tx_carrier(rc, 38000);
+
+	err = rc_register_device(rc);
+	if (err)
+		goto free_rcdev;
+
+	usb_set_intfdata(intf, uirt);
+
+	return 0;
+
+free_rcdev:
+	usb_kill_urb(uirt->urb_in);
+	usb_kill_urb(uirt->urb_out);
+	usb_free_urb(uirt->urb_in);
+	usb_free_urb(uirt->urb_out);
+	rc_free_device(rc);
+free_uirt:
+	kfree(uirt->in);
+	kfree(uirt->out);
+	kfree(uirt);
+	return err;
+}
+
+static void uirt_disconnect(struct usb_interface *intf)
+{
+	struct uirt *ir = usb_get_intfdata(intf);
+
+	rc_unregister_device(ir->rc);
+	usb_set_intfdata(intf, NULL);
+	usb_kill_urb(ir->urb_in);
+	usb_kill_urb(ir->urb_out);
+	usb_free_urb(ir->urb_in);
+	usb_free_urb(ir->urb_out);
+	kfree(ir->in);
+	kfree(ir->out);
+	kfree(ir);
+}
+
+static const struct usb_device_id uirt_table[] = {
+	{ USB_DEVICE(0x0403, 0xf850) },
+	{ }
+};
+
+static struct usb_driver uirt_driver = {
+	.name = KBUILD_MODNAME,
+	.probe = uirt_probe,
+	.disconnect = uirt_disconnect,
+	.id_table = uirt_table,
+};
+
+module_usb_driver(uirt_driver);
+
+MODULE_AUTHOR("Sean Young <sean@mess.org>");
+MODULE_DESCRIPTION("USB-UIRT driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(usb, uirt_table);
-- 
2.31.1

Re: [PATCH v3 2/3] media: rc: new driver for USB-UIRT device

[Johan Hovold]

On Thu, May 06, 2021 at 01:44:54PM +0100, Sean Young wrote:
> See http://www.usbuirt.com/

No proper commit message?

> Signed-off-by: Sean Young <sean@mess.org>

> +// Read IR in wideband mode. The byte stream is delivered in packets,
> +// and the values which come in two bytes may straddle two packets
> +static void uirt_wideband(struct uirt *uirt, u32 len)
> +{
> +	uint i, duration, carrier, pulses;
> +
> +	for (i = 2; i < len; i++) {
> +		switch (uirt->rx_state) {
> +		case RX_STATE_INTERSPACE_HIGH:
> +			uirt->rx_state = RX_STATE_INTERSPACE_LOW;
> +			break;
> +		case RX_STATE_INTERSPACE_LOW:
> +			uirt->rx_state = RX_STATE_ON_HIGH;
> +			break;
> +		case RX_STATE_ON_HIGH:
> +			uirt->high = uirt->in[i];
> +			uirt->rx_state = RX_STATE_ON_LOW;
> +			break;
> +		case RX_STATE_ON_LOW:
> +			// duration is in 400ns units
> +			duration = (uirt->high << 8) | uirt->in[i];
> +			uirt->last_duration = duration;
> +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> +				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
> +				.pulse = true,
> +			}));
> +			uirt->rx_state = RX_STATE_FREQ_HIGH;
> +			break;
> +		case RX_STATE_FREQ_HIGH:
> +			if (uirt->in[i] & 0x80) {
> +				uirt->high = uirt->in[i] & 0x7f;
> +				uirt->rx_state = RX_STATE_FREQ_LOW;
> +				break;
> +			}
> +
> +			uirt->high = 0;
> +			fallthrough;
> +		case RX_STATE_FREQ_LOW:
> +			pulses = (uirt->high << 8) | uirt->in[i];
> +			if (pulses) {
> +				dev_dbg(uirt->dev, "carrier duration %u pulses %u",
> +					uirt->last_duration, pulses);
> +
> +				// calculate the Hz of pulses in duration 400ns units
> +				carrier = DIV_ROUND_CLOSEST_ULL(pulses * 10000000ull,
> +								uirt->last_duration * 4);

Looks like uirt->last_duration comes from the external device and you
don't have any sanity checks to prevent division by zero here.

> +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> +					.carrier = carrier,
> +					.carrier_report = true,
> +				}));
> +			}
> +			uirt->rx_state = RX_STATE_OFF_HIGH;
> +			break;
> +		case RX_STATE_OFF_HIGH:
> +			if (uirt->in[i] == 0xff) {
> +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> +					.duration = IR_TIMEOUT,
> +					.timeout = true,
> +				}));
> +				uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> +			} else {
> +				uirt->high = uirt->in[i];
> +				uirt->rx_state = RX_STATE_OFF_LOW;
> +			}
> +			break;
> +		case RX_STATE_OFF_LOW:
> +			// duration is in 400ns units
> +			duration = (uirt->high << 8) | uirt->in[i];
> +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> +				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
> +				.pulse = false,
> +			}));
> +			uirt->rx_state = RX_STATE_ON_HIGH;
> +			break;
> +		}
> +	}
> +
> +	ir_raw_event_handle(uirt->rc);
> +}
> +
> +static void uirt_response(struct uirt *uirt, u32 len)
> +{
> +	int i;
> +
> +	dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in);
> +
> +	// Do we have more IR to transmit
> +	if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 &&
> +	    uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) {
> +		u32 len;
> +		int err;
> +
> +		len = min_t(u32, uirt->tx_len, MAX_PACKET);
> +
> +		memcpy(uirt->out, uirt->tx_buf, len);
> +		uirt->urb_out->transfer_buffer_length = len;
> +
> +		uirt->tx_len -= len;
> +		uirt->tx_buf += len;
> +
> +		err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC);
> +		if (err != 0)
> +			dev_warn(uirt->dev,
> +				 "failed to submit out urb: %d\n", err);
> +	}
> +
> +	// if we only have two bytes, it just gives us the serial line status
> +	if (len <= 2)
> +		return;
> +
> +	switch (uirt->cmd_state) {
> +	case CMD_STATE_GETVERSION:
> +		if (len == 10) {
> +			// check checksum
> +			u8 checksum = 0;
> +
> +			for (i = 2; i < len; i++)
> +				checksum += uirt->in[i];
> +
> +			if (checksum != 0) {
> +				dev_err(uirt->dev, "checksum does not match: %*phN\n",
> +					len, uirt->in);
> +				return;
> +			}
> +
> +			dev_info(uirt->dev,
> +				 "USB-UIRT firmware v%u.%u protocol v%u.%u %02u-%02u-%04u",
> +				 uirt->in[2], uirt->in[3], uirt->in[4],
> +				 uirt->in[5], uirt->in[6], uirt->in[7],
> +				 2000 + uirt->in[8]);
> +
> +			complete(&uirt->cmd_done);
> +			uirt->cmd_state = CMD_STATE_IRDATA;
> +			return;
> +		}
> +		break;
> +	case CMD_STATE_DOTXRAW:
> +	case CMD_STATE_STREAMING_TX:
> +	case CMD_STATE_SETMODERAW:
> +	case CMD_STATE_SETMODEWIDEBAND:
> +		if (len == 3) {
> +			switch (uirt->in[2]) {
> +			case 0x20:
> +				// 0x20 transmitting is expected during streaming tx
> +				if (uirt->cmd_state == CMD_STATE_STREAMING_TX)
> +					return;
> +
> +				if (uirt->cmd_state == CMD_STATE_DOTXRAW)
> +					complete(&uirt->cmd_done);
> +				else
> +					dev_err(uirt->dev, "device transmitting");
> +				break;
> +			case 0x21:
> +				if (uirt->tx_len) {
> +					dev_err(uirt->dev, "tx completed with %u left to send",
> +						uirt->tx_len);
> +				} else {
> +					if (uirt->cmd_state == CMD_STATE_SETMODERAW)
> +						uirt->wideband = false;
> +					if (uirt->cmd_state == CMD_STATE_SETMODEWIDEBAND)
> +						uirt->wideband = true;
> +
> +					complete(&uirt->cmd_done);
> +				}
> +				break;
> +			case 0x80:
> +				dev_err(uirt->dev, "checksum error");
> +				break;
> +			case 0x81:
> +				dev_err(uirt->dev, "timeout");
> +				break;
> +			case 0x82:
> +				dev_err(uirt->dev, "command error");
> +				break;
> +			default:
> +				dev_err(uirt->dev, "unknown response");
> +			}
> +
> +			uirt->cmd_state = CMD_STATE_IRDATA;
> +			return;
> +		}
> +	default:
> +		break;
> +	}
> +
> +	if (uirt->wideband)
> +		uirt_wideband(uirt, len);
> +	else
> +		uirt_raw_mode(uirt, len);
> +}

Your code assumes that you'll always get one message per transfer, but
since the device uses a regular FTDI chip with a FIFO this isn't
guaranteed. 

> +
> +static void uirt_out_callback(struct urb *urb)
> +{
> +	struct uirt *uirt = urb->context;
> +
> +	if (urb->status)
> +		dev_warn(uirt->dev, "out urb status: %d\n", urb->status);

Disconnecting a device shouldn't print a warning.

> +}
> +
> +static void uirt_in_callback(struct urb *urb)
> +{
> +	struct uirt *uirt = urb->context;
> +	int ret;
> +
> +	if (urb->status == 0)
> +		uirt_response(uirt, urb->actual_length);
> +	else
> +		dev_dbg(uirt->dev, "in urb status: %d\n", urb->status);
> +
> +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (ret && ret != -ENODEV)
> +		dev_warn(uirt->dev, "failed to resubmit urb: %d\n", ret);
> +}

> +static int init_ftdi(struct usb_device *udev)
> +{
> +	int err;
> +
> +	// set the baud rate
> +	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> +			      FTDI_SIO_SET_BAUDRATE_REQUEST,
> +			      FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE,
> +			      0x4009, 0x0001,
> +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> +	if (err)
> +		return err;
> +
> +	// enabling rts/cts flow control
> +	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> +			      FTDI_SIO_SET_FLOW_CTRL_REQUEST,
> +			      FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
> +			      0, FTDI_SIO_RTS_CTS_HS,
> +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> +	if (err)
> +		return err;

Does the device UART actually have RTS wired up?

> +
> +	// Set latency in milliseconds. The USB-UIRT will generate a
> +	// urb every latency milliseconds (IR or not), so this should be
> +	// set as high as possible.
> +	//
> +	// The USB-UIRT has a IR timeout of 14ms (i.e. it sends the 0xff
> +	// byte after 14ms of IR silence. So if we set the timeout to a higher
> +	// value, the 0xff just gets sent in a separate packet, and there
> +	// is a small delay for the IR to be processed.
> +	return usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> +			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
> +			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
> +			      50, 0,

Magic constant; it's not entirely clear from the code (or comment) what
50 is here. The latency go as high as 255 IIRC.

> +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> +}
> +
> +static int uirt_probe(struct usb_interface *intf,
> +		      const struct usb_device_id *id)
> +{
> +	struct usb_host_interface *idesc = intf->cur_altsetting;
> +	struct usb_device *usbdev = interface_to_usbdev(intf);
> +	struct usb_endpoint_descriptor *ep_in = NULL;
> +	struct usb_endpoint_descriptor *ep_out = NULL;
> +	struct usb_endpoint_descriptor *ep = NULL;
> +	struct uirt *uirt;
> +	struct rc_dev *rc;
> +	struct urb *urb;
> +	int i, pipe, err = -ENOMEM;
> +
> +	for (i = 0; i < idesc->desc.bNumEndpoints; i++) {
> +		ep = &idesc->endpoint[i].desc;
> +
> +		if (!ep_in && usb_endpoint_is_bulk_in(ep) &&
> +		    usb_endpoint_maxp(ep) == MAX_PACKET)
> +			ep_in = ep;
> +
> +		if (!ep_out && usb_endpoint_is_bulk_out(ep) &&
> +		    usb_endpoint_maxp(ep) == MAX_PACKET)
> +			ep_out = ep;
> +	}

You should use usb_find_common_endpoints() instead of open coding even
if you'd still need to check the endpoint size with your implementation.

> +
> +	if (!ep_in || !ep_out) {
> +		dev_err(&intf->dev, "required endpoints not found\n");
> +		return -ENODEV;
> +	}
> +
> +	uirt = kzalloc(sizeof(*uirt), GFP_KERNEL);
> +	if (!uirt)
> +		return -ENOMEM;
> +
> +	uirt->in = kmalloc(MAX_PACKET, GFP_KERNEL);
> +	if (!uirt->in)
> +		goto free_uirt;
> +
> +	uirt->out = kmalloc(MAX_PACKET, GFP_KERNEL);
> +	if (!uirt->out)
> +		goto free_uirt;
> +
> +	rc = rc_allocate_device(RC_DRIVER_IR_RAW);
> +	if (!rc)
> +		goto free_uirt;
> +
> +	urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!urb)
> +		goto free_rcdev;
> +
> +	pipe = usb_rcvbulkpipe(usbdev, ep_in->bEndpointAddress);
> +	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->in, MAX_PACKET,
> +			  uirt_in_callback, uirt);
> +	uirt->urb_in = urb;
> +
> +	urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!urb)
> +		goto free_rcdev;
> +
> +	pipe = usb_sndbulkpipe(usbdev, ep_out->bEndpointAddress);
> +	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->out, MAX_PACKET,
> +			  uirt_out_callback, uirt);
> +
> +	uirt->dev = &intf->dev;
> +	uirt->usbdev = usbdev;
> +	uirt->rc = rc;
> +	uirt->urb_out = urb;
> +	uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> +
> +	err = usb_submit_urb(uirt->urb_in, GFP_KERNEL);
> +	if (err != 0) {
> +		dev_err(uirt->dev, "failed to submit read urb: %d\n", err);
> +		goto free_rcdev;
> +	}
> +
> +	err = init_ftdi(usbdev);
> +	if (err) {
> +		dev_err(uirt->dev, "failed to setup ftdi: %d\n", err);
> +		goto free_rcdev;
> +	}
> +
> +	err = uirt_setup(uirt);
> +	if (err)
> +		goto free_rcdev;
> +
> +	usb_make_path(usbdev, uirt->phys, sizeof(uirt->phys));
> +
> +	rc->device_name = "USB-UIRT";
> +	rc->driver_name = KBUILD_MODNAME;
> +	rc->input_phys = uirt->phys;
> +	usb_to_input_id(usbdev, &rc->input_id);
> +	rc->dev.parent = &intf->dev;
> +	rc->priv = uirt;
> +	rc->tx_ir = uirt_tx;
> +	rc->s_tx_carrier = uirt_set_tx_carrier;
> +	rc->s_learning_mode = uirt_set_rx_wideband;
> +	rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> +	rc->map_name = RC_MAP_RC6_MCE;
> +	rc->rx_resolution = UNIT_US;
> +	rc->timeout = IR_TIMEOUT;
> +
> +	uirt_set_tx_carrier(rc, 38000);
> +
> +	err = rc_register_device(rc);
> +	if (err)
> +		goto free_rcdev;
> +
> +	usb_set_intfdata(intf, uirt);
> +
> +	return 0;
> +
> +free_rcdev:
> +	usb_kill_urb(uirt->urb_in);
> +	usb_kill_urb(uirt->urb_out);

Looks lazy to kill the URBs before they've been allocated, better to add
another label even if it's not a bug.

> +	usb_free_urb(uirt->urb_in);
> +	usb_free_urb(uirt->urb_out);
> +	rc_free_device(rc);
> +free_uirt:
> +	kfree(uirt->in);
> +	kfree(uirt->out);
> +	kfree(uirt);
> +	return err;
> +}

Johan

Re: [PATCH v3 2/3] media: rc: new driver for USB-UIRT device

[Sean Young]

On Fri, May 14, 2021 at 01:38:11PM +0200, Johan Hovold wrote:
> On Thu, May 06, 2021 at 01:44:54PM +0100, Sean Young wrote:
> > See http://www.usbuirt.com/
> 
> No proper commit message?

No sure what to say here what's not already in the first line of the commit.

Maybe I could mention the fact this uses fdti usb serial chip.

> > Signed-off-by: Sean Young <sean@mess.org>
> 
> > +// Read IR in wideband mode. The byte stream is delivered in packets,
> > +// and the values which come in two bytes may straddle two packets
> > +static void uirt_wideband(struct uirt *uirt, u32 len)
> > +{
> > +	uint i, duration, carrier, pulses;
> > +
> > +	for (i = 2; i < len; i++) {
> > +		switch (uirt->rx_state) {
> > +		case RX_STATE_INTERSPACE_HIGH:
> > +			uirt->rx_state = RX_STATE_INTERSPACE_LOW;
> > +			break;
> > +		case RX_STATE_INTERSPACE_LOW:
> > +			uirt->rx_state = RX_STATE_ON_HIGH;
> > +			break;
> > +		case RX_STATE_ON_HIGH:
> > +			uirt->high = uirt->in[i];
> > +			uirt->rx_state = RX_STATE_ON_LOW;
> > +			break;
> > +		case RX_STATE_ON_LOW:
> > +			// duration is in 400ns units
> > +			duration = (uirt->high << 8) | uirt->in[i];
> > +			uirt->last_duration = duration;
> > +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > +				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
> > +				.pulse = true,
> > +			}));
> > +			uirt->rx_state = RX_STATE_FREQ_HIGH;
> > +			break;
> > +		case RX_STATE_FREQ_HIGH:
> > +			if (uirt->in[i] & 0x80) {
> > +				uirt->high = uirt->in[i] & 0x7f;
> > +				uirt->rx_state = RX_STATE_FREQ_LOW;
> > +				break;
> > +			}
> > +
> > +			uirt->high = 0;
> > +			fallthrough;
> > +		case RX_STATE_FREQ_LOW:
> > +			pulses = (uirt->high << 8) | uirt->in[i];
> > +			if (pulses) {
> > +				dev_dbg(uirt->dev, "carrier duration %u pulses %u",
> > +					uirt->last_duration, pulses);
> > +
> > +				// calculate the Hz of pulses in duration 400ns units
> > +				carrier = DIV_ROUND_CLOSEST_ULL(pulses * 10000000ull,
> > +								uirt->last_duration * 4);
> 
> Looks like uirt->last_duration comes from the external device and you
> don't have any sanity checks to prevent division by zero here.

Yes, you are right. In fact, if this is zero the reported IR duration is
also garbage. I'll fix this.

> 
> > +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > +					.carrier = carrier,
> > +					.carrier_report = true,
> > +				}));
> > +			}
> > +			uirt->rx_state = RX_STATE_OFF_HIGH;
> > +			break;
> > +		case RX_STATE_OFF_HIGH:
> > +			if (uirt->in[i] == 0xff) {
> > +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > +					.duration = IR_TIMEOUT,
> > +					.timeout = true,
> > +				}));
> > +				uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> > +			} else {
> > +				uirt->high = uirt->in[i];
> > +				uirt->rx_state = RX_STATE_OFF_LOW;
> > +			}
> > +			break;
> > +		case RX_STATE_OFF_LOW:
> > +			// duration is in 400ns units
> > +			duration = (uirt->high << 8) | uirt->in[i];
> > +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > +				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
> > +				.pulse = false,
> > +			}));
> > +			uirt->rx_state = RX_STATE_ON_HIGH;
> > +			break;
> > +		}
> > +	}
> > +
> > +	ir_raw_event_handle(uirt->rc);
> > +}
> > +
> > +static void uirt_response(struct uirt *uirt, u32 len)
> > +{
> > +	int i;
> > +
> > +	dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in);
> > +
> > +	// Do we have more IR to transmit
> > +	if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 &&
> > +	    uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) {
> > +		u32 len;
> > +		int err;
> > +
> > +		len = min_t(u32, uirt->tx_len, MAX_PACKET);
> > +
> > +		memcpy(uirt->out, uirt->tx_buf, len);
> > +		uirt->urb_out->transfer_buffer_length = len;
> > +
> > +		uirt->tx_len -= len;
> > +		uirt->tx_buf += len;
> > +
> > +		err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC);
> > +		if (err != 0)
> > +			dev_warn(uirt->dev,
> > +				 "failed to submit out urb: %d\n", err);
> > +	}
> > +
> > +	// if we only have two bytes, it just gives us the serial line status
> > +	if (len <= 2)
> > +		return;
> > +
> > +	switch (uirt->cmd_state) {
> > +	case CMD_STATE_GETVERSION:
> > +		if (len == 10) {
> > +			// check checksum
> > +			u8 checksum = 0;
> > +
> > +			for (i = 2; i < len; i++)
> > +				checksum += uirt->in[i];
> > +
> > +			if (checksum != 0) {
> > +				dev_err(uirt->dev, "checksum does not match: %*phN\n",
> > +					len, uirt->in);
> > +				return;
> > +			}
> > +
> > +			dev_info(uirt->dev,
> > +				 "USB-UIRT firmware v%u.%u protocol v%u.%u %02u-%02u-%04u",
> > +				 uirt->in[2], uirt->in[3], uirt->in[4],
> > +				 uirt->in[5], uirt->in[6], uirt->in[7],
> > +				 2000 + uirt->in[8]);
> > +
> > +			complete(&uirt->cmd_done);
> > +			uirt->cmd_state = CMD_STATE_IRDATA;
> > +			return;
> > +		}
> > +		break;
> > +	case CMD_STATE_DOTXRAW:
> > +	case CMD_STATE_STREAMING_TX:
> > +	case CMD_STATE_SETMODERAW:
> > +	case CMD_STATE_SETMODEWIDEBAND:
> > +		if (len == 3) {
> > +			switch (uirt->in[2]) {
> > +			case 0x20:
> > +				// 0x20 transmitting is expected during streaming tx
> > +				if (uirt->cmd_state == CMD_STATE_STREAMING_TX)
> > +					return;
> > +
> > +				if (uirt->cmd_state == CMD_STATE_DOTXRAW)
> > +					complete(&uirt->cmd_done);
> > +				else
> > +					dev_err(uirt->dev, "device transmitting");
> > +				break;
> > +			case 0x21:
> > +				if (uirt->tx_len) {
> > +					dev_err(uirt->dev, "tx completed with %u left to send",
> > +						uirt->tx_len);
> > +				} else {
> > +					if (uirt->cmd_state == CMD_STATE_SETMODERAW)
> > +						uirt->wideband = false;
> > +					if (uirt->cmd_state == CMD_STATE_SETMODEWIDEBAND)
> > +						uirt->wideband = true;
> > +
> > +					complete(&uirt->cmd_done);
> > +				}
> > +				break;
> > +			case 0x80:
> > +				dev_err(uirt->dev, "checksum error");
> > +				break;
> > +			case 0x81:
> > +				dev_err(uirt->dev, "timeout");
> > +				break;
> > +			case 0x82:
> > +				dev_err(uirt->dev, "command error");
> > +				break;
> > +			default:
> > +				dev_err(uirt->dev, "unknown response");
> > +			}
> > +
> > +			uirt->cmd_state = CMD_STATE_IRDATA;
> > +			return;
> > +		}
> > +	default:
> > +		break;
> > +	}
> > +
> > +	if (uirt->wideband)
> > +		uirt_wideband(uirt, len);
> > +	else
> > +		uirt_raw_mode(uirt, len);
> > +}
> 
> Your code assumes that you'll always get one message per transfer, but
> since the device uses a regular FTDI chip with a FIFO this isn't
> guaranteed. 

I guess you're talking about that data can straddle multiple packets
because there is a fifo involved, or there can be more data before/after
a command response.

Let me see what I can do about that.

> > +
> > +static void uirt_out_callback(struct urb *urb)
> > +{
> > +	struct uirt *uirt = urb->context;
> > +
> > +	if (urb->status)
> > +		dev_warn(uirt->dev, "out urb status: %d\n", urb->status);
> 
> Disconnecting a device shouldn't print a warning.

Ok.

> > +}
> > +
> > +static void uirt_in_callback(struct urb *urb)
> > +{
> > +	struct uirt *uirt = urb->context;
> > +	int ret;
> > +
> > +	if (urb->status == 0)
> > +		uirt_response(uirt, urb->actual_length);
> > +	else
> > +		dev_dbg(uirt->dev, "in urb status: %d\n", urb->status);
> > +
> > +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> > +	if (ret && ret != -ENODEV)
> > +		dev_warn(uirt->dev, "failed to resubmit urb: %d\n", ret);
> > +}
> 
> > +static int init_ftdi(struct usb_device *udev)
> > +{
> > +	int err;
> > +
> > +	// set the baud rate
> > +	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > +			      FTDI_SIO_SET_BAUDRATE_REQUEST,
> > +			      FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE,
> > +			      0x4009, 0x0001,
> > +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> > +	if (err)
> > +		return err;
> > +
> > +	// enabling rts/cts flow control
> > +	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > +			      FTDI_SIO_SET_FLOW_CTRL_REQUEST,
> > +			      FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
> > +			      0, FTDI_SIO_RTS_CTS_HS,
> > +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> > +	if (err)
> > +		return err;
> 
> Does the device UART actually have RTS wired up?

I don't know TBH. However it does have CTS wired up.

> > +
> > +	// Set latency in milliseconds. The USB-UIRT will generate a
> > +	// urb every latency milliseconds (IR or not), so this should be
> > +	// set as high as possible.
> > +	//
> > +	// The USB-UIRT has a IR timeout of 14ms (i.e. it sends the 0xff
> > +	// byte after 14ms of IR silence. So if we set the timeout to a higher
> > +	// value, the 0xff just gets sent in a separate packet, and there
> > +	// is a small delay for the IR to be processed.
> > +	return usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > +			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
> > +			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
> > +			      50, 0,
> 
> Magic constant; it's not entirely clear from the code (or comment) what
> 50 is here. The latency go as high as 255 IIRC.

You're right, the comment is not clear. I'll update it.

> > +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> > +}
> > +
> > +static int uirt_probe(struct usb_interface *intf,
> > +		      const struct usb_device_id *id)
> > +{
> > +	struct usb_host_interface *idesc = intf->cur_altsetting;
> > +	struct usb_device *usbdev = interface_to_usbdev(intf);
> > +	struct usb_endpoint_descriptor *ep_in = NULL;
> > +	struct usb_endpoint_descriptor *ep_out = NULL;
> > +	struct usb_endpoint_descriptor *ep = NULL;
> > +	struct uirt *uirt;
> > +	struct rc_dev *rc;
> > +	struct urb *urb;
> > +	int i, pipe, err = -ENOMEM;
> > +
> > +	for (i = 0; i < idesc->desc.bNumEndpoints; i++) {
> > +		ep = &idesc->endpoint[i].desc;
> > +
> > +		if (!ep_in && usb_endpoint_is_bulk_in(ep) &&
> > +		    usb_endpoint_maxp(ep) == MAX_PACKET)
> > +			ep_in = ep;
> > +
> > +		if (!ep_out && usb_endpoint_is_bulk_out(ep) &&
> > +		    usb_endpoint_maxp(ep) == MAX_PACKET)
> > +			ep_out = ep;
> > +	}
> 
> You should use usb_find_common_endpoints() instead of open coding even
> if you'd still need to check the endpoint size with your implementation.

I was not aware of that function, thanks. This makes the code much shorter.

> > +
> > +	if (!ep_in || !ep_out) {
> > +		dev_err(&intf->dev, "required endpoints not found\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	uirt = kzalloc(sizeof(*uirt), GFP_KERNEL);
> > +	if (!uirt)
> > +		return -ENOMEM;
> > +
> > +	uirt->in = kmalloc(MAX_PACKET, GFP_KERNEL);
> > +	if (!uirt->in)
> > +		goto free_uirt;
> > +
> > +	uirt->out = kmalloc(MAX_PACKET, GFP_KERNEL);
> > +	if (!uirt->out)
> > +		goto free_uirt;
> > +
> > +	rc = rc_allocate_device(RC_DRIVER_IR_RAW);
> > +	if (!rc)
> > +		goto free_uirt;
> > +
> > +	urb = usb_alloc_urb(0, GFP_KERNEL);
> > +	if (!urb)
> > +		goto free_rcdev;
> > +
> > +	pipe = usb_rcvbulkpipe(usbdev, ep_in->bEndpointAddress);
> > +	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->in, MAX_PACKET,
> > +			  uirt_in_callback, uirt);
> > +	uirt->urb_in = urb;
> > +
> > +	urb = usb_alloc_urb(0, GFP_KERNEL);
> > +	if (!urb)
> > +		goto free_rcdev;
> > +
> > +	pipe = usb_sndbulkpipe(usbdev, ep_out->bEndpointAddress);
> > +	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->out, MAX_PACKET,
> > +			  uirt_out_callback, uirt);
> > +
> > +	uirt->dev = &intf->dev;
> > +	uirt->usbdev = usbdev;
> > +	uirt->rc = rc;
> > +	uirt->urb_out = urb;
> > +	uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> > +
> > +	err = usb_submit_urb(uirt->urb_in, GFP_KERNEL);
> > +	if (err != 0) {
> > +		dev_err(uirt->dev, "failed to submit read urb: %d\n", err);
> > +		goto free_rcdev;
> > +	}
> > +
> > +	err = init_ftdi(usbdev);
> > +	if (err) {
> > +		dev_err(uirt->dev, "failed to setup ftdi: %d\n", err);
> > +		goto free_rcdev;
> > +	}
> > +
> > +	err = uirt_setup(uirt);
> > +	if (err)
> > +		goto free_rcdev;
> > +
> > +	usb_make_path(usbdev, uirt->phys, sizeof(uirt->phys));
> > +
> > +	rc->device_name = "USB-UIRT";
> > +	rc->driver_name = KBUILD_MODNAME;
> > +	rc->input_phys = uirt->phys;
> > +	usb_to_input_id(usbdev, &rc->input_id);
> > +	rc->dev.parent = &intf->dev;
> > +	rc->priv = uirt;
> > +	rc->tx_ir = uirt_tx;
> > +	rc->s_tx_carrier = uirt_set_tx_carrier;
> > +	rc->s_learning_mode = uirt_set_rx_wideband;
> > +	rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> > +	rc->map_name = RC_MAP_RC6_MCE;
> > +	rc->rx_resolution = UNIT_US;
> > +	rc->timeout = IR_TIMEOUT;
> > +
> > +	uirt_set_tx_carrier(rc, 38000);
> > +
> > +	err = rc_register_device(rc);
> > +	if (err)
> > +		goto free_rcdev;
> > +
> > +	usb_set_intfdata(intf, uirt);
> > +
> > +	return 0;
> > +
> > +free_rcdev:
> > +	usb_kill_urb(uirt->urb_in);
> > +	usb_kill_urb(uirt->urb_out);
> 
> Looks lazy to kill the URBs before they've been allocated, better to add
> another label even if it's not a bug.

Ok, will do.

> > +	usb_free_urb(uirt->urb_in);
> > +	usb_free_urb(uirt->urb_out);
> > +	rc_free_device(rc);
> > +free_uirt:
> > +	kfree(uirt->in);
> > +	kfree(uirt->out);
> > +	kfree(uirt);
> > +	return err;
> > +}

Thank you for the review. I'll send out a v4 when its ready.


Sean

Re: [PATCH v3 2/3] media: rc: new driver for USB-UIRT device

[Johan Hovold]

On Sat, May 15, 2021 at 10:52:41AM +0100, Sean Young wrote:
> On Fri, May 14, 2021 at 01:38:11PM +0200, Johan Hovold wrote:
> > On Thu, May 06, 2021 at 01:44:54PM +0100, Sean Young wrote:
> > > See http://www.usbuirt.com/
> > 
> > No proper commit message?
> 
> No sure what to say here what's not already in the first line of the
> commit.
> 
> Maybe I could mention the fact this uses fdti usb serial chip.

That'd be good. Perhaps mention why you chose to implement a kernel
driver for this device which appears to already be supported by the lirc
daemon from user space too.
 
> > > +static void uirt_response(struct uirt *uirt, u32 len)
> > > +{
> > > +	int i;
> > > +
> > > +	dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in);
> > > +
> > > +	// Do we have more IR to transmit
> > > +	if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 &&
> > > +	    uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) {
> > > +		u32 len;
> > > +		int err;
> > > +
> > > +		len = min_t(u32, uirt->tx_len, MAX_PACKET);
> > > +
> > > +		memcpy(uirt->out, uirt->tx_buf, len);
> > > +		uirt->urb_out->transfer_buffer_length = len;
> > > +
> > > +		uirt->tx_len -= len;
> > > +		uirt->tx_buf += len;
> > > +
> > > +		err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC);
> > > +		if (err != 0)
> > > +			dev_warn(uirt->dev,
> > > +				 "failed to submit out urb: %d\n", err);
> > > +	}
> > > +
> > > +	// if we only have two bytes, it just gives us the serial line status
> > > +	if (len <= 2)
> > > +		return;
> > > +
> > > +	switch (uirt->cmd_state) {
> > > +	case CMD_STATE_GETVERSION:
> > > +		if (len == 10) {
> > > +			// check checksum
> > > +			u8 checksum = 0;
> > > +
> > > +			for (i = 2; i < len; i++)
> > > +				checksum += uirt->in[i];
> > > +
> > > +			if (checksum != 0) {
> > > +				dev_err(uirt->dev, "checksum does not match: %*phN\n",
> > > +					len, uirt->in);
> > > +				return;
> > > +			}
> > > +
> > > +			dev_info(uirt->dev,
> > > +				 "USB-UIRT firmware v%u.%u protocol v%u.%u %02u-%02u-%04u",
> > > +				 uirt->in[2], uirt->in[3], uirt->in[4],
> > > +				 uirt->in[5], uirt->in[6], uirt->in[7],
> > > +				 2000 + uirt->in[8]);
> > > +
> > > +			complete(&uirt->cmd_done);
> > > +			uirt->cmd_state = CMD_STATE_IRDATA;
> > > +			return;
> > > +		}
> > > +		break;
> > > +	case CMD_STATE_DOTXRAW:
> > > +	case CMD_STATE_STREAMING_TX:
> > > +	case CMD_STATE_SETMODERAW:
> > > +	case CMD_STATE_SETMODEWIDEBAND:
> > > +		if (len == 3) {
> > > +			switch (uirt->in[2]) {
> > > +			case 0x20:
> > > +				// 0x20 transmitting is expected during streaming tx
> > > +				if (uirt->cmd_state == CMD_STATE_STREAMING_TX)
> > > +					return;
> > > +
> > > +				if (uirt->cmd_state == CMD_STATE_DOTXRAW)
> > > +					complete(&uirt->cmd_done);
> > > +				else
> > > +					dev_err(uirt->dev, "device transmitting");
> > > +				break;
> > > +			case 0x21:
> > > +				if (uirt->tx_len) {
> > > +					dev_err(uirt->dev, "tx completed with %u left to send",
> > > +						uirt->tx_len);
> > > +				} else {
> > > +					if (uirt->cmd_state == CMD_STATE_SETMODERAW)
> > > +						uirt->wideband = false;
> > > +					if (uirt->cmd_state == CMD_STATE_SETMODEWIDEBAND)
> > > +						uirt->wideband = true;
> > > +
> > > +					complete(&uirt->cmd_done);
> > > +				}
> > > +				break;
> > > +			case 0x80:
> > > +				dev_err(uirt->dev, "checksum error");
> > > +				break;
> > > +			case 0x81:
> > > +				dev_err(uirt->dev, "timeout");
> > > +				break;
> > > +			case 0x82:
> > > +				dev_err(uirt->dev, "command error");
> > > +				break;
> > > +			default:
> > > +				dev_err(uirt->dev, "unknown response");
> > > +			}
> > > +
> > > +			uirt->cmd_state = CMD_STATE_IRDATA;
> > > +			return;
> > > +		}
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	if (uirt->wideband)
> > > +		uirt_wideband(uirt, len);
> > > +	else
> > > +		uirt_raw_mode(uirt, len);
> > > +}
> > 
> > Your code assumes that you'll always get one message per transfer, but
> > since the device uses a regular FTDI chip with a FIFO this isn't
> > guaranteed. 
> 
> I guess you're talking about that data can straddle multiple packets
> because there is a fifo involved, or there can be more data before/after
> a command response.

Right.

> Let me see what I can do about that.

You'd get buffering for free if you let the tty layer handle this...

> > > +static int init_ftdi(struct usb_device *udev)
> > > +{
> > > +	int err;
> > > +
> > > +	// set the baud rate
> > > +	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > > +			      FTDI_SIO_SET_BAUDRATE_REQUEST,
> > > +			      FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE,
> > > +			      0x4009, 0x0001,
> > > +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	// enabling rts/cts flow control
> > > +	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > > +			      FTDI_SIO_SET_FLOW_CTRL_REQUEST,
> > > +			      FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
> > > +			      0, FTDI_SIO_RTS_CTS_HS,
> > > +			      NULL, 0, USB_CTRL_SET_TIMEOUT);
> > > +	if (err)
> > > +		return err;
> > 
> > Does the device UART actually have RTS wired up?
> 
> I don't know TBH. However it does have CTS wired up.

When using a serial driver, RTS would be asserted on open and
(typically) deasserted on close. Not sure it matters.

The FTDI driver would also clear the receive FIFO when opening the port
I believe.

Johan

[PATCH v3 3/3] USB: serial: blacklist USB-UIRT when driver is selected

[Sean Young]

The USB-UIRT device has its own driver, so blacklist the fdti driver
from using it if the driver has been enabled.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/usb/serial/ftdi_sio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 542073d2f0dd..2320bda57796 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -95,7 +95,9 @@ static int   ftdi_jtag_probe(struct usb_serial *serial);
 static int   ftdi_NDI_device_setup(struct usb_serial *serial);
 static int   ftdi_stmclite_probe(struct usb_serial *serial);
 static int   ftdi_8u2232c_probe(struct usb_serial *serial);
+#if !IS_ENABLED(CONFIG_IR_UIRT)
 static void  ftdi_USB_UIRT_setup(struct ftdi_private *priv);
+#endif
 static void  ftdi_HE_TIRA1_setup(struct ftdi_private *priv);
 
 static const struct ftdi_sio_quirk ftdi_jtag_quirk = {
@@ -106,9 +108,11 @@ static const struct ftdi_sio_quirk ftdi_NDI_device_quirk = {
 	.probe	= ftdi_NDI_device_setup,
 };
 
+#if !IS_ENABLED(CONFIG_IR_UIRT)
 static const struct ftdi_sio_quirk ftdi_USB_UIRT_quirk = {
 	.port_probe = ftdi_USB_UIRT_setup,
 };
+#endif
 
 static const struct ftdi_sio_quirk ftdi_HE_TIRA1_quirk = {
 	.port_probe = ftdi_HE_TIRA1_setup,
@@ -568,8 +572,10 @@ static const struct usb_device_id id_table_combined[] = {
 	{ USB_DEVICE(OCT_VID, OCT_DK201_PID) },
 	{ USB_DEVICE(FTDI_VID, FTDI_HE_TIRA1_PID),
 		.driver_info = (kernel_ulong_t)&ftdi_HE_TIRA1_quirk },
+#if !IS_ENABLED(CONFIG_IR_UIRT)
 	{ USB_DEVICE(FTDI_VID, FTDI_USB_UIRT_PID),
 		.driver_info = (kernel_ulong_t)&ftdi_USB_UIRT_quirk },
+#endif
 	{ USB_DEVICE(FTDI_VID, PROTEGO_SPECIAL_1) },
 	{ USB_DEVICE(FTDI_VID, PROTEGO_R2X0) },
 	{ USB_DEVICE(FTDI_VID, PROTEGO_SPECIAL_3) },
@@ -2292,6 +2298,7 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
 	return 0;
 }
 
+#if !IS_ENABLED(CONFIG_IR_UIRT)
 /* Setup for the USB-UIRT device, which requires hardwired
  * baudrate (38400 gets mapped to 312500) */
 /* Called from usbserial:serial_probe */
@@ -2301,6 +2308,7 @@ static void ftdi_USB_UIRT_setup(struct ftdi_private *priv)
 	priv->custom_divisor = 77;
 	priv->force_baud = 38400;
 }
+#endif
 
 /* Setup for the HE-TIRA1 device, which requires hardwired
  * baudrate (38400 gets mapped to 100000) and RTS-CTS enabled.  */
-- 
2.31.1

Re: [PATCH v3 3/3] USB: serial: blacklist USB-UIRT when driver is selected

[Johan Hovold]

On Thu, May 06, 2021 at 01:44:55PM +0100, Sean Young wrote:
> The USB-UIRT device has its own driver, so blacklist the fdti driver
> from using it if the driver has been enabled.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 542073d2f0dd..2320bda57796 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -95,7 +95,9 @@ static int   ftdi_jtag_probe(struct usb_serial *serial);
>  static int   ftdi_NDI_device_setup(struct usb_serial *serial);
>  static int   ftdi_stmclite_probe(struct usb_serial *serial);
>  static int   ftdi_8u2232c_probe(struct usb_serial *serial);
> +#if !IS_ENABLED(CONFIG_IR_UIRT)
>  static void  ftdi_USB_UIRT_setup(struct ftdi_private *priv);
> +#endif
>  static void  ftdi_HE_TIRA1_setup(struct ftdi_private *priv);
>  
>  static const struct ftdi_sio_quirk ftdi_jtag_quirk = {
> @@ -106,9 +108,11 @@ static const struct ftdi_sio_quirk ftdi_NDI_device_quirk = {
>  	.probe	= ftdi_NDI_device_setup,
>  };
>  
> +#if !IS_ENABLED(CONFIG_IR_UIRT)
>  static const struct ftdi_sio_quirk ftdi_USB_UIRT_quirk = {

Please use __maybe_unused instead of sprinkling ifdefs throughout the
driver.

>  	.port_probe = ftdi_USB_UIRT_setup,
>  };
> +#endif
>  
>  static const struct ftdi_sio_quirk ftdi_HE_TIRA1_quirk = {
>  	.port_probe = ftdi_HE_TIRA1_setup,
> @@ -568,8 +572,10 @@ static const struct usb_device_id id_table_combined[] = {
>  	{ USB_DEVICE(OCT_VID, OCT_DK201_PID) },
>  	{ USB_DEVICE(FTDI_VID, FTDI_HE_TIRA1_PID),
>  		.driver_info = (kernel_ulong_t)&ftdi_HE_TIRA1_quirk },
> +#if !IS_ENABLED(CONFIG_IR_UIRT)
>  	{ USB_DEVICE(FTDI_VID, FTDI_USB_UIRT_PID),
>  		.driver_info = (kernel_ulong_t)&ftdi_USB_UIRT_quirk },
> +#endif

This would still be needed.

>  	{ USB_DEVICE(FTDI_VID, PROTEGO_SPECIAL_1) },
>  	{ USB_DEVICE(FTDI_VID, PROTEGO_R2X0) },
>  	{ USB_DEVICE(FTDI_VID, PROTEGO_SPECIAL_3) },
> @@ -2292,6 +2298,7 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
>  	return 0;
>  }
>  
> +#if !IS_ENABLED(CONFIG_IR_UIRT)
>  /* Setup for the USB-UIRT device, which requires hardwired
>   * baudrate (38400 gets mapped to 312500) */
>  /* Called from usbserial:serial_probe */
> @@ -2301,6 +2308,7 @@ static void ftdi_USB_UIRT_setup(struct ftdi_private *priv)
>  	priv->custom_divisor = 77;
>  	priv->force_baud = 38400;
>  }
> +#endif
>  
>  /* Setup for the HE-TIRA1 device, which requires hardwired
>   * baudrate (38400 gets mapped to 100000) and RTS-CTS enabled.  */

Johan

Re: [PATCH v3 3/3] USB: serial: blacklist USB-UIRT when driver is selected

[Sean Young]

On Fri, May 14, 2021 at 01:40:21PM +0200, Johan Hovold wrote:
> On Thu, May 06, 2021 at 01:44:55PM +0100, Sean Young wrote:
> > The USB-UIRT device has its own driver, so blacklist the fdti driver
> > from using it if the driver has been enabled.
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/usb/serial/ftdi_sio.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> > index 542073d2f0dd..2320bda57796 100644
> > --- a/drivers/usb/serial/ftdi_sio.c
> > +++ b/drivers/usb/serial/ftdi_sio.c
> > @@ -95,7 +95,9 @@ static int   ftdi_jtag_probe(struct usb_serial *serial);
> >  static int   ftdi_NDI_device_setup(struct usb_serial *serial);
> >  static int   ftdi_stmclite_probe(struct usb_serial *serial);
> >  static int   ftdi_8u2232c_probe(struct usb_serial *serial);
> > +#if !IS_ENABLED(CONFIG_IR_UIRT)
> >  static void  ftdi_USB_UIRT_setup(struct ftdi_private *priv);
> > +#endif
> >  static void  ftdi_HE_TIRA1_setup(struct ftdi_private *priv);
> >  
> >  static const struct ftdi_sio_quirk ftdi_jtag_quirk = {
> > @@ -106,9 +108,11 @@ static const struct ftdi_sio_quirk ftdi_NDI_device_quirk = {
> >  	.probe	= ftdi_NDI_device_setup,
> >  };
> >  
> > +#if !IS_ENABLED(CONFIG_IR_UIRT)
> >  static const struct ftdi_sio_quirk ftdi_USB_UIRT_quirk = {
> 
> Please use __maybe_unused instead of sprinkling ifdefs throughout the
> driver.

Good point.

> >  	.port_probe = ftdi_USB_UIRT_setup,
> >  };
> > +#endif
> >  
> >  static const struct ftdi_sio_quirk ftdi_HE_TIRA1_quirk = {
> >  	.port_probe = ftdi_HE_TIRA1_setup,
> > @@ -568,8 +572,10 @@ static const struct usb_device_id id_table_combined[] = {
> >  	{ USB_DEVICE(OCT_VID, OCT_DK201_PID) },
> >  	{ USB_DEVICE(FTDI_VID, FTDI_HE_TIRA1_PID),
> >  		.driver_info = (kernel_ulong_t)&ftdi_HE_TIRA1_quirk },
> > +#if !IS_ENABLED(CONFIG_IR_UIRT)
> >  	{ USB_DEVICE(FTDI_VID, FTDI_USB_UIRT_PID),
> >  		.driver_info = (kernel_ulong_t)&ftdi_USB_UIRT_quirk },
> > +#endif
> 
> This would still be needed.

I agree having the quirk in place would be useful, but if vid/pid is listed
in the id_table then both uirt and ftdi_sio have the same vid/pid listed,
which is not a great idea. How can this work?

Thanks

> 
> >  	{ USB_DEVICE(FTDI_VID, PROTEGO_SPECIAL_1) },
> >  	{ USB_DEVICE(FTDI_VID, PROTEGO_R2X0) },
> >  	{ USB_DEVICE(FTDI_VID, PROTEGO_SPECIAL_3) },
> > @@ -2292,6 +2298,7 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
> >  	return 0;
> >  }
> >  
> > +#if !IS_ENABLED(CONFIG_IR_UIRT)
> >  /* Setup for the USB-UIRT device, which requires hardwired
> >   * baudrate (38400 gets mapped to 312500) */
> >  /* Called from usbserial:serial_probe */
> > @@ -2301,6 +2308,7 @@ static void ftdi_USB_UIRT_setup(struct ftdi_private *priv)
> >  	priv->custom_divisor = 77;
> >  	priv->force_baud = 38400;
> >  }
> > +#endif
> >  
> >  /* Setup for the HE-TIRA1 device, which requires hardwired
> >   * baudrate (38400 gets mapped to 100000) and RTS-CTS enabled.  */
> 
> Johan

Re: [PATCH v3 3/3] USB: serial: blacklist USB-UIRT when driver is selected

[Johan Hovold]

On Sat, May 15, 2021 at 10:56:28AM +0100, Sean Young wrote:
> On Fri, May 14, 2021 at 01:40:21PM +0200, Johan Hovold wrote:
> > On Thu, May 06, 2021 at 01:44:55PM +0100, Sean Young wrote:
> > > The USB-UIRT device has its own driver, so blacklist the fdti driver
> > > from using it if the driver has been enabled.
> > > 
> > > Signed-off-by: Sean Young <sean@mess.org>
> > > ---
> > >  drivers/usb/serial/ftdi_sio.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> > > index 542073d2f0dd..2320bda57796 100644
> > > --- a/drivers/usb/serial/ftdi_sio.c
> > > +++ b/drivers/usb/serial/ftdi_sio.c
> > > @@ -95,7 +95,9 @@ static int   ftdi_jtag_probe(struct usb_serial *serial);
> > >  static int   ftdi_NDI_device_setup(struct usb_serial *serial);
> > >  static int   ftdi_stmclite_probe(struct usb_serial *serial);
> > >  static int   ftdi_8u2232c_probe(struct usb_serial *serial);
> > > +#if !IS_ENABLED(CONFIG_IR_UIRT)
> > >  static void  ftdi_USB_UIRT_setup(struct ftdi_private *priv);
> > > +#endif
> > >  static void  ftdi_HE_TIRA1_setup(struct ftdi_private *priv);
> > >  
> > >  static const struct ftdi_sio_quirk ftdi_jtag_quirk = {
> > > @@ -106,9 +108,11 @@ static const struct ftdi_sio_quirk ftdi_NDI_device_quirk = {
> > >  	.probe	= ftdi_NDI_device_setup,
> > >  };
> > >  
> > > +#if !IS_ENABLED(CONFIG_IR_UIRT)
> > >  static const struct ftdi_sio_quirk ftdi_USB_UIRT_quirk = {
> > 
> > Please use __maybe_unused instead of sprinkling ifdefs throughout the
> > driver.
> 
> Good point.
> 
> > >  	.port_probe = ftdi_USB_UIRT_setup,
> > >  };
> > > +#endif
> > >  
> > >  static const struct ftdi_sio_quirk ftdi_HE_TIRA1_quirk = {
> > >  	.port_probe = ftdi_HE_TIRA1_setup,
> > > @@ -568,8 +572,10 @@ static const struct usb_device_id id_table_combined[] = {
> > >  	{ USB_DEVICE(OCT_VID, OCT_DK201_PID) },
> > >  	{ USB_DEVICE(FTDI_VID, FTDI_HE_TIRA1_PID),
> > >  		.driver_info = (kernel_ulong_t)&ftdi_HE_TIRA1_quirk },
> > > +#if !IS_ENABLED(CONFIG_IR_UIRT)
> > >  	{ USB_DEVICE(FTDI_VID, FTDI_USB_UIRT_PID),
> > >  		.driver_info = (kernel_ulong_t)&ftdi_USB_UIRT_quirk },
> > > +#endif
> > 
> > This would still be needed.
> 
> I agree having the quirk in place would be useful, but if vid/pid is listed
> in the id_table then both uirt and ftdi_sio have the same vid/pid listed,
> which is not a great idea. How can this work?

Sorry if I was being unclear; I meant that this ifdef would still be
needed.

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> This is a new rc-core driver for the USB-UIRT which you can see here
> http://www.usbuirt.com/
> 
> This device is supported in lirc, via the usb serial kernel driver. This
> driver is both for rc-core, which means it can use kernel/BPF decoding
> ec. Also this implement is superior because it can:
>  - support learning mode
>  - setting transmit carrier
>  - larger transmits using streaming tx command

This looks like something which should have been implemented as a
line-discipline or serdev driver instead of reimplementing a minimal
on-off ftdi driver and tying it closely to the RC subsystem.

Why can't you just add support for the above features to whatever
subsystem is managing this device today?

Serdev still doesn't support hotplugging unfortunately so that route may
take a bit more work.

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Sean Young]

On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > This is a new rc-core driver for the USB-UIRT which you can see here
> > http://www.usbuirt.com/
> > 
> > This device is supported in lirc, via the usb serial kernel driver. This
> > driver is both for rc-core, which means it can use kernel/BPF decoding
> > ec. Also this implement is superior because it can:
> >  - support learning mode
> >  - setting transmit carrier
> >  - larger transmits using streaming tx command
> 
> This looks like something which should have been implemented as a
> line-discipline or serdev driver instead of reimplementing a minimal
> on-off ftdi driver and tying it closely to the RC subsystem.

The device is an infrared device, I'm not sure what it is lost by
doing it this way. The "minimal on-off ftdi driver" is super trivial.

> Why can't you just add support for the above features to whatever
> subsystem is managing this device today?
> 
> Serdev still doesn't support hotplugging unfortunately so that route may
> take a bit more work.

There seems to be at least three ways of attaching drivers to serial
devices: serio, serdev, and line-discipline. All seem to have limitations,
as you say none of them provide a way of hotplugging devices without
user-space attaching them through an ioctl or so.

If you want to go down this route, then ideally you'd want a quirk on
fdti saying "attach usb-uirt serdev device to this pid/vid". Considering
module dependencies, I don't know how that could work without again
userspace getting involved.

Getting userspace involved seem like a big song and dance because the
device uses an fdti device, even though it's not a serial port because
it's hardwired for infrared functions, no db9 connector in sight.


Sean

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > > This is a new rc-core driver for the USB-UIRT which you can see here
> > > http://www.usbuirt.com/
> > > 
> > > This device is supported in lirc, via the usb serial kernel driver. This
> > > driver is both for rc-core, which means it can use kernel/BPF decoding
> > > ec. Also this implement is superior because it can:
> > >  - support learning mode
> > >  - setting transmit carrier
> > >  - larger transmits using streaming tx command
> > 
> > This looks like something which should have been implemented as a
> > line-discipline or serdev driver instead of reimplementing a minimal
> > on-off ftdi driver and tying it closely to the RC subsystem.
> 
> The device is an infrared device, I'm not sure what it is lost by
> doing it this way. The "minimal on-off ftdi driver" is super trivial.

It's still code duplication (and I meant to say "one-off" above").

What is preventing you from supporting the above functionality through
lirc?

> > Why can't you just add support for the above features to whatever
> > subsystem is managing this device today?
> > 
> > Serdev still doesn't support hotplugging unfortunately so that route may
> > take a bit more work.
> 
> There seems to be at least three ways of attaching drivers to serial
> devices: serio, serdev, and line-discipline. All seem to have limitations,
> as you say none of them provide a way of hotplugging devices without
> user-space attaching them through an ioctl or so.

serio is also a line-discipline driver, which unlike serdev needs to be
set up by user space.

And the problem with serdev is that it does not (yet) support
hotplugging (specifically hangups) so it can't be enabled for USB serial
just yet.

> If you want to go down this route, then ideally you'd want a quirk on
> fdti saying "attach usb-uirt serdev device to this pid/vid". Considering
> module dependencies, I don't know how that could work without again
> userspace getting involved.

We'd just reuse or add another matching mechanism for USB devices. This
can be handled without user-space interaction just fine as long as you
have a dedicated device id as you do here.

> Getting userspace involved seem like a big song and dance because the
> device uses an fdti device, even though it's not a serial port because
> it's hardwired for infrared functions, no db9 connector in sight.

Far from every USB serial device have a db9 connector (e.g. modems,
barcode scanners, development board consoles, etc.) and you still have a
UART in your device.

In principle reimplementing a one-off ftdi driver is wrong but since
parts of the infrastructure needed to avoid this is still missing it may
be acceptable, especially if you can't get this to work with lirc.

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Sean Young]

On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:
> On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > > > This is a new rc-core driver for the USB-UIRT which you can see here
> > > > http://www.usbuirt.com/
> > > > 
> > > > This device is supported in lirc, via the usb serial kernel driver. This
> > > > driver is both for rc-core, which means it can use kernel/BPF decoding
> > > > ec. Also this implement is superior because it can:
> > > >  - support learning mode
> > > >  - setting transmit carrier
> > > >  - larger transmits using streaming tx command
> > > 
> > > This looks like something which should have been implemented as a
> > > line-discipline or serdev driver instead of reimplementing a minimal
> > > on-off ftdi driver and tying it closely to the RC subsystem.
> > 
> > The device is an infrared device, I'm not sure what it is lost by
> > doing it this way. The "minimal on-off ftdi driver" is super trivial.
> 
> It's still code duplication (and I meant to say "one-off" above").
> 
> What is preventing you from supporting the above functionality through
> lirc?

I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc
chardev. If you use the lirc daemon, you don't use rc-core which comes with
IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of
rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will
work, including the IRP encoder/BPF compiler I'm working on (very slowly).

The other nice thing is that IR TX feeds data from an urb interrupt handler,
so you don't need to rely userspace being scheduled quickly enough to feed
more data before the device runs out.

> 
> > > Why can't you just add support for the above features to whatever
> > > subsystem is managing this device today?
> > > 
> > > Serdev still doesn't support hotplugging unfortunately so that route may
> > > take a bit more work.
> > 
> > There seems to be at least three ways of attaching drivers to serial
> > devices: serio, serdev, and line-discipline. All seem to have limitations,
> > as you say none of them provide a way of hotplugging devices without
> > user-space attaching them through an ioctl or so.
> 
> serio is also a line-discipline driver, which unlike serdev needs to be
> set up by user space.

serio is unusable for this case. serio does not allow more than one byte
to be written nor does it have callbacks for CTS readiness.

> And the problem with serdev is that it does not (yet) support
> hotplugging (specifically hangups) so it can't be enabled for USB serial
> just yet.
> 
> > If you want to go down this route, then ideally you'd want a quirk on
> > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering
> > module dependencies, I don't know how that could work without again
> > userspace getting involved.
> 
> We'd just reuse or add another matching mechanism for USB devices. This
> can be handled without user-space interaction just fine as long as you
> have a dedicated device id as you do here.

Right, ok. I don't quite follow what you have in mind. If at all possible
keep me in the loop for any patches for this, I'm happy to test/re-write
this driver and the drivers/media/rc/ir_toy.c driver on top of any such
patches.

There are a bunch old serial usb device IR devices and even older non-usb
serial devices that would be nice to have supported, if anyone is still
using them.

> > Getting userspace involved seem like a big song and dance because the
> > device uses an fdti device, even though it's not a serial port because
> > it's hardwired for infrared functions, no db9 connector in sight.
> 
> Far from every USB serial device have a db9 connector (e.g. modems,
> barcode scanners, development board consoles, etc.) and you still have a
> UART in your device.
> 
> In principle reimplementing a one-off ftdi driver is wrong but since
> parts of the infrastructure needed to avoid this is still missing it may
> be acceptable, especially if you can't get this to work with lirc.

Thanks -- that's a good compromise.


Sean

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:
> On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:
> > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > > > > This is a new rc-core driver for the USB-UIRT which you can see here
> > > > > http://www.usbuirt.com/
> > > > > 
> > > > > This device is supported in lirc, via the usb serial kernel driver. This
> > > > > driver is both for rc-core, which means it can use kernel/BPF decoding
> > > > > ec. Also this implement is superior because it can:
> > > > >  - support learning mode
> > > > >  - setting transmit carrier
> > > > >  - larger transmits using streaming tx command
> > > > 
> > > > This looks like something which should have been implemented as a
> > > > line-discipline or serdev driver instead of reimplementing a minimal
> > > > on-off ftdi driver and tying it closely to the RC subsystem.
> > > 
> > > The device is an infrared device, I'm not sure what it is lost by
> > > doing it this way. The "minimal on-off ftdi driver" is super trivial.
> > 
> > It's still code duplication (and I meant to say "one-off" above").
> > 
> > What is preventing you from supporting the above functionality through
> > lirc?
> 
> I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc
> chardev. If you use the lirc daemon, you don't use rc-core which comes with
> IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of
> rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will
> work, including the IRP encoder/BPF compiler I'm working on (very slowly).

Ok, but apart from BPF that sound like other stuff and not the three
items you list above? Is there anything preventing those items from
being implemented in user space?

Obviously a user-space implementation (e.g. accessing the device through
/dev/ttyUSB0) is not going to be able to use in-kernel RC functionality.
For that you'd need to use either a line-discipline or serdev driver,
that is, a kernel driver, but not everything has to live in the kernel.

> The other nice thing is that IR TX feeds data from an urb interrupt handler,
> so you don't need to rely userspace being scheduled quickly enough to feed
> more data before the device runs out.

The tty layer and tty drivers provide write buffering so that need not
be an issue.

> > > > Why can't you just add support for the above features to whatever
> > > > subsystem is managing this device today?
> > > > 
> > > > Serdev still doesn't support hotplugging unfortunately so that route may
> > > > take a bit more work.
> > > 
> > > There seems to be at least three ways of attaching drivers to serial
> > > devices: serio, serdev, and line-discipline. All seem to have limitations,
> > > as you say none of them provide a way of hotplugging devices without
> > > user-space attaching them through an ioctl or so.
> > 
> > serio is also a line-discipline driver, which unlike serdev needs to be
> > set up by user space.
> 
> serio is unusable for this case. serio does not allow more than one byte
> to be written nor does it have callbacks for CTS readiness.

Ok, but you could still implement this as an rc-core line-discipline or
serdev driver. And when you use hardware flow control as you do here,
you shouldn't need any callbacks for CTS events either, right?
 
> > And the problem with serdev is that it does not (yet) support
> > hotplugging (specifically hangups) so it can't be enabled for USB serial
> > just yet.
> > 
> > > If you want to go down this route, then ideally you'd want a quirk on
> > > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering
> > > module dependencies, I don't know how that could work without again
> > > userspace getting involved.
> > 
> > We'd just reuse or add another matching mechanism for USB devices. This
> > can be handled without user-space interaction just fine as long as you
> > have a dedicated device id as you do here.
> 
> Right, ok. I don't quite follow what you have in mind. If at all possible
> keep me in the loop for any patches for this, I'm happy to test/re-write
> this driver and the drivers/media/rc/ir_toy.c driver on top of any such
> patches.

Thanks for that pointer. Judging from a quick look, the new driver
appears to based on this one. By abstracting the serial interface bits
in a generic RC serdev/ldisc driver you may be able reuse more code,
even if I'm not in a position to judge how much of the IR protocol bits
that can be shared.

> There are a bunch old serial usb device IR devices and even older non-usb
> serial devices that would be nice to have supported, if anyone is still
> using them.

I noticed that drivers/media/rc/serial_ir.c also appears to use a
similar approach of implementing a minimal one-off serial (8250?) driver
and tying it closely to RC core. This one might also benefit from using
the standard serial drivers for the transport and having an RC layer on
top.

Currently it appears to use module-parameters for configuration instead
of devicetree or some to-be-implemented interface for instantiating
serdev devices from user space.

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Sean Young]

On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:
> On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:
> > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:
> > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > > > > > This is a new rc-core driver for the USB-UIRT which you can see here
> > > > > > http://www.usbuirt.com/
> > > > > > 
> > > > > > This device is supported in lirc, via the usb serial kernel driver. This
> > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding
> > > > > > ec. Also this implement is superior because it can:
> > > > > >  - support learning mode
> > > > > >  - setting transmit carrier
> > > > > >  - larger transmits using streaming tx command
> > > > > 
> > > > > This looks like something which should have been implemented as a
> > > > > line-discipline or serdev driver instead of reimplementing a minimal
> > > > > on-off ftdi driver and tying it closely to the RC subsystem.
> > > > 
> > > > The device is an infrared device, I'm not sure what it is lost by
> > > > doing it this way. The "minimal on-off ftdi driver" is super trivial.
> > > 
> > > It's still code duplication (and I meant to say "one-off" above").
> > > 
> > > What is preventing you from supporting the above functionality through
> > > lirc?
> > 
> > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc
> > chardev. If you use the lirc daemon, you don't use rc-core which comes with
> > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of
> > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will
> > work, including the IRP encoder/BPF compiler I'm working on (very slowly).
> 
> Ok, but apart from BPF that sound like other stuff and not the three
> items you list above? Is there anything preventing those items from
> being implemented in user space?

Well, after IR is decoded, you want to send decoded scancodes/key codes
to any input device, so your remote works just like any input device.

> Obviously a user-space implementation (e.g. accessing the device through
> /dev/ttyUSB0) is not going to be able to use in-kernel RC functionality.
> For that you'd need to use either a line-discipline or serdev driver,
> that is, a kernel driver, but not everything has to live in the kernel.

No, of course not. A lot of kernel functionality could live in user space,
for sure. But it doesn't.

Even if the input problem can be resolved, the lirc daemon is pretty outdated.
All the existing functionality in-kernel would have to be re-written for
userspace, and it would be total duplication of code, which you do not like.
You end up with a userspace implementation and a kernel space implementation.

There are many other IR devices that can be controlled through libusb in
userspace, which could work entirely in userspace. Same for i2c IR
devices, those could work entirely from userspace too. I don't know what
the state is of pci userspace drivers, but there certainly have been patches
for that; the line is not so clear.

I do think that the monolithic approach to kernels necessarily invokes
discussions like these, and there are no perfect answers. 

> > The other nice thing is that IR TX feeds data from an urb interrupt handler,
> > so you don't need to rely userspace being scheduled quickly enough to feed
> > more data before the device runs out.
> 
> The tty layer and tty drivers provide write buffering so that need not
> be an issue.
 
Ok. I don't know what the size of the write buffer is or what the maximum
TX size is; the IR device supports infinite streaming.

> > > > > Why can't you just add support for the above features to whatever
> > > > > subsystem is managing this device today?
> > > > > 
> > > > > Serdev still doesn't support hotplugging unfortunately so that route may
> > > > > take a bit more work.
> > > > 
> > > > There seems to be at least three ways of attaching drivers to serial
> > > > devices: serio, serdev, and line-discipline. All seem to have limitations,
> > > > as you say none of them provide a way of hotplugging devices without
> > > > user-space attaching them through an ioctl or so.
> > > 
> > > serio is also a line-discipline driver, which unlike serdev needs to be
> > > set up by user space.
> > 
> > serio is unusable for this case. serio does not allow more than one byte
> > to be written nor does it have callbacks for CTS readiness.
> 
> Ok, but you could still implement this as an rc-core line-discipline or
> serdev driver. And when you use hardware flow control as you do here,
> you shouldn't need any callbacks for CTS events either, right?

I like the idea of serdev, but like you say it's not ready yet because of
absence of hotplugging. See above about streaming.

> > > And the problem with serdev is that it does not (yet) support
> > > hotplugging (specifically hangups) so it can't be enabled for USB serial
> > > just yet.
> > > 
> > > > If you want to go down this route, then ideally you'd want a quirk on
> > > > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering
> > > > module dependencies, I don't know how that could work without again
> > > > userspace getting involved.
> > > 
> > > We'd just reuse or add another matching mechanism for USB devices. This
> > > can be handled without user-space interaction just fine as long as you
> > > have a dedicated device id as you do here.
> > 
> > Right, ok. I don't quite follow what you have in mind. If at all possible
> > keep me in the loop for any patches for this, I'm happy to test/re-write
> > this driver and the drivers/media/rc/ir_toy.c driver on top of any such
> > patches.
> 
> Thanks for that pointer. Judging from a quick look, the new driver
> appears to based on this one. By abstracting the serial interface bits
> in a generic RC serdev/ldisc driver you may be able reuse more code,
> even if I'm not in a position to judge how much of the IR protocol bits
> that can be shared.

Yes, I agree. Once hotplugging is in place. If you have patches for that,
please CC me and I can see if will work for IR drivers.

> > There are a bunch old serial usb device IR devices and even older non-usb
> > serial devices that would be nice to have supported, if anyone is still
> > using them.
> 
> I noticed that drivers/media/rc/serial_ir.c also appears to use a
> similar approach of implementing a minimal one-off serial (8250?) driver
> and tying it closely to RC core. This one might also benefit from using
> the standard serial drivers for the transport and having an RC layer on
> top.
> 
> Currently it appears to use module-parameters for configuration instead
> of devicetree or some to-be-implemented interface for instantiating
> serdev devices from user space.

serial_ir.c (called lirc_serial in the past) is a bit special. It uses
an IR sensor connected directly to DCD and an output led connected to DTR,
(depending on the configuration used). I don't think this can be done with
a standard serial port driver. If it is possible, I'd like to know.

It's a bit of an insane way of doing things, but it's super cheap to build.


Sean

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Sean Young]

On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:
> On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:
> > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:
> > > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:
> > > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> > > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> > > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > > > > > > This is a new rc-core driver for the USB-UIRT which you can see here
> > > > > > > http://www.usbuirt.com/
> > > > > > > 
> > > > > > > This device is supported in lirc, via the usb serial kernel driver. This
> > > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding
> > > > > > > ec. Also this implement is superior because it can:
> > > > > > >  - support learning mode
> > > > > > >  - setting transmit carrier
> > > > > > >  - larger transmits using streaming tx command
> > > > > > 
> > > > > > This looks like something which should have been implemented as a
> > > > > > line-discipline or serdev driver instead of reimplementing a minimal
> > > > > > on-off ftdi driver and tying it closely to the RC subsystem.
> > > > > 
> > > > > The device is an infrared device, I'm not sure what it is lost by
> > > > > doing it this way. The "minimal on-off ftdi driver" is super trivial.
> > > > 
> > > > It's still code duplication (and I meant to say "one-off" above").
> > > > 
> > > > What is preventing you from supporting the above functionality through
> > > > lirc?
> > > 
> > > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc
> > > chardev. If you use the lirc daemon, you don't use rc-core which comes with
> > > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of
> > > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will
> > > work, including the IRP encoder/BPF compiler I'm working on (very slowly).
> > 
> > Ok, but apart from BPF that sound like other stuff and not the three
> > items you list above? Is there anything preventing those items from
> > being implemented in user space?
> 
> Well, after IR is decoded, you want to send decoded scancodes/key codes
> to any input device, so your remote works just like any input device.

There is another advantage. IR decoding in userspace involves a lot more
context switches/scheduling, and it can feel laggy when the cpu is under
load (e.g. video decoding on the CPU). When you press pause/play/stop or
so you expect the response the instantatiously. A 100ms delay is noticable.

Alternatively the key-up events get delayed and you end up with multiple
un-intended button repeats. None of this happens with kernel decoding and
it feels very snappy.


Sean

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

On Mon, May 17, 2021 at 01:35:09PM +0100, Sean Young wrote:
> On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:
> > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:

> > > Ok, but apart from BPF that sound like other stuff and not the three
> > > items you list above? Is there anything preventing those items from
> > > being implemented in user space?
> > 
> > Well, after IR is decoded, you want to send decoded scancodes/key codes
> > to any input device, so your remote works just like any input device.
> 
> There is another advantage. IR decoding in userspace involves a lot more
> context switches/scheduling, and it can feel laggy when the cpu is under
> load (e.g. video decoding on the CPU). When you press pause/play/stop or
> so you expect the response the instantatiously. A 100ms delay is noticable.

RT scheduling? Sounds like you should be able to handle this way faster
than 100 ms.

> Alternatively the key-up events get delayed and you end up with multiple
> un-intended button repeats. None of this happens with kernel decoding and
> it feels very snappy.

Yeah, perhaps it's best handled in-kernel, but it seems we should be
able to handle a simple key press within 20 ms or whatever the critical
latency is here using either option (kernel or user-space driver).

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:
> On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:
> > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:
> > > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote:
> > > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote:
> > > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote:
> > > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote:
> > > > > > > This is a new rc-core driver for the USB-UIRT which you can see here
> > > > > > > http://www.usbuirt.com/
> > > > > > > 
> > > > > > > This device is supported in lirc, via the usb serial kernel driver. This
> > > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding
> > > > > > > ec. Also this implement is superior because it can:
> > > > > > >  - support learning mode
> > > > > > >  - setting transmit carrier
> > > > > > >  - larger transmits using streaming tx command
> > > > > > 
> > > > > > This looks like something which should have been implemented as a
> > > > > > line-discipline or serdev driver instead of reimplementing a minimal
> > > > > > on-off ftdi driver and tying it closely to the RC subsystem.
> > > > > 
> > > > > The device is an infrared device, I'm not sure what it is lost by
> > > > > doing it this way. The "minimal on-off ftdi driver" is super trivial.
> > > > 
> > > > It's still code duplication (and I meant to say "one-off" above").
> > > > 
> > > > What is preventing you from supporting the above functionality through
> > > > lirc?
> > > 
> > > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc
> > > chardev. If you use the lirc daemon, you don't use rc-core which comes with
> > > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of
> > > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will
> > > work, including the IRP encoder/BPF compiler I'm working on (very slowly).
> > 
> > Ok, but apart from BPF that sound like other stuff and not the three
> > items you list above? Is there anything preventing those items from
> > being implemented in user space?
> 
> Well, after IR is decoded, you want to send decoded scancodes/key codes
> to any input device, so your remote works just like any input device.

Isn't that already handled by lircd using uinput?
 
> > Obviously a user-space implementation (e.g. accessing the device through
> > /dev/ttyUSB0) is not going to be able to use in-kernel RC functionality.
> > For that you'd need to use either a line-discipline or serdev driver,
> > that is, a kernel driver, but not everything has to live in the kernel.
> 
> No, of course not. A lot of kernel functionality could live in user space,
> for sure. But it doesn't.
> 
> Even if the input problem can be resolved, the lirc daemon is pretty outdated.
> All the existing functionality in-kernel would have to be re-written for
> userspace, and it would be total duplication of code, which you do not like.
> You end up with a userspace implementation and a kernel space implementation.
> 
> There are many other IR devices that can be controlled through libusb in
> userspace, which could work entirely in userspace. Same for i2c IR
> devices, those could work entirely from userspace too. I don't know what
> the state is of pci userspace drivers, but there certainly have been patches
> for that; the line is not so clear.
> 
> I do think that the monolithic approach to kernels necessarily invokes
> discussions like these, and there are no perfect answers. 

I hear you, but we still need to have those discussions from time to
time to make sure our architecture is sane. One of the problems today
with the kernel development process appears to be that too few questions
are asked. If it builds, ship it...

In this case the device in question can already be handled in user space
by lirqd (at least to some degree) and we have infrastructure for
writing in-kernel serial client drivers (i.e. ldisc/serdev). While
neither option may support everything we need today, adding further
one-off serial-device + client combo drivers is still a step in the
wrong direction.

But I think I've got that point across by now.

> > > The other nice thing is that IR TX feeds data from an urb interrupt handler,
> > > so you don't need to rely userspace being scheduled quickly enough to feed
> > > more data before the device runs out.
> > 
> > The tty layer and tty drivers provide write buffering so that need not
> > be an issue.
>  
> Ok. I don't know what the size of the write buffer is or what the maximum
> TX size is; the IR device supports infinite streaming.

Our tty drivers typically have at least a 4k buffer for transmission.
Surely that should be enough for a remote control but perhaps there are
other more demanding applications?

> > Thanks for that pointer. Judging from a quick look, the new driver
> > appears to based on this one. By abstracting the serial interface bits
> > in a generic RC serdev/ldisc driver you may be able reuse more code,
> > even if I'm not in a position to judge how much of the IR protocol bits
> > that can be shared.
> 
> Yes, I agree. Once hotplugging is in place. If you have patches for that,
> please CC me and I can see if will work for IR drivers.

Let's hope someone steps up to fund that work then.

> > > There are a bunch old serial usb device IR devices and even older non-usb
> > > serial devices that would be nice to have supported, if anyone is still
> > > using them.
> > 
> > I noticed that drivers/media/rc/serial_ir.c also appears to use a
> > similar approach of implementing a minimal one-off serial (8250?) driver
> > and tying it closely to RC core. This one might also benefit from using
> > the standard serial drivers for the transport and having an RC layer on
> > top.
> > 
> > Currently it appears to use module-parameters for configuration instead
> > of devicetree or some to-be-implemented interface for instantiating
> > serdev devices from user space.
> 
> serial_ir.c (called lirc_serial in the past) is a bit special. It uses
> an IR sensor connected directly to DCD and an output led connected to DTR,
> (depending on the configuration used). I don't think this can be done with
> a standard serial port driver. If it is possible, I'd like to know.
> 
> It's a bit of an insane way of doing things, but it's super cheap to build.

Heh, ok. Perhaps we'll just have to live with this one then.

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Sean Young]

On Thu, May 20, 2021 at 03:31:34PM +0200, Johan Hovold wrote:
> On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:
> > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:
> > > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote:
> > > > The other nice thing is that IR TX feeds data from an urb interrupt handler,
> > > > so you don't need to rely userspace being scheduled quickly enough to feed
> > > > more data before the device runs out.
> > > 
> > > The tty layer and tty drivers provide write buffering so that need not
> > > be an issue.
> >  
> > Ok. I don't know what the size of the write buffer is or what the maximum
> > TX size is; the IR device supports infinite streaming.
> 
> Our tty drivers typically have at least a 4k buffer for transmission.
> Surely that should be enough for a remote control but perhaps there are
> other more demanding applications?

That's more than enough. The most demanding consumer IR I know of, is
an air conditioner which encodes temperature and a few others things. That's
a maximum of 439 pulse/spaces which should into 1k.
> 
> > > Thanks for that pointer. Judging from a quick look, the new driver
> > > appears to based on this one. By abstracting the serial interface bits
> > > in a generic RC serdev/ldisc driver you may be able reuse more code,
> > > even if I'm not in a position to judge how much of the IR protocol bits
> > > that can be shared.
> > 
> > Yes, I agree. Once hotplugging is in place. If you have patches for that,
> > please CC me and I can see if will work for IR drivers.
> 
> Let's hope someone steps up to fund that work then.

I'm just a volunteer. I've literally never heard anything about kernel work
being funded by anyone.

Would you mind giving a brief summary what is needed to implement hotplugging
for serdev? I get the impression it's not a lot of work, but I'm probably
missing something obvious.


Sean

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

Sorry about the late reply on this one.

On Fri, May 21, 2021 at 12:39:54PM +0100, Sean Young wrote:
> On Thu, May 20, 2021 at 03:31:34PM +0200, Johan Hovold wrote:
> > On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote:
> > > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote:

> > > > Thanks for that pointer. Judging from a quick look, the new driver
> > > > appears to based on this one. By abstracting the serial interface bits
> > > > in a generic RC serdev/ldisc driver you may be able reuse more code,
> > > > even if I'm not in a position to judge how much of the IR protocol bits
> > > > that can be shared.
> > > 
> > > Yes, I agree. Once hotplugging is in place. If you have patches for that,
> > > please CC me and I can see if will work for IR drivers.
> > 
> > Let's hope someone steps up to fund that work then.
> 
> I'm just a volunteer. I've literally never heard anything about kernel work
> being funded by anyone.

Someone always pays whether it's a client, an employer or you yourself
with your spare time.

> Would you mind giving a brief summary what is needed to implement hotplugging
> for serdev? I get the impression it's not a lot of work, but I'm probably
> missing something obvious.

First, it's the matching bits we already touched on where we would like
to be able to use something like devicetree overlays to avoid rolling a
new mechanism for every bus. But devicetree overlays has its issues
currently (e.g. theres no user-space interface for providing them and
last time I checked they could not be reverted).

Second, serdev does not use the file abstraction and does not support
hangups which is used to implement tty hotplugging (e.g. by signalling
the consumer and making all file operations become noops after a
disconnect).

This would take some thinking-through to get right, and hopefully it can
be done without having to update every current serdev driver.

Retrofitting serdev into the tty layer wasn't painless and broke things
here and there. Supporting hotplugging should be doable but it's not as
straight-forward as it may sound.

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Oliver Neukum]

Am Donnerstag, den 20.05.2021, 15:31 +0200 schrieb Johan Hovold:

Hi,

> Isn't that already handled by lircd using uinput?

The problem with that reasoning, though it is true, is

1) We would need to remove a lot of subsystems if we took that
to the logical conclusion. 

2) It makes runtime PM much harder

3) We end up with two classes of LIRC devices

> I hear you, but we still need to have those discussions from time to
> time to make sure our architecture is sane. One of the problems today
> with the kernel development process appears to be that too few
> questions
> are asked. If it builds, ship it...

Indeed, so, could we force a line discipline on a device on the kernel
level? Code duplication is bad.

> But I think I've got that point across by now.

Yes and and we need to think about the conclusion we draw from
that point. It seems to me that an architecture that pushes data
through the whole tty layer into a demon, then through uinput
is definitely not elegant.
So what else can we do, so that devices that are internally
a serial chip plus additional stuff but externally unrelated
devices? It looks to me we are in need of creativity beyond two options
here.

	Regards
		Oliver

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

Sorry about the late reply on this one too.

On Tue, May 25, 2021 at 02:25:49PM +0200, Oliver Neukum wrote:
> Am Donnerstag, den 20.05.2021, 15:31 +0200 schrieb Johan Hovold:

> > Isn't that already handled by lircd using uinput?
> 
> The problem with that reasoning, though it is true, is
> 
> 1) We would need to remove a lot of subsystems if we took that
> to the logical conclusion. 

Removing code is always nice. ;)

> 2) It makes runtime PM much harder

Possibly, depends on the bus and device.

> 3) We end up with two classes of LIRC devices

We already do, right? That's kind of my point since we have lircd
supporting uinput.

> > I hear you, but we still need to have those discussions from time to
> > time to make sure our architecture is sane. One of the problems today
> > with the kernel development process appears to be that too few
> > questions
> > are asked. If it builds, ship it...
> 
> Indeed, so, could we force a line discipline on a device on the kernel
> level? Code duplication is bad.

Not sure I understand what you have mind here. serdev is sort of a
line-discipline which we'd "force" on a device if there's a matching
description in devicetree, while line disciplines always need to be
instantiated by user space. Or are you referring to ldisc/serdev code
reuse?

> > But I think I've got that point across by now.
> 
> Yes and and we need to think about the conclusion we draw from
> that point. It seems to me that an architecture that pushes data
> through the whole tty layer into a demon, then through uinput
> is definitely not elegant.

The elegant answer is serdev, but it does not yet support the features
needed in this case (i.e. hotplugging).

Since we already support user-space drivers for these devices, I see
nothing wrong with implementing support for another one in user space
unless there are strong reasons against doing so (e.g. performance,
pm or usability). But if uinput works then great, we're done.

> So what else can we do, so that devices that are internally
> a serial chip plus additional stuff but externally unrelated
> devices? It looks to me we are in need of creativity beyond two options
> here.

Why? Leaving hotplugging aside for a moment, what is it that you cannot
do using either a serdev/ldisc driver or a user-space driver?

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Sean Young]

On Wed, Jun 23, 2021 at 03:10:20PM +0200, Johan Hovold wrote:
> Sorry about the late reply on this one too.
> 
> On Tue, May 25, 2021 at 02:25:49PM +0200, Oliver Neukum wrote:
> > Am Donnerstag, den 20.05.2021, 15:31 +0200 schrieb Johan Hovold:
> 
> > > Isn't that already handled by lircd using uinput?
> > 
> > The problem with that reasoning, though it is true, is
> > 
> > 1) We would need to remove a lot of subsystems if we took that
> > to the logical conclusion. 
> 
> Removing code is always nice. ;)

So rather than adding hotplug to serdev, we should remove line-discipline,
serdev, and serio and all its drivers from the kernel? This is taking
your own argument and applying it your code.

> > 2) It makes runtime PM much harder
> 
> Possibly, depends on the bus and device.
> 
> > 3) We end up with two classes of LIRC devices
> 
> We already do, right? That's kind of my point since we have lircd
> supporting uinput.

This is not an either-or situation, lircd is the "old" solution which is
slowing being supplanted with rc-core. All the new keymaps are rc-core and
do not work with lircd. The new rc-core tooling (in the v4l-utils package) 
does not work with lircd. lircd hasn't had any real patches merged for years
now.

There is whole new tooling in the works for rc-core which is not compatible
with lircd.

> > > I hear you, but we still need to have those discussions from time to
> > > time to make sure our architecture is sane. One of the problems today
> > > with the kernel development process appears to be that too few
> > > questions
> > > are asked. If it builds, ship it...
> > 
> > Indeed, so, could we force a line discipline on a device on the kernel
> > level? Code duplication is bad.
> 
> Not sure I understand what you have mind here. serdev is sort of a
> line-discipline which we'd "force" on a device if there's a matching
> description in devicetree, while line disciplines always need to be
> instantiated by user space. Or are you referring to ldisc/serdev code
> reuse?

I am pretty sure Oliver is suggesting that all ldisc/serdev code in
the kernel is duplication of code which can be done in userspace, by your
own argument.

> > > But I think I've got that point across by now.
> > 
> > Yes and and we need to think about the conclusion we draw from
> > that point. It seems to me that an architecture that pushes data
> > through the whole tty layer into a demon, then through uinput
> > is definitely not elegant.
> 
> The elegant answer is serdev, but it does not yet support the features
> needed in this case (i.e. hotplugging).
> 
> Since we already support user-space drivers for these devices, I see
> nothing wrong with implementing support for another one in user space
> unless there are strong reasons against doing so (e.g. performance,
> pm or usability). But if uinput works then great, we're done.

As discussed lircd has terrible latency, and lircd is out of date and
unmaintained and does not work with modern tooling and keymaps.

Also essentially your saying that any input device that connects to a
serial port should be done in user space. There are a ton of kernel
drivers doing exactly that, and that is why serio exists in the first
place.


Sean

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

On Thu, Jun 24, 2021 at 10:13:49AM +0100, Sean Young wrote:
> On Wed, Jun 23, 2021 at 03:10:20PM +0200, Johan Hovold wrote:
> > Sorry about the late reply on this one too.
> > 
> > On Tue, May 25, 2021 at 02:25:49PM +0200, Oliver Neukum wrote:
> > > Am Donnerstag, den 20.05.2021, 15:31 +0200 schrieb Johan Hovold:
> > 
> > > > Isn't that already handled by lircd using uinput?
> > > 
> > > The problem with that reasoning, though it is true, is
> > > 
> > > 1) We would need to remove a lot of subsystems if we took that
> > > to the logical conclusion. 
> > 
> > Removing code is always nice. ;)
> 
> So rather than adding hotplug to serdev, we should remove line-discipline,
> serdev, and serio and all its drivers from the kernel? This is taking
> your own argument and applying it your code.

Not at all. Not everything can be done in user space, but some things
can.

> > > 3) We end up with two classes of LIRC devices
> > 
> > We already do, right? That's kind of my point since we have lircd
> > supporting uinput.
> 
> This is not an either-or situation, lircd is the "old" solution which is
> slowing being supplanted with rc-core. All the new keymaps are rc-core and
> do not work with lircd. The new rc-core tooling (in the v4l-utils package) 
> does not work with lircd. lircd hasn't had any real patches merged for years
> now.
> 
> There is whole new tooling in the works for rc-core which is not compatible
> with lircd.

Sure, you already explained that. I was just asking (earlier) why you
didn't use the infrastructure that's already in place. If there are good
reasons for not doing so then fine. 

> > > > I hear you, but we still need to have those discussions from time to
> > > > time to make sure our architecture is sane. One of the problems today
> > > > with the kernel development process appears to be that too few
> > > > questions
> > > > are asked. If it builds, ship it...
> > > 
> > > Indeed, so, could we force a line discipline on a device on the kernel
> > > level? Code duplication is bad.
> > 
> > Not sure I understand what you have mind here. serdev is sort of a
> > line-discipline which we'd "force" on a device if there's a matching
> > description in devicetree, while line disciplines always need to be
> > instantiated by user space. Or are you referring to ldisc/serdev code
> > reuse?
> 
> I am pretty sure Oliver is suggesting that all ldisc/serdev code in
> the kernel is duplication of code which can be done in userspace, by your
> own argument.

See above.

> > > > But I think I've got that point across by now.
> > > 
> > > Yes and and we need to think about the conclusion we draw from
> > > that point. It seems to me that an architecture that pushes data
> > > through the whole tty layer into a demon, then through uinput
> > > is definitely not elegant.
> > 
> > The elegant answer is serdev, but it does not yet support the features
> > needed in this case (i.e. hotplugging).
> > 
> > Since we already support user-space drivers for these devices, I see
> > nothing wrong with implementing support for another one in user space
> > unless there are strong reasons against doing so (e.g. performance,
> > pm or usability). But if uinput works then great, we're done.
> 
> As discussed lircd has terrible latency, and lircd is out of date and
> unmaintained and does not work with modern tooling and keymaps.
> 
> Also essentially your saying that any input device that connects to a
> serial port should be done in user space. There are a ton of kernel
> drivers doing exactly that, and that is why serio exists in the first
> place.

I'm not, again see above. I'm saying that we should not make one-off
copies of serial drivers if we can avoid it.

In this case the limitations of lircd and the lack of hotplugging in
serdev may be a sufficient reason for making an exception. As we've
already discussed.

Johan

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Sean Young]

On Thu, Jun 24, 2021 at 11:41:28AM +0200, Johan Hovold wrote:
> I'm not, again see above. I'm saying that we should not make one-off
> copies of serial drivers if we can avoid it.
> 
> In this case the limitations of lircd and the lack of hotplugging in
> serdev may be a sufficient reason for making an exception. As we've
> already discussed.

Great, thanks very much. I totally agree a serdev solution would be
preferable.

In that case, can have your Signed-off-by for the ftdi_sio.c change, or
could this be merged through the usb tree please?

Thanks

Sean

Re: [PATCH v3 0/3] IR driver for USB-UIRT device

[Johan Hovold]

On Fri, Jun 25, 2021 at 09:08:00AM +0100, Sean Young wrote:
> On Thu, Jun 24, 2021 at 11:41:28AM +0200, Johan Hovold wrote:
> > I'm not, again see above. I'm saying that we should not make one-off
> > copies of serial drivers if we can avoid it.
> > 
> > In this case the limitations of lircd and the lack of hotplugging in
> > serdev may be a sufficient reason for making an exception. As we've
> > already discussed.
> 
> Great, thanks very much. I totally agree a serdev solution would be
> preferable.
> 
> In that case, can have your Signed-off-by for the ftdi_sio.c change, or
> could this be merged through the usb tree please?

Sure, I can take the ftdi change (for the current -rc) once the ir
driver hits the media tree.

Johan