Message-ID: <20150406093546.GX16062@localhost>
Date:	Mon, 6 Apr 2015 11:35:46 +0200
From:	Johan Hovold <johan@...nel.org>
To:	George McCollister <george.mccollister@...il.com>
Cc:	linux-usb@...r.kernel.org, johan@...nel.org,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] USB: serial: add nt124 usb to serial driver

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@...il.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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists