Skip to contentSkip to conversation

usb:serial add Fintek F81532/534 driver

PATCH Superseded Peter Hung 2 participants 5 messages Jul 21, 2015 – Oct 27, 2015

Subsystems (2)

v5 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Peter Hung

author
#0 lore raw

PATCH v5 1/1 usb:serial add Fintek F81532/534 driver

This driver is for Fintek F81532/F81534 USB to Serial Ports IC.

Features:
1. F81534 is 1-to-4 & F81532 is 1-to-2 serial ports IC
2. Support Baudrate from B50 to B1500000 (excluding B1000000).
3. The RTS signal can be transformed their behavior with configuration by default ioctl TIOCGRS485/TIOCSRS485 (for RS232/RS485/RS422 with transceiver)

If the driver setting with SER_RS485_ENABLED, the RTS signal will high with not in TX and low with in TX.

If the driver setting with SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND, the RTS signal will low with not in TX and high with in TX.

4. There are 4x3 output-only ic pins to control transceiver mode. It's can be controlled via gpiolib. We could found the gpio number from /sys/class/tty/ttyUSB[x]/device/gpiochap[yyy] where x is F81532/534 serial port and yyy is gpiochip number.

After we found chip number, we can export 3 gpios(M2/M1/M0) per serial port by
echo yyy > /sys/class/gpio/export
echo yyy+1 > /sys/class/gpio/export
echo yyy+2 > /sys/class/gpio/export

then we can control it with

      echo [M0 value] > /sys/class/gpio/gpio[yyy]/value
      echo [M1 value] > /sys/class/gpio/gpio[yyy+1]/value
      echo [M2 value] > /sys/class/gpio/gpio[yyy+2]/value

which M0/M1/M2 as your desired value, value is only 0 or 1.

When configure complete, It's a must to free all gpio by
echo yyy > /sys/class/gpio/unexport
echo yyy+1 > /sys/class/gpio/unexport
echo yyy+2 > /sys/class/gpio/unexport

The driver will "save" gpio configure after we release all gpio of a serial port.

For examples to change mode & gpio with F81532/534 Evalaution Board.

F81532 EVB

      port0: F81437 (RS232 only)
      port1: F81439 (RS232/RS485/RS422 ... etc.)

F81534 EVB
port0/1: F81437 (RS232 only) port2/3: F81439 (RS232/RS485/RS422 ... etc.)

1. RS232 Mode (Default IC Mode)
1. Set struct serial_rs485 flags "without" SER_RS485_ENABLED (control F81532/534 RTS control)
2. Set M2/M1/M0 as 0/0/1
(control F81532/534 output pin to control transceiver mode)

2. RS485 Mode (RTS Low when TX Mode)
1. Set struct serial_rs485 flags with SER_RS485_ENABLED
2. Set M2/M1/M0 as 0/1/0

3. RS485 Mode (RTS High when TX Mode)
1. Set struct serial_rs485 flags with SER_RS485_ENABLED and SER_RS485_RTS_ON_SEND
2. Set M2/M1/M0 as 0/1/1

4. RS422 Mode
1. The RTS mode is dont care.
2. Set M2/M1/M0 as 0/0/0

Please reference https://bitbucket.org/hpeter/fintek-general/src/
with f81534/tools to get set_gpio.c & set_mode.c. Please use it carefully.

Changelog
v5
1. Change the configure data address F81534_CUSTOM_ADDRESS_START from 0x4000 to 0x2f00 (for MP F/W ver:AA66)
2. Change f81534_port_disable/enable() from H/W mode to S/W mode It'll skip all rx data when port is not opened.
3. Some function modifier add with static (Thanks for Paul Bolle)
4. 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 | 3279 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 3290 insertions(+)
 create mode 100644 drivers/usb/serial/f81534.c
drivers/usb/serial/Kconfig +10 -0
index 56ecb8b..0642864 100644
@@ -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.
+
+	  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
drivers/usb/serial/Makefile +1 -0
index 349d9df..9e43b7b 100644
@@ -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
drivers/usb/serial/f81534.c new file +3279 -0

2,977 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line
1.9.1

Part of the diff is not shown here. View the raw message.

maintainer
#1 lore raw
Quoted 9 lines · Peter Hung

This driver is for Fintek F81532/F81534 USB to Serial Ports IC.

Features:
1. F81534 is 1-to-4 & F81532 is 1-to-2 serial ports IC
2. Support Baudrate from B50 to B1500000 (excluding B1000000).
3. The RTS signal can be transformed their behavior with configuration by default ioctl TIOCGRS485/TIOCSRS485 (for RS232/RS485/RS422 with transceiver)

Quoted 7 lines

If the driver setting with SER_RS485_ENABLED, the RTS signal will high with not in TX and low with in TX.

If the driver setting with SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND, the RTS signal will low with not in TX and high with in TX.

4. There are 4x3 output-only ic pins to control transceiver mode.

Are these pins only used to control the transceiver mode, and not for general purpose IO?

Quoted 14 lines

It's can be controlled via gpiolib. We could found the gpio number from /sys/class/tty/ttyUSB[x]/device/gpiochap[yyy] where x is F81532/534 serial port and yyy is gpiochip number.

After we found chip number, we can export 3 gpios(M2/M1/M0) per serial port by
echo yyy > /sys/class/gpio/export
echo yyy+1 > /sys/class/gpio/export
echo yyy+2 > /sys/class/gpio/export

then we can control it with

      echo [M0 value] > /sys/class/gpio/gpio[yyy]/value
      echo [M1 value] > /sys/class/gpio/gpio[yyy+1]/value
      echo [M2 value] > /sys/class/gpio/gpio[yyy+2]/value
Quoted 6 lines

which M0/M1/M2 as your desired value, value is only 0 or 1.

When configure complete, It's a must to free all gpio by
echo yyy > /sys/class/gpio/unexport
echo yyy+1 > /sys/class/gpio/unexport
echo yyy+2 > /sys/class/gpio/unexport

You don't need to document how to use the gpiolib sysfs interface here. Just mention that the driver exports these gpios.

Quoted 23 lines

The driver will "save" gpio configure after we release all gpio of a serial port.

For examples to change mode & gpio with F81532/534 Evalaution Board.

F81532 EVB

      port0: F81437 (RS232 only)
      port1: F81439 (RS232/RS485/RS422 ... etc.)

F81534 EVB
port0/1: F81437 (RS232 only) port2/3: F81439 (RS232/RS485/RS422 ... etc.)

1. RS232 Mode (Default IC Mode)
1. Set struct serial_rs485 flags "without" SER_RS485_ENABLED (control F81532/534 RTS control)
2. Set M2/M1/M0 as 0/0/1
(control F81532/534 output pin to control transceiver mode)

2. RS485 Mode (RTS Low when TX Mode)
1. Set struct serial_rs485 flags with SER_RS485_ENABLED
2. Set M2/M1/M0 as 0/1/0

Quoted 8 lines

3. RS485 Mode (RTS High when TX Mode)
1. Set struct serial_rs485 flags with SER_RS485_ENABLED and SER_RS485_RTS_ON_SEND
2. Set M2/M1/M0 as 0/1/1

4. RS422 Mode
1. The RTS mode is dont care.
2. Set M2/M1/M0 as 0/0/0

I don't think all gpios should be exported for these ports if they have special functions that the driver could control transparently (e.g. for SER_RS485_RTS_ON_SEND).

We need to come up with a way to deal with rs422 mode though.

Quoted 35 lines

Please reference https://bitbucket.org/hpeter/fintek-general/src/
with f81534/tools to get set_gpio.c & set_mode.c. Please use it carefully.

Changelog
v5
1. Change the configure data address F81534_CUSTOM_ADDRESS_START from 0x4000 to 0x2f00 (for MP F/W ver:AA66)
2. Change f81534_port_disable/enable() from H/W mode to S/W mode It'll skip all rx data when port is not opened.
3. Some function modifier add with static (Thanks for Paul Bolle)
4. 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.

Quoted 4 lines

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

It's very nice to have this changelog but please put it below the cutoff line below as it does not need to go into the commit message itself once applied.

Quoted 2 lines

---

That is, put the change log here.

Quoted 46 lines
 drivers/usb/serial/Kconfig  |   10 +
 drivers/usb/serial/Makefile |    1 +
 drivers/usb/serial/f81534.c | 3279 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 3290 insertions(+)
 create mode 100644 drivers/usb/serial/f81534.c
drivers/usb/serial/Kconfig +10 -0
index 56ecb8b..0642864 100644
@@ -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.
+
+	  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
drivers/usb/serial/Makefile +1 -0
index 349d9df..9e43b7b 100644
@@ -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
drivers/usb/serial/f81534.c new file +1 -0
new file mode 100644
index 0000000..ee4b1f8
@@ -0,0 +1,3279 @@
+/*

drivers/usb/serial/f81534.c line 11

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+ * F81532/F81534 USB to Serial Ports Bridge
+ *
+ * F81532 => 2 Serial Ports
+ * F81534 => 4 Serial Ports
+ *
+ * Copyright (C) 2014 Tom Tsai ([email protected])
+ *
+ * The F81532/F81534 had 1 control endpoint for setting,
+ * 1 endport bulk-out for all serial port write out and
+ * 1 endport bulk-in for all serial port read in.

s/endport/endpoint/

Please run this this patch through a spell-checker, there seems to be quite a few typos.

drivers/usb/serial/f81534.c line 23

Quoted 12 lines
drivers/usb/serial/f81534.c new file +12 -0
+ *
+ * write urb is fixed with 512bytes, per serial port used 128Bytes.
+ * is can be described by f81534_prepare_write_buffer()
+ *
+ * read urb is 512Bytes max. per serial port used 128Bytes.
+ * is can be described by f81534_process_read_urb(), it' maybe
+ * received with 128x1,2,3,4 bytes.
+ *
+ * We can control M0/M1/M2 per ports by gpiolib, it's recommend
+ * sequence to request 3 pins, change value and free 3 pins. We'll
+ * save it when M0/M1/M2 pin all released this port to reduce write
+ * operations.

What do you mean by this?

Quoted 64 lines
drivers/usb/serial/f81534.c new file +64 -0
+ *
+ *
+ * Features:
+ * 1. F81534 is 1-to-4 & F81532 is 1-to-2 serial ports IC
+ * 2. Support Baudrate from B50 to B1500000 (excluding B1000000).
+ * 3. The RTS signal can be transformed their behavior with
+ *    configuration by default ioctl TIOCGRS485/TIOCSRS485
+ *    (for RS232/RS485/RS422 with transceiver)
+ *
+ *    If the driver setting with SER_RS485_ENABLED, the RTS signal will
+ *    high with not in TX and low with in TX.
+ *
+ *    If the driver setting with SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
+ *    the RTS signal will low with not in TX and high with in TX.
+ *
+ * 4. There are 4x3 output-only ic pins to control transceiver mode.
+ *    It's can be controlled via gpiolib. We could found the gpio
+ *    number from /sys/class/tty/ttyUSB[x]/device/gpiochap[yyy] where
+ *    x is F81532/534 serial port and yyy is gpiochip number.
+ *
+ *    After we found chip number, we can export 3 gpios(M2/M1/M0) per
+ *    serial port by
+ *       echo yyy > /sys/class/gpio/export
+ *       echo yyy+1 > /sys/class/gpio/export
+ *       echo yyy+2 > /sys/class/gpio/export
+ *
+ *    then we can control it with
+ *       echo [M0 value] > /sys/class/gpio/gpio[yyy]/value
+ *       echo [M1 value] > /sys/class/gpio/gpio[yyy+1]/value
+ *       echo [M2 value] > /sys/class/gpio/gpio[yyy+2]/value
+ *    which M0/M1/M2 as your desired value, value is only 0 or 1.
+ *
+ *    When configure complete, It's a must to free all gpio by
+ *       echo yyy > /sys/class/gpio/unexport
+ *       echo yyy+1 > /sys/class/gpio/unexport
+ *       echo yyy+2 > /sys/class/gpio/unexport
+ *
+ *    The driver will "save" gpio configure after we release
+ *    all gpio of a serial port.
+ *
+ *    For examples to change mode & gpio with F81532/534
+ *    Evalaution Board.
+ *
+ *    F81532 EVB
+ *       port0: F81437 (RS232 only)
+ *       port1: F81439 (RS232/RS485/RS422 ... etc.)
+ *    F81534 EVB
+ *       port0/1: F81437 (RS232 only)
+ *       port2/3: F81439 (RS232/RS485/RS422 ... etc.)
+ *
+ *       1. RS232 Mode (Default IC Mode)
+ *          1. Set struct serial_rs485 flags "without" SER_RS485_ENABLED
+ *             (control F81532/534 RTS control)
+ *          2. Set M2/M1/M0 as 0/0/1
+ *             (control F81532/534 output pin to control transceiver mode)
+ *
+ *       2. RS485 Mode (RTS Low when TX Mode)
+ *          1. Set struct serial_rs485 flags with SER_RS485_ENABLED
+ *          2. Set M2/M1/M0 as 0/1/0
+ *
+ *       3. RS485 Mode (RTS High when TX Mode)
+ *          1. Set struct serial_rs485 flags with SER_RS485_ENABLED and
+ *             SER_RS485_RTS_ON_SEND
+ *          2. Set M2/M1/M0 as 0/1/1

drivers/usb/serial/f81534.c line 97

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+ *
+ *       4. RS422 Mode
+ *          1. The RTS mode is dont care.
+ *          2. Set M2/M1/M0 as 0/0/0
+ *
+ *    Please reference https://bitbucket.org/hpeter/fintek-general/src/
+ *    with f81534/tools to get set_gpio.c & set_mode.c. Please use it
+ *    carefully.
+ */
+#include <asm/unaligned.h>

You don't need this include it seems.

Quoted 5 lines
drivers/usb/serial/f81534.c new file +5 -0
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_driver.h>

drivers/usb/serial/f81534.c line 111

Quoted 10 lines
drivers/usb/serial/f81534.c new file +9 -0
+#include <linux/tty_flip.h>
+#include <linux/module.h>
+#include <linux/serial.h>
+#include <linux/usb.h>
+#include <linux/usb/serial.h>
+#include <linux/serial_reg.h>
+#include <linux/uaccess.h>
 
+#include <linux/version.h>
+#include <linux/workqueue.h>

You don't need the above two includes.

drivers/usb/serial/f81534.c line 113

Quoted 2 lines
drivers/usb/serial/f81534.c new file +2 -0
+#include <linux/kfifo.h>
+#include <linux/fs.h>

Why do you include fs.h?

Quoted 13 lines
drivers/usb/serial/f81534.c new file +13 -0
+#include <linux/mutex.h>
+#include <linux/gpio.h>
+
+#define F81534_TIOCGET_RETRY	5
+
+/* Serial Port register Address */
+#define SERIAL_BASE_ADDRESS		0x1200
+#define RECEIVE_BUFFER_REGISTER		(0x00 + SERIAL_BASE_ADDRESS)
+#define TRANSMIT_HOLDING_REGISTER	(0x00 + SERIAL_BASE_ADDRESS)
+#define INTERRUPT_ENABLE_REGISTER	(0x01 + SERIAL_BASE_ADDRESS)
+#define INTERRUPT_IDENT_REGISTER	(0x02 + SERIAL_BASE_ADDRESS)
+#define FIFO_CONTROL_REGISTER		(0x02 + SERIAL_BASE_ADDRESS)
+#define LINE_CONTROL_REGISTER		(0x03 + SERIAL_BASE_ADDRESS)

drivers/usb/serial/f81534.c line 136

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+#define MODEM_CONTROL_REGISTER		(0x04 + SERIAL_BASE_ADDRESS)
+#define LINE_STATUS_REGISTER		(0x05 + SERIAL_BASE_ADDRESS)
+#define MODEM_STATUS_REGISTER		(0x06 + SERIAL_BASE_ADDRESS)
+#define CLK_SEL_REGISTER		(0x08 + SERIAL_BASE_ADDRESS)
+#define CONFIG1_REGISTER		(0x09 + SERIAL_BASE_ADDRESS)
+#define SADDRESS_REGISTER		(0x0a + SERIAL_BASE_ADDRESS)
+#define SADEN_REGISTER			(0x0b + SERIAL_BASE_ADDRESS)
+#define DIVISOR_LATCH_LSB		(0x00 + SERIAL_BASE_ADDRESS)
+#define DIVISOR_LATCH_MSB		(0x01 + SERIAL_BASE_ADDRESS)
+#define SCRATCH_PAD_REGISTER		(0x07 + SERIAL_BASE_ADDRESS)

How about keeping these sorted by offset.

Quoted 19 lines
drivers/usb/serial/f81534.c new file +19 -0
+#define MSR_ACCESS_SHADOW_REG	0x5a00
+
+#define F81534_RESERVE_ADDRESS_START	0x3000
+#define F81534_RESERVE_SIZE		8
+
+#define F81534_CUSTOM_ADDRESS_START	0x2f00
+#define F81534_CUSTOM_TOTAL_SIZE	0x10
+#define F81534_CUSTOM_DATA_SIZE	0x10
+#define F81534_CUSTOM_MAX_IDX \
+		(F81534_CUSTOM_TOTAL_SIZE/F81534_CUSTOM_DATA_SIZE)
+#define F81534_CUSTOM_NO_CUSTOM_DATA	(-1)
+#define F81534_CUSTOM_VALID_TOKEN 0xf0
+#define F81534_CONF_OFFSET	1
+#define F81534_CONF_SIZE	4
+
+#define F81534_MAX_DATA_BLOCK		64
+#define F81534_MAX_BUS_RETRY		2000
+
+/* default urb timeout for usb operations */

drivers/usb/serial/f81534.c line 165

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+#define F81534_USB_MAX_RETRY	5
+#define F81534_USB_TIMEOUT	1000
+#define F81534_CONTROL_BYTE	0x1B
+#define F81534_SET_GET_REGISTER	0xA0
+
+#define F81534_NUM_PORT	4
+#define F81534_UNUSED_PORT	0xff
+
+#define F81534_WRITE_BUFFER_SIZE	(512L)	/* size of write buffer */
+#define F81534_READ_BUFFER_SIZE		(512L)

No need for L here.

The read buffer size is never used?

drivers/usb/serial/f81534.c line 172

Quoted 7 lines
drivers/usb/serial/f81534.c new file +7 -0
+#define IC_NAME			"f81534"
+#define DRIVER_DESC \
+	"Fintek USB to Serial Ports Driver (F81532/F81534-Evaluation Board)"
+#define FINTEK_VENDOR_ID	0x1934
+#define FINTEK_DEVICE_ID	0x1202	/* RS232 four port */
+#define F81534_MAX_TX_SIZE	124L
+#define F81534_FIFO_SIZE	128L

Drop the L.

Quoted 24 lines
drivers/usb/serial/f81534.c new file +24 -0
+#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
+
+#define F81534_BUS_BUSY	0x03
+#define F81534_BUS_IDLE	0x04
+#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_CMD_ENABLE_WR 0x06
+#define F81534_CMD_PROGRAM 0x02
+#define F81534_CMD_ERASE 0x20
+#define F81534_CMD_READ_STATUS 0x05
+
+#define F81534_MEDIA_BUSY_STATUS	0x03
+
+#define F81534_1X_RXTRIGGER 0xc3
+#define F81534_8X_RXTRIGGER 0xcf

drivers/usb/serial/f81534.c line 206

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+
+#define F81534_DEFAULT_BAUD_RATE	9600
+#define F81534_MAX_BAUDRATE	1500000
+
+#define F81534_RS232_FLAG	0x00
+#define F81534_RS485_FLAG	0x03
+#define F81534_RS485_1_FLAG	0x01
+#define F81534_MODE_MASK	0x03
+
+static int m_f81534_max_tx_size = F81534_MAX_TX_SIZE;

This should not be static, either use the define directly or put it in the private data.

drivers/usb/serial/f81534.c

Quoted 2 lines
drivers/usb/serial/f81534.c new file +2 -0
+
+enum eUartMode {

Don't use CamelCase.

Quoted 6 lines
drivers/usb/serial/f81534.c new file +6 -0
+	eModeRS422,
+	eModeRS232,
+	eModeRS485,
+	eModeRS485_1,
+	eModeRS422_term,
+	eModeRS232_coexist,

drivers/usb/serial/f81534.c line 224

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	eModeRS485_1_term,
+	eModeShutdown,
+	eModeInvalid,
+};
+
+struct internal_data {
+	unsigned int address;
+	unsigned int size;
+	unsigned char buf[F81534_MAX_DATA_BLOCK];
+};

This struct is never used.

Please go though this driver and remove structures and code that you don't use.

drivers/usb/serial/f81534.c line 236

Quoted 12 lines
drivers/usb/serial/f81534.c new file +12 -0
+
+struct f81534_pin_config_data {
+	enum eUartMode eForceUartMode;
+	enum eUartMode eGPIOMode;
+	int address[9];
+	int offset[9];
+};
+
+struct reg_value {
+	int reg_address;
+	int reg_offset;
+	int reg_bit;

Looks like this should be u16 is some other appropriate size.

Quoted 14 lines
drivers/usb/serial/f81534.c new file +14 -0
+};
+
+struct pin_data {
+	struct reg_value port_mode_1;
+	struct reg_value port_mode_0;
+	struct reg_value port_io;
+};
+
+struct out_pin {
+	struct pin_data m1;
+	struct pin_data m2;
+	struct pin_data sd;
+};
+

drivers/usb/serial/f81534.c line 260

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+struct io_map_value {
+	int product_id;
+	int max_port;
+	enum eUartMode mode;
+	struct out_pin port[MAX_NUM_PORTS + 1];
+};
+
+/* The following magic numbers is F81532/534 output pin
+ * register maps
+ */

Please document this (and the above structures) better.

Preferred comment style for multi-line comments is

	/*
	 * ...
	 */
Quoted 27 lines
drivers/usb/serial/f81534.c new file +27 -0
+static struct io_map_value f81534_rs232_control = {
+	FINTEK_DEVICE_ID, F81534_NUM_PORT, eModeRS232,
+	{
+	 /* please reference f81439 io port */
+	 {
+	  {{0x2ad5, 4, 0}, {0x2ad4, 4, 1}, {0x2a90, 4, 0},},
+	  {{0x2ad5, 5, 0}, {0x2ad4, 5, 1}, {0x2a90, 5, 0},},
+	  {{0x2add, 7, 0}, {0x2adc, 7, 1}, {0x2ae8, 7, 1},},
+	  },
+	 {
+	  {{0x2add, 3, 0}, {0x2adc, 3, 1}, {0x2ae8, 3, 0},},
+	  {{0x2add, 0, 0}, {0x2adc, 0, 1}, {0x2ae8, 0, 0},},
+	  {{0x2add, 6, 0}, {0x2adc, 6, 1}, {0x2ae8, 6, 1},},
+	  },
+	 {
+	  {{0x2ad3, 6, 0}, {0x2ad2, 6, 1}, {0x2a80, 6, 0},},
+	  {{0x2add, 2, 0}, {0x2adc, 2, 1}, {0x2ae8, 2, 0},},
+	  {{0x2ad5, 0, 0}, {0x2ad4, 0, 1}, {0x2a90, 0, 1},},
+	  },
+	 {
+	  {{0x2ad5, 1, 0}, {0x2ad4, 1, 1}, {0x2a90, 1, 0},},
+	  {{0x2ad5, 2, 0}, {0x2ad4, 2, 1}, {0x2a90, 2, 0},},
+	  {{0x2ad5, 3, 0}, {0x2ad4, 3, 1}, {0x2a90, 3, 1},},
+	  },
+	 },
+
+};

<snip>

drivers/usb/serial/f81534.c line 494

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+static struct io_map_value *f81534_mode_control[eModeInvalid] = {
+	&f81534_rs422_control,
+	&f81534_rs232_control,
+	&f81534_rs485_control,
+	&f81534_rs485_1_control,
+	&f81534_rs422_term_control,
+	&f81534_rs232_coexist_control,
+	&f81534_rs485_1_term_control,
+	&f81534_shutdown_control,
+};

Looks like all of the above should be const.

Quoted 14 lines
drivers/usb/serial/f81534.c new file +14 -0
+
+
+static const struct usb_device_id id_table[] = {
+	{USB_DEVICE(FINTEK_VENDOR_ID, FINTEK_DEVICE_ID)},
+	{}			/* Terminating entry */
+};
+
+MODULE_DEVICE_TABLE(usb, id_table);
+
+enum eIC_Type {
+	eIC_F81530,
+	eIC_F81531,
+	eIC_F81532,
+	eIC_F81533,

drivers/usb/serial/f81534.c line 518

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	eIC_F81534,
+};
+
+static const char * const m_ic_name[] = {
+	"F81530",
+	"F81531",
+	"F81532",
+	"F81533",
+	"F81534",
+};

Drop the eXXX and m_xxx notations. Just just a f81534 prefix.

drivers/usb/serial/f81534.c line 526

Quoted 8 lines
drivers/usb/serial/f81534.c new file +8 -0
+
+struct f81534_serial_private {
+	enum eIC_Type ic_type;
+	struct usb_serial *serial;
+	bool phy_port_in_use[F81534_NUM_PORT];
+	spinlock_t write_urb_lock;
+	spinlock_t msr_lock;
+	u8 shadowMSR[F81534_NUM_PORT];

Avoid CamelCase, just call it shadow_msr.

Why aren't these in the port private data?

Quoted 41 lines
drivers/usb/serial/f81534.c new file +41 -0
+	unsigned long msr_time[F81534_NUM_PORT];
+	u8 port_mapping[F81534_NUM_PORT];
+	struct mutex msr_mutex;
+	struct mutex change_mode_mutex;
+	u8 reserve_data[F81534_RESERVE_SIZE];
+	u32 custom_idx;
+}
+
+struct f81534_port_private {
+	u8 shadowMCR;
+	u8 shadowLCR;
+	struct usb_serial_port *port;
+	u32 tx, rx;
+	u32 currentBaudRate;
+	struct f81534_pin_config_data port_pin_data;
+	struct gpio_chip f81534_gpio_chip;
+	atomic_t gpio_active;
+	atomic_t port_active;
+};
+
+static void f81534_update_msr(struct usb_serial_port *port,
+		unsigned char *ch);
+
+static int f81534_setup_urbs(struct usb_serial *serial);
+
+static int f81534_set_normal_register(struct usb_device *dev,
+			u16 reg, u8 data);
+
+static int f81534_get_normal_register(struct usb_device *dev,
+			u16 reg, u8 *data);
+
+static int f81534_getregister(struct usb_device *dev,
+			u8 uart, u16 reg, u8 *data);
+
+static void f81534_dtr_rts(struct usb_serial_port *port, int on);
+
+static int f81534_set_port_mode(struct usb_serial_port *port,
+			enum eUartMode eMode);
+
+static int f81534_save_configure_data(struct usb_serial_port *port);
+

drivers/usb/serial/f81534.c

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+static int f81534_switch_gpio_mode(struct usb_serial_port
+				   *serial_port, int mode);
+
+static void f81534_wakeup_all_port(struct usb_serial *serial);
+
+static int f81534_logic_to_phy_port(struct usb_serial *usbserial, int logic)
+{
+	int index;
+	struct f81534_serial_private *serial_priv =
+	    usb_get_serial_data(usbserial);

Odd line break; indent continuation lines at least two tabs further.

drivers/usb/serial/f81534.c line 586

Quoted 9 lines
drivers/usb/serial/f81534.c new file +9 -0
+
+	for (index = 0; index < F81534_NUM_PORT; ++index, --logic) {
+		if ((serial_priv->port_mapping[index] != F81534_UNUSED_PORT)
+		    && !logic)
+			return serial_priv->port_mapping[index];
+	}
+
+	dev_err(&usbserial->dev->dev, "%s could found mapping: logic: %x\n",
+			__func__, logic);

"could not find"?

Quoted 18 lines
drivers/usb/serial/f81534.c new file +18 -0
+	return F81534_UNUSED_PORT;
+}
+
+static int f81534_phy_to_logic_port(struct usb_serial *usbserial, int phy)
+{
+	int index;
+	struct f81534_serial_private *serial_priv =
+	    usb_get_serial_data(usbserial);
+
+	for (index = 0; index < F81534_NUM_PORT; ++index)
+		if (serial_priv->port_mapping[index] == phy)
+			return index;
+
+	dev_err(&usbserial->dev->dev, "%s could found mapping: phy: %x\n",
+			__func__, phy);
+	return F81534_UNUSED_PORT;
+}
+

drivers/usb/serial/f81534.c line 614

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+static int f81534_port_to_phy_index(struct usb_serial_port *port)
+{
+	return f81534_logic_to_phy_port(port->serial, port->port_number);
+}
+
+static int f81534_port_index(struct usb_serial_port *port)
+{
+	return port->port_number;
+}
+

Please describe in a comment what this function does here.

