[PATCH v5 0/3] Add support for MaxLinear/Exar USB to serial converters

[PATCH v5 0/3] Add support for MaxLinear/Exar USB to serial converters

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Hello,

This series adds support for MaxLinear/Exar USB to serial converters.
This driver only supports XR21V141X series but it can easily be extended
to other series in future.

This driver is inspired from the initial one submitted by Patong Yang:
https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop

While the initial driver was a custom tty USB driver exposing whole
new serial interface ttyXRUSBn, this version is completely based on USB
serial core thus exposing the interfaces as ttyUSBn. This will avoid
the overhead of exposing a new USB serial interface which the userspace
tools are unaware of.

This series has been tested with Hikey970 board hosting XR21V141X chip.

NOTE: I've removed all reviews and tested-by tags as the code has gone
through substantial rework. Greg, Linus, Mauro please consider reviewing
again.

Thanks,
Mani

Changes in v5:

* Incorporated review comments from Johan. Noticeable ones are:
  - Made serial and gpiolib support exclusive and used mutex to avoid
    race. The gpio requests from gpiolib will be rejected when serial
    port is in use.
  - The driver only binds to data interface but claims both control and
    data interface.
  - Handled B0 request
  - Removed all reviews as the code has gone through substantial rework.

Changes in v4:

* Multiple improvements based on Johan's review. Noticeable ones are:
  - Now the driver claims both control and data interfaces but only registers
    tty device for data interface.
  - GPIO pin status is now shared between the console and gpiolib
    implementations. This is done to avoid changing the lines spuriously.
  - A separate port_open flag is added to reject GPIO requests while the tty
    port is open.
  - Removed padding PID to gpio device.
* Added Greg and Mauro's review and tested tags.
* Included a patch from Mauro to avoid the CDC-ACM driver to claim this device
  when this driver is built.

Changes in v3:

* Dropped the check for PID and also the reg_width property.

Changes in v2:

* Dropped the code related to handling variable register size. It's all u8 now.
* Dropped the header file and moved the contents to driver itself.
* Added Linus's reviewed-by tag for gpiochip patch.
* Added PID to gpiochip label
* Dropped gpiochip for interface 0

Manivannan Sadhasivam (2):
  usb: serial: Add MaxLinear/Exar USB to Serial driver
  usb: serial: xr_serial: Add gpiochip support

Mauro Carvalho Chehab (1):
  usb: cdc-acm: Ignore Exar XR21V141X when serial driver is built

 drivers/usb/class/cdc-acm.c    |   6 +
 drivers/usb/serial/Kconfig     |   9 +
 drivers/usb/serial/Makefile    |   1 +
 drivers/usb/serial/xr_serial.c | 854 +++++++++++++++++++++++++++++++++
 4 files changed, 870 insertions(+)
 create mode 100644 drivers/usb/serial/xr_serial.c

-- 
2.25.1

[PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Manivannan Sadhasivam]

Add support for MaxLinear/Exar USB to Serial converters. This driver
only supports XR21V141X series but it can be extended to other series
from Exar as well in future.

This driver is inspired from the initial one submitted by Patong Yang:

https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop

While the initial driver was a custom tty USB driver exposing whole
new serial interface ttyXRUSBn, this version is completely based on USB
serial core thus exposing the interfaces as ttyUSBn. This will avoid
the overhead of exposing a new USB serial interface which the userspace
tools are unaware of.

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/usb/serial/Kconfig     |   9 +
 drivers/usb/serial/Makefile    |   1 +
 drivers/usb/serial/xr_serial.c | 599 +++++++++++++++++++++++++++++++++
 3 files changed, 609 insertions(+)
 create mode 100644 drivers/usb/serial/xr_serial.c

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 4007fa25a8ff..32595acb1d1d 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -644,6 +644,15 @@ config USB_SERIAL_UPD78F0730
 	  To compile this driver as a module, choose M here: the
 	  module will be called upd78f0730.
 
+config USB_SERIAL_XR
+	tristate "USB MaxLinear/Exar USB to Serial driver"
+	help
+	  Say Y here if you want to use MaxLinear/Exar USB to Serial converter
+	  devices.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called xr_serial.
+
 config USB_SERIAL_DEBUG
 	tristate "USB Debugging Device"
 	help
diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
index 2d491e434f11..4f69c2a3aff3 100644
--- a/drivers/usb/serial/Makefile
+++ b/drivers/usb/serial/Makefile
@@ -62,4 +62,5 @@ obj-$(CONFIG_USB_SERIAL_VISOR)			+= visor.o
 obj-$(CONFIG_USB_SERIAL_WISHBONE)		+= wishbone-serial.o
 obj-$(CONFIG_USB_SERIAL_WHITEHEAT)		+= whiteheat.o
 obj-$(CONFIG_USB_SERIAL_XIRCOM)			+= keyspan_pda.o
+obj-$(CONFIG_USB_SERIAL_XR)			+= xr_serial.o
 obj-$(CONFIG_USB_SERIAL_XSENS_MT)		+= xsens_mt.o
diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
new file mode 100644
index 000000000000..e166916554d6
--- /dev/null
+++ b/drivers/usb/serial/xr_serial.c
@@ -0,0 +1,599 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MaxLinear/Exar USB to Serial driver
+ *
+ * Based on the initial driver written by Patong Yang:
+ * https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
+ *
+ * Copyright (c) 2018 Patong Yang <patong.mxl@gmail.com>
+ * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/usb.h>
+#include <linux/usb/serial.h>
+
+struct xr_txrx_clk_mask {
+	u16 tx;
+	u16 rx0;
+	u16 rx1;
+};
+
+#define XR_INT_OSC_HZ			48000000U
+#define XR21V141X_MIN_SPEED		46U
+#define XR21V141X_MAX_SPEED		XR_INT_OSC_HZ
+
+/* USB Requests */
+#define XR21V141X_SET_REQ		0
+#define XR21V141X_GET_REQ		1
+
+#define XR21V141X_CLOCK_DIVISOR_0	0x04
+#define XR21V141X_CLOCK_DIVISOR_1	0x05
+#define XR21V141X_CLOCK_DIVISOR_2	0x06
+#define XR21V141X_TX_CLOCK_MASK_0	0x07
+#define XR21V141X_TX_CLOCK_MASK_1	0x08
+#define XR21V141X_RX_CLOCK_MASK_0	0x09
+#define XR21V141X_RX_CLOCK_MASK_1	0x0a
+
+/* XR21V141X register blocks */
+#define XR21V141X_UART_REG_BLOCK	0
+#define XR21V141X_UM_REG_BLOCK		4
+#define XR21V141X_UART_CUSTOM_BLOCK	0x66
+
+/* XR21V141X UART Manager Registers */
+#define XR21V141X_UM_FIFO_ENABLE_REG	0x10
+#define XR21V141X_UM_ENABLE_TX_FIFO	0x01
+#define XR21V141X_UM_ENABLE_RX_FIFO	0x02
+
+#define XR21V141X_UM_RX_FIFO_RESET	0x18
+#define XR21V141X_UM_TX_FIFO_RESET	0x1c
+
+#define XR21V141X_UART_ENABLE_TX	0x1
+#define XR21V141X_UART_ENABLE_RX	0x2
+
+#define XR21V141X_UART_MODE_RI		BIT(0)
+#define XR21V141X_UART_MODE_CD		BIT(1)
+#define XR21V141X_UART_MODE_DSR		BIT(2)
+#define XR21V141X_UART_MODE_DTR		BIT(3)
+#define XR21V141X_UART_MODE_CTS		BIT(4)
+#define XR21V141X_UART_MODE_RTS		BIT(5)
+
+#define XR21V141X_UART_BREAK_ON		0xff
+#define XR21V141X_UART_BREAK_OFF	0
+
+#define XR21V141X_UART_DATA_MASK	GENMASK(3, 0)
+#define XR21V141X_UART_DATA_7		0x7
+#define XR21V141X_UART_DATA_8		0x8
+
+#define XR21V141X_UART_PARITY_MASK	GENMASK(6, 4)
+#define XR21V141X_UART_PARITY_SHIFT	0x4
+#define XR21V141X_UART_PARITY_NONE	0x0
+#define XR21V141X_UART_PARITY_ODD	0x1
+#define XR21V141X_UART_PARITY_EVEN	0x2
+#define XR21V141X_UART_PARITY_MARK	0x3
+#define XR21V141X_UART_PARITY_SPACE	0x4
+
+#define XR21V141X_UART_STOP_MASK	BIT(7)
+#define XR21V141X_UART_STOP_SHIFT	0x7
+#define XR21V141X_UART_STOP_1		0x0
+#define XR21V141X_UART_STOP_2		0x1
+
+#define XR21V141X_UART_FLOW_MODE_NONE	0x0
+#define XR21V141X_UART_FLOW_MODE_HW	0x1
+#define XR21V141X_UART_FLOW_MODE_SW	0x2
+
+#define XR21V141X_UART_MODE_GPIO_MASK	GENMASK(2, 0)
+#define XR21V141X_UART_MODE_RTS_CTS	0x1
+#define XR21V141X_UART_MODE_DTR_DSR	0x2
+#define XR21V141X_UART_MODE_RS485	0x3
+#define XR21V141X_UART_MODE_RS485_ADDR	0x4
+
+#define XR21V141X_REG_ENABLE		0x03
+#define XR21V141X_REG_FORMAT		0x0b
+#define XR21V141X_REG_FLOW_CTRL		0x0c
+#define XR21V141X_REG_XON_CHAR		0x10
+#define XR21V141X_REG_XOFF_CHAR		0x11
+#define XR21V141X_REG_LOOPBACK		0x12
+#define XR21V141X_REG_TX_BREAK		0x14
+#define XR21V141X_REG_RS845_DELAY	0x15
+#define XR21V141X_REG_GPIO_MODE		0x1a
+#define XR21V141X_REG_GPIO_DIR		0x1b
+#define XR21V141X_REG_GPIO_INT_MASK	0x1c
+#define XR21V141X_REG_GPIO_SET		0x1d
+#define XR21V141X_REG_GPIO_CLR		0x1e
+#define XR21V141X_REG_GPIO_STATUS	0x1f
+
+static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg,
+		      u8 val)
+{
+	struct usb_serial *serial = port->serial;
+	int ret;
+
+	ret = usb_control_msg(serial->dev,
+			      usb_sndctrlpipe(serial->dev, 0),
+			      XR21V141X_SET_REQ,
+			      USB_DIR_OUT | USB_TYPE_VENDOR, val,
+			      reg | (block << 8), NULL, 0,
+			      USB_CTRL_SET_TIMEOUT);
+	if (ret < 0) {
+		dev_err(&port->dev, "Failed to set reg 0x%02x: %d\n", reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int xr_get_reg(struct usb_serial_port *port, u8 block, u8 reg,
+		      u8 *val)
+{
+	struct usb_serial *serial = port->serial;
+	u8 *dmabuf;
+	int ret;
+
+	dmabuf = kmalloc(1, GFP_KERNEL);
+	if (!dmabuf)
+		return -ENOMEM;
+
+	ret = usb_control_msg(serial->dev,
+			      usb_rcvctrlpipe(serial->dev, 0),
+			      XR21V141X_GET_REQ,
+			      USB_DIR_IN | USB_TYPE_VENDOR, 0,
+			      reg | (block << 8), dmabuf, 1,
+			      USB_CTRL_GET_TIMEOUT);
+	if (ret == 1) {
+		*val = *dmabuf;
+		ret = 0;
+	} else {
+		dev_err(&port->dev, "Failed to get reg 0x%02x: %d\n", reg, ret);
+		if (ret >= 0)
+			ret = -EIO;
+	}
+
+	kfree(dmabuf);
+
+	return ret;
+}
+
+static int xr_set_reg_uart(struct usb_serial_port *port, u8 reg, u8 val)
+{
+	return xr_set_reg(port, XR21V141X_UART_REG_BLOCK, reg, val);
+}
+
+static int xr_get_reg_uart(struct usb_serial_port *port, u8 reg, u8 *val)
+{
+	return xr_get_reg(port, XR21V141X_UART_REG_BLOCK, reg, val);
+}
+
+static int xr_set_reg_um(struct usb_serial_port *port, u8 reg, u8 val)
+{
+	return xr_set_reg(port, XR21V141X_UM_REG_BLOCK, reg, val);
+}
+
+/* Tx and Rx clock mask values obtained from section 3.3.4 of datasheet */
+static const struct xr_txrx_clk_mask xr21v141x_txrx_clk_masks[] = {
+	{ 0x000, 0x000, 0x000 },
+	{ 0x000, 0x000, 0x000 },
+	{ 0x100, 0x000, 0x100 },
+	{ 0x020, 0x400, 0x020 },
+	{ 0x010, 0x100, 0x010 },
+	{ 0x208, 0x040, 0x208 },
+	{ 0x104, 0x820, 0x108 },
+	{ 0x844, 0x210, 0x884 },
+	{ 0x444, 0x110, 0x444 },
+	{ 0x122, 0x888, 0x224 },
+	{ 0x912, 0x448, 0x924 },
+	{ 0x492, 0x248, 0x492 },
+	{ 0x252, 0x928, 0x292 },
+	{ 0x94a, 0x4a4, 0xa52 },
+	{ 0x52a, 0xaa4, 0x54a },
+	{ 0xaaa, 0x954, 0x4aa },
+	{ 0xaaa, 0x554, 0xaaa },
+	{ 0x555, 0xad4, 0x5aa },
+	{ 0xb55, 0xab4, 0x55a },
+	{ 0x6b5, 0x5ac, 0xb56 },
+	{ 0x5b5, 0xd6c, 0x6d6 },
+	{ 0xb6d, 0xb6a, 0xdb6 },
+	{ 0x76d, 0x6da, 0xbb6 },
+	{ 0xedd, 0xdda, 0x76e },
+	{ 0xddd, 0xbba, 0xeee },
+	{ 0x7bb, 0xf7a, 0xdde },
+	{ 0xf7b, 0xef6, 0x7de },
+	{ 0xdf7, 0xbf6, 0xf7e },
+	{ 0x7f7, 0xfee, 0xefe },
+	{ 0xfdf, 0xfbe, 0x7fe },
+	{ 0xf7f, 0xefe, 0xffe },
+	{ 0xfff, 0xffe, 0xffd },
+};
+
+static int xr_set_baudrate(struct tty_struct *tty,
+			   struct usb_serial_port *port)
+{
+	u32 divisor, baud, idx;
+	u16 tx_mask, rx_mask;
+	int ret;
+
+	baud = clamp(tty->termios.c_ospeed, XR21V141X_MIN_SPEED,
+		     XR21V141X_MAX_SPEED);
+	divisor = XR_INT_OSC_HZ / baud;
+	idx = ((32 * XR_INT_OSC_HZ) / baud) & 0x1f;
+	tx_mask = xr21v141x_txrx_clk_masks[idx].tx;
+
+	if (divisor & 1)
+		rx_mask = xr21v141x_txrx_clk_masks[idx].rx1;
+	else
+		rx_mask = xr21v141x_txrx_clk_masks[idx].rx0;
+
+	dev_dbg(&port->dev, "Setting baud rate: %u\n", baud);
+	/*
+	 * XR21V141X uses fractional baud rate generator with 48MHz internal
+	 * oscillator and 19-bit programmable divisor. So theoretically it can
+	 * generate most commonly used baud rates with high accuracy.
+	 */
+	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_0, (divisor & 0xff));
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_1, ((divisor >>  8) & 0xff));
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_2, ((divisor >> 16) & 0xff));
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_uart(port, XR21V141X_TX_CLOCK_MASK_0, (tx_mask & 0xff));
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_uart(port, XR21V141X_TX_CLOCK_MASK_1, ((tx_mask >>  8) & 0xff));
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_uart(port, XR21V141X_RX_CLOCK_MASK_0, (rx_mask & 0xff));
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_uart(port, XR21V141X_RX_CLOCK_MASK_1, ((rx_mask >>  8) & 0xff));
+
+	tty_encode_baud_rate(tty, baud, baud);
+
+	return ret;
+}
+
+/*
+ * According to datasheet, below is the recommended sequence for enabling UART
+ * module in XR21V141X:
+ *
+ * Enable Tx FIFO
+ * Enable Tx and Rx
+ * Enable Rx FIFO
+ */
+static int xr_uart_enable(struct usb_serial_port *port)
+{
+	int ret;
+
+	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG, XR21V141X_UM_ENABLE_TX_FIFO);
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_uart(port, XR21V141X_REG_ENABLE,
+			      XR21V141X_UART_ENABLE_TX | XR21V141X_UART_ENABLE_RX);
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG,
+			    XR21V141X_UM_ENABLE_TX_FIFO | XR21V141X_UM_ENABLE_RX_FIFO);
+
+	if (ret)
+		xr_set_reg_uart(port, XR21V141X_REG_ENABLE, 0);
+
+	return ret;
+}
+
+static int xr_uart_disable(struct usb_serial_port *port)
+{
+	int ret;
+
+	ret = xr_set_reg_uart(port, XR21V141X_REG_ENABLE, 0);
+	if (ret)
+		return ret;
+
+	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG, 0);
+
+	return ret;
+}
+
+static void xr_set_flow_mode(struct tty_struct *tty,
+			     struct usb_serial_port *port)
+{
+	unsigned int cflag = tty->termios.c_cflag;
+	int ret;
+	u8 flow, gpio_mode;
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_MODE, &gpio_mode);
+	if (ret)
+		return;
+
+	if (cflag & CRTSCTS) {
+		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
+
+		/*
+		 * RTS/CTS is the default flow control mode, so set GPIO mode
+		 * for controlling the pins manually by default.
+		 */
+		gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK;
+		gpio_mode |= XR21V141X_UART_MODE_RTS_CTS;
+		flow = XR21V141X_UART_FLOW_MODE_HW;
+	} else if (I_IXON(tty)) {
+		u8 start_char = START_CHAR(tty);
+		u8 stop_char = STOP_CHAR(tty);
+
+		dev_dbg(&port->dev, "Enabling sw flow ctrl\n");
+		flow = XR21V141X_UART_FLOW_MODE_SW;
+
+		xr_set_reg_uart(port, XR21V141X_REG_XON_CHAR, start_char);
+		xr_set_reg_uart(port, XR21V141X_REG_XOFF_CHAR, stop_char);
+	} else {
+		dev_dbg(&port->dev, "Disabling flow ctrl\n");
+		flow = XR21V141X_UART_FLOW_MODE_NONE;
+	}
+
+	/*
+	 * As per the datasheet, UART needs to be disabled while writing to
+	 * FLOW_CONTROL register.
+	 */
+	xr_uart_disable(port);
+	xr_set_reg_uart(port, XR21V141X_REG_FLOW_CTRL, flow);
+	xr_uart_enable(port);
+
+	xr_set_reg_uart(port, XR21V141X_REG_GPIO_MODE, gpio_mode);
+}
+
+static int xr_tiocmset_port(struct usb_serial_port *port,
+			    unsigned int set, unsigned int clear)
+{
+	u8 gpio_set = 0;
+	u8 gpio_clr = 0;
+	int ret = 0;
+
+	/* Modem control pins are active low, so set & clr are swapped */
+	if (set & TIOCM_RTS)
+		gpio_clr |= XR21V141X_UART_MODE_RTS;
+	if (set & TIOCM_DTR)
+		gpio_clr |= XR21V141X_UART_MODE_DTR;
+	if (clear & TIOCM_RTS)
+		gpio_set |= XR21V141X_UART_MODE_RTS;
+	if (clear & TIOCM_DTR)
+		gpio_set |= XR21V141X_UART_MODE_DTR;
+
+	/* Writing '0' to gpio_{set/clr} bits has no effect, so no need to do */
+	if (gpio_clr)
+		ret = xr_set_reg_uart(port, XR21V141X_REG_GPIO_CLR, gpio_clr);
+
+	if (gpio_set)
+		ret = xr_set_reg_uart(port, XR21V141X_REG_GPIO_SET, gpio_set);
+
+	return ret;
+}
+
+static int xr_tiocmset(struct tty_struct *tty,
+		       unsigned int set, unsigned int clear)
+{
+	struct usb_serial_port *port = tty->driver_data;
+
+	return xr_tiocmset_port(port, set, clear);
+}
+
+static void xr_dtr_rts(struct usb_serial_port *port, int on)
+{
+	if (on)
+		xr_tiocmset_port(port, TIOCM_DTR | TIOCM_RTS, 0);
+	else
+		xr_tiocmset_port(port, 0, TIOCM_DTR | TIOCM_RTS);
+}
+
+static void xr_set_termios(struct tty_struct *tty,
+			   struct usb_serial_port *port,
+			   struct ktermios *old_termios)
+{
+	struct ktermios *termios = &tty->termios;
+	int ret;
+	u8 bits = 0;
+
+	if ((old_termios && tty->termios.c_ospeed != old_termios->c_ospeed) ||
+	    !old_termios)
+		xr_set_baudrate(tty, port);
+
+	switch (C_CSIZE(tty)) {
+	case CS5:
+	fallthrough;
+	case CS6:
+		/* CS5 and CS6 are not supported, so just restore old setting */
+		termios->c_cflag &= ~CSIZE;
+		if (old_termios)
+			termios->c_cflag |= old_termios->c_cflag & CSIZE;
+		else
+			bits |= XR21V141X_UART_DATA_8;
+		break;
+	case CS7:
+		bits |= XR21V141X_UART_DATA_7;
+		break;
+	case CS8:
+	fallthrough;
+	default:
+		bits |= XR21V141X_UART_DATA_8;
+		break;
+	}
+
+	if (C_PARENB(tty)) {
+		if (C_CMSPAR(tty)) {
+			if (C_PARODD(tty))
+				bits |= XR21V141X_UART_PARITY_MARK <<
+					XR21V141X_UART_PARITY_SHIFT;
+			else
+				bits |= XR21V141X_UART_PARITY_SPACE <<
+					XR21V141X_UART_PARITY_SHIFT;
+		} else {
+			if (C_PARODD(tty))
+				bits |= XR21V141X_UART_PARITY_ODD <<
+					XR21V141X_UART_PARITY_SHIFT;
+			else
+				bits |= XR21V141X_UART_PARITY_EVEN <<
+					XR21V141X_UART_PARITY_SHIFT;
+		}
+	}
+
+	if (C_CSTOPB(tty))
+		bits |= XR21V141X_UART_STOP_2 << XR21V141X_UART_STOP_SHIFT;
+	else
+		bits |= XR21V141X_UART_STOP_1 << XR21V141X_UART_STOP_SHIFT;
+
+	ret = xr_set_reg_uart(port, XR21V141X_REG_FORMAT, bits);
+	if (ret)
+		return;
+
+	/* If baud rate is B0, clear DTR and RTS */
+	if (C_BAUD(tty) == B0)
+		xr_dtr_rts(port, 0);
+
+	xr_set_flow_mode(tty, port);
+}
+
+static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
+{
+	int ret;
+
+	ret = xr_uart_enable(port);
+	if (ret) {
+		dev_err(&port->dev, "Failed to enable UART\n");
+		return ret;
+	}
+
+	/* Setup termios */
+	if (tty)
+		xr_set_termios(tty, port, NULL);
+
+	ret = usb_serial_generic_open(tty, port);
+	if (ret) {
+		xr_uart_disable(port);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void xr_close(struct usb_serial_port *port)
+{
+	usb_serial_generic_close(port);
+
+	xr_uart_disable(port);
+}
+
+static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
+{
+	struct usb_device *usb_dev = interface_to_usbdev(serial->interface);
+	struct usb_driver *driver = serial->type->usb_driver;
+	struct usb_interface *control_interface;
+	int ret;
+
+	/* Don't bind to control interface */
+	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
+		return -ENODEV;
+
+	/* But claim the control interface during data interface probe */
+	control_interface = usb_ifnum_to_if(usb_dev, 0);
+	ret = usb_driver_claim_interface(driver, control_interface, NULL);
+	if (ret) {
+		dev_err(&serial->interface->dev, "Can't claim control interface\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int xr_tiocmget(struct tty_struct *tty)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	u8 status;
+	int ret;
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_STATUS, &status);
+	if (ret)
+		return ret;
+
+	/*
+	 * Modem control pins are active low, so reading '0' means it is active
+	 * and '1' means not active.
+	 */
+	ret = ((status & XR21V141X_UART_MODE_DTR) ? 0 : TIOCM_DTR) |
+	      ((status & XR21V141X_UART_MODE_RTS) ? 0 : TIOCM_RTS) |
+	      ((status & XR21V141X_UART_MODE_CTS) ? 0 : TIOCM_CTS) |
+	      ((status & XR21V141X_UART_MODE_DSR) ? 0 : TIOCM_DSR) |
+	      ((status & XR21V141X_UART_MODE_RI) ? 0 : TIOCM_RI) |
+	      ((status & XR21V141X_UART_MODE_CD) ? 0 : TIOCM_CD);
+
+	return ret;
+}
+
+static void xr_break_ctl(struct tty_struct *tty, int break_state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	u8 state;
+
+	if (break_state == 0)
+		state = XR21V141X_UART_BREAK_OFF;
+	else
+		state = XR21V141X_UART_BREAK_ON;
+
+	dev_dbg(&port->dev, "Turning break %s\n",
+		state == XR21V141X_UART_BREAK_OFF ? "off" : "on");
+	xr_set_reg_uart(port, XR21V141X_REG_TX_BREAK, state);
+}
+
+static int xr_port_probe(struct usb_serial_port *port)
+{
+	return 0;
+}
+
+static int xr_port_remove(struct usb_serial_port *port)
+{
+	return 0;
+}
+
+static const struct usb_device_id id_table[] = {
+	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
+	{ }
+};
+MODULE_DEVICE_TABLE(usb, id_table);
+
+static struct usb_serial_driver xr_device = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name =	"xr_serial",
+	},
+	.id_table		= id_table,
+	.num_ports		= 1,
+	.open			= xr_open,
+	.close			= xr_close,
+	.break_ctl		= xr_break_ctl,
+	.set_termios		= xr_set_termios,
+	.tiocmget		= xr_tiocmget,
+	.tiocmset		= xr_tiocmset,
+	.probe			= xr_probe,
+	.port_probe		= xr_port_probe,
+	.port_remove		= xr_port_remove,
+	.dtr_rts		= xr_dtr_rts
+};
+
+static struct usb_serial_driver * const serial_drivers[] = {
+	&xr_device, NULL
+};
+
+module_usb_serial_driver(serial_drivers, id_table);
+
+MODULE_AUTHOR("Manivannan Sadhasivam <mani@kernel.org>");
+MODULE_DESCRIPTION("MaxLinear/Exar USB to Serial driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Johan Hovold]

On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:
> Add support for MaxLinear/Exar USB to Serial converters. This driver
> only supports XR21V141X series but it can be extended to other series
> from Exar as well in future.

There are still a few issues with this driver, but I really don't want
to have to review it again in a couple of months so I've fixed it up
myself this time.

The trivial stuff I folded into this patch, and I'll submit a follow-on
series for the rest.

> This driver is inspired from the initial one submitted by Patong Yang:
> 
> https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop

Using /r/ is shorter.

> While the initial driver was a custom tty USB driver exposing whole
> new serial interface ttyXRUSBn, this version is completely based on USB
> serial core thus exposing the interfaces as ttyUSBn. This will avoid
> the overhead of exposing a new USB serial interface which the userspace
> tools are unaware of.

I added a comment about the two driver modes here (ACM and "custom").

> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/usb/serial/Kconfig     |   9 +
>  drivers/usb/serial/Makefile    |   1 +
>  drivers/usb/serial/xr_serial.c | 599 +++++++++++++++++++++++++++++++++
>  3 files changed, 609 insertions(+)
>  create mode 100644 drivers/usb/serial/xr_serial.c
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index 4007fa25a8ff..32595acb1d1d 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -644,6 +644,15 @@ config USB_SERIAL_UPD78F0730
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called upd78f0730.
>  
> +config USB_SERIAL_XR
> +	tristate "USB MaxLinear/Exar USB to Serial driver"
> +	help
> +	  Say Y here if you want to use MaxLinear/Exar USB to Serial converter
> +	  devices.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called xr_serial.
> +
>  config USB_SERIAL_DEBUG
>  	tristate "USB Debugging Device"
>  	help
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index 2d491e434f11..4f69c2a3aff3 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -62,4 +62,5 @@ obj-$(CONFIG_USB_SERIAL_VISOR)			+= visor.o
>  obj-$(CONFIG_USB_SERIAL_WISHBONE)		+= wishbone-serial.o
>  obj-$(CONFIG_USB_SERIAL_WHITEHEAT)		+= whiteheat.o
>  obj-$(CONFIG_USB_SERIAL_XIRCOM)			+= keyspan_pda.o
> +obj-$(CONFIG_USB_SERIAL_XR)			+= xr_serial.o
>  obj-$(CONFIG_USB_SERIAL_XSENS_MT)		+= xsens_mt.o
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> new file mode 100644
> index 000000000000..e166916554d6
> --- /dev/null
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -0,0 +1,599 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MaxLinear/Exar USB to Serial driver
> + *
> + * Based on the initial driver written by Patong Yang:
> + * https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> + *
> + * Copyright (c) 2018 Patong Yang <patong.mxl@gmail.com>
> + * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>

I moved your copyright notice under "MaxLinear..." so that it's clear
who claims what copyright.

> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +
> +struct xr_txrx_clk_mask {
> +	u16 tx;
> +	u16 rx0;
> +	u16 rx1;
> +};
> +
> +#define XR_INT_OSC_HZ			48000000U
> +#define XR21V141X_MIN_SPEED		46U
> +#define XR21V141X_MAX_SPEED		XR_INT_OSC_HZ
> +
> +/* USB Requests */
> +#define XR21V141X_SET_REQ		0
> +#define XR21V141X_GET_REQ		1
> +
> +#define XR21V141X_CLOCK_DIVISOR_0	0x04
> +#define XR21V141X_CLOCK_DIVISOR_1	0x05
> +#define XR21V141X_CLOCK_DIVISOR_2	0x06
> +#define XR21V141X_TX_CLOCK_MASK_0	0x07
> +#define XR21V141X_TX_CLOCK_MASK_1	0x08
> +#define XR21V141X_RX_CLOCK_MASK_0	0x09
> +#define XR21V141X_RX_CLOCK_MASK_1	0x0a
> +
> +/* XR21V141X register blocks */
> +#define XR21V141X_UART_REG_BLOCK	0
> +#define XR21V141X_UM_REG_BLOCK		4
> +#define XR21V141X_UART_CUSTOM_BLOCK	0x66
> +
> +/* XR21V141X UART Manager Registers */
> +#define XR21V141X_UM_FIFO_ENABLE_REG	0x10
> +#define XR21V141X_UM_ENABLE_TX_FIFO	0x01
> +#define XR21V141X_UM_ENABLE_RX_FIFO	0x02
> +
> +#define XR21V141X_UM_RX_FIFO_RESET	0x18
> +#define XR21V141X_UM_TX_FIFO_RESET	0x1c
> +
> +#define XR21V141X_UART_ENABLE_TX	0x1
> +#define XR21V141X_UART_ENABLE_RX	0x2
> +
> +#define XR21V141X_UART_MODE_RI		BIT(0)
> +#define XR21V141X_UART_MODE_CD		BIT(1)
> +#define XR21V141X_UART_MODE_DSR		BIT(2)
> +#define XR21V141X_UART_MODE_DTR		BIT(3)
> +#define XR21V141X_UART_MODE_CTS		BIT(4)
> +#define XR21V141X_UART_MODE_RTS		BIT(5)
> +
> +#define XR21V141X_UART_BREAK_ON		0xff
> +#define XR21V141X_UART_BREAK_OFF	0
> +
> +#define XR21V141X_UART_DATA_MASK	GENMASK(3, 0)
> +#define XR21V141X_UART_DATA_7		0x7
> +#define XR21V141X_UART_DATA_8		0x8
> +
> +#define XR21V141X_UART_PARITY_MASK	GENMASK(6, 4)
> +#define XR21V141X_UART_PARITY_SHIFT	0x4
> +#define XR21V141X_UART_PARITY_NONE	0x0
> +#define XR21V141X_UART_PARITY_ODD	0x1
> +#define XR21V141X_UART_PARITY_EVEN	0x2
> +#define XR21V141X_UART_PARITY_MARK	0x3
> +#define XR21V141X_UART_PARITY_SPACE	0x4
> +
> +#define XR21V141X_UART_STOP_MASK	BIT(7)
> +#define XR21V141X_UART_STOP_SHIFT	0x7
> +#define XR21V141X_UART_STOP_1		0x0
> +#define XR21V141X_UART_STOP_2		0x1
> +
> +#define XR21V141X_UART_FLOW_MODE_NONE	0x0
> +#define XR21V141X_UART_FLOW_MODE_HW	0x1
> +#define XR21V141X_UART_FLOW_MODE_SW	0x2
> +
> +#define XR21V141X_UART_MODE_GPIO_MASK	GENMASK(2, 0)
> +#define XR21V141X_UART_MODE_RTS_CTS	0x1
> +#define XR21V141X_UART_MODE_DTR_DSR	0x2
> +#define XR21V141X_UART_MODE_RS485	0x3
> +#define XR21V141X_UART_MODE_RS485_ADDR	0x4
> +
> +#define XR21V141X_REG_ENABLE		0x03
> +#define XR21V141X_REG_FORMAT		0x0b
> +#define XR21V141X_REG_FLOW_CTRL		0x0c
> +#define XR21V141X_REG_XON_CHAR		0x10
> +#define XR21V141X_REG_XOFF_CHAR		0x11
> +#define XR21V141X_REG_LOOPBACK		0x12
> +#define XR21V141X_REG_TX_BREAK		0x14
> +#define XR21V141X_REG_RS845_DELAY	0x15
> +#define XR21V141X_REG_GPIO_MODE		0x1a
> +#define XR21V141X_REG_GPIO_DIR		0x1b
> +#define XR21V141X_REG_GPIO_INT_MASK	0x1c
> +#define XR21V141X_REG_GPIO_SET		0x1d
> +#define XR21V141X_REG_GPIO_CLR		0x1e
> +#define XR21V141X_REG_GPIO_STATUS	0x1f
> +
> +static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg,
> +		      u8 val)

No need for line breaks.

> +{
> +	struct usb_serial *serial = port->serial;
> +	int ret;
> +
> +	ret = usb_control_msg(serial->dev,
> +			      usb_sndctrlpipe(serial->dev, 0),
> +			      XR21V141X_SET_REQ,
> +			      USB_DIR_OUT | USB_TYPE_VENDOR, val,

The request-type should include USB_RECIP_DEVICE (0) as well.

> +			      reg | (block << 8), NULL, 0,
> +			      USB_CTRL_SET_TIMEOUT);
> +	if (ret < 0) {
> +		dev_err(&port->dev, "Failed to set reg 0x%02x: %d\n", reg, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xr_get_reg(struct usb_serial_port *port, u8 block, u8 reg,
> +		      u8 *val)
> +{
> +	struct usb_serial *serial = port->serial;
> +	u8 *dmabuf;
> +	int ret;
> +
> +	dmabuf = kmalloc(1, GFP_KERNEL);
> +	if (!dmabuf)
> +		return -ENOMEM;
> +
> +	ret = usb_control_msg(serial->dev,
> +			      usb_rcvctrlpipe(serial->dev, 0),
> +			      XR21V141X_GET_REQ,
> +			      USB_DIR_IN | USB_TYPE_VENDOR, 0,
> +			      reg | (block << 8), dmabuf, 1,
> +			      USB_CTRL_GET_TIMEOUT);
> +	if (ret == 1) {
> +		*val = *dmabuf;
> +		ret = 0;
> +	} else {
> +		dev_err(&port->dev, "Failed to get reg 0x%02x: %d\n", reg, ret);
> +		if (ret >= 0)
> +			ret = -EIO;
> +	}
> +
> +	kfree(dmabuf);
> +
> +	return ret;
> +}
> +
> +static int xr_set_reg_uart(struct usb_serial_port *port, u8 reg, u8 val)
> +{
> +	return xr_set_reg(port, XR21V141X_UART_REG_BLOCK, reg, val);
> +}
> +
> +static int xr_get_reg_uart(struct usb_serial_port *port, u8 reg, u8 *val)
> +{
> +	return xr_get_reg(port, XR21V141X_UART_REG_BLOCK, reg, val);
> +}
> +
> +static int xr_set_reg_um(struct usb_serial_port *port, u8 reg, u8 val)
> +{
> +	return xr_set_reg(port, XR21V141X_UM_REG_BLOCK, reg, val);
> +}
> +
> +/* Tx and Rx clock mask values obtained from section 3.3.4 of datasheet */
> +static const struct xr_txrx_clk_mask xr21v141x_txrx_clk_masks[] = {
> +	{ 0x000, 0x000, 0x000 },
> +	{ 0x000, 0x000, 0x000 },
> +	{ 0x100, 0x000, 0x100 },
> +	{ 0x020, 0x400, 0x020 },
> +	{ 0x010, 0x100, 0x010 },
> +	{ 0x208, 0x040, 0x208 },
> +	{ 0x104, 0x820, 0x108 },
> +	{ 0x844, 0x210, 0x884 },
> +	{ 0x444, 0x110, 0x444 },
> +	{ 0x122, 0x888, 0x224 },
> +	{ 0x912, 0x448, 0x924 },
> +	{ 0x492, 0x248, 0x492 },
> +	{ 0x252, 0x928, 0x292 },
> +	{ 0x94a, 0x4a4, 0xa52 },
> +	{ 0x52a, 0xaa4, 0x54a },
> +	{ 0xaaa, 0x954, 0x4aa },
> +	{ 0xaaa, 0x554, 0xaaa },
> +	{ 0x555, 0xad4, 0x5aa },
> +	{ 0xb55, 0xab4, 0x55a },
> +	{ 0x6b5, 0x5ac, 0xb56 },
> +	{ 0x5b5, 0xd6c, 0x6d6 },
> +	{ 0xb6d, 0xb6a, 0xdb6 },
> +	{ 0x76d, 0x6da, 0xbb6 },
> +	{ 0xedd, 0xdda, 0x76e },
> +	{ 0xddd, 0xbba, 0xeee },
> +	{ 0x7bb, 0xf7a, 0xdde },
> +	{ 0xf7b, 0xef6, 0x7de },
> +	{ 0xdf7, 0xbf6, 0xf7e },
> +	{ 0x7f7, 0xfee, 0xefe },
> +	{ 0xfdf, 0xfbe, 0x7fe },
> +	{ 0xf7f, 0xefe, 0xffe },
> +	{ 0xfff, 0xffe, 0xffd },
> +};
> +
> +static int xr_set_baudrate(struct tty_struct *tty,
> +			   struct usb_serial_port *port)
> +{
> +	u32 divisor, baud, idx;
> +	u16 tx_mask, rx_mask;
> +	int ret;
> +
> +	baud = clamp(tty->termios.c_ospeed, XR21V141X_MIN_SPEED,
> +		     XR21V141X_MAX_SPEED);

You shouldn't report back anything else but B0 if that's requested, the
actual rate can be left unchanged.

> +	divisor = XR_INT_OSC_HZ / baud;
> +	idx = ((32 * XR_INT_OSC_HZ) / baud) & 0x1f;
> +	tx_mask = xr21v141x_txrx_clk_masks[idx].tx;
> +
> +	if (divisor & 1)

Nit: 0x01 since bitmask.

> +		rx_mask = xr21v141x_txrx_clk_masks[idx].rx1;
> +	else
> +		rx_mask = xr21v141x_txrx_clk_masks[idx].rx0;
> +
> +	dev_dbg(&port->dev, "Setting baud rate: %u\n", baud);
> +	/*
> +	 * XR21V141X uses fractional baud rate generator with 48MHz internal
> +	 * oscillator and 19-bit programmable divisor. So theoretically it can
> +	 * generate most commonly used baud rates with high accuracy.
> +	 */
> +	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_0, (divisor & 0xff));
> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_1, ((divisor >>  8) & 0xff));

I broke these long lines again.

> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_2, ((divisor >> 16) & 0xff));
> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_TX_CLOCK_MASK_0, (tx_mask & 0xff));
> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_TX_CLOCK_MASK_1, ((tx_mask >>  8) & 0xff));
> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_RX_CLOCK_MASK_0, (rx_mask & 0xff));
> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_RX_CLOCK_MASK_1, ((rx_mask >>  8) & 0xff));
> +
> +	tty_encode_baud_rate(tty, baud, baud);
> +
> +	return ret;
> +}
> +
> +/*
> + * According to datasheet, below is the recommended sequence for enabling UART
> + * module in XR21V141X:
> + *
> + * Enable Tx FIFO
> + * Enable Tx and Rx
> + * Enable Rx FIFO
> + */
> +static int xr_uart_enable(struct usb_serial_port *port)
> +{
> +	int ret;
> +
> +	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG, XR21V141X_UM_ENABLE_TX_FIFO);
> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_REG_ENABLE,
> +			      XR21V141X_UART_ENABLE_TX | XR21V141X_UART_ENABLE_RX);
> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG,
> +			    XR21V141X_UM_ENABLE_TX_FIFO | XR21V141X_UM_ENABLE_RX_FIFO);
> +
> +	if (ret)
> +		xr_set_reg_uart(port, XR21V141X_REG_ENABLE, 0);
> +
> +	return ret;
> +}
> +
> +static int xr_uart_disable(struct usb_serial_port *port)
> +{
> +	int ret;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_REG_ENABLE, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG, 0);
> +
> +	return ret;
> +}
> +
> +static void xr_set_flow_mode(struct tty_struct *tty,
> +			     struct usb_serial_port *port)
> +{
> +	unsigned int cflag = tty->termios.c_cflag;
> +	int ret;
> +	u8 flow, gpio_mode;
> +
> +	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_MODE, &gpio_mode);
> +	if (ret)
> +		return;
> +
> +	if (cflag & CRTSCTS) {

C_CRTSCTS(tty)

> +		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
> +
> +		/*
> +		 * RTS/CTS is the default flow control mode, so set GPIO mode
> +		 * for controlling the pins manually by default.
> +		 */
> +		gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK;

This needs to be done before the conditional so that GPIO mode is again
selected when disabling flow control, otherwise your break RTS control.

Also you never configure DTR/RTS as outputs despite all pins being
inputs by default according to the datasheet. Effectively, DTR control
is currently broken and RTS can only be used for automatic flow control.

Did you not test these signals separately?

> +		gpio_mode |= XR21V141X_UART_MODE_RTS_CTS;
> +		flow = XR21V141X_UART_FLOW_MODE_HW;
> +	} else if (I_IXON(tty)) {
> +		u8 start_char = START_CHAR(tty);
> +		u8 stop_char = STOP_CHAR(tty);
> +
> +		dev_dbg(&port->dev, "Enabling sw flow ctrl\n");
> +		flow = XR21V141X_UART_FLOW_MODE_SW;
> +
> +		xr_set_reg_uart(port, XR21V141X_REG_XON_CHAR, start_char);
> +		xr_set_reg_uart(port, XR21V141X_REG_XOFF_CHAR, stop_char);
> +	} else {
> +		dev_dbg(&port->dev, "Disabling flow ctrl\n");
> +		flow = XR21V141X_UART_FLOW_MODE_NONE;
> +	}
> +
> +	/*
> +	 * As per the datasheet, UART needs to be disabled while writing to
> +	 * FLOW_CONTROL register.
> +	 */
> +	xr_uart_disable(port);
> +	xr_set_reg_uart(port, XR21V141X_REG_FLOW_CTRL, flow);
> +	xr_uart_enable(port);
> +
> +	xr_set_reg_uart(port, XR21V141X_REG_GPIO_MODE, gpio_mode);
> +}
> +
> +static int xr_tiocmset_port(struct usb_serial_port *port,
> +			    unsigned int set, unsigned int clear)
> +{
> +	u8 gpio_set = 0;
> +	u8 gpio_clr = 0;
> +	int ret = 0;
> +
> +	/* Modem control pins are active low, so set & clr are swapped */
> +	if (set & TIOCM_RTS)
> +		gpio_clr |= XR21V141X_UART_MODE_RTS;
> +	if (set & TIOCM_DTR)
> +		gpio_clr |= XR21V141X_UART_MODE_DTR;
> +	if (clear & TIOCM_RTS)
> +		gpio_set |= XR21V141X_UART_MODE_RTS;
> +	if (clear & TIOCM_DTR)
> +		gpio_set |= XR21V141X_UART_MODE_DTR;
> +
> +	/* Writing '0' to gpio_{set/clr} bits has no effect, so no need to do */
> +	if (gpio_clr)
> +		ret = xr_set_reg_uart(port, XR21V141X_REG_GPIO_CLR, gpio_clr);
> +
> +	if (gpio_set)
> +		ret = xr_set_reg_uart(port, XR21V141X_REG_GPIO_SET, gpio_set);
> +
> +	return ret;
> +}
> +
> +static int xr_tiocmset(struct tty_struct *tty,
> +		       unsigned int set, unsigned int clear)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +
> +	return xr_tiocmset_port(port, set, clear);
> +}
> +
> +static void xr_dtr_rts(struct usb_serial_port *port, int on)
> +{
> +	if (on)
> +		xr_tiocmset_port(port, TIOCM_DTR | TIOCM_RTS, 0);
> +	else
> +		xr_tiocmset_port(port, 0, TIOCM_DTR | TIOCM_RTS);
> +}
> +
> +static void xr_set_termios(struct tty_struct *tty,
> +			   struct usb_serial_port *port,
> +			   struct ktermios *old_termios)
> +{
> +	struct ktermios *termios = &tty->termios;
> +	int ret;
> +	u8 bits = 0;
> +
> +	if ((old_termios && tty->termios.c_ospeed != old_termios->c_ospeed) ||
> +	    !old_termios)

This can be simplified.

> +		xr_set_baudrate(tty, port);
> +
> +	switch (C_CSIZE(tty)) {
> +	case CS5:
> +	fallthrough;

No need for fallthrough for empty cases.

> +	case CS6:
> +		/* CS5 and CS6 are not supported, so just restore old setting */
> +		termios->c_cflag &= ~CSIZE;
> +		if (old_termios)
> +			termios->c_cflag |= old_termios->c_cflag & CSIZE;
> +		else
> +			bits |= XR21V141X_UART_DATA_8;
> +		break;
> +	case CS7:
> +		bits |= XR21V141X_UART_DATA_7;
> +		break;
> +	case CS8:
> +	fallthrough;
> +	default:
> +		bits |= XR21V141X_UART_DATA_8;
> +		break;
> +	}
> +
> +	if (C_PARENB(tty)) {
> +		if (C_CMSPAR(tty)) {
> +			if (C_PARODD(tty))
> +				bits |= XR21V141X_UART_PARITY_MARK <<
> +					XR21V141X_UART_PARITY_SHIFT;

I'd prefer just shifting the values when defining them, which is more
consistent with how CSIZE is handled too.

> +			else
> +				bits |= XR21V141X_UART_PARITY_SPACE <<
> +					XR21V141X_UART_PARITY_SHIFT;
> +		} else {
> +			if (C_PARODD(tty))
> +				bits |= XR21V141X_UART_PARITY_ODD <<
> +					XR21V141X_UART_PARITY_SHIFT;
> +			else
> +				bits |= XR21V141X_UART_PARITY_EVEN <<
> +					XR21V141X_UART_PARITY_SHIFT;
> +		}
> +	}
> +
> +	if (C_CSTOPB(tty))
> +		bits |= XR21V141X_UART_STOP_2 << XR21V141X_UART_STOP_SHIFT;
> +	else
> +		bits |= XR21V141X_UART_STOP_1 << XR21V141X_UART_STOP_SHIFT;
> +
> +	ret = xr_set_reg_uart(port, XR21V141X_REG_FORMAT, bits);
> +	if (ret)
> +		return;
> +
> +	/* If baud rate is B0, clear DTR and RTS */
> +	if (C_BAUD(tty) == B0)
> +		xr_dtr_rts(port, 0);

This isn't sufficient; RTS will not be dropped when CRTSCTS is enabled,
and we should reassert the lines when moving from B0 too.

> +
> +	xr_set_flow_mode(tty, port);
> +}
> +
> +static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	int ret;
> +
> +	ret = xr_uart_enable(port);
> +	if (ret) {
> +		dev_err(&port->dev, "Failed to enable UART\n");
> +		return ret;
> +	}
> +
> +	/* Setup termios */
> +	if (tty)
> +		xr_set_termios(tty, port, NULL);
> +
> +	ret = usb_serial_generic_open(tty, port);
> +	if (ret) {
> +		xr_uart_disable(port);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void xr_close(struct usb_serial_port *port)
> +{
> +	usb_serial_generic_close(port);
> +
> +	xr_uart_disable(port);
> +}
> +
> +static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
> +{
> +	struct usb_device *usb_dev = interface_to_usbdev(serial->interface);

You can just use serial->dev directly.

> +	struct usb_driver *driver = serial->type->usb_driver;
> +	struct usb_interface *control_interface;
> +	int ret;
> +
> +	/* Don't bind to control interface */
> +	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
> +		return -ENODEV;
> +
> +	/* But claim the control interface during data interface probe */
> +	control_interface = usb_ifnum_to_if(usb_dev, 0);

You must check for NULL here since a device may not have an interface 0
in case you'll oops here. This can be triggered by a malicious device or
when fuzzing USB descriptors.

> +	ret = usb_driver_claim_interface(driver, control_interface, NULL);
> +	if (ret) {
> +		dev_err(&serial->interface->dev, "Can't claim control interface\n");
> +		return ret;
> +	}

And you must release the control interface again when unbinding the
driver to avoid leaking resources and to allow the driver to be rebound.

> +
> +	return 0;
> +}
> +
> +static int xr_tiocmget(struct tty_struct *tty)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	u8 status;
> +	int ret;
> +
> +	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_STATUS, &status);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Modem control pins are active low, so reading '0' means it is active
> +	 * and '1' means not active.
> +	 */
> +	ret = ((status & XR21V141X_UART_MODE_DTR) ? 0 : TIOCM_DTR) |
> +	      ((status & XR21V141X_UART_MODE_RTS) ? 0 : TIOCM_RTS) |
> +	      ((status & XR21V141X_UART_MODE_CTS) ? 0 : TIOCM_CTS) |
> +	      ((status & XR21V141X_UART_MODE_DSR) ? 0 : TIOCM_DSR) |
> +	      ((status & XR21V141X_UART_MODE_RI) ? 0 : TIOCM_RI) |
> +	      ((status & XR21V141X_UART_MODE_CD) ? 0 : TIOCM_CD);
> +
> +	return ret;
> +}
> +
> +static void xr_break_ctl(struct tty_struct *tty, int break_state)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	u8 state;
> +
> +	if (break_state == 0)
> +		state = XR21V141X_UART_BREAK_OFF;
> +	else
> +		state = XR21V141X_UART_BREAK_ON;
> +
> +	dev_dbg(&port->dev, "Turning break %s\n",
> +		state == XR21V141X_UART_BREAK_OFF ? "off" : "on");
> +	xr_set_reg_uart(port, XR21V141X_REG_TX_BREAK, state);
> +}
> +
> +static int xr_port_probe(struct usb_serial_port *port)
> +{
> +	return 0;
> +}
> +
> +static int xr_port_remove(struct usb_serial_port *port)
> +{
> +	return 0;
> +}

Again, don't add these until you need them.

> +
> +static const struct usb_device_id id_table[] = {
> +	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +static struct usb_serial_driver xr_device = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name =	"xr_serial",
> +	},
> +	.id_table		= id_table,
> +	.num_ports		= 1,
> +	.open			= xr_open,
> +	.close			= xr_close,
> +	.break_ctl		= xr_break_ctl,
> +	.set_termios		= xr_set_termios,
> +	.tiocmget		= xr_tiocmget,
> +	.tiocmset		= xr_tiocmset,
> +	.probe			= xr_probe,
> +	.port_probe		= xr_port_probe,
> +	.port_remove		= xr_port_remove,
> +	.dtr_rts		= xr_dtr_rts
> +};
> +
> +static struct usb_serial_driver * const serial_drivers[] = {
> +	&xr_device, NULL
> +};
> +
> +module_usb_serial_driver(serial_drivers, id_table);
> +
> +MODULE_AUTHOR("Manivannan Sadhasivam <mani@kernel.org>");
> +MODULE_DESCRIPTION("MaxLinear/Exar USB to Serial driver");
> +MODULE_LICENSE("GPL");

Looks good overall otherwise.

I've applied this one now, and will follow up with a series addressing
the non-trivial bits pointed out below.

Johan

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Manivannan Sadhasivam]

On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:
> On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:
> > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > only supports XR21V141X series but it can be extended to other series
> > from Exar as well in future.
> 
> There are still a few issues with this driver, but I really don't want
> to have to review it again in a couple of months so I've fixed it up
> myself this time.
> 
> The trivial stuff I folded into this patch, and I'll submit a follow-on
> series for the rest.
> 

Many thanks for doing this! These days it is really difficult to find time for
spare time stuffs.

And all of your fixes makes sense to me.

Thanks,
Mani

