[PATCH v8 0/2] usb: serial: add support for CH348

[PATCH v8 0/2] usb: serial: add support for CH348

[Corentin Labbe]

Hello

The CH348 is an octo serial to USB adapter.
The following patch adds a driver for supporting it.
Since there is no public datasheet, unfortunatly it remains some magic values.

It was tested with a large range of baud from 1200 to 1500000 and used with
success in one of our kernel CI testlab.

Regards

Changes since v1:
- use a data structure for encoding/decoding messages.
- check if needed endpoints exists
- fix URB leak in ch348_allocate_status_read error case
- test for maximum baud rate as stated by datasheet

Changes since v2:
- specify ch348_rxbuf data length
- Use correct speed_t dwDTERate instead of __le32
- test for maximum baud rate supported according to datasheet
- Use a define for CH348_TX_HDRSIZE

Changes since v3
- Fixed all reported problem from https://lore.kernel.org/lkml/Y5NDwEakGJbmB6+b@Red/T/#mb6234d0427cfdabf412190565e215995a41482dd
  Mostly reworked the endpoint mux to be the same than mx_uport

Changes since v4:
- The V4 was sent against stable and next have ch348_set_termios ktermios
  parameter const that I forgot to change

Changes since v5:
- Fixed all reported problem from
+https://lore.kernel.org/lkml/20230106135338.643951-1-clabbe@baylibre.com/T/#m044aab24dfb652ea34aa06f8ef704da9d6a2e036
- Major change is dropping of all status handling which was unused.
  It will be probably necessary to bring it back when using GPIO.
  This will be done when I will finish my next devboard.

Changes since v6:
- read and print the device version during probe
- Only request one bulk out channel from usb-serial core
- Implement status report / interrupt handling
- Fix buffer->rate calculation / enable support for slow baud rates
- use a mutex to protect against concurrent writes
- split write buffers for slow baud rates

Important note, v7 is mostly done from work of Martin Blumenstingl,
so the changelog was built from https://github.com/xdarklight/ch348/commits/main/

changes since v7:
- Use standard configuration and interrupt status macros from
  <linux/serial_reg.h> as suggested by Johan Hovold (thank you)
- Update logging (avoiding %s to print the function name and
  rate-limiting logs from ch348_process_status_urb() which may be
  called a lot) as suggested by Johan Hovold
- Use usb-serial integration for parsing all endpoint descriptors for
  us. As result of this usb-serial and ch348_process_read_urb() are
  now managing the status/interrupt endpoint as well
- Move processing of the write buffer(s) to a workqueue. ch348 does
  not allow multiple parallel write URBs (with serial TX data).
  usb-serial core however tries to always make sure that the buffers
  are full and sending them back-to-back. This does not work for ch348
  as it leads to data corruption.
- Don't use bitfields in struct ch348_status_entry as it's part of an
  ABI. Thanks to Johan Hovold for pointing this out.
- Use #defines for magic values and spell out cases as Suggested by
  Johan Hovold
- Drop support for baud rates outside the range from the datasheet.
  Slower and faster baud rates were added with v7. Testing for this was
  done by connecting two ch348 ports together. However, when using
  another device these faster and slower baud rates are not applied as
  discovered and analyzed (with a scope) by Volker Richter (thank you!)
- Keep the package type around in struct ch348 as it's needed when
  modem controls and/or GPIOs are implemented.

Again, v8 is a work from Martin, I probably will never finish this work
without him.
Great thanks to him again

Corentin Labbe (2):
  usb: serial: add support for CH348
  usb: serial: add Martin and myself as maintainers of CH348

 MAINTAINERS                 |   6 +
 drivers/usb/serial/Kconfig  |   9 +
 drivers/usb/serial/Makefile |   1 +
 drivers/usb/serial/ch348.c  | 736 ++++++++++++++++++++++++++++++++++++
 4 files changed, 752 insertions(+)
 create mode 100644 drivers/usb/serial/ch348.c

-- 
2.45.2

[PATCH v8 1/2] usb: serial: add support for CH348

[Corentin Labbe]

The CH348 is an USB octo port serial adapter.
The device multiplexes all 8 ports in the same pair of Bulk endpoints.
Since there is no public datasheet, unfortunately it remains some magic values

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 drivers/usb/serial/Kconfig  |   9 +
 drivers/usb/serial/Makefile |   1 +
 drivers/usb/serial/ch348.c  | 736 ++++++++++++++++++++++++++++++++++++
 3 files changed, 746 insertions(+)
 create mode 100644 drivers/usb/serial/ch348.c

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index ef8d1c73c754..1e1842656b54 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -112,6 +112,15 @@ config USB_SERIAL_CH341
 	  To compile this driver as a module, choose M here: the
 	  module will be called ch341.
 
+config USB_SERIAL_CH348
+	tristate "USB Winchiphead CH348 Octo Port Serial Driver"
+	help
+	  Say Y here if you want to use a Winchiphead CH348 octo port
+	  USB to serial adapter.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ch348.
+
 config USB_SERIAL_WHITEHEAT
 	tristate "USB ConnectTech WhiteHEAT Serial Driver"
 	select USB_EZUSB_FX2
diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
index c7bb1a88173e..d16ff59fde68 100644
--- a/drivers/usb/serial/Makefile
+++ b/drivers/usb/serial/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_USB_SERIAL_AIRCABLE)		+= aircable.o
 obj-$(CONFIG_USB_SERIAL_ARK3116)		+= ark3116.o
 obj-$(CONFIG_USB_SERIAL_BELKIN)			+= belkin_sa.o
 obj-$(CONFIG_USB_SERIAL_CH341)			+= ch341.o
+obj-$(CONFIG_USB_SERIAL_CH348)			+= ch348.o
 obj-$(CONFIG_USB_SERIAL_CP210X)			+= cp210x.o
 obj-$(CONFIG_USB_SERIAL_CYBERJACK)		+= cyberjack.o
 obj-$(CONFIG_USB_SERIAL_CYPRESS_M8)		+= cypress_m8.o
diff --git a/drivers/usb/serial/ch348.c b/drivers/usb/serial/ch348.c
new file mode 100644
index 000000000000..01b61d74c4a4
--- /dev/null
+++ b/drivers/usb/serial/ch348.c
@@ -0,0 +1,736 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB serial driver for USB to Octal UARTs chip ch348.
+ *
+ * Copyright (C) 2023 Corentin Labbe <clabbe@baylibre.com>
+ * With the help of Neil Armstrong <neil.armstrong@linaro.org>
+ * Copyright (C) 2024 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ *
+ * Based on the ch9344 driver:
+ *   https://github.com/WCHSoftGroup/ch9344ser_linux/
+ *   Copyright (C) 2024 Nanjing Qinheng Microelectronics Co., Ltd.
+ */
+
+#include <linux/completion.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/kfifo.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/overflow.h>
+#include <linux/serial.h>
+#include <linux/serial_reg.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_driver.h>
+#include <linux/tty_flip.h>
+#include <linux/usb.h>
+#include <linux/usb/serial.h>
+#include <linux/workqueue.h>
+
+#define CH348_CMD_TIMEOUT   2000
+
+#define CH348_CTO_D	0x01
+#define CH348_CTO_R	0x02
+
+#define CH348_CTI_C	0x10
+#define CH348_CTI_DSR	0x20
+#define CH348_CTI_R	0x40
+#define CH348_CTI_DCD	0x80
+
+#define CMD_W_R		0xC0
+#define CMD_W_BR	0x80
+
+#define CMD_WB_E	0x90
+#define CMD_RB_E	0xC0
+
+#define M_NOR		0x00
+#define M_HF		0x03
+
+#define R_MOD		0x97
+#define R_IO_D		0x98
+#define R_IO_O		0x99
+#define R_IO_I		0x9b
+#define R_TM_O		0x9c
+#define R_INIT		0xa1
+
+#define CMD_VER		0x96
+
+/* 0x10 is normally UART_MCR_LOOP but for CH348 it's UART_MCR_RTS */
+#define UART_MCR_RTS_CH348	0x10
+
+/*
+ * The CH348 multiplexes rx & tx into a pair of Bulk USB endpoints for the 8
+ * serial ports, and another pair of Bulk USB endpoints to set port settings
+ * and receive port status events.
+ *
+ * The USB serial cores ties every Bulk endpoints pairs to each ports, In our
+ * case it will set port 0 with the rx/tx endpoints and port 1 with the
+ * setup/status endpoints.
+ *
+ * For bulk writes we skip all of USB serial core's helpers and implement it on
+ * our own since for serial TX we need to not only wait for the URB to complete
+ * but also for the UART_IIR_THRI signal.
+ *
+ * For bulk reads we use USB serial core's helpers, even for the status/int
+ * handling as it simplifies our code.
+ */
+#define CH348_MAXPORT				8
+#define CH348_PORTNUM_SERIAL_RX_TX		0
+#define CH348_PORTNUM_STATUS_INT_CONFIG		1
+
+#define CH348_RX_PORT_MAX_LENGTH	30
+
+struct ch348_rxbuf {
+	u8 port;
+	u8 length;
+	u8 data[CH348_RX_PORT_MAX_LENGTH];
+} __packed;
+
+struct ch348_txbuf {
+	u8 port;
+	__le16 length;
+	u8 data[];
+} __packed;
+
+#define CH348_TX_HDRSIZE offsetof(struct ch348_txbuf, data)
+
+struct ch348_initbuf {
+	u8 cmd;
+	u8 reg;
+	u8 port;
+	__be32 baudrate;
+	u8 format;
+	u8 paritytype;
+	u8 databits;
+	u8 rate;
+	u8 unknown;
+} __packed;
+
+#define CH348_INITBUF_FORMAT_STOPBITS		0x2
+#define CH348_INITBUF_FORMAT_NO_STOPBITS	0x0
+
+/*
+ * struct ch348_port - per-port information
+ * @uartmode:		UART port current mode
+ * @baudrate:		A cached copy of current baudrate for the RX logic
+ */
+struct ch348_port {
+	u8 uartmode;
+	speed_t baudrate;
+};
+
+/*
+ * struct ch348 - main container for all this driver information
+ * @udev:		pointer to the CH348 USB device
+ * @ports:		List of per-port information
+ * @serial:		pointer to the serial structure
+ * @write_work:		worker for processing the write queues
+ * @txbuf_completion:	indicates that the TX buffer has been fully written out
+ * @tx_ep:		endpoint number for serial data transmit/write operation
+ * @config_ep:		endpoint number for configure operations
+ * @small_package:	indicates package size: small (CH348Q) or large (CH348L)
+ */
+struct ch348 {
+	struct usb_device *udev;
+	struct ch348_port ports[CH348_MAXPORT];
+	struct usb_serial *serial;
+
+	struct work_struct write_work;
+	struct completion txbuf_completion;
+
+	int tx_ep;
+	int config_ep;
+
+	bool small_package;
+};
+
+struct ch348_serial_config {
+	u8 action;
+	u8 reg;
+	u8 control;
+} __packed;
+
+struct ch348_status_entry {
+	u8 portnum;
+	u8 reg_iir;
+	union {
+		u8 lsr_signal;
+		u8 modem_signal;
+		u8 init_data[10];
+	};
+} __packed;
+
+#define CH348_STATUS_ENTRY_PORTNUM_MASK		0xf
+
+static void ch348_process_status_urb(struct usb_serial *serial, struct urb *urb)
+{
+	struct ch348 *ch348 = usb_get_serial_data(serial);
+	struct ch348_status_entry *status_entry;
+	struct usb_serial_port *port;
+	unsigned int i, status_len;
+	u8 portnum;
+
+	if (urb->actual_length < 3) {
+		dev_warn_ratelimited(&ch348->udev->dev,
+				     "Received too short status buffer with %u bytes\n",
+				     urb->actual_length);
+		return;
+	}
+
+	for (i = 0; i < urb->actual_length;) {
+		status_entry = urb->transfer_buffer + i;
+		portnum = status_entry->portnum & CH348_STATUS_ENTRY_PORTNUM_MASK;
+
+		if (portnum >= CH348_MAXPORT) {
+			dev_warn_ratelimited(&ch348->udev->dev,
+					     "Invalid port %d in status entry\n",
+					     portnum);
+			break;
+		}
+
+		port = serial->port[portnum];
+		status_len = 3;
+
+		if (!status_entry->reg_iir) {
+			dev_dbg(&port->dev, "Ignoring status with zero reg_iir\n");
+		} else if (status_entry->reg_iir == R_INIT) {
+			status_len = 12;
+		} else if ((status_entry->reg_iir & UART_IIR_ID) == UART_IIR_RLSI) {
+			if (status_entry->lsr_signal & UART_LSR_OE)
+				port->icount.overrun++;
+			if (status_entry->lsr_signal & UART_LSR_PE)
+				port->icount.parity++;
+			if (status_entry->lsr_signal & UART_LSR_FE)
+				port->icount.frame++;
+			if (status_entry->lsr_signal & UART_LSR_BI)
+				port->icount.brk++;
+		} else if ((status_entry->reg_iir & UART_IIR_ID) == UART_IIR_THRI) {
+			complete_all(&ch348->txbuf_completion);
+		} else {
+			dev_warn_ratelimited(&port->dev,
+					     "Unsupported status with reg_iir 0x%02x\n",
+					     status_entry->reg_iir);
+		}
+
+		i += status_len;
+	}
+}
+
+static void ch348_process_serial_rx_urb(struct usb_serial *serial,
+					struct urb *urb)
+{
+	unsigned int portnum, serial_rx_len, i;
+	struct usb_serial_port *port;
+	struct ch348_rxbuf *rxb;
+
+	if (urb->actual_length < 2) {
+		dev_dbg(&serial->dev->dev, "Empty rx buffer\n");
+		return;
+	}
+
+	for (i = 0; i < urb->actual_length; i += sizeof(*rxb)) {
+		rxb = urb->transfer_buffer + i;
+		portnum = rxb->port;
+		if (portnum >= CH348_MAXPORT) {
+			dev_dbg(&serial->dev->dev, "Invalid port %d\n", portnum);
+			break;
+		}
+
+		port = serial->port[portnum];
+
+		serial_rx_len = rxb->length;
+		if (serial_rx_len > CH348_RX_PORT_MAX_LENGTH) {
+			dev_dbg(&port->dev, "Invalid length %d for port %d\n",
+				serial_rx_len, portnum);
+			break;
+		}
+
+		tty_insert_flip_string(&port->port, rxb->data, serial_rx_len);
+		tty_flip_buffer_push(&port->port);
+
+		port->icount.rx += serial_rx_len;
+	}
+}
+
+static void ch348_process_read_urb(struct urb *urb)
+{
+	struct usb_serial_port *port = urb->context;
+
+	if (port->port_number == CH348_PORTNUM_SERIAL_RX_TX)
+		ch348_process_serial_rx_urb(port->serial, urb);
+	else if (port->port_number == CH348_PORTNUM_STATUS_INT_CONFIG)
+		ch348_process_status_urb(port->serial, urb);
+	else
+		dev_warn_ratelimited(&port->serial->dev->dev,
+				     "Ignoring read URB callback for unknown port/endpoint %u\n",
+				     port->port_number);
+}
+
+static int ch348_port_config(struct usb_serial_port *port, u8 action, u8 reg,
+			     u8 control)
+{
+	struct ch348 *ch348 = usb_get_serial_data(port->serial);
+	struct ch348_serial_config *buffer;
+	int ret;
+
+	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	if (port->port_number < 4)
+		reg += 0x10 * port->port_number;
+	else
+		reg += 0x10 * (port->port_number - 4) + 0x08;
+
+	buffer->action = action;
+	buffer->reg = reg;
+	buffer->control = control;
+
+	ret = usb_bulk_msg(ch348->udev, ch348->config_ep, buffer,
+			   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
+	if (ret) {
+		dev_err(&ch348->udev->dev, "Failed to write port config: %d\n", ret);
+		goto out;
+	}
+
+out:
+	kfree(buffer);
+
+	return ret;
+}
+
+static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
+		       const unsigned char *buf, int count)
+{
+	struct ch348 *ch348 = usb_get_serial_data(port->serial);
+
+	if (!count)
+		return 0;
+
+	count = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock);
+
+	schedule_work(&ch348->write_work);
+
+	return count;
+}
+
+static int ch348_set_uartmode(struct usb_serial_port *port, u8 mode)
+{
+	struct ch348 *ch348 = usb_get_serial_data(port->serial);
+	unsigned int portnum = port->port_number;
+	int ret;
+
+	if (ch348->ports[portnum].uartmode == M_NOR && mode == M_HF) {
+		ret = ch348_port_config(port, CMD_W_BR, UART_MCR,
+					UART_MCR_DTR | UART_MCR_RTS_CH348 |
+					UART_MCR_TCRTLR);
+		if (ret)
+			return ret;
+		ch348->ports[portnum].uartmode = M_HF;
+	}
+
+	if (ch348->ports[portnum].uartmode == M_HF && mode == M_NOR) {
+		ret = ch348_port_config(port, CMD_W_BR, UART_MCR,
+					UART_MCR_RTS_CH348 | UART_MCR_TCRTLR);
+		if (ret)
+			return ret;
+		ch348->ports[portnum].uartmode = M_NOR;
+	}
+	return 0;
+}
+
+static void ch348_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
+			      const struct ktermios *termios_old)
+{
+	struct ch348 *ch348 = usb_get_serial_data(port->serial);
+	struct ktermios *termios = &tty->termios;
+	int ret, portnum = port->port_number;
+	struct ch348_initbuf *buffer;
+	speed_t	baudrate;
+
+	if (termios_old && !tty_termios_hw_change(&tty->termios, termios_old))
+		return;
+
+	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
+	if (!buffer)
+		goto out;
+
+	/*
+	 * The datasheet states that only baud rates in range of 1200..6000000
+	 * are supported. Tests with an oscilloscope confirm that even when
+	 * configuring a baud rate slower than 1200 the output stays at around
+	 * 1200 baud.
+	 */
+	baudrate = clamp(tty_get_baud_rate(tty), 1200, 6000000);
+	tty_termios_encode_baud_rate(&tty->termios, baudrate, baudrate);
+	ch348->ports[port->port_number].baudrate = baudrate;
+
+	if (termios->c_cflag & PARENB) {
+		if  (termios->c_cflag & CMSPAR) {
+			if (termios->c_cflag & PARODD)
+				buffer->paritytype = 3;
+			else
+				buffer->paritytype = 4;
+		} else if (termios->c_cflag & PARODD) {
+			buffer->paritytype = 1;
+		} else {
+			buffer->paritytype = 2;
+		}
+	} else {
+		buffer->paritytype = 0;
+	}
+
+	switch (C_CSIZE(tty)) {
+	case CS5:
+		buffer->databits = 5;
+		break;
+	case CS6:
+		buffer->databits = 6;
+		break;
+	case CS7:
+		buffer->databits = 7;
+		break;
+	case CS8:
+	default:
+		buffer->databits = 8;
+		break;
+	}
+
+	buffer->cmd = CMD_WB_E | portnum;
+	buffer->reg = R_INIT;
+	buffer->port = portnum;
+	buffer->baudrate = cpu_to_be32(baudrate);
+
+	if (termios->c_cflag & CSTOPB)
+		buffer->format = CH348_INITBUF_FORMAT_STOPBITS;
+	else
+		buffer->format = CH348_INITBUF_FORMAT_NO_STOPBITS;
+
+	buffer->rate = max_t(speed_t, 5, (10000 * 15 / baudrate) + 1);
+
+	ret = usb_bulk_msg(ch348->udev, ch348->config_ep, buffer,
+			   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
+	if (ret < 0) {
+		dev_err(&ch348->udev->dev, "Failed to change line settings: err=%d\n",
+			ret);
+		goto out_free;
+	}
+
+	ret = ch348_port_config(port, CMD_W_R, UART_IER, UART_IER_RDI |
+				UART_IER_THRI | UART_IER_RLSI | UART_IER_MSI);
+	if (ret < 0)
+		goto out_free;
+
+	if (C_CRTSCTS(tty))
+		ret = ch348_set_uartmode(port, M_HF);
+	else
+		ret = ch348_set_uartmode(port, M_NOR);
+
+out_free:
+	kfree(buffer);
+out:
+	if (ret && termios_old)
+		tty->termios = *termios_old;
+}
+
+static int ch348_open(struct tty_struct *tty, struct usb_serial_port *port)
+{
+	int ret;
+
+	clear_bit(USB_SERIAL_THROTTLED, &port->flags);
+
+	if (tty)
+		ch348_set_termios(tty, port, NULL);
+
+	ret = ch348_port_config(port, CMD_W_R, UART_FCR,
+				UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
+				UART_FCR_CLEAR_XMIT | UART_FCR_T_TRIG_00 |
+				UART_FCR_R_TRIG_10);
+	if (ret) {
+		dev_err(&port->serial->dev->dev,
+			"Failed to configure UART_FCR, err=%d\n", ret);
+		return ret;
+	}
+
+	ret = ch348_port_config(port, CMD_W_BR, UART_MCR, UART_MCR_OUT2);
+	if (ret) {
+		dev_err(&port->serial->dev->dev,
+			"Failed to configure UART_MCR, err=%d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void ch348_close(struct usb_serial_port *port)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	kfifo_reset_out(&port->write_fifo);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void ch348_write_work(struct work_struct *work)
+{
+	struct ch348 *ch348 = container_of(work, struct ch348, write_work);
+	struct usb_serial_port *port, *hw_tx_port;
+	unsigned int i, max_bytes;
+	struct ch348_txbuf *rxt;
+	unsigned long flags;
+	int ret, count;
+
+	reinit_completion(&ch348->txbuf_completion);
+
+	hw_tx_port = ch348->serial->port[CH348_PORTNUM_SERIAL_RX_TX];
+	rxt = hw_tx_port->write_urbs[0]->transfer_buffer;
+
+	for (i = 0; i < CH348_MAXPORT; i++) {
+		port = ch348->serial->port[i];
+
+		if (ch348->ports[i].baudrate < 9600)
+			/*
+			 * Writing larger buffers can take longer than the
+			 * hardware allows before discarding the write buffer.
+			 * Limit the transfer size in such cases.
+			 * These values have been found by empirical testing.
+			 */
+			max_bytes = 128;
+		else
+			/*
+			 * Only ingest as many bytes as we can transfer with
+			 * one URB at a time keeping the TX header in mind.
+			 */
+			max_bytes = hw_tx_port->bulk_out_size - CH348_TX_HDRSIZE;
+
+		count = kfifo_out_locked(&port->write_fifo, rxt->data,
+					 max_bytes, &port->lock);
+		if (count)
+			break;
+	}
+
+	if (!count)
+		return;
+
+	spin_lock_irqsave(&port->lock, flags);
+	port->tx_bytes += count;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	rxt->port = port->port_number;
+	rxt->length = cpu_to_le16(count);
+
+	usb_serial_debug_data(&port->dev, __func__, count + CH348_TX_HDRSIZE,
+			      (const unsigned char *)rxt);
+
+	ret = usb_bulk_msg(ch348->udev, ch348->tx_ep, rxt,
+			   count + CH348_TX_HDRSIZE, NULL, CH348_CMD_TIMEOUT);
+	if (ret) {
+		dev_err_console(port,
+				"Failed to bulk write TX buffer, err=%d\n",
+				ret);
+		goto write_done;
+	}
+
+	if (!wait_for_completion_timeout(&ch348->txbuf_completion,
+					 msecs_to_jiffies(CH348_CMD_TIMEOUT)))
+		dev_err_console(port,
+				"Failed to wait for TX buffer to be fully written out\n");
+
+write_done:
+	spin_lock_irqsave(&port->lock, flags);
+	port->tx_bytes -= count;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	port->icount.tx += count;
+
+	schedule_work(&ch348->write_work);
+	usb_serial_port_softint(port);
+}
+
+static int ch348_submit_urbs(struct usb_serial *serial)
+{
+	int ret;
+
+	ret = usb_serial_generic_open(NULL,
+				      serial->port[CH348_PORTNUM_SERIAL_RX_TX]);
+	if (ret) {
+		dev_err(&serial->dev->dev,
+			"Failed to open RX/TX port, err=%d\n", ret);
+		return ret;
+	}
+
+	ret = usb_serial_generic_open(NULL,
+				      serial->port[CH348_PORTNUM_STATUS_INT_CONFIG]);
+	if (ret) {
+		dev_err(&serial->dev->dev,
+			"Failed to submit STATUS/INT URB, err=%d\n", ret);
+		usb_serial_generic_close(serial->port[CH348_PORTNUM_SERIAL_RX_TX]);
+	}
+
+	return ret;
+}
+
+static void ch348_kill_urbs(struct usb_serial *serial)
+{
+	usb_serial_generic_close(serial->port[CH348_PORTNUM_STATUS_INT_CONFIG]);
+	usb_serial_generic_close(serial->port[CH348_PORTNUM_SERIAL_RX_TX]);
+}
+
+static int ch348_detect_version(struct usb_serial *serial)
+{
+	struct ch348 *ch348 = usb_get_serial_data(serial);
+	u8 *version_buf;
+	int ret;
+
+	version_buf = kzalloc(4, GFP_KERNEL);
+	if (!version_buf)
+		return -ENOMEM;
+
+	ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+			      CMD_VER,
+			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			      0, 0, version_buf, 4, CH348_CMD_TIMEOUT);
+	if (ret < 0) {
+		dev_err(&serial->dev->dev, "Failed to read CMD_VER: %d\n", ret);
+		goto out;
+	}
+
+	ret = 0;
+	ch348->small_package = !!(version_buf[1] & 0x80);
+
+	dev_info(&serial->dev->dev, "Found WCH CH348%c\n",
+		 ch348->small_package ? 'Q' : 'L');
+
+out:
+	kfree(version_buf);
+
+	return ret;
+}
+
+static int ch348_attach(struct usb_serial *serial)
+{
+	struct usb_serial_port *tx_port, *config_port;
+	struct ch348 *ch348;
+	int ret;
+
+	ch348 = kzalloc(sizeof(*ch348), GFP_KERNEL);
+	if (!ch348)
+		return -ENOMEM;
+
+	usb_set_serial_data(serial, ch348);
+
+	ch348->udev = serial->dev;
+	ch348->serial = serial;
+
+	INIT_WORK(&ch348->write_work, ch348_write_work);
+
+	init_completion(&ch348->txbuf_completion);
+
+	tx_port = ch348->serial->port[CH348_PORTNUM_SERIAL_RX_TX];
+	ch348->tx_ep = usb_sndbulkpipe(ch348->udev,
+				       tx_port->bulk_out_endpointAddress);
+
+	config_port = ch348->serial->port[CH348_PORTNUM_STATUS_INT_CONFIG];
+	ch348->config_ep = usb_sndbulkpipe(ch348->udev,
+					   config_port->bulk_out_endpointAddress);
+
+	ret = ch348_detect_version(serial);
+	if (ret)
+		goto err_free_ch348;
+
+	ret = ch348_submit_urbs(serial);
+	if (ret)
+		goto err_free_ch348;
+
+	return 0;
+
+err_free_ch348:
+	kfree(ch348);
+	return ret;
+}
+
+static void ch348_release(struct usb_serial *serial)
+{
+	struct ch348 *ch348 = usb_get_serial_data(serial);
+
+	cancel_work_sync(&ch348->write_work);
+	ch348_kill_urbs(serial);
+
+	kfree(ch348);
+}
+
+static int ch348_calc_num_ports(struct usb_serial *serial,
+				struct usb_serial_endpoints *epds)
+{
+	int i;
+
+	epds->num_bulk_out = CH348_MAXPORT;
+
+	for (i = serial->type->num_bulk_out; i < CH348_MAXPORT; ++i)
+		epds->bulk_out[i] = epds->bulk_out[0];
+
+	return CH348_MAXPORT;
+}
+
+static int ch348_suspend(struct usb_serial *serial, pm_message_t message)
+{
+	struct ch348 *ch348 = usb_get_serial_data(serial);
+
+	cancel_work_sync(&ch348->write_work);
+
+	return 0;
+}
+
+static int ch348_resume(struct usb_serial *serial)
+{
+	struct ch348 *ch348 = usb_get_serial_data(serial);
+	int ret;
+
+	ret = ch348_submit_urbs(serial);
+	if (ret)
+		return ret;
+
+	schedule_work(&ch348->write_work);
+
+	return 0;
+}
+
+static const struct usb_device_id ch348_ids[] = {
+	{ USB_DEVICE(0x1a86, 0x55d9), },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(usb, ch348_ids);
+
+static struct usb_serial_driver ch348_device = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "ch348",
+	},
+	.id_table =		ch348_ids,
+	.num_ports =		CH348_MAXPORT,
+	.num_bulk_in =		2,
+	.num_bulk_out =		2,
+	.open =			ch348_open,
+	.close =		ch348_close,
+	.set_termios =		ch348_set_termios,
+	.process_read_urb =	ch348_process_read_urb,
+	.write =		ch348_write,
+	.calc_num_ports =	ch348_calc_num_ports,
+	.attach =		ch348_attach,
+	.release =		ch348_release,
+	.suspend =		ch348_suspend,
+	.resume =		ch348_resume,
+};
+
+static struct usb_serial_driver * const serial_drivers[] = {
+	&ch348_device, NULL
+};
+
+module_usb_serial_driver(serial_drivers, ch348_ids);
+
+MODULE_AUTHOR("Corentin Labbe <clabbe@baylibre.com>");
+MODULE_DESCRIPTION("USB CH348 Octo port serial converter driver");
+MODULE_LICENSE("GPL");
-- 
2.45.2

[PATCH v8 2/2] usb: serial: add Martin and myself as maintainers of CH348

[Corentin Labbe]

Martin and I did the driver and have hardware to test, set ourselves as
maintainer of it.

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 481b2dac1716..42558a70dcb0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24530,6 +24530,12 @@ F:	Documentation/usb/usb-serial.rst
 F:	drivers/usb/serial/
 F:	include/linux/usb/serial.h
 
+USB SERIAL DRIVER FOR CH348
+M:	Corentin Labbe <clabbe@baylibre.com>
+M:	Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+S:	Maintained
+F:	drivers/usb/serial/ch348.c
+
 USB SMSC75XX ETHERNET DRIVER
 M:	Steve Glendinning <steve.glendinning@shawell.net>
 L:	netdev@vger.kernel.org
-- 
2.45.2

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[David Heidelberg]

Thank you for 8th revision!

Acked-by: David Heidelberg <david@ixit.cz>

On 04/02/2025 14:58, Corentin Labbe wrote:
> The CH348 is an USB octo port serial adapter.
> The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> Since there is no public datasheet, unfortunately it remains some magic values
> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>   drivers/usb/serial/Kconfig  |   9 +
>   drivers/usb/serial/Makefile |   1 +
>   drivers/usb/serial/ch348.c  | 736 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 746 insertions(+)
>   create mode 100644 drivers/usb/serial/ch348.c
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index ef8d1c73c754..1e1842656b54 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -112,6 +112,15 @@ config USB_SERIAL_CH341
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called ch341.
>   
> +config USB_SERIAL_CH348
> +	tristate "USB Winchiphead CH348 Octo Port Serial Driver"
> +	help
> +	  Say Y here if you want to use a Winchiphead CH348 octo port
> +	  USB to serial adapter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ch348.
> +
>   config USB_SERIAL_WHITEHEAT
>   	tristate "USB ConnectTech WhiteHEAT Serial Driver"
>   	select USB_EZUSB_FX2
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index c7bb1a88173e..d16ff59fde68 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_USB_SERIAL_AIRCABLE)		+= aircable.o
>   obj-$(CONFIG_USB_SERIAL_ARK3116)		+= ark3116.o
>   obj-$(CONFIG_USB_SERIAL_BELKIN)			+= belkin_sa.o
>   obj-$(CONFIG_USB_SERIAL_CH341)			+= ch341.o
> +obj-$(CONFIG_USB_SERIAL_CH348)			+= ch348.o
>   obj-$(CONFIG_USB_SERIAL_CP210X)			+= cp210x.o
>   obj-$(CONFIG_USB_SERIAL_CYBERJACK)		+= cyberjack.o
>   obj-$(CONFIG_USB_SERIAL_CYPRESS_M8)		+= cypress_m8.o
> diff --git a/drivers/usb/serial/ch348.c b/drivers/usb/serial/ch348.c
> new file mode 100644
> index 000000000000..01b61d74c4a4
> --- /dev/null
> +++ b/drivers/usb/serial/ch348.c
> @@ -0,0 +1,736 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * USB serial driver for USB to Octal UARTs chip ch348.
> + *
> + * Copyright (C) 2023 Corentin Labbe <clabbe@baylibre.com>
> + * With the help of Neil Armstrong <neil.armstrong@linaro.org>
> + * Copyright (C) 2024 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> + *
> + * Based on the ch9344 driver:
> + *   https://github.com/WCHSoftGroup/ch9344ser_linux/
> + *   Copyright (C) 2024 Nanjing Qinheng Microelectronics Co., Ltd.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/kfifo.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
> +#include <linux/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/tty_flip.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +#include <linux/workqueue.h>
> +
> +#define CH348_CMD_TIMEOUT   2000
> +
> +#define CH348_CTO_D	0x01
> +#define CH348_CTO_R	0x02
> +
> +#define CH348_CTI_C	0x10
> +#define CH348_CTI_DSR	0x20
> +#define CH348_CTI_R	0x40
> +#define CH348_CTI_DCD	0x80
> +
> +#define CMD_W_R		0xC0
> +#define CMD_W_BR	0x80
> +
> +#define CMD_WB_E	0x90
> +#define CMD_RB_E	0xC0
> +
> +#define M_NOR		0x00
> +#define M_HF		0x03
> +
> +#define R_MOD		0x97
> +#define R_IO_D		0x98
> +#define R_IO_O		0x99
> +#define R_IO_I		0x9b
> +#define R_TM_O		0x9c
> +#define R_INIT		0xa1
> +
> +#define CMD_VER		0x96
> +
> +/* 0x10 is normally UART_MCR_LOOP but for CH348 it's UART_MCR_RTS */
> +#define UART_MCR_RTS_CH348	0x10
> +
> +/*
> + * The CH348 multiplexes rx & tx into a pair of Bulk USB endpoints for the 8
> + * serial ports, and another pair of Bulk USB endpoints to set port settings
> + * and receive port status events.
> + *
> + * The USB serial cores ties every Bulk endpoints pairs to each ports, In our
> + * case it will set port 0 with the rx/tx endpoints and port 1 with the
> + * setup/status endpoints.
> + *
> + * For bulk writes we skip all of USB serial core's helpers and implement it on
> + * our own since for serial TX we need to not only wait for the URB to complete
> + * but also for the UART_IIR_THRI signal.
> + *
> + * For bulk reads we use USB serial core's helpers, even for the status/int
> + * handling as it simplifies our code.
> + */
> +#define CH348_MAXPORT				8
> +#define CH348_PORTNUM_SERIAL_RX_TX		0
> +#define CH348_PORTNUM_STATUS_INT_CONFIG		1
> +
> +#define CH348_RX_PORT_MAX_LENGTH	30
> +
> +struct ch348_rxbuf {
> +	u8 port;
> +	u8 length;
> +	u8 data[CH348_RX_PORT_MAX_LENGTH];
> +} __packed;
> +
> +struct ch348_txbuf {
> +	u8 port;
> +	__le16 length;
> +	u8 data[];
> +} __packed;
> +
> +#define CH348_TX_HDRSIZE offsetof(struct ch348_txbuf, data)
> +
> +struct ch348_initbuf {
> +	u8 cmd;
> +	u8 reg;
> +	u8 port;
> +	__be32 baudrate;
> +	u8 format;
> +	u8 paritytype;
> +	u8 databits;
> +	u8 rate;
> +	u8 unknown;
> +} __packed;
> +
> +#define CH348_INITBUF_FORMAT_STOPBITS		0x2
> +#define CH348_INITBUF_FORMAT_NO_STOPBITS	0x0
> +
> +/*
> + * struct ch348_port - per-port information
> + * @uartmode:		UART port current mode
> + * @baudrate:		A cached copy of current baudrate for the RX logic
> + */
> +struct ch348_port {
> +	u8 uartmode;
> +	speed_t baudrate;
> +};
> +
> +/*
> + * struct ch348 - main container for all this driver information
> + * @udev:		pointer to the CH348 USB device
> + * @ports:		List of per-port information
> + * @serial:		pointer to the serial structure
> + * @write_work:		worker for processing the write queues
> + * @txbuf_completion:	indicates that the TX buffer has been fully written out
> + * @tx_ep:		endpoint number for serial data transmit/write operation
> + * @config_ep:		endpoint number for configure operations
> + * @small_package:	indicates package size: small (CH348Q) or large (CH348L)
> + */
> +struct ch348 {
> +	struct usb_device *udev;
> +	struct ch348_port ports[CH348_MAXPORT];
> +	struct usb_serial *serial;
> +
> +	struct work_struct write_work;
> +	struct completion txbuf_completion;
> +
> +	int tx_ep;
> +	int config_ep;
> +
> +	bool small_package;
> +};
> +
> +struct ch348_serial_config {
> +	u8 action;
> +	u8 reg;
> +	u8 control;
> +} __packed;
> +
> +struct ch348_status_entry {
> +	u8 portnum;
> +	u8 reg_iir;
> +	union {
> +		u8 lsr_signal;
> +		u8 modem_signal;
> +		u8 init_data[10];
> +	};
> +} __packed;
> +
> +#define CH348_STATUS_ENTRY_PORTNUM_MASK		0xf
> +
> +static void ch348_process_status_urb(struct usb_serial *serial, struct urb *urb)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +	struct ch348_status_entry *status_entry;
> +	struct usb_serial_port *port;
> +	unsigned int i, status_len;
> +	u8 portnum;
> +
> +	if (urb->actual_length < 3) {
> +		dev_warn_ratelimited(&ch348->udev->dev,
> +				     "Received too short status buffer with %u bytes\n",
> +				     urb->actual_length);
> +		return;
> +	}
> +
> +	for (i = 0; i < urb->actual_length;) {
> +		status_entry = urb->transfer_buffer + i;
> +		portnum = status_entry->portnum & CH348_STATUS_ENTRY_PORTNUM_MASK;
> +
> +		if (portnum >= CH348_MAXPORT) {
> +			dev_warn_ratelimited(&ch348->udev->dev,
> +					     "Invalid port %d in status entry\n",
> +					     portnum);
> +			break;
> +		}
> +
> +		port = serial->port[portnum];
> +		status_len = 3;
> +
> +		if (!status_entry->reg_iir) {
> +			dev_dbg(&port->dev, "Ignoring status with zero reg_iir\n");
> +		} else if (status_entry->reg_iir == R_INIT) {
> +			status_len = 12;
> +		} else if ((status_entry->reg_iir & UART_IIR_ID) == UART_IIR_RLSI) {
> +			if (status_entry->lsr_signal & UART_LSR_OE)
> +				port->icount.overrun++;
> +			if (status_entry->lsr_signal & UART_LSR_PE)
> +				port->icount.parity++;
> +			if (status_entry->lsr_signal & UART_LSR_FE)
> +				port->icount.frame++;
> +			if (status_entry->lsr_signal & UART_LSR_BI)
> +				port->icount.brk++;
> +		} else if ((status_entry->reg_iir & UART_IIR_ID) == UART_IIR_THRI) {
> +			complete_all(&ch348->txbuf_completion);
> +		} else {
> +			dev_warn_ratelimited(&port->dev,
> +					     "Unsupported status with reg_iir 0x%02x\n",
> +					     status_entry->reg_iir);
> +		}
> +
> +		i += status_len;
> +	}
> +}
> +
> +static void ch348_process_serial_rx_urb(struct usb_serial *serial,
> +					struct urb *urb)
> +{
> +	unsigned int portnum, serial_rx_len, i;
> +	struct usb_serial_port *port;
> +	struct ch348_rxbuf *rxb;
> +
> +	if (urb->actual_length < 2) {
> +		dev_dbg(&serial->dev->dev, "Empty rx buffer\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < urb->actual_length; i += sizeof(*rxb)) {
> +		rxb = urb->transfer_buffer + i;
> +		portnum = rxb->port;
> +		if (portnum >= CH348_MAXPORT) {
> +			dev_dbg(&serial->dev->dev, "Invalid port %d\n", portnum);
> +			break;
> +		}
> +
> +		port = serial->port[portnum];
> +
> +		serial_rx_len = rxb->length;
> +		if (serial_rx_len > CH348_RX_PORT_MAX_LENGTH) {
> +			dev_dbg(&port->dev, "Invalid length %d for port %d\n",
> +				serial_rx_len, portnum);
> +			break;
> +		}
> +
> +		tty_insert_flip_string(&port->port, rxb->data, serial_rx_len);
> +		tty_flip_buffer_push(&port->port);
> +
> +		port->icount.rx += serial_rx_len;
> +	}
> +}
> +
> +static void ch348_process_read_urb(struct urb *urb)
> +{
> +	struct usb_serial_port *port = urb->context;
> +
> +	if (port->port_number == CH348_PORTNUM_SERIAL_RX_TX)
> +		ch348_process_serial_rx_urb(port->serial, urb);
> +	else if (port->port_number == CH348_PORTNUM_STATUS_INT_CONFIG)
> +		ch348_process_status_urb(port->serial, urb);
> +	else
> +		dev_warn_ratelimited(&port->serial->dev->dev,
> +				     "Ignoring read URB callback for unknown port/endpoint %u\n",
> +				     port->port_number);
> +}
> +
> +static int ch348_port_config(struct usb_serial_port *port, u8 action, u8 reg,
> +			     u8 control)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	struct ch348_serial_config *buffer;
> +	int ret;
> +
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	if (port->port_number < 4)
> +		reg += 0x10 * port->port_number;
> +	else
> +		reg += 0x10 * (port->port_number - 4) + 0x08;
> +
> +	buffer->action = action;
> +	buffer->reg = reg;
> +	buffer->control = control;
> +
> +	ret = usb_bulk_msg(ch348->udev, ch348->config_ep, buffer,
> +			   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
> +	if (ret) {
> +		dev_err(&ch348->udev->dev, "Failed to write port config: %d\n", ret);
> +		goto out;
> +	}
> +
> +out:
> +	kfree(buffer);
> +
> +	return ret;
> +}
> +
> +static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
> +		       const unsigned char *buf, int count)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +
> +	if (!count)
> +		return 0;
> +
> +	count = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock);
> +
> +	schedule_work(&ch348->write_work);
> +
> +	return count;
> +}
> +
> +static int ch348_set_uartmode(struct usb_serial_port *port, u8 mode)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	unsigned int portnum = port->port_number;
> +	int ret;
> +
> +	if (ch348->ports[portnum].uartmode == M_NOR && mode == M_HF) {
> +		ret = ch348_port_config(port, CMD_W_BR, UART_MCR,
> +					UART_MCR_DTR | UART_MCR_RTS_CH348 |
> +					UART_MCR_TCRTLR);
> +		if (ret)
> +			return ret;
> +		ch348->ports[portnum].uartmode = M_HF;
> +	}
> +
> +	if (ch348->ports[portnum].uartmode == M_HF && mode == M_NOR) {
> +		ret = ch348_port_config(port, CMD_W_BR, UART_MCR,
> +					UART_MCR_RTS_CH348 | UART_MCR_TCRTLR);
> +		if (ret)
> +			return ret;
> +		ch348->ports[portnum].uartmode = M_NOR;
> +	}
> +	return 0;
> +}
> +
> +static void ch348_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
> +			      const struct ktermios *termios_old)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	struct ktermios *termios = &tty->termios;
> +	int ret, portnum = port->port_number;
> +	struct ch348_initbuf *buffer;
> +	speed_t	baudrate;
> +
> +	if (termios_old && !tty_termios_hw_change(&tty->termios, termios_old))
> +		return;
> +
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer)
> +		goto out;
> +
> +	/*
> +	 * The datasheet states that only baud rates in range of 1200..6000000
> +	 * are supported. Tests with an oscilloscope confirm that even when
> +	 * configuring a baud rate slower than 1200 the output stays at around
> +	 * 1200 baud.
> +	 */
> +	baudrate = clamp(tty_get_baud_rate(tty), 1200, 6000000);
> +	tty_termios_encode_baud_rate(&tty->termios, baudrate, baudrate);
> +	ch348->ports[port->port_number].baudrate = baudrate;
> +
> +	if (termios->c_cflag & PARENB) {
> +		if  (termios->c_cflag & CMSPAR) {
> +			if (termios->c_cflag & PARODD)
> +				buffer->paritytype = 3;
> +			else
> +				buffer->paritytype = 4;
> +		} else if (termios->c_cflag & PARODD) {
> +			buffer->paritytype = 1;
> +		} else {
> +			buffer->paritytype = 2;
> +		}
> +	} else {
> +		buffer->paritytype = 0;
> +	}
> +
> +	switch (C_CSIZE(tty)) {
> +	case CS5:
> +		buffer->databits = 5;
> +		break;
> +	case CS6:
> +		buffer->databits = 6;
> +		break;
> +	case CS7:
> +		buffer->databits = 7;
> +		break;
> +	case CS8:
> +	default:
> +		buffer->databits = 8;
> +		break;
> +	}
> +
> +	buffer->cmd = CMD_WB_E | portnum;
> +	buffer->reg = R_INIT;
> +	buffer->port = portnum;
> +	buffer->baudrate = cpu_to_be32(baudrate);
> +
> +	if (termios->c_cflag & CSTOPB)
> +		buffer->format = CH348_INITBUF_FORMAT_STOPBITS;
> +	else
> +		buffer->format = CH348_INITBUF_FORMAT_NO_STOPBITS;
> +
> +	buffer->rate = max_t(speed_t, 5, (10000 * 15 / baudrate) + 1);
> +
> +	ret = usb_bulk_msg(ch348->udev, ch348->config_ep, buffer,
> +			   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
> +	if (ret < 0) {
> +		dev_err(&ch348->udev->dev, "Failed to change line settings: err=%d\n",
> +			ret);
> +		goto out_free;
> +	}
> +
> +	ret = ch348_port_config(port, CMD_W_R, UART_IER, UART_IER_RDI |
> +				UART_IER_THRI | UART_IER_RLSI | UART_IER_MSI);
> +	if (ret < 0)
> +		goto out_free;
> +
> +	if (C_CRTSCTS(tty))
> +		ret = ch348_set_uartmode(port, M_HF);
> +	else
> +		ret = ch348_set_uartmode(port, M_NOR);
> +
> +out_free:
> +	kfree(buffer);
> +out:
> +	if (ret && termios_old)
> +		tty->termios = *termios_old;
> +}
> +
> +static int ch348_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	int ret;
> +
> +	clear_bit(USB_SERIAL_THROTTLED, &port->flags);
> +
> +	if (tty)
> +		ch348_set_termios(tty, port, NULL);
> +
> +	ret = ch348_port_config(port, CMD_W_R, UART_FCR,
> +				UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> +				UART_FCR_CLEAR_XMIT | UART_FCR_T_TRIG_00 |
> +				UART_FCR_R_TRIG_10);
> +	if (ret) {
> +		dev_err(&port->serial->dev->dev,
> +			"Failed to configure UART_FCR, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = ch348_port_config(port, CMD_W_BR, UART_MCR, UART_MCR_OUT2);
> +	if (ret) {
> +		dev_err(&port->serial->dev->dev,
> +			"Failed to configure UART_MCR, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ch348_close(struct usb_serial_port *port)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	kfifo_reset_out(&port->write_fifo);
> +	spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static void ch348_write_work(struct work_struct *work)
> +{
> +	struct ch348 *ch348 = container_of(work, struct ch348, write_work);
> +	struct usb_serial_port *port, *hw_tx_port;
> +	unsigned int i, max_bytes;
> +	struct ch348_txbuf *rxt;
> +	unsigned long flags;
> +	int ret, count;
> +
> +	reinit_completion(&ch348->txbuf_completion);
> +
> +	hw_tx_port = ch348->serial->port[CH348_PORTNUM_SERIAL_RX_TX];
> +	rxt = hw_tx_port->write_urbs[0]->transfer_buffer;
> +
> +	for (i = 0; i < CH348_MAXPORT; i++) {
> +		port = ch348->serial->port[i];
> +
> +		if (ch348->ports[i].baudrate < 9600)
> +			/*
> +			 * Writing larger buffers can take longer than the
> +			 * hardware allows before discarding the write buffer.
> +			 * Limit the transfer size in such cases.
> +			 * These values have been found by empirical testing.
> +			 */
> +			max_bytes = 128;
> +		else
> +			/*
> +			 * Only ingest as many bytes as we can transfer with
> +			 * one URB at a time keeping the TX header in mind.
> +			 */
> +			max_bytes = hw_tx_port->bulk_out_size - CH348_TX_HDRSIZE;
> +
> +		count = kfifo_out_locked(&port->write_fifo, rxt->data,
> +					 max_bytes, &port->lock);
> +		if (count)
> +			break;
> +	}
> +
> +	if (!count)
> +		return;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	port->tx_bytes += count;
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	rxt->port = port->port_number;
> +	rxt->length = cpu_to_le16(count);
> +
> +	usb_serial_debug_data(&port->dev, __func__, count + CH348_TX_HDRSIZE,
> +			      (const unsigned char *)rxt);
> +
> +	ret = usb_bulk_msg(ch348->udev, ch348->tx_ep, rxt,
> +			   count + CH348_TX_HDRSIZE, NULL, CH348_CMD_TIMEOUT);
> +	if (ret) {
> +		dev_err_console(port,
> +				"Failed to bulk write TX buffer, err=%d\n",
> +				ret);
> +		goto write_done;
> +	}
> +
> +	if (!wait_for_completion_timeout(&ch348->txbuf_completion,
> +					 msecs_to_jiffies(CH348_CMD_TIMEOUT)))
> +		dev_err_console(port,
> +				"Failed to wait for TX buffer to be fully written out\n");
> +
> +write_done:
> +	spin_lock_irqsave(&port->lock, flags);
> +	port->tx_bytes -= count;
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	port->icount.tx += count;
> +
> +	schedule_work(&ch348->write_work);
> +	usb_serial_port_softint(port);
> +}
> +
> +static int ch348_submit_urbs(struct usb_serial *serial)
> +{
> +	int ret;
> +
> +	ret = usb_serial_generic_open(NULL,
> +				      serial->port[CH348_PORTNUM_SERIAL_RX_TX]);
> +	if (ret) {
> +		dev_err(&serial->dev->dev,
> +			"Failed to open RX/TX port, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = usb_serial_generic_open(NULL,
> +				      serial->port[CH348_PORTNUM_STATUS_INT_CONFIG]);
> +	if (ret) {
> +		dev_err(&serial->dev->dev,
> +			"Failed to submit STATUS/INT URB, err=%d\n", ret);
> +		usb_serial_generic_close(serial->port[CH348_PORTNUM_SERIAL_RX_TX]);
> +	}
> +
> +	return ret;
> +}
> +
> +static void ch348_kill_urbs(struct usb_serial *serial)
> +{
> +	usb_serial_generic_close(serial->port[CH348_PORTNUM_STATUS_INT_CONFIG]);
> +	usb_serial_generic_close(serial->port[CH348_PORTNUM_SERIAL_RX_TX]);
> +}
> +
> +static int ch348_detect_version(struct usb_serial *serial)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +	u8 *version_buf;
> +	int ret;
> +
> +	version_buf = kzalloc(4, GFP_KERNEL);
> +	if (!version_buf)
> +		return -ENOMEM;
> +
> +	ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +			      CMD_VER,
> +			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> +			      0, 0, version_buf, 4, CH348_CMD_TIMEOUT);
> +	if (ret < 0) {
> +		dev_err(&serial->dev->dev, "Failed to read CMD_VER: %d\n", ret);
> +		goto out;
> +	}
> +
> +	ret = 0;
> +	ch348->small_package = !!(version_buf[1] & 0x80);
> +
> +	dev_info(&serial->dev->dev, "Found WCH CH348%c\n",
> +		 ch348->small_package ? 'Q' : 'L');
> +
> +out:
> +	kfree(version_buf);
> +
> +	return ret;
> +}
> +
> +static int ch348_attach(struct usb_serial *serial)
> +{
> +	struct usb_serial_port *tx_port, *config_port;
> +	struct ch348 *ch348;
> +	int ret;
> +
> +	ch348 = kzalloc(sizeof(*ch348), GFP_KERNEL);
> +	if (!ch348)
> +		return -ENOMEM;
> +
> +	usb_set_serial_data(serial, ch348);
> +
> +	ch348->udev = serial->dev;
> +	ch348->serial = serial;
> +
> +	INIT_WORK(&ch348->write_work, ch348_write_work);
> +
> +	init_completion(&ch348->txbuf_completion);
> +
> +	tx_port = ch348->serial->port[CH348_PORTNUM_SERIAL_RX_TX];
> +	ch348->tx_ep = usb_sndbulkpipe(ch348->udev,
> +				       tx_port->bulk_out_endpointAddress);
> +
> +	config_port = ch348->serial->port[CH348_PORTNUM_STATUS_INT_CONFIG];
> +	ch348->config_ep = usb_sndbulkpipe(ch348->udev,
> +					   config_port->bulk_out_endpointAddress);
> +
> +	ret = ch348_detect_version(serial);
> +	if (ret)
> +		goto err_free_ch348;
> +
> +	ret = ch348_submit_urbs(serial);
> +	if (ret)
> +		goto err_free_ch348;
> +
> +	return 0;
> +
> +err_free_ch348:
> +	kfree(ch348);
> +	return ret;
> +}
> +
> +static void ch348_release(struct usb_serial *serial)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +
> +	cancel_work_sync(&ch348->write_work);
> +	ch348_kill_urbs(serial);
> +
> +	kfree(ch348);
> +}
> +
> +static int ch348_calc_num_ports(struct usb_serial *serial,
> +				struct usb_serial_endpoints *epds)
> +{
> +	int i;
> +
> +	epds->num_bulk_out = CH348_MAXPORT;
> +
> +	for (i = serial->type->num_bulk_out; i < CH348_MAXPORT; ++i)
> +		epds->bulk_out[i] = epds->bulk_out[0];
> +
> +	return CH348_MAXPORT;
> +}
> +
> +static int ch348_suspend(struct usb_serial *serial, pm_message_t message)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +
> +	cancel_work_sync(&ch348->write_work);
> +
> +	return 0;
> +}
> +
> +static int ch348_resume(struct usb_serial *serial)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +	int ret;
> +
> +	ret = ch348_submit_urbs(serial);
> +	if (ret)
> +		return ret;
> +
> +	schedule_work(&ch348->write_work);
> +
> +	return 0;
> +}
> +
> +static const struct usb_device_id ch348_ids[] = {
> +	{ USB_DEVICE(0x1a86, 0x55d9), },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, ch348_ids);
> +
> +static struct usb_serial_driver ch348_device = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "ch348",
> +	},
> +	.id_table =		ch348_ids,
> +	.num_ports =		CH348_MAXPORT,
> +	.num_bulk_in =		2,
> +	.num_bulk_out =		2,
> +	.open =			ch348_open,
> +	.close =		ch348_close,
> +	.set_termios =		ch348_set_termios,
> +	.process_read_urb =	ch348_process_read_urb,
> +	.write =		ch348_write,
> +	.calc_num_ports =	ch348_calc_num_ports,
> +	.attach =		ch348_attach,
> +	.release =		ch348_release,
> +	.suspend =		ch348_suspend,
> +	.resume =		ch348_resume,
> +};
> +
> +static struct usb_serial_driver * const serial_drivers[] = {
> +	&ch348_device, NULL
> +};
> +
> +module_usb_serial_driver(serial_drivers, ch348_ids);
> +
> +MODULE_AUTHOR("Corentin Labbe <clabbe@baylibre.com>");
> +MODULE_DESCRIPTION("USB CH348 Octo port serial converter driver");
> +MODULE_LICENSE("GPL");

