Re: [PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver
Hi Johan, Johan Hovold 於 2016/8/23 下午 05:50 寫道: On Tue, Aug 23, 2016 at 04:23:44PM +0800, Ji-Ze Hong (Peter Hong) wrote: Hi Johan, Johan Hovold 於 2016/8/22 下午 09:14 寫道: I'd say it's not worth trying to avoid that extra allocation, and there will be several further allocations done in the usb_control_msg path anyway. What you have today (i.e. in v9) is fine. Ok, I'll keep set/get register the same with V9. + tty_port_num = f81534_phy_to_logic_port(serial, phy_port_num); + port = serial->port[tty_port_num]; + + /* +* The device will send back all information when we submitted +* a read URB (MSR/DATA/TX_EMPTY). But it maybe get callback +* before port_probe() or after port_remove(). +* +* So we'll verify the pointer. If the pointer is NULL, it's +* mean the port not init complete and the block will skip. +*/ + port_priv = usb_get_serial_port_data(port); Check if the port has been opened here instead, no need to store MSR for an unused port above. It's useless for MSR & Receive data when port is closed, but we need the URB to receive TX empty flag. We may not received TX empty flag if we don't process when port is closed. It'll make the port not workable. But you explicitly clear the xmit fifo on open it seems? The F81532/534 contains 2 blocks of H/W designs. One is a 16550A compatible UART with 128 bytes FIFO, and another is a USB bridge with DMA to access UART TX/RX FIFO and handle USB protocols. The clear FIFO in f81534_open() is just clean UART TX/RX FIFO, not USB bridge's RAM. So we must keep a read URB for get newest information via USB bridge likes TX empty. I'll try again to re-write the section as you mention, submit on first open(), kill on last close() and test for some times. If had no other issues, I'll apply to next patch, otherwise I'll preserve old method. Thanks for your help. -- With Best Regards, Peter Hong
Re: [PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver
On Tue, Aug 23, 2016 at 04:23:44PM +0800, Ji-Ze Hong (Peter Hong) wrote: > Hi Johan, > > Johan Hovold 於 2016/8/22 下午 09:14 寫道: > >> +{ > >> + 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) > >> + break; > >> + > >> + if (status == 0) > >> + status = -EIO; > >> + } > >> + > >> + if (status < 0) { > >> + dev_err(&dev->dev, "%s: reg: %x data: %x failed: %d\n", > >> + __func__, reg, data, status); > >> + kfree(tmp); > >> + return status; > > > > I'd use a common exit path to free tmp, and just print an error here. > > I'll change this with next patch. BTW, Alan had suggested me to re-write > set/get register function to avoid kmalloc(), but I found some issue > to re-write. > > We need to read the internal storage to determinate the port counts in > f81534_calc_num_ports(), but in this moment the usb_serial had no > private data, it still need to use kmalloc() to get memory. > > The following source code is my current modification. I'll kmalloc > the buffer if it can't get serial_private, otherwise I'll use > serial_private buffer and protected by a mutex. Should I do something > to improve it? I'd say it's not worth trying to avoid that extra allocation, and there will be several further allocations done in the usb_control_msg path anyway. What you have today (i.e. in v9) is fine. > >> +static int f81534_calc_num_ports(struct usb_serial *serial) > >> +{ > >> + unsigned char setting[F81534_CUSTOM_DATA_SIZE]; > >> + uintptr_t setting_idx; > >> + 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->dev->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); > >> + if (status) { > >> + dev_err(&serial->dev->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 > >> + */ > >> + 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->dev->dev, > >> + "%s: get custom data failed: %d\n", > >> + __func__, status); > >> + return 0; > >> + } > >> + > >> + dev_dbg(&serial->dev->dev, > >> + "%s: read configure from block: %d\n", > >> + __func__, (unsigned int)setting_idx); > >> + } else { > >> + dev_dbg(&serial->dev->dev, "%s: read configure default\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; > > > > Looks like setting[] could be uninitialised here. > > Our IC will preserve 2 section for configuration data. One is > F81534_DEF_CONF_ADDRESS_START, another is F81534_CUSTOM_ADDRESS_START. > > We'll read F81534_DEF_CONF_ADDRESS_START first for default value and > read F81534_CUSTOM_ADDRESS_START for customer value. My bad, I missed the first read above, sorry. > >> + tty_port_num = f81534_phy_to_logic_port(serial, phy_port_num); > >> + port = serial->port[tty_port_num]; > >> + > >> + /* > >> + * The device will send back a
Re: [PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver
Hi Johan, Johan Hovold 於 2016/8/22 下午 09:14 寫道: +static const struct reg_value f81534_pin_control[4][3] = { + /* M0_SDM1 M2 */ + {{0x2ae8, 7}, {0x2a90, 5}, {0x2a90, 4}, }, /* port 0 pins */ + {{0x2ae8, 6}, {0x2ae8, 0}, {0x2ae8, 3}, }, /* port 1 pins */ + {{0x2a90, 0}, {0x2ae8, 2}, {0x2a80, 6}, }, /* port 2 pins */ + {{0x2a90, 3}, {0x2a90, 2}, {0x2a90, 1}, }, /* port 3 pins */ +}; I thought we agreed to drop the transceiver configuration from the driver in favour of a user-space tool? OK, I'll remove all transceiver setting next version. +{ + 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) + break; + + if (status == 0) + status = -EIO; + } + + if (status < 0) { + dev_err(&dev->dev, "%s: reg: %x data: %x failed: %d\n", + __func__, reg, data, status); + kfree(tmp); + return status; I'd use a common exit path to free tmp, and just print an error here. I'll change this with next patch. BTW, Alan had suggested me to re-write set/get register function to avoid kmalloc(), but I found some issue to re-write. We need to read the internal storage to determinate the port counts in f81534_calc_num_ports(), but in this moment the usb_serial had no private data, it still need to use kmalloc() to get memory. The following source code is my current modification. I'll kmalloc the buffer if it can't get serial_private, otherwise I'll use serial_private buffer and protected by a mutex. Should I do something to improve it? static int f81534_set_normal_register(struct usb_serial *serial, u16 reg, u8 data) { struct f81534_serial_private *serial_priv = usb_get_serial_data(serial); struct usb_interface *interface = serial->interface; struct usb_device *dev = serial->dev; size_t count = F81534_USB_MAX_RETRY; int status; u8 *tmp; /* * The f81534_set_normal_register can be called by * f81534_calc_num_ports(). It'll run before allocate private data of * usb_serial. So we should treat as a special case. */ if (serial_priv) { mutex_lock(&serial_priv->register_mutex); tmp = &serial_priv->reg_value; } else { 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); } if (serial_priv) mutex_unlock(&serial_priv->register_mutex); else kfree(tmp); return status; } +static int f81534_command_delay(struct usb_serial *usbserial) Please explain why and when you need to use this "delay" function, and how the BUS_REG_STATUS register works. Please use "serial" consistently throughout for usb_serial pointers (instead of "usbserial"). OK, I'll add comment above f81534_command_delay() and change all usbserial to serial. +static int f81534_calc_num_ports(struct usb_serial *serial) +{ + unsigned char setting[F81534_CUSTOM_DATA_SIZE]; + uintptr_t setting_idx; + u8 num_port = 0; + int status; + size_t i; + + /* Check had custom setting */ + status = f81534_find_config_idx(serial,
Re: [PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver
On Tue, May 31, 2016 at 09:51:20AM +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) > --- Sorry about the late review, let's pick this up again. > drivers/usb/serial/Kconfig | 10 + > drivers/usb/serial/Makefile |1 + > drivers/usb/serial/f81534.c | 1528 > +++ > 3 files changed, 1539 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. > + > + 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 000..c1cb52d > --- /dev/null > +++ b/drivers/usb/serial/f81534.c > @@ -0,0 +1,1528 @@ > +/* > + * F81532/F81534 USB to Serial Ports Bridge > + * > + * F81532 => 2 Serial Ports > + * F81534 => 4 Serial Ports > + * > + * Copyright (C) 2016 Tom Tsai (tom_t...@fintek.com.tw) > + *2016 Peter Hong (peter_h...@fintek.com.tw) Is this copyrighted by Fintek too? You should add a license note to match the MODULE_LICENSE here 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 > +#include > +#include > +#include > +#include > +#include > +#include As 0-day testing reported, you need to include linux/uaccess.h as well. > +/* Serial Port register Address */ > +#define SERIAL_BASE_ADDRESS 0x1200 > +#define DIVISOR_LATCH_LSB(0x00 + SERIAL_BASE_ADDRESS) > +#define DIVISOR_LATCH_MSB(0x01 + SERIAL_BASE_ADDRESS) > +#define FIFO_CONTROL_REGISTER(0x02 + SERIAL_BASE_ADDRESS) > +#define LINE_CONTROL_REGISTER(0x03 + SERIAL_BASE_ADDRESS) > +#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS) > +#define MODEM_STATUS_REGISTER(0x06 + SERIAL_BASE_ADDRESS) > +#define CONFIG1_REGISTER (0x09 + SERIAL_BASE_ADDRESS) Please add F81534_ prefixes to these, and consider using the shorter _REG as suffix throughout. > +#define F81534_DEF_CONF_ADDRESS_START0x3000 > +#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_TOKEN0xf0 > +#define F81534_CONF_OFFSET 1 > + > +#define F81534_MAX_DATA_BLOCK64 > +#define F81534_MAX_BUS_RETRY 2000 > + > +/* 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_DELAY_READ_MSR10 > + > +#define F81534_NUM_PORT 4 > +#define F81534_UNUSED_PORT 0xff > +#de
[PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver
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) --- drivers/usb/serial/Kconfig | 10 + drivers/usb/serial/Makefile |1 + drivers/usb/serial/f81534.c | 1528 +++ 3 files changed, 1539 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. + + 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 000..c1cb52d --- /dev/null +++ b/drivers/usb/serial/f81534.c @@ -0,0 +1,1528 @@ +/* + * F81532/F81534 USB to Serial Ports Bridge + * + * F81532 => 2 Serial Ports + * F81534 => 4 Serial Ports + * + * Copyright (C) 2016 Tom Tsai (tom_t...@fintek.com.tw) + * 2016 Peter Hong (peter_h...@fintek.com.tw) + * + * 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 +#include +#include +#include +#include +#include +#include + +/* Serial Port register Address */ +#define SERIAL_BASE_ADDRESS0x1200 +#define DIVISOR_LATCH_LSB (0x00 + SERIAL_BASE_ADDRESS) +#define DIVISOR_LATCH_MSB (0x01 + SERIAL_BASE_ADDRESS) +#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS) +#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS) +#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS) +#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS) +#define CONFIG1_REGISTER (0x09 + SERIAL_BASE_ADDRESS) + +#define F81534_DEF_CONF_ADDRESS_START 0x3000 +#define F81534_DEF_CONF_SIZE 8 + +#define F81534_CUSTOM_ADDRESS_START0x2f00 +#define F81534_CUSTOM_DATA_SIZE0x10 +#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 + +/* Default URB timeout for USB operations */ +#define F81534_USB_MAX_RETRY 10 +#define F81534_USB_TIMEOUT 1000 +#define F81534_SET_GET_REGISTER0xA0 +#define F81534_DELAY_READ_MSR 10 + +#define F81534_NUM_PORT4 +#define F81534_UNUSED_PORT 0xff +#define F81534_WRITE_BUFFER_SIZE 512 + +#define IC_NAME"f81534" +#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 +#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_CHANGE0x04 + +#define F81534_BUS_BUSY0x03 +#def