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

[Peter Hung]

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