[PATCH v2] USB: serial: add nt124 usb to serial driver

[PATCH v2] USB: serial: add nt124 usb to serial driver

[George McCollister]

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.mccollister@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.

 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
+
+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",
+			__func__, retval);
+		return retval;
+	}
+
+	if (retval != len) {
+		dev_err(&port->dev,
+			"%s - short write (%d / %zu)\n",
+			__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);
+
+	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) {
+		tty = tty_port_tty_get(&port->port);
+		if (tty)
+			usb_serial_handle_dcd_change(port, tty,
+					newctrl & NT124_CTRL_DCD);
+	}
+}
+
+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;
+	size_t datalen;
+
+	/* The packet should always be at least 2 bytes because status is
+	 * included. If it's too short, discard it. */
+	if (urb->actual_length < 2)
+		return;
+
+	nt124_process_status(port, priv, data);
+
+	datalen = urb->actual_length - 2;
+
+	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);
+
+	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 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. */
+	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);
+	}
+
+	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);
+	} else
+		spin_unlock_irqrestore(&priv->ctrl_lock, flags);
+
+	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));
+	}
+
+	if (!termios_old ||
+		C_CRTSCTS(tty) != (termios_old->c_cflag & CRTSCTS)) {
+		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);
+	}
+}
+
+static bool nt124_tx_empty(struct usb_serial_port *port)
+{
+	struct nt124_private *priv = usb_get_serial_port_data(port);
+	u8 tx_empty;
+	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 */
+	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;
+	}
+
+	spin_lock_irqsave(&priv->ctrl_lock, flags);
+	if (tx_empty)
+		priv->tx_empty = true;
+	result = priv->tx_empty;
+	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,
+		      struct usb_serial_port *port)
+{
+	nt124_set_termios(tty, port, NULL);
+
+	return usb_serial_generic_open(tty, port);
+}
+
+static int nt124_port_probe(struct usb_serial_port *port)
+{
+	struct usb_interface *interface = port->serial->interface;
+	struct usb_host_interface *cur_altsetting = interface->cur_altsetting;
+	struct nt124_private *priv;
+
+	priv = devm_kzalloc(&port->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	spin_lock_init(&priv->ctrl_lock);
+	priv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
+	priv->tx_empty = true;
+
+	usb_set_serial_port_data(port, priv);
+
+	return 0;
+}
+
+static struct usb_serial_driver nt124_device = {
+	.driver = {
+		.owner =	THIS_MODULE,
+		.name =		"nt124",
+	},
+	.id_table =		id_table,
+	.num_ports =		1,
+	.open =			nt124_open,
+	.process_read_urb =	nt124_process_read_urb,
+	.tx_empty =		nt124_tx_empty,
+	.throttle =		usb_serial_generic_throttle,
+	.unthrottle =		usb_serial_generic_unthrottle,
+	.set_termios =		nt124_set_termios,
+	.tiocmget =		nt124_tiocmget,
+	.tiocmset =		nt124_tiocmset,
+	.dtr_rts =		nt124_dtr_rts,
+	.break_ctl =		nt124_break_ctl,
+	.port_probe =		nt124_port_probe,
+};
+
+static struct usb_serial_driver * const serial_drivers[] = {
+	&nt124_device, NULL
+};
+
+module_usb_serial_driver(serial_drivers, id_table);
+
+MODULE_AUTHOR("George McCollister <george.mccollister@gmail.com>");
+MODULE_DESCRIPTION("nt124 USB serial driver");
+MODULE_LICENSE("GPL");
-- 
2.2.2

Re: [PATCH v2] USB: serial: add nt124 usb to serial driver

[Johan Hovold]

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.mccollister@gmail.com>

Thanks for the v2, George. And sorry for not getting back to you sooner.
I'll try to find time to review it this week.

Johan

Re: [PATCH v2] USB: serial: add nt124 usb to serial driver

[Johan Hovold]

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.mccollister@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