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: Wed, 02 Jun 2021 22:20:38 +0200	[thread overview]
Message-ID: <3d0c69e746fba43267fed0f32dd660ddb405782c.camel@svanheule.net> (raw)
In-Reply-To: <CAHp75VdaPS0YPvNHVgeDOuz=9ACn7okHxhyNEZuyQR8=kSdNdg@mail.gmail.com>

Hi Andy,

On Tue, 2021-06-01 at 18:24 +0300, Andy Shevchenko wrote:
> On Tue, Jun 1, 2021 at 2:49 PM Michael Walle <michael@walle.cc> wrote:
> > Am 2021-05-31 17:48, schrieb Andy Shevchenko:
> > > On Mon, May 31, 2021 at 6:33 PM Sander Vanheule <sander@svanheule.net>
> > > wrote:
> > > > 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:
> > > 
> > > > 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.
> > > 
> > > Have you had a chance to look into the PCA953x driver?
> > > Sounds to me that you are missing the APIs that regmap provides.
> > 
> > What would that be? The register cache? We need to cache the
> > value somehow, because (still assuming it is write only) we cannot
> > read it back. Thus the read of the RMW, would need get the
> > value from the cache. Thus the user of gpio-regmap would need
> > to make sure, to (a) use a cache for the regmap supplied to
> > gpio-regmap and (b) populate its initial values correctly. Is
> > that what you are suggesting? And hopefully, no other user
> > of the regmap will call regcache_mark_dirty() or something
> > like that.
> > 
> > I had a quick look at the PCA953x driver but it all its
> > registers are readable according to the comment on the top
> > of the file.
> 
> Since regmap doesn't have a facility to support the registers _at the
> same offset_ with different meaning (depending on data direction), the
> only way to handle this (without redesign regmap internals) is to add
> specific "pages" via additional bits in the address space. So, let's
> say 0 = RW, 1 = RO, 2 = WO. In this case see the following offset
> mapping of the hypothetical hardware registers:
> 
> REG1 (RW)   0x00 -> 0x000
> REG2 (RW)   0x04 -> 0x004
> REG3 (RO)   0x08 -> 0x108
> REG3 (RW)   0x08 -> 0x208
> 
> Then these bits should be masked out. Something similar is done in the
> PCA953x driver for extended addresses and autoincrement.
> 
> This is what I propose to look at as the starter.

Thank you for clarifying. Essentialy this is then the same solution as an extra
cache in gpio-regmap for the output values, except the cacheing is handled by
the regmap layer.

I think this was the last issue standing, so after I implement this, I'll spin a
v4.

Best,
Sander