[PATCH V2 0/4] Transform Fintek PCIE driver from 8250 to MFD

[PATCH V2 0/4] Transform Fintek PCIE driver from 8250 to MFD

[Peter Hung]

The Fintek F81504/508/512 is a multi-function PCIE devices.
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

It had implemented in 8250_pci.c with basic serial port function.
We want to complete it. Alan & Andy recommend us to rewrite and
spilt our driver with MFD architecture.
https://lkml.org/lkml/2016/1/19/288

Paul recommed us do less code deletion to avoid confusing problem when
bisect.
https://lkml.org/lkml/2016/1/18/646

So we'll do this with following patches.
    1. Add MFD core driver.
    2. Add GPIOLIB driver.
    3. Add serial port driver.
    4. Remove old driver in 8250_pci.c

It can be workable when applied patches 1~3. After apply patch 4,
the device will control by F81504 MFD core driver.
Peter Hung (4):
  mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core
    support
  gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB
    support
  8250: 8250_f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO UART
    support
  serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

 drivers/gpio/Kconfig                  |  10 +
 drivers/gpio/Makefile                 |   1 +
 drivers/gpio/gpio-f81504.c            | 257 ++++++++++++++++++++++++++
 drivers/mfd/Kconfig                   |  11 ++
 drivers/mfd/Makefile                  |   2 +
 drivers/mfd/f81504-core.c             | 331 ++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_f81504.c | 254 ++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_pci.c    | 201 ---------------------
 drivers/tty/serial/8250/Kconfig       |  11 ++
 drivers/tty/serial/8250/Makefile      |   1 +
 include/linux/mfd/f81504.h            |  52 ++++++
 11 files changed, 930 insertions(+), 201 deletions(-)
 create mode 100644 drivers/gpio/gpio-f81504.c
 create mode 100644 drivers/mfd/f81504-core.c
 create mode 100644 drivers/tty/serial/8250/8250_f81504.c
 create mode 100644 include/linux/mfd/f81504.h

-- 
1.9.1

[PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Peter Hung]

The Fintek F81504/508/512 had implemented the basic serial port function in
8250_pci.c. We try to implement high baudrate & GPIOLIB with a spilt file
8250_f81504.c, but it seems too complex to add GPIOLIB.

Alan & Andy recommend us to rewrite and spilt our driver with MFD
architecture.
https://lkml.org/lkml/2016/1/19/288

This driver is core driver for F81504/508/512, it'll handle the generation
of UART/GPIO platform device and initialize PCIE configuration space when
probe()/resume().

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

H/W provider could changes the PCI configure space F0/F3h
values in EEPROM or ASL code to change mode.

    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

Suggested-by: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/mfd/Kconfig        |  11 ++
 drivers/mfd/Makefile       |   2 +
 drivers/mfd/f81504-core.c  | 331 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/f81504.h |  52 +++++++
 4 files changed, 396 insertions(+)
 create mode 100644 drivers/mfd/f81504-core.c
 create mode 100644 include/linux/mfd/f81504.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 9ca66de..40503a4 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -310,6 +310,17 @@ config HTC_I2CPLD
 	  This device provides input and output GPIOs through an I2C
 	  interface to one or more sub-chips.
 
+config MFD_FINTEK_F81504_CORE
+        tristate "Fintek F81504/508/512 PCIE-to-UART/GPIO MFD support"
+        depends on PCI
+        select MFD_CORE
+        default y
+        help
+          This driver generate F81504/508/512 UART & GPIO platform
+          device. It should enable CONFIG_GPIO_F81504 to get GPIOLIB
+          support and CONFIG_8250_F81504 to get serial ports support.
+          Please bulit-in kernel if you need early console support.
+
 config MFD_INTEL_QUARK_I2C_GPIO
 	tristate "Intel Quark MFD I2C GPIO"
 	depends on PCI
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 0f230a6..f7382b3 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -21,6 +21,8 @@ obj-$(CONFIG_HTC_EGPIO)		+= htc-egpio.o
 obj-$(CONFIG_HTC_PASIC3)	+= htc-pasic3.o
 obj-$(CONFIG_HTC_I2CPLD)	+= htc-i2cpld.o
 
+obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o
+
 obj-$(CONFIG_MFD_DAVINCI_VOICECODEC)	+= davinci_voicecodec.o
 obj-$(CONFIG_MFD_DM355EVM_MSP)	+= dm355evm_msp.o
 obj-$(CONFIG_MFD_TI_AM335X_TSCADC)	+= ti_am335x_tscadc.o
diff --git a/drivers/mfd/f81504-core.c b/drivers/mfd/f81504-core.c
new file mode 100644
index 0000000..9059171
--- /dev/null
+++ b/drivers/mfd/f81504-core.c
@@ -0,0 +1,331 @@
+/*
+ * Core operations for Fintek F81504/508/512 PCIE-to-UART/GPIO device
+ */
+#include <linux/platform_device.h>
+#include <linux/pci.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/version.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/f81504.h>
+
+#define F81504_DRIVER_NAME	"f81504_core"
+#define F81504_DEV_DESC		"Fintek F81504/508/512 PCIE-to-UART core"
+#define F81504_IO_REGION	8
+
+const u8 fintek_gpio_mapping[F81504_MAX_GPIO_CNT] = { 2, 3, 8, 9, 10, 11 };
+EXPORT_SYMBOL(fintek_gpio_mapping);
+
+static int f81504_port_init(struct pci_dev *dev)
+{
+	size_t i, j;
+	u32 max_port, iobase, gpio_addr;
+	u32 bar_data[3];
+	u16 tmp;
+	u8 config_base, gpio_en, f0h_data, f3h_data;
+	bool is_gpio;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+
+	/* Init GPIO IO Address */
+	pci_read_config_dword(dev, 0x18, &gpio_addr);
+	gpio_addr &= 0xffffffe0;
+	pci_write_config_byte(dev, F81504_GPIO_IO_LSB_REG, gpio_addr & 0xff);
+	pci_write_config_byte(dev, F81504_GPIO_IO_MSB_REG, (gpio_addr >> 8) &
+			0xff);
+
+	/*
+	 * 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->gpio_en;
+
+		/* re-save GPIO IO addr for called by resume() */
+		priv->gpio_ioaddr = gpio_addr;
+	} else {
+		/* Driver first init */
+		pci_read_config_byte(dev, F81504_GPIO_ENABLE_REG, &f0h_data);
+		pci_read_config_byte(dev, F81504_GPIO_MODE_REG, &f3h_data);
+
+		/* find the max set of GPIOs */
+		gpio_en = f0h_data | ~f3h_data;
+	}
+
+	switch (dev->device) {
+	case FINTEK_F81504: /* 4 ports */
+		/* F81504 max 2 sets of GPIO, others are max 6 sets*/
+		gpio_en &= 0x03;
+	case FINTEK_F81508: /* 8 ports */
+		max_port = dev->device & 0xff;
+		break;
+	case FINTEK_F81512: /* 12 ports */
+		max_port = 12;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* rewrite GPIO Mode setting */
+	pci_write_config_byte(dev, F81504_GPIO_ENABLE_REG, gpio_en & 0x3f);
+	pci_write_config_byte(dev, F81504_GPIO_MODE_REG, ~gpio_en & 0x3f);
+
+	/* 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 bit for newer step IC */
+	pci_read_config_word(dev, F81504_IRQSEL_REG, &tmp);
+	tmp |= BIT(8);
+	pci_write_config_word(dev, F81504_IRQSEL_REG, tmp);
+
+	for (i = 0; i < max_port; ++i) {
+		/* UART0 configuration offset start from 0x40 */
+		config_base = F81504_UART_START_ADDR + F81504_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;
+
+		/* 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);
+
+		/*
+		 * Force init to RS232 / Share Mode, recovery previous mode
+		 * will done in F81504 8250 platform driver resume()
+		 */
+		pci_write_config_byte(dev, config_base + 0x07, 0x01);
+	}
+
+	return 0;
+}
+
+static int f81504_prepage_serial_port(struct pci_dev *dev, int max_port)
+{
+	size_t i;
+	int status;
+	u8 tmp;
+	u16 iobase;
+	struct resource	resources = DEFINE_RES_IO(0, 0);
+	struct mfd_cell f81504_serial_cell = {
+		.name = F81504_SERIAL_NAME,
+		.num_resources	= 1,
+	};
+
+	for (i = 0; i < max_port; ++i) {
+		/* Check UART is enabled */
+		pci_read_config_byte(dev, F81504_UART_START_ADDR + i *
+				F81504_UART_OFFSET, &tmp);
+		if (!tmp)
+			continue;
+
+		/* Get UART IO Address */
+		pci_read_config_word(dev, F81504_UART_START_ADDR + i *
+				F81504_UART_OFFSET + 4, &iobase);
+
+		resources.start = iobase;
+		resources.end = iobase + F81504_IO_REGION - 1;
+
+		f81504_serial_cell.resources = &resources;
+		f81504_serial_cell.pdata_size = sizeof(i);
+		f81504_serial_cell.platform_data = &i;
+
+		status = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
+					&f81504_serial_cell, 1, NULL, dev->irq,
+					NULL);
+		if (status) {
+			dev_warn(&dev->dev, "%s: add device failed: %d\n",
+					__func__, status);
+			return status;
+		}
+	}
+
+	return 0;
+}
+
+static int f81504_prepage_gpiolib(struct pci_dev *dev)
+{
+	size_t i;
+	int status;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);
+	struct mfd_cell f81504_gpio_cell = {
+		.name = F81504_GPIO_NAME,
+	};
+
+	for (i = 0; i < ARRAY_SIZE(fintek_gpio_mapping); ++i) {
+		if (!(priv->gpio_en & BIT(i)))
+			continue;
+
+		f81504_gpio_cell.pdata_size = sizeof(i);
+		f81504_gpio_cell.platform_data = &i;
+
+		status = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
+					&f81504_gpio_cell, 1, NULL, dev->irq,
+					NULL);
+		if (status) {
+			dev_warn(&dev->dev, "%s: add device failed: %d\n",
+					__func__, status);
+			return status;
+		}
+	}
+
+	return 0;
+}
+
+static int f81504_probe(struct pci_dev *dev, const struct pci_device_id
+			*dev_id)
+{
+	int status;
+	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;
+
+	/* Save the GPIO_ENABLE_REG after f81504_port_init() for future use */
+	pci_read_config_byte(dev, F81504_GPIO_ENABLE_REG, &priv->gpio_en);
+
+	/* Save GPIO IO Addr to private data */
+	pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp);
+	priv->gpio_ioaddr = tmp << 8;
+	pci_read_config_byte(dev, F81504_GPIO_IO_LSB_REG, &tmp);
+	priv->gpio_ioaddr |= tmp;
+
+	pci_set_drvdata(dev, priv);
+
+	/* Generate UART Ports */
+	status = f81504_prepage_serial_port(dev, dev_id->driver_data);
+	if (status)
+		goto failed;
+
+	/* Generate GPIO Sets */
+	status = f81504_prepage_gpiolib(dev);
+	if (status)
+		goto failed;
+
+	return 0;
+
+failed:
+	mfd_remove_devices(&dev->dev);
+	pci_disable_device(dev);
+	return status;
+}
+
+static void f81504_remove(struct pci_dev *dev)
+{
+	mfd_remove_devices(&dev->dev);
+	pci_disable_device(dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int f81504_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int f81504_resume(struct device *dev)
+{
+	int status;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	status = pci_enable_device(pdev);
+	if (status)
+		return status;
+
+	/* Re-init PCI Configuration Space */
+	status = f81504_port_init(pdev);
+	if (status)
+		return status;
+
+	return 0;
+}
+#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},
+	{}
+};
+
+static SIMPLE_DEV_PM_OPS(f81504_pm_ops, f81504_suspend, f81504_resume);
+
+static struct pci_driver f81504_driver = {
+	.name = F81504_DRIVER_NAME,
+	.probe = f81504_probe,
+	.remove = f81504_remove,
+	.driver		= {
+		.pm	= &f81504_pm_ops,
+		.owner	= THIS_MODULE,
+	},
+	.id_table = f81504_dev_table,
+};
+
+module_pci_driver(f81504_driver);
+
+MODULE_DEVICE_TABLE(pci, f81504_dev_table);
+MODULE_DESCRIPTION(F81504_DEV_DESC);
+MODULE_AUTHOR("Peter Hong <Peter_Hong@fintek.com.tw>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/f81504.h b/include/linux/mfd/f81504.h
new file mode 100644
index 0000000..13bd0ae
--- /dev/null
+++ b/include/linux/mfd/f81504.h
@@ -0,0 +1,52 @@
+#ifndef __F81504_H__
+#define __F81504_H__
+
+#define FINTEK_VID			0x1c29
+#define FINTEK_F81504			0x1104
+#define FINTEK_F81508			0x1108
+#define FINTEK_F81512			0x1112
+
+#define FINTEK_MAX_PORT			12
+#define FINTEK_GPIO_NAME_LEN		32
+#define FINTEK_GPIO_DISPLAY		"GPIO"
+
+#define F81504_UART_START_ADDR		0x40
+#define F81504_UART_MODE_OFFSET		0x07
+#define F81504_UART_OFFSET		0x08
+
+/* RTS will control by MCR if this bit is 0 */
+#define F81504_RTS_CONTROL_BY_HW	BIT(4)
+/* only worked with FINTEK_RTS_CONTROL_BY_HW on */
+#define F81504_RTS_INVERT		BIT(5)
+
+#define F81504_CLOCK_RATE_MASK		0xc0
+#define F81504_CLKSEL_1DOT846_MHZ	0x00
+#define F81504_CLKSEL_18DOT46_MHZ	0x40
+#define F81504_CLKSEL_24_MHZ		0x80
+#define F81504_CLKSEL_14DOT77_MHZ	0xc0
+
+#define F81504_IRQSEL_REG		0xb8
+
+#define F81504_GPIO_ENABLE_REG		0xf0
+#define F81504_GPIO_IO_LSB_REG		0xf1
+#define F81504_GPIO_IO_MSB_REG		0xf2
+#define F81504_GPIO_MODE_REG		0xf3
+
+#define F81504_GPIO_START_ADDR		0xf8
+#define F81504_GPIO_OUT_EN_OFFSET	0x00
+#define F81504_GPIO_DRIVE_EN_OFFSET	0x01
+#define F81504_GPIO_SET_OFFSET		0x08
+
+#define F81504_GPIO_NAME		"f81504_gpio"
+#define F81504_SERIAL_NAME		"f81504_serial"
+#define F81504_MAX_GPIO_CNT		6
+
+extern const u8 fintek_gpio_mapping[F81504_MAX_GPIO_CNT];
+
+struct f81504_pci_private {
+	int line[FINTEK_MAX_PORT];
+	u8 gpio_en;
+	u16 gpio_ioaddr;
+	u32 uart_count, gpio_count;
+};
+#endif
-- 
1.9.1

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[One Thousand Gnomes]

> +config MFD_FINTEK_F81504_CORE
> +        tristate "Fintek F81504/508/512 PCIE-to-UART/GPIO MFD support"
> +        depends on PCI
> +        select MFD_CORE
> +        default y
> +        help
> +          This driver generate F81504/508/512 UART & GPIO platform
This driver provides the F81504/508/512 UART & GPIO platform
> +          device. It should enable CONFIG_GPIO_F81504 to get GPIOLIB
devices. You should enable CONFIG_GPIO_F81504 to get GPIOLIB
> +          support and CONFIG_8250_F81504 to get serial ports support.
port rather than ports

> +          Please bulit-in kernel if you need early console support.
This driver needs to be built into the kernel to use early console
support.



> +	switch (dev->device) {
> +	case FINTEK_F81504: /* 4 ports */
> +		/* F81504 max 2 sets of GPIO, others are max 6 sets*/
> +		gpio_en &= 0x03;
> +	case FINTEK_F81508: /* 8 ports */
> +		max_port = dev->device & 0xff;

If that is meant to fall through from F81504 into F81508 it's worth
commenting, otherwise someone reviewing the code can't always be sure it
was intentional.

> +		break;
> +	case FINTEK_F81512: /* 12 ports */
> +		max_port = 12;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* rewrite GPIO Mode setting */
> +	pci_write_config_byte(dev, F81504_GPIO_ENABLE_REG, gpio_en & 0x3f);
> +	pci_write_config_byte(dev, F81504_GPIO_MODE_REG, ~gpio_en & 0x3f);
> +
> +	/* 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]);

Take these from the pci device itself. On some non PC platforms the
values in the pci bar may be remapped by bridges and not give you the
true answer.

	pci_resource_start(dev, barnumber)


Alan

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Peter Hung]

Hi Alan,

One Thousand Gnomes 於 2016/1/28 下午 06:04 寫道:
>> +          Please bulit-in kernel if you need early console support.
> This driver needs to be built into the kernel to use early console
> support.
>

ok

>> +	switch (dev->device) {
>> +	case FINTEK_F81504: /* 4 ports */
>> +		/* F81504 max 2 sets of GPIO, others are max 6 sets*/
>> +		gpio_en &= 0x03;
>> +	case FINTEK_F81508: /* 8 ports */
>> +		max_port = dev->device & 0xff;
>
> If that is meant to fall through from F81504 into F81508 it's worth
> commenting, otherwise someone reviewing the code can't always be sure it
> was intentional.

ok, I'll add comments to describe this.

>> +	/* 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]);
>
> Take these from the pci device itself. On some non PC platforms the
> values in the pci bar may be remapped by bridges and not give you the
> true answer.
>
> 	pci_resource_start(dev, barnumber)
>

Thanks for point this out, I'll rewrite here with pci_resource_start().
-- 
With Best Regards,
Peter Hung

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Andy Shevchenko]

On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
> The Fintek F81504/508/512 had implemented the basic serial port
> function in
> 8250_pci.c. We try to implement high baudrate & GPIOLIB with a spilt
> file
> 8250_f81504.c, but it seems too complex to add GPIOLIB.
> 
> Alan & Andy recommend us to rewrite and spilt our driver with MFD
> architecture.
> https://lkml.org/lkml/2016/1/19/288


My comments below.


> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -310,6 +310,17 @@ config HTC_I2CPLD
>  	  This device provides input and output GPIOs through an I2C
>  	  interface to one or more sub-chips.
>  
> +config MFD_FINTEK_F81504_CORE
> +        tristate "Fintek F81504/508/512 PCIE-to-UART/GPIO MFD
> support"
> +        depends on PCI
> +        select MFD_CORE
> 

> +        default y

I'm not sure we have to have this always y. Perhaps 
default SERIAL_8250_PCI

> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -21,6 +21,8 @@ obj-$(CONFIG_HTC_EGPIO)		+= htc-
> egpio.o
>  obj-$(CONFIG_HTC_PASIC3)	+= htc-pasic3.o
>  obj-$(CONFIG_HTC_I2CPLD)	+= htc-i2cpld.o
>  
> +obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o

I think '_' is better than '-'. What I saw and usually do is '_' for
regular source modules and '-' for the resulting objects when they have
more than one file.

> --- /dev/null
> +++ b/drivers/mfd/f81504-core.c
> @@ -0,0 +1,331 @@
> +/*
> + * Core operations for Fintek F81504/508/512 PCIE-to-UART/GPIO
> device
> + */
> +#include <linux/platform_device.h>
> +#include <linux/pci.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/version.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/f81504.h>
> +
> +#define F81504_DRIVER_NAME	"f81504_core"

Yeah, exactly.

> +#define F81504_DEV_DESC		"Fintek F81504/508/512 PCIE-
> to-UART core"

Do you need this definition? Is it used more than once?

> +#define F81504_IO_REGION	8
> +
> +const u8 fintek_gpio_mapping[F81504_MAX_GPIO_CNT] = { 2, 3, 8, 9,
> 10, 11 };
> +EXPORT_SYMBOL(fintek_gpio_mapping);
> +
> +static int f81504_port_init(struct pci_dev *dev)
> +{
> +	size_t i, j;
> +	u32 max_port, iobase, gpio_addr;
> +	u32 bar_data[3];
> +	u16 tmp;
> +	u8 config_base, gpio_en, f0h_data, f3h_data;
> +	bool is_gpio;
> +	struct f81504_pci_private *priv = pci_get_drvdata(dev);

I would suggest to rearrange definition block here (and in the rest of
the functions in entire series) to somehow follow the following pattern

1) assignments go first, especially container_of() based ones;
2) group variables by meaning and put longer lines upper;
3) return value variable, if exists, goes last.

Besides that choose types carefully. If there is known limit for
counters, no need to define wider type than needed. Use proper types
for corresponding values.

> +
> +	/* Init GPIO IO Address */
> +	pci_read_config_dword(dev, 0x18, &gpio_addr);
> +	gpio_addr &= 0xffffffe0;


> +	pci_write_config_byte(dev, F81504_GPIO_IO_LSB_REG, gpio_addr
> & 0xff);
> +	pci_write_config_byte(dev, F81504_GPIO_IO_MSB_REG,
> (gpio_addr >> 8) &
> +			0xff);

Could it be written at once as a word?

> +	if (priv) {
> +		/* Reinit from resume(), read the previous value
> from priv */
> 

> +		gpio_en = priv->gpio_en;

I would suggest to move this line down to follow same pattern in else
branch.

> +
> +		/* re-save GPIO IO addr for called by resume() */

re-save -> Re-save
addr -> address

> +		priv->gpio_ioaddr = gpio_addr;
> +	} else {
> +		/* Driver first init */
> +		pci_read_config_byte(dev, F81504_GPIO_ENABLE_REG,
> &f0h_data);
> +		pci_read_config_byte(dev, F81504_GPIO_MODE_REG,
> &f3h_data);
> +
> +		/* find the max set of GPIOs */

Use one pattern for all comments, like "Capital letter first, and full
words avoiding abbreviations".

> +		gpio_en = f0h_data | ~f3h_data;
> +	}
> +
> +	switch (dev->device) {
> +	case FINTEK_F81504: /* 4 ports */
> +		/* F81504 max 2 sets of GPIO, others are max 6
> sets*/

Space before * is needed.

> +		gpio_en &= 0x03;
> +	case FINTEK_F81508: /* 8 ports */
> +		max_port = dev->device & 0xff;
> +		break;
> +	case FINTEK_F81512: /* 12 ports */
> +		max_port = 12;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* rewrite GPIO Mode setting */
> +	pci_write_config_byte(dev, F81504_GPIO_ENABLE_REG, gpio_en &
> 0x3f);
> +	pci_write_config_byte(dev, F81504_GPIO_MODE_REG, ~gpio_en &
> 0x3f);
> +
> +	/* 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 bit for newer step IC */
> +	pci_read_config_word(dev, F81504_IRQSEL_REG, &tmp);
> +	tmp |= BIT(8);
> +	pci_write_config_word(dev, F81504_IRQSEL_REG, tmp);
> +
> +	for (i = 0; i < max_port; ++i) {

i++, since it's more usual pattern.

> +		/* UART0 configuration offset start from 0x40 */
> +		config_base = F81504_UART_START_ADDR +
> F81504_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;
> +		}

Looks like a separate function.

func()
{
 for () {
   if ()
    return X;
 }
 return Y;
}


> +
> +		/*
> +		 * 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;


> +
> +		/* 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));

Redundant explicit casting. 

> +
> +		/* MSB UART */
> +		pci_write_config_byte(dev, config_base + 0x05,
> +				(u8)((iobase & 0xff00) >> 8));

Ditto.

And similar question, can it be written as a word?

> +
> +		pci_write_config_byte(dev, config_base + 0x06, dev-
> >irq);
> +
> +		/*
> +		 * Force init to RS232 / Share Mode, recovery
> previous mode
> +		 * will done in F81504 8250 platform driver resume()

Period at the end of sentences in multi-line comments.

> +		 */
> +		pci_write_config_byte(dev, config_base + 0x07,
> 0x01);
> +	}
> +
> +	return 0;
> +}
> +
> +static int f81504_prepage_serial_port(struct pci_dev *dev, int
> max_port)
> +{
> +	size_t i;
> +	int status;
> +	u8 tmp;
> +	u16 iobase;
> +	struct resource	resources = DEFINE_RES_IO(0, 0);


> +	struct mfd_cell f81504_serial_cell = {
> +		.name = F81504_SERIAL_NAME,
> +		.num_resources	= 1,
> +	};

> +
> +	for (i = 0; i < max_port; ++i) {
> +		/* Check UART is enabled */
> 

> +		pci_read_config_byte(dev, F81504_UART_START_ADDR + i
> *
> +				F81504_UART_OFFSET, &tmp);
> +		if (!tmp)
> +			continue;

So, this is the same as below, right?

> +
> +		/* Get UART IO Address */
> +		pci_read_config_word(dev, F81504_UART_START_ADDR + i
> *
> +				F81504_UART_OFFSET + 4, &iobase);

…why not to check here?

> +
> +		resources.start = iobase;
> +		resources.end = iobase + F81504_IO_REGION - 1;
> +
> +		f81504_serial_cell.resources = &resources;

> +		f81504_serial_cell.pdata_size = sizeof(i);

This is static, can be part of definition.

> +		f81504_serial_cell.platform_data = &i;
> +
> +		status = mfd_add_devices(&dev->dev,
> PLATFORM_DEVID_AUTO,
> +					&f81504_serial_cell, 1,
> NULL, dev->irq,
> +					NULL);
> +		if (status) {
> +			dev_warn(&dev->dev, "%s: add device failed:
> %d\n",
> +					__func__, status);

Indent _ under &.

> +			return status;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int f81504_prepage_gpiolib(struct pci_dev *dev)
> +{
> +	size_t i;
> +	int status;
> +	struct f81504_pci_private *priv = pci_get_drvdata(dev);
> +	struct mfd_cell f81504_gpio_cell = {
> +		.name = F81504_GPIO_NAME,
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(fintek_gpio_mapping); ++i) {
> +		if (!(priv->gpio_en & BIT(i)))
> +			continue;
> +
> 

> +		f81504_gpio_cell.pdata_size = sizeof(i);

Static. The problem here is badly chosen type of i. Like I mentioned at
the top of review…

> +		f81504_gpio_cell.platform_data = &i;
> +
> +		status = mfd_add_devices(&dev->dev,
> PLATFORM_DEVID_AUTO,
> +					&f81504_gpio_cell, 1, NULL,
> dev->irq,
> +					NULL);
> +		if (status) {
> +			dev_warn(&dev->dev, "%s: add device failed:
> %d\n",
> +					__func__, status);
> +			return status;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int f81504_probe(struct pci_dev *dev, const struct
> pci_device_id
> +			*dev_id)

Usually drivers are using pdev, id pair in parameters. Shorter; known
pattern.

> +{
> +	int status;
> +	u8 tmp;
> +	struct f81504_pci_private *priv;
> +
> +	status = pci_enable_device(dev);

Please, pcim_, see comment below.

> +	if (status)
> +		return status;
> +
> +	/* Init PCI Configuration Space */
> +	status = f81504_port_init(dev);
> +	if (status)

Device left enabled if not managed.

> +		return status;
> +
> +	priv = devm_kzalloc(&dev->dev, sizeof(struct
> f81504_pci_private),
> +				GFP_KERNEL);
> +	if (!priv)

Ditto.

> +		return -ENOMEM;
> +
> +	/* Save the GPIO_ENABLE_REG after f81504_port_init() for
> future use */
> +	pci_read_config_byte(dev, F81504_GPIO_ENABLE_REG, &priv-
> >gpio_en);
> +
> +	/* Save GPIO IO Addr to private data */
> 

> +	pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp);
> +	priv->gpio_ioaddr = tmp << 8;
> +	pci_read_config_byte(dev, F81504_GPIO_IO_LSB_REG, &tmp);
> +	priv->gpio_ioaddr |= tmp;

One read as word?

> +
> +	pci_set_drvdata(dev, priv);
> +
> +	/* Generate UART Ports */
> +	status = f81504_prepage_serial_port(dev, dev_id-
> >driver_data);
> +	if (status)
> +		goto failed;
> +
> +	/* Generate GPIO Sets */
> +	status = f81504_prepage_gpiolib(dev);
> +	if (status)
> +		goto failed;
> +
> +	return 0;
> +
> +failed:
> 

> +	mfd_remove_devices(&dev->dev);

mfd_add_devices takes care.

> +	pci_disable_device(dev);

pcim_ takes case.

> +	return status;
> +}
> +
> +static void f81504_remove(struct pci_dev *dev)
> +{

> +	mfd_remove_devices(&dev->dev);
> +	pci_disable_device(dev);

Ditto.

> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int f81504_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int f81504_resume(struct device *dev)
> +{
> +	int status;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> 

> +	status = pci_enable_device(pdev);
> +	if (status)
> +		return status;

It's done by PCI core I believe.

> +
> +	/* Re-init PCI Configuration Space */
> +	status = f81504_port_init(pdev);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +#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},
> +	{}

So, if you have default y for this and 8250_pci, which one will be
loaded?

> +};
> +
> +static SIMPLE_DEV_PM_OPS(f81504_pm_ops, f81504_suspend,
> f81504_resume);
> +
> +static struct pci_driver f81504_driver = {
> +	.name = F81504_DRIVER_NAME,
> +	.probe = f81504_probe,
> +	.remove = f81504_remove,
> +	.driver		= {
> +		.pm	= &f81504_pm_ops,


> +		.owner	= THIS_MODULE,

kbuild  already complained about.

> +	},
> +	.id_table = f81504_dev_table,
> +};
> +
> +module_pci_driver(f81504_driver);
> +
> +MODULE_DEVICE_TABLE(pci, f81504_dev_table);
> +MODULE_DESCRIPTION(F81504_DEV_DESC);
> +MODULE_AUTHOR("Peter Hong <Peter_Hong@fintek.com.tw>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/f81504.h b/include/linux/mfd/f81504.h
> new file mode 100644
> index 0000000..13bd0ae
> --- /dev/null
> +++ b/include/linux/mfd/f81504.h
> @@ -0,0 +1,52 @@
> +#ifndef __F81504_H__

__MFD_F…

> +#define __F81504_H__
> +
> +#define FINTEK_VID			0x1c29
> +#define FINTEK_F81504			0x1104
> +#define FINTEK_F81508			0x1108
> +#define FINTEK_F81512			0x1112
> +
> +#define FINTEK_MAX_PORT			12
> +#define FINTEK_GPIO_NAME_LEN		32
> +#define FINTEK_GPIO_DISPLAY		"GPIO"
> +
> +#define F81504_UART_START_ADDR		0x40
> +#define F81504_UART_MODE_OFFSET		0x07
> +#define F81504_UART_OFFSET		0x08
> +
> +/* RTS will control by MCR if this bit is 0 */
> +#define F81504_RTS_CONTROL_BY_HW	BIT(4)
> +/* only worked with FINTEK_RTS_CONTROL_BY_HW on */
> +#define F81504_RTS_INVERT		BIT(5)
> +
> +#define F81504_CLOCK_RATE_MASK		0xc0
> +#define F81504_CLKSEL_1DOT846_MHZ	0x00
> +#define F81504_CLKSEL_18DOT46_MHZ	0x40
> +#define F81504_CLKSEL_24_MHZ		0x80
> +#define F81504_CLKSEL_14DOT77_MHZ	0xc0
> +
> +#define F81504_IRQSEL_REG		0xb8
> +
> +#define F81504_GPIO_ENABLE_REG		0xf0
> +#define F81504_GPIO_IO_LSB_REG		0xf1
> +#define F81504_GPIO_IO_MSB_REG		0xf2
> +#define F81504_GPIO_MODE_REG		0xf3
> +
> +#define F81504_GPIO_START_ADDR		0xf8
> +#define F81504_GPIO_OUT_EN_OFFSET	0x00
> +#define F81504_GPIO_DRIVE_EN_OFFSET	0x01
> +#define F81504_GPIO_SET_OFFSET		0x08
> +
> +#define F81504_GPIO_NAME		"f81504_gpio"
> +#define F81504_SERIAL_NAME		"f81504_serial"
> +#define F81504_MAX_GPIO_CNT		6
> +
> +extern const u8 fintek_gpio_mapping[F81504_MAX_GPIO_CNT];
> +
> +struct f81504_pci_private {
> +	int line[FINTEK_MAX_PORT];
> +	u8 gpio_en;
> +	u16 gpio_ioaddr;
> +	u32 uart_count, gpio_count;
> +};
> +#endif

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

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/28 下午 07:55 寫道:
>> +        default y
>
> I'm not sure we have to have this always y. Perhaps
> default SERIAL_8250_PCI

Your comment is right, this device major function is serial port.
GPIO maybe not enabled by H/W manufacturer.

I'll set it default with SERIAL_8250_PCI.

>> +obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o
>
> I think '_' is better than '-'. What I saw and usually do is '_' for
> regular source modules and '-' for the resulting objects when they have
> more than one file.

I used f81504_core.c originally, but I found most of files are named
xxx-ooo.c when I try to modify makefile. Should I change it to
f81504_core.c ??


>> +#define F81504_DEV_DESC		"Fintek F81504/508/512 PCIE-
>> to-UART core"
>
> Do you need this definition? Is it used more than once?

ok, I'll direct use the string without define.


>> +static int f81504_port_init(struct pci_dev *dev)
>> +{
>> +	size_t i, j;
>> +	u32 max_port, iobase, gpio_addr;
>> +	u32 bar_data[3];
>> +	u16 tmp;
>> +	u8 config_base, gpio_en, f0h_data, f3h_data;
>> +	bool is_gpio;
>> +	struct f81504_pci_private *priv = pci_get_drvdata(dev);
>
> I would suggest to rearrange definition block here (and in the rest of
> the functions in entire series) to somehow follow the following pattern
>
> 1) assignments go first, especially container_of() based ones;
> 2) group variables by meaning and put longer lines upper;
> 3) return value variable, if exists, goes last.
>
> Besides that choose types carefully. If there is known limit for
> counters, no need to define wider type than needed. Use proper types
> for corresponding values.

I'll try to rearrange the definition blocks.
For the counter issue, some senior recommend me to change count from int
to size_t for performance. In this case, should I use u8 to replace
i & j ? It should be less than 12 & 6.

>> +
>> +	/* Init GPIO IO Address */
>> +	pci_read_config_dword(dev, 0x18, &gpio_addr);
>> +	gpio_addr &= 0xffffffe0;
>
>
>> +	pci_write_config_byte(dev, F81504_GPIO_IO_LSB_REG, gpio_addr
>> & 0xff);
>> +	pci_write_config_byte(dev, F81504_GPIO_IO_MSB_REG,
>> (gpio_addr >> 8) &
>> +			0xff);
>
> Could it be written at once as a word?

I tested with writing a word to "F81504_GPIO_IO_LSB_REG", but
it'll failed, so I'll remain it.

>> +	if (priv) {
>> +		/* Reinit from resume(), read the previous value
>> from priv */
>>
>
>> +		gpio_en = priv->gpio_en;
>
> I would suggest to move this line down to follow same pattern in else
> branch.

The f81504_port_init() will be called probe()/resume().

We'll initialize gpio_en = (f0h | ~f3h) and save it in private data
when probe(), reload gpio_en from private data when resume().

The F81504/508/512 can be used EEPROM to override F0h/F3h on power
on. Some motherboard designer will put the IC on board and want to cost
down EEPROM. They will setting F0/F3h with ASL code, but without EEPROM
, the IC back from resume() will get F0/F3h with 0x00, so we should
save gpio_en when probe(), and restore it when resume().

>> +
>> +		/* re-save GPIO IO addr for called by resume() */
>
> re-save -> Re-save
> addr -> address

ok

>> +		priv->gpio_ioaddr = gpio_addr;
>> +	} else {
>> +		/* Driver first init */
>> +		pci_read_config_byte(dev, F81504_GPIO_ENABLE_REG,
>> &f0h_data);
>> +		pci_read_config_byte(dev, F81504_GPIO_MODE_REG,
>> &f3h_data);
>> +
>> +		/* find the max set of GPIOs */
>
> Use one pattern for all comments, like "Capital letter first, and full
> words avoiding abbreviations".

ok

>> +	case FINTEK_F81504: /* 4 ports */
>> +		/* F81504 max 2 sets of GPIO, others are max 6
>> sets*/
>
> Space before * is needed.

ok

>> +	for (i = 0; i < max_port; ++i) {
>
> i++, since it's more usual pattern.

ok

>> +		/* 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;
>> +		}
>
> Looks like a separate function.
>
> func()
> {
>   for () {
>     if ()
>      return X;
>   }
>   return Y;
> }

ok.


>> +		/* 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));
>
> Redundant explicit casting.

ok

>> +
>> +		/* MSB UART */
>> +		pci_write_config_byte(dev, config_base + 0x05,
>> +				(u8)((iobase & 0xff00) >> 8));
>
> Ditto.
>
> And similar question, can it be written as a word?

Here can be written with a word, I'll rewrite it.

>> +
>> +		pci_write_config_byte(dev, config_base + 0x06, dev-
>>> irq);
>> +
>> +		/*
>> +		 * Force init to RS232 / Share Mode, recovery
>> previous mode
>> +		 * will done in F81504 8250 platform driver resume()
>
> Period at the end of sentences in multi-line comments.

Sorry, what this mean for ?
I cant use multi-line comments in the end ??

>> +	for (i = 0; i < max_port; ++i) {
>> +		/* Check UART is enabled */
>>
>
>> +		pci_read_config_byte(dev, F81504_UART_START_ADDR + i
>> *
>> +				F81504_UART_OFFSET, &tmp);
>> +		if (!tmp)
>> +			continue;
>
> So, this is the same as below, right?
>
>> +
>> +		/* Get UART IO Address */
>> +		pci_read_config_word(dev, F81504_UART_START_ADDR + i
>> *
>> +				F81504_UART_OFFSET + 4, &iobase);
>
> …why not to check here?

It's a bit difference, this section will check if UART enabled (tmp).
It'll generate platform device and get IO address when tmp != 0. If
tmp = 0, skip this idx, dont need to get current IO address and try
with next.


>> +		f81504_serial_cell.pdata_size = sizeof(i);
>
> This is static, can be part of definition.

ok

>> +		f81504_serial_cell.platform_data = &i;
>> +
>> +		status = mfd_add_devices(&dev->dev,
>> PLATFORM_DEVID_AUTO,
>> +					&f81504_serial_cell, 1,
>> NULL, dev->irq,
>> +					NULL);
>> +		if (status) {
>> +			dev_warn(&dev->dev, "%s: add device failed:
>> %d\n",
>> +					__func__, status);
>
> Indent _ under &.

Some senior recommend me to do at least 2-tabs to do indent when
code cross multi-line. So I'll use to 2-tabs from "dev" to do indent.
How should I do with indent ?? It seems no consensus, but Lindent
will do indent like your comments.


>
>> +		f81504_gpio_cell.pdata_size = sizeof(i);
>
> Static. The problem here is badly chosen type of i. Like I mentioned at
> the top of review…

I'll try to rewrite it with pass a structure. It seems more make sense.


>> +static int f81504_probe(struct pci_dev *dev, const struct
>> pci_device_id
>> +			*dev_id)
>
> Usually drivers are using pdev, id pair in parameters. Shorter; known
> pattern.

ok.

>> +{
>> +	int status;
>> +	u8 tmp;
>> +	struct f81504_pci_private *priv;
>> +
>> +	status = pci_enable_device(dev);
>
> Please, pcim_, see comment below.
>> +	if (status)
>> +		return status;
>> +
>> +	/* Init PCI Configuration Space */
>> +	status = f81504_port_init(dev);
>> +	if (status)
>
> Device left enabled if not managed.
>> +		return status;
>> +
>> +	priv = devm_kzalloc(&dev->dev, sizeof(struct
>> f81504_pci_private),
>> +				GFP_KERNEL);
>> +	if (!priv)
>
> Ditto.

ok, I'll rewrite it with managed type.

>> +		return -ENOMEM;
>> +
>> +	/* Save the GPIO_ENABLE_REG after f81504_port_init() for
>> future use */
>> +	pci_read_config_byte(dev, F81504_GPIO_ENABLE_REG, &priv-
>>> gpio_en);
>> +
>> +	/* Save GPIO IO Addr to private data */
>>
>
>> +	pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp);
>> +	priv->gpio_ioaddr = tmp << 8;
>> +	pci_read_config_byte(dev, F81504_GPIO_IO_LSB_REG, &tmp);
>> +	priv->gpio_ioaddr |= tmp;
>
> One read as word?

It can't read as word. The "F81504_GPIO_IO_LSB_REG" is 0xf1, It
seems can't be read word/dword from a odd address.

I'll remain it.


>> +	mfd_remove_devices(&dev->dev);
>
> mfd_add_devices takes care.
>
>> +	pci_disable_device(dev);
>
> pcim_ takes case.

ok

>> +	return status;
>> +}
>> +
>> +static void f81504_remove(struct pci_dev *dev)
>> +{
>
>> +	mfd_remove_devices(&dev->dev);
>> +	pci_disable_device(dev);
>
> Ditto.

ok


>> +static int f81504_resume(struct device *dev)
>> +{
>> +	int status;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	status = pci_enable_device(pdev);
>> +	if (status)
>> +		return status;
>
> It's done by PCI core I believe.

ok

>> +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},
>> +	{}
>
> So, if you have default y for this and 8250_pci, which one will be
> loaded?

I had tested on x86, It'll handle by 8250_pci.c when this & 8250_pci.c
all built-in mode.

BTW, due to the series patches across 3 subsystems MFD/GPIO/8250.
I make the series patches under MFD subsystem, but the buildbot
report git repository conflict with GPIO & 8250 (patch 2 & 3).

Should I split the series patches into 3 independent patch and
based on their maintainer git repository?

>> +static struct pci_driver f81504_driver = {
>> +	.name = F81504_DRIVER_NAME,
>> +	.probe = f81504_probe,
>> +	.remove = f81504_remove,
>> +	.driver		= {
>> +		.pm	= &f81504_pm_ops,
>
>
>> +		.owner	= THIS_MODULE,
>
> kbuild  already complained about.

ok

>> diff --git a/include/linux/mfd/f81504.h b/include/linux/mfd/f81504.h
>> new file mode 100644
>> index 0000000..13bd0ae
>> --- /dev/null
>> +++ b/include/linux/mfd/f81504.h
>> @@ -0,0 +1,52 @@
>> +#ifndef __F81504_H__
>
> __MFD_F…

ok

Thanks for your advices
-- 
With Best Regards,
Peter Hung

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Lee Jones]

On Fri, 29 Jan 2016, Peter Hung wrote:
> >>+obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o
> >
> >I think '_' is better than '-'. What I saw and usually do is '_' for
> >regular source modules and '-' for the resulting objects when they have
> >more than one file.
> 
> I used f81504_core.c originally, but I found most of files are named
> xxx-ooo.c when I try to modify makefile. Should I change it to
> f81504_core.c ??

I prefer '-' in MFD.

> >>+#define F81504_DEV_DESC		"Fintek F81504/508/512 PCIE-
> >>to-UART core"
> >
> >Do you need this definition? Is it used more than once?
> 
> ok, I'll direct use the string without define.

Please rid all of the {DEV_NAME,DEV_DEC} defines, they only serve to
obfuscate code.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Lee Jones]

On Fri, 29 Jan 2016, Peter Hung wrote:
> >>+obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o
> >
> >I think '_' is better than '-'. What I saw and usually do is '_' for
> >regular source modules and '-' for the resulting objects when they have
> >more than one file.
> 
> I used f81504_core.c originally, but I found most of files are named
> xxx-ooo.c when I try to modify makefile. Should I change it to
> f81504_core.c ??

I prefer '-' in MFD.

> >>+#define F81504_DEV_DESC		"Fintek F81504/508/512 PCIE-
> >>to-UART core"
> >
> >Do you need this definition? Is it used more than once?
> 
> ok, I'll direct use the string without define.

Please rid all of the {DEV_NAME,DEV_DEC} defines, they only serve to
obfuscate code.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Andy Shevchenko]

Thin out Cc list since mostly off topic message here.

On Fri, 2016-01-29 at 08:21 +0000, Lee Jones wrote:

> I prefer '-' in MFD.

Have to memorize that.

/* A bit of off topic */
I will cook and send one patch regarding to Intel Quark SoC, i.e. UART
driver, but I'm not sure it's a good idea now to use there - since
there are already too many intel_ files with _ including existing
drivers for Quark.

What do you think?

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

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Lee Jones]

On Fri, 29 Jan 2016, Andy Shevchenko wrote:

> Thin out Cc list since mostly off topic message here.
> 
> On Fri, 2016-01-29 at 08:21 +0000, Lee Jones wrote:
> 
> > I prefer '-' in MFD.
> 
> Have to memorize that.
> 
> /* A bit of off topic */
> I will cook and send one patch regarding to Intel Quark SoC, i.e. UART
> driver, but I'm not sure it's a good idea now to use there - since
> there are already too many intel_ files with _ including existing
> drivers for Quark.
> 
> What do you think?

Where is the UART driver?  If it's in with the serial devices, you'd
have to speak to the maintainer there.  Probably best to just submit
and let the maintainer let you know if something is incorrect.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Andy Shevchenko]

On Fri, 2016-01-29 at 13:50 +0800, Peter Hung wrote:

> Andy Shevchenko 於 2016/1/28 下午 07:55 寫道:
> > > +obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o

> +static int f81504_port_init(struct pci_dev *dev)
> > > +{
> > > +	size_t i, j;
> > > +	u32 max_port, iobase, gpio_addr;
> > > +	u32 bar_data[3];
> > > +	u16 tmp;
> > > +	u8 config_base, gpio_en, f0h_data, f3h_data;
> > > +	bool is_gpio;
> > > +	struct f81504_pci_private *priv = pci_get_drvdata(dev);
> > 
> > I would suggest to rearrange definition block here (and in the rest
> > of
> > the functions in entire series) to somehow follow the following
> > pattern
> > 
> > 1) assignments go first, especially container_of() based ones;
> > 2) group variables by meaning and put longer lines upper;
> > 3) return value variable, if exists, goes last.
> > 
> > Besides that choose types carefully. If there is known limit for
> > counters, no need to define wider type than needed. Use proper
> > types
> > for corresponding values.
> 
> I'll try to rearrange the definition blocks.


> For the counter issue, some senior recommend me to change count from
> int
> to size_t for performance.

Wow, how come? On which arch?

Also mark this as (1) to refer below.

>  In this case, should I use u8 to replace
> i & j ? It should be less than 12 & 6.

At least you tell compiler that it may use any suitable type. In any
case the last decision is by compiler if you don't do any tricks.

So, I suggest to use non-fixed width type like (unsigned) int and leave
everything else on compiler.


> > > +
> > > +	/* Init GPIO IO Address */
> > > +	pci_read_config_dword(dev, 0x18, &gpio_addr);
> > > +	gpio_addr &= 0xffffffe0;
> > 
> > 
> > > +	pci_write_config_byte(dev, F81504_GPIO_IO_LSB_REG,
> > > gpio_addr
> > > & 0xff);
> > > +	pci_write_config_byte(dev, F81504_GPIO_IO_MSB_REG,
> > > (gpio_addr >> 8) &
> > > +			0xff);
> > 
> > Could it be written at once as a word?
> 
> I tested with writing a word to "F81504_GPIO_IO_LSB_REG", but
> it'll failed, so I'll remain it.

Please, add a comment line above.

> 
> > > +	if (priv) {
> > > +		/* Reinit from resume(), read the previous value
> > > from priv */
> > > 
> > 
> > > +		gpio_en = priv->gpio_en;
> > 
> > I would suggest to move this line down to follow same pattern in
> > else
> > branch.

I'm talking here to make simple rearrangement like

if () {
 …
 gpio_en = …
} else {
…
 gpio_en = …
}

Thus the gpio_en assignment goes last in both branches.

> > > +
> > > +		pci_write_config_byte(dev, config_base + 0x06,
> > > dev-
> > > > irq);
> > > +
> > > +		/*
> > > +		 * Force init to RS232 / Share Mode, recovery
> > > previous mode
> > > +		 * will done in F81504 8250 platform driver
> > > resume()
> > 
> > Period at the end of sentences in multi-line comments.
> 
> Sorry, what this mean for ?
> I cant use multi-line comments in the end ??

Sentences are started with capital letters and end with period '.'
character, like this one.


> > > +	for (i = 0; i < max_port; ++i) {
> > > +		/* Check UART is enabled */
> > > 
> > 
> > > +		pci_read_config_byte(dev, F81504_UART_START_ADDR
> > > + i
> > > *
> > > +				F81504_UART_OFFSET, &tmp);
> > > +		if (!tmp)
> > > +			continue;
> > 
> > So, this is the same as below, right?
> > 
> > > +
> > > +		/* Get UART IO Address */
> > > +		pci_read_config_word(dev, F81504_UART_START_ADDR
> > > + i
> > > *
> > > +				F81504_UART_OFFSET + 4,
> > > &iobase);
> > 
> > …why not to check here?
> 
> It's a bit difference, this section will check if UART enabled (tmp).
> It'll generate platform device and get IO address when tmp != 0. If
> tmp = 0, skip this idx, dont need to get current IO address and try
> with next.

Yeah, I missed that the offset is different.

> +		if (status) {
> > > +			dev_warn(&dev->dev, "%s: add device
> > > failed:
> > > %d\n",
> > > +					__func__, status);
> > 
> > Indent _ under &.
> 
> Some senior recommend me to do at least 2-tabs to do indent when
> code cross multi-line. So I'll use to 2-tabs from "dev" to do indent.

> How should I do with indent ?? It seems no consensus, but Lindent
> will do indent like your comments.

I would suggest to use what is used in most recent drivers and modules.
I don't remember that 2 tabs fact is somehow reflected in CodingStyle.

Maybe your seniour was talkin about multi-line function definition?

> +		f81504_gpio_cell.pdata_size = sizeof(i);
> > 
> > Static. The problem here is badly chosen type of i. Like I
> > mentioned at
> > the top of review…
> 
> I'll try to rewrite it with pass a structure. It seems more make
> sense.

That's correct on one side, on the other I'm not sure you got my
comment. size_t is arch-dependent type, sizeof() is not the same
everywhere.

> +	pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp);
> > > +	priv->gpio_ioaddr = tmp << 8;
> > > +	pci_read_config_byte(dev, F81504_GPIO_IO_LSB_REG, &tmp);
> > > +	priv->gpio_ioaddr |= tmp;
> > 
> > One read as word?
> 
> It can't read as word. The "F81504_GPIO_IO_LSB_REG" is 0xf1, It
> seems can't be read word/dword from a odd address.
> 
> I'll remain it.

Put comment about that.

> 
> > > +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},
> > > +	{}
> > 
> > So, if you have default y for this and 8250_pci, which one will be
> > loaded?
> 
> I had tested on x86, It'll handle by 8250_pci.c when this &
> 8250_pci.c
> all built-in mode.

Yeah. here is the problem. When you introduce that you have to be sure
that no-one will try to build kernel with both included right now.

To avoid potential problem you may split the driver to main part and
Kconfig enabling part. Lee, what is your advice here?

> 
> BTW, due to the series patches across 3 subsystems MFD/GPIO/8250.
> I make the series patches under MFD subsystem, but the buildbot
> report git repository conflict with GPIO & 8250 (patch 2 & 3).
> 
> Should I split the series patches into 3 independent patch and
> based on their maintainer git repository?

It should go via one subsystem, otherwise user may end up with non-
working interim kernel version, i.e. when bisecting.

In case if it based on some stuff which is not in upstream yet, you
have to describe this carefully to maintainers that they may create
immutable branches and cross-apply the stuff. But I suppose it's not
your case and your series is quite straightforward.


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

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/29 下午 09:41 寫道:
> On Fri, 2016-01-29 at 13:50 +0800, Peter Hung wrote:
>> Andy Shevchenko 於 2016/1/28 下午 07:55 寫道:
>>> I would suggest to rearrange definition block here (and in the rest
>>> of
>>> the functions in entire series) to somehow follow the following
>>> pattern
>>>
>>> 1) assignments go first, especially container_of() based ones;
>>> 2) group variables by meaning and put longer lines upper;
>>> 3) return value variable, if exists, goes last.
>>>
>>> Besides that choose types carefully. If there is known limit for
>>> counters, no need to define wider type than needed. Use proper
>>> types
>>> for corresponding values.
>>
>> I'll try to rearrange the definition blocks.
>
>
>> For the counter issue, some senior recommend me to change count from
>> int
>> to size_t for performance.
>
> Wow, how come? On which arch?
>
> Also mark this as (1) to refer below.
>
>>   In this case, should I use u8 to replace
>> i & j ? It should be less than 12 & 6.
>
> At least you tell compiler that it may use any suitable type. In any
> case the last decision is by compiler if you don't do any tricks.
>
> So, I suggest to use non-fixed width type like (unsigned) int and leave
> everything else on compiler.

Sorry for my misunderstanding, not for performance.
The senior just recommend me to replace all size/count variables
to size_t.

https://lkml.org/lkml/2016/1/3/193

size_t in x86 is unsigned int, x86_64 is unsigned long, It maybe
good for prefetch or match register size I guess.

I'll make the size_t of series patches to unsigned int.

>>>> +	pci_write_config_byte(dev, F81504_GPIO_IO_LSB_REG,
>>>> gpio_addr
>>>> & 0xff);
>>>> +	pci_write_config_byte(dev, F81504_GPIO_IO_MSB_REG,
>>>> (gpio_addr >> 8) &
>>>> +			0xff);
>>>
>>> Could it be written at once as a word?
>>
>> I tested with writing a word to "F81504_GPIO_IO_LSB_REG", but
>> it'll failed, so I'll remain it.
>
> Please, add a comment line above.

ok


>>
>>>> +	if (priv) {
>>>> +		/* Reinit from resume(), read the previous value
>>>> from priv */
>>>>
>>>
>>>> +		gpio_en = priv->gpio_en;
>>>
>>> I would suggest to move this line down to follow same pattern in
>>> else
>>> branch.
>
> I'm talking here to make simple rearrangement like
>
> if () {
>   …
>   gpio_en = …
> } else {
> …
>   gpio_en = …
> }
>
> Thus the gpio_en assignment goes last in both branches.

ok


>>>> +
>>>> +		pci_write_config_byte(dev, config_base + 0x06,
>>>> dev-
>>>>> irq);
>>>> +
>>>> +		/*
>>>> +		 * Force init to RS232 / Share Mode, recovery
>>>> previous mode
>>>> +		 * will done in F81504 8250 platform driver
>>>> resume()
>>>
>>> Period at the end of sentences in multi-line comments.
>>
>> Sorry, what this mean for ?
>> I cant use multi-line comments in the end ??
>
> Sentences are started with capital letters and end with period '.'
> character, like this one.

ok.


>> +		if (status) {
>>>> +			dev_warn(&dev->dev, "%s: add device
>>>> failed:
>>>> %d\n",
>>>> +					__func__, status);
>>>
>>> Indent _ under &.
>>
>> Some senior recommend me to do at least 2-tabs to do indent when
>> code cross multi-line. So I'll use to 2-tabs from "dev" to do indent.
>
>> How should I do with indent ?? It seems no consensus, but Lindent
>> will do indent like your comments.
>
> I would suggest to use what is used in most recent drivers and modules.
> I don't remember that 2 tabs fact is somehow reflected in CodingStyle.
>
> Maybe your seniour was talkin about multi-line function definition?

ok, I'll indent multi-line statement as your comment and multi-line
function with 2 tabs.


>> +		f81504_gpio_cell.pdata_size = sizeof(i);
>>>
>>> Static. The problem here is badly chosen type of i. Like I
>>> mentioned at
>>> the top of review…
>>
>> I'll try to rewrite it with pass a structure. It seems more make
>> sense.
>
> That's correct on one side, on the other I'm not sure you got my
> comment. size_t is arch-dependent type, sizeof() is not the same
> everywhere.

I'll change it to pass unsigned int.


>> +	pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp);
>>>> +	priv->gpio_ioaddr = tmp << 8;
>>>> +	pci_read_config_byte(dev, F81504_GPIO_IO_LSB_REG, &tmp);
>>>> +	priv->gpio_ioaddr |= tmp;
>>>
>>> One read as word?
>>
>> It can't read as word. The "F81504_GPIO_IO_LSB_REG" is 0xf1, It
>> seems can't be read word/dword from a odd address.
>>
>> I'll remain it.
>
> Put comment about that.

ok


>>> So, if you have default y for this and 8250_pci, which one will be
>>> loaded?
>>
>> I had tested on x86, It'll handle by 8250_pci.c when this &
>> 8250_pci.c
>> all built-in mode.
>
> Yeah. here is the problem. When you introduce that you have to be sure
> that no-one will try to build kernel with both included right now.

Paul had recommend me to reduce the impact of code change. I'll remove
all F81504/508/512 code in 8250_pci.c until the series patches had
applied.

The device will handle with 8250_pci.c before applide patch no4, and
handle with f81504_code.c when applied patch no4. This maybe reduce the
bisect misleading.

>>
>> BTW, due to the series patches across 3 subsystems MFD/GPIO/8250.
>> I make the series patches under MFD subsystem, but the buildbot
>> report git repository conflict with GPIO & 8250 (patch 2 & 3).
>>
>> Should I split the series patches into 3 independent patch and
>> based on their maintainer git repository?
>
> It should go via one subsystem, otherwise user may end up with non-
> working interim kernel version, i.e. when bisecting.
>
> In case if it based on some stuff which is not in upstream yet, you
> have to describe this carefully to maintainers that they may create
> immutable branches and cross-apply the stuff. But I suppose it's not
> your case and your series is quite straightforward.
>

I'll still try the series patch based on MFD subsystem.
Thanks for your advices.

-- 
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/29 下午 09:41 寫道:
> On Fri, 2016-01-29 at 13:50 +0800, Peter Hung wrote:
>> Andy Shevchenko 於 2016/1/28 下午 07:55 寫道:
>>> I would suggest to rearrange definition block here (and in the rest
>>> of
>>> the functions in entire series) to somehow follow the following
>>> pattern
>>>
>>> 1) assignments go first, especially container_of() based ones;
>>> 2) group variables by meaning and put longer lines upper;
>>> 3) return value variable, if exists, goes last.
>>>
>>> Besides that choose types carefully. If there is known limit for
>>> counters, no need to define wider type than needed. Use proper
>>> types
>>> for corresponding values.
>>
>> I'll try to rearrange the definition blocks.
>
>
>> For the counter issue, some senior recommend me to change count from
>> int
>> to size_t for performance.
>
> Wow, how come? On which arch?
>
> Also mark this as (1) to refer below.
>
>>   In this case, should I use u8 to replace
>> i & j ? It should be less than 12 & 6.
>
> At least you tell compiler that it may use any suitable type. In any
> case the last decision is by compiler if you don't do any tricks.
>
> So, I suggest to use non-fixed width type like (unsigned) int and leave
> everything else on compiler.

Sorry for my misunderstanding, not for performance.
The senior just recommend me to replace all size/count variables
to size_t.

https://lkml.org/lkml/2016/1/3/193

size_t in x86 is unsigned int, x86_64 is unsigned long, It maybe
good for prefetch or match register size I guess.

I'll make the size_t of series patches to unsigned int.

>>>> +	pci_write_config_byte(dev, F81504_GPIO_IO_LSB_REG,
>>>> gpio_addr
>>>> & 0xff);
>>>> +	pci_write_config_byte(dev, F81504_GPIO_IO_MSB_REG,
>>>> (gpio_addr >> 8) &
>>>> +			0xff);
>>>
>>> Could it be written at once as a word?
>>
>> I tested with writing a word to "F81504_GPIO_IO_LSB_REG", but
>> it'll failed, so I'll remain it.
>
> Please, add a comment line above.

ok


>>
>>>> +	if (priv) {
>>>> +		/* Reinit from resume(), read the previous value
>>>> from priv */
>>>>
>>>
>>>> +		gpio_en = priv->gpio_en;
>>>
>>> I would suggest to move this line down to follow same pattern in
>>> else
>>> branch.
>
> I'm talking here to make simple rearrangement like
>
> if () {
>   …
>   gpio_en = …
> } else {
> …
>   gpio_en = …
> }
>
> Thus the gpio_en assignment goes last in both branches.

ok


>>>> +
>>>> +		pci_write_config_byte(dev, config_base + 0x06,
>>>> dev-
>>>>> irq);
>>>> +
>>>> +		/*
>>>> +		 * Force init to RS232 / Share Mode, recovery
>>>> previous mode
>>>> +		 * will done in F81504 8250 platform driver
>>>> resume()
>>>
>>> Period at the end of sentences in multi-line comments.
>>
>> Sorry, what this mean for ?
>> I cant use multi-line comments in the end ??
>
> Sentences are started with capital letters and end with period '.'
> character, like this one.

ok.


>> +		if (status) {
>>>> +			dev_warn(&dev->dev, "%s: add device
>>>> failed:
>>>> %d\n",
>>>> +					__func__, status);
>>>
>>> Indent _ under &.
>>
>> Some senior recommend me to do at least 2-tabs to do indent when
>> code cross multi-line. So I'll use to 2-tabs from "dev" to do indent.
>
>> How should I do with indent ?? It seems no consensus, but Lindent
>> will do indent like your comments.
>
> I would suggest to use what is used in most recent drivers and modules.
> I don't remember that 2 tabs fact is somehow reflected in CodingStyle.
>
> Maybe your seniour was talkin about multi-line function definition?

ok, I'll indent multi-line statement as your comment and multi-line
function with 2 tabs.


>> +		f81504_gpio_cell.pdata_size = sizeof(i);
>>>
>>> Static. The problem here is badly chosen type of i. Like I
>>> mentioned at
>>> the top of review…
>>
>> I'll try to rewrite it with pass a structure. It seems more make
>> sense.
>
> That's correct on one side, on the other I'm not sure you got my
> comment. size_t is arch-dependent type, sizeof() is not the same
> everywhere.

I'll change it to pass unsigned int.


>> +	pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp);
>>>> +	priv->gpio_ioaddr = tmp << 8;
>>>> +	pci_read_config_byte(dev, F81504_GPIO_IO_LSB_REG, &tmp);
>>>> +	priv->gpio_ioaddr |= tmp;
>>>
>>> One read as word?
>>
>> It can't read as word. The "F81504_GPIO_IO_LSB_REG" is 0xf1, It
>> seems can't be read word/dword from a odd address.
>>
>> I'll remain it.
>
> Put comment about that.

ok


>>> So, if you have default y for this and 8250_pci, which one will be
>>> loaded?
>>
>> I had tested on x86, It'll handle by 8250_pci.c when this &
>> 8250_pci.c
>> all built-in mode.
>
> Yeah. here is the problem. When you introduce that you have to be sure
> that no-one will try to build kernel with both included right now.

Paul had recommend me to reduce the impact of code change. I'll remove
all F81504/508/512 code in 8250_pci.c until the series patches had
applied.

The device will handle with 8250_pci.c before applide patch no4, and
handle with f81504_code.c when applied patch no4. This maybe reduce the
bisect misleading.

>>
>> BTW, due to the series patches across 3 subsystems MFD/GPIO/8250.
>> I make the series patches under MFD subsystem, but the buildbot
>> report git repository conflict with GPIO & 8250 (patch 2 & 3).
>>
>> Should I split the series patches into 3 independent patch and
>> based on their maintainer git repository?
>
> It should go via one subsystem, otherwise user may end up with non-
> working interim kernel version, i.e. when bisecting.
>
> In case if it based on some stuff which is not in upstream yet, you
> have to describe this carefully to maintainers that they may create
> immutable branches and cross-apply the stuff. But I suppose it's not
> your case and your series is quite straightforward.
>

I'll still try the series patch based on MFD subsystem.
Thanks for your advices.

-- 
With Best Regards,
Peter Hung

[PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[Peter Hung]

This driver is GPIOLIB driver for F81504/508/512, it'll handle the
GPIOLIB operation of this device. This module will depend on
MFD_FINTEK_F81504_CORE.

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

GPIO register:
PCI Configuration space:
    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
    F1h: IO address (LSB)
    F2h: IO address (MSB)
    F8h + 8 * set: Direction control (bitwise)
         bitx: 0 - Input mode
         bitx: 1 - Output mode
    F9h + 8 * set: Drive ability control (bitwise)
         bitx: 0 - Open drain (default)
         bitx: 1 - Push Pull
         In this driver, we only implements open drain mode.

IO space:
    (IO base + 0~5): GPIO-0x~5x in/out value (bitwise)

Suggested-by: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/gpio/Kconfig       |  10 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-f81504.c | 257 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 268 insertions(+)
 create mode 100644 drivers/gpio/gpio-f81504.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b18bea0..633a65f 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -900,6 +900,16 @@ config GPIO_WM8994
 	  Say yes here to access the GPIO signals of WM8994 audio hub
 	  CODECs from Wolfson Microelectronics.
 
+config GPIO_F81504
+        tristate "Fintek F81504/508/512 PCIE-to-UART/GPIO support"
+        depends on MFD_FINTEK_F81504_CORE
+        select MFD_CORE
+        help
+          Say yes here to support the GPIO functionality of Fintek
+          F81504/508/512 PCIE-to-UART/GPIO.
+
+          If unsure, say N.
+
 endmenu
 
 menu "PCI GPIO expanders"
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 986dbd8..06fb240 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -111,6 +111,7 @@ obj-$(CONFIG_GPIO_VX855)	+= gpio-vx855.o
 obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
 obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
 obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
+obj-$(CONFIG_GPIO_F81504)       += gpio-f81504.o
 obj-$(CONFIG_GPIO_XGENE)	+= gpio-xgene.o
 obj-$(CONFIG_GPIO_XGENE_SB)	+= gpio-xgene-sb.o
 obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
diff --git a/drivers/gpio/gpio-f81504.c b/drivers/gpio/gpio-f81504.c
new file mode 100644
index 0000000..817b926
--- /dev/null
+++ b/drivers/gpio/gpio-f81504.c
@@ -0,0 +1,257 @@
+/*
+ * Copyright (C) 2016 Fintek Corporation
+ * Based on gpio-mpc8xxx.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/pci.h>
+#include <linux/mfd/f81504.h>
+
+struct f81504_gpio_chip {
+	struct gpio_chip chip;
+	struct mutex locker;
+	u8 idx;
+	u8 save_out_en;
+	u8 save_drive_en;
+	u8 save_value;
+};
+
+static struct f81504_gpio_chip *gpio_to_f81504_chip(struct gpio_chip *chip)
+{
+	return container_of(chip, struct f81504_gpio_chip, chip);
+}
+
+static int f81504_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	u8 tmp;
+	struct f81504_gpio_chip *gc = gpio_to_f81504_chip(chip);
+	struct platform_device *pdev = to_platform_device(chip->dev);
+	struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
+
+	mutex_lock(&gc->locker);
+
+	/* set input mode */
+	pci_read_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET + F81504_GPIO_OUT_EN_OFFSET,
+			&tmp);
+	pci_write_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET + F81504_GPIO_OUT_EN_OFFSET,
+			tmp & ~BIT(offset));
+
+	mutex_unlock(&gc->locker);
+	return 0;
+}
+
+static int f81504_gpio_direction_out(struct gpio_chip *chip, unsigned offset,
+		int value)
+{
+	u8 tmp;
+	struct f81504_gpio_chip *gc = gpio_to_f81504_chip(chip);
+	struct platform_device *pdev = to_platform_device(chip->dev);
+	struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
+	struct f81504_pci_private *priv = pci_get_drvdata(pci_dev);
+
+	mutex_lock(&gc->locker);
+
+	/* set output mode */
+	pci_read_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET + F81504_GPIO_OUT_EN_OFFSET,
+			&tmp);
+	pci_write_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET + F81504_GPIO_OUT_EN_OFFSET,
+			tmp | BIT(offset));
+
+	/*
+	 * 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 + gc->idx);
+
+	if (value)
+		outb(tmp | BIT(offset), priv->gpio_ioaddr + gc->idx);
+	else
+		outb(tmp & ~BIT(offset), priv->gpio_ioaddr + gc->idx);
+
+	mutex_unlock(&gc->locker);
+	return 0;
+}
+
+static int f81504_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+	u8 tmp;
+	struct f81504_gpio_chip *gc = gpio_to_f81504_chip(chip);
+	struct platform_device *pdev = to_platform_device(chip->dev);
+	struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
+
+	mutex_lock(&gc->locker);
+	pci_read_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET, &tmp);
+	mutex_unlock(&gc->locker);
+
+	if (tmp & BIT(offset))
+		return GPIOF_DIR_OUT;
+
+	return GPIOF_DIR_IN;
+}
+
+static int f81504_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	int tmp;
+	struct f81504_gpio_chip *gc = gpio_to_f81504_chip(chip);
+	struct platform_device *pdev = to_platform_device(chip->dev);
+	struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
+	struct f81504_pci_private *priv = pci_get_drvdata(pci_dev);
+
+	mutex_lock(&gc->locker);
+	tmp = inb(priv->gpio_ioaddr + gc->idx);
+	mutex_unlock(&gc->locker);
+
+	return !!(tmp & BIT(offset));
+}
+
+static void f81504_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	f81504_gpio_direction_out(chip, offset, value);
+}
+
+static int f81504_gpio_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
+	struct f81504_pci_private *priv = pci_get_drvdata(pci_dev);
+	struct f81504_gpio_chip *gc = platform_get_drvdata(pdev);
+
+	mutex_lock(&gc->locker);
+	pci_read_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET + F81504_GPIO_OUT_EN_OFFSET,
+			&gc->save_out_en);
+
+	pci_read_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET + F81504_GPIO_DRIVE_EN_OFFSET,
+			&gc->save_drive_en);
+
+	gc->save_value = inb(priv->gpio_ioaddr + gc->idx);
+	mutex_unlock(&gc->locker);
+	return 0;
+}
+
+static int f81504_gpio_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
+	struct f81504_pci_private *priv = pci_get_drvdata(pci_dev);
+	struct f81504_gpio_chip *gc = platform_get_drvdata(pdev);
+
+	mutex_lock(&gc->locker);
+	pci_write_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET + F81504_GPIO_OUT_EN_OFFSET,
+			gc->save_out_en);
+
+	pci_write_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
+			F81504_GPIO_SET_OFFSET + F81504_GPIO_DRIVE_EN_OFFSET,
+			gc->save_drive_en);
+
+	outb(gc->save_value, priv->gpio_ioaddr + gc->idx);
+	mutex_unlock(&gc->locker);
+	return 0;
+}
+
+static int f81504_gpio_probe(struct platform_device *pdev)
+{
+	int status;
+	struct f81504_gpio_chip *gc;
+	void *data = dev_get_platdata(&pdev->dev);
+	u8 gpio_idx = *(u8 *)data;
+	char *name;
+
+	if (gpio_idx >= ARRAY_SIZE(fintek_gpio_mapping)) {
+		dev_err(&pdev->dev, "%s: gpio_idx:%d out of range.\n",
+				__func__, gpio_idx);
+		return -ENODEV;
+	}
+
+	gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
+	if (!gc)
+		return -ENOMEM;
+
+	kfree(data);
+	mutex_init(&gc->locker);
+	platform_set_drvdata(pdev, gc);
+
+	name = devm_kzalloc(&pdev->dev, FINTEK_GPIO_NAME_LEN, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+
+	/* This will display like as GPIO-1x */
+	sprintf(name, "%s-%dx", FINTEK_GPIO_DISPLAY, gpio_idx);
+
+	gc->chip.owner = THIS_MODULE;
+	gc->chip.label = name;
+	gc->chip.ngpio = 8;
+	gc->chip.dev = &pdev->dev;
+	gc->chip.get = f81504_gpio_get;
+	gc->chip.set = f81504_gpio_set;
+	gc->chip.direction_input = f81504_gpio_direction_in;
+	gc->chip.direction_output = f81504_gpio_direction_out;
+	gc->chip.get_direction = f81504_gpio_get_direction;
+	gc->chip.can_sleep = 1;
+	gc->chip.base = -1;
+	gc->idx = gpio_idx;
+
+	status = gpiochip_add(&gc->chip);
+	if (status) {
+		dev_err(&pdev->dev, "%s: gpiochip_add failed: %d\n", __func__,
+				status);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int f81504_gpio_remove(struct platform_device *pdev)
+{
+	struct f81504_gpio_chip *gc = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&gc->chip);
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(f81504_gpio_pm_ops, f81504_gpio_suspend,
+		f81504_gpio_resume);
+
+static struct platform_driver f81504_gpio_driver = {
+	.driver = {
+		.name	= F81504_GPIO_NAME,
+		.owner	= THIS_MODULE,
+		.pm     = &f81504_gpio_pm_ops,
+	},
+	.probe		= f81504_gpio_probe,
+	.remove		= f81504_gpio_remove,
+};
+
+module_platform_driver(f81504_gpio_driver);
+
+MODULE_AUTHOR("Peter Hong <Peter_Hong@fintek.com.tw>");
+MODULE_DESCRIPTION("Fintek F81504/508/512 PCIE GPIOLIB driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5319 bytes --]

Hi Peter,

[auto build test ERROR on gpio/for-next]
[also build test ERROR on v4.5-rc1 next-20160128]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Hung/Transform-Fintek-PCIE-driver-from-8250-to-MFD/20160128-172929
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: i386-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/list.h:8:0,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from include/linux/platform_device.h:14,
                    from drivers/gpio/gpio-f81504.c:18:
   drivers/gpio/gpio-f81504.c: In function 'f81504_gpio_direction_in':
>> drivers/gpio/gpio-f81504.c:41:56: error: 'struct gpio_chip' has no member named 'dev'
     struct platform_device *pdev = to_platform_device(chip->dev);
                                                           ^
   include/linux/kernel.h:841:49: note: in definition of macro 'container_of'
     const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                    ^
>> drivers/gpio/gpio-f81504.c:41:33: note: in expansion of macro 'to_platform_device'
     struct platform_device *pdev = to_platform_device(chip->dev);
                                    ^
   drivers/gpio/gpio-f81504.c: In function 'f81504_gpio_direction_out':
   drivers/gpio/gpio-f81504.c:63:56: error: 'struct gpio_chip' has no member named 'dev'
     struct platform_device *pdev = to_platform_device(chip->dev);
                                                           ^
   include/linux/kernel.h:841:49: note: in definition of macro 'container_of'
     const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                    ^
   drivers/gpio/gpio-f81504.c:63:33: note: in expansion of macro 'to_platform_device'
     struct platform_device *pdev = to_platform_device(chip->dev);
                                    ^
   drivers/gpio/gpio-f81504.c: In function 'f81504_gpio_get_direction':
   drivers/gpio/gpio-f81504.c:104:56: error: 'struct gpio_chip' has no member named 'dev'
     struct platform_device *pdev = to_platform_device(chip->dev);
                                                           ^
   include/linux/kernel.h:841:49: note: in definition of macro 'container_of'
     const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                    ^
   drivers/gpio/gpio-f81504.c:104:33: note: in expansion of macro 'to_platform_device'
     struct platform_device *pdev = to_platform_device(chip->dev);
                                    ^
   drivers/gpio/gpio-f81504.c: In function 'f81504_gpio_get':
   drivers/gpio/gpio-f81504.c:122:56: error: 'struct gpio_chip' has no member named 'dev'
     struct platform_device *pdev = to_platform_device(chip->dev);
                                                           ^
   include/linux/kernel.h:841:49: note: in definition of macro 'container_of'
     const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                    ^
   drivers/gpio/gpio-f81504.c:122:33: note: in expansion of macro 'to_platform_device'
     struct platform_device *pdev = to_platform_device(chip->dev);
                                    ^
   drivers/gpio/gpio-f81504.c: In function 'f81504_gpio_probe':
   drivers/gpio/gpio-f81504.c:212:10: error: 'struct gpio_chip' has no member named 'dev'
     gc->chip.dev = &pdev->dev;
             ^

coccinelle warnings: (new ones prefixed by >>)

>> drivers/gpio/gpio-f81504.c:246:3-8: No need to set .owner here. The core will do it.

Please review and possibly fold the followup patch.

vim +41 drivers/gpio/gpio-f81504.c

    12	 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
    13	 * GNU General Public License for more details.
    14	 *
    15	 * You should have received a copy of the GNU General Public License
    16	 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
    17	 */
  > 18	#include <linux/platform_device.h>
    19	#include <linux/gpio.h>
    20	#include <linux/pci.h>
    21	#include <linux/mfd/f81504.h>
    22	
    23	struct f81504_gpio_chip {
    24		struct gpio_chip chip;
    25		struct mutex locker;
    26		u8 idx;
    27		u8 save_out_en;
    28		u8 save_drive_en;
    29		u8 save_value;
    30	};
    31	
    32	static struct f81504_gpio_chip *gpio_to_f81504_chip(struct gpio_chip *chip)
    33	{
    34		return container_of(chip, struct f81504_gpio_chip, chip);
    35	}
    36	
    37	static int f81504_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
    38	{
    39		u8 tmp;
    40		struct f81504_gpio_chip *gc = gpio_to_f81504_chip(chip);
  > 41		struct platform_device *pdev = to_platform_device(chip->dev);
    42		struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
    43	
    44		mutex_lock(&gc->locker);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 53466 bytes --]

[PATCH] gpio: gpio-f81504: fix platform_no_drv_owner.cocci warnings

[kbuild test robot]

drivers/gpio/gpio-f81504.c:246:3-8: No need to set .owner here. The core will do it.

 Remove .owner field if calls are used which set it automatically

Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci

CC: Peter Hung <hpeter@gmail.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 gpio-f81504.c |    1 -
 1 file changed, 1 deletion(-)

--- a/drivers/gpio/gpio-f81504.c
+++ b/drivers/gpio/gpio-f81504.c
@@ -243,7 +243,6 @@ static SIMPLE_DEV_PM_OPS(f81504_gpio_pm_
 static struct platform_driver f81504_gpio_driver = {
 	.driver = {
 		.name	= F81504_GPIO_NAME,
-		.owner	= THIS_MODULE,
 		.pm     = &f81504_gpio_pm_ops,
 	},
 	.probe		= f81504_gpio_probe,

Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[Andy Shevchenko]

On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
> This driver is GPIOLIB driver for F81504/508/512, it'll handle the
> GPIOLIB operation of this device. This module will depend on
> MFD_FINTEK_F81504_CORE.
> 



> +	/*
> +	 * 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 + gc->idx);

ioread8 is a bit better since it automatically works with IO space and
MMIO. But if you are certain you will always have the address in IO
space, you can disregard this comment.

> +static int f81504_gpio_probe(struct platform_device *pdev)
> +{
> +	int status;
> +	struct f81504_gpio_chip *gc;
> +	void *data = dev_get_platdata(&pdev->dev);
> +	u8 gpio_idx = *(u8 *)data;
> +	char *name;
> +
> +	if (gpio_idx >= ARRAY_SIZE(fintek_gpio_mapping)) {
> +		dev_err(&pdev->dev, "%s: gpio_idx:%d out of
> range.\n",
> +				__func__, gpio_idx);
> +		return -ENODEV;
> +	}
> +
> +	gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> +	if (!gc)
> +		return -ENOMEM;
> +
> 

> +	kfree(data);

What the heck?


> +	mutex_init(&gc->locker);
> +	platform_set_drvdata(pdev, gc);
> +
> +	name = devm_kzalloc(&pdev->dev, FINTEK_GPIO_NAME_LEN,
> GFP_KERNEL);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	/* This will display like as GPIO-1x */
> +	sprintf(name, "%s-%dx", FINTEK_GPIO_DISPLAY, gpio_idx);
> +
> +	gc->chip.owner = THIS_MODULE;
> +	gc->chip.label = name;
> +	gc->chip.ngpio = 8;
> +	gc->chip.dev = &pdev->dev;
> +	gc->chip.get = f81504_gpio_get;
> +	gc->chip.set = f81504_gpio_set;
> +	gc->chip.direction_input = f81504_gpio_direction_in;
> +	gc->chip.direction_output = f81504_gpio_direction_out;
> +	gc->chip.get_direction = f81504_gpio_get_direction;
> +	gc->chip.can_sleep = 1;
> +	gc->chip.base = -1;
> +	gc->idx = gpio_idx;
> +
> 

> +	status = gpiochip_add(&gc->chip);
> +	if (status) {
> +		dev_err(&pdev->dev, "%s: gpiochip_add failed: %d\n",
> __func__,
> +				status);
> +		return -ENOMEM;

You ignored the status.

> +	}
> +
> +	return 0;

Perhaps just
return gpiochip_add(); ?

> +}
> +
> +static int f81504_gpio_remove(struct platform_device *pdev)
> +{
> +	struct f81504_gpio_chip *gc = platform_get_drvdata(pdev);
> +
> +	gpiochip_remove(&gc->chip);
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(f81504_gpio_pm_ops, f81504_gpio_suspend,
> +		f81504_gpio_resume);
> +
> +static struct platform_driver f81504_gpio_driver = {
> +	.driver = {
> +		.name	= F81504_GPIO_NAME,
> +		.owner	= THIS_MODULE,
> +		.pm     = &f81504_gpio_pm_ops,
> +	},
> +	.probe		= f81504_gpio_probe,
> +	.remove		= f81504_gpio_remove,
> +};
> +
> +module_platform_driver(f81504_gpio_driver);
> +
> +MODULE_AUTHOR("Peter Hong <Peter_Hong@fintek.com.tw>");
> +MODULE_DESCRIPTION("Fintek F81504/508/512 PCIE GPIOLIB driver");
> +MODULE_LICENSE("GPL");

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

Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/28 下午 08:03 寫道:
> On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
>> +	/* set output data */
>> +	tmp = inb(priv->gpio_ioaddr + gc->idx);
>
> ioread8 is a bit better since it automatically works with IO space and
> MMIO. But if you are certain you will always have the address in IO
> space, you can disregard this comment.

I had only tested on x86 environment currently. We'll try to get an ARM
platform with PCIE to test it. We'll remain it until getting new board.

>> +static int f81504_gpio_probe(struct platform_device *pdev)
>> +{
>> +	int status;
>> +	struct f81504_gpio_chip *gc;
>> +	void *data = dev_get_platdata(&pdev->dev);
>> +	u8 gpio_idx = *(u8 *)data;
>> +	char *name;
>> +
>> +	if (gpio_idx >= ARRAY_SIZE(fintek_gpio_mapping)) {
>> +		dev_err(&pdev->dev, "%s: gpio_idx:%d out of
>> range.\n",
>> +				__func__, gpio_idx);
>> +		return -ENODEV;
>> +	}
>> +
>> +	gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
>> +	if (!gc)
>> +		return -ENOMEM;
>> +
>>
>
>> +	kfree(data);
>
> What the heck?

Sorry for the big mistake, I'd confused for dev_get_platdata() &
platform_set_drvdata(). I'll rewrite this and check 8250_f81504.c
too.

>> +	status = gpiochip_add(&gc->chip);
>> +	if (status) {
>> +		dev_err(&pdev->dev, "%s: gpiochip_add failed: %d\n",
>> __func__,
>> +				status);
>> +		return -ENOMEM;
>
> You ignored the status.
>
>> +	}
>> +
>> +	return 0;
>
> Perhaps just
> return gpiochip_add(); ?

Just return gpiochip_add() seems good.

Thanks your advices
-- 
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/28 下午 08:03 寫道:
> On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
>> +	/* set output data */
>> +	tmp = inb(priv->gpio_ioaddr + gc->idx);
>
> ioread8 is a bit better since it automatically works with IO space and
> MMIO. But if you are certain you will always have the address in IO
> space, you can disregard this comment.

I had only tested on x86 environment currently. We'll try to get an ARM
platform with PCIE to test it. We'll remain it until getting new board.

>> +static int f81504_gpio_probe(struct platform_device *pdev)
>> +{
>> +	int status;
>> +	struct f81504_gpio_chip *gc;
>> +	void *data = dev_get_platdata(&pdev->dev);
>> +	u8 gpio_idx = *(u8 *)data;
>> +	char *name;
>> +
>> +	if (gpio_idx >= ARRAY_SIZE(fintek_gpio_mapping)) {
>> +		dev_err(&pdev->dev, "%s: gpio_idx:%d out of
>> range.\n",
>> +				__func__, gpio_idx);
>> +		return -ENODEV;
>> +	}
>> +
>> +	gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
>> +	if (!gc)
>> +		return -ENOMEM;
>> +
>>
>
>> +	kfree(data);
>
> What the heck?

Sorry for the big mistake, I'd confused for dev_get_platdata() &
platform_set_drvdata(). I'll rewrite this and check 8250_f81504.c
too.

>> +	status = gpiochip_add(&gc->chip);
>> +	if (status) {
>> +		dev_err(&pdev->dev, "%s: gpiochip_add failed: %d\n",
>> __func__,
>> +				status);
>> +		return -ENOMEM;
>
> You ignored the status.
>
>> +	}
>> +
>> +	return 0;
>
> Perhaps just
> return gpiochip_add(); ?

Just return gpiochip_add() seems good.

Thanks your advices
-- 
With Best Regards,
Peter Hung

Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[Linus Walleij]

On Thu, Jan 28, 2016 at 10:20 AM, Peter Hung <hpeter@gmail.com> wrote:

> This driver is GPIOLIB driver for F81504/508/512, it'll handle the
> GPIOLIB operation of this device. This module will depend on
> MFD_FINTEK_F81504_CORE.
>
> 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
>
> GPIO register:
> PCI Configuration space:
>     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
>     F1h: IO address (LSB)
>     F2h: IO address (MSB)
>     F8h + 8 * set: Direction control (bitwise)
>          bitx: 0 - Input mode
>          bitx: 1 - Output mode
>     F9h + 8 * set: Drive ability control (bitwise)
>          bitx: 0 - Open drain (default)
>          bitx: 1 - Push Pull
>          In this driver, we only implements open drain mode.
>
> IO space:
>     (IO base + 0~5): GPIO-0x~5x in/out value (bitwise)
>
> Suggested-by: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>

Overall a very nice and interesting patch set.

> +++ b/drivers/gpio/gpio-f81504.c
(...)
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>

Drivers should just
#include <linux/gpio/driver.h>

> +static struct f81504_gpio_chip *gpio_to_f81504_chip(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct f81504_gpio_chip, chip);
> +}

Avoid this construction in new code.

Use gpiochip_get_data(chip) everywhere that gpio_to_f81504_chip()
is used and register the gpiochip with gpiochip_add_data()
and the code will be simpler.

See any other driver in drivers/gpio for examples, I converted them
all.

> +static int f81504_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> +       u8 tmp;
> +       struct f81504_gpio_chip *gc = gpio_to_f81504_chip(chip);
> +       struct platform_device *pdev = to_platform_device(chip->dev);
> +       struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
> +
> +       mutex_lock(&gc->locker);
> +       pci_read_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
> +                       F81504_GPIO_SET_OFFSET, &tmp);
> +       mutex_unlock(&gc->locker);
> +
> +       if (tmp & BIT(offset))
> +               return GPIOF_DIR_OUT;
> +
> +       return GPIOF_DIR_IN;
> +}

Do not use GPIOF* flags in driver code, these are for the consumer
API. Just return 0/1.

> +       status = gpiochip_add(&gc->chip);

As mentioned, use gpiochip_add_data(&gc->chip, gc);


> +static struct platform_driver f81504_gpio_driver = {
> +       .driver = {
> +               .name   = F81504_GPIO_NAME,
> +               .owner  = THIS_MODULE,

I saw coccinelle was already complaining about this.

Looking forward to v3!

Yours,
Linus Walleij

Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[Linus Walleij]

On Thu, Jan 28, 2016 at 10:20 AM, Peter Hung <hpeter@gmail.com> wrote:

> This driver is GPIOLIB driver for F81504/508/512, it'll handle the
> GPIOLIB operation of this device. This module will depend on
> MFD_FINTEK_F81504_CORE.
>
> 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
>
> GPIO register:
> PCI Configuration space:
>     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
>     F1h: IO address (LSB)
>     F2h: IO address (MSB)
>     F8h + 8 * set: Direction control (bitwise)
>          bitx: 0 - Input mode
>          bitx: 1 - Output mode
>     F9h + 8 * set: Drive ability control (bitwise)
>          bitx: 0 - Open drain (default)
>          bitx: 1 - Push Pull
>          In this driver, we only implements open drain mode.
>
> IO space:
>     (IO base + 0~5): GPIO-0x~5x in/out value (bitwise)
>
> Suggested-by: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>

Overall a very nice and interesting patch set.

> +++ b/drivers/gpio/gpio-f81504.c
(...)
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>

Drivers should just
#include <linux/gpio/driver.h>

> +static struct f81504_gpio_chip *gpio_to_f81504_chip(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct f81504_gpio_chip, chip);
> +}

Avoid this construction in new code.

Use gpiochip_get_data(chip) everywhere that gpio_to_f81504_chip()
is used and register the gpiochip with gpiochip_add_data()
and the code will be simpler.

See any other driver in drivers/gpio for examples, I converted them
all.

> +static int f81504_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> +       u8 tmp;
> +       struct f81504_gpio_chip *gc = gpio_to_f81504_chip(chip);
> +       struct platform_device *pdev = to_platform_device(chip->dev);
> +       struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
> +
> +       mutex_lock(&gc->locker);
> +       pci_read_config_byte(pci_dev, F81504_GPIO_START_ADDR + gc->idx *
> +                       F81504_GPIO_SET_OFFSET, &tmp);
> +       mutex_unlock(&gc->locker);
> +
> +       if (tmp & BIT(offset))
> +               return GPIOF_DIR_OUT;
> +
> +       return GPIOF_DIR_IN;
> +}

Do not use GPIOF* flags in driver code, these are for the consumer
API. Just return 0/1.

> +       status = gpiochip_add(&gc->chip);

As mentioned, use gpiochip_add_data(&gc->chip, gc);


> +static struct platform_driver f81504_gpio_driver = {
> +       .driver = {
> +               .name   = F81504_GPIO_NAME,
> +               .owner  = THIS_MODULE,

I saw coccinelle was already complaining about this.

Looking forward to v3!

Yours,
Linus Walleij

Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[Peter Hung]

Hi Linus,

Linus Walleij 於 2016/2/10 下午 05:08 寫道:
> On Thu, Jan 28, 2016 at 10:20 AM, Peter Hung <hpeter@gmail.com> wrote:
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio.h>
>
> Drivers should just
> #include <linux/gpio/driver.h>

ok.

>> +static struct f81504_gpio_chip *gpio_to_f81504_chip(struct gpio_chip *chip)
>> +{
>> +       return container_of(chip, struct f81504_gpio_chip, chip);
>> +}
>
> Avoid this construction in new code.
>
> Use gpiochip_get_data(chip) everywhere that gpio_to_f81504_chip()
> is used and register the gpiochip with gpiochip_add_data()
> and the code will be simpler.
>
> See any other driver in drivers/gpio for examples, I converted them
> all.

ok. I'll re-write this section.

>> +
>> +       if (tmp & BIT(offset))
>> +               return GPIOF_DIR_OUT;
>> +
>> +       return GPIOF_DIR_IN;
>> +}
>
> Do not use GPIOF* flags in driver code, these are for the consumer
> API. Just return 0/1.

ok

>> +       status = gpiochip_add(&gc->chip);
>
> As mentioned, use gpiochip_add_data(&gc->chip, gc);
>

ok.

>> +static struct platform_driver f81504_gpio_driver = {
>> +       .driver = {
>> +               .name   = F81504_GPIO_NAME,
>> +               .owner  = THIS_MODULE,
>
> I saw coccinelle was already complaining about this.
>
> Looking forward to v3!

I had sent V3 today to resolve the issue above.

Thanks for your advices.
-- 
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

[Peter Hung]

Hi Linus,

Linus Walleij 於 2016/2/10 下午 05:08 寫道:
> On Thu, Jan 28, 2016 at 10:20 AM, Peter Hung <hpeter@gmail.com> wrote:
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio.h>
>
> Drivers should just
> #include <linux/gpio/driver.h>

ok.

>> +static struct f81504_gpio_chip *gpio_to_f81504_chip(struct gpio_chip *chip)
>> +{
>> +       return container_of(chip, struct f81504_gpio_chip, chip);
>> +}
>
> Avoid this construction in new code.
>
> Use gpiochip_get_data(chip) everywhere that gpio_to_f81504_chip()
> is used and register the gpiochip with gpiochip_add_data()
> and the code will be simpler.
>
> See any other driver in drivers/gpio for examples, I converted them
> all.

ok. I'll re-write this section.

>> +
>> +       if (tmp & BIT(offset))
>> +               return GPIOF_DIR_OUT;
>> +
>> +       return GPIOF_DIR_IN;
>> +}
>
> Do not use GPIOF* flags in driver code, these are for the consumer
> API. Just return 0/1.

ok

>> +       status = gpiochip_add(&gc->chip);
>
> As mentioned, use gpiochip_add_data(&gc->chip, gc);
>

ok.

>> +static struct platform_driver f81504_gpio_driver = {
>> +       .driver = {
>> +               .name   = F81504_GPIO_NAME,
>> +               .owner  = THIS_MODULE,
>
> I saw coccinelle was already complaining about this.
>
> Looking forward to v3!

I had sent V3 today to resolve the issue above.

Thanks for your advices.
-- 
With Best Regards,
Peter Hung

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

[Peter Hung]

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

Suggested-by: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_f81504.c | 254 ++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig       |  11 ++
 drivers/tty/serial/8250/Makefile      |   1 +
 3 files changed, 266 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_f81504.c

diff --git a/drivers/tty/serial/8250/8250_f81504.c b/drivers/tty/serial/8250/8250_f81504.c
new file mode 100644
index 0000000..543661f
--- /dev/null
+++ 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 };
+
+/* 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 platform_device *pdev = container_of(port->dev,
+					struct platform_device, dev);
+	struct pci_dev *pci_dev = to_pci_dev(pdev->dev.parent);
+
+	pci_read_config_byte(pci_dev, F81504_UART_START_ADDR +
+			F81504_UART_OFFSET * *index + F81504_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 |= F81504_RTS_CONTROL_BY_HW;
+
+		if (rs485->flags & SER_RS485_RTS_ON_SEND) {
+			/* RTS driving high on TX */
+			setting &= ~F81504_RTS_INVERT;
+		} else {
+			/* RTS driving low on TX */
+			setting |= F81504_RTS_INVERT;
+		}
+
+		rs485->delay_rts_after_send = 0;
+		rs485->delay_rts_before_send = 0;
+	} else {
+		/* Disable RTS H/W control mode */
+		setting &= ~(F81504_RTS_CONTROL_BY_HW | F81504_RTS_INVERT);
+	}
+
+	pci_write_config_byte(pci_dev, F81504_UART_START_ADDR +
+			F81504_UART_OFFSET * *index + F81504_UART_MODE_OFFSET,
+			setting);
+
+	if (rs485 != &port->rs485)
+		port->rs485 = *rs485;
+
+	return 0;
+}
+
+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) {
+		/* 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 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;
+	size_t i;
+
+	do {
+		/* read current clock source (masked with CLOCK_RATE_MASK) */
+		pci_read_config_byte(dev, F81504_UART_START_ADDR + *offset *
+				F81504_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, F81504_UART_START_ADDR +
+					*offset * F81504_UART_OFFSET,
+					tmp & ~F81504_CLOCK_RATE_MASK);
+			break;
+		}
+
+		if (!f81504_check_baudrate(baud, &i)) {
+			/* had optimize value */
+			port->uartclk = baudrate_table[i] * 16;
+			tmp = (tmp & ~F81504_CLOCK_RATE_MASK) | clock_table[i];
+			pci_write_config_byte(dev, F81504_UART_START_ADDR +
+					*offset * F81504_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 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 */
+
+	return serial8250_register_8250_port(&port);
+}
+
+static int f81504_serial_probe(struct platform_device *pdev)
+{
+	int line;
+	size_t *index = (size_t *)dev_get_platdata(&pdev->dev);
+	struct resource *io = platform_get_resource(pdev, IORESOURCE_IO, 0);
+
+	line = f81504_register_port(pdev, io->start, *index);
+	if (line < 0)
+		return line;
+
+	/*
+	 * Re-assign line to replace old port PCIE configuration space idx,
+	 * port idx is saved in per-port private data.
+	 */
+	*index = line;
+	return 0;
+}
+
+static int f81504_serial_remove(struct platform_device *pdev)
+{
+	size_t *line = (size_t *)dev_get_platdata(&pdev->dev);
+
+	serial8250_unregister_port(*line);
+	return 0;
+}
+
+
+#ifdef CONFIG_PM_SLEEP
+static int f81504_serial_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	size_t *line = (size_t *)dev_get_platdata(&pdev->dev);
+
+	serial8250_suspend_port(*line);
+	return 0;
+}
+
+static int f81504_serial_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	size_t *line = (size_t *)dev_get_platdata(&pdev->dev);
+	struct uart_8250_port *port = serial8250_get_port(*line);
+
+	f81504_rs485_config(&port->port, NULL);
+	serial8250_resume_port(*line);
+	return 0;
+}
+#endif
+
+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,
+		.pm     = &f81504_serial_pm_ops,
+	},
+	.probe		= f81504_serial_probe,
+	.remove		= f81504_serial_remove,
+};
+
+static int __init f81504_serial_init(void)
+{
+	return platform_driver_register(&f81504_serial_driver);
+}
+subsys_initcall(f81504_serial_init);
+
+static void __exit f81504_serial_exit(void)
+{
+	platform_driver_unregister(&f81504_serial_driver);
+}
+module_exit(f81504_serial_exit);
+
+MODULE_AUTHOR("Peter Hong <Peter_Hong@fintek.com.tw>");
+MODULE_DESCRIPTION("Fintek F81504/508/512 PCIE 16550A serial port driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 6412f14..736d467 100644
--- 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
+        help
+          This builds Fintek F81504/508/512 PCIE serial ports support.
+          You may be able to enable GPIO_F81504 to get GPIOLIB support if
+          the devices supported. Please enable MFD_FINTEK_F81504_CORE &
+          this with Y if you need a early console.
+
 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 e177f86..9e8c549 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_F81504)	+= 8250_f81504.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

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

[One Thousand Gnomes]

On Thu, 28 Jan 2016 17:20:37 +0800
Peter Hung <hpeter@gmail.com> 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
> 
> Suggested-by: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>

Nice

It and the GPIO driver parts

Reviewed-by: Alan Cox <gnomes@lxorguk.ukuu.org.uk>

Alan

[PATCH] 8250: 8250_f81504: fix platform_no_drv_owner.cocci warnings

[kbuild test robot]

drivers/tty/serial/8250/8250_f81504.c:233:3-8: No need to set .owner here. The core will do it.

 Remove .owner field if calls are used which set it automatically

Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci

CC: Peter Hung <hpeter@gmail.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 8250_f81504.c |    1 -
 1 file changed, 1 deletion(-)

--- a/drivers/tty/serial/8250/8250_f81504.c
+++ b/drivers/tty/serial/8250/8250_f81504.c
@@ -230,7 +230,6 @@ static SIMPLE_DEV_PM_OPS(f81504_serial_p
 static struct platform_driver f81504_serial_driver = {
 	.driver = {
 		.name	= F81504_SERIAL_NAME,
-		.owner	= THIS_MODULE,
 		.pm     = &f81504_serial_pm_ops,
 	},
 	.probe		= f81504_serial_probe,

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

[kbuild test robot]

Hi Peter,

[auto build test WARNING on gpio/for-next]
[also build test WARNING on v4.5-rc1 next-20160128]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Hung/Transform-Fintek-PCIE-driver-from-8250-to-MFD/20160128-172929
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/tty/serial/8250/8250_f81504.c:233:3-8: No need to set .owner here. The core will do it.

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH V2 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

[Peter Hung]

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

Paul recommed us do less code deletion to avoid confusing problem when
bisect.
https://lkml.org/lkml/2016/1/18/646

But this patch is sent after with following patch.
8250: 8250_f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO UART support
mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

We must remove F81504/508/512 support in 8250_pci.c and migrate to
f81504-core/8250_f81504 to enable MFD support.

Suggested-by: Paul Gortmaker <paul.gortmaker@windriver.com>
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

Re: [PATCH V2 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

[Andy Shevchenko]

On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
> Remove Fintek F81504/508/512 PCIE-to-UART device driver from
> 8250_pci.c
> 
> Paul recommed us do less code deletion to avoid confusing problem
> when
> bisect.
> https://lkml.org/lkml/2016/1/18/646
> 
> 


> -	/* 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
> },

Shouldn't you blacklist them in 8250_pci?

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

Re: [PATCH V2 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/28 下午 08:04 寫道:
> On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
>> -	/* 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
>> },
>
> Shouldn't you blacklist them in 8250_pci?
>

You are referring to add blacklist instead of remove F81504/508/512
code? or add blacklist and remove code?

-- 
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/28 下午 08:04 寫道:
> On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
>> -	/* 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
>> },
>
> Shouldn't you blacklist them in 8250_pci?
>

You are referring to add blacklist instead of remove F81504/508/512
code? or add blacklist and remove code?

-- 
With Best Regards,
Peter Hung

Re: [PATCH V2 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

[Andy Shevchenko]

On Fri, 2016-01-29 at 16:20 +0800, Peter Hung wrote:
> Hi Andy,
> 
> Andy Shevchenko 於 2016/1/28 下午 08:04 寫道:
> > On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
> > > -	/* 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
> > > },
> > 
> > Shouldn't you blacklist them in 8250_pci?
> > 
> 
> You are referring to add blacklist instead of remove F81504/508/512
> code?

No.

>  or add blacklist and remove code?

This one.

Check what lspci tells you about your device. I'm pretty sure that it
has Serial Class, which would trigger enumeration in 8250_pci.c if it
comes first.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

[Andy Shevchenko]

On Fri, 2016-01-29 at 16:20 +0800, Peter Hung wrote:
> Hi Andy,
> 
> Andy Shevchenko 於 2016/1/28 下午 08:04 寫道:
> > On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
> > > -	/* 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
> > > },
> > 
> > Shouldn't you blacklist them in 8250_pci?
> > 
> 
> You are referring to add blacklist instead of remove F81504/508/512
> code?

No.

>  or add blacklist and remove code?

This one.

Check what lspci tells you about your device. I'm pretty sure that it
has Serial Class, which would trigger enumeration in 8250_pci.c if it
comes first.

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

Re: [PATCH V2 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/29 下午 08:40 寫道:
> On Fri, 2016-01-29 at 16:20 +0800, Peter Hung wrote:
>> Hi Andy,
>>
>> Andy Shevchenko 於 2016/1/28 下午 08:04 寫道:
>>> On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
>>>> -	/* 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
>>>> },
>>>
>>> Shouldn't you blacklist them in 8250_pci?
>>>
>>
>> You are referring to add blacklist instead of remove F81504/508/512
>> code?
>
> No.
>
>>   or add blacklist and remove code?
>
> This one.

ok

> Check what lspci tells you about your device. I'm pretty sure that it
> has Serial Class, which would trigger enumeration in 8250_pci.c if it
> comes first.
>

I had add log with 8250_pci.c. It really trigger once by 8250_pci.c,
but will failed with serial_pci_guess_board(). So It can be handled by
f81504-core.c. I should add pid/vid to blacklist and comments it'll
be handled by f81504-core.c

Thanks
-- 
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 4/4] serial: 8250_pci: Remove Fintek F81504/508/512 UART driver

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/29 下午 08:40 寫道:
> On Fri, 2016-01-29 at 16:20 +0800, Peter Hung wrote:
>> Hi Andy,
>>
>> Andy Shevchenko 於 2016/1/28 下午 08:04 寫道:
>>> On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
>>>> -	/* 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
>>>> },
>>>
>>> Shouldn't you blacklist them in 8250_pci?
>>>
>>
>> You are referring to add blacklist instead of remove F81504/508/512
>> code?
>
> No.
>
>>   or add blacklist and remove code?
>
> This one.

ok

> Check what lspci tells you about your device. I'm pretty sure that it
> has Serial Class, which would trigger enumeration in 8250_pci.c if it
> comes first.
>

I had add log with 8250_pci.c. It really trigger once by 8250_pci.c,
but will failed with serial_pci_guess_board(). So It can be handled by
f81504-core.c. I should add pid/vid to blacklist and comments it'll
be handled by f81504-core.c

Thanks
-- 
With Best Regards,
Peter Hung