Add support for MaxLinear/Exar USB to serial converters
Subsystems (4)
v4 cover Add support for MaxLinear/Exar USB to serial converters
Hello,
This series adds support for MaxLinear/Exar USB to serial converters. This driver only supports XR21V141X series but it can easily be extended to other series in future.
This driver is inspired from the initial one submitted by Patong Yang:
https://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 with Hikey970 board hosting XR21V141X chip.
Thanks,
Mani
Changes in v4:
* Multiple improvements based on Johan's review. Noticeable ones are:
- Now the driver claims both control and data interfaces but only registers tty device for data interface.
- GPIO pin status is now shared between the console and gpiolib implementations. This is done to avoid changing the lines spuriously.
- A separate port_open flag is added to reject GPIO requests while the tty port is open.
- Removed padding PID to gpio device.
* Added Greg and Mauro's review and tested tags.
* Included a patch from Mauro to avoid the CDC-ACM driver to claim this device when this driver is built.
Changes in v3:
* Dropped the check for PID and also the reg_width property.
Changes in v2:
* Dropped the code related to handling variable register size. It's all u8 now.
* Dropped the header file and moved the contents to driver itself.
* Added Linus's reviewed-by tag for gpiochip patch.
* Added PID to gpiochip label
* Dropped gpiochip for interface 0
Manivannan Sadhasivam (2):
usb: serial: Add MaxLinear/Exar USB to Serial driver
usb: serial: xr_serial: Add gpiochip support
Mauro Carvalho Chehab (1):
usb: cdc-acm: Ignore Exar XR21V141X when serial driver is built
drivers/usb/class/cdc-acm.c | 6 + drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/xr_serial.c | 857 +++++++++++++++++++++++++++++++++ 4 files changed, 873 insertions(+) create mode 100644 drivers/usb/serial/xr_serial.c
Signature · 1 line
2.17.1
PATCH RESEND v4 1/3 usb: serial: Add MaxLinear/Exar USB to Serial driver
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://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.
--- drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/xr_serial.c | 650 +++++++++++++++++++++++++++++++++ 3 files changed, 660 insertions(+) create mode 100644 drivers/usb/serial/xr_serial.c
drivers/usb/serial/Kconfig +9 -0
@@ -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
drivers/usb/serial/Makefile +1 -0
@@ -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
drivers/usb/serial/xr_serial.c new file +650 -0
650 lines not on this page · raw message
Signature · 1 line
2.17.1
Johan Hovold
maintainerRe: [RESEND PATCH v4 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver
Quoted 7 lines · Manivannan Sadhasivam
Add support for MaxLinear/Exar USB to Serial converters. This driver only supports XR21V141X series but it can be extended to other series from Exar as well in future.
This driver is inspired from the initial one submitted by Patong Yang:
You've also copied code from that driver so you need to maintain its copyright as well.
Probably better you link to lore than patchwork. Do that in the file header as well.
Quoted 52 lines
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.
Reviewed-by Greg Kroah-Hartman <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/xr_serial.c | 650 +++++++++++++++++++++++++++++++++ 3 files changed, 660 insertions(+) create mode 100644 drivers/usb/serial/xr_serial.cdrivers/usb/serial/Kconfig +9
-0@@ -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" helpdrivers/usb/serial/Makefile +1
-0@@ -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.odrivers/usb/serial/xr_serial.c new file +0
-0
drivers/usb/serial/xr_serial.c line 5
Quoted 5 lines
drivers/usb/serial/xr_serial.c new file +5
-0@@ -0,0 +1,650 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* MaxLinear/Exar USB to Serial driver +* +* Based on initial driver written by Patong Yang <[email protected]>
Reproduce Patong Yang's copyright here as well.
drivers/usb/serial/xr_serial.c line 8
Quoted 3 lines
drivers/usb/serial/xr_serial.c new file +3
-0+* +* This driver claims both data and control interfaces but only registers tty +* device for data interface.
Not sure this is needed here; this is just the way CDC-ACM works.
drivers/usb/serial/xr_serial.c line 10
Quoted 2 lines
drivers/usb/serial/xr_serial.c new file +2
-0+* +* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]>
Not your employer?
drivers/usb/serial/xr_serial.c line 22
Quoted 12 lines
drivers/usb/serial/xr_serial.c new file +12
-0+*/ + +#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> + +static void xr_set_termios (struct tty_struct *tty, +struct usb_serial_port *port, +struct ktermios *old_termios);
Move set_termios() before open() so that you don't need this.
Quoted 11 lines
drivers/usb/serial/xr_serial.c new file +11
-0+ +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;
drivers/usb/serial/xr_serial.c line 43
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+u8 gpio_dir; +u8 gpio_int_mask; +u8 gpio_set; +u8 gpio_clr; +u8 gpio_status; +}; + +struct xr_port_privte { +const struct xr_uart_regs *regs; +bool port_open;
No lock? As I pointed out before, you're gonna need one to coordinate with gpiolib. Better not add the flag until you use it.
drivers/usb/serial/xr_serial.c line 52
Quoted 9 lines
drivers/usb/serial/xr_serial.c new file +9
-0+}; + +struct xr_txrx_clk_mask { +u16 tx; +u16 rx0; +u16 rx1; +}; + +#define XR21V141X_ID 0x1410
Not used.
drivers/usb/serial/xr_serial.c line 59
Quoted 7 lines
drivers/usb/serial/xr_serial.c new file +7
-0+#define XR_INT_OSC_HZ 48000000U +#define XR21V141X_MIN_SPEED 46U +#define XR21V141X_MAX_SPEED XR_INT_OSC_HZ + +/* USB Requests */ +#define XR_SET_XR21V141X 0 +#define XR_GET_XR21V141X 1
Stick to one style; either use a device-type specific prefix or suffix for the defines.
drivers/usb/serial/xr_serial.c line 67
Quoted 8 lines
drivers/usb/serial/xr_serial.c new file +8
-0+ +#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
Please 0-pad these are they are registers.
drivers/usb/serial/xr_serial.c line 80
Quoted 13 lines
drivers/usb/serial/xr_serial.c new file +13
-0+ +/* XR21V141X register blocks */ +#define XR21V141X_UART_REG_BLOCK 0 +#define XR21V141X_UART_MANAGER_REG_BLOCK 4 +#define XR21V141X_UART_CUSTOM_BLOCK 0x66 + +/* XR21V141X UART Manager Registers */ +#define XR21V141X_UART_MANAGER_FIFO_ENABLE_REG 0x10 +#define XR21V141X_UART_MANAGER_ENABLE_TX_FIFO 0x1 +#define XR21V141X_UART_MANAGER_ENABLE_RX_FIFO 0x2 + +#define XR21V141X_UART_MANAGER_RX_FIFO_RESET 0x18 +#define XR21V141X_UART_MANAGER_TX_FIFO_RESET 0x1c
s/UART_MANAGER/UM/ should do fine for the register definitions (it was just "URM" that was non-obvious).
Quoted 30 lines
drivers/usb/serial/xr_serial.c new file +30
-0+ +#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 0xff +#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
drivers/usb/serial/xr_serial.c line 120
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ +#define UART_FLOW_MODE_NONE 0x0 +#define UART_FLOW_MODE_HW 0x1 +#define UART_FLOW_MODE_SW 0x2 + +#define UART_MODE_GPIO_MASK GENMASK(2, 0) +#define UART_MODE_RTS_CTS 0x1 +#define UART_MODE_DTR_DSR 0x2 +#define UART_MODE_RS485 0x3 +#define UART_MODE_RS485_ADDR 0x4
Please slap a XR_ prefix (or replace UART_) to the above to avoid any (future) collisions with the uapi defines.
For the these last defines the registers name is actually GPIO_MODE which seems more correct that UART_MODE.
Quoted 7 lines
drivers/usb/serial/xr_serial.c new file +7
-0+ +static const struct xr_uart_regs xr21v141x_regs = { + .enable =0x03 , + .format =0x0b , + .flow_ctrl =0x0c , + .xon_char =0x10 , + .xoff_char =0x11 ,
drivers/usb/serial/xr_serial.c line 137
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ .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, +};
Please drop this abstraction for now. If you or anyone ever adds support for another device type, we can add the parts that are actually needed then. Note that you already have code below which is XR21V141X specific.
Quoted 41 lines
drivers/usb/serial/xr_serial.c new file +41
-0+ +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 ), +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%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 ), +XR_GET_XR21V141X , +USB_DIR_IN |USB_TYPE_VENDOR ,0 , + reg | (block <<8 ), dmabuf,1 , +USB_CTRL_GET_TIMEOUT ); +if (ret ==1 ) { + *val = *dmabuf; + ret =0 ;
drivers/usb/serial/xr_serial.c line 188
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-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; +}
How about adding two xr_[sg]et_uart_reg() wrappers so you don't have to specify the uart-block number for every register access? Should improve readability.
Unless you want to follow the original driver with and _ext() version taking a block number instead.
Quoted 36 lines
drivers/usb/serial/xr_serial.c new file +36
-0+ +/* 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 }, +};
drivers/usb/serial/xr_serial.c line 234
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ +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 still need to handle B0, which is a valid request and means that DTR/RTS should be deasserted.
drivers/usb/serial/xr_serial.c line 236
Quoted 2 lines
drivers/usb/serial/xr_serial.c new file +2
-0+ divisor =XR_INT_OSC_HZ / baud; + idx = (32 * divisor) &0x1f ;
This looks broken as you're throwing away the remainder (cf. datasheet 3.3.3 and original implementation).
Quoted 73 lines
drivers/usb/serial/xr_serial.c new file +73
-0+ 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 (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 )); + +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) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret; + + ret =xr_set_reg (port,XR21V141X_UART_MANAGER_REG_BLOCK , +XR21V141X_UART_MANAGER_FIFO_ENABLE_REG , +XR21V141X_UART_MANAGER_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 );
drivers/usb/serial/xr_serial.c line 319
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+if (ret) +return ret; + + ret =xr_set_reg (port,XR21V141X_UART_MANAGER_REG_BLOCK , +XR21V141X_UART_MANAGER_FIFO_ENABLE_REG , +XR21V141X_UART_MANAGER_ENABLE_TX_FIFO | +XR21V141X_UART_MANAGER_ENABLE_RX_FIFO ); + +return ret; +}
Please disable on errors.
Quoted 12 lines
drivers/usb/serial/xr_serial.c new file +12
-0+ +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_UART_MANAGER_REG_BLOCK,
drivers/usb/serial/xr_serial.c line 341
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ XR21V141X_UART_MANAGER_FIFO_ENABLE_REG, 0); + +return ret; +} + +static int xr_open (struct tty_struct *tty,struct usb_serial_port *port) +{ +int ret; +struct device *dev = &port->dev; +struct xr_port_private *port_priv =usb_get_serial_port_data (port);
Please reverse these (longest first, reverse xmas) as in the rest of the driver.
Quoted 4 lines
drivers/usb/serial/xr_serial.c new file +4
-0+ + ret =xr_uart_enable (port); +if (ret) { +dev_err (&port->dev,"Failed to enable UART\n" );
drivers/usb/serial/xr_serial.c line 355
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+return ret; + } + +/* Setup termios */ +if (tty) +xr_set_termios (tty, port,NULL ); + + ret =usb_serial_generic_open (tty, port); +if (ret) { +dev_err (dev,"usb_serial_generic_open failed: %d\n" , ret);
No need to log again.
drivers/usb/serial/xr_serial.c line 362
Quoted 7 lines
drivers/usb/serial/xr_serial.c new file +7
-0+xr_uart_disable (port); +return ret; + } + + port_priv->port_open = true; + +return ret;
return 0 for clarity.
Quoted 24 lines
drivers/usb/serial/xr_serial.c new file +24
-0+} + +static void xr_close (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +usb_serial_generic_close (port); + port_priv->port_open = false; +xr_uart_disable (port); +} + +static void xr_set_flow_mode (struct tty_struct *tty, +struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +unsigned int cflag = tty->termios.c_cflag; +int ret; +u8 flow, gpio_mode; + + ret =xr_get_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->gpio_mode, &gpio_mode); +if (ret) +return ; +
drivers/usb/serial/xr_serial.c line 396
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+/* +* RTS/CTS is the default flow control mode, so set GPIO mode +* for controlling the pins manually by default. +*/ + gpio_mode &= ~UART_MODE_GPIO_MASK ; +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)) {
This should really be I_IXON() (flow control on output), sorry about the confusion.
drivers/usb/serial/xr_serial.c line 408
Quoted 12 lines
drivers/usb/serial/xr_serial.c new file +12
-0+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 ; + +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" );
s/No/Disabling/
Quoted 18 lines
drivers/usb/serial/xr_serial.c new file +18
-0+ flow =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 (port,XR21V141X_UART_REG_BLOCK , port_priv->regs->flow_ctrl, + flow); +xr_uart_enable (port); + +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,
drivers/usb/serial/xr_serial.c line 436
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ struct ktermios *old_termios) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +struct ktermios *termios = &tty->termios; +int ret; +u8 bits =0 ; + +if (old_termios && + tty->termios.c_ospeed != old_termios->c_ospeed) +xr_set_baudrate (tty, port);
You want to set the baudrate also when no old_termios.
drivers/usb/serial/xr_serial.c line 440
Quoted 4 lines
drivers/usb/serial/xr_serial.c new file +4
-0+ +switch (C_CSIZE (tty)) { +case CS5 : +/* fall through */
Odd indentation; use "fallthrough;".
drivers/usb/serial/xr_serial.c line 442
Quoted 2 lines
drivers/usb/serial/xr_serial.c new file +2
-0+case CS6 : +dev_warn (&port->dev,"requested CSIZE setting not supported\n" );
No need to warn.
drivers/usb/serial/xr_serial.c line 447
Quoted 5 lines
drivers/usb/serial/xr_serial.c new file +5
-0+ +/* 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 ;
You need to update bits as well, and set CS8 if no old_termios.
drivers/usb/serial/xr_serial.c line 453
Quoted 6 lines
drivers/usb/serial/xr_serial.c new file +6
-0+break ; +case CS7 : + bits |=UART_DATA_7 ; +break ; +case CS8 : +/* fall through */
As above.
Quoted 11 lines
drivers/usb/serial/xr_serial.c new file +11
-0+default : + bits |=UART_DATA_8 ; +break ; + } + +if (C_PARENB (tty)) { +if (C_CMSPAR (tty)) { +if (C_PARODD (tty)) { + bits |= (UART_PARITY_MARK << +UART_PARITY_SHIFT ); + }else {
drivers/usb/serial/xr_serial.c line 474
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ bits |= (UART_PARITY_SPACE << +UART_PARITY_SHIFT ); + } + }else { +if (C_PARODD (tty)) { + bits |= (UART_PARITY_ODD << +UART_PARITY_SHIFT ); + }else { + bits |= (UART_PARITY_EVEN << +UART_PARITY_SHIFT );
Just drop the parentheses and line break here and above. And then drop the (inner) brackets.
drivers/usb/serial/xr_serial.c line 482
Quoted 8 lines
drivers/usb/serial/xr_serial.c new file +8
-0+ } + } + } + +if (C_CSTOPB (tty)) + bits |= (UART_STOP_2 <<UART_STOP_SHIFT ); +else + bits |= (UART_STOP_1 <<UART_STOP_SHIFT );
Parentheses not needed.
Quoted 44 lines
drivers/usb/serial/xr_serial.c new file +44
-0+ + ret =xr_set_reg (port,XR21V141X_UART_REG_BLOCK , + port_priv->regs->format, bits); +if (ret) +return ; + +xr_set_flow_mode (tty, port); +} + +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) +{
drivers/usb/serial/xr_serial.c line 536
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+struct usb_serial_port *port = tty->driver_data; + +return xr_tiocmset_port (port, set, clear); +} + +static int xr_attach (struct usb_serial *serial) +{ +/* Do not register tty device for the control interface */ +if (serial->interface->cur_altsetting->desc.bInterfaceNumber ==0 ) +return 1 ;
Ok, so you went for my first suggestion here instead of explicitly claiming the sibling interface.
I still think you should bind to the data interface and then explicitly claim the control interface instead, since that better reflects that these interfaces are used together (and allows for unbinding through sysfs etc).
Quoted 104 lines
drivers/usb/serial/xr_serial.c new file +104
-0+ +return 0 ; +} + +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; +u8 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); +u8 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 xr_port_private *port_priv; + + port_priv =kzalloc (sizeof (*port_priv),GFP_KERNEL ); +if (!port_priv) +return -ENOMEM ; + +/* XR21V141X specific settings */ + port_priv->regs = &xr21v141x_regs; + +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 , + .open = xr_open, + .close = xr_close, + .break_ctl = xr_break_ctl, + .set_termios = xr_set_termios, + .tiocmget = xr_tiocmget, + .tiocmset = xr_tiocmset, + .attach = xr_attach, + .port_probe = xr_port_probe, + .port_remove = xr_port_remove, + .dtr_rts = xr_dtr_rts +};
drivers/usb/serial/xr_serial.c line 650
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ +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" );
Johan
Re: [RESEND PATCH v4 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver
Hi,
Sorry for the late reply!
Quoted 149 lines · Johan Hovold
Quoted 7 lines · Manivannan Sadhasivam
Add support for MaxLinear/Exar USB to Serial converters. This driver only supports XR21V141X series but it can be extended to other series from Exar as well in future.
This driver is inspired from the initial one submitted by Patong Yang:
You've also copied code from that driver so you need to maintain its copyright as well.
Probably better you link to lore than patchwork. Do that in the file header as well.
Quoted 52 lines
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.
Reviewed-by Greg Kroah-Hartman <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/xr_serial.c | 650 +++++++++++++++++++++++++++++++++ 3 files changed, 660 insertions(+) create mode 100644 drivers/usb/serial/xr_serial.cdrivers/usb/serial/Kconfig +9
-0@@ -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" helpdrivers/usb/serial/Makefile +1
-0@@ -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.odrivers/usb/serial/xr_serial.c new file +0
-0drivers/usb/serial/xr_serial.c line 5
Quoted 5 lines
drivers/usb/serial/xr_serial.c new file +5
-0@@ -0,0 +1,650 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* MaxLinear/Exar USB to Serial driver +* +* Based on initial driver written by Patong Yang <[email protected]> Reproduce Patong Yang's copyright here as well.
drivers/usb/serial/xr_serial.c line 8
Quoted 3 lines
drivers/usb/serial/xr_serial.c new file +3
-0+* +* This driver claims both data and control interfaces but only registers tty +* device for data interface. Not sure this is needed here; this is just the way CDC-ACM works.
drivers/usb/serial/xr_serial.c line 10
Quoted 2 lines
drivers/usb/serial/xr_serial.c new file +2
-0+* +* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]> Not your employer?
drivers/usb/serial/xr_serial.c line 22
Quoted 12 lines
drivers/usb/serial/xr_serial.c new file +12
-0+*/ + +#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> + +static void xr_set_termios (struct tty_struct *tty, +struct usb_serial_port *port, +struct ktermios *old_termios);Move
set_termios()beforeopen()so that you don't need this.Quoted 11 lines
drivers/usb/serial/xr_serial.c new file +11
-0+ +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;drivers/usb/serial/xr_serial.c line 43
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+u8 gpio_dir; +u8 gpio_int_mask; +u8 gpio_set; +u8 gpio_clr; +u8 gpio_status; +}; + +struct xr_port_privte { +const struct xr_uart_regs *regs; +bool port_open;No lock? As I pointed out before, you're gonna need one to coordinate with gpiolib. Better not add the flag until you use it.
drivers/usb/serial/xr_serial.c line 52
Quoted 9 lines
drivers/usb/serial/xr_serial.c new file +9
-0+}; + +struct xr_txrx_clk_mask { +u16 tx; +u16 rx0; +u16 rx1; +}; + +#define XR21V141X_ID 0x1410Not used.
drivers/usb/serial/xr_serial.c line 59
Quoted 7 lines
drivers/usb/serial/xr_serial.c new file +7
-0+#define XR_INT_OSC_HZ 48000000U +#define XR21V141X_MIN_SPEED 46U +#define XR21V141X_MAX_SPEED XR_INT_OSC_HZ + +/* USB Requests */ +#define XR_SET_XR21V141X 0 +#define XR_GET_XR21V141X 1Stick to one style; either use a device-type specific prefix or suffix for the defines.
Quoted 10 lines
drivers/usb/serial/xr_serial.c line 67
Quoted 8 lines
drivers/usb/serial/xr_serial.c new file +8
-0+ +#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 0xaPlease 0-pad these are they are registers.
You mean adding 0 after 0x?
Quoted 17 lines
Quoted 4 lines
drivers/usb/serial/xr_serial.c new file +3
-0+ +/* XR21V141X register blocks */ +#define XR21V141X_UART_REG_BLOCK 0drivers/usb/serial/xr_serial.c line 80
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+#define XR21V141X_UART_MANAGER_REG_BLOCK 4 +#define XR21V141X_UART_CUSTOM_BLOCK 0x66 + +/* XR21V141X UART Manager Registers */ +#define XR21V141X_UART_MANAGER_FIFO_ENABLE_REG 0x10 +#define XR21V141X_UART_MANAGER_ENABLE_TX_FIFO 0x1 +#define XR21V141X_UART_MANAGER_ENABLE_RX_FIFO 0x2 + +#define XR21V141X_UART_MANAGER_RX_FIFO_RESET 0x18 +#define XR21V141X_UART_MANAGER_TX_FIFO_RESET 0x1cs/UART_MANAGER/UM/ should do fine for the register definitions (it was just "URM" that was non-obvious).
[...]
Quoted 14 lines
drivers/usb/serial/xr_serial.c line 234
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ +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 still need to handle B0, which is a valid request and means that DTR/RTS should be deasserted.
Quoted 6 lines
drivers/usb/serial/xr_serial.c line 236
Quoted 2 lines
drivers/usb/serial/xr_serial.c new file +2
-0+ divisor =XR_INT_OSC_HZ / baud; + idx = (32 * divisor) &0x1f ;This looks broken as you're throwing away the remainder (cf. datasheet 3.3.3 and original implementation).
Ah yes. Will fix it.
[...]
Quoted 50 lines
Quoted 36 lines
drivers/usb/serial/xr_serial.c new file +36
-0+ +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) +{drivers/usb/serial/xr_serial.c line 536
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+struct usb_serial_port *port = tty->driver_data; + +return xr_tiocmset_port (port, set, clear); +} + +static int xr_attach (struct usb_serial *serial) +{ +/* Do not register tty device for the control interface */ +if (serial->interface->cur_altsetting->desc.bInterfaceNumber ==0 ) +return 1 ;Ok, so you went for my first suggestion here instead of explicitly claiming the sibling interface.
Quoted 5 lines
I still think you should bind to the data interface and then explicitly claim the control interface instead, since that better reflects that these interfaces are used together (and allows for unbinding through sysfs etc).
How about something like below?
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;
/* 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);
if (usb_driver_claim_interface(driver, control_interface, NULL) != 0)
dev_err(serial->interface->dev, "Can't claim control interface");
return 0;
}Quoted 116 lines
Quoted 104 lines
drivers/usb/serial/xr_serial.c new file +104
-0+ +return 0 ; +} + +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; +u8 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); +u8 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 xr_port_private *port_priv; + + port_priv =kzalloc (sizeof (*port_priv),GFP_KERNEL ); +if (!port_priv) +return -ENOMEM ; + +/* XR21V141X specific settings */ + port_priv->regs = &xr21v141x_regs; + +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 , + .open = xr_open, + .close = xr_close, + .break_ctl = xr_break_ctl, + .set_termios = xr_set_termios, + .tiocmget = xr_tiocmget, + .tiocmset = xr_tiocmset, + .attach = xr_attach, + .port_probe = xr_port_probe, + .port_remove = xr_port_remove, + .dtr_rts = xr_dtr_rts +};drivers/usb/serial/xr_serial.c line 650
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +10
-0+ +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" );Johan
Johan Hovold
maintainerRe: [RESEND PATCH v4 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver
Quoted 3 lines · Manivannan Sadhasivam
Hi,
Sorry for the late reply!
No worries at all.
Quoted 42 lines · Johan Hovold
Quoted 31 lines · Manivannan Sadhasivam
Quoted 7 lines
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:
You've also copied code from that driver so you need to maintain its copyright as well.
Probably better you link to lore than patchwork. Do that in the file header as well.
Quoted 15 lines
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.
Reviewed-by Greg Kroah-Hartman <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/xr_serial.c | 650 +++++++++++++++++++++++++++++++++ 3 files changed, 660 insertions(+) create mode 100644 drivers/usb/serial/xr_serial.cQuoted 9 lines
drivers/usb/serial/xr_serial.c line 67
Quoted 7 lines
drivers/usb/serial/xr_serial.c new file +7
-0+#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 0xaPlease 0-pad these are they are registers.
Quoted 1 line
You mean adding 0 after 0x?
Yes, exactly.
Quoted 17 lines
Quoted 9 lines
drivers/usb/serial/xr_serial.c line 536
Quoted 5 lines
drivers/usb/serial/xr_serial.c new file +5
-0+static int xr_attach (struct usb_serial *serial) +{ +/* Do not register tty device for the control interface */ +if (serial->interface->cur_altsetting->desc.bInterfaceNumber ==0 ) +return 1 ;Ok, so you went for my first suggestion here instead of explicitly claiming the sibling interface.
Quoted 4 lines
I still think you should bind to the data interface and then explicitly claim the control interface instead, since that better reflects that these interfaces are used together (and allows for unbinding through sysfs etc).
How about something like below?
static int
xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
Quoted 16 lines
{
struct usb_device *usb_dev = interface_to_usbdev(serial->interface);
struct usb_driver *driver = serial->type->usb_driver;
struct usb_interface *control_interface;
/* 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);
if (usb_driver_claim_interface(driver, control_interface, NULL) != 0)
dev_err(serial->interface->dev, "Can't claim control interface");
return 0;
}Yes, something like that, but with error handling and a '\n' added to the error message. ;)
Johan
Johan Hovold
maintainerRe: [RESEND PATCH v4 1/3] usb: serial: Add MaxLinear/Exar USB to Serial driver
In reply to Manivannan SadhasivamQuoted 61 lines · Manivannan Sadhasivam
Add support for MaxLinear/Exar USB to Serial converters. This driver only supports XR21V141X series but it can be extended to other series from Exar as well in future.
This driver is inspired from the initial one submitted by Patong Yang:
https://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.
Reviewed-by Greg Kroah-Hartman <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/xr_serial.c | 650 +++++++++++++++++++++++++++++++++ 3 files changed, 660 insertions(+) create mode 100644 drivers/usb/serial/xr_serial.cdrivers/usb/serial/xr_serial.c new file +37
-0+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; +
drivers/usb/serial/xr_serial.c line 547
Quoted 10 lines
drivers/usb/serial/xr_serial.c new file +9
-0+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 ); +}
By the way, don't you need to configure the DTR and RTS pins as outputs for the serial driver as well?
According to the datasheet they are both configured as inputs by default (or at least RTS is).
In any case, you'll need to reconfigure each pin after they are released by gpiolib.
Johan
PATCH RESEND v4 2/3 usb: serial: xr_serial: Add gpiochip support
In reply to Manivannan SadhasivamAdd gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Inspired from cp210x usb to serial converter driver.
--- drivers/usb/serial/xr_serial.c | 209 ++++++++++++++++++++++++++++++++- 1 file changed, 208 insertions(+), 1 deletion(-)
drivers/usb/serial/xr_serial.c +208 -1
@@ -10,6 +10,7 @@* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]> */ +#include <linux/gpio/driver.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/slab.h>
@@ -17,6 +18,18 @@#include <linux/usb.h> #include <linux/usb/serial.h> +#ifdef CONFIG_GPIOLIB +enum gpio_pins { +GPIO_RI =0 , +GPIO_CD , +GPIO_DSR , +GPIO_DTR , +GPIO_CTS , +GPIO_RTS , +GPIO_MAX , +}; +#endif +static void xr_set_termios (struct tty_struct *tty,struct usb_serial_port *port,struct ktermios *old_termios);
@@ -39,6 +52,11 @@ struct xr_uart_regs { };struct xr_port_private { +#ifdef CONFIG_GPIOLIB +struct gpio_chip gc; +bool gpio_registered; +enum gpio_pins pin_status[GPIO_MAX ]; +#endif const struct xr_uart_regs *regs;bool port_open; };
@@ -390,6 +408,13 @@ static void xr_set_flow_mode(struct tty_struct *tty,*/ gpio_mode &= ~UART_MODE_GPIO_MASK ;if (cflag &CRTSCTS ) { +#ifdef CONFIG_GPIOLIB +/* Check if the CTS/RTS pins are occupied */ +if (port_priv->pin_status[GPIO_RTS ] || + port_priv->pin_status[GPIO_CTS ]) +return ; +#endif +dev_dbg (&port->dev,"Enabling hardware flow ctrl\n" ); flow =UART_FLOW_MODE_HW ; gpio_mode |=UART_MODE_RTS_CTS ;
@@ -497,6 +522,17 @@ static int xr_tiocmset_port(struct usb_serial_port *port,u8 gpio_set =0 ;u8 gpio_clr =0 ; +#ifdef CONFIG_GPIOLIB +/* Check if the RTS/DTR pins are occupied */ +if (set &TIOCM_RTS || clear &TIOCM_RTS ) +if (port_priv->pin_status[GPIO_RTS ]) +return -EBUSY ; + +if (set &TIOCM_DTR || clear &TIOCM_DTR ) +if (port_priv->pin_status[GPIO_DTR ]) +return -EBUSY ; +#endif +/* Modem control pins are active low, so set & clr are swapped */ if (set &TIOCM_RTS ) gpio_clr |=UART_MODE_RTS ;
@@ -589,9 +625,175 @@ 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); + +/* +* Do not proceed if the port is open. This is done to avoid changing +* the GPIO configuration used by the serial driver. +*/ +if (port_priv->port_open) +return -EBUSY ; + +/* Mark the GPIO pin as busy */ + port_priv->pin_status[offset] = true; + +return 0 ; +} + +static void xr_gpio_free (struct gpio_chip *gc,unsigned int offset) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +/* Mark the GPIO pin as free */ + port_priv->pin_status[offset] = false; +} + +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; +u8 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; +u8 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; +u8 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; +u8 gpio_dir; + +xr_gpio_set (gc, gpio, val); + + 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; + +return 0 ; +} + +static int xr_gpio_init (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); +int ret =0 ; + + port_priv->gc.ngpio =6 ; + port_priv->gc.label ="xr_gpios" ; + port_priv->gc.request = xr_gpio_request; + port_priv->gc.free = xr_gpio_free; + port_priv->gc.get_direction = xr_gpio_direction_get; + port_priv->gc.direction_input = xr_gpio_direction_input; + port_priv->gc.direction_output = xr_gpio_direction_output; + port_priv->gc.get = xr_gpio_get; + port_priv->gc.set = xr_gpio_set; + port_priv->gc.owner =THIS_MODULE ; + port_priv->gc.parent = &port->dev; + port_priv->gc.base =-1 ; + port_priv->gc.can_sleep = true; + + ret =gpiochip_add_data (&port_priv->gc, port); +if (!ret) + port_priv->gpio_registered = true; + +return ret; +} + +static void xr_gpio_remove (struct usb_serial_port *port) +{ +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +if (port_priv->gpio_registered) { +gpiochip_remove (&port_priv->gc); + port_priv->gpio_registered = false; + } +} + +#else + +static int xr_gpio_init (struct usb_serial_port *port) +{ +return 0 ; +} + +static void xr_gpio_remove (struct usb_serial_port *port) +{ +} + +#endif +static int xr_port_probe (struct usb_serial_port *port) {struct xr_port_private *port_priv; +int ret; port_priv =kzalloc (sizeof (*port_priv),GFP_KERNEL );if (!port_priv)
@@ -602,13 +804,18 @@ static int xr_port_probe(struct usb_serial_port *port)usb_set_serial_port_data (port, port_priv); -return 0 ; + ret =xr_gpio_init (port); +if (ret) +kfree (port_priv); + +return ret; }static int xr_port_remove (struct usb_serial_port *port) {struct xr_port_private *port_priv =usb_get_serial_port_data (port); +xr_gpio_remove (port);kfree (port_priv);return 0 ;
Signature · 1 line
2.17.1
Johan Hovold
maintainerRe: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
Quoted 30 lines · Manivannan Sadhasivam
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Inspired from cp210x usb to serial converter driver.
Cc Linus Walleij <[email protected]>Reviewed-by Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/xr_serial.c | 209 ++++++++++++++++++++++++++++++++- 1 file changed, 208 insertions(+), 1 deletion(-)drivers/usb/serial/xr_serial.c +2
-0@@ -10,6 +10,7 @@* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]> */ +#include <linux/gpio/driver.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/slab.h> @@ -17,6 +18,18 @@#include <linux/usb.h> #include <linux/usb/serial.h> +#ifdef CONFIG_GPIOLIB
drivers/usb/serial/xr_serial.c line 31
Quoted 10 lines
drivers/usb/serial/xr_serial.c +10
-0+enum gpio_pins { +GPIO_RI =0 , +GPIO_CD , +GPIO_DSR , +GPIO_DTR , +GPIO_CTS , +GPIO_RTS , +GPIO_MAX , +}; +#endif
Try to avoid littering the driver with GPIOLIB ifdefs. One or two is fine, but no more even if it means declaring an unused type. Add stubbed out helpers where appropriate.
Quoted 17 lines
drivers/usb/serial/xr_serial.c +6
-0+static void xr_set_termios (struct tty_struct *tty,struct usb_serial_port *port,struct ktermios *old_termios);@@ -39,6 +52,11 @@ struct xr_uart_regs { };struct xr_port_private { +#ifdef CONFIG_GPIOLIB +struct gpio_chip gc; +bool gpio_registered; +enum gpio_pins pin_status[GPIO_MAX ]; +#endif const struct xr_uart_regs *regs;bool port_open; };
drivers/usb/serial/xr_serial.c static void xr_set_flow_mode(struct tty_struct *tty, · line 416
Quoted 9 lines
drivers/usb/serial/xr_serial.c +6
-0@@ -390,6 +408,13 @@ static void xr_set_flow_mode(struct tty_struct *tty,*/ gpio_mode &= ~UART_MODE_GPIO_MASK ;if (cflag &CRTSCTS ) { +#ifdef CONFIG_GPIOLIB +/* Check if the CTS/RTS pins are occupied */ +if (port_priv->pin_status[GPIO_RTS ] || + port_priv->pin_status[GPIO_CTS ]) +return ; +#endif
You cannot just bail out as this could leave software flow enabled etc.
You also need to claim these pins once at open or leave them be. We don't want CRTSCTS to suddenly start toggling because a pin is released by gpiolib.
That is, determine who owns each pin at open() and keep it that way till close() (by setting some flags at open).
Quoted 8 lines
drivers/usb/serial/xr_serial.c +1
-0+dev_dbg (&port->dev,"Enabling hardware flow ctrl\n" ); flow =UART_FLOW_MODE_HW ; gpio_mode |=UART_MODE_RTS_CTS ;@@ -497,6 +522,17 @@ static int xr_tiocmset_port(struct usb_serial_port *port,u8 gpio_set =0 ;u8 gpio_clr =0 ;
drivers/usb/serial/xr_serial.c static int xr_tiocmset_port(struct usb_serial_port *port, · line 534
Quoted 10 lines
drivers/usb/serial/xr_serial.c +10
-0+#ifdef CONFIG_GPIOLIB +/* Check if the RTS/DTR pins are occupied */ +if (set &TIOCM_RTS || clear &TIOCM_RTS ) +if (port_priv->pin_status[GPIO_RTS ]) +return -EBUSY ; + +if (set &TIOCM_DTR || clear &TIOCM_DTR ) +if (port_priv->pin_status[GPIO_DTR ]) +return -EBUSY ; +#endif
Same here. And perhaps just ignoring the pins managed by gpiolib is better (cf. gpiolib and pinctrl being orthogonal).
Quoted 14 lines
drivers/usb/serial/xr_serial.c +7
-0+/* Modem control pins are active low, so set & clr are swapped */ if (set &TIOCM_RTS ) gpio_clr |=UART_MODE_RTS ;@@ -589,9 +625,175 @@ 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);
drivers/usb/serial/xr_serial.c static void xr_break_ctl(struct tty_struct *tty, int break_state) · line 643
Quoted 10 lines
drivers/usb/serial/xr_serial.c +10
-0+ +/* +* Do not proceed if the port is open. This is done to avoid changing +* the GPIO configuration used by the serial driver. +*/ +if (port_priv->port_open) +return -EBUSY ; + +/* Mark the GPIO pin as busy */ + port_priv->pin_status[offset] = true;
You need a lock to serialise against open/close properly.
drivers/usb/serial/xr_serial.c static void xr_break_ctl(struct tty_struct *tty, int break_state) · line 655
Quoted 12 lines
drivers/usb/serial/xr_serial.c +12
-0+ +return 0 ; +} + +static void xr_gpio_free (struct gpio_chip *gc,unsigned int offset) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +/* Mark the GPIO pin as free */ + port_priv->pin_status[offset] = false; +}
Johan
Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
Quoted 90 lines · Johan Hovold
Quoted 30 lines · Manivannan Sadhasivam
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Inspired from cp210x usb to serial converter driver.
Cc Linus Walleij <[email protected]>Reviewed-by Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/xr_serial.c | 209 ++++++++++++++++++++++++++++++++- 1 file changed, 208 insertions(+), 1 deletion(-)drivers/usb/serial/xr_serial.c +2
-0@@ -10,6 +10,7 @@* Copyright (c) 2020 Manivannan Sadhasivam <[email protected]> */ +#include <linux/gpio/driver.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/slab.h> @@ -17,6 +18,18 @@#include <linux/usb.h> #include <linux/usb/serial.h> +#ifdef CONFIG_GPIOLIB drivers/usb/serial/xr_serial.c line 31
Quoted 10 lines
drivers/usb/serial/xr_serial.c +10
-0+enum gpio_pins { +GPIO_RI =0 , +GPIO_CD , +GPIO_DSR , +GPIO_DTR , +GPIO_CTS , +GPIO_RTS , +GPIO_MAX , +}; +#endif Try to avoid littering the driver with GPIOLIB ifdefs. One or two is fine, but no more even if it means declaring an unused type. Add stubbed out helpers where appropriate.
Quoted 17 lines
drivers/usb/serial/xr_serial.c +6
-0+static void xr_set_termios (struct tty_struct *tty,struct usb_serial_port *port,struct ktermios *old_termios);@@ -39,6 +52,11 @@ struct xr_uart_regs { };struct xr_port_private { +#ifdef CONFIG_GPIOLIB +struct gpio_chip gc; +bool gpio_registered; +enum gpio_pins pin_status[GPIO_MAX ]; +#endif const struct xr_uart_regs *regs;bool port_open; };drivers/usb/serial/xr_serial.c static void xr_set_flow_mode(struct tty_struct *tty, · line 416
Quoted 9 lines
drivers/usb/serial/xr_serial.c +6
-0@@ -390,6 +408,13 @@ static void xr_set_flow_mode(struct tty_struct *tty,*/ gpio_mode &= ~UART_MODE_GPIO_MASK ;if (cflag &CRTSCTS ) { +#ifdef CONFIG_GPIOLIB +/* Check if the CTS/RTS pins are occupied */ +if (port_priv->pin_status[GPIO_RTS ] || + port_priv->pin_status[GPIO_CTS ]) +return ; +#endif You cannot just bail out as this could leave software flow enabled etc.
You also need to claim these pins once at open or leave them be. We don't want CRTSCTS to suddenly start toggling because a pin is released by gpiolib.
That is, determine who owns each pin at
open()and keep it that way tillclose()(by setting some flags at open).Quoted 8 lines
drivers/usb/serial/xr_serial.c +1
-0+dev_dbg (&port->dev,"Enabling hardware flow ctrl\n" ); flow =UART_FLOW_MODE_HW ; gpio_mode |=UART_MODE_RTS_CTS ;@@ -497,6 +522,17 @@ static int xr_tiocmset_port(struct usb_serial_port *port,u8 gpio_set =0 ;u8 gpio_clr =0 ;
Quoted 13 lines
drivers/usb/serial/xr_serial.c static int xr_tiocmset_port(struct usb_serial_port *port, · line 534
Quoted 10 lines
drivers/usb/serial/xr_serial.c +10
-0+#ifdef CONFIG_GPIOLIB +/* Check if the RTS/DTR pins are occupied */ +if (set &TIOCM_RTS || clear &TIOCM_RTS ) +if (port_priv->pin_status[GPIO_RTS ]) +return -EBUSY ; + +if (set &TIOCM_DTR || clear &TIOCM_DTR ) +if (port_priv->pin_status[GPIO_DTR ]) +return -EBUSY ; +#endif Same here. And perhaps just ignoring the pins managed by gpiolib is better (cf. gpiolib and pinctrl being orthogonal).
You mean, we can just make TX,RX,CTS,RTS pins controlled only by the serial driver and the rest only by gpiolib?
Thanks,
Mani
Quoted 42 lines
Quoted 15 lines
drivers/usb/serial/xr_serial.c +7
-0+/* Modem control pins are active low, so set & clr are swapped */ if (set &TIOCM_RTS ) gpio_clr |=UART_MODE_RTS ;@@ -589,9 +625,175 @@ 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);drivers/usb/serial/xr_serial.c static void xr_break_ctl(struct tty_struct *tty, int break_state) · line 643
Quoted 10 lines
drivers/usb/serial/xr_serial.c +10
-0+ +/* +* Do not proceed if the port is open. This is done to avoid changing +* the GPIO configuration used by the serial driver. +*/ +if (port_priv->port_open) +return -EBUSY ; + +/* Mark the GPIO pin as busy */ + port_priv->pin_status[offset] = true;You need a lock to serialise against open/close properly.
drivers/usb/serial/xr_serial.c static void xr_break_ctl(struct tty_struct *tty, int break_state) · line 655
Quoted 12 lines
drivers/usb/serial/xr_serial.c +12
-0+ +return 0 ; +} + +static void xr_gpio_free (struct gpio_chip *gc,unsigned int offset) +{ +struct usb_serial_port *port =gpiochip_get_data (gc); +struct xr_port_private *port_priv =usb_get_serial_port_data (port); + +/* Mark the GPIO pin as free */ + port_priv->pin_status[offset] = false; +}Johan
Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
Quoted 2 lines · Manivannan Sadhasivam
Quoted 1 line · Johan Hovold
On Sun, Jun 07, 2020 at 09:53:49PM +0530, Manivannan Sadhasivam wrote:
...
Quoted 5 lines
Quoted 2 lines
Same here. And perhaps just ignoring the pins managed by gpiolib is better (cf. gpiolib and pinctrl being orthogonal).
You mean, we can just make TX,RX,CTS,RTS pins controlled only by the serial driver and the rest only by gpiolib?
I'm wondering if you may use mctrl_gpio_*() API instead.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
Quoted 6 lines · Andy Shevchenko
Quoted 2 lines · Manivannan Sadhasivam
Quoted 1 line · Johan Hovold
On Sun, Jun 07, 2020 at 09:53:49PM +0530, Manivannan Sadhasivam wrote:
...
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Same here. And perhaps just ignoring the pins managed by gpiolib is better (cf. gpiolib and pinctrl being orthogonal).
You mean, we can just make TX,RX,CTS,RTS pins controlled only by the serial driver and the rest only by gpiolib?
I'm wondering if you may use mctrl_gpio_*() API instead.
How? mctrl_gpio APIs are a wrapper for accessing modem control gpio pins but here we are not accessing the pins but rather exposing the pins as a gpiochip. Am I missing something?
Thanks,
Mani
Quoted 4 lines
Signature · 3 lines
-- With Best Regards, Andy Shevchenko
Re: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
Quoted 2 lines · Manivannan Sadhasivam
Quoted 1 line · Andy Shevchenko
On Sun, Jul 26, 2020 at 6:53 PM Manivannan Sadhasivam <[email protected]> wrote:
...
Quoted 4 lines
Quoted 1 line
I'm wondering if you may use mctrl_gpio_*() API instead.
How?
mctrl_gpioAPIs are a wrapper for accessing modem control gpio pins but here we are not accessing the pins but rather exposing the pins as a gpiochip.
I see. Thanks for the explanation.
Quoted 1 line
Am I missing something?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Johan Hovold
maintainerRe: [RESEND PATCH v4 2/3] usb: serial: xr_serial: Add gpiochip support
In reply to Manivannan SadhasivamQuoted 57 lines · Manivannan Sadhasivam
Quoted 42 lines · Johan Hovold
Quoted 14 lines · Manivannan Sadhasivam
Add gpiochip support for Maxlinear/Exar USB to serial converter for controlling the available gpios.
Inspired from cp210x usb to serial converter driver.
Cc Linus Walleij <[email protected]>Reviewed-by Linus Walleij <[email protected]>Signed-off-by Manivannan Sadhasivam <[email protected]>--- drivers/usb/serial/xr_serial.c | 209 ++++++++++++++++++++++++++++++++- 1 file changed, 208 insertions(+), 1 deletion(-)drivers/usb/serial/xr_serial.c static void xr_set_flow_mode(struct tty_struct *tty, · line 416
Quoted 9 lines
drivers/usb/serial/xr_serial.c +6
-0@@ -390,6 +408,13 @@ static void xr_set_flow_mode(struct tty_struct *tty,*/ gpio_mode &= ~UART_MODE_GPIO_MASK ;if (cflag &CRTSCTS ) { +#ifdef CONFIG_GPIOLIB +/* Check if the CTS/RTS pins are occupied */ +if (port_priv->pin_status[GPIO_RTS ] || + port_priv->pin_status[GPIO_CTS ]) +return ; +#endif You cannot just bail out as this could leave software flow enabled etc.
You also need to claim these pins once at open or leave them be. We don't want CRTSCTS to suddenly start toggling because a pin is released by gpiolib.
That is, determine who owns each pin at
open()and keep it that way tillclose()(by setting some flags at open).Quoted 8 lines
drivers/usb/serial/xr_serial.c +1
-0+dev_dbg (&port->dev,"Enabling hardware flow ctrl\n" ); flow =UART_FLOW_MODE_HW ; gpio_mode |=UART_MODE_RTS_CTS ;@@ -497,6 +522,17 @@ static int xr_tiocmset_port(struct usb_serial_port *port,u8 gpio_set =0 ;u8 gpio_clr =0 ;Quoted 13 lines
drivers/usb/serial/xr_serial.c static int xr_tiocmset_port(struct usb_serial_port *port, · line 534
Quoted 10 lines
drivers/usb/serial/xr_serial.c +10
-0+#ifdef CONFIG_GPIOLIB +/* Check if the RTS/DTR pins are occupied */ +if (set &TIOCM_RTS || clear &TIOCM_RTS ) +if (port_priv->pin_status[GPIO_RTS ]) +return -EBUSY ; + +if (set &TIOCM_DTR || clear &TIOCM_DTR ) +if (port_priv->pin_status[GPIO_DTR ]) +return -EBUSY ; +#endif Same here. And perhaps just ignoring the pins managed by gpiolib is better (cf. gpiolib and pinctrl being orthogonal).
Quoted 2 lines
You mean, we can just make TX,RX,CTS,RTS pins controlled only by the serial driver and the rest only by gpiolib?
No, I think I just meant that you shouldn't return an error here for signals whose pins happen to be muxed for a different function (gpio).
Johan
PATCH RESEND v4 3/3 usb: cdc-acm: Ignore Exar XR21V141X when serial driver is built
In reply to Manivannan SadhasivamFrom: Mauro Carvalho Chehab <[email protected]>
The RTS/CTS line discipline for this device doesn't follow the standard. So, in order to properly support TX, a separate driver is needed.
Ensure that cdc_acm will ignore it during probe time, if the Kernel is built with support for it.
--- drivers/usb/class/cdc-acm.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/usb/class/cdc-acm.c +6 -0
@@ -1864,6 +1864,12 @@ static const struct usb_device_id acm_ids[] = { },#endif +#if IS_ENABLED (CONFIG_USB_SERIAL_XR ) + { USB_DEVICE(0x04e2, 0x1410), /* Ignore XR21V141X USB to Serial converter */ + .driver_info = IGNORE_DEVICE, + }, +#endif +/*Samsung phone in firmware update mode */ { USB_DEVICE(0x04e8, 0x685d), .driver_info = IGNORE_DEVICE,
Signature · 1 line
2.17.1