Quoted 4 lines
drivers/usb/serial/f81534.c new file +4 -0
+static int f81534_command_delay(struct usb_serial *usbserial)
+{
+	unsigned int count = F81534_MAX_BUS_RETRY;
+	unsigned char tmp;

drivers/usb/serial/f81534.c line 628

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	int status;
+	struct usb_device *dev = usbserial->dev;
+
+	do {
+		status = f81534_get_normal_register(dev,
+					F81534_BUS_REG_STATUS, &tmp);
+		if (status) {
+			dev_err(&dev->dev,
+					"%s: failed, %d\n",
+					__func__, __LINE__);

Please spell out what went wrong, for example,

	dev_err(..., "failed to get bus status: %d", status);

and include the error code instead of the line number (fix this throughout).

Quoted 4 lines
drivers/usb/serial/f81534.c new file +4 -0
+			return status;
+		}
+
+		if (tmp & F81534_BUS_BUSY)

drivers/usb/serial/f81534.c line 642

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+			continue;
+
+		if (tmp & F81534_BUS_IDLE)
+			break;
+
+	} while (--count);
+
+	if (!count) {
+		dev_err(&usbserial->dev->dev, "%s: max retry exceed !!!\n",
+				__func__);

There's rarely any need for exclamation marks in error messages.

Quoted 6 lines
drivers/usb/serial/f81534.c new file +6 -0
+		return -EIO;
+	}
+
+	status = f81534_set_normal_register(dev, F81534_BUS_REG_STATUS,
+				tmp & ~F81534_BUS_IDLE);
+	if (status) {

drivers/usb/serial/f81534.c line 657

Quoted 10 lines
drivers/usb/serial/f81534.c new file +9 -0
+		dev_err(&dev->dev, "%s: failed, %d\n", __func__, __LINE__);
+		return status;
+	}
+
+	return 0;
+}
+
 
+static int f81534_read_data(struct usb_serial *usbserial, int address,
+		int size, unsigned char *buf)

Use an appropriate size (e.g. u32) for addresses and an unsigned type (e.g. size_t) for size.

Quoted 17 lines
drivers/usb/serial/f81534.c new file +17 -0
+{
+	unsigned int count = 0;
+	unsigned int read_size = 0;
+	unsigned int block = 0;
+	unsigned char *tmp_buf;
+	int status;
+	int offset;
+	struct usb_device *dev = usbserial->dev;
+
+	tmp_buf = kzalloc(F81534_MAX_DATA_BLOCK, GFP_KERNEL);
+	if (!tmp_buf)
+		return -ENOMEM;
+
+	status = f81534_command_delay(usbserial);
+	if (status) {
+		dev_err(&dev->dev, "%s: failed, %d\n",
+				__func__, __LINE__);

drivers/usb/serial/f81534.c line 684

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+		goto error;
+	}
+
+	status = f81534_set_normal_register(dev,
+				F81534_BUS_REG_START, F81534_CMD_READ);
+	if (status) {
+		dev_err(&dev->dev, "%s: failed, %d\n",
+				__func__, __LINE__);
+		goto error;
+	}

If you really need the delay before/after every read then why not do that in a helper function?

Quoted 11 lines
drivers/usb/serial/f81534.c new file +10 -0
+
+	status = f81534_command_delay(usbserial);
+	if (status) {
+		dev_err(&dev->dev, "%s: failed, %d\n",
+				__func__, __LINE__);
+		goto error;
+	}
 
+static int f81534_write_data(struct usb_serial *usbserial, int address,
+			     int size, unsigned char *buf)
+{

drivers/usb/serial/f81534.c line 835

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	unsigned int count = 0;
+	unsigned int write_size = 0;
+	unsigned int block = 0;
+	int offset;
+	int status;
+	struct usb_device *dev = usbserial->dev;
+
+	/* This function maybe cause IC no workable
+	 * Please take this carefully
+	 */

Put this above the function, and expand on what it does (e.g. what is it you're updating etc.).

drivers/usb/serial/f81534.c line 969

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+static int f81534_erase_sector(struct usb_serial *usbserial, int address)
+{
+	u8 current_status = 0;
+	int status;
+	unsigned int count = F81534_MAX_BUS_RETRY;
+	struct usb_device *dev = usbserial->dev;
+
+	/* This function maybe cause IC no workable
+	 * Please take this carefully
+	 */

Same here.

drivers/usb/serial/f81534.c line 1126

Quoted 3 lines
drivers/usb/serial/f81534.c new file +3 -0
+static int f81534_gpio_get(struct gpio_chip *chip, unsigned gpio_num)
+{
+	struct usb_serial_port *port = container_of(

Break before container_of or initialise after declarations.

Quoted 7 lines
drivers/usb/serial/f81534.c new file +7 -0
+		chip->dev, struct usb_serial_port, dev);
+	struct f81534_port_private *port_priv =
+		usb_get_serial_port_data(port);
+	struct f81534_serial_private *serial_priv =
+		usb_get_serial_data(port->serial);
+	int current_mode;
+	int status;

drivers/usb/serial/f81534.c line 1143

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+
+	status = mutex_lock_interruptible(
+				&serial_priv->change_mode_mutex);
+	if (status) {
+		dev_err(&port->dev, "%s: interrupted!\n", __func__);
+		return status;
+	}
+
+	current_mode =
+			port_priv->port_pin_data.eGPIOMode;

Odd line break.

drivers/usb/serial/f81534.c line 1150

Quoted 7 lines
drivers/usb/serial/f81534.c new file +7 -0
+	current_mode &= BIT(gpio_num);
+
+	mutex_unlock(&serial_priv->change_mode_mutex);
+	f81534_wakeup_all_port(port->serial);
+
+	return !!current_mode;
+}

Your gpio implementation looks wrong, but let's get back to that after you explain how these pins are used.

If they are not really general purpose pins, then this isn't the right interface.

Quoted 17 lines
drivers/usb/serial/f81534.c new file +17 -0
+static int f81534_calc_baud_divisor(u32 baudrate, u32 clockrate, u32 *remain)
+{
+	u32 divisor, rem;
+
+	if (!baudrate)
+		return 0;
+
+	divisor = clockrate / baudrate;
+	rem = clockrate % baudrate;
+
+	if (remain)
+		*remain = rem;
+
+	/* Round to nearest divisor */
+	divisor = DIV_ROUND_CLOSEST(clockrate, baudrate);
+
+	return divisor;

drivers/usb/serial/f81534.c line 1304

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+}
+
+static int f81534_get_normal_register(struct usb_device *dev,
+					     u16 reg, u8 *data)
+{
+	int count = F81534_USB_MAX_RETRY;
+	int status;
+	u8 *tmp;
+
+	/* avoid smatch report: error: doing dma on the stack*/

No need for this comment.

Quoted 33 lines
drivers/usb/serial/f81534.c new file +33 -0
+	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) {
+			if (status == 0)
+				status = -EIO;
+		} else {
+			break;
+		}
+	}
+
+	if ((count <= 0) && (status <= 0)) {
+		dev_err(&dev->dev,
+			"%s ERROR reg:%x status:%i failed\n",
+			__func__, reg, status);
+		kfree(tmp);
+		return status;
+	}
+
+	*data = *tmp;

drivers/usb/serial/f81534.c line 1347

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	kfree(tmp);
+	return 0;
+}
+
+static int f81534_set_normal_register(struct usb_device *dev,
+					     u16 reg, u8 data)
+{
+	/* Our device maybe not reply when heavily loading,
+	 * We'll retry for F81534_USB_MAX_RETRY times
+	 */

Move this before the loop.

drivers/usb/serial/f81534.c line 1353

Quoted 6 lines
drivers/usb/serial/f81534.c new file +6 -0
+
+	int count = F81534_USB_MAX_RETRY;
+	int status = 0;
+	u8 *tmp;
+
+	/* avoid smatch report: error: doing dma on the stack*/

Not needed.

Quoted 49 lines
drivers/usb/serial/f81534.c new file +49 -0
+	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	*tmp = data;
+
+	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) {
+			if (status == 0)
+				status = -EIO;
+		} else {
+			break;
+		}
+	}
+
+	if ((count <= 0) && status) {
+		dev_err(&dev->dev,
+			"%s ERROR reg:%x data:0x%x status:%i failed\n",
+			__func__, reg, data, status);
+		kfree(tmp);
+		return status;
+	}
+
+	kfree(tmp);
+	return 0;
+}
+
+static int f81534_setregister(struct usb_device *dev,
+				     u8 uart, u16 reg, u8 data)
+{
+	/* Our device maybe not reply when heavily loading,
+	 * We'll retry for F81534_USB_MAX_RETRY times
+	 */
+
+	int count = F81534_USB_MAX_RETRY;
+	int status;
+	u8 *tmp;
+
+	/* avoid smatch report: error: doing dma on the stack*/
+	tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+

drivers/usb/serial/f81534.c line 1412

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	*tmp = data;
+
+	while (count--) {
+		status = usb_control_msg(dev,
+					 usb_sndctrlpipe(dev, 0),
+					 F81534_SET_GET_REGISTER,
+					 USB_TYPE_VENDOR | USB_DIR_OUT,
+					 reg + uart * 0x10,
+					 0, tmp, sizeof(u8),
+					 F81534_USB_TIMEOUT);

Looks like this one should be implemented using f81534_set_normal_register if the register offset is all that differs.

Quoted 4 lines
drivers/usb/serial/f81534.c new file +4 -0
+static int f81534_set_port_config(struct usb_device *dev,
+					 unsigned char port_number,
+					 struct usb_serial_port *port,
+					 u32 baudrate, u16 lcr)

drivers/usb/serial/f81534.c line 1447

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+{
+	struct usb_serial *serial = port->serial;
+	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
+	u16 device_port = f81534_port_to_phy_index(port);
+	u32 divisor[3] = {0, 0, 0};
+	u32 rem[3] = {0, 0, 0};
+	int status;
+	u8 value;
+	bool is485Mode = false;
+	bool needInvert = false;

Make sure you remove all CamelCase from this driver.

Quoted 14 lines
drivers/usb/serial/f81534.c new file +14 -0
+static int f81534_getregister(struct usb_device *dev,
+				     u8 uart, u16 reg, u8 *data)
+{
+	int count = F81534_USB_MAX_RETRY;
+	int status;
+	u8 *tmp;
+
+	/* avoid smatch report: error: doing dma on the stack*/
+	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

drivers/usb/serial/f81534.c line 1621

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	 */
+
+	while (count--) {
+		status = usb_control_msg(dev,
+					 usb_rcvctrlpipe(dev, 0),
+					 F81534_SET_GET_REGISTER,
+					 USB_TYPE_VENDOR | USB_DIR_IN,
+					 reg + uart * 0x10,
+					 0, tmp, sizeof(u8),
+					 F81534_USB_TIMEOUT);

Should be implemented using f81534_get_normal_register if the offset is all that differs.

Quoted 12 lines
drivers/usb/serial/f81534.c new file +12 -0
+		if (status <= 0) {
+			if (status == 0)
+				status = -EIO;
+		} else {
+			break;
+		}
+	}
+
+	if ((count <= 0) && (status <= 0)) {
+		dev_err(&dev->dev,
+			"%s ERROR port_number:%d reg:%x status:%i failed\n",
+			__func__, uart, reg + uart * 0x10, status);

drivers/usb/serial/f81534.c

Quoted 10 lines
drivers/usb/serial/f81534.c new file +9 -0
+		kfree(tmp);
+		return status;
+	}
+
+	*data = *tmp;
+	kfree(tmp);
+	return 0;
+}
 
+static int f81534_calc_custom_idx(struct usb_serial *serial, u32 *index)

What is this index used for? Please document it.

drivers/usb/serial/f81534.c line 1722

Quoted 11 lines
drivers/usb/serial/f81534.c new file +11 -0
+{
+	int idx, status;
+	u8 custom_data;
+	int offset;
+
+	for (idx = F81534_CUSTOM_MAX_IDX - 1; idx >= 0; --idx) {
+		offset = F81534_CUSTOM_ADDRESS_START +
+				     F81534_CUSTOM_DATA_SIZE * idx;
+		status =
+		    f81534_read_data(serial, offset, 1,
+				&custom_data);

Odd line breaks again.

Quoted 21 lines
drivers/usb/serial/f81534.c new file +21 -0
+		if (status) {
+			dev_err(&serial->dev->dev,
+					"%s: read error, idx:%d, status:%d\n",
+					__func__, idx, status);
+			return status;
+		}
+
+		/* if had custom setting, override
+		 * 1st byte is a indicater, 0xff is empty, 0xf0 is had data
+		 */
+
+		/* found */
+		if (custom_data == F81534_CUSTOM_VALID_TOKEN)
+			break;
+	}
+
+	*index = idx;
+	return 0;
+}
+
+static int f81534_calc_num_ports(struct usb_serial *serial)

drivers/usb/serial/f81534.c line 1753

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+{
+	struct f81534_serial_private *serial_priv = NULL;
+	int index;
+	u8 num_port = 0;
+	int status;
+	unsigned char reserve[F81534_CUSTOM_DATA_SIZE + 1];
+
+	serial_priv = kzalloc(sizeof(*serial_priv), GFP_KERNEL);
+	if (!serial_priv)
+		return 0;

You must allocate the port private data in attach, which is called after this function. Then release it a new release callback instead of at disconnect.

Note that otherwise you may leak this memory if there are errors during probe.

drivers/usb/serial/f81534.c line 1764

Quoted 11 lines
drivers/usb/serial/f81534.c new file +11 -0
+
+	usb_set_serial_data(serial, serial_priv);
+
+	/* check had custom setting */
+	status = f81534_calc_custom_idx(serial, &serial_priv->custom_idx);
+	if (status) {
+		dev_err(&serial->dev->dev,
+				"%s: f81534_calc_custom_idx read failed!!\n",
+				__func__);
+		return 0;
+	}

You could just determine the index (please document it better) again in attach, or possibly use usb_set_serial_data to store it in the private pointer.

Quoted 33 lines
drivers/usb/serial/f81534.c new file +33 -0
+
+	/* read default board setting */
+	status = f81534_read_data(serial, F81534_RESERVE_ADDRESS_START,
+				  F81534_NUM_PORT, reserve);
+	if (status) {
+		dev_err(&serial->dev->dev,
+			"%s: f81534_read_data read failed!!\n", __func__);
+		return 0;
+	}
+
+	/* if had custom setting, override
+	 * 1st byte is a indicater, 0xff is empty, 0xf0f is had data
+	 * skip with 1st data
+	 */
+
+	if (serial_priv->custom_idx != F81534_CUSTOM_NO_CUSTOM_DATA) {
+		status = f81534_read_data(serial,
+					  F81534_CUSTOM_ADDRESS_START +
+					  F81534_CUSTOM_DATA_SIZE *
+					  serial_priv->custom_idx + 1,
+					  sizeof(reserve), reserve);
+		if (status) {
+			dev_err(&serial->dev->dev,
+				"%s: get custom data failed!!\n", __func__);
+			return 0;
+		}
+
+		dev_info(&serial->dev->dev,
+			 "%s: read configure from block:%d\n", __func__,
+			 serial_priv->custom_idx);
+	} else
+		dev_info(&serial->dev->dev, "%s: read configure default\n",
+			 __func__);

drivers/usb/serial/f81534.c line 1807

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+
+	for (index = 0; index < F81534_NUM_PORT; ++index) {
+		switch (reserve[index]) {
+		case 0x37:
+		case 0x38:
+		case 0x39:
+			num_port += 1;
+			break;
+		}
+	}

What's going on here? What are those magic numbers?

Quoted 64 lines
drivers/usb/serial/f81534.c new file +64 -0
+
+	/* old style */
+	if (num_port) {
+		dev_dbg(&serial->dev->dev, "%s: old style wtih %d ports",
+			__func__, num_port);
+		return num_port;
+	}
+
+	/*new style, find all possible ports */
+	num_port = 0;
+	for (index = 0; index < F81534_NUM_PORT; ++index) {
+		if (reserve[index] & BIT(7))
+			continue;
+
+		num_port += 1;
+	}
+
+	if (num_port)
+		return num_port;
+
+	dev_err(&serial->dev->dev, "Read Failed!!, default 4 ports\n");
+	return 4;		/* nothing found */
+}
+
+static void f81534_set_termios(struct tty_struct *tty,
+			       struct usb_serial_port *port,
+			       struct ktermios *old_termios)
+{
+	struct usb_device *dev = port->serial->dev;
+	struct f81534_port_private *port_priv;
+	u32 baud;
+	u16 new_lcr = 0;
+	int status;
+
+	port_priv = usb_get_serial_port_data(port);
+
+	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;

drivers/usb/serial/f81534.c line 1881

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	default:
+	case CS8:
+		new_lcr |= UART_LCR_WLEN8;
+		break;
+	}
+
+	baud = tty_get_baud_rate(tty);
+
+	if (baud) {
+		if ((baud == 1000000) || (baud > F81534_MAX_BAUDRATE)) {

Why do you treat 1Mbaud differently here?

Quoted 6 lines
drivers/usb/serial/f81534.c new file +6 -0
+			if (old_termios)
+				baud = old_termios->c_ospeed;
+			else
+				baud = F81534_DEFAULT_BAUD_RATE;
+		}
+

drivers/usb/serial/f81534.c line 1897

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+		dev_dbg(&dev->dev, "%s-baud: %d\n", __func__, baud);
+		tty_encode_baud_rate(tty, baud, baud);
+
+		port_priv->currentBaudRate = baud;
+	}
+
+	if (C_CRTSCTS(tty) && baud) {
+		dev_dbg(&dev->dev, "%s: port:%d CRTSCTS\n", __func__,
+			f81534_port_to_phy_index(port));
+		f81534_update_mctrl(port, TIOCM_RTS, 0);

This looks wrong. You never enable hardware flow control AFAICS.

Quoted 19 lines
drivers/usb/serial/f81534.c new file +19 -0
+	}
+
+	port_priv->shadowLCR = new_lcr;
+	status =
+	    f81534_set_port_config(dev, f81534_port_to_phy_index(port), port,
+				   port_priv->currentBaudRate, new_lcr);
+	if (status < 0)
+		dev_err(&port->dev, "%s - f81534_set_port_config failed: %i\n",
+			__func__, status);
+
+}
+
+static int f81534_port_enable(struct usb_serial_port *port)
+{
+	struct f81534_port_private *port_priv =
+		usb_get_serial_port_data(port);
+
+	atomic_inc(&port_priv->port_active);
+	return 0;

drivers/usb/serial/f81534.c line 1926

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+}
+
+static int f81534_port_disable(struct usb_serial_port *port)
+{
+	struct f81534_port_private *port_priv =
+		usb_get_serial_port_data(port);
+
+	atomic_dec(&port_priv->port_active);
+	return 0;
+}

Move these above open and close.

drivers/usb/serial/f81534.c line 1932

Quoted 6 lines
drivers/usb/serial/f81534.c new file +6 -0
+
+static int f81534_prepare_gpio(struct usb_serial_port *port)
+{
+	struct f81534_port_private *port_priv =
+		usb_get_serial_port_data(port);
+	char *name = kmalloc(32, GFP_KERNEL);

Split declaration and initialisation.

drivers/usb/serial/f81534.c line 1943

Quoted 11 lines
drivers/usb/serial/f81534.c new file +11 -0
+	int idx = port->minor;
+	int rc;
+
+	if (!name)
+		return -ENOMEM;
+
+	memcpy(&port_priv->f81534_gpio_chip,
+	       &f81534_gpio_chip_templete,
+	       sizeof(f81534_gpio_chip_templete));
+
+	sprintf(name, "%s-%d", IC_NAME, idx);

Use snprintf

Quoted 12 lines
drivers/usb/serial/f81534.c new file +12 -0
+
+	port_priv->f81534_gpio_chip.label = name;
+	port_priv->f81534_gpio_chip.dev = &port->dev;
+
+	rc = gpiochip_add(&port_priv->f81534_gpio_chip);
+	if (rc) {
+		kfree(name);
+		dev_err(&port->dev, "%s: f81534_prepare_gpio failed:%d\n",
+				__func__, rc);
+		return rc;
+	}
+

drivers/usb/serial/f81534.c line 1965

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	return 0;
+}
+
+static int f81534_release_gpio(struct usb_serial_port *port)
+{
+	struct f81534_port_private *port_priv =
+		usb_get_serial_port_data(port);
+
+	gpiochip_remove(&port_priv->f81534_gpio_chip);
+	kfree(port_priv->f81534_gpio_chip.names);

Looks like you leak memory here as you never release label that you allocated above, but instead you free this NULL-pointer.

Quoted 31 lines
drivers/usb/serial/f81534.c new file +31 -0
+	return 0;
+}
+
+static int f81534_open(struct tty_struct *tty, struct usb_serial_port *port)
+{
+	int status;
+
+	status = f81534_port_enable(port);
+	if (status)
+		return status;
+
+	if (tty)
+		f81534_set_termios(tty, port, &tty->termios);
+
+	return 0;
+}
+
+static void f81534_close(struct usb_serial_port *port)
+{
+	int i;
+	unsigned long flags;
+
+	f81534_port_disable(port);
+
+	/* referenced from usb_serial_generic_close() */
+	for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i)
+		usb_kill_urb(port->write_urbs[i]);
+
+	spin_lock_irqsave(&port->lock, flags);
+	kfifo_reset_out(&port->write_fifo);
+	spin_unlock_irqrestore(&port->lock, flags);

drivers/usb/serial/f81534.c line 2006

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+}
+
+static void f81534_disconnect(struct usb_serial *serial)
+{
+	int i;
+	struct usb_serial_port *port0 = serial->port[0];
+	struct f81534_serial_private *serial_priv =
+		usb_get_serial_data(serial);
+
+	dev_dbg(&serial->dev->dev, "%s\n", __func__);

Please remove all debug statements like this one that only track what functions is being called.

drivers/usb/serial/f81534.c

Quoted 1 line
drivers/usb/serial/f81534.c new file +1 -0
+	kfree(serial_priv);

This should be freed in a .release callback.

Quoted 9 lines
drivers/usb/serial/f81534.c new file +9 -0
+
+	for (i = 0; i < ARRAY_SIZE(port0->read_urbs); ++i)
+		usb_kill_urb(port0->read_urbs[i]);
+}
+
+static int f81534_get_serial_info(struct usb_serial_port *port,
+				  struct serial_struct __user *retinfo)
+{
+	struct serial_struct tmp;

drivers/usb/serial/f81534.c line 2026

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+
+	if (!retinfo)
+		return -EFAULT;
+
+	memset(&tmp, 0, sizeof(tmp));
+
+	tmp.type = PORT_16550A;
+	tmp.port = port->port_number;
+	tmp.line = port->minor;
+	tmp.baud_base = 115200;

115200?

Quoted 35 lines
drivers/usb/serial/f81534.c new file +35 -0
+
+	if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
+		return -EFAULT;
+
+	return 0;
+}
+
+#define READ_AND_SET_NORMAL(dev, register, mask, value) \
+	({	\
+		int status_tmp = 0 ; \
+		do {	\
+			char *err_str = "%s - error: %x, status: %d\n"; \
+			u8 urb_value_tmp = 0;	\
+			status_tmp = f81534_get_normal_register(dev, \
+					register, &urb_value_tmp); \
+			if (status_tmp < 0) {	\
+				dev_err(&dev->dev, \
+						err_str, \
+						__func__,  \
+						register, \
+						status_tmp); \
+				break; \
+			}; \
+			\
+			if ((value) != 0) \
+				urb_value_tmp |= \
+						((1L << mask) & (value)) ; \
+			else \
+				urb_value_tmp &=  \
+						~(1L << mask); \
+			\
+			status_tmp = f81534_set_normal_register(dev, \
+					register, \
+					urb_value_tmp); \
+			\

drivers/usb/serial/f81534.c line 2071

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+			if (status_tmp < 0) {	\
+				dev_err(&dev->dev, \
+						err_str, \
+						__func__, \
+						register, status_tmp); \
+				break; \
+			}; \
+		} while (0); \
+		status_tmp; \
+	})

This should be a proper function.

drivers/usb/serial/f81534.c line 2074

Quoted 3 lines
drivers/usb/serial/f81534.c new file +3 -0
+
+static int f81534_switch_gpio_mode(struct usb_serial_port
+				   *serial_port, int mode)

Odd line break.

Why not pass an enum for mode?

Quoted 4 lines
drivers/usb/serial/f81534.c new file +4 -0
+{
+	int x = f81534_port_to_phy_index(serial_port);
+	int y = 0;
+	int val;

drivers/usb/serial/f81534.c line 2088

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	int status;
+	struct usb_device *dev = serial_port->serial->dev;
+	struct io_map_value *request_mode =
+	    f81534_mode_control[(mode >= eModeInvalid) ? eModeRS232 : mode];
+
+	struct pin_data *m1 = &request_mode->port[x].m1;
+	struct pin_data *m2 = &request_mode->port[x].m2;
+	struct pin_data *sd = &request_mode->port[x].sd;
+
+	struct pin_data *pins[3] = { m1, m2, sd };

Why not use an array for the three pins as you never seem to access them directly (e.g. ->m2)?

Also why are sometimes referring to "m0" and sometimes "sd"?

Quoted 13 lines
drivers/usb/serial/f81534.c new file +13 -0
+
+	if (mode >= 8)
+		return -EINVAL;
+
+	for (y = 0; y < 3; ++y) {
+		val = pins[y]->port_io.reg_bit ? 0xff : 0x00;
+		status = READ_AND_SET_NORMAL(dev,
+					pins[y]->port_io.reg_address,
+					pins[y]->port_io.reg_offset,
+					val);
+		if (status) {
+			dev_err(&serial_port->dev,
+					"%s: failed, %d\n",

drivers/usb/serial/f81534.c line 2111

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+					__func__, __LINE__);
+			return status;
+		}
+	}
+
+	return 0;
+}
+
+
+

Stray newlines.

drivers/usb/serial/f81534.c line 2116

Quoted 5 lines
drivers/usb/serial/f81534.c new file +5 -0
+static int f81534_set_port_mode(struct usb_serial_port *port,
+		enum eUartMode eMode)
+{
+	int status;
+	u8 urb_value;

urb_value?

Quoted 11 lines
drivers/usb/serial/f81534.c new file +11 -0
+	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
+
+	if (eMode > eModeInvalid)
+		return -1;
+
+	if (eMode != eModeInvalid) {
+		int data = 1;
+
+		status = f81534_getregister(port->serial->dev,
+					f81534_port_to_phy_index(port),
+					CLK_SEL_REGISTER,

drivers/usb/serial/f81534.c line 2137

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+					&urb_value);
+		if (status) {
+			dev_err(&port->dev,
+					"%s: failed, %d\n",
+					__func__, __LINE__);
+			return status;
+		}
+
+		urb_value &= ~(data << 4);
+		urb_value &= ~(data << 5);

What is this? Why not clear it in one mask operation?

Quoted 37 lines
drivers/usb/serial/f81534.c new file +37 -0
+static void f81534_process_per_serial_block(struct usb_serial_port *port,
+		unsigned char *data)
+{
+	unsigned char lsr = 0;
+	unsigned char msr, old_msr;
+	char tty_flag;
+	struct tty_struct *tty;
+	struct usb_serial *serial = port->serial;
+	struct f81534_serial_private *serial_priv =
+		usb_get_serial_data(serial);
+	struct f81534_port_private *port_priv =
+		usb_get_serial_port_data(port);
+	int phy_port_num = data[0];
+	int read_size = 0;
+	int i;
+
+	/* 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)
+	 *			meanful for F81534_TOKEN_RECEIVE only
+	 * index 3: current MSR with device read
+	 * index 4~127: serial in data block (data+lsr, must be even)
+	 */
+
+	switch (data[1]) {
+	case F81534_TOKEN_TX_EMPTY:
+		spin_lock(&serial_priv->write_urb_lock);
+		serial_priv->phy_port_in_use[phy_port_num] =
+			false;
+		spin_unlock(&serial_priv->write_urb_lock);
+		usb_serial_port_softint(port);
+		break;
+

drivers/usb/serial/f81534.c line 2348

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	case F81534_TOKEN_MSR_CHANGE:
+		break;
+
+	case F81534_TOKEN_RECEIVE:
+		read_size = data[2];
+		break;
+
+	default:
+		dev_info(&port->dev, "%s: unknown token:%02x\n",
+				__func__, data[1]);

dev_warn

Quoted 35 lines
drivers/usb/serial/f81534.c new file +35 -0
+		return;
+	}
+
+	do {
+		msr = data[3];
+		spin_lock(&serial_priv->msr_lock);
+
+		/* The MSR information can be get with any
+		 * 'correct' block data, but dont directly read MSR
+		 * from 16550A, our MCU will reported newest
+		 * or current data.
+		 *
+		 * We maybe received the same MSR. e.g.,
+		 * received MSR 0x11 and received 0x11 again.
+		 *
+		 * So we'll not use UART_MSR_ANY_DELTA to do check,
+		 * we directly compare their value.
+		 */
+		if (msr ==
+			serial_priv->shadowMSR[phy_port_num]) {
+			spin_unlock(&serial_priv->msr_lock);
+			break;
+		}
+
+		old_msr = serial_priv->shadowMSR[phy_port_num];
+		serial_priv->shadowMSR[phy_port_num] = msr;
+		serial_priv->msr_time[phy_port_num] = jiffies;
+
+		spin_unlock(&serial_priv->msr_lock);
+
+		dev_dbg(&port->dev,
+			"%s: DCD Changed: port %d from %x to %x.\n",
+			__func__, phy_port_num, old_msr,
+			msr);
+

drivers/usb/serial/f81534.c line 2393

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+		f81534_update_msr(port, &msr);
+
+		tty = tty_port_tty_get(&port->port);
+		if (!tty)
+			break;
+
+		usb_serial_handle_dcd_change(port,
+				tty, msr & UART_MSR_DCD);
+		tty_kref_put(tty);
+	} while (0);

Move this to a helper function instead.

drivers/usb/serial/f81534.c line 2400

Quoted 7 lines
drivers/usb/serial/f81534.c new file +7 -0
+
+	if (data[1] != F81534_TOKEN_RECEIVE)
+		return;
+
+	/* if the open not had open, skip this data */
+	if (atomic_read(&port_priv->port_active))
+		return;

You should not process MSR changes for a closed port above either.

drivers/usb/serial/f81534.c line 2402

Quoted 2 lines
drivers/usb/serial/f81534.c new file +2 -0
+
+	for (i = 0; i < read_size; i += 2) {

Start counting at 4 instead.

Quoted 42 lines
drivers/usb/serial/f81534.c new file +42 -0
+		tty_flag = TTY_NORMAL;
+		lsr = data[i + 5];
+
+		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 + 4]))
+				continue;
+		}
+
+		tty_insert_flip_char(&port->port, data[i + 4], tty_flag);
+	}
+
+	tty_flip_buffer_push(&port->port);
+}
+
+static void f81534_process_read_urb(struct urb *urb)
+{
+	int i;
+	int phy_port_num;
+	int tty_port_num;
+	unsigned char *ch;
+	struct usb_serial *serial;
+	struct usb_serial_port *port = NULL;

drivers/usb/serial/f81534.c line 2454

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+
+	if (!urb->actual_length)
+		return;
+
+	port = urb->context;
+	serial = port->serial;
+	ch = urb->transfer_buffer;
+
+	for (i = 0; i < urb->actual_length / F81534_RECEIVE_BLOCK_SIZE;
+			++i) {

Increment using F81534_RECEIVE_BLOCK_SIZE instead and remove the multiplications below.

Quoted 54 lines
drivers/usb/serial/f81534.c new file +54 -0
+		phy_port_num = ch[i * F81534_RECEIVE_BLOCK_SIZE];
+		tty_port_num =
+			f81534_phy_to_logic_port(serial, phy_port_num);
+		port = serial->port[tty_port_num];
+		f81534_process_per_serial_block(port,
+				&ch[i * F81534_RECEIVE_BLOCK_SIZE]);
+	}
+}
+
+static int f81534_setup_urbs(struct usb_serial *serial)
+{
+	int i = 0;
+	u8 port0_out_address;
+	int j;
+	int buffer_size;
+	struct usb_serial_port *port = NULL;
+
+	/* In our system architecture, we had 4 or 2 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
+	 */
+	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))
+			goto failed;
+
+		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])
+				goto failed;
+			port->bulk_out_buffers[j] = kmalloc(buffer_size,
+								GFP_KERNEL);
+			if (!port->bulk_out_buffers[j])
+				goto failed;
+
+			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);

