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 | 192 +++++++++++-------
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, 158 insertions(+), 85 deletions(-)
--
2.35.1
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 <[email protected]>
---
drivers/gpio/gpio-f7188x.c | 70 ++++++++++++++++++++++++++++----------
1 file changed, 52 insertions(+), 18 deletions(-)
diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index 18a3147f5a42..431ce2cda1d8 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,11 @@
#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_F71869_ID 0x0814 /* F71869 chipset ID */
#define SIO_F71869A_ID 0x1007 /* F71869A chipset ID */
#define SIO_F71882_ID 0x0541 /* F71882 chipset ID */
@@ -38,6 +37,9 @@
#define SIO_F81804_ID 0x1502 /* F81804 chipset ID, same for f81966 */
#define SIO_F81865_ID 0x0704 /* F81865 chipset ID */
+#define SIO_LD_GPIO_NUVOTON 0x07 /* GPIO logical device */
+#define SIO_NCT6116D_ID 0xD283 /* NCT6116D chipset ID */
+#define SIO_GPIO_ENABLE 0x30 /* GPIO enable */
enum chips {
f71869,
@@ -48,6 +50,7 @@ enum chips {
f81866,
f81804,
f81865,
+ nct6116d,
};
static const char * const f7188x_names[] = {
@@ -59,10 +62,12 @@ static const char * const f7188x_names[] = {
"f81866",
"f81804",
"f81865",
+ "nct6116d",
};
struct f7188x_sio {
int addr;
+ int device;
enum chips type;
};
@@ -254,6 +259,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 +280,20 @@ 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 (sio->device == SIO_LD_GPIO_NUVOTON) {
+ if (dir & BIT(offset))
+ return GPIO_LINE_DIRECTION_IN;
+
+ return GPIO_LINE_DIRECTION_OUT;
+ }
+
+ if (dir & BIT(offset))
return GPIO_LINE_DIRECTION_OUT;
return GPIO_LINE_DIRECTION_IN;
@@ -286,10 +309,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 (sio->device == SIO_LD_GPIO_FINTEK)
+ dir &= ~BIT(offset);
+ else
+ dir |= BIT(offset);
superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
superio_exit(sio->addr);
@@ -307,7 +334,7 @@ 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));
@@ -332,7 +359,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 +369,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 (sio->device == SIO_LD_GPIO_FINTEK)
+ dir |= BIT(offset);
+ else
+ dir &= ~BIT(offset);
superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
superio_exit(sio->addr);
@@ -360,7 +390,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 +418,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 +479,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 +519,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 +547,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
So that drivers building on top can find those pins with GPIO_LOOKUP
helpers.
Signed-off-by: Henning Schild <[email protected]>
---
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 431ce2cda1d8..5f2f3c01231d 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -151,10 +151,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, \
@@ -176,98 +176,98 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset,
#define gpio_out_mode(base) (base + 3)
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
On Thu, Jul 28, 2022 at 5:57 PM Henning Schild
<[email protected]> 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.
...
> +#define SIO_GPIO_ENABLE 0x30 /* GPIO enable */
I don't see how it's being utilized... (But okay, it might be good to
have as a hint for a reader who has no access to the documentation).
...
> + if (sio->device == SIO_LD_GPIO_NUVOTON) {
Everywhere else you use `device == SIO_LD_GPIO_FINTEK`, perhaps here
for consistency? However, I would rather see a field that clearly
states that it's an inverted value. Then you can use
if (sio->dir_inv)
...do something...
> + if (dir & BIT(offset))
> + return GPIO_LINE_DIRECTION_IN;
> +
> + return GPIO_LINE_DIRECTION_OUT;
> + }
> +
> + if (dir & BIT(offset))
> return GPIO_LINE_DIRECTION_OUT;
>
> return GPIO_LINE_DIRECTION_IN;
--
With Best Regards,
Andy Shevchenko
I wanted to send this series to a wider audience, somehow i messed that
up and might have to send again. Maybe the v5, and if there is no
changes i might send again as v4.
Henning
Am Thu, 28 Jul 2022 17:56:48 +0200
schrieb Henning Schild <[email protected]>:
> 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 | 192
> +++++++++++------- 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, 158 insertions(+), 85 deletions(-)
>
Am Thu, 28 Jul 2022 23:33:36 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Thu, Jul 28, 2022 at 5:57 PM Henning Schild
> <[email protected]> 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.
>
> ...
>
> > +#define SIO_GPIO_ENABLE 0x30 /* GPIO enable */
>
> I don't see how it's being utilized... (But okay, it might be good to
> have as a hint for a reader who has no access to the documentation).
Good catch. That is a leftover from code that turned out to be not
needed. Will drop.
> ...
>
> > + if (sio->device == SIO_LD_GPIO_NUVOTON) {
>
> Everywhere else you use `device == SIO_LD_GPIO_FINTEK`, perhaps here
> for consistency? However, I would rather see a field that clearly
> states that it's an inverted value. Then you can use
>
> if (sio->dir_inv)
> ...do something...
Good idea, will look into that. Given we talk about a family of chips
there might be more vendor ids that should be mapped onto "inv" in the
future.
Henning
> > + if (dir & BIT(offset))
> > + return GPIO_LINE_DIRECTION_IN;
> > +
> > + return GPIO_LINE_DIRECTION_OUT;
> > + }
> > +
> > + if (dir & BIT(offset))
> > return GPIO_LINE_DIRECTION_OUT;
> >
> > return GPIO_LINE_DIRECTION_IN;
>
Am Thu, 28 Jul 2022 17:56:49 +0200
schrieb Henning Schild <[email protected]>:
> 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.
In fact the modification of f7188x_gpio_get is missing in this patch,
the function needs to be modified for this chip variant, where the
given bit has another meaning. (value invert, not used in the driver)
Will send a fixed version
Henning
> On the
> SuperIO level the logical device is another one.
>
> Signed-off-by: Henning Schild <[email protected]>
> ---
> drivers/gpio/gpio-f7188x.c | 70
> ++++++++++++++++++++++++++++---------- 1 file changed, 52
> insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> index 18a3147f5a42..431ce2cda1d8 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,11 @@
> #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_F71869_ID 0x0814 /*
> F71869 chipset ID */ #define SIO_F71869A_ID
> 0x1007 /* F71869A chipset ID */ #define SIO_F71882_ID
> 0x0541 /* F71882 chipset ID */ @@ -38,6 +37,9 @@
> #define SIO_F81804_ID 0x1502 /* F81804 chipset ID,
> same for f81966 */ #define SIO_F81865_ID 0x0704
> /* F81865 chipset ID */
> +#define SIO_LD_GPIO_NUVOTON 0x07 /* GPIO logical
> device */ +#define SIO_NCT6116D_ID 0xD283 /* NCT6116D
> chipset ID */ +#define SIO_GPIO_ENABLE 0x30 /*
> GPIO enable */
> enum chips {
> f71869,
> @@ -48,6 +50,7 @@ enum chips {
> f81866,
> f81804,
> f81865,
> + nct6116d,
> };
>
> static const char * const f7188x_names[] = {
> @@ -59,10 +62,12 @@ static const char * const f7188x_names[] = {
> "f81866",
> "f81804",
> "f81865",
> + "nct6116d",
> };
>
> struct f7188x_sio {
> int addr;
> + int device;
> enum chips type;
> };
>
> @@ -254,6 +259,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 +280,20 @@ 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 (sio->device == SIO_LD_GPIO_NUVOTON) {
> + if (dir & BIT(offset))
> + return GPIO_LINE_DIRECTION_IN;
> +
> + return GPIO_LINE_DIRECTION_OUT;
> + }
> +
> + if (dir & BIT(offset))
> return GPIO_LINE_DIRECTION_OUT;
>
> return GPIO_LINE_DIRECTION_IN;
> @@ -286,10 +309,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 (sio->device == SIO_LD_GPIO_FINTEK)
> + dir &= ~BIT(offset);
> + else
> + dir |= BIT(offset);
> superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
>
> superio_exit(sio->addr);
> @@ -307,7 +334,7 @@ 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));
> @@ -332,7 +359,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 +369,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 (sio->device == SIO_LD_GPIO_FINTEK)
> + dir |= BIT(offset);
> + else
> + dir &= ~BIT(offset);
> superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
>
> superio_exit(sio->addr);
> @@ -360,7 +390,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 +418,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 +479,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 +519,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 +547,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;
On Thu, Jul 28, 2022 at 5:57 PM Henning Schild
<[email protected]> wrote:
> So that drivers building on top can find those pins with GPIO_LOOKUP
> helpers.
>
> Signed-off-by: Henning Schild <[email protected]>
That's nice.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Jul 28, 2022 at 5:57 PM Henning Schild
<[email protected]> wrote:
> 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.
Since it is all in-kernel users, any "legacy users" should be in the
kernel tree and you should then fix them in the patch.
If they are not in the kernel tree, they are out-of-tree drivers and
them we do not care, we can't fix the whole world.
Yours,
Linus Walleij