[PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17

Hi,
I'm in the process of adding a device tree overlay for the PiFace
Digital Raspberry Pi daughter board [0]. It's an mcp23s17 SPI GPIO port
expander. In doing so, I noticed some errors with the mcp23s08 driver.

This series adds my fixups to (which builds successfully):

	v5.8-13252-gcf4a66ae5e4a

But I have only tested them on (rpi-5.4.y):

	v5.4.51-1078-g92834e4bb4ce

They're quite trivial and backwards compatible, although I might be
wrong about "interrupt-controller". Can someone please confirm?

Many thanks,
Thomas

[0] https://github.com/raspberrypi/linux/pull/3794

Thomas Preston (3):
  pinctrl: mcp23s08: Fixup mcp23x17 regmap_config
  pinctrl: mcp23s08: Remove interrupt-controller
  devicetree: mcp23s08: Remove interrupt-controller

 .../bindings/pinctrl/pinctrl-mcp23s08.txt     |  8 -----
 drivers/pinctrl/pinctrl-mcp23s08.c            | 35 +++++++++----------
 drivers/pinctrl/pinctrl-mcp23s08.h            |  2 +-
 3 files changed, 18 insertions(+), 27 deletions(-)

-- 
2.26.2

The mcp23s08 device and friends are interrupt /client/ nodes, and should
not reference the interrupt controller device tree properties
"interrupt-controller" and "interrupt-cells" [0].

Remove the confusing "interrupt-controller" and "interrupt-cells"
properties from the pinctrl-mcp23s08 devicetree bindings documentation.

[0] Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Signed-off-by: Thomas Preston <redacted>
---
 .../devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt      | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
index 8b94aa8f5971..bb1b53030552 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt

@@ -43,10 +43,6 @@ Required device specific properties (only for SPI chips):
 - spi-max-frequency = The maximum frequency this chip is able to handle
 
 Optional properties:
-- #interrupt-cells : Should be two.
-  - first cell is the pin number
-  - second cell is used to specify flags.
-- interrupt-controller: Marks the device node as a interrupt controller.
 - drive-open-drain: Sets the ODR flag in the IOCON register. This configures
         the IRQ output as open drain active low.
 

@@ -72,8 +68,6 @@ gpiom1: gpio@20 {
 
         interrupt-parent = <&gpio1>;
         interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
-        interrupt-controller;
-        #interrupt-cells=<2>;
         microchip,irq-mirror;
 };
 

@@ -130,8 +124,6 @@ gpio21: gpio@21 {
 	interrupt-parent = <&socgpio>;
 	interrupts = <0x17 0x8>;
 	interrupt-names = "mcp23017@21 irq";
-	interrupt-controller;
-	#interrupt-cells = <0x2>;
 	microchip,irq-mirror;
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2cgpio0irq &gpio21pullups>;

-- 
2.26.2

The mcp23s08 device and friends are interrupt /client/ nodes, and should
not reference the interrupt controller device tree property
"interrupt-controller" [0].

Fix the mcp23s08 driver so that it activates interrupts when it detects
the "interrupts" property instead, which is always present if we want
interrupts enabled.

[0] Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Signed-off-by: Thomas Preston <redacted>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 7 +++----
 drivers/pinctrl/pinctrl-mcp23s08.h | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 0138638276e7..ac8926985c28 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c

@@ -566,9 +566,8 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 	if (ret < 0)
 		goto fail;
 
-	mcp->irq_controller =
-		device_property_read_bool(dev, "interrupt-controller");
-	if (mcp->irq && mcp->irq_controller) {
+	mcp->irq_enabled = device_property_present(dev, "interrupts");
+	if (mcp->irq && mcp->irq_enabled) {
 		mcp->irq_active_high =
 			device_property_read_bool(dev,
 					      "microchip,irq-active-high");

@@ -601,7 +600,7 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 			goto fail;
 	}
 
-	if (mcp->irq && mcp->irq_controller) {
+	if (mcp->irq && mcp->irq_enabled) {
 		struct gpio_irq_chip *girq = &mcp->chip.irq;
 
 		girq->chip = &mcp->irq_chip;

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.h b/drivers/pinctrl/pinctrl-mcp23s08.h
index 90dc27081a3c..1aa9b11780fc 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.h
+++ b/drivers/pinctrl/pinctrl-mcp23s08.h

@@ -30,7 +30,7 @@ struct mcp23s08 {
 	u16			irq_rise;
 	u16			irq_fall;
 	int			irq;
-	bool			irq_controller;
+	bool			irq_enabled;
 	int			cached_gpio;
 	/* lock protects regmap access with bypass/cache flags */
 	struct mutex		lock;

-- 
2.26.2

- Fix a typo where mcp23x17 configs are referred to as mcp23x16.
- Fix precious range to include INTCAP{A,B}, which clear on read.
- Fix precious range to include GPIOB, which clears on read.
- Fix volatile range to include GPIOB, to fix debugfs registers
  reporting different values than `gpioget gpiochip2 {0..15}`.

Signed-off-by: Thomas Preston <redacted>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 42b12ea14d6b..0138638276e7 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c

@@ -87,7 +87,7 @@ const struct regmap_config mcp23x08_regmap = {
 };
 EXPORT_SYMBOL_GPL(mcp23x08_regmap);
 
-static const struct reg_default mcp23x16_defaults[] = {
+static const struct reg_default mcp23x17_defaults[] = {
 	{.reg = MCP_IODIR << 1,		.def = 0xffff},
 	{.reg = MCP_IPOL << 1,		.def = 0x0000},
 	{.reg = MCP_GPINTEN << 1,	.def = 0x0000},

@@ -98,23 +98,23 @@ static const struct reg_default mcp23x16_defaults[] = {
 	{.reg = MCP_OLAT << 1,		.def = 0x0000},
 };
 
-static const struct regmap_range mcp23x16_volatile_range = {
+static const struct regmap_range mcp23x17_volatile_range = {
 	.range_min = MCP_INTF << 1,
-	.range_max = MCP_GPIO << 1,
+	.range_max = (MCP_GPIO << 1) + 1,
 };
 
-static const struct regmap_access_table mcp23x16_volatile_table = {
-	.yes_ranges = &mcp23x16_volatile_range,
+static const struct regmap_access_table mcp23x17_volatile_table = {
+	.yes_ranges = &mcp23x17_volatile_range,
 	.n_yes_ranges = 1,
 };
 
-static const struct regmap_range mcp23x16_precious_range = {
-	.range_min = MCP_GPIO << 1,
-	.range_max = MCP_GPIO << 1,
+static const struct regmap_range mcp23x17_precious_range = {
+	.range_min = MCP_INTCAP << 1,
+	.range_max = (MCP_GPIO << 1) + 1,
 };
 
-static const struct regmap_access_table mcp23x16_precious_table = {
-	.yes_ranges = &mcp23x16_precious_range,
+static const struct regmap_access_table mcp23x17_precious_table = {
+	.yes_ranges = &mcp23x17_precious_range,
 	.n_yes_ranges = 1,
 };
 

@@ -124,10 +124,10 @@ const struct regmap_config mcp23x17_regmap = {
 
 	.reg_stride = 2,
 	.max_register = MCP_OLAT << 1,
-	.volatile_table = &mcp23x16_volatile_table,
-	.precious_table = &mcp23x16_precious_table,
-	.reg_defaults = mcp23x16_defaults,
-	.num_reg_defaults = ARRAY_SIZE(mcp23x16_defaults),
+	.volatile_table = &mcp23x17_volatile_table,
+	.precious_table = &mcp23x17_precious_table,
+	.reg_defaults = mcp23x17_defaults,
+	.num_reg_defaults = ARRAY_SIZE(mcp23x17_defaults),
 	.cache_type = REGCACHE_FLAT,
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
 };

-- 
2.26.2

On 14/08/2020 11:03, Thomas Preston wrote:

I'm in the process of adding a device tree overlay for the PiFace
Digital Raspberry Pi daughter board [0]. It's an mcp23s17 SPI GPIO port
expander. In doing so, I noticed some errors with the mcp23s08 driver.

[snip]

They're quite trivial and backwards compatible, although I might be
wrong about "interrupt-controller". Can someone please confirm?

[snip]

[0] https://github.com/raspberrypi/linux/pull/3794

Actually I think I'm wrong about the interrupt-controller changes in 
patches 0002 and 0003.

I think Patch 0001 fixups are fine still.

Sorry for the noise!

On Fri, Aug 14, 2020 at 02:56:54PM +0100, Thomas Preston wrote:

On 14/08/2020 11:03, Thomas Preston wrote:

quoted

I'm in the process of adding a device tree overlay for the PiFace
Digital Raspberry Pi daughter board [0]. It's an mcp23s17 SPI GPIO port
expander. In doing so, I noticed some errors with the mcp23s08 driver.

[snip]

quoted

They're quite trivial and backwards compatible, although I might be
wrong about "interrupt-controller". Can someone please confirm?

[snip]

quoted

[0] https://github.com/raspberrypi/linux/pull/3794

Actually I think I'm wrong about the interrupt-controller changes in patches
0002 and 0003.

You are. Looking at the datasheet, the GPIOs have interrupt capability. 
GPIO controllers are typically both an interrupt client and provider.

Rob

On 17/08/2020 20:29, Rob Herring wrote:

On Fri, Aug 14, 2020 at 02:56:54PM +0100, Thomas Preston wrote:

quoted

Actually I think I'm wrong about the interrupt-controller changes in patches
0002 and 0003.

You are. Looking at the datasheet, the GPIOs have interrupt capability.
GPIO controllers are typically both an interrupt client and provider.

Thanks for the clarification.

I still think the patch 0001 is required. The precious and volatile 
ranges are broken.

On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston
[off-list ref] wrote:

- Fix a typo where mcp23x17 configs are referred to as mcp23x16.
- Fix precious range to include INTCAP{A,B}, which clear on read.
- Fix precious range to include GPIOB, which clears on read.
- Fix volatile range to include GPIOB, to fix debugfs registers
  reporting different values than `gpioget gpiochip2 {0..15}`.

Signed-off-by: Thomas Preston <redacted>

Since the other two patches seem wrong, please resend this one patch,
also include the people on TO: here: Andy, Phil and Jan, who all use
this chip a lot.

Yours,
Linus Walleij

On Fri, Aug 28, 2020 at 11:06:21AM +0200, Linus Walleij wrote:

On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston
[off-list ref] wrote:

quoted

- Fix a typo where mcp23x17 configs are referred to as mcp23x16.
- Fix precious range to include INTCAP{A,B}, which clear on read.
- Fix precious range to include GPIOB, which clears on read.
- Fix volatile range to include GPIOB, to fix debugfs registers
  reporting different values than `gpioget gpiochip2 {0..15}`.

Signed-off-by: Thomas Preston <redacted>

Since the other two patches seem wrong, please resend this one patch,
also include the people on TO: here: Andy, Phil and Jan, who all use
this chip a lot.

And it seems it combines a lot of stuff in one patch. Can we have a split with
appropriate Fixes: tags?

-- 
With Best Regards,
Andy Shevchenko

On Fri, Aug 14, 2020 at 1:35 PM Thomas Preston
[off-list ref] wrote:

- Fix a typo where mcp23x17 configs are referred to as mcp23x16.

I'm not sure it's correct. MPC23016 is an existing I²C IO expander.

- Fix precious range to include INTCAP{A,B}, which clear on read.
- Fix precious range to include GPIOB, which clears on read.
- Fix volatile range to include GPIOB, to fix debugfs registers
  reporting different values than `gpioget gpiochip2 {0..15}`.

I'm wondering if you read all the datasheets before doing these changes.
MPC2308
MPC23016
MPC23017
...

-static const struct regmap_range mcp23x16_volatile_range = {
+static const struct regmap_range mcp23x17_volatile_range = {
        .range_min = MCP_INTF << 1,
-       .range_max = MCP_GPIO << 1,
+       .range_max = (MCP_GPIO << 1) + 1,

This looks weird. Usually we do a mask or a bit based mask, like (1 << x) - 1.

 };

...

+static const struct regmap_range mcp23x17_precious_range = {
+       .range_min = MCP_INTCAP << 1,
+       .range_max = (MCP_GPIO << 1) + 1,

Ditto.

 };

-- 
With Best Regards,
Andy Shevchenko

On 28/08/2020 10:28, Andy Shevchenko wrote:

On Fri, Aug 28, 2020 at 11:06:21AM +0200, Linus Walleij wrote:

quoted

On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston
[off-list ref] wrote:

quoted

- Fix a typo where mcp23x17 configs are referred to as mcp23x16.
- Fix precious range to include INTCAP{A,B}, which clear on read.
- Fix precious range to include GPIOB, which clears on read.
- Fix volatile range to include GPIOB, to fix debugfs registers
   reporting different values than `gpioget gpiochip2 {0..15}`.

Signed-off-by: Thomas Preston <redacted>

Since the other two patches seem wrong, please resend this one patch,
also include the people on TO: here: Andy, Phil and Jan, who all use
this chip a lot.

And it seems it combines a lot of stuff in one patch. Can we have a split with
appropriate Fixes: tags?

Yeah no problem, just looking at this now.

Hey Andy, Linus,
Thanks for looking at this.

On 28/08/2020 11:09, Andy Shevchenko wrote:

On Fri, Aug 14, 2020 at 1:35 PM Thomas Preston
[off-list ref] wrote:

quoted

- Fix a typo where mcp23x17 configs are referred to as mcp23x16.

I'm not sure it's correct. MPC23016 is an existing I²C IO expander.

The MCP23016 device is not mentioned anywhere else in this driver. The 
only place this string is used is in `struct regmap_config 
mcp23x17_regmap` (another device). It seems to me that this is a typo 
but I might be wrong.

~/w/linux$ git grep -h compatible drivers/pinctrl/pinctrl-mcp23s08*
                 .compatible = "microchip,mcp23008",
                 .compatible = "microchip,mcp23017",
                 .compatible = "microchip,mcp23018",
                 .compatible = "mcp,mcp23008",
                 .compatible = "mcp,mcp23017",
                 .compatible = "microchip,mcp23s08",
                 .compatible = "microchip,mcp23s17",
                 .compatible = "microchip,mcp23s18",
                 .compatible = "mcp,mcp23s08",
                 .compatible = "mcp,mcp23s17",

Also I don't have an MC23016, so I can't test configuration for it.

quoted

- Fix precious range to include INTCAP{A,B}, which clear on read.
- Fix precious range to include GPIOB, which clears on read.
- Fix volatile range to include GPIOB, to fix debugfs registers
   reporting different values than `gpioget gpiochip2 {0..15}`.

I'm wondering if you read all the datasheets before doing these changes.
MPC2308
MPC23016
MPC23017
...

I did not! I was only changing configuration for MCP23017 devices.
What have I missed?

For reference, I think you are referring to [0], [1], [2]. I'm familiar 
with the last one.

quoted

-static const struct regmap_range mcp23x16_volatile_range = {
+static const struct regmap_range mcp23x17_volatile_range = {
         .range_min = MCP_INTF << 1,
-       .range_max = MCP_GPIO << 1,
+       .range_max = (MCP_GPIO << 1) + 1,

This looks weird. Usually we do a mask or a bit based mask, like (1 << x) - 1.

I don't think these are masks, they're addresses.

I believe the author has doubled the register indexing using a 1 bit 
shift, because the MCP23017 device is configured with sequential 
addresses (IOCON.BANK = 0). On page 12 of the datasheet [2] this looks like:

0x00 IODIRA, MCP_IODIR << 1
0x01 IODIRB
0x02 IPOLA,  MCP_IPOL << 1
0x03 IPOLB
...
0x12 GPIOA,  MCP_GPIO << 1
0x13 GPIOB

This means you can read 16 bits from MCP_GPIO << 1 and get the register 
values for both banks, or even use this for .range_min.

However, this trick doesn't work for .range_max:

	.range_max = MCP_GPIO << 1; /* 0x12 */

But I think it needs to be 0x13 to include GPIOB. Now that I'm looking 
into it, how does `mcp23x17_regmap.val_bits = 16` affect this? Perhaps 
`MCP_GPIO << 1` is fine after all.

I will whip up a v2 and test this. I'll split the changes across patches 
and fix the typo last patch - in case you don't agree with me.

Many thanks,
Thomas

[0] MCP23008 https://ww1.microchip.com/downloads/en/DeviceDoc/21919e.pdf
[1] MCP23016 http://ww1.microchip.com/downloads/en/devicedoc/20090c.pdf
[2] MCP23017 https://ww1.microchip.com/downloads/en/DeviceDoc/20001952C.pdf