| 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 |
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; > }
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(-)