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

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

[Martyn Welch]

From: Martyn Welch <martyn.welch@collabora.com>

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

Changes in v2:
 - Enumerate pi4ioe5v6534q as requiring pcal6534 fallback

 .../bindings/gpio/gpio-pca95xx.yaml           | 98 ++++++++++---------
 1 file changed, 52 insertions(+), 46 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml b/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
index 977b14db09b0..81140b066683 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
@@ -15,52 +15,58 @@ description: |+
 
 properties:
   compatible:
-    enum:
-      - exar,xra1202
-      - maxim,max7310
-      - maxim,max7312
-      - maxim,max7313
-      - maxim,max7315
-      - maxim,max7319
-      - maxim,max7320
-      - maxim,max7321
-      - maxim,max7322
-      - maxim,max7323
-      - maxim,max7324
-      - maxim,max7325
-      - maxim,max7326
-      - maxim,max7327
-      - nxp,pca6408
-      - nxp,pca6416
-      - nxp,pca9505
-      - nxp,pca9506
-      - nxp,pca9534
-      - nxp,pca9535
-      - nxp,pca9536
-      - nxp,pca9537
-      - nxp,pca9538
-      - nxp,pca9539
-      - nxp,pca9554
-      - nxp,pca9555
-      - nxp,pca9556
-      - nxp,pca9557
-      - nxp,pca9574
-      - nxp,pca9575
-      - nxp,pca9698
-      - nxp,pcal6416
-      - nxp,pcal6524
-      - nxp,pcal9535
-      - nxp,pcal9554b
-      - nxp,pcal9555a
-      - onnn,cat9554
-      - onnn,pca9654
-      - ti,pca6107
-      - ti,pca9536
-      - ti,tca6408
-      - ti,tca6416
-      - ti,tca6424
-      - ti,tca9539
-      - ti,tca9554
+    oneOf:
+      - items:
+        - const: diodes,pi4ioe5v6534q
+        - const: nxp,pcal6534
+      - items:
+        - enum:
+          - exar,xra1202
+          - maxim,max7310
+          - maxim,max7312
+          - maxim,max7313
+          - maxim,max7315
+          - maxim,max7319
+          - maxim,max7320
+          - maxim,max7321
+          - maxim,max7322
+          - maxim,max7323
+          - maxim,max7324
+          - maxim,max7325
+          - maxim,max7326
+          - maxim,max7327
+          - nxp,pca6408
+          - nxp,pca6416
+          - nxp,pca9505
+          - nxp,pca9506
+          - nxp,pca9534
+          - nxp,pca9535
+          - nxp,pca9536
+          - nxp,pca9537
+          - nxp,pca9538
+          - nxp,pca9539
+          - nxp,pca9554
+          - nxp,pca9555
+          - nxp,pca9556
+          - nxp,pca9557
+          - nxp,pca9574
+          - nxp,pca9575
+          - nxp,pca9698
+          - nxp,pcal6416
+          - nxp,pcal6524
+          - nxp,pcal6534
+          - nxp,pcal9535
+          - nxp,pcal9554b
+          - nxp,pcal9555a
+          - onnn,cat9554
+          - onnn,pca9654
+          - ti,pca6107
+          - ti,pca9536
+          - ti,tca6408
+          - ti,tca6416
+          - ti,tca6424
+          - ti,tca9539
+          - ti,tca9554
 
   reg:
     maxItems: 1
-- 
2.35.1

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

[Krzysztof Kozlowski]

On 06/09/2022 10:28, Martyn Welch wrote:
> From: Martyn Welch <martyn.welch@collabora.com>
> 
> 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>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

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

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 09:28:16AM +0100, Martyn Welch wrote:
> From: Martyn Welch <martyn.welch@collabora.com>
> 
> 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.

...

> +    oneOf:
> +      - items:
> +        - const: diodes,pi4ioe5v6534q
> +        - const: nxp,pcal6534

^^^

> +      - items:
> +        - enum:

> +          - nxp,pcal6534

^^^

Not sure why is this dup?

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Tue, Sep 6, 2022 at 2:19 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> On Tue, Sep 06, 2022 at 09:28:16AM +0100, Martyn Welch wrote:
> > From: Martyn Welch <martyn.welch@collabora.com>
> >
> > 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.
>
> ...
>
> > +    oneOf:
> > +      - items:
> > +        - const: diodes,pi4ioe5v6534q
> > +        - const: nxp,pcal6534
>
> ^^^
>
> > +      - items:
> > +        - enum:
>
> > +          - nxp,pcal6534
>
> ^^^
>
> Not sure why is this dup?

