[v2] pinctrl: mcp23s08: delete regmap reg_defaults to avoid cache sync issues

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

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

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

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.

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