register access issues in pca953x gpio driver

register access issues in pca953x gpio driver

[Uwe Kleine-König]

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?

Best regards
Uwe

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

Re: register access issues in pca953x gpio driver

[Andy Shevchenko]

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.

So, I guess he may shed a light a bit on this.

-- 
With Best Regards,
Andy Shevchenko

Re: register access issues in pca953x gpio driver

[Mark Brown]

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

On Fri, Mar 27, 2020 at 08:49:22AM +0100, Uwe Kleine-König wrote:

> @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?

If you want to dynamically change if the register is autoincrementing
you're going to have to add stuff to regmap for that, it understands
devices that autoincrement and devices that don't autoincrement but
there's currently nothing for transitioning between the two.

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

Re: register access issues in pca953x gpio driver

[Uwe Kleine-König]

Hello Mark,

On Fri, Mar 27, 2020 at 01:03:21PM +0000, Mark Brown wrote:
> On Fri, Mar 27, 2020 at 08:49:22AM +0100, Uwe Kleine-König wrote:
> 
> > @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?
> 
> If you want to dynamically change if the register is autoincrementing
> you're going to have to add stuff to regmap for that, it understands
> devices that autoincrement and devices that don't autoincrement but
> there's currently nothing for transitioning between the two.

I'm not entirely sure, but I think they all support autoincrementing but
some need the flag set and others don't. I imagine that this should be
modeled as register range [0-0x7f] (as AI = 0x80) and the accessor
callbacks have to set AI or not depending on the chip.

Best regards
Uwe

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

Re: register access issues in pca953x gpio driver

[Uwe Kleine-König]

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

Re: register access issues in pca953x gpio driver

[Mark Brown]

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

On Fri, Mar 27, 2020 at 02:46:57PM +0100, Uwe Kleine-König wrote:
> On Fri, Mar 27, 2020 at 01:03:21PM +0000, Mark Brown wrote:

> > If you want to dynamically change if the register is autoincrementing
> > you're going to have to add stuff to regmap for that, it understands
> > devices that autoincrement and devices that don't autoincrement but
> > there's currently nothing for transitioning between the two.

> I'm not entirely sure, but I think they all support autoincrementing but
> some need the flag set and others don't. I imagine that this should be
> modeled as register range [0-0x7f] (as AI = 0x80) and the accessor
> callbacks have to set AI or not depending on the chip.

So it's not that you're varying this at runtime but rather that you have
different chips?  In that case you can use read_flag_mask and
write_flag_mask to set the autoincrement flag, and use_single_write for
things that lack autoincrement.

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

[PATCH v1 0/2] gpio: pca953x: fix handling of automatic address incrementing

[Uwe Kleine-König]

Hello,

This patch addresses a bug noticed by one of our customers. I split the
fix in two; a fix and a cleanup to make the fix easier to backport and
review. (The first patch applies to 5.4 at least, the second doesn't
cleanly.)

I marked the series as RFC as I only have a PCA9505 to test and I have
no confirmation yet that the problem is indeed gone (though I'm
optimistic here. The traces look fine, but I don't have the machine on
my desk and so cannot tell if the LED now behaves fine).

Thanks to Mark Brown to give the needed hints to properly implement this
patch set.

Best regards
Uwe

Uwe Kleine-König (2):
  [RFC] gpio: pca953x: fix handling of automatic address incrementing
  [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr()

 drivers/gpio/gpio-pca953x.c | 75 +++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 36 deletions(-)

-- 
2.26.0.rc2

[PATCH v1 1/2] [RFC] gpio: pca953x: fix handling of automatic address incrementing

[Uwe Kleine-König]

Some of the chips supported by the pca953x driver need the most
significant bit in the address word set to automatically increment the
address pointer on subsequent reads and writes (example: PCA9505). With
this bit unset the same register is read multiple times on a multi-byte
read sequence. Other chips must not have this bit set and autoincrement
always (example: PCA9555).

Up to now this AI bit was interpreted to be part of the address, which
resulted in inconsistent regmap caching when a register was written with
AI set and then read without it. This happened for the PCA9505 in
pca953x_gpio_set_multiple() where pca953x_read_regs() bulk read from the
cache for registers 0x8-0xc and then wrote to registers 0x88-0x8c. (Side
note: reading 5 values from offset 0x8 yiels OP0 5 times because AI must
be set to get OP0-OP4, which is another bug that is resolved here as a
by-product.) The same problem happens when calls to gpio_set_value() and
gpio_set_array_value() were mixed.

With this patch the AI bit is always set for chips that support it. This
works as there are no code locations that make use of the behaviour with
AI unset (for the chips that support it).

Note that the call to pca953x_setup_gpio() had to be done a bit earlier
to make the NBANK macro work.

The history of this bug is a bit complicated. Commit b32cecb46bdc
("gpio: pca953x: Extract the register address mangling to single
function") changed which chips and functions are affected. Commit
3b00691cc46a ("gpio: pca953x: hack to fix 24 bit gpio expanders") used
some duct tape to make the driver at least appear to work. Commit
49427232764d ("gpio: pca953x: Perform basic regmap conversion")
introduced the caching. Commit b4818afeacbd ("gpio: pca953x: Add
set_multiple to allow multiple bits to be set in one write.") introduced
the .set_multiple() callback which didn't work for chips that need the
AI bit which was fixed later for some chips in 8958262af3fb ("gpio:
pca953x: Repair multi-byte IO address increment on PCA9575"). So I'm
sorry, I don't know which commit I should pick for a Fixes: line.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpio/gpio-pca953x.c | 44 +++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 5638b4e5355f..8168558299c2 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -307,8 +307,22 @@ static const struct regmap_config pca953x_i2c_regmap = {
 	.volatile_reg = pca953x_volatile_register,
 
 	.cache_type = REGCACHE_RBTREE,
-	/* REVISIT: should be 0x7f but some 24 bit chips use REG_ADDR_AI */
-	.max_register = 0xff,
+	.max_register = 0x7f,
+};
+
+static const struct regmap_config pca953x_ai_i2c_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.read_flag_mask = REG_ADDR_AI,
+	.write_flag_mask = REG_ADDR_AI,
+
+	.readable_reg = pca953x_readable_register,
+	.writeable_reg = pca953x_writeable_register,
+	.volatile_reg = pca953x_volatile_register,
+
+	.cache_type = REGCACHE_RBTREE,
+	.max_register = 0x7f,
 };
 
 static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
@@ -319,18 +333,6 @@ static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
 	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 	u8 regaddr = pinctrl | addr | (off / BANK_SZ);
 
-	/* Single byte read doesn't need AI bit set. */
-	if (!addrinc)
-		return regaddr;
-
-	/* Chips with 24 and more GPIOs always support Auto Increment */
-	if (write && NBANK(chip) > 2)
-		regaddr |= REG_ADDR_AI;
-
-	/* PCA9575 needs address-increment on multi-byte writes */
-	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE)
-		regaddr |= REG_ADDR_AI;
-
 	return regaddr;
 }
 
@@ -863,6 +865,7 @@ static int pca953x_probe(struct i2c_client *client,
 	int ret;
 	u32 invert = 0;
 	struct regulator *reg;
+	const struct regmap_config *regmap_config;
 
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL)
@@ -925,7 +928,17 @@ static int pca953x_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, chip);
 
-	chip->regmap = devm_regmap_init_i2c(client, &pca953x_i2c_regmap);
+	pca953x_setup_gpio(chip, chip->driver_data & PCA_GPIO_MASK);
+
+	if (NBANK(chip) > 2 || PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
+		dev_info(&client->dev, "using AI\n");
+		regmap_config = &pca953x_ai_i2c_regmap;
+	} else {
+		dev_info(&client->dev, "using no AI\n");
+		regmap_config = &pca953x_i2c_regmap;
+	}
+
+	chip->regmap = devm_regmap_init_i2c(client, regmap_config);
 	if (IS_ERR(chip->regmap)) {
 		ret = PTR_ERR(chip->regmap);
 		goto err_exit;
@@ -956,7 +969,6 @@ static int pca953x_probe(struct i2c_client *client,
 	/* initialize cached registers from their original values.
 	 * we can't share this chip with another i2c master.
 	 */
-	pca953x_setup_gpio(chip, chip->driver_data & PCA_GPIO_MASK);
 
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
 		chip->regs = &pca953x_regs;
-- 
2.26.0.rc2

[PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr()

[Uwe Kleine-König]

After the previous patch the two last parameters of
pca953x_recalc_addr() are unused and so can be dropped.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpio/gpio-pca953x.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 8168558299c2..f4943cad5d0e 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -325,8 +325,7 @@ static const struct regmap_config pca953x_ai_i2c_regmap = {
 	.max_register = 0x7f,
 };
 
-static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
-			      bool write, bool addrinc)
+static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off)
 {
 	int bank_shift = pca953x_bank_shift(chip);
 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
@@ -338,7 +337,7 @@ static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
 
 static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
 {
-	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
+	u8 regaddr = pca953x_recalc_addr(chip, reg, 0;
 	u8 value[MAX_BANK];
 	int i, ret;
 
@@ -356,7 +355,7 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long
 
 static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
 {
-	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, false, true);
+	u8 regaddr = pca953x_recalc_addr(chip, reg, 0);
 	u8 value[MAX_BANK];
 	int i, ret;
 
@@ -375,8 +374,7 @@ static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *
 static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off,
-					true, false);
+	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
 	u8 bit = BIT(off % BANK_SZ);
 	int ret;
 
@@ -390,10 +388,8 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 		unsigned off, int val)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off,
-					true, false);
-	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off,
-					true, false);
+	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
+	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off);
 	u8 bit = BIT(off % BANK_SZ);
 	int ret;
 
@@ -413,8 +409,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 inreg = pca953x_recalc_addr(chip, chip->regs->input, off,
-				       true, false);
+	u8 inreg = pca953x_recalc_addr(chip, chip->regs->input, off);
 	u8 bit = BIT(off % BANK_SZ);
 	u32 reg_val;
 	int ret;
@@ -438,8 +433,7 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off,
-					true, false);
+	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off);
 	u8 bit = BIT(off % BANK_SZ);
 
 	mutex_lock(&chip->i2c_lock);
@@ -450,8 +444,7 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
 static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off,
-					true, false);
+	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
 	u8 bit = BIT(off % BANK_SZ);
 	u32 reg_val;
 	int ret;
@@ -491,10 +484,8 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 					 unsigned int offset,
 					 unsigned long config)
 {
-	u8 pull_en_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_EN, offset,
-					     true, false);
-	u8 pull_sel_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_SEL, offset,
-					      true, false);
+	u8 pull_en_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_EN, offset);
+	u8 pull_sel_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_SEL, offset);
 	u8 bit = BIT(offset % BANK_SZ);
 	int ret;
 
-- 
2.26.0.rc2

Re: [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr()

[H. Nikolaus Schaller]

> Am 27.03.2020 um 18:22 schrieb Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> 
> After the previous patch the two last parameters of
> pca953x_recalc_addr() are unused and so can be dropped.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/gpio/gpio-pca953x.c | 31 +++++++++++--------------------
> 1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 8168558299c2..f4943cad5d0e 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -325,8 +325,7 @@ static const struct regmap_config pca953x_ai_i2c_regmap = {
> 	.max_register = 0x7f,
> };
> 
> -static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
> -			      bool write, bool addrinc)
> +static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off)
> {
> 	int bank_shift = pca953x_bank_shift(chip);
> 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> @@ -338,7 +337,7 @@ static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
> 
> static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
> {
> -	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
> +	u8 regaddr = pca953x_recalc_addr(chip, reg, 0;

here is some ")" missing.

After fix&compile it still works for the pcal6524 of the OMAP5/Pyra (LPAE build).

> 	u8 value[MAX_BANK];
> 	int i, ret;
> 
> @@ -356,7 +355,7 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long
> 
> static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
> {
> -	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, false, true);
> +	u8 regaddr = pca953x_recalc_addr(chip, reg, 0);
> 	u8 value[MAX_BANK];
> 	int i, ret;
> 
> @@ -375,8 +374,7 @@ static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *
> static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
> {
> 	struct pca953x_chip *chip = gpiochip_get_data(gc);
> -	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off,
> -					true, false);
> +	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
> 	u8 bit = BIT(off % BANK_SZ);
> 	int ret;
> 
> @@ -390,10 +388,8 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
> 		unsigned off, int val)
> {
> 	struct pca953x_chip *chip = gpiochip_get_data(gc);
> -	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off,
> -					true, false);
> -	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off,
> -					true, false);
> +	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
> +	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off);
> 	u8 bit = BIT(off % BANK_SZ);
> 	int ret;
> 
> @@ -413,8 +409,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
> static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> {
> 	struct pca953x_chip *chip = gpiochip_get_data(gc);
> -	u8 inreg = pca953x_recalc_addr(chip, chip->regs->input, off,
> -				       true, false);
> +	u8 inreg = pca953x_recalc_addr(chip, chip->regs->input, off);
> 	u8 bit = BIT(off % BANK_SZ);
> 	u32 reg_val;
> 	int ret;
> @@ -438,8 +433,7 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
> {
> 	struct pca953x_chip *chip = gpiochip_get_data(gc);
> -	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off,
> -					true, false);
> +	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off);
> 	u8 bit = BIT(off % BANK_SZ);
> 
> 	mutex_lock(&chip->i2c_lock);
> @@ -450,8 +444,7 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
> static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
> {
> 	struct pca953x_chip *chip = gpiochip_get_data(gc);
> -	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off,
> -					true, false);
> +	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
> 	u8 bit = BIT(off % BANK_SZ);
> 	u32 reg_val;
> 	int ret;
> @@ -491,10 +484,8 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
> 					 unsigned int offset,
> 					 unsigned long config)
> {
> -	u8 pull_en_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_EN, offset,
> -					     true, false);
> -	u8 pull_sel_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_SEL, offset,
> -					      true, false);
> +	u8 pull_en_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_EN, offset);
> +	u8 pull_sel_reg = pca953x_recalc_addr(chip, PCAL953X_PULL_SEL, offset);
> 	u8 bit = BIT(offset % BANK_SZ);
> 	int ret;
> 
> -- 
> 2.26.0.rc2
> 

BR and thanks,
Nikolaus Schaller

Re: [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr()

[Uwe Kleine-König]

On Fri, Mar 27, 2020 at 11:06:43PM +0100, H. Nikolaus Schaller wrote:
> 
> > Am 27.03.2020 um 18:22 schrieb Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > 
> > After the previous patch the two last parameters of
> > pca953x_recalc_addr() are unused and so can be dropped.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > drivers/gpio/gpio-pca953x.c | 31 +++++++++++--------------------
> > 1 file changed, 11 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> > index 8168558299c2..f4943cad5d0e 100644
> > --- a/drivers/gpio/gpio-pca953x.c
> > +++ b/drivers/gpio/gpio-pca953x.c
> > @@ -325,8 +325,7 @@ static const struct regmap_config pca953x_ai_i2c_regmap = {
> > 	.max_register = 0x7f,
> > };
> > 
> > -static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
> > -			      bool write, bool addrinc)
> > +static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off)
> > {
> > 	int bank_shift = pca953x_bank_shift(chip);
> > 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> > @@ -338,7 +337,7 @@ static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
> > 
> > static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
> > {
> > -	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
> > +	u8 regaddr = pca953x_recalc_addr(chip, reg, 0;
> 
> here is some ")" missing.
> 
> After fix&compile it still works for the pcal6524 of the OMAP5/Pyra
> (LPAE build).

Argh, it seems I didn't try to recompile after rebasing :-|

I'm still waiting for more feedback and then will send out a v2.

Thanks for your test,
Uwe

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

Re: [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr()

[Andy Shevchenko]

On Sat, Mar 28, 2020 at 10:27:30AM +0100, Uwe Kleine-König wrote:
> On Fri, Mar 27, 2020 at 11:06:43PM +0100, H. Nikolaus Schaller wrote:

> I'm still waiting for more feedback and then will send out a v2.

From code perspective looks good,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

If you give me some time (couple of days) I'll test it on PCA9555 (for no
regression).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr()

[Andy Shevchenko]

On Sat, Mar 28, 2020 at 09:41:23PM +0200, Andy Shevchenko wrote:
> On Sat, Mar 28, 2020 at 10:27:30AM +0100, Uwe Kleine-König wrote:
> > On Fri, Mar 27, 2020 at 11:06:43PM +0100, H. Nikolaus Schaller wrote:
> 
> > I'm still waiting for more feedback and then will send out a v2.
> 
> From code perspective looks good,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> If you give me some time (couple of days) I'll test it on PCA9555 (for no
> regression).

No regression found in my case (PCA9555 + IRQ test)
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko