From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753248AbcKJDXF (ORCPT + 2 others); Wed, 9 Nov 2016 22:23:05 -0500 Received: from mail-pf0-f195.google.com ([209.85.192.195]:33045 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751203AbcKJDXD (ORCPT ); Wed, 9 Nov 2016 22:23:03 -0500 From: "Ji-Ze Hong (Peter Hong)" Subject: Re: [PATCH V11 1/1] usb:serial: Add Fintek F81532/534 driver To: Johan Hovold References: <1476433246-19427-1-git-send-email-hpeter+linux_kernel@gmail.com> <20161102123731.GE2664@localhost> Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, tom_tsai@fintek.com.tw, peter_hong@fintek.com.tw, "Ji-Ze Hong (Peter Hong)" Message-ID: Date: Thu, 10 Nov 2016 11:22:59 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161102123731.GE2664@localhost> Content-Type: text/plain; charset=big5; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi Johan, Johan Hovold ©ó 2016/11/2 ¤U¤È 08:37 ¼g¹D: > On Fri, Oct 14, 2016 at 04:20:46PM +0800, Ji-Ze Hong (Peter Hong) wrote: >> >> Reviewed-by: Johan Hovold > > 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