[PATCH v2 0/5] none

[PATCH v2 0/5] none

[H. Nikolaus Schaller]

V2:
* added PCA_PCAL flags if matched through of-table
* fix address calculation for extended PCAL6524 registers
* hack to map LEVEL_LOW to EDGE_FALLING to be able to
  test in combination with ts3a227e driver
* improve description of bindings for optional vcc-supply
  and interrupt-controller;

2018-03-10 09:32:53: no initial description

H. Nikolaus Schaller (5):
  gpio: pca953x: set the PCA_PCAL flag also when matching by DT
  gpio: pca953x: add register definitions for pcal6524 and fix address
    calculation
  [RFC] gpio: pca953x: hack to map LEVEL irqs to EDGE irqs (may loose
    interrupts/hang)
  DTS: Bindings: pca953x driver provides an optional vcc-supply property
  DTS: Bindings: pca953x: add example how to use interrupt-controller
    and gpio-controller

 .../devicetree/bindings/gpio/gpio-pca953x.txt      | 34 +++++++++++++++++++++
 drivers/gpio/gpio-pca953x.c                        | 35 ++++++++++++++++++----
 2 files changed, 64 insertions(+), 5 deletions(-)

-- 
2.12.2

[PATCH v2 1/5] gpio: pca953x: set the PCA_PCAL flag also when matching by DT

[H. Nikolaus Schaller]

The of_device_table is missing the PCA_PCAL flag so the
pcal6524 would be operated in tca6424 compatibility mode which
does not handle the new interrupt mask registers.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpio/gpio-pca953x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index d2ead4b1cf61..4a075619b93e 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -936,8 +936,10 @@ static const struct of_device_id pca953x_dt_ids[] = {
 	{ .compatible = "nxp,pca9575", .data = OF_957X(16, PCA_INT), },
 	{ .compatible = "nxp,pca9698", .data = OF_953X(40, 0), },
 
-	{ .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_INT), },
-	{ .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_INT), },
+	{ .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_INT |
+							    PCA_PCAL), },
+	{ .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_INT |
+							     PCA_PCAL), },
 
 	{ .compatible = "maxim,max7310", .data = OF_953X( 8, 0), },
 	{ .compatible = "maxim,max7312", .data = OF_953X(16, PCA_INT), },
-- 
2.12.2

[PATCH v2 2/5] gpio: pca953x: add register definitions for pcal6524 and fix address calculation

[H. Nikolaus Schaller]

PCAL chips ("L" seems to stand for "latched") have additional
registers starting at address 0x40 to control the latches,
interrupt mask, pull-up and pull down etc.

The constants are so far defined in a way that they fit for
the pcal9555a when shifted by the number of banks, i.e. multiplied
by 2.

Now the pcal6524 has 3 banks which means the relative offset
must be multiplied by 4 which gives a wrong result if not done
carefully, since the base offset is already included in the offset.

For the basic registers shared with all pca93xx/tca64xx chips
there is no such offset.

Therefore, we add code to adjust the register number for exended
registers in this case.

And we add additional register offset constants (not yet used by
the driver code) which are specific to the pcal6524.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpio/gpio-pca953x.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4a075619b93e..c70acba710c7 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -41,9 +41,19 @@
 #define PCA957X_MSK		6
 #define PCA957X_INTS		7
 
-#define PCAL953X_IN_LATCH	34
-#define PCAL953X_INT_MASK	37
-#define PCAL953X_INT_STAT	38
+#define PCAL953X_OUT_STRENGTH	0x20
+#define PCAL953X_IN_LATCH	0x22
+#define PCAL953X_PULL_EN	0x23
+#define PCAL953X_PULL_SEL	0x24
+#define PCAL953X_INT_MASK	0x25
+#define PCAL953X_INT_STAT	0x26
+#define PCAL953X_OUT_CONF	0x27
+
+#define PCAL6524_INT_EDGE	0x28
+#define PCAL6524_INT_CLR	0x2a
+#define PCAL6524_IN_STATUS	0x2b
+#define PCAL6524_OUT_INDCONF	0x2c
+#define PCAL6524_DEBOUNCE	0x2d
 
 #define PCA_GPIO_MASK		0x00FF
 #define PCA_INT			0x0100
@@ -208,6 +218,10 @@ 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);
 
+	/* adjust register address for pcal6524 */
+	if (reg >= PCAL953X_OUT_STRENGTH)
+		reg -= PCAL953X_OUT_STRENGTH >> 1;
+
 	return i2c_smbus_write_i2c_block_data(chip->client,
 					      (reg << bank_shift) | REG_ADDR_AI,
 					      NBANK(chip), val);
@@ -250,6 +264,10 @@ 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);
 
+	/* adjust register address for pcal6524 */
+	if (reg >= PCAL953X_OUT_STRENGTH)
+		reg -= PCAL953X_OUT_STRENGTH >> 1;
+
 	return i2c_smbus_read_i2c_block_data(chip->client,
 					     (reg << bank_shift) | REG_ADDR_AI,
 					     NBANK(chip), val);
-- 
2.12.2

[PATCH v2 3/5] [RFC] gpio: pca953x: hack to map LEVEL irqs to EDGE irqs (may loose interrupts/hang)

[H. Nikolaus Schaller]

So far the interrupt type from the DTS is ignored, i.e.

	interrupt-parent = <&pcal6524>
	interrupts = <10 IRQ_TYPE_EDGE_RISING>

does not overwrite a

	devm_request_threaded_irq(..., IRQ_TYPE_LEVEL_LOW, ...)

in driver code. Therefore, the pca953x driver rejects the
setup of the irq because it can only handle EDGE interrupts
so far.

This hack translates level interrupts to edge interrupts
for the pca953x chips. This is enough for initial testing,
but not a good solution since interrupts may be lost.

If for example the connected chip requests a IRQ_TYPE_LEVEL_LOW
this may have the reason that there may be multiple different
interrupt sources in the chip - wired-or together to the
input of the pca953x. Now if we do edge detecion only,
the first interrupt will generate an EDGE_FALLING, but a
second one won't ever if the first interrupt wasn't already
processed.

IMHO a better solution would be to make the pca953x interrupt
handler check if the irq input is still in the active
level and run the device specific handler again.

But this is a major rework that I must leave to others
with more knowledge and time to work on it. This RFC
patch is for having a reminder.

Not Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpio/gpio-pca953x.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index c70acba710c7..c21cfdee2eb6 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -521,6 +521,11 @@ static int pca953x_irq_set_type(struct irq_data *d, unsigned int type)
 	int bank_nb = d->hwirq / BANK_SZ;
 	u8 mask = 1 << (d->hwirq % BANK_SZ);
 
+	if (type & IRQ_TYPE_LEVEL_LOW)
+		type |= IRQ_TYPE_EDGE_FALLING;
+	if (type & IRQ_TYPE_LEVEL_HIGH)
+		type |= IRQ_TYPE_EDGE_RISING;
+
 	if (!(type & IRQ_TYPE_EDGE_BOTH)) {
 		dev_err(&chip->client->dev, "irq %d: unsupported type %d\n",
 			d->irq, type);
-- 
2.12.2

[PATCH v2 4/5] DTS: Bindings: pca953x driver provides an optional vcc-supply property

[H. Nikolaus Schaller]

So let's describe this property in the bindings.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 Documentation/devicetree/bindings/gpio/gpio-pca953x.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
index d2a937682836..6a7cddb187c1 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
@@ -35,6 +35,7 @@ Required properties:
 Optional properties:
  - reset-gpios: GPIO specification for the RESET input. This is an
 		active low signal to the PCA953x.
+ - vcc-supply:	power supply regulator.
 
 Example:
 
-- 
2.12.2

[PATCH v2 5/5] DTS: Bindings: pca953x: add example how to use interrupt-controller and gpio-controller

[H. Nikolaus Schaller]

It is not completely obvious that these are required as
some .dts files don't specify them.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../devicetree/bindings/gpio/gpio-pca953x.txt      | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
index 6a7cddb187c1..7f3b50a6d681 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
@@ -36,6 +36,10 @@ Optional properties:
  - reset-gpios: GPIO specification for the RESET input. This is an
 		active low signal to the PCA953x.
  - vcc-supply:	power supply regulator.
+ - gpio-controller: if used as gpio expander.
+ - #gpio-cells: if used as gpio expander.
+ - interrupt-controller: if to be used as interrupt expander.
+ - #interrupt-cells: if to be used as interrupt expander.
 
 Example:
 
@@ -48,3 +52,32 @@ Example:
 		interrupt-parent = <&gpio3>;
 		interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
 	};
