Re: [PATCH v5 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 08, 2013 at 02:19:55PM +0100, 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, such 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
> 
> 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
> 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.
> Add a custom resume function.
> 
> v4->v5
> 
> Rename C_MSPAR(tty) to C_CMSPAR(tty)
> Add the C_CMSPAR(tty) macro patch to the patchset.
> Fix name of module in Kconfig.
> Remove unneeded include files.
> Add missing tab in #define.
> Use data & size rather than ch & rcv_len.
> unsigned char -> u8
> unsigned short -> u16
> unsigned long -> u32
> Use u8[4] for event, not u8 *.
> Exit out of mxuport_msr_event() when rcv_msr_event is 0.
> Remove empty lines after breaks.
> Move bits of code into else clause.
> Drop dev_dbg() statements in mxuport_set_termios
> Don't set the hardware to 0 baud, use 9600 instead.
> Express number of ports as a quirk.
> Pass firmware version around as a u32.
> mxuport_break_ctl, drop err, and rely on mxuport_send_ctrl_urb
> reporting problems.
> Protect mcr_state with a spinlock.
> Implement  mxuport_dtr_rts() using RQ_VENDOR_SET_MCR.
> Add helpers for manipulating RTS and DTS.
> Try again with B0, RTS/DTS, HW flow control.

Thanks for the update. One more iteration (or two) and I think we could
be done. :)

Some comments below.

> ---
>  drivers/usb/serial/Kconfig   |   29 +
>  drivers/usb/serial/Makefile  |    1 +
>  drivers/usb/serial/mxuport.c | 1440 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1470 insertions(+)
>  create mode 100644 drivers/usb/serial/mxuport.c
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index ddb9c51f2c99..f023e3221b89 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 mxuport.
> +
>  config USB_SERIAL_NAVMAN
>  	tristate "USB Navman GPS device"
>  	help
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index 42670f0b5bc0..9b5d6b346737 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 000000000000..c444261eda1f
> --- /dev/null
> +++ b/drivers/usb/serial/mxuport.c
> @@ -0,0 +1,1440 @@
> +/*
> + *	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/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 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 */

s/EVNET/EVENT/ ?

> +
> +/* 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_RTS_DISABLE	0x0
> +#define MX_HW_FLOWCTRL_RTS_ENABLE	0x1
> +#define MX_HW_FLOWCTRL_ENABLE		0x2
> +
> +#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
> +
> +#define MX_UPORT_2_PORT			BIT(0)
> +#define MX_UPORT_4_PORT			BIT(1)
> +#define MX_UPORT_8_PORT			BIT(2)
> +#define MX_UPORT_16_PORT		BIT(3)
> +
> +/* 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;	/* Protects mcr_state */
> +	spinlock_t spinlock;	/* Protects msr_state */
> +};
> +
> +/* Table of devices that work with this driver */
> +static struct usb_device_id mxuport_idtable[] = {
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1250_PID),
> +	  .driver_info = MX_UPORT_2_PORT },
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1251_PID),
> +	  .driver_info = MX_UPORT_2_PORT },
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1410_PID),
> +	  .driver_info = MX_UPORT_4_PORT },
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1450_PID),
> +	  .driver_info = MX_UPORT_4_PORT },
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1451_PID),
> +	  .driver_info = MX_UPORT_4_PORT },
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1618_PID),
> +	  .driver_info = MX_UPORT_8_PORT },
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1658_PID),
> +	  .driver_info = MX_UPORT_8_PORT },
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1613_PID),
> +	  .driver_info = MX_UPORT_16_PORT },
> +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1653_PID),
> +	  .driver_info = MX_UPORT_16_PORT },
> +	{}			/* 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)
> +{
> +	u8 *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 *data, int size)
> +{
> +	int i;
> +
> +	if (!port->port.console || !port->sysrq) {
> +		tty_insert_flip_string(&port->port, data, size);
> +	} else {
> +		for (i = 0; i < size; i++, data++) {
> +			if (!usb_serial_handle_sysrq_char(port, *data))
> +				tty_insert_flip_char(&port->port, *data,
> +						     TTY_NORMAL);
> +		}
> +	}
> +	tty_flip_buffer_push(&port->port);
> +}
> +
> +static void mxuport_msr_event(struct usb_serial_port *port,
> +			      struct mxuport_port *mxport,
> +			      u8 buf[4])
> +{
> +	u8 rcv_msr_hold = buf[2] & 0xF0;
> +	u16 rcv_msr_event = get_unaligned_be16(buf);
> +	unsigned long flags;
> +
> +	if (rcv_msr_event == 0)
> +		return;
> +
> +	/* Update hold reason */
> +	spin_lock_irqsave(&mxport->spinlock, flags);
> +
> +	dev_dbg(&port->dev, "%s - current MSR status = 0x%x\n",
> +		__func__, mxport->msr_state);
> +
> +	if (rcv_msr_hold & UART_MSR_CTS) {
> +		mxport->msr_state |= UART_MSR_CTS;
> +		dev_dbg(&port->dev, "%s - CTS high\n",
> +			__func__);

No need for line breaks in most of the dev_dbgs in this function anymore.

> +	} 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__);
> +	}
> +	spin_unlock_irqrestore(&mxport->spinlock, flags);
> +
> +	/* Update MSR status */
> +	if (rcv_msr_event &
> +	    (SERIAL_EV_CTS | SERIAL_EV_DSR | SERIAL_EV_RLSD)) {
> +
> +		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);
> +	}
> +}
> +
> +static void mxuport_lsr_event(struct usb_serial_port *port,
> +			      u8 buf[4])

No line break needed anymore.

> +{
> +	u8 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,
> +					   u8 buf[4],
> +					   u32 event)

No line break?

> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +
> +	dev_dbg(&port->dev, "%s - receive event : %d\n",

Why not use %04x here?

And no line break needed.

> +		__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;
> +	}
> +}
> +
> +/*
> + * 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;
> +	u8 *data = urb->transfer_buffer;
> +	u8 *end = data + urb->actual_length;
> +	struct usb_serial_port *demux_port;
> +	u8 *ch;
> +	u16 rcv_port;
> +	u16 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;
> +	u8 *data = urb->transfer_buffer;
> +	u8 *end = data + urb->actual_length;
> +	struct usb_serial_port *demux_port;
> +	u8 *ch;
> +	u16 rcv_port;
> +	u16 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;
> +	u32 txlen;
> +	u8 *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 = %d\n", __func__, txlen);

Use %u?

> +
> +	if (txlen != 0)
> +		is_empty = false;
> +
> +out:
> +	kfree(len_buf);
> +	return is_empty;
> +}
> +
> +static int mxuport_set_mcr(struct usb_serial_port *port, u8 mcr_state)
> +{
> +	int err;
> +	struct usb_serial *serial = port->serial;
> +
> +	err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_MCR,
> +				    mcr_state, port->port_number);
> +
> +	if (err)
> +		dev_dbg(&port->dev, "%s - fail to change MCR\n", __func__);
> +
> +	return err;
> +}
> +
> +static int mxuport_dtr(struct usb_serial_port *port, int on)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	int err;
> +
> +	mutex_lock(&mxport->mutex);
> +
> +	if (on)
> +		mxport->mcr_state |= UART_MCR_DTR;
> +	else
> +		mxport->mcr_state &= ~UART_MCR_DTR;
> +
> +	err = mxuport_set_mcr(port, mxport->mcr_state);
> +
> +	mutex_unlock(&mxport->mutex);
> +
> +	return err;
> +}

You should probably use SET_DTR (and rename the function
mxuport_set_dtr).

> +
> +static int mxuport_rts(struct usb_serial_port *port, int on)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	int err;
> +
> +	mutex_lock(&mxport->mutex);
> +
> +	if (on)
> +		mxport->mcr_state |= UART_MCR_RTS;
> +	else
> +		mxport->mcr_state &= ~UART_MCR_RTS;
> +
> +	err = mxuport_set_mcr(port, mxport->mcr_state);
> +
> +	mutex_unlock(&mxport->mutex);
> +
> +	return err;
> +}

I think you need to use SET_RTS and pass in a tri-state argument (e.g.
u8) for off, on, and hw-flow_enable (auto). More details below.

> +
> +static void mxuport_dtr_rts(struct usb_serial_port *port, int on)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +
> +	mutex_lock(&mxport->mutex);
> +
> +	if (on)
> +		mxport->mcr_state |= (UART_MCR_RTS | UART_MCR_DTR);
> +	else
> +		mxport->mcr_state &= ~(UART_MCR_RTS | UART_MCR_DTR);
> +
> +	mxuport_set_mcr(port, mxport->mcr_state);
> +
> +	mutex_unlock(&mxport->mutex);
> +}

Nice and clean. :)

> +
> +static int mxuport_tiocmset(struct tty_struct *tty, unsigned int set,
> +			    unsigned int clear)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	int err = 0;
> +
> +	dev_dbg(&port->dev, "%s\n", __func__);
> +
> +	/* Set MCR status */
> +	if (set & TIOCM_RTS) {
> +		err = mxuport_rts(port, 1);
> +		if (err)
> +			goto out;
> +	}
> +
> +	if (set & TIOCM_DTR) {
> +		err = mxuport_dtr(port, 1);
> +		if (err)
> +			goto out;
> +	}
> +
> +	/* Clear MCR status */
> +	if (clear & TIOCM_RTS) {
> +		err = mxuport_rts(port, 0);
> +		if (err)
> +			goto out;
> +	}
> +
> +	if (clear & TIOCM_DTR)
> +		err = mxuport_dtr(port, 0);
> +
> +out:
> +	return err;
> +}

I think you should do the requested change in one request rather than
use the new helper functions. Grab the mutex once, update a copy of
mxport->mcr_state depending on which bits are set, and then do one call
to SET_MCR (mxuport_set_mcr) and if it succeeds you update the private
mcr_state.  

That is basically what you did before but use one SET_MCR instead of
multiple SET_RTS and SET_DTS.

> +
> +static int mxuport_tiocmget(struct tty_struct *tty)
> +{
> +	struct mxuport_port *mxport;
> +	struct usb_serial_port *port = tty->driver_data;
> +	unsigned int result = 0;

No need to initialise.

> +	unsigned long flags;
> +	unsigned int msr;
> +	unsigned int mcr;
> +
> +	mxport = usb_get_serial_port_data(port);
> +
> +	mutex_lock(&mxport->mutex);
> +	spin_lock_irqsave(&mxport->spinlock, flags);
> +
> +	msr = mxport->msr_state;
> +	mcr = mxport->mcr_state;
> +
> +	spin_unlock_irqrestore(&mxport->spinlock, flags);
> +	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 int mxuport_set_termios_flow(struct tty_struct *tty,
> +				    struct usb_serial_port *port,
> +				    struct usb_serial *serial,
> +				    char *buf)

Pass the buffer as u8[4] (or u8[2]). Or perhaps simply doing a new
buffer allocation for the RQ_VENDOR_SET_CHARS if sw-flow is enabled is
better now that flow-control handling lives in its own function?

> +{
> +	u32 flow_ctrl = 0;
> +	char xon = START_CHAR(tty);
> +	char xoff = STOP_CHAR(tty);

Use unsigned char here and drop the casts below.

> +	int err;
> +	int enable;
> +
> +	/* S/W flow control settings */
> +	if (I_IXOFF(tty) || I_IXON(tty)) {
> +		buf[0] = (u8)xon;
> +		buf[1] = (u8)xoff;

Drop casts here.

> +
> +		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__);
> +	}
> +
> +	if (flow_ctrl & (MX_XON_FLOWCTRL | MX_XOFF_FLOWCTRL))
> +		enable = 1;
> +	else
> +		enable = 0;
> +
> +	err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_XONXOFF,
> +				    enable, port->port_number);
> +	if (err)
> +		goto out;

Ok, it seems the above could simplified quite a bit. The chip seems to
only have two settings for sw-flow (on or off) so there's no need to
manipulate the flow_control mask (which could be dropped). So you'd end
up with something like

	if (I_IXOFF(tty) || I_IXON(tty)) {
		...
		mxuport_send_ctrl_data_urb(serial, RQ_VENDOR_SET_CHARS,
		...
		dev_dbg(&port->dev, "XON = %02x, XOFF = %02x\n",
		...
		sw_flow_enable = 1;
	} else {
		sw_flow_enable = 0;
	}

	err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_XONXOFF,
				    sw_flow_enable, port->port_number);

> +
> +	/* H/W flow control settings */
> +	if (C_BAUD(tty) && C_CRTSCTS(tty))
> +		err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RTS,
> +					    MX_HW_FLOWCTRL_ENABLE,
> +					    port->port_number);
> +	else
> +		if (C_BAUD(tty))
> +			err = mxuport_rts(port, 1);
> +		else
> +			err = mxuport_rts(port, 0);

Hmm. This isn't quite right yet. If you move the B0 handling from
set_termios to this function as I suggested, it would be easier to see
their dependencies.

You want something along the following lines:

	#define MX_RTS_OFF		0
	#define MX_RTS_ON		1
	#define MX_RTS_HW		2
	#define MX_RTS_NO_CHANGE	3

	unsigned rts = MX_RTS_NO_CHANGE;

	/* H/W flow control settings */
	if (!old_termios || C_CRTCTS(tty) != old_termios->c_cflag & C_CRTSCTS) {
		if (C_CRTSCTS(tty))
			rts = MX_RTS_HW;
		else
			rts = MX_RTS_ON;
	}

	if (C_BAUD(tty)) {
		if (old_termios && (old_termios->c_cflag & CBAUD) == B0) {
			/* Raise DTR and RTS */
			if (C_CRTSCTS(tty))
				rts = MX_RTS_HW;
			else
				rts = MX_RTS_ON;
			mxuport_set_dtr(port, 1);
		}
	} else {
		/* Drop DTR and RTS */
		rts = MX_RTS_OFF;
		mxuport_set_dtr(port, 0);
	}

	if (rts != MX_RTS_NO_CHANGE)
		err = mxport_set_rts(port, rts);

That ought to pretty much cover it, right? 

Note that mxport_set_rts should only update last_mcr if rts !=
MX_RTS_HW.

> +
> +out:
> +	return err;
> +}
> +
> +static void mxuport_set_termios(struct tty_struct *tty,
> +				struct usb_serial_port *port,
> +				struct ktermios *old_termios)
> +{
> +	struct ktermios *termios = &tty->termios;
> +	struct usb_serial *serial = port->serial;
> +	unsigned int iflag;
> +	u8 *buf;
> +	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;
> +		break;
> +	case CS6:
> +		data_bits = MX_WORDLENGTH_6;
> +		break;
> +	case CS7:
> +		data_bits = MX_WORDLENGTH_7;
> +		break;
> +	case CS8:
> +	default:
> +		data_bits = MX_WORDLENGTH_8;
> +		break;
> +	}
> +
> +	/* Set parity of termios */
> +	if (C_PARENB(tty)) {
> +		if (C_CMSPAR(tty)) {
> +			if (C_PARODD(tty))
> +				parity = MX_PARITY_MARK;
> +			else
> +				parity = MX_PARITY_SPACE;
> +		} else if (C_PARODD(tty)) {
> +			parity = MX_PARITY_ODD;
> +		} else {
> +			parity = MX_PARITY_EVEN;
> +		}
> +	} else {
> +		parity = MX_PARITY_NONE;
> +	}
> +
> +	/* Set stop bit of termios */
> +	if (C_CSTOPB(tty))
> +		stop_bits = MX_STOP_BITS_2;
> +	else
> +		stop_bits = MX_STOP_BITS_1;
> +
> +	/* 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) {
> +			if (!C_CRTSCTS(tty))
> +				mxuport_rts(port, 1);
> +			mxuport_dtr(port, 1);
> +		} else {
> +			/* Drop DTR/RTS */
> +			mxuport_dtr_rts(port, 0);
> +		}
> +	}

