Re: [PATCH V3 3/4] 8250: 8250_f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO UART support

[Andy Shevchenko <andriy.shevchenko@linux.intel.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