[PATCH v4 0/5] add support for another simatic board

[PATCH v4 0/5] add support for another simatic board

[Henning Schild]

changes since v3:
  - update Kconfig as well
  - drop chip names from comment in driver header
  - add manufacturer check for Fintek again, Nuvoton not possible
  - drop revision printing for Nuvoton
  - restructure defines again
  - add new model 427G

changes since v2: (p1 only)
  - rename macros that change behavior
  - use chip type not device id in the macros
  - reorder defines a bit

changes since v1:
  - remove unused define
  - fix bug where (base + 2) was used as second data bit
  - add macros for "inverted" and "single data bit"

This series first enables a SuperIO GPIO driver to support a chip from
the vendor Nuvoton, the driver is for Fintek devices but those just are
very similar. And in watchdog and hwmon subsystems these SuperIO drivers
also share code and are sometimes called a family.

In another step the individual banks receive a label to tell them apart,
a step which potentially changes an interface to legacy users that might
rely on all banks having the same label, or an exact label. But since a
later patch wants to use GPIO_LOOKUP unique labels are needed and i
decided to assign them for all supported chips.

In a following patch the Simatic GPIO LED driver is extended to provide
LEDs in case that SuperIO GPIO driver can be loaded.

Last but not least the watchdog module of that same SuperIO gets loaded
on a best effort basis.

Note similar patches have appreared before as
  "[PATCH v3 0/1] add device driver for Nuvoton SIO gpio function"
The main difference here is that i added chip support to an existing
driver instead of creating a new one. And that i actually propose all
patches and do not just have the LED patch for Simatic as an example.
Also note that the patches are based on
  "[PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper"

Henning Schild (5):
  gpio-f7188x: Add GPIO support for Nuvoton NCT6116
  gpio-f7188x: use unique labels for banks/chips
  leds: simatic-ipc-leds-gpio: add new model 227G
  platform/x86: simatic-ipc: enable watchdog for 227G
  platform/x86: simatic-ipc: add new model 427G

 drivers/gpio/Kconfig                          |   3 +-
 drivers/gpio/gpio-f7188x.c                    | 229 +++++++++++-------
 drivers/leds/simple/simatic-ipc-leds-gpio.c   |  42 +++-
 drivers/platform/x86/simatic-ipc.c            |  10 +-
 .../platform_data/x86/simatic-ipc-base.h      |   1 +
 include/linux/platform_data/x86/simatic-ipc.h |   2 +
 6 files changed, 194 insertions(+), 93 deletions(-)

-- 
2.35.1

[PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[Henning Schild]

Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO chips are
very similar to the ones from Fintek. In other subsystems they also
share drivers and are called a family of drivers.

For the GPIO subsystem the only difference is that the direction bit is
reversed and that there is only one data bit per pin. On the SuperIO
level the logical device is another one.

On a chip level we do not have a manufacturer ID to check and also no
revision.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/gpio/Kconfig       |   3 +-
 drivers/gpio/gpio-f7188x.c | 107 ++++++++++++++++++++++++++++---------
 2 files changed, 84 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0642f579196f..3f64345fe40b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -874,10 +874,11 @@ config GPIO_104_IDI_48
 	  module parameter.
 
 config GPIO_F7188X
-	tristate "F71869, F71869A, F71882FG, F71889F and F81866 GPIO support"
+	tristate "Fintek and Nuvoton Super-I/O GPIO support"
 	help
 	  This option enables support for GPIOs found on Fintek Super-I/O
 	  chips F71869, F71869A, F71882FG, F71889F and F81866.
+	  As well as Nuvoton Super-I/O chip NCT6116D.
 
 	  To compile this driver as a module, choose M here: the module will
 	  be called f7188x-gpio.
diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index 18a3147f5a42..820ac5d60fda 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889 and F81866
+ * GPIO driver for Fintek and Nuvoton Super-I/O chips
  *
  * Copyright (C) 2010-2013 LaCie
  *
@@ -21,23 +21,36 @@
  */
 #define SIO_LDSEL		0x07	/* Logical device select */
 #define SIO_DEVID		0x20	/* Device ID (2 bytes) */
-#define SIO_DEVREV		0x22	/* Device revision */
-#define SIO_MANID		0x23	/* Fintek ID (2 bytes) */
 
-#define SIO_LD_GPIO		0x06	/* GPIO logical device */
 #define SIO_UNLOCK_KEY		0x87	/* Key to enable Super-I/O */
 #define SIO_LOCK_KEY		0xAA	/* Key to disable Super-I/O */
 
-#define SIO_FINTEK_ID		0x1934	/* Manufacturer ID */
+/*
+ * Fintek devices.
+ */
+#define SIO_FINTEK_DEVREV	0x22	/* Fintek Device revision */
+#define SIO_FINTEK_MANID	0x23    /* Fintek ID (2 bytes) */
+
+#define SIO_FINTEK_ID		0x1934  /* Manufacturer ID */
+
 #define SIO_F71869_ID		0x0814	/* F71869 chipset ID */
 #define SIO_F71869A_ID		0x1007	/* F71869A chipset ID */
 #define SIO_F71882_ID		0x0541	/* F71882 chipset ID */
 #define SIO_F71889_ID		0x0909	/* F71889 chipset ID */
 #define SIO_F71889A_ID		0x1005	/* F71889A chipset ID */
 #define SIO_F81866_ID		0x1010	/* F81866 chipset ID */
-#define SIO_F81804_ID		0x1502  /* F81804 chipset ID, same for f81966 */
+#define SIO_F81804_ID		0x1502  /* F81804 chipset ID, same for F81966 */
 #define SIO_F81865_ID		0x0704	/* F81865 chipset ID */
 
+#define SIO_LD_GPIO_FINTEK	0x06	/* GPIO logical device */
+
+/*
+ * Nuvoton devices.
+ */
+#define SIO_NCT6116D_ID		0xD283  /* NCT6116D chipset ID */
+
+#define SIO_LD_GPIO_NUVOTON	0x07	/* GPIO logical device */
+
 
 enum chips {
 	f71869,
@@ -48,6 +61,7 @@ enum chips {
 	f81866,
 	f81804,
 	f81865,
+	nct6116d,
 };
 
 static const char * const f7188x_names[] = {
@@ -59,10 +73,12 @@ static const char * const f7188x_names[] = {
 	"f81866",
 	"f81804",
 	"f81865",
+	"nct6116d",
 };
 
 struct f7188x_sio {
 	int addr;
+	int device;
 	enum chips type;
 };
 
@@ -170,6 +186,9 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset,
 /* Output mode register (0:open drain 1:push-pull). */
 #define gpio_out_mode(base) (base + 3)
 
+#define gpio_dir_invert(type)	((type) == nct6116d)
+#define gpio_data_single(type)	((type) == nct6116d)
+
 static struct f7188x_gpio_bank f71869_gpio_bank[] = {
 	F7188X_GPIO_BANK(0, 6, 0xF0),
 	F7188X_GPIO_BANK(10, 8, 0xE0),
@@ -254,6 +273,17 @@ static struct f7188x_gpio_bank f81865_gpio_bank[] = {
 	F7188X_GPIO_BANK(60, 5, 0x90),
 };
 
+static struct f7188x_gpio_bank nct6116d_gpio_bank[] = {
+	F7188X_GPIO_BANK(0, 8, 0xE0),
+	F7188X_GPIO_BANK(10, 8, 0xE4),
+	F7188X_GPIO_BANK(20, 8, 0xE8),
+	F7188X_GPIO_BANK(30, 8, 0xEC),
+	F7188X_GPIO_BANK(40, 8, 0xF0),
+	F7188X_GPIO_BANK(50, 8, 0xF4),
+	F7188X_GPIO_BANK(60, 8, 0xF8),
+	F7188X_GPIO_BANK(70, 1, 0xFC),
+};
+
 static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 {
 	int err;
@@ -264,13 +294,16 @@ static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 	err = superio_enter(sio->addr);
 	if (err)
 		return err;
-	superio_select(sio->addr, SIO_LD_GPIO);
+	superio_select(sio->addr, sio->device);
 
 	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
 
 	superio_exit(sio->addr);
 
-	if (dir & 1 << offset)
+	if (gpio_dir_invert(sio->type))
+		dir = ~dir;
+
+	if (dir & BIT(offset))
 		return GPIO_LINE_DIRECTION_OUT;
 
 	return GPIO_LINE_DIRECTION_IN;
@@ -286,10 +319,14 @@ static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
 	err = superio_enter(sio->addr);
 	if (err)
 		return err;
-	superio_select(sio->addr, SIO_LD_GPIO);
+	superio_select(sio->addr, sio->device);
 
 	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
-	dir &= ~BIT(offset);
+
+	if (gpio_dir_invert(sio->type))
+		dir |= BIT(offset);
+	else
+		dir &= ~BIT(offset);
 	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
 
 	superio_exit(sio->addr);
@@ -307,11 +344,11 @@ static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset)
 	err = superio_enter(sio->addr);
 	if (err)
 		return err;
-	superio_select(sio->addr, SIO_LD_GPIO);
+	superio_select(sio->addr, sio->device);
 
 	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
 	dir = !!(dir & BIT(offset));
-	if (dir)
+	if (gpio_data_single(sio->type) || dir)
 		data = superio_inb(sio->addr, gpio_data_out(bank->regbase));
 	else
 		data = superio_inb(sio->addr, gpio_data_in(bank->regbase));
@@ -332,7 +369,7 @@ static int f7188x_gpio_direction_out(struct gpio_chip *chip,
 	err = superio_enter(sio->addr);
 	if (err)
 		return err;
-	superio_select(sio->addr, SIO_LD_GPIO);
+	superio_select(sio->addr, sio->device);
 
 	data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase));
 	if (value)
@@ -342,7 +379,10 @@ static int f7188x_gpio_direction_out(struct gpio_chip *chip,
 	superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out);
 
 	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
-	dir |= BIT(offset);
+	if (gpio_dir_invert(sio->type))
+		dir &= ~BIT(offset);
+	else
+		dir |= BIT(offset);
 	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
 
 	superio_exit(sio->addr);
@@ -360,7 +400,7 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	err = superio_enter(sio->addr);
 	if (err)
 		return;
-	superio_select(sio->addr, SIO_LD_GPIO);
+	superio_select(sio->addr, sio->device);
 
 	data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase));
 	if (value)
@@ -388,7 +428,7 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset,
 	err = superio_enter(sio->addr);
 	if (err)
 		return err;
-	superio_select(sio->addr, SIO_LD_GPIO);
+	superio_select(sio->addr, sio->device);
 
 	data = superio_inb(sio->addr, gpio_out_mode(bank->regbase));
 	if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN)
@@ -449,6 +489,10 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
 		data->nr_bank = ARRAY_SIZE(f81865_gpio_bank);
 		data->bank = f81865_gpio_bank;
 		break;
+	case nct6116d:
+		data->nr_bank = ARRAY_SIZE(nct6116d_gpio_bank);
+		data->bank = nct6116d_gpio_bank;
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -479,18 +523,15 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio)
 {
 	int err;
 	u16 devid;
+	u16 manid;
 
 	err = superio_enter(addr);
 	if (err)
 		return err;
 
 	err = -ENODEV;
-	devid = superio_inw(addr, SIO_MANID);
-	if (devid != SIO_FINTEK_ID) {
-		pr_debug(DRVNAME ": Not a Fintek device at 0x%08x\n", addr);
-		goto err;
-	}
 
+	sio->device = SIO_LD_GPIO_FINTEK;
 	devid = superio_inw(addr, SIO_DEVID);
 	switch (devid) {
 	case SIO_F71869_ID:
@@ -517,17 +558,33 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio)
 	case SIO_F81865_ID:
 		sio->type = f81865;
 		break;
+	case SIO_NCT6116D_ID:
+		sio->device = SIO_LD_GPIO_NUVOTON;
+		sio->type = nct6116d;
+		break;
 	default:
-		pr_info(DRVNAME ": Unsupported Fintek device 0x%04x\n", devid);
+		pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid);
 		goto err;
 	}
+
+	/* double check manufacturer where possible */
+	if (sio->type != nct6116d) {
+		manid = superio_inw(addr, SIO_FINTEK_MANID);
+		if (manid != SIO_FINTEK_ID) {
+			pr_debug(DRVNAME ": Not a Fintek device at 0x%08x\n", addr);
+			goto err;
+		}
+	}
+
 	sio->addr = addr;
 	err = 0;
 
-	pr_info(DRVNAME ": Found %s at %#x, revision %d\n",
+	pr_info(DRVNAME ": Found %s at %#x\n",
 		f7188x_names[sio->type],
-		(unsigned int) addr,
-		(int) superio_inb(addr, SIO_DEVREV));
+		(unsigned int)addr);
+	if (sio->type != nct6116d)
+		pr_info(DRVNAME ":   revision %d\n",
+			(int)superio_inb(addr, SIO_FINTEK_DEVREV));
 
 err:
 	superio_exit(addr);
-- 
2.35.1

[PATCH v4 2/5] gpio-f7188x: use unique labels for banks/chips

[Henning Schild]

So that drivers building on top can find those pins with GPIO_LOOKUP
helpers.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Simon Guinot <simon.guinot@sequanux.org>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/gpio/gpio-f7188x.c | 138 ++++++++++++++++++-------------------
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index 820ac5d60fda..c9b18c6d8464 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -162,10 +162,10 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value);
 static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset,
 				  unsigned long config);
 
-#define F7188X_GPIO_BANK(_base, _ngpio, _regbase)			\
+#define F7188X_GPIO_BANK(_base, _ngpio, _regbase, _label)			\
 	{								\
 		.chip = {						\
-			.label            = DRVNAME,			\
+			.label            = _label,			\
 			.owner            = THIS_MODULE,		\
 			.get_direction    = f7188x_gpio_get_direction,	\
 			.direction_input  = f7188x_gpio_direction_in,	\
@@ -190,98 +190,98 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset,
 #define gpio_data_single(type)	((type) == nct6116d)
 
 static struct f7188x_gpio_bank f71869_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 6, 0xF0),
-	F7188X_GPIO_BANK(10, 8, 0xE0),
-	F7188X_GPIO_BANK(20, 8, 0xD0),
-	F7188X_GPIO_BANK(30, 8, 0xC0),
-	F7188X_GPIO_BANK(40, 8, 0xB0),
-	F7188X_GPIO_BANK(50, 5, 0xA0),
-	F7188X_GPIO_BANK(60, 6, 0x90),
+	F7188X_GPIO_BANK(0, 6, 0xF0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
+	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
+	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
+	F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"),
+	F7188X_GPIO_BANK(60, 6, 0x90, DRVNAME "-6"),
 };
 
 static struct f7188x_gpio_bank f71869a_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 6, 0xF0),
-	F7188X_GPIO_BANK(10, 8, 0xE0),
-	F7188X_GPIO_BANK(20, 8, 0xD0),
-	F7188X_GPIO_BANK(30, 8, 0xC0),
-	F7188X_GPIO_BANK(40, 8, 0xB0),
-	F7188X_GPIO_BANK(50, 5, 0xA0),
-	F7188X_GPIO_BANK(60, 8, 0x90),
-	F7188X_GPIO_BANK(70, 8, 0x80),
+	F7188X_GPIO_BANK(0, 6, 0xF0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
+	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
+	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
+	F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"),
+	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"),
+	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"),
 };
 
 static struct f7188x_gpio_bank f71882_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 8, 0xF0),
-	F7188X_GPIO_BANK(10, 8, 0xE0),
-	F7188X_GPIO_BANK(20, 8, 0xD0),
-	F7188X_GPIO_BANK(30, 4, 0xC0),
-	F7188X_GPIO_BANK(40, 4, 0xB0),
+	F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
+	F7188X_GPIO_BANK(30, 4, 0xC0, DRVNAME "-3"),
+	F7188X_GPIO_BANK(40, 4, 0xB0, DRVNAME "-4"),
 };
 
 static struct f7188x_gpio_bank f71889a_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 7, 0xF0),
-	F7188X_GPIO_BANK(10, 7, 0xE0),
-	F7188X_GPIO_BANK(20, 8, 0xD0),
-	F7188X_GPIO_BANK(30, 8, 0xC0),
-	F7188X_GPIO_BANK(40, 8, 0xB0),
-	F7188X_GPIO_BANK(50, 5, 0xA0),
-	F7188X_GPIO_BANK(60, 8, 0x90),
-	F7188X_GPIO_BANK(70, 8, 0x80),
+	F7188X_GPIO_BANK(0, 7, 0xF0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 7, 0xE0, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
+	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
+	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
+	F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"),
+	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"),
+	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"),
 };
 
 static struct f7188x_gpio_bank f71889_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 7, 0xF0),
-	F7188X_GPIO_BANK(10, 7, 0xE0),
-	F7188X_GPIO_BANK(20, 8, 0xD0),
-	F7188X_GPIO_BANK(30, 8, 0xC0),
-	F7188X_GPIO_BANK(40, 8, 0xB0),
-	F7188X_GPIO_BANK(50, 5, 0xA0),
-	F7188X_GPIO_BANK(60, 8, 0x90),
-	F7188X_GPIO_BANK(70, 8, 0x80),
+	F7188X_GPIO_BANK(0, 7, 0xF0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 7, 0xE0, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
+	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
+	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
+	F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"),
+	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"),
+	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"),
 };
 
 static struct f7188x_gpio_bank f81866_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 8, 0xF0),
-	F7188X_GPIO_BANK(10, 8, 0xE0),
-	F7188X_GPIO_BANK(20, 8, 0xD0),
-	F7188X_GPIO_BANK(30, 8, 0xC0),
-	F7188X_GPIO_BANK(40, 8, 0xB0),
-	F7188X_GPIO_BANK(50, 8, 0xA0),
-	F7188X_GPIO_BANK(60, 8, 0x90),
-	F7188X_GPIO_BANK(70, 8, 0x80),
-	F7188X_GPIO_BANK(80, 8, 0x88),
+	F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
+	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
+	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
+	F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-5"),
+	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"),
+	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"),
+	F7188X_GPIO_BANK(80, 8, 0x88, DRVNAME "-8"),
 };
 
 
 static struct f7188x_gpio_bank f81804_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 8, 0xF0),
-	F7188X_GPIO_BANK(10, 8, 0xE0),
-	F7188X_GPIO_BANK(20, 8, 0xD0),
-	F7188X_GPIO_BANK(50, 8, 0xA0),
-	F7188X_GPIO_BANK(60, 8, 0x90),
-	F7188X_GPIO_BANK(70, 8, 0x80),
-	F7188X_GPIO_BANK(90, 8, 0x98),
+	F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
+	F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-3"),
+	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-4"),
+	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-5"),
+	F7188X_GPIO_BANK(90, 8, 0x98, DRVNAME "-6"),
 };
 
 static struct f7188x_gpio_bank f81865_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 8, 0xF0),
-	F7188X_GPIO_BANK(10, 8, 0xE0),
-	F7188X_GPIO_BANK(20, 8, 0xD0),
-	F7188X_GPIO_BANK(30, 8, 0xC0),
-	F7188X_GPIO_BANK(40, 8, 0xB0),
-	F7188X_GPIO_BANK(50, 8, 0xA0),
-	F7188X_GPIO_BANK(60, 5, 0x90),
+	F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
+	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
+	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
+	F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-5"),
+	F7188X_GPIO_BANK(60, 5, 0x90, DRVNAME "-6"),
 };
 
 static struct f7188x_gpio_bank nct6116d_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 8, 0xE0),
-	F7188X_GPIO_BANK(10, 8, 0xE4),
-	F7188X_GPIO_BANK(20, 8, 0xE8),
-	F7188X_GPIO_BANK(30, 8, 0xEC),
-	F7188X_GPIO_BANK(40, 8, 0xF0),
-	F7188X_GPIO_BANK(50, 8, 0xF4),
-	F7188X_GPIO_BANK(60, 8, 0xF8),
-	F7188X_GPIO_BANK(70, 1, 0xFC),
+	F7188X_GPIO_BANK(0, 8, 0xE0, DRVNAME "-0"),
+	F7188X_GPIO_BANK(10, 8, 0xE4, DRVNAME "-1"),
+	F7188X_GPIO_BANK(20, 8, 0xE8, DRVNAME "-2"),
+	F7188X_GPIO_BANK(30, 8, 0xEC, DRVNAME "-3"),
+	F7188X_GPIO_BANK(40, 8, 0xF0, DRVNAME "-4"),
+	F7188X_GPIO_BANK(50, 8, 0xF4, DRVNAME "-5"),
+	F7188X_GPIO_BANK(60, 8, 0xF8, DRVNAME "-6"),
+	F7188X_GPIO_BANK(70, 1, 0xFC, DRVNAME "-7"),
 };
 
 static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
-- 
2.35.1

[PATCH v4 3/5] leds: simatic-ipc-leds-gpio: add new model 227G

[Henning Schild]

This adds support of the Siemens Simatic IPC227G. Its LEDs are connected
to GPIO pins provided by the gpio-f7188x module. We make sure that
gets loaded, if not enabled in the kernel config no LED support will be
available.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/leds/simple/simatic-ipc-leds-gpio.c   | 42 ++++++++++++++++---
 drivers/platform/x86/simatic-ipc.c            |  4 +-
 .../platform_data/x86/simatic-ipc-base.h      |  1 +
 include/linux/platform_data/x86/simatic-ipc.h |  1 +
 4 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c
index 4c9e663a90ba..0d73dcbeec2d 100644
--- a/drivers/leds/simple/simatic-ipc-leds-gpio.c
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
@@ -13,28 +13,45 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/x86/simatic-ipc-base.h>
 
-static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
+struct gpiod_lookup_table *simatic_ipc_led_gpio_table;
+
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table_127e = {
 	.dev_id = "leds-gpio",
 	.table = {
-		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0, GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1, GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2, GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3, GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4, GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0, GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6, GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7, GPIO_ACTIVE_HIGH),
 	},
 };
 
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table_227g = {
+	.dev_id = "leds-gpio",
+	.table = {
+		GPIO_LOOKUP_IDX("gpio-f7188x-2", 0, NULL, 0, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio-f7188x-2", 1, NULL, 1, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio-f7188x-2", 2, NULL, 2, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio-f7188x-2", 3, NULL, 3, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio-f7188x-2", 4, NULL, 4, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio-f7188x-2", 5, NULL, 5, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio-f7188x-3", 6, NULL, 6, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("gpio-f7188x-3", 7, NULL, 7, GPIO_ACTIVE_HIGH),
+	}
+};
+
 static const struct gpio_led simatic_ipc_gpio_leds[] = {
-	{ .name = "green:" LED_FUNCTION_STATUS "-3" },
 	{ .name = "red:" LED_FUNCTION_STATUS "-1" },
 	{ .name = "green:" LED_FUNCTION_STATUS "-1" },
 	{ .name = "red:" LED_FUNCTION_STATUS "-2" },
 	{ .name = "green:" LED_FUNCTION_STATUS "-2" },
 	{ .name = "red:" LED_FUNCTION_STATUS "-3" },
+	{ .name = "green:" LED_FUNCTION_STATUS "-3" },
 };
 
 static const struct gpio_led_platform_data simatic_ipc_gpio_leds_pdata = {
@@ -46,7 +63,7 @@ static struct platform_device *simatic_leds_pdev;
 
 static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev)
 {
-	gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table);
+	gpiod_remove_lookup_table(simatic_ipc_led_gpio_table);
 	platform_device_unregister(simatic_leds_pdev);
 
 	return 0;
@@ -54,10 +71,25 @@ static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev)
 
 static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
 {
+	const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
 	struct gpio_desc *gpiod;
 	int err;
 
-	gpiod_add_lookup_table(&simatic_ipc_led_gpio_table);
+	switch (plat->devmode) {
+	case SIMATIC_IPC_DEVICE_127E:
+		simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_127e;
+		break;
+	case SIMATIC_IPC_DEVICE_227G:
+		if (!IS_ENABLED(CONFIG_GPIO_F7188X))
+			return -ENODEV;
+		request_module("gpio-f7188x");
+		simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_227g;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	gpiod_add_lookup_table(simatic_ipc_led_gpio_table);
 	simatic_leds_pdev = platform_device_register_resndata(NULL,
 		"leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
 		&simatic_ipc_gpio_leds_pdata,
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index ca3647b751d5..1825ef21a86d 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -41,6 +41,7 @@ static struct {
 	{SIMATIC_IPC_IPC127E, SIMATIC_IPC_DEVICE_127E, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC227D, SIMATIC_IPC_DEVICE_227D, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC227E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_227E},
+	{SIMATIC_IPC_IPC227G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC277E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_227E},
 	{SIMATIC_IPC_IPC427D, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC427E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_427E},
@@ -65,7 +66,8 @@ static int register_platform_devices(u32 station_id)
 	}
 
 	if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
-		if (ledmode == SIMATIC_IPC_DEVICE_127E)
+		if (ledmode == SIMATIC_IPC_DEVICE_127E ||
+		    ledmode == SIMATIC_IPC_DEVICE_227G)
 			pdevname = KBUILD_MODNAME "_leds_gpio";
 		platform_data.devmode = ledmode;
 		ipc_led_platform_device =
diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h
index 39fefd48cf4d..57d6a10dfc9e 100644
--- a/include/linux/platform_data/x86/simatic-ipc-base.h
+++ b/include/linux/platform_data/x86/simatic-ipc-base.h
@@ -19,6 +19,7 @@
 #define SIMATIC_IPC_DEVICE_427E 2
 #define SIMATIC_IPC_DEVICE_127E 3
 #define SIMATIC_IPC_DEVICE_227E 4
+#define SIMATIC_IPC_DEVICE_227G 5
 
 struct simatic_ipc_platform {
 	u8	devmode;
diff --git a/include/linux/platform_data/x86/simatic-ipc.h b/include/linux/platform_data/x86/simatic-ipc.h
index f3b76b39776b..7a2e79f3be0b 100644
--- a/include/linux/platform_data/x86/simatic-ipc.h
+++ b/include/linux/platform_data/x86/simatic-ipc.h
@@ -31,6 +31,7 @@ enum simatic_ipc_station_ids {
 	SIMATIC_IPC_IPC427E = 0x00000A01,
 	SIMATIC_IPC_IPC477E = 0x00000A02,
 	SIMATIC_IPC_IPC127E = 0x00000D01,
+	SIMATIC_IPC_IPC227G = 0x00000F01,
 };
 
 static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
-- 
2.35.1

[PATCH v4 4/5] platform/x86: simatic-ipc: enable watchdog for 227G

[Henning Schild]

Just load the watchdog module, after having identified that machine.
That watchdog module does not have any autoloading support.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/platform/x86/simatic-ipc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index 1825ef21a86d..8dd686d1c9f1 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -96,6 +96,9 @@ static int register_platform_devices(u32 station_id)
 			 ipc_wdt_platform_device->name);
 	}
 
+	if (station_id == SIMATIC_IPC_IPC227G)
+		request_module("w83627hf_wdt");
+
 	if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
 	    wdtmode == SIMATIC_IPC_DEVICE_NONE) {
 		pr_warn("unsupported IPC detected, station id=%08x\n",
-- 
2.35.1

[PATCH v4 5/5] platform/x86: simatic-ipc: add new model 427G

[Henning Schild]

This adds support for the Siemens Simatic IPC427G. A board which
basically works like the 227G added in a previous patch. So all there is
to do is to add the station_id and make it take all the 227G branches.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/platform/x86/simatic-ipc.c            | 11 +++++++----
 include/linux/platform_data/x86/simatic-ipc.h |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index 8dd686d1c9f1..ca76076fc706 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -41,11 +41,12 @@ static struct {
 	{SIMATIC_IPC_IPC127E, SIMATIC_IPC_DEVICE_127E, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC227D, SIMATIC_IPC_DEVICE_227D, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC227E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_227E},
-	{SIMATIC_IPC_IPC227G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_NONE},
+	{SIMATIC_IPC_IPC227G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_227G},
 	{SIMATIC_IPC_IPC277E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_227E},
 	{SIMATIC_IPC_IPC427D, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC427E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_427E},
 	{SIMATIC_IPC_IPC477E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_427E},
+	{SIMATIC_IPC_IPC427G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_227G},
 };
 
 static int register_platform_devices(u32 station_id)
@@ -82,6 +83,11 @@ static int register_platform_devices(u32 station_id)
 			 ipc_led_platform_device->name);
 	}
 
+	if (wdtmode == SIMATIC_IPC_DEVICE_227G) {
+		request_module("w83627hf_wdt");
+		return 0;
+	}
+
 	if (wdtmode != SIMATIC_IPC_DEVICE_NONE) {
 		platform_data.devmode = wdtmode;
 		ipc_wdt_platform_device =
@@ -96,9 +102,6 @@ static int register_platform_devices(u32 station_id)
 			 ipc_wdt_platform_device->name);
 	}
 
-	if (station_id == SIMATIC_IPC_IPC227G)
-		request_module("w83627hf_wdt");
-
 	if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
 	    wdtmode == SIMATIC_IPC_DEVICE_NONE) {
 		pr_warn("unsupported IPC detected, station id=%08x\n",
diff --git a/include/linux/platform_data/x86/simatic-ipc.h b/include/linux/platform_data/x86/simatic-ipc.h
index 7a2e79f3be0b..632320ec8f08 100644
--- a/include/linux/platform_data/x86/simatic-ipc.h
+++ b/include/linux/platform_data/x86/simatic-ipc.h
@@ -32,6 +32,7 @@ enum simatic_ipc_station_ids {
 	SIMATIC_IPC_IPC477E = 0x00000A02,
 	SIMATIC_IPC_IPC127E = 0x00000D01,
 	SIMATIC_IPC_IPC227G = 0x00000F01,
+	SIMATIC_IPC_IPC427G = 0x00001001,
 };
 
 static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
-- 
2.35.1

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[Andy Shevchenko]

On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote:
> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO chips are
> very similar to the ones from Fintek. In other subsystems they also
> share drivers and are called a family of drivers.
> 
> For the GPIO subsystem the only difference is that the direction bit is
> reversed and that there is only one data bit per pin. On the SuperIO
> level the logical device is another one.
> 
> On a chip level we do not have a manufacturer ID to check and also no
> revision.

...

> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889 and F81866
> + * GPIO driver for Fintek and Nuvoton Super-I/O chips

I'm not sure it's good idea to drop it from here. It means reader has to get
this info in a hard way.

...

> +#define gpio_dir_invert(type)	((type) == nct6116d)
> +#define gpio_data_single(type)	((type) == nct6116d)

What's prevents us to add a proper prefix to these? I don't like the idea of
them having "gpio" prefix.

...

> +		pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid);
> +			pr_debug(DRVNAME ": Not a Fintek device at 0x%08x\n", addr);
> +	pr_info(DRVNAME ": Found %s at %#x\n",
> +		pr_info(DRVNAME ":   revision %d\n",

Can we, please, utilize pr_fmt()?

> +			(int)superio_inb(addr, SIO_FINTEK_DEVREV));

Explicit casting in printf() means wrong specifier in 99% of cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/5] add support for another simatic board

[Andy Shevchenko]

On Tue, Aug 23, 2022 at 12:23:39PM +0200, Henning Schild wrote:
> changes since v3:
>   - update Kconfig as well
>   - drop chip names from comment in driver header
>   - add manufacturer check for Fintek again, Nuvoton not possible
>   - drop revision printing for Nuvoton
>   - restructure defines again
>   - add new model 427G
> 
> changes since v2: (p1 only)
>   - rename macros that change behavior
>   - use chip type not device id in the macros
>   - reorder defines a bit
> 
> changes since v1:
>   - remove unused define
>   - fix bug where (base + 2) was used as second data bit
>   - add macros for "inverted" and "single data bit"

For the non-commented patches:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> This series first enables a SuperIO GPIO driver to support a chip from
> the vendor Nuvoton, the driver is for Fintek devices but those just are
> very similar. And in watchdog and hwmon subsystems these SuperIO drivers
> also share code and are sometimes called a family.
> 
> In another step the individual banks receive a label to tell them apart,
> a step which potentially changes an interface to legacy users that might
> rely on all banks having the same label, or an exact label. But since a
> later patch wants to use GPIO_LOOKUP unique labels are needed and i
> decided to assign them for all supported chips.
> 
> In a following patch the Simatic GPIO LED driver is extended to provide
> LEDs in case that SuperIO GPIO driver can be loaded.
> 
> Last but not least the watchdog module of that same SuperIO gets loaded
> on a best effort basis.
> 
> Note similar patches have appreared before as
>   "[PATCH v3 0/1] add device driver for Nuvoton SIO gpio function"
> The main difference here is that i added chip support to an existing
> driver instead of creating a new one. And that i actually propose all
> patches and do not just have the LED patch for Simatic as an example.

> Also note that the patches are based on
>   "[PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper"

No need, it's upstream (v6.0-rc1 onwards).

> Henning Schild (5):
>   gpio-f7188x: Add GPIO support for Nuvoton NCT6116
>   gpio-f7188x: use unique labels for banks/chips
>   leds: simatic-ipc-leds-gpio: add new model 227G
>   platform/x86: simatic-ipc: enable watchdog for 227G
>   platform/x86: simatic-ipc: add new model 427G
> 
>  drivers/gpio/Kconfig                          |   3 +-
>  drivers/gpio/gpio-f7188x.c                    | 229 +++++++++++-------
>  drivers/leds/simple/simatic-ipc-leds-gpio.c   |  42 +++-
>  drivers/platform/x86/simatic-ipc.c            |  10 +-
>  .../platform_data/x86/simatic-ipc-base.h      |   1 +
>  include/linux/platform_data/x86/simatic-ipc.h |   2 +
>  6 files changed, 194 insertions(+), 93 deletions(-)
> 
> -- 
> 2.35.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[Henning Schild]

Am Tue, 23 Aug 2022 17:47:38 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote:
> > Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO chips are
> > very similar to the ones from Fintek. In other subsystems they also
> > share drivers and are called a family of drivers.
> > 
> > For the GPIO subsystem the only difference is that the direction
> > bit is reversed and that there is only one data bit per pin. On the
> > SuperIO level the logical device is another one.
> > 
> > On a chip level we do not have a manufacturer ID to check and also
> > no revision.  
> 
> ...
> 
> > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> > F71889 and F81866
> > + * GPIO driver for Fintek and Nuvoton Super-I/O chips  
> 
> I'm not sure it's good idea to drop it from here. It means reader has
> to get this info in a hard way.
> 
> ...

Let us see what others say. I wanted to keep this in line with what
Kconfig says and the oneliner in the Kconfig was getting pretty
longish. Hence i decided to shorten that. Other drivers also seem to
not list all the possible chips in many places, it is all maint effort
when a new chips is added and the list is in like 5 places.

> > +#define gpio_dir_invert(type)	((type) == nct6116d)
> > +#define gpio_data_single(type)	((type) == nct6116d)  
> 
> What's prevents us to add a proper prefix to these? I don't like the
> idea of them having "gpio" prefix.
> 
> ...
> 
> > +		pr_info(DRVNAME ": Unsupported device 0x%04x\n",
> > devid);
> > +			pr_debug(DRVNAME ": Not a Fintek device at
> > 0x%08x\n", addr);
> > +	pr_info(DRVNAME ": Found %s at %#x\n",
> > +		pr_info(DRVNAME ":   revision %d\n",  
> 
> Can we, please, utilize pr_fmt()?
> 
> > +			(int)superio_inb(addr,
> > SIO_FINTEK_DEVREV));  
> 
> Explicit casting in printf() means wrong specifier in 99% of cases.
> 

For all the other comments i will wait for a second opinion. I
specifically did not change existing code for more than the functional
changes needed. And a bit of checkpatch.pl fixing.
Beautification could be done on the way but would only cause
inconsistency. That driver is what it is, if someone wants to overhaul
the style ... that should be another patch. One likely not coming from
me.

Henning

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[simon.guinot]

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

On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote:
> Am Tue, 23 Aug 2022 17:47:38 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

Hi Andy,

Thanks for this new version. It is looking good to me.

> 
> > On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote:
> > > Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO chips are
> > > very similar to the ones from Fintek. In other subsystems they also
> > > share drivers and are called a family of drivers.
> > > 
> > > For the GPIO subsystem the only difference is that the direction
> > > bit is reversed and that there is only one data bit per pin. On the
> > > SuperIO level the logical device is another one.
> > > 
> > > On a chip level we do not have a manufacturer ID to check and also
> > > no revision.  
> > 
> > ...
> > 
> > > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> > > F71889 and F81866
> > > + * GPIO driver for Fintek and Nuvoton Super-I/O chips  
> > 
> > I'm not sure it's good idea to drop it from here. It means reader has
> > to get this info in a hard way.
> > 
> > ...
> 
> Let us see what others say. I wanted to keep this in line with what
> Kconfig says and the oneliner in the Kconfig was getting pretty
> longish. Hence i decided to shorten that. Other drivers also seem to
> not list all the possible chips in many places, it is all maint effort
> when a new chips is added and the list is in like 5 places.

I agree with you that we can drop this line. It was already incomplete
and the information is quite readable a few lines below in both the
define list and the chip enumeration.

> 
> > > +#define gpio_dir_invert(type)	((type) == nct6116d)
> > > +#define gpio_data_single(type)	((type) == nct6116d)  
> > 
> > What's prevents us to add a proper prefix to these? I don't like the
> > idea of them having "gpio" prefix.
> > 
> > ...
> > 
> > > +		pr_info(DRVNAME ": Unsupported device 0x%04x\n",
> > > devid);
> > > +			pr_debug(DRVNAME ": Not a Fintek device at
> > > 0x%08x\n", addr);
> > > +	pr_info(DRVNAME ": Found %s at %#x\n",
> > > +		pr_info(DRVNAME ":   revision %d\n",  
> > 
> > Can we, please, utilize pr_fmt()?
> > 
> > > +			(int)superio_inb(addr,
> > > SIO_FINTEK_DEVREV));  
> > 
> > Explicit casting in printf() means wrong specifier in 99% of cases.
> > 
> 
> For all the other comments i will wait for a second opinion. I
> specifically did not change existing code for more than the functional
> changes needed. And a bit of checkpatch.pl fixing.
> Beautification could be done on the way but would only cause
> inconsistency. That driver is what it is, if someone wants to overhaul
> the style ... that should be another patch. One likely not coming from
> me.

About the int cast, I think you can drop it while you are updating
this line. It is unneeded.

I have no opinion on the other comments and I am OK with the rest of the
patch.

Simon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[Henning Schild]

Am Wed, 24 Aug 2022 15:10:55 +0200
schrieb simon.guinot@sequanux.org:

> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote:
> > Am Tue, 23 Aug 2022 17:47:38 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:  
> 
> Hi Andy,
> 
> Thanks for this new version. It is looking good to me.
> 
> >   
> > > On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote:  
> > > > Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO
> > > > chips are very similar to the ones from Fintek. In other
> > > > subsystems they also share drivers and are called a family of
> > > > drivers.
> > > > 
> > > > For the GPIO subsystem the only difference is that the direction
> > > > bit is reversed and that there is only one data bit per pin. On
> > > > the SuperIO level the logical device is another one.
> > > > 
> > > > On a chip level we do not have a manufacturer ID to check and
> > > > also no revision.    
> > > 
> > > ...
> > >   
> > > > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> > > > F71889 and F81866
> > > > + * GPIO driver for Fintek and Nuvoton Super-I/O chips    
> > > 
> > > I'm not sure it's good idea to drop it from here. It means reader
> > > has to get this info in a hard way.
> > > 
> > > ...  
> > 
> > Let us see what others say. I wanted to keep this in line with what
> > Kconfig says and the oneliner in the Kconfig was getting pretty
> > longish. Hence i decided to shorten that. Other drivers also seem to
> > not list all the possible chips in many places, it is all maint
> > effort when a new chips is added and the list is in like 5 places.  
> 
> I agree with you that we can drop this line. It was already incomplete
> and the information is quite readable a few lines below in both the
> define list and the chip enumeration.
> 
> >   
> > > > +#define gpio_dir_invert(type)	((type) == nct6116d)
> > > > +#define gpio_data_single(type)	((type) == nct6116d)    
> > > 
> > > What's prevents us to add a proper prefix to these? I don't like
> > > the idea of them having "gpio" prefix.
> > > 
> > > ...
> > >   
> > > > +		pr_info(DRVNAME ": Unsupported device
> > > > 0x%04x\n", devid);
> > > > +			pr_debug(DRVNAME ": Not a Fintek
> > > > device at 0x%08x\n", addr);
> > > > +	pr_info(DRVNAME ": Found %s at %#x\n",
> > > > +		pr_info(DRVNAME ":   revision %d\n",    
> > > 
> > > Can we, please, utilize pr_fmt()?
> > >   
> > > > +			(int)superio_inb(addr,
> > > > SIO_FINTEK_DEVREV));    
> > > 
> > > Explicit casting in printf() means wrong specifier in 99% of
> > > cases. 
> > 
> > For all the other comments i will wait for a second opinion. I
> > specifically did not change existing code for more than the
> > functional changes needed. And a bit of checkpatch.pl fixing.
> > Beautification could be done on the way but would only cause
> > inconsistency. That driver is what it is, if someone wants to
> > overhaul the style ... that should be another patch. One likely not
> > coming from me.  
> 
> About the int cast, I think you can drop it while you are updating
> this line. It is unneeded.

Ok two voices for doing that one fix along the way. I will send a v5
and hope nobody insists on me fixing the other findings in code i never
wrote.

regards,
Henning

> I have no opinion on the other comments and I am OK with the rest of
> the patch.
> 
> Simon

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[Hans de Goede]

Hi Henning,

On 8/24/22 15:50, Henning Schild wrote:
> Am Wed, 24 Aug 2022 15:10:55 +0200
> schrieb simon.guinot@sequanux.org:
> 
>> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote:
>>> Am Tue, 23 Aug 2022 17:47:38 +0300
>>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:  
>>
>> Hi Andy,
>>
>> Thanks for this new version. It is looking good to me.
>>
>>>   
>>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote:  
>>>>> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO
>>>>> chips are very similar to the ones from Fintek. In other
>>>>> subsystems they also share drivers and are called a family of
>>>>> drivers.
>>>>>
>>>>> For the GPIO subsystem the only difference is that the direction
>>>>> bit is reversed and that there is only one data bit per pin. On
>>>>> the SuperIO level the logical device is another one.
>>>>>
>>>>> On a chip level we do not have a manufacturer ID to check and
>>>>> also no revision.    
>>>>
>>>> ...
>>>>   
>>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
>>>>> F71889 and F81866
>>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips    
>>>>
>>>> I'm not sure it's good idea to drop it from here. It means reader
>>>> has to get this info in a hard way.
>>>>
>>>> ...  
>>>
>>> Let us see what others say. I wanted to keep this in line with what
>>> Kconfig says and the oneliner in the Kconfig was getting pretty
>>> longish. Hence i decided to shorten that. Other drivers also seem to
>>> not list all the possible chips in many places, it is all maint
>>> effort when a new chips is added and the list is in like 5 places.  
>>
>> I agree with you that we can drop this line. It was already incomplete
>> and the information is quite readable a few lines below in both the
>> define list and the chip enumeration.
>>
>>>   
>>>>> +#define gpio_dir_invert(type)	((type) == nct6116d)
>>>>> +#define gpio_data_single(type)	((type) == nct6116d)    
>>>>
>>>> What's prevents us to add a proper prefix to these? I don't like
>>>> the idea of them having "gpio" prefix.
>>>>
>>>> ...
>>>>   
>>>>> +		pr_info(DRVNAME ": Unsupported device
>>>>> 0x%04x\n", devid);
>>>>> +			pr_debug(DRVNAME ": Not a Fintek
>>>>> device at 0x%08x\n", addr);
>>>>> +	pr_info(DRVNAME ": Found %s at %#x\n",
>>>>> +		pr_info(DRVNAME ":   revision %d\n",    
>>>>
>>>> Can we, please, utilize pr_fmt()?
>>>>   
>>>>> +			(int)superio_inb(addr,
>>>>> SIO_FINTEK_DEVREV));    
>>>>
>>>> Explicit casting in printf() means wrong specifier in 99% of
>>>> cases. 
>>>
>>> For all the other comments i will wait for a second opinion. I
>>> specifically did not change existing code for more than the
>>> functional changes needed. And a bit of checkpatch.pl fixing.
>>> Beautification could be done on the way but would only cause
>>> inconsistency. That driver is what it is, if someone wants to
>>> overhaul the style ... that should be another patch. One likely not
>>> coming from me.  
>>
>> About the int cast, I think you can drop it while you are updating
>> this line. It is unneeded.
> 
> Ok two voices for doing that one fix along the way. I will send a v5
> and hope nobody insists on me fixing the other findings in code i never
> wrote.

You did not write it, but you are using it to do hw-enablement for
your company's products. So being asked to also some touch-ups
left and right while you are at it really is not unexpected IMHO.

Regards,

Hans

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[Henning Schild]

Am Wed, 24 Aug 2022 15:54:28 +0200
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi Henning,
> 
> On 8/24/22 15:50, Henning Schild wrote:
> > Am Wed, 24 Aug 2022 15:10:55 +0200
> > schrieb simon.guinot@sequanux.org:
> >   
> >> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote:  
> >>> Am Tue, 23 Aug 2022 17:47:38 +0300
> >>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:    
> >>
> >> Hi Andy,
> >>
> >> Thanks for this new version. It is looking good to me.
> >>  
> >>>     
> >>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote:
> >>>>   
> >>>>> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO
> >>>>> chips are very similar to the ones from Fintek. In other
> >>>>> subsystems they also share drivers and are called a family of
> >>>>> drivers.
> >>>>>
> >>>>> For the GPIO subsystem the only difference is that the direction
> >>>>> bit is reversed and that there is only one data bit per pin. On
> >>>>> the SuperIO level the logical device is another one.
> >>>>>
> >>>>> On a chip level we do not have a manufacturer ID to check and
> >>>>> also no revision.      
> >>>>
> >>>> ...
> >>>>     
> >>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> >>>>> F71889 and F81866
> >>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips      
> >>>>
> >>>> I'm not sure it's good idea to drop it from here. It means reader
> >>>> has to get this info in a hard way.
> >>>>
> >>>> ...    
> >>>
> >>> Let us see what others say. I wanted to keep this in line with
> >>> what Kconfig says and the oneliner in the Kconfig was getting
> >>> pretty longish. Hence i decided to shorten that. Other drivers
> >>> also seem to not list all the possible chips in many places, it
> >>> is all maint effort when a new chips is added and the list is in
> >>> like 5 places.    
> >>
> >> I agree with you that we can drop this line. It was already
> >> incomplete and the information is quite readable a few lines below
> >> in both the define list and the chip enumeration.
> >>  
> >>>     
> >>>>> +#define gpio_dir_invert(type)	((type) == nct6116d)
> >>>>> +#define gpio_data_single(type)	((type) == nct6116d)
> >>>>>  
> >>>>
> >>>> What's prevents us to add a proper prefix to these? I don't like
> >>>> the idea of them having "gpio" prefix.
> >>>>
> >>>> ...
> >>>>     
> >>>>> +		pr_info(DRVNAME ": Unsupported device
> >>>>> 0x%04x\n", devid);
> >>>>> +			pr_debug(DRVNAME ": Not a Fintek
> >>>>> device at 0x%08x\n", addr);
> >>>>> +	pr_info(DRVNAME ": Found %s at %#x\n",
> >>>>> +		pr_info(DRVNAME ":   revision %d\n",      
> >>>>
> >>>> Can we, please, utilize pr_fmt()?
> >>>>     
> >>>>> +			(int)superio_inb(addr,
> >>>>> SIO_FINTEK_DEVREV));      
> >>>>
> >>>> Explicit casting in printf() means wrong specifier in 99% of
> >>>> cases.   
> >>>
> >>> For all the other comments i will wait for a second opinion. I
> >>> specifically did not change existing code for more than the
> >>> functional changes needed. And a bit of checkpatch.pl fixing.
> >>> Beautification could be done on the way but would only cause
> >>> inconsistency. That driver is what it is, if someone wants to
> >>> overhaul the style ... that should be another patch. One likely
> >>> not coming from me.    
> >>
> >> About the int cast, I think you can drop it while you are updating
> >> this line. It is unneeded.  
> > 
> > Ok two voices for doing that one fix along the way. I will send a v5
> > and hope nobody insists on me fixing the other findings in code i
> > never wrote.  
> 
> You did not write it, but you are using it to do hw-enablement for
> your company's products. So being asked to also some touch-ups
> left and right while you are at it really is not unexpected IMHO.

Sure thing. Dropping a few characters from a line i touch anyhow is
easy enough. But i.e a refactoring to pr_fmt would feel like asking too
much in my book. That feels like work of the author or maintainer.

In fact i am just doing the homework of what i think should have long
been done by Nuvoton.

I hope that v5 will be acceptable.

Henning 

> Regards,
> 
> Hans
> 

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[Hans de Goede]

Hi,

On 8/24/22 16:17, Henning Schild wrote:
> Am Wed, 24 Aug 2022 15:54:28 +0200
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Hi Henning,
>>
>> On 8/24/22 15:50, Henning Schild wrote:
>>> Am Wed, 24 Aug 2022 15:10:55 +0200
>>> schrieb simon.guinot@sequanux.org:
>>>   
>>>> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote:  
>>>>> Am Tue, 23 Aug 2022 17:47:38 +0300
>>>>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:    
>>>>
>>>> Hi Andy,
>>>>
>>>> Thanks for this new version. It is looking good to me.
>>>>  
>>>>>     
>>>>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote:
>>>>>>   
>>>>>>> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO
>>>>>>> chips are very similar to the ones from Fintek. In other
>>>>>>> subsystems they also share drivers and are called a family of
>>>>>>> drivers.
>>>>>>>
>>>>>>> For the GPIO subsystem the only difference is that the direction
>>>>>>> bit is reversed and that there is only one data bit per pin. On
>>>>>>> the SuperIO level the logical device is another one.
>>>>>>>
>>>>>>> On a chip level we do not have a manufacturer ID to check and
>>>>>>> also no revision.      
>>>>>>
>>>>>> ...
>>>>>>     
>>>>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
>>>>>>> F71889 and F81866
>>>>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips      
>>>>>>
>>>>>> I'm not sure it's good idea to drop it from here. It means reader
>>>>>> has to get this info in a hard way.
>>>>>>
>>>>>> ...    
>>>>>
>>>>> Let us see what others say. I wanted to keep this in line with
>>>>> what Kconfig says and the oneliner in the Kconfig was getting
>>>>> pretty longish. Hence i decided to shorten that. Other drivers
>>>>> also seem to not list all the possible chips in many places, it
>>>>> is all maint effort when a new chips is added and the list is in
>>>>> like 5 places.    
>>>>
>>>> I agree with you that we can drop this line. It was already
>>>> incomplete and the information is quite readable a few lines below
>>>> in both the define list and the chip enumeration.
>>>>  
>>>>>     
>>>>>>> +#define gpio_dir_invert(type)	((type) == nct6116d)
>>>>>>> +#define gpio_data_single(type)	((type) == nct6116d)
>>>>>>>  
>>>>>>
>>>>>> What's prevents us to add a proper prefix to these? I don't like
>>>>>> the idea of them having "gpio" prefix.
>>>>>>
>>>>>> ...
>>>>>>     
>>>>>>> +		pr_info(DRVNAME ": Unsupported device
>>>>>>> 0x%04x\n", devid);
>>>>>>> +			pr_debug(DRVNAME ": Not a Fintek
>>>>>>> device at 0x%08x\n", addr);
>>>>>>> +	pr_info(DRVNAME ": Found %s at %#x\n",
>>>>>>> +		pr_info(DRVNAME ":   revision %d\n",      
>>>>>>
>>>>>> Can we, please, utilize pr_fmt()?
>>>>>>     
>>>>>>> +			(int)superio_inb(addr,
>>>>>>> SIO_FINTEK_DEVREV));      
>>>>>>
>>>>>> Explicit casting in printf() means wrong specifier in 99% of
>>>>>> cases.   
>>>>>
>>>>> For all the other comments i will wait for a second opinion. I
>>>>> specifically did not change existing code for more than the
>>>>> functional changes needed. And a bit of checkpatch.pl fixing.
>>>>> Beautification could be done on the way but would only cause
>>>>> inconsistency. That driver is what it is, if someone wants to
>>>>> overhaul the style ... that should be another patch. One likely
>>>>> not coming from me.    
>>>>
>>>> About the int cast, I think you can drop it while you are updating
>>>> this line. It is unneeded.  
>>>
>>> Ok two voices for doing that one fix along the way. I will send a v5
>>> and hope nobody insists on me fixing the other findings in code i
>>> never wrote.  
>>
>> You did not write it, but you are using it to do hw-enablement for
>> your company's products. So being asked to also some touch-ups
>> left and right while you are at it really is not unexpected IMHO.
> 
> Sure thing. Dropping a few characters from a line i touch anyhow is
> easy enough. But i.e a refactoring to pr_fmt would feel like asking too
> much in my book. That feels like work of the author or maintainer.

Right, but that assumes that the original author / maintainer is still
around and actively contributing which unfortunately is not always
the case.

Regards,

Hans

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[simon.guinot]

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

On Wed, Aug 24, 2022 at 04:24:46PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 8/24/22 16:17, Henning Schild wrote:
> > Am Wed, 24 Aug 2022 15:54:28 +0200
> > schrieb Hans de Goede <hdegoede@redhat.com>:
> > 
> >> Hi Henning,
> >>
> >> On 8/24/22 15:50, Henning Schild wrote:
> >>> Am Wed, 24 Aug 2022 15:10:55 +0200
> >>> schrieb simon.guinot@sequanux.org:
> >>>   
> >>>> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote:  
> >>>>> Am Tue, 23 Aug 2022 17:47:38 +0300
> >>>>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:    
> >>>>
> >>>> Hi Andy,
> >>>>
> >>>> Thanks for this new version. It is looking good to me.
> >>>>  
> >>>>>     
> >>>>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote:
> >>>>>>   
> >>>>>>> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO
> >>>>>>> chips are very similar to the ones from Fintek. In other
> >>>>>>> subsystems they also share drivers and are called a family of
> >>>>>>> drivers.
> >>>>>>>
> >>>>>>> For the GPIO subsystem the only difference is that the direction
> >>>>>>> bit is reversed and that there is only one data bit per pin. On
> >>>>>>> the SuperIO level the logical device is another one.
> >>>>>>>
> >>>>>>> On a chip level we do not have a manufacturer ID to check and
> >>>>>>> also no revision.      
> >>>>>>
> >>>>>> ...
> >>>>>>     
> >>>>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> >>>>>>> F71889 and F81866
> >>>>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips      
> >>>>>>
> >>>>>> I'm not sure it's good idea to drop it from here. It means reader
> >>>>>> has to get this info in a hard way.
> >>>>>>
> >>>>>> ...    
> >>>>>
> >>>>> Let us see what others say. I wanted to keep this in line with
> >>>>> what Kconfig says and the oneliner in the Kconfig was getting
> >>>>> pretty longish. Hence i decided to shorten that. Other drivers
> >>>>> also seem to not list all the possible chips in many places, it
> >>>>> is all maint effort when a new chips is added and the list is in
> >>>>> like 5 places.    
> >>>>
> >>>> I agree with you that we can drop this line. It was already
> >>>> incomplete and the information is quite readable a few lines below
> >>>> in both the define list and the chip enumeration.
> >>>>  
> >>>>>     
> >>>>>>> +#define gpio_dir_invert(type)	((type) == nct6116d)
> >>>>>>> +#define gpio_data_single(type)	((type) == nct6116d)
> >>>>>>>  
> >>>>>>
> >>>>>> What's prevents us to add a proper prefix to these? I don't like
> >>>>>> the idea of them having "gpio" prefix.
> >>>>>>
> >>>>>> ...
> >>>>>>     
> >>>>>>> +		pr_info(DRVNAME ": Unsupported device
> >>>>>>> 0x%04x\n", devid);
> >>>>>>> +			pr_debug(DRVNAME ": Not a Fintek
> >>>>>>> device at 0x%08x\n", addr);
> >>>>>>> +	pr_info(DRVNAME ": Found %s at %#x\n",
> >>>>>>> +		pr_info(DRVNAME ":   revision %d\n",      
> >>>>>>
> >>>>>> Can we, please, utilize pr_fmt()?
> >>>>>>     
> >>>>>>> +			(int)superio_inb(addr,
> >>>>>>> SIO_FINTEK_DEVREV));      
> >>>>>>
> >>>>>> Explicit casting in printf() means wrong specifier in 99% of
> >>>>>> cases.   
> >>>>>
> >>>>> For all the other comments i will wait for a second opinion. I
> >>>>> specifically did not change existing code for more than the
> >>>>> functional changes needed. And a bit of checkpatch.pl fixing.
> >>>>> Beautification could be done on the way but would only cause
> >>>>> inconsistency. That driver is what it is, if someone wants to
> >>>>> overhaul the style ... that should be another patch. One likely
> >>>>> not coming from me.    
> >>>>
> >>>> About the int cast, I think you can drop it while you are updating
> >>>> this line. It is unneeded.  
> >>>
> >>> Ok two voices for doing that one fix along the way. I will send a v5
> >>> and hope nobody insists on me fixing the other findings in code i
> >>> never wrote.  
> >>
> >> You did not write it, but you are using it to do hw-enablement for
> >> your company's products. So being asked to also some touch-ups
> >> left and right while you are at it really is not unexpected IMHO.
> > 
> > Sure thing. Dropping a few characters from a line i touch anyhow is
> > easy enough. But i.e a refactoring to pr_fmt would feel like asking too
> > much in my book. That feels like work of the author or maintainer.
> 
> Right, but that assumes that the original author / maintainer is still
> around and actively contributing which unfortunately is not always
> the case.

Actually the original author is not active but he is still keeping an
eye on the driver :)

I still review and test the patches I catch on the MLs. And I am ready
to do some maintenance work if needed.

Henning, I think you could have done the pr_fmt conversion. It is not
a big deal and it would have been nice. But indeed, you don't have to...

Simon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

[Linus Walleij]

On Wed, Aug 24, 2022 at 4:18 PM Henning Schild
<henning.schild@siemens.com> wrote:

> > You did not write it, but you are using it to do hw-enablement for
> > your company's products. So being asked to also some touch-ups
> > left and right while you are at it really is not unexpected IMHO.
>
> Sure thing. Dropping a few characters from a line i touch anyhow is
> easy enough. But i.e a refactoring to pr_fmt would feel like asking too
> much in my book. That feels like work of the author or maintainer.
>
> In fact i am just doing the homework of what i think should have long
> been done by Nuvoton.

A lot of vendors don't have much active upstream participation, they
outsource that work to people like yourself by just ignoring it.

> I hope that v5 will be acceptable.

Bartosz is applying GPIO patches now, but my principle was that
when I feel a patch makes the kernel look better after than before the
patch and no new version is coming, I just apply the patch.
This is how we deal with "perfect is the enemy of good" in practice.
That said, we are all grateful for any improvements you manage to
sneak in!

Yours,
Linus Walleij