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

[Marek Vasut <marex@denx.de>]

On 9/1/23 10:07, Peter Rosin wrote:
> 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.

OK, then just drop this patch.