drivers/usb/serial/f81534.c line 2518

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+		}
+
+		port->write_urb = port->write_urbs[0];
+		port->bulk_out_buffer = port->bulk_out_buffers[0];
+	}
+	return 0;
+
+failed:
+	return -ENOMEM;
+}

I understand better now how your firmware work. It looks like this is a bad fit for the generic implementation due to the TX_EMPTY requirement.

You need to implement your own write handling, and there's no need to have more than one write urb per port. You still want the fifo though to cache outgoing data. If you implement write combining you only need urb per device, but I'm not sure if it's worth it.

Quoted 1 line

+static int f81534_save_configure_data(struct usb_serial_port *port)

What does thing function do? And why do you need to call it on every port probe? Please add some comments.

drivers/usb/serial/f81534.c line 2689

Quoted 4 lines
drivers/usb/serial/f81534.c new file +4 -0
+	/* token of data exist */
+	data[0] = F81534_CUSTOM_VALID_TOKEN;
+	memcpy(&data[1], serial_priv->reserve_data,
+			F81534_RESERVE_SIZE);

This line does not need to be broken. There are a lot of such unnecessarily broken lines that only hurt readability. Please go through the file and fix that up.

Quoted 8 lines
drivers/usb/serial/f81534.c new file +8 -0
+static int f81534_load_configure_data(struct usb_serial_port *port)
+{
+	int status;
+	int offset;
+	unsigned char uart_flag, gpio_mode;
+	int device_port = f81534_port_to_phy_index(port);
+	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
+	struct f81534_serial_private *serial_priv =

drivers/usb/serial/f81534.c line 2748

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	    usb_get_serial_data(port->serial);
+
+	uart_flag = serial_priv->reserve_data[device_port];
+	gpio_mode = serial_priv->reserve_data[device_port + F81534_CONF_SIZE];
+
+	switch (uart_flag) {
+	/* old style setting */
+	case 0x37:
+	case 0x38:
+	case 0x39:

No magic constants please.

Quoted 37 lines
drivers/usb/serial/f81534.c new file +37 -0
+static int f81534_attach(struct usb_serial *serial)
+{
+	struct f81534_serial_private *serial_priv =
+		usb_get_serial_data(serial);
+	int status;
+	int i;
+	int offset;
+	int num_port = serial->num_ports;
+
+	serial_priv->serial = serial;
+	memset(serial_priv->port_mapping, F81534_UNUSED_PORT,
+	       sizeof(serial_priv->port_mapping));
+
+	switch (num_port) {
+	case eIC_F81532:
+	case eIC_F81534:
+		serial_priv->ic_type = num_port;
+		dev_info(&serial->dev->dev, "%s detected\n",
+			 m_ic_name[num_port]);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	for (i = 0; i < F81534_NUM_PORT; ++i)
+		serial_priv->port_mapping[i] = i;
+
+	switch (num_port) {
+	case 4:
+		break;
+	case 2:
+		serial_priv->port_mapping[1] = 3;
+		serial_priv->port_mapping[2] = F81534_UNUSED_PORT;
+		serial_priv->port_mapping[3] = F81534_UNUSED_PORT;
+		break;
+	case 1:
+		serial_priv->port_mapping[1] = F81534_UNUSED_PORT;

drivers/usb/serial/f81534.c line 2883

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+		serial_priv->port_mapping[2] = F81534_UNUSED_PORT;
+		serial_priv->port_mapping[3] = F81534_UNUSED_PORT;
+		break;
+	default:
+		dev_err(&serial->dev->dev,
+			"%s: Cant determine ports: %d, error!!!\n", __func__,
+			num_port);
+		status = -EINVAL;
+		goto failed;
+	}

You should not need this port_mapping, but rather have calc_num_ports return the actual number of ports available so that resource are not unnecessarily allocated. You can then store the mapping in the port private data instead.

Quoted 17 lines
drivers/usb/serial/f81534.c new file +17 -0
+	for (i = 0; i < F81534_NUM_PORT; ++i) {
+		/* clear fifo when plug in */
+		status = f81534_setregister(serial->dev,
+				i, FIFO_CONTROL_REGISTER,
+				UART_FCR_R_TRIG_11 |
+				UART_FCR_ENABLE_FIFO |
+				UART_FCR_CLEAR_RCVR |
+				UART_FCR_CLEAR_XMIT);
+		if (status) {
+			dev_err(&serial->dev->dev,
+				"%s FCR init error:%x failed\n", __func__, i);
+			goto failed;
+		}
+
+		/* Get MSR initial value*/
+		status = f81534_get_normal_register(serial->dev,
+					MSR_ACCESS_SHADOW_REG + i,

drivers/usb/serial/f81534.c line 2911

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+					&serial_priv->shadowMSR[i]);
+		if (status) {
+			dev_err(&serial->dev->dev,
+				"%s f81534_get_normal_register:%x failed\n",
+				__func__, MSR_ACCESS_SHADOW_REG + i);
+			goto failed;
+		}
+
+		serial_priv->msr_time[i] = jiffies;
+	}

This should be done per port in open instead.

Quoted 8 lines
drivers/usb/serial/f81534.c new file +8 -0
+	spin_lock_init(&serial_priv->write_urb_lock);
+	spin_lock_init(&serial_priv->msr_lock);
+	mutex_init(&serial_priv->msr_mutex);
+	mutex_init(&serial_priv->change_mode_mutex);
+
+	status = f81534_setup_urbs(serial);
+	if (status != 0)
+		goto failed;

drivers/usb/serial/f81534.c line 2930

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+
+	status = f81534_read_data(serial,
+				F81534_RESERVE_ADDRESS_START,
+				F81534_RESERVE_SIZE,
+				serial_priv->reserve_data);
+	if (status) {
+		dev_err(&serial->dev->dev, "%s read reserve data failed\n",
+			__func__);
+		goto failed;
+	}

Please document this "reserve" data.

drivers/usb/serial/f81534.c line 2938

Quoted 8 lines
drivers/usb/serial/f81534.c new file +8 -0
+
+	/* if had custom setting, override
+	 * 1st byte is a indicater, 0xff is empty, 0xf0 is had data
+	 * skip with 1st data
+	 */
+
+	if (serial_priv->custom_idx == F81534_CUSTOM_NO_CUSTOM_DATA)
+		return 0;

What does this mean?

Quoted 39 lines
drivers/usb/serial/f81534.c new file +38 -0
+
+	offset = F81534_CUSTOM_ADDRESS_START +
+			F81534_CUSTOM_DATA_SIZE * serial_priv->custom_idx + 1;
+	/* only read 8 bytes for mode & GPIO */
+	status = f81534_read_data(serial,
+			offset,
+			sizeof(serial_priv->reserve_data),
+			serial_priv->reserve_data);
+	if (status) {
+		dev_err(&serial->dev->dev,
+			"%s: get custom data failed, idx:%d, status:%d!!\n",
+			__func__, serial_priv->custom_idx, status);
+		goto failed;
+	}
+
+	/* We'll register port 0 bulkin only once, It'll take all port read
+	 * and information.
+	 */
+	status = usb_serial_generic_submit_read_urbs(serial->port[0],
+				GFP_KERNEL);
+	if (status) {
+		dev_err(&serial->dev->dev,
+			"%s: submit read urbs failed!! status:%d!!\n",
+			__func__, status);
+		goto failed;
+	}
+
+	return 0;
+
+failed:
+	kfree(serial_priv);
+	return status;
+}
+
 
+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);

drivers/usb/serial/f81534.c

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	struct f81534_serial_private *serial_priv =
+	    usb_get_serial_data(port->serial);
+	int r = 0;
+	int index = f81534_port_to_phy_index(port);
+	int count = F81534_TIOCGET_RETRY, result;
+	unsigned long current_jiffies = jiffies + 1;
+	unsigned long flags = 0;
+	u8 msr, mcr;
+
+	/* We'll not directly to read MSR/Shadow Register.

Why not?

drivers/usb/serial/f81534.c line 3062

Quoted 7 lines
drivers/usb/serial/f81534.c new file +7 -0
+	 * If MSR has any change, the device will notice by bulk-in
+	 * Please reference f81534_process_read_urb()
+	 *
+	 * This section will wait for 10*F81534_TIOCGET_RETRY msec to
+	 * receive MSR change. We'll use old MSR value if H/W not change
+	 * or not noticed.
+	 */

This seems like a bad idea. Why not simply return the cached value if you can not get the current state? Is it only sent when there's incoming data available?

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+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)
+{

drivers/usb/serial/f81534.c line 3115

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+	/* drop RTS and DTR */
+	if (on)
+		f81534_update_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0);
+	else
+		f81534_update_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
+}
+
+static void f81534_update_msr(struct usb_serial_port *port, unsigned char *ch)
+{
+	u8 newMSR = (u8) *ch;

Cast not needed.

Quoted 29 lines
drivers/usb/serial/f81534.c new file +29 -0
+
+	if (newMSR & UART_MSR_ANY_DELTA) {
+		/* update input line counters */
+		if (newMSR & UART_MSR_DCTS)
+			port->icount.cts++;
+		if (newMSR & UART_MSR_DDSR)
+			port->icount.dsr++;
+		if (newMSR & UART_MSR_DDCD)
+			port->icount.dcd++;
+		if (newMSR & UART_MSR_TERI)
+			port->icount.rng++;
+
+		wake_up_interruptible(&port->port.delta_msr_wait);
+	}
+}
+
+static int f81534_write_room(struct tty_struct *tty)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct f81534_port_private *port_priv;
+	struct f81534_serial_private *serial_priv =
+	    usb_get_serial_data(port->serial);
+	int port_num = f81534_port_to_phy_index(port);
+	unsigned long flags = 0;
+	int r;
+
+	port_priv = usb_get_serial_port_data(port);
+
+	spin_lock_irqsave(&serial_priv->write_urb_lock, flags);

drivers/usb/serial/f81534.c line 3154

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+
+	if (serial_priv->phy_port_in_use[port_num])
+		r = 0;
+	else
+		r = m_f81534_max_tx_size;
+
+	spin_unlock_irqrestore(&serial_priv->write_urb_lock, flags);
+
+	return r;
+}

You should always accept data if there's room in the per port fifo, but then you let TX_EMPTY pace how fast the fifo is emptied in your custom write implementation.

Quoted 57 lines
drivers/usb/serial/f81534.c new file +57 -0
+
+static int f81534_prepare_write_buffer(struct usb_serial_port *port,
+						void *dest, size_t size)
+{
+	unsigned char *ptr = (unsigned char *) dest;
+	int port_num = f81534_port_to_phy_index(port);
+	struct usb_serial *serial = port->serial;
+
+	WARN_ON(size != serial->port[0]->bulk_out_size);
+
+	if (size != F81534_WRITE_BUFFER_SIZE) {
+		WARN_ON(size != F81534_WRITE_BUFFER_SIZE);
+		return 0;
+	}
+
+	/* The block layout is fixed with 4x128 Bytes, per 128 Bytes
+	 * for a port.
+	 * index 0: port phy idx (e.g., 0,1,2,3)
+	 * index 1: only F81534_TOKEN_WRITE
+	 * index 2: serial out size
+	 * index 3: fix to 0
+	 * index 4~127: serial out data block
+	 */
+
+	ptr[0] = 0;
+	ptr[128] = 1;
+	ptr[256] = 2;
+	ptr[384] = 3;
+	ptr[128 * port_num + 0] = port_num;
+	ptr[128 * port_num + 1] = F81534_TOKEN_WRITE;
+	ptr[128 * port_num + 3] = 0;
+	ptr[128 * port_num + 2] =
+		kfifo_out_locked(&port->write_fifo,
+				&ptr[128 * port_num + 4],
+				m_f81534_max_tx_size,
+				&port->lock);
+
+	return F81534_WRITE_BUFFER_SIZE;
+}
+
+static int f81534_write(struct tty_struct *tty,
+			struct usb_serial_port *port,
+			const unsigned char *buf, int count)
+{
+	int bytes_out = 0;
+	unsigned long flags;
+	bool bCouldWrite = true;
+	int port_num = f81534_port_to_phy_index(port);
+	int updating_data;
+	struct f81534_serial_private *serial_priv =
+	    usb_get_serial_data(port->serial);
+
+	if (!count)
+		return 0;
+
+	updating_data = mutex_is_locked(&serial_priv->change_mode_mutex);
+	if (updating_data) /*someone is changing setting */

drivers/usb/serial/f81534.c line 3221

Quoted 10 lines
drivers/usb/serial/f81534.c new file +10 -0
+		return 0;
+
+	/* We suppress tx data max up to m_f81534_max_tx_size(124)
+	 * to fit generic subsystem. Our device's max tx data per port is
+	 * 124 and We should wait for the H/W reply "TX_EMPTY" to
+	 * submit next URB for this port.
+	 *
+	 * If we submit data without wait for H/W report "TX_EMPTY",
+	 * the H/W DMA engine will halt.
+	 */

So as I mentioned above, always accept data if there's room in the fifo. Then kick of a write urb, if TX_EMPTY is set for any of the ports and fill the urb based on those flags as well.

Johan

author
#2 lore raw

Johan Hovold 於 2015/9/14 下午 09:33 寫道:

Quoted 7 lines · Peter Hung
Quoted 2 lines

This driver is for Fintek F81532/F81534 USB to Serial Ports IC.

So as I mentioned above, always accept data if there's room in the fifo. Then kick of a write urb, if TX_EMPTY is set for any of the ports and fill the urb based on those flags as well.

I'll try to improve it with V6, but it'll need more time to do modification & verification. So V6 will release lately.

Thank you for your advice.

Signature · 2 lines
With Best Regards,
Peter Hung
author
#3 lore raw
In reply to Johan Hovold

Hi Johan,

Johan Hovold 於 2015/9/14 下午 09:33 寫道:

Quoted 9 lines · Peter Hung
Quoted 4 lines

4. RS422 Mode
1. The RTS mode is dont care.
2. Set M2/M1/M0 as 0/0/0

I don't think all gpios should be exported for these ports if they have special functions that the driver could control transparently (e.g. for SER_RS485_RTS_ON_SEND).

Surely, we can hide some setting with definitely setting like RS232/RS485, but the settings is only apply to our evaluation board.

Some customers will use our F81532/534 with other brand transceiver IC. The pins setting maybe changed, so we decided to separate UART & pins settings and let the 3 output pins controllable for customer.

Could I preserve currently UART & pins setting mode ?

Quoted 11 lines

drivers/usb/serial/f81534.c line 1150

Quoted 7 lines
drivers/usb/serial/f81534.c new file +7 -0
+	current_mode &= BIT(gpio_num);
+
+	mutex_unlock(&serial_priv->change_mode_mutex);
+	f81534_wakeup_all_port(port->serial);
+
+	return !!current_mode;
+}