No that is how DT compatibles work. One version of the component,
bought from NXP will look like this:

compatible = "nxp,pcal6534";

Another version bought from diodes will look like this:

compatible = "diodes,pi4ioe5v6534q", "nxp,pcal6534";

Then the drivers are probed matching from left to right,
with the "most compatible" matching first.

This also answers your question on the implementation.

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 03:08:00PM +0200, Linus Walleij wrote:
> On Tue, Sep 6, 2022 at 2:19 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Sep 06, 2022 at 09:28:16AM +0100, Martyn Welch wrote:
> > > From: Martyn Welch <martyn.welch@collabora.com>
> > >
> > > 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.
> >
> > ...
> >
> > > +    oneOf:
> > > +      - items:
> > > +        - const: diodes,pi4ioe5v6534q
> > > +        - const: nxp,pcal6534
> >
> > ^^^
> >
> > > +      - items:
> > > +        - enum:
> >
> > > +          - nxp,pcal6534
> >
> > ^^^
> >
> > Not sure why is this dup?
> 
> No that is how DT compatibles work. One version of the component,
> bought from NXP will look like this:
> 
> compatible = "nxp,pcal6534";
> 
> Another version bought from diodes will look like this:
> 
> compatible = "diodes,pi4ioe5v6534q", "nxp,pcal6534";
> 
> Then the drivers are probed matching from left to right,
> with the "most compatible" matching first.
> 
> This also answers your question on the implementation.

Then I don't understand why the const list above is only for new chips
and not for the old one where the same can be applied.

Mysterious ways of DT...

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Tue, Sep 6, 2022 at 3:20 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> On Tue, Sep 06, 2022 at 03:08:00PM +0200, Linus Walleij wrote:
> > On Tue, Sep 6, 2022 at 2:19 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Sep 06, 2022 at 09:28:16AM +0100, Martyn Welch wrote:
> > > > From: Martyn Welch <martyn.welch@collabora.com>
> > > >
> > > > 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.
> > >
> > > ...
> > >
> > > > +    oneOf:
> > > > +      - items:
> > > > +        - const: diodes,pi4ioe5v6534q
> > > > +        - const: nxp,pcal6534
> > >
> > > ^^^
> > >
> > > > +      - items:
> > > > +        - enum:
> > >
> > > > +          - nxp,pcal6534
> > >
> > > ^^^
> > >
> > > Not sure why is this dup?
> >
> > No that is how DT compatibles work. One version of the component,
> > bought from NXP will look like this:
> >
> > compatible = "nxp,pcal6534";
> >
> > Another version bought from diodes will look like this:
> >
> > compatible = "diodes,pi4ioe5v6534q", "nxp,pcal6534";
> >
> > Then the drivers are probed matching from left to right,
> > with the "most compatible" matching first.
> >
> > This also answers your question on the implementation.
>
> Then I don't understand why the const list above is only for new chips
> and not for the old one where the same can be applied.

That's YAML. It's because the const list is the most compact way
to express two precise items following after each other, and the enum
list is an implicit list of single-item const:s, as you cannot enum
tuples.

> Mysterious ways of DT...

It's not DT, it's YAML that is mysterious. DT itself is a pretty
straight-forward
grammar, while YAML is a meta-grammar describing the DT grammar
(ML stands for Meta Language).

All meta languages are mysterious.

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 03:33:34PM +0200, Linus Walleij wrote:
> On Tue, Sep 6, 2022 at 3:20 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Sep 06, 2022 at 03:08:00PM +0200, Linus Walleij wrote:
> > > On Tue, Sep 6, 2022 at 2:19 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Tue, Sep 06, 2022 at 09:28:16AM +0100, Martyn Welch wrote:

...

