| Message ID | 20201122170822.21715-2-mani@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for MaxLinear/Exar USB to serial converters | expand |
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
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
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
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
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
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
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
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
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
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);
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");
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