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

[Marek Vasut <marex@denx.de>]

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 .

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