Re: [PATCH 0/5] RTL8231 GPIO expander support

From: Sander Vanheule <sander@svanheule.net>
To: Andy Shevchenko <andy.shevchenko@gmail.com>,
	Michael Walle <michael@walle.cc>
Cc: Hans de Goede <hdegoede@redhat.com>, Andrew Lunn <andrew@lunn.ch>,
	Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
	Lee Jones <lee.jones@linaro.org>, Mark Brown <broonie@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Linux LED Subsystem <linux-leds@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/5] RTL8231 GPIO expander support
Date: Mon, 31 May 2021 17:33:27 +0200	[thread overview]
Message-ID: <272ac6af4a5ba5df4bb085617c9267e5ece61c19.camel@svanheule.net> (raw)
In-Reply-To: <CAHp75VfOrUBRQH1vrXEwHN4ZPojQfQju-_wp_3djZeozEaatug@mail.gmail.com>

On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote:
> 
> 
> On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote:
> > Am 2021-05-31 10:36, schrieb Sander Vanheule:

...

> > > 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.
> > > 
> > 
> > Ahh so this makes more sense. If the data register is really write only
> > regardless of the direction mode, then RMW doesn't make any sense at all.
> > Please note, that even if regmap caches values, it might be marked as dirty
> > and it will re-read the values from hardware. So I don't know if that will
> > help you.
> > 
> > So a possible quirk could be
> >  GPIO_REGMAP_WRITE_ONLY_DATA_REG (or something like that)
> > 
> > 
> 
> Isn’t regmap property to do a such? I don’t think any quirks are needed since hw
> works as expected.

The HW works as expected, but it is regmap-gpio's assumption that values read
from reg_set_base reflect the current output value that fails.

I looked a bit more at the provided interface, to see if this can be done with
existing regmap functionality.

The data registers must not be marked volatile, to ensure cached reads. The pin
set function can wrap the RMW in regcache_cache_only + regcache_sync, but this
causes visible glitching on my device.

The pin input values can be read by wrapping the regmap_read in
regcache_cache_bypass guards.

If only the regmap's internal lock is used, the RMW cycle is no longer atomic.
Inside the cache_only guards you can't read the input data, and inside the
cache_bypass guards other register writes cannot be performed, or the cache may
get out of sync. regcache_sync_region could be used, but maybe we would then
miss other registers that were updated in the meantime.

Am I missing something here? It seems to me like the regmap interface can't
really accommodate what's required, unless maybe the rtl8231 regmap users
perform some manual locking. This all seems terribly complicated compared to
using an internal output-value cache inside regmap-gpio.

Best,
Sander