* [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17
@ 2020-08-14 10:03 Thomas Preston
2020-08-14 10:03 ` [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config Thomas Preston
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Thomas Preston @ 2020-08-14 10:03 UTC (permalink / raw)
To: linus.walleij, robh+dt, linux-gpio, devicetree, linux-kernel
Cc: 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config
2020-08-14 10:03 [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
@ 2020-08-14 10:03 ` Thomas Preston
2020-08-28 9:06 ` Linus Walleij
2020-08-28 10:09 ` Andy Shevchenko
2020-08-14 10:03 ` [PATCH 2/3] pinctrl: mcp23s08: Remove interrupt-controller Thomas Preston
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Thomas Preston @ 2020-08-14 10:03 UTC (permalink / raw)
To: linus.walleij, robh+dt, linux-gpio, devicetree, linux-kernel
Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] pinctrl: mcp23s08: Remove interrupt-controller
2020-08-14 10:03 [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
2020-08-14 10:03 ` [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config Thomas Preston
@ 2020-08-14 10:03 ` Thomas Preston
2020-08-14 10:03 ` [PATCH 3/3] devicetree: " Thomas Preston
2020-08-14 13:56 ` [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
3 siblings, 0 replies; 12+ messages in thread
From: Thomas Preston @ 2020-08-14 10:03 UTC (permalink / raw)
To: linus.walleij, robh+dt, linux-gpio, devicetree, linux-kernel
Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] devicetree: mcp23s08: Remove interrupt-controller
2020-08-14 10:03 [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
2020-08-14 10:03 ` [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config Thomas Preston
2020-08-14 10:03 ` [PATCH 2/3] pinctrl: mcp23s08: Remove interrupt-controller Thomas Preston
@ 2020-08-14 10:03 ` Thomas Preston
2020-08-14 13:56 ` [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
3 siblings, 0 replies; 12+ messages in thread
From: Thomas Preston @ 2020-08-14 10:03 UTC (permalink / raw)
To: linus.walleij, robh+dt, linux-gpio, devicetree, linux-kernel
Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17
2020-08-14 10:03 [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
` (2 preceding siblings ...)
2020-08-14 10:03 ` [PATCH 3/3] devicetree: " Thomas Preston
@ 2020-08-14 13:56 ` Thomas Preston
2020-08-17 19:29 ` Rob Herring
3 siblings, 1 reply; 12+ messages in thread
From: Thomas Preston @ 2020-08-14 13:56 UTC (permalink / raw)
To: linus.walleij, robh+dt, linux-gpio, devicetree, linux-kernel
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!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17
2020-08-14 13:56 ` [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
@ 2020-08-17 19:29 ` Rob Herring
2020-08-18 11:09 ` Thomas Preston
0 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2020-08-17 19:29 UTC (permalink / raw)
To: Thomas Preston; +Cc: linus.walleij, linux-gpio, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17
2020-08-17 19:29 ` Rob Herring
@ 2020-08-18 11:09 ` Thomas Preston
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Preston @ 2020-08-18 11:09 UTC (permalink / raw)
To: Rob Herring; +Cc: linus.walleij, linux-gpio, devicetree, linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config
2020-08-14 10:03 ` [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config Thomas Preston
@ 2020-08-28 9:06 ` Linus Walleij
2020-08-28 9:28 ` Andy Shevchenko
2020-08-28 10:09 ` Andy Shevchenko
1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2020-08-28 9:06 UTC (permalink / raw)
To: Thomas Preston, Andy Shevchenko, Jan Kundrát, Phil Reid
Cc: open list:GPIO SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config
2020-08-28 9:06 ` Linus Walleij
@ 2020-08-28 9:28 ` Andy Shevchenko
2020-08-28 17:30 ` Thomas Preston
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-08-28 9:28 UTC (permalink / raw)
To: Linus Walleij
Cc: Thomas Preston, Jan Kundrát, Phil Reid,
open list:GPIO SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config
2020-08-14 10:03 ` [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config Thomas Preston
2020-08-28 9:06 ` Linus Walleij
@ 2020-08-28 10:09 ` Andy Shevchenko
2020-08-28 19:19 ` Thomas Preston
1 sibling, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-08-28 10:09 UTC (permalink / raw)
To: Thomas Preston
Cc: Linus Walleij, Rob Herring, open list:GPIO SUBSYSTEM, devicetree,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config
2020-08-28 9:28 ` Andy Shevchenko
@ 2020-08-28 17:30 ` Thomas Preston
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Preston @ 2020-08-28 17:30 UTC (permalink / raw)
To: Andy Shevchenko, Linus Walleij
Cc: Jan Kundrát, Phil Reid, open list:GPIO SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config
2020-08-28 10:09 ` Andy Shevchenko
@ 2020-08-28 19:19 ` Thomas Preston
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Preston @ 2020-08-28 19:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Rob Herring, open list:GPIO SUBSYSTEM, devicetree,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-08-28 19:20 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-14 10:03 [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
2020-08-14 10:03 ` [PATCH 1/3] pinctrl: mcp23s08: Fixup mcp23x17 regmap_config Thomas Preston
2020-08-28 9:06 ` Linus Walleij
2020-08-28 9:28 ` Andy Shevchenko
2020-08-28 17:30 ` Thomas Preston
2020-08-28 10:09 ` Andy Shevchenko
2020-08-28 19:19 ` Thomas Preston
2020-08-14 10:03 ` [PATCH 2/3] pinctrl: mcp23s08: Remove interrupt-controller Thomas Preston
2020-08-14 10:03 ` [PATCH 3/3] devicetree: " Thomas Preston
2020-08-14 13:56 ` [PATCH 0/3] pinctrl: mcp23s08: Fixups for mcp23x17 Thomas Preston
2020-08-17 19:29 ` Rob Herring
2020-08-18 11:09 ` Thomas Preston
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).