Re: [PATCH v2] USB: serial: add Moxa UPORT 11x0 driver

[Johan Hovold <johan@kernel.org>]

On Tue, Sep 22, 2015 at 03:08:29PM +0200, Mathieu OTHACEHE wrote:
> Add a driver which supports :
> 
> - UPort 1110  : 1 port RS-232 USB to Serial Hub.
> - UPort 1130  : 1 port RS-422/485 USB to Serial Hub.
> - UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
> - UPort 1150  : 1 port RS-232/422/485 USB to Serial Hub.
> - UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
> 
> This driver is based on GPL MOXA driver written by Hen Huang and available
> on MOXA website. The original driver was based on io_ti serial driver.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>

<snip>

> +/* Operation modes */
> +#define MXU1_UART_232				0x00
> +#define MXU1_UART_485_RECEIVER_DISABLED		0x01
> +#define MXU1_UART_485_RECEIVER_ENABLED		0x02
> +#define MXU1_TYPE_RS232				(1 << 0)
> +#define MXU1_TYPE_RS422				(1 << 1)
> +#define MXU1_TYPE_RS485				(1 << 2)

You can use BIT(n) for these.

> +/* Pipe transfer mode and timeout */
> +#define MXU1_PIPE_MODE_CONTINUOUS		0x01
> +#define MXU1_PIPE_MODE_MASK			0x03
> +#define MXU1_PIPE_TIMEOUT_MASK			0x7C
> +#define MXU1_PIPE_TIMEOUT_ENABLE		0x80
> +
> +/* Config struct */
> +struct mxu1_uart_config {
> +	__be16	wBaudRate;
> +	__be16	wFlags;
> +	u8	bDataBits;
> +	u8	bParity;
> +	u8	bStopBits;
> +	char	cXon;
> +	char	cXoff;
> +	u8	bUartMode;
> +} __packed;
> +
> +/* Purge modes */
> +#define MXU1_PURGE_OUTPUT			0x00
> +#define MXU1_PURGE_INPUT			0x80
> +
> +/* Read/Write data */
> +#define MXU1_RW_DATA_ADDR_SFR			0x10
> +#define MXU1_RW_DATA_ADDR_IDATA			0x20
> +#define MXU1_RW_DATA_ADDR_XDATA			0x30
> +#define MXU1_RW_DATA_ADDR_CODE			0x40
> +#define MXU1_RW_DATA_ADDR_GPIO			0x50
> +#define MXU1_RW_DATA_ADDR_I2C			0x60
> +#define MXU1_RW_DATA_ADDR_FLASH			0x70
> +#define MXU1_RW_DATA_ADDR_DSP			0x80
> +
> +#define MXU1_RW_DATA_UNSPECIFIED		0x00
> +#define MXU1_RW_DATA_BYTE			0x01
> +#define MXU1_RW_DATA_WORD			0x02
> +#define MXU1_RW_DATA_DOUBLE_WORD		0x04
> +
> +struct mxu1_write_data_bytes {
> +	u8	bAddrType;
> +	u8	bDataType;
> +	u8	bDataCounter;
> +	__be16	wBaseAddrHi;
> +	__be16	wBaseAddrLo;
> +	u8	bData[0];
> +} __packed;
> +
> +/* Interrupt codes */
> +#define MXU1_GET_PORT_FROM_CODE(c)		(((c) >> 4) - 3)
> +#define MXU1_GET_FUNC_FROM_CODE(c)		((c) & 0x0f)
> +#define MXU1_CODE_HARDWARE_ERROR		0xFF
> +#define MXU1_CODE_DATA_ERROR			0x03
> +#define MXU1_CODE_MODEM_STATUS			0x04
> +
> +/* Download firmware max packet size */
> +#define MXU1_DOWNLOAD_MAX_PACKET_SIZE		64
> +
> +/* Firmware image header */
> +struct mxu1_firmware_header {
> +	__le16 wLength;
> +	u8 bCheckSum;
> +} __packed;
> +
> +/* UART addresses */
> +/* UART 1 base address */
> +#define MXU1_UART1_BASE_ADDR			0xFFA0
> +/* UART 2 base address*/
> +#define MXU1_UART2_BASE_ADDR			0xFFB0
> +#define MXU1_UART_OFFSET_LCR			0x0002

Why are these unused defines here? This driver is for one-port devices, right?

> +/*UART MCR register offset */
> +#define MXU1_UART_OFFSET_MCR			0x0004
> +
> +#define MXU1_SET_SERIAL_FLAGS	    0
> +
> +#define MXU1_TRANSFER_TIMEOUT	    2
> +#define MXU1_MSR_WAIT_TIMEOUT	    (5 * HZ)

This one does not seem to be used either.

> +
> +/* Configuration ids */
> +#define MXU1_BOOT_CONFIG	    1
> +#define MXU1_ACTIVE_CONFIG	    2

Nor are these. Please drop unused defines unless useful for future feature
additions (e.g. register definitions).

> +
> +#define MXU1_DEFAULT_CLOSING_WAIT   4000		/* in .01 secs */
> +
> +struct mxu1_port {
> +	u8 mxp_msr;
> +	u8 mxp_lsr;
> +	u8 mxp_shadow_mcr;
> +	u8 mxp_uart_types;
> +	u8 mxp_uart_mode;
> +	unsigned int mxp_uart_base_addr;

u32 or u16?

> +	int mxp_flags;
> +	int mxp_msr_wait_flags;
> +	wait_queue_head_t mxp_msr_wait;	/* wait for msr change */
> +	struct mxu1_device *mxp_mxdev;
> +	struct usb_serial_port *mxp_port;
> +	spinlock_t mxp_lock;
> +	int mxp_send_break;
> +	int mxp_set_B0;
> +};
> +
> +struct mxu1_device {
> +	struct mutex mxd_lock;
> +	struct usb_serial *mxd_serial;
> +	u16 mxd_model;
> +};
> +
> +static const struct usb_device_id mxuport11_idtable[] = {
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1110_PRODUCT_ID) },
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1130_PRODUCT_ID) },
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1150_PRODUCT_ID) },
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1151_PRODUCT_ID) },
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1131_PRODUCT_ID) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, mxuport11_idtable);
> +
> +/* Write the given buffer out to the control pipe.  */
> +static int mxu1_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,
> +				 1000);

Please use a define for the timeout.

> +	if (status < 0) {
> +		dev_err(&serial->interface->dev,
> +			"%s - usb_control_msg failed (%d)\n",
> +			__func__, status);
> +		return status;
> +	}
> +
> +	if (status != size) {
> +		dev_err(&serial->interface->dev,
> +			"%s - short write (%d / %zd)\n",
> +			__func__, status, size);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Send a vendor request without any data */
> +static int mxu1_send_ctrl_urb(struct usb_serial *serial,
> +			      u8 request, u16 value, u16 index)
> +{
> +	return mxu1_send_ctrl_data_urb(serial, request, value, index,
> +				       NULL, 0);
> +}
> +
> +static int mxu1_download_firmware(struct usb_serial *serial,
> +				  const struct firmware *fw_p)
> +{
> +	int status = 0;
> +	int buffer_size;
> +	int pos;
> +	int len;
> +	int done;
> +	u8 cs = 0;
> +	u8 *buffer;
> +	struct usb_device *dev = serial->dev;
> +	struct mxu1_firmware_header *header;
> +	unsigned int pipe = usb_sndbulkpipe(dev, serial->port[0]->
> +					    bulk_out_endpointAddress);

Separate declaration and initialisation here.

> +
> +	buffer_size = fw_p->size +
> +		sizeof(struct mxu1_firmware_header);

Indent continuation lines at least two tabs further, but this line does
appear to need to be broken at all.

> +	buffer = kmalloc(buffer_size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	memcpy(buffer, fw_p->data, fw_p->size);
> +	memset(buffer + fw_p->size, 0xff, buffer_size - fw_p->size);
> +
> +	for (pos = sizeof(struct mxu1_firmware_header);

sizeof(*header) to avoid having to break the line?

> +	     pos < buffer_size; pos++)
> +		cs = (u8)(cs + buffer[pos]);

Add brackets unless you can avoid the line break above.

> +
> +	header = (struct mxu1_firmware_header *)buffer;
> +	header->wLength = cpu_to_le16(
> +		(__u16)(buffer_size - sizeof(struct mxu1_firmware_header)));

No need for cast.

Use sizeof(*header).

> +	header->bCheckSum = cs;
> +
> +	dev_dbg(&dev->dev, "%s - downloading firmware", __func__);
> +
> +	for (pos = 0; pos < buffer_size; pos += done) {
> +		len = min(buffer_size - pos, MXU1_DOWNLOAD_MAX_PACKET_SIZE);
> +
> +		status = usb_bulk_msg(dev, pipe, buffer+pos, len, &done, 1000);

Spaces around + operator.

Use a define for the timeout.

> +

Remove blank line.

> +		if (status)
> +			break;
> +	}
> +
> +	kfree(buffer);
> +
> +	if (status) {
> +		dev_err(&dev->dev, "%s - error downloading firmware, %d\n",
> +			__func__, status);

No need for __func__ when you have a self-contained error message
(which is preferred). Debug messages can be more brief and rely on
__func__.

If possible try to use a consistent style for reporting errnos (e.g.
"failed to download firmware: %d").

> +		return status;
> +	}
> +
> +	msleep_interruptible(100);
> +
> +	status = mxu1_send_ctrl_urb(serial, MXU1_RESET_EXT_DEVICE, 0, 0);
> +
> +	dev_dbg(&dev->dev, "%s - download successful (%d)", __func__, status);
> +
> +	return 0;
> +}
> +
> +static int mxu1_port_probe(struct usb_serial_port *port)
> +{
> +	struct mxu1_port *mxport;
> +
> +	mxport = kzalloc(sizeof(struct mxu1_port), GFP_KERNEL);
> +

Don't add newline before checking return values.

> +	if (!mxport)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&mxport->mxp_lock);
> +	mxport->mxp_port = port;
> +	mxport->mxp_mxdev = usb_get_serial_data(port->serial);

No need to store the serial data pointer in the port data, you can
always access it through port->serial.

> +	mxport->mxp_uart_base_addr = MXU1_UART1_BASE_ADDR;

I already asked above, but when is any other base address ever used?

> +
> +	init_waitqueue_head(&mxport->mxp_msr_wait);

Use the port msr wait queue instead.

> +
> +	switch (mxport->mxp_mxdev->mxd_model) {
> +	case MXU1_1110_PRODUCT_ID:
> +		mxport->mxp_uart_types = MXU1_TYPE_RS232;
> +		mxport->mxp_uart_mode = MXU1_UART_232;
> +		break;
> +	case MXU1_1130_PRODUCT_ID:
> +	case MXU1_1131_PRODUCT_ID:
> +		mxport->mxp_uart_types = MXU1_TYPE_RS422 | MXU1_TYPE_RS485;
> +		mxport->mxp_uart_mode = MXU1_UART_485_RECEIVER_DISABLED;
> +		break;
> +	case MXU1_1150_PRODUCT_ID:
> +	case MXU1_1151_PRODUCT_ID:
> +		mxport->mxp_uart_types =
> +			MXU1_TYPE_RS232 | MXU1_TYPE_RS422 | MXU1_TYPE_RS485;
> +		mxport->mxp_uart_mode = MXU1_UART_232;
> +		break;
> +	}
> +
> +	usb_set_serial_port_data(port, mxport);
> +
> +	port->port.closing_wait =
> +		msecs_to_jiffies(MXU1_DEFAULT_CLOSING_WAIT * 10);

Indent further.

> +	port->port.drain_delay = 1;
> +
> +	return 0;
> +}
> +
> +static int mxu1_startup(struct usb_serial *serial)
> +{
> +	struct mxu1_device *mxdev;
> +	struct usb_device *dev = serial->dev;
> +	char fw_name[32];
> +	const struct firmware *fw_p = NULL;
> +	int err;
> +
> +	dev_dbg(&dev->dev, "%s - product 0x%4X, num configurations %d, configuration value %d",
> +		__func__, le16_to_cpu(dev->descriptor.idProduct),
> +		dev->descriptor.bNumConfigurations,
> +		dev->actconfig->desc.bConfigurationValue);
> +
> +	/* create device structure */
> +	mxdev = kzalloc(sizeof(struct mxu1_device), GFP_KERNEL);
> +	if (mxdev == NULL)
> +		return -ENOMEM;
> +
> +	mutex_init(&mxdev->mxd_lock);
> +	mxdev->mxd_serial = serial;
> +	usb_set_serial_data(serial, mxdev);
> +
> +	mxdev->mxd_model = le16_to_cpu(dev->descriptor.idProduct);
> +
> +	/* if we have only 1 configuration, download firmware */
> +	if (dev->config->interface[0]->cur_altsetting->
> +	    desc.bNumEndpoints == 1) {

Use an intermediate variable for the interface descriptors, which are
accessible through serial->interface->cur_altsetting, to avoid line
break.

> +
> +		snprintf(fw_name,
> +			 sizeof(fw_name) - 1,

No need for -1 as size includes trailing NUL.

> +			 "moxa/moxa-%04x.fw",
> +			 mxdev->mxd_model);
> +
> +		err = request_firmware(&fw_p, fw_name, &serial->interface->dev);
> +		if (err) {
> +			dev_err(&serial->interface->dev, "Firmware %s not found\n",
> +				fw_name);
> +			kfree(mxdev);
> +			return err;
> +		}
> +
> +		err = mxu1_download_firmware(serial, fw_p);
> +		if (err) {
> +			kfree(mxdev);
> +			return err;
> +		}
> +
> +	}
> +
> +	if (fw_p)
> +		release_firmware(fw_p);

Move this into the conditional block above and remove the NULL-test.

> +
> +	return 0;
> +}
> +
> +static int mxu1_write_byte(struct mxu1_device *mxdev, unsigned long addr,
> +			   u8 mask, u8 byte)

Pass the usb_serial_port as first parameter for port operations (and use
it for any error or debug messages).

u32 for address?

> +{
> +	int status = 0;
> +	unsigned int size;

size_t

> +	struct mxu1_write_data_bytes *data;
> +	struct device *dev = &mxdev->mxd_serial->dev->dev;
> +
> +	dev_dbg(dev, "%s - addr 0x%08lX, mask 0x%02X, byte 0x%02X", __func__,
> +		addr, mask, byte);
> +
> +	size = sizeof(struct mxu1_write_data_bytes) + 2;
> +	data = kmalloc(size, GFP_KERNEL);

kzalloc to clear the pad (?) bytes?

> +	if (!data) {
> +		dev_err(dev, "%s - out of memory\n", __func__);

OOM errors would already have been logged so just return -ENOMEM here.

> +		return -ENOMEM;
> +	}
> +
> +	data->bAddrType = MXU1_RW_DATA_ADDR_XDATA;
> +	data->bDataType = MXU1_RW_DATA_BYTE;
> +	data->bDataCounter = 1;
> +	data->wBaseAddrHi = cpu_to_be16(addr>>16);

Spaces around binary operator missing.

> +	data->wBaseAddrLo = cpu_to_be16(addr);
> +	data->bData[0] = mask;
> +	data->bData[1] = byte;
> +
> +	status = mxu1_send_ctrl_data_urb(mxdev->mxd_serial, MXU1_WRITE_DATA, 0,
> +					 MXU1_RAM_PORT,
> +					 (u8 *)data,
> +					 size);
> +

Stray new line again.

> +	if (status < 0)
> +		dev_err(dev, "%s - failed, %d\n", __func__, status);
> +
> +	kfree(data);
> +
> +	return status;
> +}
> +
> +static int mxu1_set_mcr(struct mxu1_port *mxport, unsigned int mcr)

So pass usb-serial struct here and elsewhere.

> +{
> +	int status = 0;

No need to initialise.

> +	unsigned long flags;
> +
> +	status = mxu1_write_byte(mxport->mxp_mxdev,
> +				 mxport->mxp_uart_base_addr +
> +				 MXU1_UART_OFFSET_MCR,
> +				 MXU1_MCR_RTS | MXU1_MCR_DTR | MXU1_MCR_LOOP,
> +				 mcr);
> +
> +	spin_lock_irqsave(&mxport->mxp_lock, flags);
> +	if (!status)
> +		mxport->mxp_shadow_mcr = mcr;
> +	spin_unlock_irqrestore(&mxport->mxp_lock, flags);

Use a mutex to protect shadow_mcr so that you can to this atomically.

> +
> +	return status;
> +}
> +
> +static void mxu1_set_termios(struct tty_struct *tty1,
> +			     struct usb_serial_port *port,
> +			     struct ktermios *old_termios)
> +{
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +
> +	struct tty_struct *tty = port->port.tty;

You cannot access the tty like this. Use the passed first argument
instead.

> +

Remove stray newline above.

> +	struct mxu1_uart_config *config;
> +	tcflag_t cflag, iflag;
> +	int baud;

speed_t

> +	int status = 0;
> +	int port_number = port->port_number - port->minor;

Again, why do you need this as these are all one-port devices?

> +	unsigned int mcr;
> +
> +	dev_dbg(&port->dev, "%s - port %d", __func__, port->port_number);
> +
> +	if (!tty) {
> +		dev_dbg(&port->dev, "%s - no tty or termios", __func__);

A lot of debug messages lack a terminating \n. Fix throughout.

> +		return;
> +	}
> +
> +	cflag = tty->termios.c_cflag;
> +	iflag = tty->termios.c_iflag;
> +
> +	if (old_termios && cflag == old_termios->c_cflag
> +	    && iflag == old_termios->c_iflag) {

Use tty_termios_hw_change() if appropriate.

Place && before line break.

> +		dev_dbg(&port->dev, "%s - nothing to change", __func__);
> +		return;
> +	}
> +
> +	dev_dbg(&port->dev,
> +		"%s - clfag %08x, iflag %08x", __func__, cflag, iflag);
> +
> +	if (old_termios)
> +		dev_dbg(&port->dev, "%s - old clfag %08x, old iflag %08x",
> +			__func__,
> +			old_termios->c_cflag,
> +			old_termios->c_iflag);

Add brackets.

> +
> +	if (mxport == NULL)
> +		return;

Not needed.

> +
> +	config = kmalloc(sizeof(*config), GFP_KERNEL);

Use kzalloc.

> +	if (!config)
> +		return;
> +
> +	config->wFlags = 0;
> +
> +	/* these flags must be set */
> +	config->wFlags |= MXU1_UART_ENABLE_MS_INTS;
> +	config->wFlags |= MXU1_UART_ENABLE_AUTO_START_DMA;
> +	if (mxport->mxp_send_break == MXU1_LCR_BREAK)
> +		config->wFlags |= MXU1_UART_SEND_BREAK_SIGNAL;
> +	config->bUartMode = (u8)(mxport->mxp_uart_mode);
> +
> +	switch (cflag & CSIZE) {
> +	case CS5:
> +		config->bDataBits = MXU1_UART_5_DATA_BITS;
> +		break;
> +	case CS6:
> +		config->bDataBits = MXU1_UART_6_DATA_BITS;
> +		break;
> +	case CS7:
> +		config->bDataBits = MXU1_UART_7_DATA_BITS;
> +		break;
> +	default:
> +	case CS8:
> +		config->bDataBits = MXU1_UART_8_DATA_BITS;
> +		break;
> +	}
> +
> +	if (cflag & PARENB) {
> +		if (cflag & PARODD) {
> +			config->wFlags |= MXU1_UART_ENABLE_PARITY_CHECKING;

Move out of inner conditional.

> +			config->bParity = MXU1_UART_ODD_PARITY;
> +		} else {
> +			config->wFlags |= MXU1_UART_ENABLE_PARITY_CHECKING;
> +			config->bParity = MXU1_UART_EVEN_PARITY;
> +		}
> +	} else {
> +		config->wFlags &= ~MXU1_UART_ENABLE_PARITY_CHECKING;

You never set it.

> +		config->bParity = MXU1_UART_NO_PARITY;
> +	}

You should also clear CMSPAR in the termios structure if you do not
support it.

> +
> +	if (cflag & CSTOPB)
> +		config->bStopBits = MXU1_UART_2_STOP_BITS;
> +	else
> +		config->bStopBits = MXU1_UART_1_STOP_BITS;
> +
> +	if (cflag & CRTSCTS) {
> +		/* RTS flow control must be off to drop RTS for baud rate B0 */
> +		if ((cflag & CBAUD) != B0)
> +			config->wFlags |= MXU1_UART_ENABLE_RTS_IN;
> +		config->wFlags |= MXU1_UART_ENABLE_CTS_OUT;
> +	} else {
> +		tty->hw_stopped = 0;

No need to update this.

> +	}
> +
> +	if (I_IXOFF(tty) || I_IXON(tty)) {
> +		config->cXon  = START_CHAR(tty);
> +		config->cXoff = STOP_CHAR(tty);
> +
> +		if (I_IXOFF(tty))
> +			config->wFlags |= MXU1_UART_ENABLE_X_IN;
> +
> +		if (I_IXON(tty))
> +			config->wFlags |= MXU1_UART_ENABLE_X_OUT;
> +	}
> +
> +	baud = tty_get_baud_rate(tty);
> +	if (!baud)
> +		baud = 9600;
> +	config->wBaudRate = (__u16)(923077 / baud);

No need for cast. Missing cpu_to_be16 (store in temporary baud and flags
variables if needed).

Use a define for the baud base.

> +
> +	dev_dbg(&port->dev, "%s - BaudRate=%d, wBaudRate=%d, wFlags=0x%04X, bDataBits=%d, bParity=%d, bStopBits=%d, cXon=%d, cXoff=%d, bUartMode=%d",
> +		__func__, baud, config->wBaudRate, config->wFlags,
> +		config->bDataBits, config->bParity, config->bStopBits,
> +		config->cXon, config->cXoff, config->bUartMode);
> +
> +	cpu_to_be16s(&config->wBaudRate);
> +	cpu_to_be16s(&config->wFlags);
> +
> +	status = mxu1_send_ctrl_data_urb(port->serial, MXU1_SET_CONFIG, 0,
> +					 (u8)(MXU1_UART1_PORT + port_number),

port number again?

> +					 (u8 *)config,
> +					 sizeof(*config));
> +	if (status)
> +		dev_err(&port->dev, "%s - cannot set config on port %d, %d\n",
> +			__func__,
> +			port_number,
> +			status);

Use brackets around multi-line expressions for readability.

> +
> +	/* SET_CONFIG asserts RTS and DTR, reset them correctly */
> +	mcr = mxport->mxp_shadow_mcr;

Locking?

> +	/* if baud rate is B0, clear RTS and DTR */
> +	if ((cflag & CBAUD) == B0) {
> +
> +		mcr &= ~(MXU1_MCR_DTR | MXU1_MCR_RTS);
> +		mxport->mxp_set_B0 = true;

Why do you need this?

You should also disable automatic flow control if enabled.


> +	} else {
> +		if (mxport->mxp_set_B0)
> +			mcr |= MXU1_MCR_DTR;
> +
> +		mxport->mxp_set_B0 = false;

Raise DTR/RTS, if changing from B0 (check old termios).

> +	}
> +
> +	status = mxu1_set_mcr(mxport, mcr);
> +

Stray newline.

> +	if (status)
> +		dev_err(&port->dev,
> +			"%s - cannot set modem control on port %d, %d\n",
> +			__func__,
> +			port_number,
> +			status);

Brackets.

> +
> +	kfree(config);
> +}
> +
> +static int mxu1_ioctl_get_rs485(struct mxu1_port *mxport,
> +				struct serial_rs485 __user *rs485) {

Bracket on new line (below as well).

> +	struct serial_rs485 aux;
> +
> +	memset(&aux, 0, sizeof(aux));
> +
> +	if (mxport->mxp_uart_mode == MXU1_UART_485_RECEIVER_ENABLED)
> +		aux.flags = SER_RS485_ENABLED;
> +
> +	if (copy_to_user(rs485, &aux, sizeof(aux)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int mxu1_ioctl_set_rs485(struct mxu1_port *mxport,
> +				struct serial_rs485 __user *rs485_user) {
> +	struct serial_rs485 rs485;
> +	struct usb_serial_port *port = mxport->mxp_port;
> +
> +	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
> +		return -EFAULT;
> +
> +	if (mxport->mxp_uart_types &
> +	    (MXU1_TYPE_RS422 | MXU1_TYPE_RS485)) {

Don't break line.

> +
> +		if (rs485.flags & SER_RS485_ENABLED) {
> +			mxport->mxp_uart_mode = MXU1_UART_485_RECEIVER_ENABLED;
> +		} else {
> +		    if (mxport->mxp_uart_types & MXU1_TYPE_RS232)
> +			mxport->mxp_uart_mode = MXU1_UART_232;
> +		    else
> +			mxport->mxp_uart_mode = MXU1_UART_485_RECEIVER_DISABLED;

Indent using tabs, not spaces.

> +		}
> +	} else {
> +		dev_err(&port->dev, "%s not handled by MOXA UPort %04x\n",
> +			__func__, mxport->mxp_mxdev->mxd_model);
> +		return  -EINVAL;
> +	}
> +
> +	mxu1_set_termios(NULL, mxport->mxp_port, NULL);

Why do you need to use set_termios for this update? Why not a dedicated
helper?

> +
> +	return 0;
> +}
> +
> +static int mxu1_get_serial_info(struct mxu1_port *mxport,
> +				struct serial_struct __user *ret_arg)
> +{
> +	struct usb_serial_port *port = mxport->mxp_port;
> +	struct serial_struct ret_serial;
> +	unsigned cwait;
> +
> +	if (!ret_arg)
> +		return -EFAULT;
> +
> +	cwait = port->port.closing_wait;
> +	if (cwait != ASYNC_CLOSING_WAIT_NONE)
> +		cwait = jiffies_to_msecs(cwait) / 10;
> +
> +	memset(&ret_serial, 0, sizeof(ret_serial));
> +
> +	ret_serial.type = PORT_16550A;
> +	ret_serial.line = port->minor;
> +	ret_serial.port = port->port_number;
> +	ret_serial.flags = mxport->mxp_flags;
> +	ret_serial.xmit_fifo_size = port->bulk_out_size;
> +	ret_serial.baud_base = tty_get_baud_rate(mxport->mxp_port->port.tty);

This is the baud base, not the current the baudrate.

> +	ret_serial.close_delay = 5*HZ;
> +	ret_serial.closing_wait = cwait;
> +
> +	if (copy_to_user(ret_arg, &ret_serial, sizeof(*ret_arg)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +
> +static int mxu1_set_serial_info(struct mxu1_port *mxport,
> +				struct serial_struct __user *new_arg)
> +{
> +	struct serial_struct new_serial;
> +	unsigned cwait;
> +
> +	if (copy_from_user(&new_serial, new_arg, sizeof(new_serial)))
> +		return -EFAULT;
> +
> +	cwait = new_serial.closing_wait;
> +	if (cwait != ASYNC_CLOSING_WAIT_NONE)
> +		cwait = msecs_to_jiffies(10 * new_serial.closing_wait);
> +
> +	mxport->mxp_flags = new_serial.flags & MXU1_SET_SERIAL_FLAGS;

You're not using the flags so drop them (the define above is also 0).

> +	mxport->mxp_port->port.closing_wait = cwait;
> +
> +	return 0;
> +}
> +
> +static int mxu1_ioctl(struct tty_struct *tty,
> +		      unsigned int cmd, unsigned long arg)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +

Stray new line (just fix throughout).

> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +
> +	if (mxport == NULL)
> +		return -ENODEV;

Not needed.

> +
> +	switch (cmd) {
> +	case TIOCGSERIAL:
> +		return mxu1_get_serial_info(mxport,
> +					    (struct serial_struct __user *)arg);
> +
> +	case TIOCSSERIAL:
> +		return mxu1_set_serial_info(mxport,
> +					    (struct serial_struct __user *)arg);
> +
> +	case TIOCGRS485:
> +		return mxu1_ioctl_get_rs485(mxport,
> +					    (struct serial_rs485 __user *)
> +					    arg);

Don't break line before arg.

> +	case TIOCSRS485:
> +		return mxu1_ioctl_set_rs485(mxport,
> +					    (struct serial_rs485 __user *)
> +					    arg);

Ditto.

> +	}
> +
> +	return -ENOIOCTLCMD;
> +}
> +
> +static int mxu1_tiocmget(struct tty_struct *tty)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	unsigned int result;
> +	unsigned int msr;
> +	unsigned int mcr;
> +	unsigned long flags;
> +
> +	dev_dbg(&port->dev, "%s - port %d", __func__, port->port_number);
> +
> +	if (mxport == NULL)
> +		return -ENODEV;

Not needed. Fix throughout.

> +
> +	spin_lock_irqsave(&mxport->mxp_lock, flags);
> +	msr = mxport->mxp_msr;
> +	mcr = mxport->mxp_shadow_mcr;
> +	spin_unlock_irqrestore(&mxport->mxp_lock, flags);
> +
> +	result = ((mcr & MXU1_MCR_DTR) ? TIOCM_DTR : 0)
> +		| ((mcr & MXU1_MCR_RTS) ? TIOCM_RTS : 0)
> +		| ((mcr & MXU1_MCR_LOOP) ? TIOCM_LOOP : 0)
> +		| ((msr & MXU1_MSR_CTS) ? TIOCM_CTS : 0)
> +		| ((msr & MXU1_MSR_CD) ? TIOCM_CAR : 0)
> +		| ((msr & MXU1_MSR_RI) ? TIOCM_RI : 0)
> +		| ((msr & MXU1_MSR_DSR) ? TIOCM_DSR : 0);

But boolean operators before line break and indent further.
> +
> +	dev_dbg(&port->dev, "%s - 0x%04X", __func__, result);
> +
> +	return result;
> +}
> +
> +static int mxu1_tiocmset(struct tty_struct *tty,
> +			 unsigned int set, unsigned int clear)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	unsigned int mcr;
> +	unsigned long flags;
> +
> +	dev_dbg(&port->dev, "%s - port %d", __func__, port->port_number);
> +
> +	if (mxport == NULL)
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&mxport->mxp_lock, flags);
> +	mcr = mxport->mxp_shadow_mcr;
> +
> +	if (set & TIOCM_RTS)
> +		mcr |= MXU1_MCR_RTS;
> +	if (set & TIOCM_DTR)
> +		mcr |= MXU1_MCR_DTR;
> +	if (set & TIOCM_LOOP)
> +		mcr |= MXU1_MCR_LOOP;
> +
> +	if (clear & TIOCM_RTS)
> +		mcr &= ~MXU1_MCR_RTS;
> +	if (clear & TIOCM_DTR)
> +		mcr &= ~MXU1_MCR_DTR;
> +	if (clear & TIOCM_LOOP)
> +		mcr &= ~MXU1_MCR_LOOP;
> +
> +	spin_unlock_irqrestore(&mxport->mxp_lock, flags);
> +
> +	return mxu1_set_mcr(mxport, mcr);
> +}
> +
> +static void mxu1_break(struct tty_struct *tty, int break_state)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +
> +	dev_dbg(&port->dev, "%s - state = %d", __func__, break_state);
> +
> +	if (mxport == NULL)
> +		return;
> +
> +	if (break_state == -1)
> +		mxport->mxp_send_break = MXU1_LCR_BREAK;
> +	else
> +		mxport->mxp_send_break = 0;
> +
> +	mxu1_set_termios(NULL, mxport->mxp_port, NULL);
> +}
> +
> +static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	struct mxu1_device *mxdev;
> +	struct usb_device *dev;
> +	struct urb *urb;
> +	int port_number;
> +	int status;
> +	__u16 open_settings = (u8)(MXU1_PIPE_MODE_CONTINUOUS |
> +				     MXU1_PIPE_TIMEOUT_ENABLE |
> +				     (MXU1_TRANSFER_TIMEOUT << 2));

Use u16 and split declatation from initialisation. u8 cast looks odd.

> +	if (!mxport)
> +		return -ENODEV;
> +
> +	dev = port->serial->dev;
> +	mxdev = mxport->mxp_mxdev;
> +
> +	port_number = port->port_number - port->minor;
> +
> +	mxport->mxp_msr = 0;
> +	mxport->mxp_shadow_mcr |= (MXU1_MCR_RTS | MXU1_MCR_DTR);

Drop parentheses.

> +
> +	if (mutex_lock_interruptible(&mxdev->mxd_lock))
> +		return -ERESTARTSYS;

Why do you need this? Again, these are one-port devices.

> +
> +	dev_dbg(&port->dev, "%s - start interrupt in urb", __func__);
> +	urb = mxdev->mxd_serial->port[0]->interrupt_in_urb;

Access directly though port pointer instead.

> +	if (!urb) {
> +		dev_err(&port->dev,
> +			"%s - no interrupt urb\n",
> +			__func__);

Too many line breaks. Not needed at all here.

> +		status = -EINVAL;

If you expect an interrupt endpoint then make sure it's there already at
probe.

> +		goto release_mxd_lock;
> +	}
> +	urb->context = mxdev;
> +	status = usb_submit_urb(urb, GFP_KERNEL);
> +	if (status) {
> +		dev_err(&port->dev,
> +			"%s - submit interrupt urb failed, %d\n",
> +			__func__,
> +			status);
> +		goto release_mxd_lock;
> +	}
> +
> +	mxu1_set_termios(NULL, port, NULL);

You have a tty argument, which is non-null unless used as a console.

> +
> +	dev_dbg(&port->dev, "%s - sending MXU1_OPEN_PORT", __func__);

Drop this debugging, just log any errors.

> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_OPEN_PORT,
> +				    open_settings,
> +				    (u8)(MXU1_UART1_PORT + port_number));

Use a intermediate variable for the port number if needed at all.

> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot send open command, %d\n",
> +			__func__,
> +			status);

Unnecessary line breaks again.

> +		goto unlink_int_urb;
> +	}
> +
> +	dev_dbg(&port->dev, "%s - sending MXU1_START_PORT", __func__);
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_START_PORT,
> +				    0, (u8)(MXU1_UART1_PORT + port_number));
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot send start command, %d\n",
> +			__func__,
> +			status);
> +		goto unlink_int_urb;
> +	}
> +
> +	dev_dbg(&port->dev, "%s - sending MXU1_PURGE_PORT", __func__);
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_PURGE_PORT,
> +				    MXU1_PURGE_INPUT,
> +				    (u8)(MXU1_UART1_PORT + port_number));
> +	if (status) {
> +		dev_err(&port->dev,
> +			"%s - cannot clear input buffers, %d\n",
> +			__func__,
> +			status);
> +
> +		goto unlink_int_urb;
> +	}
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_PURGE_PORT,
> +				    MXU1_PURGE_OUTPUT,
> +				    (u8)(MXU1_UART1_PORT + port_number));
> +	if (status) {
> +		dev_err(&port->dev,
> +			"%s - cannot clear output buffers, %d\n",
> +			__func__,
> +			status);
> +
> +		goto unlink_int_urb;
> +	}
> +
> +	/* reset the data toggle on the bulk endpoints to work around bug in
> +	 * host controllers where things get out of sync some times
> +	 */

Multi-line comments should be on the following format

	/*
	 * ...
	 */

> +	usb_clear_halt(dev, port->write_urb->pipe);
> +	usb_clear_halt(dev, port->read_urb->pipe);
> +
> +	mxu1_set_termios(NULL, port, NULL);
> +
> +	dev_dbg(&port->dev, "%s - sending MXU1_OPEN_PORT (2)", __func__);
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_OPEN_PORT,
> +				    open_settings,
> +				    (u8)(MXU1_UART1_PORT + port_number));
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot send open command, %d\n",
> +			__func__,
> +			status);
> +		goto unlink_int_urb;
> +	}
> +
> +	dev_dbg(&port->dev, "%s - sending MXU1_START_PORT (2)", __func__);
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_START_PORT,
> +				    0, (u8)(MXU1_UART1_PORT + port_number));
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot send start command, %d\n",
> +			__func__,
> +			status);
> +		goto unlink_int_urb;
> +	}
> +
> +	/* start read urb */
> +	dev_dbg(&port->dev, "%s - start read urb", __func__);
> +	urb = port->read_urb;
> +
> +	if (!urb) {
> +		dev_err(&port->dev, "%s - no read urb\n", __func__);
> +		status = -EINVAL;
> +		goto unlink_int_urb;
> +	}
> +
> +	urb->context = port;
> +	urb->dev = dev;
> +	status = usb_submit_urb(urb, GFP_KERNEL);
> +
> +	if (status) {
> +		dev_err(&port->dev, "%s - submit read urb failed, %d\n",
> +			__func__, status);
> +		goto unlink_int_urb;
> +	}

Use usb_serial_generic_open() to submit the read urb(s).

> +
> +	goto release_mxd_lock;
> +
> +unlink_int_urb:
> +	usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
> +
> +release_mxd_lock:
> +	mutex_unlock(&mxdev->mxd_lock);
> +	dev_dbg(&port->dev, "%s - exit %d", __func__, status);

Drop debugging.

> +
> +	return status;
> +}
> +
> +static void mxu1_close(struct usb_serial_port *port)
> +{
> +	struct mxu1_device *mxdev;
> +	struct mxu1_port *mxport;
> +	int port_number;
> +	unsigned long flags;
> +	int status = 0;
> +
> +	dev_dbg(&port->dev, "%s - port %d", __func__, port->port_number);
> +
> +	mxdev = usb_get_serial_data(port->serial);
> +	mxport = usb_get_serial_port_data(port);
> +	if (mxdev == NULL || mxport == NULL)
> +		return;

Again, not needed (anywhere).

> +
> +	usb_kill_urb(port->read_urb);
> +	usb_kill_urb(port->write_urb);
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	kfifo_reset_out(&port->write_fifo);
> +	spin_unlock_irqrestore(&port->lock, flags);

Use generic close implementation instead.

> +
> +	port_number = port->port_number - port->minor;
> +
> +	dev_dbg(&port->dev, "%s - sending MXU1_CLOSE_PORT", __func__);
> +	status = mxu1_send_ctrl_urb(port->serial,
> +				    MXU1_CLOSE_PORT,
> +				    0, (u8)(MXU1_UART1_PORT + port_number));
> +	if (status)
> +		dev_err(&port->dev,
> +			"%s - cannot send close port command, %d\n",
> +			__func__,
> +			status);

Close before killing urbs?

> +
> +	usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
> +
> +	dev_dbg(&port->dev, "%s - exit", __func__);

Remove.

> +}
> +
> +static void mxu1_handle_new_msr(struct mxu1_port *mxport, u8 msr)
> +{
> +	struct async_icount *icount;
> +	struct tty_struct *tty;
> +	unsigned long flags;
> +
> +	dev_dbg(&mxport->mxp_port->dev, "%s - msr 0x%02X", __func__, msr);
> +
> +	if (msr & MXU1_MSR_DELTA_MASK) {
> +		spin_lock_irqsave(&mxport->mxp_lock, flags);
> +		icount = &mxport->mxp_port->icount;
> +		if (msr & MXU1_MSR_DELTA_CTS)
> +			icount->cts++;
> +		if (msr & MXU1_MSR_DELTA_DSR)
> +			icount->dsr++;
> +		if (msr & MXU1_MSR_DELTA_CD)
> +			icount->dcd++;
> +		if (msr & MXU1_MSR_DELTA_RI)
> +			icount->rng++;
> +		if (mxport->mxp_msr_wait_flags == 1) {
> +			mxport->mxp_msr_wait_flags = 0;
> +			wake_up_interruptible(&mxport->mxp_msr_wait);

Drop wait flags and use port msr wait queue.

> +		}
> +		spin_unlock_irqrestore(&mxport->mxp_lock, flags);
> +	}
> +
> +	mxport->mxp_msr = msr & MXU1_MSR_MASK;
> +
> +	/* handle CTS flow control */
> +	tty = mxport->mxp_port->port.tty;

You need to use tty_port_tty_get() and put the reference when done.

> +
> +	if (tty && C_CRTSCTS(tty)) {
> +		if (msr & MXU1_MSR_CTS) {
> +			tty->hw_stopped = 0;
> +
> +			tty_wakeup(tty);
> +		} else {
> +			tty->hw_stopped = 1;
> +		}
> +	}

But I think you drop this bit.

> +}
> +
> +static void mxu1_interrupt_callback(struct urb *urb)
> +{
> +	struct mxu1_device *mxdev = (struct mxu1_device *)urb->context;

Cast not needed. Usb-serial core would already have setup the context to
be the usb-serial port.

> +	struct usb_serial_port *port;
> +	struct usb_serial *serial = mxdev->mxd_serial;
> +	struct mxu1_port *mxport;
> +	struct device *dev = &urb->dev->dev;
> +	unsigned char *data = urb->transfer_buffer;
> +	int length = urb->actual_length;
> +	int port_number;
> +	int function;
> +	int status = 0;
> +	u8 msr;
> +
> +	dev_dbg(&urb->dev->dev, "%s", __func__);

Unless this is a common interrupt urb for a multi-port device, simply
use the port device for all debug and error messages.

> +
> +	/* Check port is valid or not */
> +	if (mxdev == NULL)
> +		return;

Not needed.
> +
> +

Stray newline.

> +	switch (urb->status) {
> +	case 0:
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +		dev_dbg(dev, "%s - urb shutting down, %d",
> +			__func__, urb->status);
> +		return;
> +	default:
> +		dev_err(dev, "%s - nonzero urb status, %d\n",
> +			__func__, urb->status);

These should be debug level as you'd usually get these when unplugging
an open device.

> +		goto exit;
> +	}
> +
> +	if (length != 2) {
> +		dev_dbg(dev, "%s - bad packet size, %d", __func__, length);
> +		goto exit;
> +	}
> +
> +	if (data[0] == MXU1_CODE_HARDWARE_ERROR) {
> +		dev_err(dev, "%s - hardware error, %d\n", __func__, data[1]);
> +		goto exit;
> +	}
> +
> +	port_number = MXU1_GET_PORT_FROM_CODE(data[0]);
> +	function = MXU1_GET_FUNC_FROM_CODE(data[0]);

Is this needed if single port devices? Use static functions rather than
macros.

> +
> +	dev_dbg(dev, "%s - port_number %d, function %d, data 0x%02X",
> +		 __func__,
> +		 port_number,
> +		 function,
> +		 data[1]);
> +
> +	if (port_number >= serial->num_ports) {
> +		dev_err(dev, "%s - bad port number, %d\n",
> +			__func__, port_number);
> +		goto exit;
> +	}
> +
> +	port = serial->port[port_number];
> +
> +	mxport = usb_get_serial_port_data(port);
> +	if (!mxport)
> +		goto exit;
> +
> +	switch (function) {
> +	case MXU1_CODE_DATA_ERROR:
> +		dev_dbg(dev, "%s - DATA ERROR, port %d, data 0x%02X\n",
> +			 __func__,
> +			 port_number,
> +			 data[1]);
> +		break;
> +
> +	case MXU1_CODE_MODEM_STATUS:
> +		msr = data[1];
> +		dev_dbg(dev, "%s - port %d, msr 0x%02X",
> +			 __func__, port_number, msr);
> +		mxu1_handle_new_msr(mxport, msr);
> +		break;
> +
> +	default:
> +		dev_err(dev, "%s - unknown interrupt code, 0x%02X\n",
> +			__func__, data[1]);
> +		break;
> +	}
> +
> +exit:
> +	status = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (status)
> +		dev_err(dev, "%s - resubmit interrupt urb failed, %d\n",
> +			__func__, status);
> +}
> +
> +static struct usb_serial_driver mxuport11_device = {
> +	.driver = {
> +		.owner		= THIS_MODULE,
> +		.name		= "mxuport11",
> +	},
> +	.description		= "MOXA UPort 11x0",
> +	.id_table		= mxuport11_idtable,

Use mxu1 prefix throughout?

> +	.num_ports		= 1,
> +	.port_probe             = mxu1_port_probe,
> +	.attach			= mxu1_startup,
> +	.open			= mxu1_open,
> +	.close			= mxu1_close,
> +	.ioctl			= mxu1_ioctl,
> +	.set_termios		= mxu1_set_termios,
> +	.tiocmget		= mxu1_tiocmget,
> +	.tiocmset		= mxu1_tiocmset,
> +	.get_icount		= usb_serial_generic_get_icount,

You may want to set tiocmiwait to the generic implementation as well.

> +	.break_ctl		= mxu1_break,
> +	.read_int_callback	= mxu1_interrupt_callback,
> +};

Johan