linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
@ 2018-08-01 11:12 Florian Eckert
  2018-08-02 21:30 ` Linus Walleij
  2018-08-03 19:08 ` Andy Shevchenko
  0 siblings, 2 replies; 10+ messages in thread
From: Florian Eckert @ 2018-08-01 11:12 UTC (permalink / raw)
  To: linux-gpio; +Cc: linus.walleij, linux-kernel, fe

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

- APU2/APU3 -> front button reset support
- APU3 -> SIM switch support

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

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 71c0ab46f216..9eb8977ba2e5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -117,6 +117,16 @@ 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.
+	  - APU2/APU3 -> front button reset support
+	  - APU3 -> SIM switch support
+
 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 1324c8f966a7..feea4effcf29 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..18171c13917a
--- /dev/null
+++ b/drivers/gpio/gpio-apu.c
@@ -0,0 +1,344 @@
+/* PC Engines APU2/APU3 GPIO device driver
+ *
+ * Copyright (C) 2018 Florian Eckert <fe@dev.tdt.de>
+ *
+ * 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
+ *
+ * This program is distributed in the hope that it will be useful
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * 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 <http://www.gnu.org/licenses/>
+ */
+
+#include <linux/dmi.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio_keys.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_WRITE     22
+#define APU_GPIO_BIT_READ      16
+#define APU_GPIO_BIT_DIR       23
+#define APU_IOSIZE             sizeof(u32)
+
+#define APU2_NUM_GPIO          1
+#define APU3_NUM_GPIO          2
+
+struct apu_gpio_pdata {
+	struct platform_device *pdev;
+	struct gpio_chip *chip;
+	unsigned long *offset;
+	void __iomem **addr;
+	int iosize; /* for devm_ioremap() */
+	spinlock_t lock;
+};
+
+static struct apu_gpio_pdata *apu_gpio;
+static struct platform_device *keydev;
+
+/* APU2 */
+static unsigned long apu2_gpio_offset[APU2_NUM_GPIO] = {
+	APU_FCH_GPIO_BASE + 89 * APU_IOSIZE, //KEY
+};
+static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL};
+
+/* APU3 */
+static unsigned long apu3_gpio_offset[APU3_NUM_GPIO] = {
+	APU_FCH_GPIO_BASE + 89 * APU_IOSIZE, //KEY
+	APU_FCH_GPIO_BASE + 90 * APU_IOSIZE, //SIM
+};
+static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL};
+
+static int gpio_apu_get_dir (struct gpio_chip *chip, unsigned 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 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 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 offset)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	val = (val >> APU_GPIO_BIT_READ) & 1;
+
+	spin_unlock(&apu_gpio->lock);
+
+	return val;
+}
+
+static void gpio_apu_set_data (struct gpio_chip *chip, unsigned offset, int value)
+{
+	u32 val;
+
+	spin_lock(&apu_gpio->lock);
+
+	val = ioread32(apu_gpio->addr[offset]);
+	if (value)
+		val |= BIT(APU_GPIO_BIT_WRITE);
+	else
+		val &= ~BIT(APU_GPIO_BIT_WRITE);
+	iowrite32(val, apu_gpio->addr[offset]);
+
+	spin_unlock(&apu_gpio->lock);
+}
+
+static const struct dmi_system_id apu_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")
+		}
+	},
+	/* PC Engines APU3 with "Legancy" bios >= 4.0.7 */
+	{
+		.ident = "apu2",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "APU3")
+		}
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
+
+static struct gpio_chip gpio_apu_chip = {
+	.label = "gpio-apu",
+	.owner = THIS_MODULE,
+	.base = -1,
+	.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 struct gpio_keys_button apu_gpio_keys[] = {
+	{
+		.desc           = "Reset button",
+		.type           = EV_KEY,
+		.code           = KEY_RESTART,
+		.debounce_interval = 60,
+		.gpio           = 510,
+		.active_low     = 1,
+	},
+};
+
+static void register_gpio_keys_polled(int id, unsigned poll_interval,
+				      unsigned nbuttons,
+				      struct gpio_keys_button *buttons)
+{
+	struct gpio_keys_platform_data pdata = { };
+	int err;
+
+	keydev = platform_device_alloc("gpio-keys-polled", id);
+	if (!keydev) {
+		printk(KERN_ERR "Failed to allocate gpio-keys platform device\n");
+		return;
+	}
+
+	pdata.poll_interval = poll_interval;
+	pdata.nbuttons = nbuttons;
+	pdata.buttons = buttons;
+
+	err = platform_device_add_data(keydev, &pdata, sizeof(pdata));
+	if (err) {
+		dev_err(&keydev->dev, "failed to add platform data to key driver (%d)", err);
+		goto err_put_pdev;
+	}
+
+	err = platform_device_add(keydev);
+	if (err) {
+		dev_err(&keydev->dev, "failed to register key platform device (%d)", err);
+		goto err_put_pdev;
+	}
+
+	return;
+
+err_put_pdev:
+	platform_device_put(keydev);
+	keydev = NULL;
+}
+
+static int __init apu_gpio_probe(struct platform_device *pdev)
+{
+	int i;
+	int ret;
+
+	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_match(DMI_PRODUCT_NAME, "APU3")) {
+		apu_gpio->offset = apu3_gpio_offset;
+		apu_gpio->addr = apu3_gpio_addr;
+		apu_gpio->iosize = APU_IOSIZE;
+		apu_gpio->chip->ngpio = ARRAY_SIZE(apu3_gpio_offset);
+		for( i = 0; i < ARRAY_SIZE(apu3_gpio_offset); i++) {
+			apu3_gpio_addr[i] = devm_ioremap(&pdev->dev,
+					apu_gpio->offset[i], apu_gpio->iosize);
+			if (!apu3_gpio_addr[i]) {
+				return -ENOMEM;
+			}
+		}
+	} else if (dmi_match(DMI_BOARD_NAME, "APU2") ||
+		   dmi_match(DMI_BOARD_NAME, "apu2") ||
+		   dmi_match(DMI_BOARD_NAME, "PC Engines apu2")) {
+		apu_gpio->offset = apu2_gpio_offset;
+		apu_gpio->addr = apu2_gpio_addr;
+		apu_gpio->iosize = APU_IOSIZE;
+		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;
+			}
+		}
+	}
+
+	ret = gpiochip_add(&gpio_apu_chip);
+	if (ret) {
+		pr_err("Adding gpiochip failed\n");
+	}
+
+	register_gpio_keys_polled(-1, 20, ARRAY_SIZE(apu_gpio_keys), apu_gpio_keys);
+
+	return ret;
+}
+
+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_match(DMI_SYS_VENDOR, "PC Engines")) {
+		pr_err("No PC Engines board detected\n");
+		return -ENODEV;
+	}
+	if (!(dmi_match(DMI_PRODUCT_NAME, "APU") ||
+	      dmi_match(DMI_PRODUCT_NAME, "APU2") ||
+	      dmi_match(DMI_PRODUCT_NAME, "APU3") ||
+	      dmi_match(DMI_PRODUCT_NAME, "apu2") ||
+	      dmi_match(DMI_PRODUCT_NAME, "PC Engines apu2"))) {
+		pr_err("Unknown PC Engines board: %s\n",
+				dmi_get_system_info(DMI_PRODUCT_NAME));
+		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);
+	}
+
+	pr_info ("%s: APU2/3 GPIO driver module loaded\n", DEVNAME);
+
+	return err;
+}
+
+static void __exit apu_gpio_exit(void)
+{
+	platform_device_unregister(keydev);
+	gpiochip_remove(apu_gpio->chip);
+	platform_device_unregister(apu_gpio->pdev);
+	platform_driver_unregister(&apu_gpio_driver);
+	pr_info ("%s: APU2/3 GPIO driver module unloaded\n", DEVNAME);
+}
+
+module_init(apu_gpio_init);
+module_exit(apu_gpio_exit);
+
+MODULE_AUTHOR("Florian Eckert <fe@dev.tdt.de>");
+MODULE_DESCRIPTION("PC Engines APU2/APU3 family GPIO driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:gpio_apu");
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-01 11:12 [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs Florian Eckert
@ 2018-08-02 21:30 ` Linus Walleij
  2018-08-03 16:08   ` Joe Perches
  2018-08-04 18:22   ` Christian Lamparter
  2018-08-03 19:08 ` Andy Shevchenko
  1 sibling, 2 replies; 10+ messages in thread
From: Linus Walleij @ 2018-08-02 21:30 UTC (permalink / raw)
  To: fe, Andy Shevchenko, Mika Westerberg
  Cc: open list:GPIO SUBSYSTEM, linux-kernel

On Wed, Aug 1, 2018 at 1:12 PM Florian Eckert <fe@dev.tdt.de> wrote:

> Add a new device driver "gpio-apu" which will now handle the GPIOs on
> APU2 and APU3 devices from PC Engines.
>
> - APU2/APU3 -> front button reset support
> - APU3 -> SIM switch support
>
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>

Hi Florian, thanks for the patch!

I looped in Andy Schevchenko and Mika Westerberg who are authorities on
x86 platform drivers in general and GPIO and pin control in particular so they
can help out with the review.

I'm a bit confused whether these things are really GPIOs or just
switches but since they can change direction they seem to be GPIOs.

I don't know if the placement of the driver is right either: I was under
the impression that this type of drivers should live under
drivers/platform/x86 but Andy can surely tell.

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

Thanks for activating the helpers! This makes everything simpler.
But your driver should (and can) use them too, just make sure to
call bgpio_init() with the right address offsets and everything should
just work.

> +       help
> +         Say Y here to support GPIO functionality on APU2/APU3 boards
> +         from PC Engines.
> +         - APU2/APU3 -> front button reset support
> +         - APU3 -> SIM switch support

I don't know about the approach to bundle GPIO keys into a GPIO
driver, but Andy will tell.

> +/* PC Engines APU2/APU3 GPIO device driver
> + *
> + * Copyright (C) 2018 Florian Eckert <fe@dev.tdt.de>
> + *
> + * 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
> + *
> + * This program is distributed in the hope that it will be useful
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * 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 <http://www.gnu.org/licenses/>

Instead of the big bulk of license text, use the simple oneline SPDX identifier.
In this case, at the top of the file:

// SPDX-License-Identifier: GPL-2.0

See Documentation/process/license-rules.rst for full info.

> +#include <linux/dmi.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>

Use just <linux/driver.h> for GPIO drivers.

> +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000

Hm this looks hardcoded. Isn't ACPI giving you the base address?
Not that I understand ACPI, but...

> +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
> +#define APU_GPIO_BIT_WRITE     22
> +#define APU_GPIO_BIT_READ      16
> +#define APU_GPIO_BIT_DIR       23
> +#define APU_IOSIZE             sizeof(u32)
> +
> +#define APU2_NUM_GPIO          1
> +#define APU3_NUM_GPIO          2
> +
> +struct apu_gpio_pdata {
> +       struct platform_device *pdev;
> +       struct gpio_chip *chip;
> +       unsigned long *offset;
> +       void __iomem **addr;
> +       int iosize; /* for devm_ioremap() */

Can you keep this in a local variable? It doesn't seem like
it needs to be in the state container.

> +       spinlock_t lock;

I think checkpatch now mandates that you put in a comment
about what this lock is locking.

> +static struct apu_gpio_pdata *apu_gpio;
> +static struct platform_device *keydev;

I don't know a about static singletons, but I guess this would
only ever probe once, so it's probably OK, but Andy knows
better.

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

Some comment on why this needs to be inversed?

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

I would write:

#include <linux/bitops.h>

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

or even fold into the read:

val = !!(~ioreaad() & BIT(APU_GPIO_BIT_DIR));

But this and all the other GPIO accessors go away if you use GPIO_GENERIC
properly with bgpio_init() and the right addresses and flags.

> +static struct gpio_chip gpio_apu_chip = {
> +       .label = "gpio-apu",
> +       .owner = THIS_MODULE,
> +       .base = -1,
> +       .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,
> +};

So instead of a static GPIO chip leave the functions undefined
and call bgpio_init() that will set them up for MMIO GPIO.

> +static struct gpio_keys_button apu_gpio_keys[] = {
> +       {
> +               .desc           = "Reset button",
> +               .type           = EV_KEY,
> +               .code           = KEY_RESTART,
> +               .debounce_interval = 60,
> +               .gpio           = 510,
> +               .active_low     = 1,
> +       },
> +};

Andy has to tell whether he likes this idea.

> +       apu_gpio->pdev = pdev;
> +       apu_gpio->chip = &gpio_apu_chip;
> +       spin_lock_init(&apu_gpio->lock);
> +
> +       if (dmi_match(DMI_PRODUCT_NAME, "APU3")) {
> +               apu_gpio->offset = apu3_gpio_offset;
> +               apu_gpio->addr = apu3_gpio_addr;
> +               apu_gpio->iosize = APU_IOSIZE;
> +               apu_gpio->chip->ngpio = ARRAY_SIZE(apu3_gpio_offset);
> +               for( i = 0; i < ARRAY_SIZE(apu3_gpio_offset); i++) {
> +                       apu3_gpio_addr[i] = devm_ioremap(&pdev->dev,
> +                                       apu_gpio->offset[i], apu_gpio->iosize);
> +                       if (!apu3_gpio_addr[i]) {
> +                               return -ENOMEM;
> +                       }
> +               }
> +       } else if (dmi_match(DMI_BOARD_NAME, "APU2") ||
> +                  dmi_match(DMI_BOARD_NAME, "apu2") ||
> +                  dmi_match(DMI_BOARD_NAME, "PC Engines apu2")) {
> +               apu_gpio->offset = apu2_gpio_offset;
> +               apu_gpio->addr = apu2_gpio_addr;
> +               apu_gpio->iosize = APU_IOSIZE;
> +               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;
> +                       }
> +               }
> +       }

