[PATCH] gpio: add NCT5104D gpio driver

[PATCH] gpio: add NCT5104D gpio driver

[Marc Pignat]

From: Tasanakorn Phaipool <tasanakorn@gmail.com>
Subject: [PATCH] gpio: add NCT5104D gpio driver

Add gpio support for Nuvoton NCT5104d found on pcengines APU, APU2 and
APU3 system boards.

Signed-off-by: Marc Pignat <marc@pignat.org>
---

Dear all,

This driver is needed to use gpios on pcengines APU boards (https://www.pcengines.ch/apu2.htm).
The original driver comes from https://github.com/tasanakorn/linux-gpio-nct5104d.

I (think I) have fixed coding style issues and I have tested it on a APU board.

Could you please have a look at it?

Thank you in advance and best regards



Marc




 drivers/gpio/Kconfig         |  10 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-nct5104d.c | 438 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 449 insertions(+)
 create mode 100644 drivers/gpio/gpio-nct5104d.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d5d3654..dd5ff27 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -602,6 +602,16 @@ config GPIO_IT87
 	  To compile this driver as a module, choose M here: the module will
 	  be called gpio_it87
 
+config GPIO_NCT5104D
+	tristate "NCT5104D GPIO support"
+	help
+	  Say yes here to support GPIO functionality of NCT5104D Super I/O chips.
+
+	  This driver is tested with Nuvoton NCT5104D Super I/O chip.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio_nct5104d
+
 config GPIO_SCH
 	tristate "Intel SCH/TunnelCreek/Centerton/Quark X1000 GPIO"
 	depends on (X86 || COMPILE_TEST) && PCI
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a7676b8..88ea5d8 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_GPIO_MSIC)		+= gpio-msic.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_NCT5104D)		+= gpio-nct5104d.o
 obj-$(CONFIG_GPIO_OCTEON)	+= gpio-octeon.o
 obj-$(CONFIG_GPIO_OMAP)		+= gpio-omap.o
 obj-$(CONFIG_GPIO_PCA953X)	+= gpio-pca953x.o
