[PATCH 2/5] dt-bindings: gpio: pca95xx: add entry for pcal6534 and PI4IOE5V6534Q

[PATCH 2/5] dt-bindings: gpio: pca95xx: add entry for pcal6534 and PI4IOE5V6534Q

[Martyn Welch]

The NXP PCAL6534 is a 34-bit I2C I/O expander similar to the PCAL6524. The
Diodes PI4IOE5V6534Q is a functionally identical chip provided by Diodes
Inc.

Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
---
 Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml b/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
index 977b14db09b0..b8106348e025 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
@@ -16,6 +16,7 @@ description: |+
 properties:
   compatible:
     enum:
+      - diodes,pi4ioe5v6534q
       - exar,xra1202
       - maxim,max7310
       - maxim,max7312
@@ -49,6 +50,7 @@ properties:
       - nxp,pca9698
       - nxp,pcal6416
       - nxp,pcal6524
+      - nxp,pcal6534
       - nxp,pcal9535
       - nxp,pcal9554b
       - nxp,pcal9555a
-- 
2.35.1

[PATCH 3/5] gpio: pca953x: Fix pca953x_gpio_set_pull_up_down()

[Martyn Welch]

A previous fix (dc87f6dd058a gpio: pca953x: Fix pca953x_gpio_set_config)
identified that pinconf_to_config_param() needed to be used to isolate
the config_param from the pinconf in pca953x_gpio_set_config(). This fix
however did not consider that this would also be needed in
pca953x_gpio_set_pull_up_down() to which it passes this config.

Perform a similar call in pca953x_gpio_set_pull_up_down() to isolate the
configuration parameter there as well, rather than passing it from
pca953x_gpio_set_config() as the configuration argument may also be needed
in pca953x_gpio_set_pull_up_down() at a later date.

Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
---
 drivers/gpio/gpio-pca953x.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index ecd7d169470b..41e7ff83a735 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -551,6 +551,7 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 	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);
+	enum pin_config_param param = pinconf_to_config_param(config);
 	int ret;
 
 	/*
@@ -563,9 +564,9 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 	mutex_lock(&chip->i2c_lock);
 
 	/* Configure pull-up/pull-down */
-	if (config == PIN_CONFIG_BIAS_PULL_UP)
+	if (param == PIN_CONFIG_BIAS_PULL_UP)
 		ret = regmap_write_bits(chip->regmap, pull_sel_reg, bit, bit);
-	else if (config == PIN_CONFIG_BIAS_PULL_DOWN)
+	else if (param == PIN_CONFIG_BIAS_PULL_DOWN)
 		ret = regmap_write_bits(chip->regmap, pull_sel_reg, bit, 0);
 	else
 		ret = 0;
@@ -573,7 +574,7 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 		goto exit;
 
 	/* Disable/Enable pull-up/pull-down */
-	if (config == PIN_CONFIG_BIAS_DISABLE)
+	if (param == PIN_CONFIG_BIAS_DISABLE)
 		ret = regmap_write_bits(chip->regmap, pull_en_reg, bit, 0);
 	else
 		ret = regmap_write_bits(chip->regmap, pull_en_reg, bit, bit);
-- 
2.35.1

[PATCH 4/5] gpio: pca953x: Swap if statements to save later complexity

[Martyn Welch]

A later patch in the series adds support for a further chip type that
shares some similarity with the PCA953X_TYPE. In order to keep the logic
simple, swap over the if and else portions where checks are made against
PCA953X_TYPE and instead check for PCA957X_TYPE.

Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
---
 drivers/gpio/gpio-pca953x.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 41e7ff83a735..19a8eb94a629 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -293,13 +293,13 @@ 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 {
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
 		bank = PCA957x_BANK_INPUT | PCA957x_BANK_OUTPUT |
 		       PCA957x_BANK_POLARITY | PCA957x_BANK_CONFIG |
 		       PCA957x_BANK_BUSHOLD;
+	} else {
+		bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
+		       PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
 	}
 
 	if (chip->driver_data & PCA_PCAL) {
@@ -316,12 +316,12 @@ 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 {
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
 		bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY |
 			PCA957x_BANK_CONFIG | PCA957x_BANK_BUSHOLD;
+	} else {
+		bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
+			PCA953x_BANK_CONFIG;
 	}
 
 	if (chip->driver_data & PCA_PCAL)
@@ -336,10 +336,10 @@ 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
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE)
 		bank = PCA957x_BANK_INPUT;
