Re: [PATCH v4 1/2] gpio: Add driver for PC Engines APU boards
Hello Linus Signed-off-by: Florian Eckert 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
Re: [PATCH v4 1/2] gpio: Add driver for PC Engines APU boards
Hello Linus Signed-off-by: Florian Eckert 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
Re: [PATCH v4 1/2] gpio: Add driver for PC Engines APU boards
On Thu, Nov 15, 2018 at 2:32 PM Florian Eckert 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
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(_gpio->lock);
> +
> + val = ~ioread32(apu_gpio->addr[offset]);
> + val = (val >> APU_GPIO_BIT_DIR) & 1;
I would just:
#include
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(_gpio->lock);
> +
> + val = ioread32(apu_gpio->addr[offset]);
> +
> + spin_unlock(_gpio->lock);
> +
> + return (val >> APU_GPIO_BIT_RD) & 1;
return !!(val & BIT(APU_GPIO_BIT_RD));
> + return devm_gpiochip_add_data(>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
On Thu, Nov 15, 2018 at 2:32 PM Florian Eckert 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
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(_gpio->lock);
> +
> + val = ~ioread32(apu_gpio->addr[offset]);
> + val = (val >> APU_GPIO_BIT_DIR) & 1;
I would just:
#include
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(_gpio->lock);
> +
> + val = ioread32(apu_gpio->addr[offset]);
> +
> + spin_unlock(_gpio->lock);
> +
> + return (val >> APU_GPIO_BIT_RD) & 1;
return !!(val & BIT(APU_GPIO_BIT_RD));
> + return devm_gpiochip_add_data(>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
[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
---
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 ..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEVNAME"gpio-apu"
+
+#define APU_FCH_ACPI_MMIO_BASE 0xFED8
+#define APU_FCH_GPIO_BASE (APU_FCH_ACPI_MMIO_BASE + 0x1500)
+#define APU_GPIO_BIT_RD16
+#define APU_GPIO_BIT_WR22
+#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(_gpio->lock);
+
+ val = ~ioread32(apu_gpio->addr[offset]);
+ val = (val >> APU_GPIO_BIT_DIR) & 1;
+
+ spin_unlock(_gpio->lock);
+
+ return val;
+}
+
+static int gpio_apu_dir_in(struct gpio_chip *chip, unsigned int offset)
+{
+ u32 val;
+
+ spin_lock(_gpio->lock);
+
+ val = ioread32(apu_gpio->addr[offset]);
+ val &= ~BIT(APU_GPIO_BIT_DIR);
+ iowrite32(val, apu_gpio->addr[offset]);
+
+ spin_unlock(_gpio->lock);
+
+ return 0;
+}
+
+static int gpio_apu_dir_out(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ u32 val;
+
+ spin_lock(_gpio->lock);
+
+ val = ioread32(apu_gpio->addr[offset]);
+ val |= BIT(APU_GPIO_BIT_DIR);
+ iowrite32(val, apu_gpio->addr[offset]);
+
+ spin_unlock(_gpio->lock);
+
+ return 0;
+}
+
+static int gpio_apu_get_data(struct gpio_chip *chip, unsigned int offset)
+{
+ u32 val;
+
+ spin_lock(_gpio->lock);
+
+
[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
---
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 ..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEVNAME"gpio-apu"
+
+#define APU_FCH_ACPI_MMIO_BASE 0xFED8
+#define APU_FCH_GPIO_BASE (APU_FCH_ACPI_MMIO_BASE + 0x1500)
+#define APU_GPIO_BIT_RD16
+#define APU_GPIO_BIT_WR22
+#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(_gpio->lock);
+
+ val = ~ioread32(apu_gpio->addr[offset]);
+ val = (val >> APU_GPIO_BIT_DIR) & 1;
+
+ spin_unlock(_gpio->lock);
+
+ return val;
+}
+
+static int gpio_apu_dir_in(struct gpio_chip *chip, unsigned int offset)
+{
+ u32 val;
+
+ spin_lock(_gpio->lock);
+
+ val = ioread32(apu_gpio->addr[offset]);
+ val &= ~BIT(APU_GPIO_BIT_DIR);
+ iowrite32(val, apu_gpio->addr[offset]);
+
+ spin_unlock(_gpio->lock);
+
+ return 0;
+}
+
+static int gpio_apu_dir_out(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ u32 val;
+
+ spin_lock(_gpio->lock);
+
+ val = ioread32(apu_gpio->addr[offset]);
+ val |= BIT(APU_GPIO_BIT_DIR);
+ iowrite32(val, apu_gpio->addr[offset]);
+
+ spin_unlock(_gpio->lock);
+
+ return 0;
+}
+
+static int gpio_apu_get_data(struct gpio_chip *chip, unsigned int offset)
+{
+ u32 val;
+
+ spin_lock(_gpio->lock);
+
+