-- 
David Heidelberg

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Nicolas Frattaroli]

On Tue, 4 Feb 2025 13:58:40 +0000 Corentin Labbe wrote:
> The CH348 is an USB octo port serial adapter.
> The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> Since there is no public datasheet, unfortunately it remains some magic
> values
>
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>  drivers/usb/serial/Kconfig  |   9 +
>  drivers/usb/serial/Makefile |   1 +
>  drivers/usb/serial/ch348.c  | 736 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 746 insertions(+)
>  create mode 100644 drivers/usb/serial/ch348.c
>

Tested-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>

Quickly gave this another test by just connecting two ports of a CH348 device
together and making sure data arrives unchowdered at the other end when sending
at full speed with various baud rates. Seems to work about as well as I
expected my jumper wires to hold up at higher baudrates.

I'll likely use this to lessen the pile of active USB-to-serial devices on my
desk while working, so many thanks for keeping up the good work on the driver.
That way it'll also get some more in-depth real world testing in th e coming
weeks, but I doubt it needs more testing at this stage.

Regards,
Nicolas Frattaroli

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[David Heidelberg]

If another Tested-by is needed to get this in, I can build kernel w/ 
driver and give it shot too.

Thanks for your work & testing!

On 30/03/2025 03:24, Nicolas Frattaroli wrote:
> On Tue, 4 Feb 2025 13:58:40 +0000 Corentin Labbe wrote:
>> The CH348 is an USB octo port serial adapter.
>> The device multiplexes all 8 ports in the same pair of Bulk endpoints.
>> Since there is no public datasheet, unfortunately it remains some magic
>> values
>>
>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
>> ---
>>   drivers/usb/serial/Kconfig  |   9 +
>>   drivers/usb/serial/Makefile |   1 +
>>   drivers/usb/serial/ch348.c  | 736 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 746 insertions(+)
>>   create mode 100644 drivers/usb/serial/ch348.c
>>
> 
> Tested-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> 
> Quickly gave this another test by just connecting two ports of a CH348 device
> together and making sure data arrives unchowdered at the other end when sending
> at full speed with various baud rates. Seems to work about as well as I
> expected my jumper wires to hold up at higher baudrates.
> 
> I'll likely use this to lessen the pile of active USB-to-serial devices on my
> desk while working, so many thanks for keeping up the good work on the driver.
> That way it'll also get some more in-depth real world testing in th e coming
> weeks, but I doubt it needs more testing at this stage.
> 
> Regards,
> Nicolas Frattaroli

