IR driver for USB-UIRT device

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

See http://www.usbuirt.com/

Signed-off-by Sean Young <[email protected]>

drivers/media/rc/uirt.c new file +36 -0

+// 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;

drivers/media/rc/uirt.c new file +134 -0

+				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;

drivers/media/rc/uirt.c new file +27 -0

+}
+
+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)

drivers/media/rc/uirt.c new file +17 -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++) {

drivers/media/rc/uirt.c new file +80 -0

+
+	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);

Quoted 1 line · Sean Young

See http://www.usbuirt.com/

No proper commit message?

Quoted 43 lines

Signed-off-by Sean Young <[email protected]>

drivers/media/rc/uirt.c new file +41 -0

+// 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",

drivers/media/rc/uirt.c line 173

Quoted 5 lines

drivers/media/rc/uirt.c new file +5 -0

+					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.

Quoted 141 lines

drivers/media/rc/uirt.c new file +140 -0

+				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)

drivers/media/rc/uirt.c line 317

Quoted 4 lines

drivers/media/rc/uirt.c new file +4 -0

+		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.

drivers/media/rc/uirt.c line 324

Quoted 7 lines

drivers/media/rc/uirt.c new file +7 -0

+
+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.

Quoted 32 lines

drivers/media/rc/uirt.c new file +31 -0

+}
+
+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),

drivers/media/rc/uirt.c line 550

Quoted 6 lines

drivers/media/rc/uirt.c new file +6 -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?

Quoted 8 lines

drivers/media/rc/uirt.c new file +8 -0

+
+	// 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

drivers/media/rc/uirt.c line 563

Quoted 5 lines

drivers/media/rc/uirt.c new file +5 -0

+	// 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.

Quoted 22 lines

drivers/media/rc/uirt.c new file +22 -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;

drivers/media/rc/uirt.c line 590

Quoted 5 lines

drivers/media/rc/uirt.c new file +5 -0

+
+		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.

Quoted 85 lines

drivers/media/rc/uirt.c new file +85 -0

+
+	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);
+

drivers/media/rc/uirt.c line 680

Quoted 5 lines

drivers/media/rc/uirt.c new file +5 -0

+	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.

drivers/media/rc/uirt.c line 689

Quoted 9 lines

drivers/media/rc/uirt.c new file +9 -0

+	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;
+}

Quoted 4 lines · Johan Hovold

Quoted 1 line · Sean Young

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.

Quoted 106 lines

Quoted 106 lines

drivers/media/rc/uirt.c new file +106 -0

+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)

Quoted 8 lines

drivers/media/rc/uirt.c line 317

Quoted 4 lines

drivers/media/rc/uirt.c new file +4 -0

+		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.

Quoted 15 lines

Quoted 15 lines

drivers/media/rc/uirt.c new file +15 -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),

Quoted 8 lines

drivers/media/rc/uirt.c line 550

Quoted 6 lines

drivers/media/rc/uirt.c new file +6 -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.

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 <[email protected]>

---
 drivers/usb/serial/ftdi_sio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/usb/serial/ftdi_sio.c +2 -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 = {

drivers/usb/serial/ftdi_sio.c +1 -0

 	.port_probe = ftdi_USB_UIRT_setup,
 };
+#endif
 
 static const struct ftdi_sio_quirk ftdi_HE_TIRA1_quirk = {
 	.port_probe = ftdi_HE_TIRA1_setup,

drivers/usb/serial/ftdi_sio.c +1 -0

 	{ 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 */

Quoted 26 lines · 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 <[email protected]>

---
 drivers/usb/serial/ftdi_sio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/usb/serial/ftdi_sio.c +2 -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,
 };
 

drivers/usb/serial/ftdi_sio.c static const struct ftdi_sio_quirk ftdi_NDI_device_quirk = { · line 112

Quoted 2 lines

drivers/usb/serial/ftdi_sio.c +1 -0

+#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.

Quoted 4 lines

drivers/usb/serial/ftdi_sio.c +1 -0

 	.port_probe = ftdi_USB_UIRT_setup,
 };
+#endif
 

drivers/usb/serial/ftdi_sio.c static const struct usb_device_id id_table_combined[] = { · line 578

Quoted 10 lines

drivers/usb/serial/ftdi_sio.c +2 -0

 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.

Quoted 13 lines

drivers/usb/serial/ftdi_sio.c +1 -0

 	{ 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 */

drivers/usb/serial/ftdi_sio.c static void ftdi_USB_UIRT_setup(struct ftdi_private *priv) · line 2314

Quoted 7 lines

drivers/usb/serial/ftdi_sio.c +1 -0

@@ -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

Quoted 27 lines · Johan Hovold

Quoted 26 lines · 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 <[email protected]>

---
 drivers/usb/serial/ftdi_sio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/usb/serial/ftdi_sio.c +2 -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,
 };
 

Quoted 5 lines

drivers/usb/serial/ftdi_sio.c static const struct ftdi_sio_quirk ftdi_NDI_device_quirk = { · line 112

Quoted 2 lines

drivers/usb/serial/ftdi_sio.c +1 -0

+#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.

Quoted 4 lines

Quoted 4 lines

drivers/usb/serial/ftdi_sio.c +1 -0

 	.port_probe = ftdi_USB_UIRT_setup,
 };
+#endif
 

Quoted 12 lines

drivers/usb/serial/ftdi_sio.c static const struct usb_device_id id_table_combined[] = { · line 578

Quoted 10 lines

drivers/usb/serial/ftdi_sio.c +2 -0

 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?

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

Quoted 9 lines · 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

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.

Quoted 11 lines · Johan Hovold

Quoted 9 lines · 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

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 16 lines · Sean Young

Quoted 14 lines · Johan Hovold

Quoted 9 lines · 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

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.

Quoted 2 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 22 lines · Johan Hovold

Quoted 12 lines · Sean Young

Quoted 11 lines · Johan Hovold

Quoted 9 lines · 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

Quoted 6 lines

Quoted 3 lines

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").

Quoted 2 lines

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.

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 23 lines · Sean Young

Quoted 22 lines · Johan Hovold

Quoted 12 lines · Sean Young

Quoted 11 lines · Johan Hovold

Quoted 9 lines · 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

Quoted 6 lines

Quoted 3 lines

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").

Quoted 8 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 2 lines

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?

Quoted 9 lines

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 33 lines · Johan Hovold

Quoted 31 lines · Sean Young

Quoted 17 lines · Johan Hovold

Quoted 16 lines · Sean Young

Quoted 14 lines · Johan Hovold

Quoted 9 lines · 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

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.

Quoted 7 lines

Quoted 2 lines

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).

Quoted 3 lines

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.

Quoted 6 lines · Sean Young

Quoted 3 lines · Johan Hovold

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?

Quoted 2 lines

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.

Quoted 33 lines · Johan Hovold

Quoted 31 lines · Sean Young

Quoted 17 lines · Johan Hovold

Quoted 16 lines · Sean Young

Quoted 14 lines · Johan Hovold

Quoted 9 lines · 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

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.

Quoted 7 lines

Quoted 2 lines

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).

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 9 lines · Sean Young

Quoted 7 lines · Johan Hovold

Quoted 3 lines · Sean Young

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.

Quoted 2 lines

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?

Quoted 9 lines

Quoted 6 lines

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.

Quoted 9 lines · Johan Hovold

Quoted 8 lines · Sean Young

Quoted 5 lines · Johan Hovold

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.

Quoted 4 lines

Quoted 2 lines

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.

Isn't that already handled by lircd using 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...

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

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

Quoted 1 line

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

Quoted 5 lines

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.

Quoted 1 line

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.

Sorry about the late reply on this one too.

Quoted 1 line · Oliver Neukum

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

Quoted 6 lines

Quoted 1 line

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. ;)

Quoted 1 line

2) It makes runtime PM much harder

Possibly, depends on the bus and device.

Quoted 1 line

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.

Quoted 6 lines

Quoted 5 lines

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...

Quoted 2 lines

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?

Quoted 6 lines

Quoted 1 line

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.

Quoted 9 lines · Johan Hovold

Sorry about the late reply on this one too.

Quoted 6 lines · Oliver Neukum

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

Quoted 1 line

Isn't that already handled by lircd using uinput?

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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...

Quoted 8 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 6 lines · Johan Hovold

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?