linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing
@ 2020-03-30 19:50 Uwe Kleine-König
  2020-03-30 19:50 ` [PATCH v2 1/2] " Uwe Kleine-König
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2020-03-30 19:50 UTC (permalink / raw)
  To: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, Mark Brown
  Cc: kernel, linux-gpio, Marcel Gudert

Hello,

this is v2, the only code change compared to v1 is that I added a
closing parenthesis to create actually compiling code :-|

Other than that I added some tags (Tested-by and Reviewed-by, some of
them I got off-list). I wasn't sure if I should apply Andy's tags also
to patch 1 (and so didn't).

Best regards
Uwe

Uwe Kleine-König (2):
  gpio: pca953x: fix handling of automatic address incrementing
  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] 8+ messages in thread

* [PATCH v2 1/2] gpio: pca953x: fix handling of automatic address incrementing
  2020-03-30 19:50 [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König
@ 2020-03-30 19:50 ` Uwe Kleine-König
  2020-03-31 10:02   ` Andy Shevchenko
  2020-03-31 10:07   ` Andy Shevchenko
  2020-03-30 19:50 ` [PATCH v2 2/2] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() Uwe Kleine-König
  2020-03-31 10:04 ` [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing Andy Shevchenko
  2 siblings, 2 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2020-03-30 19:50 UTC (permalink / raw)
  To: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, Mark Brown
  Cc: kernel, linux-gpio, Marcel Gudert

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.

Tested-by: Marcel Gudert <m.gudert@eckelmann.de>
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] 8+ messages in thread

* [PATCH v2 2/2] gpio: pca953x: drop unused parameters of pca953x_recalc_addr()
  2020-03-30 19:50 [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König
  2020-03-30 19:50 ` [PATCH v2 1/2] " Uwe Kleine-König
@ 2020-03-30 19:50 ` Uwe Kleine-König
  2020-03-31 10:04 ` [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing Andy Shevchenko
  2 siblings, 0 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2020-03-30 19:50 UTC (permalink / raw)
  To: Marek Vasut, Andy Shevchenko, H. Nikolaus Schaller, Mark Brown
  Cc: kernel, linux-gpio, Marcel Gudert

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

Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
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..34562c7e26fe 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] 8+ messages in thread

* Re: [PATCH v2 1/2] gpio: pca953x: fix handling of automatic address incrementing
  2020-03-30 19:50 ` [PATCH v2 1/2] " Uwe Kleine-König
@ 2020-03-31 10:02   ` Andy Shevchenko
  2020-03-31 10:07   ` Andy Shevchenko
  1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2020-03-31 10:02 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Marek Vasut, H. Nikolaus Schaller, Mark Brown, kernel,
	linux-gpio, Marcel Gudert

On Mon, Mar 30, 2020 at 09:50:17PM +0200, Uwe Kleine-König wrote:
> 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.
> 

Tags were applied to both patches.
Just to elaborate here as well that I have tested on PCA9555 (GPIO mode +
IRQ mode) and since it doesn't use AI, I haven't found any regressions.

Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Tested-by: Marcel Gudert <m.gudert@eckelmann.de>
> 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
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing
  2020-03-30 19:50 [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König
  2020-03-30 19:50 ` [PATCH v2 1/2] " Uwe Kleine-König
  2020-03-30 19:50 ` [PATCH v2 2/2] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() Uwe Kleine-König
@ 2020-03-31 10:04 ` Andy Shevchenko
  2 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2020-03-31 10:04 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Marek Vasut, H. Nikolaus Schaller, Mark Brown, kernel,
	linux-gpio, Marcel Gudert

On Mon, Mar 30, 2020 at 09:50:16PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> this is v2, the only code change compared to v1 is that I added a
> closing parenthesis to create actually compiling code :-|
> 
> Other than that I added some tags (Tested-by and Reviewed-by, some of
> them I got off-list). I wasn't sure if I should apply Andy's tags also
> to patch 1 (and so didn't).

Thank you for an update.
I elaborated in patch 1 the above.

> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (2):
>   gpio: pca953x: fix handling of automatic address incrementing
>   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
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] gpio: pca953x: fix handling of automatic address incrementing
  2020-03-30 19:50 ` [PATCH v2 1/2] " Uwe Kleine-König
  2020-03-31 10:02   ` Andy Shevchenko
@ 2020-03-31 10:07   ` Andy Shevchenko
  2020-04-14 10:15     ` Uwe Kleine-König
  1 sibling, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2020-03-31 10:07 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Marek Vasut, H. Nikolaus Schaller, Mark Brown, kernel,
	linux-gpio, Marcel Gudert

On Mon, Mar 30, 2020 at 09:50:17PM +0200, Uwe Kleine-König wrote:

...

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

It's allowed to put more, but I rather utilize Depends-on for that.

So, something like

Fixes: b4818afeacbd ...
Depends-on: 8958262af3fb ...

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] gpio: pca953x: fix handling of automatic address incrementing
  2020-03-31 10:07   ` Andy Shevchenko
@ 2020-04-14 10:15     ` Uwe Kleine-König
  2020-04-14 13:16       ` Andy Shevchenko
  0 siblings, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2020-04-14 10:15 UTC (permalink / raw)
  To: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski
  Cc: Marek Vasut, H. Nikolaus Schaller, Mark Brown, kernel,
	linux-gpio, Marcel Gudert

Hello,

obviously I failed to Cc: the gpio maintainers. Should I resend because
of that?

On Tue, Mar 31, 2020 at 01:07:59PM +0300, Andy Shevchenko wrote:
> On Mon, Mar 30, 2020 at 09:50:17PM +0200, Uwe Kleine-König wrote:
> 
> ...
> 
> > 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.
> 
> It's allowed to put more, but I rather utilize Depends-on for that.
> 
> So, something like
> 
> Fixes: b4818afeacbd ...
> Depends-on: 8958262af3fb ...

I don't know what is best here. Using

Fixes: b4818afeacbd ("gpio: pca953x: Add set_multiple to allow multiple bits to be set in one write.")

seems sensible. Not sure which commits are sensible to list in
Depends-on lines. I tend to just don't list any and then backport on
request of the stable maintainers iff and when application to older
versions failed.

Linus/Bartosz: Do you care for application of the tags according to your
taste?

Best regards
Uwe


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] gpio: pca953x: fix handling of automatic address incrementing
  2020-04-14 10:15     ` Uwe Kleine-König
@ 2020-04-14 13:16       ` Andy Shevchenko
  0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2020-04-14 13:16 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Linus Walleij, Bartosz Golaszewski, Marek Vasut,
	H. Nikolaus Schaller, Mark Brown, kernel, linux-gpio,
	Marcel Gudert

On Tue, Apr 14, 2020 at 12:15:45PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> obviously I failed to Cc: the gpio maintainers. Should I resend because
> of that?

I guess it's better to do because of
- this,
- v5.7-rc1 is out, makes sense to rebase.

...

> > So, something like
> > 
> > Fixes: b4818afeacbd ...
> > Depends-on: 8958262af3fb ...
> 
> I don't know what is best here. Using
> 
> Fixes: b4818afeacbd ("gpio: pca953x: Add set_multiple to allow multiple bits to be set in one write.")
> 
> seems sensible. Not sure which commits are sensible to list in
> Depends-on lines. I tend to just don't list any and then backport on
> request of the stable maintainers iff and when application to older
> versions failed.

Depends-on, in my opinion, is good to have to show at least one significant
dependency for the fix.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-04-14 13:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30 19:50 [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing Uwe Kleine-König
2020-03-30 19:50 ` [PATCH v2 1/2] " Uwe Kleine-König
2020-03-31 10:02   ` Andy Shevchenko
2020-03-31 10:07   ` Andy Shevchenko
2020-04-14 10:15     ` Uwe Kleine-König
2020-04-14 13:16       ` Andy Shevchenko
2020-03-30 19:50 ` [PATCH v2 2/2] gpio: pca953x: drop unused parameters of pca953x_recalc_addr() Uwe Kleine-König
2020-03-31 10:04 ` [PATCH v2 0/2] gpio: pca953x: fix handling of automatic address incrementing 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).