So here, after figuring out the base addresses and all, call bgpio_init()
before adding the gpio_chip, which will set up the desired access
functions.

> +       ret = gpiochip_add(&gpio_apu_chip);
> +       if (ret) {
> +               pr_err("Adding gpiochip failed\n");
> +       }
> +
> +       register_gpio_keys_polled(-1, 20, ARRAY_SIZE(apu_gpio_keys), apu_gpio_keys);

I don't know if this is a good idea?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-02 21:30 ` Linus Walleij
@ 2018-08-03 16:08   ` Joe Perches
  2018-08-04 18:22   ` Christian Lamparter
  1 sibling, 0 replies; 10+ messages in thread
From: Joe Perches @ 2018-08-03 16:08 UTC (permalink / raw)
  To: Linus Walleij, fe, Andy Shevchenko, Mika Westerberg
  Cc: open list:GPIO SUBSYSTEM, linux-kernel

On Thu, 2018-08-02 at 23:30 +0200, Linus Walleij wrote:
> On Wed, Aug 1, 2018 at 1:12 PM Florian Eckert <fe@dev.tdt.de> wrote:
[]
> > +       spinlock_t lock;
> 
> I think checkpatch now mandates that you put in a comment
> about what this lock is locking.

Please remember that checkpatch doesn't mandate anything.

