[PATCH 01/14] gpio: pca953x: Deduplicate the bank_size

[PATCH 01/14] gpio: pca953x: Deduplicate the bank_size

[Marek Vasut]

The bank_size = fls(...) code was duplicated in the driver 5 times,
pull it into separate function.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 023a32cfac42..31e3b1b52330 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -159,11 +159,16 @@ struct pca953x_chip {
 	int (*read_regs)(struct pca953x_chip *, int, u8 *);
 };
 
+static int pca953x_bank_shift(struct pca953x_chip *chip)
+{
+	return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+}
+
 static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
 				int off)
 {
 	int ret;
-	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int bank_shift = pca953x_bank_shift(chip);
 	int offset = off / BANK_SZ;
 
 	ret = i2c_smbus_read_byte_data(chip->client,
@@ -182,7 +187,7 @@ static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
 				int off)
 {
 	int ret;
-	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int bank_shift = pca953x_bank_shift(chip);
 	int offset = off / BANK_SZ;
 
 	ret = i2c_smbus_write_byte_data(chip->client,
@@ -221,7 +226,7 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 
 static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int bank_shift = pca953x_bank_shift(chip);
 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
 	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 
@@ -265,7 +270,7 @@ static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 
 static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int bank_shift = pca953x_bank_shift(chip);
 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
 	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 
@@ -402,13 +407,12 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 				      unsigned long *mask, unsigned long *bits)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
+	int bank_shift = pca953x_bank_shift(chip);
 	unsigned int bank_mask, bank_val;
-	int bank_shift, bank;
+	int bank;
 	u8 reg_val[MAX_BANK];
 	int ret;
 
-	bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
-
 	mutex_lock(&chip->i2c_lock);
 	memcpy(reg_val, chip->reg_output, NBANK(chip));
 	for (bank = 0; bank < NBANK(chip); bank++) {
-- 
2.18.0

[PATCH 02/14] gpio: pca953x: Fix AI overflow on PCAL6524

[Marek Vasut]

The PCAL_PINCTRL_MASK is too large. The extended register block on
PCAL6524, which is the largest chip with this block, has the block
limited to address range 0x40..0x7f. This is because the bit 7 in
the command register is used for the Address Increment functionality.

Trim the mask to 0x60 to match the datasheet and to prevent accidental
overwrite of the AI bit.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 31e3b1b52330..4e9c79ca69c5 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -58,7 +58,7 @@
 #define PCA_GPIO_MASK		0x00FF
 
 #define PCAL_GPIO_MASK		0x1f
-#define PCAL_PINCTRL_MASK	0xe0
+#define PCAL_PINCTRL_MASK	0x60
 
 #define PCA_INT			0x0100
 #define PCA_PCAL		0x0200
-- 
2.18.0

Re: [PATCH 02/14] gpio: pca953x: Fix AI overflow on PCAL6524

[Bartosz Golaszewski]

niedz., 2 gru 2018 o 20:36 Marek Vasut <marek.vasut@gmail.com> napisał(a):
>
> The PCAL_PINCTRL_MASK is too large. The extended register block on
> PCAL6524, which is the largest chip with this block, has the block
> limited to address range 0x40..0x7f. This is because the bit 7 in
> the command register is used for the Address Increment functionality.
>
> Trim the mask to 0x60 to match the datasheet and to prevent accidental
> overwrite of the AI bit.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 31e3b1b52330..4e9c79ca69c5 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -58,7 +58,7 @@
>  #define PCA_GPIO_MASK          0x00FF
>
>  #define PCAL_GPIO_MASK         0x1f
> -#define PCAL_PINCTRL_MASK      0xe0
> +#define PCAL_PINCTRL_MASK      0x60
>
>  #define PCA_INT                        0x0100
>  #define PCA_PCAL               0x0200
> --
> 2.18.0
>

Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

[PATCH 03/14] gpio: pca953x: Repair multi-byte IO address increment on PCA9575

[Marek Vasut]

The multi-byte IO on various pca953x chips requires the auto-increment bit,
while other chips toggle the LSbit automatically. Note that LSbit toggling
only alternates between two registers during the IO, it is not the same as
address auto-increment. The driver currently assumes that #gpios > 16 implies
auto-increment, while #gpios <= 16 implies LSbit toggling. This is incorrect
at there are chips with 16 GPIOs which require the auto-increment bit.

The PCA9575, according to NXP datasheet rev. 4.2 from 16 April 2015, section
7.3 Command Register, the bit 7 in command register is the auto-increment
bit, which allows programming multiple registers sequentially.

Set this bit both in pca953x_gpio_set_multiple(), where it fixes the multi
register programming, and in pca957x_write_regs_16(), where is simplifies
the function. In fact, the pca957x_write_regs_16() now looks rather similar
to pca953x_write_regs_24() and pca953x_write_regs_16(), which is intended
for subsequent patches.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4e9c79ca69c5..479fa376bd18 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -215,13 +215,10 @@ static int pca953x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 
 static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	int ret;
-
-	ret = i2c_smbus_write_byte_data(chip->client, reg << 1, val[0]);
-	if (ret < 0)
-		return ret;
+	u32 regaddr = (reg << 1) | REG_ADDR_AI;
 
-	return i2c_smbus_write_byte_data(chip->client, (reg << 1) + 1, val[1]);
+	return i2c_smbus_write_i2c_block_data(chip->client, regaddr,
+					      NBANK(chip), val);
 }
 
 static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
@@ -408,6 +405,7 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	int bank_shift = pca953x_bank_shift(chip);
+	u32 regaddr = chip->regs->output << bank_shift;
 	unsigned int bank_mask, bank_val;
 	int bank;
 	u8 reg_val[MAX_BANK];
@@ -426,8 +424,13 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 		}
 	}
 
-	ret = i2c_smbus_write_i2c_block_data(chip->client,
-					     chip->regs->output << bank_shift,
+	/* PCA9575 needs address-increment on multi-byte writes */
+	if ((PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) &&
+	    (NBANK(chip) > 1)) {
+		regaddr |= REG_ADDR_AI;
+	}
+
+	ret = i2c_smbus_write_i2c_block_data(chip->client, regaddr,
 					     NBANK(chip), reg_val);
 	if (ret)
 		goto exit;
-- 
2.18.0

Re: [PATCH 03/14] gpio: pca953x: Repair multi-byte IO address increment on PCA9575

[Geert Uytterhoeven]

On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> The multi-byte IO on various pca953x chips requires the auto-increment bit,
> while other chips toggle the LSbit automatically. Note that LSbit toggling
> only alternates between two registers during the IO, it is not the same as
> address auto-increment. The driver currently assumes that #gpios > 16 implies
> auto-increment, while #gpios <= 16 implies LSbit toggling. This is incorrect
> at there are chips with 16 GPIOs which require the auto-increment bit.

as there

>
> The PCA9575, according to NXP datasheet rev. 4.2 from 16 April 2015, section
> 7.3 Command Register, the bit 7 in command register is the auto-increment
> bit, which allows programming multiple registers sequentially.
>
> Set this bit both in pca953x_gpio_set_multiple(), where it fixes the multi
> register programming, and in pca957x_write_regs_16(), where is simplifies

it simplifies

> the function. In fact, the pca957x_write_regs_16() now looks rather similar
> to pca953x_write_regs_24() and pca953x_write_regs_16(), which is intended
> for subsequent patches.

for consolidation in?

>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 4e9c79ca69c5..479fa376bd18 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -215,13 +215,10 @@ static int pca953x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>
>  static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>  {
> -       int ret;
> -
> -       ret = i2c_smbus_write_byte_data(chip->client, reg << 1, val[0]);
> -       if (ret < 0)
> -               return ret;
> +       u32 regaddr = (reg << 1) | REG_ADDR_AI;
>
> -       return i2c_smbus_write_byte_data(chip->client, (reg << 1) + 1, val[1]);
> +       return i2c_smbus_write_i2c_block_data(chip->client, regaddr,
> +                                             NBANK(chip), val);
>  }
>
>  static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
> @@ -408,6 +405,7 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
>  {
>         struct pca953x_chip *chip = gpiochip_get_data(gc);
>         int bank_shift = pca953x_bank_shift(chip);
> +       u32 regaddr = chip->regs->output << bank_shift;
>         unsigned int bank_mask, bank_val;
>         int bank;
>         u8 reg_val[MAX_BANK];
> @@ -426,8 +424,13 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
>                 }
>         }
>
> -       ret = i2c_smbus_write_i2c_block_data(chip->client,
> -                                            chip->regs->output << bank_shift,
> +       /* PCA9575 needs address-increment on multi-byte writes */
> +       if ((PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) &&
> +           (NBANK(chip) > 1)) {
> +               regaddr |= REG_ADDR_AI;
> +       }
> +
> +       ret = i2c_smbus_write_i2c_block_data(chip->client, regaddr,
>                                              NBANK(chip), reg_val);
>         if (ret)
>                 goto exit;
> --
> 2.18.0
>


-- 
Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 04/14] gpio: pca953x: Unify pca95{3,7}x_write_regs_16()

[Marek Vasut]

At this point, these two functions only differ in whether they do or do not
set the address increment bit on PCA9575. Merge these two functions together
to simplify the code a bit.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 479fa376bd18..2e02b3a9ac48 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -208,14 +208,11 @@ static int pca953x_write_regs_8(struct pca953x_chip *chip, int reg, u8 *val)
 
 static int pca953x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	u16 word = get_unaligned((u16 *)val);
+	u32 regaddr = (reg << 1);
 
-	return i2c_smbus_write_word_data(chip->client, reg << 1, word);
-}
-
-static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
-{
-	u32 regaddr = (reg << 1) | 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 i2c_smbus_write_i2c_block_data(chip->client, regaddr,
 					      NBANK(chip), val);
@@ -892,10 +889,7 @@ static int pca953x_probe(struct i2c_client *client,
 		chip->write_regs = pca953x_write_regs_24;
 		chip->read_regs = pca953x_read_regs_24;
 	} else {
-		if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
-			chip->write_regs = pca953x_write_regs_16;
-		else
-			chip->write_regs = pca957x_write_regs_16;
+		chip->write_regs = pca953x_write_regs_16;
 		chip->read_regs = pca953x_read_regs_16;
 	}
 
-- 
2.18.0

[PATCH 05/14] gpio: pca953x: Unify pca953x_{read,write}_regs_{16,24}()

[Marek Vasut]

At this point, these two functions only differ in whether they do or do not
set the address increment bit. The 16 GPIO case does not need to set the AI
bit, except for PCA9575 on write, while the 24 GPIO and more case does set
the AI bit always. Merge these two functions together to simplify the code
a bit.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 49 ++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 2e02b3a9ac48..551fa69661b2 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -206,9 +206,16 @@ static int pca953x_write_regs_8(struct pca953x_chip *chip, int reg, u8 *val)
 	return i2c_smbus_write_byte_data(chip->client, reg, *val);
 }
 
-static int pca953x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
+static int pca953x_write_regs_mul(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	u32 regaddr = (reg << 1);
+	int bank_shift = pca953x_bank_shift(chip);
+	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
+	u8 regaddr = pinctrl | addr;
+
+	/* Chips with 24 and more GPIOs always support Auto Increment */
+	if (NBANK(chip) > 2)
+		regaddr |= REG_ADDR_AI;
 
 	/* PCA9575 needs address-increment on multi-byte writes */
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE)
@@ -218,17 +225,6 @@ static int pca953x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 					      NBANK(chip), val);
 }
 
-static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
-{
-	int bank_shift = pca953x_bank_shift(chip);
-	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
-	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
-
-	return i2c_smbus_write_i2c_block_data(chip->client,
-					      pinctrl | addr | REG_ADDR_AI,
-					      NBANK(chip), val);
-}
-
 static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	int ret = 0;
@@ -252,24 +248,18 @@ static int pca953x_read_regs_8(struct pca953x_chip *chip, int reg, u8 *val)
 	return ret;
 }
 
-static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
-{
-	int ret;
-
-	ret = i2c_smbus_read_word_data(chip->client, reg << 1);
-	put_unaligned(ret, (u16 *)val);
-
-	return ret;
-}
-
-static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
+static int pca953x_read_regs_mul(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	int bank_shift = pca953x_bank_shift(chip);
 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
 	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
+	u8 regaddr = pinctrl | addr;
+
+	/* Chips with 24 and more GPIOs always support Auto Increment */
+	if (NBANK(chip) > 2)
+		regaddr |= REG_ADDR_AI;
 
-	return i2c_smbus_read_i2c_block_data(chip->client,
-					     pinctrl | addr | REG_ADDR_AI,
+	return i2c_smbus_read_i2c_block_data(chip->client, regaddr,
 					     NBANK(chip), val);
 }
 
@@ -885,12 +875,9 @@ static int pca953x_probe(struct i2c_client *client,
 	if (chip->gpio_chip.ngpio <= 8) {
 		chip->write_regs = pca953x_write_regs_8;
 		chip->read_regs = pca953x_read_regs_8;
-	} else if (chip->gpio_chip.ngpio >= 24) {
-		chip->write_regs = pca953x_write_regs_24;
-		chip->read_regs = pca953x_read_regs_24;
 	} else {
-		chip->write_regs = pca953x_write_regs_16;
-		chip->read_regs = pca953x_read_regs_16;
+		chip->write_regs = pca953x_write_regs_mul;
+		chip->read_regs = pca953x_read_regs_mul;
 	}
 
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
-- 
2.18.0

[PATCH 06/14] gpio: pca953x: Unify pca953x_{read,write}_regs_{8,mul}()

[Marek Vasut]

At this point, the pca953x_{read,write}_regs_mul() can read single bank
PCA953x GPIO chips as well. Merge the _8 and _mul functions together to
simplify the code a bit.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 54 ++++++-------------------------------
 1 file changed, 8 insertions(+), 46 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 551fa69661b2..09fd8cde9ca9 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -154,9 +154,6 @@ struct pca953x_chip {
 	struct regulator *regulator;
 
 	const struct pca953x_reg_config *regs;
-
-	int (*write_regs)(struct pca953x_chip *, int, u8 *);
-	int (*read_regs)(struct pca953x_chip *, int, u8 *);
 };
 
 static int pca953x_bank_shift(struct pca953x_chip *chip)
@@ -201,17 +198,13 @@ static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
 	return 0;
 }
 
-static int pca953x_write_regs_8(struct pca953x_chip *chip, int reg, u8 *val)
-{
-	return i2c_smbus_write_byte_data(chip->client, reg, *val);
-}
-
-static int pca953x_write_regs_mul(struct pca953x_chip *chip, int reg, u8 *val)
+static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	int bank_shift = pca953x_bank_shift(chip);
 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
 	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 	u8 regaddr = pinctrl | addr;
+	int ret;
 
 	/* Chips with 24 and more GPIOs always support Auto Increment */
 	if (NBANK(chip) > 2)
@@ -221,15 +214,8 @@ static int pca953x_write_regs_mul(struct pca953x_chip *chip, int reg, u8 *val)
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE)
 		regaddr |= REG_ADDR_AI;
 
