pinctrl: mcp23s08: Reset all output latches to default at probe

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

Posted by bigunclemax@gmail.com 4 months ago

From: "Maksim Kiselev" <bigunclemax@gmail.com>

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.

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@gmail.com>
---
 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.
 	 */
-- 
2.48.1

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

Posted by Mike Looijmans 4 months ago

On 06-10-2025 09:49, bigunclemax@gmail.com wrote:
> From: "Maksim Kiselev" <bigunclemax@gmail.com>
>
> 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.

>
> 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@gmail.com>
> ---
>   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.

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

Posted by Maxim Kiselev 4 months ago

Hi

пн, 6 окт. 2025 г. в 11:04, Mike Looijmans <mike.looijmans@topic.nl>:
>
> On 06-10-2025 09:49, bigunclemax@gmail.com wrote:
> > From: "Maksim Kiselev" <bigunclemax@gmail.com>
> >
> > 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 :)

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

> >
> > 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@gmail.com>
> > ---
> >   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.
>
>
>

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

Posted by Mike Looijmans 4 months ago

On 06-10-2025 12:18, Maxim Kiselev wrote:
> Hi
>
> пн, 6 окт. 2025 г. в 11:04, Mike Looijmans <mike.looijmans@topic.nl>:
>> On 06-10-2025 09:49, bigunclemax@gmail.com wrote:
>>> From: "Maksim Kiselev" <bigunclemax@gmail.com>
>>>
>>> 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@gmail.com>
>>> ---
>>>    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.
>>
>>
>>