-- 
David Heidelberg

Re: [PATCH v8 0/2] usb: serial: add support for CH348

[Martin Blumenstingl]

Hi Johan, Hi Corentin,

Corentin, your last mail was HTML formatted instead of plain text, so
I'm not sure if it got through to everyone.

On Sat, May 3, 2025 at 3:52 PM Corentin Labbe <clabbe@baylibre.com> wrote:
>
> Hello
>
> This patch serie was sent 3 month ago and it still have no review.
> So this is a gentle ping.
>
> Having this upstream will help lot of people, and at first me since I use it heavily on all my kernelCI lab.
I agree, it would be awesome to have this patch applied.
Johan, if there's any actual blockers then please let us know so we
can address them.


Best regards,
Martin

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Johan Hovold]

On Tue, Feb 04, 2025 at 01:58:41PM +0000, Corentin Labbe wrote:
> The CH348 is an USB octo port serial adapter.
> The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> Since there is no public datasheet, unfortunately it remains some magic values

Please wrap the commit message at 72 columns or so.

> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * USB serial driver for USB to Octal UARTs chip ch348.
> + *
> + * Copyright (C) 2023 Corentin Labbe <clabbe@baylibre.com>
> + * With the help of Neil Armstrong <neil.armstrong@linaro.org>
> + * Copyright (C) 2024 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> + *
> + * Based on the ch9344 driver:
> + *   https://github.com/WCHSoftGroup/ch9344ser_linux/
> + *   Copyright (C) 2024 Nanjing Qinheng Microelectronics Co., Ltd.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/kfifo.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>

I don't think you use this one currently.

> +#include <linux/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/tty_flip.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +#include <linux/workqueue.h>
> +
> +#define CH348_CMD_TIMEOUT   2000
> +
> +#define CH348_CTO_D	0x01
> +#define CH348_CTO_R	0x02
> +
> +#define CH348_CTI_C	0x10
> +#define CH348_CTI_DSR	0x20
> +#define CH348_CTI_R	0x40
> +#define CH348_CTI_DCD	0x80
> +
> +#define CMD_W_R		0xC0

Lower case for consistency.

> +#define CMD_W_BR	0x80
> +
> +#define CMD_WB_E	0x90
> +#define CMD_RB_E	0xC0

Lower case for consistency.

> +
> +#define M_NOR		0x00
> +#define M_HF		0x03
> +
> +#define R_MOD		0x97
> +#define R_IO_D		0x98
> +#define R_IO_O		0x99
> +#define R_IO_I		0x9b
> +#define R_TM_O		0x9c
> +#define R_INIT		0xa1
> +
> +#define CMD_VER		0x96
> +
> +/* 0x10 is normally UART_MCR_LOOP but for CH348 it's UART_MCR_RTS */
> +#define UART_MCR_RTS_CH348	0x10

It's good that you looked at the standard defines to make some sense of
the magic constants you had earlier, but don't take it too far. If there
are too many exceptions and inconsistencies, it may be better to have
driver specific defines (more below).

> +/*
> + * The CH348 multiplexes rx & tx into a pair of Bulk USB endpoints for the 8
> + * serial ports, and another pair of Bulk USB endpoints to set port settings
> + * and receive port status events.
> + *
> + * The USB serial cores ties every Bulk endpoints pairs to each ports, In our
> + * case it will set port 0 with the rx/tx endpoints and port 1 with the
> + * setup/status endpoints.
> + *
> + * For bulk writes we skip all of USB serial core's helpers and implement it on
> + * our own since for serial TX we need to not only wait for the URB to complete
> + * but also for the UART_IIR_THRI signal.
> + *
> + * For bulk reads we use USB serial core's helpers, even for the status/int
> + * handling as it simplifies our code.
> + */
> +#define CH348_MAXPORT				8
> +#define CH348_PORTNUM_SERIAL_RX_TX		0
> +#define CH348_PORTNUM_STATUS_INT_CONFIG		1
> +
> +#define CH348_RX_PORT_MAX_LENGTH	30
> +
> +struct ch348_rxbuf {
> +	u8 port;
> +	u8 length;
> +	u8 data[CH348_RX_PORT_MAX_LENGTH];
> +} __packed;
> +
> +struct ch348_txbuf {
> +	u8 port;
> +	__le16 length;
> +	u8 data[];
> +} __packed;
> +
> +#define CH348_TX_HDRSIZE offsetof(struct ch348_txbuf, data)
> +
> +struct ch348_initbuf {
> +	u8 cmd;
> +	u8 reg;
> +	u8 port;
> +	__be32 baudrate;
> +	u8 format;
> +	u8 paritytype;
> +	u8 databits;
> +	u8 rate;
> +	u8 unknown;
> +} __packed;
> +
> +#define CH348_INITBUF_FORMAT_STOPBITS		0x2
> +#define CH348_INITBUF_FORMAT_NO_STOPBITS	0x0
> +
> +/*

For kernel doc, you want /** here.

> + * struct ch348_port - per-port information
> + * @uartmode:		UART port current mode
> + * @baudrate:		A cached copy of current baudrate for the RX logic
> + */
> +struct ch348_port {
> +	u8 uartmode;
> +	speed_t baudrate;
> +};
> +
> +/*

And here.

> + * struct ch348 - main container for all this driver information
> + * @udev:		pointer to the CH348 USB device
> + * @ports:		List of per-port information
> + * @serial:		pointer to the serial structure
> + * @write_work:		worker for processing the write queues
> + * @txbuf_completion:	indicates that the TX buffer has been fully written out
> + * @tx_ep:		endpoint number for serial data transmit/write operation
> + * @config_ep:		endpoint number for configure operations
> + * @small_package:	indicates package size: small (CH348Q) or large (CH348L)
> + */
> +struct ch348 {
> +	struct usb_device *udev;
> +	struct ch348_port ports[CH348_MAXPORT];
> +	struct usb_serial *serial;
> +
> +	struct work_struct write_work;
> +	struct completion txbuf_completion;
> +
> +	int tx_ep;
> +	int config_ep;
> +
> +	bool small_package;

I noticed that you don't use this one currently, but perhaps use a type
enum and call it type instead to make it more obvious what it is (can
be) used for.

> +};
> +
> +struct ch348_serial_config {
> +	u8 action;
> +	u8 reg;
> +	u8 control;
> +} __packed;
> +
> +struct ch348_status_entry {
> +	u8 portnum;
> +	u8 reg_iir;
> +	union {
> +		u8 lsr_signal;
> +		u8 modem_signal;
> +		u8 init_data[10];
> +	};
> +} __packed;
> +
> +#define CH348_STATUS_ENTRY_PORTNUM_MASK		0xf
> +
> +static void ch348_process_status_urb(struct usb_serial *serial, struct urb *urb)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +	struct ch348_status_entry *status_entry;
> +	struct usb_serial_port *port;
> +	unsigned int i, status_len;
> +	u8 portnum;
> +
> +	if (urb->actual_length < 3) {
> +		dev_warn_ratelimited(&ch348->udev->dev,
> +				     "Received too short status buffer with %u bytes\n",
> +				     urb->actual_length);

You should probably just demote to dev_dbg() as you do in the data path,
not much a user can do with this warning.

> +		return;
> +	}
> +
> +	for (i = 0; i < urb->actual_length;) {
> +		status_entry = urb->transfer_buffer + i;
> +		portnum = status_entry->portnum & CH348_STATUS_ENTRY_PORTNUM_MASK;
> +
> +		if (portnum >= CH348_MAXPORT) {
> +			dev_warn_ratelimited(&ch348->udev->dev,
> +					     "Invalid port %d in status entry\n",
> +					     portnum);

Same here.

> +			break;
> +		}
> +
> +		port = serial->port[portnum];
> +		status_len = 3;
> +
> +		if (!status_entry->reg_iir) {
> +			dev_dbg(&port->dev, "Ignoring status with zero reg_iir\n");
> +		} else if (status_entry->reg_iir == R_INIT) {
> +			status_len = 12;
> +		} else if ((status_entry->reg_iir & UART_IIR_ID) == UART_IIR_RLSI) {
> +			if (status_entry->lsr_signal & UART_LSR_OE)
> +				port->icount.overrun++;
> +			if (status_entry->lsr_signal & UART_LSR_PE)
> +				port->icount.parity++;
> +			if (status_entry->lsr_signal & UART_LSR_FE)
> +				port->icount.frame++;
> +			if (status_entry->lsr_signal & UART_LSR_BI)
> +				port->icount.brk++;
> +		} else if ((status_entry->reg_iir & UART_IIR_ID) == UART_IIR_THRI) {
> +			complete_all(&ch348->txbuf_completion);
> +		} else {
> +			dev_warn_ratelimited(&port->dev,
> +					     "Unsupported status with reg_iir 0x%02x\n",
> +					     status_entry->reg_iir);

And here.

> +		}
> +
> +		i += status_len;
> +	}
> +}
> +
> +static void ch348_process_serial_rx_urb(struct usb_serial *serial,
> +					struct urb *urb)
> +{
> +	unsigned int portnum, serial_rx_len, i;
> +	struct usb_serial_port *port;
> +	struct ch348_rxbuf *rxb;
> +
> +	if (urb->actual_length < 2) {
> +		dev_dbg(&serial->dev->dev, "Empty rx buffer\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < urb->actual_length; i += sizeof(*rxb)) {
> +		rxb = urb->transfer_buffer + i;
> +		portnum = rxb->port;
> +		if (portnum >= CH348_MAXPORT) {
> +			dev_dbg(&serial->dev->dev, "Invalid port %d\n", portnum);
> +			break;
> +		}
> +
> +		port = serial->port[portnum];
> +
> +		serial_rx_len = rxb->length;
> +		if (serial_rx_len > CH348_RX_PORT_MAX_LENGTH) {
> +			dev_dbg(&port->dev, "Invalid length %d for port %d\n",
> +				serial_rx_len, portnum);
> +			break;
> +		}
> +
> +		tty_insert_flip_string(&port->port, rxb->data, serial_rx_len);
> +		tty_flip_buffer_push(&port->port);
> +
> +		port->icount.rx += serial_rx_len;
> +	}
> +}
> +
> +static void ch348_process_read_urb(struct urb *urb)
> +{
> +	struct usb_serial_port *port = urb->context;
> +
> +	if (port->port_number == CH348_PORTNUM_SERIAL_RX_TX)
> +		ch348_process_serial_rx_urb(port->serial, urb);
> +	else if (port->port_number == CH348_PORTNUM_STATUS_INT_CONFIG)
> +		ch348_process_status_urb(port->serial, urb);
> +	else
> +		dev_warn_ratelimited(&port->serial->dev->dev,
> +				     "Ignoring read URB callback for unknown port/endpoint %u\n",
> +				     port->port_number);

This can't happen since you only submit read urbs for ports 0 and 1,
just drop.

> +}
> +
> +static int ch348_port_config(struct usb_serial_port *port, u8 action, u8 reg,
> +			     u8 control)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	struct ch348_serial_config *buffer;
> +	int ret;
> +
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	if (port->port_number < 4)
> +		reg += 0x10 * port->port_number;
> +	else
> +		reg += 0x10 * (port->port_number - 4) + 0x08;
> +
> +	buffer->action = action;
> +	buffer->reg = reg;
> +	buffer->control = control;
> +
> +	ret = usb_bulk_msg(ch348->udev, ch348->config_ep, buffer,
> +			   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
> +	if (ret) {
> +		dev_err(&ch348->udev->dev, "Failed to write port config: %d\n", ret);
> +		goto out;

No need for a goto here.

> +	}
> +
> +out:
> +	kfree(buffer);
> +
> +	return ret;
> +}
> +
> +static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
> +		       const unsigned char *buf, int count)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +
> +	if (!count)
> +		return 0;
> +
> +	count = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock);
> +
> +	schedule_work(&ch348->write_work);
> +
> +	return count;
> +}
> +
> +static int ch348_set_uartmode(struct usb_serial_port *port, u8 mode)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	unsigned int portnum = port->port_number;
> +	int ret;
> +
> +	if (ch348->ports[portnum].uartmode == M_NOR && mode == M_HF) {

AFAICS, the M_NOR and M_HF are only used to track whether hardware flow
control is enabled or not.

I guess HF stands for hardware flow, but what does M_NOR mean? No RTS?

Unless these names reflect some hardware flags, I think you should
switch to more descriptive names. And I guess you just need a boolean
flag.

> +		ret = ch348_port_config(port, CMD_W_BR, UART_MCR,
> +					UART_MCR_DTR | UART_MCR_RTS_CH348 |
> +					UART_MCR_TCRTLR);

This does not look a normal MCR register so shouldn't be using the
standard defines for it.

Perhaps you can make some sense of this by comparing with the vendor
dtr_rts() implementation too.

> +		if (ret)
> +			return ret;
> +		ch348->ports[portnum].uartmode = M_HF;
> +	}
> +
> +	if (ch348->ports[portnum].uartmode == M_HF && mode == M_NOR) {
> +		ret = ch348_port_config(port, CMD_W_BR, UART_MCR,
> +					UART_MCR_RTS_CH348 | UART_MCR_TCRTLR);
> +		if (ret)
> +			return ret;
> +		ch348->ports[portnum].uartmode = M_NOR;
> +	}
> +	return 0;
> +}
> +
> +static void ch348_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
> +			      const struct ktermios *termios_old)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	struct ktermios *termios = &tty->termios;
> +	int ret, portnum = port->port_number;
> +	struct ch348_initbuf *buffer;
> +	speed_t	baudrate;
> +
> +	if (termios_old && !tty_termios_hw_change(&tty->termios, termios_old))
> +		return;
> +
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer)
> +		goto out;
> +
> +	/*
> +	 * The datasheet states that only baud rates in range of 1200..6000000
> +	 * are supported. Tests with an oscilloscope confirm that even when
> +	 * configuring a baud rate slower than 1200 the output stays at around
> +	 * 1200 baud.
> +	 */
> +	baudrate = clamp(tty_get_baud_rate(tty), 1200, 6000000);
> +	tty_termios_encode_baud_rate(&tty->termios, baudrate, baudrate);
> +	ch348->ports[port->port_number].baudrate = baudrate;
> +
> +	if (termios->c_cflag & PARENB) {
> +		if  (termios->c_cflag & CMSPAR) {
> +			if (termios->c_cflag & PARODD)
> +				buffer->paritytype = 3;
> +			else
> +				buffer->paritytype = 4;
> +		} else if (termios->c_cflag & PARODD) {

Just use "else" here and add an inner conditional for PARODD for
symmetry and readability.

> +			buffer->paritytype = 1;
> +		} else {
> +			buffer->paritytype = 2;
> +		}
> +	} else {
> +		buffer->paritytype = 0;
> +	}

> +	buffer->cmd = CMD_WB_E | portnum;
> +	buffer->reg = R_INIT;
> +	buffer->port = portnum;
> +	buffer->baudrate = cpu_to_be32(baudrate);
> +
> +	if (termios->c_cflag & CSTOPB)
> +		buffer->format = CH348_INITBUF_FORMAT_STOPBITS;
> +	else
> +		buffer->format = CH348_INITBUF_FORMAT_NO_STOPBITS;

CSTOPB controls whether one or two stop bits is used so these defines
looks like they should be renamed.

> +
> +	buffer->rate = max_t(speed_t, 5, (10000 * 15 / baudrate) + 1);
> +
> +	ret = usb_bulk_msg(ch348->udev, ch348->config_ep, buffer,
> +			   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
> +	if (ret < 0) {
> +		dev_err(&ch348->udev->dev, "Failed to change line settings: err=%d\n",

I've already asked you to drop the redundant "err=" from all error
messages and use a consistent

	"failed to <x>: %d\n"

format *throughout* the driver.

> +			ret);
> +		goto out_free;
> +	}
> +
> +	ret = ch348_port_config(port, CMD_W_R, UART_IER, UART_IER_RDI |
> +				UART_IER_THRI | UART_IER_RLSI | UART_IER_MSI);
> +	if (ret < 0)
> +		goto out_free;
> +
> +	if (C_CRTSCTS(tty))
> +		ret = ch348_set_uartmode(port, M_HF);
> +	else
> +		ret = ch348_set_uartmode(port, M_NOR);
> +
> +out_free:
> +	kfree(buffer);
> +out:
> +	if (ret && termios_old)
> +		tty->termios = *termios_old;

You shouldn't restore all of termios in case it was just updating the
uart mode (CRTSCTS) that failed.

> +}
> +
> +static int ch348_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	int ret;
> +
> +	clear_bit(USB_SERIAL_THROTTLED, &port->flags);

You're not using the generic throttle implementation so no need to clear
this flag.

> +
> +	if (tty)
> +		ch348_set_termios(tty, port, NULL);
> +
> +	ret = ch348_port_config(port, CMD_W_R, UART_FCR,
> +				UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> +				UART_FCR_CLEAR_XMIT | UART_FCR_T_TRIG_00 |
> +				UART_FCR_R_TRIG_10);
> +	if (ret) {
> +		dev_err(&port->serial->dev->dev,
> +			"Failed to configure UART_FCR, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = ch348_port_config(port, CMD_W_BR, UART_MCR, UART_MCR_OUT2);

Probably not MCR_OUT2 here either.

> +	if (ret) {
> +		dev_err(&port->serial->dev->dev,
> +			"Failed to configure UART_MCR, err=%d\n", ret);
> +		return ret;
> +	}

The read urbs should be submitted at first open and stopped at last
close to avoid wasting resources when no one is using the device.

I know we have a few drivers that do not do this currently, but it
shouldn't be that hard to get this right from the start.

> +
> +	return 0;
> +}
> +
> +static void ch348_close(struct usb_serial_port *port)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	kfifo_reset_out(&port->write_fifo);
> +	spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static void ch348_write_work(struct work_struct *work)
> +{
> +	struct ch348 *ch348 = container_of(work, struct ch348, write_work);
> +	struct usb_serial_port *port, *hw_tx_port;
> +	unsigned int i, max_bytes;
> +	struct ch348_txbuf *rxt;
> +	unsigned long flags;
> +	int ret, count;
> +
> +	reinit_completion(&ch348->txbuf_completion);
> +
> +	hw_tx_port = ch348->serial->port[CH348_PORTNUM_SERIAL_RX_TX];
> +	rxt = hw_tx_port->write_urbs[0]->transfer_buffer;
> +
> +	for (i = 0; i < CH348_MAXPORT; i++) {
> +		port = ch348->serial->port[i];
> +
> +		if (ch348->ports[i].baudrate < 9600)

Use brackets for readability due to the comments adding multiple lines
to each branch.

> +			/*
> +			 * Writing larger buffers can take longer than the
> +			 * hardware allows before discarding the write buffer.
> +			 * Limit the transfer size in such cases.
> +			 * These values have been found by empirical testing.
> +			 */
> +			max_bytes = 128;

This is a potential buffer overflow if a (malicious) device has
endpoints smaller than this (use min()).

> +		else
> +			/*
> +			 * Only ingest as many bytes as we can transfer with
> +			 * one URB at a time keeping the TX header in mind.
> +			 */
> +			max_bytes = hw_tx_port->bulk_out_size - CH348_TX_HDRSIZE;
> +
> +		count = kfifo_out_locked(&port->write_fifo, rxt->data,
> +					 max_bytes, &port->lock);
> +		if (count)
> +			break;
> +	}

With this implementation writing data continuously to one port will
starve the others. 

The vendor implementation appears to write to more than one port in
parallel and track THRE per port which would avoid the starvation issue
and should also be much more efficient.

Just track THRE per port and only submit the write urb when it the
transmitter is empty or when it becomes empty.

> +
> +	if (!count)
> +		return;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	port->tx_bytes += count;
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	rxt->port = port->port_number;
> +	rxt->length = cpu_to_le16(count);
> +
> +	usb_serial_debug_data(&port->dev, __func__, count + CH348_TX_HDRSIZE,
> +			      (const unsigned char *)rxt);
> +
> +	ret = usb_bulk_msg(ch348->udev, ch348->tx_ep, rxt,
> +			   count + CH348_TX_HDRSIZE, NULL, CH348_CMD_TIMEOUT);
> +	if (ret) {
> +		dev_err_console(port,
> +				"Failed to bulk write TX buffer, err=%d\n",
> +				ret);
> +		goto write_done;
> +	}
> +
> +	if (!wait_for_completion_timeout(&ch348->txbuf_completion,
> +					 msecs_to_jiffies(CH348_CMD_TIMEOUT)))
> +		dev_err_console(port,
> +				"Failed to wait for TX buffer to be fully written out\n");

This would also avoid blocking for extended periods of time on the
system work queue.

> +
> +write_done:
> +	spin_lock_irqsave(&port->lock, flags);
> +	port->tx_bytes -= count;
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	port->icount.tx += count;
> +
> +	schedule_work(&ch348->write_work);
> +	usb_serial_port_softint(port);
> +}

> +static const struct usb_device_id ch348_ids[] = {
> +	{ USB_DEVICE(0x1a86, 0x55d9), },

No need for comma after USB_DEVICE().

> +	{ /* sentinel */ }
> +};

> +static struct usb_serial_driver ch348_device = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "ch348",
> +	},
> +	.id_table =		ch348_ids,
> +	.num_ports =		CH348_MAXPORT,
> +	.num_bulk_in =		2,
> +	.num_bulk_out =		2,
> +	.open =			ch348_open,
> +	.close =		ch348_close,
> +	.set_termios =		ch348_set_termios,
> +	.process_read_urb =	ch348_process_read_urb,
> +	.write =		ch348_write,
> +	.calc_num_ports =	ch348_calc_num_ports,
> +	.attach =		ch348_attach,
> +	.release =		ch348_release,
> +	.suspend =		ch348_suspend,
> +	.resume =		ch348_resume,
> +};

You should implement dtr_rts() as well.

Johan

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Martin Blumenstingl]

Hi Johan,

I'm excluding comments that are clear to me in this reply.

On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@kernel.org> wrote:
[...]
> > +     if (ret) {
> > +             dev_err(&port->serial->dev->dev,
> > +                     "Failed to configure UART_MCR, err=%d\n", ret);
> > +             return ret;
> > +     }
>
> The read urbs should be submitted at first open and stopped at last
> close to avoid wasting resources when no one is using the device.
>
> I know we have a few drivers that do not do this currently, but it
> shouldn't be that hard to get this right from the start.
If you're aware of an easy approach or you can recommend an existing
driver that implements the desired behavior then please let me know.

The speciality about ch348 is that all ports share the RX/TX URBs.
My current idea is to implement this using a ref count (for the number
of open ports) and mutex for locking.

[...]
> > +                     /*
> > +                      * Writing larger buffers can take longer than the
> > +                      * hardware allows before discarding the write buffer.
> > +                      * Limit the transfer size in such cases.
> > +                      * These values have been found by empirical testing.
> > +                      */
> > +                     max_bytes = 128;
>
> This is a potential buffer overflow if a (malicious) device has
> endpoints smaller than this (use min()).
For endpoints smaller than CH348_TX_HDRSIZE we'll also be in trouble.
Validating against CH348_TX_HDRSIZE size here doesn't make much sense
to me (as we'd never be able to progress). I think I should validate
it in ch348_attach() instead - what do you think?

> > +             else
> > +                     /*
> > +                      * Only ingest as many bytes as we can transfer with
> > +                      * one URB at a time keeping the TX header in mind.
> > +                      */
> > +                     max_bytes = hw_tx_port->bulk_out_size - CH348_TX_HDRSIZE;
> > +
> > +             count = kfifo_out_locked(&port->write_fifo, rxt->data,
> > +                                      max_bytes, &port->lock);
> > +             if (count)
> > +                     break;
> > +     }
>
> With this implementation writing data continuously to one port will
> starve the others.
>
> The vendor implementation appears to write to more than one port in
> parallel and track THRE per port which would avoid the starvation issue
> and should also be much more efficient.
>
> Just track THRE per port and only submit the write urb when it the
> transmitter is empty or when it becomes empty.
I'm trying as you suggest:
- submit the URB synchronously for port N
- submit the URB synchronously for port N + 1
- ...

This seems to work (using usb_bulk_msg). What doesn't work is
submitting URBs in parallel (this is what the vendor driver prevents
as well).

[...]
> > +     if (!wait_for_completion_timeout(&ch348->txbuf_completion,
> > +                                      msecs_to_jiffies(CH348_CMD_TIMEOUT)))
> > +             dev_err_console(port,
> > +                             "Failed to wait for TX buffer to be fully written out\n");
>
> This would also avoid blocking for extended periods of time on the
> system work queue.
I'm moving this to a timer instead.

If you have any comments on the (new) TX logic then please let me know.

[...]
> You should implement dtr_rts() as well.
This will be the first time we need the "package type" information as
CH348Q only supports CTS/RTS on the first four ports, for the last
four the signals aren't routed outside the package.
I need to see if I have other hardware with CTS/RTS pins to test this.
If not: can we omit hardware flow control in the first upstream
version?


Best regards,
Martin

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Greg KH]

On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:
> Hi Johan,
> 
> I'm excluding comments that are clear to me in this reply.
> 
> On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@kernel.org> wrote:
> [...]
> > > +     if (ret) {
> > > +             dev_err(&port->serial->dev->dev,
> > > +                     "Failed to configure UART_MCR, err=%d\n", ret);
> > > +             return ret;
> > > +     }
> >
> > The read urbs should be submitted at first open and stopped at last
> > close to avoid wasting resources when no one is using the device.
> >
> > I know we have a few drivers that do not do this currently, but it
> > shouldn't be that hard to get this right from the start.
> If you're aware of an easy approach or you can recommend an existing
> driver that implements the desired behavior then please let me know.
> 
> The speciality about ch348 is that all ports share the RX/TX URBs.
> My current idea is to implement this using a ref count (for the number
> of open ports) and mutex for locking.

How do you know if a port is "open" or not and keep track of them all?
Trying to manage that is a pain and a refcount shouldn't need locking if
you use the proper refcount_t type in a sane way.

Try to keep it simple please.

> > With this implementation writing data continuously to one port will
> > starve the others.
> >
> > The vendor implementation appears to write to more than one port in
> > parallel and track THRE per port which would avoid the starvation issue
> > and should also be much more efficient.
> >
> > Just track THRE per port and only submit the write urb when it the
> > transmitter is empty or when it becomes empty.
> I'm trying as you suggest:
> - submit the URB synchronously for port N
> - submit the URB synchronously for port N + 1
> - ...
> 
> This seems to work (using usb_bulk_msg). What doesn't work is
> submitting URBs in parallel (this is what the vendor driver prevents
> as well).

Why would submitting urbs in parallel not work?  Is the device somehow
broken and can't accept multiple requests at the same time?

thanks,

greg k-h

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Martin Blumenstingl]