-	return i2c_smbus_write_i2c_block_data(chip->client, regaddr,
-					      NBANK(chip), val);
-}
-
-static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
-{
-	int ret = 0;
-
-	ret = chip->write_regs(chip, reg, val);
+	ret = i2c_smbus_write_i2c_block_data(chip->client, regaddr,
+					     NBANK(chip), val);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed writing register\n");
 		return ret;
@@ -238,36 +224,20 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 	return 0;
 }
 
-static int pca953x_read_regs_8(struct pca953x_chip *chip, int reg, u8 *val)
-{
-	int ret;
-
-	ret = i2c_smbus_read_byte_data(chip->client, reg);
-	*val = ret;
-
-	return ret;
-}
-
-static int pca953x_read_regs_mul(struct pca953x_chip *chip, int reg, u8 *val)
+static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	int bank_shift = pca953x_bank_shift(chip);
 	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
 	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 	u8 regaddr = pinctrl | addr;
+	int ret;
 
 	/* Chips with 24 and more GPIOs always support Auto Increment */
 	if (NBANK(chip) > 2)
 		regaddr |= REG_ADDR_AI;
 
-	return i2c_smbus_read_i2c_block_data(chip->client, regaddr,
-					     NBANK(chip), val);
-}
-
-static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
-{
-	int ret;
-
-	ret = chip->read_regs(chip, reg, val);
+	ret = i2c_smbus_read_i2c_block_data(chip->client, regaddr,
+					    NBANK(chip), val);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed reading register\n");
 		return ret;
@@ -872,14 +842,6 @@ static int pca953x_probe(struct i2c_client *client,
 	 */
 	pca953x_setup_gpio(chip, chip->driver_data & PCA_GPIO_MASK);
 
-	if (chip->gpio_chip.ngpio <= 8) {
-		chip->write_regs = pca953x_write_regs_8;
-		chip->read_regs = pca953x_read_regs_8;
-	} else {
-		chip->write_regs = pca953x_write_regs_mul;
-		chip->read_regs = pca953x_read_regs_mul;
-	}
-
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
 		ret = device_pca953x_init(chip, invert);
 	else
-- 
2.18.0

[PATCH 07/14] gpio: pca953x: Factor out common code from device_pca95xx_init()

[Marek Vasut]

The PCA957x and PCA953x init functions are almost the same, except for
the different register mapping and one extra write to BKEN register in
case of PCA957x. Factor out the common code.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 09fd8cde9ca9..dc691bd52a79 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -119,18 +119,21 @@ struct pca953x_reg_config {
 	int direction;
 	int output;
 	int input;
+	int invert;
 };
 
 static const struct pca953x_reg_config pca953x_regs = {
 	.direction = PCA953X_DIRECTION,
 	.output = PCA953X_OUTPUT,
 	.input = PCA953X_INPUT,
+	.invert = PCA953X_INVERT,
 };
 
 static const struct pca953x_reg_config pca957x_regs = {
 	.direction = PCA957X_CFG,
 	.output = PCA957X_OUT,
 	.input = PCA957X_IN,
+	.invert = PCA957X_INVRT,
 };
 
 struct pca953x_chip {
@@ -679,13 +682,11 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 }
 #endif
 
-static int device_pca953x_init(struct pca953x_chip *chip, u32 invert)
+static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
 {
 	int ret;
 	u8 val[MAX_BANK];
 
-	chip->regs = &pca953x_regs;
-
 	ret = pca953x_read_regs(chip, chip->regs->output, chip->reg_output);
 	if (ret)
 		goto out;
@@ -701,7 +702,7 @@ static int device_pca953x_init(struct pca953x_chip *chip, u32 invert)
 	else
 		memset(val, 0, NBANK(chip));
 
-	ret = pca953x_write_regs(chip, PCA953X_INVERT, val);
+	ret = pca953x_write_regs(chip, chip->regs->invert, val);
 out:
 	return ret;
 }
@@ -711,22 +712,7 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
 	int ret;
 	u8 val[MAX_BANK];
 
-	chip->regs = &pca957x_regs;
-
-	ret = pca953x_read_regs(chip, chip->regs->output, chip->reg_output);
-	if (ret)
-		goto out;
-	ret = pca953x_read_regs(chip, chip->regs->direction,
-				chip->reg_direction);
-	if (ret)
-		goto out;
-
-	/* set platform specific polarity inversion */
-	if (invert)
-		memset(val, 0xFF, NBANK(chip));
-	else
-		memset(val, 0, NBANK(chip));
-	ret = pca953x_write_regs(chip, PCA957X_INVRT, val);
+	ret = device_pca95xx_init(chip, invert);
 	if (ret)
 		goto out;
 
@@ -842,10 +828,13 @@ static int pca953x_probe(struct i2c_client *client,
 	 */
 	pca953x_setup_gpio(chip, chip->driver_data & PCA_GPIO_MASK);
 
-	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
-		ret = device_pca953x_init(chip, invert);
-	else
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
+		chip->regs = &pca953x_regs;
+		ret = device_pca95xx_init(chip, invert);
+	} else {
+		chip->regs = &pca957x_regs;
 		ret = device_pca957x_init(chip, invert);
+	}
 	if (ret)
 		goto err_exit;
 
-- 
2.18.0

[PATCH 08/14] gpio: pca953x: Zap ad-hoc I2C block write in multi GPIO set

[Marek Vasut]

The ad-hoc i2c block write can be replaced by standard register accessor
function, which correctly handles all the chip details and differences.
Do so to simplify the code.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index dc691bd52a79..b3386819c550 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -364,8 +364,6 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 				      unsigned long *mask, unsigned long *bits)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	int bank_shift = pca953x_bank_shift(chip);
-	u32 regaddr = chip->regs->output << bank_shift;
 	unsigned int bank_mask, bank_val;
 	int bank;
 	u8 reg_val[MAX_BANK];
@@ -384,14 +382,7 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 		}
 	}
 
-	/* PCA9575 needs address-increment on multi-byte writes */
-	if ((PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) &&
-	    (NBANK(chip) > 1)) {
-		regaddr |= REG_ADDR_AI;
-	}
-
-	ret = i2c_smbus_write_i2c_block_data(chip->client, regaddr,
-					     NBANK(chip), reg_val);
+	ret = pca953x_write_regs(chip, chip->regs->output, reg_val);
 	if (ret)
 		goto exit;
 
-- 
2.18.0

[PATCH 09/14] gpio: pca953x: Extract the register address mangling to single function

[Marek Vasut]

Instead of having the I2C register calculation function spread across
multiple accessor functions, pull it out into a single function which
returns the adjusted register address.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 59 ++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index b3386819c550..7e66f46b41b2 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -164,17 +164,37 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
 	return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
 }
 