So move this bit to set_termios_flow as discussed above.

> +
> +	err = mxuport_set_termios_flow(tty, port, serial, buf);
> +	if (err)
> +		goto out;
> +
> +	baud = tty_get_baud_rate(tty);
> +	if (!baud)
> +		baud = 9600;
> +
> +	/* 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)
> +{
> +	u32 quirks = (u32) usb_get_serial_data(serial);

You have to cast via (unsigned long) when using u32 (for 64-bit
machines), so better to just stick to unsigned long.

Minor knit: perhaps we should reserve the word "quirks" for actual quirks
(odd driver behaviour) and use "features" here instead. Then this would
be:

	unsigned long features = (unsigned long)usb_get_serial_data(serial);

	
> +
> +	if (quirks & MX_UPORT_2_PORT)
> +		return 2;
> +

Drop the empty line.

> +	if (quirks & MX_UPORT_4_PORT)
> +		return 4;
> +	if (quirks & MX_UPORT_8_PORT)
> +		return 8;
> +	if (quirks & MX_UPORT_16_PORT)
> +		return 16;

And add one here instead.

> +	return 0;
> +}
> +
> +/* Get the version of the firmware currently running. */
> +static int mxuport_get_fw_version(struct usb_serial *serial, u32 *version)
> +{
> +	u8 *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 != 4)
> +		goto out;
> +
> +	*version = (ver_buf[0] << 16) | (ver_buf[1] << 8) | ver_buf[2];
> +	err = 0;
> +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)
> +{
> +	u8 *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;
> +	u32 version;
> +	int local_ver;
> +	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;
> +	}
> +
> +	err = mxuport_get_fw_version(serial, &version);
> +	if (err < 0)
> +		goto out;
> +
> +	dev_dbg(&serial->interface->dev, "Device firmware version v%x.%x.%x\n",
> +		(version & 0xff0000) >> 16,
> +		(version & 0xff00) >> 8,
> +		(version & 0xff));
> +
> +	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 > version) {
> +			err = mxuport_download_fw(serial, fw_p);
> +			if (err)
> +				goto out;
> +			err  = mxuport_get_fw_version(serial, &version);
> +			if (err < 0)
> +				goto out;
> +		}
> +	}
> +
> +	dev_info(&serial->interface->dev,
> +		 "Using device firmware version v%x.%x.%x\n",
> +		 (version & 0xff0000) >> 16,
> +		 (version & 0xff00) >> 8,
> +		 (version & 0xff));
> +

Perhaps you could just add a comment here about storing driver info for
later use.

> +	usb_set_serial_data(serial, (void *)id->driver_info);
> +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);
> +	spin_lock_init(&mxport->spinlock);
> +
> +	/* Set the port private data */
> +	usb_set_serial_port_data(port, mxport);
> +
> +	/* Send vendor request - set FIFO (Enable) */

The "Send vendor request" part of these comments doesn't add much
information. Just remove that bit here (and elsewhere).

> +	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;
> +
> +	/* Send vendor request - set receive host (enable) */
> +	err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN,
> +				    1, port->port_number);
> +	if (err)
> +		return err;
> +
> +	err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_OPEN,
> +				    1, port->port_number);
> +

No empty line.

> +	if (err) {
> +		mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN,
> +				      0, port->port_number);
> +		return err;
> +	}
> +
> +	/* Initial port termios */
> +	mxuport_set_termios(tty, port, NULL);
> +
> +	/*
> +	 * TODO: use RQ_VENDOR_GET_MSR, once we know what it
> +	 * returns.
> +	 */
> +	mxport->msr_state = 0;
> +
> +	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 enable;
> +
> +	mxport = usb_get_serial_port_data(port);
> +
> +	if (break_state == -1) {
> +		enable = 1;
> +		dev_dbg(&port->dev, "%s - sending break\n", __func__);
> +	} else {
> +		enable = 0;
> +		dev_dbg(&port->dev, "%s - clearing break\n", __func__);
> +	}
> +
> +	mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_BREAK,
> +			      enable, port->port_number);
> +}
> +
> +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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux