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

[Johan Hovold]

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.

> 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 ioctl TIOCGRS485/TIOCSRS485
> 
>    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. 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 it implements as
>    gpiolib interface with output-only function.
> 5. It'll save the configuration in internal storage when uart/pins mode
>    changed.
> 
>    Please reference https://bitbucket.org/hpeter/fintek-general/src/
>    with f81534/tools to get set_gpio.c & set_mode.c to change F81532/534
>    setting. Please use it carefully.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@xxxxxxxxx>
> ---
> Changelog:
> 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

Thanks for the update.

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

Also note that a review comment generally applies to all instances of
a particular issue throughout the driver even when it is not
specifically mentioned.

In order to get this driver merged, I think you should consider starting
with a subset of the functionality.

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.

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).

> +#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)

Do you really expect F81534_CUSTOM_MAX_IDX to be 1?

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

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
link?

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

> +static int f81534_command_delay(struct usb_serial *usbserial)
> +{
> +	unsigned int count = F81534_MAX_BUS_RETRY;
> +	unsigned char tmp;
> +	int status;
> +	struct usb_device *dev = usbserial->dev;
> +
> +	do {
> +		status = f81534_get_normal_register(dev, 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;
> +
> +	status = f81534_set_normal_register(dev, F81534_BUS_REG_STATUS,
> +				tmp & ~F81534_BUS_IDLE);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}

> +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).

> +	u16 reg_tmp;
> +	u8 tmp_buf[F81534_MAX_DATA_BLOCK];
> +	int status, offset;

> +static int f81534_submit_writer(struct usb_serial_port *port, gfp_t mem_flags)
> +{
> +	struct usb_serial *serial = port->serial;
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(serial);
> +	struct tty_struct *tty;
> +	struct urb *urb;
> +	bool cts_status = true;
> +	unsigned long flags;
> +	int updating_data, result;
> +
> +	tty = tty_port_tty_get(&port->port);
> +	if (!tty)
> +		return 0;
> +
> +	/* check H/W Flow status */
> +	if (C_CRTSCTS(tty)) {
> +		spin_lock_irqsave(&port_priv->msr_lock, flags);
> +		cts_status = !!(port_priv->shadow_msr & UART_MSR_CTS);
> +		spin_unlock_irqrestore(&port_priv->msr_lock, flags);
> +	}
> +
> +	tty_kref_put(tty);
> +
> +	if (!cts_status)
> +		return 0;
> +
> +	/* 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.

> +
> +	/* 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(&serial_priv->tx_empty_lock, flags);
> +
> +	if (serial_priv->is_phy_port_not_empty[port_priv->phy]) {
> +		spin_unlock_irqrestore(&serial_priv->tx_empty_lock, flags);
> +		return 0;
> +	}
> +
> +	serial_priv->is_phy_port_not_empty[port_priv->phy] = true;
> +	spin_unlock_irqrestore(&serial_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) {
> +		dev_err(&port->dev, "%s: submit error, result:%d\n", __func__,
> +				result);
> +		return result;
> +	}
> +
> +	return 0;
> +}

> +/*
> + * This function could be executed when
> + *	1. Port configuration change. (e.g., UART/GPIO Mode changed)
> + *	2. Old IC or configuration detected.
> + *         During the port probe(), We'll check the current port is final port.
> + *	   If we found a old style configuration value, the
> + *	   f81534_load_configure_data() will transform old to new default
> + *	   setting to RAM, then f81534_save_configure_data() will compare the
> + *	   flash & RAM setting, If not the same, write it with new data with
> + *	   final port probe().
> + */
> +static int f81534_save_configure_data(struct usb_serial_port *port)
> +{
> +	int status;
> +	int count;
> +	int phy;
> +	int gpio_address, uart_address;
> +	int offset;
> +	bool reConfigure = false;

Again, please remove all CamelCase.

> +	u8 uart_mode, gpio_mode;
> +	u8 data[F81534_DEF_CONF_SIZE + 1];
> +	u8 tmp[F81534_DEF_CONF_SIZE];
> +	enum uart_mode current_mode;
> +	struct usb_serial *serial = port->serial;
> +	struct f81534_port_private *port_priv;
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(serial);
> +
> +	/* compare memory with ic data */
> +	for (count = 0; count < serial->num_ports; ++count) {
> +		port_priv = usb_get_serial_port_data(serial->port[count]);
> +
> +		if (!port_priv) {
> +			dev_err(&port->dev, "%s: port_priv == NULL\n",
> +					__func__);
> +			continue;
> +		}
> +
> +		phy = port_priv->phy;
> +
> +		if (serial_priv->setting_idx == F81534_CUSTOM_NO_CUSTOM_DATA) {
> +			uart_address = F81534_DEF_CONF_ADDRESS_START + phy;
> +			gpio_address = F81534_DEF_CONF_ADDRESS_START + phy +
> +							F81534_CONF_SIZE;
> +		} else {
> +			/*
> +			 * if had custom setting, override
> +			 * 1st byte is a indicator, 0xff is empty,
> +			 * 0xf0 is had data. Skip with 1st data
> +			 */
> +
> +			uart_address = F81534_CUSTOM_ADDRESS_START +
> +				serial_priv->setting_idx *
> +				F81534_CUSTOM_DATA_SIZE + phy +
> +				F81534_CONF_OFFSET;
> +
> +			gpio_address = F81534_CUSTOM_ADDRESS_START +
> +				serial_priv->setting_idx *
> +				F81534_CUSTOM_DATA_SIZE + phy +
> +				F81534_CONF_SIZE + F81534_CONF_OFFSET;
> +		}
> +
> +		status = f81534_read_data(port->serial, uart_address, 1,
> +						&uart_mode);
> +		if (status) {
> +			dev_err(&port->dev,
> +					"%s: read uart data fail. status:%d\n",
> +					__func__, status);
> +			return status;
> +		}
> +
> +		status = f81534_read_data(port->serial, gpio_address, 1,
> +						&gpio_mode);
> +		if (status) {
> +			dev_err(&port->dev,
> +					"%s: read gpio data fail. status:%d\n",
> +					__func__, status);
> +			return status;
> +		}
> +
> +		if (port_priv->port_pin_data.gpio_mode != gpio_mode)
> +			reConfigure = true;
> +
> +		/* check uart flag */
> +		if (port_priv->port_pin_data.force_uart_mode ==
> +				uart_mode_rs232) {
> +			if ((uart_mode & F81534_MODE_MASK) !=
> +					F81534_RS232_FLAG)
> +				reConfigure = true;
> +		} else if (port_priv->port_pin_data.force_uart_mode ==
> +					uart_mode_rs485_1) {
> +			if ((uart_mode & F81534_MODE_MASK) !=
> +					F81534_RS485_1_FLAG)
> +				reConfigure = true;
> +		} else if (port_priv->port_pin_data.force_uart_mode ==
> +			   uart_mode_rs485) {
> +			if ((uart_mode & F81534_MODE_MASK) !=
> +					F81534_RS485_FLAG)
> +				reConfigure = true;
> +		} else {
> +			reConfigure = true;
> +		}
> +
> +		if (reConfigure)
> +			break;
> +	}
> +
> +	if (serial_priv->setting_idx == F81534_CUSTOM_NO_CUSTOM_DATA) {
> +		dev_info(&serial->dev->dev, "%s: force to reconfigure\n",
> +					__func__);
> +	} else if (!reConfigure) {
> +		dev_dbg(&serial->dev->dev, "%s: update-to-date\n", __func__);
> +		return 0;
> +	}
> +
> +	dev_info(&serial->dev->dev, "%s: updating\n", __func__);
> +
> +	/* next setting block */
> +	serial_priv->setting_idx =
> +			(serial_priv->setting_idx + 1) % F81534_CUSTOM_MAX_IDX;
> +	dev_info(&serial->dev->dev, "%s: saving to block index:%d\n", __func__,
> +			serial_priv->setting_idx);
> +
> +	/* erase when start block is 0 */
> +	if (!serial_priv->setting_idx) {
> +		dev_dbg(&serial->dev->dev, "%s: need erase\n", __func__);
> +
> +		/* erase */
> +		status = f81534_erase_sector(serial,
> +					F81534_CUSTOM_ADDRESS_START);
> +		if (status) {
> +			dev_err(&port->dev,
> +					"%s: erase sector failed! status:%d\n",
> +					__func__, status);
> +			return status;
> +		}
> +	} else {
> +		dev_dbg(&serial->dev->dev,
> +				"%s: dont need erase\n", __func__);
> +	}
> +
> +	/* reprogram */
> +	for (count = 0; count < serial->num_ports; ++count) {
> +		port_priv = usb_get_serial_port_data(serial->port[count]);
> +		phy = port_priv->phy;
> +		current_mode = port_priv->port_pin_data.force_uart_mode;
> +		gpio_mode = port_priv->port_pin_data.gpio_mode;
> +
> +		serial_priv->default_conf_data[phy + F81534_CONF_SIZE] =
> +								gpio_mode;
> +		serial_priv->default_conf_data[phy] &= ~(F81534_MODE_MASK);
> +
> +		/* check uart flag */
> +		if (current_mode == uart_mode_rs232) {
> +			serial_priv->default_conf_data[phy] |=
> +					F81534_RS232_FLAG;
> +		} else if (current_mode == uart_mode_rs485_1) {
> +			serial_priv->default_conf_data[phy] |=
> +					F81534_RS485_1_FLAG;
> +		} else if (current_mode == uart_mode_rs485) {
> +			serial_priv->default_conf_data[phy] |=
> +					F81534_RS485_FLAG;
> +		} else {
> +			dev_err(&serial->dev->dev,
> +					"%s: current_mode error, value:%d\n",
> +					__func__, current_mode);
> +		}
> +
> +		dev_info(&serial->dev->dev,
> +				"%s: port:%d uart_mode:%x, gpio_mode:%x\n",
> +				__func__, count,
> +				serial_priv->default_conf_data[phy + 0],
> +				gpio_mode);
> +	}
> +
> +	/*
> +	 * 1st byte is a indicator, 0xff is empty, 0xf0 is had data
> +	 * only write 8 bytes of total 4 port uart & gpio mode
> +	 * so we need write 1+8 data
> +	 */
> +
> +	/* token of data exist */
> +	data[0] = F81534_CUSTOM_VALID_TOKEN;
> +	memcpy(&data[1], serial_priv->default_conf_data, F81534_DEF_CONF_SIZE);
> +
> +	offset = F81534_CUSTOM_ADDRESS_START +
> +			F81534_CUSTOM_DATA_SIZE * serial_priv->setting_idx;
> +
> +	status = f81534_write_data(serial, offset, sizeof(data), data);
> +	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.

> +				__func__, status);
> +		return status;
> +	}
> +
> +	/* recheck save & memory data */
> +	memset(tmp, 0, sizeof(tmp));
> +
> +	status = f81534_read_data(serial,
> +				  F81534_CUSTOM_ADDRESS_START +
> +				  F81534_CUSTOM_DATA_SIZE *
> +				  serial_priv->setting_idx + 1, sizeof(tmp),
> +				  tmp);
> +	if (status) {
> +		dev_err(&port->dev,
> +				"%s: f81534_read_data failed!! status:%d\n",
> +				__func__, status);
> +		return status;
> +	}
> +
> +	for (count = 0; count < F81534_DEF_CONF_SIZE; ++count) {
> +		if (tmp[count] == serial_priv->default_conf_data[count])
> +			continue;
> +
> +		dev_err(&port->dev,
> +				"%s:read data error, count:%d, data:%x %x\n",
> +				__func__, count, tmp[count],
> +				serial_priv->default_conf_data[count]);
> +	}
> +
> +	dev_dbg(&serial->dev->dev, "%s: complete\n", __func__);
> +
> +	return 0;
> +}

> +static int f81534_prepare_gpio(struct usb_serial_port *port)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	int max_name = 32;
> +	char *name = NULL;
> +	int idx = port->minor;
> +	int rc;
> +
> +	name = devm_kzalloc(&port->dev, max_name, GFP_KERNEL);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	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.

> +
> +	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) {
> +		dev_err(&port->dev, "%s: f81534_prepare_gpio failed:%d\n",
> +				__func__, rc);
> +		return rc;
> +	}
> +
> +	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
> + */
> +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?

> +		offset = F81534_CUSTOM_ADDRESS_START +
> +					F81534_CUSTOM_DATA_SIZE * idx;
> +		status = f81534_read_data(serial, offset, 1, &custom_data);
> +		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 indicator, 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)
> +{
> +	uintptr_t setting_idx;
> +	int i;
> +	u8 num_port = 0;
> +	int status;
> +	unsigned char setting[F81534_CUSTOM_DATA_SIZE + 1];
> +
> +	/* check had custom setting */
> +	status = f81534_find_config_idx(serial, &setting_idx);
> +	if (status) {
> +		dev_err(&serial->dev->dev,
> +				"%s: f81534_find_config_idx read failed!!\n",
> +				__func__);
> +		return 0;
> +	}
> +
> +	/* Save the configuration area idx as private data for attach() */
> +	usb_set_serial_data(serial, (void *) setting_idx);

No space after casts.

> +
> +	/* 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: f81534_read_data read failed!!\n",
> +				__func__);
> +		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_CUSTOM_DATA_SIZE *
> +					  setting_idx + 1, sizeof(setting),
> +					  setting);
> +		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__,
> +				(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.

> +
> +	if (num_port) {
> +		dev_dbg(&serial->dev->dev, "%s: old style with %d ports",
> +				__func__, num_port);
> +		return num_port;
> +	}
> +
> +	/* 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_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.

> +	return 4;		/* nothing found, oldest version IC */
> +}
> +

