* register access issues in pca953x gpio driver
@ 2020-03-27 7:49 Uwe Kleine-König
2020-03-27 10:26 ` Andy Shevchenko
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2020-03-27 7:49 UTC (permalink / raw)
To: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, Mark Brown; +Cc: linux-gpio
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/ |
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: register access issues in pca953x gpio driver 2020-03-27 7:49 register access issues in pca953x gpio driver Uwe Kleine-König @ 2020-03-27 10:26 ` Andy Shevchenko 2020-03-27 13:49 ` Uwe Kleine-König 2020-03-27 13:03 ` Mark Brown 2020-03-27 17:22 ` [PATCH v1 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König 2 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2020-03-27 10:26 UTC (permalink / raw) To: Uwe Kleine-König Cc: Marek Vasut, H. Nikolaus Schaller, Mark Brown, linux-gpio 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: register access issues in pca953x gpio driver 2020-03-27 10:26 ` Andy Shevchenko @ 2020-03-27 13:49 ` Uwe Kleine-König 0 siblings, 0 replies; 13+ messages in thread From: Uwe Kleine-König @ 2020-03-27 13:49 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Marek Vasut, H. Nikolaus Schaller, Mark Brown, linux-gpio 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/ | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: register access issues in pca953x gpio driver 2020-03-27 7:49 register access issues in pca953x gpio driver Uwe Kleine-König 2020-03-27 10:26 ` Andy Shevchenko @ 2020-03-27 13:03 ` Mark Brown 2020-03-27 13:46 ` Uwe Kleine-König 2020-03-27 17:22 ` [PATCH v1 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König 2 siblings, 1 reply; 13+ messages in thread From: Mark Brown @ 2020-03-27 13:03 UTC (permalink / raw) To: Uwe Kleine-König Cc: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, linux-gpio [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: register access issues in pca953x gpio driver 2020-03-27 13:03 ` Mark Brown @ 2020-03-27 13:46 ` Uwe Kleine-König 2020-03-27 13:58 ` Mark Brown 0 siblings, 1 reply; 13+ messages in thread From: Uwe Kleine-König @ 2020-03-27 13:46 UTC (permalink / raw) To: Mark Brown; +Cc: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, linux-gpio 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/ | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: register access issues in pca953x gpio driver 2020-03-27 13:46 ` Uwe Kleine-König @ 2020-03-27 13:58 ` Mark Brown 0 siblings, 0 replies; 13+ messages in thread From: Mark Brown @ 2020-03-27 13:58 UTC (permalink / raw) To: Uwe Kleine-König Cc: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, linux-gpio [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 0/2] gpio: pca953x: fix handling of automatic address incrementing 2020-03-27 7:49 register access issues in pca953x gpio driver Uwe Kleine-König 2020-03-27 10:26 ` Andy Shevchenko 2020-03-27 13:03 ` Mark Brown @ 2020-03-27 17:22 ` Uwe Kleine-König 2020-03-27 17:22 ` [PATCH v1 1/2] [RFC] " Uwe Kleine-König 2020-03-27 17:22 ` [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() Uwe Kleine-König 2 siblings, 2 replies; 13+ messages in thread From: Uwe Kleine-König @ 2020-03-27 17:22 UTC (permalink / raw) To: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, Mark Brown Cc: kernel, linux-gpio 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 1/2] [RFC] gpio: pca953x: fix handling of automatic address incrementing 2020-03-27 17:22 ` [PATCH v1 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König @ 2020-03-27 17:22 ` Uwe Kleine-König 2020-03-27 17:22 ` [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() Uwe Kleine-König 1 sibling, 0 replies; 13+ messages in thread From: Uwe Kleine-König @ 2020-03-27 17:22 UTC (permalink / raw) To: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, Mark Brown Cc: kernel, linux-gpio 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() 2020-03-27 17:22 ` [PATCH v1 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König 2020-03-27 17:22 ` [PATCH v1 1/2] [RFC] " Uwe Kleine-König @ 2020-03-27 17:22 ` Uwe Kleine-König 2020-03-27 22:06 ` H. Nikolaus Schaller 1 sibling, 1 reply; 13+ messages in thread From: Uwe Kleine-König @ 2020-03-27 17:22 UTC (permalink / raw) To: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, Mark Brown Cc: kernel, linux-gpio 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() 2020-03-27 17:22 ` [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() Uwe Kleine-König @ 2020-03-27 22:06 ` H. Nikolaus Schaller 2020-03-28 9:27 ` Uwe Kleine-König 0 siblings, 1 reply; 13+ messages in thread From: H. Nikolaus Schaller @ 2020-03-27 22:06 UTC (permalink / raw) To: Uwe Kleine-König Cc: Marek Vasut, Andy Shevchenko, Mark Brown, kernel, open list:GPIO SUBSYSTEM, Discussions about the Letux Kernel > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() 2020-03-27 22:06 ` H. Nikolaus Schaller @ 2020-03-28 9:27 ` Uwe Kleine-König 2020-03-28 19:41 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Uwe Kleine-König @ 2020-03-28 9:27 UTC (permalink / raw) To: H. Nikolaus Schaller Cc: Marek Vasut, Andy Shevchenko, Mark Brown, kernel, open list:GPIO SUBSYSTEM, Discussions about the Letux Kernel 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/ | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() 2020-03-28 9:27 ` Uwe Kleine-König @ 2020-03-28 19:41 ` Andy Shevchenko 2020-03-30 14:07 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2020-03-28 19:41 UTC (permalink / raw) To: Uwe Kleine-König Cc: H. Nikolaus Schaller, Marek Vasut, Mark Brown, kernel, open list:GPIO SUBSYSTEM, Discussions about the Letux Kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() 2020-03-28 19:41 ` Andy Shevchenko @ 2020-03-30 14:07 ` Andy Shevchenko 0 siblings, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2020-03-30 14:07 UTC (permalink / raw) To: Uwe Kleine-König Cc: H. Nikolaus Schaller, Marek Vasut, Mark Brown, kernel, open list:GPIO SUBSYSTEM, Discussions about the Letux Kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-03-30 14:07 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-27 7:49 register access issues in pca953x gpio driver Uwe Kleine-König 2020-03-27 10:26 ` Andy Shevchenko 2020-03-27 13:49 ` Uwe Kleine-König 2020-03-27 13:03 ` Mark Brown 2020-03-27 13:46 ` Uwe Kleine-König 2020-03-27 13:58 ` Mark Brown 2020-03-27 17:22 ` [PATCH v1 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König 2020-03-27 17:22 ` [PATCH v1 1/2] [RFC] " Uwe Kleine-König 2020-03-27 17:22 ` [PATCH v1 2/2] [RFC] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() Uwe Kleine-König 2020-03-27 22:06 ` H. Nikolaus Schaller 2020-03-28 9:27 ` Uwe Kleine-König 2020-03-28 19:41 ` Andy Shevchenko 2020-03-30 14:07 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).