lkml.org 
[lkml]   [2016]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH V11 1/1] usb:serial: Add Fintek F81532/534 driver
Date
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

\
 
 \ /
  Last update: 2016-11-10 04:23    [W:0.046 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site