+
+
+Example with Interrupts:
+
+
+	gpio99: tca6424@22 {
+		compatible = "nxp,pcal6524";
+		reg = <0x22>;
+		interrupt-parent = <&gpio6>;
+		interrupts = <1 IRQ_TYPE_EDGE_FALLING>;	/* gpio6_161 */
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		vcc-supply = <&vdds_1v8_main>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio-line-names =
+			"hdmi-ct-hpd", "hdmi.ls-oe", "p02", "p03", "vibra", "fault2", "p06", "p07",
+			"en-usb", "en-host1", "en-host2", "chg-int", "p14", "p15", "mic-int", "en-modem",
+			"shdn-hs-amp", "chg-status+red", "green", "blue", "en-esata", "fault1", "p26", "p27";
+	};
+
+	ts3a227@3b {
+		compatible = "ti,ts3a227e";
+		reg = <0x3b>;
+		interrupt-parent = <&gpio99>;
+		interrupts = <14 IRQ_TYPE_EDGE_RISING>;
+		ti,micbias = <0>;	/* 2.1V */
+	};
+
-- 
2.12.2

Re: [PATCH v2 0/5] pcal6524 extensions for pca953x

[H. Nikolaus Schaller]

> Am 04.04.2018 um 21:00 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> V2:
> * added PCA_PCAL flags if matched through of-table
> * fix address calculation for extended PCAL6524 registers
> * hack to map LEVEL_LOW to EDGE_FALLING to be able to
>  test in combination with ts3a227e driver
> * improve description of bindings for optional vcc-supply
>  and interrupt-controller;
> 
> 2018-03-10 09:32:53: no initial description
> 
> H. Nikolaus Schaller (5):
>  gpio: pca953x: set the PCA_PCAL flag also when matching by DT
>  gpio: pca953x: add register definitions for pcal6524 and fix address
>    calculation
>  [RFC] gpio: pca953x: hack to map LEVEL irqs to EDGE irqs (may loose
>    interrupts/hang)
>  DTS: Bindings: pca953x driver provides an optional vcc-supply property
>  DTS: Bindings: pca953x: add example how to use interrupt-controller
>    and gpio-controller
> 
> .../devicetree/bindings/gpio/gpio-pca953x.txt      | 34 +++++++++++++++++++++
> drivers/gpio/gpio-pca953x.c                        | 35 ++++++++++++++++++----
> 2 files changed, 64 insertions(+), 5 deletions(-)
> 
> -- 
> 2.12.2
> 

Re: [PATCH v2 4/5] DTS: Bindings: pca953x driver provides an optional vcc-supply property

[Rob Herring]

On Wed, Apr 04, 2018 at 09:00:23PM +0200, H. Nikolaus Schaller wrote:
> So let's describe this property in the bindings.

It's not what the driver for some OS has. The h/w has a Vcc supply, so 
let's add a property for it...

> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio-pca953x.txt | 1 +
>  1 file changed, 1 insertion(+)

Otherwise,

Reviewed-by: Rob Herring <robh@kernel.org>

> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
> index d2a937682836..6a7cddb187c1 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
> @@ -35,6 +35,7 @@ Required properties:
>  Optional properties:
>   - reset-gpios: GPIO specification for the RESET input. This is an
>  		active low signal to the PCA953x.
> + - vcc-supply:	power supply regulator.
>  
>  Example:
>  
> -- 
> 2.12.2
> 

Re: [PATCH v2 5/5] DTS: Bindings: pca953x: add example how to use interrupt-controller and gpio-controller

[Rob Herring]

