From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754688AbcBPJNK (ORCPT ); Tue, 16 Feb 2016 04:13:10 -0500 Received: from mga04.intel.com ([192.55.52.120]:64116 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754687AbcBPJLE (ORCPT ); Tue, 16 Feb 2016 04:11:04 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,454,1449561600"; d="scan'208";a="652810999" Message-ID: <1455613899.31169.149.camel@linux.intel.com> Subject: Re: [PATCH V3 3/4] 8250: 8250_f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO UART support From: Andy Shevchenko To: Peter Hung , 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 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 Date: Tue, 16 Feb 2016 11:11:39 +0200 In-Reply-To: <1455605710-3724-4-git-send-email-hpeter+linux_kernel@gmail.com> References: <1455605710-3724-1-git-send-email-hpeter+linux_kernel@gmail.com> <1455605710-3724-4-git-send-email-hpeter+linux_kernel@gmail.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2016-02-16 at 14:55 +0800, Peter Hung wrote: > This driver is 8250 driver for F81504/508/512, it'll handle the > serial > port operation of this device. This module will depend on > MFD_FINTEK_F81504_CORE. > > The serial ports support from 50bps to 1.5Mbps with Linux baudrate > define excluding 1.0Mbps due to not support 16MHz clock source. > > PCI Configuration Space Registers, set:0~11(Max): >     40h + 8 * set: >                    bit7~6: Clock source selector >                        00: 1.8432MHz >                        01: 18.432MHz >                        10: 24MHz >                        11: 14.769MHz >                    bit0: UART enable >     41h + 8 * set: >                    bit5~4: RX trigger multiple >                        00: 1x * trigger level >                        01: 2x * trigger level >                        10: 4x * trigger level >                        11: 8x * trigger level >                    bit1~0: FIFO Size >                        11: 128Bytes >     44h + 8 * set: UART IO address (LSB) >     45h + 8 * set: UART IO address (MSB) >     47h + 8 * set: >                    bit5: RTS invert (bit4 must enable) >                    bit4: RTS auto direction enable >                          0: RTS control by MCR >                          1: RTS driven high when TX, otherwise low > Few my comments below. > +++ b/drivers/tty/serial/8250/8250_f81504.c > @@ -0,0 +1,254 @@ > +#include > +#include > +#include > +#include > +#include > + > +#include "8250.h" > + > +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 _. > + > +/* 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. > +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? > + /* find divisible clock source */ > + rem = baudrate_table[index] % baud; > + > + if (quot && !rem) { > + if (idx) > + *idx = index; > + return 0; > + } > + } > + > + return -EINVAL; > +} > + > +static void f81504_set_termios(struct uart_port *port, > + struct ktermios *termios, struct ktermios *old) > +{ > + struct platform_device *pdev = container_of(port->dev, > + struct platform_device, > dev); > + struct pci_dev *dev = to_pci_dev(pdev->dev.parent); > + unsigned int baud = tty_termios_baud_rate(termios); > + u8 tmp, *offset = (u8 *) port->private_data; Same for provate_data as above. > + /* read current clock source (masked with > CLOCK_RATE_MASK) */ ... > + /* > +  * direct use 1.8432MHz when baudrate > smaller then or > +  * equal 115200bps Check your style of comments in a _whole_ your series. /*   * Start sentence with Capital letter and end with a period.  */ > +  */ > > + if (!f81504_check_baudrate(baud, &i)) { > + /* had optimize value */ /* For one line comment */ > + /* > +  * If it can't found suitable clock source > but had old > +  * accpetable baudrate, we'll use it Typo: acceptable. Baudrate ->  baud rate. > +  */ > + baud = tty_termios_baud_rate(old); > + } else { > + /* > +  * If it can't found suitable clock source > and not old > +  * config, we'll direct set 115200bps for > future use > +  */ > +static int f81504_register_port(struct platform_device *dev, > + unsigned long address, int idx) > +{ > + struct pci_dev *pci_dev = to_pci_dev(dev->dev.parent); > + struct uart_8250_port port; > + u8 *data; > + > + memset(&port, 0, sizeof(port)); > + data = devm_kzalloc(&dev->dev, sizeof(u8), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + *data = idx; > + port.port.iotype = UPIO_PORT; > + port.port.irq = pci_dev->irq; > + port.port.flags = UPF_SKIP_TEST | UPF_FIXED_TYPE | > UPF_BOOT_AUTOCONF | > + UPF_SHARE_IRQ; > + port.port.uartclk = 1843200; > + port.port.dev = &dev->dev; > + port.port.iobase = address; > + port.port.type = PORT_16550A; > + port.port.fifosize = 128; > + port.port.rs485_config = f81504_rs485_config; > + port.port.set_termios = f81504_set_termios; > + port.tx_loadsz = 32; > + 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). > +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. > + .pm     = &f81504_serial_pm_ops, > + }, > > --- a/drivers/tty/serial/8250/Kconfig > +++ b/drivers/tty/serial/8250/Kconfig > @@ -116,6 +116,17 @@ config SERIAL_8250_PCI >     Note that serial ports on NetMos 9835 Multi-I/O cards are > handled >     by the parport_serial driver, enabled with > CONFIG_PARPORT_SERIAL. >   > +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. -- Andy Shevchenko Intel Finland Oy