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

[PATCH 0/8] 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.

Jan

Jan Kiszka (8):
  serial: exar: Preconfigure xr17v35x MPIOs as output
  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
  serial: uapi: Add support for bus termination
  gpio-exar/8250-exar: Make set of exported GPIOs configurable
  serial: exar: Add support for IOT2040 device

 drivers/gpio/gpio-exar.c            |  55 ++++++++------
 drivers/tty/serial/8250/8250_exar.c | 145 +++++++++++++++++++++++++++++++++---
 include/uapi/linux/serial.h         |   3 +
 3 files changed, 169 insertions(+), 34 deletions(-)

-- 
2.12.0

[PATCH 1/8] serial: exar: Preconfigure xr17v35x MPIOs as output

[Jan Kiszka]

This is the safe default for GPIOs with unknown external wiring.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/tty/serial/8250/8250_exar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 1270ff163f63..b4fa585156c7 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -177,13 +177,13 @@ static void setup_gpio(u8 __iomem *p)
 	writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
 	writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
 	writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
-	writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
+	writeb(0xff, p + UART_EXAR_MPIOSEL_7_0);
 	writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
 	writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
 	writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
 	writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
 	writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
-	writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
+	writeb(0xff, p + UART_EXAR_MPIOSEL_15_8);
 	writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
 }
 
-- 
2.12.0

[PATCH 2/8] gpio: exar: Fix passing in of parent PCI device

[Jan Kiszka]

This fixes reloading of the driver for the same device: 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            | 2 +-
 drivers/tty/serial/8250/8250_exar.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 081076771217..0a2085faf271 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -119,7 +119,7 @@ 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 pci_dev *pcidev = *(struct pci_dev **)pdev->dev.platform_data;
 	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..2d056d1eeca3 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -196,8 +196,12 @@ xr17v35x_register_gpio(struct pci_dev *pcidev)
 	if (!pdev)
 		return NULL;
 
-	platform_set_drvdata(pdev, pcidev);
-	if (platform_device_add(pdev) < 0) {
+	/*
+	 * platform_device_add_data kmemdups the data, therefore we can safely
+	 * pass a stack reference.
+	 */
+	if (platform_device_add_data(pdev, &pcidev, sizeof(pcidev)) < 0 ||
+	    platform_device_add(pdev) < 0) {
 		platform_device_put(pdev);
 		return NULL;
 	}
-- 
2.12.0

[PATCH 3/8] 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>
---
 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 0a2085faf271..9138ee087c5d 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -139,7 +139,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;
 
@@ -160,7 +160,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 4/8] 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>
---
 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 9138ee087c5d..d6ffb3d89b9c 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -128,14 +128,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 5/8] 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 d6ffb3d89b9c..98bd3eb1290e 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -68,7 +68,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)
@@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 	addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
 	val = exar_get(chip, addr) >> (offset % 8);
 
-	return !!val;
+	return val & 1;
 }
 
 static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
@@ -89,10 +89,10 @@ 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;
+	addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
 	val = exar_get(chip, addr) >> (offset % 8);
 
-	return !!val;
+	return val & 1;
 }
 
 static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
-- 
2.12.0

[PATCH 6/8] serial: uapi: Add support for bus termination

[Jan Kiszka]

The Siemens IOT2040 comes with a RS485 interface that allows to enable
or disable bus termination via software. Add a bit to the flags field of
serial_rs485 that applications can set in order to request this feature
from the hardware. This seems generic enough to add it for everyone.
Existing driver will simply ignore it when set.

Signed-off-by: Sascha Weisenberger <sascha.weisenberger@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/uapi/linux/serial.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index 5d59c3ebf459..d2667ecd54ac 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -122,6 +122,9 @@ struct serial_rs485 {
 #define SER_RS485_RTS_AFTER_SEND	(1 << 2)	/* Logical level for
 							   RTS pin after sent*/
 #define SER_RS485_RX_DURING_TX		(1 << 4)
+#define SER_RS485_TERMINATE_BUS		(1 << 5)	/* Enable bus
+							   termination
+							   (if supported) */
 	__u32	delay_rts_before_send;	/* Delay before send (milliseconds) */
 	__u32	delay_rts_after_send;	/* Delay after send (milliseconds) */
 	__u32	padding[5];		/* Memory is cheap, new structs
-- 
2.12.0

[PATCH 7/8] 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,
use device properties 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            | 31 ++++++++++++++++++++++---------
 drivers/tty/serial/8250/8250_exar.c | 16 ++++++++++++----
 2 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 98bd3eb1290e..e2cb1d83e4b0 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -31,6 +31,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,
@@ -51,9 +52,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;
@@ -73,10 +76,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) >> (offset % 8);
 
@@ -85,10 +90,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) >> (offset % 8);
 
@@ -98,9 +105,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);
 }
@@ -123,6 +132,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	struct exar_gpio_chip *exar_gpio;
 	void __iomem *p;
 	int index, ret;
+	u32 val;
 
 	if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
 		return -ENODEV;
@@ -152,9 +162,12 @@ 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;
+	device_property_read_u32(&pdev->dev, "ngpio", &val);
+	exar_gpio->gpio_chip.ngpio = val;
 	exar_gpio->regs = p;
 	exar_gpio->index = index;
+	device_property_read_u32(&pdev->dev, "first_gpio", &val);
+	exar_gpio->first_gpio = val;
 
 	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 2d056d1eeca3..8e9c0e9495f5 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -19,6 +19,7 @@
 #include <linux/string.h>
 #include <linux/tty.h>
 #include <linux/8250_pci.h>
+#include <linux/property.h>
 
 #include <asm/byteorder.h>
 
@@ -188,8 +189,14 @@ 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 property_entry properties[] = {
+		PROPERTY_ENTRY_U32("first_gpio", first_gpio),
+		PROPERTY_ENTRY_U32("ngpio", ngpio),
+		{ }
+	};
 	struct platform_device *pdev;
 
 	pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
@@ -197,10 +204,11 @@ xr17v35x_register_gpio(struct pci_dev *pcidev)
 		return NULL;
 
 	/*
-	 * platform_device_add_data kmemdups the data, therefore we can safely
-	 * pass a stack reference.
+	 * platform_device_add_data and platform_device_add_properties copy
+	 * the data, therefore we can safely pass a stack references.
 	 */
 	if (platform_device_add_data(pdev, &pcidev, sizeof(pcidev)) < 0 ||
+	    platform_device_add_properties(pdev, properties) < 0 ||
 	    platform_device_add(pdev) < 0) {
 		platform_device_put(pdev);
 		return NULL;
@@ -242,7 +250,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;
-- 
2.12.0

[PATCH 8/8] 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 | 121 ++++++++++++++++++++++++++++++++++--
 1 file changed, 116 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 8e9c0e9495f5..62fe0f1ddcfe 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -19,6 +19,7 @@
 #include <linux/string.h>
 #include <linux/tty.h>
 #include <linux/8250_pci.h>
+#include <linux/dmi.h>
 #include <linux/property.h>
 
 #include <asm/byteorder.h>
@@ -61,6 +62,43 @@
 #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;
 
 /**
@@ -217,6 +255,65 @@ 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 iot2000_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
 pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		   struct uart_8250_port *port, int idx)
@@ -224,10 +321,20 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	const struct exar8250_board *board = priv->board;
 	unsigned int offset = idx * 0x400;
 	unsigned int baud = 7812500;
+	bool is_iot2040;
 	u8 __iomem *p;
 	int ret;
 
 	port->port.uartclk = baud * 16;
+
+	is_iot2040 =
+		strcmp(dmi_get_system_info(DMI_BOARD_NAME),
+		       "SIMATIC IOT2000") == 0 &&
+		strcmp(dmi_get_system_info(DMI_BOARD_ASSET_TAG),
+		       "6ES7647-0AA00-1YA2") == 0;
+	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)
@@ -246,14 +353,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 = iot2000_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 2/8] gpio: exar: Fix passing in of parent PCI device

[Andy Shevchenko]

On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> This fixes reloading of the driver for the same device: 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.

>From the above I didn't clearly get what device you are talking about.
GPIO?

Can you provide step by step what you did and what bug you got?

Regarding below it looks to me a bit hackish.

>  static int gpio_exar_probe(struct platform_device *pdev)
>  {
> -       struct pci_dev *pcidev = platform_get_drvdata(pdev);
> +       struct pci_dev *pcidev = *(struct pci_dev **)pdev->dev.platform_data;
>         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..2d056d1eeca3 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -196,8 +196,12 @@ xr17v35x_register_gpio(struct pci_dev *pcidev)
>         if (!pdev)
>                 return NULL;
>
> -       platform_set_drvdata(pdev, pcidev);
> -       if (platform_device_add(pdev) < 0) {
> +       /*
> +        * platform_device_add_data kmemdups the data, therefore we can safely
> +        * pass a stack reference.
> +        */
> +       if (platform_device_add_data(pdev, &pcidev, sizeof(pcidev)) < 0 ||
> +           platform_device_add(pdev) < 0) {
>                 platform_device_put(pdev);
>                 return NULL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/8] gpio: exar: Allocate resources on behalf of the platform device

[Andy Shevchenko]

On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 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.

This sounds to me like a good catch.
FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.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 0a2085faf271..9138ee087c5d 100644
> --- a/drivers/gpio/gpio-exar.c
> +++ b/drivers/gpio/gpio-exar.c
> @@ -139,7 +139,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;
>
> @@ -160,7 +160,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
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/8] gpio: exar: Fix iomap request

[Andy Shevchenko]

On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> The UART driver already maps the resource for us. Trying to do this here
> only fails and leaves us with a non-working device.

I hoped this had been tested previously...
FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.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 9138ee087c5d..d6ffb3d89b9c 100644
> --- a/drivers/gpio/gpio-exar.c
> +++ b/drivers/gpio/gpio-exar.c
> @@ -128,14 +128,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
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] gpio: exar: Fix reading of directions and values

[Andy Shevchenko]

On Sat, May 13, 2017 at 10:29 AM, 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.

Again, I wish it was tested in the first place.

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

> @@ -68,7 +68,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;

This one is correct.

> @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>         addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
>         val = exar_get(chip, addr) >> (offset % 8);
>
> -       return !!val;
> +       return val & 1;

It should be rather

        val = exar_get(chip, addr) & BIT(offset % 8);

>  }
>
>  static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
> @@ -89,10 +89,10 @@ 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;
> +       addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;

Good catch!

>         val = exar_get(chip, addr) >> (offset % 8);
>
> -       return !!val;
> +       return val & 1;

Ditto (see above).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/8] gpio-exar/8250-exar: Make set of exported GPIOs configurable

[Andy Shevchenko]

On Sat, May 13, 2017 at 10:29 AM, 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,
> use device properties to specify a (consecutive) pin subset for
> exporting by the gpio-exar driver.

> @@ -123,6 +132,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
>         struct exar_gpio_chip *exar_gpio;
>         void __iomem *p;
>         int index, ret;
> +       u32 val;
>
>         if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
>                 return -ENODEV;
> @@ -152,9 +162,12 @@ 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;

> +       device_property_read_u32(&pdev->dev, "ngpio", &val);
> +       exar_gpio->gpio_chip.ngpio = val;

You have to check return code, otherwise you might end up with
uninitialized data.

>         exar_gpio->regs = p;
>         exar_gpio->index = index;
> +       device_property_read_u32(&pdev->dev, "first_gpio", &val);
> +       exar_gpio->first_gpio = val;

Ditto.

>  #include <linux/string.h>
>  #include <linux/tty.h>
>  #include <linux/8250_pci.h>

> +#include <linux/property.h>

Alphabetical order, please.

>  static void *
> -xr17v35x_register_gpio(struct pci_dev *pcidev)
> +xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio,
> +                      unsigned int ngpio)
>  {
> +       struct property_entry properties[] = {

> +               PROPERTY_ENTRY_U32("first_gpio", first_gpio),

This should be documented in device tree bindings, otherwise it's a
linux software hook and I'm not sure it's the best what we can do
here.

> +               PROPERTY_ENTRY_U32("ngpio", ngpio),

This should be a registered property name (I see "ngpios" in the bindings).

> +               { }
> +       };
>         struct platform_device *pdev;
>
>         pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);


>         /*
> -        * platform_device_add_data kmemdups the data, therefore we can safely
> -        * pass a stack reference.
> +        * platform_device_add_data and platform_device_add_properties copy
> +        * the data, therefore we can safely pass a stack references.
>          */

Can you formulate this in an order to reduce ping-ponging lines across
the series?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, May 13, 2017 at 10:29 AM, 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.
>

Please, refactor that using properly formed DMI structrure and use its
callback and driver data facilities/

stmmac's pattern that you obviously applied does not actually suit here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/8] gpio: exar: Fix passing in of parent PCI device

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 2326 bytes --]

On 2017-05-13 15:25, Andy Shevchenko wrote:
> On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> This fixes reloading of the driver for the same device: 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.
> 
>>From the above I didn't clearly get what device you are talking about.
> GPIO?

"This fixes reloading of the GPIO driver for the same platform device
instance as created by the exar UART driver: [...]"

Clearer?

> 
> Can you provide step by step what you did and what bug you got?

Obviously a NULL pointer: Just rmmod gpio-exar and reload it while there
is the same platform device present.

> 
> Regarding below it looks to me a bit hackish.

The alternative is a classic platform data structure, then also carrying
the properties of patch 7 - which are, BTW, not DT-related, thus shall
not form an external interface. Probably another reason to switch
everything to some struct exar_gpio_platform_data.

Jan

> 
>>  static int gpio_exar_probe(struct platform_device *pdev)
>>  {
>> -       struct pci_dev *pcidev = platform_get_drvdata(pdev);
>> +       struct pci_dev *pcidev = *(struct pci_dev **)pdev->dev.platform_data;
>>         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..2d056d1eeca3 100644
>> --- a/drivers/tty/serial/8250/8250_exar.c
>> +++ b/drivers/tty/serial/8250/8250_exar.c
>> @@ -196,8 +196,12 @@ xr17v35x_register_gpio(struct pci_dev *pcidev)
>>         if (!pdev)
>>                 return NULL;
>>
>> -       platform_set_drvdata(pdev, pcidev);
>> -       if (platform_device_add(pdev) < 0) {
>> +       /*
>> +        * platform_device_add_data kmemdups the data, therefore we can safely
>> +        * pass a stack reference.
>> +        */
>> +       if (platform_device_add_data(pdev, &pcidev, sizeof(pcidev)) < 0 ||
>> +           platform_device_add(pdev) < 0) {
>>                 platform_device_put(pdev);
>>                 return NULL;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jan Kiszka]

On 2017-05-13 15:54, Andy Shevchenko wrote:
> On Sat, May 13, 2017 at 10:29 AM, 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.
>>
> 
> Please, refactor that using properly formed DMI structrure and use its
> callback and driver data facilities/

Could you point to a specific example? The callback of the dmi_system_id
structure is not designed to handle device initializations like this one
- not to speak of having those two different initialization points here.

Thanks,
Jan

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

Re: [PATCH 5/8] gpio: exar: Fix reading of directions and values

[Jan Kiszka]

On 2017-05-13 15:36, Andy Shevchenko wrote:
> On Sat, May 13, 2017 at 10:29 AM, 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.
> 
> Again, I wish it was tested in the first place.
> 
> After addressing below:
> FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
>> @@ -68,7 +68,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;
> 
> This one is correct.
> 
>> @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>>         addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
>>         val = exar_get(chip, addr) >> (offset % 8);
>>
>> -       return !!val;
>> +       return val & 1;
> 
> It should be rather
> 
>         val = exar_get(chip, addr) & BIT(offset % 8);

That won't give us 0 or 1 as return value, thus would be incorrect.

> 
>>  }
>>
>>  static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
>> @@ -89,10 +89,10 @@ 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;
>> +       addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
> 
> Good catch!
> 
>>         val = exar_get(chip, addr) >> (offset % 8);
>>
>> -       return !!val;
>> +       return val & 1;
> 
> Ditto (see above).
> 

Same here.

Jan

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

Re: [PATCH 5/8] gpio: exar: Fix reading of directions and values

[Andy Shevchenko]

On Thu, May 18, 2017 at 8:20 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-13 15:36, Andy Shevchenko wrote:
>> On Sat, May 13, 2017 at 10:29 AM, 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.
>>
>> Again, I wish it was tested in the first place.
>>
>> After addressing below:
>> FWIW:
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>
>>> @@ -68,7 +68,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;
>>
>> This one is correct.

>>
>>> @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>>>         addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
>>>         val = exar_get(chip, addr) >> (offset % 8);
>>>
>>> -       return !!val;
>>> +       return val & 1;
>>
>> It should be rather
>>
>>         val = exar_get(chip, addr) & BIT(offset % 8);
>
> That won't give us 0 or 1 as return value, thus would be incorrect.

Full picture:

 val = exar_get(chip, addr) & BIT(offset % 8);

 return !!val;

How it could be non-(1 or 0)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] gpio: exar: Fix reading of directions and values

[Jan Kiszka]

On 2017-05-18 12:11, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 8:20 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-05-13 15:36, Andy Shevchenko wrote:
>>> On Sat, May 13, 2017 at 10:29 AM, 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.
>>>
>>> Again, I wish it was tested in the first place.
>>>
>>> After addressing below:
>>> FWIW:
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>
>>>> @@ -68,7 +68,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;
>>>
>>> This one is correct.
> 
>>>
>>>> @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>>>>         addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
>>>>         val = exar_get(chip, addr) >> (offset % 8);
>>>>
>>>> -       return !!val;
>>>> +       return val & 1;
>>>
>>> It should be rather
>>>
>>>         val = exar_get(chip, addr) & BIT(offset % 8);
>>
>> That won't give us 0 or 1 as return value, thus would be incorrect.
> 
> Full picture:
> 
>  val = exar_get(chip, addr) & BIT(offset % 8);
> 
>  return !!val;
> 
> How it could be non-(1 or 0)?
> 

Right - but what is the point of that other style?

Jan

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

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

[Andy Shevchenko]

On Thu, May 18, 2017 at 8:06 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-13 15:54, Andy Shevchenko wrote:
>> On Sat, May 13, 2017 at 10:29 AM, 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.
>>>
>>
>> Please, refactor that using properly formed DMI structrure and use its
>> callback and driver data facilities/
>
> Could you point to a specific example? The callback of the dmi_system_id
> structure is not designed to handle device initializations like this one
> - not to speak of having those two different initialization points here.

Sure.

drivers/platform/x86/dell-laptop.c:

static const struct dmi_system_id dell_quirks[] __initconst = {
        {
                .callback = dmi_matched,
                .ident = "Dell Vostro V130",
                .matches = {
                        DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
                        DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V130"),
                },
                .driver_data = &quirk_dell_vostro_v130,
        },


(just in case, dmi_matched() is part of that module)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] gpio: exar: Fix reading of directions and values

[Andy Shevchenko]

On Thu, May 18, 2017 at 1:16 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-18 12:11, Andy Shevchenko wrote:
>> On Thu, May 18, 2017 at 8:20 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

>> Full picture:
>>
>>  val = exar_get(chip, addr) & BIT(offset % 8);
>>
>>  return !!val;
>>
>> How it could be non-(1 or 0)?
>>
>
> Right - but what is the point of that other style?

gpio-exar.c is just 4th module which is using it, OTOH the rest of
GPIO drivers are using return !!val style.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] gpio: exar: Fix reading of directions and values

[Jan Kiszka]

On 2017-05-18 12:23, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 1:16 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-05-18 12:11, Andy Shevchenko wrote:
>>> On Thu, May 18, 2017 at 8:20 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>>> Full picture:
>>>
>>>  val = exar_get(chip, addr) & BIT(offset % 8);
>>>
>>>  return !!val;
>>>
>>> How it could be non-(1 or 0)?
>>>
>>
>> Right - but what is the point of that other style?
> 
> gpio-exar.c is just 4th module which is using it, OTOH the rest of
> GPIO drivers are using return !!val style.
> 

OK, consistency is valid point. Will adjust.

Jan

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

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

[Jan Kiszka]

On 2017-05-18 12:17, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 8:06 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-05-13 15:54, Andy Shevchenko wrote:
>>> On Sat, May 13, 2017 at 10:29 AM, 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.
>>>>
>>>
>>> Please, refactor that using properly formed DMI structrure and use its
>>> callback and driver data facilities/
>>
>> Could you point to a specific example? The callback of the dmi_system_id
>> structure is not designed to handle device initializations like this one
>> - not to speak of having those two different initialization points here.
> 
> Sure.
> 
> drivers/platform/x86/dell-laptop.c:
> 
> static const struct dmi_system_id dell_quirks[] __initconst = {
>         {
>                 .callback = dmi_matched,
>                 .ident = "Dell Vostro V130",
>                 .matches = {
>                         DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>                         DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V130"),
>                 },
>                 .driver_data = &quirk_dell_vostro_v130,
>         },
> 
> 
> (just in case, dmi_matched() is part of that module)
> 

OK, but that would basically replace the two lines for matching against
the DMI values with the structure above. We would still need the
existing hooks at the two points where we have them right now. Doesn't
look to me as if the code would become clearer or better separated or
easier extensible (the latter being hard anyway without knowing any
potential third special case).

Jan

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

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

[Andy Shevchenko]

On Thu, May 18, 2017 at 1:37 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-18 12:17, Andy Shevchenko wrote:
>> On Thu, May 18, 2017 at 8:06 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-05-13 15:54, Andy Shevchenko wrote:

>>>> Please, refactor that using properly formed DMI structrure and use its
>>>> callback and driver data facilities/
>>>
>>> Could you point to a specific example? The callback of the dmi_system_id
>>> structure is not designed to handle device initializations like this one
>>> - not to speak of having those two different initialization points here.
>>
>> Sure.
>>
>> drivers/platform/x86/dell-laptop.c:
>>
>> static const struct dmi_system_id dell_quirks[] __initconst = {
>>         {
>>                 .callback = dmi_matched,
>>                 .ident = "Dell Vostro V130",
>>                 .matches = {
>>                         DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>                         DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V130"),
>>                 },
>>                 .driver_data = &quirk_dell_vostro_v130,
>>         },
>>
>>
>> (just in case, dmi_matched() is part of that module)
>>
>
> OK, but that would basically replace the two lines for matching against
> the DMI values with the structure above. We would still need the
> existing hooks at the two points where we have them right now.

Right.

> Doesn't
> look to me as if the code would become clearer or better separated or
> easier extensible (the latter being hard anyway without knowing any
> potential third special case).

stmmac, that you chose as an example, should go that way in the first
place because it was my though behind which is on-to-one to yours
above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/8] serial: exar: Preconfigure xr17v35x MPIOs as output

[Linus Walleij]

On Sat, May 13, 2017 at 9:28 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> This is the safe default for GPIOs with unknown external wiring.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/8] gpio: exar: Allocate resources on behalf of the platform device

[Linus Walleij]

On Sat, May 13, 2017 at 9:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> 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>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 1/8] serial: exar: Preconfigure xr17v35x MPIOs as output

[Jan Kiszka]

Greg,

On 2017-05-13 09:28, Jan Kiszka wrote:
> This is the safe default for GPIOs with unknown external wiring.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 1270ff163f63..b4fa585156c7 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -177,13 +177,13 @@ static void setup_gpio(u8 __iomem *p)
>  	writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
>  	writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
>  	writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
> -	writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
> +	writeb(0xff, p + UART_EXAR_MPIOSEL_7_0);
>  	writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
>  	writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
>  	writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
>  	writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
>  	writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
> -	writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
> +	writeb(0xff, p + UART_EXAR_MPIOSEL_15_8);
>  	writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
>  }
>  
> 

Please drop from this tty-next, it may break Commtech adapters. I have
an update ready.

I may also send a fix on top if that is preferred, just let me know.

Thanks,
Jan

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

Re: [PATCH 1/8] serial: exar: Preconfigure xr17v35x MPIOs as output

[Andy Shevchenko]

On Fri, May 26, 2017 at 4:04 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Greg,

> Please drop from this tty-next, it may break Commtech adapters. I have
> an update ready.
>
> I may also send a fix on top if that is preferred, just let me know.

AFAIK what is in tty-next is left there, so, looks like latter is the option.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/8] serial: exar: Preconfigure xr17v35x MPIOs as output

[Greg Kroah-Hartman]

On Fri, May 26, 2017 at 03:04:26PM +0200, Jan Kiszka wrote:
> Greg,
> 
> On 2017-05-13 09:28, Jan Kiszka wrote:
> > This is the safe default for GPIOs with unknown external wiring.
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 1270ff163f63..b4fa585156c7 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -177,13 +177,13 @@ static void setup_gpio(u8 __iomem *p)
> >  	writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
> >  	writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
> >  	writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
> > -	writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
> > +	writeb(0xff, p + UART_EXAR_MPIOSEL_7_0);
> >  	writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
> >  	writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
> >  	writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
> >  	writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
> >  	writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
> > -	writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
> > +	writeb(0xff, p + UART_EXAR_MPIOSEL_15_8);
> >  	writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
> >  }
> >  
> > 
> 
> Please drop from this tty-next, it may break Commtech adapters. I have
> an update ready.
> 
> I may also send a fix on top if that is preferred, just let me know.

I need a fixup patch, I can't drop a patch, only revert it.

thanks,

greg k-h