diff --git a/drivers/gpio/gpio-nct5104d.c b/drivers/gpio/gpio-nct5104d.c
new file mode 100644
index 0000000..127d468
--- /dev/null
+++ b/drivers/gpio/gpio-nct5104d.c
@@ -0,0 +1,438 @@
+/*
+ * GPIO driver for NCT5104D
+ *
+ * Author: Tasanakorn Phaipool <tasanakorn@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+
+#define DRVNAME "gpio-nct5104d"
+
+/*
+ * 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_GPIO1_MODE		0xE0	/* GPIO1 Mode OpenDrain/Push-Pull */
+#define SIO_GPIO2_MODE		0xE1	/* GPIO2 Mode OpenDrain/Push-Pull */
+
+#define SIO_LD_GPIO		0x07	/* GPIO logical device */
+#define SIO_LD_GPIO_MODE	0x0F	/* GPIO mode control 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_NCT5104D_ID			0x1061	/* Chip ID */
+#define SIO_PCENGINES_APU_NCT5104D_ID	0xc452	/* Chip ID */
+
+enum chips { nct5104d };
+
+static const char * const nct5104d_names[] = {
+	"nct5104d"
+};
+
+struct nct5104d_sio {
+	int addr;
+	enum chips type;
+};
+
+struct nct5104d_gpio_bank {
+	struct gpio_chip chip;
+	unsigned int regbase;
+	struct nct5104d_gpio_data *data;
+};
+
+struct nct5104d_gpio_data {
+	struct nct5104d_sio *sio;
+	int nr_bank;
+	struct nct5104d_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, DRVNAME)) {
+		pr_err(DRVNAME "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.
+ */
+
+static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
+				      unsigned int offset);
+static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset);
+static int nct5104d_gpio_direction_out(struct gpio_chip *chip,
+				       unsigned int offset, int value);
+static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			      int value);
+
+#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase)			\
+	{								\
+		.chip = {						\
+			.label            = DRVNAME,			\
+			.owner            = THIS_MODULE,		\
+			.direction_input  = nct5104d_gpio_direction_in,	\
+			.get              = nct5104d_gpio_get,		\
+			.direction_output = nct5104d_gpio_direction_out,\
+			.set              = nct5104d_gpio_set,		\
+			.base             = _base,			\
+			.ngpio            = _ngpio,			\
+			.can_sleep        = true,			\
+		},							\
+		.regbase = _regbase,					\
+	}
+
+#define gpio_dir(base) (base + 0)
+#define gpio_data(base) (base + 1)
+
+static struct nct5104d_gpio_bank nct5104d_gpio_bank[] = {
+	NCT5104D_GPIO_BANK(0,  8, 0xE0),
+	NCT5104D_GPIO_BANK(10, 8, 0xE4)
+};
+
+static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	int err;
+	struct nct5104d_gpio_bank *bank =
+		container_of(chip, struct nct5104d_gpio_bank, chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	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 |= (1 << offset);
+	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
+
+	superio_exit(sio->addr);
+
+	return 0;
+}
+
+static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	int err;
+	struct nct5104d_gpio_bank *bank =
+		container_of(chip, struct nct5104d_gpio_bank, chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	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 & 1 << offset);
+}
+
+static int nct5104d_gpio_direction_out(struct gpio_chip *chip,
+				       unsigned int offset, int value)
+{
+	int err;
+	struct nct5104d_gpio_bank *bank =
+		container_of(chip, struct nct5104d_gpio_bank, chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	u8 dir, data_out;
+
+	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 |= (1 << offset);
+	else
+		data_out &= ~(1 << offset);
+	superio_outb(sio->addr, gpio_data(bank->regbase), data_out);
+
+	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
+	dir &= ~(1 << offset);
+	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
+
+	superio_exit(sio->addr);
+
+	return 0;
+}
+
+static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			      int value)
+{
+	int err;
+	struct nct5104d_gpio_bank *bank =
+		container_of(chip, struct nct5104d_gpio_bank, chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	u8 data_out;
+
+	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 |= (1 << offset);
+	else
+		data_out &= ~(1 << offset);
+	superio_outb(sio->addr, gpio_data(bank->regbase), data_out);
+
+	superio_exit(sio->addr);
+}
+
+/*
+ * Platform device and driver.
+ */
+
+static int nct5104d_gpio_probe(struct platform_device *pdev)
+{
+	int err;
+	int i;
+	struct nct5104d_sio *sio = pdev->dev.platform_data;
+	struct nct5104d_gpio_data *data;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	switch (sio->type) {
+	case nct5104d:
+		data->nr_bank = ARRAY_SIZE(nct5104d_gpio_bank);
+		data->bank = nct5104d_gpio_bank;
+		break;
+	default:
+		return -ENODEV;
+	}
+	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 nct5104d_gpio_bank *bank = &data->bank[i];
+
+		bank->chip.parent = &pdev->dev;
+		bank->data = data;
+
+		err = gpiochip_add(&bank->chip);
+		if (err) {
+			dev_err(&pdev->dev,
+				"Failed to register gpiochip %d: %d\n",
+				i, err);
+			goto err_gpiochip;
+		}
+	}
+
+	return 0;
+
+err_gpiochip:
+	for (i = i - 1; i >= 0; i--) {
+		struct nct5104d_gpio_bank *bank = &data->bank[i];
+
+		gpiochip_remove(&bank->chip);
+	}
+
+	return err;
+}
+
+static int nct5104d_gpio_remove(struct platform_device *pdev)
+{
+	int i;
+	struct nct5104d_gpio_data *data = platform_get_drvdata(pdev);
+
+	for (i = 0; i < data->nr_bank; i++) {
+		struct nct5104d_gpio_bank *bank = &data->bank[i];
+
+		gpiochip_remove(&bank->chip);
+	}
+
+	return 0;
+}
+
+static int __init nct5104d_find(int addr, struct nct5104d_sio *sio)
+{
+	int err;
+	u16 devid;
+	u8 gpio_cfg;
+
+	err = superio_enter(addr);
+	if (err)
+		return err;
+
+	err = -ENODEV;
+
+	devid = superio_inw(addr, SIO_CHIPID);
+	switch (devid) {
+	case SIO_NCT5104D_ID:
+	case SIO_PCENGINES_APU_NCT5104D_ID:
+		sio->type = nct5104d;
+		/* enable GPIO0 and GPIO1 */
+		superio_select(addr, SIO_LD_GPIO);
+		gpio_cfg = superio_inb(addr, SIO_GPIO_ENABLE);
+		gpio_cfg |= 0x03;
+		superio_outb(addr, SIO_GPIO_ENABLE, gpio_cfg);
+		break;
+	default:
+		pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid);
+		goto err;
+	}
+	sio->addr = addr;
+	err = 0;
+
+	pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n",
+		nct5104d_names[sio->type],
+		(unsigned int) addr,
+		(int) superio_inw(addr, SIO_CHIPID));
+
+	superio_select(sio->addr, SIO_LD_GPIO_MODE);
+	superio_outb(sio->addr, SIO_GPIO1_MODE, 0x0);
+	superio_outb(sio->addr, SIO_GPIO2_MODE, 0x0);
+
+err:
+	superio_exit(addr);
+	return err;
+}
+
+static struct platform_device *nct5104d_gpio_pdev;
+
+static int __init
+nct5104d_gpio_device_add(const struct nct5104d_sio *sio)
+{
+	int err;
+
+	nct5104d_gpio_pdev = platform_device_alloc(DRVNAME, -1);
+	if (!nct5104d_gpio_pdev)
+		pr_err(DRVNAME ": Error platform_device_alloc\n");
+	if (!nct5104d_gpio_pdev)
+		return -ENOMEM;
+
+	err = platform_device_add_data(nct5104d_gpio_pdev,
+				       sio, sizeof(*sio));
+	if (err) {
+		pr_err(DRVNAME "Platform data allocation failed\n");
+		goto err;
+	}
+
+	err = platform_device_add(nct5104d_gpio_pdev);
+	if (err) {
+		pr_err(DRVNAME "Device addition failed\n");
+		goto err;
+	}
+	pr_info(DRVNAME ": Device added\n");
+	return 0;
+
+err:
+	platform_device_put(nct5104d_gpio_pdev);
+
+	return err;
+}
+
+/*
+ */
+
+static struct platform_driver nct5104d_gpio_driver = {
+	.driver = {
+		.owner	= THIS_MODULE,
+		.name	= DRVNAME,
+	},
+	.probe		= nct5104d_gpio_probe,
+	.remove		= nct5104d_gpio_remove,
+};
+
+static int __init nct5104d_gpio_init(void)
+{
+	int err;
+	struct nct5104d_sio sio;
+
+	if (nct5104d_find(0x2e, &sio) &&
+	    nct5104d_find(0x4e, &sio))
+		return -ENODEV;
+
+	err = platform_driver_register(&nct5104d_gpio_driver);
+	if (!err) {
+		pr_info(DRVNAME ": platform_driver_register\n");
+		err = nct5104d_gpio_device_add(&sio);
+		if (err)
+			platform_driver_unregister(&nct5104d_gpio_driver);
+	}
+
+	return err;
+}
+subsys_initcall(nct5104d_gpio_init);
+
+static void __exit nct5104d_gpio_exit(void)
+{
+	platform_device_unregister(nct5104d_gpio_pdev);
+	platform_driver_unregister(&nct5104d_gpio_driver);
+}
+module_exit(nct5104d_gpio_exit);
+
+MODULE_DESCRIPTION("GPIO driver for Super-I/O chips NCT5104D");
+MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@gmail.com>");
+MODULE_LICENSE("GPL");
+
-- 
2.7.4

Re: [PATCH] gpio: add NCT5104D gpio driver

[Linus Walleij]

On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote:

