From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Peter Hung <hpeter@gmail.com>,
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 <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V3 3/4] 8250: 8250_f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO UART support
Date: Tue, 16 Feb 2016 11:11:39 +0200 [thread overview]
Message-ID: <1455613899.31169.149.camel@linux.intel.com> (raw)
In-Reply-To: <1455605710-3724-4-git-send-email-hpeter+linux_kernel@gmail.com>
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 <linux/pci.h>
> +#include <linux/serial_8250.h>
> +#include <linux/module.h>
> +#include <linux/version.h>
> +#include <linux/mfd/f81504.h>
> +
> +#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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
next prev parent reply other threads:[~2016-02-16 9:13 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-16 6:55 [PATCH V3 0/4] Transform Fintek PCIE driver from 8250 to MFD Peter Hung
2016-02-16 6:55 ` [PATCH V3 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support Peter Hung
2016-02-16 6:55 ` [PATCH V3 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support Peter Hung
2016-02-16 15:22 ` Linus Walleij
2016-02-17 10:13 ` Peter Hung
2016-02-16 6:55 ` [PATCH V3 3/4] 8250: 8250_f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO UART support Peter Hung
2016-02-16 9:11 ` Andy Shevchenko [this message]
2016-02-17 9:30 ` Peter Hung
2016-02-16 6:55 ` [PATCH V3 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver Peter Hung
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1455613899.31169.149.camel@linux.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=adam.lee@canonical.com \
--cc=arnd@arndb.de \
--cc=gnomes@lxorguk.ukuu.org.uk \
--cc=gnurou@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=hpeter+linux_kernel@gmail.com \
--cc=hpeter@gmail.com \
--cc=jslaby@suse.com \
--cc=lee.jones@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=manabian@gmail.com \
--cc=mans@mansr.com \
--cc=matthias.bgg@gmail.com \
--cc=paul.burton@imgtec.com \
--cc=paul.gortmaker@windriver.com \
--cc=peter@hurleysoftware.com \
--cc=peter_hong@fintek.com.tw \
--cc=ralf@linux-mips.org \
--cc=scottwood@freescale.com \
--cc=soeren.grunewald@desy.de \
--cc=tom_tsai@fintek.com.tw \
--cc=udknight@gmail.com \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).