> > This driver is inspired from the initial one submitted by Patong Yang:
> > 
> > https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> 
> Using /r/ is shorter.
> 
> > While the initial driver was a custom tty USB driver exposing whole
> > new serial interface ttyXRUSBn, this version is completely based on USB
> > serial core thus exposing the interfaces as ttyUSBn. This will avoid
> > the overhead of exposing a new USB serial interface which the userspace
> > tools are unaware of.
> 
> I added a comment about the two driver modes here (ACM and "custom").
> 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/usb/serial/Kconfig     |   9 +
> >  drivers/usb/serial/Makefile    |   1 +
> >  drivers/usb/serial/xr_serial.c | 599 +++++++++++++++++++++++++++++++++
> >  3 files changed, 609 insertions(+)
> >  create mode 100644 drivers/usb/serial/xr_serial.c
> > 
> > diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> > index 4007fa25a8ff..32595acb1d1d 100644
> > --- a/drivers/usb/serial/Kconfig
> > +++ b/drivers/usb/serial/Kconfig
> > @@ -644,6 +644,15 @@ config USB_SERIAL_UPD78F0730
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called upd78f0730.
> >  
> > +config USB_SERIAL_XR
> > +	tristate "USB MaxLinear/Exar USB to Serial driver"
> > +	help
> > +	  Say Y here if you want to use MaxLinear/Exar USB to Serial converter
> > +	  devices.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called xr_serial.
> > +
> >  config USB_SERIAL_DEBUG
> >  	tristate "USB Debugging Device"
> >  	help
> > diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> > index 2d491e434f11..4f69c2a3aff3 100644
> > --- a/drivers/usb/serial/Makefile
> > +++ b/drivers/usb/serial/Makefile
> > @@ -62,4 +62,5 @@ obj-$(CONFIG_USB_SERIAL_VISOR)			+= visor.o
> >  obj-$(CONFIG_USB_SERIAL_WISHBONE)		+= wishbone-serial.o
> >  obj-$(CONFIG_USB_SERIAL_WHITEHEAT)		+= whiteheat.o
> >  obj-$(CONFIG_USB_SERIAL_XIRCOM)			+= keyspan_pda.o
> > +obj-$(CONFIG_USB_SERIAL_XR)			+= xr_serial.o
> >  obj-$(CONFIG_USB_SERIAL_XSENS_MT)		+= xsens_mt.o
> > diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> > new file mode 100644
> > index 000000000000..e166916554d6
> > --- /dev/null
> > +++ b/drivers/usb/serial/xr_serial.c
> > @@ -0,0 +1,599 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * MaxLinear/Exar USB to Serial driver
> > + *
> > + * Based on the initial driver written by Patong Yang:
> > + * https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> > + *
> > + * Copyright (c) 2018 Patong Yang <patong.mxl@gmail.com>
> > + * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>
> 
> I moved your copyright notice under "MaxLinear..." so that it's clear
> who claims what copyright.
> 
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/tty.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/serial.h>
> > +
> > +struct xr_txrx_clk_mask {
> > +	u16 tx;
> > +	u16 rx0;
> > +	u16 rx1;
> > +};
> > +
> > +#define XR_INT_OSC_HZ			48000000U
> > +#define XR21V141X_MIN_SPEED		46U
> > +#define XR21V141X_MAX_SPEED		XR_INT_OSC_HZ
> > +
> > +/* USB Requests */
> > +#define XR21V141X_SET_REQ		0
> > +#define XR21V141X_GET_REQ		1
> > +
> > +#define XR21V141X_CLOCK_DIVISOR_0	0x04
> > +#define XR21V141X_CLOCK_DIVISOR_1	0x05
> > +#define XR21V141X_CLOCK_DIVISOR_2	0x06
> > +#define XR21V141X_TX_CLOCK_MASK_0	0x07
> > +#define XR21V141X_TX_CLOCK_MASK_1	0x08
> > +#define XR21V141X_RX_CLOCK_MASK_0	0x09
> > +#define XR21V141X_RX_CLOCK_MASK_1	0x0a
> > +
> > +/* XR21V141X register blocks */
> > +#define XR21V141X_UART_REG_BLOCK	0
> > +#define XR21V141X_UM_REG_BLOCK		4
> > +#define XR21V141X_UART_CUSTOM_BLOCK	0x66
> > +
> > +/* XR21V141X UART Manager Registers */
> > +#define XR21V141X_UM_FIFO_ENABLE_REG	0x10
> > +#define XR21V141X_UM_ENABLE_TX_FIFO	0x01
> > +#define XR21V141X_UM_ENABLE_RX_FIFO	0x02
> > +
> > +#define XR21V141X_UM_RX_FIFO_RESET	0x18
> > +#define XR21V141X_UM_TX_FIFO_RESET	0x1c
> > +
> > +#define XR21V141X_UART_ENABLE_TX	0x1
> > +#define XR21V141X_UART_ENABLE_RX	0x2
> > +
> > +#define XR21V141X_UART_MODE_RI		BIT(0)
> > +#define XR21V141X_UART_MODE_CD		BIT(1)
> > +#define XR21V141X_UART_MODE_DSR		BIT(2)
> > +#define XR21V141X_UART_MODE_DTR		BIT(3)
> > +#define XR21V141X_UART_MODE_CTS		BIT(4)
> > +#define XR21V141X_UART_MODE_RTS		BIT(5)
> > +
> > +#define XR21V141X_UART_BREAK_ON		0xff
> > +#define XR21V141X_UART_BREAK_OFF	0
> > +
> > +#define XR21V141X_UART_DATA_MASK	GENMASK(3, 0)
> > +#define XR21V141X_UART_DATA_7		0x7
> > +#define XR21V141X_UART_DATA_8		0x8
> > +
> > +#define XR21V141X_UART_PARITY_MASK	GENMASK(6, 4)
> > +#define XR21V141X_UART_PARITY_SHIFT	0x4
> > +#define XR21V141X_UART_PARITY_NONE	0x0
> > +#define XR21V141X_UART_PARITY_ODD	0x1
> > +#define XR21V141X_UART_PARITY_EVEN	0x2
> > +#define XR21V141X_UART_PARITY_MARK	0x3
> > +#define XR21V141X_UART_PARITY_SPACE	0x4
> > +
> > +#define XR21V141X_UART_STOP_MASK	BIT(7)
> > +#define XR21V141X_UART_STOP_SHIFT	0x7
> > +#define XR21V141X_UART_STOP_1		0x0
> > +#define XR21V141X_UART_STOP_2		0x1
> > +
> > +#define XR21V141X_UART_FLOW_MODE_NONE	0x0
> > +#define XR21V141X_UART_FLOW_MODE_HW	0x1
> > +#define XR21V141X_UART_FLOW_MODE_SW	0x2
> > +
> > +#define XR21V141X_UART_MODE_GPIO_MASK	GENMASK(2, 0)
> > +#define XR21V141X_UART_MODE_RTS_CTS	0x1
> > +#define XR21V141X_UART_MODE_DTR_DSR	0x2
> > +#define XR21V141X_UART_MODE_RS485	0x3
> > +#define XR21V141X_UART_MODE_RS485_ADDR	0x4
> > +
> > +#define XR21V141X_REG_ENABLE		0x03
> > +#define XR21V141X_REG_FORMAT		0x0b
> > +#define XR21V141X_REG_FLOW_CTRL		0x0c
> > +#define XR21V141X_REG_XON_CHAR		0x10
> > +#define XR21V141X_REG_XOFF_CHAR		0x11
> > +#define XR21V141X_REG_LOOPBACK		0x12
> > +#define XR21V141X_REG_TX_BREAK		0x14
> > +#define XR21V141X_REG_RS845_DELAY	0x15
> > +#define XR21V141X_REG_GPIO_MODE		0x1a
> > +#define XR21V141X_REG_GPIO_DIR		0x1b
> > +#define XR21V141X_REG_GPIO_INT_MASK	0x1c
> > +#define XR21V141X_REG_GPIO_SET		0x1d
> > +#define XR21V141X_REG_GPIO_CLR		0x1e
> > +#define XR21V141X_REG_GPIO_STATUS	0x1f
> > +
> > +static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg,
> > +		      u8 val)
> 
> No need for line breaks.
> 
> > +{
> > +	struct usb_serial *serial = port->serial;
> > +	int ret;
> > +
> > +	ret = usb_control_msg(serial->dev,
> > +			      usb_sndctrlpipe(serial->dev, 0),
> > +			      XR21V141X_SET_REQ,
> > +			      USB_DIR_OUT | USB_TYPE_VENDOR, val,
> 
> The request-type should include USB_RECIP_DEVICE (0) as well.
> 
> > +			      reg | (block << 8), NULL, 0,
> > +			      USB_CTRL_SET_TIMEOUT);
> > +	if (ret < 0) {
> > +		dev_err(&port->dev, "Failed to set reg 0x%02x: %d\n", reg, ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int xr_get_reg(struct usb_serial_port *port, u8 block, u8 reg,
> > +		      u8 *val)
> > +{
> > +	struct usb_serial *serial = port->serial;
> > +	u8 *dmabuf;
> > +	int ret;
> > +
> > +	dmabuf = kmalloc(1, GFP_KERNEL);
> > +	if (!dmabuf)
> > +		return -ENOMEM;
> > +
> > +	ret = usb_control_msg(serial->dev,
> > +			      usb_rcvctrlpipe(serial->dev, 0),
> > +			      XR21V141X_GET_REQ,
> > +			      USB_DIR_IN | USB_TYPE_VENDOR, 0,
> > +			      reg | (block << 8), dmabuf, 1,
> > +			      USB_CTRL_GET_TIMEOUT);
> > +	if (ret == 1) {
> > +		*val = *dmabuf;
> > +		ret = 0;
> > +	} else {
> > +		dev_err(&port->dev, "Failed to get reg 0x%02x: %d\n", reg, ret);
> > +		if (ret >= 0)
> > +			ret = -EIO;
> > +	}
> > +
> > +	kfree(dmabuf);
> > +
> > +	return ret;
> > +}
> > +
> > +static int xr_set_reg_uart(struct usb_serial_port *port, u8 reg, u8 val)
> > +{
> > +	return xr_set_reg(port, XR21V141X_UART_REG_BLOCK, reg, val);
> > +}
> > +
> > +static int xr_get_reg_uart(struct usb_serial_port *port, u8 reg, u8 *val)
> > +{
> > +	return xr_get_reg(port, XR21V141X_UART_REG_BLOCK, reg, val);
> > +}
> > +
> > +static int xr_set_reg_um(struct usb_serial_port *port, u8 reg, u8 val)
> > +{
> > +	return xr_set_reg(port, XR21V141X_UM_REG_BLOCK, reg, val);
> > +}
> > +
> > +/* Tx and Rx clock mask values obtained from section 3.3.4 of datasheet */
> > +static const struct xr_txrx_clk_mask xr21v141x_txrx_clk_masks[] = {
> > +	{ 0x000, 0x000, 0x000 },
> > +	{ 0x000, 0x000, 0x000 },
> > +	{ 0x100, 0x000, 0x100 },
> > +	{ 0x020, 0x400, 0x020 },
> > +	{ 0x010, 0x100, 0x010 },
> > +	{ 0x208, 0x040, 0x208 },
> > +	{ 0x104, 0x820, 0x108 },
> > +	{ 0x844, 0x210, 0x884 },
> > +	{ 0x444, 0x110, 0x444 },
> > +	{ 0x122, 0x888, 0x224 },
> > +	{ 0x912, 0x448, 0x924 },
> > +	{ 0x492, 0x248, 0x492 },
> > +	{ 0x252, 0x928, 0x292 },
> > +	{ 0x94a, 0x4a4, 0xa52 },
> > +	{ 0x52a, 0xaa4, 0x54a },
> > +	{ 0xaaa, 0x954, 0x4aa },
> > +	{ 0xaaa, 0x554, 0xaaa },
> > +	{ 0x555, 0xad4, 0x5aa },
> > +	{ 0xb55, 0xab4, 0x55a },
> > +	{ 0x6b5, 0x5ac, 0xb56 },
> > +	{ 0x5b5, 0xd6c, 0x6d6 },
> > +	{ 0xb6d, 0xb6a, 0xdb6 },
> > +	{ 0x76d, 0x6da, 0xbb6 },
> > +	{ 0xedd, 0xdda, 0x76e },
> > +	{ 0xddd, 0xbba, 0xeee },
> > +	{ 0x7bb, 0xf7a, 0xdde },
> > +	{ 0xf7b, 0xef6, 0x7de },
> > +	{ 0xdf7, 0xbf6, 0xf7e },
> > +	{ 0x7f7, 0xfee, 0xefe },
> > +	{ 0xfdf, 0xfbe, 0x7fe },
> > +	{ 0xf7f, 0xefe, 0xffe },
> > +	{ 0xfff, 0xffe, 0xffd },
> > +};
> > +
> > +static int xr_set_baudrate(struct tty_struct *tty,
> > +			   struct usb_serial_port *port)
> > +{
> > +	u32 divisor, baud, idx;
> > +	u16 tx_mask, rx_mask;
> > +	int ret;
> > +
> > +	baud = clamp(tty->termios.c_ospeed, XR21V141X_MIN_SPEED,
> > +		     XR21V141X_MAX_SPEED);
> 
> You shouldn't report back anything else but B0 if that's requested, the
> actual rate can be left unchanged.
> 
> > +	divisor = XR_INT_OSC_HZ / baud;
> > +	idx = ((32 * XR_INT_OSC_HZ) / baud) & 0x1f;
> > +	tx_mask = xr21v141x_txrx_clk_masks[idx].tx;
> > +
> > +	if (divisor & 1)
> 
> Nit: 0x01 since bitmask.
> 
> > +		rx_mask = xr21v141x_txrx_clk_masks[idx].rx1;
> > +	else
> > +		rx_mask = xr21v141x_txrx_clk_masks[idx].rx0;
> > +
> > +	dev_dbg(&port->dev, "Setting baud rate: %u\n", baud);
> > +	/*
> > +	 * XR21V141X uses fractional baud rate generator with 48MHz internal
> > +	 * oscillator and 19-bit programmable divisor. So theoretically it can
> > +	 * generate most commonly used baud rates with high accuracy.
> > +	 */
> > +	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_0, (divisor & 0xff));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_1, ((divisor >>  8) & 0xff));
> 
> I broke these long lines again.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_CLOCK_DIVISOR_2, ((divisor >> 16) & 0xff));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_TX_CLOCK_MASK_0, (tx_mask & 0xff));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_TX_CLOCK_MASK_1, ((tx_mask >>  8) & 0xff));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_RX_CLOCK_MASK_0, (rx_mask & 0xff));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_RX_CLOCK_MASK_1, ((rx_mask >>  8) & 0xff));
> > +
> > +	tty_encode_baud_rate(tty, baud, baud);
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * According to datasheet, below is the recommended sequence for enabling UART
> > + * module in XR21V141X:
> > + *
> > + * Enable Tx FIFO
> > + * Enable Tx and Rx
> > + * Enable Rx FIFO
> > + */
> > +static int xr_uart_enable(struct usb_serial_port *port)
> > +{
> > +	int ret;
> > +
> > +	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG, XR21V141X_UM_ENABLE_TX_FIFO);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_REG_ENABLE,
> > +			      XR21V141X_UART_ENABLE_TX | XR21V141X_UART_ENABLE_RX);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG,
> > +			    XR21V141X_UM_ENABLE_TX_FIFO | XR21V141X_UM_ENABLE_RX_FIFO);
> > +
> > +	if (ret)
> > +		xr_set_reg_uart(port, XR21V141X_REG_ENABLE, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +static int xr_uart_disable(struct usb_serial_port *port)
> > +{
> > +	int ret;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_REG_ENABLE, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xr_set_reg_um(port, XR21V141X_UM_FIFO_ENABLE_REG, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +static void xr_set_flow_mode(struct tty_struct *tty,
> > +			     struct usb_serial_port *port)
> > +{
> > +	unsigned int cflag = tty->termios.c_cflag;
> > +	int ret;
> > +	u8 flow, gpio_mode;
> > +
> > +	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_MODE, &gpio_mode);
> > +	if (ret)
> > +		return;
> > +
> > +	if (cflag & CRTSCTS) {
> 
> C_CRTSCTS(tty)
> 
> > +		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
> > +
> > +		/*
> > +		 * RTS/CTS is the default flow control mode, so set GPIO mode
> > +		 * for controlling the pins manually by default.
> > +		 */
> > +		gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK;
> 
> This needs to be done before the conditional so that GPIO mode is again
> selected when disabling flow control, otherwise your break RTS control.
> 
> Also you never configure DTR/RTS as outputs despite all pins being
> inputs by default according to the datasheet. Effectively, DTR control
> is currently broken and RTS can only be used for automatic flow control.
> 
> Did you not test these signals separately?
> 
> > +		gpio_mode |= XR21V141X_UART_MODE_RTS_CTS;
> > +		flow = XR21V141X_UART_FLOW_MODE_HW;
> > +	} else if (I_IXON(tty)) {
> > +		u8 start_char = START_CHAR(tty);
> > +		u8 stop_char = STOP_CHAR(tty);
> > +
> > +		dev_dbg(&port->dev, "Enabling sw flow ctrl\n");
> > +		flow = XR21V141X_UART_FLOW_MODE_SW;
> > +
> > +		xr_set_reg_uart(port, XR21V141X_REG_XON_CHAR, start_char);
> > +		xr_set_reg_uart(port, XR21V141X_REG_XOFF_CHAR, stop_char);
> > +	} else {
> > +		dev_dbg(&port->dev, "Disabling flow ctrl\n");
> > +		flow = XR21V141X_UART_FLOW_MODE_NONE;
> > +	}
> > +
> > +	/*
> > +	 * As per the datasheet, UART needs to be disabled while writing to
> > +	 * FLOW_CONTROL register.
> > +	 */
> > +	xr_uart_disable(port);
> > +	xr_set_reg_uart(port, XR21V141X_REG_FLOW_CTRL, flow);
> > +	xr_uart_enable(port);
> > +
> > +	xr_set_reg_uart(port, XR21V141X_REG_GPIO_MODE, gpio_mode);
> > +}
> > +
> > +static int xr_tiocmset_port(struct usb_serial_port *port,
> > +			    unsigned int set, unsigned int clear)
> > +{
> > +	u8 gpio_set = 0;
> > +	u8 gpio_clr = 0;
> > +	int ret = 0;
> > +
> > +	/* Modem control pins are active low, so set & clr are swapped */
> > +	if (set & TIOCM_RTS)
> > +		gpio_clr |= XR21V141X_UART_MODE_RTS;
> > +	if (set & TIOCM_DTR)
> > +		gpio_clr |= XR21V141X_UART_MODE_DTR;
> > +	if (clear & TIOCM_RTS)
> > +		gpio_set |= XR21V141X_UART_MODE_RTS;
> > +	if (clear & TIOCM_DTR)
> > +		gpio_set |= XR21V141X_UART_MODE_DTR;
> > +
> > +	/* Writing '0' to gpio_{set/clr} bits has no effect, so no need to do */
> > +	if (gpio_clr)
> > +		ret = xr_set_reg_uart(port, XR21V141X_REG_GPIO_CLR, gpio_clr);
> > +
> > +	if (gpio_set)
> > +		ret = xr_set_reg_uart(port, XR21V141X_REG_GPIO_SET, gpio_set);
> > +
> > +	return ret;
> > +}
> > +
> > +static int xr_tiocmset(struct tty_struct *tty,
> > +		       unsigned int set, unsigned int clear)
> > +{
> > +	struct usb_serial_port *port = tty->driver_data;
> > +
> > +	return xr_tiocmset_port(port, set, clear);
> > +}
> > +
> > +static void xr_dtr_rts(struct usb_serial_port *port, int on)
> > +{
> > +	if (on)
> > +		xr_tiocmset_port(port, TIOCM_DTR | TIOCM_RTS, 0);
> > +	else
> > +		xr_tiocmset_port(port, 0, TIOCM_DTR | TIOCM_RTS);
> > +}
> > +
> > +static void xr_set_termios(struct tty_struct *tty,
> > +			   struct usb_serial_port *port,
> > +			   struct ktermios *old_termios)
> > +{
> > +	struct ktermios *termios = &tty->termios;
> > +	int ret;
> > +	u8 bits = 0;
> > +
> > +	if ((old_termios && tty->termios.c_ospeed != old_termios->c_ospeed) ||
> > +	    !old_termios)
> 
> This can be simplified.
> 
> > +		xr_set_baudrate(tty, port);
> > +
> > +	switch (C_CSIZE(tty)) {
> > +	case CS5:
> > +	fallthrough;
> 
> No need for fallthrough for empty cases.
> 
> > +	case CS6:
> > +		/* CS5 and CS6 are not supported, so just restore old setting */
> > +		termios->c_cflag &= ~CSIZE;
> > +		if (old_termios)
> > +			termios->c_cflag |= old_termios->c_cflag & CSIZE;
> > +		else
> > +			bits |= XR21V141X_UART_DATA_8;
> > +		break;
> > +	case CS7:
> > +		bits |= XR21V141X_UART_DATA_7;
> > +		break;
> > +	case CS8:
> > +	fallthrough;
> > +	default:
> > +		bits |= XR21V141X_UART_DATA_8;
> > +		break;
> > +	}
> > +
> > +	if (C_PARENB(tty)) {
> > +		if (C_CMSPAR(tty)) {
> > +			if (C_PARODD(tty))
> > +				bits |= XR21V141X_UART_PARITY_MARK <<
> > +					XR21V141X_UART_PARITY_SHIFT;
> 
> I'd prefer just shifting the values when defining them, which is more
> consistent with how CSIZE is handled too.
> 
> > +			else
> > +				bits |= XR21V141X_UART_PARITY_SPACE <<
> > +					XR21V141X_UART_PARITY_SHIFT;
> > +		} else {
> > +			if (C_PARODD(tty))
> > +				bits |= XR21V141X_UART_PARITY_ODD <<
> > +					XR21V141X_UART_PARITY_SHIFT;
> > +			else
> > +				bits |= XR21V141X_UART_PARITY_EVEN <<
> > +					XR21V141X_UART_PARITY_SHIFT;
> > +		}
> > +	}
> > +
> > +	if (C_CSTOPB(tty))
> > +		bits |= XR21V141X_UART_STOP_2 << XR21V141X_UART_STOP_SHIFT;
> > +	else
> > +		bits |= XR21V141X_UART_STOP_1 << XR21V141X_UART_STOP_SHIFT;
> > +
> > +	ret = xr_set_reg_uart(port, XR21V141X_REG_FORMAT, bits);
> > +	if (ret)
> > +		return;
> > +
> > +	/* If baud rate is B0, clear DTR and RTS */
> > +	if (C_BAUD(tty) == B0)
> > +		xr_dtr_rts(port, 0);
> 
> This isn't sufficient; RTS will not be dropped when CRTSCTS is enabled,
> and we should reassert the lines when moving from B0 too.
> 
> > +
> > +	xr_set_flow_mode(tty, port);
> > +}
> > +
> > +static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
> > +{
> > +	int ret;
> > +
> > +	ret = xr_uart_enable(port);
> > +	if (ret) {
> > +		dev_err(&port->dev, "Failed to enable UART\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Setup termios */
> > +	if (tty)
> > +		xr_set_termios(tty, port, NULL);
> > +
> > +	ret = usb_serial_generic_open(tty, port);
> > +	if (ret) {
> > +		xr_uart_disable(port);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void xr_close(struct usb_serial_port *port)
> > +{
> > +	usb_serial_generic_close(port);
> > +
> > +	xr_uart_disable(port);
> > +}
> > +
> > +static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
> > +{
> > +	struct usb_device *usb_dev = interface_to_usbdev(serial->interface);
> 
> You can just use serial->dev directly.
> 
> > +	struct usb_driver *driver = serial->type->usb_driver;
> > +	struct usb_interface *control_interface;
> > +	int ret;
> > +
> > +	/* Don't bind to control interface */
> > +	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
> > +		return -ENODEV;
> > +
> > +	/* But claim the control interface during data interface probe */
> > +	control_interface = usb_ifnum_to_if(usb_dev, 0);
> 
> You must check for NULL here since a device may not have an interface 0
> in case you'll oops here. This can be triggered by a malicious device or
> when fuzzing USB descriptors.
> 
> > +	ret = usb_driver_claim_interface(driver, control_interface, NULL);
> > +	if (ret) {
> > +		dev_err(&serial->interface->dev, "Can't claim control interface\n");
> > +		return ret;
> > +	}
> 
> And you must release the control interface again when unbinding the
> driver to avoid leaking resources and to allow the driver to be rebound.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int xr_tiocmget(struct tty_struct *tty)
> > +{
> > +	struct usb_serial_port *port = tty->driver_data;
> > +	u8 status;
> > +	int ret;
> > +
> > +	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_STATUS, &status);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Modem control pins are active low, so reading '0' means it is active
> > +	 * and '1' means not active.
> > +	 */
> > +	ret = ((status & XR21V141X_UART_MODE_DTR) ? 0 : TIOCM_DTR) |
> > +	      ((status & XR21V141X_UART_MODE_RTS) ? 0 : TIOCM_RTS) |
> > +	      ((status & XR21V141X_UART_MODE_CTS) ? 0 : TIOCM_CTS) |
> > +	      ((status & XR21V141X_UART_MODE_DSR) ? 0 : TIOCM_DSR) |
> > +	      ((status & XR21V141X_UART_MODE_RI) ? 0 : TIOCM_RI) |
> > +	      ((status & XR21V141X_UART_MODE_CD) ? 0 : TIOCM_CD);
> > +
> > +	return ret;
> > +}
> > +
> > +static void xr_break_ctl(struct tty_struct *tty, int break_state)
> > +{
> > +	struct usb_serial_port *port = tty->driver_data;
> > +	u8 state;
> > +
> > +	if (break_state == 0)
> > +		state = XR21V141X_UART_BREAK_OFF;
> > +	else
> > +		state = XR21V141X_UART_BREAK_ON;
> > +
> > +	dev_dbg(&port->dev, "Turning break %s\n",
> > +		state == XR21V141X_UART_BREAK_OFF ? "off" : "on");
> > +	xr_set_reg_uart(port, XR21V141X_REG_TX_BREAK, state);
> > +}
> > +
> > +static int xr_port_probe(struct usb_serial_port *port)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int xr_port_remove(struct usb_serial_port *port)
> > +{
> > +	return 0;
> > +}
> 
> Again, don't add these until you need them.
> 
> > +
> > +static const struct usb_device_id id_table[] = {
> > +	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(usb, id_table);
> > +
> > +static struct usb_serial_driver xr_device = {
> > +	.driver = {
> > +		.owner = THIS_MODULE,
> > +		.name =	"xr_serial",
> > +	},
> > +	.id_table		= id_table,
> > +	.num_ports		= 1,
> > +	.open			= xr_open,
> > +	.close			= xr_close,
> > +	.break_ctl		= xr_break_ctl,
> > +	.set_termios		= xr_set_termios,
> > +	.tiocmget		= xr_tiocmget,
> > +	.tiocmset		= xr_tiocmset,
> > +	.probe			= xr_probe,
> > +	.port_probe		= xr_port_probe,
> > +	.port_remove		= xr_port_remove,
> > +	.dtr_rts		= xr_dtr_rts
> > +};
> > +
> > +static struct usb_serial_driver * const serial_drivers[] = {
> > +	&xr_device, NULL
> > +};
> > +
> > +module_usb_serial_driver(serial_drivers, id_table);
> > +
> > +MODULE_AUTHOR("Manivannan Sadhasivam <mani@kernel.org>");
> > +MODULE_DESCRIPTION("MaxLinear/Exar USB to Serial driver");
> > +MODULE_LICENSE("GPL");
> 
> Looks good overall otherwise.
> 
> I've applied this one now, and will follow up with a series addressing
> the non-trivial bits pointed out below.
> 
> Johan

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Johan Hovold]

On Tue, Jan 26, 2021 at 09:16:04PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:
> > On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:
> > > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > > only supports XR21V141X series but it can be extended to other series
> > > from Exar as well in future.
> > 
> > There are still a few issues with this driver, but I really don't want
> > to have to review it again in a couple of months so I've fixed it up
> > myself this time.
> > 
> > The trivial stuff I folded into this patch, and I'll submit a follow-on
> > series for the rest.
> > 
> 
> Many thanks for doing this! These days it is really difficult to find
> time for spare time stuffs.
> 
> And all of your fixes makes sense to me.

Thanks for taking a look!

Johan

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Mauro Carvalho Chehab]

Hi Johan,

Em Tue, 26 Jan 2021 17:26:36 +0100
Johan Hovold <johan@kernel.org> escreveu:

> On Tue, Jan 26, 2021 at 09:16:04PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:  
> > > On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:  
> > > > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > > > only supports XR21V141X series but it can be extended to other series
> > > > from Exar as well in future.  
> > > 
> > > There are still a few issues with this driver, but I really don't want
> > > to have to review it again in a couple of months so I've fixed it up
> > > myself this time.
> > > 
> > > The trivial stuff I folded into this patch, and I'll submit a follow-on
> > > series for the rest.
> > >   
> > 
> > Many thanks for doing this! These days it is really difficult to find
> > time for spare time stuffs.
> > 
> > And all of your fixes makes sense to me.  
> 
> Thanks for taking a look!

Thanks for merging it!

-

I'm now facing an issue with this driver. I have here two different
boards with those USB UART from MaxLinear/Exar.

The first one is identical to Mani's one:
	USB_DEVICE(0x04e2, 0x1411)
The second one is a different version of it:
	USB_DEVICE(0x04e2, 0x1424)

By looking at the final driver merged at linux-next, it sounds that
somewhere during the review of this series, it lost the priv struct,
and the xr_probe function. It also lost support for all MaxLinear/Exar
devices, except for just one model (04e2:1411).

The original submission:

	https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop

And the manufacturer's Linux driver on their website:

	https://www.maxlinear.com/support/design-tools/software-drivers

Had support for other 12 different models of the MaxLinear/Exar USB
UART. 

Those are grouped into 5 different major types:

	+	init_xr2280x_reg_map();
	+	init_xr21b142x_reg_map();
	+	init_xr21b1411_reg_map();
	+	init_xr21v141x_reg_map();
	+
	+	if ((xrusb->DeviceProduct & 0xfff0) == 0x1400)
	+		memcpy(&(xrusb->reg_map), &xr2280x_reg_map,
	+			sizeof(struct reg_addr_map));
	+	else if ((xrusb->DeviceProduct & 0xFFF0) == 0x1420)
	+		memcpy(&(xrusb->reg_map), &xr21b142x_reg_map,
	+			sizeof(struct reg_addr_map));
	+	else if (xrusb->DeviceProduct == 0x1411)
	+		memcpy(&(xrusb->reg_map), &xr21b1411_reg_map,
	+			sizeof(struct reg_addr_map));
	+	else if ((xrusb->DeviceProduct & 0xfff0) == 0x1410)
	+		memcpy(&(xrusb->reg_map), &xr21v141x_reg_map,
	+			sizeof(struct reg_addr_map));
	+	else
	+		rv = -1;

Note: Please don't be confused by "reg_map" name. This has nothing
      to do with Linux regmap API ;-)

What happens is that different USB IDs have different values for
each register. So, for instance, the UART enable register is set to
either one of the following values, depending on the value of
udev->descriptor.idProduct:

	xr21b140x_reg_map.uart_enable_addr = 0x00;
	xr21b1411_reg_map.uart_enable_addr = 0xc00;
	xr21v141x_reg_map.uart_enable_addr = 0x03;
	xr21b142x_reg_map.uart_enable_addr = 0x00;

There are other values that depend on the probing time detection,
based on other USB descriptors. Those set several fields at the
priv data that would allow to properly map the registers.

Also, there are 4 models that support multiple channels. On those,
there are one pair of register get/set for each channel.

-

In summary, while supporting just 04e2:1411 there's no need for
a private struct, in order to properly support the other models,
some autodetection is needed. The best way of doing that is to
re-add the .probe method and adding a priv struct.

As I dunno why this was dropped in the first place, I'm wondering
if it would be ok to re-introduce them.

To be clear: my main focus here is just to avoid needing to use 
Windows in order to use the serial console of the hardware with
the 0x1424 variant ;-)

I can't test the driver with the other hardware, but, IMHO, instead
of adding a hack to support 0x1424, the better (but more painful)
would be to re-add the auto-detection part and support for the
other models.

Thanks!
Mauro

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Johan Hovold]

On Mon, Feb 22, 2021 at 04:27:34PM +0100, Mauro Carvalho Chehab wrote:
> Hi Johan,
> 
> Em Tue, 26 Jan 2021 17:26:36 +0100
> Johan Hovold <johan@kernel.org> escreveu:
> 
> > On Tue, Jan 26, 2021 at 09:16:04PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:  
> > > > On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:  
> > > > > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > > > > only supports XR21V141X series but it can be extended to other series
> > > > > from Exar as well in future.  

> I'm now facing an issue with this driver. I have here two different
> boards with those USB UART from MaxLinear/Exar.
> 
> The first one is identical to Mani's one:
> 	USB_DEVICE(0x04e2, 0x1411)
> The second one is a different version of it:
> 	USB_DEVICE(0x04e2, 0x1424)
> 
> By looking at the final driver merged at linux-next, it sounds that
> somewhere during the review of this series, it lost the priv struct,
> and the xr_probe function. It also lost support for all MaxLinear/Exar
> devices, except for just one model (04e2:1411).
> 
> The original submission:
> 
> 	https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> 
> And the manufacturer's Linux driver on their website:
> 
> 	https://www.maxlinear.com/support/design-tools/software-drivers
> 
> Had support for other 12 different models of the MaxLinear/Exar USB
> UART. 

IIRC Manivannan only had access to one of these models and his original
submission (based on the patch you link to above) didn't include support
for the others. And keeping the type abstraction didn't make sense for
just one model.

> Those are grouped into 5 different major types:
> 
> 	+	init_xr2280x_reg_map();
> 	+	init_xr21b142x_reg_map();
> 	+	init_xr21b1411_reg_map();
> 	+	init_xr21v141x_reg_map();
> 	+
> 	+	if ((xrusb->DeviceProduct & 0xfff0) == 0x1400)
> 	+		memcpy(&(xrusb->reg_map), &xr2280x_reg_map,
> 	+			sizeof(struct reg_addr_map));
> 	+	else if ((xrusb->DeviceProduct & 0xFFF0) == 0x1420)
> 	+		memcpy(&(xrusb->reg_map), &xr21b142x_reg_map,
> 	+			sizeof(struct reg_addr_map));
> 	+	else if (xrusb->DeviceProduct == 0x1411)
> 	+		memcpy(&(xrusb->reg_map), &xr21b1411_reg_map,
> 	+			sizeof(struct reg_addr_map));
> 	+	else if ((xrusb->DeviceProduct & 0xfff0) == 0x1410)
> 	+		memcpy(&(xrusb->reg_map), &xr21v141x_reg_map,
> 	+			sizeof(struct reg_addr_map));
> 	+	else
> 	+		rv = -1;
> 
> Note: Please don't be confused by "reg_map" name. This has nothing
>       to do with Linux regmap API ;-)
> 
> What happens is that different USB IDs have different values for
> each register. So, for instance, the UART enable register is set to
> either one of the following values, depending on the value of
> udev->descriptor.idProduct:
> 
> 	xr21b140x_reg_map.uart_enable_addr = 0x00;
> 	xr21b1411_reg_map.uart_enable_addr = 0xc00;
> 	xr21v141x_reg_map.uart_enable_addr = 0x03;
> 	xr21b142x_reg_map.uart_enable_addr = 0x00;
> 
> There are other values that depend on the probing time detection,
> based on other USB descriptors. Those set several fields at the
> priv data that would allow to properly map the registers.
> 
> Also, there are 4 models that support multiple channels. On those,
> there are one pair of register get/set for each channel.
> 
> -
> 
> In summary, while supporting just 04e2:1411 there's no need for
> a private struct, in order to properly support the other models,
> some autodetection is needed. The best way of doing that is to
> re-add the .probe method and adding a priv struct.
> 
> As I dunno why this was dropped in the first place, I'm wondering
> if it would be ok to re-introduce them.

Sure. It was just not needed if we were only going to support one model.

> To be clear: my main focus here is just to avoid needing to use 
> Windows in order to use the serial console of the hardware with
> the 0x1424 variant ;-)
> 
> I can't test the driver with the other hardware, but, IMHO, instead
> of adding a hack to support 0x1424, the better (but more painful)
> would be to re-add the auto-detection part and support for the
> other models.

Sounds good to me. 

Johan

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Mauro Carvalho Chehab]

Em Mon, 22 Feb 2021 16:47:36 +0100
Johan Hovold <johan@kernel.org> escreveu:

> On Mon, Feb 22, 2021 at 04:27:34PM +0100, Mauro Carvalho Chehab wrote:
> > Hi Johan,
> > 
> > Em Tue, 26 Jan 2021 17:26:36 +0100
> > Johan Hovold <johan@kernel.org> escreveu:
> >   
> > > On Tue, Jan 26, 2021 at 09:16:04PM +0530, Manivannan Sadhasivam wrote:  
> > > > On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:    
> > > > > On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:    
> > > > > > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > > > > > only supports XR21V141X series but it can be extended to other series
> > > > > > from Exar as well in future.    
> 
> > I'm now facing an issue with this driver. I have here two different
> > boards with those USB UART from MaxLinear/Exar.
> > 
> > The first one is identical to Mani's one:
> > 	USB_DEVICE(0x04e2, 0x1411)
> > The second one is a different version of it:
> > 	USB_DEVICE(0x04e2, 0x1424)
> > 
> > By looking at the final driver merged at linux-next, it sounds that
> > somewhere during the review of this series, it lost the priv struct,
> > and the xr_probe function. It also lost support for all MaxLinear/Exar
> > devices, except for just one model (04e2:1411).
> > 
> > The original submission:
> > 
> > 	https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> > 
> > And the manufacturer's Linux driver on their website:
> > 
> > 	https://www.maxlinear.com/support/design-tools/software-drivers
> > 
> > Had support for other 12 different models of the MaxLinear/Exar USB
> > UART.   
> 
> IIRC Manivannan only had access to one of these models and his original
> submission (based on the patch you link to above) didn't include support
> for the others. And keeping the type abstraction didn't make sense for
> just one model.
> 
> > Those are grouped into 5 different major types:
> > 
> > 	+	init_xr2280x_reg_map();
> > 	+	init_xr21b142x_reg_map();
> > 	+	init_xr21b1411_reg_map();
> > 	+	init_xr21v141x_reg_map();
> > 	+
> > 	+	if ((xrusb->DeviceProduct & 0xfff0) == 0x1400)
> > 	+		memcpy(&(xrusb->reg_map), &xr2280x_reg_map,
> > 	+			sizeof(struct reg_addr_map));
> > 	+	else if ((xrusb->DeviceProduct & 0xFFF0) == 0x1420)
> > 	+		memcpy(&(xrusb->reg_map), &xr21b142x_reg_map,
> > 	+			sizeof(struct reg_addr_map));
> > 	+	else if (xrusb->DeviceProduct == 0x1411)
> > 	+		memcpy(&(xrusb->reg_map), &xr21b1411_reg_map,
> > 	+			sizeof(struct reg_addr_map));
> > 	+	else if ((xrusb->DeviceProduct & 0xfff0) == 0x1410)
> > 	+		memcpy(&(xrusb->reg_map), &xr21v141x_reg_map,
> > 	+			sizeof(struct reg_addr_map));
> > 	+	else
> > 	+		rv = -1;
> > 
> > Note: Please don't be confused by "reg_map" name. This has nothing
> >       to do with Linux regmap API ;-)
> > 
> > What happens is that different USB IDs have different values for
> > each register. So, for instance, the UART enable register is set to
> > either one of the following values, depending on the value of
> > udev->descriptor.idProduct:
> > 
> > 	xr21b140x_reg_map.uart_enable_addr = 0x00;
> > 	xr21b1411_reg_map.uart_enable_addr = 0xc00;
> > 	xr21v141x_reg_map.uart_enable_addr = 0x03;
> > 	xr21b142x_reg_map.uart_enable_addr = 0x00;
> > 
> > There are other values that depend on the probing time detection,
> > based on other USB descriptors. Those set several fields at the
> > priv data that would allow to properly map the registers.
> > 
> > Also, there are 4 models that support multiple channels. On those,
> > there are one pair of register get/set for each channel.
> > 
> > -
> > 
> > In summary, while supporting just 04e2:1411 there's no need for
> > a private struct, in order to properly support the other models,
> > some autodetection is needed. The best way of doing that is to
> > re-add the .probe method and adding a priv struct.
> > 
> > As I dunno why this was dropped in the first place, I'm wondering
> > if it would be ok to re-introduce them.  
> 
> Sure. It was just not needed if we were only going to support one model.
> 
> > To be clear: my main focus here is just to avoid needing to use 
> > Windows in order to use the serial console of the hardware with
> > the 0x1424 variant ;-)
> > 
> > I can't test the driver with the other hardware, but, IMHO, instead
> > of adding a hack to support 0x1424, the better (but more painful)
> > would be to re-add the auto-detection part and support for the
> > other models.  
> 
> Sounds good to me. 

Great! I'll work on a patch and submit when done.

Thanks!
Mauro

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Mauro Carvalho Chehab]

Em Mon, 22 Feb 2021 16:47:36 +0100
Johan Hovold <johan@kernel.org> escreveu:

> On Mon, Feb 22, 2021 at 04:27:34PM +0100, Mauro Carvalho Chehab wrote:
> > Hi Johan,
> > 
> > Em Tue, 26 Jan 2021 17:26:36 +0100
> > Johan Hovold <johan@kernel.org> escreveu:
> >   
> > > On Tue, Jan 26, 2021 at 09:16:04PM +0530, Manivannan Sadhasivam wrote:  
> > > > On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:    
> > > > > On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:    
> > > > > > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > > > > > only supports XR21V141X series but it can be extended to other series
> > > > > > from Exar as well in future.    
> 
> > I'm now facing an issue with this driver. I have here two different
> > boards with those USB UART from MaxLinear/Exar.
> > 
> > The first one is identical to Mani's one:
> > 	USB_DEVICE(0x04e2, 0x1411)
> > The second one is a different version of it:
> > 	USB_DEVICE(0x04e2, 0x1424)
> > 
> > By looking at the final driver merged at linux-next, it sounds that
> > somewhere during the review of this series, it lost the priv struct,
> > and the xr_probe function. It also lost support for all MaxLinear/Exar
> > devices, except for just one model (04e2:1411).
> > 
> > The original submission:
> > 
> > 	https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> > 
> > And the manufacturer's Linux driver on their website:
> > 
> > 	https://www.maxlinear.com/support/design-tools/software-drivers
> > 
> > Had support for other 12 different models of the MaxLinear/Exar USB
> > UART.   
> 
> IIRC Manivannan only had access to one of these models and his original
> submission (based on the patch you link to above) didn't include support
> for the others. And keeping the type abstraction didn't make sense for
> just one model.
> 
> > Those are grouped into 5 different major types:
> > 
> > 	+	init_xr2280x_reg_map();
> > 	+	init_xr21b142x_reg_map();
> > 	+	init_xr21b1411_reg_map();
> > 	+	init_xr21v141x_reg_map();
> > 	+
> > 	+	if ((xrusb->DeviceProduct & 0xfff0) == 0x1400)
> > 	+		memcpy(&(xrusb->reg_map), &xr2280x_reg_map,
> > 	+			sizeof(struct reg_addr_map));
> > 	+	else if ((xrusb->DeviceProduct & 0xFFF0) == 0x1420)
> > 	+		memcpy(&(xrusb->reg_map), &xr21b142x_reg_map,
> > 	+			sizeof(struct reg_addr_map));
> > 	+	else if (xrusb->DeviceProduct == 0x1411)
> > 	+		memcpy(&(xrusb->reg_map), &xr21b1411_reg_map,
> > 	+			sizeof(struct reg_addr_map));
> > 	+	else if ((xrusb->DeviceProduct & 0xfff0) == 0x1410)
> > 	+		memcpy(&(xrusb->reg_map), &xr21v141x_reg_map,
> > 	+			sizeof(struct reg_addr_map));
> > 	+	else
> > 	+		rv = -1;
> > 
> > Note: Please don't be confused by "reg_map" name. This has nothing
> >       to do with Linux regmap API ;-)
> > 
> > What happens is that different USB IDs have different values for
> > each register. So, for instance, the UART enable register is set to
> > either one of the following values, depending on the value of
> > udev->descriptor.idProduct:
> > 
> > 	xr21b140x_reg_map.uart_enable_addr = 0x00;
> > 	xr21b1411_reg_map.uart_enable_addr = 0xc00;
> > 	xr21v141x_reg_map.uart_enable_addr = 0x03;
> > 	xr21b142x_reg_map.uart_enable_addr = 0x00;
> > 
> > There are other values that depend on the probing time detection,
> > based on other USB descriptors. Those set several fields at the
> > priv data that would allow to properly map the registers.
> > 
> > Also, there are 4 models that support multiple channels. On those,
> > there are one pair of register get/set for each channel.
> > 
> > -
> > 
> > In summary, while supporting just 04e2:1411 there's no need for
> > a private struct, in order to properly support the other models,
> > some autodetection is needed. The best way of doing that is to
> > re-add the .probe method and adding a priv struct.
> > 
> > As I dunno why this was dropped in the first place, I'm wondering
> > if it would be ok to re-introduce them.  
> 
> Sure. It was just not needed if we were only going to support one model.
> 
> > To be clear: my main focus here is just to avoid needing to use 
> > Windows in order to use the serial console of the hardware with
> > the 0x1424 variant ;-)
> > 
> > I can't test the driver with the other hardware, but, IMHO, instead
> > of adding a hack to support 0x1424, the better (but more painful)
> > would be to re-add the auto-detection part and support for the
> > other models.  
> 
> Sounds good to me. 

While testing the xr_serial (as currently merged), I opted to apply
the patches on the top of vanilla Kernel 5.11 - as it sounds too risky
to use linux-next so early on a new development cycle :-)

There, I'm getting an OOPS:

	[   30.261291] BUG: kernel NULL pointer dereference, address: 00000000000000a8
	[   30.261375] #PF: supervisor write access in kernel mode
	[   30.261438] #PF: error_code(0x0002) - not-present page
	[   30.261500] PGD 0 P4D 0 
	[   30.261539] Oops: 0002 [#1] SMP PTI
	[   30.261586] CPU: 2 PID: 686 Comm: kworker/2:3 Not tainted 5.11.0+ #14
	[   30.261666] Hardware name:  /NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 05/17/2019
	[   30.261757] Workqueue: usb_hub_wq hub_event
	[   30.261816] RIP: 0010:mutex_lock+0x1e/0x40
	[   30.261875] Code: c3 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 41 54 49 89 fc e8 8d dd ff ff 31 c0 65 48 8b 14 25 c0 7b 01 00 <f0> 49 0f b1 14 24 75 04 41 5c 5d c3 4c 89 e7 e8 ae ff ff ff 41 5c
	[   30.262076] RSP: 0018:ffffb937c0767b70 EFLAGS: 00010246
	[   30.262140] RAX: 0000000000000000 RBX: ffff95e71ef75430 RCX: 0000000000000027
	[   30.262223] RDX: ffff95e70597b000 RSI: 00000000ffffdfff RDI: 00000000000000a8
	[   30.262305] RBP: ffffb937c0767b78 R08: ffff95ea76d18ac0 R09: ffffb937c0767948
	[   30.262387] R10: 0000000000000001 R11: 0000000000000001 R12: 00000000000000a8
	[   30.262469] R13: 0000000000000000 R14: ffff95e71ef75400 R15: 0000000000000000
	[   30.262551] FS:  0000000000000000(0000) GS:ffff95ea76d00000(0000) knlGS:0000000000000000
	[   30.262645] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
	[   30.262713] CR2: 00000000000000a8 CR3: 000000042be0a002 CR4: 00000000003706e0
	[   30.262796] Call Trace:
	[   30.262832]  usb_serial_disconnect+0x33/0x140
	[   30.262897]  usb_unbind_interface+0x8c/0x260
	[   30.262957]  device_release_driver_internal+0x103/0x1d0
	[   30.263026]  device_release_driver+0x12/0x20
	[   30.263083]  bus_remove_device+0xe1/0x150
	[   30.263140]  device_del+0x192/0x3f0
	[   30.263188]  ? usb_remove_ep_devs+0x1f/0x30
	[   30.263244]  usb_disable_device+0x95/0x1c0
	[   30.263300]  usb_disconnect+0xc0/0x270
	[   30.263350]  hub_event+0xa2e/0x1620

After adding this hack:

<snip>
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -1081,6 +1081,11 @@ static void usb_serial_disconnect(struct usb_interface *interface)
        struct usb_serial_port *port;
        struct tty_struct *tty;
 
+       if (!serial) {
+               dev_err(dev, "%s: Serial pointer is NULL!!!\n", __func__);
+               return;
+       }
+
        usb_serial_console_disconnect(serial);
 
        mutex_lock(&serial->disc_mutex);
</snip>

It works fine:

	[  283.005625] xr_serial 2-1:1.1: xr_serial converter detected
	[  283.005868] usb 2-1: xr_serial converter now attached to ttyUSB0
	[  283.007284] printk: console [ttyUSB0] enabled
	[  284.444419] usb 2-1: USB disconnect, device number 5
	[  284.444520] xr_serial 2-1:1.0: usb_serial_disconnect: Serial pointer is NULL!!!
	[  284.444894] printk: console [ttyUSB0] disabled
	[  284.445091] xr_serial ttyUSB0: xr_serial converter now disconnected from ttyUSB0
	[  284.445141] xr_disconnect
	[  284.445156] xr_serial 2-1:1.1: device disconnected

I'm not sure if the bug is at xr_serial or if it is inside usb-serial.c.

Any ideas?


Thanks,
Mauro

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Mauro Carvalho Chehab]

Em Thu, 25 Feb 2021 18:58:20 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Mon, 22 Feb 2021 16:47:36 +0100
> Johan Hovold <johan@kernel.org> escreveu:
> 
> > On Mon, Feb 22, 2021 at 04:27:34PM +0100, Mauro Carvalho Chehab wrote:
> > > Hi Johan,
> > > 
> > > Em Tue, 26 Jan 2021 17:26:36 +0100
> > > Johan Hovold <johan@kernel.org> escreveu:
> > >   
> > > > On Tue, Jan 26, 2021 at 09:16:04PM +0530, Manivannan Sadhasivam wrote:  
> > > > > On Thu, Jan 21, 2021 at 11:19:24AM +0100, Johan Hovold wrote:    
> > > > > > On Sun, Nov 22, 2020 at 10:38:20PM +0530, Manivannan Sadhasivam wrote:    
> > > > > > > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > > > > > > only supports XR21V141X series but it can be extended to other series
> > > > > > > from Exar as well in future.    
> > 
> > > I'm now facing an issue with this driver. I have here two different
> > > boards with those USB UART from MaxLinear/Exar.
> > > 
> > > The first one is identical to Mani's one:
> > > 	USB_DEVICE(0x04e2, 0x1411)
> > > The second one is a different version of it:
> > > 	USB_DEVICE(0x04e2, 0x1424)
> > > 
> > > By looking at the final driver merged at linux-next, it sounds that
> > > somewhere during the review of this series, it lost the priv struct,
> > > and the xr_probe function. It also lost support for all MaxLinear/Exar
> > > devices, except for just one model (04e2:1411).
> > > 
> > > The original submission:
> > > 
> > > 	https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> > > 
> > > And the manufacturer's Linux driver on their website:
> > > 
> > > 	https://www.maxlinear.com/support/design-tools/software-drivers
> > > 
> > > Had support for other 12 different models of the MaxLinear/Exar USB
> > > UART.   
> > 
> > IIRC Manivannan only had access to one of these models and his original
> > submission (based on the patch you link to above) didn't include support
> > for the others. And keeping the type abstraction didn't make sense for
> > just one model.
> > 
> > > Those are grouped into 5 different major types:
> > > 
> > > 	+	init_xr2280x_reg_map();
> > > 	+	init_xr21b142x_reg_map();
> > > 	+	init_xr21b1411_reg_map();
> > > 	+	init_xr21v141x_reg_map();
> > > 	+
> > > 	+	if ((xrusb->DeviceProduct & 0xfff0) == 0x1400)
> > > 	+		memcpy(&(xrusb->reg_map), &xr2280x_reg_map,
> > > 	+			sizeof(struct reg_addr_map));
> > > 	+	else if ((xrusb->DeviceProduct & 0xFFF0) == 0x1420)
> > > 	+		memcpy(&(xrusb->reg_map), &xr21b142x_reg_map,
> > > 	+			sizeof(struct reg_addr_map));
> > > 	+	else if (xrusb->DeviceProduct == 0x1411)
> > > 	+		memcpy(&(xrusb->reg_map), &xr21b1411_reg_map,
> > > 	+			sizeof(struct reg_addr_map));
> > > 	+	else if ((xrusb->DeviceProduct & 0xfff0) == 0x1410)
> > > 	+		memcpy(&(xrusb->reg_map), &xr21v141x_reg_map,
> > > 	+			sizeof(struct reg_addr_map));
> > > 	+	else
> > > 	+		rv = -1;
> > > 
> > > Note: Please don't be confused by "reg_map" name. This has nothing
> > >       to do with Linux regmap API ;-)
> > > 
> > > What happens is that different USB IDs have different values for
> > > each register. So, for instance, the UART enable register is set to
> > > either one of the following values, depending on the value of
> > > udev->descriptor.idProduct:
> > > 
> > > 	xr21b140x_reg_map.uart_enable_addr = 0x00;
> > > 	xr21b1411_reg_map.uart_enable_addr = 0xc00;
> > > 	xr21v141x_reg_map.uart_enable_addr = 0x03;
> > > 	xr21b142x_reg_map.uart_enable_addr = 0x00;
> > > 
> > > There are other values that depend on the probing time detection,
> > > based on other USB descriptors. Those set several fields at the
> > > priv data that would allow to properly map the registers.
> > > 
> > > Also, there are 4 models that support multiple channels. On those,
> > > there are one pair of register get/set for each channel.
> > > 
> > > -
> > > 
> > > In summary, while supporting just 04e2:1411 there's no need for
> > > a private struct, in order to properly support the other models,
> > > some autodetection is needed. The best way of doing that is to
> > > re-add the .probe method and adding a priv struct.
> > > 
> > > As I dunno why this was dropped in the first place, I'm wondering
> > > if it would be ok to re-introduce them.  
> > 
> > Sure. It was just not needed if we were only going to support one model.
> > 
> > > To be clear: my main focus here is just to avoid needing to use 
> > > Windows in order to use the serial console of the hardware with
> > > the 0x1424 variant ;-)
> > > 
> > > I can't test the driver with the other hardware, but, IMHO, instead
> > > of adding a hack to support 0x1424, the better (but more painful)
> > > would be to re-add the auto-detection part and support for the
> > > other models.  
> > 
> > Sounds good to me. 
> 
> While testing the xr_serial (as currently merged), I opted to apply
> the patches on the top of vanilla Kernel 5.11 - as it sounds too risky
> to use linux-next so early on a new development cycle :-)
> 
> There, I'm getting an OOPS:
> 
> 	[   30.261291] BUG: kernel NULL pointer dereference, address: 00000000000000a8
> 	[   30.261375] #PF: supervisor write access in kernel mode
> 	[   30.261438] #PF: error_code(0x0002) - not-present page
> 	[   30.261500] PGD 0 P4D 0 
> 	[   30.261539] Oops: 0002 [#1] SMP PTI
> 	[   30.261586] CPU: 2 PID: 686 Comm: kworker/2:3 Not tainted 5.11.0+ #14
> 	[   30.261666] Hardware name:  /NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 05/17/2019
> 	[   30.261757] Workqueue: usb_hub_wq hub_event
> 	[   30.261816] RIP: 0010:mutex_lock+0x1e/0x40
> 	[   30.261875] Code: c3 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 41 54 49 89 fc e8 8d dd ff ff 31 c0 65 48 8b 14 25 c0 7b 01 00 <f0> 49 0f b1 14 24 75 04 41 5c 5d c3 4c 89 e7 e8 ae ff ff ff 41 5c
> 	[   30.262076] RSP: 0018:ffffb937c0767b70 EFLAGS: 00010246
> 	[   30.262140] RAX: 0000000000000000 RBX: ffff95e71ef75430 RCX: 0000000000000027
> 	[   30.262223] RDX: ffff95e70597b000 RSI: 00000000ffffdfff RDI: 00000000000000a8
> 	[   30.262305] RBP: ffffb937c0767b78 R08: ffff95ea76d18ac0 R09: ffffb937c0767948
> 	[   30.262387] R10: 0000000000000001 R11: 0000000000000001 R12: 00000000000000a8
> 	[   30.262469] R13: 0000000000000000 R14: ffff95e71ef75400 R15: 0000000000000000
> 	[   30.262551] FS:  0000000000000000(0000) GS:ffff95ea76d00000(0000) knlGS:0000000000000000
> 	[   30.262645] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 	[   30.262713] CR2: 00000000000000a8 CR3: 000000042be0a002 CR4: 00000000003706e0
> 	[   30.262796] Call Trace:
> 	[   30.262832]  usb_serial_disconnect+0x33/0x140
> 	[   30.262897]  usb_unbind_interface+0x8c/0x260
> 	[   30.262957]  device_release_driver_internal+0x103/0x1d0
> 	[   30.263026]  device_release_driver+0x12/0x20
> 	[   30.263083]  bus_remove_device+0xe1/0x150
> 	[   30.263140]  device_del+0x192/0x3f0
> 	[   30.263188]  ? usb_remove_ep_devs+0x1f/0x30
> 	[   30.263244]  usb_disable_device+0x95/0x1c0
> 	[   30.263300]  usb_disconnect+0xc0/0x270
> 	[   30.263350]  hub_event+0xa2e/0x1620
> 
> After adding this hack:
> 
> <snip>
> --- a/drivers/usb/serial/usb-serial.c
> +++ b/drivers/usb/serial/usb-serial.c
> @@ -1081,6 +1081,11 @@ static void usb_serial_disconnect(struct usb_interface *interface)
>         struct usb_serial_port *port;
>         struct tty_struct *tty;
>  
> +       if (!serial) {
> +               dev_err(dev, "%s: Serial pointer is NULL!!!\n", __func__);
> +               return;
> +       }
> +
>         usb_serial_console_disconnect(serial);
>  
>         mutex_lock(&serial->disc_mutex);
> </snip>
> 
> It works fine:
> 
> 	[  283.005625] xr_serial 2-1:1.1: xr_serial converter detected
> 	[  283.005868] usb 2-1: xr_serial converter now attached to ttyUSB0
> 	[  283.007284] printk: console [ttyUSB0] enabled
> 	[  284.444419] usb 2-1: USB disconnect, device number 5
> 	[  284.444520] xr_serial 2-1:1.0: usb_serial_disconnect: Serial pointer is NULL!!!
> 	[  284.444894] printk: console [ttyUSB0] disabled
> 	[  284.445091] xr_serial ttyUSB0: xr_serial converter now disconnected from ttyUSB0
> 	[  284.445141] xr_disconnect
> 	[  284.445156] xr_serial 2-1:1.1: device disconnected
> 
> I'm not sure if the bug is at xr_serial or if it is inside usb-serial.c.
> 
> Any ideas?

Answering myself, as those devices may have two different interfaces
(one for control and another one for data), I suspect that the
driver needs to manually call usb_set_intfdata() after detecting the
data interface.

Thanks,
Mauro

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Johan Hovold]

