[PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Peter Hung]

Fintek F81504/508/512 is a multi-functional PCIE device. It contains
GPIO and serial port with high baudrate & RTS auto direction for RS485.

The serial ports support from 50bps to 1.5Mbps with Linux baudrate
define excluding 1.0Mbps due to not support 16MHz clock source.

IC function list:
	F81504: Max 2x8 GPIOs and max 4 serial ports
			port2/3 are multi-function
	F81508: Max 6x8 GPIOs and max 8 serial ports
			port2/3 are multi-function, port8/9/10/11 are gpio only
	F81512: Max 6x8 GPIOs and max 12 serial ports
			port2/3/8/9/10/11 are multi-function

We'll spilt from 8250_pci.c to new file 8250_fintek_pci.c and make it
as a kernel module with first & second patch, implements GPIOLIB with
third patch. 

Peter Hung (3):
  serial: 8250_pci: Remove Fintek PCIE UART driver
  8250_fintek_pci: Add Fintek PCIE UART driver
  8250_fintek_pci: Add GPIOLIB support

 drivers/tty/serial/8250/8250_fintek_pci.c | 767 ++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_pci.c        | 201 --------
 drivers/tty/serial/8250/Kconfig           |   9 +
 drivers/tty/serial/8250/Makefile          |   1 +
 4 files changed, 777 insertions(+), 201 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_fintek_pci.c

-- 
1.9.1

[PATCH 1/3] serial: 8250_pci: Remove Fintek PCIE UART driver

[Peter Hung]

Remove Fintek F81504/508/512 PCIE-to-UART device driver from 8250_pci.c

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_pci.c | 201 -------------------------------------
 1 file changed, 201 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 4097f3f..0eeb4a3 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1527,156 +1527,6 @@ pci_brcm_trumanage_setup(struct serial_private *priv,
 	return ret;
 }
 
-/* RTS will control by MCR if this bit is 0 */
-#define FINTEK_RTS_CONTROL_BY_HW	BIT(4)
-/* only worked with FINTEK_RTS_CONTROL_BY_HW on */
-#define FINTEK_RTS_INVERT		BIT(5)
-
-/* We should do proper H/W transceiver setting before change to RS485 mode */
-static int pci_fintek_rs485_config(struct uart_port *port,
-			       struct serial_rs485 *rs485)
-{
-	u8 setting;
-	u8 *index = (u8 *) port->private_data;
-	struct pci_dev *pci_dev = container_of(port->dev, struct pci_dev,
-						dev);
-
-	pci_read_config_byte(pci_dev, 0x40 + 8 * *index + 7, &setting);
-
-	if (!rs485)
-		rs485 = &port->rs485;
-	else if (rs485->flags & SER_RS485_ENABLED)
-		memset(rs485->padding, 0, sizeof(rs485->padding));
-	else
-		memset(rs485, 0, sizeof(*rs485));
-
-	/* F81504/508/512 not support RTS delay before or after send */
-	rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND;
-
-	if (rs485->flags & SER_RS485_ENABLED) {
-		/* Enable RTS H/W control mode */
-		setting |= FINTEK_RTS_CONTROL_BY_HW;
-
-		if (rs485->flags & SER_RS485_RTS_ON_SEND) {
-			/* RTS driving high on TX */
-			setting &= ~FINTEK_RTS_INVERT;
-		} else {
-			/* RTS driving low on TX */
-			setting |= FINTEK_RTS_INVERT;
-		}
-
-		rs485->delay_rts_after_send = 0;
-		rs485->delay_rts_before_send = 0;
-	} else {
-		/* Disable RTS H/W control mode */
-		setting &= ~(FINTEK_RTS_CONTROL_BY_HW | FINTEK_RTS_INVERT);
-	}
-
-	pci_write_config_byte(pci_dev, 0x40 + 8 * *index + 7, setting);
-
-	if (rs485 != &port->rs485)
-		port->rs485 = *rs485;
-
-	return 0;
-}
-
-static int pci_fintek_setup(struct serial_private *priv,
-			    const struct pciserial_board *board,
-			    struct uart_8250_port *port, int idx)
-{
-	struct pci_dev *pdev = priv->dev;
-	u8 *data;
-	u8 config_base;
-	u16 iobase;
-
-	config_base = 0x40 + 0x08 * idx;
-
-	/* Get the io address from configuration space */
-	pci_read_config_word(pdev, config_base + 4, &iobase);
-
-	dev_dbg(&pdev->dev, "%s: idx=%d iobase=0x%x", __func__, idx, iobase);
-
-	port->port.iotype = UPIO_PORT;
-	port->port.iobase = iobase;
-	port->port.rs485_config = pci_fintek_rs485_config;
-
-	data = devm_kzalloc(&pdev->dev, sizeof(u8), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	/* preserve index in PCI configuration space */
-	*data = idx;
-	port->port.private_data = data;
-
-	return 0;
-}
-
-static int pci_fintek_init(struct pci_dev *dev)
-{
-	unsigned long iobase;
-	u32 max_port, i;
-	u32 bar_data[3];
-	u8 config_base;
-	struct serial_private *priv = pci_get_drvdata(dev);
-	struct uart_8250_port *port;
-
-	switch (dev->device) {
-	case 0x1104: /* 4 ports */
-	case 0x1108: /* 8 ports */
-		max_port = dev->device & 0xff;
-		break;
-	case 0x1112: /* 12 ports */
-		max_port = 12;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	/* Get the io address dispatch from the BIOS */
-	pci_read_config_dword(dev, 0x24, &bar_data[0]);
-	pci_read_config_dword(dev, 0x20, &bar_data[1]);
-	pci_read_config_dword(dev, 0x1c, &bar_data[2]);
-
-	for (i = 0; i < max_port; ++i) {
-		/* UART0 configuration offset start from 0x40 */
-		config_base = 0x40 + 0x08 * i;
-
-		/* Calculate Real IO Port */
-		iobase = (bar_data[i / 4] & 0xffffffe0) + (i % 4) * 8;
-
-		/* Enable UART I/O port */
-		pci_write_config_byte(dev, config_base + 0x00, 0x01);
-
-		/* Select 128-byte FIFO and 8x FIFO threshold */
-		pci_write_config_byte(dev, config_base + 0x01, 0x33);
-
-		/* LSB UART */
-		pci_write_config_byte(dev, config_base + 0x04,
-				(u8)(iobase & 0xff));
-
-		/* MSB UART */
-		pci_write_config_byte(dev, config_base + 0x05,
-				(u8)((iobase & 0xff00) >> 8));
-
-		pci_write_config_byte(dev, config_base + 0x06, dev->irq);
-
-		if (priv) {
-			/* re-apply RS232/485 mode when
-			 * pciserial_resume_ports()
-			 */
-			port = serial8250_get_port(priv->line[i]);
-			pci_fintek_rs485_config(&port->port, NULL);
-		} else {
-			/* First init without port data
-			 * force init to RS232 Mode
-			 */
-			pci_write_config_byte(dev, config_base + 0x07, 0x01);
-		}
-	}
-
-	return max_port;
-}
-
 static int skip_tx_en_setup(struct serial_private *priv,
 			const struct pciserial_board *board,
 			struct uart_8250_port *port, int idx)
@@ -2707,31 +2557,6 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
 		.subdevice	= PCI_ANY_ID,
 		.setup		= pci_brcm_trumanage_setup,
 	},
-	{
-		.vendor		= 0x1c29,
-		.device		= 0x1104,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= pci_fintek_setup,
-		.init		= pci_fintek_init,
-	},
-	{
-		.vendor		= 0x1c29,
-		.device		= 0x1108,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= pci_fintek_setup,
-		.init		= pci_fintek_init,
-	},
-	{
-		.vendor		= 0x1c29,
-		.device		= 0x1112,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= pci_fintek_setup,
-		.init		= pci_fintek_init,
-	},
-
 	/*
 	 * Default "match everything" terminator entry
 	 */
@@ -2933,9 +2758,6 @@ enum pci_board_num_t {
 	pbn_omegapci,
 	pbn_NETMOS9900_2s_115200,
 	pbn_brcm_trumanage,
-	pbn_fintek_4,
-	pbn_fintek_8,
-	pbn_fintek_12,
 	pbn_wch384_4,
 	pbn_pericom_PI7C9X7951,
 	pbn_pericom_PI7C9X7952,
@@ -3738,24 +3560,6 @@ static struct pciserial_board pci_boards[] = {
 		.reg_shift	= 2,
 		.base_baud	= 115200,
 	},
-	[pbn_fintek_4] = {
-		.num_ports	= 4,
-		.uart_offset	= 8,
-		.base_baud	= 115200,
-		.first_offset	= 0x40,
-	},
-	[pbn_fintek_8] = {
-		.num_ports	= 8,
-		.uart_offset	= 8,
-		.base_baud	= 115200,
-		.first_offset	= 0x40,
-	},
-	[pbn_fintek_12] = {
-		.num_ports	= 12,
-		.uart_offset	= 8,
-		.base_baud	= 115200,
-		.first_offset	= 0x40,
-	},
 	[pbn_wch384_4] = {
 		.flags		= FL_BASE0,
 		.num_ports	= 4,
@@ -5581,11 +5385,6 @@ static struct pci_device_id serial_pci_tbl[] = {
 		0,
 		0, pbn_exar_XR17V358 },
 
-	/* Fintek PCI serial cards */
-	{ PCI_DEVICE(0x1c29, 0x1104), .driver_data = pbn_fintek_4 },
-	{ PCI_DEVICE(0x1c29, 0x1108), .driver_data = pbn_fintek_8 },
-	{ PCI_DEVICE(0x1c29, 0x1112), .driver_data = pbn_fintek_12 },
-
 	/*
 	 * These entries match devices with class COMMUNICATION_SERIAL,
 	 * COMMUNICATION_MODEM or COMMUNICATION_MULTISERIAL
-- 
1.9.1

[PATCH 2/3] 8250_fintek_pci: Add Fintek PCIE UART driver

[Peter Hung]

Add Fintek F81504/508/512 PCIE-to-UART driver 8250_fintek_pci.c

Our device driver had already merge into 8250_pci.c, but it's only
implemented basic UART function. This patch will add new file
8250_fintek_pci.c to implements serial ports and high baudrate
functions.

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek_pci.c | 417 ++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig           |   9 +
 drivers/tty/serial/8250/Makefile          |   1 +
 3 files changed, 427 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_fintek_pci.c

diff --git a/drivers/tty/serial/8250/8250_fintek_pci.c b/drivers/tty/serial/8250/8250_fintek_pci.c
new file mode 100644
index 0000000..5d9ea01a
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_fintek_pci.c
@@ -0,0 +1,417 @@
+/*
+ *  Base port operations for Fintek F81504/508/512 PCI-to-UARTs 16550A-type
+ *  serial ports
+ */
+#include <linux/pci.h>
+#include <linux/serial_8250.h>
+#include <linux/module.h>
+#include "8250.h"
+
+#define FINTEK_VID		0x1c29
+#define FINTEK_F81504		0x1104
+#define FINTEK_F81508		0x1108
+#define FINTEK_F81512		0x1112
+
+#define FINTEK_MAX_PORT		12
+#define DRIVER_NAME		"f81504_serial"
+#define DEV_DESC		"Fintek F81504/508/512 PCIE-to-UART"
+
+#define UART_START_ADDR		0x40
+#define UART_MODE_OFFSET	0x07
+#define UART_OFFSET		0x08
+
+/* RTS will control by MCR if this bit is 0 */
+#define RTS_CONTROL_BY_HW	BIT(4)
+/* only worked with FINTEK_RTS_CONTROL_BY_HW on */
+#define RTS_INVERT		BIT(5)
+
+#define CLOCK_RATE_MASK		0xc0
+#define CLKSEL_1DOT846_MHZ	0x00
+#define CLKSEL_18DOT46_MHZ	0x40
+#define CLKSEL_24_MHZ		0x80
+#define CLKSEL_14DOT77_MHZ	0xc0
+
+#define IRQSEL_REG		0xb8
+
+static u32 baudrate_table[] = { 1500000, 1152000, 921600 };
+static u8 clock_table[] = { CLKSEL_24_MHZ, CLKSEL_18DOT46_MHZ,
+		CLKSEL_14DOT77_MHZ };
+
+struct f81504_pci_private {
+	int line[FINTEK_MAX_PORT];
+	u32 uart_count;
+};
+
+/* 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;
+	struct pci_dev *pci_dev = container_of(port->dev, struct pci_dev, dev);
+
+	pci_read_config_byte(pci_dev, UART_START_ADDR + UART_OFFSET * *index +
+			UART_MODE_OFFSET, &setting);
+
+	if (!rs485)
+		rs485 = &port->rs485;
+	else if (rs485->flags & SER_RS485_ENABLED)
+		memset(rs485->padding, 0, sizeof(rs485->padding));
+	else
+		memset(rs485, 0, sizeof(*rs485));
+
+	/* F81504/508/512 not support RTS delay before or after send */
+	rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND;
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		/* Enable RTS H/W control mode */
+		setting |= RTS_CONTROL_BY_HW;
+
+		if (rs485->flags & SER_RS485_RTS_ON_SEND) {
+			/* RTS driving high on TX */
+			setting &= ~RTS_INVERT;
+		} else {
+			/* RTS driving low on TX */
+			setting |= RTS_INVERT;
+		}
+
+		rs485->delay_rts_after_send = 0;
+		rs485->delay_rts_before_send = 0;
+	} else {
+		/* Disable RTS H/W control mode */
+		setting &= ~(RTS_CONTROL_BY_HW | RTS_INVERT);
+	}
+
+	pci_write_config_byte(pci_dev, UART_START_ADDR + UART_OFFSET * *index +
+			UART_MODE_OFFSET, setting);
+
+	if (rs485 != &port->rs485)
+		port->rs485 = *rs485;
+
+	return 0;
+}
+
+static int f81504_check_baudrate(u32 baud, int *idx)
+{
+	size_t index;
+	u32 quot, rem;
+
+	for (index = 0; index < ARRAY_SIZE(baudrate_table); ++index) {
+		/* Clock source must largeer than desire baudrate */
+		if (baud > baudrate_table[index])
+			continue;
+
+		quot = DIV_ROUND_CLOSEST(baudrate_table[index], baud);
+		/* 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 pci_dev *dev = container_of(port->dev, struct pci_dev, dev);
+	unsigned int baud = tty_termios_baud_rate(termios);
+	u8 tmp, *offset = (u8 *)port->private_data;
+	int i;
+
+	do {
+		/* read current clock source (masked with CLOCK_RATE_MASK) */
+		pci_read_config_byte(dev, UART_START_ADDR + *offset *
+				UART_OFFSET, &tmp);
+
+		if (baud <= 115200) {
+			/*
+			 * direct use 1.8432MHz when baudrate smaller then or
+			 * equal 115200bps
+			 */
+			port->uartclk = 115200 * 16;
+			pci_write_config_byte(dev, UART_START_ADDR + *offset *
+					UART_OFFSET, tmp & ~CLOCK_RATE_MASK);
+			break;
+		}
+
+		if (!f81504_check_baudrate(baud, &i)) {
+			/* had optimize value */
+			port->uartclk = baudrate_table[i] * 16;
+			tmp = (tmp & ~CLOCK_RATE_MASK) | clock_table[i];
+			pci_write_config_byte(dev, UART_START_ADDR + *offset *
+					UART_OFFSET, tmp);
+			break;
+		}
+
+		if (old && !f81504_check_baudrate(tty_termios_baud_rate(old),
+				NULL)) {
+			/*
+			 * If it can't found suitable clock source but had old
+			 * accpetable baudrate, we'll use it
+			 */
+			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
+			 */
+			baud = 115200;
+		}
+
+		if (tty_termios_baud_rate(termios))
+			tty_termios_encode_baud_rate(termios, baud, baud);
+	} while (1);
+
+	serial8250_do_set_termios(port, termios, old);
+}
+
+static int f81504_register_port(struct pci_dev *dev, unsigned long address,
+				int idx)
+{
+	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.mapbase = 0;
+	port.port.membase = NULL;
+	port.port.regshift = 0;
+	port.port.irq = dev->irq;
+	port.port.flags = UPF_SKIP_TEST | UPF_FIXED_TYPE | UPF_BOOT_AUTOCONF |
+			UPF_SHARE_IRQ;
+	port.port.uartclk = 115200 * 16;
+	port.port.dev = &dev->dev;
+	port.port.iobase = address;
+	port.port.type = PORT_16550A;
+	port.port.fifosize = 128;
+	port.tx_loadsz = 32;
+	port.port.private_data = data;	/* save current idx */
+	port.port.set_termios = f81504_set_termios;
+	port.port.rs485_config = f81504_rs485_config;
+
+	return serial8250_register_8250_port(&port);
+}
+
+static int f81504_port_init(struct pci_dev *dev)
+{
+	size_t i;
+	u32 max_port, iobase;
+	u32 bar_data[3];
+	u16 tmp;
+	u8 config_base;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct uart_8250_port *port;
+
+	switch (dev->device) {
+	case FINTEK_F81504: /* 4 ports */
+	case FINTEK_F81508: /* 8 ports */
+		max_port = dev->device & 0xff;
+		break;
+	case FINTEK_F81512: /* 12 ports */
+		max_port = 12;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Get the UART IO address dispatch from the BIOS */
+	pci_read_config_dword(dev, 0x24, &bar_data[0]);
+	pci_read_config_dword(dev, 0x20, &bar_data[1]);
+	pci_read_config_dword(dev, 0x1c, &bar_data[2]);
+
+	/* Compatible for newer step IC */
+	pci_read_config_word(dev, IRQSEL_REG, &tmp);
+	tmp |= BIT(8);
+	pci_write_config_word(dev, IRQSEL_REG, tmp);
+
+	for (i = 0; i < max_port; ++i) {
+		/* UART0 configuration offset start from 0x40 */
+		config_base = UART_START_ADDR + UART_OFFSET * i;
+
+		/* Calculate Real IO Port */
+		iobase = (bar_data[i / 4] & 0xffffffe0) + (i % 4) * 8;
+
+		/* Enable UART I/O port */
+		pci_write_config_byte(dev, config_base + 0x00, 0x01);
+
+		/* Select 128-byte FIFO and 8x FIFO threshold */
+		pci_write_config_byte(dev, config_base + 0x01, 0x33);
+
+		/* LSB UART */
+		pci_write_config_byte(dev, config_base + 0x04,
+				(u8)(iobase & 0xff));
+
+		/* MSB UART */
+		pci_write_config_byte(dev, config_base + 0x05,
+				(u8)((iobase & 0xff00) >> 8));
+
+		pci_write_config_byte(dev, config_base + 0x06, dev->irq);
+
+		/* First init. force init to RS232 Mode */
+		pci_write_config_byte(dev, config_base + 0x07, 0x01);
+	}
+
+	if (!priv)
+		return 0;
+
+	/* re-apply RS232/485 mode when f81504_resume() */
+	for (i = 0; i < priv->uart_count; ++i) {
+		port = serial8250_get_port(priv->line[i]);
+		f81504_rs485_config(&port->port, NULL);
+	}
+
+	return 0;
+}
+
+static int f81504_probe(struct pci_dev *dev, const struct pci_device_id
+				*dev_id)
+{
+	int status;
+	size_t i;
+	u16 iobase;
+	u8 tmp;
+	struct f81504_pci_private *priv;
+
+	status = pci_enable_device(dev);
+	if (status)
+		return status;
+
+	/* Init PCI Configuration Space */
+	status = f81504_port_init(dev);
+	if (status)
+		return status;
+
+	priv = devm_kzalloc(&dev->dev, sizeof(struct f81504_pci_private),
+				GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	pci_set_drvdata(dev, priv);
+
+	/* Generate UART Ports */
+	for (i = 0; i < dev_id->driver_data; ++i) {
+		/* Check UART is enabled */
+		pci_read_config_byte(dev, UART_START_ADDR + i * UART_OFFSET,
+					&tmp);
+		if (!tmp)
+			continue;
+
+		/* Get UART IO Address */
+		pci_read_config_word(dev, UART_START_ADDR + i * UART_OFFSET +
+					4, &iobase);
+
+		/* Register to serial port */
+		priv->line[priv->uart_count] = f81504_register_port(dev,
+								iobase, i);
+		++priv->uart_count;
+	}
+
+	return 0;
+}
+
+static void f81504_remove(struct pci_dev *dev)
+{
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	size_t i;
+
+	for (i = 0; i < priv->uart_count; ++i) {
+		if (priv->line[i] < 0)
+			continue;
+
+		serial8250_unregister_port(priv->line[i]);
+	}
+
+	pci_disable_device(dev);
+}
+
+#ifdef CONFIG_PM
+static int f81504_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	size_t i;
+	int status;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+
+	status = pci_save_state(dev);
+	if (status)
+		return status;
+
+	status = pci_set_power_state(dev, pci_choose_state(dev, state));
+	if (status)
+		return status;
+
+	for (i = 0; i < priv->uart_count; ++i) {
+		if (priv->line[i] < 0)
+			continue;
+
+		serial8250_suspend_port(priv->line[i]);
+	}
+
+	return 0;
+}
+
+static int f81504_resume(struct pci_dev *dev)
+{
+	size_t i;
+	int status;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+
+	status = pci_set_power_state(dev, PCI_D0);
+	if (status)
+		return status;
+
+	pci_restore_state(dev);
+
+	/* Init PCI Configuration Space */
+	status = f81504_port_init(dev);
+	if (status)
+		return status;
+
+	for (i = 0; i < priv->uart_count; ++i) {
+		if (priv->line[i] < 0)
+			continue;
+
+		serial8250_resume_port(priv->line[i]);
+	}
+
+	return 0;
+}
+#else
+#define f81504_suspend NULL
+#define f81504_resume NULL
+#endif
+
+static const struct pci_device_id f81504_dev_table[] = {
+	/* Fintek PCI serial cards */
+	{PCI_DEVICE(FINTEK_VID, FINTEK_F81504), .driver_data = 4},
+	{PCI_DEVICE(FINTEK_VID, FINTEK_F81508), .driver_data = 8},
+	{PCI_DEVICE(FINTEK_VID, FINTEK_F81512), .driver_data = 12},
+	{}
+};
+
+MODULE_DEVICE_TABLE(pci, f81504_dev_table);
+
+static struct pci_driver f81504_driver = {
+	.name = DRIVER_NAME,
+	.probe = f81504_probe,
+	.remove = f81504_remove,
+	.suspend = f81504_suspend,
+	.resume = f81504_resume,
+	.id_table = f81504_dev_table,
+};
+
+module_pci_driver(f81504_driver);
+
+MODULE_DESCRIPTION(DEV_DESC);
+MODULE_AUTHOR("Peter Hong <Peter_Hong@fintek.com.tw>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index b03cb517..0533a6b 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -116,6 +116,15 @@ 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_FINTEK_PCI
+        tristate "Fintek F81504/508/512 PCI-E to Multi Serial Ports support"
+        depends on SERIAL_8250 && PCI
+        default m
+        help
+          This driver works for Fintek F81504/508/512 PCIE-to-UART/GPIO
+          multi-function device. We'll use GPIOLIB interface to control
+          GPIOs if your card vendor reserve some pin as gpio.
+
 config SERIAL_8250_HP300
 	tristate
 	depends on SERIAL_8250 && HP300
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index b9b9bca..83ed91f 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_SERIAL_8250)		+= 8250.o 8250_base.o
 8250_base-$(CONFIG_SERIAL_8250_DMA)	+= 8250_dma.o
 obj-$(CONFIG_SERIAL_8250_GSC)		+= 8250_gsc.o
 obj-$(CONFIG_SERIAL_8250_PCI)		+= 8250_pci.o
+obj-$(CONFIG_SERIAL_8250_FINTEK_PCI)	+= 8250_fintek_pci.o
 obj-$(CONFIG_SERIAL_8250_HP300)		+= 8250_hp300.o
 obj-$(CONFIG_SERIAL_8250_CS)		+= serial_cs.o
 obj-$(CONFIG_SERIAL_8250_ACORN)		+= 8250_acorn.o
-- 
1.9.1

[PATCH 3/3] 8250_fintek_pci: Add GPIOLIB support

[Peter Hung]

Add GPIOLIB support for F81504/508/512 Multi-Functional PCIE device

F81504: Max 2x8 GPIOs and max 4 serial port
        port2/3 are multi-function
F81508: Max 6x8 GPIOs and max 8 serial port
        port2/3 are multi-function, port8/9/10/11 are gpio only
F81512: Max 6x8 GPIOs and max 12 serial port
        port2/3/8/9/10/11 are multi-function

The mode is controlled via PCI configuration space F0h & F3h. Customers
can use EEPROM or BIOS to override register value. The driver will save
and configurate in f81504_port_init().

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek_pci.c | 361 +++++++++++++++++++++++++++++-
 1 file changed, 359 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_fintek_pci.c b/drivers/tty/serial/8250/8250_fintek_pci.c
index 5d9ea01a..08c320c 100644
--- a/drivers/tty/serial/8250/8250_fintek_pci.c
+++ b/drivers/tty/serial/8250/8250_fintek_pci.c
@@ -4,6 +4,7 @@
  */
 #include <linux/pci.h>
 #include <linux/serial_8250.h>
+#include <linux/gpio.h>
 #include <linux/module.h>
 #include "8250.h"
 
@@ -13,8 +14,11 @@
 #define FINTEK_F81512		0x1112
 
 #define FINTEK_MAX_PORT		12
+#define FINTEK_MAX_GPIO_SET	6
+#define FINTEK_GPIO_MAX_NAME	32
 #define DRIVER_NAME		"f81504_serial"
 #define DEV_DESC		"Fintek F81504/508/512 PCIE-to-UART"
+#define GPIO_DISPLAY_NAME	"GPIO"
 
 #define UART_START_ADDR		0x40
 #define UART_MODE_OFFSET	0x07
@@ -25,6 +29,16 @@
 /* only worked with FINTEK_RTS_CONTROL_BY_HW on */
 #define RTS_INVERT		BIT(5)
 
+#define GPIO_ENABLE_REG		0xf0
+#define GPIO_IO_LSB_REG		0xf1
+#define GPIO_IO_MSB_REG		0xf2
+#define PIN_SET_MODE_REG	0xf3
+
+#define GPIO_START_ADDR		0xf8
+#define GPIO_OUT_EN_OFFSET	0x00
+#define GPIO_DRIVE_EN_OFFSET	0x01
+#define GPIO_SET_OFFSET		0x08
+
 #define CLOCK_RATE_MASK		0xc0
 #define CLKSEL_1DOT846_MHZ	0x00
 #define CLKSEL_18DOT46_MHZ	0x40
@@ -36,12 +50,261 @@
 static u32 baudrate_table[] = { 1500000, 1152000, 921600 };
 static u8 clock_table[] = { CLKSEL_24_MHZ, CLKSEL_18DOT46_MHZ,
 		CLKSEL_14DOT77_MHZ };
+static u8 fintek_gpio_mapping[FINTEK_MAX_GPIO_SET] = { 2, 3, 8, 9, 10, 11 };
 
 struct f81504_pci_private {
 	int line[FINTEK_MAX_PORT];
 	u32 uart_count;
+	u32 gpio_count;
+	u16 gpio_ioaddr;
+	u8 f0_gpio_flag;
+	struct mutex locker;
+#ifdef CONFIG_GPIOLIB
+	struct f81504_gpio_set {
+		struct gpio_chip chip;
+		u8 idx;
+		u8 save_out_en;
+		u8 save_drive_en;
+		u8 save_value;
+	} gpio_set[FINTEK_MAX_GPIO_SET];
+#endif
 };
 
+#ifdef CONFIG_GPIOLIB
+static struct f81504_gpio_set *gpio_to_f81504_chip(struct gpio_chip *gc)
+{
+	return container_of(gc, struct f81504_gpio_set, chip);
+}
+
+static int f81504_gpio_get(struct gpio_chip *chip, unsigned gpio_num)
+{
+	int tmp;
+	struct pci_dev *dev = container_of(chip->dev, struct pci_dev, dev);
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct f81504_gpio_set *set = gpio_to_f81504_chip(chip);
+
+	mutex_lock(&priv->locker);
+	tmp = inb(priv->gpio_ioaddr + set->idx);
+	mutex_unlock(&priv->locker);
+
+	return !!(tmp & BIT(gpio_num));
+}
+
+static int f81504_gpio_direction_in(struct gpio_chip *chip, unsigned gpio_num)
+{
+	u8 tmp;
+	struct pci_dev *dev = container_of(chip->dev, struct pci_dev, dev);
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct f81504_gpio_set *set = gpio_to_f81504_chip(chip);
+
+	mutex_lock(&priv->locker);
+
+	/* set input mode */
+	pci_read_config_byte(dev, GPIO_START_ADDR + set->idx *
+			GPIO_SET_OFFSET + GPIO_OUT_EN_OFFSET, &tmp);
+	pci_write_config_byte(dev, GPIO_START_ADDR + set->idx *
+			GPIO_SET_OFFSET + GPIO_OUT_EN_OFFSET,
+			tmp & ~BIT(gpio_num));
+
+	mutex_unlock(&priv->locker);
+	return 0;
+}
+
+static int f81504_gpio_direction_out(struct gpio_chip *chip,
+				     unsigned gpio_num, int val)
+{
+	u8 tmp;
+	struct pci_dev *dev = container_of(chip->dev, struct pci_dev, dev);
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct f81504_gpio_set *set = gpio_to_f81504_chip(chip);
+
+	mutex_lock(&priv->locker);
+
+	/* set output mode */
+	pci_read_config_byte(dev, GPIO_START_ADDR + set->idx *
+			GPIO_SET_OFFSET + GPIO_OUT_EN_OFFSET, &tmp);
+	pci_write_config_byte(dev, GPIO_START_ADDR + set->idx *
+			GPIO_SET_OFFSET + GPIO_OUT_EN_OFFSET,
+			tmp | BIT(gpio_num));
+
+	/*
+	 * The GPIO default driven mode for this device is open-drain. The
+	 * GPIOLIB had no change GPIO mode API currently. So we leave the
+	 * Push-Pull code below.
+	 *
+	 * pci_read_config_byte(dev, GPIO_START_ADDR + idx * GPIO_SET_OFFSET +
+	 *			GPIO_DRIVE_EN_OFFSET, &tmp);
+	 * pci_write_config_byte(dev, GPIO_START_ADDR + idx * GPIO_SET_OFFSET +
+	 *			GPIO_DRIVE_EN_OFFSET, tmp | BIT(gpio_num));
+	 */
+
+	/* set output data */
+	tmp = inb(priv->gpio_ioaddr + set->idx);
+
+	if (val)
+		outb(tmp | BIT(gpio_num), priv->gpio_ioaddr + set->idx);
+	else
+		outb(tmp & ~BIT(gpio_num), priv->gpio_ioaddr + set->idx);
+
+	mutex_unlock(&priv->locker);
+
+	return 0;
+}
+
+static void f81504_gpio_set(struct gpio_chip *chip, unsigned gpio_num, int val)
+{
+	f81504_gpio_direction_out(chip, gpio_num, val);
+}
+
+static int f81504_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+	u8 tmp;
+	struct pci_dev *dev = container_of(chip->dev, struct pci_dev, dev);
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct f81504_gpio_set *set = gpio_to_f81504_chip(chip);
+
+	mutex_lock(&priv->locker);
+	pci_read_config_byte(dev, GPIO_START_ADDR + set->idx * GPIO_SET_OFFSET,
+				&tmp);
+	mutex_unlock(&priv->locker);
+
+	if (tmp & BIT(offset))
+		return GPIOF_DIR_OUT;
+
+	return GPIOF_DIR_IN;
+}
+
+static void f81504_save_gpio_config(struct pci_dev *dev)
+{
+	size_t i;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct f81504_gpio_set *set;
+
+	mutex_lock(&priv->locker);
+
+	for (i = 0; i < priv->gpio_count; ++i) {
+		set = &priv->gpio_set[i];
+
+		pci_read_config_byte(dev, GPIO_START_ADDR + set->idx *
+				GPIO_SET_OFFSET + GPIO_OUT_EN_OFFSET,
+				&set->save_out_en);
+
+		pci_read_config_byte(dev, GPIO_START_ADDR + set->idx *
+				GPIO_SET_OFFSET + GPIO_DRIVE_EN_OFFSET,
+				&set->save_drive_en);
+
+		set->save_value = inb(priv->gpio_ioaddr + set->idx);
+	}
+
+	mutex_unlock(&priv->locker);
+}
+
+static void f81504_restore_gpio_config(struct pci_dev *dev)
+{
+	size_t i;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct f81504_gpio_set *set;
+
+	mutex_lock(&priv->locker);
+
+	for (i = 0; i < priv->gpio_count; ++i) {
+		set = &priv->gpio_set[i];
+
+		pci_write_config_byte(dev, GPIO_START_ADDR + set->idx *
+				GPIO_SET_OFFSET + GPIO_OUT_EN_OFFSET,
+				set->save_out_en);
+
+		pci_write_config_byte(dev, GPIO_START_ADDR + set->idx *
+				GPIO_SET_OFFSET + GPIO_DRIVE_EN_OFFSET,
+				set->save_drive_en);
+
+		outb(set->save_value, priv->gpio_ioaddr + set->idx);
+	}
+
+	mutex_unlock(&priv->locker);
+}
+
+static int f81504_prepage_gpiolib(struct pci_dev *dev)
+{
+	size_t i;
+	int status;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct f81504_gpio_set *set;
+	char *name;
+
+	for (i = 0; i < FINTEK_MAX_GPIO_SET; ++i) {
+		if (!(priv->f0_gpio_flag & BIT(i)))
+			continue;
+
+		/* F81504 had max 2 sets GPIO */
+		if (dev->device == FINTEK_F81504 && i >= 2)
+			break;
+
+		name = devm_kzalloc(&dev->dev, FINTEK_GPIO_MAX_NAME,
+					GFP_KERNEL);
+		if (!name) {
+			status = -ENOMEM;
+			goto failed;
+		}
+
+		sprintf(name, "%s-%zu", GPIO_DISPLAY_NAME, i);
+		set = &priv->gpio_set[priv->gpio_count];
+
+		set->chip.owner = THIS_MODULE;
+		set->chip.label = name;
+		set->chip.ngpio = 8;
+		set->chip.dev = &dev->dev;
+		set->chip.get = f81504_gpio_get;
+		set->chip.set = f81504_gpio_set;
+		set->chip.direction_input = f81504_gpio_direction_in;
+		set->chip.direction_output = f81504_gpio_direction_out;
+		set->chip.get_direction = f81504_gpio_get_direction;
+		set->chip.base = -1;
+		set->idx = i;
+
+		status = gpiochip_add(&set->chip);
+		if (status)
+			goto failed;
+
+		++priv->gpio_count;
+	}
+
+	return 0;
+
+failed:
+	for (i = 0; i < priv->gpio_count; ++i)
+		gpiochip_remove(&priv->gpio_set[i].chip);
+
+	return status;
+}
+
+static void f81504_remove_gpiolib(struct pci_dev *dev)
+{
+	size_t i;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+
+	for (i = 0; i < priv->gpio_count; ++i)
+		gpiochip_remove(&priv->gpio_set[i].chip);
+}
+#else
+static int f81504_prepage_gpiolib(struct pci_dev *dev)
+{
+	return 0;
+}
+
+static void f81504_remove_gpiolib(struct pci_dev *dev)
+{
+}
+
+static void f81504_save_gpio_config(struct pci_dev *dev)
+{
+}
+
+static void f81504_restore_gpio_config(struct pci_dev *dev)
+{
+}
+#endif
+
 /* 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)
@@ -205,14 +468,53 @@ static int f81504_register_port(struct pci_dev *dev, unsigned long address,
 
 static int f81504_port_init(struct pci_dev *dev)
 {
-	size_t i;
+	size_t i, j;
 	u32 max_port, iobase;
 	u32 bar_data[3];
 	u16 tmp;
-	u8 config_base;
+	u8 config_base, gpio_en, f0h_data, f3h_data;
+	bool is_gpio;
 	struct f81504_pci_private *priv = pci_get_drvdata(dev);
 	struct uart_8250_port *port;
 
+	/*
+	 * The PCI board is multi-function, some serial port can converts to
+	 * GPIO function. Customers could changes the F0/F3h values in EEPROM
+	 *
+	 * F0h bit0~5: Enable GPIO0~5
+	 *     bit6~7: Reserve
+	 *
+	 * F3h bit0~5: Multi-Functional Flag (0:GPIO/1:UART)
+	 *              bit0: UART2 pin out for UART2 / GPIO0
+	 *              bit1: UART3 pin out for UART3 / GPIO1
+	 *              bit2: UART8 pin out for UART8 / GPIO2
+	 *              bit3: UART9 pin out for UART9 / GPIO3
+	 *              bit4: UART10 pin out for UART10 / GPIO4
+	 *              bit5: UART11 pin out for UART11 / GPIO5
+	 *     bit6~7: Reserve
+	 */
+	if (priv) {
+		/* Reinit from resume(), read the previous value from priv */
+		gpio_en = priv->f0_gpio_flag;
+	} else {
+		/* Driver first init */
+		pci_read_config_byte(dev, GPIO_ENABLE_REG, &f0h_data);
+		pci_read_config_byte(dev, PIN_SET_MODE_REG, &f3h_data);
+
+		/* find the max set of GPIOs */
+		gpio_en = f0h_data | ~f3h_data;
+	}
+
+	/* rewrite GPIO setting */
+	pci_write_config_byte(dev, GPIO_ENABLE_REG, gpio_en & 0x3f);
+	pci_write_config_byte(dev, PIN_SET_MODE_REG, ~gpio_en & 0x3f);
+
+	/* Init GPIO IO Address */
+	pci_read_config_dword(dev, 0x18, &iobase);
+	iobase &= 0xffffffe0;
+	pci_write_config_byte(dev, GPIO_IO_LSB_REG, (iobase >> 0) & 0xff);
+	pci_write_config_byte(dev, GPIO_IO_MSB_REG, (iobase >> 8) & 0xff);
+
 	switch (dev->device) {
 	case FINTEK_F81504: /* 4 ports */
 	case FINTEK_F81508: /* 8 ports */
@@ -238,6 +540,31 @@ static int f81504_port_init(struct pci_dev *dev)
 	for (i = 0; i < max_port; ++i) {
 		/* UART0 configuration offset start from 0x40 */
 		config_base = UART_START_ADDR + UART_OFFSET * i;
+		is_gpio = false;
+
+		/* find every port to check is multi-function port? */
+		for (j = 0; j < ARRAY_SIZE(fintek_gpio_mapping); ++j) {
+			if (fintek_gpio_mapping[j] != i || !(gpio_en & BIT(j)))
+				continue;
+
+			/*
+			 * This port is multi-function and enabled as gpio
+			 * mode. So we'll not configure it as serial port.
+			 */
+			is_gpio = true;
+			break;
+		}
+
+		/*
+		 * If the serial port is setting to gpio mode, don't init it.
+		 * Disable the serial port for user-space application to
+		 * control.
+		 */
+		if (is_gpio) {
+			/* Disable current serial port */
+			pci_write_config_byte(dev, config_base + 0x00, 0x00);
+			continue;
+		}
 
 		/* Calculate Real IO Port */
 		iobase = (bar_data[i / 4] & 0xffffffe0) + (i % 4) * 8;
@@ -298,6 +625,16 @@ static int f81504_probe(struct pci_dev *dev, const struct pci_device_id
 		return -ENOMEM;
 
 	pci_set_drvdata(dev, priv);
+	mutex_init(&priv->locker);
+
+	/* Save the GPIO_ENABLE_REG after f81504_port_init() for future use */
+	pci_read_config_byte(dev, GPIO_ENABLE_REG, &priv->f0_gpio_flag);
+
+	/* Save GPIO IO Addr to private data */
+	pci_read_config_byte(dev, GPIO_IO_MSB_REG, &tmp);
+	priv->gpio_ioaddr = tmp << 8;
+	pci_read_config_byte(dev, GPIO_IO_LSB_REG, &tmp);
+	priv->gpio_ioaddr |= tmp;
 
 	/* Generate UART Ports */
 	for (i = 0; i < dev_id->driver_data; ++i) {
@@ -317,7 +654,23 @@ static int f81504_probe(struct pci_dev *dev, const struct pci_device_id
 		++priv->uart_count;
 	}
 
+	/* Generate GPIO Sets */
+	status = f81504_prepage_gpiolib(dev);
+	if (status)
+		goto fail;
+
 	return 0;
+
+fail:
+	for (i = 0; i < priv->uart_count; ++i) {
+		if (priv->line[i] < 0)
+			continue;
+
+		serial8250_unregister_port(priv->line[i]);
+	}
+
+	pci_disable_device(dev);
+	return status;
 }
 
 static void f81504_remove(struct pci_dev *dev)
@@ -332,6 +685,7 @@ static void f81504_remove(struct pci_dev *dev)
 		serial8250_unregister_port(priv->line[i]);
 	}
 
+	f81504_remove_gpiolib(dev);
 	pci_disable_device(dev);
 }
 
@@ -342,6 +696,8 @@ static int f81504_suspend(struct pci_dev *dev, pm_message_t state)
 	int status;
 	struct f81504_pci_private *priv = pci_get_drvdata(dev);
 
+	f81504_save_gpio_config(dev);
+
 	status = pci_save_state(dev);
 	if (status)
 		return status;
@@ -384,6 +740,7 @@ static int f81504_resume(struct pci_dev *dev)
 		serial8250_resume_port(priv->line[i]);
 	}
 
+	f81504_restore_gpio_config(dev);
 	return 0;
 }
 #else
-- 
1.9.1

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Paul Gortmaker]

[[PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file] On 19/01/2016 (Tue 10:41) Peter Hung wrote:

> Fintek F81504/508/512 is a multi-functional PCIE device. It contains
> GPIO and serial port with high baudrate & RTS auto direction for RS485.
> 

Some general high level comments (meaning I've not looked at the code in
detail, but have looked at this series) and I wonder about some issues:

> The serial ports support from 50bps to 1.5Mbps with Linux baudrate
> define excluding 1.0Mbps due to not support 16MHz clock source.

How does this differ from what was achieved or possible with the old way
of things?  What was the limitation in the existing 8250 code sharing
that required Fintek code to fork and become independent? 

How much code was just copied 8250 boilerplate vs. being a new
implementation?  The diffstat shows approx 500 lines of new code.  What
does that add vs. just copying?

Don't get me wrong -- forking workarounds for buggy hardware into
smaller workaround files and/or Kconfigs can be a win for everyone else,
but we should be clear on why we do them.

> 
> IC function list:
> 	F81504: Max 2x8 GPIOs and max 4 serial ports
> 			port2/3 are multi-function
> 	F81508: Max 6x8 GPIOs and max 8 serial ports
> 			port2/3 are multi-function, port8/9/10/11 are gpio only
> 	F81512: Max 6x8 GPIOs and max 12 serial ports
> 			port2/3/8/9/10/11 are multi-function
> 
> We'll spilt from 8250_pci.c to new file 8250_fintek_pci.c and make it
> as a kernel module with first & second patch, implements GPIOLIB with
> third patch. 

> 
> Peter Hung (3):
>   serial: 8250_pci: Remove Fintek PCIE UART driver

If someone had 8250 (PCI) builtin before, and Fintek stops working,
they will most guaranteed bisect to this commit above where you remove
support.  That is less than ideal.  We try to avoid code deletions or
Kconfig addtions that will be obvious bisect magnets.

>   8250_fintek_pci: Add Fintek PCIE UART driver

This creates a new Kconfig var. which is default=m.  How does that work
if people were using these for built-in early console support in the
past?  Are these cards universal, or should it be default=m if (...)
based on a Kconfig where this hardware exists?

>   8250_fintek_pci: Add GPIOLIB support

What does this add?  The commit log is not at all clear.  Leaving me to
ask if it does belong in the core PCI support code at all?  I honestly
don't know, since I don't know the hardware details here.  The commit
long logs could go a long way to closing this knowledge gap if the 0/N
listed the shortcomings and the 3/3 here indicated what the GPIO magic
had managed to add.

Again, this may be obvious to others, but the long logs should try and
give a hint to people on the fringe who maybe don't have all the
specific Fintek hardware details when reading the logs.

P.

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Peter Hung]

Hi Paul,

Paul Gortmaker 於 2016/1/19 上午 11:56 寫道:
>> The serial ports support from 50bps to 1.5Mbps with Linux baudrate
>> define excluding 1.0Mbps due to not support 16MHz clock source.
>
> How does this differ from what was achieved or possible with the old way
> of things?  What was the limitation in the existing 8250 code sharing
> that required Fintek code to fork and become independent?

The architecture of 8250_pci.c is good for PCIE device with 8250
compatible serial ports. We want to implement all functions of
F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
implement GPIOLIB in 8250_pci.c

Could I implement GPIOLIB within 8250_pci.c instead of a newer file?

> How much code was just copied 8250 boilerplate vs. being a new
> implementation?  The diffstat shows approx 500 lines of new code.  What
> does that add vs. just copying?

Due to this IC contains 8250-compatible ports, the most functions is
copy from fintek section of 8250_pci.c. The differences are highbaud
rate & GPIOLIB implementations.

>
> If someone had 8250 (PCI) builtin before, and Fintek stops working,
> they will most guaranteed bisect to this commit above where you remove
> support.  That is less than ideal.  We try to avoid code deletions or
> Kconfig addtions that will be obvious bisect magnets.

It can be prevented if implements GPIOLIB in 8250_pci.c.

>>    8250_fintek_pci: Add Fintek PCIE UART driver
>
> This creates a new Kconfig var. which is default=m.  How does that work
> if people were using these for built-in early console support in the
> past?  Are these cards universal, or should it be default=m if (...)
> based on a Kconfig where this hardware exists?

Thanks for point this out, for the early console I should make the
default mode to SERIAL_8250 if it need to split as a new file.

>>    8250_fintek_pci: Add GPIOLIB support
>
> What does this add?  The commit log is not at all clear.  Leaving me to
> ask if it does belong in the core PCI support code at all?  I honestly
> don't know, since I don't know the hardware details here.  The commit
> long logs could go a long way to closing this knowledge gap if the 0/N
> listed the shortcomings and the 3/3 here indicated what the GPIO magic
> had managed to add.

Sorry for the ambiguous logs. We'll implement GPIOLIB due to the
following circumstance.

Some H/W manufacturer use this IC and transform some port into GPIO
mode. The current 8250_pci.c not handle this so it maybe confuse
end-user.

> Again, this may be obvious to others, but the long logs should try and
> give a hint to people on the fringe who maybe don't have all the
> specific Fintek hardware details when reading the logs.
>

I'll try to make more sense with long long.
Thanks for your advices,
-- 
With Best Regards,
Peter Hung

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Andy Shevchenko]

On Tue, 2016-01-19 at 16:45 +0800, Peter Hung wrote:
> Hi Paul,
> 
> Paul Gortmaker 於 2016/1/19 上午 11:56 寫道:
> > > The serial ports support from 50bps to 1.5Mbps with Linux
> > > baudrate
> > > define excluding 1.0Mbps due to not support 16MHz clock source.
> > 
> > How does this differ from what was achieved or possible with the
> > old way
> > of things?  What was the limitation in the existing 8250 code
> > sharing
> > that required Fintek code to fork and become independent?
> 
> The architecture of 8250_pci.c is good for PCIE device with 8250
> compatible serial ports. We want to implement all functions of
> F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
> implement GPIOLIB in 8250_pci.c
> 
> Could I implement GPIOLIB within 8250_pci.c instead of a newer file?

Hm… So, can we stick with separate driver, or you're gonna shake for
each reviewer's comment?

> 
> > How much code was just copied 8250 boilerplate vs. being a new
> > implementation?  The diffstat shows approx 500 lines of new
> > code.  What
> > does that add vs. just copying?
> 
> Due to this IC contains 8250-compatible ports, the most functions is
> copy from fintek section of 8250_pci.c. The differences are highbaud
> rate & GPIOLIB implementations.

I agree with Paul, I think what you have done is to:

1) split out existing code to separate driver (no your changes, but
minimum necessary to this split) — one patch!
2) clean up it (at least I see the old PM code which should be
refactored)
3) enhance functionality accordingly to what you need.

> 
> > 
> > If someone had 8250 (PCI) builtin before, and Fintek stops working,
> > they will most guaranteed bisect to this commit above where you
> > remove
> > support.  That is less than ideal.  We try to avoid code deletions
> > or
> > Kconfig addtions that will be obvious bisect magnets.
> 
> It can be prevented if implements GPIOLIB in 8250_pci.c.

Yeah, see item 1) above.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[One Thousand Gnomes]

> The architecture of 8250_pci.c is good for PCIE device with 8250
> compatible serial ports. We want to implement all functions of
> F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
> implement GPIOLIB in 8250_pci.c

Your device is multi-function. Create an MFD driver for it. Make the
8250 driver bind to the MFD, and provide your own baud rate methods
within the standard 8250 layer

Implement the GPIO lines in a GPIO driver that also binds to the MFD and
lives in drivers/gpio

All the needed pieces already exist to implement it cleanly this way
without duplicating a ton of code.

Alan

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Andy Shevchenko]

On Tue, 2016-01-19 at 12:33 +0000, One Thousand Gnomes wrote:
> > The architecture of 8250_pci.c is good for PCIE device with 8250
> > compatible serial ports. We want to implement all functions of
> > F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
> > implement GPIOLIB in 8250_pci.c
> 
> Your device is multi-function. Create an MFD driver for it. Make the
> 8250 driver bind to the MFD, and provide your own baud rate methods
> within the standard 8250 layer

Ouch, somehow I missed this one!

Peter, Alan's suggestion is really worth to try.

> 
> Implement the GPIO lines in a GPIO driver that also binds to the MFD
> and
> lives in drivers/gpio
> 
> All the needed pieces already exist to implement it cleanly this way
> without duplicating a ton of code.
> 
> Alan

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Peter Hung]

Hi Andy, Alan

Andy Shevchenko 於 2016/1/19 下午 09:21 寫道:
>> Your device is multi-function. Create an MFD driver for it. Make the
>> 8250 driver bind to the MFD, and provide your own baud rate methods
>> within the standard 8250 layer
>
> Ouch, somehow I missed this one!
>
> Peter, Alan's suggestion is really worth to try.
>

Thanks for point this. It seems good to probe on MFD driver, them MFD
register platform devices to invoke platform driver to initialize
sub-parts. I'll try to survey first.

But I had a new question, If I really do it with MFD subsystem, it'll
split into 3 parts, MFD probe(driver/mfd) / GPIO (driver/gpio) / UART
(drivers/tty/serial/8250). It'll cross more than 2 subsystems and 
maintainers How should I do to organize the patches?

For examples, I should remove the probe function in 8250_pci.c and
move it to new MFD file. It should organize it in the same patch as Paul
said, but this patch will need 2 subsystem maintainer to do with the
same patch, it seems weird.

Andy had cc "[PATCH v5] serial: 8250: add gpio support to exar" to me,
could I use the same way to do GPIOLIB? First add a platform driver
for F81504 gpio and add platform device into 8250_pci.c? It seems to
be good and simple to implement.

-- 
With Best Regards,
Peter Hung

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Sudip Mukherjee]

On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:
> Hi Andy, Alan
> 
> Andy Shevchenko 於 2016/1/19 下午 09:21 寫道:
> >>Your device is multi-function. Create an MFD driver for it. Make the
> >>8250 driver bind to the MFD, and provide your own baud rate methods
> >>within the standard 8250 layer
> >
> >Ouch, somehow I missed this one!
> >
> >Peter, Alan's suggestion is really worth to try.
> >
> 
> Thanks for point this. It seems good to probe on MFD driver, them MFD
> register platform devices to invoke platform driver to initialize
> sub-parts. I'll try to survey first.
> 
> But I had a new question, If I really do it with MFD subsystem, it'll
> split into 3 parts, MFD probe(driver/mfd) / GPIO (driver/gpio) / UART
> (drivers/tty/serial/8250). It'll cross more than 2 subsystems and
> maintainers How should I do to organize the patches?
> 
> For examples, I should remove the probe function in 8250_pci.c and
> move it to new MFD file. It should organize it in the same patch as Paul
> said, but this patch will need 2 subsystem maintainer to do with the
> same patch, it seems weird.
> 
> Andy had cc "[PATCH v5] serial: 8250: add gpio support to exar" to me,
> could I use the same way to do GPIOLIB? First add a platform driver
> for F81504 gpio and add platform device into 8250_pci.c? It seems to
> be good and simple to implement.

+ Rob

Your hardware and my hardware both are almost same, so I guess the
discussion and the decision will apply to both of us.
And to have it as MFD, we can have a look at sm501.c it has serial and
gpio both.
But my personal opinion, if we move out the serial port related code
into a new driver (a new Kconfig symbol) userspace of many system will
break if this new symbol is not enabled by the distributions. But in the
way I have done the new symbol needs to be enabled only if the user
wants to use the GPIO capability. If that is not enabled GPIO cannot be
used but it will never break the serial port related code for them.
I think we should give a thought to that before splitting out the codes
from 8250_pci.

regards
sudip

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Peter Hung]

Hi Sudip,

Sudip Mukherjee 於 2016/1/20 下午 02:22 寫道:
> On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:

> But my personal opinion, if we move out the serial port related code
> into a new driver (a new Kconfig symbol) userspace of many system will
> break if this new symbol is not enabled by the distributions. But in the
> way I have done the new symbol needs to be enabled only if the user
> wants to use the GPIO capability. If that is not enabled GPIO cannot be
> used but it will never break the serial port related code for them.
> I think we should give a thought to that before splitting out the codes
> from 8250_pci.

I agree with your opinion. I'm trying to implement GPIO with 2 ways,
One is like yours, add platform_device with in 8250_pci.c and implement
GPIOLIB platform driver with in 'driver/gpio", and the other is trying
split out from 8250_pci.c to MFD.

In my personal opinion, the first method is less impact with compatible
old system.
-- 
With Best Regards,
Peter Hung

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Sudip Mukherjee]

On Wed, Jan 20, 2016 at 04:24:36PM +0800, Peter Hung wrote:
> Hi Sudip,
> 
> Sudip Mukherjee 於 2016/1/20 下午 02:22 寫道:
> >On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:
> 
> >But my personal opinion, if we move out the serial port related code
> >into a new driver (a new Kconfig symbol) userspace of many system will
> >break if this new symbol is not enabled by the distributions. But in the
> >way I have done the new symbol needs to be enabled only if the user
> >wants to use the GPIO capability. If that is not enabled GPIO cannot be
> >used but it will never break the serial port related code for them.
> >I think we should give a thought to that before splitting out the codes
> >from 8250_pci.
> 
> I agree with your opinion. I'm trying to implement GPIO with 2 ways,
> One is like yours, add platform_device with in 8250_pci.c and implement
> GPIOLIB platform driver with in 'driver/gpio", and the other is trying
> split out from 8250_pci.c to MFD.
> 
> In my personal opinion, the first method is less impact with compatible
> old system.

Looks like no one else is in support of our opinion. Fair enough, I will
split out the related code from 8250_pci and create the MFD driver this
weekend for my hardware.

regards
sudip

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Andy Shevchenko]

On Fri, Jan 22, 2016 at 12:53 PM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On Wed, Jan 20, 2016 at 04:24:36PM +0800, Peter Hung wrote:
>> Hi Sudip,
>>
>> Sudip Mukherjee 於 2016/1/20 下午 02:22 寫道:
>> >On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:
>>
>> >But my personal opinion, if we move out the serial port related code
>> >into a new driver (a new Kconfig symbol) userspace of many system will
>> >break if this new symbol is not enabled by the distributions. But in the
>> >way I have done the new symbol needs to be enabled only if the user
>> >wants to use the GPIO capability. If that is not enabled GPIO cannot be
>> >used but it will never break the serial port related code for them.
>> >I think we should give a thought to that before splitting out the codes
>> >from 8250_pci.
>>
>> I agree with your opinion. I'm trying to implement GPIO with 2 ways,
>> One is like yours, add platform_device with in 8250_pci.c and implement
>> GPIOLIB platform driver with in 'driver/gpio", and the other is trying
>> split out from 8250_pci.c to MFD.
>>
>> In my personal opinion, the first method is less impact with compatible
>> old system.
>
> Looks like no one else is in support of our opinion. Fair enough, I will
> split out the related code from 8250_pci and create the MFD driver this
> weekend for my hardware.

Yeah, MFD looks preferable.

Btw, don't forget to backlist your devices in 8250_pci since they
quite possible provide a PCI class which is used by 8250_pci driver
for default enumeration.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Sudip Mukherjee]

On Fri, Jan 22, 2016 at 03:44:16PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 22, 2016 at 12:53 PM, Sudip Mukherjee
> <sudipm.mukherjee@gmail.com> wrote:
> > On Wed, Jan 20, 2016 at 04:24:36PM +0800, Peter Hung wrote:
> >> Hi Sudip,
> >>
> >> Sudip Mukherjee 於 2016/1/20 下午 02:22 寫道:
> >> >On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:
> >>
> >> >But my personal opinion, if we move out the serial port related code
> >> >into a new driver (a new Kconfig symbol) userspace of many system will
> >> >break if this new symbol is not enabled by the distributions. But in the
> >> >way I have done the new symbol needs to be enabled only if the user
> >> >wants to use the GPIO capability. If that is not enabled GPIO cannot be
> >> >used but it will never break the serial port related code for them.
> >> >I think we should give a thought to that before splitting out the codes
> >> >from 8250_pci.
> >>
> >> I agree with your opinion. I'm trying to implement GPIO with 2 ways,
> >> One is like yours, add platform_device with in 8250_pci.c and implement
> >> GPIOLIB platform driver with in 'driver/gpio", and the other is trying
> >> split out from 8250_pci.c to MFD.
> >>
> >> In my personal opinion, the first method is less impact with compatible
> >> old system.
> >
> > Looks like no one else is in support of our opinion. Fair enough, I will
> > split out the related code from 8250_pci and create the MFD driver this
> > weekend for my hardware.
> 
> Yeah, MFD looks preferable.
> 
> Btw, don't forget to backlist your devices in 8250_pci since they
> quite possible provide a PCI class which is used by 8250_pci driver
> for default enumeration.

One doubt. If I have understood correctly the main reason you have asked
me to split the code out of 8250_pci so that the size reduces. But
pci_xr17v35x_setup() is also used by another card which has
PCI_VENDOR_ID_COMMTECH. So even if I create a separate file for exar cards,
almost identical function will still remain in 8250_pci.

regards
sudip

Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

[Andy Shevchenko]

On Fri, 2016-01-29 at 23:08 +0530, Sudip Mukherjee wrote:
> On Fri, Jan 22, 2016 at 03:44:16PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 22, 2016 at 12:53 PM, Sudip Mukherjee
> > <sudipm.mukherjee@gmail.com> wrote:
> > > On Wed, Jan 20, 2016 at 04:24:36PM +0800, Peter Hung wrote:
> > > > Hi Sudip,
> > > > 
> > > > Sudip Mukherjee 於 2016/1/20 下午 02:22 寫道:
> > > > > On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:
> > > > 
> > > > > But my personal opinion, if we move out the serial port
> > > > > related code
> > > > > into a new driver (a new Kconfig symbol) userspace of many
> > > > > system will
> > > > > break if this new symbol is not enabled by the distributions.
> > > > > But in the
> > > > > way I have done the new symbol needs to be enabled only if
> > > > > the user
> > > > > wants to use the GPIO capability. If that is not enabled GPIO
> > > > > cannot be
> > > > > used but it will never break the serial port related code for
> > > > > them.
> > > > > I think we should give a thought to that before splitting out
> > > > > the codes
> > > > > from 8250_pci.
> > > > 
> > > > I agree with your opinion. I'm trying to implement GPIO with 2
> > > > ways,
> > > > One is like yours, add platform_device with in 8250_pci.c and
> > > > implement
> > > > GPIOLIB platform driver with in 'driver/gpio", and the other is
> > > > trying
> > > > split out from 8250_pci.c to MFD.
> > > > 
> > > > In my personal opinion, the first method is less impact with
> > > > compatible
> > > > old system.
> > > 
> > > Looks like no one else is in support of our opinion. Fair enough,
> > > I will
> > > split out the related code from 8250_pci and create the MFD
> > > driver this
> > > weekend for my hardware.
> > 
> > Yeah, MFD looks preferable.
> > 
> > Btw, don't forget to backlist your devices in 8250_pci since they
> > quite possible provide a PCI class which is used by 8250_pci driver
> > for default enumeration.
> 
> One doubt. If I have understood correctly the main reason you have
> asked
> me to split the code out of 8250_pci so that the size reduces. But
> pci_xr17v35x_setup() is also used by another card which has
> PCI_VENDOR_ID_COMMTECH. So even if I create a separate file for exar
> cards,
> almost identical function will still remain in 8250_pci.

For me looks like re-branded Exar chip (Exar is a real chip vendor,
right?). Even names fall in different pattern.


Also,
        {       PCI_VENDOR_ID_COMMTECH,
PCI_DEVICE_ID_COMMTECH_4222PCIE,
                PCI_ANY_ID, PCI_ANY_ID,
                0,
                0, pbn_exar_XR17V352 },
^^^^!


Move those IDs to your driver as well.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy