On Wed, Oct 23, 2013 at 02:51:49PM +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.
>
> 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
>
> v3->v4
Thanks for addressing most of my comments to v3.
Please remember to add the version to the subject prefix (e.g. "PATCH
v4"), and if possible try to send v5 as a reply to this mail. The git
send-email command has an --in-reply-to switch if you want to use that.
> Remove support for rs485 - No hardware to test it on.
> Fix formatting of multi line comments.
> Use HEADER_SIZE instead of hard coded 4.
> Use EIO instread of ECOMM.
> Drop use of spinlocks on mxuport_{un}throttle().
> Remove unneeded parenthesis.
> Split mxuport_process_read_urb_event() into a number of functions.
> Check for zero bytes of data.
> Don't needlessly initialize variables.
> Fold mxuport_port_init() into mxuport_port_open()
> Correctly handle the on parameter in mxuport_dtr_rts().
> Drop mxuport_get_serial_info and the now empty ioctl handler.
> Splitup mxuport_set_termios() into smaller functions
> Make use of C_* macros for cflags.
> Terminate the firmware download on error.
> Remove some unneeded dev_dbg()'s.
> Let usb-serial core fill .write function in usb_serial_driver.
> Change usb_serial_driver.name to match filename.
> Move tiocmget() and tiocmset() next to each other.
> Only one declaration per line.
> Remove unused udev
> Convert a few dev_dbg() to dev_err() for real errors.
> Remove unneeded usb_set_serial_data(serial, NULL).
> Use RQ_VENDOR_SET_RX_HOST_EN in mxuport_close()
> Replace the spinlock protecting msr and lsr state with a mutex.
You still need the spinlock for MSR. More details below.
> Add a custom resume function.
>
> Depends on the patch:
>
> http://comments.gmane.org/gmane.linux.kernel/1584028
>
> For C_MSPAR(tty)
Adding a macro for CMSPAR is a good idea (although it should be called
C_CMSPAR), and as we discussed last week, you should submit both patches
as a series so that Greg can take both through the USB-tree.
> ---
> drivers/usb/serial/Kconfig | 29 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/mxuport.c | 1471 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1501 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.
That would be mxuport.
> +
> 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..1b7c2b5
> --- /dev/null
> +++ b/drivers/usb/serial/mxuport.c
> @@ -0,0 +1,1471 @@
> +/*
> + * mxuport.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/ioctl.h>
You don't need input.h or ioctl.h (anymore).
> +#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 */
Apparently some missing indentation here (before the value) when
looking at the applied patch.
> +#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 in input buffer */
> +#define RQ_VENDOR_GET_OUTQUEUE 0x85 /* Data 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 */
> + struct mutex mutex;
> +};
> +
> +/* 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);
> +
> +/*
> + * 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 + HEADER_SIZE,
> + size - HEADER_SIZE,
> + &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 + HEADER_SIZE;
> +}
> +
> +/* 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 -EIO;
> + }
> +
> + 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 -EIO;
> + }
> +
> + 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;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN,
> + 0, port->port_number);
> +}
> +
> +/*
> + * 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;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN,
> + 1, port->port_number);
> +}
> +
> +/*
> + * 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)
How about just calling the arguments data and length (or size)?
> +{
> + 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);
> +}
> +
> +
> +static void mxuport_msr_event(struct usb_serial_port *port,
> + struct mxuport_port *mxport,
> + unsigned char *buf)
The event buffers are always four byte, right? Perhaps you should pass
them as
unsigned char buf[4]
(or use u8 buf[4]) throughout.
> +{
> + unsigned char rcv_msr_hold = buf[2] & 0xF0;
> + unsigned short rcv_msr_event = get_unaligned_be16(buf);
I think should consider using u16 here and throughout (e.g. for port
numbers and length below) instead of short.
> +
> + dev_dbg(&port->dev, "%s - current MSR status = 0x%x\n",
> + __func__, mxport->msr_state);
Move under the lock below.
> +
> + /* Update hold reason */
> + if (rcv_msr_event != 0) {
As I mentioned before, you only need to check the rcv_msr_event once and
return if it's not set in the beginning of the function. That way you
can reduce the indentation of most of the function one level.
> + mutex_lock(&mxport->mutex);
I said in my review of v3 that you needed a mutex to protect the
modem-control register (MCR). However, you still need to use a spinlock
for the modem-status register (MSR) as you cannot sleep in a completion
handler (process_read_urb).
> +
> + if (rcv_msr_hold & UART_MSR_CTS) {
> + mxport->msr_state |= UART_MSR_CTS;
> + dev_dbg(&port->dev, "%s - CTS high\n",
> + __func__);
> + } else {
> + mxport->msr_state &= ~UART_MSR_CTS;
> + dev_dbg(&port->dev, "%s - CTS low\n",
> + __func__);
> + }
> +
> + if (rcv_msr_hold & UART_MSR_DSR) {
> + mxport->msr_state |= UART_MSR_DSR;
> + dev_dbg(&port->dev, "%s - DSR high\n",
> + __func__);
> + } else {
> + mxport->msr_state &= ~UART_MSR_DSR;
> + dev_dbg(&port->dev, "%s - DSR low\n",
> + __func__);
> + }
> +
> + if (rcv_msr_hold & UART_MSR_DCD) {
> + mxport->msr_state |= UART_MSR_DCD;
> + dev_dbg(&port->dev, "%s - DCD high\n",
> + __func__);
> + } else {
> + mxport->msr_state &= ~UART_MSR_DCD;
> + dev_dbg(&port->dev, "%s - DCD low\n",
> + __func__);
> + }
> + mutex_unlock(&mxport->mutex);
> + }
> +
> + /* Update MSR status */
> + if (rcv_msr_event &
> + (SERIAL_EV_CTS | SERIAL_EV_DSR | SERIAL_EV_RLSD)) {
> + mutex_lock(&mxport->mutex);
No need for locking.
> +
> + 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);
> + mutex_unlock(&mxport->mutex);
> + }
> +}
> +
> +static void mxuport_lsr_event(struct usb_serial_port *port,
> + unsigned char *buf)
unsigned char buf[4]?
> +{
> + unsigned char 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__);
> + }
> +}
> +
> +/*
> + * 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 char buf[4]
> + unsigned long event)
> +{
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +
> + 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.
> + */
> + break;
> +
> + case UPORT_EVNET_MSR:
> + mxuport_msr_event(port, mxport, buf);
> + break;
> +
> + case UPORT_EVNET_LSR:
> + mxuport_lsr_event(port, buf);
> + 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.
> + */
> + break;
> +
> + default:
> + break;
> + }
You can remove the empty lines after breaks.
> +}
> +
> +/*
> + * 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;
> + unsigned short 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);
> + if (!rcv_len || 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)) {
> + ch = data + HEADER_SIZE;
> + mxuport_process_read_urb_data(demux_port, ch, rcv_len);
> + } else {
> + dev_dbg(&demux_port->dev, "%s - data for closed port\n",
> + __func__);
> + }
> + data += HEADER_SIZE + rcv_len;
> + }
> +}
> +
> +/*
> + * 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;
> + unsigned short 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)) {
> + ch = data + HEADER_SIZE;
> + rcv_event = get_unaligned_be16(data + 2);
> + mxuport_process_read_urb_event(demux_port, ch,
> + rcv_event);
> + } else {
> + dev_dbg(&demux_port->dev,
> + "%s - event for closed port\n", __func__);
> + }
> + 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;
> +}
> +
> +/* 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;
> + unsigned int mcr;
> + int err = 0;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + mutex_lock(&mxport->mutex);
> +
> + 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);
> + 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:
> + mutex_unlock(&mxport->mutex);
> +
> + return err;
> +}
> +
> +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;
> +
> + mxport = usb_get_serial_port_data(port);
> +
> + mutex_lock(&mxport->mutex);
> +
> + msr = mxport->msr_state;
MSR still needs a spinlock as mentioned above.
> + mcr = mxport->mcr_state;
> +
> + mutex_unlock(&mxport->mutex);
> +
> + 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;
> +}
> +
> +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;
> + int err;
> +
> + mutex_lock(&mxport->mutex);
> +
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> + !!on, port->port_number);
> + if (err) {
> + dev_dbg(&port->dev, "%s - fail to change DTR\n", __func__);
> + goto out;
> + }
> + mxport->mcr_state &= ~UART_MCR_DTR;
Put this in an else-branch below.
> + if (on)
> + mxport->mcr_state |= UART_MCR_DTR;
> +
> + 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.
> + if (on)
> + mxport->mcr_state |= UART_MCR_RTS;
> +
> +out:
> + mutex_unlock(&mxport->mutex);
> +}
> +
> +static int mxuport_set_termios_flow(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct usb_serial *serial,
> + char *buf)
> +{
> + unsigned long flow_ctrl = 0;
> + char xon = START_CHAR(tty);
> + char xoff = STOP_CHAR(tty);
> + int err;
> +
> + /* S/W flow control settings */
> + if (I_IXOFF(tty) || I_IXON(tty)) {
> +
> + /* Submit the vendor request */
Remove blank line and comment.
> + 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 */
Comment not not needed.
> + if (flow_ctrl & (MX_XON_FLOWCTRL | MX_XOFF_FLOWCTRL)) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_XONXOFF,
> + 1, port->port_number);
> + } else {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_XONXOFF,
> + 0, port->port_number);
> + }
Please rewrite this is one mxuport_send_ctrl_urb with an variable
argument instead of having to calls.
> + if (err)
> + goto out;
> +
> + /* H/W flow control settings */
> + if (C_CRTSCTS(tty)) {
> + 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 */
Again, not needed.
> + if (flow_ctrl & MX_HW_FLOWCTRL) {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RTS,
> + MX_HW_FLOWCTRL_ENABLE,
> + port->port_number);
> + } else {
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RTS,
> + MX_HW_FLOWCTRL_DISABLE,
> + port->port_number);
> + }
Please rewrite this is one mxuport_send_ctrl_urb with an variable
argument instead of having to calls.
I noticed that RQ_VENDOR_SET_RTS is used both to control RTS directly
(e.g. in tiocmset and dtr_rts) and to enable hw-flow control. It's
basically tri-state: off, on and auto. And MX_HW_FLOWCTRL_DISABLE above
is defined as 1 and actually raises RTS. You might want to clarify this
by using the same set of defines, and at least drop
MX_HW_FLOWCTRL_DISABLE above as its side-effect is not obvious.
This should also be thought through in relation to the B0 handling below
(more comments follow).
You may need to reintroduce the C_BAUD check when setting hw-flow so
that whether to disable, and if so, which state to set depends also on
if B0 is requested or not.
> +
> +out:
> + return err;
> +}
> +
> +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;
> + struct usb_serial *serial = port->serial;
> + unsigned char *buf;
> + unsigned int iflag;
> + u8 data_bits;
> + u8 stop_bits;
> + u8 parity;
> + int baud;
> + int err;
> +
> + dev_dbg(&port->dev, "%s\n", __func__);
> +
> + /* Check that they really want us to change something */
> + iflag = termios->c_iflag;
> +
> + if (old_termios &&
> + !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;
> + }
> +
> + buf = kmalloc(4, GFP_KERNEL);
> + if (!buf)
> + return;
> +
> + /* Set data bit of termios */
> + switch (C_CSIZE(tty)) {
> + 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;
> + }
Why not simply drop the debugging here? You are already printing the
result at the end of set_termios. At least reduce it to one dev_dbg as
the constants apparently are 5..8.
> +
> + /* Set parity of termios */
> + if (C_PARENB(tty)) {
> + if (C_MSPAR(tty)) {
> + if (C_PARODD(tty)) {
> + 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 (C_PARODD(tty)) {
> + 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__);
> + }
You could possibly also drop the debug statements here as well. You are
already printing the result at the end of set_termios, albeit in encoded
form.
> +
> + /* Set stop bit of termios */
> + if (C_CSTOPB(tty)) {
> + 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__);
> + }
Here too.
> +
> + /* 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;
> +
> + if (C_BAUD(tty)) {
> + if (old_termios && (old_termios->c_cflag & CBAUD) == B0) {
> + /* Raise DTR/RTS */
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> + 1, port->port_number);
> + if (err)
> + goto out;
> +
> + mxport->mcr_state |= (UART_MCR_DTR | UART_MCR_RTS);
> + } else {
> + /* Drop DTR/RTS */
> + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> + 0, port->port_number);
> + if (err)
> + goto out;
> + 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);
> + if (err)
> + goto out;
> + }
This isn't quite right. You want to raise DTR and (possibly) RTS when
transitioning from B0 (or when !old_termios), and drop DTR and RTS when
B0 is requested. That is, something like:
if (C_BAUD(tty)) {
if (old_termios && old_termios->c_cflag & CBAUD == B0)
/* raise DTR/RTS */
} else {
/* drop DTR/RTS */
}
Looks like you just put the else branch on the wrong conditional.
In fact, to properly handle hardware-assisted hardware flow control, it
should possibly even be:
if (C_BAUD(tty)) {
if (old_termios && old_termios->c_cflag & CBAUD == B0) {
if (!C_CRTSCTS(tty))
/* raise RTS */
/* raise DTR */
} else {
/* drop DTR/RTS (and disable hw-flow) */
}
You also need locking for mcr_state (mutex).
You probably don't want to use both SET_DTR and SET_MCR. If you can
manipulate both DTR and RTS in one call (SET_MCR), why not use that here
as well as in in dtr_rts and tiocmset?
You should move these control signal-manipulations to
mxport_set_termios_flow above as they have dependencies (mentioned
above).
> +
> + err = mxuport_set_termios_flow(tty, port, serial, buf);
> + if (err)
> + goto out;
> +
> + baud = tty_get_baud_rate(tty);
You need to handle B0 here as well. The praxis is to use 9600 baud when
B0 is requested (rather than trying to set 0 baud in hardware). That is,
if (!baud)
baud = 9600;
should do the trick.
> +
> + /* 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);
> +
> +out:
> + kfree(buf);
> +}
> +
> +/*
> + * 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;
> +}
Do you think you could encode this information in the device table
using the driver_info field as you did for the quirk handling (now
dropped)?
I'm trying to keep all device-specific data in one place rather than
spreading it all over drivers. This could be ok for now, but if anyone
starts adding devices with different VID as well as PID it quickly
becomes a mess.
That is, add something like:
#define MX_UPORT_2_PORT 0x01
#define MX_UPORT_4_PORT 0x02
#define MX_UPORT_8_PORT 0x04
...
{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1618_PID),
.driver_info = MX_UPORT_8_PORT },
and use usb_set_serial_data(serial, (void *)id->driver_info) in probe
and usb_get_serial_data in calc_num_ports.
This way it would also be easy to add back quirks if ever needed.
> +
> +/*
> + * 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_serial *serial,
> + unsigned char *ver)
Use unsigned char ver[3].
I also think it'd be cleaner if you just pass a (u32 *) to store the
version as an integer and simply return 0 or negative error here.
> +{
> + unsigned char *ver_buf;
> + int err;
> +
> + 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)
> +{
> + unsigned char *fw_buf = NULL;
> + size_t txlen;
> + size_t fwidx;
> + int err;
> +
> + 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) {
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_STOP_FW_DOWN,
> + 0, 0);
> + goto out;
> + }
> +
> + 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);
> + const struct firmware *fw_p = NULL;
> + unsigned char ver_buf[3];
> + int local_ver;
u32?
> + char buf[32];
> + int dev_ver;
u32?
> + 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;
> + }
> + }
> + }
> +
> + 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;
> +
> + mxport = devm_kzalloc(&port->dev, sizeof(struct mxuport_port),
> + GFP_KERNEL);
> + if (!mxport)
> + return -ENOMEM;
> +
> + mutex_init(&mxport->mutex);
> +
> + /* 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 err;
> +}
> +
> +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;
> + int ret;
> +
> + 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;
> + int err;
> + int i;
> + int j;
> +
> + /*
> + * 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_err(&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_err(&port1->dev,
> + "%s - USB submit read bulk urb failed. (status = %d)\n",
> + __func__, 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);
> +
> + return err;
> +}
> +
> +static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + int err;
> +
> + 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);
Do you want to enable reception before opening?
> +
> + if (err) {
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_OPEN, 0,
> + port->port_number);
> +
> + return err;
> + }
> +
> + /* Initial port termios */
> + mxuport_set_termios(tty, port, NULL);
> +
> + /* Initial private parameters of port */
> + mxport->msr_state = 0;
Again, why not use RQ_VENDOR_GET_MSR here?
> +
> + return err;
> +}
> +
> +static void mxuport_close(struct usb_serial_port *port)
> +{
> + struct usb_serial *serial = port->serial;
> +
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_OPEN, 0,
> + port->port_number);
> +
> + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN, 0,
> + port->port_number);
> +
> +}
> +
> +/* 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);
> + }
As I said before: You never check err above. Drop or add dev_err/dbg?
> +
> + return;
Not needed.
> +}
> +
> +static int mxuport_resume(struct usb_serial *serial)
> +{
> + struct usb_serial_port *port;
> + int c = 0;
> + int i;
> + int r;
> +
> + for (i = 0; i < 2; i++) {
> + port = serial->port[i];
> +
> + r = usb_serial_generic_submit_read_urbs(port, GFP_NOIO);
> + if (r < 0)
> + c++;
> + }
> +
> + for (i = 0; i < serial->num_ports; i++) {
> + port = serial->port[i];
> + if (!test_bit(ASYNCB_INITIALIZED, &port->port.flags))
> + continue;
> +
> + r = usb_serial_generic_write_start(port, GFP_NOIO);
> + if (r < 0)
> + c++;
> + }
> +
> + return c ? -EIO : 0;
> +}
> +
> +static struct usb_serial_driver mxuport_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "mxuport",
> + },
> + .description = "MOXA UPort",
> + .id_table = mxuport_idtable,
> + .num_ports = 0,
> + .probe = mxuport_probe,
> + .port_probe = mxuport_port_probe,
> + .attach = mxuport_attach,
> + .calc_num_ports = mxuport_calc_num_ports,
> + .open = mxuport_open,
> + .close = mxuport_close,
> + .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,
> + .resume = mxuport_resume,
> +};
> +
> +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