On Thu, Feb 25, 2021 at 07:04:05PM +0100, Mauro Carvalho Chehab wrote:
> Em Thu, 25 Feb 2021 18:58:20 +0100
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> > While testing the xr_serial (as currently merged), I opted to apply
> > the patches on the top of vanilla Kernel 5.11 - as it sounds too risky
> > to use linux-next so early on a new development cycle :-)
> > 
> > There, I'm getting an OOPS:
> > 
> > 	[   30.261291] BUG: kernel NULL pointer dereference, address: 00000000000000a8
> > 	[   30.261375] #PF: supervisor write access in kernel mode
> > 	[   30.261438] #PF: error_code(0x0002) - not-present page
> > 	[   30.261500] PGD 0 P4D 0 
> > 	[   30.261539] Oops: 0002 [#1] SMP PTI
> > 	[   30.261586] CPU: 2 PID: 686 Comm: kworker/2:3 Not tainted 5.11.0+ #14
> > 	[   30.261666] Hardware name:  /NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 05/17/2019
> > 	[   30.261757] Workqueue: usb_hub_wq hub_event
> > 	[   30.261816] RIP: 0010:mutex_lock+0x1e/0x40

> > 	[   30.262796] Call Trace:
> > 	[   30.262832]  usb_serial_disconnect+0x33/0x140
> > 	[   30.262897]  usb_unbind_interface+0x8c/0x260
> > 	[   30.262957]  device_release_driver_internal+0x103/0x1d0
> > 	[   30.263026]  device_release_driver+0x12/0x20
> > 	[   30.263083]  bus_remove_device+0xe1/0x150
> > 	[   30.263140]  device_del+0x192/0x3f0
> > 	[   30.263188]  ? usb_remove_ep_devs+0x1f/0x30
> > 	[   30.263244]  usb_disable_device+0x95/0x1c0
> > 	[   30.263300]  usb_disconnect+0xc0/0x270
> > 	[   30.263350]  hub_event+0xa2e/0x1620
> > 
> > After adding this hack:
> > 
> > <snip>
> > --- a/drivers/usb/serial/usb-serial.c
> > +++ b/drivers/usb/serial/usb-serial.c
> > @@ -1081,6 +1081,11 @@ static void usb_serial_disconnect(struct usb_interface *interface)
> >         struct usb_serial_port *port;
> >         struct tty_struct *tty;
> >  
> > +       if (!serial) {
> > +               dev_err(dev, "%s: Serial pointer is NULL!!!\n", __func__);
> > +               return;
> > +       }
> > +
> >         usb_serial_console_disconnect(serial);
> >  
> >         mutex_lock(&serial->disc_mutex);
> > </snip>
> > 
> > It works fine:
> > 
> > 	[  283.005625] xr_serial 2-1:1.1: xr_serial converter detected
> > 	[  283.005868] usb 2-1: xr_serial converter now attached to ttyUSB0
> > 	[  283.007284] printk: console [ttyUSB0] enabled
> > 	[  284.444419] usb 2-1: USB disconnect, device number 5
> > 	[  284.444520] xr_serial 2-1:1.0: usb_serial_disconnect: Serial pointer is NULL!!!
> > 	[  284.444894] printk: console [ttyUSB0] disabled
> > 	[  284.445091] xr_serial ttyUSB0: xr_serial converter now disconnected from ttyUSB0
> > 	[  284.445141] xr_disconnect
> > 	[  284.445156] xr_serial 2-1:1.1: device disconnected
> > 
> > I'm not sure if the bug is at xr_serial or if it is inside usb-serial.c.
> > 
> > Any ideas?
> 
> Answering myself, as those devices may have two different interfaces
> (one for control and another one for data), I suspect that the
> driver needs to manually call usb_set_intfdata() after detecting the
> data interface.

Thanks for reporting this.

I'm afraid it's a bit more involved than that; we'd need to add support
to USB-serial core for managing a sibling interface and either one being
disconnected first. This has implications for suspend as well.

I think we should just not claim the control interface for now since it
not currently used by the driver. I'll send a fix.

Johan

Re: [PATCH v5 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver

[Mauro Carvalho Chehab]

Em Fri, 26 Feb 2021 11:07:07 +0100
Johan Hovold <johan@kernel.org> escreveu:

> On Thu, Feb 25, 2021 at 07:04:05PM +0100, Mauro Carvalho Chehab wrote:
> > Em Thu, 25 Feb 2021 18:58:20 +0100
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:  
> 
> > > While testing the xr_serial (as currently merged), I opted to apply
> > > the patches on the top of vanilla Kernel 5.11 - as it sounds too risky
> > > to use linux-next so early on a new development cycle :-)
> > > 
> > > There, I'm getting an OOPS:
> > > 
> > > 	[   30.261291] BUG: kernel NULL pointer dereference, address: 00000000000000a8
> > > 	[   30.261375] #PF: supervisor write access in kernel mode
> > > 	[   30.261438] #PF: error_code(0x0002) - not-present page
> > > 	[   30.261500] PGD 0 P4D 0 
> > > 	[   30.261539] Oops: 0002 [#1] SMP PTI
> > > 	[   30.261586] CPU: 2 PID: 686 Comm: kworker/2:3 Not tainted 5.11.0+ #14
> > > 	[   30.261666] Hardware name:  /NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 05/17/2019
> > > 	[   30.261757] Workqueue: usb_hub_wq hub_event
> > > 	[   30.261816] RIP: 0010:mutex_lock+0x1e/0x40  
> 
> > > 	[   30.262796] Call Trace:
> > > 	[   30.262832]  usb_serial_disconnect+0x33/0x140
> > > 	[   30.262897]  usb_unbind_interface+0x8c/0x260
> > > 	[   30.262957]  device_release_driver_internal+0x103/0x1d0
> > > 	[   30.263026]  device_release_driver+0x12/0x20
> > > 	[   30.263083]  bus_remove_device+0xe1/0x150
> > > 	[   30.263140]  device_del+0x192/0x3f0
> > > 	[   30.263188]  ? usb_remove_ep_devs+0x1f/0x30
> > > 	[   30.263244]  usb_disable_device+0x95/0x1c0
> > > 	[   30.263300]  usb_disconnect+0xc0/0x270
> > > 	[   30.263350]  hub_event+0xa2e/0x1620
> > > 
> > > After adding this hack:
> > > 
> > > <snip>
> > > --- a/drivers/usb/serial/usb-serial.c
> > > +++ b/drivers/usb/serial/usb-serial.c
> > > @@ -1081,6 +1081,11 @@ static void usb_serial_disconnect(struct usb_interface *interface)
> > >         struct usb_serial_port *port;
> > >         struct tty_struct *tty;
> > >  
> > > +       if (!serial) {
> > > +               dev_err(dev, "%s: Serial pointer is NULL!!!\n", __func__);
> > > +               return;
> > > +       }
> > > +
> > >         usb_serial_console_disconnect(serial);
> > >  
> > >         mutex_lock(&serial->disc_mutex);
> > > </snip>
> > > 
> > > It works fine:
> > > 
> > > 	[  283.005625] xr_serial 2-1:1.1: xr_serial converter detected
> > > 	[  283.005868] usb 2-1: xr_serial converter now attached to ttyUSB0
> > > 	[  283.007284] printk: console [ttyUSB0] enabled
> > > 	[  284.444419] usb 2-1: USB disconnect, device number 5
> > > 	[  284.444520] xr_serial 2-1:1.0: usb_serial_disconnect: Serial pointer is NULL!!!
> > > 	[  284.444894] printk: console [ttyUSB0] disabled
> > > 	[  284.445091] xr_serial ttyUSB0: xr_serial converter now disconnected from ttyUSB0
> > > 	[  284.445141] xr_disconnect
> > > 	[  284.445156] xr_serial 2-1:1.1: device disconnected
> > > 
> > > I'm not sure if the bug is at xr_serial or if it is inside usb-serial.c.
> > > 
> > > Any ideas?  
> > 
> > Answering myself, as those devices may have two different interfaces
> > (one for control and another one for data), I suspect that the
> > driver needs to manually call usb_set_intfdata() after detecting the
> > data interface.  
> 
> Thanks for reporting this.
> 
> I'm afraid it's a bit more involved than that; we'd need to add support
> to USB-serial core for managing a sibling interface and either one being
> disconnected first. This has implications for suspend as well.
> 
> I think we should just not claim the control interface for now since it
> not currently used by the driver. I'll send a fix.

Thanks!

Yeah, I had a similar patch, moving out from 
usb_driver_release_interface(), as it ends calling the serial
disconnect method.

Btw, for other xr_serial models, the driver will need to use
the control interface, as it is used to do things like
setting up the number of bits, stop bits and parity.

I'm still working on the patch.

There, I'm using usb_get_intf() in order to avoid use-after-free
at disconnect time, but I'm pretty sure something else is needed
due to PM.

FYI, that's the probe/disconnect part of the changeset. It works
fine with both XR21B1424 and XR21V1410 models.

I'm not sure if the probing part will work for the other ones. The
original driver does something a lot more complex, parsing the
CDC union tables that are present only at the control interfaces.

Adding support for parsing it, while keeping using usb-serial
as-is would be very difficult.

Perhaps the right solution would be to let usb-serial parse the
CDC union structs, for the drivers that would need that.

Maybe we could add a new probe_cdc ops (or something similar)
that would enable some logic there for it to work with separate
data and control interfaces.

Thanks,
Mauro

diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index 483d07dee19d..679ac10be963 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -545,39 +878,70 @@ static void xr_close(struct usb_serial_port *port)
 
 static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
 {
-	struct usb_driver *driver = serial->type->usb_driver;
-	struct usb_interface *control_interface;
-	int ret;
-
-	/* Don't bind to control interface */
-	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
+	struct usb_interface *intf = serial->interface;
+	struct usb_endpoint_descriptor *data_ep;
+	struct usb_device *udev = serial->dev;
+	struct xr_port_private *port_priv;
+	struct usb_interface *ctrl_intf;
+	int ifnum, ctrl_ifnum;
+
+	/* Attach only data interfaces */
+	ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
+	if (!(ifnum % 2))
 		return -ENODEV;
 
-	/* But claim the control interface during data interface probe */
-	control_interface = usb_ifnum_to_if(serial->dev, 0);
-	if (!control_interface)
-		return -ENODEV;
+	/* Control interfaces are the even numbers */
+	ctrl_ifnum = ifnum - ifnum % 2;
 
-	ret = usb_driver_claim_interface(driver, control_interface, NULL);
-	if (ret) {
-		dev_err(&serial->interface->dev, "Failed to claim control interface\n");
-		return ret;
-	}
+	port_priv = kzalloc(sizeof(*port_priv), GFP_KERNEL);
+	if (!port_priv)
+		return -ENOMEM;
+
+	data_ep = &intf->cur_altsetting->endpoint[0].desc;
+	ctrl_intf = usb_ifnum_to_if(udev, ctrl_ifnum);
+
+	port_priv->data_if = intf;
+	port_priv->control_if = usb_get_intf(ctrl_intf);
+	port_priv->model = id->driver_info;
+	port_priv->channel = data_ep->bEndpointAddress;
+
+	usb_set_serial_data(serial, port_priv);
+
+	dev_info(&intf->dev, "port %d registered: control if: %d, data if: %d\n",
+		 ifnum / 2, ctrl_ifnum, ifnum);
 
 	return 0;
 }
 
 static void xr_disconnect(struct usb_serial *serial)
 {
+	struct xr_port_private *port_priv = usb_get_serial_data(serial);
 	struct usb_driver *driver = serial->type->usb_driver;
-	struct usb_interface *control_interface;
 
-	control_interface = usb_ifnum_to_if(serial->dev, 0);
-	usb_driver_release_interface(driver, control_interface);
+	usb_put_intf(port_priv->control_if);
+
+	usb_driver_release_interface(driver, port_priv->data_if);
 }
 
 static const struct usb_device_id id_table[] = {
-	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
+	{ USB_DEVICE(0x04e2, 0x1400), .driver_info = XR2280X},
+	{ USB_DEVICE(0x04e2, 0x1401), .driver_info = XR2280X},
+	{ USB_DEVICE(0x04e2, 0x1402), .driver_info = XR2280X},
+	{ USB_DEVICE(0x04e2, 0x1403), .driver_info = XR2280X},
+
+	{ USB_DEVICE(0x04e2, 0x1410), .driver_info = XR21V141X},
+	{ USB_DEVICE(0x04e2, 0x1411), .driver_info = XR21B1411},
+	{ USB_DEVICE(0x04e2, 0x1412), .driver_info = XR21V141X},
+	{ USB_DEVICE(0x04e2, 0x1414), .driver_info = XR21V141X},
+
+	{ USB_DEVICE(0x04e2, 0x1420), .driver_info = XR21B142X},
+#if 0
+	/* FIXME: this one has just control interface! */
+	{ USB_DEVICE(0x04e2, 0x1421), .driver_info = XR21B1421},
+#endif
+	{ USB_DEVICE(0x04e2, 0x1422), .driver_info = XR21B142X},
+	{ USB_DEVICE(0x04e2, 0x1424), .driver_info = XR21B142X},
+
 	{ }
 };
 MODULE_DEVICE_TABLE(usb, id_table);

[PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Manivannan Sadhasivam]

Add gpiochip support for Maxlinear/Exar USB to serial converter
for controlling the available gpios.

Inspired from cp210x usb to serial converter driver.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-gpio@vger.kernel.org
Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/usb/serial/xr_serial.c | 267 ++++++++++++++++++++++++++++++++-
 1 file changed, 261 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index e166916554d6..ca63527a5310 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -9,6 +9,7 @@
  * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -16,6 +17,28 @@
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
 
+#ifdef CONFIG_GPIOLIB
+enum gpio_pins {
+	GPIO_RI = 0,
+	GPIO_CD,
+	GPIO_DSR,
+	GPIO_DTR,
+	GPIO_CTS,
+	GPIO_RTS,
+	GPIO_MAX,
+};
+#endif
+
+struct xr_port_private {
+#ifdef CONFIG_GPIOLIB
+	struct gpio_chip gc;
+	bool gpio_registered;
+	enum gpio_pins pin_status[GPIO_MAX];
+#endif
+	struct mutex gpio_lock;	/* protects GPIO state */
+	bool port_open;
+};
+
 struct xr_txrx_clk_mask {
 	u16 tx;
 	u16 rx0;
@@ -106,6 +129,8 @@ struct xr_txrx_clk_mask {
 #define XR21V141X_REG_GPIO_CLR		0x1e
 #define XR21V141X_REG_GPIO_STATUS	0x1f
 
+static int xr_cts_rts_check(struct xr_port_private *port_priv);
+
 static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg,
 		      u8 val)
 {
@@ -309,6 +334,7 @@ static int xr_uart_disable(struct usb_serial_port *port)
 static void xr_set_flow_mode(struct tty_struct *tty,
 			     struct usb_serial_port *port)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
 	unsigned int cflag = tty->termios.c_cflag;
 	int ret;
 	u8 flow, gpio_mode;
@@ -318,6 +344,17 @@ static void xr_set_flow_mode(struct tty_struct *tty,
 		return;
 
 	if (cflag & CRTSCTS) {
+		/*
+		 * Check if the CTS/RTS pins are occupied by GPIOLIB. If yes,
+		 * then use no flow control.
+		 */
+		if (xr_cts_rts_check(port_priv)) {
+			dev_dbg(&port->dev,
+				"CTS/RTS pins are occupied, so disabling flow control\n");
+			flow = XR21V141X_UART_FLOW_MODE_NONE;
+			goto exit;
+		}
+
 		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
 
 		/*
@@ -341,6 +378,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
 		flow = XR21V141X_UART_FLOW_MODE_NONE;
 	}
 
+exit:
 	/*
 	 * As per the datasheet, UART needs to be disabled while writing to
 	 * FLOW_CONTROL register.
@@ -355,18 +393,22 @@ static void xr_set_flow_mode(struct tty_struct *tty,
 static int xr_tiocmset_port(struct usb_serial_port *port,
 			    unsigned int set, unsigned int clear)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
 	u8 gpio_set = 0;
 	u8 gpio_clr = 0;
 	int ret = 0;
 
-	/* Modem control pins are active low, so set & clr are swapped */
-	if (set & TIOCM_RTS)
+	/*
+	 * Modem control pins are active low, so set & clr are swapped. And
+	 * ignore the pins that are occupied by GPIOLIB.
+	 */
+	if ((set & TIOCM_RTS) && !(port_priv->pin_status[GPIO_RTS]))
 		gpio_clr |= XR21V141X_UART_MODE_RTS;
-	if (set & TIOCM_DTR)
+	if ((set & TIOCM_DTR) && !(port_priv->pin_status[GPIO_DTR]))
 		gpio_clr |= XR21V141X_UART_MODE_DTR;
-	if (clear & TIOCM_RTS)
+	if ((clear & TIOCM_RTS) && !(port_priv->pin_status[GPIO_RTS]))
 		gpio_set |= XR21V141X_UART_MODE_RTS;
-	if (clear & TIOCM_DTR)
+	if ((clear & TIOCM_DTR) && !(port_priv->pin_status[GPIO_DTR]))
 		gpio_set |= XR21V141X_UART_MODE_DTR;
 
 	/* Writing '0' to gpio_{set/clr} bits has no effect, so no need to do */
@@ -464,6 +506,7 @@ static void xr_set_termios(struct tty_struct *tty,
 
 static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
 	int ret;
 
 	ret = xr_uart_enable(port);
@@ -482,13 +525,23 @@ static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
 		return ret;
 	}
 
+	mutex_lock(&port_priv->gpio_lock);
+	port_priv->port_open = true;
+	mutex_unlock(&port_priv->gpio_lock);
+
 	return 0;
 }
 
 static void xr_close(struct usb_serial_port *port)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
 	usb_serial_generic_close(port);
 
+	mutex_lock(&port_priv->gpio_lock);
+	port_priv->port_open = false;
+	mutex_unlock(&port_priv->gpio_lock);
+
 	xr_uart_disable(port);
 }
 
@@ -553,13 +606,215 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
 	xr_set_reg_uart(port, XR21V141X_REG_TX_BREAK, state);
 }
 
-static int xr_port_probe(struct usb_serial_port *port)
+#ifdef CONFIG_GPIOLIB
+
+static int xr_cts_rts_check(struct xr_port_private *port_priv)
 {
+	if (port_priv->pin_status[GPIO_RTS] || port_priv->pin_status[GPIO_CTS])
+		return -EBUSY;
+
 	return 0;
 }
 
+static int xr_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret = 0;
+
+	mutex_lock(&port_priv->gpio_lock);
+	/*
+	 * Do not proceed if the port is open. This is done to avoid changing
+	 * the GPIO configuration used by the serial driver.
+	 */
+	if (port_priv->port_open) {
+		ret = -EBUSY;
+		goto err_out;
+	}
+
+	/* Mark the GPIO pin as busy */
+	port_priv->pin_status[offset] = true;
+
+err_out:
+	mutex_unlock(&port_priv->gpio_lock);
+
+	return ret;
+}
+
+static void xr_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	mutex_lock(&port_priv->gpio_lock);
+	/*
+	 * Do not proceed if the port is open. This is done to avoid toggling
+	 * of pins suddenly when the serial port is in use.
+	 */
+	if (port_priv->port_open)
+		goto err_out;
+
+	/* Mark the GPIO pin as free */
+	port_priv->pin_status[offset] = false;
+
+err_out:
+	mutex_unlock(&port_priv->gpio_lock);
+}
+
+static int xr_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	u8 gpio_status;
+	int ret;
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_STATUS, &gpio_status);
+	if (ret)
+		return ret;
+
+	return !!(gpio_status & BIT(gpio));
+}
+
+static void xr_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+
+	if (val)
+		xr_set_reg_uart(port, XR21V141X_REG_GPIO_SET, BIT(gpio));
+	else
+		xr_set_reg_uart(port, XR21V141X_REG_GPIO_CLR, BIT(gpio));
+}
+
+static int xr_gpio_direction_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	u8 gpio_dir;
+	int ret;
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_DIR, &gpio_dir);
+	if (ret)
+		return ret;
+
+	/* Logic 0 = input and Logic 1 = output */
+	return !(gpio_dir & BIT(gpio));
+}
+
+static int xr_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	u8 gpio_dir;
+	int ret;
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_DIR, &gpio_dir);
+	if (ret)
+		return ret;
+
+	gpio_dir &= ~BIT(gpio);
+
+	return xr_set_reg_uart(port, XR21V141X_REG_GPIO_DIR, gpio_dir);
+}
+
+static int xr_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
+				    int val)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	u8 gpio_dir;
+	int ret;
+
+	xr_gpio_set(gc, gpio, val);
+
+	ret = xr_get_reg_uart(port, XR21V141X_REG_GPIO_DIR, &gpio_dir);
+	if (ret)
+		return ret;
+
+	gpio_dir |= BIT(gpio);
+
+	ret = xr_set_reg_uart(port, XR21V141X_REG_GPIO_DIR, gpio_dir);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int xr_gpio_init(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+	int ret = 0;
+
+	port_priv->gc.ngpio = 6;
+	port_priv->gc.label = "xr_gpios";
+	port_priv->gc.request = xr_gpio_request;
+	port_priv->gc.free = xr_gpio_free;
+	port_priv->gc.get_direction = xr_gpio_direction_get;
+	port_priv->gc.direction_input = xr_gpio_direction_input;
+	port_priv->gc.direction_output = xr_gpio_direction_output;
+	port_priv->gc.get = xr_gpio_get;
+	port_priv->gc.set = xr_gpio_set;
+	port_priv->gc.owner = THIS_MODULE;
+	port_priv->gc.parent = &port->dev;
+	port_priv->gc.base = -1;
+	port_priv->gc.can_sleep = true;
+
+	ret = gpiochip_add_data(&port_priv->gc, port);
+	if (!ret)
+		port_priv->gpio_registered = true;
+
+	return ret;
+}
+
+static void xr_gpio_remove(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	if (port_priv->gpio_registered) {
+		gpiochip_remove(&port_priv->gc);
+		port_priv->gpio_registered = false;
+	}
+}
+
+#else
+
+static int xr_gpio_init(struct usb_serial_port *port)
+{
+	return 0;
+}
+
+static void xr_gpio_remove(struct usb_serial_port *port)
+{
+}
+
+static int xr_cts_rts_check(struct xr_port_private *port_priv)
+{
+	return 0;
+}
+
+#endif
+
+static int xr_port_probe(struct usb_serial_port *port)
+{
+	struct xr_port_private *port_priv;
+	int ret;
+
+	port_priv = kzalloc(sizeof(*port_priv), GFP_KERNEL);
+	if (!port_priv)
+		return -ENOMEM;
+
+	usb_set_serial_port_data(port, port_priv);
+	mutex_init(&port_priv->gpio_lock);
+
+	ret = xr_gpio_init(port);
+	if (ret)
+		kfree(port_priv);
+
+	return ret;
+}
+
 static int xr_port_remove(struct usb_serial_port *port)
 {
+	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+	xr_gpio_remove(port);
+	kfree(port_priv);
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:

> Add gpiochip support for Maxlinear/Exar USB to serial converter
> for controlling the available gpios.
>
> Inspired from cp210x usb to serial converter driver.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

This looks good to me overall, provided that it plays well with the
serial port.

One minor notice:

> +enum gpio_pins {
> +       GPIO_RI = 0,
> +       GPIO_CD,
> +       GPIO_DSR,
> +       GPIO_DTR,
> +       GPIO_CTS,
> +       GPIO_RTS,
> +       GPIO_MAX,
> +};

You know the names of the pins...

> +       port_priv->gc.ngpio = 6;
> +       port_priv->gc.label = "xr_gpios";
> +       port_priv->gc.request = xr_gpio_request;
> +       port_priv->gc.free = xr_gpio_free;
> +       port_priv->gc.get_direction = xr_gpio_direction_get;
> +       port_priv->gc.direction_input = xr_gpio_direction_input;
> +       port_priv->gc.direction_output = xr_gpio_direction_output;
> +       port_priv->gc.get = xr_gpio_get;
> +       port_priv->gc.set = xr_gpio_set;
> +       port_priv->gc.owner = THIS_MODULE;
> +       port_priv->gc.parent = &port->dev;
> +       port_priv->gc.base = -1;
> +       port_priv->gc.can_sleep = true;

So assign port_priv->gc.names here as well with an array
of strings with the names ("RI", "CD", ... etc).
This makes it look really nice in userspace if you do
e.g. "lsgpio".

With that:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Tue, Dec 01, 2020 at 03:37:38PM +0100, Linus Walleij wrote:
> On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
> 
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.

> One minor notice:
> 
> > +enum gpio_pins {
> > +       GPIO_RI = 0,
> > +       GPIO_CD,
> > +       GPIO_DSR,
> > +       GPIO_DTR,
> > +       GPIO_CTS,
> > +       GPIO_RTS,
> > +       GPIO_MAX,
> > +};
> 
> You know the names of the pins...
> 
> > +       port_priv->gc.ngpio = 6;
> > +       port_priv->gc.label = "xr_gpios";
> > +       port_priv->gc.request = xr_gpio_request;
> > +       port_priv->gc.free = xr_gpio_free;
> > +       port_priv->gc.get_direction = xr_gpio_direction_get;
> > +       port_priv->gc.direction_input = xr_gpio_direction_input;
> > +       port_priv->gc.direction_output = xr_gpio_direction_output;
> > +       port_priv->gc.get = xr_gpio_get;
> > +       port_priv->gc.set = xr_gpio_set;
> > +       port_priv->gc.owner = THIS_MODULE;
> > +       port_priv->gc.parent = &port->dev;
> > +       port_priv->gc.base = -1;
> > +       port_priv->gc.can_sleep = true;
> 
> So assign port_priv->gc.names here as well with an array
> of strings with the names ("RI", "CD", ... etc).
> This makes it look really nice in userspace if you do
> e.g. "lsgpio".

Last time we tried that gpiolib still used a flat namespace so that you
can't have have more than one device using the same names. Unless that
has changed this is a no-go. See

	https://lore.kernel.org/r/20180930122703.7115-1-johan@kernel.org

for our previous discussion about this.

Johan

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Tue, Dec 1, 2020 at 4:50 PM Johan Hovold <johan@kernel.org> wrote:
> On Tue, Dec 01, 2020 at 03:37:38PM +0100, Linus Walleij wrote:
> > On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:

> > You know the names of the pins...
> >
> > > +       port_priv->gc.ngpio = 6;
> > > +       port_priv->gc.label = "xr_gpios";
> > > +       port_priv->gc.request = xr_gpio_request;
> > > +       port_priv->gc.free = xr_gpio_free;
> > > +       port_priv->gc.get_direction = xr_gpio_direction_get;
> > > +       port_priv->gc.direction_input = xr_gpio_direction_input;
> > > +       port_priv->gc.direction_output = xr_gpio_direction_output;
> > > +       port_priv->gc.get = xr_gpio_get;
> > > +       port_priv->gc.set = xr_gpio_set;
> > > +       port_priv->gc.owner = THIS_MODULE;
> > > +       port_priv->gc.parent = &port->dev;
> > > +       port_priv->gc.base = -1;
> > > +       port_priv->gc.can_sleep = true;
> >
> > So assign port_priv->gc.names here as well with an array
> > of strings with the names ("RI", "CD", ... etc).
> > This makes it look really nice in userspace if you do
> > e.g. "lsgpio".
>
> Last time we tried that gpiolib still used a flat namespace so that you
> can't have have more than one device using the same names. Unless that
> has changed this is a no-go. See
>
>         https://lore.kernel.org/r/20180930122703.7115-1-johan@kernel.org
>
> for our previous discussion about this.

Hm hm yeah we actually put in a nasty warning there since:

                gpio = gpio_name_to_desc(gc->names[i]);
                if (gpio)
                        dev_warn(&gdev->dev,
                                 "Detected name collision for GPIO name '%s'\n",
                                 gc->names[i]);


A better approach might be to create an array of names
prepended with something device-unique like the USB
bus topology? Or do we need a helper to help naming the
GPIOs? What would be helpful here?

name = kasprintf(GFP_KERNEL, "%s-NAME", topology_str);

Yours,
Linus Walleij

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Sat, Dec 05, 2020 at 11:21:09PM +0100, Linus Walleij wrote:
> On Tue, Dec 1, 2020 at 4:50 PM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Dec 01, 2020 at 03:37:38PM +0100, Linus Walleij wrote:
> > > On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
> 
> > > You know the names of the pins...
> > >
> > > > +       port_priv->gc.ngpio = 6;
> > > > +       port_priv->gc.label = "xr_gpios";
> > > > +       port_priv->gc.request = xr_gpio_request;
> > > > +       port_priv->gc.free = xr_gpio_free;
> > > > +       port_priv->gc.get_direction = xr_gpio_direction_get;
> > > > +       port_priv->gc.direction_input = xr_gpio_direction_input;
> > > > +       port_priv->gc.direction_output = xr_gpio_direction_output;
> > > > +       port_priv->gc.get = xr_gpio_get;
> > > > +       port_priv->gc.set = xr_gpio_set;
> > > > +       port_priv->gc.owner = THIS_MODULE;
> > > > +       port_priv->gc.parent = &port->dev;
> > > > +       port_priv->gc.base = -1;
> > > > +       port_priv->gc.can_sleep = true;
> > >
> > > So assign port_priv->gc.names here as well with an array
> > > of strings with the names ("RI", "CD", ... etc).
> > > This makes it look really nice in userspace if you do
> > > e.g. "lsgpio".
> >
> > Last time we tried that gpiolib still used a flat namespace so that you
> > can't have have more than one device using the same names. Unless that
> > has changed this is a no-go. See
> >
> >         https://lore.kernel.org/r/20180930122703.7115-1-johan@kernel.org
> >
> > for our previous discussion about this.
> 
> Hm hm yeah we actually put in a nasty warning there since:
> 
>                 gpio = gpio_name_to_desc(gc->names[i]);
>                 if (gpio)
>                         dev_warn(&gdev->dev,
>                                  "Detected name collision for GPIO name '%s'\n",
>                                  gc->names[i]);
> 
> 
> A better approach might be to create an array of names
> prepended with something device-unique like the USB
> bus topology? Or do we need a helper to help naming the
> GPIOs? What would be helpful here?
> 
> name = kasprintf(GFP_KERNEL, "%s-NAME", topology_str);

Well we started discussing this back when we only had the sysfs
interface which suffered from the same problem. I thought the chardev
interface was supposed to get rid of the assumption of a flat name
space? Perhaps in v3 of the ABI. ;P

If this is too built into the new chardev interface as well to be fixed
up, a unique prefix is the only way to go. Perhaps gpiolib can just
prefix it with the controller name?

	gpiochip508-CBUS0

Based on a hotpluggable bus flag? But what about any other non-pluggable
IC, which provides a few named GPIO lines and of which there could be
more than one in a system?

The topology is already encoded in sysfs and it seems backwards to have
each and every gpio driver reconstruct it.

Johan

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Tue, Dec 8, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:
> [Me]

> > A better approach might be to create an array of names
> > prepended with something device-unique like the USB
> > bus topology? Or do we need a helper to help naming the
> > GPIOs? What would be helpful here?
> >
> > name = kasprintf(GFP_KERNEL, "%s-NAME", topology_str);
>
> Well we started discussing this back when we only had the sysfs
> interface which suffered from the same problem. I thought the chardev
> interface was supposed to get rid of the assumption of a flat name
> space? Perhaps in v3 of the ABI. ;P

It's "mostly true" that the line names are unique per-chip actually,
because people don't like the nasty warning message. I wonder
if anything would really break if I go in and make a patch to
enforce it, since all drivers passing ->names in the gpiochip
are in the kernel we can check them all.

If the names are unique-per-chip, we can add a restriction like this
with the requirement:

depends on !GPIO_SYSFS

so it can't even be compiled in if someone is using the sysfs.

That should solve the situation where people are (ab)using
the sysfs and getting name collisions as a result.

Then it should be fine for any driver to provide a names array
provided all the names are unique on that gpiochip.

I doubt it would break anything, but let's see what Geert says.
He has some special usecases in the gpio-aggregator driver
which will incidentally look for just linenames when
aggregating gpios, but I feel it is a bit thick for it to work
with multiple hot-pluggable GPIO chips as well, I don't think
that is its usecase. (We all want to be perfect but...)

> But what about any other non-pluggable
> IC, which provides a few named GPIO lines and of which there could be
> more than one in a system?

I think if there are such, and the lines are unique per-chip
we should make the drivers depend on !GPIO_SYSFS.

> The topology is already encoded in sysfs and it seems backwards to have
> each and every gpio driver reconstruct it.

I agree.

I think if this driver already has unique line-names per-gpiochip
we could actually make it depend on !GPIO_SYSFS and
just add the names.

Yours,
Linus Walleij

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Manivannan Sadhasivam]

On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:
> On Tue, Dec 8, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:
> > [Me]
> 
> > > A better approach might be to create an array of names
> > > prepended with something device-unique like the USB
> > > bus topology? Or do we need a helper to help naming the
> > > GPIOs? What would be helpful here?
> > >
> > > name = kasprintf(GFP_KERNEL, "%s-NAME", topology_str);
> >
> > Well we started discussing this back when we only had the sysfs
> > interface which suffered from the same problem. I thought the chardev
> > interface was supposed to get rid of the assumption of a flat name
> > space? Perhaps in v3 of the ABI. ;P
> 
> It's "mostly true" that the line names are unique per-chip actually,
> because people don't like the nasty warning message. I wonder
> if anything would really break if I go in and make a patch to
> enforce it, since all drivers passing ->names in the gpiochip
> are in the kernel we can check them all.
> 
> If the names are unique-per-chip, we can add a restriction like this
> with the requirement:
> 
> depends on !GPIO_SYSFS
> 

This sounds reasonable to me.

> so it can't even be compiled in if someone is using the sysfs.
> 
> That should solve the situation where people are (ab)using
> the sysfs and getting name collisions as a result.
> 
> Then it should be fine for any driver to provide a names array
> provided all the names are unique on that gpiochip.
> 
> I doubt it would break anything, but let's see what Geert says.
> He has some special usecases in the gpio-aggregator driver
> which will incidentally look for just linenames when
> aggregating gpios, but I feel it is a bit thick for it to work
> with multiple hot-pluggable GPIO chips as well, I don't think
> that is its usecase. (We all want to be perfect but...)
> 
> > But what about any other non-pluggable
> > IC, which provides a few named GPIO lines and of which there could be
> > more than one in a system?
> 
> I think if there are such, and the lines are unique per-chip
> we should make the drivers depend on !GPIO_SYSFS.
> 
> > The topology is already encoded in sysfs and it seems backwards to have
> > each and every gpio driver reconstruct it.
> 
> I agree.
> 
> I think if this driver already has unique line-names per-gpiochip
> we could actually make it depend on !GPIO_SYSFS and
> just add the names.
> 

Sure thing.

Johan, if you are okay with this I can resubmit incorporating Linus's
suggestion.

Thanks,
Mani

> Yours,
> Linus Walleij

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Tue, Dec 08, 2020 at 06:22:50PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:
> > On Tue, Dec 8, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:

> > I think if this driver already has unique line-names per-gpiochip
> > we could actually make it depend on !GPIO_SYSFS and
> > just add the names.
> 
> Sure thing.
> 
> Johan, if you are okay with this I can resubmit incorporating Linus's
> suggestion.

Let's wait a bit with adding the names.

That can possibly be done as a follow-up too even if removing GPIO_SYSFS
support later is not ideal in case that's the path chosen (we'd have a
similar problem with the existing USB-serial GPIO implementations though).

Johan

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:
> On Tue, Dec 8, 2020 at 10:57 AM Johan Hovold <johan@kernel.org> wrote:

> > Well we started discussing this back when we only had the sysfs
> > interface which suffered from the same problem. I thought the chardev
> > interface was supposed to get rid of the assumption of a flat name
> > space? Perhaps in v3 of the ABI. ;P
> 
> It's "mostly true" that the line names are unique per-chip actually,
> because people don't like the nasty warning message. I wonder
> if anything would really break if I go in and make a patch to
> enforce it, since all drivers passing ->names in the gpiochip
> are in the kernel we can check them all.
> 
> If the names are unique-per-chip, we can add a restriction like this
> with the requirement:
> 
> depends on !GPIO_SYSFS
> 
> so it can't even be compiled in if someone is using the sysfs.
>
> That should solve the situation where people are (ab)using
> the sysfs and getting name collisions as a result.

Would it possible to set a flag to suppress just the sysfs entry
renaming instead?

Despite its flaws the sysfs interface is still very convenient and I'd
prefer not to disable it just because of the line names.

> Then it should be fine for any driver to provide a names array
> provided all the names are unique on that gpiochip.

So it sounds like there's nothing preventing per-chip-unique names in
the rest of gpiolib and the new chardev interface then? Are the
user-space libraries able to cope with it, etc?

> I doubt it would break anything, but let's see what Geert says.
> He has some special usecases in the gpio-aggregator driver
> which will incidentally look for just linenames when
> aggregating gpios, but I feel it is a bit thick for it to work
> with multiple hot-pluggable GPIO chips as well, I don't think
> that is its usecase. (We all want to be perfect but...)

Ok.

> > But what about any other non-pluggable
> > IC, which provides a few named GPIO lines and of which there could be
> > more than one in a system?
> 
> I think if there are such, and the lines are unique per-chip
> we should make the drivers depend on !GPIO_SYSFS.

Or just suppress the sysfs-entry renaming if that's the only thing
that's blocking this.

Johan

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Wed, Dec 9, 2020 at 4:24 PM Johan Hovold <johan@kernel.org> wrote:
> On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:

> > depends on !GPIO_SYSFS
> >
> > so it can't even be compiled in if someone is using the sysfs.
> >
> > That should solve the situation where people are (ab)using
> > the sysfs and getting name collisions as a result.
>
> Would it possible to set a flag to suppress just the sysfs entry
> renaming instead?

Hm you mean that when a GPIO is "exported" in sysfs
it should not get a symbolic name from the names but instead
just the number?

I bet someone has written their scripts to take advantage of
the symbolic names so I suspect the task becomes bigger
like suppress the sysfs entry renaming if and only if there is
a namespace collision.

But I think we can do that, doesn't seem too hard?

I just hacked up this:
https://lore.kernel.org/linux-gpio/20201209161821.92931-1-linus.walleij@linaro.org/T/#u

> Despite its flaws the sysfs interface is still very convenient and I'd
> prefer not to disable it just because of the line names.

Would these conveniences be identical to those listed
in my recent TODO entry?
https://lore.kernel.org/linux-gpio/20201204083533.65830-1-linus.walleij@linaro.org/

There are several other issues with the sysfs, so making it conflict
with other drivers is almost  plus in the direction of discouragement
from the GPIO submaintainer point of view, but I do see that
people like it for the reasons in the TODO. :/

I am strongly encouraging any developer with a few spare cycles
on their hands to go and implement the debugfs facility because
we can make it so much better than the sysfs, easier and
more convenient for testing etc.

> > Then it should be fine for any driver to provide a names array
> > provided all the names are unique on that gpiochip.
>
> So it sounds like there's nothing preventing per-chip-unique names in
> the rest of gpiolib and the new chardev interface then? Are the
> user-space libraries able to cope with it, etc?

Yes the documentation refers to libgpiod a very well maintained
library:
https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html
https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/

Then there are the the example tools included with the kernel
that provide a second implementation for the same interfaces
using just the C standard library:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/gpio

I usually use the tools myself.

Yours,
Linus Walleij

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:
> On Wed, Dec 9, 2020 at 4:24 PM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:
> 
> > > depends on !GPIO_SYSFS
> > >
> > > so it can't even be compiled in if someone is using the sysfs.
> > >
> > > That should solve the situation where people are (ab)using
> > > the sysfs and getting name collisions as a result.
> >
> > Would it possible to set a flag to suppress just the sysfs entry
> > renaming instead?
> 
> Hm you mean that when a GPIO is "exported" in sysfs
> it should not get a symbolic name from the names but instead
> just the number?

Right.

> I bet someone has written their scripts to take advantage of
> the symbolic names so I suspect the task becomes bigger
> like suppress the sysfs entry renaming if and only if there is
> a namespace collision.
> 
> But I think we can do that, doesn't seem too hard?
> 
> I just hacked up this:
> https://lore.kernel.org/linux-gpio/20201209161821.92931-1-linus.walleij@linaro.org/T/#u

I just replied to that thread, but to summarize, you can't rely on
having the sysfs code detect collisions since that will trigger a bunch
of nasty warnings and backtraces. We also don't want the sysfs interface
for a specific USB device to depend on probe order (only the first one
plugged in gets to use the line names). And adding line names now could
in fact be what breaks currently working scripts.

> > Despite its flaws the sysfs interface is still very convenient and I'd
> > prefer not to disable it just because of the line names.
> 
> Would these conveniences be identical to those listed
> in my recent TODO entry?
> https://lore.kernel.org/linux-gpio/20201204083533.65830-1-linus.walleij@linaro.org/

Indeed.

> There are several other issues with the sysfs, so making it conflict
> with other drivers is almost  plus in the direction of discouragement
> from the GPIO submaintainer point of view, but I do see that
> people like it for the reasons in the TODO. :/
> 
> I am strongly encouraging any developer with a few spare cycles
> on their hands to go and implement the debugfs facility because
> we can make it so much better than the sysfs, easier and
> more convenient for testing etc.

Don't you run the risk of having people enable debugfs in production
systems now just so they can use the old-style interface?

Side note: if you skip the "export" part of the interface, how would you
indicate that a line is already in use or not available (e.g.
gpio-range-reserved)?

> > > Then it should be fine for any driver to provide a names array
> > > provided all the names are unique on that gpiochip.
> >
> > So it sounds like there's nothing preventing per-chip-unique names in
> > the rest of gpiolib and the new chardev interface then? Are the
> > user-space libraries able to cope with it, etc?
> 
> Yes the documentation refers to libgpiod a very well maintained
> library:
> https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html
> https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/

Just did a super quick check and it seems libgpiod still assumes a flat
name space. For example, gpiod_chip_find_line() returns only the first
line found that matches a name. Shouldn't be impossible to extend, but
just want to make sure this flat namespace assumption hasn't been to
heavily relied upon.

Johan

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Thu, Dec 10, 2020 at 09:53:45AM +0100, Johan Hovold wrote:
> On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:
> > On Wed, Dec 9, 2020 at 4:24 PM Johan Hovold <johan@kernel.org> wrote:

> > > So it sounds like there's nothing preventing per-chip-unique names in
> > > the rest of gpiolib and the new chardev interface then? Are the
> > > user-space libraries able to cope with it, etc?
> > 
> > Yes the documentation refers to libgpiod a very well maintained
> > library:
> > https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html
> > https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
> 
> Just did a super quick check and it seems libgpiod still assumes a flat
> name space. For example, gpiod_chip_find_line() returns only the first
> line found that matches a name. Shouldn't be impossible to extend, but
> just want to make sure this flat namespace assumption hasn't been to
> heavily relied upon.

That should have been gpiod_line_find() (which in turn uses the above
mentioned helper for per-chip lookups).

Johan

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Thu, Dec 10, 2020 at 9:53 AM Johan Hovold <johan@kernel.org> wrote:
> On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:

> I just replied to that thread, but to summarize, you can't rely on
> having the sysfs code detect collisions since that will trigger a bunch
> of nasty warnings and backtraces. We also don't want the sysfs interface
> for a specific USB device to depend on probe order (only the first one
> plugged in gets to use the line names). And adding line names now could
> in fact be what breaks currently working scripts.

Yes the sysfs ABI is very volatile and easy to break.

As pointed out in the other reply, sysfs base GPIO number is all
wibbly-wobbly on anything hot-pluggable so in a way I feel it
is the right thing to disallow sysfs altogether on hotpluggable
devices.

> > I am strongly encouraging any developer with a few spare cycles
> > on their hands to go and implement the debugfs facility because
> > we can make it so much better than the sysfs, easier and
> > more convenient for testing etc.
>
> Don't you run the risk of having people enable debugfs in production
> systems now just so they can use the old-style interface?

That risk always exist of course. For this and many other reasons.
I just have to trust developers to understand that debugfs is named
debugfs for a reason.

> Side note: if you skip the "export" part of the interface, how would you
> indicate that a line is already in use or not available (e.g.
> gpio-range-reserved)?

