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.

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,
+	},

It seems RATIONAL API is not used here.


-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy