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

[Sander Vanheule <sander@svanheule.net>]

Hi Michael,

On Tue, 2025-10-21 at 09:33 +0200, Michael Walle wrote:
> > +	/* 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.

It would certainly make the code simpler, but it may impact performance a bit.
E.g. a bit-banged I2C bus doesn't need to update the output value (only the
direction), so using regmap_update_bits() saves half the writes when the output
data register can be properly cached.

Similar to gpio-mmio.c, gpio-regmap.c could also provide multiple setters, so
the branch(es) in this call would only occur once at init, to sacrifice code
size for a bit of performance. Feel free to let me know what your preference is,
otherwise I'll keep the patch as-is.

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

I was referring to bgpio_set(). AFAICT gpiod_direction_input() and
gpiod_direction_output() call the driver unconditionally, without checking if
the gpiolib state would change for this pin.

> 
> In any case, feel free to add.
> 
> Reviewed-by: Michael Walle <mwalle@kernel.org>

Thanks!

Best,
Sander