The idea is that if you poke around there you know what you're
doing or ready to face the consequences.

I am thinking if people want to toggle LEDs and switches from
debugfs for testing and hacking they'd be alright with corrupting
the SPI interface if they make mistakes.

The chardev ABI is the only thing which we really designed with
some users, multiple users, compatibility and security in mind,
yet we had to revamp it once from scratch...

> Just did a super quick check and it seems libgpiod still assumes a flat
> name space. For example, gpiod_chip_find_line() returns only the first
> line found that matches a name. Shouldn't be impossible to extend, but
> just want to make sure this flat namespace assumption hasn't been to
> heavily relied upon.

The unique way to identify a GPIO is gpiochip instance (with
topology from sysfs) and then a line number on that chip.
This is done e.g. in the example tool
tools/gpio/gpio-hammer.c

As you can see the tool doesn't use these line names.

The line names are really like symbolic links or something.
But they are indeed in a flat namespace so we should try to
at least make them unique if it turns out people love to use
these.

As it is now system designers mostly use device tree to assign
line names and they try to make these unique because they don't
like the nasty warnings from gpiolib.

If I google for the phrase "Detected name collision for GPIO name"
I just find the code, our discussions and some USB serial devices
warning about this so far.

Maybe we should just make a patch to disallow it?

Yours,
Linus Walleij

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Sat, Dec 12, 2020 at 01:03:32AM +0100, Linus Walleij wrote:
> On Thu, Dec 10, 2020 at 9:53 AM Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:
> 
> > I just replied to that thread, but to summarize, you can't rely on
> > having the sysfs code detect collisions since that will trigger a bunch
> > of nasty warnings and backtraces. We also don't want the sysfs interface
> > for a specific USB device to depend on probe order (only the first one
> > plugged in gets to use the line names). And adding line names now could
> > in fact be what breaks currently working scripts.
> 
> Yes the sysfs ABI is very volatile and easy to break.
> 
> As pointed out in the other reply, sysfs base GPIO number is all
> wibbly-wobbly on anything hot-pluggable so in a way I feel it
> is the right thing to disallow sysfs altogether on hotpluggable
> devices.

No, the gpio numbers will of course vary, but since gpiolib exports the
base number for the chip, a scripts can easily determine the right gpio
number as base + offset.

Having probe order break that by sometimes exporting the line using it's
name is what would be a problem.

> > Just did a super quick check and it seems libgpiod still assumes a flat
> > name space. For example, gpiod_chip_find_line() returns only the first
> > line found that matches a name. Shouldn't be impossible to extend, but
> > just want to make sure this flat namespace assumption hasn't been to
> > heavily relied upon.
> 
> The unique way to identify a GPIO is gpiochip instance (with
> topology from sysfs) and then a line number on that chip.
> This is done e.g. in the example tool
> tools/gpio/gpio-hammer.c
> 
> As you can see the tool doesn't use these line names.
>
> The line names are really like symbolic links or something.
> But they are indeed in a flat namespace so we should try to
> at least make them unique if it turns out people love to use
> these.

Not necessarily, they could be unique per chip as we're discussing here
with respect to hotpluggable controllers. We just can't have it both
ways.

> As it is now system designers mostly use device tree to assign
> line names and they try to make these unique because they don't
> like the nasty warnings from gpiolib.
>
> If I google for the phrase "Detected name collision for GPIO name"
> I just find the code, our discussions and some USB serial devices
> warning about this so far.
> 
> Maybe we should just make a patch to disallow it?

That would make it impossible to provide name lines on hotpluggable
controllers, which would be nice to support.

Johan

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Linus Walleij]

On Mon, Dec 14, 2020 at 9:58 AM Johan Hovold <johan@kernel.org> wrote:
> On Sat, Dec 12, 2020 at 01:03:32AM +0100, Linus Walleij wrote:

> > If I google for the phrase "Detected name collision for GPIO name"
> > I just find the code, our discussions and some USB serial devices
> > warning about this so far.
> >
> > Maybe we should just make a patch to disallow it?
>
> That would make it impossible to provide name lines on hotpluggable
> controllers, which would be nice to support.

I merged a patch for this now, let's tighten this loose end up.

Also: thanks for poking me about this, I should have looked into
this ages ago :/ focus you know...

Yours,
Linus Walleij

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Mon, Dec 14, 2020 at 10:19:07AM +0100, Linus Walleij wrote:
> On Mon, Dec 14, 2020 at 9:58 AM Johan Hovold <johan@kernel.org> wrote:
> > On Sat, Dec 12, 2020 at 01:03:32AM +0100, Linus Walleij wrote:
> 
> > > If I google for the phrase "Detected name collision for GPIO name"
> > > I just find the code, our discussions and some USB serial devices
> > > warning about this so far.
> > >
> > > Maybe we should just make a patch to disallow it?
> >
> > That would make it impossible to provide name lines on hotpluggable
> > controllers, which would be nice to support.
> 
> I merged a patch for this now, let's tighten this loose end up.
> 
> Also: thanks for poking me about this, I should have looked into
> this ages ago :/ focus you know...

Yeah, tell me about it. :/

Johan

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Manivannan Sadhasivam]

Hi Linus,

On Tue, Dec 01, 2020 at 03:37:38PM +0100, Linus Walleij wrote:
> On Sun, Nov 22, 2020 at 6:08 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
> 
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.
> >
> > Inspired from cp210x usb to serial converter driver.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: linux-gpio@vger.kernel.org
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> 
> This looks good to me overall, provided that it plays well with the
> serial port.
> 
> One minor notice:
> 
> > +enum gpio_pins {
> > +       GPIO_RI = 0,
> > +       GPIO_CD,
> > +       GPIO_DSR,
> > +       GPIO_DTR,
> > +       GPIO_CTS,
> > +       GPIO_RTS,
> > +       GPIO_MAX,
> > +};
> 
> You know the names of the pins...
> 
> > +       port_priv->gc.ngpio = 6;
> > +       port_priv->gc.label = "xr_gpios";
> > +       port_priv->gc.request = xr_gpio_request;
> > +       port_priv->gc.free = xr_gpio_free;
> > +       port_priv->gc.get_direction = xr_gpio_direction_get;
> > +       port_priv->gc.direction_input = xr_gpio_direction_input;
> > +       port_priv->gc.direction_output = xr_gpio_direction_output;
> > +       port_priv->gc.get = xr_gpio_get;
> > +       port_priv->gc.set = xr_gpio_set;
> > +       port_priv->gc.owner = THIS_MODULE;
> > +       port_priv->gc.parent = &port->dev;
> > +       port_priv->gc.base = -1;
> > +       port_priv->gc.can_sleep = true;
> 
> So assign port_priv->gc.names here as well with an array
> of strings with the names ("RI", "CD", ... etc).
> This makes it look really nice in userspace if you do
> e.g. "lsgpio".
> 

As Johan stated, this doesn't work with multiple devices attached to the system.
That's the reason for not adding the line names.

This gives me the motivation to get my hands dirty with gpiolib (but I fear of
breaking the ABI)...

> With that:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 

Thanks for the review!

Regards,
Mani

> Yours,
> Linus Walleij

Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Sun, Nov 22, 2020 at 10:38:21PM +0530, Manivannan Sadhasivam wrote:
> Add gpiochip support for Maxlinear/Exar USB to serial converter
> for controlling the available gpios.
> 
> Inspired from cp210x usb to serial converter driver.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/usb/serial/xr_serial.c | 267 ++++++++++++++++++++++++++++++++-
>  1 file changed, 261 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index e166916554d6..ca63527a5310 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -9,6 +9,7 @@
>   * Copyright (c) 2020 Manivannan Sadhasivam <mani@kernel.org>
>   */
>  
> +#include <linux/gpio/driver.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -16,6 +17,28 @@
>  #include <linux/usb.h>
>  #include <linux/usb/serial.h>
>  
> +#ifdef CONFIG_GPIOLIB
> +enum gpio_pins {
> +	GPIO_RI = 0,
> +	GPIO_CD,
> +	GPIO_DSR,
> +	GPIO_DTR,
> +	GPIO_CTS,
> +	GPIO_RTS,
> +	GPIO_MAX,
> +};
> +#endif
> +
> +struct xr_port_private {
> +#ifdef CONFIG_GPIOLIB
> +	struct gpio_chip gc;
> +	bool gpio_registered;
> +	enum gpio_pins pin_status[GPIO_MAX];

This isn't an array of enum gpio_pins, rather you use the enum as an
index into the array which only stores a boolean value per pin.

Please rename the array and fix the type (e.g. bool) so that it is clear
how from the name how it is being used, for example, "gpio_requested" or
"gpio_in_use".

> +#endif
> +	struct mutex gpio_lock;	/* protects GPIO state */
> +	bool port_open;
> +};
> +
>  struct xr_txrx_clk_mask {
>  	u16 tx;
>  	u16 rx0;
> @@ -106,6 +129,8 @@ struct xr_txrx_clk_mask {
>  #define XR21V141X_REG_GPIO_CLR		0x1e
>  #define XR21V141X_REG_GPIO_STATUS	0x1f
>  
> +static int xr_cts_rts_check(struct xr_port_private *port_priv);
> +
>  static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg,
>  		      u8 val)
>  {
> @@ -309,6 +334,7 @@ static int xr_uart_disable(struct usb_serial_port *port)
>  static void xr_set_flow_mode(struct tty_struct *tty,
>  			     struct usb_serial_port *port)
>  {
> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
>  	unsigned int cflag = tty->termios.c_cflag;
>  	int ret;
>  	u8 flow, gpio_mode;
> @@ -318,6 +344,17 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>  		return;
>  
>  	if (cflag & CRTSCTS) {
> +		/*
> +		 * Check if the CTS/RTS pins are occupied by GPIOLIB. If yes,

GPIOLIB doesn't "occupy" anything. Use something like "claimed by"
instead.

> +		 * then use no flow control.
> +		 */
> +		if (xr_cts_rts_check(port_priv)) {
> +			dev_dbg(&port->dev,
> +				"CTS/RTS pins are occupied, so disabling flow control\n");

Ditto.

And there's no need to ignore a request for sw flow control if the pins
are in use. Just move the check above the conditional and make the
first branch depend on it.

You also need to clear CRTSCTS in termios if it cannot be set.

> +			flow = XR21V141X_UART_FLOW_MODE_NONE;
> +			goto exit;
> +		}
> +
>  		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
>  
>  		/*
> @@ -341,6 +378,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>  		flow = XR21V141X_UART_FLOW_MODE_NONE;
>  	}
>  
> +exit:
>  	/*
>  	 * As per the datasheet, UART needs to be disabled while writing to
>  	 * FLOW_CONTROL register.
> @@ -355,18 +393,22 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>  static int xr_tiocmset_port(struct usb_serial_port *port,
>  			    unsigned int set, unsigned int clear)
>  {
> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
>  	u8 gpio_set = 0;
>  	u8 gpio_clr = 0;
>  	int ret = 0;
>  
> -	/* Modem control pins are active low, so set & clr are swapped */
> -	if (set & TIOCM_RTS)
> +	/*
> +	 * Modem control pins are active low, so set & clr are swapped. And
> +	 * ignore the pins that are occupied by GPIOLIB.
> +	 */
> +	if ((set & TIOCM_RTS) && !(port_priv->pin_status[GPIO_RTS]))
>  		gpio_clr |= XR21V141X_UART_MODE_RTS;
> -	if (set & TIOCM_DTR)
> +	if ((set & TIOCM_DTR) && !(port_priv->pin_status[GPIO_DTR]))
>  		gpio_clr |= XR21V141X_UART_MODE_DTR;
> -	if (clear & TIOCM_RTS)
> +	if ((clear & TIOCM_RTS) && !(port_priv->pin_status[GPIO_RTS]))
>  		gpio_set |= XR21V141X_UART_MODE_RTS;
> -	if (clear & TIOCM_DTR)
> +	if ((clear & TIOCM_DTR) && !(port_priv->pin_status[GPIO_DTR]))
>  		gpio_set |= XR21V141X_UART_MODE_DTR;
>  
>  	/* Writing '0' to gpio_{set/clr} bits has no effect, so no need to do */
> @@ -464,6 +506,7 @@ static void xr_set_termios(struct tty_struct *tty,
>  
>  static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
>  {
> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
>  	int ret;
>  
>  	ret = xr_uart_enable(port);
> @@ -482,13 +525,23 @@ static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
>  		return ret;
>  	}
>  
> +	mutex_lock(&port_priv->gpio_lock);
> +	port_priv->port_open = true;
> +	mutex_unlock(&port_priv->gpio_lock);

This needs to be done before calling set_termios() above.

> +
>  	return 0;
>  }
>  
>  static void xr_close(struct usb_serial_port *port)
>  {
> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> +
>  	usb_serial_generic_close(port);
>  
> +	mutex_lock(&port_priv->gpio_lock);
> +	port_priv->port_open = false;
> +	mutex_unlock(&port_priv->gpio_lock);
> +
>  	xr_uart_disable(port);
>  }
>  
> @@ -553,13 +606,215 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
>  	xr_set_reg_uart(port, XR21V141X_REG_TX_BREAK, state);
>  }
>  
> -static int xr_port_probe(struct usb_serial_port *port)
> +#ifdef CONFIG_GPIOLIB
> +
> +static int xr_cts_rts_check(struct xr_port_private *port_priv)
>  {
> +	if (port_priv->pin_status[GPIO_RTS] || port_priv->pin_status[GPIO_CTS])
> +		return -EBUSY;
> +
>  	return 0;
>  }
>  
> +static int xr_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct usb_serial_port *port = gpiochip_get_data(gc);
> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> +	int ret = 0;
> +
> +	mutex_lock(&port_priv->gpio_lock);
> +	/*
> +	 * Do not proceed if the port is open. This is done to avoid changing
> +	 * the GPIO configuration used by the serial driver.
> +	 */
> +	if (port_priv->port_open) {
> +		ret = -EBUSY;
> +		goto err_out;
> +	}
> +
> +	/* Mark the GPIO pin as busy */
> +	port_priv->pin_status[offset] = true;

What about GPIO5/RTS#? It may still be configured for flow control even
if the port is now closed. You need to switch to GPIO mode.

> +
> +err_out:
> +	mutex_unlock(&port_priv->gpio_lock);
> +
> +	return ret;
> +}
> +
> +static void xr_gpio_free(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct usb_serial_port *port = gpiochip_get_data(gc);
> +	struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> +
> +	mutex_lock(&port_priv->gpio_lock);
> +	/*
> +	 * Do not proceed if the port is open. This is done to avoid toggling
> +	 * of pins suddenly when the serial port is in use.
> +	 */
> +	if (port_priv->port_open)
> +		goto err_out;
> +
> +	/* Mark the GPIO pin as free */
> +	port_priv->pin_status[offset] = false;

You cannot fail this even if the port is open since otherwise the pin
will remain claimed.

You may need to cache the valid pins at serial port open instead as I
already mentioned.

Also, you need to figure out how to coordinate the pin-configuration
with the serial driver. I suggested added a call to configure DTR/RTS at
outputs on open(), but perhaps this should only be done once in case
they are not really used for modem control and instead needs to be
reconfigured as inputs by the gpio driver (i.e. before opening the
port). 

I'm still not sure about how best to handle this remuxing at runtime in
order to avoid having incidentally setting up an incorrect pin
configuration.

Ideally, the muxing should be determined in EEPROM or be based on
VID/PID and not be allowed to change at runtime at all...

I'm not convinced that the current approach is a good idea yet.

> +
> +err_out:
> +	mutex_unlock(&port_priv->gpio_lock);
> +}

Johan

[PATCH v5 3/3] usb: cdc-acm: Ignore Exar XR21V141X when serial driver is built

[Manivannan Sadhasivam]

From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

The RTS/CTS line discipline for this device doesn't follow
the standard. So, in order to properly support TX, a separate
driver is needed.

Ensure that cdc_acm will ignore it during probe time, if the
Kernel is built with support for it.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 30ef946a8e1a..719829e6b6db 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1890,6 +1890,12 @@ static const struct usb_device_id acm_ids[] = {
 	},
 #endif
 
+#if IS_ENABLED(CONFIG_USB_SERIAL_XR)
+	{ USB_DEVICE(0x04e2, 0x1410),   /* Ignore XR21V141X USB to Serial converter */
+	.driver_info = IGNORE_DEVICE,
+	},
+#endif
+
 	/*Samsung phone in firmware update mode */
 	{ USB_DEVICE(0x04e8, 0x685d),
 	.driver_info = IGNORE_DEVICE,
-- 
2.25.1

Re: [PATCH v5 3/3] usb: cdc-acm: Ignore Exar XR21V141X when serial driver is built

[Johan Hovold]

On Sun, Nov 22, 2020 at 10:38:22PM +0530, Manivannan Sadhasivam wrote:
> From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> The RTS/CTS line discipline for this device doesn't follow
> the standard. So, in order to properly support TX, a separate
> driver is needed.

This took a bit of effort to understand, but I think I know what you're
referring to now. In ACM mode the device has RTS/CTS flow control
enabled, which means TX will be disabled in case CTS isn't wired up
correctly.

It as nothing to do with line disciplines though.

> Ensure that cdc_acm will ignore it during probe time, if the
> Kernel is built with support for it.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

> ---
>  drivers/usb/class/cdc-acm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 30ef946a8e1a..719829e6b6db 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -1890,6 +1890,12 @@ static const struct usb_device_id acm_ids[] = {
>  	},
>  #endif
>  
> +#if IS_ENABLED(CONFIG_USB_SERIAL_XR)
> +	{ USB_DEVICE(0x04e2, 0x1410),   /* Ignore XR21V141X USB to Serial converter */
> +	.driver_info = IGNORE_DEVICE,
> +	},
> +#endif
> +
>  	/*Samsung phone in firmware update mode */
>  	{ USB_DEVICE(0x04e8, 0x685d),
>  	.driver_info = IGNORE_DEVICE,

I've rewritten the commit message and applied this one now.

Johan

Re: [PATCH v5 0/3] Add support for MaxLinear/Exar USB to serial converters

[Manivannan Sadhasivam]

On Sun, Nov 22, 2020 at 10:38:19PM +0530, Manivannan Sadhasivam wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Hello,
> 
> This series adds support for MaxLinear/Exar USB to serial converters.
> This driver only supports XR21V141X series but it can easily be extended
> to other series in future.
> 
> This driver is inspired from the initial one submitted by Patong Yang:
> https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> 
> While the initial driver was a custom tty USB driver exposing whole
> new serial interface ttyXRUSBn, this version is completely based on USB
> serial core thus exposing the interfaces as ttyUSBn. This will avoid
> the overhead of exposing a new USB serial interface which the userspace
> tools are unaware of.
> 
> This series has been tested with Hikey970 board hosting XR21V141X chip.
> 
> NOTE: I've removed all reviews and tested-by tags as the code has gone
> through substantial rework. Greg, Linus, Mauro please consider reviewing
> again.
> 

Any chance to get this series into v5.11?

Thanks,
Mani

> Thanks,
> Mani
> 
> Changes in v5:
> 
> * Incorporated review comments from Johan. Noticeable ones are:
>   - Made serial and gpiolib support exclusive and used mutex to avoid
>     race. The gpio requests from gpiolib will be rejected when serial
>     port is in use.
>   - The driver only binds to data interface but claims both control and
>     data interface.
>   - Handled B0 request
>   - Removed all reviews as the code has gone through substantial rework.
> 
> Changes in v4:
> 
> * Multiple improvements based on Johan's review. Noticeable ones are:
>   - Now the driver claims both control and data interfaces but only registers
>     tty device for data interface.
>   - GPIO pin status is now shared between the console and gpiolib
>     implementations. This is done to avoid changing the lines spuriously.
>   - A separate port_open flag is added to reject GPIO requests while the tty
>     port is open.
>   - Removed padding PID to gpio device.
> * Added Greg and Mauro's review and tested tags.
> * Included a patch from Mauro to avoid the CDC-ACM driver to claim this device
>   when this driver is built.
> 
> Changes in v3:
> 
> * Dropped the check for PID and also the reg_width property.
> 
> Changes in v2:
> 
> * Dropped the code related to handling variable register size. It's all u8 now.
> * Dropped the header file and moved the contents to driver itself.
> * Added Linus's reviewed-by tag for gpiochip patch.
> * Added PID to gpiochip label
> * Dropped gpiochip for interface 0
> 
> Manivannan Sadhasivam (2):
>   usb: serial: Add MaxLinear/Exar USB to Serial driver
>   usb: serial: xr_serial: Add gpiochip support
> 
> Mauro Carvalho Chehab (1):
>   usb: cdc-acm: Ignore Exar XR21V141X when serial driver is built
> 
>  drivers/usb/class/cdc-acm.c    |   6 +
>  drivers/usb/serial/Kconfig     |   9 +
>  drivers/usb/serial/Makefile    |   1 +
>  drivers/usb/serial/xr_serial.c | 854 +++++++++++++++++++++++++++++++++
>  4 files changed, 870 insertions(+)
>  create mode 100644 drivers/usb/serial/xr_serial.c
> 
> -- 
> 2.25.1
> 

Re: [PATCH v5 0/3] Add support for MaxLinear/Exar USB to serial converters

[Johan Hovold]

On Tue, Dec 08, 2020 at 04:21:28PM +0530, Manivannan Sadhasivam wrote:
> On Sun, Nov 22, 2020 at 10:38:19PM +0530, Manivannan Sadhasivam wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Hello,
> > 
> > This series adds support for MaxLinear/Exar USB to serial converters.
> > This driver only supports XR21V141X series but it can easily be extended
> > to other series in future.
> > 
> > This driver is inspired from the initial one submitted by Patong Yang:
> > https://lore.kernel.org/linux-usb/20180404070634.nhspvmxcjwfgjkcv@advantechmxl-desktop
> > 
> > While the initial driver was a custom tty USB driver exposing whole
> > new serial interface ttyXRUSBn, this version is completely based on USB
> > serial core thus exposing the interfaces as ttyUSBn. This will avoid
> > the overhead of exposing a new USB serial interface which the userspace
> > tools are unaware of.
> > 
> > This series has been tested with Hikey970 board hosting XR21V141X chip.
> > 
> > NOTE: I've removed all reviews and tested-by tags as the code has gone
> > through substantial rework. Greg, Linus, Mauro please consider reviewing
> > again.
> > 
> 
> Any chance to get this series into v5.11?

No, sorry, reviewing this one again will be at the top of my list after
the merge window opens. Hopefully we'll have reached some conclusions
regarding the line names by then too.

Johan