> > > > > +    oneOf:
> > > > > +      - items:
> > > > > +        - const: diodes,pi4ioe5v6534q
> > > > > +        - const: nxp,pcal6534
> > > >
> > > > ^^^
> > > >
> > > > > +      - items:
> > > > > +        - enum:
> > > >
> > > > > +          - nxp,pcal6534
> > > >
> > > > ^^^
> > > >
> > > > Not sure why is this dup?
> > >
> > > No that is how DT compatibles work. One version of the component,
> > > bought from NXP will look like this:
> > >
> > > compatible = "nxp,pcal6534";
> > >
> > > Another version bought from diodes will look like this:
> > >
> > > compatible = "diodes,pi4ioe5v6534q", "nxp,pcal6534";
> > >
> > > Then the drivers are probed matching from left to right,
> > > with the "most compatible" matching first.
> > >
> > > This also answers your question on the implementation.
> >
> > Then I don't understand why the const list above is only for new chips
> > and not for the old one where the same can be applied.
> 
> That's YAML. It's because the const list is the most compact way
> to express two precise items following after each other, and the enum
> list is an implicit list of single-item const:s, as you cannot enum
> tuples.

This makes a lot of sense, thank you for explaining this. Indeed, now I
understand the absence of Diodes in the code.

> > Mysterious ways of DT...
> 
> It's not DT, it's YAML that is mysterious. DT itself is a pretty
> straight-forward
> grammar, while YAML is a meta-grammar describing the DT grammar
> (ML stands for Meta Language).
> 
> All meta languages are mysterious.

Yeah :-)

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Tue, Sep 6, 2022 at 8:33 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Sep 6, 2022 at 3:20 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Sep 06, 2022 at 03:08:00PM +0200, Linus Walleij wrote:
> > > On Tue, Sep 6, 2022 at 2:19 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Tue, Sep 06, 2022 at 09:28:16AM +0100, Martyn Welch wrote:
> > > > > From: Martyn Welch <martyn.welch@collabora.com>
> > > > >
> > > > > 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.
> > > >
> > > > ...
> > > >
> > > > > +    oneOf:
> > > > > +      - items:
> > > > > +        - const: diodes,pi4ioe5v6534q
> > > > > +        - const: nxp,pcal6534
> > > >
> > > > ^^^
> > > >
> > > > > +      - items:
> > > > > +        - enum:
> > > >
> > > > > +          - nxp,pcal6534
> > > >
> > > > ^^^
> > > >
> > > > Not sure why is this dup?
> > >
> > > No that is how DT compatibles work. One version of the component,
> > > bought from NXP will look like this:
> > >
> > > compatible = "nxp,pcal6534";
> > >
> > > Another version bought from diodes will look like this:
> > >
> > > compatible = "diodes,pi4ioe5v6534q", "nxp,pcal6534";
> > >
> > > Then the drivers are probed matching from left to right,
> > > with the "most compatible" matching first.
> > >
> > > This also answers your question on the implementation.
> >
> > Then I don't understand why the const list above is only for new chips
> > and not for the old one where the same can be applied.
>
> That's YAML. It's because the const list is the most compact way
> to express two precise items following after each other, and the enum
> list is an implicit list of single-item const:s, as you cannot enum
> tuples.
>
> > Mysterious ways of DT...
>
> It's not DT, it's YAML that is mysterious. DT itself is a pretty
> straight-forward
> grammar, while YAML is a meta-grammar describing the DT grammar

Not YAML, but json-schema is the grammar. YAML is just the file format
and it's a JSON compatible subset of YAML (no anchors, refs, tags).

> (ML stands for Meta Language).

Huh? yaml.org says: YAML Ain't Markup Language™

Rob

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

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 09:19:52AM -0500, Rob Herring wrote:
> On Tue, Sep 6, 2022 at 8:33 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Sep 6, 2022 at 3:20 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Sep 06, 2022 at 03:08:00PM +0200, Linus Walleij wrote:

...

> > > Mysterious ways of DT...
> >
> > It's not DT, it's YAML that is mysterious. DT itself is a pretty
> > straight-forward
> > grammar, while YAML is a meta-grammar describing the DT grammar
> 
> Not YAML, but json-schema is the grammar. YAML is just the file format
> and it's a JSON compatible subset of YAML (no anchors, refs, tags).
> 
> > (ML stands for Meta Language).
> 
> Huh? yaml.org says: YAML Ain't Markup Language™

But Markup != Meta :-)

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Tue, Sep 6, 2022 at 10:28 AM Martyn Welch
<martyn.welch@collabora.co.uk> wrote:

> From: Martyn Welch <martyn.welch@collabora.com>
>
> 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>
> ---
>
> Changes in v2:
>  - Enumerate pi4ioe5v6534q as requiring pcal6534 fallback

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

