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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
