diff mbox series

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

Message ID 20260902154525.4090276-3-mehmet.fide@gmail.com
State New
Headers show
Series gpio: mmio: report the line direction on chips without direction registers | expand

Commit Message

Mehmet Fide Sept. 2, 2026, 3:45 p.m. UTC 
From: Mehmet Fide <mehmet.fide@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@kernel.org>
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
 drivers/gpio/gpio-mmio.c | 59 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 4 deletions(-)

Comments

Linus Walleij Sept. 2, 2026, 5:31 p.m. UTC | #1 
Hi Mehmet,

thanks for your patch!

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

> From: Mehmet Fide <mehmet.fide@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@kernel.org>
> Signed-off-by: Mehmet Fide <mehmet.fide@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
Mehmet Fide Sept. 2, 2026, 10:25 p.m. UTC | #2 
From: Mehmet Fide <mehmet.fide@screeningeagle.com>

> 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?

Yes, that is cleaner: gpio-mmio then knows nothing about pinctrl, the
same way it does not for set_config today. v4 will add the callback and
gpiochip_generic_get_config() as a mirror of gpiochip_generic_config(),
with gpio-mmio installing it for the pinctrl backend and seeding the
shadow through gc->get_config.

One detail for the generic helper: with CONFIG_PINCTRL off the
pinctrl_gpio_get_config() stub returns 0 and leaves *config alone, so
the helper returns -ENOTSUPP there instead of pretending it answered,
like gpiochip_generic_config() does for a chip without pin ranges.

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

Agreed, nothing outside the chip needs it.

Patch 1 stays as it is, minus the npins check I already told the
Sashiko bot was redundant.

Thanks,
Mehmet
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 7e4b3e8d609f..987285c33f45 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -49,6 +49,7 @@  o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/spinlock.h>
@@ -372,7 +373,17 @@  static int gpio_mmio_dir_in_err(struct gpio_chip *gc, unsigned int gpio)
 
 static int gpio_mmio_simple_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
-	return gpio_mmio_dir_return(gc, gpio, false);
+	struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+	int ret;
+
+	ret = gpio_mmio_dir_return(gc, gpio, false);
+	if (ret)
+		return ret;
+
+	guard(raw_spinlock_irqsave)(&chip->lock);
+	chip->sdir &= ~gpio_mmio_line2mask(gc, gpio);
+
+	return 0;
 }
 
 static int gpio_mmio_dir_out_err(struct gpio_chip *gc, unsigned int gpio,
@@ -384,9 +395,19 @@  static int gpio_mmio_dir_out_err(struct gpio_chip *gc, unsigned int gpio,
 static int gpio_mmio_simple_dir_out(struct gpio_chip *gc, unsigned int gpio,
 				    int val)
 {
+	struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+	int ret;
+
 	gc->set(gc, gpio, val);
 
-	return gpio_mmio_dir_return(gc, gpio, true);
+	ret = gpio_mmio_dir_return(gc, gpio, true);
+	if (ret)
+		return ret;
+
+	guard(raw_spinlock_irqsave)(&chip->lock);
+	chip->sdir |= gpio_mmio_line2mask(gc, gpio);
+
+	return 0;
 }
 
 static int gpio_mmio_dir_in(struct gpio_chip *gc, unsigned int gpio)
@@ -601,20 +622,50 @@  static int gpio_mmio_setup_direction(struct gpio_generic_chip *chip,
 			gc->direction_input = gpio_mmio_dir_in_err;
 		else
 			gc->direction_input = gpio_mmio_simple_dir_in;
+
+		if (cfg->flags & GPIO_GENERIC_PINCTRL_BACKEND) {
+			chip->dir_unreadable = true;
+			gc->get_direction = gpio_mmio_get_dir;
+		}
 	}
 
 	return 0;
 }
 
+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;
+
+	guard(raw_spinlock_irqsave)(&chip->lock);
+	if (config)
+		chip->sdir |= gpio_mmio_line2mask(gc, gpio);
+	else
+		chip->sdir &= ~gpio_mmio_line2mask(gc, gpio);
+}
+
 static int gpio_mmio_request(struct gpio_chip *gc, unsigned int gpio_pin)
 {
 	struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+	int ret;
 
 	if (gpio_pin >= gc->ngpio)
 		return -EINVAL;
 
-	if (chip->pinctrl)
-		return gpiochip_generic_request(gc, gpio_pin);
+	if (chip->pinctrl) {
+		ret = gpiochip_generic_request(gc, gpio_pin);
+		if (ret)
+			return ret;
+		gpio_mmio_seed_dir_from_pinctrl(gc, gpio_pin);
+	}
 
 	return 0;
 }