Documentation/process/4.Coding.rst does have:

   Certain things should always be commented.  Uses of memory barriers should
   be accompanied by a line explaining why the barrier is necessary.  The
   locking rules for data structures generally need to be explained somewhere.

So there should be some comment somewhere in the code
for the spinlock.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-01 11:12 [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs Florian Eckert
  2018-08-02 21:30 ` Linus Walleij
@ 2018-08-03 19:08 ` Andy Shevchenko
  2018-08-07 11:47   ` Florian Eckert
  2018-08-24 10:56   ` Piotr Król
  1 sibling, 2 replies; 10+ messages in thread
From: Andy Shevchenko @ 2018-08-03 19:08 UTC (permalink / raw)
  To: Florian Eckert
  Cc: open list:GPIO SUBSYSTEM, Linus Walleij, Linux Kernel Mailing List

On Wed, Aug 1, 2018 at 2:12 PM, Florian Eckert <fe@dev.tdt.de> wrote:

Thanks for the patch, my comments below.

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

> - APU2/APU3 -> front button reset support
> - APU3 -> SIM switch support

Good.
Can we see some specification for those platforms?

> +/* PC Engines APU2/APU3 GPIO device driver
> + *
> + * Copyright (C) 2018 Florian Eckert <fe@dev.tdt.de>

> + *
> + * 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
> + *
> + * This program is distributed in the hope that it will be useful
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * 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 <http://www.gnu.org/licenses/>

SPDX, please!

> + */

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

These both looks very strange in here.

> +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
> +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)

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

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

