Message-ID: <20161102123731.GE2664@localhost>
Date:   Wed, 2 Nov 2016 13:37:31 +0100
From:   Johan Hovold <johan@...nel.org>
To:     "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
Cc:     johan@...nel.org, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        tom_tsai@...tek.com.tw, peter_hong@...tek.com.tw,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V11 1/1] usb:serial: Add Fintek F81532/534 driver

On Fri, Oct 14, 2016 at 04:20:46PM +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.
> 
> Reviewed-by: Johan Hovold <johan@...nel.org>

You must never add other peoples' Reviewed-by tags unless you've
explicitly been given permission to do so (e.g. "fix this minor thing up
and then you can add...").

Please make sure to read the section about Reviewed-by tags in
Documentation/SubmittingPatches.

> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@...il.com>
> ---
> Changelog:
> V11
>     1. Reduce F81534_MAX_BUS_RETRY from 2000 to 20. We are only using
>        internal SPI bus to read flash when attach() & calc_num_ports()
>        and never read flash when the F81532/534 in full loading, so we
>        can reduce retry count.

Does this mean you can remove that retry mechanism completely?

>     2. Modify attach() & calc_num_ports() read default value only when
>        can't find the custom setting.
>     3. Change tx_empty protect method from spinlock to set_bit()/
>        clear_bit()/test_and_clear_bit().
>     4. Move calculate tty_idx[i] from port_probe() to attach().
>     5. Add f81534_tx_empty()

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

You never replies to my question about why this is not 124 as for rx.

> +#define F81534_MAX_RX_SIZE		124
> +#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_CHANGE		0x04
> +
> +/*
> + * We used interal SPI bus to access FLASH section. We must wait the SPI bus to
> + * idle if we performed any command.
> + *
> + * SPI Bus status register: F81534_BUS_REG_STATUS
> + *	Bit 0/1	: BUSY
> + *	Bit 2	: IDLE
> + */
> +#define F81534_BUS_BUSY			(BIT(0) | BIT(1))
> +#define F81534_BUS_IDLE			BIT(2)
> +#define F81534_BUS_READ_DATA		0x1004
> +#define F81534_BUS_REG_STATUS		0x1003
> +#define F81534_BUS_REG_START		0x1002
> +#define F81534_BUS_REG_END		0x1001
> +
> +#define F81534_CMD_READ			0x03
> +
> +#define F81534_DEFAULT_BAUD_RATE	9600
> +#define F81534_MAX_BAUDRATE		115200
> +
> +#define F81534_PORT_CONF_DISABLE_PORT	BIT(3)
> +#define F81534_PORT_CONF_NOT_EXIST_PORT	BIT(7)
> +#define F81534_PORT_UNAVAILABLE		\
> +	(F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
> +
> +#define F81534_1X_RXTRIGGER		0xc3
> +#define F81534_8X_RXTRIGGER		0xcf
> +
> +static const struct usb_device_id f81534_id_table[] = {
> +	{USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID)},
> +	{USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID)},

Add a space after { and before }.

> +	{}			/* Terminating entry */
> +};
> +
> +#define F81534_TX_EMPTY_BIT		0
> +
> +struct f81534_serial_private {
> +	u8 conf_data[F81534_DEF_CONF_SIZE];
> +	int tty_idx[F81534_NUM_PORT];
> +	u8 setting_idx;
> +	int opened_port;
> +	struct mutex urb_mutex;
> +};
> +
> +struct f81534_port_private {
> +	struct mutex mcr_mutex;
> +	unsigned long tx_empty;
> +	spinlock_t msr_lock;
> +	u8 shadow_mcr;
> +	u8 shadow_msr;
> +	u8 phy_num;
> +};


> +static int f81534_set_normal_register(struct usb_serial *serial, u16 reg,

What do mean by "normal" here? Could you give this a more descriptive
name?

Perhaps just call this one f81534_set_register and add a "port" or
"uart" infix to the current f81534_set_register below (e.g. rename it
f81534_set_uart_register, and similar for get).

Or simply replace "normal" with "generic" above.

> +					u8 data)
> +{
> +	struct usb_interface *interface = serial->interface;
> +	struct usb_device *dev = serial->dev;
> +	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) {
> +			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);
> +	}
> +
> +	kfree(tmp);
> +	return status;
> +}

