* [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues
@ 2025-10-09 13:26 bigunclemax
2025-10-13 13:22 ` Linus Walleij
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: bigunclemax @ 2025-10-09 13:26 UTC (permalink / raw)
Cc: bigunclemax, Mike Looijmans, Linus Walleij, linux-gpio,
linux-kernel
From: Maksim Kiselev <bigunclemax@gmail.com>
The probe function does not guarantee that chip registers are in their
default state. Thus using reg_defaults for regmap is incorrect.
For example, the chip may have already been configured by the bootloader
before the Linux driver loads, or the mcp might not have a reset at all
and not reset a state between reboots.
In such cases, using reg_defaults leads to the cache values diverging
from the actual registers values in the chip.
Previous attempts to fix consequences of this issue were made in
'commit 3ede3f8b4b4b ("pinctrl: mcp23s08: Reset all pins to input at
probe")', but this is insufficient. The OLAT register reset is also
required. And there's still potential for new issues arising due to cache
desynchronization of other registers.
Therefore, remove reg_defaults entirely to eliminate the root cause
of these problems.
Also remove the force reset all pins to input at probe as it is no longer
required.
Link: https://lore.kernel.org/all/20251006074934.27180-1-bigunclemax@gmail.com/
Suggested-by: Mike Looijmans <mike.looijmans@topic.nl>
Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
---
drivers/pinctrl/pinctrl-mcp23s08.c | 34 ------------------------------
1 file changed, 34 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 78ff7930649d2..0b329661b5978 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -44,17 +44,6 @@
#define MCP_GPIO 0x09
#define MCP_OLAT 0x0a
-static const struct reg_default mcp23x08_defaults[] = {
- {.reg = MCP_IODIR, .def = 0xff},
- {.reg = MCP_IPOL, .def = 0x00},
- {.reg = MCP_GPINTEN, .def = 0x00},
- {.reg = MCP_DEFVAL, .def = 0x00},
- {.reg = MCP_INTCON, .def = 0x00},
- {.reg = MCP_IOCON, .def = 0x00},
- {.reg = MCP_GPPU, .def = 0x00},
- {.reg = MCP_OLAT, .def = 0x00},
-};
-
static const struct regmap_range mcp23x08_volatile_range = {
.range_min = MCP_INTF,
.range_max = MCP_GPIO,
@@ -82,25 +71,12 @@ const struct regmap_config mcp23x08_regmap = {
.reg_stride = 1,
.volatile_table = &mcp23x08_volatile_table,
.precious_table = &mcp23x08_precious_table,
- .reg_defaults = mcp23x08_defaults,
- .num_reg_defaults = ARRAY_SIZE(mcp23x08_defaults),
.cache_type = REGCACHE_FLAT,
.max_register = MCP_OLAT,
.disable_locking = true, /* mcp->lock protects the regmap */
};
EXPORT_SYMBOL_GPL(mcp23x08_regmap);
-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},
- {.reg = MCP_DEFVAL << 1, .def = 0x0000},
- {.reg = MCP_INTCON << 1, .def = 0x0000},
- {.reg = MCP_IOCON << 1, .def = 0x0000},
- {.reg = MCP_GPPU << 1, .def = 0x0000},
- {.reg = MCP_OLAT << 1, .def = 0x0000},
-};
-
static const struct regmap_range mcp23x17_volatile_range = {
.range_min = MCP_INTF << 1,
.range_max = MCP_GPIO << 1,
@@ -129,8 +105,6 @@ const struct regmap_config mcp23x17_regmap = {
.max_register = MCP_OLAT << 1,
.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,
.disable_locking = true, /* mcp->lock protects the regmap */
@@ -614,14 +588,6 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
mcp->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
- /*
- * Reset the chip - we don't really know what state it's in, so reset
- * all pins to input first to prevent surprises.
- */
- ret = mcp_write(mcp, MCP_IODIR, mcp->chip.ngpio == 16 ? 0xFFFF : 0xFF);
- if (ret < 0)
- return ret;
-
/* verify MCP_IOCON.SEQOP = 0, so sequential reads work,
* and MCP_IOCON.HAEN = 1, so we work with all chips.
*/
--
2.48.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues
2025-10-09 13:26 [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues bigunclemax
@ 2025-10-13 13:22 ` Linus Walleij
2025-10-20 19:40 ` Sander Vanheule
2025-10-24 6:54 ` Mike Looijmans
2 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2025-10-13 13:22 UTC (permalink / raw)
To: bigunclemax, Dmitry Mastykin, Evgenii Shatokhin,
Arturas Moskvinas, Uwe Kleine-König, Andy Shevchenko,
Andreas Kaessens, Zou Wei, Radim Pavlik
Cc: Mike Looijmans, linux-gpio, linux-kernel
Hi Maksim,
thanks for your patch!
On Thu, Oct 9, 2025 at 3:29 PM <bigunclemax@gmail.com> wrote:
>
> From: Maksim Kiselev <bigunclemax@gmail.com>
>
> The probe function does not guarantee that chip registers are in their
> default state. Thus using reg_defaults for regmap is incorrect.
>
> For example, the chip may have already been configured by the bootloader
> before the Linux driver loads, or the mcp might not have a reset at all
> and not reset a state between reboots.
>
> In such cases, using reg_defaults leads to the cache values diverging
> from the actual registers values in the chip.
>
> Previous attempts to fix consequences of this issue were made in
> 'commit 3ede3f8b4b4b ("pinctrl: mcp23s08: Reset all pins to input at
> probe")', but this is insufficient. The OLAT register reset is also
> required. And there's still potential for new issues arising due to cache
> desynchronization of other registers.
>
> Therefore, remove reg_defaults entirely to eliminate the root cause
> of these problems.
>
> Also remove the force reset all pins to input at probe as it is no longer
> required.
>
> Link: https://lore.kernel.org/all/20251006074934.27180-1-bigunclemax@gmail.com/
> Suggested-by: Mike Looijmans <mike.looijmans@topic.nl>
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
I would surely like to see some Tested-by on this patch because
this driver has *many* users.
I added some people to the To: line who recently made changes to this
driver, maybe they can test.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues
@ 2025-10-18 19:31 Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2025-10-18 19:31 UTC (permalink / raw)
To: Linus Walleij
Cc: bigunclemax, Dmitry Mastykin, Evgenii Shatokhin,
Arturas Moskvinas, Uwe Kleine-König, Andreas Kaessens,
Zou Wei, Radim Pavlik, Mike Looijmans, linux-gpio, linux-kernel
On Mon, Oct 13, 2025 at 03:22:19PM +0200, Linus Walleij wrote:
> Hi Maksim,
>
> thanks for your patch!
>
> On Thu, Oct 9, 2025 at 3:29 PM <bigunclemax@gmail.com> wrote:
> >
> > From: Maksim Kiselev <bigunclemax@gmail.com>
> >
> > The probe function does not guarantee that chip registers are in their
> > default state. Thus using reg_defaults for regmap is incorrect.
> >
> > For example, the chip may have already been configured by the bootloader
> > before the Linux driver loads, or the mcp might not have a reset at all
> > and not reset a state between reboots.
> >
> > In such cases, using reg_defaults leads to the cache values diverging
> > from the actual registers values in the chip.
> >
> > Previous attempts to fix consequences of this issue were made in
> > 'commit 3ede3f8b4b4b ("pinctrl: mcp23s08: Reset all pins to input at
> > probe")', but this is insufficient. The OLAT register reset is also
> > required. And there's still potential for new issues arising due to cache
> > desynchronization of other registers.
> >
> > Therefore, remove reg_defaults entirely to eliminate the root cause
> > of these problems.
> >
> > Also remove the force reset all pins to input at probe as it is no longer
> > required.
> >
> > Link: https://lore.kernel.org/all/20251006074934.27180-1-bigunclemax@gmail.com/
> > Suggested-by: Mike Looijmans <mike.looijmans@topic.nl>
> > Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
>
> I would surely like to see some Tested-by on this patch because
> this driver has *many* users.
>
> I added some people to the To: line who recently made changes to this
> driver, maybe they can test.
To add, I would suggest to look at Cypress driver, it uses the method to
recover defaults from the actual HW state.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues
2025-10-09 13:26 [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues bigunclemax
2025-10-13 13:22 ` Linus Walleij
@ 2025-10-20 19:40 ` Sander Vanheule
2025-10-22 14:45 ` Mike Looijmans
2025-10-24 6:54 ` Mike Looijmans
2 siblings, 1 reply; 6+ messages in thread
From: Sander Vanheule @ 2025-10-20 19:40 UTC (permalink / raw)
To: bigunclemax
Cc: Mike Looijmans, Linus Walleij, linux-gpio, linux-kernel,
Andy Shevchenko
Hi,
On Thu, 2025-10-09 at 16:26 +0300, bigunclemax@gmail.com wrote:
> From: Maksim Kiselev <bigunclemax@gmail.com>
>
> The probe function does not guarantee that chip registers are in their
> default state. Thus using reg_defaults for regmap is incorrect.
>
> ---
>
> @@ -82,25 +71,12 @@ const struct regmap_config mcp23x08_regmap = {
> .reg_stride = 1,
> .volatile_table = &mcp23x08_volatile_table,
> .precious_table = &mcp23x08_precious_table,
> - .reg_defaults = mcp23x08_defaults,
> - .num_reg_defaults = ARRAY_SIZE(mcp23x08_defaults),
> .cache_type = REGCACHE_FLAT,
> .max_register = MCP_OLAT,
> .disable_locking = true, /* mcp->lock protects the regmap */
As Andy mentioned, the problem you will now have to deal with is that your cache
is not initialized at all. Unlike the other cache types, REGCACHE_FLAT will
zero-initialize its cache, perhaps making your cache sync issues worse.
You have two options to initialize the cache properly:
* Provide .num_reg_defaults_raw (= MCP_OLAT + 1). This will give you a warning
on probe about the cache defaults being initialized from hardware.
* Switch to another cache type (REGCACHE_MAPLE), which is aware of (in)valid
cache entries. regmap will then init the cache on the first access to a
register.
You could also combine the two, like the Cypress driver Andy referred to
(pinctrl-cy8c95x0.c). In that case you get cache loading at init, instead of at
first use, but without the risk of missing something.
Best,
Sander
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues
2025-10-20 19:40 ` Sander Vanheule
@ 2025-10-22 14:45 ` Mike Looijmans
0 siblings, 0 replies; 6+ messages in thread
From: Mike Looijmans @ 2025-10-22 14:45 UTC (permalink / raw)
To: Sander Vanheule, bigunclemax
Cc: Linus Walleij, linux-gpio, linux-kernel, Andy Shevchenko
On 10/20/25 21:40, Sander Vanheule wrote:
> Hi,
>
> On Thu, 2025-10-09 at 16:26 +0300, bigunclemax@gmail.com wrote:
>> From: Maksim Kiselev <bigunclemax@gmail.com>
>>
>> The probe function does not guarantee that chip registers are in their
>> default state. Thus using reg_defaults for regmap is incorrect.
>>
>> ---
>>
>> @@ -82,25 +71,12 @@ const struct regmap_config mcp23x08_regmap = {
>> .reg_stride = 1,
>> .volatile_table = &mcp23x08_volatile_table,
>> .precious_table = &mcp23x08_precious_table,
>> - .reg_defaults = mcp23x08_defaults,
>> - .num_reg_defaults = ARRAY_SIZE(mcp23x08_defaults),
>> .cache_type = REGCACHE_FLAT,
>> .max_register = MCP_OLAT,
>> .disable_locking = true, /* mcp->lock protects the regmap */
> As Andy mentioned, the problem you will now have to deal with is that your cache
> is not initialized at all. Unlike the other cache types, REGCACHE_FLAT will
> zero-initialize its cache, perhaps making your cache sync issues worse.
Ouch...
I have access to hardware this week (boards with 2 and 3 of the I2C
chips), I'll be able to do some hands-on testing, and report back.
> You have two options to initialize the cache properly:
> * Provide .num_reg_defaults_raw (= MCP_OLAT + 1). This will give you a warning
> on probe about the cache defaults being initialized from hardware.
> * Switch to another cache type (REGCACHE_MAPLE), which is aware of (in)valid
> cache entries. regmap will then init the cache on the first access to a
> register.
Using REGCACHE_MAPLE sounds like the obvious solution to me. That's what most other drivers use.
> You could also combine the two, like the Cypress driver Andy referred to
> (pinctrl-cy8c95x0.c). In that case you get cache loading at init, instead of at
> first use, but without the risk of missing something.
--
Mike Looijmans
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues
2025-10-09 13:26 [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues bigunclemax
2025-10-13 13:22 ` Linus Walleij
2025-10-20 19:40 ` Sander Vanheule
@ 2025-10-24 6:54 ` Mike Looijmans
2 siblings, 0 replies; 6+ messages in thread
From: Mike Looijmans @ 2025-10-24 6:54 UTC (permalink / raw)
To: bigunclemax; +Cc: Linus Walleij, linux-gpio, linux-kernel
On 10/9/25 15:26, bigunclemax@gmail.com wrote:
> From: Maksim Kiselev <bigunclemax@gmail.com>
>
> The probe function does not guarantee that chip registers are in their
> default state. Thus using reg_defaults for regmap is incorrect.
Did some testing on actual hardware with this patch "as is". As Sander
mentioned, the REGCACHE_FLAT caching mode assumes all registers are "0"
initially, which causes similar issues as what we're trying to solve.
I've verified that by setting a bit to "1" in the OLAT register (using
i2cset) for an unused GPIO line. After a reboot, the kernel resets it to
"0" because of the caching issue.
I then changed the cache_type to REGCACHE_MAPLE as indicated below, and
then the kernel doesn't touch that bit any longer (as it should).
So once you've amended the patch with REGCACHE_MAPLE, you have my:
Tested-by: Mike Looijmans <mike.looijmans@topic.nl>
> ...
> static const struct regmap_range mcp23x08_volatile_range = {
> .range_min = MCP_INTF,
> .range_max = MCP_GPIO,
> @@ -82,25 +71,12 @@ const struct regmap_config mcp23x08_regmap = {
> .reg_stride = 1,
> .volatile_table = &mcp23x08_volatile_table,
> .precious_table = &mcp23x08_precious_table,
> - .reg_defaults = mcp23x08_defaults,
> - .num_reg_defaults = ARRAY_SIZE(mcp23x08_defaults),
> .cache_type = REGCACHE_FLAT,
Must be REGCACHE_MAPLE
> .max_register = MCP_OLAT,
> .disable_locking = true, /* mcp->lock protects the regmap */
> };
> EXPORT_SYMBOL_GPL(mcp23x08_regmap);
>
> -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},
> - {.reg = MCP_DEFVAL << 1, .def = 0x0000},
> - {.reg = MCP_INTCON << 1, .def = 0x0000},
> - {.reg = MCP_IOCON << 1, .def = 0x0000},
> - {.reg = MCP_GPPU << 1, .def = 0x0000},
> - {.reg = MCP_OLAT << 1, .def = 0x0000},
> -};
> -
> static const struct regmap_range mcp23x17_volatile_range = {
> .range_min = MCP_INTF << 1,
> .range_max = MCP_GPIO << 1,
> @@ -129,8 +105,6 @@ const struct regmap_config mcp23x17_regmap = {
> .max_register = MCP_OLAT << 1,
> .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,
Must be REGCACHE_MAPLE
> .val_format_endian = REGMAP_ENDIAN_LITTLE,
> .disable_locking = true, /* mcp->lock protects the regmap */
> @@ -614,14 +588,6 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
> ...
--
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-24 6:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-09 13:26 [PATCH v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues bigunclemax
2025-10-13 13:22 ` Linus Walleij
2025-10-20 19:40 ` Sander Vanheule
2025-10-22 14:45 ` Mike Looijmans
2025-10-24 6:54 ` Mike Looijmans
-- strict thread matches above, loose matches on Subject: below --
2025-10-18 19:31 Andy Shevchenko
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).