Re: [RFC PATCH 1/2] gpio: regmap: Force writes for aliased data regs

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]

Hi,

On Mon Oct 20, 2025 at 1:56 PM CEST, Sander Vanheule wrote:
> GPIO chips often have data input and output fields aliased to the same
> offset. Since gpio-regmap performs a value update before the direction
> update (to prevent glitches), a pin currently configured as input may
> cause regmap_update_bits() to not perform a write.
>
> This may cause unexpected line states when the current input state
> equals the requested output state:
>
>         OUT   IN      OUT
>     DIR ''''''\...|.../''''''
>
>     pin ....../'''|'''\......
>              (1) (2) (3)
>
>     1. Line was configurad as out-low, but is reconfigured to input.
>        External logic results in high value.
>     2. Set output value high. regmap_update_bits() sees the value is
>        already high and discards the register write.
>     3. Line is switched to output, maintaining the stale output config
>        (low) instead of the requested config (high).
>
> By switching to regmap_write_bits(), a write of the requested output
> value can be forced, irrespective of the read state. Do this only for
> aliased registers, so the more efficient regmap_update_bits() can still
> be used for distinct registers.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  drivers/gpio/gpio-regmap.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index ab9e4077fa60..ba3c19206ccf 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -93,7 +93,7 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
>  	unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> -	unsigned int reg, mask;
> +	unsigned int reg, mask, mask_val;
>  	int ret;
>  
>  	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> @@ -101,9 +101,15 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
>  		return ret;
>  
>  	if (val)
> -		ret = regmap_update_bits(gpio->regmap, reg, mask, mask);
> +		mask_val = mask;
>  	else
> -		ret = regmap_update_bits(gpio->regmap, reg, mask, 0);
> +		mask_val = 0;
> +
> +	/* ignore input values which shadow the old output value */
> +	if (gpio->reg_dat_base == gpio->reg_set_base)
> +		ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val);
> +	else
> +		ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);

I wonder if we should just switch to regmap_write_bits() entirely.

In patch 2, you've wrote:

> The generic gpiochip implementation stores a shadow value of the
> pin output data, which is updated and written to hardware on output
> data changes. Pin input values are always obtained by reading the
> aliased data register from hardware.

I couldn't find that in the code though. But if the gpiolib only
updates the output register on changes, the write part in
regmap_update_bits() would always occur.

In any case, feel free to add.

Reviewed-by: Michael Walle <mwalle@kernel.org>

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]