Add device driver for APU2/APU3 GPIOs

PATCH Superseded Florian Eckert 2 participants 5 messages Nov 15, 2018 – Nov 20, 2018

Subsystems (2)

GPIO x86 platform drivers

lore

Share & tools

lore

Florian Eckert

author

2018-11-15 13:31 UTC #0 lore raw

v4 cover Add device driver for APU2/APU3 GPIOs

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

Signature · 1 line

2.11.0

Florian Eckert

author

2018-11-15 13:32 UTC #1 lore raw

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

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 <[email protected]>

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

drivers/gpio/Kconfig +8 -0

index 833a1b51c948..f9e603d5670c 100644
@@ -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

drivers/gpio/Makefile +1 -0

index 671c4477c951..9c27523fb189 100644
@@ -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

drivers/gpio/gpio-apu.c new file +299 -0

299 lines not on this page · raw message

Signature · 1 line

2.11.0

Linus Walleij

2018-11-19 13:46 UTC #2 lore raw

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

Quoted 8 lines · 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

Quoted 8 lines

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 <[email protected]>

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

drivers/gpio/Kconfig config GPIO_AMDPT · line 123

Quoted 4 lines

drivers/gpio/Kconfig +4 -0

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

drivers/gpio/gpio-apu.c line 25

Quoted 3 lines

drivers/gpio/gpio-apu.c new file +3 -0

+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 "*".

Quoted 1 line

+static struct apu_gpio_pdata *apu_gpio;

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

drivers/gpio/gpio-apu.c line 66

Quoted 8 lines

drivers/gpio/gpio-apu.c new file +8 -0

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

drivers/gpio/gpio-apu.c line 114

Quoted 11 lines

drivers/gpio/gpio-apu.c new file +11 -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;

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

Quoted 1 line

+ 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

Florian Eckert

author

2018-11-20 07:14 UTC #3 lore raw

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

Hello Linus

Quoted 5 lines

Quoted 1 line
Signed-off-by Florian Eckert <[email protected]>
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

Florian Eckert

author

2018-11-15 13:32 UTC #4 lore raw

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

In reply to 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 <[email protected]>

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

drivers/platform/x86/Kconfig +11 -0

index 54f6a40c75c6..5cd27c2174cb 100644
@@ -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

drivers/platform/x86/Makefile +1 -0

index 39ae94135406..f899cc4c6b48 100644
@@ -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

drivers/platform/x86/pcengines-apu-platform.c new file +114 -0

114 lines not on this page · raw message

Signature · 1 line

2.11.0