I looped in Bjorn Helgaas to have a look at how this thing probes. Please
keep him on CC for future reposts.

I'd like William to look at this driver too, he's the authority on
EISA type cards
and port-mapped GPIO IO. Please keep him on CC too.

Please look a bit at drivers/gpio/gpio-it87.c, and gpio-f7188x.c it seems to me
that this is a simlar or identical chip to one of these. In that case,
you should
augment and extend an existing driver instead of writing a new one.
But I haven't
drilled into the problem and I don't know EISA well enough.

> +++ b/drivers/gpio/gpio-nct5104d.c
> @@ -0,0 +1,438 @@
> +/*
> + * GPIO driver for NCT5104D
> + *
> + * Author: Tasanakorn Phaipool <tasanakorn@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/gpio.h>

Use #include <linux/gpio/driver.h> only

> +#define DRVNAME "gpio-nct5104d"

Usually I prefer to just hardcode the string when used.

> +/*
> + * 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_GPIO1_MODE         0xE0    /* GPIO1 Mode OpenDrain/Push-Pull */
> +#define SIO_GPIO2_MODE         0xE1    /* GPIO2 Mode OpenDrain/Push-Pull */
> +
> +#define SIO_LD_GPIO            0x07    /* GPIO logical device */
> +#define SIO_LD_GPIO_MODE       0x0F    /* GPIO mode control 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_NCT5104D_ID                        0x1061  /* Chip ID */
> +#define SIO_PCENGINES_APU_NCT5104D_ID  0xc452  /* Chip ID */
> +
> +enum chips { nct5104d };
> +
> +static const char * const nct5104d_names[] = {
> +       "nct5104d"
> +};

This enum and name array seems a bit overkill? Are you already planning
to add support for a bunch of other chips?

> +/*
> + * GPIO chip.
> + */
> +
> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
> +                                     unsigned int offset);
> +static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset);
> +static int nct5104d_gpio_direction_out(struct gpio_chip *chip,
> +                                      unsigned int offset, int value);
> +static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +                             int value);

Do you really have to forward-declare all this?

I strongly prefer if you move code around so as to avoid it.

> +#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase)                    \
> +       {                                                               \
> +               .chip = {                                               \
> +                       .label            = DRVNAME,                    \
> +                       .owner            = THIS_MODULE,                \
> +                       .direction_input  = nct5104d_gpio_direction_in, \
> +                       .get              = nct5104d_gpio_get,          \
> +                       .direction_output = nct5104d_gpio_direction_out,\
> +                       .set              = nct5104d_gpio_set,          \
> +                       .base             = _base,                      \
> +                       .ngpio            = _ngpio,                     \
> +                       .can_sleep        = true,                       \
> +               },                                                      \
> +               .regbase = _regbase,                                    \
> +       }

Please also implement .get_direction(), it is very helpful to have.

> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
> +                                     unsigned int offset)
> +{
> +       int err;
> +       struct nct5104d_gpio_bank *bank =
> +               container_of(chip, struct nct5104d_gpio_bank, chip);
> +       struct nct5104d_sio *sio = bank->data->sio;
> +       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 |= (1 << offset);

Please use:
#include <linux/bitops.h>

dir |= BIT(offset);

This applies to all sites using this pattern.

> +               err = gpiochip_add(&bank->chip);

Can you use devm_gpiochip_add_data() (you can pass NULL as data)
so you do not need the .remove() call below at all?

> +err_gpiochip:
> +       for (i = i - 1; i >= 0; i--) {
> +               struct nct5104d_gpio_bank *bank = &data->bank[i];
> +
> +               gpiochip_remove(&bank->chip);

Then conversely this needs devm_gpiochip_remove().

> +static int nct5104d_gpio_remove(struct platform_device *pdev)

And this could go away.

> +static int __init nct5104d_find(int addr, struct nct5104d_sio *sio)
> +{
> +       int err;
> +       u16 devid;
> +       u8 gpio_cfg;
> +
> +       err = superio_enter(addr);
> +       if (err)
> +               return err;
> +
> +       err = -ENODEV;
> +
> +       devid = superio_inw(addr, SIO_CHIPID);
> +       switch (devid) {
> +       case SIO_NCT5104D_ID:
> +       case SIO_PCENGINES_APU_NCT5104D_ID:
> +               sio->type = nct5104d;
> +               /* enable GPIO0 and GPIO1 */
> +               superio_select(addr, SIO_LD_GPIO);
> +               gpio_cfg = superio_inb(addr, SIO_GPIO_ENABLE);
> +               gpio_cfg |= 0x03;
> +               superio_outb(addr, SIO_GPIO_ENABLE, gpio_cfg);
> +               break;
> +       default:
> +               pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid);
> +               goto err;
> +       }
> +       sio->addr = addr;
> +       err = 0;
> +
> +       pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n",
> +               nct5104d_names[sio->type],
> +               (unsigned int) addr,
> +               (int) superio_inw(addr, SIO_CHIPID));
> +
> +       superio_select(sio->addr, SIO_LD_GPIO_MODE);
> +       superio_outb(sio->addr, SIO_GPIO1_MODE, 0x0);
> +       superio_outb(sio->addr, SIO_GPIO2_MODE, 0x0);
> +
> +err:
> +       superio_exit(addr);
> +       return err;
> +}
> +
> +static struct platform_device *nct5104d_gpio_pdev;
> +
> +static int __init
> +nct5104d_gpio_device_add(const struct nct5104d_sio *sio)
> +{
> +       int err;
> +
> +       nct5104d_gpio_pdev = platform_device_alloc(DRVNAME, -1);
> +       if (!nct5104d_gpio_pdev)
> +               pr_err(DRVNAME ": Error platform_device_alloc\n");
> +       if (!nct5104d_gpio_pdev)
> +               return -ENOMEM;
> +
> +       err = platform_device_add_data(nct5104d_gpio_pdev,
> +                                      sio, sizeof(*sio));
> +       if (err) {
> +               pr_err(DRVNAME "Platform data allocation failed\n");
> +               goto err;
> +       }
> +
> +       err = platform_device_add(nct5104d_gpio_pdev);
> +       if (err) {
> +               pr_err(DRVNAME "Device addition failed\n");
> +               goto err;
> +       }
> +       pr_info(DRVNAME ": Device added\n");
> +       return 0;
> +
> +err:
> +       platform_device_put(nct5104d_gpio_pdev);
> +
> +       return err;
> +}
> +
> +/*
> + */
> +
> +static struct platform_driver nct5104d_gpio_driver = {
> +       .driver = {
> +               .owner  = THIS_MODULE,
> +               .name   = DRVNAME,
> +       },
> +       .probe          = nct5104d_gpio_probe,
> +       .remove         = nct5104d_gpio_remove,
> +};
> +
> +static int __init nct5104d_gpio_init(void)
> +{
> +       int err;
> +       struct nct5104d_sio sio;
> +
> +       if (nct5104d_find(0x2e, &sio) &&
> +           nct5104d_find(0x4e, &sio))
> +               return -ENODEV;
> +
> +       err = platform_driver_register(&nct5104d_gpio_driver);
> +       if (!err) {
> +               pr_info(DRVNAME ": platform_driver_register\n");
> +               err = nct5104d_gpio_device_add(&sio);
> +               if (err)
> +                       platform_driver_unregister(&nct5104d_gpio_driver);
> +       }
> +
> +       return err;
> +}
> +subsys_initcall(nct5104d_gpio_init);
> +
> +static void __exit nct5104d_gpio_exit(void)
> +{
> +       platform_device_unregister(nct5104d_gpio_pdev);
> +       platform_driver_unregister(&nct5104d_gpio_driver);
> +}
> +module_exit(nct5104d_gpio_exit);

