Date:	Mon, 11 Jan 2016 14:06:10 +0800
From:	Peter Hung <hpeter@...il.com>
To:	Johan Hovold <johan@...nel.org>
Cc:	gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, tom_tsai@...tek.com.tw,
	peter_hong@...tek.com.tw,
	Peter Hung <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V7 1/1] usb:serial: Add Fintek F81532/534 driver

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.

F81532
https://drive.google.com/file/d/0B8vRwwYO7aMFOTRRMmhWQVNvajQ/view?usp=sharing

F81534
https://drive.google.com/file/d/0B8vRwwYO7aMFV29pQWJqbVBNc00/view?usp=sharing

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

> 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		\
>> +			(F81534_CUSTOM_TOTAL_SIZE/F81534_CUSTOM_DATA_SIZE)
>
> 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)
>> +#endif
>
> Drop this.

OK


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

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

OK.

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

OK.


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

OK.


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

OK.


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

OK.


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

OK.


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

OK.


>> +static int f81534_setup_urbs(struct usb_serial *serial)
>
> Rename f81534_setup_ports.

OK.

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

OK.

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

OK.

Thanks for your advices.
-- 
With Best Regards,
Peter Hung

Powered by blists - more mailing lists