+static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
+			      bool write, bool addrinc)
+{
+	int bank_shift = pca953x_bank_shift(chip);
+	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+	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;
+}
+
 static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
 				int off)
 {
+	u8 regaddr = pca953x_recalc_addr(chip, reg, off, false, false);
 	int ret;
-	int bank_shift = pca953x_bank_shift(chip);
-	int offset = off / BANK_SZ;
 
-	ret = i2c_smbus_read_byte_data(chip->client,
-				(reg << bank_shift) + offset);
+	ret = i2c_smbus_read_byte_data(chip->client, regaddr);
 	*val = ret;
-
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed reading register\n");
 		return ret;
@@ -186,13 +206,10 @@ static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
 static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
 				int off)
 {
+	u8 regaddr = pca953x_recalc_addr(chip, reg, off, true, false);
 	int ret;
-	int bank_shift = pca953x_bank_shift(chip);
-	int offset = off / BANK_SZ;
-
-	ret = i2c_smbus_write_byte_data(chip->client,
-					(reg << bank_shift) + offset, val);
 
+	ret = i2c_smbus_write_byte_data(chip->client, regaddr, val);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed writing register\n");
 		return ret;
@@ -203,20 +220,9 @@ static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
 
 static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	int bank_shift = pca953x_bank_shift(chip);
-	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
-	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
-	u8 regaddr = pinctrl | addr;
+	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
 	int ret;
 
-	/* Chips with 24 and more GPIOs always support Auto Increment */
-	if (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;
-
 	ret = i2c_smbus_write_i2c_block_data(chip->client, regaddr,
 					     NBANK(chip), val);
 	if (ret < 0) {
@@ -229,16 +235,9 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 
 static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	int bank_shift = pca953x_bank_shift(chip);
-	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
-	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
-	u8 regaddr = pinctrl | addr;
+	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, false, true);
 	int ret;
 
-	/* Chips with 24 and more GPIOs always support Auto Increment */
-	if (NBANK(chip) > 2)
-		regaddr |= REG_ADDR_AI;
-
 	ret = i2c_smbus_read_i2c_block_data(chip->client, regaddr,
 					    NBANK(chip), val);
 	if (ret < 0) {
-- 
2.18.0

[PATCH 10/14] gpio: pca953x: Perform basic regmap conversion

[Marek Vasut]

Convert the driver to use regmap to access the chips. Due to the convoluted
register mapping scheme, implement read/write/volatile check functions that
untangle the mess and perform check accordingly. This patch does not zap the
internal register cache of the PCA953x driver, nor does it push the regmap
access down into the gpiochip accessors to simplify the review. All that is
in subsequent patches.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 159 ++++++++++++++++++++++++++++++++++--
 1 file changed, 151 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 7e66f46b41b2..389fa891f342 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/platform_data/pca953x.h>
+#include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -30,6 +31,8 @@
 #define PCA953X_INVERT		0x02
 #define PCA953X_DIRECTION	0x03
 
+#define REG_ADDR_MASK		0x3f
+#define REG_ADDR_EXT		0x40
 #define REG_ADDR_AI		0x80
 
 #define PCA957X_IN		0x00
@@ -141,6 +144,7 @@ struct pca953x_chip {
 	u8 reg_output[MAX_BANK];
 	u8 reg_direction[MAX_BANK];
 	struct mutex i2c_lock;
+	struct regmap *regmap;
 
 #ifdef CONFIG_GPIO_PCA953X_IRQ
 	struct mutex irq_lock;
@@ -164,6 +168,141 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
 	return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
 }
 
+#define PCA953x_BANK_INPUT	BIT(0)
+#define PCA953x_BANK_OUTPUT	BIT(1)
+#define PCA953x_BANK_POLARITY	BIT(2)
+#define PCA953x_BANK_CONFIG	BIT(3)
+
+#define PCA957x_BANK_INPUT	BIT(0)
+#define PCA957x_BANK_POLARITY	BIT(1)
+#define PCA957x_BANK_BUSHOLD	BIT(2)
+#define PCA957x_BANK_CONFIG	BIT(4)
+#define PCA957x_BANK_OUTPUT	BIT(5)
+
+#define PCAL9xxx_BANK_IN_LATCH	BIT(8 + 2)
+#define PCAL9xxx_BANK_IRQ_MASK	BIT(8 + 5)
+#define PCAL9xxx_BANK_IRQ_STAT	BIT(8 + 6)
+
+/*
+ * We care about the following registers:
+ * - Standard set, below 0x40, each port can be replicated up to 8 times
+ *   - PCA953x standard
+ *     Input port			0x00 + 0 * bank_size	R
+ *     Output port			0x00 + 1 * bank_size	RW
+ *     Polarity Inversion port		0x00 + 2 * bank_size	RW
+ *     Configuration port		0x00 + 3 * bank_size	RW
+ *   - PCA957x with mixed up registers
+ *     Input port			0x00 + 0 * bank_size	R
+ *     Polarity Inversion port		0x00 + 1 * bank_size	RW
+ *     Bus hold port			0x00 + 2 * bank_size	RW
+ *     Configuration port		0x00 + 4 * bank_size	RW
+ *     Output port			0x00 + 5 * bank_size	RW
+ *
+ * - Extended set, above 0x40, often chip specific.
+ *   - PCAL6524/PCAL9555A with custom PCAL IRQ handling:
+ *     Input latch register		0x40 + 2 * bank_size	RW
+ *     Interrupt mask register		0x40 + 5 * bank_size	RW
+ *     Interrupt status register	0x40 + 6 * bank_size	R
+ *
+ * - Registers with bit 0x80 set, the AI bit
+ *   The bit is cleared and the registers fall into one of the
+ *   categories above.
+ */
+
+static bool pca953x_check_register(struct pca953x_chip *chip, unsigned int reg,
+				   u32 checkbank)
+{
+	int bank_shift = pca953x_bank_shift(chip);
+	int bank = (reg & REG_ADDR_MASK) >> bank_shift;
+	int offset = reg & (BIT(bank_shift) - 1);
+
+	/* Special PCAL extended register check. */
+	if (reg & REG_ADDR_EXT) {
+		if (!(chip->driver_data & PCA_PCAL))
+			return false;
+		bank += 8;
+	}
+
+	/* Register is not in the matching bank. */
+	if (!(BIT(bank) & checkbank))
+		return false;
+
+	/* Register is not within allowed range of bank. */
+	if (offset >= NBANK(chip))
+		return false;
+
+	return true;
+}
+
+static bool pca953x_readable_register(struct device *dev, unsigned int reg)
+{
+	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	u32 bank;
+
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
+		bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
+		       PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
+	} else {
+		bank = PCA957x_BANK_INPUT | PCA957x_BANK_OUTPUT |
+		       PCA957x_BANK_POLARITY | PCA957x_BANK_CONFIG |
+		       PCA957x_BANK_BUSHOLD;
+	}
+
+	if (chip->driver_data & PCA_PCAL) {
+		bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_IRQ_MASK |
+			PCAL9xxx_BANK_IRQ_STAT;
+	}
+
+	return pca953x_check_register(chip, reg, bank);
+}
+
+static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
+{
+	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	u32 bank;
+
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
+		bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
+			PCA953x_BANK_CONFIG;
+	} else {
+		bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY |
+			PCA957x_BANK_CONFIG | PCA957x_BANK_BUSHOLD;
+	}
+
+	if (chip->driver_data & PCA_PCAL)
+		bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_IRQ_MASK;
+
+	return pca953x_check_register(chip, reg, bank);
+}
+
+static bool pca953x_volatile_register(struct device *dev, unsigned int reg)
+{
+	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	u32 bank;
+
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
+		bank = PCA953x_BANK_INPUT;
+	else
+		bank = PCA957x_BANK_INPUT;
+
+	if (chip->driver_data & PCA_PCAL)
+		bank |= PCAL9xxx_BANK_IRQ_STAT;
+
+	return pca953x_check_register(chip, reg, bank);
+}
+
+const struct regmap_config pca953x_i2c_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.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,
 			      bool write, bool addrinc)
 {
@@ -193,8 +332,7 @@ static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
 	u8 regaddr = pca953x_recalc_addr(chip, reg, off, false, false);
 	int ret;
 
-	ret = i2c_smbus_read_byte_data(chip->client, regaddr);
-	*val = ret;
+	ret = regmap_read(chip->regmap, regaddr, val);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed reading register\n");
 		return ret;
@@ -209,7 +347,7 @@ static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
 	u8 regaddr = pca953x_recalc_addr(chip, reg, off, true, false);
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(chip->client, regaddr, val);
+	ret = regmap_write(chip->regmap, regaddr, val);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed writing register\n");
 		return ret;
@@ -223,8 +361,7 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
 	int ret;
 
-	ret = i2c_smbus_write_i2c_block_data(chip->client, regaddr,
-					     NBANK(chip), val);
+	ret = regmap_bulk_write(chip->regmap, regaddr, val, NBANK(chip));
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed writing register\n");
 		return ret;
@@ -238,8 +375,7 @@ static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
 	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, false, true);
 	int ret;
 
-	ret = i2c_smbus_read_i2c_block_data(chip->client, regaddr,
-					    NBANK(chip), val);
+	ret = regmap_bulk_read(chip->regmap, regaddr, val, NBANK(chip));
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed reading register\n");
 		return ret;
@@ -793,6 +929,14 @@ static int pca953x_probe(struct i2c_client *client,
 		}
 	}
 
+	i2c_set_clientdata(client, chip);
+
+	chip->regmap = devm_regmap_init_i2c(client, &pca953x_i2c_regmap);
+	if (IS_ERR(chip->regmap)) {
+		ret = PTR_ERR(chip->regmap);
+		goto err_exit;
+	}
+
 	mutex_init(&chip->i2c_lock);
 	/*
 	 * In case we have an i2c-mux controlled by a GPIO provided by an
@@ -843,7 +987,6 @@ static int pca953x_probe(struct i2c_client *client,
 			dev_warn(&client->dev, "setup failed, %d\n", ret);
 	}
 
-	i2c_set_clientdata(client, chip);
 	return 0;
 
 err_exit:
-- 
2.18.0

[PATCH 11/14] gpio: pca953x: Zap ad-hoc reg_direction cache

[Marek Vasut]

Replace the ad-hoc reg_direction direction register caching with generic
regcache cache. This reduces code duplication.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 52 ++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 389fa891f342..a01e9f158439 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -142,7 +142,6 @@ static const struct pca953x_reg_config pca957x_regs = {
 struct pca953x_chip {
 	unsigned gpio_start;
 	u8 reg_output[MAX_BANK];
-	u8 reg_direction[MAX_BANK];
 	struct mutex i2c_lock;
 	struct regmap *regmap;
 
@@ -387,18 +386,13 @@ static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
 static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 reg_val;
+	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off,
+					true, false);
+	u8 bit = BIT(off % BANK_SZ);
 	int ret;
 
 	mutex_lock(&chip->i2c_lock);
-	reg_val = chip->reg_direction[off / BANK_SZ] | (1u << (off % BANK_SZ));
-
-	ret = pca953x_write_single(chip, chip->regs->direction, reg_val, off);
-	if (ret)
-		goto exit;
-
-	chip->reg_direction[off / BANK_SZ] = reg_val;
-exit:
+	ret = regmap_write_bits(chip->regmap, dirreg, bit, bit);
 	mutex_unlock(&chip->i2c_lock);
 	return ret;
 }
@@ -407,6 +401,9 @@ 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 bit = BIT(off % BANK_SZ);
 	u8 reg_val;
 	int ret;
 
@@ -426,12 +423,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 	chip->reg_output[off / BANK_SZ] = reg_val;
 
 	/* then direction */
-	reg_val = chip->reg_direction[off / BANK_SZ] & ~(1u << (off % BANK_SZ));
-	ret = pca953x_write_single(chip, chip->regs->direction, reg_val, off);
-	if (ret)
-		goto exit;
-
-	chip->reg_direction[off / BANK_SZ] = reg_val;
+	ret = regmap_write_bits(chip->regmap, dirreg, bit, 0);
 exit:
 	mutex_unlock(&chip->i2c_lock);
 	return ret;
@@ -483,16 +475,19 @@ 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 bit = BIT(off % BANK_SZ);
 	u32 reg_val;
 	int ret;
 
 	mutex_lock(&chip->i2c_lock);
-	ret = pca953x_read_single(chip, chip->regs->direction, &reg_val, off);
+	ret = regmap_read(chip->regmap, dirreg, &reg_val);
 	mutex_unlock(&chip->i2c_lock);
 	if (ret < 0)
 		return ret;
 
-	return !!(reg_val & (1u << (off % BANK_SZ)));
+	return !!(reg_val & bit);
 }
 
 static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
@@ -580,6 +575,9 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
 	u8 new_irqs;
 	int level, i;
 	u8 invert_irq_mask[MAX_BANK];
+	u8 reg_direction[MAX_BANK];
+
+	regmap_read(chip->regmap, chip->regs->direction, reg_direction);
 
 	if (chip->driver_data & PCA_PCAL) {
 		/* Enable latch on interrupt-enabled inputs */
@@ -595,7 +593,7 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
 	/* Look for any newly setup interrupt */
 	for (i = 0; i < NBANK(chip); i++) {
 		new_irqs = chip->irq_trig_fall[i] | chip->irq_trig_raise[i];
-		new_irqs &= ~chip->reg_direction[i];
+		new_irqs &= reg_direction[i];
 
 		while (new_irqs) {
 			level = __ffs(new_irqs);
@@ -660,6 +658,7 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
 	bool pending_seen = false;
 	bool trigger_seen = false;
 	u8 trigger[MAX_BANK];
+	u8 reg_direction[MAX_BANK];
 	int ret, i;
 
 	if (chip->driver_data & PCA_PCAL) {
@@ -690,8 +689,9 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
 		return false;
 
 	/* Remove output pins from the equation */
+	regmap_read(chip->regmap, chip->regs->direction, reg_direction);
 	for (i = 0; i < NBANK(chip); i++)
-		cur_stat[i] &= chip->reg_direction[i];
+		cur_stat[i] &= reg_direction[i];
 
 	memcpy(old_stat, chip->irq_stat, NBANK(chip));
 
@@ -745,6 +745,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 			     int irq_base)
 {
 	struct i2c_client *client = chip->client;
+	u8 reg_direction[MAX_BANK];
 	int ret, i;
 
 	if (client->irq && irq_base != -1
@@ -759,8 +760,9 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 		 * interrupt.  We have to rely on the previous read for
 		 * this purpose.
 		 */
+		regmap_read(chip->regmap, chip->regs->direction, reg_direction);
 		for (i = 0; i < NBANK(chip); i++)
-			chip->irq_stat[i] &= chip->reg_direction[i];
+			chip->irq_stat[i] &= reg_direction[i];
 		mutex_init(&chip->irq_lock);
 
 		ret = devm_request_threaded_irq(&client->dev,
@@ -817,9 +819,9 @@ static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
 	if (ret)
 		goto out;
 
-	ret = pca953x_read_regs(chip, chip->regs->direction,
-				chip->reg_direction);
-	if (ret)
+	ret = regcache_sync_region(chip->regmap, chip->regs->direction,
+				   chip->regs->direction + NBANK(chip));
+	if (ret != 0)
 		goto out;
 
 	/* set platform specific polarity inversion */
@@ -937,6 +939,8 @@ static int pca953x_probe(struct i2c_client *client,
 		goto err_exit;
 	}
 
+	regcache_mark_dirty(chip->regmap);
+
 	mutex_init(&chip->i2c_lock);
 	/*
 	 * In case we have an i2c-mux controlled by a GPIO provided by an
-- 
2.18.0

[PATCH 12/14] gpio: pca953x: Zap ad-hoc reg_output cache

[Marek Vasut]

Replace the ad-hoc reg_output output register caching with generic
regcache cache. Drop pca953x_write_single() which is no longer used.
This reduces code duplication.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 65 +++++++++----------------------------
 1 file changed, 15 insertions(+), 50 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index a01e9f158439..8155f1dd667b 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -141,7 +141,6 @@ static const struct pca953x_reg_config pca957x_regs = {
 
 struct pca953x_chip {
 	unsigned gpio_start;
-	u8 reg_output[MAX_BANK];
 	struct mutex i2c_lock;
 	struct regmap *regmap;
 
@@ -340,21 +339,6 @@ static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
 	return 0;
 }
 
-static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
-				int off)
-{
-	u8 regaddr = pca953x_recalc_addr(chip, reg, off, true, false);
-	int ret;
-
-	ret = regmap_write(chip->regmap, regaddr, val);
-	if (ret < 0) {
-		dev_err(&chip->client->dev, "failed writing register\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
@@ -403,25 +387,17 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 	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 bit = BIT(off % BANK_SZ);
-	u8 reg_val;
 	int ret;
 
 	mutex_lock(&chip->i2c_lock);
 	/* set output level */
-	if (val)
-		reg_val = chip->reg_output[off / BANK_SZ]
-			| (1u << (off % BANK_SZ));
-	else
-		reg_val = chip->reg_output[off / BANK_SZ]
-			& ~(1u << (off % BANK_SZ));
-
-	ret = pca953x_write_single(chip, chip->regs->output, reg_val, off);
+	ret = regmap_write_bits(chip->regmap, outreg, bit, val ? bit : 0);
 	if (ret)
 		goto exit;
 
-	chip->reg_output[off / BANK_SZ] = reg_val;
-
 	/* then direction */
 	ret = regmap_write_bits(chip->regmap, dirreg, bit, 0);
 exit:
@@ -452,23 +428,12 @@ 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 reg_val;
-	int ret;
+	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off,
+					true, false);
+	u8 bit = BIT(off % BANK_SZ);
 
 	mutex_lock(&chip->i2c_lock);
-	if (val)
-		reg_val = chip->reg_output[off / BANK_SZ]
-			| (1u << (off % BANK_SZ));
-	else
-		reg_val = chip->reg_output[off / BANK_SZ]
-			& ~(1u << (off % BANK_SZ));
-
-	ret = pca953x_write_single(chip, chip->regs->output, reg_val, off);
-	if (ret)
-		goto exit;
-
-	chip->reg_output[off / BANK_SZ] = reg_val;
-exit:
+	regmap_write_bits(chip->regmap, outreg, bit, val ? bit : 0);
 	mutex_unlock(&chip->i2c_lock);
 }
 
@@ -500,7 +465,10 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 	int ret;
 
 	mutex_lock(&chip->i2c_lock);
-	memcpy(reg_val, chip->reg_output, NBANK(chip));
+	ret = pca953x_read_regs(chip, chip->regs->output, reg_val);
+	if (ret)
+		goto exit;
+
 	for (bank = 0; bank < NBANK(chip); bank++) {
 		bank_mask = mask[bank / sizeof(*mask)] >>
 			   ((bank % sizeof(*mask)) * 8);
@@ -512,11 +480,7 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 		}
 	}
 
-	ret = pca953x_write_regs(chip, chip->regs->output, reg_val);
-	if (ret)
-		goto exit;
-
-	memcpy(chip->reg_output, reg_val, NBANK(chip));
+	pca953x_write_regs(chip, chip->regs->output, reg_val);
 exit:
 	mutex_unlock(&chip->i2c_lock);
 }
@@ -815,8 +779,9 @@ static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
 	int ret;
 	u8 val[MAX_BANK];
 
-	ret = pca953x_read_regs(chip, chip->regs->output, chip->reg_output);
-	if (ret)
+	ret = regcache_sync_region(chip->regmap, chip->regs->output,
+				   chip->regs->output + NBANK(chip));
+	if (ret != 0)
 		goto out;
 
 	ret = regcache_sync_region(chip->regmap, chip->regs->direction,
-- 
2.18.0

[PATCH 13/14] gpio: pca953x: Zap single use of pca953x_read_single()

[Marek Vasut]

Drop pca953x_write_single() which is used in one place.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 8155f1dd667b..7c0122fac383 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -324,21 +324,6 @@ static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
 	return regaddr;
 }
 
-static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
-				int off)
-{
-	u8 regaddr = pca953x_recalc_addr(chip, reg, off, false, false);
-	int ret;
-
-	ret = regmap_read(chip->regmap, regaddr, val);
-	if (ret < 0) {
-		dev_err(&chip->client->dev, "failed reading register\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
@@ -408,11 +393,14 @@ 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 bit = BIT(off % BANK_SZ);
 	u32 reg_val;
 	int ret;
 
 	mutex_lock(&chip->i2c_lock);
-	ret = pca953x_read_single(chip, chip->regs->input, &reg_val, off);
+	ret = regmap_read(chip->regmap, inreg, &reg_val);
 	mutex_unlock(&chip->i2c_lock);
 	if (ret < 0) {
 		/* NOTE:  diagnostic already emitted; that's all we should
@@ -422,7 +410,7 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 		return 0;
 	}
 
-	return (reg_val & (1u << (off % BANK_SZ))) ? 1 : 0;
+	return !!(reg_val & bit);
 }
 
 static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
-- 
2.18.0

[PATCH 14/14] gpio: pca953x: Restore registers after suspend/resume cycle

[Marek Vasut]

It is possible that the PCA953x is powered down during suspend.
Use regmap cache to assure the registers in the PCA953x are in
line with the driver state after resume.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 92 +++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 7c0122fac383..e2c2e97b7321 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -972,6 +972,95 @@ static int pca953x_remove(struct i2c_client *client)
 	return ret;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int pca953x_regcache_sync(struct device *dev)
+{
+	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	int ret;
+
+	/*
+	 * The ordering between direction and output is important,
+	 * sync these registers first and only then sync the rest.
+	 */
+	ret = regcache_sync_region(chip->regmap, chip->regs->direction,
+				   chip->regs->direction + NBANK(chip));
+	if (ret != 0) {
+		dev_err(dev, "Failed to sync GPIO dir registers: %d\n", ret);
+		return ret;
+	}
+
+	ret = regcache_sync_region(chip->regmap, chip->regs->output,
+				   chip->regs->output + NBANK(chip));
+	if (ret != 0) {
+		dev_err(dev, "Failed to sync GPIO out registers: %d\n", ret);
+		return ret;
+	}
+
+#ifdef CONFIG_GPIO_PCA953X_IRQ
+	if (chip->driver_data & PCA_PCAL) {
+		pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask);
+		pca953x_write_regs(chip, PCAL953X_INT_MASK, invert_irq_mask);
+
+		ret = regcache_sync_region(chip->regmap, PCAL953X_IN_LATCH,
+					   PCAL953X_IN_LATCH + NBANK(chip));
+		if (ret != 0) {
+			dev_err(dev, "Failed to sync INT latch registers: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = regcache_sync_region(chip->regmap, PCAL953X_INT_MASK,
+					   PCAL953X_INT_MASK + NBANK(chip));
+		if (ret != 0) {
+			dev_err(dev, "Failed to sync INT mask registers: %d\n",
+				ret);
+			return ret;
+		}
+	}
+#endif
+
+	return 0;
+}
+
+static int pca953x_suspend(struct device *dev)
+{
+	struct pca953x_chip *chip = dev_get_drvdata(dev);
+
+	regcache_mark_dirty(chip->regmap);
+	pca953x_regcache_sync(dev);
+	regcache_cache_only(chip->regmap, true);
+
+	regulator_disable(chip->regulator);
+
+	return 0;
+}
+
+static int pca953x_resume(struct device *dev)
+{
+	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regulator_enable(chip->regulator);
+	if (ret != 0) {
+		dev_err(dev, "Failed to enable regulator: %d\n", ret);
+		return 0;
+	}
+
+	regcache_cache_only(chip->regmap, false);
+	ret = pca953x_regcache_sync(dev);
+	if (ret)
+		return ret;
+
+	ret = regcache_sync(chip->regmap);
+	if (ret != 0) {
+		dev_err(dev, "Failed to restore register map: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
 /* convenience to stop overlong match-table lines */
 #define OF_953X(__nrgpio, __int) (void *)(__nrgpio | PCA953X_TYPE | __int)
 #define OF_957X(__nrgpio, __int) (void *)(__nrgpio | PCA957X_TYPE | __int)
@@ -1015,9 +1104,12 @@ static const struct of_device_id pca953x_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, pca953x_dt_ids);
 
+static SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
+
 static struct i2c_driver pca953x_driver = {
 	.driver = {
 		.name	= "pca953x",
+		.pm	= &pca953x_pm_ops,
 		.of_match_table = pca953x_dt_ids,
 		.acpi_match_table = ACPI_PTR(pca953x_acpi_ids),
 	},
-- 
2.18.0

Re: [PATCH 14/14] gpio: pca953x: Restore registers after suspend/resume cycle

[Geert Uytterhoeven]

Hi Marek,

On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> It is possible that the PCA953x is powered down during suspend.
> Use regmap cache to assure the registers in the PCA953x are in
> line with the driver state after resume.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Thanks for your series!

Background info: the main motivation for this series is to make sure SATA
keeps working after system suspend/resume on the Salvator-XS development
board, where the SATA functionality is configured using a gpio hog.

With your series applied, the SATA link seems to be functional after resume.
Dmesg difference:

     ata1: link resume succeeded after 1 retries
    -ata1: SATA link down (SStatus 0 SControl 300)
    -ata1: link resume succeeded after 1 retries
    -ata1: SATA link down (SStatus 0 SControl 300)
    -ata1: link resume succeeded after 1 retries
    -ata1: SATA link down (SStatus 0 SControl 300)
    -ata1.00: disabled
    -sd 0:0:0:0: rejecting I/O to offline device
    +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
    +ata1.00: configured for UDMA/133

However, when trying to read from an attached hard drive, it fails:

    ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
    ata1.00: failed command: READ DMA
    ata1.00: cmd c8/00:20:00:00:00/00:00:00:00:00/e0 tag 0 dma 16384 in
             res 40/00:00:01:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout)
    ata1.00: status: { DRDY }
    ata1: hard resetting link
    ata1: link resume succeeded after 1 retries
    ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
    ata1.00: configured for UDMA/133
    sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00
driverbyte=0x08
    sd 0:0:0:0: [sda] tag#0 Sense Key : 0x5 [current]
    sd 0:0:0:0: [sda] tag#0 ASC=0x21 ASCQ=0x4
    sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 00 00 00 00 20 00
    print_req_error: I/O error, dev sda, sector 0
    ata1: EH complete
    ...
    Buffer I/O error on dev sda, logical block 0, async page read

Does SATA work for you after resume!
This could still be an issue in the sata_rcar driver.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 14/14] gpio: pca953x: Restore registers after suspend/resume cycle

[Marek Vasut]

On 12/03/2018 06:55 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> It is possible that the PCA953x is powered down during suspend.
>> Use regmap cache to assure the registers in the PCA953x are in
>> line with the driver state after resume.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Thanks for your series!
> 
> Background info: the main motivation for this series is to make sure SATA
> keeps working after system suspend/resume on the Salvator-XS development
> board, where the SATA functionality is configured using a gpio hog.
> 
> With your series applied, the SATA link seems to be functional after resume.
> Dmesg difference:
> 
>      ata1: link resume succeeded after 1 retries
>     -ata1: SATA link down (SStatus 0 SControl 300)
>     -ata1: link resume succeeded after 1 retries
>     -ata1: SATA link down (SStatus 0 SControl 300)
>     -ata1: link resume succeeded after 1 retries
>     -ata1: SATA link down (SStatus 0 SControl 300)
>     -ata1.00: disabled
>     -sd 0:0:0:0: rejecting I/O to offline device
>     +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>     +ata1.00: configured for UDMA/133
> 
> However, when trying to read from an attached hard drive, it fails:
> 
>     ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
>     ata1.00: failed command: READ DMA
>     ata1.00: cmd c8/00:20:00:00:00/00:00:00:00:00/e0 tag 0 dma 16384 in
>              res 40/00:00:01:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout)
>     ata1.00: status: { DRDY }
>     ata1: hard resetting link
>     ata1: link resume succeeded after 1 retries
>     ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>     ata1.00: configured for UDMA/133
>     sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00
> driverbyte=0x08
>     sd 0:0:0:0: [sda] tag#0 Sense Key : 0x5 [current]
>     sd 0:0:0:0: [sda] tag#0 ASC=0x21 ASCQ=0x4
>     sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 00 00 00 00 20 00
>     print_req_error: I/O error, dev sda, sector 0
>     ata1: EH complete
>     ...
>     Buffer I/O error on dev sda, logical block 0, async page read
> 
> Does SATA work for you after resume!

Yes!
http://paste.debian.net/1054228/

> This could still be an issue in the sata_rcar driver.

I wonder if this has to do with the SATA link being cut off by the GPIO
mux at a bad time OR restored at a bad time.

-- 
Best regards,
Marek Vasut

Re: [PATCH 14/14] gpio: pca953x: Restore registers after suspend/resume cycle

[Geert Uytterhoeven]

Hi Marek,

On Mon, Dec 3, 2018 at 6:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> > It is possible that the PCA953x is powered down during suspend.
> > Use regmap cache to assure the registers in the PCA953x are in
> > line with the driver state after resume.
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> Thanks for your series!
>
> Background info: the main motivation for this series is to make sure SATA
> keeps working after system suspend/resume on the Salvator-XS development
> board, where the SATA functionality is configured using a gpio hog.
>
> With your series applied, the SATA link seems to be functional after resume.
> Dmesg difference:
>
>      ata1: link resume succeeded after 1 retries
>     -ata1: SATA link down (SStatus 0 SControl 300)
>     -ata1: link resume succeeded after 1 retries
>     -ata1: SATA link down (SStatus 0 SControl 300)
>     -ata1: link resume succeeded after 1 retries
>     -ata1: SATA link down (SStatus 0 SControl 300)
>     -ata1.00: disabled
>     -sd 0:0:0:0: rejecting I/O to offline device
>     +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>     +ata1.00: configured for UDMA/133
>
> However, when trying to read from an attached hard drive, it fails:
>
>     ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
>     ata1.00: failed command: READ DMA
>     ata1.00: cmd c8/00:20:00:00:00/00:00:00:00:00/e0 tag 0 dma 16384 in
>              res 40/00:00:01:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout)
>     ata1.00: status: { DRDY }
>     ata1: hard resetting link
>     ata1: link resume succeeded after 1 retries
>     ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>     ata1.00: configured for UDMA/133
>     sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00
> driverbyte=0x08
>     sd 0:0:0:0: [sda] tag#0 Sense Key : 0x5 [current]
>     sd 0:0:0:0: [sda] tag#0 ASC=0x21 ASCQ=0x4
>     sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 00 00 00 00 20 00
>     print_req_error: I/O error, dev sda, sector 0
>     ata1: EH complete
>     ...
>     Buffer I/O error on dev sda, logical block 0, async page read
>
> Does SATA work for you after resume!
> This could still be an issue in the sata_rcar driver.

FTR, it wasn't. With V3 as present in gpio/for-next, SATA works fine after
resume.
Major difference seems to be the use of regmap_bulk_read() vs. regmap_read().

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 14/14] gpio: pca953x: Restore registers after suspend/resume cycle

[Geert Uytterhoeven]

Hi Marek,

On Tue, Dec 18, 2018 at 1:57 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Dec 3, 2018 at 6:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> > > It is possible that the PCA953x is powered down during suspend.
> > > Use regmap cache to assure the registers in the PCA953x are in
> > > line with the driver state after resume.
> > >
> > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >
> > Thanks for your series!
> >
> > Background info: the main motivation for this series is to make sure SATA
> > keeps working after system suspend/resume on the Salvator-XS development
> > board, where the SATA functionality is configured using a gpio hog.
> >
> > With your series applied, the SATA link seems to be functional after resume.
> > Dmesg difference:
> >
> >      ata1: link resume succeeded after 1 retries
> >     -ata1: SATA link down (SStatus 0 SControl 300)
> >     -ata1: link resume succeeded after 1 retries
> >     -ata1: SATA link down (SStatus 0 SControl 300)
> >     -ata1: link resume succeeded after 1 retries
> >     -ata1: SATA link down (SStatus 0 SControl 300)
> >     -ata1.00: disabled
> >     -sd 0:0:0:0: rejecting I/O to offline device
> >     +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> >     +ata1.00: configured for UDMA/133
> >
> > However, when trying to read from an attached hard drive, it fails:
> >
> >     ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
> >     ata1.00: failed command: READ DMA
> >     ata1.00: cmd c8/00:20:00:00:00/00:00:00:00:00/e0 tag 0 dma 16384 in
> >              res 40/00:00:01:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout)
> >     ata1.00: status: { DRDY }
> >     ata1: hard resetting link
> >     ata1: link resume succeeded after 1 retries
> >     ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> >     ata1.00: configured for UDMA/133
> >     sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00
> > driverbyte=0x08
> >     sd 0:0:0:0: [sda] tag#0 Sense Key : 0x5 [current]
> >     sd 0:0:0:0: [sda] tag#0 ASC=0x21 ASCQ=0x4
> >     sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 00 00 00 00 20 00
> >     print_req_error: I/O error, dev sda, sector 0
> >     ata1: EH complete
> >     ...
> >     Buffer I/O error on dev sda, logical block 0, async page read
> >
> > Does SATA work for you after resume!
> > This could still be an issue in the sata_rcar driver.
>
> FTR, it wasn't. With V3 as present in gpio/for-next, SATA works fine after
> resume.
> Major difference seems to be the use of regmap_bulk_read() vs. regmap_read().

I think I found the real reason: my failing config had IPMMU enabled for SATA
virtualization, but the IPMMU driver doesn't handle suspend/resume yet.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 14/14] gpio: pca953x: Restore registers after suspend/resume cycle

[Geert Uytterhoeven]

Hi Marek,

On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> It is possible that the PCA953x is powered down during suspend.
> Use regmap cache to assure the registers in the PCA953x are in
> line with the driver state after resume.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

> +static int pca953x_suspend(struct device *dev)
> +{
> +       struct pca953x_chip *chip = dev_get_drvdata(dev);
> +
> +       regcache_mark_dirty(chip->regmap);
> +       pca953x_regcache_sync(dev);
> +       regcache_cache_only(chip->regmap, true);

Based on the discussion about adding suspend/resume support to the VC5
clock driver, I guess the two sync calls above are not needed here?

> +
> +       regulator_disable(chip->regulator);
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 14/14] gpio: pca953x: Restore registers after suspend/resume cycle

[Marek Vasut]

On 12/05/2018 03:39 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> It is possible that the PCA953x is powered down during suspend.
>> Use regmap cache to assure the registers in the PCA953x are in
>> line with the driver state after resume.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
>> +static int pca953x_suspend(struct device *dev)
>> +{
>> +       struct pca953x_chip *chip = dev_get_drvdata(dev);
>> +
>> +       regcache_mark_dirty(chip->regmap);
>> +       pca953x_regcache_sync(dev);
>> +       regcache_cache_only(chip->regmap, true);
> 
> Based on the discussion about adding suspend/resume support to the VC5
> clock driver, I guess the two sync calls above are not needed here?

Yes

-- 
Best regards,
Marek Vasut

Re: [PATCH 01/14] gpio: pca953x: Deduplicate the bank_size

[Bartosz Golaszewski]

niedz., 2 gru 2018 o 20:36 Marek Vasut <marek.vasut@gmail.com> napisał(a):
>
> The bank_size = fls(...) code was duplicated in the driver 5 times,
> pull it into separate function.
>

Shouldn't it be bank_shift in the commit message?

Bart

> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 023a32cfac42..31e3b1b52330 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -159,11 +159,16 @@ struct pca953x_chip {
>         int (*read_regs)(struct pca953x_chip *, int, u8 *);
>  };
>
> +static int pca953x_bank_shift(struct pca953x_chip *chip)
> +{
> +       return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +}
> +
>  static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
>                                 int off)
>  {
>         int ret;
> -       int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +       int bank_shift = pca953x_bank_shift(chip);
>         int offset = off / BANK_SZ;
>
>         ret = i2c_smbus_read_byte_data(chip->client,
> @@ -182,7 +187,7 @@ static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
>                                 int off)
>  {
>         int ret;
> -       int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +       int bank_shift = pca953x_bank_shift(chip);
>         int offset = off / BANK_SZ;
>
>         ret = i2c_smbus_write_byte_data(chip->client,
> @@ -221,7 +226,7 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>
>  static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>  {
> -       int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +       int bank_shift = pca953x_bank_shift(chip);
>         int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>         int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>
> @@ -265,7 +270,7 @@ static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>
>  static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>  {
> -       int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +       int bank_shift = pca953x_bank_shift(chip);
>         int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>         int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>
> @@ -402,13 +407,12 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
>                                       unsigned long *mask, unsigned long *bits)
>  {
>         struct pca953x_chip *chip = gpiochip_get_data(gc);
> +       int bank_shift = pca953x_bank_shift(chip);
>         unsigned int bank_mask, bank_val;
> -       int bank_shift, bank;
> +       int bank;
>         u8 reg_val[MAX_BANK];
>         int ret;
>
> -       bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> -
>         mutex_lock(&chip->i2c_lock);
>         memcpy(reg_val, chip->reg_output, NBANK(chip));
>         for (bank = 0; bank < NBANK(chip); bank++) {
> --
> 2.18.0
>

Re: [PATCH 01/14] gpio: pca953x: Deduplicate the bank_size

[Marek Vasut]

On 12/03/2018 10:36 AM, Bartosz Golaszewski wrote:
> niedz., 2 gru 2018 o 20:36 Marek Vasut <marek.vasut@gmail.com> napisał(a):
>>
>> The bank_size = fls(...) code was duplicated in the driver 5 times,
>> pull it into separate function.
>>
> 
> Shouldn't it be bank_shift in the commit message?

It should, fixed

-- 
Best regards,
Marek Vasut