Re: [PATCH 1/2] i2c: mux: pca954x: Make sure the mux remains configured the same as before resume

[Peter Rosin <peda@axentia.se>]

Hi!

2023-08-31 at 23:50, Marek Vasut wrote:
> On 8/31/23 23:24, Peter Rosin wrote:
>> Hi!
> 
> Hi,
> 
>> 2023-08-31 at 20:17, Marek Vasut wrote:
>>> The current implementation of pca954x_init() rewrites content of data->last_chan
>>> which is then populated into the mux select register. Skip this part, so that the
>>> mux is populated with content of data->last_chan as it was set before suspend.
>>> This way, the mux state is retained across suspend/resume cycle.
>>
>> I fail to see in what situation this change makes a significant
>> difference? For me, it's a nice conservative thing to initialize
>> to the default state after something comparatively heavy such as
>> a suspend/resume cycle. If there is a significant difference,
>> then maybe it's not the usual access patterns after resume since
>> there are probably other chips initializing as well, in which
>> case this change might make things worse depending on what
>> devices you do have and what idle-state you have configured.
> 
> Isn't it better to keep the hardware in the same state it was before it entered suspend ? For me, that's the behavior I would expect from suspend/resume .

Ok, in either case the current behavior isn't a bug. Please drop
the Fixes-tag.

> 
>>> Fixes: e65e228eb096 ("i2c: mux: pca954x: support property idle-state")
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Peter Rosin <peda@axentia.se>
>>> Cc: Wolfram Sang <wsa@kernel.org>
>>> Cc: linux-i2c@vger.kernel.org
>>> ---
>>>   drivers/i2c/muxes/i2c-mux-pca954x.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> index 2219062104fbc..97cf475dde0f4 100644
>>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> @@ -620,9 +620,9 @@ static int pca954x_resume(struct device *dev)
>>>       struct pca954x *data = i2c_mux_priv(muxc);
>>>       int ret;
>>>   -    ret = pca954x_init(client, data);
>>> +    ret = i2c_smbus_write_byte(client, data->last_chan);
>>>       if (ret < 0)
>>> -        dev_err(&client->dev, "failed to verify mux presence\n");
>>> +        dev_err(&client->dev, "failed to restore mux state\n");
>>
>> data->last_chan is no longer cleared in case the write fails. Is that a
>> problem?
> 
> If the write fails here, the hardware is in undefined state anyway .
> Either the next attempt to flip the switch would help bring it back, or, the system is in undefined state.

Being in an undefined state with last_chan being zero is better than
being in an undefined state with last_chan holding some other value,
so that writing the register isn't skipped in the following call to
pca954x_select_chan(). This is the whole point of clearing last_chan
on error. Notice how pca954x_select_chan() also clears last_chan on
error.

Cheers,
Peter