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

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

[Marek Vasut]

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.

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");
 
 	return ret;
 }
-- 
2.40.1

[PATCH 2/2] i2c: mux: pca954x: Resume the mux early

[Marek Vasut]

The mux resumes alongside its subdevices, which means the subdevices may
resume while the mux is not yet fully configured and those subdevices may
fail to access I2C registers through the mux. Use resume_early to resume
the mux before any of the other i2c devices.

Signed-off-by: Marek Vasut <marex@denx.de>
---
NOTE: This might be wrong approach
---
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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 97cf475dde0f4..87fd8d3ba56b2 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -627,12 +627,14 @@ static int pca954x_resume(struct device *dev)
 	return ret;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(pca954x_pm, NULL, pca954x_resume);
+static const struct dev_pm_ops pca954x_pm = {
+	.resume_early = pca954x_resume,
+};
 
 static struct i2c_driver pca954x_driver = {
 	.driver		= {
 		.name	= "pca954x",
-		.pm	= pm_sleep_ptr(&pca954x_pm),
+		.pm	= &pca954x_pm,
 		.of_match_table = pca954x_of_match,
 	},
 	.probe		= pca954x_probe,
-- 
2.40.1

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

[Peter Rosin]

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.

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

Cheers,
Peter

>  
>  	return ret;
>  }

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

[Marek Vasut]

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.

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

[Peter Rosin]

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

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

[Marek Vasut]

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.