[PATCH v4 0/2] Add device driver for APU2/APU3 GPIOs

[PATCH v4 0/2] Add device driver for APU2/APU3 GPIOs

[Florian Eckert]

Changes v2:
- Update SPDX short identifier
- Remove gpio-keys-polled device moved to arch/x86/platform
- Fix styling
- Use spinnlock only there where it is useful
- Removed useless output on driver load
- Do bit manipulation later not on IO
- Add additional GPIOs handling mpci2_reset and mpcie3_reset.
- Add name to GPIOs exported via sysfs

Changes v3:
- Add a new platform device for the frontpanel push button.
- Get global variables from the heap
- Fix errors/warnings generated by ./scripts/checkpatch.pl

Changes v4:
gpio-apu.c
- Move bit shifting out of spinnlock
- Change declaration of int to unsigned int
- Remove redundant blank line
- Use dmi table callback
- Remove noise
pcengines-apu-platform.c
- Move platform device to drivers/platform/x86
- Remove needless include
- Add dmi information so that this device is only present on APU2
  APU3 boards from PC Engines

Until now it was not possible to get more information to detect the
MMIO_BASE address from the ACPI subsystem.

Florian Eckert (2):
  gpio: Add driver for PC Engines APU boards
  platform: Add reset button device for PC Engines APU boards

 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-apu.c                       | 299 ++++++++++++++++++++++++++
 drivers/platform/x86/Kconfig                  |  11 +
 drivers/platform/x86/Makefile                 |   1 +
 drivers/platform/x86/pcengines-apu-platform.c | 114 ++++++++++
 6 files changed, 434 insertions(+)
 create mode 100644 drivers/gpio/gpio-apu.c
 create mode 100644 drivers/platform/x86/pcengines-apu-platform.c

-- 
2.11.0

[PATCH v4 1/2] gpio: Add driver for PC Engines APU boards

[Florian Eckert]

Add a new device driver "gpio-apu" which will handle the GPIOs on APU2
and APU3 devices from PC Engines.

