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

[Sander Vanheule <sander@svanheule.net>]

Hi Michael,

On Mon, 2025-10-20 at 15:02 +0200, Michael Walle wrote:
> Hi Sander,
> 
> 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.
> 
> Have you looked at the .volatile_reg callback of the regmap api?
> You might use the same heuristics, i.e. .reg_dat_base == .reg_set_base
> to implement that callback. That way you'd just have to
> (unconditionally) set that callback in gpio_regmap_register() and
> regmap should take care of the rest.

Maybe I'm missing something here, but I'm not sure what difference that would
make. .volatile_reg is part of the regmap config, so when gpio_regmap_register()
is called, the regmap has already been created. We can't change the
.volatile_reg callback (and we shouldn't, it's up to the user to define it).

FWIW, I did test this with a regmap config that marks the aliased data registers
as volatile. The issue isn't that an invalid cache is being read. The problem is
that writes are being optimized away when they shouldn't:

   1. Read register from hardware (volatile) or cache (non-volatile).
   2. Update bits in mask to requested value
   3. Write updated value to hardware if:
         A. This is a forced write (i.e. regmap_write_bits), or
         B. The updated value differs from the original.

Marking the register as volatile doesn't change the behavior, only the source of
the initial value _regmap_update_bits() uses. Step 3B is the problematic one
when using regmap_update_bits(). Per the diagram above, the comparison may
happen against an input value differing from the (invisible) output state, which
would hide the state change.

Best,
Sander