[PATCH v2 0/6] serial/gpio: exar: Fixes and support for IOT2000

[PATCH v2 0/6] serial/gpio: exar: Fixes and support for IOT2000

[Jan Kiszka]

This makes the gpio-exar driver usable, which was prevented by a number
of fatal bugs, and adds support for the SIMATIC IOT2040 to the 8250-exar
driver and, indirectly, to gpio-exar as well. It's a cross-subsystem
series, so I'm also cross-posting to the serial and gpio lists.

Changes in v2:
 - dropped already merged patches
 - switched to platform_data structure for exar <-> gpio driver
   communication
 - use dmi_check_system() to express platform match
 - adjusted bit logic style in exar_get_direction/value

Jan

Jan Kiszka (6):
  gpio: exar: Fix passing in of parent PCI device
  gpio: exar: Allocate resources on behalf of the platform device
  gpio: exar: Fix iomap request
  gpio: exar: Fix reading of directions and values
  gpio-exar/8250-exar: Make set of exported GPIOs configurable
  serial: exar: Add support for IOT2040 device

 drivers/gpio/gpio-exar.c                |  54 +++++++-----
 drivers/tty/serial/8250/8250_exar.c     | 151 ++++++++++++++++++++++++++++++--
 include/linux/platform_data/gpio-exar.h |  25 ++++++
 3 files changed, 198 insertions(+), 32 deletions(-)
 create mode 100644 include/linux/platform_data/gpio-exar.h

-- 
2.12.0

[PATCH v2 1/6] gpio: exar: Fix passing in of parent PCI device

[Jan Kiszka]

This fixes reloading of the GPIO driver for the same platform device
instance as created by the exar UART driver: First of all, the driver
sets drvdata to its own value during probing and does not restore the
original value on exit. But this won't help anyway as the core clears
drvdata after the driver left.

Use stable platform_data instead.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/gpio/gpio-exar.c                |  4 +++-
 drivers/tty/serial/8250/8250_exar.c     |  8 ++++++--
 include/linux/platform_data/gpio-exar.h | 22 ++++++++++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/platform_data/gpio-exar.h

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 081076771217..d62e57513144 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/platform_data/gpio-exar.h>
 #include <linux/platform_device.h>
 
 #define EXAR_OFFSET_MPIOLVL_LO 0x90
@@ -119,7 +120,8 @@ static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
 
 static int gpio_exar_probe(struct platform_device *pdev)
 {
-	struct pci_dev *pcidev = platform_get_drvdata(pdev);
+	struct gpio_exar_pdata *pdata = pdev->dev.platform_data;
+	struct pci_dev *pcidev = pdata->parent;
 	struct exar_gpio_chip *exar_gpio;
 	void __iomem *p;
 	int index, ret;
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index b4fa585156c7..0a806daaff8b 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/platform_data/gpio-exar.h>
 #include <linux/serial_core.h>
 #include <linux/serial_reg.h>
 #include <linux/slab.h>
@@ -191,13 +192,16 @@ static void *
 xr17v35x_register_gpio(struct pci_dev *pcidev)
 {
 	struct platform_device *pdev;
+	struct gpio_exar_pdata pdata;
 
 	pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
 	if (!pdev)
 		return NULL;
 
-	platform_set_drvdata(pdev, pcidev);
-	if (platform_device_add(pdev) < 0) {
+	pdata.parent = pcidev;
+
+	if (platform_device_add_data(pdev, &pdata, sizeof(pdata)) < 0 ||
+	    platform_device_add(pdev) < 0) {
 		platform_device_put(pdev);
 		return NULL;
 	}
diff --git a/include/linux/platform_data/gpio-exar.h b/include/linux/platform_data/gpio-exar.h
new file mode 100644
index 000000000000..1a13e9ddaf7d
--- /dev/null
+++ b/include/linux/platform_data/gpio-exar.h
@@ -0,0 +1,22 @@
+/*
+ * GPIO handling for Exar XR17V35X chip
+ *
+ * Copyright (c) 2017 Siemens AG
+ *
+ * Written by Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __GPIO_EXAR_PDATA_H
+#define __GPIO_EXAR_PDATA_H
+
+struct pci_dev;
+
+struct gpio_exar_pdata {
+	struct pci_dev *parent;
+};
+
+#endif /* __GPIO_EXAR_PDATA_H */
-- 
2.12.0

[PATCH v2 2/6] gpio: exar: Allocate resources on behalf of the platform device

[Jan Kiszka]

Do not allocate resources on behalf of the parent device but on our own.
Otherwise, cleanup does not properly work if gpio-exar is removed but
not the parent device.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/gpio-exar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index d62e57513144..b1dc5fff21ad 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -141,7 +141,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	if (!p)
 		return -ENOMEM;
 
-	exar_gpio = devm_kzalloc(&pcidev->dev, sizeof(*exar_gpio), GFP_KERNEL);
+	exar_gpio = devm_kzalloc(&pdev->dev, sizeof(*exar_gpio), GFP_KERNEL);
 	if (!exar_gpio)
 		return -ENOMEM;
 
@@ -162,7 +162,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	exar_gpio->regs = p;
 	exar_gpio->index = index;
 
-	ret = devm_gpiochip_add_data(&pcidev->dev,
+	ret = devm_gpiochip_add_data(&pdev->dev,
 				     &exar_gpio->gpio_chip, exar_gpio);
 	if (ret)
 		goto err_destroy;
-- 
2.12.0

[PATCH v2 3/6] gpio: exar: Fix iomap request

[Jan Kiszka]

The UART driver already maps the resource for us. Trying to do this here
only fails and leaves us with a non-working device.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/gpio/gpio-exar.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index b1dc5fff21ad..4ed1f8bdeda7 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -130,14 +130,10 @@ static int gpio_exar_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	/*
-	 * Map the pci device to get the register addresses.
-	 * We will need to read and write those registers to control
-	 * the GPIO pins.
-	 * Using managed functions will save us from unmaping on exit.
-	 * As the device is enabled using managed functions by the
-	 * UART driver we can also use managed functions here.
+	 * The UART driver must have mapped region 0 prior to registering this
+	 * device - use it.
 	 */
-	p = pcim_iomap(pcidev, 0, 0);
+	p = pcim_iomap_table(pcidev)[0];
 	if (!p)
 		return -ENOMEM;
 
-- 
2.12.0

[PATCH v2 4/6] gpio: exar: Fix reading of directions and values

[Jan Kiszka]

First, the logic for translating a register bit to the return code of
exar_get_direction and exar_get_value were wrong. And second, there was
a flip regarding the register bank in exar_get_direction.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/gpio/gpio-exar.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 4ed1f8bdeda7..d8b6296c11de 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -69,7 +69,7 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
 	value = readb(exar_gpio->regs + reg);
 	mutex_unlock(&exar_gpio->lock);
 
-	return !!value;
+	return value;
 }
 
 static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
@@ -79,7 +79,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 	int val;
 
 	addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
-	val = exar_get(chip, addr) >> (offset % 8);
+	val = exar_get(chip, addr) & BIT(offset % 8);
 
 	return !!val;
 }
@@ -90,8 +90,8 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
 	unsigned int addr;
 	int val;
 
-	addr = bank ? EXAR_OFFSET_MPIOLVL_LO : EXAR_OFFSET_MPIOLVL_HI;
-	val = exar_get(chip, addr) >> (offset % 8);
+	addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
+	val = exar_get(chip, addr) & BIT(offset % 8);
 
 	return !!val;
 }
-- 
2.12.0

[PATCH v2 5/6] gpio-exar/8250-exar: Make set of exported GPIOs configurable

[Jan Kiszka]

On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the
rest is required to operate the UART. To allow modeling this case,
expand the platform device data structure to specify a (consecutive) pin
subset for exporting by the gpio-exar driver.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/gpio/gpio-exar.c                | 28 +++++++++++++++++++---------
 drivers/tty/serial/8250/8250_exar.c     |  7 +++++--
 include/linux/platform_data/gpio-exar.h |  3 +++
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index d8b6296c11de..e0698a1ae012 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -32,6 +32,7 @@ struct exar_gpio_chip {
 	int index;
 	void __iomem *regs;
 	char name[20];
+	unsigned int first_gpio;
 };
 
 static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
@@ -52,9 +53,11 @@ static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
 static int exar_set_direction(struct gpio_chip *chip, int direction,
 			      unsigned int offset)
 {
-	unsigned int bank = offset / 8;
-	unsigned int addr;
+	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+	unsigned int bank, addr;
 
+	offset += exar_gpio->first_gpio;
+	bank = offset / 8;
 	addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
 	exar_update(chip, addr, direction, offset % 8);
 	return 0;
@@ -74,10 +77,12 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
 
 static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
-	unsigned int bank = offset / 8;
-	unsigned int addr;
+	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+	unsigned int bank, addr;
 	int val;
 
+	offset += exar_gpio->first_gpio;
+	bank = offset / 8;
 	addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
 	val = exar_get(chip, addr) & BIT(offset % 8);
 
@@ -86,10 +91,12 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 
 static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
 {
-	unsigned int bank = offset / 8;
-	unsigned int addr;
+	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+	unsigned int bank, addr;
 	int val;
 
+	offset += exar_gpio->first_gpio;
+	bank = offset / 8;
 	addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
 	val = exar_get(chip, addr) & BIT(offset % 8);
 
@@ -99,9 +106,11 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
 static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 			   int value)
 {
-	unsigned int bank = offset / 8;
-	unsigned int addr;
+	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+	unsigned int bank, addr;
 
+	offset += exar_gpio->first_gpio;
+	bank = offset / 8;
 	addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
 	exar_update(chip, addr, value, offset % 8);
 }
@@ -154,9 +163,10 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	exar_gpio->gpio_chip.get = exar_get_value;
 	exar_gpio->gpio_chip.set = exar_set_value;
 	exar_gpio->gpio_chip.base = -1;
-	exar_gpio->gpio_chip.ngpio = 16;
+	exar_gpio->gpio_chip.ngpio = pdata->ngpio;
 	exar_gpio->regs = p;
 	exar_gpio->index = index;
+	exar_gpio->first_gpio = pdata->first_gpio;
 
 	ret = devm_gpiochip_add_data(&pdev->dev,
 				     &exar_gpio->gpio_chip, exar_gpio);
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 0a806daaff8b..d9c52288d6c9 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -189,7 +189,8 @@ static void setup_gpio(u8 __iomem *p)
 }
 
 static void *
-xr17v35x_register_gpio(struct pci_dev *pcidev)
+xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio,
+		       unsigned int ngpio)
 {
 	struct platform_device *pdev;
 	struct gpio_exar_pdata pdata;
@@ -199,6 +200,8 @@ xr17v35x_register_gpio(struct pci_dev *pcidev)
 		return NULL;
 
 	pdata.parent = pcidev;
+	pdata.first_gpio = first_gpio;
+	pdata.ngpio = ngpio;
 
 	if (platform_device_add_data(pdev, &pdata, sizeof(pdata)) < 0 ||
 	    platform_device_add(pdev) < 0) {
@@ -242,7 +245,7 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		/* Setup Multipurpose Input/Output pins. */
 		setup_gpio(p);
 
-		port->port.private_data = xr17v35x_register_gpio(pcidev);
+		port->port.private_data = xr17v35x_register_gpio(pcidev, 0, 16);
 	}
 
 	return 0;
diff --git a/include/linux/platform_data/gpio-exar.h b/include/linux/platform_data/gpio-exar.h
index 1a13e9ddaf7d..1754f5a2842d 100644
--- a/include/linux/platform_data/gpio-exar.h
+++ b/include/linux/platform_data/gpio-exar.h
@@ -17,6 +17,9 @@ struct pci_dev;
 
 struct gpio_exar_pdata {
 	struct pci_dev *parent;
+
+	unsigned int first_gpio;
+	unsigned int ngpio;
 };
 
 #endif /* __GPIO_EXAR_PDATA_H */
-- 
2.12.0

[PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Jan Kiszka]

This implements the setup of RS232 and the switch-over to RS485 or RS422
for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
to switch between the different modes. The external logic is controlled
via MPIO pins of the EXAR controller.

Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
an LED.

As the XR17V352 used on the IOT2040 is not equipped with an external
EEPROM, it cannot present itself as IOT2040-variant via subvendor/
subdevice IDs. Thus, we have to check via DMI for the target platform.

Co-developed with Sascha Weisenberger.

Signed-off-by: Sascha Weisenberger <sascha.weisenberger@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/tty/serial/8250/8250_exar.c | 136 ++++++++++++++++++++++++++++++++++--
 1 file changed, 131 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index d9c52288d6c9..5da6f1c27b50 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -9,6 +9,7 @@
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License.
  */
+#include <linux/dmi.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -61,6 +62,45 @@
 #define UART_EXAR_MPIOSEL_15_8	0x99	/* MPIOSEL[15:8] */
 #define UART_EXAR_MPIOOD_15_8	0x9a	/* MPIOOD[15:8] */
 
+#define UART_EXAR_RS485_DLY(x)	(x << 4)
+
+/*
+ * IOT2040 MPIO wiring semantics:
+ *
+ * MPIO		Port	Function
+ * ----		----	--------
+ * 0		2 	Mode bit 0
+ * 1		2	Mode bit 1
+ * 2		2	Terminate bus
+ * 3		-	<reserved>
+ * 4		3	Mode bit 0
+ * 5		3	Mode bit 1
+ * 6		3	Terminate bus
+ * 7		-	<reserved>
+ * 8		2	Enable
+ * 9		3	Enable
+ * 10		-	Red LED
+ * 11..15	-	<unused>
+ */
+
+/* IOT2040 MPIOs 0..7 */
+#define IOT2040_UART_MODE_RS232		0x01
+#define IOT2040_UART_MODE_RS485		0x02
+#define IOT2040_UART_MODE_RS422		0x03
+#define IOT2040_UART_TERMINATE_BUS	0x04
+
+#define IOT2040_UART1_MASK		0x0f
+#define IOT2040_UART2_SHIFT		4
+
+#define IOT2040_UARTS_DEFAULT_MODE	0x11	/* both RS232 */
+#define IOT2040_UARTS_GPIO_LO_MODE	0x88	/* reserved pins as input */
+
+/* IOT2040 MPIOs 8..15 */
+#define IOT2040_UARTS_ENABLE		0x03
+#define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */
+
+static bool is_iot2040;
+
 struct exar8250;
 
 /**
@@ -212,6 +252,82 @@ xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio,
 	return pdev;
 }
 
+static int iot2040_rs485_config(struct uart_port *port,
+				struct serial_rs485 *rs485)
+{
+	u8 __iomem *p = port->membase;
+	u8 mask = IOT2040_UART1_MASK;
+	u8 mode, value;
+	bool is_rs485 = false;
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		is_rs485 = true;
+		if (rs485->flags & SER_RS485_RX_DURING_TX)
+			mode = IOT2040_UART_MODE_RS422;
+		else
+			mode = IOT2040_UART_MODE_RS485;
+
+		if (rs485->flags & SER_RS485_TERMINATE_BUS)
+			mode |= IOT2040_UART_TERMINATE_BUS;
+	} else {
+		mode = IOT2040_UART_MODE_RS232;
+	}
+
+	if (port->line == 3) {
+		mask <<= IOT2040_UART2_SHIFT;
+		mode <<= IOT2040_UART2_SHIFT;
+	}
+
+	value = readb(p + UART_EXAR_MPIOLVL_7_0);
+	value &= ~mask;
+	value |= mode;
+	writeb(value, p + UART_EXAR_MPIOLVL_7_0);
+
+	value = readb(p + UART_EXAR_FCTR);
+	if (is_rs485)
+		value |= UART_FCTR_EXAR_485;
+	else
+		value &= ~UART_FCTR_EXAR_485;
+	writeb(value, p + UART_EXAR_FCTR);
+
+	if (is_rs485)
+		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
+
+	return 0;
+}
+
+static int iot2040_setup_gpio(struct pci_dev *pcidev,
+			      struct uart_8250_port *port)
+{
+	u8 __iomem *p = port->port.membase;
+
+	writeb(IOT2040_UARTS_DEFAULT_MODE, p + UART_EXAR_MPIOLVL_7_0);
+	writeb(IOT2040_UARTS_GPIO_LO_MODE, p + UART_EXAR_MPIOSEL_7_0);
+	writeb(IOT2040_UARTS_ENABLE, p + UART_EXAR_MPIOLVL_15_8);
+	writeb(IOT2040_UARTS_GPIO_HI_MODE, p + UART_EXAR_MPIOSEL_15_8);
+
+	port->port.private_data = xr17v35x_register_gpio(pcidev, 10, 1);
+
+	return 0;
+}
+
+static int iot2040_match(const struct dmi_system_id *dmi)
+{
+	is_iot2040 = true;
+	return 1;
+}
+
+static const struct dmi_system_id exar_platforms[] = {
+	{
+		.callback = iot2040_match,
+		.ident = "IOT2040",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "SIMATIC IOT2000"),
+			DMI_MATCH(DMI_BOARD_ASSET_TAG, "6ES7647-0AA00-1YA2"),
+		},
+	}
+};
+
 static int
 pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		   struct uart_8250_port *port, int idx)
@@ -222,7 +338,13 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	u8 __iomem *p;
 	int ret;
 
+	dmi_check_system(exar_platforms);
+
 	port->port.uartclk = baud * 16;
+
+	if (is_iot2040)
+		port->port.rs485_config = iot2040_rs485_config;
+
 	/*
 	 * Setup the uart clock for the devices on expansion slot to
 	 * half the clock speed of the main chip (which is 125MHz)
@@ -241,14 +363,18 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	writeb(128, p + UART_EXAR_TXTRG);
 	writeb(128, p + UART_EXAR_RXTRG);
 
-	if (idx == 0) {
-		/* Setup Multipurpose Input/Output pins. */
-		setup_gpio(p);
+	if (idx != 0)
+		return 0;
+
+	/* Setup Multipurpose Input/Output pins. */
+	setup_gpio(p);
 
+	if (is_iot2040)
+		ret = iot2040_setup_gpio(pcidev, port);
+	else
 		port->port.private_data = xr17v35x_register_gpio(pcidev, 0, 16);
-	}
 
-	return 0;
+	return ret;
 }
 
 static void pci_xr17v35x_exit(struct pci_dev *pcidev)
-- 
2.12.0

Re: [PATCH v2 4/6] gpio: exar: Fix reading of directions and values

[Andy Shevchenko]

On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> First, the logic for translating a register bit to the return code of
> exar_get_direction and exar_get_value were wrong. And second, there was
> a flip regarding the register bank in exar_get_direction.
>

FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/gpio/gpio-exar.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
> index 4ed1f8bdeda7..d8b6296c11de 100644
> --- a/drivers/gpio/gpio-exar.c
> +++ b/drivers/gpio/gpio-exar.c
> @@ -69,7 +69,7 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
>         value = readb(exar_gpio->regs + reg);
>         mutex_unlock(&exar_gpio->lock);
>
> -       return !!value;
> +       return value;
>  }
>
>  static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
> @@ -79,7 +79,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>         int val;
>
>         addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
> -       val = exar_get(chip, addr) >> (offset % 8);
> +       val = exar_get(chip, addr) & BIT(offset % 8);
>
>         return !!val;
>  }
> @@ -90,8 +90,8 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
>         unsigned int addr;
>         int val;
>
> -       addr = bank ? EXAR_OFFSET_MPIOLVL_LO : EXAR_OFFSET_MPIOLVL_HI;
> -       val = exar_get(chip, addr) >> (offset % 8);
> +       addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
> +       val = exar_get(chip, addr) & BIT(offset % 8);
>
>         return !!val;
>  }
> --
> 2.12.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Andy Shevchenko]

On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> This implements the setup of RS232 and the switch-over to RS485 or RS422
> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
> to switch between the different modes. The external logic is controlled
> via MPIO pins of the EXAR controller.
>
> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
> an LED.
>
> As the XR17V352 used on the IOT2040 is not equipped with an external
> EEPROM, it cannot present itself as IOT2040-variant via subvendor/
> subdevice IDs. Thus, we have to check via DMI for the target platform.
>
> Co-developed with Sascha Weisenberger.

Few nits below and one comment that should be addressed.

> +#define UART_EXAR_RS485_DLY(x) (x << 4)

((x) << 4)

> +static bool is_iot2040;

No, please, use driver data of DMI and hide this in corresponding structure.
Or even assign  port->port.rs485_config in the callback function.

Moreover, can't you use port->port.rs485_config != NULL instead?

> +
>  struct exar8250;
>
>  /**
> @@ -212,6 +252,82 @@ xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio,
>         return pdev;
>  }
>
> +static int iot2040_rs485_config(struct uart_port *port,
> +                               struct serial_rs485 *rs485)
> +{
> +       u8 __iomem *p = port->membase;
> +       u8 mask = IOT2040_UART1_MASK;
> +       u8 mode, value;

> +       bool is_rs485 = false;
> +
> +       if (rs485->flags & SER_RS485_ENABLED) {
> +               is_rs485 = true;

bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);

if (is_rs485) {
...
} else {
...
}

> +       return 0;
> +}

> +static int iot2040_match(const struct dmi_system_id *dmi)
> +{
> +       is_iot2040 = true;
> +       return 1;
> +}

See above.

> +
> +static const struct dmi_system_id exar_platforms[] = {
> +       {
> +               .callback = iot2040_match,
> +               .ident = "IOT2040",
> +               .matches = {
> +                       DMI_MATCH(DMI_BOARD_NAME, "SIMATIC IOT2000"),
> +                       DMI_MATCH(DMI_BOARD_ASSET_TAG, "6ES7647-0AA00-1YA2"),
> +               },
> +       }
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Jan Kiszka]

On 2017-05-18 18:33, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> This implements the setup of RS232 and the switch-over to RS485 or RS422
>> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
>> to switch between the different modes. The external logic is controlled
>> via MPIO pins of the EXAR controller.
>>
>> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
>> an LED.
>>
>> As the XR17V352 used on the IOT2040 is not equipped with an external
>> EEPROM, it cannot present itself as IOT2040-variant via subvendor/
>> subdevice IDs. Thus, we have to check via DMI for the target platform.
>>
>> Co-developed with Sascha Weisenberger.
> 
> Few nits below and one comment that should be addressed.
> 
>> +#define UART_EXAR_RS485_DLY(x) (x << 4)
> 
> ((x) << 4)

Yep.

> 
>> +static bool is_iot2040;
> 
> No, please, use driver data of DMI and hide this in corresponding structure.
> Or even assign  port->port.rs485_config in the callback function.
> 
> Moreover, can't you use port->port.rs485_config != NULL instead?

There are two cases to be handled on IOT2040: the setting of the
rs485_config and the different setup of the GPIOs, but the latter at a
specific point in the initialization only. So I don't see yet how
driver_data could come into play and help.

> 
>> +
>>  struct exar8250;
>>
>>  /**
>> @@ -212,6 +252,82 @@ xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio,
>>         return pdev;
>>  }
>>
>> +static int iot2040_rs485_config(struct uart_port *port,
>> +                               struct serial_rs485 *rs485)
>> +{
>> +       u8 __iomem *p = port->membase;
>> +       u8 mask = IOT2040_UART1_MASK;
>> +       u8 mode, value;
> 
>> +       bool is_rs485 = false;
>> +
>> +       if (rs485->flags & SER_RS485_ENABLED) {
>> +               is_rs485 = true;
> 
> bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
> 
> if (is_rs485) {
> ...
> } else {
> ...
> }
> 

OK.

>> +       return 0;
>> +}
> 
>> +static int iot2040_match(const struct dmi_system_id *dmi)
>> +{
>> +       is_iot2040 = true;
>> +       return 1;
>> +}
> 
> See above.

See above.

Jan

> 
>> +
>> +static const struct dmi_system_id exar_platforms[] = {
>> +       {
>> +               .callback = iot2040_match,
>> +               .ident = "IOT2040",
>> +               .matches = {
>> +                       DMI_MATCH(DMI_BOARD_NAME, "SIMATIC IOT2000"),
>> +                       DMI_MATCH(DMI_BOARD_ASSET_TAG, "6ES7647-0AA00-1YA2"),
>> +               },
>> +       }
>> +};
> 

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Jan Kiszka]

On 2017-05-18 18:39, Jan Kiszka wrote:
> On 2017-05-18 18:33, Andy Shevchenko wrote:
>> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> This implements the setup of RS232 and the switch-over to RS485 or RS422
>>> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
>>> to switch between the different modes. The external logic is controlled
>>> via MPIO pins of the EXAR controller.
>>>
>>> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
>>> an LED.
>>>
>>> As the XR17V352 used on the IOT2040 is not equipped with an external
>>> EEPROM, it cannot present itself as IOT2040-variant via subvendor/
>>> subdevice IDs. Thus, we have to check via DMI for the target platform.
>>>
>>> Co-developed with Sascha Weisenberger.
>>
>> Few nits below and one comment that should be addressed.
>>
>>> +#define UART_EXAR_RS485_DLY(x) (x << 4)
>>
>> ((x) << 4)
> 
> Yep.
> 
>>
>>> +static bool is_iot2040;
>>
>> No, please, use driver data of DMI and hide this in corresponding structure.
>> Or even assign  port->port.rs485_config in the callback function.
>>
>> Moreover, can't you use port->port.rs485_config != NULL instead?
> 
> There are two cases to be handled on IOT2040: the setting of the
> rs485_config and the different setup of the GPIOs, but the latter at a
> specific point in the initialization only. So I don't see yet how
> driver_data could come into play and help.
> 

OK, got - hacking...

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 1/6] gpio: exar: Fix passing in of parent PCI device

[Andy Shevchenko]

On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> This fixes reloading of the GPIO driver for the same platform device
> instance as created by the exar UART driver: First of all, the driver
> sets drvdata to its own value during probing and does not restore the
> original value on exit. But this won't help anyway as the core clears
> drvdata after the driver left.
>
> Use stable platform_data instead.

Okay, basically what we are trying to do here is to reinvent part of
MFD framework.

I'd like to hear Linus' and others opinions if it worth to use it instead.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Jan Kiszka]

On 2017-05-18 18:58, Jan Kiszka wrote:
> On 2017-05-18 18:39, Jan Kiszka wrote:
>> On 2017-05-18 18:33, Andy Shevchenko wrote:
>>> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> This implements the setup of RS232 and the switch-over to RS485 or RS422
>>>> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
>>>> to switch between the different modes. The external logic is controlled
>>>> via MPIO pins of the EXAR controller.
>>>>
>>>> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
>>>> an LED.
>>>>
>>>> As the XR17V352 used on the IOT2040 is not equipped with an external
>>>> EEPROM, it cannot present itself as IOT2040-variant via subvendor/
>>>> subdevice IDs. Thus, we have to check via DMI for the target platform.
>>>>
>>>> Co-developed with Sascha Weisenberger.
>>>
>>> Few nits below and one comment that should be addressed.
>>>
>>>> +#define UART_EXAR_RS485_DLY(x) (x << 4)
>>>
>>> ((x) << 4)
>>
>> Yep.
>>
>>>
>>>> +static bool is_iot2040;
>>>
>>> No, please, use driver data of DMI and hide this in corresponding structure.
>>> Or even assign  port->port.rs485_config in the callback function.
>>>
>>> Moreover, can't you use port->port.rs485_config != NULL instead?
>>
>> There are two cases to be handled on IOT2040: the setting of the
>> rs485_config and the different setup of the GPIOs, but the latter at a
>> specific point in the initialization only. So I don't see yet how
>> driver_data could come into play and help.
>>
> 
> OK, got - hacking...

It looks like this now (will resent as part of v3, just waiting for
further comments):

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index d9c52288d6c9..01826c094515 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -9,6 +9,7 @@
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License.
  */
+#include <linux/dmi.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -61,8 +62,50 @@
 #define UART_EXAR_MPIOSEL_15_8	0x99	/* MPIOSEL[15:8] */
 #define UART_EXAR_MPIOOD_15_8	0x9a	/* MPIOOD[15:8] */
 
+#define UART_EXAR_RS485_DLY(x)	((x) << 4)
+
+/*
+ * IOT2040 MPIO wiring semantics:
+ *
+ * MPIO		Port	Function
+ * ----		----	--------
+ * 0		2 	Mode bit 0
+ * 1		2	Mode bit 1
+ * 2		2	Terminate bus
+ * 3		-	<reserved>
+ * 4		3	Mode bit 0
+ * 5		3	Mode bit 1
+ * 6		3	Terminate bus
+ * 7		-	<reserved>
+ * 8		2	Enable
+ * 9		3	Enable
+ * 10		-	Red LED
+ * 11..15	-	<unused>
+ */
+
+/* IOT2040 MPIOs 0..7 */
+#define IOT2040_UART_MODE_RS232		0x01
+#define IOT2040_UART_MODE_RS485		0x02
+#define IOT2040_UART_MODE_RS422		0x03
+#define IOT2040_UART_TERMINATE_BUS	0x04
+
+#define IOT2040_UART1_MASK		0x0f
+#define IOT2040_UART2_SHIFT		4
+
+#define IOT2040_UARTS_DEFAULT_MODE	0x11	/* both RS232 */
+#define IOT2040_UARTS_GPIO_LO_MODE	0x88	/* reserved pins as input */
+
+/* IOT2040 MPIOs 8..15 */
+#define IOT2040_UARTS_ENABLE		0x03
+#define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */
+
 struct exar8250;
 
+struct exar8250_platform {
+	int (*rs485_config)(struct uart_port *, struct serial_rs485 *);
+	int (*register_gpio)(struct pci_dev *, struct uart_8250_port *);
+};
+
 /**
  * struct exar8250_board - board information
  * @num_ports: number of serial ports
@@ -189,7 +232,7 @@ static void setup_gpio(u8 __iomem *p)
 }
 
 static void *
-xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio,
+__xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio,
 		       unsigned int ngpio)
 {
 	struct platform_device *pdev;
@@ -212,17 +255,113 @@ xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio,
 	return pdev;
 }
 
+static int xr17v35x_register_gpio(struct pci_dev *pcidev,
+				  struct uart_8250_port *port)
+{
+	port->port.private_data = __xr17v35x_register_gpio(pcidev, 0, 16);
+
+	return 0;
+}
+
+static int iot2040_rs485_config(struct uart_port *port,
+				struct serial_rs485 *rs485)
+{
+	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
+	u8 __iomem *p = port->membase;
+	u8 mask = IOT2040_UART1_MASK;
+	u8 mode, value;
+
+	if (is_rs485) {
+		if (rs485->flags & SER_RS485_RX_DURING_TX)
+			mode = IOT2040_UART_MODE_RS422;
+		else
+			mode = IOT2040_UART_MODE_RS485;
+
+		if (rs485->flags & SER_RS485_TERMINATE_BUS)
+			mode |= IOT2040_UART_TERMINATE_BUS;
+	} else {
+		mode = IOT2040_UART_MODE_RS232;
+	}
+
+	if (port->line == 3) {
+		mask <<= IOT2040_UART2_SHIFT;
+		mode <<= IOT2040_UART2_SHIFT;
+	}
+
+	value = readb(p + UART_EXAR_MPIOLVL_7_0);
+	value &= ~mask;
+	value |= mode;
+	writeb(value, p + UART_EXAR_MPIOLVL_7_0);
+
+	value = readb(p + UART_EXAR_FCTR);
+	if (is_rs485)
+		value |= UART_FCTR_EXAR_485;
+	else
+		value &= ~UART_FCTR_EXAR_485;
+	writeb(value, p + UART_EXAR_FCTR);
+
+	if (is_rs485)
+		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
+
+	return 0;
+}
+
+static int iot2040_register_gpio(struct pci_dev *pcidev,
+			      struct uart_8250_port *port)
+{
+	u8 __iomem *p = port->port.membase;
+
+	writeb(IOT2040_UARTS_DEFAULT_MODE, p + UART_EXAR_MPIOLVL_7_0);
+	writeb(IOT2040_UARTS_GPIO_LO_MODE, p + UART_EXAR_MPIOSEL_7_0);
+	writeb(IOT2040_UARTS_ENABLE, p + UART_EXAR_MPIOLVL_15_8);
+	writeb(IOT2040_UARTS_GPIO_HI_MODE, p + UART_EXAR_MPIOSEL_15_8);
+
+	port->port.private_data = __xr17v35x_register_gpio(pcidev, 10, 1);
+
+	return 0;
+}
+
+static const struct exar8250_platform exar8250_default_platform = {
+	.register_gpio = xr17v35x_register_gpio,
+};
+
+static const struct exar8250_platform iot2040_platform = {
+	.rs485_config = iot2040_rs485_config,
+	.register_gpio = iot2040_register_gpio,
+};
+
+static const struct dmi_system_id exar_platforms[] = {
+	{
+		.ident = "IOT2040",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "SIMATIC IOT2000"),
+			DMI_MATCH(DMI_BOARD_ASSET_TAG, "6ES7647-0AA00-1YA2"),
+		},
+		.driver_data = (void *)&iot2040_platform,
+	}
+};
+
 static int
 pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		   struct uart_8250_port *port, int idx)
 {
 	const struct exar8250_board *board = priv->board;
+	const struct exar8250_platform *platform;
+	const struct dmi_system_id *dmi_match;
 	unsigned int offset = idx * 0x400;
 	unsigned int baud = 7812500;
 	u8 __iomem *p;
 	int ret;
 
+	dmi_match = dmi_first_match(exar_platforms);
+	if (dmi_match)
+		platform = dmi_match->driver_data;
+	else
+		platform = &exar8250_default_platform;
+
 	port->port.uartclk = baud * 16;
+	port->port.rs485_config = platform->rs485_config;
+
 	/*
 	 * Setup the uart clock for the devices on expansion slot to
 	 * half the clock speed of the main chip (which is 125MHz)
@@ -245,10 +384,10 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		/* Setup Multipurpose Input/Output pins. */
 		setup_gpio(p);
 
-		port->port.private_data = xr17v35x_register_gpio(pcidev, 0, 16);
+		ret = platform->register_gpio(pcidev, port);
 	}
 
-	return 0;
+	return ret;
 }
 
 static void pci_xr17v35x_exit(struct pci_dev *pcidev)
-- 
2.12.

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Andy Shevchenko]

On Thu, May 18, 2017 at 7:39 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-18 18:33, Andy Shevchenko wrote:
>> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

>>> +static bool is_iot2040;
>>
>> No, please, use driver data of DMI and hide this in corresponding structure.
>> Or even assign  port->port.rs485_config in the callback function.
>>
>> Moreover, can't you use port->port.rs485_config != NULL instead?
>
> There are two cases to be handled on IOT2040: the setting of the
> rs485_config and the different setup of the GPIOs, but the latter at a
> specific point in the initialization only. So I don't see yet how
> driver_data could come into play and help.

struct exar_iot2040_setup {
...rs485_config();
...setup_gpio();
};

struct exar_iot2040_setup iot2040_setup = {
...
};

DMI:

.driver_data = (void *)&iot2040_setup;

Above is just unfinished proposal, since I have noticed your new mail.
So, it seems we are on the same page.

One thing, I still would consider to use device properties instead of
platform data (with consideration of MFD framework usage).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] gpio-exar/8250-exar: Make set of exported GPIOs configurable

[Andy Shevchenko]

On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the
> rest is required to operate the UART. To allow modeling this case,
> expand the platform device data structure to specify a (consecutive) pin
> subset for exporting by the gpio-exar driver.

> +       unsigned int first_gpio;

Perhaps pin?
Or shift?

Because first_gpio a bit confusing with Linux side of GPIO.

> -       unsigned int bank = offset / 8;
> -       unsigned int addr;
> +       struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> +       unsigned int bank, addr;
>
> +       offset += exar_gpio->first_gpio;
> +       bank = offset / 8;

Can't we instead do something like the following:

struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
unsigned int bank = (offset + exar_gpio->pin) / 8;
unsigned int line = (offset + exar_gpio->pin) % 8;


> +       pdata.first_gpio = first_gpio;
> +       pdata.ngpio = ngpio;

Still thinking about device properties ("ngpios" and something like
"exar8250,gpio-start").

> +       unsigned int first_gpio;
> +       unsigned int ngpio;

u16 ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] gpio-exar/8250-exar: Make set of exported GPIOs configurable

[Jan Kiszka]

On 2017-05-18 19:43, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the
>> rest is required to operate the UART. To allow modeling this case,
>> expand the platform device data structure to specify a (consecutive) pin
>> subset for exporting by the gpio-exar driver.
> 
>> +       unsigned int first_gpio;
> 
> Perhaps pin?
> Or shift?
> 
> Because first_gpio a bit confusing with Linux side of GPIO.

Ack, going for "pin".

> 
>> -       unsigned int bank = offset / 8;
>> -       unsigned int addr;
>> +       struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
>> +       unsigned int bank, addr;
>>
>> +       offset += exar_gpio->first_gpio;
>> +       bank = offset / 8;
> 
> Can't we instead do something like the following:
> 
> struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> unsigned int bank = (offset + exar_gpio->pin) / 8;
> unsigned int line = (offset + exar_gpio->pin) % 8;
> 

OK, I'm using this pattern now:

	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
		EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;

> 
>> +       pdata.first_gpio = first_gpio;
>> +       pdata.ngpio = ngpio;
> 
> Still thinking about device properties ("ngpios" and something like
> "exar8250,gpio-start").

Changed back to properties, removing all platform data.

But what's the purpose of prefixing the name here? This does not have
anything to do with device trees. It's a private parameter channel
between the creating device driver and the gpio driver, and there will
be no other bindings.

> 
>> +       unsigned int first_gpio;
>> +       unsigned int ngpio;
> 
> u16 ?
> 

If we do that, then we would rather have to choose u8. But this is
pointless restriction. I prefer to stay with the native type.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 1/6] gpio: exar: Fix passing in of parent PCI device

[Jan Kiszka]

On 2017-05-18 19:14, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> This fixes reloading of the GPIO driver for the same platform device
>> instance as created by the exar UART driver: First of all, the driver
>> sets drvdata to its own value during probing and does not restore the
>> original value on exit. But this won't help anyway as the core clears
>> drvdata after the driver left.
>>
>> Use stable platform_data instead.
> 
> Okay, basically what we are trying to do here is to reinvent part of
> MFD framework.
> 
> I'd like to hear Linus' and others opinions if it worth to use it instead.
> 

I've looked into MFD modeling, but it would only make sense if we break
up the exar driver, change its xr17v35x part into a platform device and
create a dual-cell MFD for the PCI device. I don't think that would be
beneficial here. There are also dependencies between the UART part and
the MPIOs, specifically during init. All that would create a lot of
churn to the existing exar code.

I'm now passing the parent reference via device.parent instead of using
platform data.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 1/6] gpio: exar: Fix passing in of parent PCI device

[Linus Walleij]

On Sun, May 21, 2017 at 1:44 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-18 19:14, Andy Shevchenko wrote:
>> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> This fixes reloading of the GPIO driver for the same platform device
>>> instance as created by the exar UART driver: First of all, the driver
>>> sets drvdata to its own value during probing and does not restore the
>>> original value on exit. But this won't help anyway as the core clears
>>> drvdata after the driver left.
>>>
>>> Use stable platform_data instead.
>>
>> Okay, basically what we are trying to do here is to reinvent part of
>> MFD framework.
>>
>> I'd like to hear Linus' and others opinions if it worth to use it instead.
>
> I've looked into MFD modeling, but it would only make sense if we break
> up the exar driver, change its xr17v35x part into a platform device and
> create a dual-cell MFD for the PCI device. I don't think that would be
> beneficial here. There are also dependencies between the UART part and
> the MPIOs, specifically during init. All that would create a lot of
> churn to the existing exar code.
>
> I'm now passing the parent reference via device.parent instead of using
> platform data.

Actually I am pretty much OK with either, there are gray areas in
the device model and so it has to be sometimes.

I'd just like Greg's ACK on this so I can merge the whole series
through the GPIO tree.

Incidentally, he is the device model maintainer so he might have
some comments. Or be as tolerant as me. I don't know.

Greg?

Yours,
Linus Walleij

Re: [PATCH v2 5/6] gpio-exar/8250-exar: Make set of exported GPIOs configurable

[Linus Walleij]

On Thu, May 18, 2017 at 4:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the
> rest is required to operate the UART. To allow modeling this case,
> expand the platform device data structure to specify a (consecutive) pin
> subset for exporting by the gpio-exar driver.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I will need Greg's ACK on this too, but it seems
you are respinning it.

Yours,
Linus Walleij

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Linus Walleij]

On Thu, May 18, 2017 at 4:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> This implements the setup of RS232 and the switch-over to RS485 or RS422
> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
> to switch between the different modes. The external logic is controlled
> via MPIO pins of the EXAR controller.
>
> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
> an LED.
>
> As the XR17V352 used on the IOT2040 is not equipped with an external
> EEPROM, it cannot present itself as IOT2040-variant via subvendor/
> subdevice IDs. Thus, we have to check via DMI for the target platform.
>
> Co-developed with Sascha Weisenberger.
>
> Signed-off-by: Sascha Weisenberger <sascha.weisenberger@siemens.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Will this thing collide with your fixups so that Greg should take all
exar patches or can this be applied to the serial tree orthogonally?

Yours,
Linus Walleij

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Jan Kiszka]

On 2017-05-22 17:46, Linus Walleij wrote:
> On Thu, May 18, 2017 at 4:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> This implements the setup of RS232 and the switch-over to RS485 or RS422
>> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
>> to switch between the different modes. The external logic is controlled
>> via MPIO pins of the EXAR controller.
>>
>> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
>> an LED.
>>
>> As the XR17V352 used on the IOT2040 is not equipped with an external
>> EEPROM, it cannot present itself as IOT2040-variant via subvendor/
>> subdevice IDs. Thus, we have to check via DMI for the target platform.
>>
>> Co-developed with Sascha Weisenberger.
>>
>> Signed-off-by: Sascha Weisenberger <sascha.weisenberger@siemens.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Will this thing collide with your fixups so that Greg should take all
> exar patches or can this be applied to the serial tree orthogonally?

This patch has to come last. It depends on patch 5, e.g., and that has
dependencies as well.

FWIW, v3 of this series is coming, just requires another testing round.

Jan


-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 5/6] gpio-exar/8250-exar: Make set of exported GPIOs configurable

[Andy Shevchenko]

On Sun, May 21, 2017 at 2:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-18 19:43, Andy Shevchenko wrote:
>> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the

>>> +       pdata.first_gpio = first_gpio;
>>> +       pdata.ngpio = ngpio;
>>
>> Still thinking about device properties ("ngpios" and something like
>> "exar8250,gpio-start").
>
> Changed back to properties, removing all platform data.
>
> But what's the purpose of prefixing the name here? This does not have
> anything to do with device trees. It's a private parameter channel
> between the creating device driver and the gpio driver, and there will
> be no other bindings.

To avoid potential collision with registered official property, that's
why better to use prefix.
(I didn't find anything like GPIO start / pin in registered
properties, maybe there is one)

>>> +       unsigned int first_gpio;
>>> +       unsigned int ngpio;
>>
>> u16 ?

> If we do that, then we would rather have to choose u8. But this is
> pointless restriction. I prefer to stay with the native type.

Still for properties it would be u32, wouldn't it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Andy Shevchenko]

On Mon, May 22, 2017 at 7:28 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-22 17:46, Linus Walleij wrote:
>> On Thu, May 18, 2017 at 4:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> This patch has to come last. It depends on patch 5, e.g., and that has
> dependencies as well.

Btw, what about patch 1? I thought it might be shifted close to the
end of the series.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] serial: exar: Add support for IOT2040 device

[Jan Kiszka]

On 2017-05-22 18:34, Andy Shevchenko wrote:
> On Mon, May 22, 2017 at 7:28 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-05-22 17:46, Linus Walleij wrote:
>>> On Thu, May 18, 2017 at 4:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> This patch has to come last. It depends on patch 5, e.g., and that has
>> dependencies as well.
> 
> Btw, what about patch 1? I thought it might be shifted close to the
> end of the series.

Why? Patch 1 is the first step to fix the gpio-exar driver breakages.

But maybe I should split up those fixes and cleanups from the
preparations and enabling of the IOT2000.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 5/6] gpio-exar/8250-exar: Make set of exported GPIOs configurable

[Jan Kiszka]

On 2017-05-22 18:33, Andy Shevchenko wrote:
> On Sun, May 21, 2017 at 2:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-05-18 19:43, Andy Shevchenko wrote:
>>> On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the
> 
>>>> +       pdata.first_gpio = first_gpio;
>>>> +       pdata.ngpio = ngpio;
>>>
>>> Still thinking about device properties ("ngpios" and something like
>>> "exar8250,gpio-start").
>>
>> Changed back to properties, removing all platform data.
>>
>> But what's the purpose of prefixing the name here? This does not have
>> anything to do with device trees. It's a private parameter channel
>> between the creating device driver and the gpio driver, and there will
>> be no other bindings.
> 
> To avoid potential collision with registered official property, that's
> why better to use prefix.
> (I didn't find anything like GPIO start / pin in registered
> properties, maybe there is one)

When using the "public" channel devices properties, we cannot prevent
that people set some for the device, despite it is not supposed to be
controlled by DT or ACPI. But I don't see where default properties
should come from, except via intentionally designed DTs or ACPI tables.

Anyway, I can prefix.

> 
>>>> +       unsigned int first_gpio;
>>>> +       unsigned int ngpio;
>>>
>>> u16 ?
> 
>> If we do that, then we would rather have to choose u8. But this is
>> pointless restriction. I prefer to stay with the native type.
> 
> Still for properties it would be u32, wouldn't it?
> 

Because properties ask for a type width, yes. I can align both to u32,
though.

Jan