Re: [PATCH v3 0/6] RTL8231 GPIO expander support

[Sander Vanheule <sander@svanheule.net>]

On Sun, 2021-05-30 at 23:22 +0200, Michael Walle wrote:
> Am 2021-05-30 20:16, schrieb Andy Shevchenko:
> > On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> 
> > wrote:
> > > On 5/30/21 6:19 PM, Sander Vanheule wrote:
> > > > As Michael suggested, I tried raw register reads and writes, to eliminate
> > > > any
> > > > side effects of the intermediate code. I didn't use the ioctls (this isn't
> > > > a
> > > > netdev), but I found regmap's debugfs write functionality, which allowed
> > > > me to
> > > > do the same.
> > > > 
> > > > I was trying to reproduce the behaviour I reported earlier, but couldn't.
> > > > The
> > > > output levels were always the intended ones. At some point I realised that
> > > > the
> > > > regmap_update_bits function does a read-modify-write, which might shadow
> > > > the
> > > > actual current output value.
> > > > For example:
> > > >  * Set output low: current out is low
> > > >  * Change to input with pull-up: current out is still low, but DATAx reads
> > > > high
> > > >  * Set output high: RMW reads a high value (the input), so assumes a write
> > > > is
> > > >    not necessary, leaving the old output value (low).
> > > > 
> > > > Currently, I see two options:
> > > >  * Use regmap_update_bits_base to avoid the lazy RMW behaviour
> > > >  * Add a cache for the output data values to the driver, and only use
> > > > these
> > > >    values to write to the output registers. This would allow keeping lazy
> > > > RMW
> > > >    behaviour, which may be a benefit on slow busses.
> > > > 
> > > > With either of these implemented, if I set the output value before the
> > > > direction, everything works! :-)
> > > > 
> > > > Would you like this to be added to regmap-gpio, or should I revert back to
> > > > a
> > > > device-specific implementation?
> > > 
> > > Regmap allows you to mark certain ranges as volatile, so that they 
> > > will not
> > > be cached, these GPIO registers containing the current pin value seems 
> > > like
> > > a good candidate for this. This is also necessary to make reading the 
> > > GPIO
> > > work without getting back a stale, cached value.
> > 
> > After all it seems a simple missed proper register configuration in
> > the driver for regmap.
> > Oh, as usual something easy-to-solve requires tons of time to find it. 
> > :-)
> > 
> > Sander, I think you may look at gpio-pca953x.c to understand how it
> > works (volatility of registers).
> 
> But as far as I see is the regmap instantiated without a cache?

That's correct, there currently is no cache, although I could add one.

The data register rather appears to be implemented as a read-only (pin inputs)
register and a write-only (pin outputs) register, aliased on the same register
address.

As I understand, marking the DATA registers as volatile wouldn't help. With a
cache this would force reads to not use the cache, which is indeed required for
the pin input values (DATA register reads). However, the output values (DATA
register writes) can in fact be cached.
Looking at _regmap_update_bits(), marking a register as volatile would only make
a difference if regmap.reg_update_bits is implemented. On an MDIO bus, this
would also be emulated with a lazy RMW (see mdiobus_modify()), which is why I
chose not to implement it for regmap-mdio.

So, I still think the issue lies with the lazy RMW behaviour. The patch below
would force a register update when reg_set_base (the data output register) and
reg_dat_base (the data input register) are identical. Otherwise the two
registers are assumed to have conventional RW behaviour. I'm just not entirely
sure gpio-regmap.c is the right place for this.

---8<---

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 95553734e169..c2fccd19548a 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -81,13 +81,16 @@ static void 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);
+       bool force = gpio->reg_set_base == gpio->reg_dat_base;
        unsigned int reg, mask;
 
        gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
        if (val)
-               regmap_update_bits(gpio->regmap, reg, mask, mask);
+               regmap_update_bits_base(gpio->regmap, reg, mask, mask, NULL,
+                                       false, force);
        else
-               regmap_update_bits(gpio->regmap, reg, mask, 0);
+               regmap_update_bits_base(gpio->regmap, reg, mask, 0, NULL,
+                                       false, force);
 }
 
 static void gpio_regmap_set_with_clear(struct gpio_chip *chip,

--
Best,
Sander