Accessing GPIOs from userspace using recent kernels

[Javier Martinez Canillas]

Hello Tony,

On Fri, May 23, 2014 at 5:11 PM, Tony Lindgren <tony at atomide.com> wrote:
> * Linus Walleij <linus.walleij at linaro.org> [140523 04:36]:
>> On Fri, May 16, 2014 at 12:15 PM, Peter TB Brett <peter at peter-b.co.uk> wrote:
>> > The current call chain seems to be: gpiod_export() --> gpiod_request() -->
>> > omap_gpio_request().  Looking at other GPIO drivers, it seems like
>> > omap_gpio_request() should eventually call pinctrl_request_gpio().  Would be
>> > useful if someone who knows about OMAP4/gpio/pinctrl could take a look at
>> > this.

I looked briefly at adding pinctrl back-end commands to the OMAP GPIO
driver. That is to make all GPIO operations fall through to the
pinctrl-single driver as Linus suggested before. The changes in the
GPIO driver are quite trivial, here is a RFC patch [0]  that has only
build tested but I think is useful to at least discuss this.

Now, in order to make that patch to actually work someone has to
register the chip GPIO range to pin controller mapping with
gpiochip_add_pin_range() or something similar to make
pinctrl_get_device_gpio_range() to succeed. The pinctrl-single driver
has a "pinctrl-single,gpio-range" property that can be used to define
a GPIO range to be registered.

But the problem is as you said that since there are two different hw
blocks for pin muxing and GPIO control, the pins that can be
multiplexed as GPIO are scattered all over the padconf registers
address space. So there isn't an easy way to specify the mapping and
we will have to add an entry on "pinctrl-single,gpio-range" for every
single GPIO pin (that's from 128 to 256 entries depending on the SoC).

To make even more complicated, the padconf registers offset for GPIO
pins are SoC specific so we need a mapping for each SoC.

So I wonder if is worth to add all that information to the DTS files.
Athough on the other hand is nice to have a better coordination
between the GPIO and pinctrl drivers and as Tony said there are use
cases where this is needed to workaround some silicion erratas.

>
> If we do this, we also need a solution to prevent automatic remuxing
> of GPIO pins from happending. For wake-up events, some drivers need
> to remux a rx pin to GPIO input for the duration of idle, and then
> back to device rx pin. This is needed on some other platforms too
> AFAIK.
>
> For the drivers needing GPIO wake-up events, request_gpio is done in
> the driver after drivers/base/pinctrl.c has already muxed the device
> pin to rx. At minimum we would get warnings about reserved pins if
> we tried to automatically mux them to GPIO.
>
> We may be able to use some GPIO specific property to prevent
> automatic remuxing as we discussed in the #armlinux few days ago.
>

Yes, adding a GPIO specific property to prevent automatic remuxing
sounds sensible to me.

> Related to automatic remuxing of GPIO pins, there are also other
> needs for pinctrl and GPIO interaction. We need to remux GPIO output
> pins to input + pull + safe_mode to prevent the GPIO pins losing
> value briefly during off-idle. That's the gpio errata 1.158 at as
> shown at least at [1]. Because the GPIO to pinctrl mapping is
> sparse and SoC specific, there's currently now obvious way to do
> that. And we would need few new GPIO functions to tell pinctrl
> subsystem about the change.
>

I'm not that familiar with the pinctrl-single driver but can't that
errata be handled on pinctrl-single without the GPIO OMAP driver
intervention?

I mean if we already add the complete GPIO <--> pin mapping using
"pinctrl-single,gpio-range" then the pinctrl-single driver will know
what pins can be muxed as GPIO and which ones were set as output with
pinctrl_gpio_direction_output() and then can just mux these output
GPIO pins to input + pull + safe_mode during off-idle. Or am I
completely lost here? :-)

> Regards,
>
> Tony
>
>
> [1] https://www.gitorious.org/rowboat/kernel/commit/86b15f21298b749a9d8216ff1839d33ad542464e?format=patch

Best regards,
Javier

[0]
commit 96c886987219e37395a160f8bd0d228509c1d4f0
Author: Javier Martinez Canillas <javier at dowhile0.org>
Date:   Fri May 30 20:50:39 2014 +0200

    gpio: omap: Make GPIO operations fall through pinctrl-single

    On OMAP platforms, there are two diferent hardware blocks for
    I/O multiplexing / pad configuration and GPIO control. So two
    different drivers are used: pinctrl-single and gpio-omap.

    Since two separate drivers are used there is no coordination
    between these and a I/O pad is not configured as a GPIO when
    a GPIO pin is requested.

    This patch adds pinctrl back-end commands to the GPIO OMAP
    driver so the pinmux_ops functions in the pinctrl-single
    driver are called for each GPIO operation.

    Signed-off-by: Javier Martinez Canillas <javier at dowhile0.org>

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 00f29aa..cee63c6 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -27,6 +27,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/gpio.h>
 #include <linux/bitops.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_data/gpio-omap.h>

 #define OFF_MODE       1
@@ -664,6 +665,11 @@ static int omap_gpio_request(struct gpio_chip
*chip, unsigned offset)
 {
        struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
        unsigned long flags;
+       int ret;
+
+       ret = pinctrl_request_gpio(chip->base + offset);
+       if (ret)
+               return ret;

        /*
         * If this is the first gpio_request for the bank,
@@ -704,6 +710,8 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
         */
        if (!BANK_USED(bank))
                pm_runtime_put(bank->dev);
+
+       pinctrl_free_gpio(chip->base + offset);
 }

 /*
@@ -947,6 +955,11 @@ static int gpio_input(struct gpio_chip *chip,
unsigned offset)
 {
        struct gpio_bank *bank;
        unsigned long flags;
+       int ret;
+
+       ret = pinctrl_gpio_direction_input(chip->base + offset);
+       if (ret)
+               return ret;

        bank = container_of(chip, struct gpio_bank, chip);
        spin_lock_irqsave(&bank->lock, flags);
@@ -973,6 +986,11 @@ static int gpio_output(struct gpio_chip *chip,
unsigned offset, int value)
 {
        struct gpio_bank *bank;
        unsigned long flags;
+       int ret;
+
+       ret = pinctrl_gpio_direction_output(chip->base + offset);
+       if (ret)
+               return ret;

        bank = container_of(chip, struct gpio_bank, chip);
        spin_lock_irqsave(&bank->lock, flags);