Hi, Sorry for the late reply! On Wed, Jul 01, 2020 at 12:34:33PM +0200, Johan Hovold wrote: > On Sun, Jun 07, 2020 at 09:53:48PM +0530, Manivannan Sadhasivam wrote: > > Add support for MaxLinear/Exar USB to Serial converters. This driver > > only supports XR21V141X series but it can be extended to other series > > from Exar as well in future. > > > > This driver is inspired from the initial one submitted by Patong Yang: > > > > https://patchwork.kernel.org/patch/10543261/ > > 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. > > > 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 <gregkh@xxxxxxxxxxxxxxxxxxx> > > Tested-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > Signed-off-by: Manivannan Sadhasivam <mani@xxxxxxxxxx> > > --- > > 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 > > > > 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..bb7df79cc129 > > --- /dev/null > > +++ b/drivers/usb/serial/xr_serial.c > > @@ -0,0 +1,650 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * MaxLinear/Exar USB to Serial driver > > + * > > + * Based on initial driver written by Patong Yang <patong.mxl@xxxxxxxxx> > > Reproduce Patong Yang's copyright here as well. > > > + * > > + * 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. > > > + * > > + * Copyright (c) 2020 Manivannan Sadhasivam <mani@xxxxxxxxxx> > > Not your employer? > > > + */ > > + > > +#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. > > > + > > +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_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. > > > +}; > > + > > +struct xr_txrx_clk_mask { > > + u16 tx; > > + u16 rx0; > > + u16 rx1; > > +}; > > + > > +#define XR21V141X_ID 0x1410 > > Not used. > > > +#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. > > > + > > +#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. You mean adding 0 after 0x? > > > + > > +/* 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). [...] > > + > > +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. > > > + 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. [...] > > + > > +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 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). > 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; } > > + > > + 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 > > +}; > > + > > +static struct usb_serial_driver * const serial_drivers[] = { > > + &xr_device, NULL > > +}; > > + > > +module_usb_serial_driver(serial_drivers, id_table); > > + > > +MODULE_AUTHOR("Manivannan Sadhasivam <mani@xxxxxxxxxx>"); > > +MODULE_DESCRIPTION("MaxLinear/Exar USB to Serial driver"); > > +MODULE_LICENSE("GPL"); > > Johan
