[PATCH v5 0/7] pcal6524 extensions and fixes for pca953x driver

[PATCH v5 0/7] pcal6524 extensions and fixes for pca953x driver

[H. Nikolaus Schaller]

V5:
* fix wrong split up between patches 1/7and 2/7.

2018-04-26 19:35:07: V4:
* introduced PCA_LATCH_INT constant to make of_table more
  readable (suggested by Andy Shevchenko)
* converted all register constants to hex in a separate
  patch (suggested by Andy Shevchenko)
* separated additional pcal953x and pcal6524 register
  definitions into separate patches (suggested by Andy Shevchenko)
* made special pcal6524 address adjustment more readable
  (suggested by Andy Shevchenko)
* moved gpio-controller and interrupt-controller to the
  "required" section (reviewed by Rob Herring)

2018-04-10 18:07:07: V3:
* add Reported-by: and Reviewed-by:
* fix wording for bindings description and example
* convert all register offsets to hex
* omit the LEVEL-IRQ RFC/hack commit

2018-04-04 21:00:27: 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 (7):
  gpio: pca953x: convert register constants to hex
  gpio: pca953x: add more register definitions for pcal953x
  gpio: pca953x: add more register definitions for pcal6524
  gpio: pca953x: define masks for addressing common and extended
    registers
  gpio: pca953x: fix address calculation for pcal6524
  DTS: Bindings: pca953x add 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                        | 53 ++++++++++++++--------
 2 files changed, 69 insertions(+), 18 deletions(-)

-- 
2.12.2

[PATCH v5 1/7] gpio: pca953x: convert register constants to hex

[H. Nikolaus Schaller]

which makes it easier to match them with the data sheets.

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index d02964983b5b..508d0f6dceb7 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -25,25 +25,25 @@
 
 #include <asm/unaligned.h>
 
-#define PCA953X_INPUT		0
-#define PCA953X_OUTPUT		1
-#define PCA953X_INVERT		2
-#define PCA953X_DIRECTION	3
+#define PCA953X_INPUT		0x00
+#define PCA953X_OUTPUT		0x01
+#define PCA953X_INVERT		0x02
+#define PCA953X_DIRECTION	0x03
 
 #define REG_ADDR_AI		0x80
 
-#define PCA957X_IN		0
-#define PCA957X_INVRT		1
-#define PCA957X_BKEN		2
-#define PCA957X_PUPD		3
-#define PCA957X_CFG		4
-#define PCA957X_OUT		5
-#define PCA957X_MSK		6
-#define PCA957X_INTS		7
-
-#define PCAL953X_IN_LATCH	34
-#define PCAL953X_INT_MASK	37
-#define PCAL953X_INT_STAT	38
+#define PCA957X_IN		0x00
+#define PCA957X_INVRT		0x01
+#define PCA957X_BKEN		0x02
+#define PCA957X_PUPD		0x03
+#define PCA957X_CFG		0x04
+#define PCA957X_OUT		0x05
+#define PCA957X_MSK		0x06
+#define PCA957X_INTS		0x07
+
+#define PCAL953X_IN_LATCH	0x22
+#define PCAL953X_INT_MASK	0x25
+#define PCAL953X_INT_STAT	0x26
 
 #define PCA_GPIO_MASK		0x00FF
 #define PCA_INT			0x0100
-- 
2.12.2

[PATCH v5 2/7] gpio: pca953x: add more register definitions for pcal953x

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

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 508d0f6dceb7..fecd0e0aba93 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -41,9 +41,13 @@
 #define PCA957X_MSK		0x06
 #define PCA957X_INTS		0x07
 
+#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 PCA_GPIO_MASK		0x00FF
 #define PCA_INT			0x0100
-- 
2.12.2

[PATCH v5 3/7] gpio: pca953x: add more register definitions for pcal6524

[H. Nikolaus Schaller]

The pcal6524 has another set of registers to fine control
the interrupt handling.

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index fecd0e0aba93..2b667166e855 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -49,6 +49,12 @@
 #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
 #define PCA_PCAL		0x0200
-- 
2.12.2

