[PATCH v3 0/1] add device driver for Nuvoton SIO gpio function

[PATCH v3 0/1] add device driver for Nuvoton SIO gpio function

[Henning Schild]

changes since v2:
 - move from subsys_initcall to module_init
 - add 2 more patches to show how it can be used later
 - v2 is based on [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

changes since v1:
 - implement get_direction function
 - style changes requested in review

This adds gpio support for several Super IO chips from Nuvoton. The
driver was originally developed by Nuvoton and i am just contributing it
on behalf, because other patches i will send later will require access
to the gpios. The driver is valid on its own.
In fact v2 of this series shows a future user, not to be merged right
away but to show what is planned.

The driver supports several chips, of which i only managed to test one
but did not want to drop the others.

I hope the original authors will help with the testing and addressing
review feedback. The changes i did so far mainly are inspired by similar
drivers and some just concern coding style. If more has to be done and
the original authors do not jump in, we might start off with just that
one chip i can test and add the others later on.

Henning Schild (3):
  gpio: nct6116d: add new driver for several Nuvoton super io chips
  leds: simatic-ipc-leds-gpio: add new model 227G
  platform/x86: simatic-ipc: enable watchdog for 227G

 drivers/gpio/Kconfig                          |   9 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-nct6116d.c                  | 412 ++++++++++++++++++
 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 +
 7 files changed, 467 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpio/gpio-nct6116d.c

-- 
2.35.1

[PATCH v3 1/3] gpio: nct6116d: add new driver for several Nuvoton super io chips

[Henning Schild]

This patch adds gpio support for several Nuvoton NCTXXX chips. These
Super-I/O chips offer multiple functions of which several already have
drivers in the kernel, i.e. hwmon and watchdog.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/gpio/Kconfig         |   9 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-nct6116d.c | 412 +++++++++++++++++++++++++++++++++++
 3 files changed, 422 insertions(+)
 create mode 100644 drivers/gpio/gpio-nct6116d.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b01961999ced..1f1ec035f3c6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -899,6 +899,15 @@ config GPIO_IT87
 	  To compile this driver as a module, choose M here: the module will
 	  be called gpio_it87.
 
+config GPIO_NCT6116D
+	tristate "Nuvoton Super-I/O GPIO support"
+	help
+	  This option enables support for GPIOs found on Nuvoton Super-I/O
+	  chips NCT5104D, NCT6106D, NCT6116D, NCT6122D.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio_nct6116d.
+
 config GPIO_SCH
 	tristate "Intel SCH/TunnelCreek/Centerton/Quark X1000 GPIO"
 	depends on (X86 || COMPILE_TEST) && ACPI
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 14352f6dfe8e..87f1b0a0cda2 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -107,6 +107,7 @@ obj-$(CONFIG_GPIO_MT7621)		+= gpio-mt7621.o
 obj-$(CONFIG_GPIO_MVEBU)		+= gpio-mvebu.o
 obj-$(CONFIG_GPIO_MXC)			+= gpio-mxc.o
 obj-$(CONFIG_GPIO_MXS)			+= gpio-mxs.o
+obj-$(CONFIG_GPIO_NCT6116D)		+= gpio-nct6116d.o
 obj-$(CONFIG_GPIO_OCTEON)		+= gpio-octeon.o
 obj-$(CONFIG_GPIO_OMAP)			+= gpio-omap.o
 obj-$(CONFIG_GPIO_PALMAS)		+= gpio-palmas.o
diff --git a/drivers/gpio/gpio-nct6116d.c b/drivers/gpio/gpio-nct6116d.c
new file mode 100644
index 000000000000..2ff92f3e11aa
--- /dev/null
+++ b/drivers/gpio/gpio-nct6116d.c
@@ -0,0 +1,412 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GPIO driver for Nuvoton Super-I/O chips NCT5104D, NCT6106D, NCT6116D, NCT6122D
+ *
+ * Authors:
+ *  Tasanakorn Phaipool <tasanakorn@gmail.com>
+ *  Sheng-Yuan Huang <syhuang3@nuvoton.com>
+ *  Kuan-Wei Ho <cwho@nuvoton.com>
+ *  Henning Schild <henning.schild@siemens.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+/*
+ * Super-I/O registers
+ */
+#define SIO_LDSEL		0x07	/* Logical device select */
+#define SIO_CHIPID		0x20	/* Chaip ID (2 bytes) */
+#define SIO_GPIO_ENABLE		0x30	/* GPIO enable */
+
+#define SIO_LD_GPIO		0x07	/* 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_ID_MASK		GENMASK(15, 4)
+#define SIO_NCT5104D_ID		0x1061
+#define SIO_NCT6106D_ID		0xC452
+#define SIO_NCT6116D_ID		0xD282
+#define SIO_NCT6122D_ID		0xD2A3
+
+enum chips {
+	nct5104d,
+	nct6106d,
+	nct6116d,
+	nct6122d,
+};
+
+static const char * const nct6116d_names[] = {
+	[nct5104d] = "nct5104d",
+	[nct6106d] = "nct6106d",
+	[nct6116d] = "nct6116d",
+	[nct6122d] = "nct6122d",
+};
+
+struct nct6116d_sio {
+	int addr;
+	enum chips type;
+};
+
+struct nct6116d_gpio_bank {
+	struct gpio_chip chip;
+	unsigned int regbase;
+	struct nct6116d_gpio_data *data;
+};
+
+struct nct6116d_gpio_data {
+	struct nct6116d_sio *sio;
+	int nr_bank;
+	struct nct6116d_gpio_bank *bank;
+};
+
+/*
+ * Super-I/O functions.
+ */
+
+static inline int superio_inb(int base, int reg)
+{
+	outb(reg, base);
+	return inb(base + 1);
+}
+
+static int superio_inw(int base, int reg)
+{
+	int val;
+
+	outb(reg++, base);
+	val = inb(base + 1) << 8;
+	outb(reg, base);
+	val |= inb(base + 1);
+
+	return val;
+}
+
+static inline void superio_outb(int base, int reg, int val)
+{
+	outb(reg, base);
+	outb(val, base + 1);
+}
+
+static inline int superio_enter(int base)
+{
+	/* Don't step on other drivers' I/O space by accident. */
+	if (!request_muxed_region(base, 2, KBUILD_MODNAME)) {
+		pr_err("I/O address 0x%04x already in use\n", base);
+		return -EBUSY;
+	}
+
+	/* According to the datasheet the key must be send twice. */
+	outb(SIO_UNLOCK_KEY, base);
+	outb(SIO_UNLOCK_KEY, base);
+
+	return 0;
+}
+
+static inline void superio_select(int base, int ld)
+{
+	outb(SIO_LDSEL, base);
+	outb(ld, base + 1);
+}
+
+static inline void superio_exit(int base)
+{
+	outb(SIO_LOCK_KEY, base);
+	release_region(base, 2);
+}
+
+/*
+ * GPIO chip.
+ */
+
+#define gpio_dir(base) ((base) + 0)
+#define gpio_data(base) ((base) + 1)
+
+static inline void *nct6116d_to_gpio_bank(struct gpio_chip *chip)
+{
+	return container_of(chip, struct nct6116d_gpio_bank, chip);
+}
+
+static int nct6116d_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
+	struct nct6116d_sio *sio = bank->data->sio;
+	int err;
+	u8 dir;
+
+	err = superio_enter(sio->addr);
+	if (err)
+		return err;
+	superio_select(sio->addr, SIO_LD_GPIO);
+
+	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
+
+	superio_exit(sio->addr);
+
+	if (dir & 1 << offset)
+		return GPIO_LINE_DIRECTION_OUT;
+
+	return GPIO_LINE_DIRECTION_IN;
+}
+
+static int nct6116d_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
+{
+	struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
+	struct nct6116d_sio *sio = bank->data->sio;
+	int err;
+	u8 dir;
+
+	err = superio_enter(sio->addr);
+	if (err)
+		return err;
+	superio_select(sio->addr, SIO_LD_GPIO);
+
+	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
+	dir |= BIT(offset);
+	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
+
+	superio_exit(sio->addr);
+
+	return 0;
+}
+
+static int nct6116d_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
+	struct nct6116d_sio *sio = bank->data->sio;
+	int err;
+	u8 data;
+
+	err = superio_enter(sio->addr);
+	if (err)
+		return err;
+	superio_select(sio->addr, SIO_LD_GPIO);
+
+	data = superio_inb(sio->addr, gpio_data(bank->regbase));
+
+	superio_exit(sio->addr);
+
+	return !!(data & BIT(offset));
+}
+
+static int nct6116d_gpio_direction_out(struct gpio_chip *chip,
+				     unsigned int offset, int value)
+{
+	struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
+	struct nct6116d_sio *sio = bank->data->sio;
+	u8 dir, data_out;
+	int err;
+
+	err = superio_enter(sio->addr);
+	if (err)
+		return err;
+	superio_select(sio->addr, SIO_LD_GPIO);
+
+	data_out = superio_inb(sio->addr, gpio_data(bank->regbase));
+	if (value)
+		data_out |= BIT(offset);
+	else
+		data_out &= ~BIT(offset);
+	superio_outb(sio->addr, gpio_data(bank->regbase), data_out);
+
+	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
+	dir &= ~BIT(offset);
+	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
+
+	superio_exit(sio->addr);
+
+	return 0;
+}
+
+static void nct6116d_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
+	struct nct6116d_sio *sio = bank->data->sio;
+	u8 data_out;
+	int err;
+
+	err = superio_enter(sio->addr);
+	if (err)
+		return;
+	superio_select(sio->addr, SIO_LD_GPIO);
+
+	data_out = superio_inb(sio->addr, gpio_data(bank->regbase));
+	if (value)
+		data_out |= BIT(offset);
+	else
+		data_out &= ~BIT(offset);
+	superio_outb(sio->addr, gpio_data(bank->regbase), data_out);
+
+	superio_exit(sio->addr);
+}
+
+#define NCT6116D_GPIO_BANK(_base, _ngpio, _regbase, _label)			\
+	{									\
+		.chip = {							\
+			.label            = _label,				\
+			.owner            = THIS_MODULE,			\
+			.get_direction    = nct6116d_gpio_get_direction,	\
+			.direction_input  = nct6116d_gpio_direction_in,		\
+			.get              = nct6116d_gpio_get,			\
+			.direction_output = nct6116d_gpio_direction_out,	\
+			.set              = nct6116d_gpio_set,			\
+			.base             = _base,				\
+			.ngpio            = _ngpio,				\
+			.can_sleep        = false,				\
+		},								\
+		.regbase = _regbase,						\
+	}
+
+static struct nct6116d_gpio_bank nct6116d_gpio_bank[] = {
+	NCT6116D_GPIO_BANK(0, 8, 0xE0, KBUILD_MODNAME "-0"),
+	NCT6116D_GPIO_BANK(10, 8, 0xE4, KBUILD_MODNAME "-1"),
+	NCT6116D_GPIO_BANK(20, 8, 0xE8, KBUILD_MODNAME "-2"),
+	NCT6116D_GPIO_BANK(30, 8, 0xEC, KBUILD_MODNAME "-3"),
+	NCT6116D_GPIO_BANK(40, 8, 0xF0, KBUILD_MODNAME "-4"),
+};
+
+/*
+ * Platform device and driver.
+ */
+
+static int nct6116d_gpio_probe(struct platform_device *pdev)
+{
+	struct nct6116d_sio *sio = pdev->dev.platform_data;
+	struct nct6116d_gpio_data *data;
+	int err;
+	int i;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->nr_bank = ARRAY_SIZE(nct6116d_gpio_bank);
+	data->bank = nct6116d_gpio_bank;
+	data->sio = sio;
+
+	platform_set_drvdata(pdev, data);
+
+	/* For each GPIO bank, register a GPIO chip. */
+	for (i = 0; i < data->nr_bank; i++) {
+		struct nct6116d_gpio_bank *bank = &data->bank[i];
+
+		bank->chip.parent = &pdev->dev;
+		bank->data = data;
+
+		err = devm_gpiochip_add_data(&pdev->dev, &bank->chip, bank);
+		if (err)
+			return dev_err_probe(&pdev->dev, err,
+				"Failed to register gpiochip %d\n", i);
+	}
+
+	return 0;
+}
+
+static int __init nct6116d_find(int addr, struct nct6116d_sio *sio)
+{
+	u16 devid;
+	int err;
+
+	err = superio_enter(addr);
+	if (err)
+		return err;
+
+	devid = superio_inw(addr, SIO_CHIPID);
+	superio_exit(addr);
+	switch (devid & SIO_ID_MASK) {
+	case SIO_NCT5104D_ID & SIO_ID_MASK:
+		sio->type = nct5104d;
+		break;
+	case SIO_NCT6106D_ID & SIO_ID_MASK:
+		sio->type = nct6106d;
+		break;
+	case SIO_NCT6116D_ID & SIO_ID_MASK:
+		sio->type = nct6116d;
+		break;
+	case SIO_NCT6122D_ID & SIO_ID_MASK:
+		sio->type = nct6122d;
+		break;
+	default:
+		pr_info("Unsupported device 0x%04x\n", devid);
+		return -ENODEV;
+	}
+	sio->addr = addr;
+
+	pr_info("Found %s at 0x%x chip id 0x%04x\n",
+		nct6116d_names[sio->type], addr, devid);
+	return 0;
+}
+
+static struct platform_device *nct6116d_gpio_pdev;
+
+static int __init
+nct6116d_gpio_device_add(const struct nct6116d_sio *sio)
+{
+	int err;
+
+	nct6116d_gpio_pdev = platform_device_alloc(KBUILD_MODNAME, -1);
+	if (!nct6116d_gpio_pdev)
+		return -ENOMEM;
+
+	err = platform_device_add_data(nct6116d_gpio_pdev, sio, sizeof(*sio));
+	if (err) {
+		pr_err("Platform data allocation failed\n");
+		goto err;
+	}
+
+	err = platform_device_add(nct6116d_gpio_pdev);
+	if (err) {
+		pr_err("Device addition failed\n");
+		goto err;
+	}
+
+	return 0;
+
+err:
+	platform_device_put(nct6116d_gpio_pdev);
+
+	return err;
+}
+
+static struct platform_driver nct6116d_gpio_driver = {
+	.driver = {
+		.name	= KBUILD_MODNAME,
+	},
+	.probe		= nct6116d_gpio_probe,
+};
+
+static int __init nct6116d_gpio_init(void)
+{
+	struct nct6116d_sio sio;
+	int err;
+
+	if (nct6116d_find(0x2e, &sio) &&
+	    nct6116d_find(0x4e, &sio))
+		return -ENODEV;
+
+	err = platform_driver_register(&nct6116d_gpio_driver);
+	if (!err) {
+		err = nct6116d_gpio_device_add(&sio);
+		if (err)
+			platform_driver_unregister(&nct6116d_gpio_driver);
+	}
+
+	return err;
+}
+
+static void __exit nct6116d_gpio_exit(void)
+{
+	platform_device_unregister(nct6116d_gpio_pdev);
+	platform_driver_unregister(&nct6116d_gpio_driver);
+}
+module_init(nct6116d_gpio_init);
+module_exit(nct6116d_gpio_exit);
+
+MODULE_DESCRIPTION("GPIO driver for Nuvoton Super-I/O chips  NCT5104D, NCT6106D, NCT6116D, NCT6122D");
+MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.35.1

[PATCH v3 2/3] 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_nct6116d 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..2931e2e2dcd4 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_nct6116d-2", 0, NULL, 0, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 1, NULL, 1, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 2, NULL, 2, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 3, NULL, 3, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 4, NULL, 4, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 5, NULL, 5, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 6, NULL, 6, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("gpio_nct6116d-3", 6, 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_NCT6116D))
+			return -ENOTSUPP;
+		request_module("gpio_nct6116d");
+		simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_227g;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	gpiod_add_lookup_table(simatic_ipc_led_gpio_table);
 	simatic_leds_pdev = platform_device_register_resndata(NULL,
 		"leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
 		&simatic_ipc_gpio_leds_pdata,
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index ca3647b751d5..1825ef21a86d 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -41,6 +41,7 @@ static struct {
 	{SIMATIC_IPC_IPC127E, SIMATIC_IPC_DEVICE_127E, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC227D, SIMATIC_IPC_DEVICE_227D, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC227E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_227E},
+	{SIMATIC_IPC_IPC227G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC277E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_227E},
 	{SIMATIC_IPC_IPC427D, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_NONE},
 	{SIMATIC_IPC_IPC427E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_427E},
@@ -65,7 +66,8 @@ static int register_platform_devices(u32 station_id)
 	}
 
 	if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
-		if (ledmode == SIMATIC_IPC_DEVICE_127E)
+		if (ledmode == SIMATIC_IPC_DEVICE_127E ||
+		    ledmode == SIMATIC_IPC_DEVICE_227G)
 			pdevname = KBUILD_MODNAME "_leds_gpio";
 		platform_data.devmode = ledmode;
 		ipc_led_platform_device =
diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h
index 39fefd48cf4d..57d6a10dfc9e 100644
--- a/include/linux/platform_data/x86/simatic-ipc-base.h
+++ b/include/linux/platform_data/x86/simatic-ipc-base.h
@@ -19,6 +19,7 @@
 #define SIMATIC_IPC_DEVICE_427E 2
 #define SIMATIC_IPC_DEVICE_127E 3
 #define SIMATIC_IPC_DEVICE_227E 4
+#define SIMATIC_IPC_DEVICE_227G 5
 
 struct simatic_ipc_platform {
 	u8	devmode;
diff --git a/include/linux/platform_data/x86/simatic-ipc.h b/include/linux/platform_data/x86/simatic-ipc.h
index f3b76b39776b..7a2e79f3be0b 100644
--- a/include/linux/platform_data/x86/simatic-ipc.h
+++ b/include/linux/platform_data/x86/simatic-ipc.h
@@ -31,6 +31,7 @@ enum simatic_ipc_station_ids {
 	SIMATIC_IPC_IPC427E = 0x00000A01,
 	SIMATIC_IPC_IPC477E = 0x00000A02,
 	SIMATIC_IPC_IPC127E = 0x00000D01,
+	SIMATIC_IPC_IPC227G = 0x00000F01,
 };
 
 static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
-- 
2.35.1

[PATCH v3 3/3] 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 0/1] add device driver for Nuvoton SIO gpio function

[Andy Shevchenko]

On Tue, Jul 12, 2022 at 4:32 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> changes since v2:
>  - move from subsys_initcall to module_init
>  - add 2 more patches to show how it can be used later
>  - v2 is based on [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper
>
> changes since v1:
>  - implement get_direction function
>  - style changes requested in review

JFYI: You have a strange subject. Had you used `git format-patch
--cover-letter ...`?

> This adds gpio support for several Super IO chips from Nuvoton. The
> driver was originally developed by Nuvoton and i am just contributing it
> on behalf, because other patches i will send later will require access
> to the gpios. The driver is valid on its own.
> In fact v2 of this series shows a future user, not to be merged right
> away but to show what is planned.
>
> The driver supports several chips, of which i only managed to test one
> but did not want to drop the others.
>
> I hope the original authors will help with the testing and addressing
> review feedback. The changes i did so far mainly are inspired by similar
> drivers and some just concern coding style. If more has to be done and
> the original authors do not jump in, we might start off with just that
> one chip i can test and add the others later on.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] gpio: nct6116d: add new driver for several Nuvoton super io chips

[Andy Shevchenko]

On Tue, Jul 12, 2022 at 4:32 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> This patch adds gpio support for several Nuvoton NCTXXX chips. These
> Super-I/O chips offer multiple functions of which several already have
> drivers in the kernel, i.e. hwmon and watchdog.

...

> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>

+ bits.h
+ types.h.

...

> +static inline int superio_enter(int base)
> +{
> +       /* Don't step on other drivers' I/O space by accident. */
> +       if (!request_muxed_region(base, 2, KBUILD_MODNAME)) {
> +               pr_err("I/O address 0x%04x already in use\n", base);
> +               return -EBUSY;
> +       }
> +
> +       /* According to the datasheet the key must be send twice. */

sent

> +       outb(SIO_UNLOCK_KEY, base);
> +       outb(SIO_UNLOCK_KEY, base);
> +
> +       return 0;
> +}

...

> +#define gpio_dir(base) ((base) + 0)
> +#define gpio_data(base) ((base) + 1)

Dunno, why haven't you changed this? It might be a hidden mine for the
future GPIO library development. I recommend using namespace whenever
it feels right, and here it's exactly the case.

> +       if (dir & 1 << offset)

Another ignored comment... BIT(offset)

I'm stopping here to let you double check what version you have sent.

-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

Note that this patch will only apply on another patch series which is
currently waiting for feedback from the LED subsystem. This patch and
in fact the next one are basically only sent to show how i am planning
to continue with that given p1 gets merged.
But i will have to wait for the series i depend on.

Am Tue, 12 Jul 2022 16:32:36 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> This adds support of the Siemens Simatic IPC227G. Its LEDs are
> connected to GPIO pins provided by the gpio_nct6116d 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..2931e2e2dcd4 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_nct6116d-2", 0, NULL, 0,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 1, NULL, 1,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 2, NULL, 2,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 3, NULL, 3,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 4, NULL, 4,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 5, NULL, 5,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 6, NULL, 6,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-3", 6, 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_NCT6116D))
> +			return -ENOTSUPP;
> +		request_module("gpio_nct6116d");

This is where the "magic" happens. We basically say that we need that
gpio driver to be loaded or builtin. We do not create a
platform_device, because that gpio driver does that on its own and has
enumeration code to find and ident which chip. Here we really just say
we need that guy to have LEDs.

Not sure that is a good/acceptable pattern. But to show why i use it
here i also decided to include the watchdog support. That watchdog
module has no MODULE_ALIAS at all and the only way to get it would be
builtin or modprobe.
If i wanted to show hwmon code for those Super I/Os ... i would have
the same problem. Drivers to some degree are already in the tree, but
with no autoloading support.

Even if i went to use platform_device_register for that new nct gpio
module, i would still end up using request_module in the simatic ipc
platform to "load modules needed for some boards".

regards,
Henning

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

Re: [PATCH v3 0/1] add device driver for Nuvoton SIO gpio function

[Henning Schild]

Am Tue, 12 Jul 2022 16:32:34 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> changes since v2:

 - moved Kconfig switch to correct section

>  - move from subsys_initcall to module_init
>  - add 2 more patches to show how it can be used later
>  - v2 is based on [PATCH v6 00/12] platform/x86: introduce p2sb_bar()
> helper
> 
> changes since v1:
>  - implement get_direction function
>  - style changes requested in review
> 
> This adds gpio support for several Super IO chips from Nuvoton. The
> driver was originally developed by Nuvoton and i am just contributing
> it on behalf, because other patches i will send later will require
> access to the gpios. The driver is valid on its own.
> In fact v2 of this series shows a future user, not to be merged right
> away but to show what is planned.
> 
> The driver supports several chips, of which i only managed to test one
> but did not want to drop the others.
> 
> I hope the original authors will help with the testing and addressing
> review feedback. The changes i did so far mainly are inspired by
> similar drivers and some just concern coding style. If more has to be
> done and the original authors do not jump in, we might start off with
> just that one chip i can test and add the others later on.
> 
> Henning Schild (3):
>   gpio: nct6116d: add new driver for several Nuvoton super io chips
>   leds: simatic-ipc-leds-gpio: add new model 227G
>   platform/x86: simatic-ipc: enable watchdog for 227G
> 
>  drivers/gpio/Kconfig                          |   9 +
>  drivers/gpio/Makefile                         |   1 +
>  drivers/gpio/gpio-nct6116d.c                  | 412
> ++++++++++++++++++ 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 +
>  7 files changed, 467 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/gpio/gpio-nct6116d.c
> 

Re: [PATCH v3 0/1] add device driver for Nuvoton SIO gpio function

[Henning Schild]

Am Tue, 12 Jul 2022 16:42:46 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Tue, Jul 12, 2022 at 4:32 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > changes since v2:
> >  - move from subsys_initcall to module_init
> >  - add 2 more patches to show how it can be used later
> >  - v2 is based on [PATCH v6 00/12] platform/x86: introduce
> > p2sb_bar() helper
> >
> > changes since v1:
> >  - implement get_direction function
> >  - style changes requested in review  
> 
> JFYI: You have a strange subject. Had you used `git format-patch
> --cover-letter ...`?

Yes, but i changed that subject. Took the old line and turned v2 into
v3. What is strange about it?

Henning

> > This adds gpio support for several Super IO chips from Nuvoton. The
> > driver was originally developed by Nuvoton and i am just
> > contributing it on behalf, because other patches i will send later
> > will require access to the gpios. The driver is valid on its own.
> > In fact v2 of this series shows a future user, not to be merged
> > right away but to show what is planned.
> >
> > The driver supports several chips, of which i only managed to test
> > one but did not want to drop the others.
> >
> > I hope the original authors will help with the testing and
> > addressing review feedback. The changes i did so far mainly are
> > inspired by similar drivers and some just concern coding style. If
> > more has to be done and the original authors do not jump in, we
> > might start off with just that one chip i can test and add the
> > others later on.  
> 
> 

Re: [PATCH v3 0/1] add device driver for Nuvoton SIO gpio function

[Andy Shevchenko]

On Tue, Jul 12, 2022 at 5:16 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Tue, 12 Jul 2022 16:42:46 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Tue, Jul 12, 2022 at 4:32 PM Henning Schild
> > <henning.schild@siemens.com> wrote:

> > JFYI: You have a strange subject. Had you used `git format-patch
> > --cover-letter ...`?
>
> Yes, but i changed that subject. Took the old line and turned v2 into
> v3. What is strange about it?

The 0/1 while it has to be 0/3.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/1] add device driver for Nuvoton SIO gpio function

[Henning Schild]

Am Tue, 12 Jul 2022 17:22:45 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Tue, Jul 12, 2022 at 5:16 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > Am Tue, 12 Jul 2022 16:42:46 +0200
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > On Tue, Jul 12, 2022 at 4:32 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> 
> > > JFYI: You have a strange subject. Had you used `git format-patch
> > > --cover-letter ...`?  
> >
> > Yes, but i changed that subject. Took the old line and turned v2
> > into v3. What is strange about it?  
> 
> The 0/1 while it has to be 0/3.

A right, copy/paste mistake.

Henning

Re: [PATCH v3 1/3] gpio: nct6116d: add new driver for several Nuvoton super io chips

[Bartosz Golaszewski]

