pcal6524 extensions and fixes for pca953x driver

PATCH In review H. Nikolaus Schaller 5 participants 13 messages May 17, 2018 – Jun 6, 2018 GPIO

lore

Share & tools

lore

H. Nikolaus Schaller

author

2018-05-17 04:59 UTC #0 lore raw

v7 cover pcal6524 extensions and fixes for pca953x driver

V7:
* replace PCAL register masks by hex constants

2018-05-16 19:01:29: V6:

* added proper attribution to the formula used for fixing the pcal6524 register address (changes commit message only)
* add back missing first patch from V2 that defines the PCA_LATCH_INT constant
* removed patches already merged

2018-04-28 18:33:42: V5:

* fix wrong split up between patches 1/7 and 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 (3):
gpio: pca953x: set the PCA_PCAL flag also when matching by DT
gpio: pca953x: define masks for addressing common and extended
registers
gpio: pca953x: fix address calculation for pcal6524

 drivers/gpio/gpio-pca953x.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Signature · 1 line

2.12.2

H. Nikolaus Schaller

author

2018-05-17 04:59 UTC #1 lore raw

PATCH v7 1/3 gpio: pca953x: set the PCA_PCAL flag also when matching by DT

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 <[email protected]>

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

drivers/gpio/gpio-pca953x.c +3 -2

index 7d37692d672e..2b667166e855 100644
@@ -58,6 +58,7 @@
 #define PCA_GPIO_MASK		0x00FF
 #define PCA_INT			0x0100
 #define PCA_PCAL		0x0200
+#define PCA_LATCH_INT (PCA_PCAL | PCA_INT)
 #define PCA953X_TYPE		0x1000
 #define PCA957X_TYPE		0x2000
 #define PCA_TYPE_MASK		0xF000

@@ -946,8 +947,8 @@ 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_LATCH_INT), },
+	{ .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_LATCH_INT), },
 
 	{ .compatible = "maxim,max7310", .data = OF_953X( 8, 0), },
 	{ .compatible = "maxim,max7312", .data = OF_953X(16, PCA_INT), },

Signature · 1 line

2.12.2

Linus Walleij

2018-05-23 11:46 UTC #2 lore raw

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

Quoted 6 lines · 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.
Suggested-by Andy Shevchenko <[email protected]>
Signed-off-by H. Nikolaus Schaller <[email protected]>

Patch applied.

Yours,
Linus Walleij

H. Nikolaus Schaller

author

2018-05-17 04:59 UTC #3 lore raw

PATCH v7 2/3 gpio: pca953x: define masks for addressing common and extended registers

In reply to 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).

Reviewed-by Andy Shevchenko <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/gpio/gpio-pca953x.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpio/gpio-pca953x.c +4 -0

index 2b667166e855..c682921d7019 100644
@@ -56,6 +56,10 @@
 #define PCAL6524_DEBOUNCE	0x2d
 
 #define PCA_GPIO_MASK		0x00FF
+
+#define PCAL_GPIO_MASK		0x1f
+#define PCAL_PINCTRL_MASK	0xe0
+
 #define PCA_INT			0x0100
 #define PCA_PCAL		0x0200
 #define PCA_LATCH_INT (PCA_PCAL | PCA_INT)

Signature · 1 line

2.12.2

Linus Walleij

2018-05-23 11:47 UTC #4 lore raw

Re: [PATCH v7 2/3] gpio: pca953x: define masks for addressing common and extended registers

Quoted 6 lines · 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).
Reviewed-by Andy Shevchenko <[email protected]>
Signed-off-by H. Nikolaus Schaller <[email protected]>

Patch applied.

Yours,
Linus Walleij

H. Nikolaus Schaller

author

2018-05-17 04:59 UTC #5 lore raw

PATCH v7 3/3 gpio: pca953x: fix address calculation for pcal6524

In reply to 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 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 <[email protected]>.

Signed-off-by H. Nikolaus Schaller <[email protected]>

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

drivers/gpio/gpio-pca953x.c +6 -2

index c682921d7019..4ad553f4e41f 100644
@@ -222,9 +222,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);
 }

@@ -264,9 +266,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);
 }

Signature · 1 line

2.12.2

Linus Walleij

2018-05-23 11:48 UTC #6 lore raw

Re: [PATCH v7 3/3] gpio: pca953x: fix address calculation for pcal6524

Quoted 11 lines · 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.

Quoted 8 lines

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 <[email protected]>.
Suggested-by Andy Shevchenko <[email protected]>
Signed-off-by H. Nikolaus Schaller <[email protected]>

Patch applied.

Yours,
Linus Walleij

Pavel Machek

2018-05-23 14:06 UTC #7 lore raw

Re: [PATCH v7 3/3] gpio: pca953x: fix address calculation for pcal6524

In reply to H. Nikolaus Schaller

Quoted 28 lines · 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 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 <[email protected]>.
Suggested-by Andy Shevchenko <[email protected]>
Signed-off-by H. Nikolaus Schaller <[email protected]>
---
 drivers/gpio/gpio-pca953x.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/gpio/gpio-pca953x.c +0 -0
index c682921d7019..4ad553f4e41f 100644

drivers/gpio/gpio-pca953x.c static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val) · line 226

Quoted 5 lines

drivers/gpio/gpio-pca953x.c +2 -0

@@ -222,9 +222,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;

Is this reasonable to do on each register access? Compiler will not be able to optimize out fls and shifts, right?

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Andy Shevchenko

2018-06-05 15:37 UTC #8 lore raw

Re: [PATCH v7 3/3] gpio: pca953x: fix address calculation for pcal6524

Quoted 18 lines · Pavel Machek

Quoted 16 lines · 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 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 <[email protected]>.

Quoted 6 lines

Review
Quoted 3 lines
      int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
(quoted diff) +2 -0
+     int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+     int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
Is this reasonable to do on each register access? Compiler will not be able to optimize out fls and shifts, right?

On modern CPUs fls() is one assembly command. OTOH, any proposal to do this better?

What I can see is that bank_shift is invariant to the function, and maybe cached.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Pavel Machek

2018-06-05 20:39 UTC #9 lore raw

Re: [PATCH v7 3/3] gpio: pca953x: fix address calculation for pcal6524

Quoted 23 lines · Andy Shevchenko

Quoted 21 lines · Pavel Machek
Quoted 20 lines · 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 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 <[email protected]>.
      int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
(quoted diff) +2 -0
+     int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+     int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;

Quoted 8 lines

Quoted 2 lines
Is this reasonable to do on each register access? Compiler will not be able to optimize out fls and shifts, right?
On modern CPUs fls() is one assembly command. OTOH, any proposal to do this better?
What I can see is that bank_shift is invariant to the function, and maybe cached.

Yes, I thought that caching bank_shift might be good idea. I thought it was constant for given chip...

Best regards,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

H. Nikolaus Schaller

2018-06-06 05:33 UTC #10 lore raw

Re: [PATCH v7 3/3] gpio: pca953x: fix address calculation for pcal6524

Hi,

Quoted 29 lines

Am 05.06.2018 um 22:39 schrieb Pavel Machek <[email protected]>:
Quoted 26 lines · Andy Shevchenko
Quoted 24 lines · Pavel Machek
Quoted 18 lines · 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 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 <[email protected]>.
     int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
Review
Quoted 2 lines
(quoted diff) +2 -0
+     int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+     int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
Is this reasonable to do on each register access? Compiler will not be able to optimize out fls and shifts, right?

Quoted 8 lines

Quoted 5 lines
On modern CPUs fls() is one assembly command. OTOH, any proposal to do this better?
What I can see is that bank_shift is invariant to the function, and maybe cached.
Yes, I thought that caching bank_shift might be good idea. I thought it was constant for given chip...

Yes, it is an f(chip), but the question that comes to my mind is if optimization is worth any effort. This is an accessor method over i2c which tends to be slow (100 / 400kHz SCL) compared to the CPU. So saving 1 or 2 CPU cycles here doesn't seem to be a significant improvement. Maybe it is more valuable to improve the code path through the i2c core?

BR,
Nikolaus

Pavel Machek

2018-06-06 07:35 UTC #11 lore raw

Re: [PATCH v7 3/3] gpio: pca953x: fix address calculation for pcal6524

Quoted 34 lines · H. Nikolaus Schaller

Hi,
Quoted 32 lines
Am 05.06.2018 um 22:39 schrieb Pavel Machek <[email protected]>:
Quoted 29 lines · Andy Shevchenko
Quoted 18 lines · Pavel Machek
Quoted 16 lines · 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 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 <[email protected]>.
Quoted 6 lines
Review
Quoted 3 lines
     int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
(quoted diff) +2 -0
+     int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+     int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
Is this reasonable to do on each register access? Compiler will not be able to optimize out fls and shifts, right?
On modern CPUs fls() is one assembly command. OTOH, any proposal to do this better?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
What I can see is that bank_shift is invariant to the function, and maybe cached.
Yes, I thought that caching bank_shift might be good idea. I thought it was constant for given chip...
Yes, it is an f(chip), but the question that comes to my mind is if optimization is worth any effort. This is an accessor method over

It will also be less ugly. Copy&pasted complex exprepsion all over the driver is not nice.
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Andy Shevchenko

Reviewed-by on the v7 series

2018-05-19 18:58 UTC #12 lore raw

In reply to H. Nikolaus Schaller

Quoted 3 lines · H. Nikolaus Schaller

V7:
* replace PCAL register masks by hex constants

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 50 lines

2018-05-16 19:01:29: V6:
* added proper attribution to the formula used for fixing the pcal6524 register address (changes commit message only)
* add back missing first patch from V2 that defines the PCA_LATCH_INT constant
* removed patches already merged
2018-04-28 18:33:42: V5:
* fix wrong split up between patches 1/7 and 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 (3):
gpio: pca953x: set the PCA_PCAL flag also when matching by DT
gpio: pca953x: define masks for addressing common and extended
registers
gpio: pca953x: fix address calculation for pcal6524
 drivers/gpio/gpio-pca953x.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
--
2.12.2

Signature · 2 lines

With Best Regards,
Andy Shevchenko