[PATCH v5 4/7] gpio: pca953x: define masks for addressing common and extended registers

[H. Nikolaus Schaller]

These mask bits are to be used to map the extended register
addreseses (which are defined for an unsupported 8-bit pcal chip)
to 16 and 24 bit chips (pcal6524).

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 2b667166e855..fc863faa3ce4 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -56,6 +56,9 @@
 #define PCAL6524_DEBOUNCE	0x2d
 
 #define PCA_GPIO_MASK		0x00FF
+#define PCAL_GPIO_MASK		GENMASK(4, 0)
+#define PCAL_PINCTRL_MASK	(~PCAL_GPIO_MASK)
+
 #define PCA_INT			0x0100
 #define PCA_PCAL		0x0200
 #define PCA_LATCH_INT (PCA_PCAL | PCA_INT)
-- 
2.12.2

[PATCH v5 5/7] gpio: pca953x: fix address calculation for pcal6524

[H. Nikolaus Schaller]

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

Now, the pcal6524 has 3 banks which means the relative offset
is multiplied by 4 for the standard registers.

Simply applying the bit shift to the extended registers gives
a wrong result, since the base offset is already included in
the offset.

Therefore, we add code to the 24 bit accessor functions to
adjust the register number for these exended registers.

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index fc863faa3ce4..4194495a7990 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -221,9 +221,11 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 
 	return i2c_smbus_write_i2c_block_data(chip->client,
-					      (reg << bank_shift) | REG_ADDR_AI,
+					      pinctrl | addr | REG_ADDR_AI,
 					      NBANK(chip), val);
 }
 
@@ -263,9 +265,11 @@ static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+	int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
 
 	return i2c_smbus_read_i2c_block_data(chip->client,
-					     (reg << bank_shift) | REG_ADDR_AI,
+					     pinctrl | addr | REG_ADDR_AI,
 					     NBANK(chip), val);
 }
 
-- 
2.12.2

[PATCH v5 6/7] DTS: Bindings: pca953x add an optional vcc-supply property

[H. Nikolaus Schaller]

Hardware can have a switchable Vcc supply, so let's add it to
the bindings (the current Linux driver code already supports it).

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 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 v5 7/7] 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 and
how to use them. So we provide a tested example.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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..88f228665507 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
@@ -31,6 +31,10 @@ Required properties:
 	ti,tca9554
 	onnn,pca9654
 	exar,xra1202
+ - 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.
 
 Optional properties:
  - reset-gpios: GPIO specification for the RESET input. This is an
@@ -48,3 +52,32 @@ Example:
 		interrupt-parent = <&gpio3>;
 		interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
 	};
+
+
+Example with Interrupts:
+
+
+	gpio99: 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 v5 0/7] pcal6524 extensions and fixes for pca953x driver

[Linus Walleij]

On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> V5:
> * fix wrong split up between patches 1/7and 2/7.

v5 is looking really nice, giving reviewers a chance to ACK
etc before applying, but will probably apply it anyways in a
few days.

Nice work on this series!

Yours,
Linus Walleij

Re: [PATCH v5 5/7] gpio: pca953x: fix address calculation for pcal6524

[Andy Shevchenko]

On Sat, Apr 28, 2018 at 7:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> The register constants are so far defined in a way that they fit
> for the pcal9555a when shifted by the number of banks, i.e. are
> multiplied by 2 in the accessor function.
>
> Now, the pcal6524 has 3 banks which means the relative offset
> is multiplied by 4 for the standard registers.
>
> Simply applying the bit shift to the extended registers gives
> a wrong result, since the base offset is already included in
> the offset.
>
> Therefore, we add code to the 24 bit accessor functions to
> adjust the register number for these exended registers.
>

Suggested-by ?

> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index fc863faa3ce4..4194495a7990 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -221,9 +221,11 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>  static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>  {
>         int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +       int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> +       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>
>         return i2c_smbus_write_i2c_block_data(chip->client,
> -                                             (reg << bank_shift) | REG_ADDR_AI,
> +                                             pinctrl | addr | REG_ADDR_AI,
>                                               NBANK(chip), val);
>  }
>
> @@ -263,9 +265,11 @@ static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>  static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>  {
>         int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +       int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> +       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>
>         return i2c_smbus_read_i2c_block_data(chip->client,
> -                                            (reg << bank_shift) | REG_ADDR_AI,
> +                                            pinctrl | addr | REG_ADDR_AI,
>                                              NBANK(chip), val);
>  }
>
> --
> 2.12.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 4/7] gpio: pca953x: define masks for addressing common and extended registers

[Andy Shevchenko]

On Sat, Apr 28, 2018 at 7:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> These mask bits are to be used to map the extended register
> addreseses (which are defined for an unsupported 8-bit pcal chip)
> to 16 and 24 bit chips (pcal6524).
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 2b667166e855..fc863faa3ce4 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -56,6 +56,9 @@
>  #define PCAL6524_DEBOUNCE      0x2d
>
>  #define PCA_GPIO_MASK          0x00FF

+ empty line (the above is about contents, not addresses)

> +#define PCAL_GPIO_MASK         GENMASK(4, 0)
> +#define PCAL_PINCTRL_MASK      (~PCAL_GPIO_MASK)

I'm not sure which would be better here

1) to follow existing style
0x1F
0xE0

2) to use GENMASK() in both definitions

3) as it in this patch.


Whatever Linus prefers.

> +
>  #define PCA_INT                        0x0100
>  #define PCA_PCAL               0x0200
>  #define PCA_LATCH_INT (PCA_PCAL | PCA_INT)
> --
> 2.12.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 0/7] pcal6524 extensions and fixes for pca953x driver

[Andy Shevchenko]

On Sat, Apr 28, 2018 at 7:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> V5:
> * fix wrong split up between patches 1/7and 2/7.
>
> 2018-04-26 19:35:07: V4:
> * introduced PCA_LATCH_INT constant to make of_table more
>   readable (suggested by Andy Shevchenko)
> * converted all register constants to hex in a separate
>   patch (suggested by Andy Shevchenko)
> * separated additional pcal953x and pcal6524 register
>   definitions into separate patches (suggested by Andy Shevchenko)
> * made special pcal6524 address adjustment more readable
>   (suggested by Andy Shevchenko)
> * moved gpio-controller and interrupt-controller to the
>   "required" section (reviewed by Rob Herring)
>
> 2018-04-10 18:07:07: V3:
> * add Reported-by: and Reviewed-by:
> * fix wording for bindings description and example
> * convert all register offsets to hex
> * omit the LEVEL-IRQ RFC/hack commit
>
> 2018-04-04 21:00:27: 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
>

After addressing comments for two patches,

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

for patches 1-5.

> H. Nikolaus Schaller (7):
>   gpio: pca953x: convert register constants to hex
>   gpio: pca953x: add more register definitions for pcal953x
>   gpio: pca953x: add more register definitions for pcal6524
>   gpio: pca953x: define masks for addressing common and extended
>     registers
>   gpio: pca953x: fix address calculation for pcal6524
>   DTS: Bindings: pca953x add 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                        | 53 ++++++++++++++--------
>  2 files changed, 69 insertions(+), 18 deletions(-)
>
> --
> 2.12.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 5/7] gpio: pca953x: fix address calculation for pcal6524

[H. Nikolaus Schaller]

Hi Andy,

> Am 02.05.2018 um 14:28 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
> On Sat, Apr 28, 2018 at 7:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> The register constants are so far defined in a way that they fit
>> for the pcal9555a when shifted by the number of banks, i.e. are
>> multiplied by 2 in the accessor function.
>> 
>> Now, the pcal6524 has 3 banks which means the relative offset
>> is multiplied by 4 for the standard registers.
>> 
>> Simply applying the bit shift to the extended registers gives
>> a wrong result, since the base offset is already included in
>> the offset.
>> 
>> Therefore, we add code to the 24 bit accessor functions to
>> adjust the register number for these exended registers.
>> 
> 
> Suggested-by ?