On Tue, Jul 12, 2022 at 4:32 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> This patch adds gpio support for several Nuvoton NCTXXX chips. These
> Super-I/O chips offer multiple functions of which several already have
> drivers in the kernel, i.e. hwmon and watchdog.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/gpio/Kconfig         |   9 +
>  drivers/gpio/Makefile        |   1 +
>  drivers/gpio/gpio-nct6116d.c | 412 +++++++++++++++++++++++++++++++++++
>  3 files changed, 422 insertions(+)
>  create mode 100644 drivers/gpio/gpio-nct6116d.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b01961999ced..1f1ec035f3c6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -899,6 +899,15 @@ config GPIO_IT87
>           To compile this driver as a module, choose M here: the module will
>           be called gpio_it87.
>
> +config GPIO_NCT6116D
> +       tristate "Nuvoton Super-I/O GPIO support"
> +       help
> +         This option enables support for GPIOs found on Nuvoton Super-I/O
> +         chips NCT5104D, NCT6106D, NCT6116D, NCT6122D.
> +
> +         To compile this driver as a module, choose M here: the module will
> +         be called gpio_nct6116d.
> +
>  config GPIO_SCH
>         tristate "Intel SCH/TunnelCreek/Centerton/Quark X1000 GPIO"
>         depends on (X86 || COMPILE_TEST) && ACPI
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 14352f6dfe8e..87f1b0a0cda2 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -107,6 +107,7 @@ obj-$(CONFIG_GPIO_MT7621)           += gpio-mt7621.o
>  obj-$(CONFIG_GPIO_MVEBU)               += gpio-mvebu.o
>  obj-$(CONFIG_GPIO_MXC)                 += gpio-mxc.o
>  obj-$(CONFIG_GPIO_MXS)                 += gpio-mxs.o
> +obj-$(CONFIG_GPIO_NCT6116D)            += gpio-nct6116d.o
>  obj-$(CONFIG_GPIO_OCTEON)              += gpio-octeon.o
>  obj-$(CONFIG_GPIO_OMAP)                        += gpio-omap.o
>  obj-$(CONFIG_GPIO_PALMAS)              += gpio-palmas.o
> diff --git a/drivers/gpio/gpio-nct6116d.c b/drivers/gpio/gpio-nct6116d.c
> new file mode 100644
> index 000000000000..2ff92f3e11aa
> --- /dev/null
> +++ b/drivers/gpio/gpio-nct6116d.c
> @@ -0,0 +1,412 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPIO driver for Nuvoton Super-I/O chips NCT5104D, NCT6106D, NCT6116D, NCT6122D
> + *
> + * Authors:
> + *  Tasanakorn Phaipool <tasanakorn@gmail.com>
> + *  Sheng-Yuan Huang <syhuang3@nuvoton.com>
> + *  Kuan-Wei Ho <cwho@nuvoton.com>
> + *  Henning Schild <henning.schild@siemens.com>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * Super-I/O registers
> + */
> +#define SIO_LDSEL              0x07    /* Logical device select */
> +#define SIO_CHIPID             0x20    /* Chaip ID (2 bytes) */
> +#define SIO_GPIO_ENABLE                0x30    /* GPIO enable */
> +
> +#define SIO_LD_GPIO            0x07    /* 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_ID_MASK            GENMASK(15, 4)
> +#define SIO_NCT5104D_ID                0x1061
> +#define SIO_NCT6106D_ID                0xC452
> +#define SIO_NCT6116D_ID                0xD282
> +#define SIO_NCT6122D_ID                0xD2A3
> +
> +enum chips {
> +       nct5104d,
> +       nct6106d,
> +       nct6116d,
> +       nct6122d,
> +};
> +
> +static const char * const nct6116d_names[] = {
> +       [nct5104d] = "nct5104d",
> +       [nct6106d] = "nct6106d",
> +       [nct6116d] = "nct6116d",
> +       [nct6122d] = "nct6122d",
> +};
> +
> +struct nct6116d_sio {
> +       int addr;
> +       enum chips type;
> +};
> +
> +struct nct6116d_gpio_bank {
> +       struct gpio_chip chip;
> +       unsigned int regbase;
> +       struct nct6116d_gpio_data *data;
> +};
> +
> +struct nct6116d_gpio_data {
> +       struct nct6116d_sio *sio;
> +       int nr_bank;
> +       struct nct6116d_gpio_bank *bank;
> +};
> +
> +/*
> + * Super-I/O functions.
> + */
> +
> +static inline int superio_inb(int base, int reg)
> +{
> +       outb(reg, base);
> +       return inb(base + 1);
> +}
> +
> +static int superio_inw(int base, int reg)
> +{
> +       int val;
> +
> +       outb(reg++, base);
> +       val = inb(base + 1) << 8;
> +       outb(reg, base);
> +       val |= inb(base + 1);
> +
> +       return val;
> +}
> +
> +static inline void superio_outb(int base, int reg, int val)
> +{
> +       outb(reg, base);
> +       outb(val, base + 1);
> +}
> +
> +static inline int superio_enter(int base)
> +{
> +       /* Don't step on other drivers' I/O space by accident. */
> +       if (!request_muxed_region(base, 2, KBUILD_MODNAME)) {
> +               pr_err("I/O address 0x%04x already in use\n", base);
> +               return -EBUSY;
> +       }
> +
> +       /* According to the datasheet the key must be send twice. */
> +       outb(SIO_UNLOCK_KEY, base);
> +       outb(SIO_UNLOCK_KEY, base);
> +
> +       return 0;
> +}
> +
> +static inline void superio_select(int base, int ld)
> +{
> +       outb(SIO_LDSEL, base);
> +       outb(ld, base + 1);
> +}
> +
> +static inline void superio_exit(int base)
> +{
> +       outb(SIO_LOCK_KEY, base);
> +       release_region(base, 2);
> +}
> +
> +/*
> + * GPIO chip.
> + */
> +
> +#define gpio_dir(base) ((base) + 0)
> +#define gpio_data(base) ((base) + 1)
> +
> +static inline void *nct6116d_to_gpio_bank(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct nct6116d_gpio_bank, chip);
> +}
> +
> +static int nct6116d_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
> +       struct nct6116d_sio *sio = bank->data->sio;
> +       int err;
> +       u8 dir;
> +
> +       err = superio_enter(sio->addr);
> +       if (err)
> +               return err;
> +       superio_select(sio->addr, SIO_LD_GPIO);
> +
> +       dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> +
> +       superio_exit(sio->addr);
> +
> +       if (dir & 1 << offset)
> +               return GPIO_LINE_DIRECTION_OUT;
> +
> +       return GPIO_LINE_DIRECTION_IN;
> +}
> +
> +static int nct6116d_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
> +       struct nct6116d_sio *sio = bank->data->sio;
> +       int err;
> +       u8 dir;
> +
> +       err = superio_enter(sio->addr);
> +       if (err)
> +               return err;
> +       superio_select(sio->addr, SIO_LD_GPIO);
> +
> +       dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> +       dir |= BIT(offset);
> +       superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
> +
> +       superio_exit(sio->addr);
> +
> +       return 0;
> +}
> +
> +static int nct6116d_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
> +       struct nct6116d_sio *sio = bank->data->sio;
> +       int err;
> +       u8 data;
> +
> +       err = superio_enter(sio->addr);
> +       if (err)
> +               return err;
> +       superio_select(sio->addr, SIO_LD_GPIO);
> +
> +       data = superio_inb(sio->addr, gpio_data(bank->regbase));
> +
> +       superio_exit(sio->addr);
> +
> +       return !!(data & BIT(offset));
> +}
> +
> +static int nct6116d_gpio_direction_out(struct gpio_chip *chip,
> +                                    unsigned int offset, int value)
> +{
> +       struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
> +       struct nct6116d_sio *sio = bank->data->sio;
> +       u8 dir, data_out;
> +       int err;
> +
> +       err = superio_enter(sio->addr);
> +       if (err)
> +               return err;
> +       superio_select(sio->addr, SIO_LD_GPIO);
> +
> +       data_out = superio_inb(sio->addr, gpio_data(bank->regbase));
> +       if (value)
> +               data_out |= BIT(offset);
> +       else
> +               data_out &= ~BIT(offset);
> +       superio_outb(sio->addr, gpio_data(bank->regbase), data_out);
> +
> +       dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> +       dir &= ~BIT(offset);
> +       superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
> +
> +       superio_exit(sio->addr);
> +
> +       return 0;
> +}
> +
> +static void nct6116d_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +       struct nct6116d_gpio_bank *bank = nct6116d_to_gpio_bank(chip);
> +       struct nct6116d_sio *sio = bank->data->sio;
> +       u8 data_out;
> +       int err;
> +
> +       err = superio_enter(sio->addr);
> +       if (err)
> +               return;
> +       superio_select(sio->addr, SIO_LD_GPIO);
> +
> +       data_out = superio_inb(sio->addr, gpio_data(bank->regbase));
> +       if (value)
> +               data_out |= BIT(offset);
> +       else
> +               data_out &= ~BIT(offset);
> +       superio_outb(sio->addr, gpio_data(bank->regbase), data_out);
> +
> +       superio_exit(sio->addr);
> +}
> +
> +#define NCT6116D_GPIO_BANK(_base, _ngpio, _regbase, _label)                    \
> +       {                                                                       \
> +               .chip = {                                                       \
> +                       .label            = _label,                             \
> +                       .owner            = THIS_MODULE,                        \
> +                       .get_direction    = nct6116d_gpio_get_direction,        \
> +                       .direction_input  = nct6116d_gpio_direction_in,         \
> +                       .get              = nct6116d_gpio_get,                  \
> +                       .direction_output = nct6116d_gpio_direction_out,        \
> +                       .set              = nct6116d_gpio_set,                  \
> +                       .base             = _base,                              \
> +                       .ngpio            = _ngpio,                             \
> +                       .can_sleep        = false,                              \
> +               },                                                              \
> +               .regbase = _regbase,                                            \
> +       }
> +
> +static struct nct6116d_gpio_bank nct6116d_gpio_bank[] = {
> +       NCT6116D_GPIO_BANK(0, 8, 0xE0, KBUILD_MODNAME "-0"),
> +       NCT6116D_GPIO_BANK(10, 8, 0xE4, KBUILD_MODNAME "-1"),
> +       NCT6116D_GPIO_BANK(20, 8, 0xE8, KBUILD_MODNAME "-2"),
> +       NCT6116D_GPIO_BANK(30, 8, 0xEC, KBUILD_MODNAME "-3"),
> +       NCT6116D_GPIO_BANK(40, 8, 0xF0, KBUILD_MODNAME "-4"),
> +};
> +
> +/*
> + * Platform device and driver.
> + */
> +
> +static int nct6116d_gpio_probe(struct platform_device *pdev)
> +{
> +       struct nct6116d_sio *sio = pdev->dev.platform_data;
> +       struct nct6116d_gpio_data *data;
> +       int err;
> +       int i;
> +
> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->nr_bank = ARRAY_SIZE(nct6116d_gpio_bank);
> +       data->bank = nct6116d_gpio_bank;
> +       data->sio = sio;
> +
> +       platform_set_drvdata(pdev, data);
> +
> +       /* For each GPIO bank, register a GPIO chip. */
> +       for (i = 0; i < data->nr_bank; i++) {
> +               struct nct6116d_gpio_bank *bank = &data->bank[i];
> +
> +               bank->chip.parent = &pdev->dev;
> +               bank->data = data;
> +
> +               err = devm_gpiochip_add_data(&pdev->dev, &bank->chip, bank);
> +               if (err)
> +                       return dev_err_probe(&pdev->dev, err,
> +                               "Failed to register gpiochip %d\n", i);
> +       }
> +
> +       return 0;
> +}
> +
> +static int __init nct6116d_find(int addr, struct nct6116d_sio *sio)
> +{
> +       u16 devid;
> +       int err;
> +
> +       err = superio_enter(addr);
> +       if (err)
> +               return err;
> +
> +       devid = superio_inw(addr, SIO_CHIPID);
> +       superio_exit(addr);
> +       switch (devid & SIO_ID_MASK) {
> +       case SIO_NCT5104D_ID & SIO_ID_MASK:
> +               sio->type = nct5104d;
> +               break;
> +       case SIO_NCT6106D_ID & SIO_ID_MASK:
> +               sio->type = nct6106d;
> +               break;
> +       case SIO_NCT6116D_ID & SIO_ID_MASK:
> +               sio->type = nct6116d;
> +               break;
> +       case SIO_NCT6122D_ID & SIO_ID_MASK:
> +               sio->type = nct6122d;
> +               break;
> +       default:
> +               pr_info("Unsupported device 0x%04x\n", devid);
> +               return -ENODEV;
> +       }
> +       sio->addr = addr;
> +
> +       pr_info("Found %s at 0x%x chip id 0x%04x\n",
> +               nct6116d_names[sio->type], addr, devid);
> +       return 0;
> +}
> +
> +static struct platform_device *nct6116d_gpio_pdev;
> +
> +static int __init
> +nct6116d_gpio_device_add(const struct nct6116d_sio *sio)
> +{
> +       int err;
> +
> +       nct6116d_gpio_pdev = platform_device_alloc(KBUILD_MODNAME, -1);
> +       if (!nct6116d_gpio_pdev)
> +               return -ENOMEM;
> +
> +       err = platform_device_add_data(nct6116d_gpio_pdev, sio, sizeof(*sio));
> +       if (err) {
> +               pr_err("Platform data allocation failed\n");
> +               goto err;
> +       }
> +
> +       err = platform_device_add(nct6116d_gpio_pdev);
> +       if (err) {
> +               pr_err("Device addition failed\n");
> +               goto err;
> +       }
> +
> +       return 0;
> +
> +err:
> +       platform_device_put(nct6116d_gpio_pdev);
> +
> +       return err;
> +}
> +
> +static struct platform_driver nct6116d_gpio_driver = {
> +       .driver = {
> +               .name   = KBUILD_MODNAME,
> +       },
> +       .probe          = nct6116d_gpio_probe,
> +};
> +
> +static int __init nct6116d_gpio_init(void)
> +{
> +       struct nct6116d_sio sio;
> +       int err;
> +
> +       if (nct6116d_find(0x2e, &sio) &&
> +           nct6116d_find(0x4e, &sio))
> +               return -ENODEV;
> +
> +       err = platform_driver_register(&nct6116d_gpio_driver);
> +       if (!err) {
> +               err = nct6116d_gpio_device_add(&sio);
> +               if (err)
> +                       platform_driver_unregister(&nct6116d_gpio_driver);
> +       }
> +
> +       return err;
> +}

