[PATCH v3 0/4] add support for another simatic board

[PATCH v3 0/4] add support for another simatic board

[Henning Schild]

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 (4):
  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

 drivers/gpio/gpio-f7188x.c                    | 193 ++++++++++--------
 drivers/leds/simple/simatic-ipc-leds-gpio.c   |  42 +++-
 drivers/platform/x86/simatic-ipc.c            |   7 +-
 .../platform_data/x86/simatic-ipc-base.h      |   1 +
 include/linux/platform_data/x86/simatic-ipc.h |   1 +
 5 files changed, 157 insertions(+), 87 deletions(-)

-- 
2.35.1

[PATCH v3 1/4] 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.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/gpio/gpio-f7188x.c | 71 +++++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index 18a3147f5a42..7b05ecc611e9 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889 and F81866
+ * and Nuvoton Super-I/O NCT6116D
  *
  * Copyright (C) 2010-2013 LaCie
  *
@@ -22,13 +23,12 @@
 #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 */
+#define SIO_LD_GPIO_FINTEK	0x06	/* GPIO logical device */
+#define SIO_LD_GPIO_NUVOTON	0x07	/* GPIO logical device */
 #define SIO_F71869_ID		0x0814	/* F71869 chipset ID */
 #define SIO_F71869A_ID		0x1007	/* F71869A chipset ID */
 #define SIO_F71882_ID		0x0541	/* F71882 chipset ID */
@@ -37,7 +37,7 @@
 #define SIO_F81866_ID		0x1010	/* F81866 chipset ID */
 #define SIO_F81804_ID		0x1502  /* F81804 chipset ID, same for f81966 */
 #define SIO_F81865_ID		0x0704	/* F81865 chipset ID */
-
+#define SIO_NCT6116D_ID		0xD283  /* NCT6116D chipset ID */
 
 enum chips {
 	f71869,
@@ -48,6 +48,7 @@ enum chips {
 	f81866,
 	f81804,
 	f81865,
+	nct6116d,
 };
 
 static const char * const f7188x_names[] = {
@@ -59,10 +60,12 @@ static const char * const f7188x_names[] = {
 	"f81866",
 	"f81804",
 	"f81865",
+	"nct6116d",
 };
 
 struct f7188x_sio {
 	int addr;
+	int device;
 	enum chips type;
 };
 
@@ -170,6 +173,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 +260,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 +281,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 +306,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 +331,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 +356,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 +366,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 +387,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 +415,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 +476,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;
 	}
@@ -485,12 +516,8 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio)
 		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,8 +544,12 @@ 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;
 	}
 	sio->addr = addr;
-- 
2.35.1

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

[simon.guinot]

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

On Thu, Aug 11, 2022 at 05:39:05PM +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.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/gpio/gpio-f7188x.c | 71 +++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> index 18a3147f5a42..7b05ecc611e9 100644
> --- a/drivers/gpio/gpio-f7188x.c
> +++ b/drivers/gpio/gpio-f7188x.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889 and F81866
> + * and Nuvoton Super-I/O NCT6116D
>   *
>   * Copyright (C) 2010-2013 LaCie
>   *
> @@ -22,13 +23,12 @@
>  #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 */
> +#define SIO_LD_GPIO_FINTEK	0x06	/* GPIO logical device */
> +#define SIO_LD_GPIO_NUVOTON	0x07	/* GPIO logical device */

Please indulge me and add a new line here.

>  #define SIO_F71869_ID		0x0814	/* F71869 chipset ID */
>  #define SIO_F71869A_ID		0x1007	/* F71869A chipset ID */
>  #define SIO_F71882_ID		0x0541	/* F71882 chipset ID */
> @@ -37,7 +37,7 @@
>  #define SIO_F81866_ID		0x1010	/* F81866 chipset ID */
>  #define SIO_F81804_ID		0x1502  /* F81804 chipset ID, same for f81966 */
>  #define SIO_F81865_ID		0x0704	/* F81865 chipset ID */
> -
> +#define SIO_NCT6116D_ID		0xD283  /* NCT6116D chipset ID */
>  

... snip ...

> @@ -485,12 +516,8 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio)
>  		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;
> -	}

Sorry for missing that at my first review. You can't remove this block
of code. This driver is poking around on the I2C bus, which is not
great. So we want to make sure as much as possible that we are speaking
to the right device.

Simon

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

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

[Henning Schild]

Am Fri, 12 Aug 2022 10:43:03 +0200
schrieb simon.guinot@sequanux.org:

> On Thu, Aug 11, 2022 at 05:39:05PM +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.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/gpio/gpio-f7188x.c | 71
> > +++++++++++++++++++++++++++----------- 1 file changed, 51
> > insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> > index 18a3147f5a42..7b05ecc611e9 100644
> > --- a/drivers/gpio/gpio-f7188x.c
> > +++ b/drivers/gpio/gpio-f7188x.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> > F71889 and F81866
> > + * and Nuvoton Super-I/O NCT6116D
> >   *
> >   * Copyright (C) 2010-2013 LaCie
> >   *
> > @@ -22,13 +23,12 @@
> >  #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 */ +#define SIO_LD_GPIO_FINTEK	0x06	/* GPIO
> > logical device */ +#define SIO_LD_GPIO_NUVOTON	0x07
> > /* GPIO logical device */  
> 
> Please indulge me and add a new line here.

Mhh ... how about you write exactly how you would like to have that
define block. So we do not have taste issues in the next round.

> >  #define SIO_F71869_ID		0x0814	/* F71869
> > chipset ID */ #define SIO_F71869A_ID		0x1007
> > /* F71869A chipset ID */ #define SIO_F71882_ID
> > 0x0541	/* F71882 chipset ID */ @@ -37,7 +37,7 @@
> >  #define SIO_F81866_ID		0x1010	/* F81866
> > chipset ID */ #define SIO_F81804_ID		0x1502  /*
> > F81804 chipset ID, same for f81966 */ #define SIO_F81865_ID
> > 	0x0704	/* F81865 chipset ID */ -
> > +#define SIO_NCT6116D_ID		0xD283  /* NCT6116D chipset
> > ID */ 
> 
> ... snip ...
> 
> > @@ -485,12 +516,8 @@ static int __init f7188x_find(int addr, struct
> > f7188x_sio *sio) 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;
> > -	}  
> 
> Sorry for missing that at my first review. You can't remove this block
> of code. This driver is poking around on the I2C bus, which is not
> great. So we want to make sure as much as possible that we are
> speaking to the right device.

Ok fair enough, we can make that more conservative and match the two
manufacturers and also make sure that not one can bring a chip id that
the other one uses for another chip.
A v4 is coming earliest in 1.5 weeks.

Henning

> Simon

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

[simon.guinot]

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

On Fri, Aug 12, 2022 at 12:23:12PM +0200, Henning Schild wrote:
> Am Fri, 12 Aug 2022 10:43:03 +0200
> schrieb simon.guinot@sequanux.org:
> 
> > On Thu, Aug 11, 2022 at 05:39:05PM +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.
> > > 
> > > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > > ---
> > >  drivers/gpio/gpio-f7188x.c | 71
> > > +++++++++++++++++++++++++++----------- 1 file changed, 51
> > > insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> > > index 18a3147f5a42..7b05ecc611e9 100644
> > > --- a/drivers/gpio/gpio-f7188x.c
> > > +++ b/drivers/gpio/gpio-f7188x.c
> > > @@ -1,6 +1,7 @@
> > >  // SPDX-License-Identifier: GPL-2.0-or-later
> > >  /*
> > >   * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> > > F71889 and F81866
> > > + * and Nuvoton Super-I/O NCT6116D
> > >   *
> > >   * Copyright (C) 2010-2013 LaCie
> > >   *
> > > @@ -22,13 +23,12 @@
> > >  #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 */ +#define SIO_LD_GPIO_FINTEK	0x06	/* GPIO
> > > logical device */ +#define SIO_LD_GPIO_NUVOTON	0x07
> > > /* GPIO logical device */  
> > 
> > Please indulge me and add a new line here.
> 
> Mhh ... how about you write exactly how you would like to have that
> define block. So we do not have taste issues in the next round.

Sure. Considering the manufacturer IDs you have to keep and add, I would
go with your original approach (i.e. a section per manufacturer). But
I would add extra new lines and comments and use a sligthly different
namming for the LD_GPIO definitions.

/*
 * Super-I/O registers
 */
#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_UNLOCK_KEY          0x87    /* Key to enable Super-I/O */
#define SIO_LOCK_KEY            0xAA    /* Key to disable Super-I/O */

/*
 * Fintek devices.
 */
#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_F81865_ID           0x0704  /* F81865 chipset ID */

#define SIO_FINTEK_LD_GPIO      0x06    /* GPIO logical device */

/*
 * Nuvoton devices.
 */
#define SIO_NUVOTON_ID          0xXXXX  /* Manufacturer ID */

#define SIO_NCT6116D_ID         0xD283  /* NCT6116D chipset ID */

#define SIO_NUVOTON_LD_GPIO     0x07    /* GPIO logical device */

Please, note it is not only a matter of taste. New lines and comments
are really helping the reader. Also, note that I am not asking for this
change, only suggesting it.

> 
> > >  #define SIO_F71869_ID		0x0814	/* F71869
> > > chipset ID */ #define SIO_F71869A_ID		0x1007
> > > /* F71869A chipset ID */ #define SIO_F71882_ID
> > > 0x0541	/* F71882 chipset ID */ @@ -37,7 +37,7 @@
> > >  #define SIO_F81866_ID		0x1010	/* F81866
> > > chipset ID */ #define SIO_F81804_ID		0x1502  /*
> > > F81804 chipset ID, same for f81966 */ #define SIO_F81865_ID
> > > 	0x0704	/* F81865 chipset ID */ -
> > > +#define SIO_NCT6116D_ID		0xD283  /* NCT6116D chipset
> > > ID */ 
> > 
> > ... snip ...
> > 
> > > @@ -485,12 +516,8 @@ static int __init f7188x_find(int addr, struct
> > > f7188x_sio *sio) 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;
> > > -	}  
> > 
> > Sorry for missing that at my first review. You can't remove this block
> > of code. This driver is poking around on the I2C bus, which is not
> > great. So we want to make sure as much as possible that we are
> > speaking to the right device.
> 
> Ok fair enough, we can make that more conservative and match the two
> manufacturers and also make sure that not one can bring a chip id that
> the other one uses for another chip.
> A v4 is coming earliest in 1.5 weeks.

Great. Thanks for your patience.

Simon

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

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

[Henning Schild]

Am Fri, 12 Aug 2022 13:22:04 +0200
schrieb simon.guinot@sequanux.org:

> On Fri, Aug 12, 2022 at 12:23:12PM +0200, Henning Schild wrote:
> > Am Fri, 12 Aug 2022 10:43:03 +0200
> > schrieb simon.guinot@sequanux.org:
> >   
> > > On Thu, Aug 11, 2022 at 05:39:05PM +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.
> > > > 
> > > > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > > > ---
> > > >  drivers/gpio/gpio-f7188x.c | 71
> > > > +++++++++++++++++++++++++++----------- 1 file changed, 51
> > > > insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpio/gpio-f7188x.c
> > > > b/drivers/gpio/gpio-f7188x.c index 18a3147f5a42..7b05ecc611e9
> > > > 100644 --- a/drivers/gpio/gpio-f7188x.c
> > > > +++ b/drivers/gpio/gpio-f7188x.c
> > > > @@ -1,6 +1,7 @@
> > > >  // SPDX-License-Identifier: GPL-2.0-or-later
> > > >  /*
> > > >   * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> > > > F71889 and F81866
> > > > + * and Nuvoton Super-I/O NCT6116D
> > > >   *
> > > >   * Copyright (C) 2010-2013 LaCie
> > > >   *
> > > > @@ -22,13 +23,12 @@
> > > >  #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 */ +#define SIO_LD_GPIO_FINTEK
> > > > 0x06	/* GPIO logical device */ +#define
> > > > SIO_LD_GPIO_NUVOTON	0x07 /* GPIO logical device */    
> > > 
> > > Please indulge me and add a new line here.  
> > 
> > Mhh ... how about you write exactly how you would like to have that
> > define block. So we do not have taste issues in the next round.  
> 
> Sure. Considering the manufacturer IDs you have to keep and add, I
> would go with your original approach (i.e. a section per
> manufacturer). But I would add extra new lines and comments and use a
> sligthly different namming for the LD_GPIO definitions.
> 
> /*
>  * Super-I/O registers
>  */
> #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_UNLOCK_KEY          0x87    /* Key to enable Super-I/O */
> #define SIO_LOCK_KEY            0xAA    /* Key to disable Super-I/O */
> 
> /*
>  * Fintek devices.
>  */
> #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_F81865_ID           0x0704  /* F81865 chipset ID */
> 
> #define SIO_FINTEK_LD_GPIO      0x06    /* GPIO logical device */
> 
> /*
>  * Nuvoton devices.
>  */
> #define SIO_NUVOTON_ID          0xXXXX  /* Manufacturer ID */
> 
> #define SIO_NCT6116D_ID         0xD283  /* NCT6116D chipset ID */
> 
> #define SIO_NUVOTON_LD_GPIO     0x07    /* GPIO logical device */
> 
> Please, note it is not only a matter of taste. New lines and comments
> are really helping the reader. Also, note that I am not asking for
> this change, only suggesting it.

This is fine. I will take this. Only slight difference will be in the
revid and manid, those are Fintek specific and do not apply for Nuvoton.

regards,
Henning

> >   
> > > >  #define SIO_F71869_ID		0x0814	/* F71869
> > > > chipset ID */ #define SIO_F71869A_ID		0x1007
> > > > /* F71869A chipset ID */ #define SIO_F71882_ID
> > > > 0x0541	/* F71882 chipset ID */ @@ -37,7 +37,7 @@
> > > >  #define SIO_F81866_ID		0x1010	/* F81866
> > > > chipset ID */ #define SIO_F81804_ID		0x1502  /*
> > > > F81804 chipset ID, same for f81966 */ #define SIO_F81865_ID
> > > > 	0x0704	/* F81865 chipset ID */ -
> > > > +#define SIO_NCT6116D_ID		0xD283  /* NCT6116D
> > > > chipset ID */   
> > > 
> > > ... snip ...
> > >   
> > > > @@ -485,12 +516,8 @@ static int __init f7188x_find(int addr,
> > > > struct f7188x_sio *sio) 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;
> > > > -	}    
> > > 
> > > Sorry for missing that at my first review. You can't remove this
> > > block of code. This driver is poking around on the I2C bus, which
> > > is not great. So we want to make sure as much as possible that we
> > > are speaking to the right device.  
> > 
> > Ok fair enough, we can make that more conservative and match the two
> > manufacturers and also make sure that not one can bring a chip id
> > that the other one uses for another chip.
> > A v4 is coming earliest in 1.5 weeks.  
> 
> Great. Thanks for your patience.
> 
> Simon

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

[Henning Schild]

Am Fri, 12 Aug 2022 10:43:03 +0200
schrieb simon.guinot@sequanux.org:

> On Thu, Aug 11, 2022 at 05:39:05PM +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.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/gpio/gpio-f7188x.c | 71
> > +++++++++++++++++++++++++++----------- 1 file changed, 51
> > insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> > index 18a3147f5a42..7b05ecc611e9 100644
> > --- a/drivers/gpio/gpio-f7188x.c
> > +++ b/drivers/gpio/gpio-f7188x.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> > F71889 and F81866
> > + * and Nuvoton Super-I/O NCT6116D
> >   *
> >   * Copyright (C) 2010-2013 LaCie
> >   *
> > @@ -22,13 +23,12 @@
> >  #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 */ +#define SIO_LD_GPIO_FINTEK	0x06	/* GPIO
> > logical device */ +#define SIO_LD_GPIO_NUVOTON	0x07
> > /* GPIO logical device */  
> 
> Please indulge me and add a new line here.
> 
> >  #define SIO_F71869_ID		0x0814	/* F71869
> > chipset ID */ #define SIO_F71869A_ID		0x1007
> > /* F71869A chipset ID */ #define SIO_F71882_ID
> > 0x0541	/* F71882 chipset ID */ @@ -37,7 +37,7 @@
> >  #define SIO_F81866_ID		0x1010	/* F81866
> > chipset ID */ #define SIO_F81804_ID		0x1502  /*
> > F81804 chipset ID, same for f81966 */ #define SIO_F81865_ID
> > 	0x0704	/* F81865 chipset ID */ -
> > +#define SIO_NCT6116D_ID		0xD283  /* NCT6116D chipset
> > ID */ 
> 
> ... snip ...
> 
> > @@ -485,12 +516,8 @@ static int __init f7188x_find(int addr, struct
> > f7188x_sio *sio) 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;
> > -	}  
> 
> Sorry for missing that at my first review. You can't remove this block
> of code. This driver is poking around on the I2C bus, which is not
> great. So we want to make sure as much as possible that we are
> speaking to the right device.

Unfortunately the Nuvoton Super IOs do not have a global manufacturer
ID, just that chip ID in their global control registers.

So i think we should really just drop the checking of the manufacturer
ID all together, like proposed here. Or we could check manid+chipid for
fintek and only chipid for nuvoton (like i.e. all the wdt and hwmon
drivers for nuvoton do already).

In fact i will implement the best checking we can do, so match
manufacturer and chip where possible and drop to chip only where not.

Henning

> Simon

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

[Henning Schild]

Am Fri, 12 Aug 2022 10:37:02 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thursday, August 11, 2022, Henning Schild
> <henning.schild@siemens.com> 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.
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/gpio/gpio-f7188x.c | 71
> > +++++++++++++++++++++++++++----------- 1 file changed, 51
> > insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> > index 18a3147f5a42..7b05ecc611e9 100644
> > --- a/drivers/gpio/gpio-f7188x.c
> > +++ b/drivers/gpio/gpio-f7188x.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882,
> > F71889 and F81866
> > + * and Nuvoton Super-I/O NCT6116D
> >   *
> >   * Copyright (C) 2010-2013 LaCie
> >   *
> > @@ -22,13 +23,12 @@
> >  #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 */
> > +#define SIO_LD_GPIO_FINTEK     0x06    /* GPIO logical device */
> > +#define SIO_LD_GPIO_NUVOTON    0x07    /* GPIO logical device */
> >  #define SIO_F71869_ID          0x0814  /* F71869 chipset ID */
> >  #define SIO_F71869A_ID         0x1007  /* F71869A chipset ID */
> >  #define SIO_F71882_ID          0x0541  /* F71882 chipset ID */
> > @@ -37,7 +37,7 @@
> >  #define SIO_F81866_ID          0x1010  /* F81866 chipset ID */
> >  #define SIO_F81804_ID          0x1502  /* F81804 chipset ID, same
> > for f81966 */
> >  #define SIO_F81865_ID          0x0704  /* F81865 chipset ID */
> > -  
> 
> 
> 
> Logical split by leaving blank line here is good to have.

Already discussed those bits and changed them. There will be another
round because i will re-introduce the vendor ID check. Let us see what
that will do to those defines.

But i will give Simon the last call here.

> 
> 
> > +#define SIO_NCT6116D_ID                0xD283  /* NCT6116D chipset
> > ID */
> >
> >  enum chips {
> >         f71869,
> > @@ -48,6 +48,7 @@ enum chips {
> >         f81866,
> >         f81804,
> >         f81865,
> > +       nct6116d,
> >  };
> >
> >  static const char * const f7188x_names[] = {
> > @@ -59,10 +60,12 @@ static const char * const f7188x_names[] = {
> >         "f81866",
> >         "f81804",
> >         "f81865",
> > +       "nct6116d",
> >  };
> >
> >  struct f7188x_sio {
> >         int addr;
> > +       int device;
> >         enum chips type;
> >  };
> >
> > @@ -170,6 +173,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)  
> 
> 
> 
> I think with namespace prefix it should be fine, otherwise it might
> be a renaming burden in the future.
> 
> Also I would rather see them static inline one-liners, so compiler
> will perform a type check.
> 

Simon already acked that. The additional code is in line with other
code already in that file. So while your suggestion is valid and i also
wondered why it was defines instead of inline functions, i will leave
it like that for consistency reasons.

Henning

> >  static struct f7188x_gpio_bank f71869_gpio_bank[] = {
> >         F7188X_GPIO_BANK(0, 6, 0xF0),
> >         F7188X_GPIO_BANK(10, 8, 0xE0),
> > @@ -254,6 +260,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 +281,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 +306,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 +331,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 +356,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 +366,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 +387,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 +415,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 +476,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;
> >         }
> > @@ -485,12 +516,8 @@ static int __init f7188x_find(int addr, struct
> > f7188x_sio *sio)
> >                 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,8 +544,12 @@ 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;
> >         }
> >         sio->addr = addr;
> > --
> > 2.35.1
> >
> >  
> 

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

[Henning Schild]

Am Thu, 11 Aug 2022 17:39:05 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> 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.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/gpio/gpio-f7188x.c | 71
> +++++++++++++++++++++++++++----------- 1 file changed, 51
> insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> index 18a3147f5a42..7b05ecc611e9 100644
> --- a/drivers/gpio/gpio-f7188x.c
> +++ b/drivers/gpio/gpio-f7188x.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889
> and F81866
> + * and Nuvoton Super-I/O NCT6116D

That Kconfig item could also use an update, will include in next
version.

Henning

>   *
>   * Copyright (C) 2010-2013 LaCie
>   *
> @@ -22,13 +23,12 @@
>  #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 */ +#define SIO_LD_GPIO_FINTEK	0x06	/* GPIO logical
> device */ +#define SIO_LD_GPIO_NUVOTON	0x07	/* GPIO
> logical device */ #define SIO_F71869_ID		0x0814
> /* F71869 chipset ID */ #define SIO_F71869A_ID
> 0x1007	/* F71869A chipset ID */ #define SIO_F71882_ID
> 	0x0541	/* F71882 chipset ID */ @@ -37,7 +37,7 @@
>  #define SIO_F81866_ID		0x1010	/* F81866 chipset
> ID */ #define SIO_F81804_ID		0x1502  /* F81804 chipset
> ID, same for f81966 */ #define SIO_F81865_ID
> 0x0704	/* F81865 chipset ID */ -
> +#define SIO_NCT6116D_ID		0xD283  /* NCT6116D chipset
> ID */ 
>  enum chips {
>  	f71869,
> @@ -48,6 +48,7 @@ enum chips {
>  	f81866,
>  	f81804,
>  	f81865,
> +	nct6116d,
>  };
>  
>  static const char * const f7188x_names[] = {
> @@ -59,10 +60,12 @@ static const char * const f7188x_names[] = {
>  	"f81866",
>  	"f81804",
>  	"f81865",
> +	"nct6116d",
>  };
>  
>  struct f7188x_sio {
>  	int addr;
> +	int device;
>  	enum chips type;
>  };
>  
> @@ -170,6 +173,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 +260,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 +281,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 +306,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 +331,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 +356,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 +366,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 +387,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 +415,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 +476,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;
>  	}
> @@ -485,12 +516,8 @@ static int __init f7188x_find(int addr, struct
> f7188x_sio *sio) 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,8 +544,12 @@ 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;
>  	}
>  	sio->addr = addr;

[PATCH v3 2/4] gpio-f7188x: use unique labels for banks/chips

[Henning Schild]

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

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 7b05ecc611e9..45b466b04070 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -149,10 +149,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,	\
@@ -177,98 +177,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

Re: [PATCH v3 2/4] gpio-f7188x: use unique labels for banks/chips

[Henning Schild]

Am Fri, 12 Aug 2022 10:39:08 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thursday, August 11, 2022, Henning Schild
> <henning.schild@siemens.com> wrote:
> 
> > So that drivers building on top can find those pins with GPIO_LOOKUP
> > helpers.  
> 
> 
> 
> Missed given tag. Do we need to bother reviewing your patches?

Sorry but i have no idea what you are talking about, please help me
out. Whatever i did miss seems to be pretty relevant it seems.

Henning

> 
> > 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 7b05ecc611e9..45b466b04070 100644
> > --- a/drivers/gpio/gpio-f7188x.c
> > +++ b/drivers/gpio/gpio-f7188x.c
> > @@ -149,10 +149,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,   \ @@ -177,98 +177,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
> >
> >  
> 

Re: [PATCH v3 2/4] gpio-f7188x: use unique labels for banks/chips

[Andy Shevchenko]

On Mon, Aug 22, 2022 at 4:21 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Fri, 12 Aug 2022 10:39:08 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Thursday, August 11, 2022, Henning Schild
> > <henning.schild@siemens.com> wrote:
> >
> > > So that drivers building on top can find those pins with GPIO_LOOKUP
> > > helpers.
> >
> > Missed given tag. Do we need to bother reviewing your patches?
>
> Sorry but i have no idea what you are talking about, please help me
> out. Whatever i did miss seems to be pretty relevant it seems.

If I remember correctly somebody gave you an Acked-by (or
Reviewed-by?) tag in previous versions of the series. I don't see it
included.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/4] gpio-f7188x: use unique labels for banks/chips

[Linus Walleij]

On Mon, Aug 22, 2022 at 11:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 22, 2022 at 4:21 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Fri, 12 Aug 2022 10:39:08 +0200
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > On Thursday, August 11, 2022, Henning Schild
> > > <henning.schild@siemens.com> wrote:
> > >
> > > > So that drivers building on top can find those pins with GPIO_LOOKUP
> > > > helpers.
> > >
> > > Missed given tag. Do we need to bother reviewing your patches?
> >
> > Sorry but i have no idea what you are talking about, please help me
> > out. Whatever i did miss seems to be pretty relevant it seems.
>
> If I remember correctly somebody gave you an Acked-by (or
> Reviewed-by?) tag in previous versions of the series. I don't see it
> included.

I think I added a Reviewed-by but it came in probably after this
version was posted due to me being slow on processing my
inbox, so this one is likely on me.

Yours,
Linus Walleij

[PATCH v3 3/4] 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.

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

Re: [PATCH v3 3/4] leds: simatic-ipc-leds-gpio: add new model 227G

[Hans de Goede]

Hi,

On 8/11/22 17:39, Henning Schild wrote:
> 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.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


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

[PATCH v3 4/4] 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

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

[Hans de Goede]

Hi,

On 8/11/22 17:39, Henning Schild wrote:
> 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>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Patches which are added to review-hans now are intended for
the next rc1. This branch will get rebased to the next rc1 when
it is out and after the rebasing the contents of review-hans
will be pushed to the platform-drivers-x86/for-next branch.

Regards,

Hans


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

Re: [PATCH v3 0/4] add support for another simatic board

[Henning Schild]

Am Thu, 11 Aug 2022 17:39:04 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

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

patches 1 and 2 are independent of those other patches and they add
value on their own, i would be happy if they got picked while waiting
for these other ones.

Henning

> 
> Henning Schild (4):
>   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
> 
>  drivers/gpio/gpio-f7188x.c                    | 193
> ++++++++++-------- drivers/leds/simple/simatic-ipc-leds-gpio.c   |
> 42 +++- drivers/platform/x86/simatic-ipc.c            |   7 +-
>  .../platform_data/x86/simatic-ipc-base.h      |   1 +
>  include/linux/platform_data/x86/simatic-ipc.h |   1 +
>  5 files changed, 157 insertions(+), 87 deletions(-)
>