I'm not thrilled by this "plug-and-play" that seems very far from autodetection.

I need help reviewing this from someone who knows how to deal with
this kind of platform drivers on port-mapped I/O.

Yours,
Linus Walleij

Re: [PATCH] gpio: add NCT5104D gpio driver

[William Breathitt Gray]

On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote:
>On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote:
>
>I looped in Bjorn Helgaas to have a look at how this thing probes. Please
>keep him on CC for future reposts.
>
>I'd like William to look at this driver too, he's the authority on
>EISA type cards
>and port-mapped GPIO IO. Please keep him on CC too.
>
>Please look a bit at drivers/gpio/gpio-it87.c, and gpio-f7188x.c it seems to me
>that this is a simlar or identical chip to one of these. In that case,
>you should
>augment and extend an existing driver instead of writing a new one.
>But I haven't
>drilled into the problem and I don't know EISA well enough.
>

Hi Linus,

This looks like a Super I/O chip (correct me if I'm mistaken Marc).
Super I/O chips typically communicate via the LPC bus (which is software
compatible with the ISA bus) hence the ioport operations.

The F7188x and IT87xx drivers handle respective Super I/O chips as well.
It appears that a lot of code duplication is occurring between these
three drivers; this makes sense since the functionality would be similar
between these devices.

>> +++ b/drivers/gpio/gpio-nct5104d.c
>> @@ -0,0 +1,438 @@
>> +/*
>> + * GPIO driver for NCT5104D
>> + *
>> + * Author: Tasanakorn Phaipool <tasanakorn@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/gpio.h>
>
>Use #include <linux/gpio/driver.h> only
>
>> +#define DRVNAME "gpio-nct5104d"
>
>Usually I prefer to just hardcode the string when used.
>
>> +/*
>> + * 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_GPIO1_MODE         0xE0    /* GPIO1 Mode OpenDrain/Push-Pull */
>> +#define SIO_GPIO2_MODE         0xE1    /* GPIO2 Mode OpenDrain/Push-Pull */
>> +
>> +#define SIO_LD_GPIO            0x07    /* GPIO logical device */
>> +#define SIO_LD_GPIO_MODE       0x0F    /* GPIO mode control 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_NCT5104D_ID                        0x1061  /* Chip ID */
>> +#define SIO_PCENGINES_APU_NCT5104D_ID  0xc452  /* Chip ID */
>> +
>> +enum chips { nct5104d };
>> +
>> +static const char * const nct5104d_names[] = {
>> +       "nct5104d"
>> +};
>
>This enum and name array seems a bit overkill? Are you already planning
>to add support for a bunch of other chips?
>
>> +/*
>> + * GPIO chip.
>> + */
>> +
>> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
>> +                                     unsigned int offset);
>> +static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset);
>> +static int nct5104d_gpio_direction_out(struct gpio_chip *chip,
>> +                                      unsigned int offset, int value);
>> +static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset,
>> +                             int value);
>
>Do you really have to forward-declare all this?
>
>I strongly prefer if you move code around so as to avoid it.
>
>> +#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase)                    \
>> +       {                                                               \
>> +               .chip = {                                               \
>> +                       .label            = DRVNAME,                    \
>> +                       .owner            = THIS_MODULE,                \
>> +                       .direction_input  = nct5104d_gpio_direction_in, \
>> +                       .get              = nct5104d_gpio_get,          \
>> +                       .direction_output = nct5104d_gpio_direction_out,\
>> +                       .set              = nct5104d_gpio_set,          \
>> +                       .base             = _base,                      \
>> +                       .ngpio            = _ngpio,                     \
>> +                       .can_sleep        = true,                       \
>> +               },                                                      \
>> +               .regbase = _regbase,                                    \
>> +       }
>
>Please also implement .get_direction(), it is very helpful to have.
>
>> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
>> +                                     unsigned int offset)
>> +{
>> +       int err;
>> +       struct nct5104d_gpio_bank *bank =
>> +               container_of(chip, struct nct5104d_gpio_bank, chip);
>> +       struct nct5104d_sio *sio = bank->data->sio;
>> +       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 |= (1 << offset);
>
>Please use:
>#include <linux/bitops.h>
>
>dir |= BIT(offset);
>
>This applies to all sites using this pattern.
>
>> +               err = gpiochip_add(&bank->chip);
>
>Can you use devm_gpiochip_add_data() (you can pass NULL as data)
>so you do not need the .remove() call below at all?
>
>> +err_gpiochip:
>> +       for (i = i - 1; i >= 0; i--) {
>> +               struct nct5104d_gpio_bank *bank = &data->bank[i];
>> +
>> +               gpiochip_remove(&bank->chip);
>
>Then conversely this needs devm_gpiochip_remove().
>
>> +static int nct5104d_gpio_remove(struct platform_device *pdev)
>
>And this could go away.
>
>> +static int __init nct5104d_find(int addr, struct nct5104d_sio *sio)
>> +{
>> +       int err;
>> +       u16 devid;
>> +       u8 gpio_cfg;
>> +
>> +       err = superio_enter(addr);
>> +       if (err)
>> +               return err;
>> +
>> +       err = -ENODEV;
>> +
>> +       devid = superio_inw(addr, SIO_CHIPID);
>> +       switch (devid) {
>> +       case SIO_NCT5104D_ID:
>> +       case SIO_PCENGINES_APU_NCT5104D_ID:
>> +               sio->type = nct5104d;
>> +               /* enable GPIO0 and GPIO1 */
>> +               superio_select(addr, SIO_LD_GPIO);
>> +               gpio_cfg = superio_inb(addr, SIO_GPIO_ENABLE);
>> +               gpio_cfg |= 0x03;
>> +               superio_outb(addr, SIO_GPIO_ENABLE, gpio_cfg);
>> +               break;
>> +       default:
>> +               pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid);
>> +               goto err;
>> +       }
>> +       sio->addr = addr;
>> +       err = 0;
>> +
>> +       pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n",
>> +               nct5104d_names[sio->type],
>> +               (unsigned int) addr,
>> +               (int) superio_inw(addr, SIO_CHIPID));
>> +
>> +       superio_select(sio->addr, SIO_LD_GPIO_MODE);
>> +       superio_outb(sio->addr, SIO_GPIO1_MODE, 0x0);
>> +       superio_outb(sio->addr, SIO_GPIO2_MODE, 0x0);
>> +
>> +err:
>> +       superio_exit(addr);
>> +       return err;
>> +}
>> +
>> +static struct platform_device *nct5104d_gpio_pdev;
>> +
>> +static int __init
>> +nct5104d_gpio_device_add(const struct nct5104d_sio *sio)
>> +{
>> +       int err;
>> +
>> +       nct5104d_gpio_pdev = platform_device_alloc(DRVNAME, -1);
>> +       if (!nct5104d_gpio_pdev)
>> +               pr_err(DRVNAME ": Error platform_device_alloc\n");
>> +       if (!nct5104d_gpio_pdev)
>> +               return -ENOMEM;
>> +
>> +       err = platform_device_add_data(nct5104d_gpio_pdev,
>> +                                      sio, sizeof(*sio));
>> +       if (err) {
>> +               pr_err(DRVNAME "Platform data allocation failed\n");
>> +               goto err;
>> +       }
>> +
>> +       err = platform_device_add(nct5104d_gpio_pdev);
>> +       if (err) {
>> +               pr_err(DRVNAME "Device addition failed\n");
>> +               goto err;
>> +       }
>> +       pr_info(DRVNAME ": Device added\n");
>> +       return 0;
>> +
>> +err:
>> +       platform_device_put(nct5104d_gpio_pdev);
>> +
>> +       return err;
>> +}
>> +
>> +/*
>> + */
>> +
>> +static struct platform_driver nct5104d_gpio_driver = {
>> +       .driver = {
>> +               .owner  = THIS_MODULE,
>> +               .name   = DRVNAME,
>> +       },
>> +       .probe          = nct5104d_gpio_probe,
>> +       .remove         = nct5104d_gpio_remove,
>> +};
>> +
>> +static int __init nct5104d_gpio_init(void)
>> +{
>> +       int err;
>> +       struct nct5104d_sio sio;
>> +
>> +       if (nct5104d_find(0x2e, &sio) &&
>> +           nct5104d_find(0x4e, &sio))
>> +               return -ENODEV;
>> +
>> +       err = platform_driver_register(&nct5104d_gpio_driver);
>> +       if (!err) {
>> +               pr_info(DRVNAME ": platform_driver_register\n");
>> +               err = nct5104d_gpio_device_add(&sio);
>> +               if (err)
>> +                       platform_driver_unregister(&nct5104d_gpio_driver);
>> +       }
>> +
>> +       return err;
>> +}
>> +subsys_initcall(nct5104d_gpio_init);
>> +
>> +static void __exit nct5104d_gpio_exit(void)
>> +{
>> +       platform_device_unregister(nct5104d_gpio_pdev);
>> +       platform_driver_unregister(&nct5104d_gpio_driver);
>> +}
>> +module_exit(nct5104d_gpio_exit);
>

Marc,

I recommend utilizing the isa_driver since you are interfacing a super
I/O chip. This should simplify your __init and __exit code by removing
the need for all those platform_driver setup calls you make; instead you
would simply call isa_register_driver.

Check out the respective __init, __exit, probe, and remove code in
drivers/watchdog/ebc-c384_wdt.c file a simple example of how to use the
isa_driver calls. If you need more specific help, let me know and I'll
go into more detail.

>I'm not thrilled by this "plug-and-play" that seems very far from autodetection.
>

Linus,

I believe the best course of action forward would be the development of
a "super_io_driver" to handle the detection of these type of devices.
Super I/O chips typically have a ioport base address at 0x2E; this is
why you see all of these Super I/O chip drivers probing that address.

Since device IDs can be found at the standard Super I/O base address, it
would be useful to have a super_io_driver structure with an id_table
member filled with possible device IDs of devices supported by the
respective driver. I imagine that this would enable automatic driver
loads and device probes such as we have with the PCI bus driver, where a
super_io_driver structure can be populated by the driver author and then
passed to a super_io_register_driver call.

Furthermore, a Super I/O driver could provide the common read/write
operations we see across the existing Super I/O drivers, thus
eliminating the code duplication (superio_outb, superio_inw, etc. become
standardized).

This plan would obviously require more time to get right, so if you
decide to merge this particular driver now, I suggest at least using the
isa_driver idioms for now to ease the transition to a possible future
super_io_driver.

William Breathitt Gray

>I need help reviewing this from someone who knows how to deal with
>this kind of platform drivers on port-mapped I/O.
>
>Yours,
>Linus Walleij

Re: [PATCH] gpio: add NCT5104D gpio driver

[Marc Pignat]

Dear all,

Thank you for your feedback!

On 02/22/2017 10:04 PM, William Breathitt Gray wrote:
> On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote:
>> On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote:
...
>> Please look a bit at drivers/gpio/gpio-it87.c, and gpio-f7188x.c it seems to me
>> that this is a simlar or identical chip to one of these. In that case,

Looking at gpio-f7188x.c is probably the best feedback.

In fact the patch I proposed is just a copy+modifications of an old version of 
this file.

So someone (who has put his name in the author field) has copied the gpio-f7188x.c,
changed something like 20 lines (excluding identifiers renaming) and pushed it to
github. I took it from github, tested it and passed it through checkpatch.pl and
proposed it for inclusion into the mainline kernel.

>> you should
>> augment and extend an existing driver instead of writing a new one.

Sure!

>> But I haven't
...
> 
> This plan would obviously require more time to get right, so if you
> decide to merge this particular driver now, I suggest at least using the
> isa_driver idioms for now to ease the transition to a possible future
> super_io_driver.

super_io_driver seems a good idea, in the meantime, I will try to merge the nct5104
code into gpio-f7188x.c, without the isa_driver idiom.

Best regards



Marc

[PATCH,v2 0/1] gpio: add NCT5104D gpio driver

[Marc Pignat]

Hi all,

Here is a v2, with fixes or at least responses to your comments.

On 02/22/2017 10:04 PM, William Breathitt Gray wrote:
> On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote:
>> On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote:
>>
...
>> Please look a bit at drivers/gpio/gpio-it87.c, and gpio-f7188x.c it seems to me
>> that this is a simlar or identical chip to one of these. In that case,
>> you should

gpio-f7188x is really similar, but I failed to add nct5104d into it, the
registers are not at the same addresses, input/output is inverted and open
drain management is different.

So I restarted the gpio-nct5104d.c driver using gpio-f7188x.c.

...
...

>>
>> This enum and name array seems a bit overkill? Are you already planning
>> to add support for a bunch of other chips?

No, removed.

>>
>>> +/*
>>> + * GPIO chip.
>>> + */
>>> +
>>> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
>>> +                                     unsigned int offset);
>>> +static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset);
>>> +static int nct5104d_gpio_direction_out(struct gpio_chip *chip,
>>> +                                      unsigned int offset, int value);
>>> +static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset,
>>> +                             int value);
>>
>> Do you really have to forward-declare all this?

No, but was like that in gpio-f7188x.c, fixed.

>>
>> I strongly prefer if you move code around so as to avoid it.
>>
>>> +#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase)                    \
>>> +       {                                                               \
>>> +               .chip = {                                               \
>>> +                       .label            = DRVNAME,                    \
>>> +                       .owner            = THIS_MODULE,                \
>>> +                       .direction_input  = nct5104d_gpio_direction_in, \
>>> +                       .get              = nct5104d_gpio_get,          \
>>> +                       .direction_output = nct5104d_gpio_direction_out,\
>>> +                       .set              = nct5104d_gpio_set,          \
>>> +                       .base             = _base,                      \
>>> +                       .ngpio            = _ngpio,                     \
>>> +                       .can_sleep        = true,                       \
>>> +               },                                                      \
>>> +               .regbase = _regbase,                                    \
>>> +       }
>>
>> Please also implement .get_direction(), it is very helpful to have.

done

