On Wed, Sep 25, 2013 at 11:53:00AM +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 URB is for port0.
>
> 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>
>
> ---
>
> v1->v2
>
> 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
>
> v2->v3
>
> Clarify the meaning of interface in mx_uart_config.
> Remove buffer from mxuport_send_async_ctrl_urb().
> No need to multiply USB_CTRL_SET_TIMEOUT by HZ.
> Simplify buffer allocation and free.
> Swap (un)throttle to synchronous interface, remove async code.
> Pass usb_serial to mxuport_[recv|send]_ctrl_urb()
> Add missing {} on if else statement.
> Use unsigned char type, remove casts.
> Replace constants with defines.
> Add error handling when disabling HW flow control.
> Verify port is open before passing it recieved data
> Verify contents of received data & events.
> Remove broken support for alternative baud rates.
> Fix B0 and modem line handling. Remove uart_cfg structure.
> Remove hold_reason, which was set, but never used.
> s/is/if/
> Move parts of port open into port probe.
> Remove mxport->port and pass usb_serial_port instead.
> Remove mxport->flags which is no longer used.
> Allocate buffers before starting firmware download.
> Remove redundent "detected" debug message.
> Use devm_kzalloc() to simply memory handling.
> Remove repeated debug message when sending break.
> Implement mxuport_dtr_rts().
> Add locking around mcr_state.
> Remove unused lsr_state from mxuport_port.
> Support 485 via official IOCTL and sysfs.
> Use usleep_range() instread of mdelay().
> Simplify the comments.
> Use port0 as a template when setting up bulk out endpoints.
> Set bulk_in_size for unused endpoints to 0.
> Rebase to v3.12-rc2
Thanks for the update, Andrew. It's starting to look quite good. Some
more comments on v3 below.
>
> TODO
> Custom resume function
You have to implement a custom resume function as usb-serial will use
the incompatible generic one otherwise. Model it after the generic
implementation, but always submit the read urbs for port 0 and 1 (only)
while calling usb_serial_generic_write_start for any open port. Note that
this requires exporting usb_serial_generic_write_start (I'll submit a
patch).
>
> ---
> drivers/usb/serial/Kconfig | 29 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/mxuport.c | 1611 ++++++++++++++++++++++++++++++++++++++++++
That's another 160 lines gone since v2. Great!
> 3 files changed, 1641 insertions(+)
> create mode 100644 drivers/usb/serial/mxuport.c
>
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index ddb9c51..36bfd48 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -472,6 +472,35 @@ config USB_SERIAL_MOS7840
> To compile this driver as a module, choose M here: the
> module will be called mos7840. If unsure, choose N.
>
> +config USB_SERIAL_MOXA_UPORT
> + tristate "USB Moxa UPORT USB Serial Driver"
> + ---help---
> + Say Y here if you want to use a MOXA UPort Serial hub.
> +
> + This driver supports:
> +
> + [2 Port]
> + - UPort 1250 : 2 Port RS-232/422/485 USB to Serial Hub
> + - UPort 1250I : 2 Port RS-232/422/485 USB to Serial Hub with
> + Isolation
> +
> + [4 Port]
> + - UPort 1410 : 4 Port RS-232 USB to Serial Hub
> + - UPort 1450 : 4 Port RS-232/422/485 USB to Serial Hub
> + - UPort 1450I : 4 Port RS-232/422/485 USB to Serial Hub with
> + Isolation
> +
> + [8 Port]
> + - UPort 1610-8 : 8 Port RS-232 USB to Serial Hub
> + - UPort 1650-8 : 8 Port RS-232/422/485 USB to Serial Hub
> +
> + [16 Port]
> + - UPort 1610-16 : 16 Port RS-232 USB to Serial Hub
> + - UPort 1650-16 : 16 Port RS-232/422/485 USB to Serial Hub
> +
> + To compile this driver as a module, choose M here: the
> + module will be called mx_uport.
> +
> config USB_SERIAL_NAVMAN
> tristate "USB Navman GPS device"
> help
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index 42670f0..9b5d6b3 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_USB_SERIAL_MCT_U232) += mct_u232.o
> obj-$(CONFIG_USB_SERIAL_METRO) += metro-usb.o
> obj-$(CONFIG_USB_SERIAL_MOS7720) += mos7720.o
> obj-$(CONFIG_USB_SERIAL_MOS7840) += mos7840.o
> +obj-$(CONFIG_USB_SERIAL_MOXA_UPORT) += mxuport.o
> obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o
> obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o
> obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o
> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> new file mode 100644
> index 0000000..ff5b452
> --- /dev/null
> +++ b/drivers/usb/serial/mxuport.c
> @@ -0,0 +1,1611 @@
> +/*
> + * 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 /* Get modem status register */
> +
> +/* 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 modem control event type */
> +#define SERIAL_EV_XOFF 0x40 /* XOFF received */
> +
> +/* 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_HW_FLOWCTRL_ENABLE 0x2
> +#define MX_HW_FLOWCTRL_DISABLE 0x1
> +
> +#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 {
> + u8 mcr_state; /* Last MCR state */
> + u8 msr_state; /* Last MSR state */
> + int interface; /* RS232, RS485, RS422 */
> + bool four_wire_rs485; /* 4 wire RS485 */
> +};
> +
> +#define MX_UPORT_QUIRK_RS232_ONLY 0x01
> +
> +/* 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) ,
> + .driver_info = MX_UPORT_QUIRK_RS232_ONLY },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1450_PID) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1451_PID) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1618_PID),
> + .driver_info = MX_UPORT_QUIRK_RS232_ONLY },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1658_PID) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1613_PID) ,
> + .driver_info = MX_UPORT_QUIRK_RS232_ONLY },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1653_PID) },
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, mxuport_idtable);
> +
> +/*
> + * 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);
Use your HEADER_SIZE define here instead of the constant?
> +
> + 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;
Same here.
> +}
> +
> +/* Read the given buffer in from the control pipe. */
> +static int mxuport_recv_ctrl_urb(struct usb_serial *serial,
> + u8 request, u16 value, u16 index,
> + u8 *data, size_t size)
> +{
> + int status;
> +
> + status = usb_control_msg(serial->dev,
> + usb_rcvctrlpipe(serial->dev, 0),
> + request,
> + (USB_DIR_IN | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE), value, index,
> + data, size,
> + USB_CTRL_GET_TIMEOUT);
> +
> + if (status < 0) {
> + dev_dbg(&serial->interface->dev,
> + "%s - recv usb_control_msg failed. (%d)\n",
> + __func__, status);
> + return status;
> + }
> +
> + if (status != size) {
> + dev_dbg(&serial->interface->dev,
> + "%s - wanted to recv %zd, but only read %d\n",
> + __func__, size, status);
> + return -ECOMM;
You should return -EIO here instead of -ECOMM (even if the latter is
used by a couple of old serial drivers).
> + }
> +
> + return status;
> +}
> +
> +/* Write the given buffer out to the control pipe. */
> +static int mxuport_send_ctrl_data_urb(struct usb_serial *serial,
> + u8 request,
> + u16 value, u16 index,
> + u8 *data, size_t size)
> +{
> + int status;
> +
> + status = usb_control_msg(serial->dev,
> + usb_sndctrlpipe(serial->dev, 0),
> + request,
> + (USB_DIR_OUT | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE), value, index,
> + data, size,
> + USB_CTRL_SET_TIMEOUT);
> + if (status < 0) {
> + dev_dbg(&serial->interface->dev,
> + "%s - send usb_control_msg failed. (%d)\n",
> + __func__, status);
> + return status;
> + }
> +
> + if (status != size) {
> + dev_dbg(&serial->interface->dev,
> + "%s - wanted to write %zd, but only wrote %d\n",
> + __func__, size, status);
> + return -ECOMM;
Use -EIO here as well.
> + }
> +
> + return status;
> +}
> +
> +/* Send a vendor request without any data */
> +static int mxuport_send_ctrl_urb(struct usb_serial *serial,
> + u8 request, u16 value, u16 index)
> +{
> + return mxuport_send_ctrl_data_urb(serial, 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;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + spin_lock_irqsave(&port->lock, flags);
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN,
> + 0, port->port_number);
> + spin_unlock_irqrestore(&port->lock, flags);
You must not call mxuport_send_ctrl_urb (usb_control_msg) in atomic
context. The tty-layer will prevent concurrent throttle/unthrottle so
simply drop the spin lock here.
> +}
> +
> +/*
> + * 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 usb_serial *serial = port->serial;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + spin_lock_irqsave(&port->lock, flags);
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN,
> + 1, port->port_number);
> + spin_unlock_irqrestore(&port->lock, flags);
Same here.
> +}
> +
> +/*
> + * Processes one chunk of data received for a port. Mostly a copy of
> + * usb_serial_generic_process_read_urb().
> + */
> +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) {
> + 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);
> +}
> +
> +/*
> + * 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 short rcv_msr_event;
> + unsigned char rcv_msr_hold, lsr_event;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s - receive event : %ld\n",
> + __func__, event);
> +
> + switch (event) {
> + case UPORT_EVNET_SEND_NEXT:
> + /*
> + Sent as part of the flow control on device buffers.
> + Not currently used. */
The preferred multi-line comment style is
/*
* ...
*/
> + break;
> +
> + case UPORT_EVNET_MSR:
> + rcv_msr_event = get_unaligned_be16(buf);
> + rcv_msr_hold = (buf[2] & 0xF0);
Parentheses not needed.
> +
> + 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)) {
Why not switch the conditional branches and drop the negation here and
below?
> + mxport->msr_state &= ~UART_MSR_CTS;
> + dev_dbg(&port->dev, "%s - CTS low\n",
> + __func__);
> + } else {
> + mxport->msr_state |= UART_MSR_CTS;
> + dev_dbg(&port->dev, "%s - CTS high\n",
> + __func__);
> + }
> +
> + if (!(rcv_msr_hold & UART_MSR_DSR)) {
> + mxport->msr_state &= ~UART_MSR_DSR;
> + dev_dbg(&port->dev, "%s - DSR low\n",
> + __func__);
> + } else {
> + mxport->msr_state |= UART_MSR_DSR;
> + dev_dbg(&port->dev, "%s - DSR high\n",
> + __func__);
> + }
> +
> + if (!(rcv_msr_hold & UART_MSR_DCD)) {
> + mxport->msr_state &= ~UART_MSR_DCD;
> + dev_dbg(&port->dev, "%s - DCD low\n",
> + __func__);
> + } else {
> + mxport->msr_state |= UART_MSR_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);
> + }
I suggest you break out the modem-status handling to a helper function.
Should increase readability (and reduce indentation). This way you could
also use a common initial test on rcv_msr_event (e.g. return if 0) for
both state updating and interrupt counting.
> + break;
> +
> + case UPORT_EVNET_LSR:
> + lsr_event = buf[2];
> +
> + 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__);
> + }
> +
Perhaps add a helper for LSR which can be expanded if anyone wants to
implement more elaborate LSR handling (e.g. break handling).
> + break;
> +
> + case UPORT_EVNET_MCR:
> + /*
> + Event to indicate a change in XON/XOFF from the
> + peer. Currently not used. We just continue sending
> + the device data and it will buffer it if
> + needed. This event could be used for flow control
> + between the host and the device. */
Multi-line comment style.
> + break;
> +
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * One URB can contain data for multiple ports. Demultiplex the data,
> + * checking the port exists, is opened and the message is valid.
> + */
> +static void mxuport_process_read_urb_demux_data(struct urb *urb)
> +{
> + struct usb_serial_port *port = urb->context;
> + struct usb_serial *serial = port->serial;
> + unsigned char *data = urb->transfer_buffer;
> + unsigned char *end = data + urb->actual_length;
> + struct usb_serial_port *demux_port;
> + unsigned char *ch;
> + unsigned short rcv_port, rcv_len;
> +
> + while (data < end) {
> + if (data + HEADER_SIZE > end) {
> + dev_warn(&port->dev, "%s - message with short header\n",
> + __func__);
> + return;
> + }
> +
> + rcv_port = get_unaligned_be16(data);
> + if (rcv_port >= serial->num_ports) {
> + dev_warn(&port->dev, "%s - message for invalid port\n",
> + __func__);
> + return;
> + }
> +
> + demux_port = serial->port[rcv_port];
> + rcv_len = get_unaligned_be16(data + 2);
Can rcv_len ever be 0?
> + if (data + HEADER_SIZE + rcv_len > end) {
> + dev_warn(&port->dev, "%s - short data\n",
> + __func__);
> + return;
> + }
> +
> + if (!test_bit(ASYNCB_INITIALIZED, &demux_port->port.flags)) {
I'd remove the negation and switch the branches.
> + dev_dbg(&demux_port->dev, "%s - data for closed port\n",
> + __func__);
> + } else {
> + ch = data + HEADER_SIZE;
> + mxuport_process_read_urb_data(demux_port, ch, rcv_len);
> + }
> + data += (HEADER_SIZE + rcv_len);
Parentheses not needed.
> + }
> +}
> +
> +/*
> + * One URB can contain events for multiple ports. Demultiplex the event,
> + * checking the port exists, and is opened.
> + */
> +static void mxuport_process_read_urb_demux_event(struct urb *urb)
> +{
> + struct usb_serial_port *port = urb->context;
> + struct usb_serial *serial = port->serial;
> + unsigned char *data = urb->transfer_buffer;
> + unsigned char *end = data + urb->actual_length;
> + struct usb_serial_port *demux_port;
> + unsigned char *ch;
> + unsigned short rcv_port, rcv_event;
> +
> + while (data < end) {
> + if (data + MAX_EVENT_LENGTH > end) {
> + dev_warn(&port->dev, "%s - message with short event\n",
> + __func__);
> + return;
> + }
> +
> + rcv_port = get_unaligned_be16(data);
> + if (rcv_port >= serial->num_ports) {
> + dev_warn(&port->dev, "%s - message for invalid port\n",
> + __func__);
> + return;
> + }
> +
> + demux_port = serial->port[rcv_port];
> + if (!test_bit(ASYNCB_INITIALIZED, &demux_port->port.flags)) {
Switch the conditional branches here as well?
> + dev_dbg(&demux_port->dev,
> + "%s - event for closed port\n", __func__);
> + } else {
> + ch = data + HEADER_SIZE;
> + rcv_event = get_unaligned_be16(data + 2);
> + mxuport_process_read_urb_event(demux_port, ch,
> + rcv_event);
> + }
> + data += MAX_EVENT_LENGTH;
> + }
> +}
> +
> +/*
> + * This is called when we have received data on the bulk in
> + * endpoint. Depending on which port it was received on, it can
> + * contain serial data or events.
> + */
> +static void mxuport_process_read_urb(struct urb *urb)
> +{
> + struct usb_serial_port *port = urb->context;
> + struct usb_serial *serial = port->serial;
> +
> + if (port == serial->port[0])
> + mxuport_process_read_urb_demux_data(urb);
> +
> + if (port == serial->port[1])
> + mxuport_process_read_urb_demux_event(urb);
> +}
> +
> +/*
> + * 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_serial *serial = port->serial;
> + 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(serial,
> + 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;
> +}
> +
> +/*
> + * Initialize one port, by enabling its FIFO, set to RS-232 mode, open
> + * the port and enable reception.
> + */
Now you're setting RS232 mode at port probe.
> +static int mxuport_init_port(struct usb_serial_port *port)
> +{
> + struct usb_serial *serial = port->serial;
> + int err = 0;
No need to initialise.
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + /* Send vendor request - port open */
> +
> + err = mxuport_send_ctrl_urb(serial, 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, RQ_VENDOR_SET_RX_HOST_EN,
> + 1, port->port_number);
> + return err;
> +}
How about simply folding this one into open now that some of it has gone
into port_probe?
Would it be better (and possible) to enable reception before opening?
What about error handling; Should you close the port or disable rx
if one of the calls fail? This could perhaps prevent data being received
on non-open ports (handled in process_read_urb path, but still).
> +
> +/* 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;
It's probably better to just call this one ret or res, or similar. But I
see you use err pretty consistently throughout so just keep it if you
prefer.
> + unsigned int mcr;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + spin_lock_irqsave(&port->lock, flags);
> +
> + mcr = mxport->mcr_state;
> +
> + /* Set MCR status */
> + if (set & TIOCM_RTS) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RTS,
> + 1, port->port_number);
Again, you cannot call mxuport_send_ctrl_urb in atomic context. You need
to use a mutex to fully handle concurrency (some serial drivers are
currently not doing this).
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to set RTS\n", __func__);
> + goto out;
> + }
> + mcr |= UART_MCR_RTS;
> + }
> +
> + if (set & TIOCM_DTR) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> + 1, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to set DTR\n", __func__);
> + goto out;
> + }
> + mcr |= UART_MCR_DTR;
> + }
> +
> + /* Clear MCR status */
> + if (clear & TIOCM_RTS) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RTS,
> + 0, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to clear RTS\n",
> + __func__);
> + goto out;
> + }
> + mcr &= ~UART_MCR_RTS;
> + }
> +
> + if (clear & TIOCM_DTR) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> + 0, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to clear DTR\n",
> + __func__);
> + goto out;
> + }
> + mcr &= ~UART_MCR_DTR;
> + }
> +
> + mxport->mcr_state = mcr;
> +
> +out:
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + return err;
> +}
> +
> +
> +static void mxuport_dtr_rts(struct usb_serial_port *port, int on)
> +{
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + unsigned long flags;
> + int err;
> +
> + spin_lock_irqsave(&port->lock, flags);
> +
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> + on, port->port_number);
Again, you cannot use mxuport_send_ctrl_urb in atomic context, and
should use a mutex instead.
The tty layer should always pass 0 or 1, but perhaps use !!on depending
on what the device can handle?
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to change DTR\n", __func__);
> + goto out;
> + }
> + mxport->mcr_state &= ~UART_MCR_DTR;
You want to update mcr_state depending on on (and possibly err) and not
unconditionally clear mcr_state here.
> +
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RTS,
> + on, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to change RTS\n", __func__);
> + goto out;
> + }
> + mxport->mcr_state &= ~UART_MCR_RTS;
Same here.
> +
> +out:
> + spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +/* Return information about the port */
You can drop this comment.
> +static int mxuport_get_serial_info(struct usb_serial_port *port,
> + struct serial_struct __user *ret_arg)
> +{
> + 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.baud_base = 921600;
> +
> + if (copy_to_user(ret_arg, &tmp_serial, sizeof(*ret_arg)))
> + return -EFAULT;
> +
> + return 0;
> +}
You're not really returning anything useful here (the port number is
accessible through sysfs). Perhaps you should just drop
TIOCGSERIAL-support?
> +
> +static ssize_t show_four_wire(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct usb_serial_port *port = to_usb_serial_port(dev);
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +
> + return sprintf(buf, "%i\n", mxport->four_wire_rs485);
Use scnprintf (and PAGE_SIZE) even if it's a bool.
> +}
> +
> +static ssize_t store_four_wire(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct usb_serial_port *port = to_usb_serial_port(dev);
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +
> + if (strtobool(buf, &mxport->four_wire_rs485) < 0)
> + return -EINVAL;
> +
> + return size;
> +}
> +
> +static DEVICE_ATTR(4_wire_rs485, S_IWUSR | S_IRUGO,
> + show_four_wire, store_four_wire);
Use DEVICE_ATTR_RW.
You dropped RS422-support from v3. Was that intentional?
Hmmm. I've been thinking a bit how best to handle this, and I think we
should consider adding a SER_RS485_4_WIRE flag to serial_rs485 instead
of having a custom attribute. That still leaves RS422, which could
possibly be enabled using the RS485-ioctl as well (or we use a
rs422-attribute for now).
I'll get back to you on this.
> +
> +static int mxuport_set_rs485(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct serial_rs485 __user *new_arg)
> +{
> + struct usb_serial *serial = port->serial;
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct serial_rs485 new_rs485;
> + int quirk;
> + int err;
> +
> + if (copy_from_user(&new_rs485, new_arg, sizeof(new_rs485)))
> + return -EFAULT;
> +
> + quirk = (int)usb_get_serial_data(serial);
You need to cast as unsigned long here (for 64-bit machines). (You
mentioned you were aware of this off-list, but including for
completeness).
> + if (quirk & MX_UPORT_QUIRK_RS232_ONLY)
> + return -ENOIOCTLCMD;
> +
> + mxport->interface = MX_INT_RS232;
> + if (new_rs485.flags & SER_RS485_ENABLED) {
> + if (mxport->four_wire_rs485)
> + mxport->interface = MX_INT_2W_RS485;
> + else
> + mxport->interface = MX_INT_4W_RS485;
Looks like you switched 2-wire and 4-wire here.
> + }
> +
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_INTERFACE,
> + mxport->interface, port->port_number);
> + if (err)
> + return err;
> +
> + /*
> + * Clear bits that are not supported and return it to user
> + * space.
> + */
> + new_rs485.flags &= SER_RS485_ENABLED;
> + new_rs485.delay_rts_before_send = 0;
> + new_rs485.delay_rts_after_send = 0;
Perhaps you should simply clear the whole struct and reset the supported
flags?
> + if (copy_to_user(new_arg, &new_rs485, sizeof(new_rs485)))
> + return -EFAULT;
> +
> + return err;
> +}
> +
> +static int mxuport_get_rs485(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct serial_rs485 __user *arg)
> +{
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct serial_rs485 rs485;
> +
> + memset(&rs485, 0, sizeof(rs485));
> + if (mxport->interface == MX_INT_2W_RS485 ||
> + mxport->interface == MX_INT_4W_RS485)
> + rs485.flags |= SER_RS485_ENABLED;
> +
> + if (copy_to_user(arg, &rs485, sizeof(rs485)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +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;
> + unsigned int old_cflag = 0, old_iflag = 0;
> + unsigned long flags;
> + struct usb_serial *serial = port->serial;
> + u8 data_bits, parity, stop_bits, flow_ctrl = 0;
> + int baud, err;
> + unsigned char *buf;
> + char xon = START_CHAR(tty);
> + char xoff = STOP_CHAR(tty);
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + /* Check that they really want us to change something */
> + cflag = termios->c_cflag;
> + iflag = termios->c_iflag;
> +
> + if (old_termios) {
> + old_iflag = old_termios->c_iflag;
> + old_cflag = old_termios->c_cflag;
> + }
> +
> + if (cflag == old_cflag &&
> + iflag == old_iflag) {
I'd drop old_cflag and old_iflag (and iflag) and simply do something like
if (old_termios &&
!tty_tty_termios_hw_change(&tty->termios, old_termios) &&
tty->termios->c_iflag == old_termios->c_iflag) {
> + dev_dbg(&port->dev, "%s - nothing to change\n", __func__);
> + return;
> + }
> +
> + dev_dbg(&port->dev, "%s - old clfag %08x old iflag %08x\n",
> + __func__, old_cflag, old_iflag);
You could drop this.
> +
> + buf = kmalloc(4, GFP_KERNEL);
> + if (!buf)
> + return;
> +
> + cflag = tty->termios.c_cflag;
> +
> + /* Set data bit of termios */
> + switch (cflag & CSIZE) {
You could use the C_SIZE(tty) macro and friends for the cflags as you
do for the iflags below (and get rid of the cflag-variable as well).
> + case CS5:
> + data_bits = MX_WORDLENGTH_5;
> + dev_dbg(&port->dev, "%s - data bits = 5\n", __func__);
> + break;
> + case CS6:
> + data_bits = MX_WORDLENGTH_6;
> + dev_dbg(&port->dev, "%s - data bits = 6\n", __func__);
> + break;
> + case CS7:
> + data_bits = MX_WORDLENGTH_7;
> + dev_dbg(&port->dev, "%s - data bits = 7\n", __func__);
> + break;
> + case CS8:
> + default:
> + 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) {
> + parity = MX_PARITY_MARK;
> + dev_dbg(&port->dev, "%s - parity = mark\n",
> + __func__);
> + } else {
> + parity = MX_PARITY_SPACE;
> + dev_dbg(&port->dev, "%s - parity = space\n",
> + __func__);
> + }
> + } else if (cflag & PARODD) {
> + parity = MX_PARITY_ODD;
> + dev_dbg(&port->dev, "%s - parity = odd\n", __func__);
> + } else {
> + parity = MX_PARITY_EVEN;
> + dev_dbg(&port->dev, "%s - parity = even\n", __func__);
> + }
> + } else {
> + parity = MX_PARITY_NONE;
> + dev_dbg(&port->dev, "%s - parity = none\n", __func__);
> + }
> +
> + /* Set stop bit of termios */
> + if (cflag & CSTOPB) {
> + stop_bits = MX_STOP_BITS_2;
> + dev_dbg(&port->dev, "%s - stop bits = 2\n", __func__);
> + } else {
> + stop_bits = MX_STOP_BITS_1;
> + dev_dbg(&port->dev, "%s - stop bits = 1\n", __func__);
> + }
> +
> + /* Submit the vendor request */
> + buf[0] = data_bits;
> + buf[1] = parity;
> + buf[2] = stop_bits;
> + buf[3] = 0;
> +
> + err = mxuport_send_ctrl_data_urb(serial, RQ_VENDOR_SET_LINE,
> + 0, port->port_number, buf, 4);
> +
> + if (err != 4)
> + goto out;
> +
> + /* H/W flow control settings */
> + if ((cflag & CRTSCTS) && (cflag & CBAUD)) {
You should probably ignore CBAUD here.
> + flow_ctrl |= MX_HW_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - RTS/CTS is enabled\n", __func__);
> + } else {
> + flow_ctrl &= ~MX_HW_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - RTS/CTS is disabled\n", __func__);
> + }
> +
> + /* Submit the vendor request */
> + if (flow_ctrl & MX_HW_FLOWCTRL) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RTS,
> + MX_HW_FLOWCTRL_ENABLE,
> + port->port_number);
> + if (err)
> + goto out;
> + } else {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RTS,
> + MX_HW_FLOWCTRL_DISABLE,
> + port->port_number);
> + if (err)
> + goto out;
> + }
Why not merge the two conditionals above? You probably don't want to
raise RTS (MX_HW_FLOWCTRL_DISABLE) unless actually disabling hw flow
control.
> +
> + baud = tty_get_baud_rate(tty);
> +
> + /* Reassert DTR on transition from B0 */
> + if ((old_cflag & CBAUD) == B0) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> + 1, port->port_number);
> + if (err)
> + goto out;
> + }
> + /* Deassert DTR & RTS on transition to B0 */
> + if ((cflag & CBAUD) == B0) {
> + spin_lock_irqsave(&port->lock, flags);
> +
> + mxport->mcr_state &= ~(UART_MCR_DTR | UART_MCR_RTS);
> +
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_MCR,
> + mxport->mcr_state,
> + port->port_number);
Again, mxuport_send_ctrl_urb cannot be used in atomic context. Use a
mutex.
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + if (err)
> + goto out;
> + }
DTR should only be raised if transitioning from B0. Use something like
if (C_BAUD(tty)) {
if (old_termios && old_termios->c_cflag & CBAUD == B0)
/* raise DTR/RTS */
} else {
/* drop DTR/RTS */
}
Remember to update mcr_state in both branches.
> +
> + /* S/W flow control settings */
> + if (I_IXOFF(tty) || I_IXON(tty)) {
> +
> + /* Submit the vendor request */
> + buf[0] = (unsigned char)xon;
> + buf[1] = (unsigned char)xoff;
> +
> + err = mxuport_send_ctrl_data_urb(serial, RQ_VENDOR_SET_CHARS,
> + 0, port->port_number,
> + buf, 2);
> +
> + if (err != 2)
> + goto out;
> + }
> +
> + /* If we are implementing outbound XON/XOFF */
> + if (I_IXON(tty)) {
> + 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 {
> + 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)) {
> + 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 {
> + flow_ctrl &= ~MX_XOFF_FLOWCTRL;
> + dev_dbg(&port->dev, "%s - inbound XON/XOFF is disabled\n",
> + __func__);
> + }
> +
> + /* Submit the vendor request */
> + if (flow_ctrl & (MX_XON_FLOWCTRL | MX_XOFF_FLOWCTRL)) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_XONXOFF,
> + 1, port->port_number);
> + if (err)
> + goto out;
> + } else {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_XONXOFF,
> + 0, port->port_number);
> + if (err)
> + goto out;
> + }
> +
> + /* Submit the vendor request - Little Endian */
> + put_unaligned_le32(baud, buf);
> +
> + err = mxuport_send_ctrl_data_urb(serial, RQ_VENDOR_SET_BAUD,
> + 0, port->port_number,
> + buf, 4);
> +
> + if (err != 4)
> + goto out;
> +
> + /* Dump serial settings */
> + dev_dbg(&port->dev, "baud_rate : %d\n", baud);
> + dev_dbg(&port->dev, "data_bits : %d\n", data_bits);
> + dev_dbg(&port->dev, "parity : %d\n", parity);
> + dev_dbg(&port->dev, "stop_bits : %d\n", stop_bits);
> + dev_dbg(&port->dev, "flow_ctrl : %d\n", flow_ctrl);
You'd want %x here.
> + dev_dbg(&port->dev, "xon : 0x%x\n", xon);
> + dev_dbg(&port->dev, "xoff : 0x%x\n", xoff);
> +
> +out:
> + kfree(buf);
> + return;
> +}
This function is some 240 lines long. Perhaps you could consider
splitting it up (e.g. separate handling of software flow control)?
> +
> +/*
> + * Handles any ioctl calls to the driver. This mostly means RS485
> + * operations.
> + */
> +static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> +
> + switch (cmd) {
> +
> + case TIOCGSERIAL:
> + dev_dbg(&port->dev, "%s - TIOCGSERIAL\n", __func__);
> + return mxuport_get_serial_info(
> + port, (struct serial_struct __user *)arg);
> + break;
> +
> + case TIOCSRS485:
> + dev_dbg(&port->dev, "%s - TIOCSRS485\n", __func__);
> + return mxuport_set_rs485(tty, port,
> + (struct serial_rs485 __user *)arg);
> + break;
> +
> + case TIOCGRS485:
> + dev_dbg(&port->dev, "%s - TIOGSRS485\n", __func__);
> + return mxuport_get_rs485(tty, port,
> + (struct serial_rs485 __user *)arg);
> + break;
> +
> + default:
> + return -ENOIOCTLCMD;
> + }
> +
> + return 0;
> +}
Remove the breaks after returns. I'd also remove the empty lines from
the switch statement.
> +
> +
> +/*
> + * 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.
> + */
Multi-line comment style suggests an opening /*\n.
> +static int mxuport_get_fw_version(struct usb_serial *serial,
> + unsigned char *ver)
> +{
> + int err;
> + unsigned char *ver_buf;
> +
> + ver_buf = kzalloc(4, GFP_KERNEL);
> + if (!ver_buf)
> + return -ENOMEM;
> +
> + /* Send vendor request - Get firmware version from SDRAM */
> + err = mxuport_recv_ctrl_urb(serial, 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_serial *serial,
> + const struct firmware *fw_p)
> +{
> + int err;
> + size_t txlen, fwidx;
> + unsigned char *fw_buf = NULL;
> +
> + fw_buf = kmalloc(DOWN_BLOCK_SIZE, GFP_KERNEL);
> + if (fw_buf == NULL)
> + return -ENOMEM;
> +
> + dev_dbg(&serial->interface->dev, "Starting download firmware ...\n");
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_START_FW_DOWN, 0, 0);
> + if (err)
> + goto out;
> +
> + 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(serial, RQ_VENDOR_FW_DATA,
> + 0, 0, fw_buf, txlen);
> + if (err != txlen)
> + goto out;
Do you want to stop the FW_DOWN on errors here?
> +
> + fwidx += txlen;
> + usleep_range(1000, 2000);
> +
> + } while (fwidx < fw_p->size);
> +
> + msleep(1000);
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_STOP_FW_DOWN, 0, 0);
> + if (err)
> + goto out;
> +
> + msleep(1000);
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_QUERY_FW_READY, 0, 0);
> +
> +out:
> + kfree(fw_buf);
> + return err;
> +}
> +
> +static int mxuport_probe(struct usb_serial *serial,
> + const struct usb_device_id *id)
> +{
> + u16 productid = le16_to_cpu(serial->dev->descriptor.idProduct);
> + 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(serial, RQ_VENDOR_QUERY_FW_CONFIG, 0, 0);
> + if (err) {
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_RESET_DEVICE, 0, 0);
> + return err;
> + }
> +
> + dev_ver = mxuport_get_fw_version(serial, ver_buf);
> + if (dev_ver < 0) {
> + err = dev_ver;
> + goto out;
> + }
> +
> + dev_dbg(&serial->interface->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, &serial->interface->dev);
> + if (err) {
> + dev_warn(&serial->interface->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(&serial->interface->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(serial, fw_p);
> + if (err)
> + goto out;
> + dev_ver = mxuport_get_fw_version(serial, ver_buf);
> + if (dev_ver < 0) {
> + err = dev_ver;
> + goto out;
> + }
> + }
> + }
> +
> + usb_set_serial_data(serial, (void *)id->driver_info);
> +
> + dev_info(&serial->interface->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 usb_serial *serial = port->serial;
> + struct mxuport_port *mxport;
> + int err;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
You can drop this.
> +
> + mxport = devm_kzalloc(&port->dev, sizeof(struct mxuport_port),
> + GFP_KERNEL);
> + if (!mxport)
> + return -ENOMEM;
> +
> + /* Set the port private data */
> + usb_set_serial_port_data(port, mxport);
> +
> + /* Send vendor request - set FIFO (Enable) */
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_FIFO_DISABLE,
> + 0, port->port_number);
> + if (err)
> + return err;
> +
> + /* Send vendor request - set transmission mode (Hi-Performance) */
> + err = mxuport_send_ctrl_urb(serial, 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, RQ_VENDOR_SET_INTERFACE,
> + MX_INT_RS232,
> + port->port_number);
> + if (err)
> + return err;
> +
> + return device_create_file(&port->dev, &dev_attr_4_wire_rs485);
> +}
> +
> +static int mxuport_port_remove(struct usb_serial_port *port)
> +{
> + device_remove_file(&port->dev, &dev_attr_4_wire_rs485);
> +
> + return 0;
> +}
> +
> +static int mxuport_alloc_write_urb(struct usb_serial *serial,
> + struct usb_serial_port *port,
> + struct usb_serial_port *port0,
> + int j)
> +{
> + struct usb_device *dev = interface_to_usbdev(serial->interface);
> +
> + set_bit(j, &port->write_urbs_free);
> + port->write_urbs[j] = usb_alloc_urb(0, GFP_KERNEL);
> + if (!port->write_urbs[j])
> + return -ENOMEM;
> +
> + port->bulk_out_buffers[j] = kmalloc(port0->bulk_out_size, GFP_KERNEL);
> + if (!port->bulk_out_buffers[j])
> + return -ENOMEM;
> +
> + usb_fill_bulk_urb(port->write_urbs[j], dev,
> + usb_sndbulkpipe(dev, port->bulk_out_endpointAddress),
> + port->bulk_out_buffers[j],
> + port->bulk_out_size,
> + serial->type->write_bulk_callback,
> + port);
> + return 0;
> +}
> +
> +
> +static int mxuport_alloc_write_urbs(struct usb_serial *serial,
> + struct usb_serial_port *port,
> + struct usb_serial_port *port0)
> +{
> + int j, ret;
You should try sticking to one declaration per line here and elsewhere.
> +
> + for (j = 0; j < ARRAY_SIZE(port->write_urbs); ++j) {
> + ret = mxuport_alloc_write_urb(serial, port, port0, j);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
> +
> +
> +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 usb_device *udev;
> + int err = -ENOMEM;
No need to initialise, right?
> + int i, j;
> +
> + /* Get product info from device */
> + udev = serial->dev;
You never use udev.
> +
> + /*
> + * Throw away all but the first allocated write URBs so we can
> + * set them up again to fit the multiplexing scheme.
> + */
> + for (i = 1; 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;
> + }
> +
> + /*
> + * 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.
> + */
> + for (i = 1; i < serial->num_ports; ++i) {
> + port = serial->port[i];
> + port->bulk_out_size = port0->bulk_out_size;
> + port->bulk_out_endpointAddress =
> + port0->bulk_out_endpointAddress;
> +
> + err = mxuport_alloc_write_urbs(serial, port, port0);
> + if (err)
> + goto out;
> +
> + 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;
> + port->bulk_in_size = 0;
> + }
> + }
> +
> + /* 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);
dev_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);
dev_err?
> + goto out;
> + }
> + return 0;
> +
> +out:
> + /* Shutdown any bulk transfers that might be going on */
> + usb_serial_generic_close(port1);
> + usb_serial_generic_close(port0);
> +
> + usb_set_serial_data(serial, NULL);
Not needed.
> +
> + return err;
> +}
> +
> +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->msr_state = 0;
Do you want to use RQ_VENDOR_GET_MSR here?
> + mxport->mcr_state = UART_MCR_DTR | UART_MCR_RTS;
The tty-port implementation will call dtr_rts which will set mcr_state
shortly after open returns.
> +
> + return err;
> +}
> +
> +static void mxuport_close(struct usb_serial_port *port)
> +{
> + struct usb_serial *serial = port->serial;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
You should drop this one as well.
> +
> + /*
> + * Send vendor request to device to tell it to close the
> + * port
> + */
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_OPEN, 0,
> + port->port_number);
> +}
What about RQ_VENDOR_SET_RX_HOST_EN?
> +
> +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;
> + mcr = mxport->mcr_state;
> +
> + spin_unlock_irqrestore(&port->lock, flags);
You need to update this if you use a mutex for mcr.
> +
> + 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;
> +}
How about moving this function after (or before) tiocmset?
> +
> +/* Send 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, RQ_VENDOR_SET_BREAK,
> + 1, port->port_number);
> + } else {
> + dev_dbg(&port->dev, "%s - clearing break\n", __func__);
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_BREAK,
> + 0, port->port_number);
> + }
You never check err above. Drop or add dev_err/dbg?
> +
> + return;
Drop this.
> +}
> +
> +static struct usb_serial_driver mxuport_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "mx-uport",
Use "mxuport" here to match the filename?
> + },
> + .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,
Will be set by usb-serial core if left unset.
> + .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,
> + .dtr_rts = mxuport_dtr_rts,
> + .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");
> --
> 1.8.4.rc3
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
