Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs

From: Florian Eckert
Date: Tue Aug 07 2018 - 07:47:27 EST

On 2018-08-03 21:08, Andy Shevchenko wrote:
- APU2/APU3 -> front button reset support
- APU3 -> SIM switch support

Can we see some specification for those platforms?

I think the informations from Christian Lamparter are OK?


+ * GNU General Public License for more details
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <>

SPDX, please!

I have already updated my patch in my git to use this identifier.
// SPDX-License-Identifier: GPL-2.0
This was a hint from Linus Walleji

+ */

+#include <linux/gpio.h>
+#include <linux/gpio_keys.h>

These both looks very strange in here.

On the front of the APU2/APU3 there is a SMD-push-button which is connected to
one of the GPIOs. This is used as a reset button for the system (reboot/factory-reset).
I am also not sure if this is the right place. But for the first review i thought this
will be ok.

The geode, the old board from PC-Engines
added the key gpios to this file
mybe this is the right place too x86/platfrom/apu/apu.c?

+#define APU_FCH_ACPI_MMIO_BASE 0xFED80000

Wow! Can we see ACPI tables for these boards? Care to share (via some
file share service) output of `acpidump -o tables.dat` ?

I have copied this from the leds-apu.c driver which is already upstream.

+#define APU_GPIO_BIT_WRITE 22
+#define APU_GPIO_BIT_READ 16
+#define APU_GPIO_BIT_DIR 23

WR and RD looks shorter,
And please keep them sorted by value.

Ok will fix this.

+#define APU_IOSIZE sizeof(u32)

This is usual for x86 stuff, no need to have a definition, I think.

Ok will fix this.

+static unsigned long apu3_gpio_offset[APU3_NUM_GPIO] = {
+static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL};

+static int gpio_apu_get_dir (struct gpio_chip *chip, unsigned offset)

Style! We do not use space between func and its parameter list.


+ u32 val;
+ spin_lock(&apu_gpio->lock);

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

This is unusual (I mean ~). Better to leave IO alone and do bits
manipulations latter on.


+ val = (val >> APU_GPIO_BIT_DIR) & 1;

Do you need this under spin lock?

No i donÂt.
Will fix this.

+ apu_gpio->chip->ngpio = ARRAY_SIZE(apu2_gpio_offset);
+ for( i = 0; i < ARRAY_SIZE(apu2_gpio_offset); i++) {
+ apu2_gpio_addr[i] = devm_ioremap(&pdev->dev,
+ apu_gpio->offset[i], apu_gpio->iosize);
+ if (!apu2_gpio_addr[i]) {
+ return -ENOMEM;
+ }
+ }
+ }

The above should be part either as callback or driver_data of DMI entries.

I have copied this from the leds-apu.c driver

+ ret = gpiochip_add(&gpio_apu_chip);


What do you mean with "devm_"?

+ if (ret) {

+ pr_err("Adding gpiochip failed\n");

dev_err(), but I consider this message completely useless.

Thanks will remove this too.

+ }

+ register_gpio_keys_polled(-1, 20, ARRAY_SIZE(apu_gpio_keys), apu_gpio_keys);

Not part of this driver. Remove.

As described above should this go to a file "apu.c" in the directory "apu" under

+ return ret;


Consider to use module_platform_driver() and accompanying data
structures and functions.

Ok thanks will update this

Will update my patch with your hints thanks