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

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

[Thomas Preston]

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

[PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config

[Thomas Preston]

- 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 <thomas.preston@codethink.co.uk>
---
 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

[PATCH 2/3] pinctrl: mcp23s08: Remove interrupt-controller

[Thomas Preston]

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 <thomas.preston@codethink.co.uk>
---
 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

[PATCH 3/3] devicetree: mcp23s08: Remove interrupt-controller

[Thomas Preston]

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 <thomas.preston@codethink.co.uk>
---
 .../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

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

[Thomas Preston]

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!

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

[Rob Herring]

On Fri, Aug 14, 2020 at 02:56:54PM +0100, Thomas Preston wrote:
> 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.

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

Rob

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

[Thomas Preston]

On 17/08/2020 20:29, Rob Herring wrote:
> On Fri, Aug 14, 2020 at 02:56:54PM +0100, Thomas Preston wrote:
>> 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.

Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config

[Linus Walleij]

On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston
<thomas.preston@codethink.co.uk> 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 <thomas.preston@codethink.co.uk>

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

Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config

[Andy Shevchenko]

On Fri, Aug 28, 2020 at 11:06:21AM +0200, Linus Walleij wrote:
> On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston
> <thomas.preston@codethink.co.uk> 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 <thomas.preston@codethink.co.uk>
> 
> 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

Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 1:35 PM Thomas Preston
<thomas.preston@codethink.co.uk> 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

Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config

[Thomas Preston]

On 28/08/2020 10:28, Andy Shevchenko wrote:
> On Fri, Aug 28, 2020 at 11:06:21AM +0200, Linus Walleij wrote:
>> On Fri, Aug 14, 2020 at 12:04 PM Thomas Preston
>> <thomas.preston@codethink.co.uk> 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 <thomas.preston@codethink.co.uk>
>>
>> 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.

Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config

[Thomas Preston]

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
> <thomas.preston@codethink.co.uk> 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.
> 

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.

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

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