Re: [PATCH V10 1/1] usb:serial: Add Fintek F81532/534 driver

[Johan Hovold]

On Thu, Sep 01, 2016 at 09:56:01AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> This driver is for Fintek F81532/F81534 USB to Serial Ports IC.
> 
> F81532 spec:
> https://drive.google.com/file/d/0B8vRwwYO7aMFOTRRMmhWQVNvajQ/view?usp=
> sharing
> 
> F81534 spec:
> https://drive.google.com/file/d/0B8vRwwYO7aMFV29pQWJqbVBNc00/view?usp=
> sharing
> 
> Features:
> 1. F81532 is 1-to-2 & F81534 is 1-to-4 serial ports IC
> 2. Support Baudrate from B50 to B115200.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@xxxxxxxxx>
> ---
> Changelog:
> V10
>     1. Change the submit/kill timming for read URBs, submit when first
>        serial port open and kill when final port close.
>     2. Remove all source code about controlling GPIOs.
>     3. Change the f81534_tiocmget() from reading shadow MSR with delay
>        20ms to read MSR register directly.
>     4. Using tty_port_initialized() to check port opened/closed.
>     5. Add sanity check for variables.
> 
> v9
>     1. Remove lots of code to make more generic for F81532/534. e.g.,
>        high baud rate support, RS485/422 mode switch, most of GPIO
>        control and internal storage write functional.
>     2. Change f81534_tiocmget() MSR delay from schedule_timeout_killable()
>        to wait_for_completion_killable_timeout(). This IC will delayed
>        receiving MSR change when doing loop-back test e.g., BurnInTest.
>        We'll reset completion "msr_done" in f81534_update_mctrl(). If we
>        changed MCR, the next f81534_tiocmget() will delay for 20ms or
>        continue with new MSR arrived.
>     3. Fix for non-zero buffer allocated in f81534_setup_ports(). It'll
>        make device malfunctional with incorrect tx length for other ports.
> 
> v8
>     1. Remove driver mode GPIOLIB & RS485 control support, the driver will
>        only load GPIO/UART Mode when driver attach() & port_probe().
>     2. Add more documents for 3 generation IC with f81534_calc_num_ports().
>     3. Simplify the GPIO register structure "f81534_pin_control".
>     4. Change all counter type from int to size_t.
>     5. Change some failed message with failed: "status code" and remove all
>        exclamation mark in messages.
>     6. Change all save blocks to block0 due to the driver is only used 1
>        block (block0) to save data.
>     7. Change read MSR in open() instead of port_probe().
>     8. use GFP_ATOMIC kmalloc mode in write().
>     9. Maintain old style with 1 read URBs and 4 write URBs like mxuports.c
>        I had tested with submit 4 read URBs, but it'll make some port freeze
>        when doing BurnInTest Port test.
> 
> v7
>     1. Make all gpiolib function with #ifdef CONFIG_GPIOLIB marco block.
>        Due to F81532/534 could run without gpiolib, we implements
>        f81534_prepare_gpio()/f81534_release_gpio() always success without
>        CONFIG_GPIOLIB.
>     2. Fix crash when receiving MSR change on driver load/unload. It's cause
>        by f81534_process_read_urb() get read URB callback data, but port
>        private data is not init complete or released. We solve with 2
>        modifications.
> 
>        1. add null pointer check with f81534_process_read_urb(). We'll skip
>           this report when port_priv = NULL.
>        2. when "one" port f81534_port_remove() is called, kill the port-0
>           read URB before kfree port_priv.
> 
> v6
>     1. Re-implement the write()/resume() function. Due to this device cant be
>        suitable with generic write(), we'll do the submit write URB when
>        write()/received tx empty/set_termios()/resume()
>     2. Logic/Phy Port mapping rewrite in f81534_port_probe() &
>        f81534_phy_to_logic_port().
>     3. Introduced "Port Hide" function. Some customer use F81532 reference
>        design for HW layout, but really use F81534 IC. We'll check
>        F81534_PORT_CONF_DISABLE_PORT flag with in uart mode field to do
>        port hide with port not used. It can be avoid end-user to use not
>        layouted port.
>     4. The 4x3 output-only open-drain pins for F81532/534 is designed for
>        control outer devices (with our EVB for examples, the 4 sets of pins
>        are designed to control transceiver mode). So we decide to implement
>        with gpiolib interface.
>     5. Add device vendor id with 0x2c42
> 
> v5
>     1. Change f81534_port_disable/enable() from H/W mode to S/W mode
>        It'll skip all rx data when port is not opened.
>     2. Some function modifier add with static (Thanks for Paul Bolle)
>     3. It's will direct return when count=0 in f81534_write() to
>        reduce spin_lock usage.
> 
> v4
>     1. clearify f81534_process_read_urb() with
>        f81534_process_per_serial_block(). (referenced from mxuport.c)
>     2. We limited f81534_write() max tx kfifo with 124Bytes.
>        Original subsystem is designed for auto tranmiting fifo data
>        if available. But we must wait for tx_empty for next tx data
>        (H/W design).
> 
>        With this kfifo size limit, we can use generic subsystem api with
>        f81534_write(). When usb_serial_generic_write_start() called after
>        first write URB complete, the fifo will no data. The generic
>        subsystem of write will go to idle state. Until we received
>        TX_EMPTY and release write spinlock, the fifo will fill max
>        124Bytes by following f81534_write().
> 
> v3
>     1. Migrate read, write and some routine from custom code to usbserial
>        subsystem callback function.
>     2. Use more defines to replece magic numbers to make it meaningful
>     3. Make more comments as document in source code.
> 
> v2
>     1. v1 version submit to staging tree, but Greg KH advised me to
>        cleanup source code & re-submit it to correct subsystem
>     2. Remove all custom ioctl commands
> 
>  drivers/usb/serial/Kconfig  |   10 +
>  drivers/usb/serial/Makefile |    1 +
>  drivers/usb/serial/f81534.c | 1437 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1448 insertions(+)
>  create mode 100644 drivers/usb/serial/f81534.c
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index 56ecb8b..0642864 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -255,6 +255,16 @@ config USB_SERIAL_F81232
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called f81232.
>  
> +config USB_SERIAL_F8153X
> +	tristate "USB Fintek F81532/534 Multi-Ports Serial Driver"
> +	help
> +	  Say Y here if you want to use the Fintek F81532/534 Multi-Ports
> +	  usb to serial adapter.

s/usb/USB/

> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called f81534.
> +
> +
>  config USB_SERIAL_GARMIN
>         tristate "USB Garmin GPS driver"
>         help
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index 349d9df..9e43b7b 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_USB_SERIAL_EDGEPORT)		+= io_edgeport.o
>  obj-$(CONFIG_USB_SERIAL_EDGEPORT_TI)		+= io_ti.o
>  obj-$(CONFIG_USB_SERIAL_EMPEG)			+= empeg.o
>  obj-$(CONFIG_USB_SERIAL_F81232)			+= f81232.o
> +obj-$(CONFIG_USB_SERIAL_F8153X)			+= f81534.o
>  obj-$(CONFIG_USB_SERIAL_FTDI_SIO)		+= ftdi_sio.o
>  obj-$(CONFIG_USB_SERIAL_GARMIN)			+= garmin_gps.o
>  obj-$(CONFIG_USB_SERIAL_IPAQ)			+= ipaq.o
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> new file mode 100644
> index 0000000..c2ce637
> --- /dev/null
> +++ b/drivers/usb/serial/f81534.c
> @@ -0,0 +1,1437 @@
> +/*
> + * F81532/F81534 USB to Serial Ports Bridge
> + *
> + * F81532 => 2 Serial Ports
> + * F81534 => 4 Serial Ports
> + *
> + * 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.
> + *
> + * Copyright (C) Feature Integration Technology Inc., (Fintek)
> + *		 Tom Tsai (Tom_Tsai@xxxxxxxxxxxxx)
> + *		 Peter Hong (Peter_Hong@xxxxxxxxxxxxx)

You should include the year in the copyright notice as well.

> + *
> + * The F81532/F81534 had 1 control endpoint for setting, 1 endpoint bulk-out
> + * for all serial port TX and 1 endpoint bulk-in for all serial port read in
> + * (Read Data/MSR/LSR).
> + *
> + * Write URB is fixed with 512bytes, per serial port used 128Bytes.
> + * It can be described by f81534_prepare_write_buffer()
> + *
> + * Read URB is 512Bytes max, per serial port used 128Bytes.
> + * It can be described by f81534_process_read_urb() and maybe received with
> + * 128x1,2,3,4 bytes.
> + *
> + */
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +
> +/* Serial Port register Address */
> +#define F81534_UART_BASE_ADDRESS	0x1200
> +#define F81534_DIVISOR_LSB_REG		(0x00 + F81534_UART_BASE_ADDRESS)
> +#define F81534_DIVISOR_MSB_REG		(0x01 + F81534_UART_BASE_ADDRESS)
> +#define F81534_FIFO_CONTROL_REG		(0x02 + F81534_UART_BASE_ADDRESS)
> +#define F81534_LINE_CONTROL_REG		(0x03 + F81534_UART_BASE_ADDRESS)
> +#define F81534_MODEM_CONTROL_REG	(0x04 + F81534_UART_BASE_ADDRESS)
> +#define F81534_MODEM_STATUS_REG		(0x06 + F81534_UART_BASE_ADDRESS)
> +#define F81534_CONFIG1_REG		(0x09 + F81534_UART_BASE_ADDRESS)
> +
> +#define F81534_DEF_CONF_ADDRESS_START	0x3000
> +#define F81534_DEF_CONF_SIZE		8
> +
> +#define F81534_CUSTOM_ADDRESS_START	0x2f00
> +#define F81534_CUSTOM_DATA_SIZE		0x10
> +#define F81534_CUSTOM_NO_CUSTOM_DATA	(-1)
> +#define F81534_CUSTOM_VALID_TOKEN	0xf0
> +#define F81534_CONF_OFFSET		1
> +
> +#define F81534_MAX_DATA_BLOCK		64
> +#define F81534_MAX_BUS_RETRY		2000

This seems a bit excessive, is 2000 retries really needed?

> +/* Default URB timeout for USB operations */
> +#define F81534_USB_MAX_RETRY		10
> +#define F81534_USB_TIMEOUT		1000
> +#define F81534_SET_GET_REGISTER		0xA0
> +
> +#define F81534_NUM_PORT			4
> +#define F81534_UNUSED_PORT		0xff
> +#define F81534_WRITE_BUFFER_SIZE	512
> +
> +#define DRIVER_DESC			"Fintek F81532/F81534"
> +#define FINTEK_VENDOR_ID_1		0x1934
> +#define FINTEK_VENDOR_ID_2		0x2C42
> +#define FINTEK_DEVICE_ID		0x1202
> +#define F81534_MAX_TX_SIZE		100

Should this one not be 124 just like for rx?

> +#define F81534_MAX_RX_SIZE		124
> +#define F81534_RECEIVE_BLOCK_SIZE	128
> +
> +#define F81534_TOKEN_RECEIVE		0x01
> +#define F81534_TOKEN_WRITE		0x02
> +#define F81534_TOKEN_TX_EMPTY		0x03
> +#define F81534_TOKEN_MSR_CHANGE		0x04
> +
> +/*
> + * We used interal SPI bus to access FLASH section. We must wait the SPI bus to
> + * idle if we performed any command.
> + *
> + * SPI Bus status register: F81534_BUS_REG_STATUS
> + *	Bit 0/1	: BUSY
> + *	Bit 2	: IDLE
> + */
> +#define F81534_BUS_BUSY			(BIT(0) | BIT(1))
> +#define F81534_BUS_IDLE			BIT(2)
> +#define F81534_BUS_READ_DATA		0x1004
> +#define F81534_BUS_REG_STATUS		0x1003
> +#define F81534_BUS_REG_START		0x1002
> +#define F81534_BUS_REG_END		0x1001
> +
> +#define F81534_CMD_READ			0x03
> +
> +#define F81534_DEFAULT_BAUD_RATE	9600
> +#define F81534_MAX_BAUDRATE		115200
> +
> +#define F81534_PORT_CONF_DISABLE_PORT	BIT(3)
> +#define F81534_PORT_CONF_NOT_EXIST_PORT	BIT(7)
> +#define F81534_PORT_UNAVAILABLE		\
> +	(F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
> +
> +#define F81534_1X_RXTRIGGER		0xc3
> +#define F81534_8X_RXTRIGGER		0xcf
> +
> +static const struct usb_device_id f81534_id_table[] = {
> +	{USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID)},
> +	{USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID)},
> +	{}			/* Terminating entry */
> +};
> +
> +struct f81534_serial_private {
> +	u8 conf_data[F81534_DEF_CONF_SIZE];
> +	int tty_idx[F81534_NUM_PORT];
> +	u32 setting_idx;

This should be u8.

> +	int opened_port;
> +	struct mutex urb_mutex;
> +};
> +
> +struct f81534_port_private {
> +	bool is_tx_not_empty;

Please invert this one and drop the "_not" infix.

> +	spinlock_t tx_empty_lock;
> +	struct mutex mcr_mutex;
> +	spinlock_t msr_lock;
> +	u8 shadow_mcr;
> +	u8 shadow_msr;
> +	u8 phy;
> +};
> +
> +/*
> + * Get the current logical port index of this device. e.g., If this port is
> + * ttyUSB2 and start port is ttyUSB0, this will return 2.
> + */
> +static int f81534_port_index(struct usb_serial_port *port)
> +{
> +	return port->port_number;
> +}

Just use port->port_number directly instead of this helper.

> +/*
> + * Find logic serial port index with H/W phy index mapping. Due to our device
> + * can be enable/disable port by internal storage to make the port phy no
> + * continuously, we can use this to find phy & logical port mapping.
> + */
> +static int f81534_phy_to_logic_port(struct usb_serial *serial, int phy)
> +{
> +	struct f81534_serial_private *priv = usb_get_serial_data(serial);
> +	size_t count = 0;
> +	size_t i;
> +
> +	for (i = 0; i < phy; ++i) {
> +		if (priv->conf_data[i] & F81534_PORT_UNAVAILABLE)
> +			continue;
> +
> +		++count;
> +	}
> +
> +	dev_dbg(&serial->interface->dev, "%s: phy: %d count: %zu\n", __func__,
> +			phy, count);
> +	return count;
> +}
> +
> +static int f81534_logic_to_phy_port(struct usb_serial *serial,
> +					struct usb_serial_port *port)
> +{
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(port->serial);
> +	int port_index = f81534_port_index(port);
> +	int count = 0;
> +	int i;
> +
> +	for (i = 0; i < F81534_NUM_PORT; ++i) {
> +		if (serial_priv->conf_data[i] & F81534_PORT_UNAVAILABLE)
> +			continue;
> +
> +		if (port_index == count)
> +			return i;
> +
> +		++count;
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int f81534_set_normal_register(struct usb_serial *serial, u16 reg,
> +					u8 data)
> +{
> +	struct usb_interface *interface = serial->interface;
> +	struct usb_device *dev = serial->dev;
> +	size_t count = F81534_USB_MAX_RETRY;
> +	int status;
> +	u8 *tmp;
> +
> +	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	*tmp = data;
> +
> +	/*
> +	 * Our device maybe not reply when heavily loading, We'll retry for
> +	 * F81534_USB_MAX_RETRY times.
> +	 */
> +	while (count--) {
> +		status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> +					 F81534_SET_GET_REGISTER,
> +					 USB_TYPE_VENDOR | USB_DIR_OUT,
> +					 reg, 0, tmp, sizeof(u8),
> +					 F81534_USB_TIMEOUT);
> +		if (status > 0) {
> +			status = 0;
> +			break;
> +		} else if (status == 0) {
> +			status = -EIO;
> +		}
> +	}
> +
> +	if (status < 0) {
> +		dev_err(&interface->dev, "%s: reg: %x data: %x failed: %d\n",
> +				__func__, reg, data, status);
> +	}
> +
> +	kfree(tmp);
> +	return status;
> +}
> +
> +static int f81534_get_normal_register(struct usb_serial *serial, u16 reg,
> +					u8 *data)
> +{
> +	struct usb_interface *interface = serial->interface;
> +	struct usb_device *dev = serial->dev;
> +	size_t count = F81534_USB_MAX_RETRY;
> +	int status;
> +	u8 *tmp;
> +
> +	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Our device maybe not reply when heavily loading, We'll retry for
> +	 * F81534_USB_MAX_RETRY times.
> +	 */
> +	while (count--) {
> +		status = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
> +					 F81534_SET_GET_REGISTER,
> +					 USB_TYPE_VENDOR | USB_DIR_IN,
> +					 reg, 0, tmp, sizeof(u8),
> +					 F81534_USB_TIMEOUT);
> +		if (status > 0) {
> +			status = 0;
> +			break;
> +		} else if (status == 0) {
> +			status = -EIO;
> +		}
> +	}
> +
> +	if (status < 0) {
> +		dev_err(&interface->dev, "%s: reg: %x failed: %d\n", __func__,
> +				reg, status);
> +		goto end;
> +	}
> +
> +	*data = *tmp;
> +
> +end:
> +	kfree(tmp);
> +	return status;
> +}
> +
> +static int f81534_setregister(struct usb_serial *serial, u8 uart, u16 reg,
> +				u8 data)

Add an underscore ('_') between "set" and "register".

If you pass the usb_serial_port instead of usb_serial struct you can
handle the phy mapping here instead of at every call site.

Same for get_register.

> +{
> +	return f81534_set_normal_register(serial, reg + uart * 0x10, data);

Please use a define for the uart register offset (0x10) as well.

> +}
> +
> +static int f81534_getregister(struct usb_serial *serial, u8 uart, u16 reg,
> +				u8 *data)
> +{
> +	return f81534_get_normal_register(serial, reg + uart * 0x10, data);

Same here.

> +}
> +
> +/*
> + * If we try to access the internal flash via SPI bus, we should check the bus
> + * status for every command. e.g., F81534_BUS_REG_START/F81534_BUS_REG_END
> + */
> +static int f81534_command_delay(struct usb_serial *serial)
> +{
> +	size_t count = F81534_MAX_BUS_RETRY;
> +	unsigned char tmp;

Please use u8 consistently throughout for register values.

> +	int status;
> +
> +	do {
> +		status = f81534_get_normal_register(serial,
> +							F81534_BUS_REG_STATUS,
> +							&tmp);
> +		if (status)
> +			return status;
> +
> +		if (tmp & F81534_BUS_BUSY)
> +			continue;
> +
> +		if (tmp & F81534_BUS_IDLE)
> +			break;
> +
> +	} while (--count);
> +
> +	if (!count)
> +		return -EIO;

This seems to deserve an error message.

> +
> +	status = f81534_set_normal_register(serial, F81534_BUS_REG_STATUS,
> +				tmp & ~F81534_BUS_IDLE);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static int f81534_get_normal_register_with_delay(struct usb_serial *serial,
> +							u16 reg, u8 *data)
> +{
> +	int status;
> +
> +	status = f81534_get_normal_register(serial, reg, data);
> +	if (status)
> +		return status;
> +
> +	status = f81534_command_delay(serial);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static int f81534_set_normal_register_with_delay(struct usb_serial *serial,
> +							u16 reg, u8 data)
> +{
> +	int status;
> +
> +	status = f81534_set_normal_register(serial, reg, data);
> +	if (status)
> +		return status;
> +
> +	status = f81534_command_delay(serial);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static int f81534_read_data(struct usb_serial *serial, u32 address,
> +				size_t size, unsigned char *buf)
> +{
> +	u8 tmp_buf[F81534_MAX_DATA_BLOCK];
> +	size_t block = 0;
> +	size_t read_size;
> +	size_t count;
> +	int status;
> +	int offset;
> +	u16 reg_tmp;
> +
> +	status = f81534_set_normal_register_with_delay(serial,
> +				F81534_BUS_REG_START, F81534_CMD_READ);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_normal_register_with_delay(serial,
> +				F81534_BUS_REG_START, (address >> 16) & 0xff);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_normal_register_with_delay(serial,
> +				F81534_BUS_REG_START, (address >> 8) & 0xff);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_normal_register_with_delay(serial,
> +				F81534_BUS_REG_START, (address >> 0) & 0xff);
> +	if (status)
> +		return status;
> +
> +	/* Continuous read mode */
> +	do {
> +		read_size = min_t(u32, F81534_MAX_DATA_BLOCK, size);

Use size_t as type here instead of u32.

> +
> +		for (count = 0; count < read_size; ++count) {
> +			/* To write F81534_BUS_REG_END when final byte */
> +			if (size <= F81534_MAX_DATA_BLOCK && read_size ==
> +					count + 1)

I suggest you break the line after && (instead of after ==).

> +				reg_tmp = F81534_BUS_REG_END;
> +			else
> +				reg_tmp = F81534_BUS_REG_START;
> +
> +			/*
> +			 * Dummy code, force IC to generate a read pulse, the
> +			 * set of value 0xf1 is dont care (any value is ok)
> +			 */
> +			status = f81534_set_normal_register_with_delay(serial,
> +						reg_tmp, 0xf1);
> +			if (status)
> +				return status;
> +
> +			status = f81534_get_normal_register_with_delay(serial,
> +						F81534_BUS_READ_DATA,
> +						&tmp_buf[count]);
> +			if (status)
> +				return status;
> +
> +			offset = count + block * F81534_MAX_DATA_BLOCK;
> +			buf[offset] = tmp_buf[count];
> +		}
> +
> +		size -= read_size;
> +		++block;
> +	} while (size);
> +
> +	return 0;
> +}
> +
> +static int f81534_prepare_write_buffer(struct usb_serial_port *port,
> +					void *dest, size_t size)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	unsigned char *ptr = (unsigned char *)dest;

Cast not needed, and please use a more descriptive name like "buf".

> +	int port_num = port_priv->phy;
> +	int i;
> +	u8 tx_len;
> +
> +	/*
> +	 * The block layout is fixed with 4x128 Bytes, per 128 Bytes a port.
> +	 * index 0: port phy idx (e.g., 0,1,2,3)
> +	 * index 1: only F81534_TOKEN_WRITE
> +	 * index 2: serial TX out length
> +	 * index 3: fix to 0
> +	 * index 4~127: serial out data block
> +	 */
> +	for (i = 0; i < F81534_NUM_PORT; ++i) {
> +		ptr[F81534_RECEIVE_BLOCK_SIZE * i] = i;
> +		ptr[F81534_RECEIVE_BLOCK_SIZE * i + 1] = F81534_TOKEN_WRITE;
> +		ptr[F81534_RECEIVE_BLOCK_SIZE * i + 2] = 0;
> +		ptr[F81534_RECEIVE_BLOCK_SIZE * i + 3] = 0;

Minor nit: please put the constant factor on the right of the
multiplication operator here and below.

> +	}
> +
> +	tx_len = kfifo_out_locked(&port->write_fifo,
> +				&ptr[F81534_RECEIVE_BLOCK_SIZE * port_num + 4],
> +				F81534_MAX_TX_SIZE, &port->lock);
> +
> +	ptr[F81534_RECEIVE_BLOCK_SIZE * port_num + 2] = tx_len;
> +
> +	return F81534_WRITE_BUFFER_SIZE;

You don't use the return value of this function so just make it void.

> +}
> +
> +static int f81534_submit_writer(struct usb_serial_port *port, gfp_t mem_flags)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	struct urb *urb;
> +	unsigned long flags;
> +	int result;
> +
> +	/* Check is any data in write_fifo */
> +	spin_lock_irqsave(&port->lock, flags);
> +
> +	if (kfifo_is_empty(&port->write_fifo)) {
> +		spin_unlock_irqrestore(&port->lock, flags);
> +		return 0;
> +	}
> +
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	/* Check H/W is TXEMPTY */
> +	spin_lock_irqsave(&port_priv->tx_empty_lock, flags);
> +
> +	if (port_priv->is_tx_not_empty) {
> +		spin_unlock_irqrestore(&port_priv->tx_empty_lock, flags);
> +		return 0;
> +	}

You can use a flags field and atomic bit ops to modify the tx_empty flag
and drop the tx_empty lock. You may also want to check the tx_empty flag
under the port lock above.

> +	port_priv->is_tx_not_empty = true;
> +	spin_unlock_irqrestore(&port_priv->tx_empty_lock, flags);
> +
> +	urb = port->write_urbs[0];
> +	f81534_prepare_write_buffer(port, port->bulk_out_buffers[0],
> +					port->bulk_out_size);
> +	urb->transfer_buffer_length = F81534_WRITE_BUFFER_SIZE;
> +
> +	result = usb_submit_urb(urb, mem_flags);
> +	if (result) {
> +		spin_lock_irqsave(&port_priv->tx_empty_lock, flags);
> +		port_priv->is_tx_not_empty = false;
> +		spin_unlock_irqrestore(&port_priv->tx_empty_lock, flags);
> +
> +		dev_err(&port->dev, "%s: submit failed: %d\n", __func__,
> +				result);
> +		return result;
> +	}
> +
> +	usb_serial_port_softint(port);
> +	return 0;
> +}
> +
> +static u32 f81534_calc_baud_divisor(u32 baudrate, u32 clockrate)
> +{
> +	if (!baudrate)
> +		return 0;
> +
> +	/* Round to nearest divisor */
> +	return DIV_ROUND_CLOSEST(clockrate, baudrate);
> +}
> +
> +static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
> +					u8 lcr)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	struct usb_serial *serial = port->serial;
> +	u16 device_port = port_priv->phy;

The uart argument to f81534_setregister is u8 so use u8 here, but it may
be better to deal with the mapping in f81534_setregister as mentioned
above.

Please also use a consistent name for these variables. You currently use
"phy", "uart", "device_port", "phy_port_num", etc, which is needlessly
confusing.

Perhaps use "phy_num" as opposed to (usb serial) "port_num"?

> +	u32 divisor;
> +	int status;
> +	u8 value;
> +
> +	if (baudrate <= 1200)
> +		value = F81534_1X_RXTRIGGER;	/* 128 FIFO & TL: 1x */
> +	else
> +		value = F81534_8X_RXTRIGGER;	/* 128 FIFO & TL: 8x */
> +
> +	status = f81534_setregister(serial, device_port, F81534_CONFIG1_REG,
> +					value);
> +	if (status) {
> +		dev_err(&port->dev, "%s: CONFIG1 setting failed.\n", __func__);

Nit: drop the period ('.') from these error messages for consistency.

> +		return status;
> +	}
> +
> +	if (baudrate <= 1200)
> +		value = UART_FCR_TRIGGER_1 | UART_FCR_ENABLE_FIFO; /* TL: 1 */
> +	else
> +		value = UART_FCR_R_TRIG_11 | UART_FCR_ENABLE_FIFO; /* TL: 14 */
> +
> +	status = f81534_setregister(serial, device_port,
> +					F81534_FIFO_CONTROL_REG, value);
> +	if (status) {
> +		dev_err(&port->dev, "%s: FCR setting failed.\n", __func__);
> +		return status;
> +	}
> +
> +	divisor = f81534_calc_baud_divisor(baudrate, F81534_MAX_BAUDRATE);
> +	value = UART_LCR_DLAB;
> +	status = f81534_setregister(serial, device_port,
> +					F81534_LINE_CONTROL_REG, value);
> +	if (status) {
> +		dev_err(&port->dev, "%s: set LCR failed.\n", __func__);
> +		return status;
> +	}
> +
> +	value = divisor & 0xff;
> +	status = f81534_setregister(serial, device_port,
> +					F81534_DIVISOR_LSB_REG, value);
> +	if (status) {
> +		dev_err(&port->dev, "%s: set DLAB LSB failed.\n", __func__);
> +		return status;
> +	}
> +
> +	value = (divisor >> 8) & 0xff;
> +	status = f81534_setregister(serial, device_port,
> +					F81534_DIVISOR_MSB_REG, value);
> +	if (status) {
> +		dev_err(&port->dev, "%s: set DLAB MSB failed.\n", __func__);
> +		return status;
> +	}
> +
> +	status = f81534_setregister(serial, device_port,
> +					F81534_LINE_CONTROL_REG, lcr);
> +	if (status) {
> +		dev_err(&port->dev, "%s: set LCR failed.\n", __func__);
> +		return status;
> +	}
> +
> +	return 0;
> +}
> +
> +static int f81534_update_mctrl(struct usb_serial_port *port, unsigned int set,
> +				unsigned int clear)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	int status;
> +	u8 tmp;
> +
> +	mutex_lock(&port_priv->mcr_mutex);
> +
> +	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0) {
> +		mutex_unlock(&port_priv->mcr_mutex);
> +		return 0;	/* no change */
> +	}
> +
> +	/* 'Set' takes precedence over 'Clear' */
> +	clear &= ~set;
> +
> +	/* Always enable UART_MCR_OUT2 */
> +	tmp = UART_MCR_OUT2 | port_priv->shadow_mcr;
> +
> +	if (clear & TIOCM_DTR)
> +		tmp &= ~UART_MCR_DTR;
> +
> +	if (clear & TIOCM_RTS)
> +		tmp &= ~UART_MCR_RTS;
> +
> +	if (set & TIOCM_DTR)
> +		tmp |= UART_MCR_DTR;
> +
> +	if (set & TIOCM_RTS)
> +		tmp |= UART_MCR_RTS;
> +
> +	status = f81534_setregister(port->serial, port_priv->phy,
> +					F81534_MODEM_CONTROL_REG, tmp);
> +	if (status < 0) {
> +		dev_err(&port->dev, "%s: MCR write failed.\n", __func__);
> +		mutex_unlock(&port_priv->mcr_mutex);
> +		return status;
> +	}
> +
> +	port_priv->shadow_mcr = tmp;
> +	mutex_unlock(&port_priv->mcr_mutex);
> +	return 0;
> +}
> +
> +/*
> + * This function will search the data area with token F81534_CUSTOM_VALID_TOKEN
> + * for latest configuration index. If nothing found (*index = -1), the caller
> + * will load default configure in F81534_DEF_CONF_ADDRESS_START section.
> + *
> + * Due to we only use block0 to save data, so *index should be 0 or
> + * F81534_CUSTOM_NO_CUSTOM_DATA(-1).
> + */
> +static int f81534_find_config_idx(struct usb_serial *serial, uintptr_t *index)

Just pass an unsigned type for index, what you do with the index (e.g.
store it as a pointer) is up to the caller and need not leak to this
function.

> +{
> +	u8 custom_data;
> +	int status;
> +
> +	status = f81534_read_data(serial, F81534_CUSTOM_ADDRESS_START, 1,
> +				&custom_data);
> +	if (status) {
> +		dev_err(&serial->interface->dev, "%s: read failed: %d\n",
> +				__func__, status);
> +		return status;
> +	}
> +
> +	/*
> +	 * If had custom setting, override. The 1st byte is a
> +	 * indicator. 0xff is empty, F81534_CUSTOM_VALID_TOKEN is had
> +	 * data. read and skip with 1st data.
> +	 */
> +	if (custom_data == F81534_CUSTOM_VALID_TOKEN)
> +		*index = 0;
> +	else
> +		*index = F81534_CUSTOM_NO_CUSTOM_DATA;
> +
> +	return 0;
> +}
> +
> +/*
> + * We had 2 generation of F81532/534 IC. All has an internal storage.
> + *
> + * 1st is pure USB-to-TTL RS232 IC and designed for 4 ports only, no any
> + * internal data will used. All mode and gpio control should manually set
> + * by AP or Driver and all storage space value are 0xff. The
> + * f81534_calc_num_ports() will run to final we marked as "oldest version"
> + * for this IC.
> + *
> + * 2rd is designed to more generic to use any transceiver and this is our
> + * mass production type. We'll save data in F81534_CUSTOM_ADDRESS_START
> + * (0x2f00) with 9bytes. The 1st byte is a indicater. If the token is not

s/not// ?

> + * F81534_CUSTOM_VALID_TOKEN(0xf0), the IC is 2nd gen type, the following
> + * 4bytes save port mode (0:RS232/1:RS485 Invert/2:RS485), and the last
> + * 4bytes save GPIO state(value from 0~7 to represent 3 GPIO output pin).
> + * The f81534_calc_num_ports() will run to "new style" with checking
> + * F81534_PORT_UNAVAILABLE section.
> + */
> +static int f81534_calc_num_ports(struct usb_serial *serial)
> +{
> +	unsigned char setting[F81534_CUSTOM_DATA_SIZE];
> +	uintptr_t setting_idx;

So just use u8 here.

> +	u8 num_port = 0;
> +	int status;
> +	size_t i;
> +
> +	/* Check had custom setting */
> +	status = f81534_find_config_idx(serial, &setting_idx);
> +	if (status) {
> +		dev_err(&serial->interface->dev, "%s: find idx failed: %d\n",
> +				__func__, status);
> +		return 0;
> +	}
> +
> +	/* Save the configuration area idx as private data for attach() */
> +	usb_set_serial_data(serial, (void *)setting_idx);
> +
> +	/* Read default board setting */
> +	status = f81534_read_data(serial, F81534_DEF_CONF_ADDRESS_START,
> +				  F81534_NUM_PORT, setting);

Why read the default setting in case you already know you have custom
data?

> +	if (status) {
> +		dev_err(&serial->interface->dev, "%s: read failed: %d\n",
> +				__func__, status);
> +		return 0;
> +	}
> +
> +	/*
> +	 * If had custom setting, override it. 1st byte is a indicator, 0xff
> +	 * is empty, F81534_CUSTOM_VALID_TOKEN is had data, then skip with 1st
> +	 * data

It's really hard to tell what you're trying to say here, please
rephrase.

> +	 */
> +	if (setting_idx != F81534_CUSTOM_NO_CUSTOM_DATA) {
> +		status = f81534_read_data(serial, F81534_CUSTOM_ADDRESS_START +
> +						F81534_CONF_OFFSET,
> +						sizeof(setting), setting);
> +		if (status) {
> +			dev_err(&serial->interface->dev,
> +					"%s: get custom data failed: %d\n",
> +					__func__, status);
> +			return 0;
> +		}
> +
> +		dev_dbg(&serial->interface->dev,
> +				"%s: read config from block: %d\n", __func__,
> +				(unsigned int)setting_idx);
> +	} else {
> +		dev_dbg(&serial->interface->dev, "%s: read default config\n",
> +				__func__);
> +	}
> +
> +	/* New style, find all possible ports */
> +	num_port = 0;
> +	for (i = 0; i < F81534_NUM_PORT; ++i) {
> +		if (setting[i] & F81534_PORT_UNAVAILABLE)
> +			continue;
> +
> +		++num_port;
> +	}
> +
> +	if (num_port)
> +		return num_port;
> +
> +	dev_warn(&serial->interface->dev, "%s: Read Failed. default 4 ports\n",
> +			__func__);
> +	return 4;		/* Nothing found, oldest version IC */
> +}
> +
> +static void f81534_set_termios(struct tty_struct *tty,
> +				struct usb_serial_port *port,
> +				struct ktermios *old_termios)
> +{
> +	u8 new_lcr = 0;
> +	int status;
> +	u32 baud;
> +
> +	if (C_BAUD(tty) == B0)
> +		f81534_update_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
> +	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
> +		f81534_update_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
> +
> +	if (C_PARENB(tty)) {
> +		new_lcr |= UART_LCR_PARITY;
> +
> +		if (!C_PARODD(tty))
> +			new_lcr |= UART_LCR_EPAR;
> +
> +		if (C_CMSPAR(tty))
> +			new_lcr |= UART_LCR_SPAR;
> +	}
> +
> +	if (C_CSTOPB(tty))
> +		new_lcr |= UART_LCR_STOP;
> +
> +	switch (C_CSIZE(tty)) {
> +	case CS5:
> +		new_lcr |= UART_LCR_WLEN5;
> +		break;
> +	case CS6:
> +		new_lcr |= UART_LCR_WLEN6;
> +		break;
> +	case CS7:
> +		new_lcr |= UART_LCR_WLEN7;
> +		break;
> +	default:
> +	case CS8:
> +		new_lcr |= UART_LCR_WLEN8;
> +		break;
> +	}
> +
> +	baud = tty_get_baud_rate(tty);
> +	if (!baud)
> +		return;
> +
> +	if (baud > F81534_MAX_BAUDRATE) {
> +		if (old_termios)
> +			baud = old_termios->c_ospeed;

Use tty_termios_baud_rate() here.

> +		else
> +			baud = F81534_DEFAULT_BAUD_RATE;
> +	}
> +
> +	dev_dbg(&port->dev, "%s: baud: %d\n", __func__, baud);
> +	tty_encode_baud_rate(tty, baud, baud);

This is only needed in case the requested speed is not supported (e.g.
baud > F81534_MAX_BAUDRATE).

> +
> +	status = f81534_set_port_config(port, baud, new_lcr);
> +	if (status < 0) {
> +		dev_err(&port->dev, "%s: set port config failed: %d\n",
> +				__func__, status);
> +	}
> +}
> +
> +static int f81534_submit_read_urb(struct usb_serial *serial, gfp_t flags)
> +{
> +	return usb_serial_generic_submit_read_urbs(serial->port[0], flags);
> +}
> +
> +static void f81534_msr_changed(struct usb_serial_port *port, u8 msr)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	struct tty_struct *tty;
> +	unsigned long flags;
> +	u8 old_msr;
> +
> +	if (!(msr & UART_MSR_ANY_DELTA))
> +		return;
> +
> +	spin_lock_irqsave(&port_priv->msr_lock, flags);
> +	old_msr = port_priv->shadow_msr;
> +	port_priv->shadow_msr = msr;
> +	spin_unlock_irqrestore(&port_priv->msr_lock, flags);
> +
> +	dev_dbg(&port->dev, "%s: MSR from %02x to %02x\n", __func__, old_msr,
> +			msr);
> +
> +	/* Update input line counters */
> +	if (msr & UART_MSR_DCTS)
> +		port->icount.cts++;
> +	if (msr & UART_MSR_DDSR)
> +		port->icount.dsr++;
> +	if (msr & UART_MSR_DDCD)
> +		port->icount.dcd++;
> +	if (msr & UART_MSR_TERI)
> +		port->icount.rng++;
> +
> +	wake_up_interruptible(&port->port.delta_msr_wait);
> +
> +	if (!(msr & UART_MSR_DDCD))
> +		return;
> +
> +	dev_dbg(&port->dev, "%s: DCD Changed: port %d from %x to %x.\n",
> +			__func__, port_priv->phy, old_msr, msr);
> +
> +	tty = tty_port_tty_get(&port->port);
> +	if (!tty)
> +		return;
> +
> +	usb_serial_handle_dcd_change(port, tty, msr & UART_MSR_DCD);
> +	tty_kref_put(tty);
> +}
> +
> +static int f81534_read_msr(struct usb_serial_port *port)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	struct usb_serial *serial = port->serial;
> +	int phy = port_priv->phy;
> +	int status;
> +	unsigned long flags;
> +	u8 msr;
> +
> +	/* Get MSR initial value */
> +	status = f81534_getregister(serial, phy, F81534_MODEM_STATUS_REG,
> +					&msr);
> +	if (status)
> +		return status;
> +
> +	/* Force update current state */
> +	spin_lock_irqsave(&port_priv->msr_lock, flags);
> +	port_priv->shadow_msr = msr;
> +	spin_unlock_irqrestore(&port_priv->msr_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int f81534_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(port->serial);
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +	int phy = port_priv->phy;
> +	int status;
> +
> +	status = f81534_setregister(port->serial, phy,
> +				F81534_FIFO_CONTROL_REG, UART_FCR_ENABLE_FIFO |
> +				UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> +	if (status) {
> +		dev_err(&port->dev, "%s: Clear FIFO failed: %d\n", __func__,
> +				status);
> +		return status;
> +	}
> +
> +	if (tty)
> +		f81534_set_termios(tty, port, &tty->termios);

You should pass NULL as old_termios here.

> +
> +	status = f81534_read_msr(port);
> +	if (status)
> +		return status;
> +
> +	mutex_lock(&serial_priv->urb_mutex);
> +
> +	spin_lock_irqsave(&port_priv->tx_empty_lock, flags);
> +	port_priv->is_tx_not_empty = false;
> +	spin_unlock_irqrestore(&port_priv->tx_empty_lock, flags);
> +
> +	/* Submit Read URBs for first port opened */
> +	if (!serial_priv->opened_port) {
> +		status = f81534_submit_read_urb(port->serial, GFP_KERNEL);
> +		if (status)
> +			goto exit;
> +	}
> +
> +	serial_priv->opened_port++;
> +
> +exit:
> +	mutex_unlock(&serial_priv->urb_mutex);
> +
> +	return status;
> +}
> +
> +static void f81534_close(struct usb_serial_port *port)
> +{
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(port->serial);
> +	struct usb_serial_port *port0 = port->serial->port[0];
> +	unsigned long flags;
> +	size_t i;
> +
> +	/* Referenced from usb_serial_generic_close() */
> +	for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i)
> +		usb_kill_urb(port->write_urbs[i]);

You only use the first bulk urb for writing.

> +	spin_lock_irqsave(&port->lock, flags);
> +	kfifo_reset_out(&port->write_fifo);
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	/* Kill Read URBs when final port closed */
> +	mutex_lock(&serial_priv->urb_mutex);
> +	serial_priv->opened_port--;
> +
> +	if (!serial_priv->opened_port) {
> +		for (i = 0; i < ARRAY_SIZE(port0->read_urbs); ++i)
> +			usb_kill_urb(port0->read_urbs[i]);
> +	}
> +
> +	mutex_unlock(&serial_priv->urb_mutex);
> +}
> +
> +static int f81534_get_serial_info(struct usb_serial_port *port,
> +				  struct serial_struct __user *retinfo)
> +{
> +	struct f81534_port_private *port_priv;
> +	struct serial_struct tmp;
> +
> +	port_priv = usb_get_serial_port_data(port);
> +
> +	if (!retinfo)
> +		return -EFAULT;

NULL may actually a valid user-space pointer, so just skip this check.

> +
> +	memset(&tmp, 0, sizeof(tmp));
> +
> +	tmp.type = PORT_16550A;
> +	tmp.port = port->port_number;
> +	tmp.line = port->minor;
> +	tmp.baud_base = F81534_MAX_BAUDRATE;
> +
> +	if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int f81534_ioctl(struct tty_struct *tty, unsigned int cmd,
> +			unsigned long arg)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +
> +	switch (cmd) {
> +	case TIOCGSERIAL:
> +		return f81534_get_serial_info(port,
> +						(struct serial_struct __user *)
> +						arg);

Add a local void __user *uarg variable and cast above instead.

> +	default:
> +		break;
> +	}
> +
> +	return -ENOIOCTLCMD;
> +}
> +
> +static void f81534_process_per_serial_block(struct usb_serial_port *port,
> +		unsigned char *data)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	int phy_port_num = data[0];
> +	size_t i, read_size = 0;
> +	unsigned long flags;
> +	char tty_flag;
> +	int status;
> +	u8 lsr;
> +
> +	if (phy_port_num >= F81534_NUM_PORT) {
> +		dev_err(&port->dev, "%s: phy_port_num: %d larger than: %d\n",
> +				__func__, phy_port_num, F81534_NUM_PORT);
> +		return;
> +	}
> +
> +	/*
> +	 * The block layout is 128 Bytes
> +	 * index 0: port phy idx (e.g., 0,1,2,3),
> +	 * index 1: It's could be
> +	 *			F81534_TOKEN_RECEIVE
> +	 *			F81534_TOKEN_TX_EMPTY
> +	 *			F81534_TOKEN_MSR_CHANGE
> +	 * index 2: serial in size (data+lsr, must be even)
> +	 *			meaningful for F81534_TOKEN_RECEIVE only
> +	 * index 3: current MSR with this device
> +	 * index 4~127: serial in data block (data+lsr, must be even)
> +	 */
> +	switch (data[1]) {
> +	case F81534_TOKEN_TX_EMPTY:
> +		spin_lock_irqsave(&port_priv->tx_empty_lock, flags);
> +		port_priv->is_tx_not_empty = false;
> +		spin_unlock_irqrestore(&port_priv->tx_empty_lock, flags);
> +
> +		/* Try to submit writer */
> +		status = f81534_submit_writer(port, GFP_ATOMIC);
> +		if (status)
> +			dev_err(&port->dev, "%s: submit failed\n", __func__);
> +		return;
> +
> +	case F81534_TOKEN_MSR_CHANGE:
> +		f81534_msr_changed(port, data[3]);
> +		return;
> +
> +	case F81534_TOKEN_RECEIVE:
> +		read_size = data[2];
> +		if (read_size > F81534_MAX_RX_SIZE) {
> +			dev_err(&port->dev,
> +				"%s: phy: %d read_size: %zu larger than: %d\n",
> +				__func__, phy_port_num, read_size,
> +				F81534_NUM_PORT);

You probably want F81534_MAX_RX_SIZE here.

> +			return;
> +		}
> +
> +		break;
> +
> +	default:
> +		dev_warn(&port->dev, "%s: unknown token: %02x\n", __func__,
> +				data[1]);
> +		return;
> +	}
> +
> +	for (i = 4; i < 4 + read_size; i += 2) {
> +		tty_flag = TTY_NORMAL;
> +		lsr = data[i + 1];
> +
> +		if (lsr & UART_LSR_BRK_ERROR_BITS) {
> +			if (lsr & UART_LSR_BI) {
> +				tty_flag = TTY_BREAK;
> +				port->icount.brk++;
> +				usb_serial_handle_break(port);
> +			} else if (lsr & UART_LSR_PE) {
> +				tty_flag = TTY_PARITY;
> +				port->icount.parity++;
> +			} else if (lsr & UART_LSR_FE) {
> +				tty_flag = TTY_FRAME;
> +				port->icount.frame++;
> +			}
> +
> +			if (lsr & UART_LSR_OE) {
> +				port->icount.overrun++;
> +				tty_insert_flip_char(&port->port, 0,
> +						TTY_OVERRUN);
> +			}
> +		}
> +
> +		if (port->port.console && port->sysrq) {
> +			if (usb_serial_handle_sysrq_char(port, data[i]))
> +				continue;
> +		}
> +
> +		tty_insert_flip_char(&port->port, data[i], tty_flag);
> +	}
> +
> +	tty_flip_buffer_push(&port->port);
> +}
> +
> +static void f81534_process_read_urb(struct urb *urb)
> +{
> +	struct f81534_serial_private *serial_priv;
> +	struct usb_serial_port *port;
> +	struct usb_serial *serial;
> +	unsigned char *buf;
> +	int phy_port_num;
> +	int tty_port_num;
> +	size_t i;
> +
> +	if (!urb->actual_length ||
> +		(urb->actual_length % F81534_RECEIVE_BLOCK_SIZE))

Parenthesis not needed. Indent continuation lines at least two tabs
further. I'd add braces for readability reasons.

> +		return;
> +
> +	port = urb->context;
> +	serial = port->serial;
> +	buf = urb->transfer_buffer;
> +	serial_priv = usb_get_serial_data(serial);
> +
> +	for (i = 0; i < urb->actual_length; i += F81534_RECEIVE_BLOCK_SIZE) {
> +		phy_port_num = buf[i];
> +		if (phy_port_num >= F81534_NUM_PORT) {
> +			dev_err(&port->dev,
> +				"%s: phy_port_num: %d larger than: %d\n",
> +				__func__, phy_port_num, F81534_NUM_PORT);
> +			continue;
> +		}
> +
> +		tty_port_num = serial_priv->tty_idx[phy_port_num];
> +		port = serial->port[tty_port_num];
> +
> +		if (tty_port_initialized(&port->port))
> +			f81534_process_per_serial_block(port, &buf[i]);
> +	}
> +}
> +
> +static void f81534_write_usb_callback(struct urb *urb)
> +{
> +	struct usb_serial_port *port = urb->context;
> +
> +	switch (urb->status) {
> +	case 0:
> +		break;
> +	case -ENOENT:
> +	case -ECONNRESET:
> +	case -ESHUTDOWN:
> +		dev_dbg(&port->dev, "%s - urb stopped: %d\n",
> +				__func__, urb->status);
> +		return;
> +	case -EPIPE:
> +		dev_err(&port->dev, "%s - urb stopped: %d\n",
> +				__func__, urb->status);
> +		return;
> +	default:
> +		dev_dbg(&port->dev, "%s - nonzero urb status: %d\n",
> +				__func__, urb->status);
> +		break;
> +	}
> +}
> +
> +static int f81534_setup_ports(struct usb_serial *serial)
> +{
> +	struct usb_serial_port *port;
> +	u8 port0_out_address;
> +	int buffer_size;
> +	size_t i;
> +	size_t j;
> +
> +	/*
> +	 * In our system architecture, we had 2 or 4 serial ports,
> +	 * but only get 1 set of bulk in/out endpoints.
> +	 *
> +	 * The usb-serial subsystem will generate port 0 data,
> +	 * but port 1/2/3 will not. It's will generate write URB and buffer
> +	 * by following code and use the port0 read URB for read operation.
> +	 */
> +	for (i = 1; i < serial->num_ports; ++i) {
> +		port0_out_address = serial->port[0]->bulk_out_endpointAddress;
> +		buffer_size = serial->port[0]->bulk_out_size;
> +		port = serial->port[i];
> +
> +		if (kfifo_alloc(&port->write_fifo, PAGE_SIZE, GFP_KERNEL))
> +			return -ENOMEM;
> +
> +		port->bulk_out_size = buffer_size;
> +		port->bulk_out_endpointAddress = port0_out_address;
> +
> +		for (j = 0; j < ARRAY_SIZE(port->write_urbs); ++j) {
> +			set_bit(j, &port->write_urbs_free);
> +
> +			port->write_urbs[j] = usb_alloc_urb(0, GFP_KERNEL);
> +			if (!port->write_urbs[j])
> +				return -ENOMEM;
> +
> +			port->bulk_out_buffers[j] = kzalloc(buffer_size,
> +								GFP_KERNEL);
> +			if (!port->bulk_out_buffers[j])
> +				return -ENOMEM;
> +
> +			usb_fill_bulk_urb(port->write_urbs[j], serial->dev,
> +					usb_sndbulkpipe(serial->dev,
> +						port0_out_address),
> +					port->bulk_out_buffers[j], buffer_size,
> +					serial->type->write_bulk_callback,
> +					port);
> +		}
> +
> +		port->write_urb = port->write_urbs[0];
> +		port->bulk_out_buffer = port->bulk_out_buffers[0];
> +	}
> +
> +	return 0;
> +}
> +
> +static int f81534_attach(struct usb_serial *serial)
> +{
> +	uintptr_t setting_idx = (uintptr_t)usb_get_serial_data(serial);

Use u8 for setting_idx.

> +	struct f81534_serial_private *serial_priv;
> +	int status;
> +
> +	serial_priv = devm_kzalloc(&serial->interface->dev,
> +					sizeof(*serial_priv), GFP_KERNEL);
> +	if (!serial_priv)
> +		return -ENOMEM;
> +
> +	usb_set_serial_data(serial, serial_priv);
> +	serial_priv->setting_idx = setting_idx;
> +
> +	mutex_init(&serial_priv->urb_mutex);
> +
> +	status = f81534_setup_ports(serial);
> +	if (status)
> +		return status;
> +
> +	/*
> +	 * The default configuration layout:
> +	 *	byte 0/1/2/3: uart setting
> +	 */
> +	status = f81534_read_data(serial, F81534_DEF_CONF_ADDRESS_START,
> +				F81534_DEF_CONF_SIZE, serial_priv->conf_data);
> +	if (status) {
> +		dev_err(&serial->interface->dev,
> +				"%s: read reserve data failed: %d\n", __func__,
> +				status);
> +		return status;
> +	}

As for calc_num_ports, why read the default config when you know you
have a custom config?

> +
> +	/*
> +	 * If serial_priv->setting_idx == F81534_CUSTOM_NO_CUSTOM_DATA
> +	 * it's mean for no configuration in custom section, so we'll use
> +	 * default config read from F81534_DEF_CONF_ADDRESS_START
> +	 */
> +	if (serial_priv->setting_idx == F81534_CUSTOM_NO_CUSTOM_DATA)
> +		return 0;
> +
> +	/* Only read 8 bytes for mode & GPIO */
> +	status = f81534_read_data(serial, F81534_CUSTOM_ADDRESS_START +
> +					F81534_CONF_OFFSET,
> +					sizeof(serial_priv->conf_data),
> +					serial_priv->conf_data);
> +	if (status) {
> +		dev_err(&serial->interface->dev,
> +				"%s: idx: %d get data failed: %d\n", __func__,
> +				serial_priv->setting_idx, status);
> +		return status;
> +	}
> +
> +	return 0;
> +}
> +
> +static int f81534_port_probe(struct usb_serial_port *port)
> +{
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(port->serial);
> +	struct f81534_port_private *port_priv;
> +	int idx;
> +
> +	port_priv = devm_kzalloc(&port->dev, sizeof(*port_priv), GFP_KERNEL);
> +	if (!port_priv)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&port_priv->msr_lock);
> +	spin_lock_init(&port_priv->tx_empty_lock);
> +	mutex_init(&port_priv->mcr_mutex);
> +
> +	/* Assign logic-to-phy mapping */
> +	port_priv->phy = f81534_logic_to_phy_port(port->serial, port);
> +	if (port_priv->phy < 0 || port_priv->phy >= F81534_NUM_PORT)
> +		return -ENODEV;
> +
> +	/* Assign phy-to-logic mapping */
> +	idx = f81534_phy_to_logic_port(port->serial, port_priv->phy);
> +	serial_priv->tty_idx[port_priv->phy] = idx;
> +	if (idx < 0 || idx >= F81534_NUM_PORT)
> +		return -ENODEV;

The phy_to_logic mapping should be set up in attach (i.e. before any
port could have been opened).

> +
> +	usb_set_serial_port_data(port, port_priv);
> +	dev_dbg(&port->dev, "%s: mapping to phy: %d, tty_idx: %d\n", __func__,
> +			port_priv->phy, idx);
> +
> +	return 0;
> +}
> +
> +static int f81534_tiocmget(struct tty_struct *tty)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	int status;
> +	int r;
> +	u8 msr;
> +	u8 mcr;
> +
> +	/* Read current MSR from device */
> +	status = f81534_getregister(port->serial, port_priv->phy,
> +					F81534_MODEM_STATUS_REG, &msr);
> +	if (status)
> +		return status;
> +
> +	mutex_lock(&port_priv->mcr_mutex);
> +	mcr = port_priv->shadow_mcr;
> +	mutex_unlock(&port_priv->mcr_mutex);
> +
> +	r = (mcr & UART_MCR_DTR ? TIOCM_DTR : 0) |
> +	    (mcr & UART_MCR_RTS ? TIOCM_RTS : 0) |
> +	    (msr & UART_MSR_CTS ? TIOCM_CTS : 0) |
> +	    (msr & UART_MSR_DCD ? TIOCM_CAR : 0) |
> +	    (msr & UART_MSR_RI ? TIOCM_RI : 0) |
> +	    (msr & UART_MSR_DSR ? TIOCM_DSR : 0);
> +
> +	return r;
> +}
> +
> +static int f81534_tiocmset(struct tty_struct *tty,
> +			   unsigned int set, unsigned int clear)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +
> +	return f81534_update_mctrl(port, set, clear);
> +}
> +
> +static void f81534_dtr_rts(struct usb_serial_port *port, int on)
> +{
> +	if (on)
> +		f81534_update_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
> +	else
> +		f81534_update_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
> +}
> +
> +static int f81534_write(struct tty_struct *tty,
> +			struct usb_serial_port *port,
> +			const unsigned char *buf, int count)
> +{
> +	int bytes_out, status;
> +
> +	if (!count)
> +		return 0;
> +
> +	bytes_out = kfifo_in_locked(&port->write_fifo, buf, count,
> +					&port->lock);
> +
> +	status = f81534_submit_writer(port, GFP_ATOMIC);
> +	if (status) {
> +		dev_err(&port->dev, "%s: submit failed\n", __func__);
> +		return status;
> +	}
> +
> +	return bytes_out;
> +}
> +
> +static int f81534_resume(struct usb_serial *serial)
> +{
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(serial);
> +	struct usb_serial_port *port;
> +	int error = 0;
> +	int status;
> +	size_t i;
> +
> +	/*
> +	 * We'll register port 0 bulkin when port had opened, It'll take all
> +	 * port received data, MSR register change and TX_EMPTY information.
> +	 */
> +	mutex_lock(&serial_priv->urb_mutex);
> +
> +	if (serial_priv->opened_port) {
> +		status = f81534_submit_read_urb(serial, GFP_NOIO);
> +		if (status) {
> +			mutex_unlock(&serial_priv->urb_mutex);
> +			return status;
> +		}
> +	}
> +
> +	mutex_unlock(&serial_priv->urb_mutex);
> +
> +	for (i = 0; i < serial->num_ports; i++) {
> +		port = serial->port[i];
> +		if (!tty_port_initialized(&port->port))
> +			continue;
> +
> +		status = f81534_submit_writer(port, GFP_NOIO);
> +		if (status) {
> +			dev_err(&port->dev, "%s: submit failed\n", __func__);
> +			++error;
> +		}
> +	}
> +
> +	return error ? -EIO : 0;

Please avoid using the ternary operator.

> +}
> +
> +static struct usb_serial_driver f81534_device = {
> +	.driver = {
> +		   .owner = THIS_MODULE,
> +		   .name = "f81534",
> +	},
> +	.description =		DRIVER_DESC,
> +	.id_table =		f81534_id_table,
> +	.open =			f81534_open,
> +	.close =		f81534_close,
> +	.write =		f81534_write,
> +	.calc_num_ports =	f81534_calc_num_ports,
> +	.attach =		f81534_attach,
> +	.port_probe =		f81534_port_probe,
> +	.dtr_rts =		f81534_dtr_rts,
> +	.process_read_urb =	f81534_process_read_urb,
> +	.ioctl =		f81534_ioctl,
> +	.tiocmget =		f81534_tiocmget,
> +	.tiocmset =		f81534_tiocmset,
> +	.write_bulk_callback =	f81534_write_usb_callback,
> +	.set_termios =		f81534_set_termios,
> +	.resume =		f81534_resume,
> +};

You should also implement the tx_empty callback in order to make
sure you wait for buffered data to be sent on close.

Since you're relying on the generic implementation of chars_in_buffer
any bytes in flight will not be accounted for (port->tx_bytes), but if
you just return the value of the tx_empty flag you maintain you should
be ok.

> +
> +static struct usb_serial_driver *const serial_drivers[] = {
> +	&f81534_device, NULL
> +};
> +
> +module_usb_serial_driver(serial_drivers, f81534_id_table);
> +
> +MODULE_DEVICE_TABLE(usb, f81534_id_table);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_AUTHOR("Peter Hong <Peter_Hong@xxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Tom Tsai <Tom_Tsai@xxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");

Johan