pinctrl: mcp23s08: init reg_defaults from HW at probe and switch cache type

[PATCH v3] pinctrl: mcp23s08: init reg_defaults from HW at probe and switch cache type

Posted by bigunclemax@gmail.com 3 months, 1 week ago

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 and provide num_reg_defaults_raw. In that
case the cache defaults being initialized from hardware.

Also switch cache type to REGCACHE_MAPLE, which is aware of (in)valid
cache entries.

And remove the force reset all pins to input at probe as it is no longer
required.

Link: https://lore.kernel.org/all/20251009132651.649099-2-bigunclemax@gmail.com/
Suggested-by: Mike Looijmans <mike.looijmans@topic.nl>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Sander Vanheule <sander@svanheule.net>
Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 40 +++---------------------------
 1 file changed, 4 insertions(+), 36 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 78ff7930649d..edda2893945d 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,13 @@ 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,
+	.num_reg_defaults_raw = MCP_OLAT + 1,
+	.cache_type = 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,9 +106,8 @@ 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,
+	.num_reg_defaults_raw = MCP_OLAT + 1,
+	.cache_type = REGCACHE_MAPLE,
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
 	.disable_locking = true, /* mcp->lock protects the regmap */
 };
@@ -614,14 +590,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

Re: [PATCH v3] pinctrl: mcp23s08: init reg_defaults from HW at probe and switch cache type

Posted by Andy Shevchenko 3 months, 1 week ago

On Mon, Oct 27, 2025 at 01:46:26PM +0300, bigunclemax@gmail.com wrote:
> 
> 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 and provide num_reg_defaults_raw. In that
> case the cache defaults being initialized from hardware.
> 
> Also switch cache type to REGCACHE_MAPLE, which is aware of (in)valid
> cache entries.
> 
> And remove the force reset all pins to input at probe as it is no longer
> required.

> ---

No changelog? No need to resend, just reply with the missing piece.

>  drivers/pinctrl/pinctrl-mcp23s08.c | 40 +++---------------------------
>  1 file changed, 4 insertions(+), 36 deletions(-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] pinctrl: mcp23s08: init reg_defaults from HW at probe and switch cache type

Posted by Maxim Kiselev 3 months, 1 week ago

Hi !

пн, 27 окт. 2025 г. в 14:49, Andy Shevchenko
<andriy.shevchenko@linux.intel.com>:
>
> On Mon, Oct 27, 2025 at 01:46:26PM +0300, bigunclemax@gmail.com wrote:
> >
> > 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 and provide num_reg_defaults_raw. In that
> > case the cache defaults being initialized from hardware.
> >
> > Also switch cache type to REGCACHE_MAPLE, which is aware of (in)valid
> > cache entries.
> >
> > And remove the force reset all pins to input at probe as it is no longer
> > required.
>
> > ---
>
> No changelog? No need to resend, just reply with the missing piece.

Sorry, my bad.

Changelog:
v3:
 - changed cache type from REGCACHE_FLAT to REGCACHE_MAPLE
 - added .num_reg_defaults_raw to init regs cache from HW at probe

v2:
 - rollback v1 changes
 - dropped .reg_defaults of mcp23x regmaps
 - dropped reset all pins to input at probe (commit 3ede3f8b4b4b)


>
> >  drivers/pinctrl/pinctrl-mcp23s08.c | 40 +++---------------------------
> >  1 file changed, 4 insertions(+), 36 deletions(-)
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3] pinctrl: mcp23s08: init reg_defaults from HW at probe and switch cache type

Posted by Linus Walleij 3 months, 1 week ago

On Mon, Oct 27, 2025 at 1:04 PM Maxim Kiselev <bigunclemax@gmail.com> wrote:

> Changelog:
> v3:
>  - changed cache type from REGCACHE_FLAT to REGCACHE_MAPLE
>  - added .num_reg_defaults_raw to init regs cache from HW at probe
>
> v2:
>  - rollback v1 changes
>  - dropped .reg_defaults of mcp23x regmaps
>  - dropped reset all pins to input at probe (commit 3ede3f8b4b4b)

It'd be great if I could get some ACKs/R-Bs and/or Tested-by:s on
this patch.

Is this fixes (for-v6.18) material or is v6.19 fine?

I'm good with putting it in queue for v6.19 but for fixes I would
really like a few tested-by:s given how widely this driver is used.

Yours,
Linus Walleij