Skip to contentSkip to conversation

IR driver for USB-UIRT device

PATCH Superseded Sean Young 3 participants 8 messages May 4, 2021 – May 5, 2021

Subsystems (4)

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
author maintainer
#1 lore raw

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

In reply to Sean Young

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

---
 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%)
drivers/usb/serial/ftdi_sio.c +1 -1
index c867592477c9..542073d2f0dd 100644
@@ -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 <[email protected]>, Bill Ryder <[email protected]>, Kuba Ober <[email protected]>, Andreas Mohr, Johan Hovold <[email protected]>"
include/linux/usb/ftdi_sio.h renamed from drivers/usb/serial/ftdi_sio.h +0 -0
similarity index 100%
rename from drivers/usb/serial/ftdi_sio.h
rename to include/linux/usb/ftdi_sio.h
Signature · 1 line
2.31.1
author maintainer
#5 lore raw

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

In reply to Sean Young

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

---
 drivers/usb/serial/ftdi_sio.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/usb/serial/ftdi_sio.c +8 -0
index 542073d2f0dd..2320bda57796 100644
@@ -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.  */
Signature · 1 line
2.31.1

Sean Young

author maintainer
#0 lore raw

cover IR driver for USB-UIRT device

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

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                       | 737 ++++++++++++++++++
 drivers/usb/serial/ftdi_sio.c                 |  10 +-
 .../serial => include/linux/usb}/ftdi_sio.h   |   0
 5 files changed, 758 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/rc/uirt.c
 rename {drivers/usb/serial => include/linux/usb}/ftdi_sio.h (100%)
Signature · 1 line
2.31.1
author maintainer
#2 lore raw

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

See http://www.usbuirt.com/

---
 drivers/media/rc/Kconfig  |  11 +
 drivers/media/rc/Makefile |   1 +
 drivers/media/rc/uirt.c   | 737 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 749 insertions(+)
 create mode 100644 drivers/media/rc/uirt.c
drivers/media/rc/Kconfig +11 -0
index f016b35c2b17..dff85a5bfd1e 100644
@@ -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
drivers/media/rc/Makefile +1 -0
index 4927f585ebc2..17037915e742 100644
@@ -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
drivers/media/rc/uirt.c new file +737 -0

737 lines not on this page · raw message

Signature · 1 line
2.31.1
author maintainer
#7 lore raw

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

Quoted 5 lines · Sergei Shtylyov
Quoted 1 line · Sean Young

The USB-UIRT device has its own driver, so blacklist the fdti driver

It's ftdi, right?

Yes, sorry that's a typo.

Thanks,

Sean

#3 lore raw

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

In reply to Sean Young

Am Dienstag, den 04.05.2021, 18:52 +0100 schrieb Sean Young:

Quoted 2 lines

See http://www.usbuirt.com/

Hi,

nice driver, just a few issues.
I have marked them inline.

Regards
Oliver

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+#define WDR_TIMEOUT 5000 /* default urb timeout */

That is the default ctrl timeout. Do you need this?

Quoted 13 lines
(quoted diff) +12 -0
+#define WDR_SHORT_TIMEOUT 1000	/* shorter urb timeout */
+#define UNIT_US 50
+#define IR_TIMEOUT 12500
+#define MAX_PACKET 64
 
+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

Review

Quoted 10 lines
(quoted diff) +10 -0
+	// 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

A constant checksum? Now that is a new concept.

Quoted 105 lines
drivers/media/rc/uirt.c new file +103 -0
+	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_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);

Review

Quoted 1 line
(quoted diff) +1 -0
+		return err;

Massive memory leak. You cannot just return.

author maintainer
#4 lore raw

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

Hi,

Quoted 8 lines · Oliver Neukum

Am Dienstag, den 04.05.2021, 18:52 +0100 schrieb Sean Young:

Quoted 2 lines

See http://www.usbuirt.com/

Hi,

nice driver, just a few issues.
I have marked them inline.

Thank you for the review, I appreciate it.

Quoted 7 lines

Regards
Oliver

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+#define WDR_TIMEOUT 5000 /* default urb timeout */

That is the default ctrl timeout. Do you need this?

Good point, no I don't.

Quoted 17 lines
Quoted 17 lines
(quoted diff) +16 -0
+#define WDR_SHORT_TIMEOUT 1000	/* shorter urb timeout */
+#define UNIT_US 50
+#define IR_TIMEOUT 12500
+#define MAX_PACKET 64
 
+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);
+
Quoted 8 lines

drivers/media/rc/uirt.c line 475

Quoted 6 lines
drivers/media/rc/uirt.c new file +6 -0
+	out = kmalloc(count * 2 + 3, GFP_KERNEL);
+	if (!out)
+		return -ENOMEM;
+
+	out[0] = 0x25; // Streaming Transmit
+	out[1] = 0xdb; // checksum

A constant checksum? Now that is a new concept.

Yes, this really needs a comment. It is a checksum over the command, which in this case is just the previous byte. I'll add a comment.

Quoted 100 lines
Quoted 100 lines
drivers/media/rc/uirt.c new file +99 -0
+	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_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;
Quoted 8 lines

Review

Quoted 6 lines
drivers/media/rc/uirt.c new file +4 -0
+
+	err = usb_submit_urb(uirt->urb_in, GFP_KERNEL);
+	if (err != 0) {
+		dev_err(uirt->dev, "failed to submit read urb: %d\n",

err);

(quoted diff) +1 -0
+		return err;

Massive memory leak. You cannot just return.

Yes, that's broken. Thanks for catching that. I'll go over the error paths.

Thanks again for the review, I'll send out v2 when I'm done.

Sean