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

[Ji-Ze Hong (Peter Hong)]

Hi Johan,

Johan Hovold 於 2016/11/2 下午 08:37 寫道:
> On Fri, Oct 14, 2016 at 04:20:46PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> > Reviewed-by: Johan Hovold <johan@xxxxxxxxxx>
>
> 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.

Sorry for misusing "Reviewed-by" tags.

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

The mechanism is for checking SPI Bus busy status. We need to perform
read/write operation only when the bus idle. So we need remain the check
mechanism.

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

The limit for TX with 100 bytes is tuned for high-speed TX performance.
But this patch is not contained for high-baudrate, so we'll change it
for 124 byte with next patch.


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

OK, I'll rename such functions with following:

Generic read/write USB:
  f81534_set/get_normal_register -> f81534_set/get_register

UART port read/write:
  f81534_set/get_register -> f81534_set/get_port_register

SPI bus read/write
  f81534_set_normal_register_with_delay -> f81534_set/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?

Sorry for misleading.
I'll rename f81534_command_delay to f81534_wait_for_spi_idle.


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

OK, I'll add probe() to check bulk_in/out endpoints count & packet size.



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

Thanks for your notice. We had tested it on USB 2.0 Full-speed, the
packet size will reduce to 64, it will cause buffer overflow. So I'll
reconfirm the packet size in probe().


Thanks for your comments.
--
With Best Regards,
Peter Hong