APU2 (https://pcengines.ch/schema/apu2c.pdf page 7):
- G32 is "button_reset" connected to the smd-button on the frontpanel
- G50 is "mpcie2_reset" connected to mPCIe2 reset line
- G51 is "mpcie3_reset" connected to mPCIe3 reset line

APU3 (https://pcengines.ch/schema/apu3c.pdf page 7):
- G32 is "button_reset" connected to the smd-button on the frontpanel
- G50 is "mpcie2_reset" connected to mPCIe2 reset line
- G51 is "mpcie3_reset" connected to mPCIe3 reset line
- G33 is "simswap" connected to SIM switch IC to swap the SIM between
  mPCIe2 and mPCIe3 slot

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/gpio/Kconfig    |   8 ++
 drivers/gpio/Makefile   |   1 +
 drivers/gpio/gpio-apu.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 308 insertions(+)
 create mode 100644 drivers/gpio/gpio-apu.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 833a1b51c948..f9e603d5670c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -117,6 +117,14 @@ config GPIO_AMDPT
 	  driver for GPIO functionality on Promontory IOHub
 	  Require ACPI ASL code to enumerate as a platform device.
 
+config GPIO_APU
+	tristate "PC Engines APU2/APU3 GPIO support"
+	depends on X86
+	select GPIO_GENERIC
+	help
+	  Say Y here to support GPIO functionality on APU2/APU3 boards
+	  from PC Engines.
+
 config GPIO_ASPEED
 	tristate "Aspeed GPIO support"
 	depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 671c4477c951..9c27523fb189 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_GPIO_ALTERA)  	+= gpio-altera.o
 obj-$(CONFIG_GPIO_ALTERA_A10SR)	+= gpio-altera-a10sr.o
 obj-$(CONFIG_GPIO_AMD8111)	+= gpio-amd8111.o
 obj-$(CONFIG_GPIO_AMDPT)	+= gpio-amdpt.o
+obj-$(CONFIG_GPIO_APU)		+= gpio-apu.o
 obj-$(CONFIG_GPIO_ARIZONA)	+= gpio-arizona.o
 obj-$(CONFIG_GPIO_ATH79)	+= gpio-ath79.o
 obj-$(CONFIG_GPIO_ASPEED)	+= gpio-aspeed.o
diff --git a/drivers/gpio/gpio-apu.c b/drivers/gpio/gpio-apu.c
new file mode 100644
index 000000000000..cd7788edebab
--- /dev/null
+++ b/drivers/gpio/gpio-apu.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0
+/* PC Engines APU2/APU3 GPIO device driver
+ *
+ * Copyright (C) 2018 Florian Eckert <fe@dev.tdt.de>
+ */
+
+#include <linux/dmi.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define DEVNAME                "gpio-apu"
+
+#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
+#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
+#define APU_GPIO_BIT_RD        16
+#define APU_GPIO_BIT_WR        22
+#define APU_GPIO_BIT_DIR       23
+
+struct apu_gpio_pdata {
+	struct platform_device *pdev;
+	struct gpio_chip *chip;
+	unsigned long *offset;		/* base register offset */
+	void __iomem **addr;		/* remapped iomem addresses */
+	spinlock_t lock;		/* lock register access */
+};
+
+static struct apu_gpio_pdata *apu_gpio;
+
+/* APU2 */
+static unsigned long apu2_gpio_offset[] = {
+	APU_FCH_GPIO_BASE + 89 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 67 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 66 * sizeof(u32),
+};
+static const char * const apu2_gpio_names[] = {
+	"button_reset",
+	"mpcie2_reset",
+	"mpcie3_reset",
+};
+
+/* APU3 */
+static unsigned long apu3_gpio_offset[] = {
+	APU_FCH_GPIO_BASE + 89 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 67 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 66 * sizeof(u32),
+	APU_FCH_GPIO_BASE + 90 * sizeof(u32),
+};
+static const char * const apu3_gpio_names[] = {
+	"button_reset",
+	"mpcie2_reset",
+	"mpcie3_reset",
+	"simswap",
+};
+
+static int gpio_apu_get_dir(struct gpio_chip *chip, unsigned int offset)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ~ioread32(apu_gpio->addr[offset]);
+	val = (val >> APU_GPIO_BIT_DIR) & 1;
+
+	spin_unlock(&apu_gpio->lock);
+
+	return val;
+}
+
+static int gpio_apu_dir_in(struct gpio_chip *chip, unsigned int offset)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	val &= ~BIT(APU_GPIO_BIT_DIR);
+	iowrite32(val, apu_gpio->addr[offset]);
+
+	spin_unlock(&apu_gpio->lock);
+
+	return 0;
+}
+
+static int gpio_apu_dir_out(struct gpio_chip *chip, unsigned int offset,
+		int value)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	val |= BIT(APU_GPIO_BIT_DIR);
+	iowrite32(val, apu_gpio->addr[offset]);
+
+	spin_unlock(&apu_gpio->lock);
+
+	return 0;
+}
+
+static int gpio_apu_get_data(struct gpio_chip *chip, unsigned int offset)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+
+	spin_unlock(&apu_gpio->lock);
+
+	return (val >> APU_GPIO_BIT_RD) & 1;
+}
+
+static void gpio_apu_set_data(struct gpio_chip *chip, unsigned int offset,
+		int value)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	if (value)
+		val |= BIT(APU_GPIO_BIT_WR);
+	else
+		val &= ~BIT(APU_GPIO_BIT_WR);
+	iowrite32(val, apu_gpio->addr[offset]);
+
+	spin_unlock(&apu_gpio->lock);
+}
+
+static const struct dmi_system_id apu2_gpio_dmi_table[] __initconst = {
+	/* PC Engines APU2 with "Legacy" bios < 4.0.8 */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "APU2")
+		}
+	},
+	/* PC Engines APU2 with "Legacy" bios >= 4.0.8 */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu2")
+		}
+	},
+	/* PC Engines APU2 with "Mainline" bios */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu2")
+		}
+	},
+	{}
+}
+MODULE_DEVICE_TABLE(dmi, apu2_gpio_dmi_table);
+
+static const struct dmi_system_id apu3_gpio_dmi_table[] __initconst = {
+	/* PC Engines APU3 with "Legacy" bios < 4.0.8 */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "APU3")
+		}
+	},
+	/* PC Engines APU3 with "Legacy" bios >= 4.0.8 */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu3")
+		}
+	},
+	/* PC Engines APU3 with "Mainline" bios */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu3")
+		}
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(dmi, apu3_gpio_dmi_table);
+
+static struct gpio_chip gpio_apu_chip = {
+	.label = "gpio-apu",
+	.owner = THIS_MODULE,
+	.base = 20,
+	.get_direction = gpio_apu_get_dir,
+	.direction_input = gpio_apu_dir_in,
+	.direction_output = gpio_apu_dir_out,
+	.get = gpio_apu_get_data,
+	.set = gpio_apu_set_data,
+};
+
+static int __init apu_gpio_probe(struct platform_device *pdev)
+{
+	unsigned int i;
+
+	apu_gpio = devm_kzalloc(&pdev->dev, sizeof(*apu_gpio), GFP_KERNEL);
+	if (!apu_gpio)
+		return -ENOMEM;
+
+	apu_gpio->pdev = pdev;
+	apu_gpio->chip = &gpio_apu_chip;
+	spin_lock_init(&apu_gpio->lock);
+
+	if (dmi_check_system(apu3_gpio_dmi_table)) {
+		apu_gpio->addr = devm_kzalloc(&pdev->dev,
+				sizeof(apu3_gpio_offset),
+				GFP_KERNEL);
+
+		if (!apu_gpio->addr)
+			return -ENOMEM;
+
+		apu_gpio->offset = apu3_gpio_offset;
+		apu_gpio->chip->names = apu3_gpio_names;
+		apu_gpio->chip->ngpio = ARRAY_SIZE(apu3_gpio_offset);
+		for (i = 0; i < ARRAY_SIZE(apu3_gpio_offset); i++) {
+			apu_gpio->addr[i] = devm_ioremap(&pdev->dev,
+					apu_gpio->offset[i], sizeof(u32));
+			if (!apu_gpio->addr[i])
+				return -ENOMEM;
+		}
+	} else if (dmi_check_system(apu2_gpio_dmi_table)) {
+		apu_gpio->addr = devm_kzalloc(&pdev->dev,
+				sizeof(apu2_gpio_offset),
+				GFP_KERNEL);
+
+		if (!apu_gpio->addr)
+			return -ENOMEM;
+
+		apu_gpio->offset = apu2_gpio_offset;
+		apu_gpio->chip->names = apu2_gpio_names;
+		apu_gpio->chip->ngpio = ARRAY_SIZE(apu2_gpio_offset);
+		for (i = 0; i < ARRAY_SIZE(apu2_gpio_offset); i++) {
+			apu_gpio->addr[i] = devm_ioremap(&pdev->dev,
+					apu_gpio->offset[i], sizeof(u32));
+			if (!apu_gpio->addr[i])
+				return -ENOMEM;
+		}
+	}
+
+	return devm_gpiochip_add_data(&pdev->dev, apu_gpio->chip, NULL);
+}
+
+static struct platform_driver apu_gpio_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+static int __init apu_gpio_init(void)
+{
+	struct platform_device *pdev;
+	int err;
+
+	if (!(dmi_check_system(apu2_gpio_dmi_table)) &&
+		!(dmi_check_system(apu3_gpio_dmi_table))) {
+		pr_err("No PC Engines board detected\n");
+		return -ENODEV;
+	}
+
+	pdev = platform_device_register_simple(KBUILD_MODNAME, -1, NULL, 0);
+	if (IS_ERR(pdev)) {
+		pr_err("Device allocation failed\n");
+		return PTR_ERR(pdev);
+	}
+
+	err = platform_driver_probe(&apu_gpio_driver, apu_gpio_probe);
+	if (err) {
+		pr_err("Probe platform driver failed\n");
+		platform_device_unregister(pdev);
+	}
+
+	return err;
+}
+
+static void __exit apu_gpio_exit(void)
+{
+	gpiochip_remove(apu_gpio->chip);
+	platform_device_unregister(apu_gpio->pdev);
+	platform_driver_unregister(&apu_gpio_driver);
+}
+
+module_init(apu_gpio_init);
+module_exit(apu_gpio_exit);
+
+MODULE_AUTHOR("Florian Eckert");
+MODULE_DESCRIPTION("PC Engines APU2/3 family GPIO driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:gpio_apu");
-- 
2.11.0

[PATCH v4 2/2] platform: Add reset button device for PC Engines APU boards

[Florian Eckert]

Add a platform/x86 device "gpio-keys-polled" for the frontpanel reset button.
This device uses the gpio-apu driver for APU borads from PC Engines.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/platform/x86/Kconfig                  |  11 +++
 drivers/platform/x86/Makefile                 |   1 +
 drivers/platform/x86/pcengines-apu-platform.c | 114 ++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)
 create mode 100644 drivers/platform/x86/pcengines-apu-platform.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 54f6a40c75c6..5cd27c2174cb 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1288,6 +1288,17 @@ config INTEL_ATOMISP2_PM
 	  To compile this driver as a module, choose M here: the module
 	  will be called intel_atomisp2_pm.
 
+config PCENGINES_APU_PLATFORM
+	bool "PCEngines APU System Support"
+	depends on X86_64 && DMI && GPIOLIB
+	help
+	  This option enables system support for the PCEngines APU platform.
+	  At present this just adds the GPIO reset button platform device on
+	  APU2/APU3 boards.
+
+	  Note: You must still enable the drivers for GPIO and LED support
+	  (GPIO_APU & LEDS_APU) to actually use the LEDs and the GPIOs.
+
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 39ae94135406..f899cc4c6b48 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -96,3 +96,4 @@ obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
 obj-$(CONFIG_INTEL_CHTDC_TI_PWRBTN)	+= intel_chtdc_ti_pwrbtn.o
 obj-$(CONFIG_I2C_MULTI_INSTANTIATE)	+= i2c-multi-instantiate.o
 obj-$(CONFIG_INTEL_ATOMISP2_PM)	+= intel_atomisp2_pm.o
+obj-$(CONFIG_PCENGINES_APU_PLATFORM)	+= pcengines-apu-platform.o
diff --git a/drivers/platform/x86/pcengines-apu-platform.c b/drivers/platform/x86/pcengines-apu-platform.c
new file mode 100644
index 000000000000..3bfbaa93cb11
--- /dev/null
+++ b/drivers/platform/x86/pcengines-apu-platform.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Specific setup for PC-Engines APU2/APU3 devices
+ *
+ * Copyright (C) 2018 Florian Eckert <fe@dev.tdt.de>
+ */
+
+#include <linux/dmi.h>
+#include <linux/gpio_keys.h>
+#include <linux/input.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+static const struct dmi_system_id apu2_gpio_dmi_table[] __initconst = {
+	/* PC Engines APU2 with "Legacy" bios < 4.0.8 */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "APU2")
+		}
+	},
+	/* PC Engines APU2 with "Legacy" bios >= 4.0.8 */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu2")
+		}
+	},
+	/* PC Engines APU2 with "Mainline" bios */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu2")
+		}
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(dmi, apu2_gpio_dmi_table);
+
+static const struct dmi_system_id apu3_gpio_dmi_table[] __initconst = {
+	/* PC Engines APU3 with "Legacy" bios < 4.0.8 */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "APU3")
+		}
+	},
+	/* PC Engines APU3 with "Legacy" bios >= 4.0.8 */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu3")
+		}
+	},
+	/* PC Engines APU3 with "Mainline" bios */
+	{
+		.ident = "apu3",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu3")
+		}
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(dmi, apu3_gpio_dmi_table);
+
+
+static struct gpio_keys_button apu_gpio_buttons[] = {
+	{
+		.code			= KEY_RESTART,
+		.gpio			= 20,
+		.active_low		= 1,
+		.desc			= "Reset button",
+		.type			= EV_KEY,
+		.debounce_interval	= 60,
+	}
+};
+
+static struct gpio_keys_platform_data apu_buttons_data = {
+	.buttons		= apu_gpio_buttons,
+	.nbuttons		= ARRAY_SIZE(apu_gpio_buttons),
+	.poll_interval		= 20,
+};
+
+static struct platform_device apu_button_dev = {
+	.name			= "gpio-keys-polled",
+	.id			= 1,
+	.dev = {
+		.platform_data		= &apu_buttons_data,
+	}
+};
+
+static int __init apu_init(void)
+{
+	if (!(dmi_check_system(apu2_gpio_dmi_table)) &&
+		!(dmi_check_system(apu3_gpio_dmi_table))) {
+		return -ENODEV;
+	}
+
+	return platform_device_register(&apu_button_dev);
+}
+
+static void __exit apu_exit(void)
+{
+	platform_device_unregister(&apu_button_dev);
+}
+
+module_init(apu_init);
+module_exit(apu_exit);
-- 
2.11.0

