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

["Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>]

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

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