On Wed, Jul 16, 2025 at 9:44 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:
> > Hi Johan,
> >
> > I'm excluding comments that are clear to me in this reply.
> >
> > On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@kernel.org> wrote:
> > [...]
> > > > +     if (ret) {
> > > > +             dev_err(&port->serial->dev->dev,
> > > > +                     "Failed to configure UART_MCR, err=%d\n", ret);
> > > > +             return ret;
> > > > +     }
> > >
> > > The read urbs should be submitted at first open and stopped at last
> > > close to avoid wasting resources when no one is using the device.
> > >
> > > I know we have a few drivers that do not do this currently, but it
> > > shouldn't be that hard to get this right from the start.
> > If you're aware of an easy approach or you can recommend an existing
> > driver that implements the desired behavior then please let me know.
> >
> > The speciality about ch348 is that all ports share the RX/TX URBs.
> > My current idea is to implement this using a ref count (for the number
> > of open ports) and mutex for locking.
>
> How do you know if a port is "open" or not and keep track of them all?
> Trying to manage that is a pain and a refcount shouldn't need locking if
> you use the proper refcount_t type in a sane way.
>
> Try to keep it simple please.
I'm currently refcounting from usb_serial_driver.{open,close}
The additional challenge is that I need to open two URBs at the right
time to "avoid wasting resources". At least in my head I can't make it
work without an additional lock.

The following is a simplified/pseudo-code version of what I have at
the moment (which is called from my ch348_open):
static int ch348_submit_urbs(struct usb_serial *serial)
{
  int ret = 0;

  mutex_lock(&ch348->manage_urbs_lock);

  if (refcount_read(&ch348->num_open_ports))
    goto out_increment_refcount;

  ret = usb_serial_generic_open(NULL, serial_rx);
  if (ret)
    goto out_unlock;

  ret = usb_serial_generic_open(NULL, status);
  if (ret) {
    usb_serial_generic_close(serial_rx); /* undo the first
usb_serial_generic_open */
    goto out_unlock;
  }

out_increment_refcount:
  refcount_inc(&ch348->num_open_ports);

out_unlock:
  mutex_unlock(&ch348->manage_urbs_lock);

  return ret;
}

My understanding is that usb-serial core does not provide any locking
around .open/.close.

> > > With this implementation writing data continuously to one port will
> > > starve the others.
> > >
> > > The vendor implementation appears to write to more than one port in
> > > parallel and track THRE per port which would avoid the starvation issue
> > > and should also be much more efficient.
> > >
> > > Just track THRE per port and only submit the write urb when it the
> > > transmitter is empty or when it becomes empty.
> > I'm trying as you suggest:
> > - submit the URB synchronously for port N
> > - submit the URB synchronously for port N + 1
> > - ...
> >
> > This seems to work (using usb_bulk_msg). What doesn't work is
> > submitting URBs in parallel (this is what the vendor driver prevents
> > as well).
>
> Why would submitting urbs in parallel not work?  Is the device somehow
> broken and can't accept multiple requests at the same time?
I don't know the reason behind this.
These are requests to the same bulk out endpoint. When submitting
multiple serial TX requests at the same time then some of the data is
lost / corrupted.

The vendor driver basically does this in their write function (very
much simplified, see [0] for their original code):
  spin_lock_irqsave(&ch9344->write_lock, flags);
  usb_submit_urb(wb->urb, GFP_ATOMIC); /* part of ch9344_start_wb */
  spin_unlock_irqrestore(&ch9344->write_lock, flags);

If you have any suggestions: please tell me - I'm happy to try them out!


Best regards,
Martin


[0] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/e0a38c4f4f9d4c1f5e2e3a352b7b1010b33aa322/driver/ch9344.c#L1330-L1404

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Greg KH]

On Wed, Jul 16, 2025 at 10:28:22AM +0200, Martin Blumenstingl wrote:
> On Wed, Jul 16, 2025 at 9:44 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:
> > > Hi Johan,
> > >
> > > I'm excluding comments that are clear to me in this reply.
> > >
> > > On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@kernel.org> wrote:
> > > [...]
> > > > > +     if (ret) {
> > > > > +             dev_err(&port->serial->dev->dev,
> > > > > +                     "Failed to configure UART_MCR, err=%d\n", ret);
> > > > > +             return ret;
> > > > > +     }
> > > >
> > > > The read urbs should be submitted at first open and stopped at last
> > > > close to avoid wasting resources when no one is using the device.
> > > >
> > > > I know we have a few drivers that do not do this currently, but it
> > > > shouldn't be that hard to get this right from the start.
> > > If you're aware of an easy approach or you can recommend an existing
> > > driver that implements the desired behavior then please let me know.
> > >
> > > The speciality about ch348 is that all ports share the RX/TX URBs.
> > > My current idea is to implement this using a ref count (for the number
> > > of open ports) and mutex for locking.
> >
> > How do you know if a port is "open" or not and keep track of them all?
> > Trying to manage that is a pain and a refcount shouldn't need locking if
> > you use the proper refcount_t type in a sane way.
> >
> > Try to keep it simple please.
> I'm currently refcounting from usb_serial_driver.{open,close}
> The additional challenge is that I need to open two URBs at the right
> time to "avoid wasting resources". At least in my head I can't make it
> work without an additional lock.

Urbs really aren't all that large of a "resource", so don't worry about
that.  Get it working properly first before attempting to care about
small buffers like this :)

> The following is a simplified/pseudo-code version of what I have at
> the moment (which is called from my ch348_open):
> static int ch348_submit_urbs(struct usb_serial *serial)
> {
>   int ret = 0;
> 
>   mutex_lock(&ch348->manage_urbs_lock);
> 
>   if (refcount_read(&ch348->num_open_ports))
>     goto out_increment_refcount;
> 
>   ret = usb_serial_generic_open(NULL, serial_rx);
>   if (ret)
>     goto out_unlock;
> 
>   ret = usb_serial_generic_open(NULL, status);
>   if (ret) {
>     usb_serial_generic_close(serial_rx); /* undo the first
> usb_serial_generic_open */
>     goto out_unlock;
>   }

That's odd, why use NULL for a tty device?  Ah, we don't even use it for
anything anymore, maybe that should be fixed...

Anyway, just submit the urbs, why use usb_serial_generic_* at all for
the status urb?  That's not normal.

And are you trying to only have one set of urbs out for any port being
opened (i.e. you only have one control, one read, and one write urb for
the whole device, and the port info are multiplexed over these urbs?  Or
do you have one endpoint per port?)

If you are sharing endpoints, try looking at one of the other usb-serial
drivers that do this today, like io_edgeport.c, that has had shared
endpoints for 25 years, it's not a new thing :)

> out_increment_refcount:
>   refcount_inc(&ch348->num_open_ports);
> 
> out_unlock:
>   mutex_unlock(&ch348->manage_urbs_lock);
> 
>   return ret;
> }
> 
> My understanding is that usb-serial core does not provide any locking
> around .open/.close.

Nor should it, these are independent per-port.

> > > > With this implementation writing data continuously to one port will
> > > > starve the others.
> > > >
> > > > The vendor implementation appears to write to more than one port in
> > > > parallel and track THRE per port which would avoid the starvation issue
> > > > and should also be much more efficient.
> > > >
> > > > Just track THRE per port and only submit the write urb when it the
> > > > transmitter is empty or when it becomes empty.
> > > I'm trying as you suggest:
> > > - submit the URB synchronously for port N
> > > - submit the URB synchronously for port N + 1
> > > - ...
> > >
> > > This seems to work (using usb_bulk_msg). What doesn't work is
> > > submitting URBs in parallel (this is what the vendor driver prevents
> > > as well).
> >
> > Why would submitting urbs in parallel not work?  Is the device somehow
> > broken and can't accept multiple requests at the same time?
> I don't know the reason behind this.
> These are requests to the same bulk out endpoint. When submitting
> multiple serial TX requests at the same time then some of the data is
> lost / corrupted.
> 
> The vendor driver basically does this in their write function (very
> much simplified, see [0] for their original code):
>   spin_lock_irqsave(&ch9344->write_lock, flags);
>   usb_submit_urb(wb->urb, GFP_ATOMIC); /* part of ch9344_start_wb */
>   spin_unlock_irqrestore(&ch9344->write_lock, flags);

that's crazy, why the timeout logic in there?  This is a write function,
submit the data to the device and get out of the way, that's all that
should be needed here.

> If you have any suggestions: please tell me - I'm happy to try them out!

Try keeping it simple first, the vendor driver looks like it was written
by someone who was paid per line of code :)

good luck!

greg k-h

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Martin Blumenstingl]

On Wed, Jul 16, 2025 at 10:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jul 16, 2025 at 10:28:22AM +0200, Martin Blumenstingl wrote:
> > On Wed, Jul 16, 2025 at 9:44 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:
> > > > Hi Johan,
> > > >
> > > > I'm excluding comments that are clear to me in this reply.
> > > >
> > > > On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@kernel.org> wrote:
> > > > [...]
> > > > > > +     if (ret) {
> > > > > > +             dev_err(&port->serial->dev->dev,
> > > > > > +                     "Failed to configure UART_MCR, err=%d\n", ret);
> > > > > > +             return ret;
> > > > > > +     }
> > > > >
> > > > > The read urbs should be submitted at first open and stopped at last
> > > > > close to avoid wasting resources when no one is using the device.
> > > > >
> > > > > I know we have a few drivers that do not do this currently, but it
> > > > > shouldn't be that hard to get this right from the start.
> > > > If you're aware of an easy approach or you can recommend an existing
> > > > driver that implements the desired behavior then please let me know.
> > > >
> > > > The speciality about ch348 is that all ports share the RX/TX URBs.
> > > > My current idea is to implement this using a ref count (for the number
> > > > of open ports) and mutex for locking.
> > >
> > > How do you know if a port is "open" or not and keep track of them all?
> > > Trying to manage that is a pain and a refcount shouldn't need locking if
> > > you use the proper refcount_t type in a sane way.
> > >
> > > Try to keep it simple please.
> > I'm currently refcounting from usb_serial_driver.{open,close}
> > The additional challenge is that I need to open two URBs at the right
> > time to "avoid wasting resources". At least in my head I can't make it
> > work without an additional lock.
>
> Urbs really aren't all that large of a "resource", so don't worry about
> that.  Get it working properly first before attempting to care about
> small buffers like this :)
I understood that this is a requirement from Johan, he wrote (on May
12th in this thread):
> The read urbs should be submitted at first open and stopped at last
> close to avoid wasting resources when no one is using the device.
>
> I know we have a few drivers that do not do this currently, but it
> shouldn't be that hard to get this right from the start.

The original approach was to submit these URBs in .attach (e.g. during
probe time) and kill them in .detach

> > The following is a simplified/pseudo-code version of what I have at
> > the moment (which is called from my ch348_open):
> > static int ch348_submit_urbs(struct usb_serial *serial)
> > {
> >   int ret = 0;
> >
> >   mutex_lock(&ch348->manage_urbs_lock);
> >
> >   if (refcount_read(&ch348->num_open_ports))
> >     goto out_increment_refcount;
> >
> >   ret = usb_serial_generic_open(NULL, serial_rx);
> >   if (ret)
> >     goto out_unlock;
> >
> >   ret = usb_serial_generic_open(NULL, status);
> >   if (ret) {
> >     usb_serial_generic_close(serial_rx); /* undo the first
> > usb_serial_generic_open */
> >     goto out_unlock;
> >   }
>
> That's odd, why use NULL for a tty device?  Ah, we don't even use it for
> anything anymore, maybe that should be fixed...
usb_serial_generic_open() doesn't use the tty internally - which is
why passing NULL doesn't matter now (but who knows what's going to
happen in future, so better move away from it).

> Anyway, just submit the urbs, why use usb_serial_generic_* at all for
> the status urb?  That's not normal.
I can either submit the urbs myself or use usb_serial_generic_submit_read_urbs()

> And are you trying to only have one set of urbs out for any port being
> opened (i.e. you only have one control, one read, and one write urb for
> the whole device, and the port info are multiplexed over these urbs?  Or
> do you have one endpoint per port?)
CH348 provides up to 8 serial ports using these four endpoints, so
multiplexing is going on:
- one bulk out for TX (see struct ch348_txbuf)
- one bulk in for RX (see struct ch348_rxbuf)
- one bulk out for CONFIG handling (see struct ch348_config_buf)
- one bulk in for STATUS handling (see struct ch348_status_entry)

> If you are sharing endpoints, try looking at one of the other usb-serial
> drivers that do this today, like io_edgeport.c, that has had shared
> endpoints for 25 years, it's not a new thing :)
My understanding is that io_edgeport is submits the URBs that are
shared across ports outside of .open/.close.
So this will be a question for Johan: am I still good with the
original approach - or can you convince Greg that a different approach
is better?

[...]
> > > > > With this implementation writing data continuously to one port will
> > > > > starve the others.
> > > > >
> > > > > The vendor implementation appears to write to more than one port in
> > > > > parallel and track THRE per port which would avoid the starvation issue
> > > > > and should also be much more efficient.
> > > > >
> > > > > Just track THRE per port and only submit the write urb when it the
> > > > > transmitter is empty or when it becomes empty.
> > > > I'm trying as you suggest:
> > > > - submit the URB synchronously for port N
> > > > - submit the URB synchronously for port N + 1
> > > > - ...
> > > >
> > > > This seems to work (using usb_bulk_msg). What doesn't work is
> > > > submitting URBs in parallel (this is what the vendor driver prevents
> > > > as well).
> > >
> > > Why would submitting urbs in parallel not work?  Is the device somehow
> > > broken and can't accept multiple requests at the same time?
> > I don't know the reason behind this.
> > These are requests to the same bulk out endpoint. When submitting
> > multiple serial TX requests at the same time then some of the data is
> > lost / corrupted.
> >
> > The vendor driver basically does this in their write function (very
> > much simplified, see [0] for their original code):
> >   spin_lock_irqsave(&ch9344->write_lock, flags);
> >   usb_submit_urb(wb->urb, GFP_ATOMIC); /* part of ch9344_start_wb */
> >   spin_unlock_irqrestore(&ch9344->write_lock, flags);
>
> that's crazy, why the timeout logic in there?  This is a write function,
> submit the data to the device and get out of the way, that's all that
> should be needed here.
From what I've seen during my tests:
- we fire the URB with TX data
- the device receives it and puts the data into a per-port TX buffer
- it indicates that it's done processing the URB
- the TX buffer is then written out (the host can move on do something else)
- the device signals to the host (via the STATUS endpoint) that it has
written out all data from the TX buffer

With that timeout logic my understanding is that they're trying to
catch cases where the last step (STATUS signal) did not work (for
whatever reason) so that the host can continue sending more data.

> > If you have any suggestions: please tell me - I'm happy to try them out!
>
> Try keeping it simple first, the vendor driver looks like it was written
> by someone who was paid per line of code :)
The original approach when upstreaming the ch348 driver was just to
submit TX URBs (without any custom code, just letting usb-serial core
handle it).
Corentin and Nicolas even wrote test programs to help reproduce issues
we've seen with the initial driver versions.
In other words: I don't know how to simplify our (to be upstreamed)
version without breaking something :-(

Also I'm not paid for this driver (at all - that also includes payment
per line of code), so there's no incentive there ;-).


Best regards,
Martin

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Greg KH]

On Wed, Jul 16, 2025 at 11:31:49AM +0200, Martin Blumenstingl wrote:
> On Wed, Jul 16, 2025 at 10:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jul 16, 2025 at 10:28:22AM +0200, Martin Blumenstingl wrote:
> > > On Wed, Jul 16, 2025 at 9:44 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:
> > > > > Hi Johan,
> > > > >
> > > > > I'm excluding comments that are clear to me in this reply.
> > > > >
> > > > > On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@kernel.org> wrote:
> > > > > [...]
> > > > > > > +     if (ret) {
> > > > > > > +             dev_err(&port->serial->dev->dev,
> > > > > > > +                     "Failed to configure UART_MCR, err=%d\n", ret);
> > > > > > > +             return ret;
> > > > > > > +     }
> > > > > >
> > > > > > The read urbs should be submitted at first open and stopped at last
> > > > > > close to avoid wasting resources when no one is using the device.
> > > > > >
> > > > > > I know we have a few drivers that do not do this currently, but it
> > > > > > shouldn't be that hard to get this right from the start.
> > > > > If you're aware of an easy approach or you can recommend an existing
> > > > > driver that implements the desired behavior then please let me know.
> > > > >
> > > > > The speciality about ch348 is that all ports share the RX/TX URBs.
> > > > > My current idea is to implement this using a ref count (for the number
> > > > > of open ports) and mutex for locking.
> > > >
> > > > How do you know if a port is "open" or not and keep track of them all?
> > > > Trying to manage that is a pain and a refcount shouldn't need locking if
> > > > you use the proper refcount_t type in a sane way.
> > > >
> > > > Try to keep it simple please.
> > > I'm currently refcounting from usb_serial_driver.{open,close}
> > > The additional challenge is that I need to open two URBs at the right
> > > time to "avoid wasting resources". At least in my head I can't make it
> > > work without an additional lock.
> >
> > Urbs really aren't all that large of a "resource", so don't worry about
> > that.  Get it working properly first before attempting to care about
> > small buffers like this :)
> I understood that this is a requirement from Johan, he wrote (on May
> 12th in this thread):
> > The read urbs should be submitted at first open and stopped at last
> > close to avoid wasting resources when no one is using the device.
> >
> > I know we have a few drivers that do not do this currently, but it
> > shouldn't be that hard to get this right from the start.
> 
> The original approach was to submit these URBs in .attach (e.g. during
> probe time) and kill them in .detach

Ok, that's fine, but as you are multiplexing stuff, so reference counts
are going to get complex.  I'll defer to Johan, but this seems messy.

> > If you are sharing endpoints, try looking at one of the other usb-serial
> > drivers that do this today, like io_edgeport.c, that has had shared
> > endpoints for 25 years, it's not a new thing :)
> My understanding is that io_edgeport is submits the URBs that are
> shared across ports outside of .open/.close.

Yes it does.

sorry about that, I misunderstood Johan's review comments here.  I'll
defer to him.


> So this will be a question for Johan: am I still good with the
> original approach - or can you convince Greg that a different approach
> is better?
> 
> [...]
> > > > > > With this implementation writing data continuously to one port will
> > > > > > starve the others.
> > > > > >
> > > > > > The vendor implementation appears to write to more than one port in
> > > > > > parallel and track THRE per port which would avoid the starvation issue
> > > > > > and should also be much more efficient.
> > > > > >
> > > > > > Just track THRE per port and only submit the write urb when it the
> > > > > > transmitter is empty or when it becomes empty.
> > > > > I'm trying as you suggest:
> > > > > - submit the URB synchronously for port N
> > > > > - submit the URB synchronously for port N + 1
> > > > > - ...
> > > > >
> > > > > This seems to work (using usb_bulk_msg). What doesn't work is
> > > > > submitting URBs in parallel (this is what the vendor driver prevents
> > > > > as well).
> > > >
> > > > Why would submitting urbs in parallel not work?  Is the device somehow
> > > > broken and can't accept multiple requests at the same time?
> > > I don't know the reason behind this.
> > > These are requests to the same bulk out endpoint. When submitting
> > > multiple serial TX requests at the same time then some of the data is
> > > lost / corrupted.
> > >
> > > The vendor driver basically does this in their write function (very
> > > much simplified, see [0] for their original code):
> > >   spin_lock_irqsave(&ch9344->write_lock, flags);
> > >   usb_submit_urb(wb->urb, GFP_ATOMIC); /* part of ch9344_start_wb */
> > >   spin_unlock_irqrestore(&ch9344->write_lock, flags);
> >
> > that's crazy, why the timeout logic in there?  This is a write function,
> > submit the data to the device and get out of the way, that's all that
> > should be needed here.
> >From what I've seen during my tests:
> - we fire the URB with TX data
> - the device receives it and puts the data into a per-port TX buffer
> - it indicates that it's done processing the URB
> - the TX buffer is then written out (the host can move on do something else)
> - the device signals to the host (via the STATUS endpoint) that it has
> written out all data from the TX buffer
> 
> With that timeout logic my understanding is that they're trying to
> catch cases where the last step (STATUS signal) did not work (for
> whatever reason) so that the host can continue sending more data.

Why can't the host just keep sending data?  Only "stall" if you don't
have an active urb to send?  Or just keep creating new urbs for every
send transaction and then destroying them when finished?  That way the
data gets queued up in the kernel (have a max able to be allocated so
you don't create a DoS accidentally), and you should be ok.  I think
some of the other drivers do this, or used to in the past.

> > > If you have any suggestions: please tell me - I'm happy to try them out!
> >
> > Try keeping it simple first, the vendor driver looks like it was written
> > by someone who was paid per line of code :)
> The original approach when upstreaming the ch348 driver was just to
> submit TX URBs (without any custom code, just letting usb-serial core
> handle it).

Ah, and what happened with that version?  Did it not work?

thanks,

greg k-h

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Martin Blumenstingl]

[...]
> > >From what I've seen during my tests:
> > - we fire the URB with TX data
> > - the device receives it and puts the data into a per-port TX buffer
> > - it indicates that it's done processing the URB
> > - the TX buffer is then written out (the host can move on do something else)
> > - the device signals to the host (via the STATUS endpoint) that it has
> > written out all data from the TX buffer
> >
> > With that timeout logic my understanding is that they're trying to
> > catch cases where the last step (STATUS signal) did not work (for
> > whatever reason) so that the host can continue sending more data.
>
> Why can't the host just keep sending data?  Only "stall" if you don't
> have an active urb to send?  Or just keep creating new urbs for every
> send transaction and then destroying them when finished?  That way the
> data gets queued up in the kernel (have a max able to be allocated so
> you don't create a DoS accidentally), and you should be ok.  I think
> some of the other drivers do this, or used to in the past.
The usb-serial framework queues up data in a kfifo (with max size = PAGE_SIZE).
After step 3 (CH348 indicates to the host that it has ingested the
URB) we can continue sending data for another port - that's what I'm
working on for v9.

> > > > If you have any suggestions: please tell me - I'm happy to try them out!
> > >
> > > Try keeping it simple first, the vendor driver looks like it was written
> > > by someone who was paid per line of code :)
> > The original approach when upstreaming the ch348 driver was just to
> > submit TX URBs (without any custom code, just letting usb-serial core
> > handle it).
>
> Ah, and what happened with that version?  Did it not work?
TX data was truncated/corrupted. There's a recent-ish commit in the
vendor driver [0] which suggests that parallel (they're calling it
"independent upload on multiple serial ports") is possible starting
with chip revision 0x8a but I cannot test this (since my test board
has a CH348Q revision 0x86).


Best regards,
Martin


[0] https://github.com/WCHSoftGroup/ch9344ser_linux/commit/875d57b8c6a7dd3f4359eb9adfe3779e2bbb0ac1

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Johan Hovold]

On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:

> On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@kernel.org> wrote:

> > The read urbs should be submitted at first open and stopped at last
> > close to avoid wasting resources when no one is using the device.
> >
> > I know we have a few drivers that do not do this currently, but it
> > shouldn't be that hard to get this right from the start.

> If you're aware of an easy approach or you can recommend an existing
> driver that implements the desired behavior then please let me know.
> 
> The speciality about ch348 is that all ports share the RX/TX URBs.
> My current idea is to implement this using a ref count (for the number
> of open ports) and mutex for locking.

Just use a mutex and integer (not refcount) to count the number of open
ports. Submit the urbs on first open and stop them on last close.

Not doing so, and instead submitting at attach(), means that the host
controller will be wasting power by polling the endpoints continuously
as long as the device is plugged in.

> [...]
> > > +                     /*
> > > +                      * Writing larger buffers can take longer than the
> > > +                      * hardware allows before discarding the write buffer.
> > > +                      * Limit the transfer size in such cases.
> > > +                      * These values have been found by empirical testing.
> > > +                      */
> > > +                     max_bytes = 128;
> >
> > This is a potential buffer overflow if a (malicious) device has
> > endpoints smaller than this (use min()).

> For endpoints smaller than CH348_TX_HDRSIZE we'll also be in trouble.
> Validating against CH348_TX_HDRSIZE size here doesn't make much sense
> to me (as we'd never be able to progress). I think I should validate
> it in ch348_attach() instead - what do you think?

Sure you can do that.

> > > +             else
> > > +                     /*
> > > +                      * Only ingest as many bytes as we can transfer with
> > > +                      * one URB at a time keeping the TX header in mind.
> > > +                      */
> > > +                     max_bytes = hw_tx_port->bulk_out_size - CH348_TX_HDRSIZE;
> > > +
> > > +             count = kfifo_out_locked(&port->write_fifo, rxt->data,
> > > +                                      max_bytes, &port->lock);
> > > +             if (count)
> > > +                     break;
> > > +     }
> >
> > With this implementation writing data continuously to one port will
> > starve the others.
> >
> > The vendor implementation appears to write to more than one port in
> > parallel and track THRE per port which would avoid the starvation issue
> > and should also be much more efficient.
> >
> > Just track THRE per port and only submit the write urb when it the
> > transmitter is empty or when it becomes empty.

> I'm trying as you suggest:
> - submit the URB synchronously for port N
> - submit the URB synchronously for port N + 1
> - ...
> 
> This seems to work (using usb_bulk_msg). What doesn't work is
> submitting URBs in parallel (this is what the vendor driver prevents
> as well).

No, the vendor driver tracks THRE per port
(ttyport[portnum].write_empty) and allows writing to more than one port
in parallel (e.g. releases the device write_lock while waiting for the
transfer to complete).

I thought the problem was that you could not submit another urb for the
*same* port until the device buffer for that port had been emptied?

This seems to be what the vendor driver is preventing.

> > You should implement dtr_rts() as well.

> This will be the first time we need the "package type" information as
> CH348Q only supports CTS/RTS on the first four ports, for the last
> four the signals aren't routed outside the package.
> I need to see if I have other hardware with CTS/RTS pins to test this.

Just connect a multimeter to the DTR and RTS pins and verify that they
are asserted on open and deasserted on close after issuing those control
requests (see ch9344_port_dtr_rts()).

I didn't mean that you need to support hw flow control (CRTSCTS).

Johan

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Johan Hovold]

On Wed, Jul 16, 2025 at 11:31:49AM +0200, Martin Blumenstingl wrote:
> On Wed, Jul 16, 2025 at 10:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > And are you trying to only have one set of urbs out for any port being
> > opened (i.e. you only have one control, one read, and one write urb for
> > the whole device, and the port info are multiplexed over these urbs?  Or
> > do you have one endpoint per port?)

> CH348 provides up to 8 serial ports using these four endpoints, so
> multiplexing is going on:
> - one bulk out for TX (see struct ch348_txbuf)
> - one bulk in for RX (see struct ch348_rxbuf)
> - one bulk out for CONFIG handling (see struct ch348_config_buf)
> - one bulk in for STATUS handling (see struct ch348_status_entry)
> 
> > If you are sharing endpoints, try looking at one of the other usb-serial
> > drivers that do this today, like io_edgeport.c, that has had shared
> > endpoints for 25 years, it's not a new thing :)

> My understanding is that io_edgeport is submits the URBs that are
> shared across ports outside of .open/.close.
> So this will be a question for Johan: am I still good with the
> original approach - or can you convince Greg that a different approach
> is better?

It's definitely better not to waste power when the device is plugged in
but not in use. :)

Take a look at f81534 for an example of how this can be implemented.

Johan

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Martin Blumenstingl]

On Fri, Jul 25, 2025 at 12:07 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:
>
> > On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@kernel.org> wrote:
>
> > > The read urbs should be submitted at first open and stopped at last
> > > close to avoid wasting resources when no one is using the device.
> > >
> > > I know we have a few drivers that do not do this currently, but it
> > > shouldn't be that hard to get this right from the start.
>
> > If you're aware of an easy approach or you can recommend an existing
> > driver that implements the desired behavior then please let me know.
> >
> > The speciality about ch348 is that all ports share the RX/TX URBs.
> > My current idea is to implement this using a ref count (for the number
> > of open ports) and mutex for locking.
>
> Just use a mutex and integer (not refcount) to count the number of open
> ports. Submit the urbs on first open and stop them on last close.
>
> Not doing so, and instead submitting at attach(), means that the host
> controller will be wasting power by polling the endpoints continuously
> as long as the device is plugged in.
Thanks, my code wasn't miles off of f81534.c but I'm following that
more closely now.

[...]
> > I'm trying as you suggest:
> > - submit the URB synchronously for port N
> > - submit the URB synchronously for port N + 1
> > - ...
> >
> > This seems to work (using usb_bulk_msg). What doesn't work is
> > submitting URBs in parallel (this is what the vendor driver prevents
> > as well).
>
> No, the vendor driver tracks THRE per port
> (ttyport[portnum].write_empty) and allows writing to more than one port
> in parallel (e.g. releases the device write_lock while waiting for the
> transfer to complete).
>
> I thought the problem was that you could not submit another urb for the
> *same* port until the device buffer for that port had been emptied?
>
> This seems to be what the vendor driver is preventing.
I managed to get it to work now without any unnecessary waiting.
When I switched to just waiting for per-port THRE I accidentally
re-used the same URB (along with its buffer) for all ports. This of
course "corrupts" data, but it's my fault instead of the chip/firmware
causing it.
That's why I was referring to data corruption earlier.
Thanks for your persistence and for making me look at my code again
with a fresh mind.

> > > You should implement dtr_rts() as well.
>
> > This will be the first time we need the "package type" information as
> > CH348Q only supports CTS/RTS on the first four ports, for the last
> > four the signals aren't routed outside the package.
> > I need to see if I have other hardware with CTS/RTS pins to test this.
>
> Just connect a multimeter to the DTR and RTS pins and verify that they
> are asserted on open and deasserted on close after issuing those control
> requests (see ch9344_port_dtr_rts()).
Do I need to set anything special in termios for this to work?
The datasheet has a special note about DTR/TNOW (on page 8 for the CFG pin):
> Unified configuration: During power-on, if the CFG pin is
> at a high level or not connected, all DTRx/ TNOWx pins
> are configured to function as TNOW. CFG pin is low, all
> DTRx/ TNOWx pins are configured for DTR function.

On my test board the CFG pin is HIGH. From how I understand you, RTS
should at least change (even if DTR is in TNOW mode).
No matter what I do: both pins are always LOW (right after modprobe,
after opening the console, closing the console again, ...).
I even set up the vendor driver to test this: it's the same situation there.

If we need to make the DTR and RTS output something then the only way
I know of right now is to switch them to GPIO mode (I have code for
this but it's another ~300 lines patch on top of this).
So I'd like to not implement .dtr_rts and drop all CRTSCTS related code for now.


Best regards,
Martin

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Johan Hovold]

On Sat, Jul 26, 2025 at 04:54:17PM +0200, Martin Blumenstingl wrote:
> On Fri, Jul 25, 2025 at 12:07 PM Johan Hovold <johan@kernel.org> wrote:

> > No, the vendor driver tracks THRE per port
> > (ttyport[portnum].write_empty) and allows writing to more than one port
> > in parallel (e.g. releases the device write_lock while waiting for the
> > transfer to complete).
> >
> > I thought the problem was that you could not submit another urb for the
> > *same* port until the device buffer for that port had been emptied?
> >
> > This seems to be what the vendor driver is preventing.

> I managed to get it to work now without any unnecessary waiting.
> When I switched to just waiting for per-port THRE I accidentally
> re-used the same URB (along with its buffer) for all ports. This of
> course "corrupts" data, but it's my fault instead of the chip/firmware
> causing it.
> That's why I was referring to data corruption earlier.
> Thanks for your persistence and for making me look at my code again
> with a fresh mind.

Glad to hear you got it working. Did you confirm that you really need to
wait for THRE before submitting the next URB too? I don't see why the
vendor driver would be doing this otherwise, but perhaps it only affects
older, broken firmware, or something.

> > > > You should implement dtr_rts() as well.
> >
> > > This will be the first time we need the "package type" information as
> > > CH348Q only supports CTS/RTS on the first four ports, for the last
> > > four the signals aren't routed outside the package.
> > > I need to see if I have other hardware with CTS/RTS pins to test this.
> >
> > Just connect a multimeter to the DTR and RTS pins and verify that they
> > are asserted on open and deasserted on close after issuing those control
> > requests (see ch9344_port_dtr_rts()).

> Do I need to set anything special in termios for this to work?

The TTY layer will assert DTR/RTS on open() and deassert on close() as
long as HUPCL is set. If you implement tiocmset() you can use that to
toggle the lines as well.

> The datasheet has a special note about DTR/TNOW (on page 8 for the CFG pin):
> > Unified configuration: During power-on, if the CFG pin is
> > at a high level or not connected, all DTRx/ TNOWx pins
> > are configured to function as TNOW. CFG pin is low, all
> > DTRx/ TNOWx pins are configured for DTR function.

Got a link to the datasheet? Not sure what they refer to as TNOW.

> On my test board the CFG pin is HIGH. From how I understand you, RTS
> should at least change (even if DTR is in TNOW mode).
> No matter what I do: both pins are always LOW (right after modprobe,
> after opening the console, closing the console again, ...).
> I even set up the vendor driver to test this: it's the same situation there.

I don't think the console code will assert DTR/RTS, you need to open the
port as a regular tty.

> If we need to make the DTR and RTS output something then the only way
> I know of right now is to switch them to GPIO mode (I have code for
> this but it's another ~300 lines patch on top of this).

That should not be needed. It looked like the vendor driver had some
variant of the usual request to control the modem lines. That should be
all that is needed.

Look at the vendor driver implementation of ch9344_tty_tiocmset() and
ch9344_port_dtr_rts().

Johan

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Martin Blumenstingl]

Hi Johan,

On Tue, Jul 29, 2025 at 11:43 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Sat, Jul 26, 2025 at 04:54:17PM +0200, Martin Blumenstingl wrote:
> > On Fri, Jul 25, 2025 at 12:07 PM Johan Hovold <johan@kernel.org> wrote:
>
> > > No, the vendor driver tracks THRE per port
> > > (ttyport[portnum].write_empty) and allows writing to more than one port
> > > in parallel (e.g. releases the device write_lock while waiting for the
> > > transfer to complete).
> > >
> > > I thought the problem was that you could not submit another urb for the
> > > *same* port until the device buffer for that port had been emptied?
> > >
> > > This seems to be what the vendor driver is preventing.
>
> > I managed to get it to work now without any unnecessary waiting.
> > When I switched to just waiting for per-port THRE I accidentally
> > re-used the same URB (along with its buffer) for all ports. This of
> > course "corrupts" data, but it's my fault instead of the chip/firmware
> > causing it.
> > That's why I was referring to data corruption earlier.
> > Thanks for your persistence and for making me look at my code again
> > with a fresh mind.
>
> Glad to hear you got it working. Did you confirm that you really need to
> wait for THRE before submitting the next URB too? I don't see why the
> vendor driver would be doing this otherwise, but perhaps it only affects
> older, broken firmware, or something.
I'm using Corentin's test script [0] for sending data and by
connecting RX6 to TX7 and TX6 to RX7.

For a 1024 byte buffer:
[ 3029.068311] ch348 ttyUSB6: submitted 509 bytes for urb 0
[ 3029.068827] ch348 ttyUSB6: submitted 509 bytes for urb 1
[ 3029.069363] ch348 ttyUSB7: submitted 5 bytes for urb 0
[ 3029.069902] ch348 ttyUSB7: UART_IIR_THRI - unknown byte: 0x00
[ 3029.215272] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
[ 3029.215908] ch348 ttyUSB6: submitted 6 bytes for urb 0
[ 3029.233628] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
-> data is received without corruption

With a 2048 byte buffer the general flow seems fine:
[ 3031.073101] ch348 ttyUSB6: submitted 509 bytes for urb 0
[ 3031.073777] ch348 ttyUSB6: submitted 509 bytes for urb 1
[ 3031.220068] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
[ 3031.220697] ch348 ttyUSB6: submitted 509 bytes for urb 0
[ 3031.221342] ch348 ttyUSB6: submitted 509 bytes for urb 1
[ 3031.512113] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
[ 3031.512795] ch348 ttyUSB6: submitted 12 bytes for urb 0
[ 3031.513359] ch348 ttyUSB7: submitted 5 bytes for urb 0
[ 3031.513859] ch348 ttyUSB7: UART_IIR_THRI - unknown byte: 0x00
[ 3031.530476] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
However, the receiving end sees different data (at around byte 513-518
in my tests) than we wanted to send.

My general flow is:
- check if we have received THRE - if not: don't transmit more data on this port
- submit up to two URBs with up to 512 - 3 (CH348_TX_HDRSIZE) bytes to
not exceed the HW TX FIFO size of 1024 bytes (page 1 in the datasheet)
if the kfifo has enough data

If you want me to test something else then please let me know and I'll try it.
Otherwise I'll not dig much deeper, given the fact that I don't know
how the firmware works (e.g. in which order they send the status to
the host and what kind of state they hold internally) and we can still
optimize this later.

[...]
> > The datasheet has a special note about DTR/TNOW (on page 8 for the CFG pin):
> > > Unified configuration: During power-on, if the CFG pin is
> > > at a high level or not connected, all DTRx/ TNOWx pins
> > > are configured to function as TNOW. CFG pin is low, all
> > > DTRx/ TNOWx pins are configured for DTR function.
>
> Got a link to the datasheet? Not sure what they refer to as TNOW.
Sure, try the direct link [1] - and if it doesn't work try [2].
It doesn't document any registers, so it's a high-level datasheet -
nor a programmers handbook.

> > On my test board the CFG pin is HIGH. From how I understand you, RTS
> > should at least change (even if DTR is in TNOW mode).
> > No matter what I do: both pins are always LOW (right after modprobe,
> > after opening the console, closing the console again, ...).
> > I even set up the vendor driver to test this: it's the same situation there.
>
> I don't think the console code will assert DTR/RTS, you need to open the
> port as a regular tty.
>
> > If we need to make the DTR and RTS output something then the only way
> > I know of right now is to switch them to GPIO mode (I have code for
> > this but it's another ~300 lines patch on top of this).
>
> That should not be needed. It looked like the vendor driver had some
> variant of the usual request to control the modem lines. That should be
> all that is needed.
>
> Look at the vendor driver implementation of ch9344_tty_tiocmset() and
> ch9344_port_dtr_rts().
Thank you for the pointers. For today I -ETIMEDOUT, I'll take a look
at these in the next few days.


Best regards,
Martin


[0] https://github.com/montjoie/lava-tests/blob/5d4d83f2f71c37432dcdcc55ce3e31e74d133737/test2a2.py
[1] https://www.wch-ic.com/download/file?id=300
[2] https://www.wch-ic.com/downloads/CH348DS1_PDF.html

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Johan Hovold]

On Tue, Jul 29, 2025 at 10:45:20PM +0200, Martin Blumenstingl wrote:
> On Tue, Jul 29, 2025 at 11:43 AM Johan Hovold <johan@kernel.org> wrote:
> > On Sat, Jul 26, 2025 at 04:54:17PM +0200, Martin Blumenstingl wrote:

> > > I managed to get it to work now without any unnecessary waiting.
> > > When I switched to just waiting for per-port THRE I accidentally
> > > re-used the same URB (along with its buffer) for all ports. This of
> > > course "corrupts" data, but it's my fault instead of the chip/firmware
> > > causing it.
> > > That's why I was referring to data corruption earlier.
> > > Thanks for your persistence and for making me look at my code again
> > > with a fresh mind.
> >
> > Glad to hear you got it working. Did you confirm that you really need to
> > wait for THRE before submitting the next URB too? I don't see why the
> > vendor driver would be doing this otherwise, but perhaps it only affects
> > older, broken firmware, or something.

> I'm using Corentin's test script [0] for sending data and by
> connecting RX6 to TX7 and TX6 to RX7.

May be better to use a second different device (rather than loopback)
for testing so that you can separate any tx issues from rx issues.

> For a 1024 byte buffer:
> [ 3029.068311] ch348 ttyUSB6: submitted 509 bytes for urb 0
> [ 3029.068827] ch348 ttyUSB6: submitted 509 bytes for urb 1
> [ 3029.069363] ch348 ttyUSB7: submitted 5 bytes for urb 0
> [ 3029.069902] ch348 ttyUSB7: UART_IIR_THRI - unknown byte: 0x00
> [ 3029.215272] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> [ 3029.215908] ch348 ttyUSB6: submitted 6 bytes for urb 0
> [ 3029.233628] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> -> data is received without corruption
> 
> With a 2048 byte buffer the general flow seems fine:
> [ 3031.073101] ch348 ttyUSB6: submitted 509 bytes for urb 0
> [ 3031.073777] ch348 ttyUSB6: submitted 509 bytes for urb 1
> [ 3031.220068] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> [ 3031.220697] ch348 ttyUSB6: submitted 509 bytes for urb 0
> [ 3031.221342] ch348 ttyUSB6: submitted 509 bytes for urb 1
> [ 3031.512113] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> [ 3031.512795] ch348 ttyUSB6: submitted 12 bytes for urb 0
> [ 3031.513359] ch348 ttyUSB7: submitted 5 bytes for urb 0
> [ 3031.513859] ch348 ttyUSB7: UART_IIR_THRI - unknown byte: 0x00
> [ 3031.530476] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> However, the receiving end sees different data (at around byte 513-518
> in my tests) than we wanted to send.
> 
> My general flow is:
> - check if we have received THRE - if not: don't transmit more data on this port
> - submit up to two URBs with up to 512 - 3 (CH348_TX_HDRSIZE) bytes to
> not exceed the HW TX FIFO size of 1024 bytes (page 1 in the datasheet)
> if the kfifo has enough data

If you're going to wait for the device fifo to clear completely you can
just use a single urb with larger (1k) buffer too.

> If you want me to test something else then please let me know and I'll try it.
> Otherwise I'll not dig much deeper, given the fact that I don't know
> how the firmware works (e.g. in which order they send the status to
> the host and what kind of state they hold internally) and we can still
> optimize this later.

Yeah, as long as you are certain that the generic implementation does
not work and that you indeed need to track THRE per port.

> [...]
> > > The datasheet has a special note about DTR/TNOW (on page 8 for the CFG pin):
> > > > Unified configuration: During power-on, if the CFG pin is
> > > > at a high level or not connected, all DTRx/ TNOWx pins
> > > > are configured to function as TNOW. CFG pin is low, all
> > > > DTRx/ TNOWx pins are configured for DTR function.
> >
> > Got a link to the datasheet? Not sure what they refer to as TNOW.

> Sure, try the direct link [1] - and if it doesn't work try [2].
> It doesn't document any registers, so it's a high-level datasheet -
> nor a programmers handbook.

Ok, so TNOW is used for RS485 to signal that the device is transmitting.
 
> > > On my test board the CFG pin is HIGH. From how I understand you, RTS
> > > should at least change (even if DTR is in TNOW mode).
> > > No matter what I do: both pins are always LOW (right after modprobe,
> > > after opening the console, closing the console again, ...).
> > > I even set up the vendor driver to test this: it's the same situation there.
> >
> > I don't think the console code will assert DTR/RTS, you need to open the
> > port as a regular tty.

Yes, even if the device is configured in hardware for TNOW mode (instead
of DTR function) you should still be able to control RTS (at least as
long as the device is not configured for automatic hardware flow control).

Johan

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Martin Blumenstingl]

On Mon, Aug 4, 2025 at 2:32 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Jul 29, 2025 at 10:45:20PM +0200, Martin Blumenstingl wrote:
> > On Tue, Jul 29, 2025 at 11:43 AM Johan Hovold <johan@kernel.org> wrote:
> > > On Sat, Jul 26, 2025 at 04:54:17PM +0200, Martin Blumenstingl wrote:
>
> > > > I managed to get it to work now without any unnecessary waiting.
> > > > When I switched to just waiting for per-port THRE I accidentally
> > > > re-used the same URB (along with its buffer) for all ports. This of
> > > > course "corrupts" data, but it's my fault instead of the chip/firmware
> > > > causing it.
> > > > That's why I was referring to data corruption earlier.
> > > > Thanks for your persistence and for making me look at my code again
> > > > with a fresh mind.
> > >
> > > Glad to hear you got it working. Did you confirm that you really need to
> > > wait for THRE before submitting the next URB too? I don't see why the
> > > vendor driver would be doing this otherwise, but perhaps it only affects
> > > older, broken firmware, or something.
>
> > I'm using Corentin's test script [0] for sending data and by
> > connecting RX6 to TX7 and TX6 to RX7.
>
> May be better to use a second different device (rather than loopback)
> for testing so that you can separate any tx issues from rx issues.
I'll double-check it using a second device. The RX path has largely
been unmodified since the original submission, so it's likely that the
issue is indeed with the TX path.

> > For a 1024 byte buffer:
> > [ 3029.068311] ch348 ttyUSB6: submitted 509 bytes for urb 0
> > [ 3029.068827] ch348 ttyUSB6: submitted 509 bytes for urb 1
> > [ 3029.069363] ch348 ttyUSB7: submitted 5 bytes for urb 0
> > [ 3029.069902] ch348 ttyUSB7: UART_IIR_THRI - unknown byte: 0x00
> > [ 3029.215272] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > [ 3029.215908] ch348 ttyUSB6: submitted 6 bytes for urb 0
> > [ 3029.233628] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > -> data is received without corruption
> >
> > With a 2048 byte buffer the general flow seems fine:
> > [ 3031.073101] ch348 ttyUSB6: submitted 509 bytes for urb 0
> > [ 3031.073777] ch348 ttyUSB6: submitted 509 bytes for urb 1
> > [ 3031.220068] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > [ 3031.220697] ch348 ttyUSB6: submitted 509 bytes for urb 0
> > [ 3031.221342] ch348 ttyUSB6: submitted 509 bytes for urb 1
> > [ 3031.512113] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > [ 3031.512795] ch348 ttyUSB6: submitted 12 bytes for urb 0
> > [ 3031.513359] ch348 ttyUSB7: submitted 5 bytes for urb 0
> > [ 3031.513859] ch348 ttyUSB7: UART_IIR_THRI - unknown byte: 0x00
> > [ 3031.530476] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > However, the receiving end sees different data (at around byte 513-518
> > in my tests) than we wanted to send.
> >
> > My general flow is:
> > - check if we have received THRE - if not: don't transmit more data on this port
> > - submit up to two URBs with up to 512 - 3 (CH348_TX_HDRSIZE) bytes to
> > not exceed the HW TX FIFO size of 1024 bytes (page 1 in the datasheet)
> > if the kfifo has enough data
>
> If you're going to wait for the device fifo to clear completely you can
> just use a single urb with larger (1k) buffer too.
I set .bulk_out_size = 1024 in struct usb_serial_driver. Writing a 1k
buffer immediately results in:
   ch348 1-1:1.0: device disconnected

I don't know if I need to set some kind of flag on the URB to have it
split or whether the kernel / USB controller does that automatically
(as you can tell: I'm not familiar with USB).
If not: 512 byte transfers at a time it is.

> > If you want me to test something else then please let me know and I'll try it.
> > Otherwise I'll not dig much deeper, given the fact that I don't know
> > how the firmware works (e.g. in which order they send the status to
> > the host and what kind of state they hold internally) and we can still
> > optimize this later.
>
> Yeah, as long as you are certain that the generic implementation does
> not work and that you indeed need to track THRE per port.
I'll give it one more round in the next few days. If I can't get the
generic implementation to work then I'll call the current approach
good.

[...]
> > > > On my test board the CFG pin is HIGH. From how I understand you, RTS
> > > > should at least change (even if DTR is in TNOW mode).
> > > > No matter what I do: both pins are always LOW (right after modprobe,
> > > > after opening the console, closing the console again, ...).
> > > > I even set up the vendor driver to test this: it's the same situation there.
> > >
> > > I don't think the console code will assert DTR/RTS, you need to open the
> > > port as a regular tty.
>
> Yes, even if the device is configured in hardware for TNOW mode (instead
> of DTR function) you should still be able to control RTS (at least as
> long as the device is not configured for automatic hardware flow control).
I think I made it work, sort of.
It's a bit annoying because of code I don't understand. It seems that
R_4 has the following settings:
0x00 DTR off
0x01 DTR on
0x10 RTS off
0x11 RTS on
0x08 activate (used during port initialization)
0x50 HW flow on
0x51 no RTS / HW flow off

That said, poking 0x00, 0x01, 0x10 and 0x11 by themselves didn't do much.
One also has to write 0x06 to the per-port VEN_R register.
The vendor driver only does that in .set_termios, which I call
questionable until someone calls me out on this and is willing to
share a good reason why that's a good idea ;-)

However, I'm unable to control the RTS line of port 1. It works for
port 0, port 2 and 3 but not for port 1.
Ports 4-7 don't have the TNOW/DTR and RTS lines routed outside the
package, so I can't test these.


Best regards,
Martin

Re: [PATCH v8 1/2] usb: serial: add support for CH348

[Johan Hovold]

On Mon, Aug 04, 2025 at 11:35:35PM +0200, Martin Blumenstingl wrote:
> On Mon, Aug 4, 2025 at 2:32 PM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Jul 29, 2025 at 10:45:20PM +0200, Martin Blumenstingl wrote:

> > > My general flow is:
> > > - check if we have received THRE - if not: don't transmit more data on this port
> > > - submit up to two URBs with up to 512 - 3 (CH348_TX_HDRSIZE) bytes to
> > > not exceed the HW TX FIFO size of 1024 bytes (page 1 in the datasheet)
> > > if the kfifo has enough data
> >
> > If you're going to wait for the device fifo to clear completely you can
> > just use a single urb with larger (1k) buffer too.

> I set .bulk_out_size = 1024 in struct usb_serial_driver. Writing a 1k
> buffer immediately results in:
>    ch348 1-1:1.0: device disconnected
> 
> I don't know if I need to set some kind of flag on the URB to have it
> split or whether the kernel / USB controller does that automatically
> (as you can tell: I'm not familiar with USB).
> If not: 512 byte transfers at a time it is.

The host controller should split the buffer, but apparently this crashes
the device firmware.

> > > > > On my test board the CFG pin is HIGH. From how I understand you, RTS
> > > > > should at least change (even if DTR is in TNOW mode).
> > > > > No matter what I do: both pins are always LOW (right after modprobe,
> > > > > after opening the console, closing the console again, ...).
> > > > > I even set up the vendor driver to test this: it's the same situation there.
> > > >
> > > > I don't think the console code will assert DTR/RTS, you need to open the
> > > > port as a regular tty.
> >
> > Yes, even if the device is configured in hardware for TNOW mode (instead
> > of DTR function) you should still be able to control RTS (at least as
> > long as the device is not configured for automatic hardware flow control).

> I think I made it work, sort of.
> It's a bit annoying because of code I don't understand. It seems that
> R_4 has the following settings:
> 0x00 DTR off
> 0x01 DTR on
> 0x10 RTS off
> 0x11 RTS on
> 0x08 activate (used during port initialization)
> 0x50 HW flow on
> 0x51 no RTS / HW flow off
> 
> That said, poking 0x00, 0x01, 0x10 and 0x11 by themselves didn't do much.
> One also has to write 0x06 to the per-port VEN_R register.
> The vendor driver only does that in .set_termios, which I call
> questionable until someone calls me out on this and is willing to
> share a good reason why that's a good idea ;-)
> 
> However, I'm unable to control the RTS line of port 1. It works for
> port 0, port 2 and 3 but not for port 1.
> Ports 4-7 don't have the TNOW/DTR and RTS lines routed outside the
> package, so I can't test these.

Sounds like good progress. Have you made sure HW flow isn't just enabled
by default on port 1 or similar?

Johan