+	else
+		bank = PCA953x_BANK_INPUT;
 
 	if (chip->driver_data & PCA_PCAL)
 		bank |= PCAL9xxx_BANK_IRQ_STAT;
@@ -1069,13 +1069,12 @@ 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.
 	 */
-
-	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
-		chip->regs = &pca953x_regs;
-		ret = device_pca95xx_init(chip, invert);
-	} else {
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
 		chip->regs = &pca957x_regs;
 		ret = device_pca957x_init(chip, invert);
+	} else {
+		chip->regs = &pca953x_regs;
+		ret = device_pca95xx_init(chip, invert);
 	}
 	if (ret)
 		goto err_exit;
-- 
2.35.1

[PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible

[Martyn Welch]

Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. These
devices, which have identical register layouts and features, are broadly a
34-bit version of the PCAL6524.

However, whilst the registers are broadly what you'd expect for a 34-bit
version of the PCAL6524, the spacing of the registers has been
compacted. This has the unfortunate effect of breaking the bit shift
based mechanism that is employed to work out register locations used by
the other chips supported by this driver, resulting in special handling
needing to be introduced in pca953x_recalc_addr() and
pca953x_check_register().

Datasheet: https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf
Datasheet: https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf
Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
---
 drivers/gpio/gpio-pca953x.c | 101 +++++++++++++++++++++++++++++++-----
 1 file changed, 89 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 19a8eb94a629..ef1f0a603007 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -66,8 +66,10 @@
 #define PCA_LATCH_INT		(PCA_PCAL | PCA_INT)
 #define PCA953X_TYPE		BIT(12)
 #define PCA957X_TYPE		BIT(13)
+#define PCAL653X_TYPE		BIT(14)
 #define PCA_TYPE_MASK		GENMASK(15, 12)
 
+
 #define PCA_CHIP_TYPE(x)	((x) & PCA_TYPE_MASK)
 
 static const struct i2c_device_id pca953x_id[] = {
@@ -91,6 +93,7 @@ static const struct i2c_device_id pca953x_id[] = {
 
 	{ "pcal6416", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
 	{ "pcal6524", 24 | PCA953X_TYPE | PCA_LATCH_INT, },
+	{ "pcal6534", 34 | PCAL653X_TYPE | PCA_LATCH_INT, },
 	{ "pcal9535", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
 	{ "pcal9554b", 8  | PCA953X_TYPE | PCA_LATCH_INT, },
 	{ "pcal9555a", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
@@ -107,6 +110,8 @@ static const struct i2c_device_id pca953x_id[] = {
 	{ "tca9539", 16 | PCA953X_TYPE | PCA_INT, },
 	{ "tca9554", 8  | PCA953X_TYPE | PCA_INT, },
 	{ "xra1202", 8  | PCA953X_TYPE },
+
+	{ "pi4ioe5v6534q", 34 | PCAL653X_TYPE | PCA_LATCH_INT, },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca953x_id);
@@ -261,20 +266,56 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
  * - Registers with bit 0x80 set, the AI bit
  *   The bit is cleared and the registers fall into one of the
  *   categories above.
+ *
+ *   Unfortunately, whilst the PCAL6534 chip (and compatibles) broadly follow
+ *   the same register layout as the PCAL6524, the spacing of the registers has
+ *   been fundamentally altered by compacting them and thus does not obey the
+ *   same rules, including being able to use bit shifting to determine bank.
+ *   These chips hence need special handling here.
  */
 
 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);
+	int bank;
+	int offset;
+
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
+		if (reg > 0x2f) {
+			/*
+			 * Reserved block between 14h and 2Fh does not align on
+			 * expected bank boundaries like other devices.
+			 */
+			int temp = reg - 0x30;
+
+			bank = temp / NBANK(chip);
+			offset = temp - (bank * NBANK(chip));
+			bank += 8;
+		} else if (reg > 0x53) {
+			/* Handle lack of reserved registers after output port
+			 * configuration register to form a bank.
+			 */
+			int temp = reg - 0x54;
+
+			bank = temp / NBANK(chip);
+			offset = temp - (bank * NBANK(chip));
+			bank += 16;
+		} else {
+			bank = reg / NBANK(chip);
+			offset = reg - (bank * NBANK(chip));
+		}
+	} else {
+		int bank_shift = pca953x_bank_shift(chip);
 
-	/* Special PCAL extended register check. */
-	if (reg & REG_ADDR_EXT) {
-		if (!(chip->driver_data & PCA_PCAL))
-			return false;
-		bank += 8;
+		bank = (reg & REG_ADDR_MASK) >> bank_shift;
+		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. */
@@ -381,10 +422,42 @@ static const struct regmap_config pca953x_ai_i2c_regmap = {
 
 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;
-	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
-	u8 regaddr = pinctrl | addr | (off / BANK_SZ);
+	int addr;
+	int pinctrl;
+	u8 regaddr;
+
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
+		/* The PCAL6534 and compatible chips have altered bank alignment that doesn't
+		 * fit within the bit shifting scheme used for other devices.
+		 */
+		addr = (reg & PCAL_GPIO_MASK) * NBANK(chip);
+
+		switch (reg) {
+		case PCAL953X_OUT_STRENGTH:
+		case PCAL953X_IN_LATCH:
+		case PCAL953X_PULL_EN:
+		case PCAL953X_PULL_SEL:
+		case PCAL953X_INT_MASK:
+		case PCAL953X_INT_STAT:
+		case PCAL953X_OUT_CONF:
+			pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1) + 0x20;
+			break;
+		case PCAL6524_INT_EDGE:
+		case PCAL6524_INT_CLR:
+		case PCAL6524_IN_STATUS:
+		case PCAL6524_OUT_INDCONF:
+		case PCAL6524_DEBOUNCE:
+			pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1) + 0x1c;
+			break;
+		}
+		regaddr = pinctrl + addr + (off / BANK_SZ);
+	} else {
+		int bank_shift = pca953x_bank_shift(chip);
+
+		addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+		pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
+		regaddr = pinctrl | addr | (off / BANK_SZ);
+	}
 
 	return regaddr;
 }
@@ -1215,6 +1288,7 @@ static int pca953x_resume(struct device *dev)
 #endif
 
 /* convenience to stop overlong match-table lines */
+#define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int))
 #define OF_953X(__nrgpio, __int) (void *)(__nrgpio | PCA953X_TYPE | __int)
 #define OF_957X(__nrgpio, __int) (void *)(__nrgpio | PCA957X_TYPE | __int)
 
@@ -1239,6 +1313,7 @@ static const struct of_device_id pca953x_dt_ids[] = {
 
 	{ .compatible = "nxp,pcal6416", .data = OF_953X(16, PCA_LATCH_INT), },
 	{ .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_LATCH_INT), },
+	{ .compatible = "nxp,pcal6534", .data = OF_653X(34, PCA_LATCH_INT), },
 	{ .compatible = "nxp,pcal9535", .data = OF_953X(16, PCA_LATCH_INT), },
 	{ .compatible = "nxp,pcal9554b", .data = OF_953X( 8, PCA_LATCH_INT), },
 	{ .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_LATCH_INT), },
@@ -1261,6 +1336,8 @@ static const struct of_device_id pca953x_dt_ids[] = {
 	{ .compatible = "onnn,pca9655", .data = OF_953X(16, PCA_INT), },
 
 	{ .compatible = "exar,xra1202", .data = OF_953X( 8, 0), },
+
+	{ .compatible = "diodes,pi4ioe5v6534q", .data = OF_653X(34, PCA_LATCH_INT), },
 	{ }
 };
 
-- 
2.35.1

Re: [PATCH 2/5] dt-bindings: gpio: pca95xx: add entry for pcal6534 and PI4IOE5V6534Q

[Krzysztof Kozlowski]

On 29/08/2022 16:39, Martyn Welch wrote:
> The NXP PCAL6534 is a 34-bit I2C I/O expander similar to the PCAL6524. The
> Diodes PI4IOE5V6534Q is a functionally identical chip provided by Diodes
> Inc.
> 
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

Then diodes should be followed by fallback (and use only one compatible
to bind).

Best regards,
Krzysztof

Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible

[Bartosz Golaszewski]

On Mon, Aug 29, 2022 at 3:39 PM Martyn Welch <martyn.welch@collabora.com> wrote:
>
> Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. These
> devices, which have identical register layouts and features, are broadly a
> 34-bit version of the PCAL6524.
>
> However, whilst the registers are broadly what you'd expect for a 34-bit
> version of the PCAL6524, the spacing of the registers has been
> compacted. This has the unfortunate effect of breaking the bit shift
> based mechanism that is employed to work out register locations used by
> the other chips supported by this driver, resulting in special handling
> needing to be introduced in pca953x_recalc_addr() and
> pca953x_check_register().
>
> Datasheet: https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf
> Datasheet: https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
> ---

Is this series complete? I don't have patch 1/5 in my inbox and
neither does patchwork.

Bart

Re: [PATCH 3/5] gpio: pca953x: Fix pca953x_gpio_set_pull_up_down()

[Linus Walleij]

On Mon, Aug 29, 2022 at 3:39 PM Martyn Welch <martyn.welch@collabora.com> wrote:

> A previous fix (dc87f6dd058a gpio: pca953x: Fix pca953x_gpio_set_config)
> identified that pinconf_to_config_param() needed to be used to isolate
> the config_param from the pinconf in pca953x_gpio_set_config(). This fix
> however did not consider that this would also be needed in
> pca953x_gpio_set_pull_up_down() to which it passes this config.
>
> Perform a similar call in pca953x_gpio_set_pull_up_down() to isolate the
> configuration parameter there as well, rather than passing it from
> pca953x_gpio_set_config() as the configuration argument may also be needed
> in pca953x_gpio_set_pull_up_down() at a later date.
>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 4/5] gpio: pca953x: Swap if statements to save later complexity

[Linus Walleij]

On Mon, Aug 29, 2022 at 3:39 PM Martyn Welch <martyn.welch@collabora.com> wrote:

> A later patch in the series adds support for a further chip type that
> shares some similarity with the PCA953X_TYPE. In order to keep the logic
> simple, swap over the if and else portions where checks are made against
> PCA953X_TYPE and instead check for PCA957X_TYPE.
>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/5] dt-bindings: gpio: pca95xx: add entry for pcal6534 and PI4IOE5V6534Q

[Linus Walleij]

On Tue, Aug 30, 2022 at 9:50 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 29/08/2022 16:39, Martyn Welch wrote:
> > The NXP PCAL6534 is a 34-bit I2C I/O expander similar to the PCAL6524. The
> > Diodes PI4IOE5V6534Q is a functionally identical chip provided by Diodes
> > Inc.
> >
> > Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
>
> Then diodes should be followed by fallback (and use only one compatible
> to bind).

Ugh I don't think we have done a very good job at providing fallbacks
(several compatibles) for this hardware. Just looking at the list makes
me suspicious.

The fallback scheme is pretty hard to maintain when vendors are a bit
unclear on whether things are really compatible or not, and sometimes
they are compatible but rather not say :(

Yours,
Linus Walleij

Re: [PATCH 2/5] dt-bindings: gpio: pca95xx: add entry for pcal6534 and PI4IOE5V6534Q

[Krzysztof Kozlowski]

On 31/08/2022 16:26, Linus Walleij wrote:
> On Tue, Aug 30, 2022 at 9:50 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 29/08/2022 16:39, Martyn Welch wrote:
>>> The NXP PCAL6534 is a 34-bit I2C I/O expander similar to the PCAL6524. The
>>> Diodes PI4IOE5V6534Q is a functionally identical chip provided by Diodes
>>> Inc.
>>>
>>> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
>>
>> Then diodes should be followed by fallback (and use only one compatible
>> to bind).
> 
> Ugh I don't think we have done a very good job at providing fallbacks
> (several compatibles) for this hardware. Just looking at the list makes
> me suspicious.
> 
> The fallback scheme is pretty hard to maintain when vendors are a bit
> unclear on whether things are really compatible or not, and sometimes
> they are compatible but rather not say :(

If you have specific+fallback compatible (e.g. diodes,pi4ioe5v6534q,
nxp,pcal6534), you can always introduce changes in the driver because it
will match to the specific one (diodes). You could even introduce
incompatible changes, if you insist, and the effect would be the same as
adding now two compatibles in the driver.

In the same time having fallback saves you useless entries in the driver
like:
{ .compatible = "nxp,pca9556", .data = OF_953X( 8, 0), }
{ .compatible = "nxp,pca9557", .data = OF_953X( 8, 0), }

I mean, really, this will grow. I was not CC-ed on the driver change -
for some reason only on some pieces of patchset - so difficult to say
how it looks here, but judging by description ("identical chip") it is
exactly the same as above.

I don't insist on it, but for most of other pieces of complex devices
(SoC IP blocks) we follow such approach.

Best regards,
Krzysztof

Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible

[Martyn Welch]

On Wed, 2022-08-31 at 13:57 +0200, Bartosz Golaszewski wrote:
> On Mon, Aug 29, 2022 at 3:39 PM Martyn Welch
> <martyn.welch@collabora.com> wrote:
> > 
> > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q.
> > These
> > devices, which have identical register layouts and features, are
> > broadly a
> > 34-bit version of the PCAL6524.
> > 
> > However, whilst the registers are broadly what you'd expect for a
> > 34-bit
> > version of the PCAL6524, the spacing of the registers has been
> > compacted. This has the unfortunate effect of breaking the bit
> > shift
> > based mechanism that is employed to work out register locations
> > used by
> > the other chips supported by this driver, resulting in special
> > handling
> > needing to be introduced in pca953x_recalc_addr() and
> > pca953x_check_register().
> > 
> > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf
> > Datasheet:
> > https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf
> > Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
> > ---
> 
> Is this series complete? I don't have patch 1/5 in my inbox and
> neither does patchwork.
> 

I used get_maintainers to generate the recipients, it's a patch for the
binding documentation relating to the driver so didn't get sent to the
same lists:

https://lore.kernel.org/lkml/20220829133923.1114555-3-martyn.welch@collabora.com/T/

Martyn

> Bart

Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible

[Bartosz Golaszewski]

On Wed, Aug 31, 2022 at 6:30 PM Martyn Welch <martyn.welch@collabora.com> wrote:
>
> On Wed, 2022-08-31 at 13:57 +0200, Bartosz Golaszewski wrote:
> > On Mon, Aug 29, 2022 at 3:39 PM Martyn Welch
> > <martyn.welch@collabora.com> wrote:
> > >
> > > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q.
> > > These
> > > devices, which have identical register layouts and features, are
> > > broadly a
> > > 34-bit version of the PCAL6524.
> > >
> > > However, whilst the registers are broadly what you'd expect for a
> > > 34-bit
> > > version of the PCAL6524, the spacing of the registers has been
> > > compacted. This has the unfortunate effect of breaking the bit
> > > shift
> > > based mechanism that is employed to work out register locations
> > > used by
> > > the other chips supported by this driver, resulting in special
> > > handling
> > > needing to be introduced in pca953x_recalc_addr() and
> > > pca953x_check_register().
> > >
> > > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf
> > > Datasheet:
> > > https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf
> > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
> > > ---
> >
> > Is this series complete? I don't have patch 1/5 in my inbox and
> > neither does patchwork.
> >
>
> I used get_maintainers to generate the recipients, it's a patch for the
> binding documentation relating to the driver so didn't get sent to the
> same lists:
>
> https://lore.kernel.org/lkml/20220829133923.1114555-3-martyn.welch@collabora.com/T/
>
> Martyn
>
> > Bart
>

Ah, ok. Just for completeness, please send the entire series to
everyone unless it's lots of patches.

Bart

Re: [PATCH 3/5] gpio: pca953x: Fix pca953x_gpio_set_pull_up_down()

[Andy Shevchenko]

On Mon, Aug 29, 2022 at 4:43 PM Martyn Welch <martyn.welch@collabora.com> wrote:
>
> A previous fix (dc87f6dd058a gpio: pca953x: Fix pca953x_gpio_set_config)

Format is wrong. Please read documentation on how to refer to the
commits in the tree.

> identified that pinconf_to_config_param() needed to be used to isolate
> the config_param from the pinconf in pca953x_gpio_set_config(). This fix
> however did not consider that this would also be needed in
> pca953x_gpio_set_pull_up_down() to which it passes this config.
>
> Perform a similar call in pca953x_gpio_set_pull_up_down() to isolate the
> configuration parameter there as well, rather than passing it from
> pca953x_gpio_set_config() as the configuration argument may also be needed
> in pca953x_gpio_set_pull_up_down() at a later date.

...

>         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);

> +       enum pin_config_param param = pinconf_to_config_param(config);

I would move it up before the u8 variable.

The code looks good otherwise.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks for fixing this!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] gpio: pca953x: Swap if statements to save later complexity

[Andy Shevchenko]

On Mon, Aug 29, 2022 at 4:43 PM Martyn Welch <martyn.welch@collabora.com> wrote:
>
> A later patch in the series adds support for a further chip type that
> shares some similarity with the PCA953X_TYPE. In order to keep the logic
> simple,

If you say so...
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> swap over the if and else portions where checks are made against
> PCA953X_TYPE and instead check for PCA957X_TYPE.
>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 41e7ff83a735..19a8eb94a629 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -293,13 +293,13 @@ 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 {
> +       if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
>                 bank = PCA957x_BANK_INPUT | PCA957x_BANK_OUTPUT |
>                        PCA957x_BANK_POLARITY | PCA957x_BANK_CONFIG |
>                        PCA957x_BANK_BUSHOLD;
> +       } else {
> +               bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
> +                      PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
>         }
>
>         if (chip->driver_data & PCA_PCAL) {
> @@ -316,12 +316,12 @@ 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 {
> +       if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
>                 bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY |
>                         PCA957x_BANK_CONFIG | PCA957x_BANK_BUSHOLD;
> +       } else {
> +               bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
> +                       PCA953x_BANK_CONFIG;
>         }
>
>         if (chip->driver_data & PCA_PCAL)
> @@ -336,10 +336,10 @@ 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
> +       if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE)
>                 bank = PCA957x_BANK_INPUT;
> +       else
> +               bank = PCA953x_BANK_INPUT;
>
>         if (chip->driver_data & PCA_PCAL)
>                 bank |= PCAL9xxx_BANK_IRQ_STAT;
> @@ -1069,13 +1069,12 @@ 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.
>          */
> -
> -       if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
> -               chip->regs = &pca953x_regs;
> -               ret = device_pca95xx_init(chip, invert);
> -       } else {
> +       if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
>                 chip->regs = &pca957x_regs;
>                 ret = device_pca957x_init(chip, invert);
> +       } else {
> +               chip->regs = &pca953x_regs;
> +               ret = device_pca95xx_init(chip, invert);
>         }
>         if (ret)
>                 goto err_exit;
> --
> 2.35.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible

[Andy Shevchenko]

On Mon, Aug 29, 2022 at 4:52 PM Martyn Welch <martyn.welch@collabora.com> wrote:
>
> Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. These
> devices, which have identical register layouts and features, are broadly a
> 34-bit version of the PCAL6524.
>
> However, whilst the registers are broadly what you'd expect for a 34-bit
> version of the PCAL6524, the spacing of the registers has been
> compacted. This has the unfortunate effect of breaking the bit shift
> based mechanism that is employed to work out register locations used by
> the other chips supported by this driver, resulting in special handling
> needing to be introduced in pca953x_recalc_addr() and
> pca953x_check_register().

This still needs an alternative deep review. I'll do my best to get
into it sooner than later.

...

>  #define PCA953X_TYPE           BIT(12)
>  #define PCA957X_TYPE           BIT(13)
> +#define PCAL653X_TYPE          BIT(14)
>  #define PCA_TYPE_MASK          GENMASK(15, 12)
>
> +

Stray change.

>  #define PCA_CHIP_TYPE(x)       ((x) & PCA_TYPE_MASK)

...

>  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);
> +       int bank;
> +       int offset;
> +
> +       if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
> +               if (reg > 0x2f) {
> +                       /*
> +                        * Reserved block between 14h and 2Fh does not align on
> +                        * expected bank boundaries like other devices.
> +                        */
> +                       int temp = reg - 0x30;
> +
> +                       bank = temp / NBANK(chip);
> +                       offset = temp - (bank * NBANK(chip));
> +                       bank += 8;
> +               } else if (reg > 0x53) {
> +                       /* Handle lack of reserved registers after output port
> +                        * configuration register to form a bank.
> +                        */
> +                       int temp = reg - 0x54;
> +
> +                       bank = temp / NBANK(chip);
> +                       offset = temp - (bank * NBANK(chip));
> +                       bank += 16;

Can we rather put this into a separate function?

> +               } else {
> +                       bank = reg / NBANK(chip);
> +                       offset = reg - (bank * NBANK(chip));
> +               }
> +       } else {
> +               int bank_shift = pca953x_bank_shift(chip);
>
> -       /* Special PCAL extended register check. */
> -       if (reg & REG_ADDR_EXT) {
> -               if (!(chip->driver_data & PCA_PCAL))
> -                       return false;
> -               bank += 8;
> +               bank = (reg & REG_ADDR_MASK) >> bank_shift;
> +               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;
> +               }
>         }

All the same, split this to be like

if (current)
 do_current_things
else
 do_new_type

...

>  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;
> -       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
> -       u8 regaddr = pinctrl | addr | (off / BANK_SZ);
> +       int addr;
> +       int pinctrl;
> +       u8 regaddr;
> +
> +       if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
> +               /* The PCAL6534 and compatible chips have altered bank alignment that doesn't
> +                * fit within the bit shifting scheme used for other devices.
> +                */
> +               addr = (reg & PCAL_GPIO_MASK) * NBANK(chip);
> +
> +               switch (reg) {
> +               case PCAL953X_OUT_STRENGTH:
> +               case PCAL953X_IN_LATCH:
> +               case PCAL953X_PULL_EN:
> +               case PCAL953X_PULL_SEL:
> +               case PCAL953X_INT_MASK:
> +               case PCAL953X_INT_STAT:
> +               case PCAL953X_OUT_CONF:
> +                       pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1) + 0x20;
> +                       break;
> +               case PCAL6524_INT_EDGE:
> +               case PCAL6524_INT_CLR:
> +               case PCAL6524_IN_STATUS:
> +               case PCAL6524_OUT_INDCONF:
> +               case PCAL6524_DEBOUNCE:
> +                       pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1) + 0x1c;
> +                       break;
> +               }
> +               regaddr = pinctrl + addr + (off / BANK_SZ);
> +       } else {
> +               int bank_shift = pca953x_bank_shift(chip);
> +
> +               addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> +               pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
> +               regaddr = pinctrl | addr | (off / BANK_SZ);
> +       }

Looking at all these, why not add the callbacks for recalc_addr and
check_reg and assign them in the ->probe()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible

[Martyn Welch]

On Thu, 2022-09-01 at 00:02 +0300, Andy Shevchenko wrote:
> On Mon, Aug 29, 2022 at 4:52 PM Martyn Welch
> <martyn.welch@collabora.com> wrote:
> > 
> > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q.
> > These
> > devices, which have identical register layouts and features, are
> > broadly a
> > 34-bit version of the PCAL6524.
> > 
> > However, whilst the registers are broadly what you'd expect for a
> > 34-bit
> > version of the PCAL6524, the spacing of the registers has been
> > compacted. This has the unfortunate effect of breaking the bit
> > shift
> > based mechanism that is employed to work out register locations
> > used by
> > the other chips supported by this driver, resulting in special
> > handling
> > needing to be introduced in pca953x_recalc_addr() and
> > pca953x_check_register().
> 
> This still needs an alternative deep review. I'll do my best to get
> into it sooner than later.
> 

Thanks much appreciated.

...

> >  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;
> > -       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
> > -       u8 regaddr = pinctrl | addr | (off / BANK_SZ);
> > +       int addr;
> > +       int pinctrl;
> > +       u8 regaddr;
> > +
> > +       if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
> > +               /* The PCAL6534 and compatible chips have altered
> > bank alignment that doesn't
> > +                * fit within the bit shifting scheme used for
> > other devices.
> > +                */
> > +               addr = (reg & PCAL_GPIO_MASK) * NBANK(chip);
> > +
> > +               switch (reg) {
> > +               case PCAL953X_OUT_STRENGTH:
> > +               case PCAL953X_IN_LATCH:
> > +               case PCAL953X_PULL_EN:
> > +               case PCAL953X_PULL_SEL:
> > +               case PCAL953X_INT_MASK:
> > +               case PCAL953X_INT_STAT:
> > +               case PCAL953X_OUT_CONF:
> > +                       pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1)
> > + 0x20;
> > +                       break;
> > +               case PCAL6524_INT_EDGE:
> > +               case PCAL6524_INT_CLR:
> > +               case PCAL6524_IN_STATUS:
> > +               case PCAL6524_OUT_INDCONF:
> > +               case PCAL6524_DEBOUNCE:
> > +                       pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1)
> > + 0x1c;
> > +                       break;
> > +               }
> > +               regaddr = pinctrl + addr + (off / BANK_SZ);
> > +       } else {
> > +               int bank_shift = pca953x_bank_shift(chip);
> > +
> > +               addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> > +               pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
> > +               regaddr = pinctrl | addr | (off / BANK_SZ);
> > +       }
> 
> Looking at all these, why not add the callbacks for recalc_addr and
> check_reg and assign them in the ->probe()?
> 

Yeah, that sounds like a good plan. I'll look into doing that.

Martyn

Re: [PATCH 2/5] dt-bindings: gpio: pca95xx: add entry for pcal6534 and PI4IOE5V6534Q

[Linus Walleij]

On Wed, Aug 31, 2022 at 3:34 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:

> If you have specific+fallback compatible (e.g. diodes,pi4ioe5v6534q,
> nxp,pcal6534), you can always introduce changes in the driver because it
> will match to the specific one (diodes). You could even introduce
> incompatible changes, if you insist, and the effect would be the same as
> adding now two compatibles in the driver.

I know, what I mean is that this binding should probably have fallback
compatibles for the TI, Exar, Onnn and Maxim compatibles listed in
the bindings, I bet they are all compatible with some nxp,pcaNNNN.

We could fix that, perhaps.

Yours,
Linus Walleij