> +static void f81534_close(struct usb_serial_port *port)
> +{
> +	int i;
> +	unsigned long flags;
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(port->serial);
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	int phy = port_priv->phy;
> +
> +	atomic_dec(&serial_priv->port_active[phy]);
> +
> +	/* 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);
> +}
> +
> +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_get_serial_info(struct usb_serial_port *port,
> +				  struct serial_struct __user *retinfo)
> +{
> +	struct serial_struct tmp;
> +	struct f81534_port_private *port_priv;
> +
> +	port_priv = usb_get_serial_port_data(port);
> +	if (!port_priv)
> +		return -EFAULT;
> +
> +	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 = port_priv->current_baud_base;
> +
> +	if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int f81534_set_port_mode(struct usb_serial_port *port,
> +		enum uart_mode eMode)

CamelCase

> +{
> +	int status;
> +	u8 tmp;
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +
> +	if (eMode > uart_mode_invalid)
> +		return -EINVAL;
> +
> +	if (eMode != uart_mode_invalid) {
> +		status = f81534_getregister(port->serial->dev, port_priv->phy,
> +						CLK_SEL_REGISTER, &tmp);
> +		if (status)
> +			return status;
> +
> +		tmp &= ~(F81534_RS485_MODE | F81534_RS485_INVERT);
> +
> +		switch (port_priv->port_pin_data.force_uart_mode) {
> +		case uart_mode_rs232:
> +		case uart_mode_shutdown:
> +		case uart_mode_rs232_coexist:
> +			break;
> +
> +		case uart_mode_rs485:
> +			tmp |= (F81534_RS485_MODE | F81534_RS485_INVERT);
> +			dev_dbg(&port->dev, "%s: uart_mode_rs485 URB:%x\n",
> +					__func__, tmp);
> +			break;
> +
> +		default:
> +			tmp |= F81534_RS485_MODE;
> +			dev_dbg(&port->dev, "%s others URB:%x\n",
> +					__func__, tmp);
> +			break;
> +
> +		}
> +
> +		status = f81534_setregister(port->serial->dev, port_priv->phy,
> +						CLK_SEL_REGISTER, tmp);
> +		if (status)
> +			return status;
> +	}
> +
> +	port_priv->port_pin_data.force_uart_mode = eMode;
> +	return 0;
> +}

> +static int f81534_setup_urbs(struct usb_serial *serial)

Rename f81534_setup_ports.

> +{
> +	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);
> +		}
> +
> +		port->write_urb = port->write_urbs[0];
> +		port->bulk_out_buffer = port->bulk_out_buffers[0];
> +	}
> +
> +	return 0;
> +failed:
> +	return -ENOMEM;
> +}

> +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.

> +	if (status) {
> +		dev_err(&port->dev, "%s: submit failed\n", __func__);
> +		return status;
> +	}
> +
> +	return bytes_out;
> +}

> +static struct usb_serial_driver f81534_device = {
> +	.driver = {
> +		   .owner = THIS_MODULE,
> +		   .name = IC_NAME,
> +		   },
> +	.description = DRIVER_DESC,

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

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/