Re: [PATCH v5 3/3] gpio: mmio: track the direction of chips without direction registers

[Bough Chen]

On Thu, Sep 03, 2026 at 09:59:40AM +0200, Mehmet Fide wrote:
> From: Mehmet Fide <mehmet.fide@xxxxxxxxxxxxxxxxxx>
> 
> 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 there from PIN_CONFIG_OUTPUT_ENABLE
> through the chip's get_config(), which is gpiochip_generic_get_config()
> for the pinctrl backend, and the line reports what the pin controller
> says. Lines pinctrl cannot answer for keep the input default, which is
> what gpiolib assumed before.

dir_unreadable is now set from a second source (PINCTRL_BACKEND with no
direction register, in addition to GPIO_GENERIC_UNREADABLE_REG_DIR);
worth a line in the commit message.

Reviewed-by: Haibo Chen <haibo.chen@xxxxxxx>

Regards
Haibo Chen

> 
> Suggested-by: Bartosz Golaszewski <brgl@xxxxxxxxxx>
> Signed-off-by: Mehmet Fide <mehmet.fide@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/gpio/gpio-mmio.c | 60 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index 7e4b3e8d609f..0709ffaab4de 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,51 @@ 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;
> +			gc->get_config = gpiochip_generic_get_config;
> +		}
>  	}
>  
>  	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 (!gc->get_config || chip->reg_dir_out || chip->reg_dir_in)
> +		return;
> +
> +	config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
> +	if (gc->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;
>  }
> -- 
> 2.54.0
>