From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756934AbcBQJaZ (ORCPT ); Wed, 17 Feb 2016 04:30:25 -0500 Received: from mail-pf0-f172.google.com ([209.85.192.172]:35849 "EHLO mail-pf0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756551AbcBQJaS (ORCPT ); Wed, 17 Feb 2016 04:30:18 -0500 Subject: Re: [PATCH V3 3/4] 8250: 8250_f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO UART support To: Andy Shevchenko , linus.walleij@linaro.org, gnurou@gmail.com, gregkh@linuxfoundation.org, paul.gortmaker@windriver.com, lee.jones@linaro.org, jslaby@suse.com, gnomes@lxorguk.ukuu.org.uk, peter_hong@fintek.com.tw References: <1455605710-3724-1-git-send-email-hpeter+linux_kernel@gmail.com> <1455605710-3724-4-git-send-email-hpeter+linux_kernel@gmail.com> <1455613899.31169.149.camel@linux.intel.com> Cc: heikki.krogerus@linux.intel.com, peter@hurleysoftware.com, soeren.grunewald@desy.de, udknight@gmail.com, adam.lee@canonical.com, arnd@arndb.de, manabian@gmail.com, scottwood@freescale.com, yamada.masahiro@socionext.com, paul.burton@imgtec.com, mans@mansr.com, matthias.bgg@gmail.com, ralf@linux-mips.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, linux-serial@vger.kernel.org, tom_tsai@fintek.com.tw, Peter Hung From: Peter Hung Message-ID: <56C43DA7.9080400@gmail.com> Date: Wed, 17 Feb 2016 17:30:15 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1455613899.31169.149.camel@linux.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Andy, Andy Shevchenko 於 2016/2/16 下午 05:11 寫道: > On Tue, 2016-02-16 at 14:55 +0800, Peter Hung wrote: >> +static u32 baudrate_table[] = { 1500000, 1152000, 921600 }; >> +static u8 clock_table[] = { F81504_CLKSEL_24_MHZ, >> F81504_CLKSEL_18DOT46_MHZ, >> + F81504_CLKSEL_14DOT77_MHZ }; > > I suggest to replace DOT by _. ok >> +/* We should do proper H/W transceiver setting before change to >> RS485 mode */ >> +static int f81504_rs485_config(struct uart_port *port, >> + struct serial_rs485 *rs485) >> +{ >> + u8 setting; >> + u8 *index = (u8 *) port->private_data; > > private_data is a type of void *, therefore no need to have an explicit > casting. ok >> +static int f81504_check_baudrate(u32 baud, size_t *idx) >> +{ >> + size_t index; >> + u32 quot, rem; >> + >> + for (index = 0; index < ARRAY_SIZE(baudrate_table); ++index) > > Post-increment is also okay. > >> { >> + /* Clock source must largeer than desire baudrate */ >> + if (baud > baudrate_table[index]) >> + continue; >> + >> + quot = DIV_ROUND_CLOSEST(baudrate_table[index], >> baud); > > So, how quot is used and is it possible to set, for example, baud rate > as 1000000 or 576000? The IC don't support B1000000 due to no 16MHz clock source. The quot & rem is only use for compare, and it's must be not 0 when the code run to calculate DIV_ROUND_CLOSEST. So quot is a redundancy here. This function will find the suitable clock source for future use. We'll pass suitable baud rate * 16 to port->uartclk for serial8250_do_set_termios() to do advance divider operations. I'll rewrite this section with remove quot and direct check the "baudrate_table[index] % baud" divisible. >> + u8 tmp, *offset = (u8 *) port->private_data; > > Same for provate_data as above. ok >> + /* >> + * direct use 1.8432MHz when baudrate >> smaller then or >> + * equal 115200bps > > Check your style of comments in a _whole_ your series. ok >> + /* >> + * If it can't found suitable clock source >> but had old >> + * accpetable baudrate, we'll use it > > Typo: acceptable. > Baudrate -> baud rate. ok >> + port.port.private_data = data; /* save current idx */ > > Not sure you need to allocate memory for that at all, or maybe use a > struct with one member (for now). > We just pass the index of PCI configuration space currently. So I just set a allocated u8 memory to private data. We'll maintain current method. >> +static SIMPLE_DEV_PM_OPS(f81504_serial_pm_ops, >> f81504_serial_suspend, >> + f81504_serial_resume); >> + >> +static struct platform_driver f81504_serial_driver = { >> + .driver = { >> + .name = F81504_SERIAL_NAME, >> + .owner = THIS_MODULE, > > You perhaps don't need this. Check the rest of the modules. ok >> +config SERIAL_8250_F81504 >> + tristate "Fintek F81504/508/512 16550 PCIE device support" >> if EXPERT >> + depends on SERIAL_8250 && MFD_FINTEK_F81504_CORE >> + default SERIAL_8250 >> + select RATIONAL > > It seems RATIONAL API is not used here. > This driver hadn't use RATIONAL API. I'll remove it. Thanks for your advice. -- With Best Regards, Peter Hung