> +static int f81534_set_register(struct usb_serial_port *port, u16 reg, u8 data)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +
> +	return f81534_set_normal_register(port->serial,
> +			reg + port_priv->phy_num * F81534_UART_OFFSET, data);
> +}
> +
> +static int f81534_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +
> +	return f81534_get_normal_register(port->serial,
> +			reg + port_priv->phy_num * F81534_UART_OFFSET, data);
> +}
> +
> +/*
> + * If we try to access the internal flash via SPI bus, we should check the bus
> + * status for every command. e.g., F81534_BUS_REG_START/F81534_BUS_REG_END
> + */
> +static int f81534_command_delay(struct usb_serial *serial)

Can you give this function a more descriptive name as well, perhaps
f81534_wait_for_spi_idle()?

> +{
> +	size_t count = F81534_MAX_BUS_RETRY;
> +	u8 tmp;
> +	int status;
> +
> +	do {
> +		status = f81534_get_normal_register(serial,
> +							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) {
> +		dev_err(&serial->interface->dev, "%s: retry exceed than %d\n",

Please rewrite this error message. Perhaps "timed out waiting for idle bus"
or something similar.

> +			__func__, F81534_MAX_BUS_RETRY);
> +		return -EIO;
> +	}
> +
> +	status = f81534_set_normal_register(serial, F81534_BUS_REG_STATUS,
> +				tmp & ~F81534_BUS_IDLE);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static int f81534_get_normal_register_with_delay(struct usb_serial *serial,
> +							u16 reg, u8 *data)

And then call these f81534_get_spi_register?

> +{
> +	int status;
> +
> +	status = f81534_get_normal_register(serial, reg, data);
> +	if (status)
> +		return status;
> +
> +	status = f81534_command_delay(serial);
> +	if (status)
> +		return status;

Why do you need a delay after reading?

> +
> +	return 0;
> +}
> +
> +static int f81534_set_normal_register_with_delay(struct usb_serial *serial,
> +							u16 reg, u8 data)
> +{
> +	int status;
> +
> +	status = f81534_set_normal_register(serial, reg, data);
> +	if (status)
> +		return status;
> +
> +	status = f81534_command_delay(serial);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static int f81534_read_data(struct usb_serial *serial, u32 address,
> +				size_t size, u8 *buf)

And call this f81534_read_flash (or read_spi) which is more descriptive.

> +{
> +	u8 tmp_buf[F81534_MAX_DATA_BLOCK];
> +	size_t block = 0;
> +	size_t read_size;
> +	size_t count;
> +	int status;
> +	int offset;
> +	u16 reg_tmp;
> +
> +	status = f81534_set_normal_register_with_delay(serial,
> +				F81534_BUS_REG_START, F81534_CMD_READ);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_normal_register_with_delay(serial,
> +				F81534_BUS_REG_START, (address >> 16) & 0xff);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_normal_register_with_delay(serial,
> +				F81534_BUS_REG_START, (address >> 8) & 0xff);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_normal_register_with_delay(serial,
> +				F81534_BUS_REG_START, (address >> 0) & 0xff);
> +	if (status)
> +		return status;
> +
> +	/* Continuous read mode */
> +	do {
> +		read_size = min_t(size_t, F81534_MAX_DATA_BLOCK, size);
> +
> +		for (count = 0; count < read_size; ++count) {
> +			/* To write F81534_BUS_REG_END when final byte */
> +			if (size <= F81534_MAX_DATA_BLOCK &&
> +					read_size == count + 1)
> +				reg_tmp = F81534_BUS_REG_END;
> +			else
> +				reg_tmp = F81534_BUS_REG_START;
> +
> +			/*
> +			 * Dummy code, force IC to generate a read pulse, the
> +			 * set of value 0xf1 is dont care (any value is ok)
> +			 */
> +			status = f81534_set_normal_register_with_delay(serial,
> +						reg_tmp, 0xf1);
> +			if (status)
> +				return status;
> +
> +			status = f81534_get_normal_register_with_delay(serial,
> +						F81534_BUS_READ_DATA,
> +						&tmp_buf[count]);
> +			if (status)
> +				return status;
> +
> +			offset = count + block * F81534_MAX_DATA_BLOCK;
> +			buf[offset] = tmp_buf[count];
> +		}
> +
> +		size -= read_size;
> +		++block;
> +	} while (size);
> +
> +	return 0;
> +}
> +
> +static void f81534_prepare_write_buffer(struct usb_serial_port *port, u8 *buf,
> +					size_t size)

You never use size in this function. You need to make sure you never
overwrite the provided buffer using some sanity checks.

> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	int phy_num = port_priv->phy_num;
> +	u8 tx_len;
> +	int i;
> +
> +	/*
> +	 * The block layout is fixed with 4x128 Bytes, per 128 Bytes a port.
> +	 * index 0: port phy idx (e.g., 0,1,2,3)
> +	 * index 1: only F81534_TOKEN_WRITE
> +	 * index 2: serial TX out length
> +	 * index 3: fix to 0
> +	 * index 4~127: serial out data block
> +	 */
> +	for (i = 0; i < F81534_NUM_PORT; ++i) {
> +		buf[i * F81534_RECEIVE_BLOCK_SIZE] = i;
> +		buf[i * F81534_RECEIVE_BLOCK_SIZE + 1] = F81534_TOKEN_WRITE;
> +		buf[i * F81534_RECEIVE_BLOCK_SIZE + 2] = 0;
> +		buf[i * F81534_RECEIVE_BLOCK_SIZE + 3] = 0;
> +	}
> +
> +	tx_len = kfifo_out_locked(&port->write_fifo,
> +				&buf[phy_num * F81534_RECEIVE_BLOCK_SIZE + 4],
> +				F81534_MAX_TX_SIZE, &port->lock);
> +
> +	buf[phy_num * F81534_RECEIVE_BLOCK_SIZE + 2] = tx_len;
> +}
> +
> +static int f81534_submit_writer(struct usb_serial_port *port, gfp_t mem_flags)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	struct urb *urb;
> +	unsigned long flags;
> +	int result;
> +
> +	/* 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 */
> +	if (!test_and_clear_bit(F81534_TX_EMPTY_BIT, &port_priv->tx_empty))
> +		return 0;
> +
> +	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;

You need to make sure the buffers have the expected size. They are
currently set to the endpoint size, but you can you can make sure they
are never smaller than F81534_WRITE_BUFFER_SIZE by setting bulk_out_size
in the usb_serial_driver struct.

> +
> +	result = usb_submit_urb(urb, mem_flags);
> +	if (result) {
> +		set_bit(F81534_TX_EMPTY_BIT, &port_priv->tx_empty);
> +		dev_err(&port->dev, "%s: submit failed: %d\n", __func__,
> +				result);
> +		return result;
> +	}
> +
> +	usb_serial_port_softint(port);
> +	return 0;
> +}

> +static int f81534_update_mctrl(struct usb_serial_port *port, unsigned int set,
> +				unsigned int clear)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	int status;
> +	u8 tmp;
> +
> +	mutex_lock(&port_priv->mcr_mutex);
> +
> +	if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0) {
> +		mutex_unlock(&port_priv->mcr_mutex);
> +		return 0;	/* no change */
> +	}

No need to hold the mutex for the above check.

> +
> +	/* 'Set' takes precedence over 'Clear' */
> +	clear &= ~set;
> +
> +	/* Always enable UART_MCR_OUT2 */
> +	tmp = UART_MCR_OUT2 | port_priv->shadow_mcr;
> +
> +	if (clear & TIOCM_DTR)
> +		tmp &= ~UART_MCR_DTR;
> +
> +	if (clear & TIOCM_RTS)
> +		tmp &= ~UART_MCR_RTS;
> +
> +	if (set & TIOCM_DTR)
> +		tmp |= UART_MCR_DTR;
> +
> +	if (set & TIOCM_RTS)
> +		tmp |= UART_MCR_RTS;
> +
> +	status = f81534_set_register(port, F81534_MODEM_CONTROL_REG, tmp);
> +	if (status < 0) {
> +		dev_err(&port->dev, "%s: MCR write failed\n", __func__);
> +		mutex_unlock(&port_priv->mcr_mutex);
> +		return status;
> +	}
> +
> +	port_priv->shadow_mcr = tmp;
> +	mutex_unlock(&port_priv->mcr_mutex);
> +	return 0;
> +}

> +static int f81534_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	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 status;
> +
> +	status = f81534_set_register(port,
> +				F81534_FIFO_CONTROL_REG, UART_FCR_ENABLE_FIFO |
> +				UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> +	if (status) {
> +		dev_err(&port->dev, "%s: Clear FIFO failed: %d\n", __func__,
> +				status);
> +		return status;
> +	}
> +
> +	if (tty)
> +		f81534_set_termios(tty, port, NULL);
> +
> +	status = f81534_read_msr(port);
> +	if (status)
> +		return status;
> +
> +	mutex_lock(&serial_priv->urb_mutex);
> +
> +	set_bit(F81534_TX_EMPTY_BIT, &port_priv->tx_empty);

Move this outside of the locked section.
> +
> +	/* Submit Read URBs for first port opened */
> +	if (!serial_priv->opened_port) {
> +		status = f81534_submit_read_urb(port->serial, GFP_KERNEL);
> +		if (status)
> +			goto exit;
> +	}
> +
> +	serial_priv->opened_port++;
> +
> +exit:
> +	mutex_unlock(&serial_priv->urb_mutex);
> +
> +	return status;
> +}

> +static void f81534_process_per_serial_block(struct usb_serial_port *port,
> +		u8 *data)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +	int phy_num = data[0];
> +	size_t read_size = 0;
> +	size_t i;
> +	char tty_flag;
> +	int status;
> +	u8 lsr;
> +
> +	if (phy_num >= F81534_NUM_PORT) {
> +		dev_err(&port->dev, "%s: phy_num: %d larger than: %d\n",
> +				__func__, phy_num, F81534_NUM_PORT);
> +		return;
> +	}

You already verified this in f81534_process_read_urb() below.

> +
> +	/*
> +	 * The block layout is 128 Bytes
> +	 * index 0: port phy idx (e.g., 0,1,2,3),
> +	 * index 1: It's could be
> +	 *			F81534_TOKEN_RECEIVE
> +	 *			F81534_TOKEN_TX_EMPTY
> +	 *			F81534_TOKEN_MSR_CHANGE
> +	 * index 2: serial in size (data+lsr, must be even)
> +	 *			meaningful for F81534_TOKEN_RECEIVE only
> +	 * index 3: current MSR with this device
> +	 * index 4~127: serial in data block (data+lsr, must be even)
> +	 */
> +	switch (data[1]) {
> +	case F81534_TOKEN_TX_EMPTY:
> +		set_bit(F81534_TX_EMPTY_BIT, &port_priv->tx_empty);
> +
> +		/* Try to submit writer */
> +		status = f81534_submit_writer(port, GFP_ATOMIC);
> +		if (status)
> +			dev_err(&port->dev, "%s: submit failed\n", __func__);
> +		return;
> +
> +	case F81534_TOKEN_MSR_CHANGE:
> +		f81534_msr_changed(port, data[3]);
> +		return;
> +
> +	case F81534_TOKEN_RECEIVE:
> +		read_size = data[2];
> +		if (read_size > F81534_MAX_RX_SIZE) {
> +			dev_err(&port->dev,
> +				"%s: phy: %d read_size: %zu larger than: %d\n",
> +				__func__, phy_num, read_size,
> +				F81534_MAX_RX_SIZE);
> +			return;
> +		}
> +
> +		break;
> +
> +	default:
> +		dev_warn(&port->dev, "%s: unknown token: %02x\n", __func__,
> +				data[1]);
> +		return;
> +	}
> +
> +	for (i = 4; i < 4 + read_size; i += 2) {
> +		tty_flag = TTY_NORMAL;
> +		lsr = data[i + 1];
> +
> +		if (lsr & UART_LSR_BRK_ERROR_BITS) {
> +			if (lsr & UART_LSR_BI) {
> +				tty_flag = TTY_BREAK;
> +				port->icount.brk++;
> +				usb_serial_handle_break(port);
> +			} else if (lsr & UART_LSR_PE) {
> +				tty_flag = TTY_PARITY;
> +				port->icount.parity++;
> +			} else if (lsr & UART_LSR_FE) {
> +				tty_flag = TTY_FRAME;
> +				port->icount.frame++;
> +			}
> +
> +			if (lsr & UART_LSR_OE) {
> +				port->icount.overrun++;
> +				tty_insert_flip_char(&port->port, 0,
> +						TTY_OVERRUN);
> +			}
> +		}
> +
> +		if (port->port.console && port->sysrq) {
> +			if (usb_serial_handle_sysrq_char(port, data[i]))
> +				continue;
> +		}
> +
> +		tty_insert_flip_char(&port->port, data[i], tty_flag);
> +	}
> +
> +	tty_flip_buffer_push(&port->port);
> +}
> +
> +static void f81534_process_read_urb(struct urb *urb)
> +{
> +	struct f81534_serial_private *serial_priv;
> +	struct usb_serial_port *port;
> +	struct usb_serial *serial;
> +	u8 *buf;
> +	int phy_port_num;
> +	int tty_port_num;
> +	size_t i;
> +
> +	if (!urb->actual_length ||
> +			urb->actual_length % F81534_RECEIVE_BLOCK_SIZE) {
> +		return;
> +	}
> +
> +	port = urb->context;
> +	serial = port->serial;
> +	buf = urb->transfer_buffer;
> +	serial_priv = usb_get_serial_data(serial);
> +
> +	for (i = 0; i < urb->actual_length; i += F81534_RECEIVE_BLOCK_SIZE) {
> +		phy_port_num = buf[i];
> +		if (phy_port_num >= F81534_NUM_PORT) {
> +			dev_err(&port->dev,
> +				"%s: phy_port_num: %d larger than: %d\n",
> +				__func__, phy_port_num, F81534_NUM_PORT);
> +			continue;
> +		}
> +
> +		tty_port_num = serial_priv->tty_idx[phy_port_num];
> +		port = serial->port[tty_port_num];
> +
> +		if (tty_port_initialized(&port->port))
> +			f81534_process_per_serial_block(port, &buf[i]);
> +	}
> +}

> +static bool f81534_tx_empty(struct usb_serial_port *port)
> +{
> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> +
> +	return !!test_bit(F81534_TX_EMPTY_BIT, &port_priv->tx_empty);

!! not needed.

> +}
> +
> +static int f81534_resume(struct usb_serial *serial)
> +{
> +	struct f81534_serial_private *serial_priv =
> +			usb_get_serial_data(serial);
> +	struct usb_serial_port *port;
> +	int status;
> +	size_t i;
> +
> +	/*
> +	 * We'll register port 0 bulkin when port had opened, It'll take all
> +	 * port received data, MSR register change and TX_EMPTY information.
> +	 */
> +	mutex_lock(&serial_priv->urb_mutex);
> +
> +	if (serial_priv->opened_port) {
> +		status = f81534_submit_read_urb(serial, GFP_NOIO);
> +		if (status) {
> +			mutex_unlock(&serial_priv->urb_mutex);
> +			return status;
> +		}
> +	}
> +
> +	mutex_unlock(&serial_priv->urb_mutex);
> +
> +	for (i = 0; i < serial->num_ports; i++) {
> +		port = serial->port[i];
> +		if (!tty_port_initialized(&port->port))
> +			continue;
> +
> +		status = f81534_submit_writer(port, GFP_NOIO);
> +		if (status) {
> +			dev_err(&port->dev, "%s: submit failed\n", __func__);
> +			return status;

You may still want to continue with the remaining ports as you did in
v10 (but return an error in the end).

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct usb_serial_driver f81534_device = {
> +	.driver = {
> +		   .owner = THIS_MODULE,
> +		   .name = "f81534",
> +	},
> +	.description =		DRIVER_DESC,
> +	.id_table =		f81534_id_table,
> +	.open =			f81534_open,
> +	.close =		f81534_close,
> +	.write =		f81534_write,
> +	.tx_empty =		f81534_tx_empty,
> +	.calc_num_ports =	f81534_calc_num_ports,
> +	.attach =		f81534_attach,

You need to add a .probe function in which you make sure that you have
the expected bulk-in and out endpoints (or you could crash later on).

You could also verify the endpoint size in this function instead of
setting the .bulk_out_size field mentioned above.

Thanks,
Johan

Powered by blists - more mailing lists