[PATCH v3 2/2] gpio: mmio: track the direction of chips without direction registers

[Linus Walleij]

Hi Mehmet,

thanks for your patch!

On Wed, Sep 2, 2026 at 5:45 PM Mehmet Fide <mehmet.fide at gmail.com> wrote:

> From: Mehmet Fide <mehmet.fide at screeningeagle.com>
>
> A generic chip with GPIO_GENERIC_PINCTRL_BACKEND and no direction
> registers sets the direction through pinctrl but has no get_direction
> callback, so every gpiod_get_direction() call trips the WARN in gpiolib
> and the direction gpiolib reports is whatever it assumed. On a Vybrid
> Colibri module that is 21 backtraces per boot.
>
> Keep the direction of such a chip in the existing shadow: the direction
> setters update sdir under the chip lock, and get_direction() is the
> shadow-reading path already used for unreadable direction registers.
> That keeps the callback usable in atomic context, which it has to be:
> gpiochip_lock_as_irq() calls it for !can_sleep chips from
> gpiochip_irq_domain_activate(), under the irq descriptor lock.
>
> The pad's actual state is read once, in process context, when a line is
> requested: gpiolib calls request() right before get_direction() for a
> new line, so the shadow is seeded from PIN_CONFIG_OUTPUT_ENABLE there and
> the line reports what the pin controller says. Lines pinctrl cannot
> answer for keep the input default, which is what gpiolib assumed before.
>
> Suggested-by: Bartosz Golaszewski <brgl at kernel.org>
> Signed-off-by: Mehmet Fide <mehmet.fide at screeningeagle.com>

For one I really like the approach to ask the pin controller
back-end about the direction state when the GPIO registers
doesn't know this!

I have more of an implementation question here:

gpiolib already supports this:

struct gpio_chip {
(...)
        int                     (*set_config)(struct gpio_chip *gc,
                                              unsigned int offset,
                                              unsigned long config);

this sets one specific config at a time. With a generic pin control
back-end it is simply populated with gpiochip_generic_config()
from gpiolib.c which will call pinctrl_gpio_set_config() for
the corresponding pin.

What about just implementing generic optional get_config()
in struct gpio_chip, implement a likewise generic
gpiochip_generic_get_config() in gpiolib and use that as
the fallback?

        int                     (*get_config)(struct gpio_chip *gc,
                                              unsigned int offset,
                                              unsigned long *config);


Then the implementation becomes pretty straight-forward
from that point, and this:

+static void gpio_mmio_seed_dir_from_pinctrl(struct gpio_chip *gc,
+                                           unsigned int gpio)
+{
+       struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+       unsigned long config;
+
+       if (!IS_ENABLED(CONFIG_PINCTRL) || chip->reg_dir_out ||
chip->reg_dir_in)
+               return;
+
+       config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
+       if (pinctrl_gpio_get_config(gc, gpio, &config))
+               return;

Can drop all the checks for !IS_ENABLED(CONFIG_PINCTRL) as this is done
generically in in gpiolib and:

+       config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
+       if (gc->get_config(gc, gpio, &config))
+               return;

I don't think it is necessary to provide any consumer API for this
such as gpiod_get_config(gpiod); as no-one really needs it, we can
keep it as a private thing in struct gpio_chip for now.

Yours,
Linus Walleij