Messages in this thread | | | From | "Ji-Ze Hong (Peter Hong)" <> | Subject | Re: [PATCH V11 1/1] usb:serial: Add Fintek F81532/534 driver | Date | Thu, 10 Nov 2016 11:22:59 +0800 |
| |
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
| |