Re: [PATCH v1] pinctrl: mcp23s08: Reset all output latches to default at probe

[Mike Looijmans]

On 06-10-2025 12:18, Maxim Kiselev wrote:
> Hi
>
> пн, 6 окт. 2025 г. в 11:04, Mike Looijmans <mike.looijmans@xxxxxxxx>:
> > On 06-10-2025 09:49, bigunclemax@xxxxxxxxx wrote:
> > > From: "Maksim Kiselev" <bigunclemax@xxxxxxxxx>
> > >
> > > It appears that resetting only the direction register is not sufficient,
> > > it's also necessary to reset the OLAT register to its default values.
> > >
> > > Otherwise, the following situation can occur:
> > >
> > > If a pin was configured as OUT=1 before driver probe(Ex: IODIR=1,IOLAT=1),
> > > then after loading the MCP driver, the cache will be populated from
> > > reg_defaults with IOLAT=0 (while the actual value in the chip is 1).
> > > A subsequent setting OUT=0 will fail because
> > > mcp_update_bits(mcp, MCP_OLAT, ...) calls regmap_update_bits(),
> > > which will check that the value to be set (0) matches the cached value (0)
> > > and thus skip writing actual value to the MCP chip.
> > Maybe it's be better to fix the underlying issue: This driver should not be
> > using a pre-populated regmap cache. Unless it performs a hard reset, the
> > driver has no way of knowing what the initial values are, it should just read
> > them from the chip.
> I agree with you here, thought about it, but consider such a change
> too radical :)

Yeah, I had the same thought when I created the patch that resets the 
direction register :)


> Okay, I'll remove the reg_defaults in the second version.

I think it's the proper solution. You should remove the direction register 
reset as well (basically revert my patch).

M.

> > > To avoid this, the OLAT register must be explicitly reset at probe.
> > >
> > > Fixes: 3ede3f8b4b4b ("pinctrl: mcp23s08: Reset all pins to input at probe")
> > > Signed-off-by: Maksim Kiselev <bigunclemax@xxxxxxxxx>
> > > ---
> > >    drivers/pinctrl/pinctrl-mcp23s08.c | 5 +++++
> > >    1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> > > index 78ff7930649d..23af441aa468 100644
> > > --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> > > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> > > @@ -622,6 +622,11 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
> > >        if (ret < 0)
> > >                return ret;
> > >
> > > +     /* Also reset all out latches to default values */
> > > +     ret = mcp_write(mcp, MCP_OLAT, 0x0);
> > > +     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.
> > >         */
> > Mike.