>>
>>> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
>>> +                                     unsigned int offset)
>>> +{
>>> +       int err;
>>> +       struct nct5104d_gpio_bank *bank =
>>> +               container_of(chip, struct nct5104d_gpio_bank, chip);
>>> +       struct nct5104d_sio *sio = bank->data->sio;
>>> +       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 |= (1 << offset);
>>
>> Please use:
>> #include <linux/bitops.h>
>>
>> dir |= BIT(offset);
>>
>> This applies to all sites using this pattern.

done

>>
>>> +               err = gpiochip_add(&bank->chip);
>>
>> Can you use devm_gpiochip_add_data() (you can pass NULL as data)
>> so you do not need the .remove() call below at all?


done

>>
>>> +err_gpiochip:
>>> +       for (i = i - 1; i >= 0; i--) {
>>> +               struct nct5104d_gpio_bank *bank = &data->bank[i];
>>> +
>>> +               gpiochip_remove(&bank->chip);
>>
>> Then conversely this needs devm_gpiochip_remove().
>>
>>> +static int nct5104d_gpio_remove(struct platform_device *pdev)
>>
>> And this could go away.

done

...

>>
> 
> Marc,
> 
> I recommend utilizing the isa_driver since you are interfacing a super
> I/O chip. This should simplify your __init and __exit code by removing
> the need for all those platform_driver setup calls you make; instead you
> would simply call isa_register_driver.
> 
> Check out the respective __init, __exit, probe, and remove code in
> drivers/watchdog/ebc-c384_wdt.c file a simple example of how to use the
> isa_driver calls. If you need more specific help, let me know and I'll
> go into more detail.
> 
>> I'm not thrilled by this "plug-and-play" that seems very far from autodetection.

Sure ISA driver seems a little more clean, but it seems recent kernel are
not compiled with CONFIG_ISA_BUS_API. 

Best regards


Marc

Re: [PATCH,v2 0/1] gpio: add NCT5104D gpio driver

[Linus Walleij]

On Tue, Feb 28, 2017 at 9:14 AM Marc Pignat <marc@pignat.org> wrote:
> On 02/22/2017 10:04 PM, William Breathitt Gray wrote:
> > On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote:
> >> On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote:

> >> I'm not thrilled by this "plug-and-play" that seems very far from autodetection.
>
> Sure ISA driver seems a little more clean, but it seems recent kernel are
> not compiled with CONFIG_ISA_BUS_API.

Has this changed these days so we can get this driver in using
the proper ISA abstractions?

Yours,
Linus Walleij

Re: [PATCH,v2 0/1] gpio: add NCT5104D gpio driver

[William Breathitt Gray]

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

On Thu, Mar 25, 2021 at 09:23:17AM +0100, Linus Walleij wrote:
> On Tue, Feb 28, 2017 at 9:14 AM Marc Pignat <marc@pignat.org> wrote:
> > On 02/22/2017 10:04 PM, William Breathitt Gray wrote:
> > > On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote:
> > >> On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote:
> 
> > >> I'm not thrilled by this "plug-and-play" that seems very far from autodetection.
> >
> > Sure ISA driver seems a little more clean, but it seems recent kernel are
> > not compiled with CONFIG_ISA_BUS_API.
> 
> Has this changed these days so we can get this driver in using
> the proper ISA abstractions?
> 
> Yours,
> Linus Walleij

It's been some years so I don't quite remember how we had things back
then, but at least nowadays we typically have a "select ISA_BUS_API"
line for the driver's Kconfig block. I suspect this is no longer an
issue then because CONFIG_ISA_BUS_API will be compiled along with the
drivers that select it.

William Breathitt Gray

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

[PATCH,v2 1/1] gpio: add NCT5104D gpio driver

[Marc Pignat]

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0504307..e899792 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -620,6 +620,14 @@ config GPIO_IT87
 	  To compile this driver as a module, choose M here: the module will
 	  be called gpio_it87
 
+config GPIO_NCT5104D
+	tristate "Nuvotron NCT5104D GPIO support"
+	help
+	  Enables GPIO support for the Nuvotron NCT5104D  Super-I/O chip.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called nct5104d-gpio.
+
 config GPIO_SCH
 	tristate "Intel SCH/TunnelCreek/Centerton/Quark X1000 GPIO"
 	depends on (X86 || COMPILE_TEST) && PCI
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index becb96c..4b35109 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_GPIO_MSIC)		+= gpio-msic.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_NCT5104D)	+= gpio-nct5104d.o
 obj-$(CONFIG_GPIO_OCTEON)	+= gpio-octeon.o
 obj-$(CONFIG_GPIO_OMAP)		+= gpio-omap.o
 obj-$(CONFIG_GPIO_PCA953X)	+= gpio-pca953x.o
