From: Manivannan Sadhasivam <[email protected]>
Hello,
This series adds support for MaxLinear/Exar USB to serial converters.
This driver only supports XR21V141X series but provision has been made
to support other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
https://patchwork.kernel.org/patch/10543261/
While the initial driver was a custom tty USB driver exposing whole
new serial interface ttyXRUSBn, this version is completely based on USB
serial core thus exposing the interfaces as ttyUSBn. This will avoid
the overhead of exposing a new USB serial interface which the userspace
tools are unaware of.
This series has been tested on Hikey970 board hosting XR21V141X chip.
Thanks,
Mani
Manivannan Sadhasivam (2):
usb: serial: Add MaxLinear/Exar USB to Serial driver
usb: serial: xr_serial: Add gpiochip support
drivers/usb/serial/Kconfig | 9 +
drivers/usb/serial/Makefile | 1 +
drivers/usb/serial/xr_serial.c | 728 +++++++++++++++++++++++++++++++++
drivers/usb/serial/xr_serial.h | 110 +++++
4 files changed, 848 insertions(+)
create mode 100644 drivers/usb/serial/xr_serial.c
create mode 100644 drivers/usb/serial/xr_serial.h
--
2.17.1
From: Manivannan Sadhasivam <[email protected]>
Add support for MaxLinear/Exar USB to Serial converters. This driver
only supports XR21V141X series but provision has been made to support
other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
https://patchwork.kernel.org/patch/10543261/
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 <[email protected]>
---
drivers/usb/serial/Kconfig | 9 +
drivers/usb/serial/Makefile | 1 +
drivers/usb/serial/xr_serial.c | 544 +++++++++++++++++++++++++++++++++
drivers/usb/serial/xr_serial.h | 103 +++++++
4 files changed, 657 insertions(+)
create mode 100644 drivers/usb/serial/xr_serial.c
create mode 100644 drivers/usb/serial/xr_serial.h
diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 25d7e0c36d38..8f6ad9f94735 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..ea4a0b167d3f
--- /dev/null
+++ b/drivers/usb/serial/xr_serial.c
@@ -0,0 +1,544 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MaxLinear/Exar USB to Serial driver
+ *
+ * Based on initial driver written by Patong Yang <[email protected]>
+ *
+ * Copyright (c) 2020 Manivannan Sadhasivam <[email protected]>
+ */
+
+#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>
+
+#include "xr_serial.h"
+
+static const struct xr_uart_regs xr21v141x_regs = {
+ .enable = 0x03,
+ .format = 0x0b,
+ .flow_ctrl = 0x0c,
+ .xon_char = 0x10,
+ .xoff_char = 0x11,
+ .loopback = 0x12,
+ .tx_break = 0x14,
+ .rs485_delay = 0x15,
+ .gpio_mode = 0x1a,
+ .gpio_dir = 0x1b,
+ .gpio_int_mask = 0x1c,
+ .gpio_set = 0x1d,
+ .gpio_clr = 0x1e,
+ .gpio_status = 0x1f,
+};
+
+static int xr_set_reg(struct usb_serial_port *port, u8 block, u16 reg,
+ u16 val)
+{
+ struct usb_serial *serial = port->serial;
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret = -EINVAL;
+
+ if (port_priv->idProduct == XR21V141X_ID) {
+ /* XR21V141X uses custom command for writing UART registers */
+ ret = usb_control_msg(serial->dev,
+ usb_sndctrlpipe(serial->dev, 0),
+ XR_SET_XR21V141X,
+ 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%x status: %d\n",
+ reg, ret);
+
+ return ret;
+}
+
+static int xr_get_reg(struct usb_serial_port *port, u8 block, u16 reg,
+ u16 *val)
+{
+ struct usb_serial *serial = port->serial;
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ void *dmabuf;
+ int ret = -EINVAL;
+
+ dmabuf = kmalloc(sizeof(reg), GFP_KERNEL);
+ if (!dmabuf)
+ return -ENOMEM;
+
+ if (port_priv->idProduct == XR21V141X_ID) {
+ /* XR21V141X uses custom command for reading UART registers */
+ ret = usb_control_msg(serial->dev,
+ usb_rcvctrlpipe(serial->dev, 0),
+ XR_GET_XR21V141X,
+ USB_DIR_IN | USB_TYPE_VENDOR, 0,
+ reg | (block << 8), dmabuf,
+ port_priv->reg_width,
+ USB_CTRL_SET_TIMEOUT);
+ }
+
+ if (ret == port_priv->reg_width) {
+ memcpy(val, dmabuf, port_priv->reg_width);
+ ret = 0;
+ } else {
+ dev_err(&port->dev, "Failed to get reg 0x%x status: %d\n",
+ reg, ret);
+ if (ret >= 0)
+ ret = -EIO;
+ }
+
+ kfree(dmabuf);
+
+ return ret;
+}
+
+/* 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 = XR_INT_OSC_HZ / tty->termios.c_ospeed;
+ u32 idx = (32 * divisor) & 0x1f;
+ u16 tx_mask = xr21v141x_txrx_clk_masks[idx].tx;
+ u16 rx_mask = (divisor & 1) ? xr21v141x_txrx_clk_masks[idx].rx1 :
+ xr21v141x_txrx_clk_masks[idx].rx0;
+ int ret;
+
+ dev_dbg(&port->dev, "Setting baud rate :%u\n", tty->termios.c_ospeed);
+
+ /*
+ * 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(port, XR21V141X_UART_REG_BLOCK,
+ XR21V141X_CLOCK_DIVISOR_0, (divisor & 0xff));
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ XR21V141X_CLOCK_DIVISOR_1, ((divisor >> 8) & 0xff));
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ XR21V141X_CLOCK_DIVISOR_2, ((divisor >> 16) & 0xff));
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ XR21V141X_TX_CLOCK_MASK_0, (tx_mask & 0xff));
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ XR21V141X_TX_CLOCK_MASK_1, ((tx_mask >> 8) & 0xff));
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ XR21V141X_RX_CLOCK_MASK_0, (rx_mask & 0xff));
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ XR21V141X_RX_CLOCK_MASK_1, ((rx_mask >> 8) & 0xff));
+
+ 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)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret;
+
+ ret = xr_set_reg(port, XR21V141X_URM_REG_BLOCK,
+ XR21V141X_URM_FIFO_ENABLE_REG,
+ XR21V141X_URM_ENABLE_TX_FIFO);
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->enable,
+ UART_ENABLE_TX | UART_ENABLE_RX);
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_URM_REG_BLOCK,
+ XR21V141X_URM_FIFO_ENABLE_REG,
+ XR21V141X_URM_ENABLE_TX_FIFO |
+ XR21V141X_URM_ENABLE_RX_FIFO);
+
+ return ret;
+}
+
+static int xr_uart_disable(struct usb_serial_port *port)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret;
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->enable, 0);
+ if (ret)
+ return ret;
+
+ ret = xr_set_reg(port, XR21V141X_URM_REG_BLOCK,
+ XR21V141X_URM_FIFO_ENABLE_REG, 0);
+
+ return ret;
+}
+
+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;
+ }
+
+ ret = xr_set_baudrate(tty, port);
+ if (ret) {
+ dev_err(&port->dev, "Failed to set baudrate\n");
+ return ret;
+ }
+
+ return usb_serial_generic_open(tty, port);
+}
+
+static void xr_close(struct usb_serial_port *port)
+{
+ usb_serial_generic_close(port);
+
+ xr_uart_disable(port);
+}
+
+static void xr_set_flow_mode(struct tty_struct *tty,
+ struct usb_serial_port *port,
+ unsigned int cflag)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ u16 flow, gpio_mode;
+ int ret;
+
+ ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_mode, &gpio_mode);
+ if (ret)
+ return;
+
+ if (cflag & CRTSCTS) {
+ dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
+ flow = UART_FLOW_MODE_HW;
+ gpio_mode |= UART_MODE_RTS_CTS;
+ } else if (I_IXOFF(tty) || 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 = UART_FLOW_MODE_SW;
+ gpio_mode |= UART_MODE_GPIO;
+
+ xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->xon_char, start_char);
+ xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->xoff_char, stop_char);
+ } else {
+ dev_dbg(&port->dev, "No flow ctrl\n");
+ flow = UART_FLOW_MODE_NONE;
+ }
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->flow_ctrl, flow);
+ if (ret)
+ return;
+
+ xr_set_reg(port, XR21V141X_UART_REG_BLOCK, port_priv->regs->gpio_mode,
+ gpio_mode);
+}
+
+static void xr_set_termios(struct tty_struct *tty,
+ struct usb_serial_port *port,
+ struct ktermios *old_termios)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ struct device *dev = &port->dev;
+ unsigned int cflag, old_cflag;
+ int ret;
+ u16 bits;
+
+ cflag = tty->termios.c_cflag;
+ old_cflag = old_termios->c_cflag;
+
+ if (tty->termios.c_ospeed != old_termios->c_ospeed)
+ xr_set_baudrate(tty, port);
+
+ ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->format, &bits);
+ if (ret)
+ return;
+
+ /* If the number of data bits is to be updated */
+ if ((cflag & CSIZE) != (old_cflag & CSIZE)) {
+ bits &= ~UART_DATA_MASK;
+ switch (cflag & CSIZE) {
+ case CS7:
+ bits |= UART_DATA_7;
+ dev_dbg(dev, "Setting data bits: 7\n");
+ break;
+ case CS8:
+ /* CS9 not supported */
+ default:
+ bits |= UART_DATA_8;
+ dev_dbg(dev, "Setting data bits: 8\n");
+ break;
+ }
+ }
+
+ if ((cflag & (PARENB | PARODD | CMSPAR)) !=
+ (old_cflag & (PARENB | PARODD | CMSPAR))) {
+ bits &= ~UART_PARITY_MASK;
+ if (cflag & PARENB) {
+ if (cflag & CMSPAR) {
+ if (cflag & PARODD) {
+ bits |= (UART_PARITY_MARK <<
+ UART_PARITY_SHIFT);
+ dev_dbg(dev, "Setting parity: MARK\n");
+ } else {
+ bits |= (UART_PARITY_SPACE <<
+ UART_PARITY_SHIFT);
+ dev_dbg(dev, "Setting parity: SPACE\n");
+ }
+ } else {
+ if (cflag & PARODD) {
+ bits |= (UART_PARITY_ODD <<
+ UART_PARITY_SHIFT);
+ dev_dbg(dev, "Setting parity: ODD\n");
+ } else {
+ bits |= (UART_PARITY_EVEN <<
+ UART_PARITY_SHIFT);
+ dev_dbg(dev, "Setting parity: EVEN\n");
+ }
+ }
+ }
+ }
+
+ if ((cflag & CSTOPB) != (old_cflag & CSTOPB)) {
+ bits &= ~UART_STOP_MASK;
+ if (cflag & CSTOPB) {
+ bits |= (UART_STOP_2 << UART_STOP_SHIFT);
+ dev_dbg(dev, "Setting stop bits: 2\n");
+ } else {
+ bits |= (UART_STOP_1 << UART_STOP_SHIFT);
+ dev_dbg(dev, "Setting stop bits: 1\n");
+ }
+ }
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->format, (bits & 0xff));
+ if (ret)
+ return;
+
+ xr_set_flow_mode(tty, port, cflag);
+}
+
+static int xr_tiocmset_port(struct usb_serial_port *port,
+ unsigned int set, unsigned int clear)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret = 0;
+ u8 gpio_set = 0;
+ u8 gpio_clr = 0;
+
+ /* Modem control pins are active low, so set & clr are swapped */
+ if (set & TIOCM_RTS)
+ gpio_clr |= UART_MODE_RTS;
+ if (set & TIOCM_DTR)
+ gpio_clr |= UART_MODE_DTR;
+ if (clear & TIOCM_RTS)
+ gpio_set |= UART_MODE_RTS;
+ if (clear & TIOCM_DTR)
+ gpio_set |= 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(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_clr, gpio_clr);
+
+ if (gpio_set)
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->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 int xr_tiocmget(struct tty_struct *tty)
+{
+ struct usb_serial_port *port = tty->driver_data;
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret;
+ u16 status;
+
+ ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->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 & UART_MODE_DTR) ? 0 : TIOCM_DTR) |
+ ((status & UART_MODE_RTS) ? 0 : TIOCM_RTS) |
+ ((status & UART_MODE_CTS) ? 0 : TIOCM_CTS) |
+ ((status & UART_MODE_DSR) ? 0 : TIOCM_DSR) |
+ ((status & UART_MODE_RI) ? 0 : TIOCM_RI) |
+ ((status & 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;
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ u16 state;
+
+ if (break_state == 0)
+ state = UART_BREAK_OFF;
+ else
+ state = UART_BREAK_ON;
+
+ dev_dbg(&port->dev, "Turning break %s\n",
+ state == UART_BREAK_OFF ? "off" : "on");
+ xr_set_reg(port, XR21V141X_UART_REG_BLOCK, port_priv->regs->tx_break,
+ state);
+}
+
+static int xr_port_probe(struct usb_serial_port *port)
+{
+ struct usb_serial *serial = port->serial;
+ struct xr_port_private *port_priv;
+
+ port_priv = kzalloc(sizeof(*port_priv), GFP_KERNEL);
+ if (!port_priv)
+ return -ENOMEM;
+
+ port_priv->idProduct = le16_to_cpu(serial->dev->descriptor.idProduct);
+
+ /* XR21V141X specific settings */
+ if (port_priv->idProduct == XR21V141X_ID) {
+ port_priv->regs = &xr21v141x_regs;
+ port_priv->reg_width = 1;
+ }
+
+ usb_set_serial_port_data(port, port_priv);
+
+ return 0;
+}
+
+static int xr_port_remove(struct usb_serial_port *port)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+ kfree(port_priv);
+
+ 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,
+ .bulk_in_size = 384,
+ .bulk_out_size = 128,
+ .open = xr_open,
+ .close = xr_close,
+ .break_ctl = xr_break_ctl,
+ .set_termios = xr_set_termios,
+ .tiocmget = xr_tiocmget,
+ .tiocmset = xr_tiocmset,
+ .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 <[email protected]>");
+MODULE_DESCRIPTION("MaxLinear/Exar USB to Serial driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/usb/serial/xr_serial.h b/drivers/usb/serial/xr_serial.h
new file mode 100644
index 000000000000..d2977ef847a0
--- /dev/null
+++ b/drivers/usb/serial/xr_serial.h
@@ -0,0 +1,103 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef __LINUX_USB_SERIAL_XR_SERIAL_H
+#define __LINUX_USB_SERIAL_XR_SERIAL_H
+
+struct xr_uart_regs {
+ u8 enable;
+ u8 format;
+ u8 flow_ctrl;
+ u8 xon_char;
+ u8 xoff_char;
+ u8 loopback;
+ u8 tx_break;
+ u8 rs485_delay;
+ u8 gpio_mode;
+ u8 gpio_dir;
+ u8 gpio_int_mask;
+ u8 gpio_set;
+ u8 gpio_clr;
+ u8 gpio_status;
+};
+
+struct xr_port_private {
+ const struct xr_uart_regs *regs;
+ u16 idProduct;
+ u8 reg_width;
+};
+
+struct xr_txrx_clk_mask {
+ u16 tx;
+ u16 rx0;
+ u16 rx1;
+};
+
+#define XR21V141X_ID 0x1410
+#define XR_INT_OSC_HZ 48000000
+
+/* USB Requests */
+#define XR_SET_XR21V141X 0
+#define XR_GET_XR21V141X 1
+
+#define XR21V141X_CLOCK_DIVISOR_0 0x4
+#define XR21V141X_CLOCK_DIVISOR_1 0x5
+#define XR21V141X_CLOCK_DIVISOR_2 0x6
+#define XR21V141X_TX_CLOCK_MASK_0 0x7
+#define XR21V141X_TX_CLOCK_MASK_1 0x8
+#define XR21V141X_RX_CLOCK_MASK_0 0x9
+#define XR21V141X_RX_CLOCK_MASK_1 0xa
+
+/* XR21V141X register blocks */
+#define XR21V141X_UART_REG_BLOCK 0
+#define XR21V141X_URM_REG_BLOCK 4
+#define XR21V141X_UART_CUSTOM_BLOCK 0x66
+
+/* XR21V141X UART Manager Registers */
+#define XR21V141X_URM_FIFO_ENABLE_REG 0x10
+#define XR21V141X_URM_ENABLE_TX_FIFO 0x1
+#define XR21V141X_URM_ENABLE_RX_FIFO 0x2
+
+#define XR21V141X_URM_RX_FIFO_RESET 0x18
+#define XR21V141X_URM_TX_FIFO_RESET 0x1c
+
+#define UART_ENABLE_TX 0x1
+#define UART_ENABLE_RX 0x2
+
+#define UART_MODE_RI BIT(0)
+#define UART_MODE_CD BIT(1)
+#define UART_MODE_DSR BIT(2)
+#define UART_MODE_DTR BIT(3)
+#define UART_MODE_CTS BIT(4)
+#define UART_MODE_RTS BIT(5)
+
+#define UART_BREAK_ON 0xffff
+#define UART_BREAK_OFF 0
+
+#define UART_DATA_MASK GENMASK(3, 0)
+#define UART_DATA_7 0x7
+#define UART_DATA_8 0x8
+
+#define UART_PARITY_MASK GENMASK(6, 4)
+#define UART_PARITY_SHIFT 0x4
+#define UART_PARITY_NONE 0x0
+#define UART_PARITY_ODD 0x1
+#define UART_PARITY_EVEN 0x2
+#define UART_PARITY_MARK 0x3
+#define UART_PARITY_SPACE 0x4
+
+#define UART_STOP_MASK BIT(7)
+#define UART_STOP_SHIFT 0x7
+#define UART_STOP_1 0x0
+#define UART_STOP_2 0x1
+
+#define UART_FLOW_MODE_NONE 0x0
+#define UART_FLOW_MODE_HW 0x1
+#define UART_FLOW_MODE_SW 0x2
+
+#define UART_MODE_GPIO 0x0
+#define UART_MODE_RTS_CTS 0x1
+#define UART_MODE_DTR_DSR 0x2
+#define UART_MODE_RS485 0x3
+#define UART_MODE_RS485_ADDR 0x4
+
+#endif /* __LINUX_USB_SERIAL_XR_SERIAL_H */
--
2.17.1
From: Manivannan Sadhasivam <[email protected]>
Add gpiochip support for Maxlinear/Exar USB to serial converter
for controlling the available gpios.
Cc: Linus Walleij <[email protected]>
Cc: [email protected]
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/usb/serial/xr_serial.c | 186 ++++++++++++++++++++++++++++++++-
drivers/usb/serial/xr_serial.h | 7 ++
2 files changed, 192 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index ea4a0b167d3f..d86fd40839f8 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
state);
}
+#ifdef CONFIG_GPIOLIB
+
+static int xr_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+ struct usb_serial_port *port = gpiochip_get_data(gc);
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+ /* Check if the requested GPIO is occupied */
+ if (port_priv->gpio_altfunc & BIT(offset))
+ return -ENODEV;
+
+ return 0;
+}
+
+static int xr_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct usb_serial_port *port = gpiochip_get_data(gc);
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret;
+ u16 gpio_status;
+
+ ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_status, &gpio_status);
+ if (ret)
+ return ret;
+
+ return !!(gpio_status & BIT(gpio));
+}
+
+static void xr_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct usb_serial_port *port = gpiochip_get_data(gc);
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+ if (val)
+ xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_set, BIT(gpio));
+ else
+ xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_clr, BIT(gpio));
+}
+
+static int xr_gpio_direction_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct usb_serial_port *port = gpiochip_get_data(gc);
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret;
+ u16 gpio_dir;
+
+ ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_dir, &gpio_dir);
+ if (ret)
+ return ret;
+
+ /* Logic 0 = input and Logic 1 = output */
+ return !(gpio_dir & BIT(gpio));
+}
+
+static int xr_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct usb_serial_port *port = gpiochip_get_data(gc);
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret;
+ u16 gpio_dir;
+
+ ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_dir, &gpio_dir);
+ if (ret)
+ return ret;
+
+ gpio_dir &= ~BIT(gpio);
+
+ return xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_dir, gpio_dir);
+}
+
+static int xr_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
+ int val)
+{
+ struct usb_serial_port *port = gpiochip_get_data(gc);
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret;
+ u16 gpio_dir;
+
+ ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_dir, &gpio_dir);
+ if (ret)
+ return ret;
+
+ gpio_dir |= BIT(gpio);
+
+ ret = xr_set_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_dir, gpio_dir);
+ if (ret)
+ return ret;
+
+ xr_gpio_set(gc, gpio, val);
+
+ return 0;
+}
+
+static int xr21v141x_gpio_init(struct usb_serial_port *port)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret;
+ u16 gpio_mode;
+
+ port_priv->gc.ngpio = 6;
+
+ ret = xr_get_reg(port, XR21V141X_UART_REG_BLOCK,
+ port_priv->regs->gpio_mode, &gpio_mode);
+ if (ret)
+ return ret;
+
+ /* Mark all pins which are not in GPIO mode */
+ if (gpio_mode & UART_MODE_RTS_CTS)
+ port_priv->gpio_altfunc |= (BIT(4) | BIT(5));
+ else if (gpio_mode & UART_MODE_DTR_DSR)
+ port_priv->gpio_altfunc |= (BIT(2) | BIT(3));
+ else if (gpio_mode & UART_MODE_RS485)
+ port_priv->gpio_altfunc |= BIT(5);
+ else if (gpio_mode & UART_MODE_RS485_ADDR)
+ port_priv->gpio_altfunc |= BIT(5);
+ else
+ port_priv->gpio_altfunc = 0; /* All GPIOs are available */
+
+ return ret;
+}
+
+static int xr_gpio_init(struct usb_serial_port *port)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ int ret = 0;
+
+ if (port_priv->idProduct == XR21V141X_ID)
+ ret = xr21v141x_gpio_init(port);
+
+ if (ret < 0)
+ return ret;
+
+ port_priv->gc.label = "xr_gpios";
+ port_priv->gc.request = xr_gpio_request;
+ port_priv->gc.get_direction = xr_gpio_direction_get;
+ port_priv->gc.direction_input = xr_gpio_direction_input;
+ port_priv->gc.direction_output = xr_gpio_direction_output;
+ port_priv->gc.get = xr_gpio_get;
+ port_priv->gc.set = xr_gpio_set;
+ port_priv->gc.owner = THIS_MODULE;
+ port_priv->gc.parent = &port->dev;
+ port_priv->gc.base = -1;
+ port_priv->gc.can_sleep = true;
+
+ ret = gpiochip_add_data(&port_priv->gc, port);
+ if (!ret)
+ port_priv->gpio_registered = true;
+
+ return ret;
+}
+
+static void xr_gpio_remove(struct usb_serial_port *port)
+{
+ struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+
+ if (port_priv->gpio_registered) {
+ gpiochip_remove(&port_priv->gc);
+ port_priv->gpio_registered = false;
+ }
+}
+
+#else
+
+static int xr_gpio_init(struct usb_serial_port *port)
+{
+ return 0;
+}
+
+static void xr_gpio_remove(struct usb_serial_port *port)
+{
+ /* Nothing to do */
+}
+
+#endif
+
static int xr_port_probe(struct usb_serial_port *port)
{
struct usb_serial *serial = port->serial;
@@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)
usb_set_serial_port_data(port, port_priv);
- return 0;
+ return xr_gpio_init(port);
}
static int xr_port_remove(struct usb_serial_port *port)
{
struct xr_port_private *port_priv = usb_get_serial_port_data(port);
+ xr_gpio_remove(port);
kfree(port_priv);
return 0;
diff --git a/drivers/usb/serial/xr_serial.h b/drivers/usb/serial/xr_serial.h
index d2977ef847a0..079098cf553a 100644
--- a/drivers/usb/serial/xr_serial.h
+++ b/drivers/usb/serial/xr_serial.h
@@ -3,6 +3,8 @@
#ifndef __LINUX_USB_SERIAL_XR_SERIAL_H
#define __LINUX_USB_SERIAL_XR_SERIAL_H
+#include <linux/gpio/driver.h>
+
struct xr_uart_regs {
u8 enable;
u8 format;
@@ -21,6 +23,11 @@ struct xr_uart_regs {
};
struct xr_port_private {
+#ifdef CONFIG_GPIOLIB
+ struct gpio_chip gc;
+ bool gpio_registered;
+ u8 gpio_altfunc;
+#endif
const struct xr_uart_regs *regs;
u16 idProduct;
u8 reg_width;
--
2.17.1
On Wed, Apr 29, 2020 at 01:26:50AM +0530, [email protected] wrote:
> From: Manivannan Sadhasivam <[email protected]>
>
> Add support for MaxLinear/Exar USB to Serial converters. This driver
> only supports XR21V141X series but provision has been made to support
> other series in future.
>
> This driver is inspired from the initial one submitted by Patong Yang:
>
> https://patchwork.kernel.org/patch/10543261/
>
> 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.
Nice work!
Some comments below:
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MaxLinear/Exar USB to Serial driver
> + *
> + * Based on initial driver written by Patong Yang <[email protected]>
> + *
> + * Copyright (c) 2020 Manivannan Sadhasivam <[email protected]>
> + */
> +
> +#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>
> +
> +#include "xr_serial.h"
No need for a .h file for a single .c file.
> +static int xr_get_reg(struct usb_serial_port *port, u8 block, u16 reg,
> + u16 *val)
> +{
> + struct usb_serial *serial = port->serial;
> + struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> + void *dmabuf;
> + int ret = -EINVAL;
> +
> + dmabuf = kmalloc(sizeof(reg), GFP_KERNEL);
So that is 2 bytes?
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + if (port_priv->idProduct == XR21V141X_ID) {
> + /* XR21V141X uses custom command for reading UART registers */
> + ret = usb_control_msg(serial->dev,
> + usb_rcvctrlpipe(serial->dev, 0),
> + XR_GET_XR21V141X,
> + USB_DIR_IN | USB_TYPE_VENDOR, 0,
> + reg | (block << 8), dmabuf,
> + port_priv->reg_width,
> + USB_CTRL_SET_TIMEOUT);
> + }
> +
> + if (ret == port_priv->reg_width) {
> + memcpy(val, dmabuf, port_priv->reg_width);
But here you copy ->reg_width bytes in? How do you know val can hold
that much? It's only set to be 1, so you copy 1 byte to a 16bit value?
What part of the 16bits did you just copy those 8 bits to (hint, think
cpu endian issues...)
That feels really really odd and a bit broken.
> --- /dev/null
> +++ b/drivers/usb/serial/xr_serial.h
> @@ -0,0 +1,103 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
Are you sure about the "+"? I have to ask :)
> +
> +#ifndef __LINUX_USB_SERIAL_XR_SERIAL_H
> +#define __LINUX_USB_SERIAL_XR_SERIAL_H
As you will drop this file, just a general statement, no need for
__LINUX as this is all Linux :)
thanks,
greg k-h
Hi Greg,
On Wed, Apr 29, 2020 at 09:20:36AM +0200, Greg KH wrote:
> On Wed, Apr 29, 2020 at 01:26:50AM +0530, [email protected] wrote:
> > From: Manivannan Sadhasivam <[email protected]>
> >
> > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > only supports XR21V141X series but provision has been made to support
> > other series in future.
> >
> > This driver is inspired from the initial one submitted by Patong Yang:
> >
> > https://patchwork.kernel.org/patch/10543261/
> >
> > 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.
>
> Nice work!
>
> Some comments below:
>
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * MaxLinear/Exar USB to Serial driver
> > + *
> > + * Based on initial driver written by Patong Yang <[email protected]>
> > + *
> > + * Copyright (c) 2020 Manivannan Sadhasivam <[email protected]>
> > + */
> > +
> > +#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>
> > +
> > +#include "xr_serial.h"
>
> No need for a .h file for a single .c file.
>
Yeah but since this driver can support multiple series of XR chips (they
might have separate register definitions and such), I thought it is a good
idea to have a header file to keep the driver sane. But can club it to the
source file for now.
> > +static int xr_get_reg(struct usb_serial_port *port, u8 block, u16 reg,
> > + u16 *val)
> > +{
> > + struct usb_serial *serial = port->serial;
> > + struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> > + void *dmabuf;
> > + int ret = -EINVAL;
> > +
> > + dmabuf = kmalloc(sizeof(reg), GFP_KERNEL);
>
> So that is 2 bytes?
>
Explanation below...
> > + if (!dmabuf)
> > + return -ENOMEM;
> > +
> > + if (port_priv->idProduct == XR21V141X_ID) {
> > + /* XR21V141X uses custom command for reading UART registers */
> > + ret = usb_control_msg(serial->dev,
> > + usb_rcvctrlpipe(serial->dev, 0),
> > + XR_GET_XR21V141X,
> > + USB_DIR_IN | USB_TYPE_VENDOR, 0,
> > + reg | (block << 8), dmabuf,
> > + port_priv->reg_width,
> > + USB_CTRL_SET_TIMEOUT);
> > + }
> > +
> > + if (ret == port_priv->reg_width) {
> > + memcpy(val, dmabuf, port_priv->reg_width);
>
> But here you copy ->reg_width bytes in? How do you know val can hold
> that much? It's only set to be 1, so you copy 1 byte to a 16bit value?
> What part of the 16bits did you just copy those 8 bits to (hint, think
> cpu endian issues...)
>
> That feels really really odd and a bit broken.
>
Right. The reason is, the other series which can be supported by this driver
have different register widths. For instance XR2280x. I haven't used them
personally but seen this in initial driver. So I just used the max u16 type
to make the reg_{set/get} routines work with those.
But agree, I should've used le16_to_cpu() cast to avoid endian issues.
If you think this hack is not required now, I can just use u8 and worry about
compatibility later.
> > --- /dev/null
> > +++ b/drivers/usb/serial/xr_serial.h
> > @@ -0,0 +1,103 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
>
> Are you sure about the "+"? I have to ask :)
>
I'm not a fan but since I've inherited the code from initial driver (which
was GPL-2.0+), I kept it.
> > +
> > +#ifndef __LINUX_USB_SERIAL_XR_SERIAL_H
> > +#define __LINUX_USB_SERIAL_XR_SERIAL_H
>
> As you will drop this file, just a general statement, no need for
> __LINUX as this is all Linux :)
>
Sure.
Thanks,
Mani
> thanks,
>
> greg k-h
On Wed, Apr 29, 2020 at 01:10:26PM +0530, Manivannan Sadhasivam wrote:
> Hi Greg,
>
> On Wed, Apr 29, 2020 at 09:20:36AM +0200, Greg KH wrote:
> > On Wed, Apr 29, 2020 at 01:26:50AM +0530, [email protected] wrote:
> > > From: Manivannan Sadhasivam <[email protected]>
> > >
> > > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > > only supports XR21V141X series but provision has been made to support
> > > other series in future.
> > >
> > > This driver is inspired from the initial one submitted by Patong Yang:
> > >
> > > https://patchwork.kernel.org/patch/10543261/
> > >
> > > 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.
> >
> > Nice work!
> >
> > Some comments below:
> >
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * MaxLinear/Exar USB to Serial driver
> > > + *
> > > + * Based on initial driver written by Patong Yang <[email protected]>
> > > + *
> > > + * Copyright (c) 2020 Manivannan Sadhasivam <[email protected]>
> > > + */
> > > +
> > > +#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>
> > > +
> > > +#include "xr_serial.h"
> >
> > No need for a .h file for a single .c file.
> >
>
> Yeah but since this driver can support multiple series of XR chips (they
> might have separate register definitions and such), I thought it is a good
> idea to have a header file to keep the driver sane. But can club it to the
> source file for now.
Don't worry about future stuff, focus on what you need to do now :)
> > > +static int xr_get_reg(struct usb_serial_port *port, u8 block, u16 reg,
> > > + u16 *val)
> > > +{
> > > + struct usb_serial *serial = port->serial;
> > > + struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> > > + void *dmabuf;
> > > + int ret = -EINVAL;
> > > +
> > > + dmabuf = kmalloc(sizeof(reg), GFP_KERNEL);
> >
> > So that is 2 bytes?
> >
>
> Explanation below...
>
> > > + if (!dmabuf)
> > > + return -ENOMEM;
> > > +
> > > + if (port_priv->idProduct == XR21V141X_ID) {
> > > + /* XR21V141X uses custom command for reading UART registers */
> > > + ret = usb_control_msg(serial->dev,
> > > + usb_rcvctrlpipe(serial->dev, 0),
> > > + XR_GET_XR21V141X,
> > > + USB_DIR_IN | USB_TYPE_VENDOR, 0,
> > > + reg | (block << 8), dmabuf,
> > > + port_priv->reg_width,
> > > + USB_CTRL_SET_TIMEOUT);
> > > + }
> > > +
> > > + if (ret == port_priv->reg_width) {
> > > + memcpy(val, dmabuf, port_priv->reg_width);
> >
> > But here you copy ->reg_width bytes in? How do you know val can hold
> > that much? It's only set to be 1, so you copy 1 byte to a 16bit value?
> > What part of the 16bits did you just copy those 8 bits to (hint, think
> > cpu endian issues...)
> >
> > That feels really really odd and a bit broken.
> >
>
> Right. The reason is, the other series which can be supported by this driver
> have different register widths. For instance XR2280x. I haven't used them
> personally but seen this in initial driver. So I just used the max u16 type
> to make the reg_{set/get} routines work with those.
Drop the whole "different register width" stuff for now, as the driver
does not support it and it adds additional complexity that is hard to
review for no good reason. If you want to add support for new devices
later, _then_ we can add support for that.
Don't over-engineer :)
> But agree, I should've used le16_to_cpu() cast to avoid endian issues.
You have to, the code is broken as-is right now.
> If you think this hack is not required now, I can just use u8 and worry about
> compatibility later.
Please do so.
thanks,
greg k-h
On Tue, Apr 28, 2020 at 9:57 PM <[email protected]> wrote:
> From: Manivannan Sadhasivam <[email protected]>
>
> Add gpiochip support for Maxlinear/Exar USB to serial converter
> for controlling the available gpios.
>
> Cc: Linus Walleij <[email protected]>
> Cc: [email protected]
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
That's a nice and clean GPIO driver.
I would change this:
port_priv->gc.label = "xr_gpios";
to something that is device-unique, like "xr-gpios-<serial number>"
which makes it easy to locate the GPIOs on a specific serial converter
for lab use. However the USB serial maintainers know better what
to use here. Whatever makes a USB-to-serial unique from a TTY
point of view is probably fine with me too.
My idea is that people might want to know which USB cable
this is sitting on, so I have this USB cable and from this label
I can always figure out which GPIO device it is.
Either way, it is not a super-big issue so:
Reviewed-by: Linus Walleij <[email protected]>
Is this a off-the-shelf product that can be bought or is it mainly
integrated on boards?
I'm asking because I'm looking for a neat USB-to-serial adapter
with some GPIOs (2 is enough) that can be used for reset and
power cycling of lab boards using one simple piece of equipment.
Yours,
Linus Walleij
Hi Linus,
On Wed, Apr 29, 2020 at 02:12:24PM +0200, Linus Walleij wrote:
> On Tue, Apr 28, 2020 at 9:57 PM <[email protected]> wrote:
>
> > From: Manivannan Sadhasivam <[email protected]>
> >
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.
> >
> > Cc: Linus Walleij <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
>
> That's a nice and clean GPIO driver.
Thanks for the compliments :)
>
> I would change this:
>
> port_priv->gc.label = "xr_gpios";
>
> to something that is device-unique, like "xr-gpios-<serial number>"
> which makes it easy to locate the GPIOs on a specific serial converter
> for lab use. However the USB serial maintainers know better what
> to use here. Whatever makes a USB-to-serial unique from a TTY
> point of view is probably fine with me too.
>
> My idea is that people might want to know which USB cable
> this is sitting on, so I have this USB cable and from this label
> I can always figure out which GPIO device it is.
>
Sounds reasonable. I can postfix the PID as below:
port_priv->gc.label = devm_kasprintf(port->dev, GFP_KERNEL, "XR%04x",
port_priv->idProduct);
So this will become, "XR1410".
> Either way, it is not a super-big issue so:
> Reviewed-by: Linus Walleij <[email protected]>
>
> Is this a off-the-shelf product that can be bought or is it mainly
> integrated on boards?
>
Both I believe, though I have only used it integrated in dev boards. But
a quick googling gives me below,
https://www.digikey.in/product-detail/en/maxlinear-inc/XR21V1410IL-0C-EB/1016-1425-ND/2636664
Thanks,
Mani
> I'm asking because I'm looking for a neat USB-to-serial adapter
> with some GPIOs (2 is enough) that can be used for reset and
> power cycling of lab boards using one simple piece of equipment.
>
> Yours,
> Linus Walleij
On Wed, Apr 29, 2020 at 11:29:04AM +0200, Greg KH wrote:
> On Wed, Apr 29, 2020 at 01:10:26PM +0530, Manivannan Sadhasivam wrote:
> > Hi Greg,
> >
> > On Wed, Apr 29, 2020 at 09:20:36AM +0200, Greg KH wrote:
> > > On Wed, Apr 29, 2020 at 01:26:50AM +0530, [email protected] wrote:
> > > > From: Manivannan Sadhasivam <[email protected]>
> > > >
> > > > Add support for MaxLinear/Exar USB to Serial converters. This driver
> > > > only supports XR21V141X series but provision has been made to support
> > > > other series in future.
> > > >
> > > > This driver is inspired from the initial one submitted by Patong Yang:
> > > >
> > > > https://patchwork.kernel.org/patch/10543261/
> > > >
> > > > 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.
> > >
> > > Nice work!
> > >
> > > Some comments below:
> > >
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * MaxLinear/Exar USB to Serial driver
> > > > + *
> > > > + * Based on initial driver written by Patong Yang <[email protected]>
> > > > + *
> > > > + * Copyright (c) 2020 Manivannan Sadhasivam <[email protected]>
> > > > + */
> > > > +
> > > > +#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>
> > > > +
> > > > +#include "xr_serial.h"
> > >
> > > No need for a .h file for a single .c file.
> > >
> >
> > Yeah but since this driver can support multiple series of XR chips (they
> > might have separate register definitions and such), I thought it is a good
> > idea to have a header file to keep the driver sane. But can club it to the
> > source file for now.
>
> Don't worry about future stuff, focus on what you need to do now :)
>
Alright, will do :)
> > > > +static int xr_get_reg(struct usb_serial_port *port, u8 block, u16 reg,
> > > > + u16 *val)
> > > > +{
> > > > + struct usb_serial *serial = port->serial;
> > > > + struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> > > > + void *dmabuf;
> > > > + int ret = -EINVAL;
> > > > +
> > > > + dmabuf = kmalloc(sizeof(reg), GFP_KERNEL);
> > >
> > > So that is 2 bytes?
> > >
> >
> > Explanation below...
> >
> > > > + if (!dmabuf)
> > > > + return -ENOMEM;
> > > > +
> > > > + if (port_priv->idProduct == XR21V141X_ID) {
> > > > + /* XR21V141X uses custom command for reading UART registers */
> > > > + ret = usb_control_msg(serial->dev,
> > > > + usb_rcvctrlpipe(serial->dev, 0),
> > > > + XR_GET_XR21V141X,
> > > > + USB_DIR_IN | USB_TYPE_VENDOR, 0,
> > > > + reg | (block << 8), dmabuf,
> > > > + port_priv->reg_width,
> > > > + USB_CTRL_SET_TIMEOUT);
> > > > + }
> > > > +
> > > > + if (ret == port_priv->reg_width) {
> > > > + memcpy(val, dmabuf, port_priv->reg_width);
> > >
> > > But here you copy ->reg_width bytes in? How do you know val can hold
> > > that much? It's only set to be 1, so you copy 1 byte to a 16bit value?
> > > What part of the 16bits did you just copy those 8 bits to (hint, think
> > > cpu endian issues...)
> > >
> > > That feels really really odd and a bit broken.
> > >
> >
> > Right. The reason is, the other series which can be supported by this driver
> > have different register widths. For instance XR2280x. I haven't used them
> > personally but seen this in initial driver. So I just used the max u16 type
> > to make the reg_{set/get} routines work with those.
>
> Drop the whole "different register width" stuff for now, as the driver
> does not support it and it adds additional complexity that is hard to
> review for no good reason. If you want to add support for new devices
> later, _then_ we can add support for that.
>
> Don't over-engineer :)
>
Sure!
> > But agree, I should've used le16_to_cpu() cast to avoid endian issues.
>
> You have to, the code is broken as-is right now.
>
We don't need the conversion if u8 is used everywhere :)
Thanks,
Mani
> > If you think this hack is not required now, I can just use u8 and worry about
> > compatibility later.
>
> Please do so.
>
> thanks,
>
> greg k-h
Hi Greg,
On Wed, Apr 29, 2020 at 01:26:51AM +0530, [email protected] wrote:
> From: Manivannan Sadhasivam <[email protected]>
>
> Add gpiochip support for Maxlinear/Exar USB to serial converter
> for controlling the available gpios.
>
> Cc: Linus Walleij <[email protected]>
> Cc: [email protected]
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> drivers/usb/serial/xr_serial.c | 186 ++++++++++++++++++++++++++++++++-
> drivers/usb/serial/xr_serial.h | 7 ++
> 2 files changed, 192 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index ea4a0b167d3f..d86fd40839f8 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
> state);
> }
>
> +#ifdef CONFIG_GPIOLIB
> +
[...]
> +
> +static int xr_gpio_init(struct usb_serial_port *port)
> +{
> + struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> + int ret = 0;
> +
> + if (port_priv->idProduct == XR21V141X_ID)
> + ret = xr21v141x_gpio_init(port);
> +
> + if (ret < 0)
> + return ret;
> +
> + port_priv->gc.label = "xr_gpios";
> + port_priv->gc.request = xr_gpio_request;
> + port_priv->gc.get_direction = xr_gpio_direction_get;
> + port_priv->gc.direction_input = xr_gpio_direction_input;
> + port_priv->gc.direction_output = xr_gpio_direction_output;
> + port_priv->gc.get = xr_gpio_get;
> + port_priv->gc.set = xr_gpio_set;
> + port_priv->gc.owner = THIS_MODULE;
> + port_priv->gc.parent = &port->dev;
> + port_priv->gc.base = -1;
> + port_priv->gc.can_sleep = true;
> +
> + ret = gpiochip_add_data(&port_priv->gc, port);
> + if (!ret)
> + port_priv->gpio_registered = true;
> +
> + return ret;
> +}
> +
> +static void xr_gpio_remove(struct usb_serial_port *port)
> +{
> + struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> +
> + if (port_priv->gpio_registered) {
> + gpiochip_remove(&port_priv->gc);
> + port_priv->gpio_registered = false;
> + }
> +}
> +
> +#else
> +
> +static int xr_gpio_init(struct usb_serial_port *port)
> +{
> + return 0;
> +}
> +
> +static void xr_gpio_remove(struct usb_serial_port *port)
> +{
> + /* Nothing to do */
> +}
> +
> +#endif
> +
> static int xr_port_probe(struct usb_serial_port *port)
> {
> struct usb_serial *serial = port->serial;
> @@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)
>
> usb_set_serial_port_data(port, port_priv);
>
> - return 0;
> + return xr_gpio_init(port);
Just realised that the gpiochip is registered for 2 interfaces exposed by
this chip. This is due to the fact that this chip presents CDC-ACM model,
so there are 2 interfaces (interrupt and bulk IN/OUT).
We shouldn't need gpiochip for interface 0. So what is the recommended way
to filter that?
Thanks,
Mani
> }
>
> static int xr_port_remove(struct usb_serial_port *port)
> {
> struct xr_port_private *port_priv = usb_get_serial_port_data(port);
>
> + xr_gpio_remove(port);
> kfree(port_priv);
>
> return 0;
> diff --git a/drivers/usb/serial/xr_serial.h b/drivers/usb/serial/xr_serial.h
> index d2977ef847a0..079098cf553a 100644
> --- a/drivers/usb/serial/xr_serial.h
> +++ b/drivers/usb/serial/xr_serial.h
> @@ -3,6 +3,8 @@
> #ifndef __LINUX_USB_SERIAL_XR_SERIAL_H
> #define __LINUX_USB_SERIAL_XR_SERIAL_H
>
> +#include <linux/gpio/driver.h>
> +
> struct xr_uart_regs {
> u8 enable;
> u8 format;
> @@ -21,6 +23,11 @@ struct xr_uart_regs {
> };
>
> struct xr_port_private {
> +#ifdef CONFIG_GPIOLIB
> + struct gpio_chip gc;
> + bool gpio_registered;
> + u8 gpio_altfunc;
> +#endif
> const struct xr_uart_regs *regs;
> u16 idProduct;
> u8 reg_width;
> --
> 2.17.1
>
On Wed, Apr 29, 2020 at 11:17:27PM +0530, Manivannan Sadhasivam wrote:
> Hi Greg,
>
> On Wed, Apr 29, 2020 at 01:26:51AM +0530, [email protected] wrote:
> > From: Manivannan Sadhasivam <[email protected]>
> >
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.
> >
> > Cc: Linus Walleij <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > drivers/usb/serial/xr_serial.c | 186 ++++++++++++++++++++++++++++++++-
> > drivers/usb/serial/xr_serial.h | 7 ++
> > 2 files changed, 192 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> > index ea4a0b167d3f..d86fd40839f8 100644
> > --- a/drivers/usb/serial/xr_serial.c
> > +++ b/drivers/usb/serial/xr_serial.c
> > @@ -476,6 +476,189 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
> > state);
> > }
> >
> > +#ifdef CONFIG_GPIOLIB
> > +
>
> [...]
>
> > +
> > +static int xr_gpio_init(struct usb_serial_port *port)
> > +{
> > + struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> > + int ret = 0;
> > +
> > + if (port_priv->idProduct == XR21V141X_ID)
> > + ret = xr21v141x_gpio_init(port);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + port_priv->gc.label = "xr_gpios";
> > + port_priv->gc.request = xr_gpio_request;
> > + port_priv->gc.get_direction = xr_gpio_direction_get;
> > + port_priv->gc.direction_input = xr_gpio_direction_input;
> > + port_priv->gc.direction_output = xr_gpio_direction_output;
> > + port_priv->gc.get = xr_gpio_get;
> > + port_priv->gc.set = xr_gpio_set;
> > + port_priv->gc.owner = THIS_MODULE;
> > + port_priv->gc.parent = &port->dev;
> > + port_priv->gc.base = -1;
> > + port_priv->gc.can_sleep = true;
> > +
> > + ret = gpiochip_add_data(&port_priv->gc, port);
> > + if (!ret)
> > + port_priv->gpio_registered = true;
> > +
> > + return ret;
> > +}
> > +
> > +static void xr_gpio_remove(struct usb_serial_port *port)
> > +{
> > + struct xr_port_private *port_priv = usb_get_serial_port_data(port);
> > +
> > + if (port_priv->gpio_registered) {
> > + gpiochip_remove(&port_priv->gc);
> > + port_priv->gpio_registered = false;
> > + }
> > +}
> > +
> > +#else
> > +
> > +static int xr_gpio_init(struct usb_serial_port *port)
> > +{
> > + return 0;
> > +}
> > +
> > +static void xr_gpio_remove(struct usb_serial_port *port)
> > +{
> > + /* Nothing to do */
> > +}
> > +
> > +#endif
> > +
> > static int xr_port_probe(struct usb_serial_port *port)
> > {
> > struct usb_serial *serial = port->serial;
> > @@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)
> >
> > usb_set_serial_port_data(port, port_priv);
> >
> > - return 0;
> > + return xr_gpio_init(port);
>
> Just realised that the gpiochip is registered for 2 interfaces exposed by
> this chip. This is due to the fact that this chip presents CDC-ACM model,
> so there are 2 interfaces (interrupt and bulk IN/OUT).
>
> We shouldn't need gpiochip for interface 0. So what is the recommended way
> to filter that?
Not create the gpiochip for interface 0? :)
I really don't know what else to say here, sorry.
greg k-h
On Wed, Apr 29, 2020 at 06:19:18PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 29, 2020 at 02:12:24PM +0200, Linus Walleij wrote:
> > On Tue, Apr 28, 2020 at 9:57 PM <[email protected]> wrote:
> >
> > > From: Manivannan Sadhasivam <[email protected]>
> > >
> > > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > > for controlling the available gpios.
> > >
> > > Cc: Linus Walleij <[email protected]>
> > > Cc: [email protected]
> > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > I would change this:
> >
> > port_priv->gc.label = "xr_gpios";
> >
> > to something that is device-unique, like "xr-gpios-<serial number>"
> > which makes it easy to locate the GPIOs on a specific serial converter
> > for lab use. However the USB serial maintainers know better what
> > to use here. Whatever makes a USB-to-serial unique from a TTY
> > point of view is probably fine with me too.
> >
> > My idea is that people might want to know which USB cable
> > this is sitting on, so I have this USB cable and from this label
> > I can always figure out which GPIO device it is.
I think we've had this discussion before. First, not every device has a
unique serial number. Second, we already have a universal way of
distinguishing devices namely by using the bus topology. That's
available through sysfs and shouldn't have to be be re-encoded by every
driver in the gpiochip name.
> Sounds reasonable. I can postfix the PID as below:
>
> port_priv->gc.label = devm_kasprintf(port->dev, GFP_KERNEL, "XR%04x",
> port_priv->idProduct);
>
> So this will become, "XR1410".
So this doesn't really buy us anything; what if you have two of these
devices?
Johan
On Wed, Apr 29, 2020 at 11:17:27PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 29, 2020 at 01:26:51AM +0530, [email protected] wrote:
> > From: Manivannan Sadhasivam <[email protected]>
> >
> > Add gpiochip support for Maxlinear/Exar USB to serial converter
> > for controlling the available gpios.
> >
> > Cc: Linus Walleij <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > static int xr_port_probe(struct usb_serial_port *port)
> > {
> > struct usb_serial *serial = port->serial;
> > @@ -495,13 +678,14 @@ static int xr_port_probe(struct usb_serial_port *port)
> >
> > usb_set_serial_port_data(port, port_priv);
> >
> > - return 0;
> > + return xr_gpio_init(port);
>
> Just realised that the gpiochip is registered for 2 interfaces exposed by
> this chip. This is due to the fact that this chip presents CDC-ACM model,
> so there are 2 interfaces (interrupt and bulk IN/OUT).
>
> We shouldn't need gpiochip for interface 0. So what is the recommended way
> to filter that?
Your driver should only bind to the data interface, but also claim the
control interface (i.e. the reverse of what cdc-acm is doing).
This CDC model doesn't really fit the assumptions of usb-serial core,
but it might be doable. Try returning 1 from the attach callback for the
control interface so that core claims it but doesn't register a tty
device.
Johan
On Tue, May 19, 2020 at 10:57 AM Johan Hovold <[email protected]> wrote:
> > On Wed, Apr 29, 2020 at 02:12:24PM +0200, Linus Walleij wrote:
> > > to something that is device-unique, like "xr-gpios-<serial number>"
> > > which makes it easy to locate the GPIOs on a specific serial converter
> > > for lab use. However the USB serial maintainers know better what
> > > to use here. Whatever makes a USB-to-serial unique from a TTY
> > > point of view is probably fine with me too.
> > >
> > > My idea is that people might want to know which USB cable
> > > this is sitting on, so I have this USB cable and from this label
> > > I can always figure out which GPIO device it is.
>
> I think we've had this discussion before. First, not every device has a
> unique serial number. Second, we already have a universal way of
> distinguishing devices namely by using the bus topology. That's
> available through sysfs and shouldn't have to be be re-encoded by every
> driver in the gpiochip name.
I remember I even referred to this myself, but I've been waning a bit
on it recently, because it turns out that userspace/users aren't very
good at parsing sysfs for topology.
For userspace other than udev there seems to be a kind of agreement
gap. Dunno how best to bridge it though. Education maybe.
Yours,
Linus Walleij
On Mon, May 25, 2020 at 03:02:15PM +0200, Linus Walleij wrote:
> On Mon, May 25, 2020 at 1:12 PM Greg KH <[email protected]> wrote:
>
> > > I remember I even referred to this myself, but I've been waning a bit
> > > on it recently, because it turns out that userspace/users aren't very
> > > good at parsing sysfs for topology.
> >
> > Which is why they could use libudev :)
>
> Yet they insist on using things like Busybox' mdev (e.g. OpenWrt)
> or Android ... or is Android using libudev now? I'd be delighted
> if they did.
No, Android is not using libudev yet, they seem to be reinventing the
same thing, slowly, over time :(
greg k-h
On Mon, May 25, 2020 at 10:59:59AM +0200, Linus Walleij wrote:
> On Tue, May 19, 2020 at 10:57 AM Johan Hovold <[email protected]> wrote:
> > > On Wed, Apr 29, 2020 at 02:12:24PM +0200, Linus Walleij wrote:
>
> > > > to something that is device-unique, like "xr-gpios-<serial number>"
> > > > which makes it easy to locate the GPIOs on a specific serial converter
> > > > for lab use. However the USB serial maintainers know better what
> > > > to use here. Whatever makes a USB-to-serial unique from a TTY
> > > > point of view is probably fine with me too.
> > > >
> > > > My idea is that people might want to know which USB cable
> > > > this is sitting on, so I have this USB cable and from this label
> > > > I can always figure out which GPIO device it is.
> >
> > I think we've had this discussion before. First, not every device has a
> > unique serial number. Second, we already have a universal way of
> > distinguishing devices namely by using the bus topology. That's
> > available through sysfs and shouldn't have to be be re-encoded by every
> > driver in the gpiochip name.
>
> I remember I even referred to this myself, but I've been waning a bit
> on it recently, because it turns out that userspace/users aren't very
> good at parsing sysfs for topology.
Which is why they could use libudev :)
thanks,
greg k-h
On Mon, May 25, 2020 at 1:12 PM Greg KH <[email protected]> wrote:
> > I remember I even referred to this myself, but I've been waning a bit
> > on it recently, because it turns out that userspace/users aren't very
> > good at parsing sysfs for topology.
>
> Which is why they could use libudev :)
Yet they insist on using things like Busybox' mdev (e.g. OpenWrt)
or Android ... or is Android using libudev now? I'd be delighted
if they did.
Yours,
Linus Walleij