On Wed, Apr 04, 2018 at 09:00:24PM +0200, H. Nikolaus Schaller wrote:
> It is not completely obvious that these are required as
> some .dts files don't specify them.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  .../devicetree/bindings/gpio/gpio-pca953x.txt      | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
> index 6a7cddb187c1..7f3b50a6d681 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
> @@ -36,6 +36,10 @@ Optional properties:
>   - reset-gpios: GPIO specification for the RESET input. This is an
>  		active low signal to the PCA953x.
>   - vcc-supply:	power supply regulator.
> + - gpio-controller: if used as gpio expander.
> + - #gpio-cells: if used as gpio expander.
> + - interrupt-controller: if to be used as interrupt expander.
> + - #interrupt-cells: if to be used as interrupt expander.

These should be required. It's a mistake if dts files don't have them.

>  
>  Example:
>  
> @@ -48,3 +52,32 @@ Example:
>  		interrupt-parent = <&gpio3>;
>  		interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
>  	};
> +
> +
> +Example with Interrupts:
> +
> +
> +	gpio99: tca6424@22 {

gpio@22

> +		compatible = "nxp,pcal6524";
> +		reg = <0x22>;
> +		interrupt-parent = <&gpio6>;
> +		interrupts = <1 IRQ_TYPE_EDGE_FALLING>;	/* gpio6_161 */
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		vcc-supply = <&vdds_1v8_main>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-line-names =
> +			"hdmi-ct-hpd", "hdmi.ls-oe", "p02", "p03", "vibra", "fault2", "p06", "p07",
> +			"en-usb", "en-host1", "en-host2", "chg-int", "p14", "p15", "mic-int", "en-modem",
> +			"shdn-hs-amp", "chg-status+red", "green", "blue", "en-esata", "fault1", "p26", "p27";
> +	};
> +
> +	ts3a227@3b {
> +		compatible = "ti,ts3a227e";
> +		reg = <0x3b>;
> +		interrupt-parent = <&gpio99>;
> +		interrupts = <14 IRQ_TYPE_EDGE_RISING>;
> +		ti,micbias = <0>;	/* 2.1V */
> +	};
> +
> -- 
> 2.12.2
> 

Re: [PATCH v2 4/5] DTS: Bindings: pca953x driver provides an optional vcc-supply property

[H. Nikolaus Schaller]

> Am 10.04.2018 um 15:55 schrieb Rob Herring <robh@kernel.org>:
> 
> On Wed, Apr 04, 2018 at 09:00:23PM +0200, H. Nikolaus Schaller wrote:
>> So let's describe this property in the bindings.
> 
> It's not what the driver for some OS has. The h/w has a Vcc supply, so 
> let's add a property for it...

Ah, ok. It is a matter of speaker's standpoint :)

> 
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> Documentation/devicetree/bindings/gpio/gpio-pca953x.txt | 1 +
>> 1 file changed, 1 insertion(+)
> 
> Otherwise,
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks,
Nikolaus

> 
>> 
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
>> index d2a937682836..6a7cddb187c1 100644
>> --- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
>> @@ -35,6 +35,7 @@ Required properties:
>> Optional properties:
>>  - reset-gpios: GPIO specification for the RESET input. This is an
>> 		active low signal to the PCA953x.
>> + - vcc-supply:	power supply regulator.
>> 
>> Example:
>> 
>> -- 
>> 2.12.2
>> 

Re: [PATCH v2 5/5] DTS: Bindings: pca953x: add example how to use interrupt-controller and gpio-controller

[H. Nikolaus Schaller]

> Am 10.04.2018 um 15:57 schrieb Rob Herring <robh@kernel.org>:
> 
> On Wed, Apr 04, 2018 at 09:00:24PM +0200, H. Nikolaus Schaller wrote:
>> It is not completely obvious that these are required as
>> some .dts files don't specify them.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../devicetree/bindings/gpio/gpio-pca953x.txt      | 33 ++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
>> index 6a7cddb187c1..7f3b50a6d681 100644
>> --- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
>> @@ -36,6 +36,10 @@ Optional properties:
>>  - reset-gpios: GPIO specification for the RESET input. This is an
>> 		active low signal to the PCA953x.
>>  - vcc-supply:	power supply regulator.
>> + - gpio-controller: if used as gpio expander.
>> + - #gpio-cells: if used as gpio expander.
>> + - interrupt-controller: if to be used as interrupt expander.
>> + - #interrupt-cells: if to be used as interrupt expander.
> 
> These should be required. It's a mistake if dts files don't have them.