Your gpio implementation looks wrong, but let's get back to that after you explain how these pins are used.

Quoted 2 lines

If they are not really general purpose pins, then this isn't the right interface.

The 4x3 pins is output-only mode to control transceiver, so I'm only implement the output functions and let all input functions failed.

Could you give some advices to me if the gpiolib interface cant be used ?

Sorry for late reply, I'm almost ready to submit v6 when clarify some questions.

Thanks,

Signature · 2 lines
With Best Regards,
Peter Hung
maintainer
#4 lore raw
Quoted 13 lines · Peter Hung

Hi Johan,

Johan Hovold 於 2015/9/14 下午 09:33 寫道:

Quoted 9 lines · Peter Hung
Quoted 4 lines

4. RS422 Mode
1. The RTS mode is dont care.
2. Set M2/M1/M0 as 0/0/0

I don't think all gpios should be exported for these ports if they have special functions that the driver could control transparently (e.g. for SER_RS485_RTS_ON_SEND).

Quoted 8 lines

Surely, we can hide some setting with definitely setting like RS232/RS485, but the settings is only apply to our evaluation board.

Some customers will use our F81532/534 with other brand transceiver IC. The pins setting maybe changed, so we decided to separate UART & pins settings and let the 3 output pins controllable for customer.

Could I preserve currently UART & pins setting mode ?

If the pins can be set independently of the mode, and in principle be used as general purpose output-only pins, then you should use the gpiolib interface.

However, in the last patch it did look like the gpio-implementation was intertwined with mode selection, something which made it seem like a bad fit.

Would you be able to separate it, and specifically, make the gpio and mode-handling generic and not specific for your evaluation board?

Thanks,
Johan