> +#define APU_IOSIZE             sizeof(u32)

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

> +struct apu_gpio_pdata {
> +       struct platform_device *pdev;
> +       struct gpio_chip *chip;
> +       unsigned long *offset;
> +       void __iomem **addr;
> +       int iosize; /* for devm_ioremap() */
> +       spinlock_t lock;
> +};
> +
> +static struct apu_gpio_pdata *apu_gpio;
> +static struct platform_device *keydev;
> +
> +/* APU2 */
> +static unsigned long apu2_gpio_offset[APU2_NUM_GPIO] = {
> +       APU_FCH_GPIO_BASE + 89 * APU_IOSIZE, //KEY
> +};
> +static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL};
> +
> +/* APU3 */
> +static unsigned long apu3_gpio_offset[APU3_NUM_GPIO] = {
> +       APU_FCH_GPIO_BASE + 89 * APU_IOSIZE, //KEY
> +       APU_FCH_GPIO_BASE + 90 * APU_IOSIZE, //SIM
> +};
> +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?

> +
> +       spin_unlock(&apu_gpio->lock);
> +
> +       return val;
> +}


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

Similar comments as per _get_dir().

> +static struct gpio_keys_button apu_gpio_keys[] = {

> +};
> +
> +static void register_gpio_keys_polled(int id, unsigned poll_interval,
> +                                     unsigned nbuttons,
> +                                     struct gpio_keys_button *buttons)
> +{

> +}

Above must not be here.

> +       if (dmi_match(DMI_PRODUCT_NAME, "APU3")) {
> +               apu_gpio->offset = apu3_gpio_offset;
> +               apu_gpio->addr = apu3_gpio_addr;
> +               apu_gpio->iosize = APU_IOSIZE;
> +               apu_gpio->chip->ngpio = ARRAY_SIZE(apu3_gpio_offset);
> +               for( i = 0; i < ARRAY_SIZE(apu3_gpio_offset); i++) {
> +                       apu3_gpio_addr[i] = devm_ioremap(&pdev->dev,
> +                                       apu_gpio->offset[i], apu_gpio->iosize);
> +                       if (!apu3_gpio_addr[i]) {
> +                               return -ENOMEM;
> +                       }
> +               }
> +       } else if (dmi_match(DMI_BOARD_NAME, "APU2") ||
> +                  dmi_match(DMI_BOARD_NAME, "apu2") ||
> +                  dmi_match(DMI_BOARD_NAME, "PC Engines apu2")) {
> +               apu_gpio->offset = apu2_gpio_offset;
> +               apu_gpio->addr = apu2_gpio_addr;
> +               apu_gpio->iosize = APU_IOSIZE;
> +               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.

> +       ret = gpiochip_add(&gpio_apu_chip);

devm_

> +       if (ret) {

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

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

> +       }

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

Not part of this driver. Remove.

> +       return ret;
> +}

> +module_init(apu_gpio_init);
> +module_exit(apu_gpio_exit);

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

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-02 21:30 ` Linus Walleij
  2018-08-03 16:08   ` Joe Perches
@ 2018-08-04 18:22   ` Christian Lamparter
  2018-08-07 11:18     ` Florian Eckert
  1 sibling, 1 reply; 10+ messages in thread
From: Christian Lamparter @ 2018-08-04 18:22 UTC (permalink / raw)
  To: Linus Walleij
  Cc: fe, Andy Shevchenko, Mika Westerberg, open list:GPIO SUBSYSTEM,
	linux-kernel

On Thursday, August 2, 2018 11:30:00 PM CEST Linus Walleij wrote:
> On Wed, Aug 1, 2018 at 1:12 PM Florian Eckert <fe@dev.tdt.de> wrote:
> 
> > Add a new device driver "gpio-apu" which will now handle the GPIOs on
> > APU2 and APU3 devices from PC Engines.
> >
> > - APU2/APU3 -> front button reset support
> > - APU3 -> SIM switch support
> >
> > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> 
> Hi Florian, thanks for the patch!

There have been multiple attempts at upstreaming a gpio/pinctrl driver for
the different APU SoC by now. The last attempt I remember took place on 
this ML too: <https://lkml.org/lkml/2017/12/20/824>

But sadly most attempts stalled, because there is already a leds-apu driver 
<https://github.com/torvalds/linux/blob/master/drivers/leds/leds-apu.c>
(since the gpio-driver was never accepted) that will do in a pinch for the
LEDs (which most people care about).

> I looped in Andy Schevchenko and Mika Westerberg who are authorities on
> x86 platform drivers in general and GPIO and pin control in particular
> so they can help out with the review.
Intel helping AMD. That's nice to see ;-)

> I'm a bit confused whether these things are really GPIOs or just
> switches but since they can change direction they seem to be GPIOs.

Yes, it's a real gpio/pinctrl in AMDs Tech docs.
<https://support.amd.com/TechDocs/52740_16h_Models_30h-3Fh_BKDG.pdf>

Some of the pins are muxed. I.e GPIO167-170 can either be a GPIO or
used for HD audio.
"The HD audio controller supports up to four codecs with one AZ_SDIN
pin from each codec. The four AZ_SDIN pins are multiplexed with
GPIO167-170 (GPIOxA7-GPIOxAA). If a particular pin is to be used for
HD audio functionality". There's a full table in "3.26.12.1 GPIO Registers". 




As for the APUs. The vendor (PC Engines) happily provides
PDFs and schematics for their boards:
<https://www.pcengines.ch/pdf/apu1.pdf>
<https://www.pcengines.ch/schema/apu1c.pdf>
<https://www.pcengines.ch/pdf/apu2.pdf>
<http://pcengines.ch/schema/apu2c.pdf>

So, it's possible to repurpose several test points as additional
GPIOs and more.

Note2:
On both boards there is also a dedicated GPIO pin header J19, but
these pins are controlled by the SuperIO Nuvoton NCT5104D.

> > +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.
> > +         - APU2/APU3 -> front button reset support
> > +         - APU3 -> SIM switch support
Well, by design this driver will sort of clash with the leds-apu driver. 
 
Regards,
Christian



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-04 18:22   ` Christian Lamparter
@ 2018-08-07 11:18     ` Florian Eckert
  2018-08-07 19:18       ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Florian Eckert @ 2018-08-07 11:18 UTC (permalink / raw)
  To: Christian Lamparter
  Cc: Linus Walleij, Andy Shevchenko, Mika Westerberg,
	open list:GPIO SUBSYSTEM, linux-kernel

Hello Andy

I think this are the information you want to have.

On 2018-08-04 20:22, Christian Lamparter wrote:
> As for the APUs. The vendor (PC Engines) happily provides
> PDFs and schematics for their boards:
> <https://www.pcengines.ch/pdf/apu1.pdf>
> <https://www.pcengines.ch/schema/apu1c.pdf>
> <https://www.pcengines.ch/pdf/apu2.pdf>
> <http://pcengines.ch/schema/apu2c.pdf>





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-03 19:08 ` Andy Shevchenko
@ 2018-08-07 11:47   ` Florian Eckert
  2018-08-24 10:56   ` Piotr Król
  1 sibling, 0 replies; 10+ messages in thread
From: Florian Eckert @ 2018-08-07 11:47 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, Linus Walleij, Linux Kernel Mailing List

On 2018-08-03 21:08, Andy Shevchenko wrote:
>> - APU2/APU3 -> front button reset support
>> - APU3 -> SIM switch support
> 
> Good.
> Can we see some specification for those platforms?
> 

I think the informations from Christian Lamparter are OK?

<https://www.pcengines.ch/pdf/apu1.pdf>
<https://www.pcengines.ch/schema/apu1c.pdf>
<https://www.pcengines.ch/pdf/apu2.pdf>
<http://pcengines.ch/schema/apu2c.pdf>

>> + * 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 
>> <http://www.gnu.org/licenses/>
> 
> 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
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/platform/geode/alix.c#n47
mybe this is the right place too x86/platfrom/apu/apu.c?

>> +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
>> +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
> 
> 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.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c#n43

> 
>> +#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] = {
>> +       APU_FCH_GPIO_BASE + 89 * APU_IOSIZE, //KEY
>> +       APU_FCH_GPIO_BASE + 90 * APU_IOSIZE, //SIM
>> +};
>> +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.
> 

OK

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

OK

> 
>> +       val = (val >> APU_GPIO_BIT_DIR) & 1;
> 
> Do you need this under spin lock?
> 

No i don´t.
Will fix this.
Thanks

>> +               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
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c#n226

> 
>> +       ret = gpiochip_add(&gpio_apu_chip);
> 
> devm_
> 

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
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/platform
?

>> +       return ret;
>> +}
> 
>> +module_init(apu_gpio_init);
>> +module_exit(apu_gpio_exit);
> 
> 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-07 11:18     ` Florian Eckert
@ 2018-08-07 19:18       ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2018-08-07 19:18 UTC (permalink / raw)
  To: Florian Eckert, Christian Lamparter
  Cc: Linus Walleij, Mika Westerberg, open list:GPIO SUBSYSTEM, linux-kernel

