Re: [PATCH v6] USB: serial: nct_usb_serial: add support for Nuvoton USB adapter

[Sheng-Yuan Huang]

Hi Johan,

Thanks for the review. Please see my replies below.

> You never replied to my previous comments here:
>
> First, can you even control RTS when the device is in hardware flow
> control mode?

Thanks for raising this. I confirmed that the chip does not provide
automatic hardware flow control, so I think the driver still needs to
handle this explicitly. Based on that, I think the existing driver-side
handling should be kept for now.

> Second, I think you're supposed to just stop reading from the device
> here to exert back pressure so that it in turn deasserts RTS.

I am not sure that approach would work here, since the chip does not
provide automatic hardware flow control.

Best regards,
Sheng-Yuan Huang

> Johan Hovold <johan@xxxxxxxxxx> 於 2026年5月7日週四 上午12:14寫道：
>
> On Tue, Mar 17, 2026 at 02:14:46PM +0800, hsyemail2@xxxxxxxxx wrote:
> > From: Sheng-Yuan Huang <syhuang3@xxxxxxxxxxx>
> >
> > Add support for the Nuvoton USB-to-serial adapter, which provides
> > multiple serial ports over a single USB interface.
> >
> > The device exposes one control endpoint, one bulk-in endpoint, and
> > one bulk-out endpoint for data transfer. Port status is reported via
> > an interrupt-in or bulk-in endpoint, depending on device configuration.
> >
> > This driver implements basic TTY operations.
> >
> > Signed-off-by: Sheng-Yuan Huang <syhuang3@xxxxxxxxxxx>
> > ---
> >
> > v6:
> > - Address review comments from Oliver Neukum
> > - Thanks to Oliver Neukum and Johan Hovold for their review
> > - Narrow the protected scope in nct_tiocmset_helper(): port_lock now
> >   protects only cached HCR updates, while command transmission is
> >   serialized separately.
> > - Serialize the open transaction to prevent concurrent open calls
> >   from returning success before startup completes or before the open
> >   command is sent.
> > - Serialize the close transaction to avoid resource races.
>
> Please include the changelog for all revisions here when resending.
>
> >  drivers/usb/serial/Kconfig          |   10 +
> >  drivers/usb/serial/Makefile         |    1 +
> >  drivers/usb/serial/nct_usb_serial.c | 1455 +++++++++++++++++++++++++++
> >  3 files changed, 1466 insertions(+)
> >  create mode 100644 drivers/usb/serial/nct_usb_serial.c
> >
> > diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> > index ef8d1c73c754..96a022bb9b21 100644
> > --- a/drivers/usb/serial/Kconfig
> > +++ b/drivers/usb/serial/Kconfig
> > @@ -443,6 +443,16 @@ config USB_SERIAL_NAVMAN
> >         To compile this driver as a module, choose M here: the
> >         module will be called navman.
> >
> > +config USB_SERIAL_NUV_MULTI_UART
> > +     tristate "USB Nuvoton Multi-Ports Serial Driver"
> > +     depends on USB_SERIAL
> > +     help
> > +     Say Y here if you want to use a Nuvoton Multi-Ports USB to
> > +     serial converter device
> > +
> > +     To compile this driver as a module, choose M here: the
> > +     module will be called nct_usb_serial.
> > +
> >  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 c7bb1a88173e..c07919a52076 100644
> > --- a/drivers/usb/serial/Makefile
> > +++ b/drivers/usb/serial/Makefile
> > @@ -41,6 +41,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_NUV_MULTI_UART)              += nct_usb_serial.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/nct_usb_serial.c b/drivers/usb/serial/nct_usb_serial.c
> > new file mode 100644
> > index 000000000000..89dbfdc92136
> > --- /dev/null
> > +++ b/drivers/usb/serial/nct_usb_serial.c
> > @@ -0,0 +1,1455 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2024-2025 Nuvoton Corp
> > + * Copyright (C) 2024-2025 Sheng-Yuan Huang <syhuang3@xxxxxxxxxxx>
> > + *
> > + * Nuvoton USB to serial adapter driver
> > + *
> > + * This device interface consists of one control endpoint for configuration,
> > + * one bulk-out endpoint used for transmitting data for all serial ports,
> > + * and one bulk-in endpoint for receiving data from all serial ports.
> > + * The status of the ports may be reported via either an interrupt endpoint
> > + * or the bulk-in endpoint, depending on the device configuration.
> > + *
> > + * The number of serial ports is configurable in firmware and reported to
> > + * the driver via a vendor command at probe time.
> > + *
> > + * Host-device handshake sequence:
> > + *   1. Host sends SET_INIT to initialize the device.
> > + *   2. Host sends GET_PORTS_SUPPORT to query enabled ports (bitmask).
> > + *   3. For each port open: Host sends SET_OPEN_PORT.
> > + *   4. Host sends SET_CONFIG/SET_BAUD to configure UART parameters.
> > + *   5. For each port close: Host sends SET_CLOSE_PORT.
> > + */
> > +
> > +#include <linux/bitops.h>
>
> I don't think this one is used.
>
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
>
> > +#include <linux/moduleparam.h>
>
> Nor this one.
>
> > +#include <linux/serial.h>
>
> Nor this one?
>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/tty.h>
>
> > +#include <linux/tty_driver.h>
> > +#include <linux/tty_port.h>
>
> These two are typically included via tty.h (and you don't need the first
> one either way).
>
> > +#include <linux/tty_flip.h>
>
> > +#include <linux/uaccess.h>
>
> Same here.
>
> > +#include <linux/usb.h>
> > +#include <linux/usb/serial.h>
> > +#include <linux/kfifo.h>
> > +#include <linux/mutex.h>
> > +
> > +#define NCT_WRITE_FIFO_SIZE 1024
> > +
> > +#define NCT_VENDOR_ID                        0x0416
> > +#define NCT_PRODUCT_ID                       0x200b
> > +#define NCT_USB_CLASS                        0xff
> > +#define NCT_USB_SUBCLASS             0x0
> > +#define NCT_USB_PROTOCOL             0x1
> > +
> > +#define NCT_MAX_VENDOR_READ_SIZE     8
> > +
> > +static const struct usb_device_id id_table[] = {
> > +     {USB_DEVICE_AND_INTERFACE_INFO(NCT_VENDOR_ID, NCT_PRODUCT_ID,
> > +                                    NCT_USB_CLASS, NCT_USB_SUBCLASS,
> > +                                    NCT_USB_PROTOCOL)},
>
> I already asked you why you match on the interface class and protocol
> here. Does the device have more than one interface?
>
> Please post the corresponding output of usb-devices.
>
> > +     {} /* Terminating entry */
> > +};
> > +
> > +MODULE_DEVICE_TABLE(usb, id_table);
> > +
> > +#define NCT_MAX_SEND_BULK_SIZE 128
>
> This one is unused. What does it represent and should it be used instead
> of removed?
>
> I also asked you about the meaning of the following defines in v4, but
> you never replied:
>
>         > +#define NCT_MAX_PACKAGE_SIZE         4096 /* The max size of one
>         > writing package */
>         > +#define NCT_MAX_BULK_IN_SIZE         512
>         > +#define NCT_MAX_BULK_OUT_SIZE        512
>
>         The last three are not used either. Might they become useful? Can you
>         expand on their meaning?
>
> > +/* The port does not exist in FW (for device status). */
> > +#define NCT_EMPTY_PORT       0xff
> > +
> > +/* Hardware configuration */
> > +#define NCT_MAX_NUM_COM_DEVICES              8
> > +
> > +#define NCT_DEFAULT_BAUD             14 /* 115200 */
> > +static const unsigned int NCT_BAUD_SUP[] = {
> > +     /* It should be the same as FW's baud-rate table. */
> > +     B0,     B50,    B75,    B150,   B300,   B600,   B1200,
> > +     B1800,  B2400,  B4800,  B9600,  B19200, B38400, B57600,
> > +     B115200, B230400, B460800, B921600, B1500000
> > +};
> > +
> > +/* USB request */
> > +#define NCT_VENDOR_COM_READ_REQUEST_TYPE     0xc0
> > +#define NCT_VENDOR_COM_WRITE_REQUEST_TYPE    0x40
> > +#define NCT_VENDOR_COM_READ_REQUEST          0x01
> > +#define NCT_VENDOR_COM_WRITE_REQUEST         0x01
> > +/* Index definition */
> > +#define NCT_VCOM_INDEX_GLOBAL        0x0f
> > +
> > +/* Command */
> > +#define NCT_VCOM_GET_NUM_PORTS               0
> > +#define NCT_VCOM_GET_PORTS_SUPPORT   1
> > +#define NCT_VCOM_GET_BAUD            2
> > +#define NCT_VCOM_SET_INIT            3
> > +#define NCT_VCOM_SET_CONFIG          4
> > +#define NCT_VCOM_SET_BAUD            5
> > +#define NCT_VCOM_SET_HCR             6
> > +#define NCT_VCOM_SET_OPEN_PORT               7
> > +#define NCT_VCOM_SET_CLOSE_PORT              8
> > +#define NCT_VCOM_SILENT                      9
> > +/* Use bulk-in status instead of interrupt-in status */
> > +#define NCT_VCON_SET_BULK_IN_STATUS  10
> > +
> > +struct nct_vendor_cmd {
> > +     /* bits[3:0]: index, bits[11:4]: cmd, bits[15:12]: reserved */
> > +     __le16 val;
> > +};
>
> Why you need nct_ctrl_msg at all? It looks like you just keep converting
> back forth between host and le16 for no good reason.
>
> Can't you just use host order
>
>         u16 cmd
>
> throughout and pass that to usb_control_msg_recv() in
> nct_vendor_read/write() (and drop conversions in nct_build_cmd(),
> nct_build_ctrl_msg() and nct_vendor_read/write())?
>
> > +
> > +#define NCT_CMD_INDEX_MASK   0x000f
> > +#define NCT_CMD_CMD_MASK     0x0ff0
> > +#define NCT_CMD_CMD_SHIFT    4
> > +
> > +static inline __le16 nct_build_cmd(u8 cmd_code, u8 index)
> > +{
> > +     return cpu_to_le16(((u16)cmd_code << NCT_CMD_CMD_SHIFT) |
> > +                        (index & NCT_CMD_INDEX_MASK));
> > +}
> > +
> > +static inline u8 nct_get_cmd_index(__le16 val)
> > +{
> > +     return le16_to_cpu(val) & NCT_CMD_INDEX_MASK;
> > +}
>
> This one is not used and can be dropped.
>
> > +static inline u8 nct_get_cmd_cmd(__le16 val)
> > +{
> > +     return (le16_to_cpu(val) & NCT_CMD_CMD_MASK) >> NCT_CMD_CMD_SHIFT;
> > +}
> > +
> > +#define NCT_HDR_MAGIC                0xa5
> > +#define NCT_HDR_MAGIC2               0x5a
> > +#define NCT_HDR_MAGIC_STATUS 0x5b
> > +
> > +struct nct_packet_header {
> > +     u8 magic;
> > +     u8 magic2;
> > +     __le16 len_and_idx;  /* bits[3:0]: idx, bits[15:4]: len */
> > +} __packed;
> > +
> > +#define NCT_HDR_IDX_MASK     0x000f
> > +#define NCT_HDR_LEN_MASK     0xfff0
> > +#define NCT_HDR_LEN_SHIFT    4
> > +
> > +static inline void nct_set_hdr_idx_len(struct nct_packet_header *hdr,
> > +                                    u8 idx, u16 len)
> > +{
> > +     hdr->len_and_idx = cpu_to_le16((len << NCT_HDR_LEN_SHIFT) |
> > +                                    (idx & NCT_HDR_IDX_MASK));
> > +}
> > +
> > +static inline u8 nct_get_hdr_idx(const struct nct_packet_header *hdr)
> > +{
> > +     return le16_to_cpu(hdr->len_and_idx) & NCT_HDR_IDX_MASK;
> > +}
> > +
> > +static inline u16 nct_get_hdr_len(const struct nct_packet_header *hdr)
> > +{
> > +     return (le16_to_cpu(hdr->len_and_idx) & NCT_HDR_LEN_MASK) >>
> > +             NCT_HDR_LEN_SHIFT;
> > +}
> > +
> > +/* The definitions are for the fields of nct_ctrl_msg. */
> > +#define NCT_VCOM_1_STOP_BIT          0
> > +#define NCT_VCOM_2_STOP_BITS         1
> > +#define NCT_VCOM_PARITY_NONE         0
> > +#define NCT_VCOM_PARITY_ODD          1
> > +#define NCT_VCOM_PARITY_EVEN         2
> > +#define NCT_VCOM_DL5                 0
> > +#define NCT_VCOM_DL6                 1
> > +#define NCT_VCOM_DL7                 2
> > +#define NCT_VCOM_DL8                 3
> > +#define NCT_VCOM_DISABLE_FLOW_CTRL   0
> > +#define NCT_VCOM_XOFF                        1
> > +#define NCT_VCOM_RTS_CTS             2
> > +
> > +struct nct_ctrl_msg {
> > +     __le16 val;
> > +};
>
> And similar for nct_ctrl_msg. Why not used host order (u16 msg)
> throughout and only keep the final conversion to LE in
> nct_vendor_write()?
>
> > +#define NCT_CTRL_STOP_BIT_MASK               0x0001
> > +#define NCT_CTRL_PARITY_MASK         0x0006
> > +#define NCT_CTRL_PARITY_SHIFT                1
> > +#define NCT_CTRL_DATA_LEN_MASK               0x0018
> > +#define NCT_CTRL_DATA_LEN_SHIFT              3
> > +#define NCT_CTRL_FLOW_MASK           0x0060
> > +#define NCT_CTRL_FLOW_SHIFT          5
> > +#define NCT_CTRL_SPD_MASK            0x0f80
> > +#define NCT_CTRL_SPD_SHIFT           7
> > +#define NCT_CTRL_RESERVED_MASK               0xf000
> > +#define NCT_CTRL_RESERVED_SHIFT              12
> > +
> > +static inline __le16 nct_build_ctrl_msg(u8 stop_bit, u8 parity, u8 data_len,
> > +                                     u8 flow, u8 spd)
> > +{
> > +     u16 val = 0;
> > +
> > +     val |= (stop_bit & NCT_CTRL_STOP_BIT_MASK);
> > +     val |= ((parity & (NCT_CTRL_PARITY_MASK >> NCT_CTRL_PARITY_SHIFT)) <<
> > +             NCT_CTRL_PARITY_SHIFT);
> > +     val |= ((data_len &
> > +              (NCT_CTRL_DATA_LEN_MASK >> NCT_CTRL_DATA_LEN_SHIFT)) <<
> > +             NCT_CTRL_DATA_LEN_SHIFT);
> > +     val |= ((flow & (NCT_CTRL_FLOW_MASK >> NCT_CTRL_FLOW_SHIFT)) <<
> > +             NCT_CTRL_FLOW_SHIFT);
> > +     val |= ((spd & (NCT_CTRL_SPD_MASK >> NCT_CTRL_SPD_SHIFT)) <<
> > +             NCT_CTRL_SPD_SHIFT);
>
> If you shift the argument before you mask above you can drop the mask
> shift.
>
> > +
> > +     return cpu_to_le16(val);
> > +}
> > +
> > +#define NCT_USR_RDR  0x01
> > +#define NCT_USR_ORR  0x02
> > +#define NCT_USR_PBER 0x04
> > +#define NCT_USR_NSER 0x08
> > +#define NCT_USR_SBD  0x10
> > +#define NCT_USR_TBRE 0x20
> > +#define NCT_USR_TSRE 0x40
> > +#define NCT_USR_RFEI 0x80
> > +#define NCT_HSR_TCTS 0x01
> > +#define NCT_HSR_TDSR 0x02
> > +#define NCT_HSR_FERI 0x04
> > +#define NCT_HSR_TDCD 0x08
> > +#define NCT_HSR_CTS  0x10
> > +#define NCT_HSR_DSR  0x20
> > +#define NCT_HSR_RI   0x40
> > +#define NCT_HSR_DCD  0x80
> > +#define NCT_HCR_DTR  0x01
> > +#define NCT_HCR_RTS  0x02
> > +
> > +#define NCT_UART_STATE_MSR_MASK (NCT_HSR_TCTS | NCT_HSR_TDSR | \
> > +                              NCT_HSR_TDCD | NCT_HSR_DCD)
> > +
> > +struct nct_port_status {
> > +     u8 index;
> > +     u8 usr;
> > +     u8 hsr;
> > +     u8 hcr;
> > +};
> > +
> > +struct nct_serial {
> > +     /* Protects the private data in structure 'nct_serial'. */
>
> As I mentioned before, such comments doesn't really add any value so you
> can drop it.
>
> > +     spinlock_t serial_lock;
> > +     /* Serializes open/close startup-shutdown transactions. */
> > +     struct mutex open_close_mutex;
>
> Just call it 'mutex'. You only have one. Or at least shorten it to
> 'open_mutex'.
>
> > +     unsigned int open_count;
> > +
> > +     /* Reading data information */
> > +     struct nct_tty_port *cur_port;
> > +     int cur_len;
> > +
> > +     bool use_bulk_status;
> > +     u8 en_device_mask;
> > +     u8 last_assigned_hw_idx;
> > +     struct usb_endpoint_descriptor *bulk_out_ep;
> > +     struct usb_serial_port *port_by_hw_idx[NCT_MAX_NUM_COM_DEVICES];
> > +};
> > +
> > +struct nct_tty_port {
> > +     u8 hw_idx;
> > +     u8 usr;
> > +     u8 hsr;
> > +     u8 hcr;
> > +     /*
> > +      * Flow control - stop writing data to device.
> > +      * 0: Write enable, 1: Stop writing
> > +      */
> > +     bool flow_stop_wrt;
> > +
> > +     /* Serializes modem-control transactions sent to the device. */
> > +     struct mutex mctrl_mutex;
> > +     spinlock_t port_lock; /* Protects the port data. */
>
> Please drop the comment, it adds no values.
>
> > +     bool write_urb_in_use;
> > +};
> > +
> > +/* Functions */
> > +
> > +/* Read from USB control pipe */
> > +static int nct_vendor_read(struct usb_interface *intf,
> > +                        struct nct_vendor_cmd cmd, void *buf,
> > +                        int size)
> > +{
> > +     struct usb_device *udev = interface_to_usbdev(intf);
> > +     int ret;
> > +
> > +     if (size > NCT_MAX_VENDOR_READ_SIZE)
> > +             return -EINVAL;
> > +
> > +     ret = usb_control_msg_recv(udev, 0,
> > +                                NCT_VENDOR_COM_READ_REQUEST,
> > +                                NCT_VENDOR_COM_READ_REQUEST_TYPE,
> > +                                le16_to_cpu(cmd.val),
> > +                                intf->cur_altsetting->desc.bInterfaceNumber,
> > +                                buf, size, 100, GFP_KERNEL);
> > +
> > +     return ret;
> > +}
> > +
> > +static int nct_vendor_write(struct usb_interface *intf,
> > +                         struct nct_vendor_cmd cmd, u16 val)
> > +{
> > +     struct device *dev = &intf->dev;
> > +     struct usb_device *udev = interface_to_usbdev(intf);
> > +     __le16 le_val = cpu_to_le16(val);
> > +     int ret;
> > +
> > +     ret = usb_control_msg_send(udev, 0,
> > +                                NCT_VENDOR_COM_WRITE_REQUEST,
> > +                                NCT_VENDOR_COM_WRITE_REQUEST_TYPE,
> > +                                le16_to_cpu(cmd.val),
> > +                                intf->cur_altsetting->desc.bInterfaceNumber,
> > +                                &le_val, sizeof(le_val), 100, GFP_KERNEL);
> > +     if (ret)
> > +             dev_err(dev, "failed to write [%04x]: %d\n",
> > +                     nct_get_cmd_cmd(cmd.val), ret);
> > +
> > +     return ret;
> > +}
> > +
> > +/*
> > + * Prepare write buffer by extracting data from core write FIFO and
> > + * adding header
> > + */
> > +static int nct_prepare_write_buffer(struct usb_serial_port *port,
> > +                                 void *dest, size_t size)
> > +{
> > +     unsigned int count;
> > +     struct nct_packet_header hdr;
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
>
> Nit: Please consider reording these reverse christmas tree style, which
> tends to be easier to read (i.e. put the longest lines first).
>
> > +
> > +     if (size <= sizeof(hdr))
> > +             return 0;
>
> The buffer will at least be as large as the endpoint max packet size so
> this is not needed unless something is really broken.
>
> You can specify explicit buffer sizes in the driver structure to avoid
> this check:
>
>         .bulk_in_size
>         .bulk_out_size
>
> and the buffer can be larger than the endpoint (if the device can handle
> it).
>
> If that's not enough, you can add sanity checks to probe.
>
> > +
> > +     /* Leave room for header */
> > +     count = kfifo_out_locked(&port->write_fifo, dest + sizeof(hdr),
> > +                              size - sizeof(hdr), &port->lock);
> > +
> > +     if (!count)
> > +             return 0;
>
> Core has already made sure there is data in the fifo.
>
> > +
> > +     hdr.magic = NCT_HDR_MAGIC;
> > +     hdr.magic2 = NCT_HDR_MAGIC2;
> > +     nct_set_hdr_idx_len(&hdr, tport->hw_idx, count);
> > +
> > +     /*
> > +      * Copy header into buffer (use memcpy to avoid aliasing/unaligned
> > +      * access).
> > +      */
> > +     memcpy(dest, &hdr, sizeof(hdr));
>
> And the buffer alignment is at least 8-byte aligned
> (ARCH_KMALLOC_MINALIGN) so this is not needed either.
>
> > +
> > +     return count + sizeof(hdr);
> > +}
> > +
> > +static u16 nct_set_baud(struct usb_interface *intf, u16 index,
> > +                     unsigned int cflag, bool *found)
> > +{
> > +     struct nct_vendor_cmd cmd;
> > +     struct nct_ctrl_msg msg;
> > +     u16 i;
> > +     u8 spd = NCT_DEFAULT_BAUD;
> > +
> > +     *found = false;
> > +     cmd.val = nct_build_cmd(NCT_VCOM_SET_BAUD, index);
> > +     for (i = 0; i < ARRAY_SIZE(NCT_BAUD_SUP); i++) {
> > +             if ((cflag & CBAUD) != NCT_BAUD_SUP[i])
> > +                     continue;
> > +
> > +             spd = i;
> > +             /*
> > +              * Create control message
> > +              * Note: The NCT_VCOM_SET_BAUD only set the baud rate
> > +              */
> > +             msg.val = nct_build_ctrl_msg(0, 0, 0, 0, spd);
> > +             if (nct_vendor_write(intf, cmd, le16_to_cpu(msg.val)))
> > +                     dev_err(&intf->dev, "set index %d speed error\n",
> > +                             index);
>
> Please add brackets for readability wherever you have multi-line
> statements (as I mentioned before).
>
> In this case you could also just drop the line break as the resulting
> line is short enough.
>
> > +             else
> > +                     *found = true;
> > +
> > +             break;
> > +     }
> > +
> > +     return spd;
> > +}
> > +
> > +static void nct_set_termios(struct tty_struct *tty,
> > +                         struct usb_serial_port *port,
> > +                         const struct ktermios *old)
> > +{
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +     struct usb_serial *serial = port->serial;
> > +     struct usb_interface *intf = serial->interface;
> > +     struct ktermios *termios = &tty->termios;
> > +     struct nct_ctrl_msg msg;
> > +     struct nct_vendor_cmd cmd;
> > +     u8 stop_bit;
> > +     u8 parity;
> > +     u8 data_len;
> > +     u8 flow;
> > +     bool baud_found = false;
> > +     speed_t old_baud;
> > +
> > +     /* Device does not support CMSPAR. */
> > +     termios->c_cflag &= ~CMSPAR;
> > +
> > +     cmd.val = nct_build_cmd(NCT_VCOM_SET_CONFIG, tport->hw_idx);
> > +
> > +     /* Set stop bit */
> > +     if (C_CSTOPB(tty))
> > +             stop_bit = NCT_VCOM_2_STOP_BITS;
> > +     else
> > +             stop_bit = NCT_VCOM_1_STOP_BIT;
> > +
> > +     /* Set parity */
> > +     if (C_PARENB(tty)) {
> > +             if (C_PARODD(tty))
> > +                     parity = NCT_VCOM_PARITY_ODD;
> > +             else
> > +                     parity = NCT_VCOM_PARITY_EVEN;
> > +     } else {
> > +             parity = NCT_VCOM_PARITY_NONE;
> > +     }
>
> As I mentioned before you need to clear CMSPAR in the termios if you
> don't support sticky parity.
>
> > +
> > +     /* Set data bit length */
> > +     switch (C_CSIZE(tty)) {
> > +     case CS5:
> > +             data_len = NCT_VCOM_DL5;
> > +             break;
> > +     case CS6:
> > +             data_len = NCT_VCOM_DL6;
> > +             break;
> > +     case CS7:
> > +             data_len = NCT_VCOM_DL7;
> > +             break;
> > +     case CS8:
> > +     default:
> > +             data_len = NCT_VCOM_DL8;
> > +             break;
> > +     }
> > +
> > +     /* Set flow control */
> > +     if (C_CRTSCTS(tty))
> > +             flow = NCT_VCOM_RTS_CTS;
> > +     else if (I_IXON(tty))
> > +             flow = NCT_VCOM_XOFF;
> > +     else
> > +             flow = NCT_VCOM_DISABLE_FLOW_CTRL;
> > +
> > +     tty_port_set_cts_flow(tty->port, C_CRTSCTS(tty));
>
> I don't think you need to set this flag as the device appears to support
> automatic hardware flow control (NCT_VCOM_RTS_CTS). More below.
>
> > +     /* Create control message (spd=0: speed set separately via SET_BAUD) */
> > +     msg.val = nct_build_ctrl_msg(stop_bit, parity, data_len, flow, 0);
> > +     nct_vendor_write(intf, cmd, le16_to_cpu(msg.val));
> > +
> > +     /*
> > +      * Set baud rate if speed changed.
> > +      * Always call tty_encode_baud_rate() to keep kernel's baud rate
> > +      * synchronized with the device, especially during initial open
> > +      * when the device uses its default baud rate.
> > +      */
> > +     if (!old || old->c_ospeed != termios->c_ospeed)
> > +             nct_set_baud(intf, tport->hw_idx, termios->c_cflag, &baud_found);
> > +
> > +     if (baud_found) {
> > +             tty_encode_baud_rate(tty, tty_get_baud_rate(tty),
> > +                                  tty_get_baud_rate(tty));
> > +     } else {
> > +             if (old && tty_termios_baud_rate(old))
> > +                     old_baud = tty_termios_baud_rate(old);
> > +             else
> > +                     old_baud = 115200; /* Sync with device default */
> > +
> > +             tty_encode_baud_rate(tty, old_baud, old_baud);
> > +     }
> > +}
> > +
> > +static int nct_break(struct tty_struct *tty, int break_state)
> > +{
> > +     struct usb_serial_port *port = tty->driver_data;
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +     struct usb_serial *serial = port->serial;
> > +     struct usb_interface *intf = serial->interface;
> > +     struct nct_vendor_cmd cmd;
> > +
> > +     cmd.val = nct_build_cmd(NCT_VCOM_SILENT, tport->hw_idx);
> > +
> > +     return nct_vendor_write(intf, cmd, 0);
> > +}
> > +
> > +static int nct_tiocmset_helper(struct tty_struct *tty, unsigned int set,
> > +                            unsigned int clear)
>
> Drop the "_helper" suffix.
>
> > +{
> > +     struct usb_serial_port *port = tty->driver_data;
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +     struct usb_serial *serial = port->serial;
> > +     struct usb_interface *intf = serial->interface;
> > +     struct nct_ctrl_msg msg;
> > +     struct nct_vendor_cmd cmd;
> > +     u8 hcr;
> > +     int ret;
> > +
> > +     mutex_lock(&tport->mctrl_mutex);
> > +
> > +     /*
> > +      * port_lock serializes updates of the cached modem-control bits (hcr).
> > +      * The control transfer may sleep, so only the hcr read-modify-write is
> > +      * done under the spinlock.
> > +      */
> > +     spin_lock_irq(&tport->port_lock);
> > +     hcr = tport->hcr;
> > +
> > +     if (set & TIOCM_RTS)
> > +             hcr |= NCT_HCR_RTS;
> > +     if (set & TIOCM_DTR)
> > +             hcr |= NCT_HCR_DTR;
> > +     if (clear & TIOCM_RTS)
> > +             hcr &= ~NCT_HCR_RTS;
> > +     if (clear & TIOCM_DTR)
> > +             hcr &= ~NCT_HCR_DTR;
> > +
> > +     tport->hcr = hcr;
> > +     spin_unlock_irq(&tport->port_lock);
> > +
> > +     cmd.val = nct_build_cmd(NCT_VCOM_SET_HCR, tport->hw_idx);
> > +     msg.val = cpu_to_le16(hcr);
> > +     ret = nct_vendor_write(intf, cmd, le16_to_cpu(msg.val));
> > +     mutex_unlock(&tport->mctrl_mutex);
> > +
> > +     return ret;
> > +}
> > +
> > +static int nct_tiocmget(struct tty_struct *tty)
> > +{
> > +     struct usb_serial_port *port = tty->driver_data;
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +     unsigned int res;
> > +     u8 hcr, hsr;
> > +
> > +     spin_lock_irq(&tport->port_lock);
> > +     hcr = tport->hcr;
> > +     hsr = tport->hsr;
> > +     spin_unlock_irq(&tport->port_lock);
> > +     res = ((hcr & NCT_HCR_DTR) ? TIOCM_DTR : 0) |
> > +           ((hcr & NCT_HCR_RTS) ? TIOCM_RTS : 0) |
> > +           ((hsr & NCT_HSR_CTS) ? TIOCM_CTS : 0) |
> > +           ((hsr & NCT_HSR_DSR) ? TIOCM_DSR : 0) |
> > +           ((hsr & NCT_HSR_RI) ? TIOCM_RI : 0) |
> > +           ((hsr & NCT_HSR_DCD) ? TIOCM_CD : 0);
> > +
> > +     return res;
> > +}
> > +
> > +static void nct_rx_throttle(struct tty_struct *tty)
> > +{
> > +     /* Handle RTS line for RTS/CTS flow control */
> > +     if (C_CRTSCTS(tty))
> > +             nct_tiocmset_helper(tty, 0, TIOCM_RTS);
> > +}
>
> You never replied to my previous comments here:
>
>         First, can you even control RTS when the device is in hardware
>         flow control mode?
>
>         Second, I think you're supposed to just stop reading from the
>         device here to exert back pressure so that it in turn deasserts
>         RTS.
>
> But perhaps the latter does not work here as the bulk-in endpoint is
> shared.
>
> > +static void nct_rx_unthrottle(struct tty_struct *tty)
> > +{
> > +     /* Handle RTS line for RTS/CTS flow control */
> > +     if (C_CRTSCTS(tty))
> > +             nct_tiocmset_helper(tty, TIOCM_RTS, 0);
> > +}
> > +
> > +static int nct_serial_write(struct tty_struct *tty,
> > +                         struct usb_serial_port *port,
> > +                         const unsigned char *buf,
> > +                         int count)
> > +{
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +     unsigned int written;
> > +     int ret;
> > +
> > +     /* Flow control */
> > +     if (tty_port_cts_enabled(tty->port))
> > +             if (tport->flow_stop_wrt)
> > +                     return 0;
>
> Similarly here, you should be able to rely on the device to deassert RTS
> when its buffers fill up, right?
>
> I apparently already asked you this in a comment to v4.
>
> Please make sure to address and/or respond to all comments before
> sending another version.
>
> > +
> > +     if (!port)
> > +             return -ENXIO;
>
> How could port be NULL here?
>
> > +
> > +     /* Copy into core write FIFO */
> > +     written = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock);
> > +
>
> Nit: drop the newline
>
> > +     if (written) {
> > +             ret = usb_serial_generic_write_start(port, GFP_ATOMIC);
> > +             if (ret && ret != -EBUSY)
> > +                     dev_err(&port->dev,
> > +                             "usb_serial_generic_write_start failed: %d\n",
> > +                             ret);
> > +     }
> > +
> > +     return written;
> > +}
>
> Without the flow control bits you should be able to use the generic
> write implementation directly here (or possibly call it after checking).
>
> > +
> > +static unsigned int nct_write_room(struct tty_struct *tty)
> > +{
> > +     struct usb_serial_port *port = tty->driver_data;
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +     unsigned long flags;
> > +     unsigned int room;
> > +
> > +     spin_lock_irqsave(&tport->port_lock, flags);
> > +     room = kfifo_avail(&port->write_fifo);
> > +     spin_unlock_irqrestore(&tport->port_lock, flags);
> > +     return room;
> > +}
>
> Just use the generic implementation.
>
> > +static unsigned int nct_chars_in_buffer(struct tty_struct *tty)
> > +{
> > +     struct usb_serial_port *port = tty->driver_data;
> > +     unsigned int qlen;
> > +
> > +     /* Get queued bytes in core write FIFO */
> > +     qlen = kfifo_len(&port->write_fifo);
> > +     return qlen;
> > +}
>
> Here too.
>
> > +/*
> > + *  Starts reads urb on all ports. It is to avoid potential issues caused by
> > + *  multiple ports being opened almost simultaneously.
> > + *  It must be called AFTER startup, with urbs initialized.
> > + *  Returns 0 if successful, non-zero error otherwise.
> > + */
> > +static int nct_startup_device(struct usb_serial *serial)
> > +{
> > +     int ret = 0;
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     struct usb_serial_port *port;
> > +     unsigned long flags;
> > +     bool first_open = false;
> > +
> > +     /* Caller must hold serial_priv->open_close_mutex. */
>
> You can use lockdep_assert_held() to document this.
>
> > +
> > +     /* Start URBs on first open */
> > +     spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +     if (serial_priv->open_count++ == 0)
> > +             first_open = true;
> > +     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
>
> Why do you take the serial_lock spinlock for this when you already hold
> the open mutex?
>
> > +     /* Only the first open submits read_urb and, if needed, interrupt_in_urb. */
> > +     if (!first_open)
> > +             goto out_return;
>
> Just return directly (and skip ret initialisation above).
>
> > +
> > +     /* Start reading from bulk in endpoint */
> > +     port = serial->port[0];
> > +     ret = usb_submit_urb(port->read_urb, GFP_KERNEL);
> > +     if (ret) {
> > +             dev_err(&port->dev, "failed to submit read urb: %d\n", ret);
> > +             goto err_rollback;
> > +     }
>
> Why not use both URBs which is more efficient (i.e.
> usb_serial_generic_submit_read_urbs())?
>
> > +
> > +     /* For getting status from interrupt-in */
> > +     if (!serial_priv->use_bulk_status) {
> > +             /* Start reading from interrupt pipe */
> > +             port = serial->port[0];
> > +             ret = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
> > +             if (ret) {
> > +                     dev_err(&port->dev,
> > +                             "failed to submit interrupt urb: %d\n",
> > +                             ret);
> > +                     goto err_kill_read;
> > +             }
> > +     }
> > +
> > +     ret = 0;
> > +     goto out_return;
>
> Just return explicit zero here.
>
> > +
> > +err_kill_read:
> > +     usb_kill_urb(serial->port[0]->read_urb);
> > +err_rollback:
> > +     spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +     if (serial_priv->open_count)
> > +             serial_priv->open_count--;
> > +     if (!serial_priv->open_count) {
> > +             serial_priv->cur_port = NULL;
> > +             serial_priv->cur_len = 0;
> > +     }
> > +     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > +
> > +out_return:
> > +     return ret;
> > +}
> > +
> > +static void nct_shutdown_device(struct usb_serial *serial)
> > +{
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     unsigned long flags;
> > +     bool last_close = false;
> > +
> > +     /* Caller must hold serial_priv->open_close_mutex. */
> > +
> > +     spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +     if (serial_priv->open_count) {
> > +             serial_priv->open_count--;
> > +             if (!serial_priv->open_count) {
> > +                     last_close = true;
> > +                     serial_priv->cur_port = NULL;
> > +                     serial_priv->cur_len = 0;
> > +             }
> > +     }
> > +     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > +
> > +     if (!last_close)
> > +             return;
> > +
> > +     usb_kill_urb(serial->port[0]->read_urb);
> > +     if (!serial_priv->use_bulk_status)
> > +             usb_kill_urb(serial->port[0]->interrupt_in_urb);
> > +}
> > +
> > +static int nct_open(struct tty_struct *tty, struct usb_serial_port *port)
> > +{
> > +     struct nct_vendor_cmd cmd;
> > +     struct nct_ctrl_msg msg;
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +     struct usb_serial *serial = port->serial;
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     struct usb_interface *intf = serial->interface;
> > +     int ret;
> > +
> > +     if (!port->serial)
> > +             return -ENXIO;
>
> How could this happen?
>
> > +     mutex_lock(&serial_priv->open_close_mutex);
> > +
> > +     /* Be sure the device is started up */
> > +     ret = nct_startup_device(port->serial);
> > +     if (ret)
> > +             goto out_unlock;
> > +
> > +     cmd.val = nct_build_cmd(NCT_VCOM_SET_OPEN_PORT, tport->hw_idx);
> > +     msg.val = cpu_to_le16(0);
> > +     ret = nct_vendor_write(intf, cmd, le16_to_cpu(msg.val));
> > +     if (ret) {
> > +             dev_err(&port->dev, "Failed to open port: %d\n", ret);
> > +             nct_shutdown_device(serial);
> > +             goto out_unlock;
>
> Nit: I prefer "err_unlock" to make it clear that this path is only used
> on errors.
>
> > +     }
> > +
> > +     mutex_unlock(&serial_priv->open_close_mutex);
> > +
> > +     wake_up_interruptible(&port->port.open_wait);
>
> This one should only be called on carrier detect changes.
>
> > +
> > +     /*
> > +      * Delay 1ms for firmware to configure hardware after opening the port.
> > +      * (Especially at high speed)
> > +      */
> > +     usleep_range(1000, 2000);
>
> Nit: add a newline here.
>
> > +     return 0;
> > +
> > +out_unlock:
> > +     mutex_unlock(&serial_priv->open_close_mutex);
>
> Nit: and here.
>
> > +     return ret;
> > +}
> > +
> > +static void nct_close(struct usb_serial_port *port)
> > +{
> > +     struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +     struct usb_serial *serial = port->serial;
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     struct usb_interface *intf = serial->interface;
> > +     struct nct_ctrl_msg msg;
> > +     struct nct_vendor_cmd cmd;
> > +
> > +     mutex_lock(&port->serial->disc_mutex);
> > +     mutex_lock(&serial_priv->open_close_mutex);
> > +     /* If disconnected, don't send the close-command to the firmware */
> > +     if (port->serial->disconnected)
> > +             goto exit;
>
> Nit: "out_unlock" is more descriptive.
>
> > +
> > +     /* Send 'Close Port' to the device */
> > +     cmd.val = nct_build_cmd(NCT_VCOM_SET_CLOSE_PORT, tport->hw_idx);
> > +     msg.val = cpu_to_le16(0);
> > +     nct_vendor_write(intf, cmd, le16_to_cpu(msg.val));
> > +
> > +exit:
> > +     nct_shutdown_device(port->serial);
> > +     mutex_unlock(&serial_priv->open_close_mutex);
> > +     mutex_unlock(&port->serial->disc_mutex);
> > +}
> > +
> > +static void nct_update_status(struct usb_serial *serial, unsigned char *data)
> > +{
> > +     struct nct_port_status *nps = (struct nct_port_status *)data;
> > +     struct usb_interface *intf = serial->interface;
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     struct nct_tty_port *tport;
> > +     struct tty_struct *tty;
> > +     struct usb_serial_port *port;
> > +     unsigned long flags;
> > +
> > +     if (nps->index >= NCT_MAX_NUM_COM_DEVICES) {
> > +             if (nps->index != NCT_EMPTY_PORT) /* Un-used port */
> > +                     dev_warn(&intf->dev, "receive wrong H/W index\n");
> > +             return;
> > +     }
> > +     if (!(nps->hsr & (NCT_UART_STATE_MSR_MASK | NCT_HSR_CTS)))
> > +             return; /* No state changes */
> > +
> > +     port = serial_priv->port_by_hw_idx[nps->index];
> > +     if (!port)
> > +             return;
> > +
> > +     tport = usb_get_serial_port_data(port);
> > +     if (!tport)
> > +             return;
> > +
> > +     spin_lock_irqsave(&tport->port_lock, flags);
> > +     tport->usr = nps->usr;
> > +     tport->hsr = nps->hsr;
> > +     tport->hcr = nps->hcr;
> > +     spin_unlock_irqrestore(&tport->port_lock, flags);
> > +
> > +     if (serial->disconnected) {
> > +             dev_err(&intf->dev,
> > +                     "device disconnected, skip status update\n");
> > +             return;
> > +     }
> > +
> > +     tty = tty_port_tty_get(&port->port);
> > +     if (!tty)
> > +             return; /* The port has been closed. */
> > +
> > +     if (nps->hsr & NCT_UART_STATE_MSR_MASK) {
> > +             if (nps->hsr & NCT_HSR_DCD) {
> > +                     struct tty_ldisc *ld = tty_ldisc_ref(tty);
> > +
> > +                     if (ld) {
> > +                             if (ld->ops->dcd_change)
> > +                                     ld->ops->dcd_change(tty, 0x01);
> > +                             tty_ldisc_deref(ld);
> > +                     }
> > +                     wake_up_interruptible(&tty->port->open_wait);
> > +             }
>
> Can't you use usb_serial_handle_dcd_change() here?
>
> > +     }
> > +
> > +     /* Flow control */
> > +     if (tty_port_cts_enabled(&port->port)) {
> > +             if ((nps->hsr & NCT_HSR_CTS)) {
> > +                     if (tport->flow_stop_wrt)
> > +                             tport->flow_stop_wrt = false;
> > +             } else {
> > +                     tport->flow_stop_wrt = true;
> > +             }
> > +     }
>
> So this may be needed either if the device stops transmitting when CTS
> is deasserted.
>
> > +
> > +     tty_kref_put(tty);
> > +}
> > +
> > +/*
> > + * nct_usb_serial_read - process incoming bulk-in data.
> > + *
> > + * The device multiplexes all UART ports over a single bulk-in endpoint.
> > + * Each data segment is prefixed with a 4-byte header:
> > + *
> > + *   byte 0: magic (0xa5)
> > + *   byte 1: magic2
> > + *           0x5a = data packet
> > + *           0x5b = status packet
> > + *   bytes 2-3: length and index (little-endian 16-bit)
> > + *           bits [3:0]  = port index (1-based, valid range 1-7; 0 is reserved)
> > + *           bits [15:4] = payload length in bytes
> > + *
> > + * Multiple packets for different ports may be concatenated within a
> > + * single URB. The driver allows a packet's payload to span across URB
> > + * boundaries. In that case, the driver records the remaining length (cur_len)
> > + * and target port (cur_port) to reassemble the data on subsequent URBs.
> > + *
> > + * Status packets (magic2 = 0x5b) carry an array of 4-byte per-port
> > + * status structures instead of UART data:
> > + *
> > + *   byte 0: port index (1-based; 0 = reserved, 0xff = unused entry)
> > + *   byte 1: UART status (USR)
> > + *           bit0: RX data ready
> > + *           bit1: overrun error
> > + *           bit2: parity error
> > + *           bit3: framing error
> > + *           bit4: break detected
> > + *           bit5: TX buffer empty
> > + *           bit6: TX shift register empty
> > + *           bit7: RX FIFO error
> > + *   byte 2: modem status (MSR)
> > + *           bit0: CTS change
> > + *           bit1: DSR change
> > + *           bit2: RI change
> > + *           bit3: DCD change
> > + *           bit4: CTS level
> > + *           bit5: DSR level
> > + *           bit6: RI level
> > + *           bit7: DCD level
> > + *   byte 3: modem control (MCR)
> > + *           bit0: DTR level
> > + *           bit1: RTS level
> > + */
>
> Thanks for adding this.
>
> > +static void nct_usb_serial_read(struct urb *urb)
> > +{
> > +     struct usb_serial_port *port = urb->context;
> > +     struct usb_serial *serial = port->serial;
> > +     struct usb_interface *intf = serial->interface;
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     struct nct_tty_port *tport;
> > +     struct nct_packet_header *hdr = NULL;
> > +     unsigned char *data = urb->transfer_buffer;
> > +     int i, j;
> > +     int actual_len = urb->actual_length;
> > +     int len = 0;
> > +     u8 hdr_idx;
> > +     struct nct_port_status *nps;
> > +     unsigned long flags;
> > +
> > +     if (!urb->actual_length)
> > +             return;
> > +
> > +again:
> > +     spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +     tport = serial_priv->cur_port;
> > +     if (!tport) {
> > +             /*
> > +              * Handle a new data packet (i.e., it is not
> > +              * the remaining data without a header).
> > +              * The packet does not need reassembly at this point.
> > +              */
> > +             for (i = 0; i < urb->actual_length; i++) {
> > +                     if (i + sizeof(struct nct_packet_header) >
> > +                         urb->actual_length) {
> > +                             /* Not enough data for a header */
> > +                             spin_unlock_irqrestore(&serial_priv->serial_lock,
> > +                                                    flags);
> > +                             return;
> > +                     }
> > +
> > +                     hdr = (struct nct_packet_header *)data;
> > +                     /* Decode the header */
> > +
> > +                     /*
> > +                      * Status data is also transmitted via bulk-in
> > +                      * pipe.
> > +                      * Current FW reports a fixed 24-byte status payload.
> > +                      */
> > +                     if (serial_priv->use_bulk_status &&
> > +                         hdr->magic == NCT_HDR_MAGIC &&
> > +                         hdr->magic2 == NCT_HDR_MAGIC_STATUS &&
> > +                         nct_get_hdr_len(hdr) == 24 &&
> > +                         actual_len >= 28) {
> > +                             /*
> > +                              * Notice: actual_len will be decreased,
> > +                              * it is equal to urb->actual_length
> > +                              * only at the beginning.
> > +                              */
> > +
> > +                             /*
> > +                              * Status report.
> > +                              * It should be a standalone package in
> > +                              * one URB.
> > +                              */
> > +                             data += sizeof(struct nct_packet_header);
> > +                             actual_len -= sizeof(struct nct_packet_header);
> > +
> > +                             nps = (struct nct_port_status *)data;
> > +                             if ((actual_len % sizeof(*nps)) != 0) {
> > +                                     dev_err(&intf->dev,
> > +                                             "bad status len: %d\n",
> > +                                             actual_len);
> > +                                     spin_unlock_irqrestore(
> > +                                             &serial_priv->serial_lock,
> > +                                             flags);
> > +                                     return;
> > +                             }
> > +
> > +                             for (j = 0; j < (actual_len / sizeof(*nps));
> > +                                  j++) {
> > +                                     nct_update_status(serial,
> > +                                                       (unsigned char *)nps);
>
> Why do you cast to unsigned char * only to cast back in
> nct_update_status() again?
>
> > +                                     nps++;
> > +                             }
>
> Looks like you fixed the bug I pointed out here (but it's not mentioned
> in the change log) and tried to limit the line lengths by adding line
> breaks (which doesn't necessarily improve readability), but the
> indentation is still six levels.
>
> Did you try to look into adding helper functions to break the parsing up
> and make it more readable?
>
> > +
> > +                             spin_unlock_irqrestore(&serial_priv->serial_lock,
> > +                                                    flags);
> > +                             return;
> > +                     }
> > +
> > +                     hdr_idx = nct_get_hdr_idx(hdr);
> > +                     if (hdr->magic == NCT_HDR_MAGIC &&
> > +                         hdr->magic2 == NCT_HDR_MAGIC2 &&
> > +                         hdr_idx > 0 &&
> > +                         hdr_idx < NCT_MAX_NUM_COM_DEVICES &&
> > +                         nct_get_hdr_len(hdr) <= 512)
> > +                             break;
> > +
> > +                     data++;
> > +                     actual_len--;
> > +                     if (!actual_len) {
> > +                             dev_err(&intf->dev,
> > +                                     "decode serial packet size failed\n");
> > +                             spin_unlock_irqrestore(&serial_priv->serial_lock,
> > +                                                    flags);
> > +                             return;
> > +                     }
> > +             }
> > +             /*
> > +              * Resolve TTY port by the idx in header and ensure it is valid.
> > +              * Record it for two purposes:
> > +              *   (1) If the current packet payload is incomplete, the next URB
> > +              *       continues payload bytes without a new header.
> > +              *   (2) Route received bytes to the correct TTY port.
> > +              */
> > +             port = serial_priv->port_by_hw_idx[nct_get_hdr_idx(hdr)];
> > +             if (!port) {
> > +                     dev_err(&intf->dev,
> > +                             "decode serial packet index failed\n");
> > +                     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > +                     return;
> > +             }
> > +
> > +             tport = usb_get_serial_port_data(port);
> > +             if (!tport) {
> > +                     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > +                     return;
> > +             }
> > +             /*
> > +              * Calculate the data length.
> > +              * Then, check if the length specified in the header matches
> > +              * the data length. If not, it indicates that the data we
> > +              * received spans across two (or more) packets.
> > +              */
> > +             actual_len -= sizeof(struct nct_packet_header);
> > +             data += sizeof(struct nct_packet_header);
> > +             /* actual_len: the data length of the data we got this time */
> > +             if (nct_get_hdr_len(hdr) > actual_len) {
> > +                     /*
> > +                      * It means the length specified in the header (the
> > +                      * custom header) is greater than the length of the
> > +                      * data we received.
> > +                      * Therefore, the data received this time belongs to a
> > +                      * single packet payload segment (i.e. no next header in this URB).
> > +                      */
> > +                     len = actual_len;
> > +                     /*
> > +                      * cur_len: Record how many data does not handle yet
> > +                      */
> > +                     serial_priv->cur_len = nct_get_hdr_len(hdr) - len;
> > +                     /*
> > +                      * Record the current port. When we receive the
> > +                      * remaining data for this packet next time
> > +                      */
> > +                     serial_priv->cur_port = tport;
> > +             } else {
> > +                     /*
> > +                      * The received data crosses packet boundaries (does not belong
> > +                      * to the same header). We only handle data by
> > +                      * the length in header. And we will handle
> > +                      * another packet when 'goto "again" '.
> > +                      */
> > +                     len = nct_get_hdr_len(hdr);
> > +             }
> > +     } else { /* Handling remaining data that crosses packet boundaries */
> > +             if (serial_priv->cur_len > actual_len) {
> > +                     /*
> > +                      * The unhandled part of the data exceeds the data we
> > +                      * received this time. We only handle the data we
> > +                      * have, expecting more data to be received later.
> > +                      */
> > +                     len = actual_len;
> > +             } else {
> > +                     /*
> > +                      * This means the packet has been fully handled.
> > +                      * Clear 'cur_port' as no additional data needs to be
> > +                      * attached to the current packet.
> > +                      */
> > +                     len = serial_priv->cur_len;
> > +                     serial_priv->cur_port = NULL;
> > +             }
> > +             serial_priv->cur_len -= len;
> > +     }
> > +     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > +     tty_insert_flip_string(&port->port, data, len);
> > +     data += len;
> > +     /*
> > +      * Send data to the tty device (according to the port identified above).
> > +      */
> > +     tty_flip_buffer_push(&port->port);
> > +     actual_len -= len;
> > +
> > +     /*
> > +      * It means that the data we received this time contains two or
> > +      * more data packets, so it needs to continue processing the next
> > +      * data packets.
> > +      */
> > +     if (actual_len > 0)
> > +             goto again;
> > +}
> > +
> > +static void nct_process_read_bulk(struct urb *urb)
> > +{
> > +     struct usb_serial_port *port =  urb->context;
>
> nit:                                   ^ double space
>
> > +     struct nct_serial *serial_priv = usb_get_serial_data(port->serial);
> > +     bool stopped = false;
> > +     int status = urb->status;
> > +     int ret;
> > +
> > +     switch (status) {
> > +     case 0:
> > +             nct_usb_serial_read(urb);
> > +             break;
> > +     case -ENOENT:
> > +     case -ECONNRESET:
> > +     case -ESHUTDOWN:
> > +             if (!(status == -ENOENT
> > +                   && serial_priv
>
> serial_priv is not NULL here
>
> > +                   && serial_priv->open_count
> > +                   && !port->serial->disconnected)) {
> > +                     dev_dbg(&port->dev, "urb halted, status=%d\n", status);
> > +                     stopped = true;
>
> Why did you add this? It should not be needed.
>
> > +             }
> > +             break;
> > +     case -EPIPE:
> > +             dev_err(&port->dev, "urb stalled (EPIPE)\n");
> > +             stopped = true;
> > +             break;
> > +     default:
> > +             dev_dbg(&port->dev, "nonzero urb status: %d\n", status);
> > +             break;
> > +     }
> > +
> > +     if (stopped)
> > +             return;
> > +
> > +     ret = usb_submit_urb(urb, GFP_ATOMIC);
> > +     if (ret != 0 && ret != -EPERM)
> > +             dev_err(&port->dev,
> > +                     "failed resubmitting urb, ret=%d\n",
> > +                     ret);
>
> Again, please use bracket around all blocks with multi-line statements
> (throughout).
>
> And please use the following format consistently for errors
>
>         "failed to ...: %d\n", ret
>
> (i.e. ": " instead of ", ret=").
>
> > +}
> > +
> > +static void nct_read_bulk_callback(struct urb *urb)
> > +{
> > +     struct usb_serial_port *port = urb->context;
> > +
> > +     /* Do not resubmit if the urb is being killed */
> > +     if (urb->status == -ESHUTDOWN)
> > +             return;
>
> Why check -ESHUTDOWN twice?
>
> I apparently already pointed this out.
>
> > +
> > +     if (!port || !port->serial)
> > +             return;
>
> Neither will be NULL.
>
> I already said this.
>
> Please don't ignore feedback, no one likes repeating themselves (and
> especially not kernel maintainers).
>
> > +
> > +     if (port->serial->disconnected)
> > +             return;
>
> This should not be needed either.
>
> > +
> > +     /* Processing data */
> > +     nct_process_read_bulk(urb);
> > +}
> > +
> > +static int nct_calc_num_ports(struct usb_serial *serial,
> > +                           struct usb_serial_endpoints *epds)
> > +{
> > +     struct nct_vendor_cmd cmd;
> > +     u8 buf[8];
>
> Move these after the following struct declarations.
>
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     struct usb_interface *intf = serial->interface;
> > +     struct usb_host_interface *iface_desc = intf->cur_altsetting;
> > +     struct usb_endpoint_descriptor *bulk_in = NULL;
> > +     struct usb_endpoint_descriptor *bulk_out = NULL;
> > +     struct usb_endpoint_descriptor *int_in = NULL;
> > +     int ret;
> > +     int i;
> > +     int num_ports;
> > +
> > +     /* Send init command */
> > +     cmd.val = nct_build_cmd(NCT_VCOM_SET_INIT, NCT_VCOM_INDEX_GLOBAL);
> > +     ret = nct_vendor_write(intf, cmd, 0);
> > +     if (ret) {
> > +             dev_err(&intf->dev, "set COM init error\n");
> > +             return ret;
> > +     }
> > +
> > +     /* Get ports' index supported by the device(/FW) */
> > +     cmd.val = nct_build_cmd(NCT_VCOM_GET_PORTS_SUPPORT, NCT_VCOM_INDEX_GLOBAL);
> > +     ret = nct_vendor_read(intf, cmd, buf, 1);
> > +     if (ret) {
> > +             dev_err(&intf->dev,
>
> Line break not needed.
>
> > +                     "get COM port index error\n");
> > +             return ret;
> > +     }
> > +
> > +     if (!serial_priv) {
> > +             serial_priv = kzalloc_obj(*serial_priv, GFP_KERNEL);
> > +             if (!serial_priv)
> > +                     return -ENOMEM;
> > +             spin_lock_init(&serial_priv->serial_lock);
> > +             mutex_init(&serial_priv->open_close_mutex);
> > +             usb_set_serial_data(serial, serial_priv);
> > +     }
>
> This does not belong here in calc_num_ports(), but if you really must
> allocate memory you currently need to use devm_kzalloc() as otherwise it
> may not be freed in all error paths.
>
> But its better to avoid that and defer allocation to attach() if
> you can.
>
> > +     serial_priv->en_device_mask = buf[0];
>
> Can't you just store the mask in the serial port driver data
> temporarily, and retrieve it in attach when you setup the driver data
> (or just query the device again)?
>
> > +     serial_priv->last_assigned_hw_idx = 0; /* Note: hw_idx is based on 1 */
> > +
> > +     for (i = 0, num_ports = 0; i < NCT_MAX_NUM_COM_DEVICES; i++) {
> > +             if (!(buf[0] & BIT(i)))
> > +                     continue;       /* The port is disabled */
> > +
> > +             num_ports++;
> > +     }
> > +
> > +     for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) {
> > +             struct usb_endpoint_descriptor *endpoint =
> > +                     &iface_desc->endpoint[i].desc;
> > +
> > +             if (!bulk_in && usb_endpoint_is_bulk_in(endpoint))
> > +                     bulk_in = endpoint;
> > +             if (!bulk_out && usb_endpoint_is_bulk_out(endpoint))
> > +                     bulk_out = endpoint;
> > +             if (!int_in && usb_endpoint_is_int_in(endpoint))
> > +                     int_in = endpoint;
> > +     }
>
> Use may be able to use usb_find_common_endpoints() here, but I don't
> think you need it either since core will already have stored them in
> epds->bulk_in[0], etc.
>
> > +
> > +     if (!bulk_in || !bulk_out)
> > +             return -ENODEV;
>
> You already set the driver num_bulk_out and num_bulk_in fields so core
> would already have rejected any device which doesn't have at least that
> many endpoints.
>
> > +
> > +     for (i = 0; i < num_ports; i++) {
> > +             epds->bulk_in[i] = bulk_in;
>
> The in endpoints do not need to be set for each port, just the bulk_out
> one so that the write urbs are allocated.
>
> > +             epds->bulk_out[i] = bulk_out;
>
> > +             if (int_in && i == 0)
> > +                     epds->interrupt_in[i] = int_in;
>
> No need for this either.
>
> > +     }
> > +     epds->num_bulk_out = num_ports;
> > +
> > +     return num_ports;
> > +}
> > +
> > +static int nct_attach(struct usb_serial *serial)
> > +{
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +
> > +     if (!serial_priv)
> > +             return -ENODEV;
> > +
> > +     serial_priv->use_bulk_status = true;
> > +     if (serial->port[0] && serial->port[0]->interrupt_in_urb)
>
> No need to check that port[0] exists (e.g. since you require at least
> one endpoint).
>
> > +             serial_priv->use_bulk_status = false;
> > +
> > +     return 0;
> > +}
> > +
> > +static int nct_port_probe(struct usb_serial_port *port)
> > +{
> > +     struct nct_tty_port *tport;
> > +     struct usb_serial *serial = port->serial;
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     unsigned long flags;
> > +     int i, ret;
> > +     unsigned int fifo_state;
> > +
> > +     tport = kzalloc_obj(*tport, GFP_KERNEL);
> > +     if (!tport)
> > +             return -ENOMEM;
> > +
> > +     /* Assign hw_idx */
> > +     mutex_init(&tport->mctrl_mutex);
> > +     spin_lock_init(&tport->port_lock);
> > +
> > +     spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +     for (i = serial_priv->last_assigned_hw_idx + 1;
> > +          i < NCT_MAX_NUM_COM_DEVICES; i++) {
> > +             if ((serial_priv->en_device_mask & (1 << i)) == 0)
> > +                     continue; /* The port is disabled */
> > +
> > +             tport->hw_idx = i;
> > +             serial_priv->last_assigned_hw_idx = i;
> > +             break;
> > +     }
> > +     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
>
> It seems better to set up the port mapping for all ports once at
> attach().
>
> > +
> > +     usb_set_serial_port_data(port, tport);
> > +
> > +     spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +     serial_priv->port_by_hw_idx[tport->hw_idx] = port;
> > +     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
>
> > +     fifo_state = kfifo_initialized(&port->write_fifo);
> > +
> > +     /*
> > +      * Keep the FIFO if it is already initialized.
> > +      */
> > +     if (fifo_state)
> > +             return 0;
> > +     ret = kfifo_alloc(&port->write_fifo, NCT_WRITE_FIFO_SIZE, GFP_KERNEL);
> > +     if (ret) {
> > +             kfree(tport);
> > +             return ret;
> > +     }
>
> This should not be needed either as core allocates the write_fifo for
> all ports with a bulk_out endpoint (which you just set up in
> calc_num_ports()).
>
> > +
> > +     return 0;
> > +}
> > +
> > +static void nct_interrupt_in_callback(struct urb *urb)
> > +{
> > +     struct usb_serial_port *port = urb->context;
> > +     int status = urb->status;
> > +     struct usb_serial *serial = port->serial;
> > +     unsigned char *data = urb->transfer_buffer;
> > +     int retval;
> > +     int i;
> > +     int actual_len = urb->actual_length;
> > +     struct nct_port_status *nps;
>
> Again, please try to approximate reverse xmas style declarations.
>
> > +
> > +     switch (status) {
> > +     case 0:
> > +             /* Success */
> > +             if ((actual_len % sizeof(*nps)) != 0)
> > +                     return;
> > +
> > +             nps = (struct nct_port_status *)data;
> > +             for (i = 0; i < (actual_len / sizeof(*nps)); i++) {
> > +                     nct_update_status(serial, (unsigned char *)nps);
> > +                     nps++;
> > +             }
>
> Looks like this could share a helper function with the bulk_status
> parsing?
>
> > +             break;
> > +
> > +     case -ECONNRESET:
> > +     case -ENOENT:
> > +     case -ESHUTDOWN:
> > +             /* This urb is terminated, clean up */
> > +             return;
> > +     default:
> > +             break;
> > +     }
>
> Nit: add a newline
>
> > +     retval = usb_submit_urb(urb, GFP_ATOMIC);
> > +     if (retval)
> > +             dev_err(&port->dev, "submit intr URB failed, ret=%d\n",
> > +                     retval);
> > +}
> > +
> > +static void nct_disconnect(struct usb_serial *serial)
> > +{
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     unsigned long flags;
> > +
> > +     /* Reset status */
> > +     spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +     serial_priv->open_count = 0;
> > +     serial_priv->cur_port = NULL;
> > +     serial_priv->cur_len = 0;
> > +     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
>
> The device is going away so there's no need to reset any state (and all
> ports would already have been closed).
>
> > +
> > +}
> > +
> > +static void nct_port_remove(struct usb_serial_port *port)
> > +{
> > +     struct nct_tty_port *priv = usb_get_serial_port_data(port);
> > +     struct nct_serial *serial_priv = usb_get_serial_data(port->serial);
> > +     unsigned long flags;
> > +
> > +     if (priv) {
>
> priv is non-NULL here
>
> > +             spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +             if (priv->hw_idx < NCT_MAX_NUM_COM_DEVICES)
>
> hw_idx should never be invalid here, right?
>
> > +                     serial_priv->port_by_hw_idx[priv->hw_idx] = NULL;
> > +             spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > +     }
> > +     kfree(priv);
> > +}
> > +
> > +static void nct_release(struct usb_serial *serial)
> > +{
> > +     struct nct_serial *serial_priv;
> > +
> > +     serial_priv = usb_get_serial_data(serial);
> > +     kfree(serial_priv);
> > +}
> > +
> > +static int nct_suspend(struct usb_serial *serial, pm_message_t message)
> > +{
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     int i;
> > +
> > +     /* Stop all URBs */
> > +     usb_kill_urb(serial->port[0]->read_urb);
> > +     for (i = 0; i < serial->type->num_ports; i++)
> > +             usb_kill_urb(serial->port[i]->write_urb);
>
> The generic implementation uses both write urbs.
>
> > +
> > +     if (!serial_priv->use_bulk_status)
> > +             usb_kill_urb(serial->port[0]->interrupt_in_urb);
>
> But you strictly don't need to implement suspend as you only use URBs
> allocated by core, which core will kill for you on suspend.
>
> > +
> > +     return 0;
> > +}
> > +
> > +static int nct_resume(struct usb_serial *serial)
> > +{
> > +     struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > +     struct usb_interface *intf = serial->interface;
> > +     unsigned long flags;
> > +     int ret;
> > +
> > +     /* Reset driver internal state */
> > +     spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +     serial_priv->cur_port = NULL;
> > +     serial_priv->cur_len = 0;
> > +     spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > +
> > +     /* Resubmit URBs */
> > +     if (!serial_priv->open_count)
> > +             return 0;
> > +
> > +     ret = usb_submit_urb(serial->port[0]->read_urb, GFP_KERNEL);
> > +     if (ret)
> > +             dev_err(&intf->dev, "submit read URB failed, ret=%d\n",
> > +                     ret);
> > +
> > +     if (!serial_priv->use_bulk_status) {
> > +             ret = usb_submit_urb(serial->port[0]->interrupt_in_urb,
> > +                                  GFP_KERNEL);
> > +             if (ret)
> > +                     dev_err(&intf->dev,
> > +                             "submit interrupt URB failed, ret=%d\n",
> > +                             ret);
> > +     }
>
> You may be able to use usb_serial_generic_resume() here instead of open
> coding, which also takes care of restarting writes in case there is
> still data in the fifos.
>
> At least if you are going to use both read urbs back to back.
>
> > +
> > +     return 0;
> > +}
> > +
> > +static struct usb_serial_driver nct_usb_serial_device = {
> > +     .driver = {
> > +             .name =         "nct_mtuart",
> > +     },
> > +     .description =          "Nuvoton USB to serial adapter",
> > +     .id_table =             id_table,
> > +     .num_bulk_in =          1,
> > +     .num_bulk_out =         1,
> > +     .open =                 nct_open,
> > +     .close =                nct_close,
> > +     .write =                nct_serial_write,
> > +     .write_room =           nct_write_room,
> > +     .write_bulk_callback =  usb_serial_generic_write_bulk_callback,
> > +     .prepare_write_buffer = nct_prepare_write_buffer,
> > +     .read_bulk_callback =   nct_read_bulk_callback,
> > +     .read_int_callback =    nct_interrupt_in_callback,
> > +     .chars_in_buffer =      nct_chars_in_buffer,
> > +     .throttle =             nct_rx_throttle,
> > +     .unthrottle =           nct_rx_unthrottle,
> > +     .attach =               nct_attach,
> > +     .calc_num_ports =       nct_calc_num_ports,
> > +     .set_termios =          nct_set_termios,
> > +     .break_ctl =            nct_break,
> > +     .tiocmget =             nct_tiocmget,
> > +     .tiocmset =             nct_tiocmset_helper,
> > +     .disconnect =           nct_disconnect,
> > +     .release =              nct_release,
> > +     .port_probe =           nct_port_probe,
> > +     .port_remove =          nct_port_remove,
> > +     .suspend =              nct_suspend,
> > +     .resume =               nct_resume,
> > +};
> > +
> > +static struct usb_serial_driver * const nct_serial_drivers[] = {
> > +     &nct_usb_serial_device, NULL
> > +};
> > +
> > +module_usb_serial_driver(nct_serial_drivers, id_table);
> > +MODULE_DESCRIPTION("Nuvoton USB to serial adaptor driver");
> > +MODULE_AUTHOR("Sheng-Yuan Huang <syhuang3@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL v2");
>
> > +
>
> Nit: stray newline
>
> Again, I think this looks pretty good over all.
>
> Johan

<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
乾淨無病毒。www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>