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

From: Peter Hung
Date: Mon Jan 11 2016 - 01:06:16 EST

Hi Johan,

Johan Hovold æ 2016/1/4 äå 02:58 åé:
On Wed, Dec 02, 2015 at 03:18:59PM +0800, Peter Hung wrote:
This driver is for Fintek F81532/F81534 USB to Serial Ports IC.

Do you have a pointer to datasheets and/or information about these
devices? Can't seem to find anything on Fintek's homepage.



I noticed that you have not considered all my comments on earlier
revisions of this driver. Please make sure to address all issues raised.

I'll recheck it again.

We may need to come up with a generic interface for switching tranceiver
modes as there are more drivers that could benefit from this. I think we
should prevent mode switching for a port that is open as does not seem
to make much sense.

How about you drop mode-switching support completely for now (e.g.
gpiolib and rs485-ioctl) and focus on getting the basic functionality
right first?

You could also consider extending you user-space tool and use that to
switch modes (e.g. reprogram the device eeprom) through libudev, at
least until a generic interface is in place.

Ok, we'll remove gpiolib & rs485-ioctl with next version and only remain
the initialization of gpio/mode control on attach() once.

Also we'll provide tools via libusb to modify the configuration data on

As for the basic design, you should use per-interface read and write
urbs. Submit the read urb(s) when the first port is opened, and kill
them when the last device is closed. Similarly for the write urbs,
submit one when there's at least one port with data in its fifo that it
non-busy (TX_EMPTY is set).

The method you mentioned is more make sense, but it'll changes the
driver tx flow and makes it more difficultly to maintain.

Could I preserve current flow? The current is 1 read-urb submits on
attach() & resume() and per-port write-urb submits on write() or
read-urb callback TX_EMPTY. The style is the same with "mxuport.c".

+#define F81534_CUSTOM_ADDRESS_START 0x2f00
+#define F81534_CUSTOM_TOTAL_SIZE 0x10
+#define F81534_CUSTOM_DATA_SIZE 0x10
+#define F81534_CUSTOM_MAX_IDX \

Do you really expect F81534_CUSTOM_MAX_IDX to be 1?

Yes, we'll remove the TOTAL_SIZE and set MAX_IDX = 1 with next version.

+#ifndef C_CMSPAR
+#define C_CMSPAR(tty) _C_FLAG((tty), CMSPAR)

Drop this.


+ * The following magic numbers is F81532/534 output pin
+ * register maps
+ */
+static const struct io_map_value f81534_rs232_control = {
+ FINTEK_DEVICE_ID, F81534_NUM_PORT, uart_mode_rs232,
+ {
+ /* please reference f81439 io port */

I can't seem to find anything about a f81439 io port. Do you have a

I believe I already asked you to describe the following function in a
comment here.

On next version we'll add more documents with this section and
change it to "f81534_pin_control". It controls F81532/534 output pin.
The pin default value can be changed with user-space tools.

+static int f81534_read_data(struct usb_serial *usbserial, u32 address,
+ u32 size, unsigned char *buf)
+ u32 read_size, count;
+ u32 block = 0;

Please use size_t for the size, count and block variables (throughout).


+ /* someone is changing setting, pause TX */
+ updating_data = mutex_is_locked(&serial_priv->change_mode_mutex);
+ if (updating_data)
+ return 0;

This cannot be done in a race-free way. You should just prevent mode
changes while the port is open.

OK, we'll remove it next version.

+ bool reConfigure = false;

Again, please remove all CamelCase.


+ if (status) {
+ dev_err(&port->dev,
+ "%s: f81534_write_data failed!! status:%d\n",

Please remove all exclamation marks (!) from all error messages.

Please also try to spell out what went wrong instead of relying on the
function name, for example in this case:

"failed to save configuration data: %d\n"

Also add a space between the final colon (:) and the error code.


+ memcpy(&port_priv->f81534_gpio_chip, &f81534_gpio_chip_templete,
+ sizeof(f81534_gpio_chip_templete));
+ snprintf(name, max_name - 1, "%s-%d", IC_NAME, idx);

The size includes the trailing NUL so use max_name only here.

OK, but this section will be removed next version.

+static int f81534_find_config_idx(struct usb_serial *serial, uintptr_t *index)
+ int idx, status;
+ u8 custom_data;
+ int offset;
+ for (idx = F81534_CUSTOM_MAX_IDX - 1; idx >= 0; --idx) {

Why go through all this trouble when F81534_CUSTOM_MAX_IDX is a constant
defined as 1?

The define is for the historical, we original designed for a section to
store the data (about 0x100 / 0x10 = 0x10 set of data), but It makes
more difficulty for H/W design. So we reduce from 0x100 to 0x10,
only 1 set of data remained with MP IC.

I'll reduce the code for next version.

+ /* Save the configuration area idx as private data for attach() */
+ usb_set_serial_data(serial, (void *) setting_idx);

No space after casts.


+ dev_info(&serial->dev->dev,
+ "%s: read configure from block:%d\n", __func__,
+ (int) setting_idx);
+ } else {
+ dev_info(&serial->dev->dev, "%s: read configure default\n",
+ __func__);
+ }

Demote these info messages to debug level.


+ for (i = 0; i < F81534_NUM_PORT; ++i) {
+ /*
+ * For older configuration use. We'll transform it to newer
+ * setting after load it with final port probed.
+ */
+ switch (setting[i]) {
+ case F81534_OLD_CONFIG_37:
+ case F81534_OLD_CONFIG_38:
+ case F81534_OLD_CONFIG_39:
+ ++num_port;
+ break;
+ }
+ }

Please explain what this old and new configuration style is and why you
need it.

Please also document the format of your configuration data.

We had 3 generation of F81532/534. All gen 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.

2nd is designed to match our transceivers(F81437/438/439). We'll save data in F81534_DEF_CONF_ADDRESS_START(0x3000) with 8bytes. The first
4bytes is transceiver type, value is only 0x37/0x38/0x39 to
represent F81437/438/439, and the following 4bytes are save mode & gpio
state, the last 4bytes will be limited by the first 4bytes transceiver
type. The f81534_calc_num_ports() will run to "older configuration"
with checking F81534_OLD_CONFIG_37/F81534_OLD_CONFIG_38
/F81534_OLD_CONFIG_39 section.

3rd 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
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.

I'll add this document in code with next version.

+ if (num_port)
+ return num_port;
+ dev_err(&serial->dev->dev, "Read Failed!!, default 4 ports\n");

If this is really an error you should bail out and return 0, otherwise
use dev_dbg or possible _warn here.

OK, we'll change it as dev_warn().

+static void f81534_release(struct usb_serial *serial)
+ struct f81534_serial_private *serial_priv =
+ usb_get_serial_data(serial);
+ kfree(serial_priv);

Please try to keep related function together. Place the release callback
after f81534_attach().


+static int f81534_set_port_mode(struct usb_serial_port *port,
+ enum uart_mode eMode)



+static int f81534_setup_urbs(struct usb_serial *serial)

Rename f81534_setup_ports.


+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_KERNEL);

You must use GFP_ATOMIC here.


Please use a concise description here (e.g. "Fintek F81532/F81534")


Thanks for your advices.
With Best Regards,
Peter Hung