On Tue, 2018-08-07 at 13:18 +0200, Florian Eckert wrote:
> Hello Andy
> 
> I think this are the information you want to have.
> 

I was rather asking for something like TRM.

By schematics, I meant a simplified GPIO buffers and pin control
explained on hardware level.

Below has nothing to do with either, unfortunately.

> On 2018-08-04 20:22, Christian Lamparter wrote:
> > As for the APUs. The vendor (PC Engines) happily provides
> > PDFs and schematics for their boards:
> > <https://www.pcengines.ch/pdf/apu1.pdf>
> > <https://www.pcengines.ch/schema/apu1c.pdf>
> > <https://www.pcengines.ch/pdf/apu2.pdf>
> > <http://pcengines.ch/schema/apu2c.pdf>

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-03 19:08 ` Andy Shevchenko
  2018-08-07 11:47   ` Florian Eckert
@ 2018-08-24 10:56   ` Piotr Król
  2018-08-30  5:54     ` Florian Eckert
  1 sibling, 1 reply; 10+ messages in thread
From: Piotr Król @ 2018-08-24 10:56 UTC (permalink / raw)
  To: Andy Shevchenko, Florian Eckert
  Cc: open list:GPIO SUBSYSTEM, Linus Walleij, Linux Kernel Mailing List

On 08/03/2018 09:08 PM, Andy Shevchenko wrote:

Hi Andy,

(...)

>> +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
>> +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
> 
> Wow! Can we see ACPI tables for these boards? Care to share (via some
> file share service) output of `acpidump -o tables.dat` ?

Please find acpidump [1]. FYI I'm PC Engines firmware maintainer
(firmware is coreboot based), so I can fix required things. I'm pretty
sure that ACPI tables are not in best shape.

[1] https://pastebin.com/2neC1Sri

Best Regards,
--
Piotr Król
Embedded Systems Consultant
https://3mdeb.com | @3mdeb_com



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
  2018-08-24 10:56   ` Piotr Król
@ 2018-08-30  5:54     ` Florian Eckert
  0 siblings, 0 replies; 10+ messages in thread
From: Florian Eckert @ 2018-08-30  5:54 UTC (permalink / raw)
  To: Piotr Król
  Cc: Andy Shevchenko, open list:GPIO SUBSYSTEM, Linus Walleij,
	Linux Kernel Mailing List

On 2018-08-24 12:56, Piotr Król wrote:
> On 08/03/2018 09:08 PM, Andy Shevchenko wrote:
> 
> Hi Andy,
> 
> (...)
> 
>>> +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
>>> +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
>> 
>> Wow! Can we see ACPI tables for these boards? Care to share (via some
>> file share service) output of `acpidump -o tables.dat` ?
> 
> Please find acpidump [1]. FYI I'm PC Engines firmware maintainer
> (firmware is coreboot based), so I can fix required things. I'm pretty
> sure that ACPI tables are not in best shape.
> 

Thanks for the dump,

By the way I have seen that the driver
https://github.com/torvalds/linux/blob/master/drivers/gpio/gpio-amdpt.c
is using the acpi to get the mmio address. I think this is the way to go
to get the acpi mmio address in the gpio-apu driver as well.

What is the right "acpi_device_id" for the APU3?
https://github.com/torvalds/linux/blob/master/drivers/gpio/gpio-amdpt.c#L146


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-08-30  5:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-01 11:12 [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs Florian Eckert
2018-08-02 21:30 ` Linus Walleij
2018-08-03 16:08   ` Joe Perches
2018-08-04 18:22   ` Christian Lamparter
2018-08-07 11:18     ` Florian Eckert
2018-08-07 19:18       ` Andy Shevchenko
2018-08-03 19:08 ` Andy Shevchenko
2018-08-07 11:47   ` Florian Eckert
2018-08-24 10:56   ` Piotr Król
2018-08-30  5:54     ` Florian Eckert

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).