| Message ID | 20230831181753.154787-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] i2c: mux: pca954x: Make sure the mux remains configured the same as before resume | expand |
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.
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
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.
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; }
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(-)