Detecting that we need to adjust the registers generally was from me,
but your suggestion of an improved formula should of course be mentioned
and appreciated!

I'll think about a good formulation for v6.

BR and thanks,
Nikolaus

> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpio/gpio-pca953x.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index fc863faa3ce4..4194495a7990 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -221,9 +221,11 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>> static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>> {
>>        int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>> +       int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>> +       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>> 
>>        return i2c_smbus_write_i2c_block_data(chip->client,
>> -                                             (reg << bank_shift) | REG_ADDR_AI,
>> +                                             pinctrl | addr | REG_ADDR_AI,
>>                                              NBANK(chip), val);
>> }
>> 
>> @@ -263,9 +265,11 @@ static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>> static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>> {
>>        int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>> +       int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>> +       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>> 
>>        return i2c_smbus_read_i2c_block_data(chip->client,
>> -                                            (reg << bank_shift) | REG_ADDR_AI,
>> +                                            pinctrl | addr | REG_ADDR_AI,
>>                                             NBANK(chip), val);
>> }
>> 
>> --
>> 2.12.2
>> 
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 4/7] gpio: pca953x: define masks for addressing common and extended registers

[H. Nikolaus Schaller]

> Am 02.05.2018 um 14:29 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
> On Sat, Apr 28, 2018 at 7:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> These mask bits are to be used to map the extended register
>> addreseses (which are defined for an unsupported 8-bit pcal chip)
>> to 16 and 24 bit chips (pcal6524).
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpio/gpio-pca953x.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index 2b667166e855..fc863faa3ce4 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -56,6 +56,9 @@
>> #define PCAL6524_DEBOUNCE      0x2d
>> 
>> #define PCA_GPIO_MASK          0x00FF
> 
> + empty line (the above is about contents, not addresses)

ok.

> 
>> +#define PCAL_GPIO_MASK         GENMASK(4, 0)
>> +#define PCAL_PINCTRL_MASK      (~PCAL_GPIO_MASK)
> 
> I'm not sure which would be better here
> 
> 1) to follow existing style
> 0x1F
> 0xE0

I have also thought about this.

> 
> 2) to use GENMASK() in both definitions
> 
> 3) as it in this patch.
> 
> 
> Whatever Linus prefers.

Ok, waiting for his suggestion.

> 
>> +
>> #define PCA_INT                        0x0100
>> #define PCA_PCAL               0x0200
>> #define PCA_LATCH_INT (PCA_PCAL | PCA_INT)
>> --
>> 2.12.2
>> 

BR and thanks,
Nikolaus

Re: [PATCH v5 5/7] gpio: pca953x: fix address calculation for pcal6524

[H. Nikolaus Schaller]

Hi,

> Am 02.05.2018 um 14:35 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Andy,
> 
>> Am 02.05.2018 um 14:28 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>> 
>> On Sat, Apr 28, 2018 at 7:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> The register constants are so far defined in a way that they fit
>>> for the pcal9555a when shifted by the number of banks, i.e. are
>>> multiplied by 2 in the accessor function.
>>> 
>>> Now, the pcal6524 has 3 banks which means the relative offset
>>> is multiplied by 4 for the standard registers.
>>> 
>>> Simply applying the bit shift to the extended registers gives
>>> a wrong result, since the base offset is already included in
>>> the offset.
>>> 
>>> Therefore, we add code to the 24 bit accessor functions to
>>> adjust the register number for these exended registers.
>>> 
>> 
>> Suggested-by ?
> 
> Detecting that we need to adjust the registers generally was from me,
> but your suggestion of an improved formula should of course be mentioned
> and appreciated!
> 
> I'll think about a good formulation for v6.

Would the following commit message be ok for you?

...

Therefore, we have to add code to the 24 bit accessor functions
that adjusts the register number for these exended registers.

The formula finally used was developed and proposed by
Andy Shevchenko <andy.shevchenko@gmail.com>.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

BR and thanks,
Nikolaus


> 
> BR and thanks,
> Nikolaus
> 
>> 
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/gpio/gpio-pca953x.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>>> index fc863faa3ce4..4194495a7990 100644
>>> --- a/drivers/gpio/gpio-pca953x.c
>>> +++ b/drivers/gpio/gpio-pca953x.c
>>> @@ -221,9 +221,11 @@ static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>>> static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>>> {
>>>       int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>>> +       int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>>> +       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>>> 
>>>       return i2c_smbus_write_i2c_block_data(chip->client,
>>> -                                             (reg << bank_shift) | REG_ADDR_AI,
>>> +                                             pinctrl | addr | REG_ADDR_AI,
>>>                                             NBANK(chip), val);
>>> }
>>> 
>>> @@ -263,9 +265,11 @@ static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
>>> static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
>>> {
>>>       int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
>>> +       int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
>>> +       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
>>> 
>>>       return i2c_smbus_read_i2c_block_data(chip->client,
>>> -                                            (reg << bank_shift) | REG_ADDR_AI,
>>> +                                            pinctrl | addr | REG_ADDR_AI,
>>>                                            NBANK(chip), val);
>>> }
>>> 
>>> --
>>> 2.12.2
>>> 
>> 
>> 
>> 
>> -- 
>> With Best Regards,
>> Andy Shevchenko
> 
> _______________________________________________
> Kernel mailing list
> Kernel@pyra-handheld.com
> http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel

Re: [PATCH v5 4/7] gpio: pca953x: define masks for addressing common and extended registers

[H. Nikolaus Schaller]

Hi Linus,

> Am 02.05.2018 um 14:36 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> 
>> Am 02.05.2018 um 14:29 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>> 
>> On Sat, Apr 28, 2018 at 7:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> These mask bits are to be used to map the extended register
>>> addreseses (which are defined for an unsupported 8-bit pcal chip)
>>> to 16 and 24 bit chips (pcal6524).
>>> 

>>> 
>>> +#define PCAL_GPIO_MASK         GENMASK(4, 0)
>>> +#define PCAL_PINCTRL_MASK      (~PCAL_GPIO_MASK)
>> 
>> I'm not sure which would be better here
>> 
>> 1) to follow existing style
>> 0x1F
>> 0xE0
>> 
>> 2) to use GENMASK() in both definitions
>> 
>> 3) as it in this patch.
>> 
>> 
>> Whatever Linus prefers.
> 
> Ok, waiting for his suggestion.

Any advice if we should change or keep this?

(Please do not merge before I submit a v6 because there are
some more suggested-by and reviewed-by).

BR and thanks,
Nikolaus

Re: [PATCH v5 5/7] gpio: pca953x: fix address calculation for pcal6524

[Andy Shevchenko]

On Fri, May 4, 2018 at 10:30 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> Am 02.05.2018 um 14:35 schrieb H. Nikolaus Schaller <hns@goldelico.com>:

>>> Suggested-by ?
>>
>> Detecting that we need to adjust the registers generally was from me,
>> but your suggestion of an improved formula should of course be mentioned
>> and appreciated!
>>
>> I'll think about a good formulation for v6.
>
> Would the following commit message be ok for you?

Yes, it's perfect.

>
> ...
>
> Therefore, we have to add code to the 24 bit accessor functions
> that adjusts the register number for these exended registers.
>
> The formula finally used was developed and proposed by
> Andy Shevchenko <andy.shevchenko@gmail.com>.
>
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/7] gpio: pca953x: convert register constants to hex

[Linus Walleij]

On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> which makes it easier to match them with the data sheets.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

Patch applied with Andy's ACK.

Yours,
Linus Walleij

Re: [PATCH v5 2/7] gpio: pca953x: add more register definitions for pcal953x

[Linus Walleij]

On Sat, Apr 28, 2018 at 6:31 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.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

Patch applied with Andy's ACK.

Yours,
Linus Walleij

Re: [PATCH v5 3/7] gpio: pca953x: add more register definitions for pcal6524

[Linus Walleij]

On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> The pcal6524 has another set of registers to fine control
> the interrupt handling.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

Patch applied with Andy's ACK.

Yours,
Linus Walleij

Re: [PATCH v5 0/7] pcal6524 extensions and fixes for pca953x driver

[Linus Walleij]

On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> V5:
> * fix wrong split up between patches 1/7and 2/7.

I applied patches 1, 2, 3 so we get some movement on the patch
set and not too much for you to rebase.

It's fine to just resend the rest next time.

Yours,
Linus Walleij

Re: [PATCH v5 6/7] DTS: Bindings: pca953x add an optional vcc-supply property

[Linus Walleij]

On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> Hardware can have a switchable Vcc supply, so let's add it to
> the bindings (the current Linux driver code already supports it).
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Patch applied.

Yours,
Linus Walleij

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

[Linus Walleij]

On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

> It is not completely obvious that these are required and
> how to use them. So we provide a tested example.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v5 0/7] pcal6524 extensions and fixes for pca953x driver

[Linus Walleij]

On Wed, May 16, 2018 at 1:53 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
>> V5:
>> * fix wrong split up between patches 1/7and 2/7.
>
> I applied patches 1, 2, 3 so we get some movement on the patch
> set and not too much for you to rebase.
>
> It's fine to just resend the rest next time.

Oh also 6,7 was ripe. So only two patches to resend :)

Yours,
Linus Walleij

Re: [PATCH v5 0/7] pcal6524 extensions and fixes for pca953x driver

[H. Nikolaus Schaller]

Hi,

> Am 16.05.2018 um 13:53 schrieb Linus Walleij <linus.walleij@linaro.org>:
> 
> On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> 
>> V5:
>> * fix wrong split up between patches 1/7and 2/7.
> 
> I applied patches 1, 2, 3 so we get some movement on the patch
> set and not too much for you to rebase.

thanks!

Well, I already had edited the commit messages for resending...

> 
> It's fine to just resend the rest next time.

There is only one point open before resending:

what is the preferred style to be used for PCAL_GPIO_MASK?

* GENMASK(4, 0)
* or 0x1f

BR,
Nikolaus

Re: [PATCH v5 0/7] pcal6524 extensions and fixes for pca953x driver

[Linus Walleij]

On Wed, May 16, 2018 at 3:32 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> Am 16.05.2018 um 13:53 schrieb Linus Walleij <linus.walleij@linaro.org>:
>> On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>
>>> V5:
>>> * fix wrong split up between patches 1/7and 2/7.
>>
>> I applied patches 1, 2, 3 so we get some movement on the patch
>> set and not too much for you to rebase.
>
> thanks!
>
> Well, I already had edited the commit messages for resending...
>
>>
>> It's fine to just resend the rest next time.
>
> There is only one point open before resending:
>
> what is the preferred style to be used for PCAL_GPIO_MASK?
>
> * GENMASK(4, 0)
> * or 0x1f

No strong opinion... sorry. Whatever you & the other driver
contributors are most convenient with.

Yours,
Linus Walleij

Re: [PATCH v5 0/7] pcal6524 extensions and fixes for pca953x driver

[H. Nikolaus Schaller]

> Am 16.05.2018 um 16:31 schrieb Linus Walleij <linus.walleij@linaro.org>:
> 
> On Wed, May 16, 2018 at 3:32 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> Am 16.05.2018 um 13:53 schrieb Linus Walleij <linus.walleij@linaro.org>:
>>> On Sat, Apr 28, 2018 at 6:31 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> 
>>>> V5:
>>>> * fix wrong split up between patches 1/7and 2/7.
>>> 
>>> I applied patches 1, 2, 3 so we get some movement on the patch
>>> set and not too much for you to rebase.
>> 
>> thanks!
>> 
>> Well, I already had edited the commit messages for resending...
>> 
>>> 
>>> It's fine to just resend the rest next time.
>> 
>> There is only one point open before resending:
>> 
>> what is the preferred style to be used for PCAL_GPIO_MASK?
>> 
>> * GENMASK(4, 0)
>> * or 0x1f
> 
> No strong opinion... sorry. Whatever you & the other driver
> contributors are most convenient with.

Ok, then let's keep the GENMASK(4, 0) since nobody did complain
about it.

BR and thanks,
Nikolaus