I'm confused - have we not discussed removing this part?

Bart

> +
> +static void __exit nct6116d_gpio_exit(void)
> +{
> +       platform_device_unregister(nct6116d_gpio_pdev);
> +       platform_driver_unregister(&nct6116d_gpio_driver);
> +}
> +module_init(nct6116d_gpio_init);
> +module_exit(nct6116d_gpio_exit);
> +
> +MODULE_DESCRIPTION("GPIO driver for Nuvoton Super-I/O chips  NCT5104D, NCT6106D, NCT6116D, NCT6122D");
> +MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@gmail.com>");
> +MODULE_LICENSE("GPL");
> --
> 2.35.1
>

Re: [PATCH v3 1/3] gpio: nct6116d: add new driver for several Nuvoton super io chips

[Henning Schild]

Am Wed, 13 Jul 2022 09:27:56 +0200
schrieb Bartosz Golaszewski <brgl@bgdev.pl>:

> On Tue, Jul 12, 2022 at 4:32 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > This patch adds gpio support for several Nuvoton NCTXXX chips. These
> > Super-I/O chips offer multiple functions of which several already
> > have drivers in the kernel, i.e. hwmon and watchdog.
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/gpio/Kconfig         |   9 +
> >  drivers/gpio/Makefile        |   1 +
> >  drivers/gpio/gpio-nct6116d.c | 412
> > +++++++++++++++++++++++++++++++++++ 3 files changed, 422
> > insertions(+) create mode 100644 drivers/gpio/gpio-nct6116d.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index b01961999ced..1f1ec035f3c6 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -899,6 +899,15 @@ config GPIO_IT87
> >           To compile this driver as a module, choose M here: the
> > module will be called gpio_it87.
> >
> > +config GPIO_NCT6116D
> > +       tristate "Nuvoton Super-I/O GPIO support"
> > +       help
> > +         This option enables support for GPIOs found on Nuvoton
> > Super-I/O
> > +         chips NCT5104D, NCT6106D, NCT6116D, NCT6122D.
> > +
> > +         To compile this driver as a module, choose M here: the
> > module will
> > +         be called gpio_nct6116d.
> > +
> >  config GPIO_SCH
> >         tristate "Intel SCH/TunnelCreek/Centerton/Quark X1000 GPIO"
> >         depends on (X86 || COMPILE_TEST) && ACPI
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 14352f6dfe8e..87f1b0a0cda2 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -107,6 +107,7 @@ obj-$(CONFIG_GPIO_MT7621)           +=
> > gpio-mt7621.o obj-$(CONFIG_GPIO_MVEBU)               += gpio-mvebu.o
> >  obj-$(CONFIG_GPIO_MXC)                 += gpio-mxc.o
> >  obj-$(CONFIG_GPIO_MXS)                 += gpio-mxs.o
> > +obj-$(CONFIG_GPIO_NCT6116D)            += gpio-nct6116d.o
> >  obj-$(CONFIG_GPIO_OCTEON)              += gpio-octeon.o
> >  obj-$(CONFIG_GPIO_OMAP)                        += gpio-omap.o
> >  obj-$(CONFIG_GPIO_PALMAS)              += gpio-palmas.o
> > diff --git a/drivers/gpio/gpio-nct6116d.c
> > b/drivers/gpio/gpio-nct6116d.c new file mode 100644
> > index 000000000000..2ff92f3e11aa
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-nct6116d.c
> > @@ -0,0 +1,412 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * GPIO driver for Nuvoton Super-I/O chips NCT5104D, NCT6106D,
> > NCT6116D, NCT6122D
> > + *
> > + * Authors:
> > + *  Tasanakorn Phaipool <tasanakorn@gmail.com>
> > + *  Sheng-Yuan Huang <syhuang3@nuvoton.com>
> > + *  Kuan-Wei Ho <cwho@nuvoton.com>
> > + *  Henning Schild <henning.schild@siemens.com>
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +/*
> > + * Super-I/O registers
> > + */
> > +#define SIO_LDSEL              0x07    /* Logical device select */
> > +#define SIO_CHIPID             0x20    /* Chaip ID (2 bytes) */
> > +#define SIO_GPIO_ENABLE                0x30    /* GPIO enable */
> > +
> > +#define SIO_LD_GPIO            0x07    /* 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_ID_MASK            GENMASK(15, 4)
> > +#define SIO_NCT5104D_ID                0x1061
> > +#define SIO_NCT6106D_ID                0xC452
> > +#define SIO_NCT6116D_ID                0xD282
> > +#define SIO_NCT6122D_ID                0xD2A3
> > +
> > +enum chips {
> > +       nct5104d,
> > +       nct6106d,
> > +       nct6116d,
> > +       nct6122d,
> > +};
> > +
> > +static const char * const nct6116d_names[] = {
> > +       [nct5104d] = "nct5104d",
> > +       [nct6106d] = "nct6106d",
> > +       [nct6116d] = "nct6116d",
> > +       [nct6122d] = "nct6122d",
> > +};
> > +
> > +struct nct6116d_sio {
> > +       int addr;
> > +       enum chips type;
> > +};
> > +
> > +struct nct6116d_gpio_bank {
> > +       struct gpio_chip chip;
> > +       unsigned int regbase;
> > +       struct nct6116d_gpio_data *data;
> > +};
> > +
> > +struct nct6116d_gpio_data {
> > +       struct nct6116d_sio *sio;
> > +       int nr_bank;
> > +       struct nct6116d_gpio_bank *bank;
> > +};
> > +
> > +/*
> > + * Super-I/O functions.
> > + */
> > +
> > +static inline int superio_inb(int base, int reg)
> > +{
> > +       outb(reg, base);
> > +       return inb(base + 1);
> > +}
> > +
> > +static int superio_inw(int base, int reg)
> > +{
> > +       int val;
> > +
> > +       outb(reg++, base);
> > +       val = inb(base + 1) << 8;
> > +       outb(reg, base);
> > +       val |= inb(base + 1);
> > +
> > +       return val;
> > +}
> > +
> > +static inline void superio_outb(int base, int reg, int val)
> > +{
> > +       outb(reg, base);
> > +       outb(val, base + 1);
> > +}
> > +
> > +static inline int superio_enter(int base)
> > +{
> > +       /* Don't step on other drivers' I/O space by accident. */
> > +       if (!request_muxed_region(base, 2, KBUILD_MODNAME)) {
> > +               pr_err("I/O address 0x%04x already in use\n", base);
> > +               return -EBUSY;
> > +       }
> > +
> > +       /* According to the datasheet the key must be send twice. */
> > +       outb(SIO_UNLOCK_KEY, base);
> > +       outb(SIO_UNLOCK_KEY, base);
> > +
> > +       return 0;
> > +}
> > +
> > +static inline void superio_select(int base, int ld)
> > +{
> > +       outb(SIO_LDSEL, base);
> > +       outb(ld, base + 1);
> > +}
> > +
> > +static inline void superio_exit(int base)
> > +{
> > +       outb(SIO_LOCK_KEY, base);
> > +       release_region(base, 2);
> > +}
> > +
> > +/*
> > + * GPIO chip.
> > + */
> > +
> > +#define gpio_dir(base) ((base) + 0)
> > +#define gpio_data(base) ((base) + 1)
> > +
> > +static inline void *nct6116d_to_gpio_bank(struct gpio_chip *chip)
> > +{
> > +       return container_of(chip, struct nct6116d_gpio_bank, chip);
> > +}
> > +
> > +static int nct6116d_gpio_get_direction(struct gpio_chip *chip,
> > unsigned int offset) +{
> > +       struct nct6116d_gpio_bank *bank =
> > nct6116d_to_gpio_bank(chip);
> > +       struct nct6116d_sio *sio = bank->data->sio;
> > +       int err;
> > +       u8 dir;
> > +
> > +       err = superio_enter(sio->addr);
> > +       if (err)
> > +               return err;
> > +       superio_select(sio->addr, SIO_LD_GPIO);
> > +
> > +       dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> > +
> > +       superio_exit(sio->addr);
> > +
> > +       if (dir & 1 << offset)
> > +               return GPIO_LINE_DIRECTION_OUT;
> > +
> > +       return GPIO_LINE_DIRECTION_IN;
> > +}
> > +
> > +static int nct6116d_gpio_direction_in(struct gpio_chip *chip,
> > unsigned int offset) +{
> > +       struct nct6116d_gpio_bank *bank =
> > nct6116d_to_gpio_bank(chip);
> > +       struct nct6116d_sio *sio = bank->data->sio;
> > +       int err;
> > +       u8 dir;
> > +
> > +       err = superio_enter(sio->addr);
> > +       if (err)
> > +               return err;
> > +       superio_select(sio->addr, SIO_LD_GPIO);
> > +
> > +       dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> > +       dir |= BIT(offset);
> > +       superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
> > +
> > +       superio_exit(sio->addr);
> > +
> > +       return 0;
> > +}
> > +
> > +static int nct6116d_gpio_get(struct gpio_chip *chip, unsigned int
> > offset) +{
> > +       struct nct6116d_gpio_bank *bank =
> > nct6116d_to_gpio_bank(chip);
> > +       struct nct6116d_sio *sio = bank->data->sio;
> > +       int err;
> > +       u8 data;
> > +
> > +       err = superio_enter(sio->addr);
> > +       if (err)
> > +               return err;
> > +       superio_select(sio->addr, SIO_LD_GPIO);
> > +
> > +       data = superio_inb(sio->addr, gpio_data(bank->regbase));
> > +
> > +       superio_exit(sio->addr);
> > +
> > +       return !!(data & BIT(offset));
> > +}
> > +
> > +static int nct6116d_gpio_direction_out(struct gpio_chip *chip,
> > +                                    unsigned int offset, int value)
> > +{
> > +       struct nct6116d_gpio_bank *bank =
> > nct6116d_to_gpio_bank(chip);
> > +       struct nct6116d_sio *sio = bank->data->sio;
> > +       u8 dir, data_out;
> > +       int err;
> > +
> > +       err = superio_enter(sio->addr);
> > +       if (err)
> > +               return err;
> > +       superio_select(sio->addr, SIO_LD_GPIO);
> > +
> > +       data_out = superio_inb(sio->addr, gpio_data(bank->regbase));
> > +       if (value)
> > +               data_out |= BIT(offset);
> > +       else
> > +               data_out &= ~BIT(offset);
> > +       superio_outb(sio->addr, gpio_data(bank->regbase), data_out);
> > +
> > +       dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> > +       dir &= ~BIT(offset);
> > +       superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
> > +
> > +       superio_exit(sio->addr);
> > +
> > +       return 0;
> > +}
> > +
> > +static void nct6116d_gpio_set(struct gpio_chip *chip, unsigned int
> > offset, int value) +{
> > +       struct nct6116d_gpio_bank *bank =
> > nct6116d_to_gpio_bank(chip);
> > +       struct nct6116d_sio *sio = bank->data->sio;
> > +       u8 data_out;
> > +       int err;
> > +
> > +       err = superio_enter(sio->addr);
> > +       if (err)
> > +               return;
> > +       superio_select(sio->addr, SIO_LD_GPIO);
> > +
> > +       data_out = superio_inb(sio->addr, gpio_data(bank->regbase));
> > +       if (value)
> > +               data_out |= BIT(offset);
> > +       else
> > +               data_out &= ~BIT(offset);
> > +       superio_outb(sio->addr, gpio_data(bank->regbase), data_out);
> > +
> > +       superio_exit(sio->addr);
> > +}
> > +
> > +#define NCT6116D_GPIO_BANK(_base, _ngpio, _regbase, _label)
> >             \
> > +       {
> >             \
> > +               .chip = {
> >             \
> > +                       .label            = _label,
> >             \
> > +                       .owner            = THIS_MODULE,
> >             \
> > +                       .get_direction    =
> > nct6116d_gpio_get_direction,        \
> > +                       .direction_input  =
> > nct6116d_gpio_direction_in,         \
> > +                       .get              = nct6116d_gpio_get,
> >             \
> > +                       .direction_output =
> > nct6116d_gpio_direction_out,        \
> > +                       .set              = nct6116d_gpio_set,
> >             \
> > +                       .base             = _base,
> >             \
> > +                       .ngpio            = _ngpio,
> >             \
> > +                       .can_sleep        = false,
> >             \
> > +               },
> >             \
> > +               .regbase = _regbase,
> >             \
> > +       }
> > +
> > +static struct nct6116d_gpio_bank nct6116d_gpio_bank[] = {
> > +       NCT6116D_GPIO_BANK(0, 8, 0xE0, KBUILD_MODNAME "-0"),
> > +       NCT6116D_GPIO_BANK(10, 8, 0xE4, KBUILD_MODNAME "-1"),
> > +       NCT6116D_GPIO_BANK(20, 8, 0xE8, KBUILD_MODNAME "-2"),
> > +       NCT6116D_GPIO_BANK(30, 8, 0xEC, KBUILD_MODNAME "-3"),
> > +       NCT6116D_GPIO_BANK(40, 8, 0xF0, KBUILD_MODNAME "-4"),
> > +};
> > +
> > +/*
> > + * Platform device and driver.
> > + */
> > +
> > +static int nct6116d_gpio_probe(struct platform_device *pdev)
> > +{
> > +       struct nct6116d_sio *sio = pdev->dev.platform_data;
> > +       struct nct6116d_gpio_data *data;
> > +       int err;
> > +       int i;
> > +
> > +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       data->nr_bank = ARRAY_SIZE(nct6116d_gpio_bank);
> > +       data->bank = nct6116d_gpio_bank;
> > +       data->sio = sio;
> > +
> > +       platform_set_drvdata(pdev, data);
> > +
> > +       /* For each GPIO bank, register a GPIO chip. */
> > +       for (i = 0; i < data->nr_bank; i++) {
> > +               struct nct6116d_gpio_bank *bank = &data->bank[i];
> > +
> > +               bank->chip.parent = &pdev->dev;
> > +               bank->data = data;
> > +
> > +               err = devm_gpiochip_add_data(&pdev->dev,
> > &bank->chip, bank);
> > +               if (err)
> > +                       return dev_err_probe(&pdev->dev, err,
> > +                               "Failed to register gpiochip %d\n",
> > i);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int __init nct6116d_find(int addr, struct nct6116d_sio *sio)
> > +{
> > +       u16 devid;
> > +       int err;
> > +
> > +       err = superio_enter(addr);
> > +       if (err)
> > +               return err;
> > +
> > +       devid = superio_inw(addr, SIO_CHIPID);
> > +       superio_exit(addr);
> > +       switch (devid & SIO_ID_MASK) {
> > +       case SIO_NCT5104D_ID & SIO_ID_MASK:
> > +               sio->type = nct5104d;
> > +               break;
> > +       case SIO_NCT6106D_ID & SIO_ID_MASK:
> > +               sio->type = nct6106d;
> > +               break;
> > +       case SIO_NCT6116D_ID & SIO_ID_MASK:
> > +               sio->type = nct6116d;
> > +               break;
> > +       case SIO_NCT6122D_ID & SIO_ID_MASK:
> > +               sio->type = nct6122d;
> > +               break;
> > +       default:
> > +               pr_info("Unsupported device 0x%04x\n", devid);
> > +               return -ENODEV;
> > +       }
> > +       sio->addr = addr;
> > +
> > +       pr_info("Found %s at 0x%x chip id 0x%04x\n",
> > +               nct6116d_names[sio->type], addr, devid);
> > +       return 0;
> > +}
> > +
> > +static struct platform_device *nct6116d_gpio_pdev;
> > +
> > +static int __init
> > +nct6116d_gpio_device_add(const struct nct6116d_sio *sio)
> > +{
> > +       int err;
> > +
> > +       nct6116d_gpio_pdev = platform_device_alloc(KBUILD_MODNAME,
> > -1);
> > +       if (!nct6116d_gpio_pdev)
> > +               return -ENOMEM;
> > +
> > +       err = platform_device_add_data(nct6116d_gpio_pdev, sio,
> > sizeof(*sio));
> > +       if (err) {
> > +               pr_err("Platform data allocation failed\n");
> > +               goto err;
> > +       }
> > +
> > +       err = platform_device_add(nct6116d_gpio_pdev);
> > +       if (err) {
> > +               pr_err("Device addition failed\n");
> > +               goto err;
> > +       }
> > +
> > +       return 0;
> > +
> > +err:
> > +       platform_device_put(nct6116d_gpio_pdev);
> > +
> > +       return err;
> > +}
> > +
> > +static struct platform_driver nct6116d_gpio_driver = {
> > +       .driver = {
> > +               .name   = KBUILD_MODNAME,
> > +       },
> > +       .probe          = nct6116d_gpio_probe,
> > +};
> > +
> > +static int __init nct6116d_gpio_init(void)
> > +{
> > +       struct nct6116d_sio sio;
> > +       int err;
> > +
> > +       if (nct6116d_find(0x2e, &sio) &&
> > +           nct6116d_find(0x4e, &sio))
> > +               return -ENODEV;
> > +
> > +       err = platform_driver_register(&nct6116d_gpio_driver);
> > +       if (!err) {
> > +               err = nct6116d_gpio_device_add(&sio);
> > +               if (err)
> > +
> > platform_driver_unregister(&nct6116d_gpio_driver);
> > +       }
> > +
> > +       return err;
> > +}  
> 
> I'm confused - have we not discussed removing this part?

Ah, i thought the problem was the use of subsys_initcall because the
comment was under that line.

To he honest i do not know all the details since i really just received
that driver.

What is happening here is that some init code goes and probes well
known ports to discover which chip might be connected there. Looking at
hwmon, watchdog and similar gpios ... that is the established pattern
for Super IOs.
Not DT or ACPI bindings. Something has to load that module to make it
init, where it will go and enumerate/poke around.

While i could likely put a platform_device_register_simple("driver",
0x42, "chip42") into the simatic platform, then the driver would be
pretty useless when not having ACPI (for there Super IOs in general!).
There already are hwmon and watchdog drivers for exactly that chip.

watchdog/w83627hf_wdt.c
hwmon/nct6775*

All are based on someone has to "know" and "modprobe", which will cause
"finding"

The pattern we have here seems all copied from gpio/gpio-f7188x.c,
another super similar driver is gpio/gpio-winbond.c (which is param
based and not at all reusable in other drivers).

Looking at hwmon or watchdog, Nuvoton/Fintek/Windbond are sometimes
called a family. But the driver landscape in the kernel is very spread
and a lot of copied around code. I did not even look into tty/serial or
whatever other functions these Super I/Os offer.

Looking at the way Super IO chips are driven in the kernel, it seems
all about writing a driver for each sub-function (uart, hwmon, watchdog
... and gpio). Where even very similar chips gets new drivers instead of
making existing drivers more generic.

I am just observing this and proposing a "similar copy", which i did
not even write myself. At some point it might be better to rewrite all
that and make Super I/Os platforms that discover the chip once and then
register all the many devices they have. Where ressources are properly
reserved and not that really weird "superio_enter" with
"request_muxed_region(base, 2, DRVNAME)" which can be found all over
the place. But hey that allows a very smooth mix of in-tree and
out-of-tree.

When reviewing this driver i suggest to measure it against i.e. f7188
or winbond and maybe others.

Say i would manage to make the nct6116d chip supported by f7188, would
that be acceptable? I would have the "init pattern" i need without
copying it. But i would add a Nuvoton chip to a Fintek driver ... might
be the same family ... no clue.

Henning

> Bart
> 
> > +
> > +static void __exit nct6116d_gpio_exit(void)
> > +{
> > +       platform_device_unregister(nct6116d_gpio_pdev);
> > +       platform_driver_unregister(&nct6116d_gpio_driver);
> > +}
> > +module_init(nct6116d_gpio_init);
> > +module_exit(nct6116d_gpio_exit);
> > +
> > +MODULE_DESCRIPTION("GPIO driver for Nuvoton Super-I/O chips
> > NCT5104D, NCT6106D, NCT6116D, NCT6122D");
> > +MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@gmail.com>");
> > +MODULE_LICENSE("GPL"); --
> > 2.35.1
> >  

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

[Henning Schild]

Am Tue, 12 Jul 2022 16:32:36 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> This adds support of the Siemens Simatic IPC227G. Its LEDs are
> connected to GPIO pins provided by the gpio_nct6116d 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..2931e2e2dcd4 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_nct6116d-2", 0, NULL, 0,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 1, NULL, 1,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 2, NULL, 2,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 3, NULL, 3,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 4, NULL, 4,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 5, NULL, 5,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-2", 6, NULL, 6,
> GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP_IDX("gpio_nct6116d-3", 6, 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_NCT6116D))
> +			return -ENOTSUPP;
> +		request_module("gpio_nct6116d");

When i did send v3 i wanted to point attention mainly to this here and
get an opinion on that.

Here i am in a context where i already know exactly which board i am
on. Now i need to bring up my gpio before putting those LEDs on top.

I meanwhile managed to add the chip nct6116d into gpio/gpio-f7188x.c,
which i think looks better and is significantly less code. Easier to
maintain hopefully, but less strict separation of concerns. With a
Fintec driver driving a Nuvoton chip ... no matter how very similar
they are, some people might have objections. I will send the code in a
few days so we can discuss.

But be it in that existing gpio driver or in a new one, the need to init
that one from here or the simatic-platform will remain. And for hwmon
and watchdog i plan to use the same "request_module" trick.

Only here it is guarded by an IS_ENABLED because the feature is not
optional, it is needed for LEDs. And for watchdog i just request the
module ... so it comes up should it be configured into the kernel.

checkpatch did not like ENOTSUPP, happy to take suggestions on a better
error code to say "LEDs can only work when GPIO is enabled ... and
probed"

regards,
Henning

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

Re: [PATCH v3 1/3] gpio: nct6116d: add new driver for several Nuvoton super io chips

[Bartosz Golaszewski]

On Wed, Jul 13, 2022 at 12:39 PM Henning Schild
<henning.schild@siemens.com> wrote:
>

[snip]

> > > +
> > > +static struct platform_driver nct6116d_gpio_driver = {
> > > +       .driver = {
> > > +               .name   = KBUILD_MODNAME,
> > > +       },
> > > +       .probe          = nct6116d_gpio_probe,
> > > +};
> > > +
> > > +static int __init nct6116d_gpio_init(void)
> > > +{
> > > +       struct nct6116d_sio sio;
> > > +       int err;
> > > +
> > > +       if (nct6116d_find(0x2e, &sio) &&
> > > +           nct6116d_find(0x4e, &sio))
> > > +               return -ENODEV;
> > > +
> > > +       err = platform_driver_register(&nct6116d_gpio_driver);
> > > +       if (!err) {
> > > +               err = nct6116d_gpio_device_add(&sio);
> > > +               if (err)
> > > +
> > > platform_driver_unregister(&nct6116d_gpio_driver);
> > > +       }
> > > +
> > > +       return err;
> > > +}
> >
> > I'm confused - have we not discussed removing this part?
>
> Ah, i thought the problem was the use of subsys_initcall because the
> comment was under that line.
>
> To he honest i do not know all the details since i really just received
> that driver.
>
> What is happening here is that some init code goes and probes well
> known ports to discover which chip might be connected there. Looking at
> hwmon, watchdog and similar gpios ... that is the established pattern
> for Super IOs.

I just thought that you would use the simatic driver you mentioned to
trigger the creation of the devices upon some event. This is what I
got from your previous email. But that's fine - if there's a repeating
pattern of doing it this way, then I won't object. I'm not an expert
on Super IO kernel drivers.

> Not DT or ACPI bindings. Something has to load that module to make it
> init, where it will go and enumerate/poke around.
>
> While i could likely put a platform_device_register_simple("driver",
> 0x42, "chip42") into the simatic platform, then the driver would be
> pretty useless when not having ACPI (for there Super IOs in general!).
> There already are hwmon and watchdog drivers for exactly that chip.
>
> watchdog/w83627hf_wdt.c
> hwmon/nct6775*
>
> All are based on someone has to "know" and "modprobe", which will cause
> "finding"
>
> The pattern we have here seems all copied from gpio/gpio-f7188x.c,
> another super similar driver is gpio/gpio-winbond.c (which is param
> based and not at all reusable in other drivers).
>
> Looking at hwmon or watchdog, Nuvoton/Fintek/Windbond are sometimes
> called a family. But the driver landscape in the kernel is very spread
> and a lot of copied around code. I did not even look into tty/serial or
> whatever other functions these Super I/Os offer.
>
> Looking at the way Super IO chips are driven in the kernel, it seems
> all about writing a driver for each sub-function (uart, hwmon, watchdog
> ... and gpio). Where even very similar chips gets new drivers instead of
> making existing drivers more generic.
>
> I am just observing this and proposing a "similar copy", which i did
> not even write myself. At some point it might be better to rewrite all
> that and make Super I/Os platforms that discover the chip once and then
> register all the many devices they have. Where ressources are properly
> reserved and not that really weird "superio_enter" with
> "request_muxed_region(base, 2, DRVNAME)" which can be found all over
> the place. But hey that allows a very smooth mix of in-tree and
> out-of-tree.
>

A note on that: the kernel community explicitly has zero regard for
out-of-tree code. :)

Bart

> When reviewing this driver i suggest to measure it against i.e. f7188
> or winbond and maybe others.
>
> Say i would manage to make the nct6116d chip supported by f7188, would
> that be acceptable? I would have the "init pattern" i need without
> copying it. But i would add a Nuvoton chip to a Fintek driver ... might
> be the same family ... no clue.
>
> Henning
>
> > Bart
> >
> > > +
> > > +static void __exit nct6116d_gpio_exit(void)
> > > +{
> > > +       platform_device_unregister(nct6116d_gpio_pdev);
> > > +       platform_driver_unregister(&nct6116d_gpio_driver);
> > > +}
> > > +module_init(nct6116d_gpio_init);
> > > +module_exit(nct6116d_gpio_exit);
> > > +
> > > +MODULE_DESCRIPTION("GPIO driver for Nuvoton Super-I/O chips
> > > NCT5104D, NCT6106D, NCT6116D, NCT6122D");
> > > +MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@gmail.com>");
> > > +MODULE_LICENSE("GPL"); --
> > > 2.35.1
> > >
>

Re: [PATCH v3 1/3] gpio: nct6116d: add new driver for several Nuvoton super io chips

[Henning Schild]

Am Tue, 19 Jul 2022 11:39:44 +0200
schrieb Bartosz Golaszewski <brgl@bgdev.pl>:

> On Wed, Jul 13, 2022 at 12:39 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >  
> 
> [snip]
> 
> > > > +
> > > > +static struct platform_driver nct6116d_gpio_driver = {
> > > > +       .driver = {
> > > > +               .name   = KBUILD_MODNAME,
> > > > +       },
> > > > +       .probe          = nct6116d_gpio_probe,
> > > > +};
> > > > +
> > > > +static int __init nct6116d_gpio_init(void)
> > > > +{
> > > > +       struct nct6116d_sio sio;
> > > > +       int err;
> > > > +
> > > > +       if (nct6116d_find(0x2e, &sio) &&
> > > > +           nct6116d_find(0x4e, &sio))
> > > > +               return -ENODEV;
> > > > +
> > > > +       err = platform_driver_register(&nct6116d_gpio_driver);
> > > > +       if (!err) {
> > > > +               err = nct6116d_gpio_device_add(&sio);
> > > > +               if (err)
> > > > +
> > > > platform_driver_unregister(&nct6116d_gpio_driver);
> > > > +       }
> > > > +
> > > > +       return err;
> > > > +}  
> > >
> > > I'm confused - have we not discussed removing this part?  
> >
> > Ah, i thought the problem was the use of subsys_initcall because the
> > comment was under that line.
> >
> > To he honest i do not know all the details since i really just
> > received that driver.
> >
> > What is happening here is that some init code goes and probes well
> > known ports to discover which chip might be connected there.
> > Looking at hwmon, watchdog and similar gpios ... that is the
> > established pattern for Super IOs.  
> 
> I just thought that you would use the simatic driver you mentioned to
> trigger the creation of the devices upon some event. This is what I
> got from your previous email. But that's fine - if there's a repeating
> pattern of doing it this way, then I won't object. I'm not an expert
> on Super IO kernel drivers.

I am not an expert either, just looked at several similar drivers for
inspiration.

In the next round i will propose an "adding the chip" to an existing
driver. That will greatly reduce the amount of code needed. And we do
not have to copy the pattern, we just reuse the existing one.

> > Not DT or ACPI bindings. Something has to load that module to make
> > it init, where it will go and enumerate/poke around.
> >
> > While i could likely put a platform_device_register_simple("driver",
> > 0x42, "chip42") into the simatic platform, then the driver would be
> > pretty useless when not having ACPI (for there Super IOs in
> > general!). There already are hwmon and watchdog drivers for exactly
> > that chip.
> >
> > watchdog/w83627hf_wdt.c
> > hwmon/nct6775*
> >
> > All are based on someone has to "know" and "modprobe", which will
> > cause "finding"
> >
> > The pattern we have here seems all copied from gpio/gpio-f7188x.c,
> > another super similar driver is gpio/gpio-winbond.c (which is param
> > based and not at all reusable in other drivers).
> >
> > Looking at hwmon or watchdog, Nuvoton/Fintek/Windbond are sometimes
> > called a family. But the driver landscape in the kernel is very
> > spread and a lot of copied around code. I did not even look into
> > tty/serial or whatever other functions these Super I/Os offer.
> >
> > Looking at the way Super IO chips are driven in the kernel, it seems
> > all about writing a driver for each sub-function (uart, hwmon,
> > watchdog ... and gpio). Where even very similar chips gets new
> > drivers instead of making existing drivers more generic.
> >
> > I am just observing this and proposing a "similar copy", which i did
> > not even write myself. At some point it might be better to rewrite
> > all that and make Super I/Os platforms that discover the chip once
> > and then register all the many devices they have. Where ressources
> > are properly reserved and not that really weird "superio_enter" with
> > "request_muxed_region(base, 2, DRVNAME)" which can be found all over
> > the place. But hey that allows a very smooth mix of in-tree and
> > out-of-tree.
> >  
> 
> A note on that: the kernel community explicitly has zero regard for
> out-of-tree code. :)

I know ;). One of the reasons i am here with "my" simatic stuff.

Henning

> Bart
> 
> > When reviewing this driver i suggest to measure it against i.e.
> > f7188 or winbond and maybe others.
> >
> > Say i would manage to make the nct6116d chip supported by f7188,
> > would that be acceptable? I would have the "init pattern" i need
> > without copying it. But i would add a Nuvoton chip to a Fintek
> > driver ... might be the same family ... no clue.
> >
> > Henning
> >  
> > > Bart
> > >  
> > > > +
> > > > +static void __exit nct6116d_gpio_exit(void)
> > > > +{
> > > > +       platform_device_unregister(nct6116d_gpio_pdev);
> > > > +       platform_driver_unregister(&nct6116d_gpio_driver);
> > > > +}
> > > > +module_init(nct6116d_gpio_init);
> > > > +module_exit(nct6116d_gpio_exit);
> > > > +
> > > > +MODULE_DESCRIPTION("GPIO driver for Nuvoton Super-I/O chips
> > > > NCT5104D, NCT6106D, NCT6116D, NCT6122D");
> > > > +MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@gmail.com>");
> > > > +MODULE_LICENSE("GPL"); --
> > > > 2.35.1
> > > >  
> >