On Tue, Mar 03, 2015 at 11:57:04AM -0600, George McCollister wrote: > This driver is for the NovaTech 124 4x serial expansion board for the > NovaTech OrionLXm. > > Firmware source code can be found here: > https://github.com/novatechweb/nt124 > > Signed-off-by: George McCollister <george.mccollis...@gmail.com> > --- > Changes to v1: > - Added description after nt124.c on line 2. > - Removed DRIVER_AUTHOR and DRIVER_DESC, use MODULE macros directly. > - Removed some unnecessary new lines and comments. > - Removed __packed from struct nt124_line_coding. > - Added locking around ctrlin and ctrlout. > - Switch ctrlin and ctrlout from unsigned int to u16. > - Removed serial_transmit and added tx_empty. Use a hybrid > notification/polling method to accurately determine when transmission is > finished while minimizing bus traffic (see comments in the code for > details). > - Removed flowctrl from struct nt124_line_coding. > - Use u16 for request and value, size_t for len arguments of > nt124_ctrl_msg() > - Use USB_CTRL_SET_TIMEOUT instead of 5000. > - Use %04x for 16-bit variables and %zu for size_t variables in dev_dbg() > and > dev_err(). > - Removed use of ?: constructs. > - Removed nt124_set_control, nt124_set_line, nt124_send_break and > - nt124_set_flowctrl macros in favor of calling nt124_ctrl_msg() directly. > - Renamed nt124_process_notify() to nt124_process_status(). > - Call usb_serial_handle_dcd_change() unconditionally when DCD has changed. > - Removed in argument list assignments. > - Use usb_translate_errors() in nt124_port_tiocmset(). > - Use C_CSTOPB, C_CSIZE, C_PARENB, C_CMSPAR, C_PARODD, C_CRTSCTS macros. > - Raise/lower RTS on !B0/B0. > - Added NT124_BREAK_ON and NT124_BREAK_OFF #defines. > - Change nt124_open() to just call nt124_set_termios() followed by > usb_serial_generic_open(). > - Don't set bulk_in_size and bulk_out_size. > - Performed thorough testing.
Thanks for the update. Looks really good, but I have few comments below. > drivers/usb/serial/Kconfig | 9 + > drivers/usb/serial/Makefile | 1 + > drivers/usb/serial/nt124.c | 501 > ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 511 insertions(+) > create mode 100644 drivers/usb/serial/nt124.c > > diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig > index b7cf198..677a26a 100644 > --- a/drivers/usb/serial/Kconfig > +++ b/drivers/usb/serial/Kconfig > @@ -510,6 +510,15 @@ config USB_SERIAL_NAVMAN > To compile this driver as a module, choose M here: the > module will be called navman. > > +config USB_SERIAL_NT124 > + tristate "USB NovaTech 124 Serial Driver" > + help > + Say Y here if you want to use the NovaTech 124 4x USB to serial > + board. > + > + To compile this driver as a module, choose M here: the > + module will be called nt124. > + > config USB_SERIAL_PL2303 > tristate "USB Prolific 2303 Single Port Serial Driver" > help > diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile > index 349d9df..f88eaab 100644 > --- a/drivers/usb/serial/Makefile > +++ b/drivers/usb/serial/Makefile > @@ -39,6 +39,7 @@ obj-$(CONFIG_USB_SERIAL_MOS7720) += mos7720.o > obj-$(CONFIG_USB_SERIAL_MOS7840) += mos7840.o > obj-$(CONFIG_USB_SERIAL_MXUPORT) += mxuport.o > obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o > +obj-$(CONFIG_USB_SERIAL_NT124) += nt124.o > obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o > obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o > obj-$(CONFIG_USB_SERIAL_OPTION) += option.o > diff --git a/drivers/usb/serial/nt124.c b/drivers/usb/serial/nt124.c > new file mode 100644 > index 0000000..d837593 > --- /dev/null > +++ b/drivers/usb/serial/nt124.c > @@ -0,0 +1,501 @@ > +/* > + * nt124.c - Driver for nt124 4x serial board based on STM32F103 > + * > + * Copyright (c) 2014 - 2015 NovaTech LLC > + * > + * Portions derived from the cdc-acm driver > + * > + * The original intention was to implement a cdc-acm compliant > + * 4x USB to serial converter in the STM32F103 however several problems > arose. > + * The STM32F103 didn't have enough end points to implement 4 ports. > + * CTS control was required by the application. > + * Accurate notification of transmission completion was required. > + * RTSCTS flow control support was required. > + * > + * The interrupt endpoint was eliminated and the control line information > + * was moved to the first two bytes of the bulk in endpoint message. CTS > + * control and mechanisms to enable RTSCTS flow control and deliver TXEMPTY > + * information were added. > + * > + * Firmware source code can be found here: > + * https://github.com/novatechweb/nt124 > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/tty.h> > +#include <linux/tty_driver.h> > +#include <linux/tty_flip.h> > +#include <linux/module.h> > +#include <linux/usb.h> > +#include <linux/usb/serial.h> > +#include <asm/unaligned.h> > + > +#define NT124_VID 0x2aeb > +#define NT124_USB_PID 124 I see why you use decimal here, but please use hex also for the product id, which is the format we ultimately expose to userspace. > + > +static const struct usb_device_id id_table[] = { > + { USB_DEVICE(NT124_VID, NT124_USB_PID) }, > + { }, > +}; > + > +MODULE_DEVICE_TABLE(usb, id_table); > + > +/* > + * Output control lines. > + */ > +#define NT124_CTRL_DTR 0x01 > +#define NT124_CTRL_RTS 0x02 > + > +/* > + * Input control lines and line errors. > + */ > +#define NT124_CTRL_DCD 0x01 > +#define NT124_CTRL_DSR 0x02 > +#define NT124_CTRL_BRK 0x04 > +#define NT124_CTRL_RI 0x08 > +#define NT124_CTRL_FRAMING 0x10 > +#define NT124_CTRL_PARITY 0x20 > +#define NT124_CTRL_OVERRUN 0x40 > +#define NT124_CTRL_TXEMPTY 0x80 > +#define NT124_CTRL_CTS 0x100 > + > +#define USB_NT124_REQ_SET_LINE_CODING 0x20 > +#define USB_NT124_REQ_SET_CONTROL_LINE_STATE 0x22 > +#define USB_NT124_REQ_SEND_BREAK 0x23 > +#define USB_NT124_REQ_SET_FLOW_CONTROL 0x90 > +#define USB_NT124_REQ_GET_TXEMPTY 0x91 > + > +#define NT124_BREAK_OFF 0x0 > +#define NT124_BREAK_ON 0xffff > + > +#define NT124_STOP_BITS_1 0 > +#define NT124_STOP_BITS_2 2 > + > +#define NT124_PARITY_NONE 0 > +#define NT124_PARITY_ODD 1 > +#define NT124_PARITY_EVEN 2 > + > +#define NT124_RTSCTS_FLOW_CONTROL_OFF 0 > +#define NT124_RTSCTS_FLOW_CONTROL_ON 1 > + > +struct nt124_line_coding { > + __le32 dwDTERate; > + u8 bCharFormat; > + u8 bParityType; > + u8 bDataBits; > +}; > + > +struct nt124_private { > + u16 bInterfaceNumber; > + /* input control lines (DCD, DSR, RI, break, overruns) */ > + u16 ctrlin; > + /* output control lines (DTR, RTS) */ > + u16 ctrlout; > + spinlock_t ctrl_lock; > + struct nt124_line_coding line; > + bool tx_empty; > +}; > + > +static int nt124_ctrl_msg(struct usb_serial_port *port, u16 request, u16 > value, > + void *buf, size_t len) > +{ > + struct nt124_private *priv = usb_get_serial_port_data(port); > + int retval; > + > + retval = usb_control_msg(port->serial->dev, > + usb_sndctrlpipe(port->serial->dev, 0), > + request, USB_DIR_OUT | USB_TYPE_VENDOR, value, > + priv->bInterfaceNumber, > + buf, len, USB_CTRL_SET_TIMEOUT); > + > + dev_dbg(&port->dev, > + "%s - rq 0x%04x, val 0x%04x, len %zu, result %d\n", > + __func__, request, value, len, retval); > + > + if (retval < 0) { > + dev_err(&port->dev, > + "%s - usb_control_msg failed (%d)\n", Merge with previous line? > + __func__, retval); > + return retval; > + } > + > + if (retval != len) { > + dev_err(&port->dev, > + "%s - short write (%d / %zu)\n", Same here (and some places below). > + __func__, retval, len); > + return -EIO; > + } > + > + return 0; > +} > + > +static void nt124_process_status(struct usb_serial_port *port, > + struct nt124_private *priv, > + unsigned char *data) > +{ > + u16 ctrlin; > + u16 newctrl; > + unsigned long flags; > + struct tty_struct *tty; > + > + newctrl = get_unaligned_le16(data); You can just use le16_to_cpu here as this is the beginning of the transfer buffer, which will be properly aligned. I'd also retrieve the status in process_read_urb below just after checking that length >=2 , and then pass the converted status to this function instead of the buffer. > + > + spin_lock_irqsave(&priv->ctrl_lock, flags); > + ctrlin = priv->ctrlin; > + priv->ctrlin = newctrl; > + if (newctrl & NT124_CTRL_TXEMPTY) > + priv->tx_empty = true; > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); > + > + if (ctrlin & ~newctrl & NT124_CTRL_DCD) { Please use XOR here. > + tty = tty_port_tty_get(&port->port); > + if (tty) > + usb_serial_handle_dcd_change(port, tty, > + newctrl & NT124_CTRL_DCD); Use brackets for the conditional block. Missing tty_kref_put(tty). > + } > +} > + > +static void nt124_process_read_urb(struct urb *urb) > +{ > + struct usb_serial_port *port = urb->context; > + struct nt124_private *priv = usb_get_serial_port_data(port); > + unsigned char *data = (unsigned char *)urb->transfer_buffer; No cast needed. > + size_t datalen; > + > + /* The packet should always be at least 2 bytes because status is > + * included. If it's too short, discard it. */ Multi-line comments should be on the following format: /* * ... */ > + if (urb->actual_length < 2) > + return; > + > + nt124_process_status(port, priv, data); > + > + datalen = urb->actual_length - 2; > + No newline. > + if (!datalen) > + return; > + > + tty_insert_flip_string(&port->port, &data[2], datalen); > + tty_flip_buffer_push(&port->port); > +} > + > +static int nt124_tiocmget(struct tty_struct *tty) > +{ > + struct usb_serial_port *port = tty->driver_data; > + struct nt124_private *priv = usb_get_serial_port_data(port); > + int result = 0; > + unsigned long flags; > + u16 ctrlout; > + u16 ctrlin; > + > + spin_lock_irqsave(&priv->ctrl_lock, flags); > + ctrlout = priv->ctrlout; > + ctrlin = priv->ctrlin; > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); > + > + if (ctrlout & NT124_CTRL_DTR) > + result |= TIOCM_DTR; > + > + if (ctrlout & NT124_CTRL_RTS) > + result |= TIOCM_RTS; > + > + if (ctrlin & NT124_CTRL_DSR) > + result |= TIOCM_DSR; > + > + if (ctrlin & NT124_CTRL_RI) > + result |= TIOCM_RI; > + > + if (ctrlin & NT124_CTRL_DCD) > + result |= TIOCM_CD; > + > + if (ctrlin & NT124_CTRL_CTS) > + result |= TIOCM_CTS; > + > + return result; > +} > + > +static int nt124_port_tiocmset(struct usb_serial_port *port, > + unsigned int set, unsigned int clear) > +{ > + struct nt124_private *priv = usb_get_serial_port_data(port); > + unsigned int newctrl; > + unsigned long flags; > + int ret; > + > + spin_lock_irqsave(&priv->ctrl_lock, flags); > + newctrl = priv->ctrlout; > + > + if (set & TIOCM_DTR) > + newctrl |= NT124_CTRL_DTR; > + > + if (set & TIOCM_RTS) > + newctrl |= NT124_CTRL_RTS; > + > + if (clear & TIOCM_DTR) > + newctrl &= ~NT124_CTRL_DTR; > + > + if (clear & TIOCM_RTS) > + newctrl &= ~NT124_CTRL_RTS; > + > + if (priv->ctrlout == newctrl) { > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); > + return 0; > + } > + > + priv->ctrlout = newctrl; > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); You really want a mutex (for ctrlout) here and make sure the control message below is covered as well so the update is atomic. tiocmget and set_termios needs to be updated as well accordingly. > + > + ret = nt124_ctrl_msg(port, USB_NT124_REQ_SET_CONTROL_LINE_STATE, > + newctrl, NULL, 0); > + > + return usb_translate_errors(ret); > +} > + > +static int nt124_tiocmset(struct tty_struct *tty, > + unsigned int set, unsigned int clear) > +{ > + struct usb_serial_port *port = tty->driver_data; > + > + return nt124_port_tiocmset(port, set, clear); > +} > + > +static void nt124_dtr_rts(struct usb_serial_port *port, int on) > +{ > + if (on) > + nt124_port_tiocmset(port, TIOCM_DTR | TIOCM_RTS, 0); > + else > + nt124_port_tiocmset(port, 0, TIOCM_DTR | TIOCM_RTS); > +} > + > +static void nt124_set_termios(struct tty_struct *tty, > + struct usb_serial_port *port, > + struct ktermios *termios_old) > +{ > + struct nt124_private *priv = usb_get_serial_port_data(port); > + struct ktermios *termios = &tty->termios; > + struct nt124_line_coding newline; > + int newctrl; u16? > + u16 flowcontrol; > + unsigned long flags; > + > + newline.dwDTERate = cpu_to_le32(tty_get_baud_rate(tty)); > + if (C_CSTOPB(tty)) > + newline.bCharFormat = NT124_STOP_BITS_2; > + else > + newline.bCharFormat = NT124_STOP_BITS_1; > + > + /* Mark and space parity aren't supported. > + * Use the old setting or no parity if termios_old isn't available. */ Comment style again (check all multi-line comments). > + if (C_PARENB(tty) && C_CMSPAR(tty)) { > + termios->c_cflag &= ~(PARENB | PARODD | CMSPAR); > + if (termios_old) > + termios->c_cflag |= termios_old->c_cflag & > + (PARENB | PARODD | CMSPAR); Please add brackets. > + } > + > + if (C_PARENB(tty)) { > + if (C_PARODD(tty)) > + newline.bParityType = NT124_PARITY_ODD; > + else > + newline.bParityType = NT124_PARITY_EVEN; > + } else { > + newline.bParityType = NT124_PARITY_NONE; > + } > + > + /* 5 and 6 databits aren't supported. > + * Use the old setting or 8 databits if termios_old isn't available. */ > + if (C_CSIZE(tty) == CS5 || C_CSIZE(tty) == CS6) { > + termios->c_cflag &= ~CSIZE; > + if (termios_old) > + termios->c_cflag |= termios_old->c_cflag & CSIZE; > + else > + termios->c_cflag |= CS8; > + } > + > + switch (C_CSIZE(tty)) { > + case CS7: > + newline.bDataBits = 7; > + break; > + case CS8: > + default: > + newline.bDataBits = 8; > + break; > + } > + > + spin_lock_irqsave(&priv->ctrl_lock, flags); > + newctrl = priv->ctrlout; > + if (C_BAUD(tty) == B0) { > + newline.dwDTERate = priv->line.dwDTERate; > + newctrl &= ~(NT124_CTRL_DTR | NT124_CTRL_RTS); > + } else if (termios_old && (termios_old->c_cflag & CBAUD) == B0) { > + newctrl |= NT124_CTRL_DTR | NT124_CTRL_RTS; > + } > + > + if (newctrl != priv->ctrlout) { > + priv->ctrlout = newctrl; > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); > + nt124_ctrl_msg(port, USB_NT124_REQ_SET_CONTROL_LINE_STATE, > + newctrl, NULL, 0); Indent continuation lines at least two tabs further. > + } else > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); Brackets on else block. > + > + if (memcmp(&priv->line, &newline, sizeof(newline))) { > + memcpy(&priv->line, &newline, sizeof(newline)); > + dev_dbg(&port->dev, "%s - set line: %u %u %u %u\n", > + __func__, > + le32_to_cpu(newline.dwDTERate), > + newline.bCharFormat, newline.bParityType, > + newline.bDataBits); > + nt124_ctrl_msg(port, USB_NT124_REQ_SET_LINE_CODING, 0, > + &priv->line, sizeof(priv->line)); You need to allocate priv->line separately from the containing struct as you use it for DMA transfers. Alternatively, you can kmalloc newline on every set_termios call and use that for the control request here. > + } > + > + if (!termios_old || > + C_CRTSCTS(tty) != (termios_old->c_cflag & CRTSCTS)) { Increase indentation and drop parentheses. > + if (C_CRTSCTS(tty)) > + flowcontrol = NT124_RTSCTS_FLOW_CONTROL_ON; > + else > + flowcontrol = NT124_RTSCTS_FLOW_CONTROL_OFF; > + nt124_ctrl_msg(port, USB_NT124_REQ_SET_FLOW_CONTROL, > + flowcontrol, NULL, 0); > + } This should take B0 into account. Take a look at how mxuport handles this for example. > +} > + > +static bool nt124_tx_empty(struct usb_serial_port *port) > +{ > + struct nt124_private *priv = usb_get_serial_port_data(port); > + u8 tx_empty; You need to kmalloc this buffer as some archs cannot do DMA to the stack. > + int retval; > + unsigned long flags; > + bool result; > + > + /* If tx_empty has already been marked false there is no need to poll > + * for it's value since we can rely on a bulk in empty notification. > + * In the common situation of tcdrain() being called after transmitting > + * we prevent a considerable amount of bus traffic. */ > + spin_lock_irqsave(&priv->ctrl_lock, flags); > + if (!priv->tx_empty) { > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); > + return false; > + } > + > + /* Set tx_empty to false, since it can only set it to true below > + * there is no possibility to lose a tx_empty notification from the > + * bulk in packet while the lock isn't held */ The code looks correct, but could you try to reword this to better describe what is going on here? > + priv->tx_empty = false; > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); > + > + retval = usb_control_msg(port->serial->dev, > + usb_rcvctrlpipe(port->serial->dev, 0), > + USB_NT124_REQ_GET_TXEMPTY, > + USB_DIR_IN | USB_TYPE_VENDOR, > + 0, > + priv->bInterfaceNumber, > + &tx_empty, > + sizeof(tx_empty), > + USB_CTRL_SET_TIMEOUT); > + if (retval < 0) { > + dev_err(&port->dev, > + "%s - usb_control_msg failed (%d)\n", > + __func__, retval); > + return true; > + } > + > + if (retval != sizeof(tx_empty)) { > + dev_err(&port->dev, > + "%s - short read (%d / %zu)\n", > + __func__, retval, sizeof(tx_empty)); > + return true; > + } Refactor this bit as a generic helper (e.g. nt124_vendor_read and rename nt124_ctrl_msg as nt124_vendor_write) to improve readability. > + > + spin_lock_irqsave(&priv->ctrl_lock, flags); > + if (tx_empty) > + priv->tx_empty = true; > + result = priv->tx_empty; This bit isn't obvious either so please add a comment here as well (or make sure it's covered by the description above). > + spin_unlock_irqrestore(&priv->ctrl_lock, flags); > + > + return result; > +} > + > +static void nt124_break_ctl(struct tty_struct *tty, int state) > +{ > + struct usb_serial_port *port = tty->driver_data; > + int retval; > + u16 val; > + > + if (state) > + val = NT124_BREAK_ON; > + else > + val = NT124_BREAK_OFF; > + > + retval = nt124_ctrl_msg(port, USB_NT124_REQ_SEND_BREAK, val, NULL, 0); > + if (retval < 0) > + dev_err(&port->dev, "%s - send break failed\n", __func__); > +} > + > +static int nt124_open(struct tty_struct *tty, No line break. > + struct usb_serial_port *port) > +{ > + nt124_set_termios(tty, port, NULL); This needs to be conditional on tty, which can be NULL in case the device is used as a console. > + > + return usb_serial_generic_open(tty, port); > +} Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