Re: [PATCH v4 1/2] gpio: Add driver for PC Engines APU boards

[Linus Walleij]

On Thu, Nov 15, 2018 at 2:32 PM Florian Eckert <fe@dev.tdt.de> wrote:

> Add a new device driver "gpio-apu" which will handle the GPIOs on APU2
> and APU3 devices from PC Engines.
>
> APU2 (https://pcengines.ch/schema/apu2c.pdf page 7):
> - G32 is "button_reset" connected to the smd-button on the frontpanel
> - G50 is "mpcie2_reset" connected to mPCIe2 reset line
> - G51 is "mpcie3_reset" connected to mPCIe3 reset line
>
> APU3 (https://pcengines.ch/schema/apu3c.pdf page 7):
> - G32 is "button_reset" connected to the smd-button on the frontpanel
> - G50 is "mpcie2_reset" connected to mPCIe2 reset line
> - G51 is "mpcie3_reset" connected to mPCIe3 reset line
> - G33 is "simswap" connected to SIM switch IC to swap the SIM between
>   mPCIe2 and mPCIe3 slot
>
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>

This is looking better and better! Thanks to everyone helping out
and thanks for your perseverance Florian!

> +config GPIO_APU
> +       tristate "PC Engines APU2/APU3 GPIO support"
> +       depends on X86
> +       select GPIO_GENERIC

Excellent idea.

But it seems you are not using this. You would be using
it if you used bgpio_init() but if I understand correctly this
driver cannot use that because this GPIO is something like
one register per pin, correct?

Let me suggest:

> +struct apu_gpio_pdata {
> +       struct platform_device *pdev;
> +       struct gpio_chip *chip;

Make that a real member of this struct and not a pointer.
I.e. just remove the "*".

> +static struct apu_gpio_pdata *apu_gpio;

Why a static local? It seems you can just pass around
the pointer.

> +static int gpio_apu_get_dir(struct gpio_chip *chip, unsigned int offset)
> +{
> +       u32 val;
> +
> +       spin_lock(&apu_gpio->lock);
> +
> +       val = ~ioread32(apu_gpio->addr[offset]);
> +       val = (val >> APU_GPIO_BIT_DIR) & 1;

I would just:

#include <linux/bits.h>

val = ~ioread32(apu_gpio->addr[offset]);
spin_unlock();

return !!(val & BIT(APU_GPIO_BIT_DIR));

This clamps the value to [0,1] in a nice way.

> +static int gpio_apu_get_data(struct gpio_chip *chip, unsigned int offset)
> +{
> +       u32 val;
> +
> +       spin_lock(&apu_gpio->lock);
> +
> +       val = ioread32(apu_gpio->addr[offset]);
> +
> +       spin_unlock(&apu_gpio->lock);
> +
> +       return (val >> APU_GPIO_BIT_RD) & 1;

return !!(val & BIT(APU_GPIO_BIT_RD));

> +       return devm_gpiochip_add_data(&pdev->dev, apu_gpio->chip, NULL);

Instead of passing NULL pas apu_gpio as the last argument and in
all callbacks you can use:

struct apu_gpio_pdata *apu_gpio = gpiochip_get_data(gc);

To get a pointer to the per-instance state container.

Yours,
Linus Walleij

Re: [PATCH v4 1/2] gpio: Add driver for PC Engines APU boards

[Florian Eckert]

Hello Linus

>> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> 
> This is looking better and better! Thanks to everyone helping out
> and thanks for your perseverance Florian!
> 

I have to thanks for reviewing my driver.
This is the way opensource works.

Thanks for the feedback i will update the driver with your suggestions.

   - Florian