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 <[email protected]>
---
drivers/usb/serial/Kconfig | 9 +
drivers/usb/serial/Makefile | 1 +
drivers/usb/serial/nt124.c | 429 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 439 insertions(+)
create mode 100644 drivers/usb/serial/nt124.c
diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index a69f7cd..6dfc340 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -509,6 +509,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 nt124 serial device"
+ 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..d7557ff
--- /dev/null
+++ b/drivers/usb/serial/nt124.c
@@ -0,0 +1,429 @@
+/*
+ * nt124.c
+ *
+ * Copyright (c) 2014 NovaTech LLC
+ *
+ * Driver for nt124 4x serial board based on STM32F103
+ *
+ * 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 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
+
+#define DRIVER_AUTHOR "George McCollister <[email protected]>"
+#define DRIVER_DESC "nt124 USB serial driver"
+
+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_SET_FLOW_CONTROL 0x90
+
+struct nt124_line_coding {
+ __le32 dwDTERate;
+ u8 bCharFormat;
+ u8 bParityType;
+ u8 bDataBits;
+} __packed;
+
+struct nt124_private {
+ /* USB interface */
+ u16 bInterfaceNumber;
+ /* input control lines (DCD, DSR, RI, break, overruns) */
+ unsigned int ctrlin;
+ /* output control lines (DTR, RTS) */
+ unsigned int ctrlout;
+ /* termios CLOCAL */
+ unsigned char clocal;
+ /* bits, stop, parity */
+ struct nt124_line_coding line;
+ int serial_transmit;
+ unsigned int flowctrl;
+};
+
+static int nt124_ctrl_msg(struct usb_serial_port *port, int request, int value,
+ void *buf, int 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_TYPE_CLASS | USB_RECIP_INTERFACE, value,
+ priv->bInterfaceNumber,
+ buf, len, 5000);
+
+ dev_dbg(&port->dev,
+ "%s - rq 0x%02x, val %#x, len %#x, result %d\n",
+ __func__, request, value, len, retval);
+
+ return retval < 0 ? retval : 0;
+}
+
+#define nt124_set_control(port, control) \
+ nt124_ctrl_msg(port, USB_NT124_REQ_SET_CONTROL_LINE_STATE, control, \
+ NULL, 0)
+#define nt124_set_line(port, line) \
+ nt124_ctrl_msg(port, USB_NT124_REQ_SET_LINE_CODING, 0, line, \
+ sizeof *(line))
+#define nt124_send_break(port, ms) \
+ nt124_ctrl_msg(port, USB_NT124_REQ_SEND_BREAK, ms, NULL, 0)
+#define nt124_set_flowctrl(port, flowctrl) \
+ nt124_ctrl_msg(port, USB_NT124_SET_FLOW_CONTROL, flowctrl, NULL, 0)
+
+static void nt124_process_notify(struct usb_serial_port *port,
+ struct nt124_private *priv,
+ unsigned char *data)
+{
+ int newctrl;
+ unsigned long flags;
+
+ newctrl = get_unaligned_le16(data);
+ if (newctrl & NT124_CTRL_TXEMPTY) {
+ spin_lock_irqsave(&port->lock, flags);
+ priv->serial_transmit = 0;
+ spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_tty_wakeup(&port->port);
+ }
+
+ if (!priv->clocal && (priv->ctrlin & ~newctrl & NT124_CTRL_DCD)) {
+ dev_dbg(&port->dev, "%s - calling hangup\n",
+ __func__);
+ tty_port_tty_hangup(&port->port, false);
+ }
+
+ priv->ctrlin = newctrl;
+}
+
+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;
+
+ if (urb->actual_length < 2)
+ return;
+
+ nt124_process_notify(port, priv, data);
+
+ if (urb->actual_length == 2)
+ return;
+
+ tty_insert_flip_string(&port->port, &data[2],
+ urb->actual_length - 2);
+ 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);
+
+ return (priv->ctrlout & NT124_CTRL_DTR ? TIOCM_DTR : 0) |
+ (priv->ctrlout & NT124_CTRL_RTS ? TIOCM_RTS : 0) |
+ (priv->ctrlin & NT124_CTRL_DSR ? TIOCM_DSR : 0) |
+ (priv->ctrlin & NT124_CTRL_RI ? TIOCM_RI : 0) |
+ (priv->ctrlin & NT124_CTRL_DCD ? TIOCM_CD : 0) |
+ (priv->ctrlin & NT124_CTRL_CTS ? TIOCM_CTS : 0);
+}
+
+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;
+
+ newctrl = priv->ctrlout;
+ set = (set & TIOCM_DTR ? NT124_CTRL_DTR : 0) |
+ (set & TIOCM_RTS ? NT124_CTRL_RTS : 0);
+ clear = (clear & TIOCM_DTR ? NT124_CTRL_DTR : 0) |
+ (clear & TIOCM_RTS ? NT124_CTRL_RTS : 0);
+
+ newctrl = (newctrl & ~clear) | set;
+
+ if (priv->ctrlout == newctrl)
+ return 0;
+ return nt124_set_control(port, priv->ctrlout = newctrl);
+}
+
+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 = priv->ctrlout;
+
+ newline.dwDTERate = cpu_to_le32(tty_get_baud_rate(tty));
+ newline.bCharFormat = termios->c_cflag & CSTOPB ? 2 : 0;
+ newline.bParityType = termios->c_cflag & PARENB ?
+ (termios->c_cflag & PARODD ? 1 : 2) +
+ (termios->c_cflag & CMSPAR ? 2 : 0) : 0;
+ switch (termios->c_cflag & CSIZE) {
+ case CS5:
+ newline.bDataBits = 5;
+ break;
+ case CS6:
+ newline.bDataBits = 6;
+ break;
+ case CS7:
+ newline.bDataBits = 7;
+ break;
+ case CS8:
+ default:
+ newline.bDataBits = 8;
+ break;
+ }
+ priv->clocal = ((termios->c_cflag & CLOCAL) != 0);
+
+ if (C_BAUD(tty) == B0) {
+ newline.dwDTERate = priv->line.dwDTERate;
+ newctrl &= ~NT124_CTRL_DTR;
+ } else if (termios_old && (termios_old->c_cflag & CBAUD) == B0) {
+ newctrl |= NT124_CTRL_DTR;
+ }
+
+ if (newctrl != priv->ctrlout)
+ nt124_set_control(port, priv->ctrlout = newctrl);
+
+ if (memcmp(&priv->line, &newline, sizeof(newline))) {
+ memcpy(&priv->line, &newline, sizeof(newline));
+ dev_dbg(&port->dev, "%s - set line: %d %d %d %d\n",
+ __func__,
+ le32_to_cpu(newline.dwDTERate),
+ newline.bCharFormat, newline.bParityType,
+ newline.bDataBits);
+ nt124_set_line(port, &priv->line);
+ }
+
+ if (termios->c_cflag & CRTSCTS && !priv->flowctrl) {
+ priv->flowctrl = 1;
+ nt124_set_flowctrl(port, priv->flowctrl);
+ } else if (!(termios->c_cflag & CRTSCTS) && priv->flowctrl) {
+ priv->flowctrl = 0;
+ nt124_set_flowctrl(port, priv->flowctrl);
+ }
+}
+
+static void nt124_write_bulk_callback(struct urb *urb)
+{
+ unsigned long flags;
+ struct usb_serial_port *port = urb->context;
+ struct nt124_private *priv = usb_get_serial_port_data(port);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i) {
+ if (port->write_urbs[i] == urb)
+ break;
+ }
+ spin_lock_irqsave(&port->lock, flags);
+ port->tx_bytes -= urb->transfer_buffer_length;
+ if (!urb->status)
+ priv->serial_transmit = 1;
+ set_bit(i, &port->write_urbs_free);
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ switch (urb->status) {
+ case 0:
+ break;
+ case -ENOENT:
+ case -ECONNRESET:
+ case -ESHUTDOWN:
+ dev_dbg(&port->dev, "%s - urb stopped: %d\n",
+ __func__, urb->status);
+ return;
+ case -EPIPE:
+ dev_err_console(port, "%s - urb stopped: %d\n",
+ __func__, urb->status);
+ return;
+ default:
+ dev_err_console(port, "%s - nonzero urb status: %d\n",
+ __func__, urb->status);
+ goto resubmit;
+ }
+
+resubmit:
+ usb_serial_generic_write_start(port, GFP_ATOMIC);
+ usb_serial_port_softint(port);
+}
+
+static int nt124_chars_in_buffer(struct tty_struct *tty)
+{
+ struct usb_serial_port *port = tty->driver_data;
+ struct nt124_private *priv = usb_get_serial_port_data(port);
+ unsigned long flags;
+ int chars;
+
+ if (!port->bulk_out_size)
+ return 0;
+
+ spin_lock_irqsave(&port->lock, flags);
+ chars = kfifo_len(&port->write_fifo) + port->tx_bytes +
+ priv->serial_transmit;
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ dev_dbg(&port->dev, "%s - returns %d\n", __func__, chars);
+ return chars;
+}
+
+static void nt124_break_ctl(struct tty_struct *tty, int state)
+{
+ struct usb_serial_port *port = tty->driver_data;
+ int retval;
+
+ retval = nt124_send_break(port, state ? 0xffff : 0);
+ if (retval < 0)
+ dev_dbg(&port->dev, "%s - send break failed\n", __func__);
+}
+
+static int nt124_open(struct tty_struct *tty,
+ struct usb_serial_port *port)
+{
+ struct nt124_private *priv = usb_get_serial_port_data(port);
+ int result = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = 0;
+ port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ priv->flowctrl = 0;
+ nt124_set_termios(tty, port, NULL);
+ nt124_set_flowctrl(port, priv->flowctrl);
+
+ if (port->bulk_in_size)
+ result = usb_serial_generic_submit_read_urbs(port, GFP_KERNEL);
+
+ return result;
+}
+
+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;
+
+ priv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
+
+ 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,
+ .bulk_in_size = 32,
+ .bulk_out_size = 32,
+ .open = nt124_open,
+ .process_read_urb = nt124_process_read_urb,
+ .write_bulk_callback = nt124_write_bulk_callback,
+ .chars_in_buffer = nt124_chars_in_buffer,
+ .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(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
--
2.1.0
George,
On Mon, Dec 08, 2014 at 05:24:17PM -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 <[email protected]>
> ---
> drivers/usb/serial/Kconfig | 9 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/nt124.c | 429 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 439 insertions(+)
> create mode 100644 drivers/usb/serial/nt124.c
>
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
[...]
It applies to next without any errors, there are no checkpatch issues
other than recommending updating the MAINTAINERS file, and I can't find
any other obvious problems. Looks good to me.
Reviewed-by: Jeremiah Mahler <[email protected]>
--
- Jeremiah Mahler
Thanks for the review.
I didn't update the MAINTAINERS file because I looked in it and didn't
see any other individual usb/serial drivers listed with maintainers.
-George
On Mon, Dec 8, 2014 at 6:29 PM, Jeremiah Mahler <[email protected]> wrote:
> George,
>
> On Mon, Dec 08, 2014 at 05:24:17PM -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 <[email protected]>
>> ---
>> drivers/usb/serial/Kconfig | 9 +
>> drivers/usb/serial/Makefile | 1 +
>> drivers/usb/serial/nt124.c | 429 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 439 insertions(+)
>> create mode 100644 drivers/usb/serial/nt124.c
>>
>> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> [...]
>
> It applies to next without any errors, there are no checkpatch issues
> other than recommending updating the MAINTAINERS file, and I can't find
> any other obvious problems. Looks good to me.
>
> Reviewed-by: Jeremiah Mahler <[email protected]>
>
> --
> - Jeremiah Mahler
On Mon, Dec 08, 2014 at 05:24:17PM -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
Great, and thanks for the patch!
> Signed-off-by: George McCollister <[email protected]>
> ---
> drivers/usb/serial/Kconfig | 9 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/nt124.c | 429 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 439 insertions(+)
> create mode 100644 drivers/usb/serial/nt124.c
>
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index a69f7cd..6dfc340 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -509,6 +509,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 nt124 serial device"
USB NovaTech 124 Serial Driver (or NovaTech nt124)
> + 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..d7557ff
> --- /dev/null
> +++ b/drivers/usb/serial/nt124.c
> @@ -0,0 +1,429 @@
> +/*
> + * nt124.c
Put a brief description here instead.
> + *
> + * Copyright (c) 2014 NovaTech LLC
> + *
> + * Driver for nt124 4x serial board based on STM32F103
For example use something like this above.
> + *
> + * 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 in endpoint message. CTS control
bulk in endpoint
> + * 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
> +
> +#define DRIVER_AUTHOR "George McCollister <[email protected]>"
> +#define DRIVER_DESC "nt124 USB serial driver"
Just use the MODULE macros directly (at the end of the file), no need
for the defines.
> +
> +static const struct usb_device_id id_table[] = {
> + { USB_DEVICE(NT124_VID, NT124_USB_PID) },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +/*
> + * Output control lines.
> + */
> +
No new line.
> +#define NT124_CTRL_DTR 0x01
> +#define NT124_CTRL_RTS 0x02
> +
> +/*
> + * Input control lines and line errors.
> + */
> +
Same here.
> +#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_SET_FLOW_CONTROL 0x90
> +
> +struct nt124_line_coding {
> + __le32 dwDTERate;
> + u8 bCharFormat;
> + u8 bParityType;
> + u8 bDataBits;
> +} __packed;
__packed not needed.
> +
> +struct nt124_private {
> + /* USB interface */
Superfluous comment.
> + u16 bInterfaceNumber;
> + /* input control lines (DCD, DSR, RI, break, overruns) */
> + unsigned int ctrlin;
> + /* output control lines (DTR, RTS) */
> + unsigned int ctrlout;
Locking is missing for both of these (cdc-acm isn't always a great
example).
Use u16 for both?
> + /* termios CLOCAL */
> + unsigned char clocal;
Not needed (see below).
> + /* bits, stop, parity */
Superfluous comment.
> + struct nt124_line_coding line;
> + int serial_transmit;
Comment on this one, or just call it tx_empty.
And use bool.
> + unsigned int flowctrl;
Don't think you need this one either (more below).
> +};
> +
> +static int nt124_ctrl_msg(struct usb_serial_port *port, int request, int value,
> + void *buf, int len)
Use u16 (or unsigned int) for request and value, size_t for 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_TYPE_CLASS | USB_RECIP_INTERFACE, value,
> + priv->bInterfaceNumber,
> + buf, len, 5000);
Use a define for the timeout (e.g. USB_CTRL_SET_TIMEOUT).
> +
> + dev_dbg(&port->dev,
> + "%s - rq 0x%02x, val %#x, len %#x, result %d\n",
Request and val are 16-bit so use %04x for both, and %zu for len.
> + __func__, request, value, len, retval);
> +
> + return retval < 0 ? retval : 0;
Avoid ?: constructs.
Add proper error handling and add a dev_err for that case.
> +}
> +
> +#define nt124_set_control(port, control) \
> + nt124_ctrl_msg(port, USB_NT124_REQ_SET_CONTROL_LINE_STATE, control, \
> + NULL, 0)
> +#define nt124_set_line(port, line) \
> + nt124_ctrl_msg(port, USB_NT124_REQ_SET_LINE_CODING, 0, line, \
> + sizeof *(line))
> +#define nt124_send_break(port, ms) \
> + nt124_ctrl_msg(port, USB_NT124_REQ_SEND_BREAK, ms, NULL, 0)
> +#define nt124_set_flowctrl(port, flowctrl) \
> + nt124_ctrl_msg(port, USB_NT124_SET_FLOW_CONTROL, flowctrl, NULL, 0)
Don't use macros for these. Either call the ctrl_msg helper directly or
define proper inline functions.
> +
> +static void nt124_process_notify(struct usb_serial_port *port,
Rename process_status?
> + struct nt124_private *priv,
> + unsigned char *data)
> +{
> + int newctrl;
Use an unsigned type.
> + unsigned long flags;
> +
> + newctrl = get_unaligned_le16(data);
> + if (newctrl & NT124_CTRL_TXEMPTY) {
> + spin_lock_irqsave(&port->lock, flags);
> + priv->serial_transmit = 0;
> + spin_unlock_irqrestore(&port->lock, flags);
> + tty_port_tty_wakeup(&port->port);
> + }
Just store tx_empty. No need to do the wake up and not sure you'll need
the locking (see below).
> +
> + if (!priv->clocal && (priv->ctrlin & ~newctrl & NT124_CTRL_DCD)) {
> + dev_dbg(&port->dev, "%s - calling hangup\n",
> + __func__);
> + tty_port_tty_hangup(&port->port, false);
> + }
Call usb_serial_handle_dcd_change() unconditionally when DCD has changed
here instead.
> +
> + priv->ctrlin = newctrl;
Locking missing.
> +}
> +
> +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;
> +
> + if (urb->actual_length < 2)
> + return;
> +
> + nt124_process_notify(port, priv, data);
> +
> + if (urb->actual_length == 2)
> + return;
> +
> + tty_insert_flip_string(&port->port, &data[2],
> + urb->actual_length - 2);
No need to break line.
Use a temporary for length as well.
> + 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);
> +
> + return (priv->ctrlout & NT124_CTRL_DTR ? TIOCM_DTR : 0) |
> + (priv->ctrlout & NT124_CTRL_RTS ? TIOCM_RTS : 0) |
> + (priv->ctrlin & NT124_CTRL_DSR ? TIOCM_DSR : 0) |
> + (priv->ctrlin & NT124_CTRL_RI ? TIOCM_RI : 0) |
> + (priv->ctrlin & NT124_CTRL_DCD ? TIOCM_CD : 0) |
> + (priv->ctrlin & NT124_CTRL_CTS ? TIOCM_CTS : 0);
Locking is missing. Use temporary variables.
> +}
> +
> +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;
> +
> + newctrl = priv->ctrlout;
Locking throughout.
> + set = (set & TIOCM_DTR ? NT124_CTRL_DTR : 0) |
> + (set & TIOCM_RTS ? NT124_CTRL_RTS : 0);
> + clear = (clear & TIOCM_DTR ? NT124_CTRL_DTR : 0) |
> + (clear & TIOCM_RTS ? NT124_CTRL_RTS : 0);
Expand these without the ?:.
> +
> + newctrl = (newctrl & ~clear) | set;
> +
> + if (priv->ctrlout == newctrl)
> + return 0;
Add newline.
> + return nt124_set_control(port, priv->ctrlout = newctrl);
Don't do assignments in the argument list.
Also use usb_translate_errors on any error returned from USB core before
passing it to user space here.
> +}
> +
> +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);
Spaces around |.
> + else
> + nt124_port_tiocmset(port, 0, TIOCM_DTR|TIOCM_RTS);
Ditto.
> +}
> +
> +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 = priv->ctrlout;
> +
> + newline.dwDTERate = cpu_to_le32(tty_get_baud_rate(tty));
> + newline.bCharFormat = termios->c_cflag & CSTOPB ? 2 : 0;
Do you support 1.5 stop bits (e.g. when using 5 data bits)?
> + newline.bParityType = termios->c_cflag & PARENB ?
> + (termios->c_cflag & PARODD ? 1 : 2) +
> + (termios->c_cflag & CMSPAR ? 2 : 0) : 0;
This hardly readable. Don't use ?:
Add new line.
> + switch (termios->c_cflag & CSIZE) {
C_CSIZE(tty)
> + case CS5:
> + newline.bDataBits = 5;
> + break;
> + case CS6:
> + newline.bDataBits = 6;
> + break;
> + case CS7:
> + newline.bDataBits = 7;
> + break;
> + case CS8:
> + default:
> + newline.bDataBits = 8;
> + break;
> + }
> + priv->clocal = ((termios->c_cflag & CLOCAL) != 0);
Not needed.
> +
> + if (C_BAUD(tty) == B0) {
> + newline.dwDTERate = priv->line.dwDTERate;
> + newctrl &= ~NT124_CTRL_DTR;
> + } else if (termios_old && (termios_old->c_cflag & CBAUD) == B0) {
> + newctrl |= NT124_CTRL_DTR;
> + }
You need to raise or lower RTS here as well.
And you might need to take flow control into account as well (e.g. see
mxuport driver).
> +
> + if (newctrl != priv->ctrlout)
> + nt124_set_control(port, priv->ctrlout = newctrl);
No assignments in argument lists.
Locking.
> +
> + if (memcmp(&priv->line, &newline, sizeof(newline))) {
> + memcpy(&priv->line, &newline, sizeof(newline));
> + dev_dbg(&port->dev, "%s - set line: %d %d %d %d\n",
> + __func__,
%u
> + le32_to_cpu(newline.dwDTERate),
> + newline.bCharFormat, newline.bParityType,
> + newline.bDataBits);
> + nt124_set_line(port, &priv->line);
> + }
> +
> + if (termios->c_cflag & CRTSCTS && !priv->flowctrl) {
C_CRTSCTS(tty), just compare with old termios for changes, no need to
store flowctrl.
> + priv->flowctrl = 1;
> + nt124_set_flowctrl(port, priv->flowctrl);
> + } else if (!(termios->c_cflag & CRTSCTS) && priv->flowctrl) {
> + priv->flowctrl = 0;
> + nt124_set_flowctrl(port, priv->flowctrl);
> + }
> +}
> +
> +static void nt124_write_bulk_callback(struct urb *urb)
> +{
> + unsigned long flags;
> + struct usb_serial_port *port = urb->context;
> + struct nt124_private *priv = usb_get_serial_port_data(port);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i) {
> + if (port->write_urbs[i] == urb)
> + break;
> + }
> + spin_lock_irqsave(&port->lock, flags);
> + port->tx_bytes -= urb->transfer_buffer_length;
> + if (!urb->status)
> + priv->serial_transmit = 1;
Why do you do this?
This functions is just a copy of usb_serial_generic_write_bulk_callback
apart from this, and you probably don't need it.
> + set_bit(i, &port->write_urbs_free);
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + switch (urb->status) {
> + case 0:
> + break;
> + case -ENOENT:
> + case -ECONNRESET:
> + case -ESHUTDOWN:
> + dev_dbg(&port->dev, "%s - urb stopped: %d\n",
> + __func__, urb->status);
> + return;
> + case -EPIPE:
> + dev_err_console(port, "%s - urb stopped: %d\n",
> + __func__, urb->status);
> + return;
> + default:
> + dev_err_console(port, "%s - nonzero urb status: %d\n",
> + __func__, urb->status);
> + goto resubmit;
> + }
> +
> +resubmit:
> + usb_serial_generic_write_start(port, GFP_ATOMIC);
> + usb_serial_port_softint(port);
> +}
> +
> +static int nt124_chars_in_buffer(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct nt124_private *priv = usb_get_serial_port_data(port);
> + unsigned long flags;
> + int chars;
> +
> + if (!port->bulk_out_size)
> + return 0;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + chars = kfifo_len(&port->write_fifo) + port->tx_bytes +
> + priv->serial_transmit;
This is not correct.
Implement the tx_empty() callback instead, and use the generic
chars_in_buffer implementation.
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + dev_dbg(&port->dev, "%s - returns %d\n", __func__, chars);
> + return chars;
> +}
> +
> +static void nt124_break_ctl(struct tty_struct *tty, int state)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + int retval;
> +
> + retval = nt124_send_break(port, state ? 0xffff : 0);
No ?:
Use defines for 0xffff and 0 (e.g. break on and off).
> + if (retval < 0)
> + dev_dbg(&port->dev, "%s - send break failed\n", __func__);
dev_err
> +}
> +
> +static int nt124_open(struct tty_struct *tty,
> + struct usb_serial_port *port)
> +{
> + struct nt124_private *priv = usb_get_serial_port_data(port);
> + int result = 0;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + port->throttled = 0;
> + port->throttle_req = 0;
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + priv->flowctrl = 0;
Drop this and keep the current settings.
> + nt124_set_termios(tty, port, NULL);
> + nt124_set_flowctrl(port, priv->flowctrl);
Drop this. This is handled by tty and dtr_rts().
> +
> + if (port->bulk_in_size)
> + result = usb_serial_generic_submit_read_urbs(port, GFP_KERNEL);
Call usb_serial_generic_open() instead (and skip the throttle-flags bit
above).
> +
> + return result;
> +}
> +
> +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;
> +
> + priv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
> +
> + 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,
> + .bulk_in_size = 32,
> + .bulk_out_size = 32,
Why do you reduce these buffer sizes? They can be bigger than the
endpoint size for increased throughput.
> + .open = nt124_open,
> + .process_read_urb = nt124_process_read_urb,
> + .write_bulk_callback = nt124_write_bulk_callback,
> + .chars_in_buffer = nt124_chars_in_buffer,
> + .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(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL");
Thanks,
Johan
Johan Hovold <[email protected]> wrote:
> On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote:
> > + newline.bParityType = termios->c_cflag & PARENB ?
> > + (termios->c_cflag & PARODD ? 1 : 2) +
> > + (termios->c_cflag & CMSPAR ? 2 : 0) : 0;
>
> This hardly readable. Don't use ?:
>
There's also C_PARENB(tty), C_PARODD(tty), and C_CMSPAR(tty) macros
available, in addition to the others that Johan pointed out.
Sincerely,
Karl P
Johan,
Thanks for the thorough review.
On Wed, Dec 10, 2014 at 7:04 AM, Johan Hovold <[email protected]> wrote:
> On Mon, Dec 08, 2014 at 05:24:17PM -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
>
> Great, and thanks for the patch!
>
>> Signed-off-by: George McCollister <[email protected]>
>> ---
>> drivers/usb/serial/Kconfig | 9 +
>> drivers/usb/serial/Makefile | 1 +
>> drivers/usb/serial/nt124.c | 429 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 439 insertions(+)
>> create mode 100644 drivers/usb/serial/nt124.c
>>
>> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
>> index a69f7cd..6dfc340 100644
>> --- a/drivers/usb/serial/Kconfig
>> +++ b/drivers/usb/serial/Kconfig
>> @@ -509,6 +509,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 nt124 serial device"
>
> USB NovaTech 124 Serial Driver (or NovaTech nt124)
I'll use 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..d7557ff
>> --- /dev/null
>> +++ b/drivers/usb/serial/nt124.c
>> @@ -0,0 +1,429 @@
>> +/*
>> + * nt124.c
>
> Put a brief description here instead.
Okay
>
>> + *
>> + * Copyright (c) 2014 NovaTech LLC
>> + *
>> + * Driver for nt124 4x serial board based on STM32F103
>
> For example use something like this above.
Okay
>
>> + *
>> + * 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 in endpoint message. CTS control
>
> bulk in endpoint
Okay
>
>> + * 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
>> +
>> +#define DRIVER_AUTHOR "George McCollister <[email protected]>"
>> +#define DRIVER_DESC "nt124 USB serial driver"
>
> Just use the MODULE macros directly (at the end of the file), no need
> for the defines.
Okay
>
>> +
>> +static const struct usb_device_id id_table[] = {
>> + { USB_DEVICE(NT124_VID, NT124_USB_PID) },
>> + { },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(usb, id_table);
>> +
>> +/*
>> + * Output control lines.
>> + */
>> +
>
> No new line.
Okay
>
>> +#define NT124_CTRL_DTR 0x01
>> +#define NT124_CTRL_RTS 0x02
>> +
>> +/*
>> + * Input control lines and line errors.
>> + */
>> +
>
> Same here.
Okay
>
>> +#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_SET_FLOW_CONTROL 0x90
>> +
>> +struct nt124_line_coding {
>> + __le32 dwDTERate;
>> + u8 bCharFormat;
>> + u8 bParityType;
>> + u8 bDataBits;
>> +} __packed;
>
> __packed not needed.
Okay
>
>> +
>> +struct nt124_private {
>> + /* USB interface */
>
> Superfluous comment.
Okay
>
>> + u16 bInterfaceNumber;
>> + /* input control lines (DCD, DSR, RI, break, overruns) */
>> + unsigned int ctrlin;
>> + /* output control lines (DTR, RTS) */
>> + unsigned int ctrlout;
>
> Locking is missing for both of these (cdc-acm isn't always a great
> example).
>
> Use u16 for both?
Sure
>
>> + /* termios CLOCAL */
>> + unsigned char clocal;
>
> Not needed (see below).
Okay
>
>> + /* bits, stop, parity */
>
> Superfluous comment.
Okay
>
>> + struct nt124_line_coding line;
>> + int serial_transmit;
>
> Comment on this one, or just call it tx_empty.
>
> And use bool.
I'll just replace it with bool tx_empty
>
>> + unsigned int flowctrl;
>
> Don't think you need this one either (more below).
I'll remove it.
>
>> +};
>
>
>> +
>> +static int nt124_ctrl_msg(struct usb_serial_port *port, int request, int value,
>> + void *buf, int len)
>
> Use u16 (or unsigned int) for request and value, size_t for len.
Okay
>
>> +{
>> + 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_TYPE_CLASS | USB_RECIP_INTERFACE, value,
>> + priv->bInterfaceNumber,
>> + buf, len, 5000);
>
> Use a define for the timeout (e.g. USB_CTRL_SET_TIMEOUT).
Okay
>
>> +
>> + dev_dbg(&port->dev,
>> + "%s - rq 0x%02x, val %#x, len %#x, result %d\n",
>
> Request and val are 16-bit so use %04x for both, and %zu for len.
Okay
>
>> + __func__, request, value, len, retval);
>> +
>> + return retval < 0 ? retval : 0;
>
> Avoid ?: constructs.
Okay
>
> Add proper error handling and add a dev_err for that case.
Okay
>
>> +}
>> +
>> +#define nt124_set_control(port, control) \
>> + nt124_ctrl_msg(port, USB_NT124_REQ_SET_CONTROL_LINE_STATE, control, \
>> + NULL, 0)
>> +#define nt124_set_line(port, line) \
>> + nt124_ctrl_msg(port, USB_NT124_REQ_SET_LINE_CODING, 0, line, \
>> + sizeof *(line))
>> +#define nt124_send_break(port, ms) \
>> + nt124_ctrl_msg(port, USB_NT124_REQ_SEND_BREAK, ms, NULL, 0)
>> +#define nt124_set_flowctrl(port, flowctrl) \
>> + nt124_ctrl_msg(port, USB_NT124_SET_FLOW_CONTROL, flowctrl, NULL, 0)
>
> Don't use macros for these. Either call the ctrl_msg helper directly or
> define proper inline functions.
Okay, I'll probably just call them directly.
>
>> +
>> +static void nt124_process_notify(struct usb_serial_port *port,
>
> Rename process_status?
Sure
>
>> + struct nt124_private *priv,
>> + unsigned char *data)
>> +{
>> + int newctrl;
>
> Use an unsigned type.
I'll use u16
>
>> + unsigned long flags;
>> +
>> + newctrl = get_unaligned_le16(data);
>> + if (newctrl & NT124_CTRL_TXEMPTY) {
>> + spin_lock_irqsave(&port->lock, flags);
>> + priv->serial_transmit = 0;
>> + spin_unlock_irqrestore(&port->lock, flags);
>> + tty_port_tty_wakeup(&port->port);
>> + }
>
> Just store tx_empty. No need to do the wake up and not sure you'll need
> the locking (see below).
Okay, I'll drop the tty_port_tty_wakeup
>
>> +
>> + if (!priv->clocal && (priv->ctrlin & ~newctrl & NT124_CTRL_DCD)) {
>> + dev_dbg(&port->dev, "%s - calling hangup\n",
>> + __func__);
>> + tty_port_tty_hangup(&port->port, false);
>> + }
>
> Call usb_serial_handle_dcd_change() unconditionally when DCD has changed
> here instead.
Okay
>
>> +
>> + priv->ctrlin = newctrl;
>
> Locking missing.
I'll add a spinlock and use it around ctrlin and ctrlout with irqsave.
>
>> +}
>> +
>> +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;
>> +
>> + if (urb->actual_length < 2)
>> + return;
>> +
>> + nt124_process_notify(port, priv, data);
>> +
>> + if (urb->actual_length == 2)
>> + return;
>> +
>> + tty_insert_flip_string(&port->port, &data[2],
>> + urb->actual_length - 2);
>
> No need to break line.
Okay
>
> Use a temporary for length as well.
I'll make a temporary variable of type size_t named 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);
>> +
>> + return (priv->ctrlout & NT124_CTRL_DTR ? TIOCM_DTR : 0) |
>> + (priv->ctrlout & NT124_CTRL_RTS ? TIOCM_RTS : 0) |
>> + (priv->ctrlin & NT124_CTRL_DSR ? TIOCM_DSR : 0) |
>> + (priv->ctrlin & NT124_CTRL_RI ? TIOCM_RI : 0) |
>> + (priv->ctrlin & NT124_CTRL_DCD ? TIOCM_CD : 0) |
>> + (priv->ctrlin & NT124_CTRL_CTS ? TIOCM_CTS : 0);
>
> Locking is missing. Use temporary variables.
Okay
>
>> +}
>> +
>> +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;
>> +
>> + newctrl = priv->ctrlout;
>
> Locking throughout.
Okay
>
>> + set = (set & TIOCM_DTR ? NT124_CTRL_DTR : 0) |
>> + (set & TIOCM_RTS ? NT124_CTRL_RTS : 0);
>> + clear = (clear & TIOCM_DTR ? NT124_CTRL_DTR : 0) |
>> + (clear & TIOCM_RTS ? NT124_CTRL_RTS : 0);
>
> Expand these without the ?:.
Okay
>
>> +
>> + newctrl = (newctrl & ~clear) | set;
>> +
>> + if (priv->ctrlout == newctrl)
>> + return 0;
>
> Add newline.
Okay
>
>> + return nt124_set_control(port, priv->ctrlout = newctrl);
>
> Don't do assignments in the argument list.
Okay, this needs to be changed to put locking around ctrlout anyway.
>
> Also use usb_translate_errors on any error returned from USB core before
> passing it to user space here.
Okay
>
>> +}
>> +
>> +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);
>
> Spaces around |.
Okay
>
>> + else
>> + nt124_port_tiocmset(port, 0, TIOCM_DTR|TIOCM_RTS);
>
> Ditto.
Okay
>
>> +}
>> +
>> +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 = priv->ctrlout;
>> +
>> + newline.dwDTERate = cpu_to_le32(tty_get_baud_rate(tty));
>> + newline.bCharFormat = termios->c_cflag & CSTOPB ? 2 : 0;
>
> Do you support 1.5 stop bits (e.g. when using 5 data bits)?
No
>
>> + newline.bParityType = termios->c_cflag & PARENB ?
>> + (termios->c_cflag & PARODD ? 1 : 2) +
>> + (termios->c_cflag & CMSPAR ? 2 : 0) : 0;
>
> This hardly readable. Don't use ?:
Okay
>
> Add new line.
Okay
>
>> + switch (termios->c_cflag & CSIZE) {
>
> C_CSIZE(tty)
Okay
>
>> + case CS5:
>> + newline.bDataBits = 5;
>> + break;
>> + case CS6:
>> + newline.bDataBits = 6;
>> + break;
I should probably just remove CS5 and CS6 since I don't think they
will work anyway.
>> + case CS7:
>> + newline.bDataBits = 7;
>> + break;
>> + case CS8:
>> + default:
>> + newline.bDataBits = 8;
>> + break;
>> + }
>> + priv->clocal = ((termios->c_cflag & CLOCAL) != 0);
>
> Not needed.
Okay
>
>> +
>> + if (C_BAUD(tty) == B0) {
>> + newline.dwDTERate = priv->line.dwDTERate;
>> + newctrl &= ~NT124_CTRL_DTR;
>> + } else if (termios_old && (termios_old->c_cflag & CBAUD) == B0) {
>> + newctrl |= NT124_CTRL_DTR;
>> + }
>
> You need to raise or lower RTS here as well.
Oops, I missed that.
>
> And you might need to take flow control into account as well (e.g. see
> mxuport driver).
Okay
>
>> +
>> + if (newctrl != priv->ctrlout)
>> + nt124_set_control(port, priv->ctrlout = newctrl);
>
> No assignments in argument lists.
>
> Locking.
Okay
>
>> +
>> + if (memcmp(&priv->line, &newline, sizeof(newline))) {
>> + memcpy(&priv->line, &newline, sizeof(newline));
>> + dev_dbg(&port->dev, "%s - set line: %d %d %d %d\n",
>> + __func__,
>
> %u
Okay
>
>> + le32_to_cpu(newline.dwDTERate),
>> + newline.bCharFormat, newline.bParityType,
>> + newline.bDataBits);
>> + nt124_set_line(port, &priv->line);
>> + }
>> +
>> + if (termios->c_cflag & CRTSCTS && !priv->flowctrl) {
>
> C_CRTSCTS(tty), just compare with old termios for changes, no need to
> store flowctrl.
Okay
>
>> + priv->flowctrl = 1;
>> + nt124_set_flowctrl(port, priv->flowctrl);
>> + } else if (!(termios->c_cflag & CRTSCTS) && priv->flowctrl) {
>> + priv->flowctrl = 0;
>> + nt124_set_flowctrl(port, priv->flowctrl);
>> + }
>> +}
>> +
>> +static void nt124_write_bulk_callback(struct urb *urb)
>> +{
>> + unsigned long flags;
>> + struct usb_serial_port *port = urb->context;
>> + struct nt124_private *priv = usb_get_serial_port_data(port);
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i) {
>> + if (port->write_urbs[i] == urb)
>> + break;
>> + }
>> + spin_lock_irqsave(&port->lock, flags);
>> + port->tx_bytes -= urb->transfer_buffer_length;
>> + if (!urb->status)
>> + priv->serial_transmit = 1;
>
> Why do you do this?
>
> This functions is just a copy of usb_serial_generic_write_bulk_callback
> apart from this, and you probably don't need it.
I'll remove this function and switch to using tx_empty.
>
>> + set_bit(i, &port->write_urbs_free);
>> + spin_unlock_irqrestore(&port->lock, flags);
>> +
>> + switch (urb->status) {
>> + case 0:
>> + break;
>> + case -ENOENT:
>> + case -ECONNRESET:
>> + case -ESHUTDOWN:
>> + dev_dbg(&port->dev, "%s - urb stopped: %d\n",
>> + __func__, urb->status);
>> + return;
>> + case -EPIPE:
>> + dev_err_console(port, "%s - urb stopped: %d\n",
>> + __func__, urb->status);
>> + return;
>> + default:
>> + dev_err_console(port, "%s - nonzero urb status: %d\n",
>> + __func__, urb->status);
>> + goto resubmit;
>> + }
>> +
>> +resubmit:
>> + usb_serial_generic_write_start(port, GFP_ATOMIC);
>> + usb_serial_port_softint(port);
>> +}
>> +
>> +static int nt124_chars_in_buffer(struct tty_struct *tty)
>> +{
>> + struct usb_serial_port *port = tty->driver_data;
>> + struct nt124_private *priv = usb_get_serial_port_data(port);
>> + unsigned long flags;
>> + int chars;
>> +
>> + if (!port->bulk_out_size)
>> + return 0;
>> +
>> + spin_lock_irqsave(&port->lock, flags);
>> + chars = kfifo_len(&port->write_fifo) + port->tx_bytes +
>> + priv->serial_transmit;
>
> This is not correct.
>
> Implement the tx_empty() callback instead, and use the generic
> chars_in_buffer implementation.
I'll remove this function and switch to using tx_empty.
>
>> + spin_unlock_irqrestore(&port->lock, flags);
>> +
>> + dev_dbg(&port->dev, "%s - returns %d\n", __func__, chars);
>> + return chars;
>> +}
>> +
>> +static void nt124_break_ctl(struct tty_struct *tty, int state)
>> +{
>> + struct usb_serial_port *port = tty->driver_data;
>> + int retval;
>> +
>> + retval = nt124_send_break(port, state ? 0xffff : 0);
>
> No ?:
Okay
>
> Use defines for 0xffff and 0 (e.g. break on and off).
Okay
>
>> + if (retval < 0)
>> + dev_dbg(&port->dev, "%s - send break failed\n", __func__);
>
> dev_err
Okay
>
>> +}
>> +
>> +static int nt124_open(struct tty_struct *tty,
>> + struct usb_serial_port *port)
>> +{
>> + struct nt124_private *priv = usb_get_serial_port_data(port);
>> + int result = 0;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&port->lock, flags);
>> + port->throttled = 0;
>> + port->throttle_req = 0;
>> + spin_unlock_irqrestore(&port->lock, flags);
>> +
>> + priv->flowctrl = 0;
>
> Drop this and keep the current settings.
Okay
>
>> + nt124_set_termios(tty, port, NULL);
>> + nt124_set_flowctrl(port, priv->flowctrl);
>
> Drop this. This is handled by tty and dtr_rts().
Okay
>
>> +
>> + if (port->bulk_in_size)
>> + result = usb_serial_generic_submit_read_urbs(port, GFP_KERNEL);
>
> Call usb_serial_generic_open() instead (and skip the throttle-flags bit
> above).
Okay, so looks like the only thing I will do in this function is call
nt124_set_termios() followed by usb_serial_generic_open().
>
>> +
>> + return result;
>> +}
>> +
>> +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;
>> +
>> + priv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
>> +
>> + 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,
>> + .bulk_in_size = 32,
>> + .bulk_out_size = 32,
>
> Why do you reduce these buffer sizes? They can be bigger than the
> endpoint size for increased throughput.
I'll leave them out like most of the other drivers (and retest) unless
you have another suggestion.
>
>> + .open = nt124_open,
>> + .process_read_urb = nt124_process_read_urb,
>> + .write_bulk_callback = nt124_write_bulk_callback,
>> + .chars_in_buffer = nt124_chars_in_buffer,
>> + .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(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +MODULE_LICENSE("GPL");
>
> Thanks,
> Johan
-George
Johan,
While working on the tx_empty changes you suggested it occurred to me
that it might not be obvious to others that the firmware doesn't send
a packet with the NT124_CTRL_TXEMPTY flag cleared when it begins
transmitting. The practical implication is that if the driver sets
tx_empty = true when it sees NT124_CTRL_TXEMPTY, tx_empty must be
reset to false somewhere when more data is transmitted. Perhaps I
could add prepare_write_buffer and do it in there before calling
usb_serial_generic_prepare_write_buffer(). Does that sound acceptable?
If so I'll also initialize tx_empty = true in nt124_port_probe.
Regards,
George
On Fri, Dec 12, 2014 at 09:01:03AM -0600, George McCollister wrote:
> On Wed, Dec 10, 2014 at 7:04 AM, Johan Hovold <[email protected]> wrote:
> > On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote:
> >> + switch (termios->c_cflag & CSIZE) {
> >
> > C_CSIZE(tty)
> Okay
> >
> >> + case CS5:
> >> + newline.bDataBits = 5;
> >> + break;
> >> + case CS6:
> >> + newline.bDataBits = 6;
> >> + break;
> I should probably just remove CS5 and CS6 since I don't think they
> will work anyway.
Ok. Remember to update the passed termios with the settings that you
actually end up using (i.e. settings from old_termios).
> >> +static int nt124_open(struct tty_struct *tty,
> >> + struct usb_serial_port *port)
> >> +{
> >> + struct nt124_private *priv = usb_get_serial_port_data(port);
> >> + int result = 0;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&port->lock, flags);
> >> + port->throttled = 0;
> >> + port->throttle_req = 0;
> >> + spin_unlock_irqrestore(&port->lock, flags);
> >> +
> >> + priv->flowctrl = 0;
> >
> > Drop this and keep the current settings.
> Okay
> >
> >> + nt124_set_termios(tty, port, NULL);
> >> + nt124_set_flowctrl(port, priv->flowctrl);
> >
> > Drop this. This is handled by tty and dtr_rts().
> Okay
> >
> >> +
> >> + if (port->bulk_in_size)
> >> + result = usb_serial_generic_submit_read_urbs(port, GFP_KERNEL);
> >
> > Call usb_serial_generic_open() instead (and skip the throttle-flags bit
> > above).
> Okay, so looks like the only thing I will do in this function is call
> nt124_set_termios() followed by usb_serial_generic_open().
Yes, that should do it.
> >> +static struct usb_serial_driver nt124_device = {
> >> + .driver = {
> >> + .owner = THIS_MODULE,
> >> + .name = "nt124",
> >> + },
> >> + .id_table = id_table,
> >> + .num_ports = 1,
> >> + .bulk_in_size = 32,
> >> + .bulk_out_size = 32,
> >
> > Why do you reduce these buffer sizes? They can be bigger than the
> > endpoint size for increased throughput.
> I'll leave them out like most of the other drivers (and retest) unless
> you have another suggestion.
That's perfectly fine, and means that the buffer sizes will match the
endpoint sizes (they will in fact never be smaller than that).
Johan
On Sun, Dec 14, 2014 at 11:51:11AM -0600, George McCollister wrote:
> Johan,
>
> While working on the tx_empty changes you suggested it occurred to me
> that it might not be obvious to others that the firmware doesn't send
> a packet with the NT124_CTRL_TXEMPTY flag cleared when it begins
> transmitting. The practical implication is that if the driver sets
> tx_empty = true when it sees NT124_CTRL_TXEMPTY, tx_empty must be
> reset to false somewhere when more data is transmitted. Perhaps I
> could add prepare_write_buffer and do it in there before calling
> usb_serial_generic_prepare_write_buffer(). Does that sound acceptable?
Hmm. There's no way to query that flag? And the status is sent (as bulk
in data) periodically or only when data has been received? And not when
the actual status changes?
A potential problem with using prepare_write_buffer would be on failures
to submit the write urb, in which case this flag might never be cleared.
Johan
On Mon, Dec 15, 2014 at 3:52 AM, Johan Hovold <[email protected]> wrote:
> On Sun, Dec 14, 2014 at 11:51:11AM -0600, George McCollister wrote:
>> Johan,
>>
>> While working on the tx_empty changes you suggested it occurred to me
>> that it might not be obvious to others that the firmware doesn't send
>> a packet with the NT124_CTRL_TXEMPTY flag cleared when it begins
>> transmitting. The practical implication is that if the driver sets
>> tx_empty = true when it sees NT124_CTRL_TXEMPTY, tx_empty must be
>> reset to false somewhere when more data is transmitted. Perhaps I
>> could add prepare_write_buffer and do it in there before calling
>> usb_serial_generic_prepare_write_buffer(). Does that sound acceptable?
>
> Hmm. There's no way to query that flag? And the status is sent (as bulk
> in data) periodically or only when data has been received? And not when
> the actual status changes?
The bulk in packets are not sent periodically only on TXEMPTY, other
line change or received data. There's no way to query the flag, though
we're still at the stage we can make modifications to the firmware if
there's justification. One of the design goals is to minimize
unnecessary USB traffic so if there's a place to clear the flag in the
driver without querying it would be preferable.
>
> A potential problem with using prepare_write_buffer would be on failures
> to submit the write urb, in which case this flag might never be cleared.
Yes, this could be a problem though I wonder how many (if any)
recoverable situations this would happen in that didn't eventually
lead to a transmission. Adding a timer with a long timeout that set
tx_empty = true crossed my mind but that doesn't really seem any less
error prone than the original issue. I could of course duplicate a
bunch of code from generic.c and make a few minor changes to set
tx_empty = true but that obviously isn't desirable either.
If none of the above solutions sound acceptable, let me know I and I
guess I'll change the firmware to allow polling of TXEMPTY with a
control message and remove it from the bulk in packet.
Thanks again,
George
> Johan
On Mon, Dec 15, 2014 at 10:09:22AM -0600, George McCollister wrote:
> On Mon, Dec 15, 2014 at 3:52 AM, Johan Hovold <[email protected]> wrote:
> > On Sun, Dec 14, 2014 at 11:51:11AM -0600, George McCollister wrote:
> >> Johan,
> >>
> >> While working on the tx_empty changes you suggested it occurred to me
> >> that it might not be obvious to others that the firmware doesn't send
> >> a packet with the NT124_CTRL_TXEMPTY flag cleared when it begins
> >> transmitting. The practical implication is that if the driver sets
> >> tx_empty = true when it sees NT124_CTRL_TXEMPTY, tx_empty must be
> >> reset to false somewhere when more data is transmitted. Perhaps I
> >> could add prepare_write_buffer and do it in there before calling
> >> usb_serial_generic_prepare_write_buffer(). Does that sound acceptable?
> >
> > Hmm. There's no way to query that flag? And the status is sent (as bulk
> > in data) periodically or only when data has been received? And not when
> > the actual status changes?
>
> The bulk in packets are not sent periodically only on TXEMPTY, other
> line change or received data. There's no way to query the flag, though
> we're still at the stage we can make modifications to the firmware if
> there's justification. One of the design goals is to minimize
> unnecessary USB traffic so if there's a place to clear the flag in the
> driver without querying it would be preferable.
>
> > A potential problem with using prepare_write_buffer would be on failures
> > to submit the write urb, in which case this flag might never be cleared.
>
> Yes, this could be a problem though I wonder how many (if any)
> recoverable situations this would happen in that didn't eventually
> lead to a transmission. Adding a timer with a long timeout that set
> tx_empty = true crossed my mind but that doesn't really seem any less
> error prone than the original issue. I could of course duplicate a
> bunch of code from generic.c and make a few minor changes to set
> tx_empty = true but that obviously isn't desirable either.
>
> If none of the above solutions sound acceptable, let me know I and I
> guess I'll change the firmware to allow polling of TXEMPTY with a
> control message and remove it from the bulk in packet.
I think it would be best if you could add a way to query the driver. It
seems like that is the only way to avoid having write race with the
tx_empty bulk-in status reports.
Thanks,
Johan