On Thu, Sep 05, 2013 at 04:06:43PM +0200, Andrew Lunn wrote:
> Add a driver which supports the following Moxa USB to serial converters:
> * 2 ports : UPort 1250, UPort 1250I
> * 4 ports : UPort 1410, UPort 1450, UPort 1450I
> * 8 ports : UPort 1610-8, UPort 1650-8
> * 16 ports : UPort 1610-16, UPort 1650-16
>
> The UPORT devices don't directy fit the USB serial model. USB serial
> assumes a bulk in/out endpoint pair per serial port. Thus a dual port
> USB serial device is expected to have two bulk in/out pairs. The Moxa
> UPORT only has one pair for data transfer and places a header on each
> transfer over the endpoint indicating for which port the transfer
> relates to. There is a second endpoint pair for events, just as modem
> control lines changing state, setting baud rates etc. Again, a
> multiplexing header is used on these endpoints.
>
> This difference to the model results in some additional code which
> other drivers don't have:
>
> Some ports need to have a kfifo explicitily allocated since the
> framework does not allocate one if there is no associated endpoints.
> The framework will however free it on unload of the module.
>
> All data transfers are made on port0, yet the locks are taken on PortN.
> urb->context points to PortN, even though the URL is for port0.
You probably meant "URB" here.
> Where possible, code from the generic driver is called. However
> mxuport_process_read_urb_data() is mostly a cut/paste of
> usb_serial_generic_process_read_urb().
>
> The driver will attempt to load firmware from userspace and compare
> the available version and the running version. If the available
> version is newer, it will be download into RAM of the device and
> started. This is optional and the driver appears to work O.K. with
> older firmware in the devices ROM.
>
> This driver is based on the MOXA driver and retains MOXAs copyright.
>
> Signed-off-by: Andrew Lunn <andrew@xxxxxxx>
>
> ---
>
> Changes since v1:
>
> Remove debug module parameter.
> Add missing \n to dev_dbg() strings.
> Consistent dev_dbg("%s - <message in lowercase>\n", __func__);
> Don't log failed allocations.
> Remove defensive checks for NULL mx_port.
> Use snprintf() instead of sprintf().
> Use port->icount and usb_serial_generic_get_icount().
> Break firmware download into smaller functions.
> Don't DMA to/from buffers on the stack.
> Use more of the generic write functions.
> Make use of port_probe() and port_remove().
> Indent the device structure.
> Minor cleanups in mxuport_close()
> __u8 -> u8, __u16 -> u16.
> remove mxuport_wait_until_sent() and implemented mxuport_tx_empty()
> Remove mxuport_write_room() and use the generic equivelent.
> Remove unneeded struct mx_uart_config members
> Make use of generic functions for receiving data and events.
> Name mxport consistently.
> Use usb_serial_generic_tiocmiwait()
> Let line discipline handle TCFLSH ioctl.
> Import contents of mxuport.h into mxuport.c
> Use shifts and ORs to create version number.
> Use port_number, not minor
>
> drivers/usb/serial/Kconfig | 29 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/mxuport.c | 1771 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1801 insertions(+)
> create mode 100644 drivers/usb/serial/mxuport.c
Thanks for addressing most of my comments on v1 (I repeat some below,
though). The diffstat shows you've shaved of some 420 lines. That's a
good start. :)
In general the code looks good, but there are still a few things that
should be fixed or improved. Details below.
[...]
> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> new file mode 100644
> index 0000000..fe75ef1
> --- /dev/null
> +++ b/drivers/usb/serial/mxuport.c
> @@ -0,0 +1,1771 @@
> +/*
> + * mx-uport.c - MOXA UPort series driver
> + *
> + * Copyright (c) 2006 Moxa Technologies Co., Ltd.
> + * Copyright (c) 2013 Andrew Lunn <andrew@xxxxxxx>
> + *
> + * 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.
> + *
> + * Supports the following Moxa USB to serial converters:
> + * 2 ports : UPort 1250, UPort 1250I
> + * 4 ports : UPort 1410, UPort 1450, UPort 1450I
> + * 8 ports : UPort 1610-8, UPort 1650-8
> + * 16 ports : UPort 1610-16, UPort 1650-16
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/firmware.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/tty_flip.h>
> +#include <linux/uaccess.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +#include <asm/unaligned.h>
> +
> +/* Definitions for driver info */
> +#define DRIVER_AUTHOR \
> + "<support@xxxxxxxx>" \
> + "Andrew Lunn <andrew@xxxxxxx>"
> +
> +/* Definitions for the vendor ID and device ID */
> +#define MX_USBSERIAL_VID 0x110A
> +#define MX_UPORT1250_PID 0x1250
> +#define MX_UPORT1251_PID 0x1251
> +#define MX_UPORT1410_PID 0x1410
> +#define MX_UPORT1450_PID 0x1450
> +#define MX_UPORT1451_PID 0x1451
> +#define MX_UPORT1618_PID 0x1618
> +#define MX_UPORT1658_PID 0x1658
> +#define MX_UPORT1613_PID 0x1613
> +#define MX_UPORT1653_PID 0x1653
> +
> +/* Definitions for USB info */
> +#define HEADER_SIZE 4
> +#define MAX_EVENT_LENGTH 8
> +#define DOWN_BLOCK_SIZE 64
> +
> +/* Definitions for firmware info */
> +#define VER_ADDR_1 0x20
> +#define VER_ADDR_2 0x24
> +#define VER_ADDR_3 0x28
> +
> +/* Definitions for USB vendor request */
> +#define RQ_VENDOR_NONE 0x00
> +#define RQ_VENDOR_SET_BAUD 0x01 /* set baud rate */
> +#define RQ_VENDOR_SET_LINE 0x02 /* set line status */
> +#define RQ_VENDOR_SET_CHARS 0x03 /* set Xon/Xoff chars */
> +#define RQ_VENDOR_SET_RTS 0x04 /* set RTS */
> +#define RQ_VENDOR_SET_DTR 0x05 /* set DTR */
> +#define RQ_VENDOR_SET_XONXOFF 0x06 /* set auto Xon/Xoff */
> +#define RQ_VENDOR_SET_RX_HOST_EN 0x07 /* set RX host enable */
> +#define RQ_VENDOR_SET_OPEN 0x08 /* set open/close port */
> +#define RQ_VENDOR_PURGE 0x09 /* purge Rx/Tx buffer */
> +#define RQ_VENDOR_SET_MCR 0x0A /* set MCR register */
> +#define RQ_VENDOR_SET_BREAK 0x0B /* set Break signal */
> +
> +#define RQ_VENDOR_START_FW_DOWN 0x0C /* start firmware download */
> +#define RQ_VENDOR_STOP_FW_DOWN 0x0D /* stop firmware download */
> +#define RQ_VENDOR_QUERY_FW_READY 0x0E /* query if new firmware ready */
> +
> +#define RQ_VENDOR_SET_FIFO_DISABLE 0x0F /* set fifo disable */
> +#define RQ_VENDOR_SET_INTERFACE 0x10 /* set interface */
> +#define RQ_VENDOR_SET_HIGH_PERFOR 0x11 /* set hi-performance */
> +
> +#define RQ_VENDOR_ERASE_BLOCK 0x12 /* erase flash block */
> +#define RQ_VENDOR_WRITE_PAGE 0x13 /* write flash page */
> +#define RQ_VENDOR_PREPARE_WRITE 0x14 /* prepare write flash */
> +#define RQ_VENDOR_CONFIRM_WRITE 0x15 /* confirm write flash */
> +#define RQ_VENDOR_LOCATE 0x16 /* locate the device */
> +
> +#define RQ_VENDOR_START_ROM_DOWN 0x17 /* start firmware download */
> +#define RQ_VENDOR_ROM_DATA 0x18 /* rom file data */
> +#define RQ_VENDOR_STOP_ROM_DOWN 0x19 /* stop firmware download */
> +#define RQ_VENDOR_FW_DATA 0x20 /* firmware data */
> +
> +#define RQ_VENDOR_RESET_DEVICE 0x23 /* Try to reset the device */
> +#define RQ_VENDOR_QUERY_FW_CONFIG 0x24
> +
> +#define RQ_VENDOR_GET_VERSION 0x81 /* get firmware version */
> +#define RQ_VENDOR_GET_PAGE 0x82 /* read flash page */
> +#define RQ_VENDOR_GET_ROM_PROC 0x83 /* get ROM process state */
> +
> +#define RQ_VENDOR_GET_INQUEUE 0x84 /* data remaining in input buffer */
> +#define RQ_VENDOR_GET_OUTQUEUE 0x85 /* data remaining in output buffer */
> +
> +#define RQ_VENDOR_GET_MSR 0x86
> +
> +/* Definitions for UPort event type */
> +#define UPORT_EVENT_NONE 0 /* None */
> +#define UPORT_EVNET_TXBUF_THRESHOLD 1 /* Tx buffer threshold */
> +#define UPORT_EVNET_SEND_NEXT 2 /* Send next */
> +#define UPORT_EVNET_MSR 3 /* Modem status */
> +#define UPORT_EVNET_LSR 4 /* Line status */
> +#define UPORT_EVNET_MCR 5 /* Modem control */
> +
> +/* Definitions for serial event type */
> +#define SERIAL_EV_CTS 0x0008 /* CTS changed state */
> +#define SERIAL_EV_DSR 0x0010 /* DSR changed state */
> +#define SERIAL_EV_RLSD 0x0020 /* RLSD changed state */
> +
> +/* Definitions for line control of communication */
> +#define MX_WORDLENGTH_5 5
> +#define MX_WORDLENGTH_6 6
> +#define MX_WORDLENGTH_7 7
> +#define MX_WORDLENGTH_8 8
> +
> +#define MX_PARITY_NONE 0
> +#define MX_PARITY_ODD 1
> +#define MX_PARITY_EVEN 2
> +#define MX_PARITY_MARK 3
> +#define MX_PARITY_SPACE 4
> +
> +#define MX_STOP_BITS_1 0
> +#define MX_STOP_BITS_1_5 1
> +#define MX_STOP_BITS_2 2
> +
> +#define MX_NO_FLOWCTRL 0x0
> +#define MX_HW_FLOWCTRL 0x1
> +#define MX_XON_FLOWCTRL 0x2
> +#define MX_XOFF_FLOWCTRL 0x4
> +
> +#define MX_INT_RS232 0
> +#define MX_INT_2W_RS485 1
> +#define MX_INT_RS422 2
> +#define MX_INT_4W_RS485 3
> +
> +/* Definitions for holding reason */
> +#define MX_WAIT_FOR_CTS 0x0001
> +#define MX_WAIT_FOR_DSR 0x0002
> +#define MX_WAIT_FOR_DCD 0x0004
> +#define MX_WAIT_FOR_XON 0x0008
> +#define MX_WAIT_FOR_START_TX 0x0010
> +#define MX_WAIT_FOR_UNTHROTTLE 0x0020
> +#define MX_WAIT_FOR_LOW_WATER 0x0040
> +#define MX_WAIT_FOR_SEND_NEXT 0x0080
> +
> +/* This structure holds all of the local port information */
> +struct mxuport_port {
> + int flags; /* for async_struct and serial_struct flags
> + field */
> + int set_B0;
> +
> + u8 mcr_state; /* last MCR state */
> +
> + u8 msr_state; /* last MSR state */
> + u8 lsr_state; /* last LSR state */
> + unsigned long hold_reason;
You go to great lengths to update this hold_reason but I can't see that
you ever use it?
> +
> + struct usb_serial_port *port; /* loop back to the owner of this
> + object */
> + struct mx_uart_config *uart_cfg; /* configuration of UART */
> +};
> +
> +/* Configuration of UART */
> +struct mx_uart_config {
> + u8 data_bits; /* 5..8 - data bits per character */
> + u8 parity; /* parity settings */
> + u8 stop_bits; /* stop bits settings */
> + u8 flow_ctrl; /* flow control settings */
> + int interface; /* interface is defined */
This description isn't very clear.
> +};
> +
> +/* Table of devices that work with this driver */
> +static struct usb_device_id mxuport_idtable[] = {
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1250_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1251_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1410_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1450_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1451_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1618_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1658_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1613_PID)},
> + {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1653_PID)},
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, mxuport_idtable);
> +
> +/*
> + * mxuport_prepare_write_buffer - fill in the buffer, ready for
> + * sending
> + *
> + * Add a four byte header containing the port number and the number of
> + * bytes of data in the message. Return the number of bytes in the
> + * buffer.
> + */
> +static int mxuport_prepare_write_buffer(struct usb_serial_port *port,
> + void *dest, size_t size)
> +{
> + unsigned char *buf = dest;
> + int count;
> +
> + count = kfifo_out_locked(&port->write_fifo, buf + 4, size - 4,
> + &port->lock);
> +
> + put_unaligned_be16(port->port_number, buf);
> + put_unaligned_be16(count, buf + 2);
> +
> + dev_dbg(&port->dev, "%s - size %zd count %d\n", __func__,
> + size, count);
> +
> + return count + 4;
> +}
> +
> +
> +/*
> + * mxuport_control_callback
> + *
> + * This is the callback function for when we have finished sending
> + * control urb on the control pipe.
> + */
> +static void mxuport_control_callback(struct urb *urb)
> +{
> + struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
You should pass the usb_serial_port (or usb_serial) rather than the
ctrlrequest as context in order to get uniform and more informative
error messages if anything goes wrong. You can still access the
ctrlrequest as urb->setup_packet.
> + struct device *dev = &urb->dev->dev;
Use &port->dev (or &serial->interface->dev).
> +
> + switch (urb->status) {
> + case 0:
> + break;
> +
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + /* This urb is terminated, clean up */
> + dev_dbg(dev, "%s - urb shutting down with status: %d\n",
> + __func__, urb->status);
> + break;
> +
> + default:
> + dev_err(dev, "%s - nonzero control status received: %d\n",
> + __func__, urb->status);
> + break;
> + }
> +
> + kfree(req);
> + usb_free_urb(urb);
You don't free the transfer_buffer even though you allow control
requests to pass a buffer. You don't use that part of the interface at
the moment, so either disallow buffers to be passed below or make sure
to free them here.
> +}
> +
> +/*
> + * mxuport_send_aysnc_ctrl_urb - send asynchronously a vendor request with
> + * data
> + *
> + * This function writes the given buffer out to the control pipe, but
> + * does not wait around for it to complete.
> + */
> +static void mxuport_send_async_ctrl_urb(struct usb_device *udev,
> + u8 request,
> + u16 value,
> + u16 index, u8 *data, size_t size)
You only call this function from throttle/unthrottle. You should
probably pass the usb_serial_port rather than the usb_device to be used
for error messages and urb context (at least unless you see any
immediate uses of non-port-specific control requests, but then you
should pass the usb_serial struct instead).
Make sure to decide whether to allow the data buffer to be passed or
not, as mentioned above.
> +{
> + struct usb_ctrlrequest *req;
> + int err;
> + struct urb *urb = usb_alloc_urb(0, GFP_ATOMIC);
Please split up the definition and allocation and add a newline here.
If you think this could be useful in other contexts than
throttle/unthrottle where you currently call it while holding a
spinlock, I'd pass the memory flag as a parameter (instead of always
using GFP_ATOMIC).
> + if (!urb)
> + return;
> +
> + req = kmalloc(sizeof(struct usb_ctrlrequest), GFP_ATOMIC);
> + if (!req) {
> + usb_free_urb(urb);
> + return;
> + }
> +
> + req->bRequestType = USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE;
> + req->bRequest = request;
> + req->wValue = cpu_to_le16(value);
> + req->wIndex = cpu_to_le16(index);
> + req->wLength = cpu_to_le16(size);
> +
> + usb_fill_control_urb(urb,
> + udev,
> + usb_sndctrlpipe(udev, 0),
> + (void *)req,
> + data, size, mxuport_control_callback, req);
As mentioned above, data is currently never freed.
> +
> + err = usb_submit_urb(urb, GFP_ATOMIC);
> + if (err < 0) {
> + dev_dbg(&udev->dev,
> + "%s - error submitting the control message: status=%d\n",
> + __func__, err);
> + kfree(req);
> + usb_free_urb(urb);
> + }
> +}
> +
> +/*
> + * mxuport_recv_ctrl_urb - receive vendor request
> + *
> + * This function reads the given buffer in from the control pipe.
> + */
> +static int mxuport_recv_ctrl_urb(struct usb_device *udev,
> + u8 request,
> + u16 value, u16 index, u8 *data, size_t size)
> +{
Pass the usb_serial struct instead of usb_device and use the interface
device in the error messages below.
> + int status = usb_control_msg(udev,
> + usb_rcvctrlpipe(udev, 0),
> + request,
> + (USB_DIR_IN | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE), value, index,
> + data, size,
> + HZ * USB_CTRL_GET_TIMEOUT);
Please split definition and initialisation here and below.
You don't want to use HZ here as USB_CTRL_GET_TIMEOUT is already
specified in ms.
> +
> + if (status < 0) {
> + dev_dbg(&udev->dev, "%s - recv usb_control_msg failed. (%d)\n",
> + __func__, status);
> + return status;
> + }
> +
> + if (status != size) {
> + dev_dbg(&udev->dev,
> + "%s - wanted to recv %zd, but only read %d\n",
> + __func__, size, status);
> + return -ECOMM;
> + }
> +
> + return status;
> +}
> +
> +/*
> + * mxuport_send_ctrl_data_urb - send vendor request with data
> + *
> + * This function writes the given buffer out to the control pipe.
> + */
> +static int mxuport_send_ctrl_data_urb(struct usb_device *udev,
> + u8 request,
> + u16 value, u16 index,
> + u8 *data, size_t size)
> +{
Pass the usb_serial struct instead of usb_device and use the interface
device in the error messages below.
> + int status = usb_control_msg(udev,
> + usb_sndctrlpipe(udev, 0),
> + request,
> + (USB_DIR_OUT | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE), value, index,
> + data, size,
> + HZ * USB_CTRL_SET_TIMEOUT);
Don't use HZ here either.
> + if (status < 0) {
> + dev_dbg(&udev->dev, "%s - send usb_control_msg failed. (%d)\n",
> + __func__, status);
> + return status;
> + }
> +
> + if (status != size) {
> + dev_dbg(&udev->dev,
> + "%s - wanted to write %zd, but only wrote %d\n",
> + __func__, size, status);
> + return -ECOMM;
> + }
> +
> + return status;
> +}
> +
> +/*
> + * mxuport_send_ctrl_urb - send vendor request without any data
> + *
> + * This function writes the given request out to the control pipe.
> + */
> +static int mxuport_send_ctrl_urb(struct usb_device *udev,
> + u8 request, u16 value, u16 index)
> +{
> + return mxuport_send_ctrl_data_urb(udev, request, value, index,
> + NULL, 0);
> +}
> +
> +/*
> + * mxuport_throttle - throttle function of driver
> + *
> + * This function is called by the tty driver when it wants to stop the
> + * data being read from the port. Since all the data comers over one
> + * bulk in endpoint, we cannot stop submitting urbs by setting
> + * port->throttle. Instead tell the device to stop sending us data for
> + * the port.
> + */
> +static void mxuport_throttle(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct usb_serial *serial = port->serial;
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + spin_lock_irqsave(&port->lock, flags);
> + mxport->hold_reason |= MX_WAIT_FOR_UNTHROTTLE;
> +
> + mxuport_send_async_ctrl_urb(serial->dev,
> + RQ_VENDOR_SET_RX_HOST_EN,
> + 0, port->port_number, NULL, 0);
> + spin_unlock_irqrestore(&port->lock, flags);
> +}
I don't see you ever checking for MX_WAIT_FOR_UNTHROTTLE. Why not get
rid of it? Then you should be able to use the synchronous interface to
disable rx for the port in question, and also get rid of the
asynchronous control interface above.
> +
> +/*
> + * mxuport_unthrottle - unthrottle function of driver
> + *
> + * This function is called by the tty driver when it wants to resume
> + * the data being read from the port. Tell the device it can resume
> + * sending us received data from the port.
> + */
> +static void mxuport_unthrottle(struct tty_struct *tty)
> +{
> +
> + struct usb_serial_port *port = tty->driver_data;
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + spin_lock_irqsave(&port->lock, flags);
> + mxport->hold_reason &= ~MX_WAIT_FOR_UNTHROTTLE;
> +
> + mxuport_send_async_ctrl_urb(serial->dev,
> + RQ_VENDOR_SET_RX_HOST_EN,
> + 1, port->port_number, NULL, 0);
> + spin_unlock_irqrestore(&port->lock, flags);
> +}
See above.
> +
> +/*
> + * mxuport_process_read_urb_chunk
> + *
> + * Processes one chunk of data received for a port. If the port is not
> + * open, discard the data. Otherwise pass it onto the tty
> + * layer. Mostly a copy of usb_serial_generic_process_read_urb().
> + */
You actually never check that the port is indeed open, which is
something you'd want to do (although perhaps below in process_read_urb
instead).
> +static void mxuport_process_read_urb_data(struct usb_serial_port *port,
> + char *ch, int rcv_len)
> +{
> + int i;
> +
> + if (!port->port.console || !port->sysrq)
Missing braces.
> + tty_insert_flip_string(&port->port, ch, rcv_len);
> + else {
> + for (i = 0; i < rcv_len; i++, ch++) {
> + if (!usb_serial_handle_sysrq_char(port, *ch))
> + tty_insert_flip_char(&port->port, *ch,
> + TTY_NORMAL);
> + }
> + }
> + tty_flip_buffer_push(&port->port);
> +}
> +
> +/*
> + * mxuport_update_recv_event - Process received events
> + *
> + * When something interesting happens, modem control lines XON/XOFF
> + * etc, the device sends an event. Process these events.
> + */
> +static void mxuport_process_read_urb_event(struct usb_serial_port *port,
> + unsigned char *buf,
> + unsigned long event)
> +{
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + unsigned long rcv_msr_event, rcv_msr_hold;
> + unsigned long lsr_event, flags;
> + unsigned long mcr_event;
Use unsigned char instead of long for most of these.
> +
> + dev_dbg(&port->dev, "%s - receive event : %ld\n",
> + __func__, event);
> +
> + switch (event) {
> + case UPORT_EVNET_SEND_NEXT:
> + spin_lock_irqsave(&port->lock, flags);
> + if (mxport->hold_reason & MX_WAIT_FOR_SEND_NEXT)
> + mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
> + spin_unlock_irqrestore(&port->lock, flags);
> + break;
> +
> + case UPORT_EVNET_MSR:
> + rcv_msr_event = get_unaligned_be16(buf);
> + rcv_msr_hold = ((unsigned long)buf[2] & 0xF0);
No need for cast.
> +
> + dev_dbg(&port->dev, "%s - current MSR status = 0x%x\n",
> + __func__, mxport->msr_state);
> +
> + /* Update hold reason */
> + if (rcv_msr_event != 0) {
> + spin_lock_irqsave(&port->lock, flags);
> +
> + if (!(rcv_msr_hold & UART_MSR_CTS)) {
> + mxport->hold_reason |= MX_WAIT_FOR_CTS;
> + mxport->msr_state &= ~UART_MSR_CTS;
> + dev_dbg(&port->dev, "%s - CTS low\n",
> + __func__);
> + } else {
> + mxport->hold_reason &= ~MX_WAIT_FOR_CTS;
> + mxport->msr_state |= UART_MSR_CTS;
> + dev_dbg(&port->dev, "%s - CTS high\n",
> + __func__);
> + }
> +
> + if (!(rcv_msr_hold & UART_MSR_DSR)) {
> + mxport->hold_reason |= MX_WAIT_FOR_DSR;
> + mxport->msr_state &= ~UART_MSR_DSR;
> + dev_dbg(&port->dev, "%s - DSR low\n",
> + __func__);
> + } else {
> + mxport->msr_state |= UART_MSR_DSR;
> + mxport->hold_reason &= ~MX_WAIT_FOR_DSR;
> + dev_dbg(&port->dev, "%s - DSR high\n",
> + __func__);
> + }
> +
> + if (!(rcv_msr_hold & UART_MSR_DCD)) {
> + mxport->hold_reason |= MX_WAIT_FOR_DCD;
> + mxport->msr_state &= ~UART_MSR_DCD;
> + dev_dbg(&port->dev, "%s - DCD low\n",
> + __func__);
> + } else {
> + mxport->msr_state |= UART_MSR_DCD;
> + mxport->hold_reason &= ~MX_WAIT_FOR_DCD;
> + dev_dbg(&port->dev, "%s - DCD high\n",
> + __func__);
> + }
> + spin_unlock_irqrestore(&port->lock, flags);
> + }
> +
> + /* Update MSR status */
> + if (rcv_msr_event &
> + (SERIAL_EV_CTS | SERIAL_EV_DSR | SERIAL_EV_RLSD)) {
> + spin_lock_irqsave(&port->lock, flags);
> +
> + if (rcv_msr_event & SERIAL_EV_CTS) {
> + port->icount.cts++;
> + dev_dbg(&port->dev, "%s - CTS change\n",
> + __func__);
> + }
> +
> + if (rcv_msr_event & SERIAL_EV_DSR) {
> + port->icount.dsr++;
> + dev_dbg(&port->dev, "%s - DSR change\n",
> + __func__);
> + }
> +
> + if (rcv_msr_event & SERIAL_EV_RLSD) {
> + port->icount.dcd++;
> + dev_dbg(&port->dev, "%s - DCD change\n",
> + __func__);
> + }
> + wake_up_interruptible(&port->port.delta_msr_wait);
> + spin_unlock_irqrestore(&port->lock, flags);
> + }
> + break;
> +
> + case UPORT_EVNET_LSR:
> + lsr_event = (unsigned long)buf[2] & 0xFF;
No need for cast or mask.
> +
> + spin_lock_irqsave(&port->lock, flags);
> +
> + if (lsr_event & UART_LSR_BI) {
> + port->icount.brk++;
> + dev_dbg(&port->dev, "%s - break error\n", __func__);
> + }
> +
> + if (lsr_event & UART_LSR_FE) {
> + port->icount.frame++;
> + dev_dbg(&port->dev, "%s - frame error\n", __func__);
> + }
> +
> + if (lsr_event & UART_LSR_PE) {
> + port->icount.parity++;
> + dev_dbg(&port->dev, "%s - parity error\n", __func__);
> + }
> +
> + if (lsr_event & UART_LSR_OE) {
> + port->icount.overrun++;
> + dev_dbg(&port->dev, "%s - overrun error\n", __func__);
> + }
> +
> + mxport->lsr_state = lsr_event;
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + break;
> +
> + case UPORT_EVNET_MCR:
> + mcr_event = (unsigned long)buf[0] & 0xFF;
No need for cast or mask.
> +
> + spin_lock_irqsave(&port->lock, flags);
> + if (mcr_event & 0x40)
Use a define for the constant.
> + mxport->hold_reason |= MX_WAIT_FOR_XON;
> + else
> + mxport->hold_reason &= ~MX_WAIT_FOR_XON;
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> +
> + break;
> +
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * mxuport_process_read_urb
> + *
> + * This is called when we have received data on the bulk in
> + * endpoint. The urb can contain serial data or events for a number of
> + * ports. Loop over the data chunk for chunk, demultiplexing.
> + */
> +static void mxuport_process_read_urb(struct urb *urb)
> +{
> + struct usb_serial_port *port = urb->context;
> + struct usb_serial_port *demux_port;
> + struct usb_serial *serial = port->serial;
> + unsigned char *data = urb->transfer_buffer;
> + unsigned char *end = data + urb->actual_length;
> + struct device *dev = &urb->dev->dev;
You don't need this, use &port->dev for debugging below.
> + u16 rcv_port, rcv_len, rcv_event;
> + char *ch;
> +
> + if (urb->actual_length < HEADER_SIZE) {
> + dev_dbg(dev, "%s - read bulk callback with no data\n",
> + __func__);
> + return;
> + }
> +
> + while (data < end) {
> + rcv_port = get_unaligned_be16(data);
> + ch = data + HEADER_SIZE;
> + demux_port = serial->port[rcv_port];
You'd want to make sure the demux_port is open here.
> + if (port == serial->port[0]) {
> + /* Received serial data */
> + rcv_len = get_unaligned_be16(data + 2);
> + mxuport_process_read_urb_data(demux_port, ch, rcv_len);
> + data += (HEADER_SIZE + rcv_len);
> + } else if (port == serial->port[1]) {
> + /* Events reports */
> + rcv_event = get_unaligned_be16(data + 2);
> + mxuport_process_read_urb_event(demux_port, ch,
> + rcv_event);
> + data += MAX_EVENT_LENGTH;
> + } else
> + return;
This should never happen.
> + }
You probably want to rewrite this loop in order to avoid accessing data
beyond the end of the buffer for example if it has been corrupted (you
trust rcv_len blindly, and assume all but the first packet have a
complete header and that event packets are well-formed).
> +}
> +
> +/*
> + * mxuport_tx_empty - Check is the TX queue is empty
s/is/if/
> + *
> + * Ask the device how many bytes it has queued to be sent out. If
> + * there are none, return true.
> + */
> +static bool mxuport_tx_empty(struct usb_serial_port *port)
> +{
> + struct usb_device *udev = port->serial->dev;
> + bool is_empty = true;
> + unsigned long txlen;
> + unsigned char *len_buf;
> + int err;
> +
> + len_buf = kzalloc(4, GFP_KERNEL);
> + if (!len_buf)
> + goto out;
> +
> + err = mxuport_recv_ctrl_urb(udev,
> + RQ_VENDOR_GET_OUTQUEUE,
> + 0, port->port_number,
> + len_buf, 4);
> + if (err < 0)
> + goto out;
> +
> + txlen = get_unaligned_be32(len_buf);
> + dev_dbg(&port->dev, "%s - tx len = %ld\n", __func__, txlen);
> +
> + if (txlen != 0)
> + is_empty = false;
> +
> +out:
> + kfree(len_buf);
> + return is_empty;
> +}
> +
> +/*
> + * mxuport_init_port - Initialize the port
> + *
> + * Initialize one port, by enabling its FIFO, set to RS-232 mode, open
> + * the port and enable reception.
> + */
> +static int mxuport_init_port(struct usb_serial_port *port)
> +{
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + int err = 0;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + /* Send vendor request - set FIFO (Enable) */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_FIFO_DISABLE,
> + 0, port->port_number);
> + if (err)
> + return err;
> +
> + /* Send vendor request - set transmition mode (Hi-Performance) */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_HIGH_PERFOR,
> + 0, port->port_number);
> + if (err)
> + return err;
> +
> + /* Send vendor request - set interface (RS-232) */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_INTERFACE,
> + mxport->uart_cfg->interface,
> + port->port_number);
> + if (err)
> + return err;
> +
> + /* Send vendor request - port open */
> +
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_OPEN,
> + 1, port->port_number);
> + if (err)
> + return err;
> +
> + /* Send vendor request - set receive host (enable) */
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RX_HOST_EN,
> + 1, port->port_number);
> + return err;
> +}
Shouldn't this be split up over port_probe and open, where you initialise
(first three requests) at probe, but enable reception (last two
requests) at open?
> +
> +/*
> + * mxuport_change_port_settings - Reconfigure the port based on the tty
> + * settings
> + *
> + * Based on the ttys termios settings, send commands to configure the
> + * port as required.
> + */
> +static int mxuport_change_port_settings(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct mxuport_port *mxport)
> +{
> + struct usb_serial *serial = port->serial;
> + struct mx_uart_config config;
> + int baud, err, status;
> + unsigned int cflag;
> + unsigned char *buf;
> + char xon = START_CHAR(tty);
> + char xoff = STOP_CHAR(tty);
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + if (!tty) {
> + dev_dbg(&port->dev, "%s - no tty structures\n", __func__);
> + return 0;
> + }
This can never happen (and you already dereferenced above).
> +
> + cflag = tty->termios.c_cflag;
> + memset(&config, 0, sizeof(config));
> +
> + /* initial local uart config */
> + config.interface = mxport->uart_cfg->interface;
> +
> + /* Set data bit of termios */
> + switch (cflag & CSIZE) {
> + case CS5:
> + config.data_bits = MX_WORDLENGTH_5;
> + dev_dbg(&port->dev, "%s - data bits = 5\n", __func__);
> + break;
> + case CS6:
> + config.data_bits = MX_WORDLENGTH_6;
> + dev_dbg(&port->dev, "%s - data bits = 6\n", __func__);
> + break;
> + case CS7:
> + config.data_bits = MX_WORDLENGTH_7;
> + dev_dbg(&port->dev, "%s - data bits = 7\n", __func__);
> + break;
> + case CS8:
> + default:
> + config.data_bits = MX_WORDLENGTH_8;
> + dev_dbg(&port->dev, "%s - data bits = 8\n", __func__);
> + break;
> + }
> +
> + /* Set parity of termios */
> + if (cflag & PARENB) {
> + if (cflag & CMSPAR) {
> + if (cflag & PARODD) {
> + config.parity = MX_PARITY_MARK;
> + dev_dbg(&port->dev, "%s - parity = mark\n",
> + __func__);
> + } else {
> + config.parity = MX_PARITY_SPACE;
> + dev_dbg(&port->dev, "%s - parity = space\n",
> + __func__);
> + }
> + } else if (cflag & PARODD) {
> + config.parity = MX_PARITY_ODD;
> + dev_dbg(&port->dev, "%s - parity = odd\n", __func__);
> + } else {
> + config.parity = MX_PARITY_EVEN;
> + dev_dbg(&port->dev, "%s - parity = even\n", __func__);
> + }
> + } else {
> + config.parity = MX_PARITY_NONE;
> + dev_dbg(&port->dev, "%s - parity = none\n", __func__);
> + }
> +
> + /* Set stop bit of termios */
> + if (cflag & CSTOPB) {
> + config.stop_bits = MX_STOP_BITS_2;
> + dev_dbg(&port->dev, "%s - stop bits = 2\n", __func__);
> + } else {
> + config.stop_bits = MX_STOP_BITS_1;
> + dev_dbg(&port->dev, "%s - stop bits = 1\n", __func__);
> + }
> +
> + buf = kmalloc(4, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
You allocated three buffers in this function. Why not simply allocate
one at the beginning and add a common error path for deallocation (e.g.
goto err_buf)? It should make it easier to follow what's going on.
> +
> + /* Submit the vendor request */
> + buf[0] = (unsigned char)config.data_bits;
> + buf[1] = (unsigned char)config.parity;
> + buf[2] = (unsigned char)config.stop_bits;
Casts not needed.
> + buf[3] = 0;
> +
> + status = mxuport_send_ctrl_data_urb(serial->dev, RQ_VENDOR_SET_LINE,
> + 0, port->port_number,
> + buf, 4);
> +
> + kfree(buf);
> +
> + if (status != 4)
> + return status;
> +
> + /* Flow control settings */
> + config.flow_ctrl = 0;
> +
> + /* H/W flow control settings */
> + if ((cflag & CRTSCTS) && (cflag & CBAUD)) {
> + config.flow_ctrl |= MX_HW_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - RTS/CTS is enabled\n", __func__);
> + } else {
> + config.flow_ctrl &= ~MX_HW_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - RTS/CTS is disabled\n", __func__);
> + }
> +
> + /* Submit the vendor request */
> + if (config.flow_ctrl & MX_HW_FLOWCTRL) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RTS,
> + 0x2, port->port_number);
> + if (err)
> + return err;
> + } else {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RTS,
> + 0x1, port->port_number);
No error handling?
> + }
Use defines for the constants in the SET_RTS requests.
> +
> + /* if baud rate is B0, drop DTR and RTS */
> + if (!(cflag & CBAUD)) {
> + mxport->set_B0 = 1;
> + mxport->mcr_state &= ~(UART_MCR_DTR | UART_MCR_RTS);
> +
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_MCR,
> + mxport->mcr_state,
> + port->port_number);
> + if (err)
> + return err;
> + } else {
> + if (mxport->set_B0) {
> + mxport->set_B0 = 0;
> + err = mxuport_send_ctrl_urb(serial->dev,
> + RQ_VENDOR_SET_DTR,
> + 1, port->port_number);
> + if (err)
> + return err;
> + }
> + }
You shouldn't need set_B0. Make sure to pass the old termios settings
and detect B0 <-> non-B0 transitions using that struct. I see that
you're currently calling change_port_settings from setserial in order to
handle ASYNC_SPD_MASK. There are some issues with it's current
implementation, but if you really want to support it it's of course
doable (see below).
> +
> + /* S/W flow control settings */
> + if (I_IXOFF(tty) || I_IXON(tty)) {
> + buf = kmalloc(2, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + /* Submit the vendor request */
> + buf[0] = (unsigned char)xon;
> + buf[1] = (unsigned char)xoff;
> +
> + status = mxuport_send_ctrl_data_urb(serial->dev,
> + RQ_VENDOR_SET_CHARS,
> + 0, port->port_number,
> + buf, 2);
> +
> + kfree(buf);
> + if (status != 2)
> + return status;
> + }
> +
> + /* if we are implementing outbound XON/XOFF */
> + if (I_IXON(tty)) {
> + config.flow_ctrl |= MX_XON_FLOWCTRL;
> + dev_dbg(&port->dev,
> + "%s - outbound XON/XOFF is enabled, XON = 0x%2x,"
> + "XOFF = 0x%2x\n", __func__, xon, xoff);
> + } else {
> + config.flow_ctrl &= ~MX_XON_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - outbound XON/XOFF is disabled\n",
> + __func__);
> + }
> +
> + /* if we are implementing inbound XON/XOFF */
> + if (I_IXOFF(tty)) {
> + config.flow_ctrl |= MX_XOFF_FLOWCTRL;
> + dev_dbg(&port->dev,
> + "%s - inbound XON/XOFF is enabled, XON = 0x%2x,"
> + " XOFF = 0x%2x\n", __func__, xon, xoff);
> + } else {
> + config.flow_ctrl &= ~MX_XOFF_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - inbound XON/XOFF is disabled\n",
> + __func__);
> + }
> +
> + /* Submit the vendor request */
> + if (config.flow_ctrl & (MX_XON_FLOWCTRL | MX_XOFF_FLOWCTRL)) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_XONXOFF,
> + 1, port->port_number);
> + if (err)
> + return err;
> + } else {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_XONXOFF,
> + 0, port->port_number);
> + if (err)
> + return err;
> + }
> +
> + /* Set baud rate of termios */
> + baud = tty_get_baud_rate(tty);
> + if (!baud) {
> + /* pick a default using 9600 */
> + baud = 9600;
> + }
> +
> + buf = kmalloc(4, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + /* Submit the vendor request - Little Endian */
> + put_unaligned_le32(baud, buf);
> +
> + status = mxuport_send_ctrl_data_urb(serial->dev, RQ_VENDOR_SET_BAUD,
> + 0, port->port_number,
> + buf, 4);
> +
> + kfree(buf);
> + if (status != 4)
> + return status;
> +
> + /* Finally, store the uart settings to private structure */
> + memcpy(mxport->uart_cfg, &config, sizeof(struct mx_uart_config));
> +
> + /* Dump serial settings */
> + dev_dbg(&port->dev, "baud_rate : %d\n", baud);
> + dev_dbg(&port->dev, "data_bits : %d\n", mxport->uart_cfg->data_bits);
> + dev_dbg(&port->dev, "parity : %d\n", mxport->uart_cfg->parity);
> + dev_dbg(&port->dev, "stop_bits : %d\n", mxport->uart_cfg->stop_bits);
> + dev_dbg(&port->dev, "flow_ctrl : %d\n", mxport->uart_cfg->flow_ctrl);
> + dev_dbg(&port->dev, "xon : 0x%x\n", xon);
> + dev_dbg(&port->dev, "xoff : 0x%x\n", xoff);
> + dev_dbg(&port->dev, "Interface : %d\n", mxport->uart_cfg->interface);
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_tiocmset - Set the modem control registers
> + *
> + * Sets or clears RTS & DTR.
> + */
> +static int mxuport_tiocmset(struct tty_struct *tty, unsigned int set,
> + unsigned int clear)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + int err = 0;
> + unsigned int mcr;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + mcr = mxport->mcr_state;
You should use some locking for mcr_state (as mentioned in v1 review).
> +
> + /* set MCR status */
> + if (set & TIOCM_RTS) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RTS,
> + 1, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to set RTS\n", __func__);
> + return err;
> + }
> + mcr |= UART_MCR_RTS;
> + }
> +
> + if (set & TIOCM_DTR) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_DTR,
> + 1, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to set DTR\n", __func__);
> + return err;
> + }
> + mcr |= UART_MCR_DTR;
> + }
> +
> + /* clear MCR status */
> + if (clear & TIOCM_RTS) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RTS,
> + 0, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to clear RTS\n",
> + __func__);
> + return err;
> + }
> + mcr &= ~UART_MCR_RTS;
> + }
> +
> + if (clear & TIOCM_DTR) {
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_DTR,
> + 0, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to clear DTR\n",
> + __func__);
> + return err;
> + }
> + mcr &= ~UART_MCR_DTR;
> + }
> +
> + mxport->mcr_state = mcr;
> +
> + return err;
> +}
> +
> +/*
> + * mxuport_get_serial_info - Return information about the port
> + *
> + */
> +static int mxuport_get_serial_info(struct mxuport_port *mxport,
> + struct serial_struct __user *ret_arg)
> +{
> + struct usb_serial_port *port = mxport->port;
> + struct serial_struct tmp_serial;
> +
> + if (!ret_arg)
> + return -EFAULT;
> +
> + memset(&tmp_serial, 0, sizeof(tmp_serial));
> +
> + tmp_serial.type = PORT_16550A;
> + tmp_serial.line = port->minor;
> + tmp_serial.port = port->port_number;
> + tmp_serial.flags = mxport->flags;
> + tmp_serial.baud_base = 921600;
> +
> + if (copy_to_user(ret_arg, &tmp_serial, sizeof(*ret_arg)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_set_serial_info
> + *
> + * Set the interface type, e.g. RS-232, 2W RS-484, etc...
> + * Also set the alternative baud rates, 57600 and above
> + */
> +static int mxuport_set_serial_info(struct tty_struct *tty,
> + struct mxuport_port *mxport,
> + struct serial_struct __user *new_arg)
> +{
> + int err;
> + struct usb_serial_port *port = mxport->port;
> + struct usb_device *udev = port->serial->dev;
> + u16 productid = le16_to_cpu(udev->descriptor.idProduct);
> + struct mxuport_port old_cfg;
> + struct serial_struct new_serial;
> +
> + if (copy_from_user(&new_serial, new_arg, sizeof(new_serial)))
> + return -EFAULT;
> +
> + old_cfg = *mxport;
> +
> + if (!capable(CAP_SYS_ADMIN)) {
> + if (((new_serial.flags & ~ASYNC_USR_MASK) !=
> + (mxport->flags & ~ASYNC_USR_MASK)))
> + return -EPERM;
> + mxport->flags = ((mxport->flags & ~ASYNC_USR_MASK) |
> + (new_serial.flags & ASYNC_USR_MASK));
> + }
> + mxport->flags = ((mxport->flags & ~ASYNC_FLAGS) |
> + (new_serial.flags & ASYNC_FLAGS));
> +
> +
> + /* set port interface */
> + if ((productid == MX_UPORT1250_PID) ||
> + (productid == MX_UPORT1251_PID) ||
> + (productid == MX_UPORT1450_PID) ||
> + (productid == MX_UPORT1451_PID) ||
> + (productid == MX_UPORT1658_PID) ||
> + (productid == MX_UPORT1653_PID)) {
As I mentioned in my review of v1, try to keep product specific details
in the id_table above by using the driver_info field to store quirks or
features such as
#define MX_UPORT_QUIRK_RS232_ONLY 0x01
...
{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1410_PID),
.driver_info = MX_UPORT_QUIRK_RS232_ONLY },
for the product IDs that only support rs232 mode.
> + if ((new_serial.port >= 0) && (new_serial.port <= 3)) {
> + if (old_cfg.uart_cfg->interface != new_serial.port) {
> + mxport->uart_cfg->interface = new_serial.port;
> + err = mxuport_send_ctrl_urb(
> + udev,
> + RQ_VENDOR_SET_INTERFACE,
> + mxport->uart_cfg->
> + interface, port->port_number);
> + if (err)
> + return err;
> + }
> + }
> + } else {
> + if (old_cfg.uart_cfg->interface != new_serial.port)
> + return -EINVAL;
> + }
Ok, here you're abusing the setserial interface to set the Moxa-specific
interface-mode (rs232, 2-wire rs485, rs422, 4-wire rs485). We already
have an ioctl to enable rs-485:
Documentation/serial/serial-rs485.txt
Do you think you could use that interface? Perhaps those ioctls along
with a custom moxa "2-wire" sysfs flag would suffice to be able to
select one of the four modes?
> +
> + /* set alternative baud rate */
> + if ((old_cfg.flags & ASYNC_SPD_MASK) !=
> + (mxport->flags & ASYNC_SPD_MASK)) {
> + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
> + tty->alt_speed = 57600;
> + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
> + tty->alt_speed = 115200;
> + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_SHI)
> + tty->alt_speed = 230400;
> + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_WARP)
> + tty->alt_speed = 460800;
> + mxuport_change_port_settings(tty, port, mxport);
> + }
Do you really want and need this? Especially as you don't implement
custom divisor support. The above implementation is not correct either
way as these flags should only take effect when the user requests
38.4kb. I'd just drop it, if I where you.
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_set_termios
> + *
> + * Change the port configuration based on termios. Check there is
> + * something to change, and call the lower level function if there is.
> + */
> +static void mxuport_set_termios(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct ktermios *old_termios)
> +{
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct ktermios *termios = &tty->termios;
> + unsigned int cflag, iflag;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + if (!tty) {
> + dev_dbg(&port->dev, "%s - no tty\n", __func__);
> + return;
> + }
This will never happen.
> +
> + /* check that they really want us to change something */
> +
> + cflag = termios->c_cflag;
> + iflag = termios->c_iflag;
> +
> + if (old_termios) {
> + if (cflag == old_termios->c_cflag &&
> + iflag == old_termios->c_iflag) {
> + dev_dbg(&port->dev, "%s - nothing to change\n",
> + __func__);
> + return;
> + }
> + }
> +
> + if (old_termios) {
> + dev_dbg(&port->dev, "%s - old clfag %08x old iflag %08x\n",
> + __func__, old_termios->c_cflag,
> + old_termios->c_iflag);
> + }
> +
> + /* change the port settings to the new ones specified */
> + mxuport_change_port_settings(tty, port, mxport);
As mentioned above, I'd pass old_termios here to be used to detect
B0-transitions.
> +
> + return;
> +}
> +
> +/*
> + * mxuport_ioctl - I/O control function of driver
> + *
> + * This function handles any ioctl calls to the driver.
> + */
> +static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +
> + switch (cmd) {
> +
> + case TIOCGSERIAL:
> + dev_dbg(&port->dev, "%s - TIOCGSERIAL\n", __func__);
> + return mxuport_get_serial_info(
> + mxport, (struct serial_struct __user *)arg);
> + break;
> +
> + case TIOCSSERIAL:
> + dev_dbg(&port->dev, "%s - TIOCSSERIAL\n", __func__);
> + return mxuport_set_serial_info(
> + tty, mxport, (struct serial_struct __user *)arg);
> + break;
> +
> + default:
> + return -ENOIOCTLCMD;
> + }
> +
> + return 0;
> +}
> +
> +
> +/*
> + * mxuport_calc_num_port
> + *
> + * Determine how many ports this device has dynamically. It will be
> + * called after the probe() callback is called, but before attach().
> + */
> +static int mxuport_calc_num_ports(struct usb_serial *serial)
> +{
> + u16 product = le16_to_cpu(serial->dev->descriptor.idProduct);
> +
> + switch (product) {
> + case MX_UPORT1250_PID:
> + case MX_UPORT1251_PID:
> + return 2;
> + case MX_UPORT1410_PID:
> + case MX_UPORT1450_PID:
> + case MX_UPORT1451_PID:
> + return 4;
> + case MX_UPORT1618_PID:
> + case MX_UPORT1658_PID:
> + return 8;
> + case MX_UPORT1613_PID:
> + case MX_UPORT1653_PID:
> + return 16;
> +
> + }
> + return 0;
> +}
> +
> +/* Get the version of the firmware currently running. Return both as a
> + * a single decimal value, and three individual point values. The
> + * single value can be used simple comparisons of versions, while the
> + * point values is more human friendly.
> + */
> +static int mxuport_get_fw_version(struct usb_device *udev,
> + unsigned char *ver)
> +{
> + int err;
> + unsigned char *ver_buf = kzalloc(4, GFP_KERNEL);
Please separate definition and allocation here and elsewhere.
> +
> + if (!ver_buf)
> + return -ENOMEM;
> +
> + /* Send vendor request - Get firmware version from SDRAM */
> + err = mxuport_recv_ctrl_urb(udev, RQ_VENDOR_GET_VERSION, 0, 0,
> + ver_buf, 4);
> + if (err < 0)
> + goto out;
> +
> + err = (ver_buf[0] << 16) | (ver_buf[1] << 8) | ver_buf[2];
> + memcpy(ver, ver_buf, 3);
> +out:
> + kfree(ver_buf);
> + return err;
> +}
> +
> +/*
> + * Given a firmware blob, download it to the device.
> + */
> +static int mxuport_download_fw(struct usb_device *udev,
> + const struct firmware *fw_p)
> +{
> + int err;
> + size_t txlen, fwidx;
> + unsigned char *fw_buf = NULL;
> +
> + dev_dbg(&udev->dev, "Starting download firmware ...\n");
Pass the usb_serial struct instead of usb_device and use
&serial->interface->dev for any debugging or info messages.
> + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_START_FW_DOWN, 0, 0);
> + if (err)
> + goto out;
> +
> + /* Download firmware to device. */
> + fw_buf = kmalloc(DOWN_BLOCK_SIZE, GFP_KERNEL);
> + if (fw_buf == NULL) {
> + err = -ENOMEM;
> + goto out;
> + }
Do the allocation before you set FW_DOWN mode?
> +
> + fwidx = 0;
> + do {
> + txlen = min_t(size_t, (fw_p->size - fwidx), DOWN_BLOCK_SIZE);
> +
> + memcpy(fw_buf, &fw_p->data[fwidx], txlen);
> + err = mxuport_send_ctrl_data_urb(udev, RQ_VENDOR_FW_DATA, 0, 0,
> + fw_buf, txlen);
> + if (err != txlen)
> + goto out;
> +
> + fwidx += txlen;
> + mdelay(1);
You should be able to use usleep_range here, right? That's preferred
over the busy-waiting mdelay.
> +
> + } while (fwidx < fw_p->size);
> +
> + msleep(1000);
> + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_STOP_FW_DOWN, 0, 0);
> + if (err)
> + goto out;
> +
> + msleep(1000);
> + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_QUERY_FW_READY, 0, 0);
> +
> +out:
> + kfree(fw_buf);
> + return err;
> +}
> +
> +/*
> + * mxuport_probe - device has been found, can we driver it?
Interesting formulation... ;) Generally these lines in your function
headers listing mostly just the name of the function doesn't really add
much and could be removed (e.g. "mxuport_attach - attach function of
driver"). Try to avoid "over-commenting".
> + *
> + * This will be called when the device is inserted into the system,
> + * but before the device has been fully initialized by the usb_serial
> + * subsystem. Check the version of the firmware running on the
> + * device. If there is a newer version available, download it.
> + */
> +static int mxuport_probe(struct usb_serial *serial,
> + const struct usb_device_id *id)
> +{
> + u16 productid = le16_to_cpu(serial->dev->descriptor.idProduct);
> + struct usb_device *udev = serial->dev;
> + int dev_ver, local_ver;
> + unsigned char ver_buf[3];
> + const struct firmware *fw_p = NULL;
> + char buf[32];
> + int err;
> +
> + /* Load our firmware */
> + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_QUERY_FW_CONFIG, 0, 0);
> + if (err) {
> + mxuport_send_ctrl_urb(udev, RQ_VENDOR_RESET_DEVICE, 0, 0);
> + return err;
> + }
> +
> + dev_ver = mxuport_get_fw_version(udev, ver_buf);
> + if (dev_ver < 0) {
> + err = dev_ver;
> + goto out;
> + }
> +
> + dev_dbg(&udev->dev, "Device firmware version v%x.%x.%x\n",
> + ver_buf[0], ver_buf[1], ver_buf[2]);
> +
> + snprintf(buf, sizeof(buf) - 1, "moxa/moxa-%04x.fw", productid);
> +
> + err = request_firmware(&fw_p, buf, &udev->dev);
> + if (err) {
> + dev_warn(&udev->dev, "Firmware %s not found\n", buf);
> +
> + /* Use the firmware already in the device */
> + err = 0;
> + } else {
> + local_ver = ((fw_p->data[VER_ADDR_1] << 16) |
> + (fw_p->data[VER_ADDR_2] << 8) |
> + fw_p->data[VER_ADDR_3]);
> + dev_dbg(&udev->dev, "Available firmware version v%x.%x.%x\n",
> + fw_p->data[VER_ADDR_1], fw_p->data[VER_ADDR_2],
> + fw_p->data[VER_ADDR_3]);
> + if (local_ver > dev_ver) {
> + err = mxuport_download_fw(udev, fw_p);
> + if (err)
> + goto out;
> + dev_ver = mxuport_get_fw_version(udev, ver_buf);
> + if (dev_ver < 0) {
> + err = dev_ver;
> + goto out;
> + }
> + }
> + }
> +
> + dev_info(&udev->dev, "Using device firmware version v%x.%x.%x\n",
> + ver_buf[0], ver_buf[1], ver_buf[2]);
> +out:
> + if (fw_p)
> + release_firmware(fw_p);
> + return err;
> +}
> +
> +
> +static int mxuport_port_probe(struct usb_serial_port *port)
> +{
> + struct mxuport_port *mxport;
> +
> + dev_dbg(&port->serial->dev->dev, "%s\n", __func__);
Use &port->dev.
> +
> + mxport = kzalloc(sizeof(struct mxuport_port), GFP_KERNEL);
> + if (!mxport)
Just return -ENOMEM here.
> + goto out;
> +
> + /* Loop back to the owner of this object */
> + mxport->port = port;
You should be able to remove this one from the port data and simply pass
the usb_serial_port struct around (e.g. to setserial).
> +
> + /* Initial UART configuration */
> + mxport->uart_cfg =
> + kzalloc(sizeof(struct mx_uart_config), GFP_KERNEL);
> + if (!mxport->uart_cfg)
> + goto out;
> +
> + mxport->uart_cfg->data_bits = MX_WORDLENGTH_8;
> + mxport->uart_cfg->parity = MX_PARITY_NONE;
> + mxport->uart_cfg->stop_bits = MX_STOP_BITS_1;
> + mxport->uart_cfg->flow_ctrl = MX_NO_FLOWCTRL;
> + mxport->uart_cfg->interface = MX_INT_RS232;
In fact, you don't need these either, except possibly flow_ctrl (tested
in close). You store the updated values in mxuport_change_port_settings
but never use them. I looks like you should simply get rid of the
uart_cfg strut altogether and store what state you actually need
directly in the mxuport_port struct.
> +
> + /* Set the port private data */
> + usb_set_serial_port_data(port, mxport);
> +
> + return 0;
> +
> +out:
> + kfree(mxport);
> + return -ENOMEM;
> +}
> +
> +/*
> + * mxuport_attach - attach function of driver
> + *
> + * This will be called when the struct usb_serial structure is fully
> + * set up. Create private structures per port, and set the device into
> + * a default configuration.
> + */
> +static int mxuport_attach(struct usb_serial *serial)
> +{
> + struct usb_serial_port *port0 = serial->port[0];
> + struct usb_serial_port *port1 = serial->port[1];
> + struct usb_serial_port *port;
> + struct mxuport_port *mxport;
> + struct usb_device *udev;
> + struct usb_host_interface *iface_desc;
> + struct usb_endpoint_descriptor *endpoint = NULL;
> + struct usb_device *dev = interface_to_usbdev(serial->interface);
> + int buffer_size;
> + int err = -ENOMEM;
> + int i, j;
> +
> + /* Get product info from device */
> + udev = serial->dev;
> + dev_dbg(&udev->dev, "%s detected\n", serial->type->description);
Use &serial->interface->dev for debugging in attach. This one is really
not needed though, as it is logged by usb-serial core.
> +
> + /*
> + * Throw away all the allocated write URBs so we can set
> + * them up again to fit the multiplexing scheme.
> + */
> + for (i = 0; i < serial->num_bulk_out; ++i) {
> + port = serial->port[i];
> + for (j = 0; j < ARRAY_SIZE(port->write_urbs); ++j) {
> + usb_free_urb(port->write_urbs[j]);
> + kfree(port->bulk_out_buffers[j]);
> + port->write_urbs[j] = NULL;
> + port->bulk_out_buffers[j] = NULL;
> + }
> + port->write_urbs_free = 0;
> + }
There's no need to free and later reallocate the first write urb, right?
> +
> + /* Find the first bulk out interface - has to exist */
> + iface_desc = serial->interface->cur_altsetting;
> + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> + endpoint = &iface_desc->endpoint[i].desc;
> + if (usb_endpoint_is_bulk_out(endpoint))
> + break;
> + }
You don't need this, just use the fields of port0 already filled in by
usb-serial.
> +
> + /*
> + * All write data is sent over the first bulk out endpoint,
> + * with an added header to indicate the port. Allocate URBs
> + * for each port to the first bulk out endpoint.
> + */
> + buffer_size = usb_endpoint_maxp(endpoint);
Skip this...
> + for (i = 0; i < serial->num_ports; ++i) {
...and only iterate over ports with num > 0...
> + port = serial->port[i];
> + port->bulk_out_size = buffer_size;
...and then use
port->bulk_out_size = port0->bulk_out_size
and similar throughout.
> + port->bulk_out_endpointAddress = endpoint->bEndpointAddress;
> + for (j = 0; j < ARRAY_SIZE(port->write_urbs); ++j) {
> + set_bit(j, &port->write_urbs_free);
> + port->write_urbs[j] = usb_alloc_urb(0, GFP_KERNEL);
> + if (!port->write_urbs[j])
> + goto out;
> + port->bulk_out_buffers[j] = kmalloc(buffer_size,
> + GFP_KERNEL);
> + if (!port->bulk_out_buffers[j])
> + goto out;
> + usb_fill_bulk_urb(port->write_urbs[j], dev,
> + usb_sndbulkpipe(dev,
> + endpoint->bEndpointAddress),
> + port->bulk_out_buffers[j],
> + buffer_size,
> + serial->type->write_bulk_callback,
> + port);
> + }
> +
> + port->write_urb = port->write_urbs[0];
> + port->bulk_out_buffer = port->bulk_out_buffers[0];
> +
> + /*
> + * Ensure each port has a fifo. The framework only
> + * allocates a fifo to ports with a bulk out endpoint,
> + * where as we need one for every port.
> + */
> + if (!kfifo_initialized(&port->write_fifo)) {
> + err = kfifo_alloc(&port->write_fifo, PAGE_SIZE,
> + GFP_KERNEL);
> + if (err)
> + goto out;
> + }
> + }
> +
> + /*
> + * Add data from the ports is received on the first bulk in
> + * endpoint, with a multiplex header. The second bulk in is
> + * used for events. Throw away all but the first two bulk in
> + * urbs.
> + */
> + for (i = 2; i < serial->num_bulk_in; ++i) {
> + port = serial->port[i];
> + for (j = 0; j < ARRAY_SIZE(port->read_urbs); ++j) {
> + usb_free_urb(port->read_urbs[j]);
> + kfree(port->bulk_in_buffers[j]);
> + port->read_urbs[j] = NULL;
> + port->bulk_in_buffers[j] = NULL;
> + }
> + }
Do any of these devices actually have more than two bulk-in endpoints?
You should make sure to set bulk_in_size to 0 as well (used during
resume).
Actually, you will have to override the generic resume implementation
anyway, as the generic one will fail to resubmit the read urbs unless
port 0 and 1 are open.
> +
> + /* Start to read serial data from the device */
> + err = usb_serial_generic_submit_read_urbs(port0, GFP_KERNEL);
> + if (err) {
> + dev_dbg(&port0->dev,
> + "%s - USB submit read bulk urb failed. (status = %d)\n",
> + __func__, err);
> + goto out;
> + }
> +
> + /* Endpoints on Port1 is used for events */
> + err = usb_serial_generic_submit_read_urbs(port1, GFP_KERNEL);
> + if (err) {
> + dev_dbg(&port1->dev,
> + "%s - USB submit read bulk urb failed. (status = %d)\n",
> + __func__, err);
> + goto out;
> + }
> + return 0;
Hmm. So here you're submitting read urbs for two ports before their
port_probe has run and allocated the port private data. Specifically,
if you get an early event packet on port1 you end up with a NULL-deref
when processing it.
You could either start reading in port_probe (for port0 and port1, or
equivalently, for any port with a bulk-in endpoint) or implement
something more elaborate by submitting the urbs on first port open and
killing them on last close. You'd need a mutex and a counter for that.
> +
> +out:
> + /* shutdown any bulk transfers that might be going on */
> + usb_serial_generic_close(port1);
> + usb_serial_generic_close(port0);
> +
> + for (; i >= 0; i--) {
> + mxport = usb_get_serial_port_data(serial->port[i]);
> +
> + if (mxport->uart_cfg != NULL)
> + kfree(mxport->uart_cfg);
> +
> + kfree(mxport);
> + usb_set_serial_port_data(serial->port[i], NULL);
> + }
This is wrong. The private port data has not been allocated yet
(port_probe has not run), so you'd get a NULL-pointer dereference with
this. Just remove it.
> + usb_set_serial_data(serial, NULL);
Remove this as well. You've never set it.
> +
> + return err;
> +}
> +
> +/* mxuport_port_remove - remove function of one port.
> + *
> + * Free the private memory.
> + */
I'd just remove these kind of comments. It's obvious from the code what
is going on.
> +static int mxuport_port_remove(struct usb_serial_port *port)
> +{
> + struct mxuport_port *mxport;
> +
> + dev_dbg(&port->serial->dev->dev, "%s\n", __func__);
Use &port->dev here as well. Please check your usage throughout. You
should only use
&serial->interface->dev
or
&port->dev
> +
> + mxport = usb_get_serial_port_data(port);
> + if (mxport->uart_cfg != NULL)
If you get here, uart_cfg has been allocated, so remove the test.
> + kfree(mxport->uart_cfg);
> +
> + kfree(mxport);
> + usb_set_serial_port_data(port, NULL);
Not really needed.
> +
> + return 0;
> +}
> +
> +/*
> + * mxuport_open - open function of driver
> + *
> + * This function is called by the tty driver when a port is opened
> + * If successful, we return 0.
> + * Otherwise, we return a negative error number.
> + */
> +static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> + int err = 0;
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +
> + /* Initial port settings */
> + err = mxuport_init_port(port);
> + if (err)
> + return err;
> +
> + /* Initial port termios */
> + mxuport_set_termios(tty, port, NULL);
> +
> + /* Initial private parameters of port */
> + mxport->hold_reason = 0;
> + mxport->lsr_state = 0;
> + mxport->msr_state = 0;
> + mxport->set_B0 = 0;
> + mxport->mcr_state = UART_MCR_DTR | UART_MCR_RTS;
As I mentioned in my previous round of comments, you should consider
implementing dtr_rts() so that DTR/RTS are actually raised on open.
> +
> + return err;
> +}
> +
> +/*
> + * mxuport_close - close function of driver
> + *
> + * This function is called by the tty driver when a port is closed.
> + */
Again, have a look at all the function headers and consider removing a
bunch of them, at least from the usb-serial operations.
> +static void mxuport_close(struct usb_serial_port *port)
> +{
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + /*
> + * Send vendor request to device to tell it to close the
> + * port
> + */
> + mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_OPEN, 0,
> + port->port_number);
> +
> + /*
> + * Reenable the S/W flow control to prevent cannot write
> + * data out that receive XOFF char before.
> + */
Could you fix the wording here? It's not really clear what you mean.
> + mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_XONXOFF,
> + 0, port->port_number);
> + if (mxport->uart_cfg->flow_ctrl &
> + (MX_XON_FLOWCTRL | MX_XOFF_FLOWCTRL))
> + mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_XONXOFF,
> + 1, port->port_number);
> +}
> +
> +static int mxuport_tiocmget(struct tty_struct *tty)
> +{
> + struct mxuport_port *mxport;
> + struct usb_serial_port *port = tty->driver_data;
> + unsigned int result = 0;
> + unsigned int msr;
> + unsigned int mcr;
> + unsigned long flags;
> +
> + mxport = usb_get_serial_port_data(port);
> +
> + spin_lock_irqsave(&port->lock, flags);
> +
> + msr = mxport->msr_state;
> +
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + mcr = mxport->mcr_state;
I've already mentioned mcr_state locking.
> +
> + result = (((mcr & UART_MCR_DTR) ? TIOCM_DTR : 0) | /* 0x002 */
> + ((mcr & UART_MCR_RTS) ? TIOCM_RTS : 0) | /* 0x004 */
> + ((msr & UART_MSR_CTS) ? TIOCM_CTS : 0) | /* 0x020 */
> + ((msr & UART_MSR_DCD) ? TIOCM_CAR : 0) | /* 0x040 */
> + ((msr & UART_MSR_RI) ? TIOCM_RI : 0) | /* 0x080 */
> + ((msr & UART_MSR_DSR) ? TIOCM_DSR : 0)); /* 0x100 */
> +
> + dev_dbg(&port->dev, "%s - MSR return 0x%x\n", __func__, result);
> +
> + return result;
> +}
> +
> +/*
> + * mxuport_break_ctl - break function of driver
> + *
> + * This function sends a break to the port.
> + */
> +static void mxuport_break_ctl(struct tty_struct *tty, int break_state)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct usb_serial *serial = port->serial;
> + struct mxuport_port *mxport;
> + int err;
> +
> + mxport = usb_get_serial_port_data(port);
> +
> + if (break_state == -1) {
> + dev_dbg(&port->dev, "%s - sending break\n", __func__);
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_BREAK,
> + 1, port->port_number);
> + } else {
> + dev_dbg(&port->dev, "%s - clearing break\n", __func__);
> + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_BREAK,
> + 0, port->port_number);
> + }
> + if (err) {
> + dev_dbg(&port->dev,
> + "%s - error sending break set/clear command.\n",
> + __func__);
> + }
You have already logged errors in mxuport_send_ctrl(_data)_urb. Does the
dbg here really add anything?
> +
> + return;
> +}
> +
> +static struct usb_serial_driver mxuport_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "mx-uport",
> + },
> + .description = "MOXA UPort",
> + .id_table = mxuport_idtable,
> + .num_ports = 0,
> + .probe = mxuport_probe,
> + .port_probe = mxuport_port_probe,
> + .port_remove = mxuport_port_remove,
> + .attach = mxuport_attach,
> + .calc_num_ports = mxuport_calc_num_ports,
> + .open = mxuport_open,
> + .close = mxuport_close,
> + .write = usb_serial_generic_write,
> + .ioctl = mxuport_ioctl,
> + .set_termios = mxuport_set_termios,
> + .break_ctl = mxuport_break_ctl,
> + .tx_empty = mxuport_tx_empty,
> + .tiocmiwait = usb_serial_generic_tiocmiwait,
> + .get_icount = usb_serial_generic_get_icount,
> + .throttle = mxuport_throttle,
> + .unthrottle = mxuport_unthrottle,
> + .tiocmget = mxuport_tiocmget,
> + .tiocmset = mxuport_tiocmset,
> + .process_read_urb = mxuport_process_read_urb,
> + .prepare_write_buffer = mxuport_prepare_write_buffer,
> +};
> +
> +static struct usb_serial_driver *const serial_drivers[] = {
> + &mxuport_device, NULL
> +};
> +
> +module_usb_serial_driver(serial_drivers, mxuport_idtable);
> +
> +/*
> + * Module Information
> + */
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_LICENSE("GPL");
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
