Re: register access issues in pca953x gpio driver - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Marek Vasut <marek.vasut@gmail.com>,
	"H. Nikolaus Schaller" <hns@goldelico.com>,
	Mark Brown <broonie@kernel.org>,
	linux-gpio@vger.kernel.org
Subject: Re: register access issues in pca953x gpio driver
Date: Fri, 27 Mar 2020 14:49:21 +0100	[thread overview]
Message-ID: <20200327134921.aaqm6jbrxy2epwzt@pengutronix.de> (raw)
In-Reply-To: <20200327102659.GN1922688@smile.fi.intel.com>

On Fri, Mar 27, 2020 at 12:26:59PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 27, 2020 at 08:49:22AM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > I have an issue with an pca9505 when the .set_multiple callback is used.
> > That chip has a bit ("AI") in the register address that makes the
> > address increment automatically on subsequent reads and writes.
> > 
> > The problem (that was already noticed in commit 3b00691cc46a ("gpio:
> > pca953x: hack to fix 24 bit gpio expanders")) is that the regmap stuff
> > isn't aware of this bit and so register accesses that make use of the auto
> > incrementing are not matched to those without it.
> > 
> > Additionally there is a bug in pca953x_recalc_addr() that results in the
> > AI bit only be set for register writes. (That's the issue that made me
> > notice this problem. The result is that in .set_multiple the read
> > accesses bank 0's register only (when the hardware is hit) or uses the
> > read cache from a location without AI set and then writes using AI set.)
> > 
> > I didn't try to understand if fixing pca953x_recalc_addr() to not set AI
> > depending on write fixes all issues. But to make the register access in
> > the driver robust I'm convinced we need to fix the regmap stuff to
> > understand the AI bit.
> > 
> > @broonie: I don't know regmap good enough to instantly know the right
> > magic to do this. Can you give a rough overview what would be needed?
> 
> Uwe, thank you for the report. Personally I didn't try set_multiple() with this
> driver and as you noticed Marek did a big refactoring to the driver to that
> part in particular.

FTR: I wasn't aware that I use it either. That's what is gets used when
the chip is accessed using gpioctl. (One side effect is btw that even if
I only use a single GPIO, setting always involves writing all 5 (in my
case for the 40 pin chip) output registers.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |