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

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

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

On Mon Oct 20, 2025 at 3:25 PM CEST, Sander Vanheule wrote:
> 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).

Ha, yes ofc, you're right. It seems I really need some more sleep.

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

Ah, now I got it. Thanks for the explanation. Let me get back to
your initial patch tomorrow.

-michael

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