diff --git a/drivers/gpio/gpio-nct5104d.c b/drivers/gpio/gpio-nct5104d.c
new file mode 100644
index 0000000..d36518a
--- /dev/null
+++ b/drivers/gpio/gpio-nct5104d.c
@@ -0,0 +1,432 @@
+/*
+ * GPIO driver for Nuvoton Super-I/O NCT5104D
+ *
+ * Copyright (C) 2017 Marc Pignat
+ *
+ * Inspired from gpio-nct5104d.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/gpio/driver.h>
+#include <linux/bitops.h>
+
+#define DRVNAME "gpio-nct5104d"
+
+/*
+ * Super-I/O registers
+ */
+#define SIO_LDSEL		0x07	/* Logical device select */
+#define SIO_DEVID		0x20	/* Device ID (2 bytes) */
+
+#define SIO_LD_GPIO		0x07	/* GPIO logical device */
+#define SIO_GPIO_ENABLE		0x30	/* GPIO enable */
+#define SIO_LD_GPIO_MODE	0x0F	/* GPIO mode control device */
+#define SIO_GPIO1_MODE		0xE0	/* GPIO1 Mode OpenDrain/Push-Pull */
+#define SIO_GPIO2_MODE		0xE1	/* GPIO2 Mode OpenDrain/Push-Pull */
+
+#define SIO_UNLOCK_KEY		0x87	/* Key to enable Super-I/O */
+#define SIO_LOCK_KEY		0xAA	/* Key to disable Super-I/O */
+
+#define SIO_NCT5104D_ID		0x1061
+#define SIO_PCENGINES_APU_ID	0xc452
+
+struct nct5104d_sio {
+	int addr;
+};
+
+struct nct5104d_gpio_bank {
+	struct gpio_chip chip;
+	unsigned int regbase;
+	unsigned int reg_od;
+	struct nct5104d_gpio_data *data;
+};
+
+struct nct5104d_gpio_data {
+	struct nct5104d_sio *sio;
+	int nr_bank;
+	struct nct5104d_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, DRVNAME)) {
+		pr_err(DRVNAME "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);
+}
+
+#define gpio_dir(base) (base + 0)
+#define gpio_data(base) (base + 1)
+
+static int nct5104d_gpio_get_direction(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	int err;
+
+	struct nct5104d_gpio_bank *bank = gpiochip_get_data(chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	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);
+
+	return !!(dir & BIT(offset));
+}
+
+static int nct5104d_gpio_direction_in(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	int err;
+	struct nct5104d_gpio_bank *bank = gpiochip_get_data(chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	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 nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	int err;
+	struct nct5104d_gpio_bank *bank = gpiochip_get_data(chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	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 nct5104d_gpio_direction_out(struct gpio_chip *chip,
+				     unsigned int offset, int value)
+{
+	int err;
+	struct nct5104d_gpio_bank *bank = gpiochip_get_data(chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	u8 dir, data_out;
+
+	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 nct5104d_gpio_set(struct gpio_chip *chip,
+			      unsigned int offset, int value)
+{
+	int err;
+	struct nct5104d_gpio_bank *bank = gpiochip_get_data(chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	u8 data_out;
+
+	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);
+}
+
+static int nct5104d_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+				    unsigned long config)
+{
+	int err;
+	enum pin_config_param param = pinconf_to_config_param(config);
+	struct nct5104d_gpio_bank *bank = gpiochip_get_data(chip);
+	struct nct5104d_sio *sio = bank->data->sio;
+	u8 data;
+
+	if (param != PIN_CONFIG_DRIVE_OPEN_DRAIN &&
+	    param != PIN_CONFIG_DRIVE_PUSH_PULL)
+		return -ENOTSUPP;
+
+	err = superio_enter(sio->addr);
+	if (err)
+		return err;
+	superio_select(sio->addr, SIO_LD_GPIO_MODE);
+
+	data = superio_inb(sio->addr, bank->reg_od);
+	if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN)
+		data |= BIT(offset);
+	else
+		data &= ~BIT(offset);
+	superio_outb(sio->addr, bank->reg_od, data);
+
+	superio_exit(sio->addr);
+	return 0;
+}
+
+#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase, _reg_od)		\
+	{								\
+		.chip = {						\
+			.label            = DRVNAME,			\
+			.owner            = THIS_MODULE,		\
+			.get_direction    = nct5104d_gpio_get_direction,\
+			.direction_input  = nct5104d_gpio_direction_in,	\
+			.get              = nct5104d_gpio_get,		\
+			.direction_output = nct5104d_gpio_direction_out,\
+			.set              = nct5104d_gpio_set,		\
+			.set_config       = nct5104d_gpio_set_config,	\
+			.base             = _base,			\
+			.ngpio            = _ngpio,			\
+			.can_sleep        = true,			\
+		},							\
+		.regbase = _regbase,					\
+		.reg_od = _reg_od,					\
+	}
+
+static struct nct5104d_gpio_bank nct5104d_gpio_bank[] = {
+	NCT5104D_GPIO_BANK(0,  8, 0xE0, SIO_GPIO1_MODE),
+	NCT5104D_GPIO_BANK(10, 8, 0xE4, SIO_GPIO2_MODE)
+};
+
+/*
+ * Platform device and driver.
+ */
+
+static int nct5104d_gpio_probe(struct platform_device *pdev)
+{
+	int err;
+	int i;
+	struct nct5104d_sio *sio = dev_get_platdata(&pdev->dev);
+	struct nct5104d_gpio_data *data;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->nr_bank = ARRAY_SIZE(nct5104d_gpio_bank);
+	data->bank = nct5104d_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 nct5104d_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) {
+			dev_err(&pdev->dev,
+				"Failed to register gpiochip %d: %d\n",
+				i, err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int __init nct5104d_find(int addr, struct nct5104d_sio *sio)
+{
+	int err;
+	u16 devid;
+	u8 gpio_cfg;
+
+	err = superio_enter(addr);
+	if (err)
+		return err;
+
+	devid = superio_inw(addr, SIO_DEVID);
+
+	if (devid != SIO_NCT5104D_ID &&
+	    devid != SIO_PCENGINES_APU_ID) {
+		err = -ENODEV;
+		goto err;
+	}
+
+	sio->addr = addr;
+	err = 0;
+
+	pr_info(DRVNAME ": Found at %#x\n", (unsigned int) addr);
+
+	superio_select(addr, SIO_LD_GPIO);
+
+	/* Enable GPIO0-1 */
+	gpio_cfg = superio_inb(addr, SIO_GPIO_ENABLE);
+	gpio_cfg |= 0x03;
+	superio_outb(addr, SIO_GPIO_ENABLE, gpio_cfg);
+
+err:
+	superio_exit(addr);
+	return err;
+}
+
+static struct platform_device *nct5104d_gpio_pdev;
+
+static int __init
+nct5104d_gpio_device_add(const struct nct5104d_sio *sio)
+{
+	int err;
+
+	nct5104d_gpio_pdev = platform_device_alloc(DRVNAME, -1);
+	if (!nct5104d_gpio_pdev)
+		return -ENOMEM;
+
+	err = platform_device_add_data(nct5104d_gpio_pdev,
+				       sio, sizeof(*sio));
+	if (err) {
+		pr_err(DRVNAME "Platform data allocation failed\n");
+		goto err;
+	}
+
+	err = platform_device_add(nct5104d_gpio_pdev);
+	if (err) {
+		pr_err(DRVNAME "Device addition failed\n");
+		goto err;
+	}
+
+	return 0;
+
+err:
+	platform_device_put(nct5104d_gpio_pdev);
+
+	return err;
+}
+
+/*
+ * Try to match a supported Fintek device by reading the (hard-wired)
+ * configuration I/O ports. If available, then register both the platform
+ * device and driver to support the GPIOs.
+ */
+
+static struct platform_driver nct5104d_gpio_driver = {
+	.driver = {
+		.name	= DRVNAME,
+	},
+	.probe		= nct5104d_gpio_probe,
+};
+
+static int __init nct5104d_gpio_init(void)
+{
+	int err;
+	struct nct5104d_sio sio;
+
+	if (nct5104d_find(0x2e, &sio) &&
+	    nct5104d_find(0x4e, &sio))
+		return -ENODEV;
+
+	err = platform_driver_register(&nct5104d_gpio_driver);
+	if (!err) {
+		err = nct5104d_gpio_device_add(&sio);
+		if (err)
+			platform_driver_unregister(&nct5104d_gpio_driver);
+	}
+
+	return err;
+}
+
+static void __exit nct5104d_gpio_exit(void)
+{
+	platform_device_unregister(nct5104d_gpio_pdev);
+	platform_driver_unregister(&nct5104d_gpio_driver);
+}
+subsys_initcall(nct5104d_gpio_init);
+module_exit(nct5104d_gpio_exit);
+
+MODULE_DESCRIPTION("GPIO driver for Super-I/O chip Nuvoton NCT5104D");
+MODULE_AUTHOR("Marc Pignat <marc@pignat.org>");
+MODULE_LICENSE("GPL");