Yours,
Linus Walleij

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

[Rob Herring]

On Tue, 06 Sep 2022 09:28:16 +0100, Martyn Welch wrote:
> From: Martyn Welch <martyn.welch@collabora.com>
> 
> 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>
> ---
> 
> Changes in v2:
>  - Enumerate pi4ioe5v6534q as requiring pcal6534 fallback
> 
>  .../bindings/gpio/gpio-pca95xx.yaml           | 98 ++++++++++---------
>  1 file changed, 52 insertions(+), 46 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml:20:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
./Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml:23:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
./Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml:24:11: [warning] wrong indentation: expected 12 but found 10 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

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

[Martyn Welch]

From: Martyn Welch <martyn.welch@collabora.com>

A previous fix, commit 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Changes in v2:
 - Re-order enum before u8

 drivers/gpio/gpio-pca953x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index ecd7d169470b..62367c9d6e99 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -548,6 +548,8 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 					 unsigned int offset,
 					 unsigned long config)
 {
+	enum pin_config_param param = pinconf_to_config_param(config);
+
 	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);
@@ -563,9 +565,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 +575,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 v2 4/5] gpio: pca953x: Swap if statements to save later complexity

[Martyn Welch]

From: Martyn Welch <martyn.welch@collabora.com>

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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Changes in v2:
 - None

 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 62367c9d6e99..1e8f038734e0 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;
@@ -1070,13 +1070,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 v2 5/5] gpio: pca953x: Add support for PCAL6534

[Martyn Welch]

From: Martyn Welch <martyn.welch@collabora.com>

Add support for the NXP PCAL6534. This device is 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. To accommodate ths, callback
functions have been added to allow alterate implementations of
pca953x_recalc_addr() and pca953x_check_register() for the PCAL6534.

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

Changes in v2:
 - Removed stray change
 - Removed pi4ioe5v6534q ID (should use pcal6534)
 - Added callbacks to allow differing implementations of check_register and
   recalc_addr to be provided

 drivers/gpio/gpio-pca953x.c | 138 ++++++++++++++++++++++++++++++------
 1 file changed, 117 insertions(+), 21 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 1e8f038734e0..548958f1e989 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -66,6 +66,7 @@
 #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)
@@ -91,6 +92,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 +109,7 @@ static const struct i2c_device_id pca953x_id[] = {
 	{ "tca9539", 16 | PCA953X_TYPE | PCA_INT, },
 	{ "tca9554", 8  | PCA953X_TYPE | PCA_INT, },
 	{ "xra1202", 8  | PCA953X_TYPE },
+
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca953x_id);
@@ -211,6 +214,10 @@ struct pca953x_chip {
 	struct regulator *regulator;
 
 	const struct pca953x_reg_config *regs;
+
+	u8 (*recalc_addr)(struct pca953x_chip *chip, int reg , int off);
+	bool (*check_reg)(struct pca953x_chip *chip, unsigned int reg,
+		          u32 checkbank);
 };
 
 static int pca953x_bank_shift(struct pca953x_chip *chip)
@@ -288,6 +295,54 @@ static bool pca953x_check_register(struct pca953x_chip *chip, unsigned int reg,
 	return true;
 }
 
+/*
+ * 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 pcal6534_check_register(struct pca953x_chip *chip, unsigned int reg,
+				    u32 checkbank)
+{
+	int bank;
+	int offset;
+
+	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));
+	}
+
+	/* 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);
@@ -308,7 +363,7 @@ static bool pca953x_readable_register(struct device *dev, unsigned int reg)
 			PCAL9xxx_BANK_IRQ_STAT;
 	}
 
-	return pca953x_check_register(chip, reg, bank);
+	return chip->check_reg(chip, reg, bank);
 }
 
 static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
@@ -328,7 +383,7 @@ static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
 		bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN |
 			PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK;
 
-	return pca953x_check_register(chip, reg, bank);
+	return chip->check_reg(chip, reg, bank);
 }
 
 static bool pca953x_volatile_register(struct device *dev, unsigned int reg)
@@ -344,7 +399,7 @@ static bool pca953x_volatile_register(struct device *dev, unsigned int reg)
 	if (chip->driver_data & PCA_PCAL)
 		bank |= PCAL9xxx_BANK_IRQ_STAT;
 
-	return pca953x_check_register(chip, reg, bank);
+	return chip->check_reg(chip, reg, bank);
 }
 
 static const struct regmap_config pca953x_i2c_regmap = {
@@ -384,14 +439,45 @@ 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);
 
-	return regaddr;
+	return pinctrl | addr | (off / BANK_SZ);
+}
+
+/* The PCAL6534 and compatible chips have altered bank alignment that doesn't
+ * fit within the bit shifting scheme used for other devices.
+ */
+static u8 pcal6534_recalc_addr(struct pca953x_chip *chip, int reg, int off)
+{
+	int addr;
+	int pinctrl;
+
+	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;
+	}
+
+	return pinctrl + addr + (off / BANK_SZ);
 }
 
 static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
 {
-	u8 regaddr = pca953x_recalc_addr(chip, reg, 0);
+	u8 regaddr = chip->recalc_addr(chip, reg, 0);
 	u8 value[MAX_BANK];
 	int i, ret;
 
@@ -409,7 +495,7 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long
 
 static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
 {
-	u8 regaddr = pca953x_recalc_addr(chip, reg, 0);
+	u8 regaddr = chip->recalc_addr(chip, reg, 0);
 	u8 value[MAX_BANK];
 	int i, ret;
 
@@ -428,7 +514,7 @@ static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *
 static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
+	u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
 	u8 bit = BIT(off % BANK_SZ);
 	int ret;
 
@@ -442,8 +528,8 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 		unsigned off, int val)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
-	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off);
+	u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
+	u8 outreg = chip->recalc_addr(chip, chip->regs->output, off);
 	u8 bit = BIT(off % BANK_SZ);
 	int ret;
 
@@ -463,7 +549,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 inreg = pca953x_recalc_addr(chip, chip->regs->input, off);
+	u8 inreg = chip->recalc_addr(chip, chip->regs->input, off);
 	u8 bit = BIT(off % BANK_SZ);
 	u32 reg_val;
 	int ret;
@@ -480,7 +566,7 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 outreg = pca953x_recalc_addr(chip, chip->regs->output, off);
+	u8 outreg = chip->recalc_addr(chip, chip->regs->output, off);
 	u8 bit = BIT(off % BANK_SZ);
 
 	mutex_lock(&chip->i2c_lock);
@@ -491,7 +577,7 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
 static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 dirreg = pca953x_recalc_addr(chip, chip->regs->direction, off);
+	u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
 	u8 bit = BIT(off % BANK_SZ);
 	u32 reg_val;
 	int ret;
@@ -550,8 +636,8 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 {
 	enum pin_config_param param = pinconf_to_config_param(config);
 
-	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 pull_en_reg = chip->recalc_addr(chip, PCAL953X_PULL_EN, offset);
+	u8 pull_sel_reg = chip->recalc_addr(chip, PCAL953X_PULL_SEL, offset);
 	u8 bit = BIT(offset % BANK_SZ);
 	int ret;
 
@@ -914,13 +1000,13 @@ static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
 	u8 regaddr;
 	int ret;
 
-	regaddr = pca953x_recalc_addr(chip, chip->regs->output, 0);
+	regaddr = chip->recalc_addr(chip, chip->regs->output, 0);
 	ret = regcache_sync_region(chip->regmap, regaddr,
 				   regaddr + NBANK(chip) - 1);
 	if (ret)
 		goto out;
 
-	regaddr = pca953x_recalc_addr(chip, chip->regs->direction, 0);
+	regaddr = chip->recalc_addr(chip, chip->regs->direction, 0);
 	ret = regcache_sync_region(chip->regmap, regaddr,
 				   regaddr + NBANK(chip) - 1);
 	if (ret)
@@ -1039,6 +1125,14 @@ static int pca953x_probe(struct i2c_client *client,
 		regmap_config = &pca953x_i2c_regmap;
 	}
 
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
+		chip->recalc_addr = pcal6534_recalc_addr;
+		chip->check_reg = pcal6534_check_register;
+	} else {
+		chip->recalc_addr = pca953x_recalc_addr;
+		chip->check_reg = pca953x_check_register;
+	}
+
 	chip->regmap = devm_regmap_init_i2c(client, regmap_config);
 	if (IS_ERR(chip->regmap)) {
 		ret = PTR_ERR(chip->regmap);
@@ -1133,14 +1227,14 @@ static int pca953x_regcache_sync(struct device *dev)
 	 * The ordering between direction and output is important,
 	 * sync these registers first and only then sync the rest.
 	 */
-	regaddr = pca953x_recalc_addr(chip, chip->regs->direction, 0);
+	regaddr = chip->recalc_addr(chip, chip->regs->direction, 0);
 	ret = regcache_sync_region(chip->regmap, regaddr, regaddr + NBANK(chip) - 1);
 	if (ret) {
 		dev_err(dev, "Failed to sync GPIO dir registers: %d\n", ret);
 		return ret;
 	}
 
-	regaddr = pca953x_recalc_addr(chip, chip->regs->output, 0);
+	regaddr = chip->recalc_addr(chip, chip->regs->output, 0);
 	ret = regcache_sync_region(chip->regmap, regaddr, regaddr + NBANK(chip) - 1);
 	if (ret) {
 		dev_err(dev, "Failed to sync GPIO out registers: %d\n", ret);
@@ -1149,7 +1243,7 @@ static int pca953x_regcache_sync(struct device *dev)
 
 #ifdef CONFIG_GPIO_PCA953X_IRQ
 	if (chip->driver_data & PCA_PCAL) {
-		regaddr = pca953x_recalc_addr(chip, PCAL953X_IN_LATCH, 0);
+		regaddr = chip->recalc_addr(chip, PCAL953X_IN_LATCH, 0);
 		ret = regcache_sync_region(chip->regmap, regaddr,
 					   regaddr + NBANK(chip) - 1);
 		if (ret) {
@@ -1158,7 +1252,7 @@ static int pca953x_regcache_sync(struct device *dev)
 			return ret;
 		}
 
-		regaddr = pca953x_recalc_addr(chip, PCAL953X_INT_MASK, 0);
+		regaddr = chip->recalc_addr(chip, PCAL953X_INT_MASK, 0);
 		ret = regcache_sync_region(chip->regmap, regaddr,
 					   regaddr + NBANK(chip) - 1);
 		if (ret) {
@@ -1216,6 +1310,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)
 
@@ -1240,6 +1335,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), },
-- 
2.35.1

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

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 09:28:19AM +0100, Martyn Welch wrote:
> From: Martyn Welch <martyn.welch@collabora.com>
> 
> Add support for the NXP PCAL6534. This device is 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. To accommodate ths, callback
> functions have been added to allow alterate implementations of
> pca953x_recalc_addr() and pca953x_check_register() for the PCAL6534.


This looks much cleaner!

...

> @@ -107,6 +109,7 @@ static const struct i2c_device_id pca953x_id[] = {
>  	{ "tca9539", 16 | PCA953X_TYPE | PCA_INT, },
>  	{ "tca9554", 8  | PCA953X_TYPE | PCA_INT, },
>  	{ "xra1202", 8  | PCA953X_TYPE },
> +
>  	{ }

Missed Diodes?

>  };
>  MODULE_DEVICE_TABLE(i2c, pca953x_id);

...

> +	u8 (*recalc_addr)(struct pca953x_chip *chip, int reg , int off);
> +	bool (*check_reg)(struct pca953x_chip *chip, unsigned int reg,
> +		          u32 checkbank);

I would think of splitting this change. Like in a separate patch you simply
create this interface and only add what you need in the next one.

...

> +static bool pcal6534_check_register(struct pca953x_chip *chip, unsigned int reg,
> +				    u32 checkbank)
> +{
> +	int bank;
> +	int offset;
> +
> +	if (reg > 0x2f) {

I guess code read and generation wise the

	if (reg >= 0x30) {

is slightly better.

> +		/*
> +		 * 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));

Parentheses are not needed fur multiplication, but if you insist...

> +		bank += 8;

> +	} else if (reg > 0x53) {

In the similar way...

> +		/* Handle lack of reserved registers after output port
> +		 * configuration register to form a bank.
> +		 */

Comment style

/*
 * Handle...
 */

> +		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));
> +	}
> +
> +	/* 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;
> +}

...

> -	u8 regaddr = pinctrl | addr | (off / BANK_SZ);
>  
> -	return regaddr;
> +	return pinctrl | addr | (off / BANK_SZ);

Stray change, or anything I have missed?

...

> +/* The PCAL6534 and compatible chips have altered bank alignment that doesn't
> + * fit within the bit shifting scheme used for other devices.
> + */

Comment style.

...

> @@ -1240,6 +1335,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), },

Do you decide to drop Diodes compatible from the code?

-- 
With Best Regards,
Andy Shevchenko

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

[Martyn Welch]

On Tue, 2022-09-06 at 15:24 +0300, Andy Shevchenko wrote:
> On Tue, Sep 06, 2022 at 09:28:19AM +0100, Martyn Welch wrote:
> > From: Martyn Welch <martyn.welch@collabora.com>
> > 
> > Add support for the NXP PCAL6534. This device is 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. To accommodate ths,
> > callback
> > functions have been added to allow alterate implementations of
> > pca953x_recalc_addr() and pca953x_check_register() for the
> > PCAL6534.
> 
> 
> This looks much cleaner!
> 
> ...
> 
> > @@ -107,6 +109,7 @@ static const struct i2c_device_id pca953x_id[]
> > = {
> >         { "tca9539", 16 | PCA953X_TYPE | PCA_INT, },
> >         { "tca9554", 8  | PCA953X_TYPE | PCA_INT, },
> >         { "xra1202", 8  | PCA953X_TYPE },
> > +
> >         { }
> 
> Missed Diodes?
> 

Dropped as it is expected for the DTBs of devices with a pi4ioe5v6534q
also have a compatible for pcal6534. hence it's not needed in the
driver (at least until someone finds a difference which needs to be
explicitly handled for one and not the other).

> >  };
> >  MODULE_DEVICE_TABLE(i2c, pca953x_id);
> 
> ...
> 
> > +       u8 (*recalc_addr)(struct pca953x_chip *chip, int reg , int
> > off);
> > +       bool (*check_reg)(struct pca953x_chip *chip, unsigned int
> > reg,
> > +                         u32 checkbank);
> 
> I would think of splitting this change. Like in a separate patch you
> simply
> create this interface and only add what you need in the next one.
> 

Can do, though I didn't feel you were particularly fussed about me
having split that out...

> ...
> 
> > +static bool pcal6534_check_register(struct pca953x_chip *chip,
> > unsigned int reg,
> > +                                   u32 checkbank)
> > +{
> > +       int bank;
> > +       int offset;
> > +
> > +       if (reg > 0x2f) {
> 
> I guess code read and generation wise the
> 
>         if (reg >= 0x30) {
> 
> is slightly better.

OK.

> 
> > +               /*
> > +                * 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));
> 
> Parentheses are not needed fur multiplication, but if you insist...
> 



> > +               bank += 8;
> 
> > +       } else if (reg > 0x53) {
> 
> In the similar way...
> 
> > +               /* Handle lack of reserved registers after output
> > port
> > +                * configuration register to form a bank.
> > +                */
> 
> Comment style
> 
> /*
>  * Handle...
>  */
> 

ack

> > +               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));
> > +       }
> > +
> > +       /* 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;
> > +}
> 
> ...
> 
> > -       u8 regaddr = pinctrl | addr | (off / BANK_SZ);
> >  
> > -       return regaddr;
> > +       return pinctrl | addr | (off / BANK_SZ);
> 
> Stray change, or anything I have missed?
> 

Yeah, can remove that change...

> ...
> 
> > +/* The PCAL6534 and compatible chips have altered bank alignment
> > that doesn't
> > + * fit within the bit shifting scheme used for other devices.
> > + */
> 
> Comment style.
> 
> ...
> 
> > @@ -1240,6 +1335,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), },
> 
> Do you decide to drop Diodes compatible from the code?
> 

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

[Andy Shevchenko]

On Tue, Sep 06, 2022 at 03:01:51PM +0100, Martyn Welch wrote:
> On Tue, 2022-09-06 at 15:24 +0300, Andy Shevchenko wrote:
> > On Tue, Sep 06, 2022 at 09:28:19AM +0100, Martyn Welch wrote:

...

> > > +       u8 (*recalc_addr)(struct pca953x_chip *chip, int reg , int
> > > off);
> > > +       bool (*check_reg)(struct pca953x_chip *chip, unsigned int
> > > reg,
> > > +                         u32 checkbank);
> > 
> > I would think of splitting this change. Like in a separate patch you
> > simply
> > create this interface and only add what you need in the next one.
> 
> Can do, though I didn't feel you were particularly fussed about me
> having split that out...

Oh, it just would be cleaner to see what you have done for a new chip support.
Up to you and maintainers.

-- 
With Best Regards,
Andy Shevchenko