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

[Mike Looijmans <mike.looijmans@topic.nl>]

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