Well, it seems to be a config option if the chip handles interrupts
at all, but I also tend to say they are required.

> 
>> 
>> Example:
>> 
>> @@ -48,3 +52,32 @@ Example:
>> 		interrupt-parent = <&gpio3>;
>> 		interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
>> 	};
>> +
>> +
>> +Example with Interrupts:
>> +
>> +
>> +	gpio99: tca6424@22 {
> 
> gpio@22

Ok!

> 
>> +		compatible = "nxp,pcal6524";
>> +		reg = <0x22>;
>> +		interrupt-parent = <&gpio6>;
>> +		interrupts = <1 IRQ_TYPE_EDGE_FALLING>;	/* gpio6_161 */
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +		vcc-supply = <&vdds_1v8_main>;
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +		gpio-line-names =
>> +			"hdmi-ct-hpd", "hdmi.ls-oe", "p02", "p03", "vibra", "fault2", "p06", "p07",
>> +			"en-usb", "en-host1", "en-host2", "chg-int", "p14", "p15", "mic-int", "en-modem",
>> +			"shdn-hs-amp", "chg-status+red", "green", "blue", "en-esata", "fault1", "p26", "p27";
>> +	};
>> +
>> +	ts3a227@3b {
>> +		compatible = "ti,ts3a227e";
>> +		reg = <0x3b>;
>> +		interrupt-parent = <&gpio99>;
>> +		interrupts = <14 IRQ_TYPE_EDGE_RISING>;
>> +		ti,micbias = <0>;	/* 2.1V */
>> +	};
>> +
>> -- 
>> 2.12.2
>> 

BR and thanks,
Nikolaus

Re: [PATCH v2 3/5] [RFC] gpio: pca953x: hack to map LEVEL irqs to EDGE irqs (may loose interrupts/hang)

[Andy Shevchenko]

On Wed, Apr 4, 2018 at 10:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> in driver code. Therefore, the pca953x driver rejects the
> setup of the irq because it can only handle EDGE interrupts
> so far.
>
> This hack translates level interrupts to edge interrupts
> for the pca953x chips. This is enough for initial testing,
> but not a good solution since interrupts may be lost.
>
> If for example the connected chip requests a IRQ_TYPE_LEVEL_LOW
> this may have the reason that there may be multiple different
> interrupt sources in the chip - wired-or together to the
> input of the pca953x. Now if we do edge detecion only,
> the first interrupt will generate an EDGE_FALLING, but a
> second one won't ever if the first interrupt wasn't already
> processed.
>
> IMHO a better solution would be to make the pca953x interrupt
> handler check if the irq input is still in the active
> level and run the device specific handler again.

What I thought about is to enable such exclusively for "L" variants of
the chips and check latched values.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] gpio: pca953x: set the PCA_PCAL flag also when matching by DT

[Andy Shevchenko]

On Wed, Apr 4, 2018 at 10:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> The of_device_table is missing the PCA_PCAL flag so the
> pcal6524 would be operated in tca6424 compatibility mode which
> does not handle the new interrupt mask registers.
>

Reported-by or Suggested-by?

> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index d2ead4b1cf61..4a075619b93e 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -936,8 +936,10 @@ static const struct of_device_id pca953x_dt_ids[] = {
>         { .compatible = "nxp,pca9575", .data = OF_957X(16, PCA_INT), },
>         { .compatible = "nxp,pca9698", .data = OF_953X(40, 0), },
>
> -       { .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_INT), },
> -       { .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_INT), },
> +       { .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_INT |
> +                                                           PCA_PCAL), },
> +       { .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_INT |
> +                                                            PCA_PCAL), },
>
>         { .compatible = "maxim,max7310", .data = OF_953X( 8, 0), },
>         { .compatible = "maxim,max7312", .data = OF_953X(16, PCA_INT), },
> --
> 2.12.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] gpio: pca953x: add register definitions for pcal6524 and fix address calculation

[Andy Shevchenko]

On Wed, Apr 4, 2018 at 10:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> PCAL chips ("L" seems to stand for "latched") have additional
> registers starting at address 0x40 to control the latches,
> interrupt mask, pull-up and pull down etc.
>
> The constants are so far defined in a way that they fit for
> the pcal9555a when shifted by the number of banks, i.e. multiplied
> by 2.
>
> Now the pcal6524 has 3 banks which means the relative offset
> must be multiplied by 4 which gives a wrong result if not done
> carefully, since the base offset is already included in the offset.
>
> For the basic registers shared with all pca93xx/tca64xx chips
> there is no such offset.
>
> Therefore, we add code to adjust the register number for exended
> registers in this case.
>
> And we add additional register offset constants (not yet used by
> the driver code) which are specific to the pcal6524.

> -#define PCAL953X_IN_LATCH      34
> -#define PCAL953X_INT_MASK      37
> -#define PCAL953X_INT_STAT      38

I prefer either to change first all the rest to be 2 digit hex values
first, or leave decimal in this patch.

> +#define PCAL953X_OUT_STRENGTH  0x20
> +#define PCAL953X_IN_LATCH      0x22
> +#define PCAL953X_PULL_EN       0x23
> +#define PCAL953X_PULL_SEL      0x24
> +#define PCAL953X_INT_MASK      0x25
> +#define PCAL953X_INT_STAT      0x26
> +#define PCAL953X_OUT_CONF      0x27
> +
> +#define PCAL6524_INT_EDGE      0x28
> +#define PCAL6524_INT_CLR       0x2a
> +#define PCAL6524_IN_STATUS     0x2b
> +#define PCAL6524_OUT_INDCONF   0x2c
> +#define PCAL6524_DEBOUNCE      0x2d

> +       /* adjust register address for pcal6524 */
> +       if (reg >= PCAL953X_OUT_STRENGTH)
> +               reg -= PCAL953X_OUT_STRENGTH >> 1;

I don't like this condition. Can we avoid relying on the register
offset in regard to some flag / compatible string / etc?

> +       /* adjust register address for pcal6524 */
> +       if (reg >= PCAL953X_OUT_STRENGTH)
> +               reg -= PCAL953X_OUT_STRENGTH >> 1;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] gpio: pca953x: add register definitions for pcal6524 and fix address calculation

[H. Nikolaus Schaller]

Hi,

> Am 10.04.2018 um 16:41 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
> On Wed, Apr 4, 2018 at 10:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> PCAL chips ("L" seems to stand for "latched") have additional
>> registers starting at address 0x40 to control the latches,
>> interrupt mask, pull-up and pull down etc.
>> 
>> The constants are so far defined in a way that they fit for
>> the pcal9555a when shifted by the number of banks, i.e. multiplied
>> by 2.
>> 
>> Now the pcal6524 has 3 banks which means the relative offset
>> must be multiplied by 4 which gives a wrong result if not done
>> carefully, since the base offset is already included in the offset.
>> 
>> For the basic registers shared with all pca93xx/tca64xx chips
>> there is no such offset.
>> 
>> Therefore, we add code to adjust the register number for exended
>> registers in this case.
>> 
>> And we add additional register offset constants (not yet used by
>> the driver code) which are specific to the pcal6524.
> 
>> -#define PCAL953X_IN_LATCH      34
>> -#define PCAL953X_INT_MASK      37
>> -#define PCAL953X_INT_STAT      38
> 
> I prefer either to change first all the rest to be 2 digit hex values
> first, or leave decimal in this patch.

Ok, let's change them all to hex (like in the data sheets).

> 
>> +#define PCAL953X_OUT_STRENGTH  0x20
>> +#define PCAL953X_IN_LATCH      0x22
>> +#define PCAL953X_PULL_EN       0x23
>> +#define PCAL953X_PULL_SEL      0x24
>> +#define PCAL953X_INT_MASK      0x25
>> +#define PCAL953X_INT_STAT      0x26
>> +#define PCAL953X_OUT_CONF      0x27
>> +
>> +#define PCAL6524_INT_EDGE      0x28
>> +#define PCAL6524_INT_CLR       0x2a
>> +#define PCAL6524_IN_STATUS     0x2b
>> +#define PCAL6524_OUT_INDCONF   0x2c
>> +#define PCAL6524_DEBOUNCE      0x2d
> 
>> +       /* adjust register address for pcal6524 */
>> +       if (reg >= PCAL953X_OUT_STRENGTH)
>> +               reg -= PCAL953X_OUT_STRENGTH >> 1;
> 
> I don't like this condition. Can we avoid relying on the register
> offset in regard to some flag / compatible string / etc?

I don't think so.

for TCA6416 and PCAL9555 the real registers are e.g.:
PCA953X_INPUT		0x00 / 0x01
PCA953X_OUTPUT		0x02 / 0x03

i.e. every address constant is shifted left 1 bit in the 16 bit
accessors.

for PCAL9555 some extended registers:

PCAL953X_OUT_STRENGTH	0x40 / 0x41 / 0x42 / 0x43
PCAL953X_IN_LATCH	0x44 / 0x45

i.e. PCAL953X_IN_LATCH is also simply shifted left by 1 bit
because the extended constants are defined that they fit.

for TCA6424 and PCAL6524:
PCA953X_INPUT		0x00 / 0x01 / 0x02
PCA953X_OUTPUT		0x04 / 0x05 / 0x06

every address is shifted left 2 bits in the 24 bit accessors.

for PCAL6524:
PCAL953X_OUT_STRENGTH	0x40 / 0x41 / 0x42 / 0x43 / 0x44 / 0x45
PCAL953X_IN_LATCH	0x48 / 0x49 / 0x4a

simply shifting left by 2 bits gives wrong result which is this
special case. It is not a general chip property but a mix of chip
bit-width and depends on accessing the first or second bank.

So the full condition to apply this address fix is:

if (24 bit access && second bank access)
	do correction.

and it can be simplified to what I suggest, because the 8/16/24 bit
accessors are already separate functions.

I don't think this can't be hard-coded into flags / compatible string.

> 
>> +       /* adjust register address for pcal6524 */
>> +       if (reg >= PCAL953X_OUT_STRENGTH)
>> +               reg -= PCAL953X_OUT_STRENGTH >> 1;
> 
> Ditto.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

BR and thanks,
Nikolaus

Re: [PATCH v2 3/5] [RFC] gpio: pca953x: hack to map LEVEL irqs to EDGE irqs (may loose interrupts/hang)

[H. Nikolaus Schaller]

Hi Andy,

> Am 10.04.2018 um 16:38 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
> On Wed, Apr 4, 2018 at 10:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> 
>> in driver code. Therefore, the pca953x driver rejects the
>> setup of the irq because it can only handle EDGE interrupts
>> so far.
>> 
>> This hack translates level interrupts to edge interrupts
>> for the pca953x chips. This is enough for initial testing,
>> but not a good solution since interrupts may be lost.
>> 
>> If for example the connected chip requests a IRQ_TYPE_LEVEL_LOW
>> this may have the reason that there may be multiple different
>> interrupt sources in the chip - wired-or together to the
>> input of the pca953x. Now if we do edge detecion only,
>> the first interrupt will generate an EDGE_FALLING, but a
>> second one won't ever if the first interrupt wasn't already
>> processed.
>> 
>> IMHO a better solution would be to make the pca953x interrupt
>> handler check if the irq input is still in the active
>> level and run the device specific handler again.
> 
> What I thought about is to enable such exclusively for "L" variants of
> the chips and check latched values.

Yes, something like this, although we have to think about if the same can't
be done for other chips.

The question is who will pick up this topic and when. My job for this chip
was to check if it can basically work and the hack shows that, but others
must make the driver perfect.

So for the moment I keep it in our workplace and remove it from the
patch set.

I am sure that there will be some more kernel developers as soon
as the Pyra handheld hardware is more widely available.

BR,
Nikolaus