| Message ID | 20240102-j7200-pcie-s2r-v2-4-8e4f7d228ec2@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > > The mux_chip_resume() function restores a mux_chip using the cached state > of each mux. ... > +int mux_chip_resume(struct mux_chip *mux_chip) > +{ > + int ret, i; > + > + for (i = 0; i < mux_chip->controllers; ++i) { > + struct mux_control *mux = &mux_chip->mux[i]; > + if (mux->cached_state != MUX_CACHE_UNKNOWN) { if (_state == ...) continue; ? > + ret = mux_control_set(mux, mux->cached_state); > + if (ret < 0) { > + dev_err(&mux_chip->dev, "unable to restore state\n"); > + return ret; > + } > + } > + } > + return 0; > +}
Hi Thomas, ... > +/** > + * mux_chip_resume() - restores the mux-chip state > + * @mux_chip: The mux-chip to resume. > + * > + * Restores the mux-chip state. > + * > + * Return: Zero on success or a negative errno on error. > + */ > +int mux_chip_resume(struct mux_chip *mux_chip) > +{ > + int ret, i; you could put this 'ret' in the innermost indentation part, which here is inside the 'if (...)' (or inside the 'for (...)' if you follow Andy's suggestion). Andi > + for (i = 0; i < mux_chip->controllers; ++i) { > + struct mux_control *mux = &mux_chip->mux[i]; > + > + if (mux->cached_state != MUX_CACHE_UNKNOWN) { > + ret = mux_control_set(mux, mux->cached_state); > + if (ret < 0) { > + dev_err(&mux_chip->dev, "unable to restore state\n"); > + return ret; > + } > + } > + } > + return 0; > +} > +EXPORT_SYMBOL_GPL(mux_chip_resume);
Hi! 2024-01-26 at 15:36, Thomas Richard wrote: > The mux_chip_resume() function restores a mux_chip using the cached state > of each mux. > > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > --- > drivers/mux/core.c | 27 +++++++++++++++++++++++++++ > include/linux/mux/driver.h | 1 + > 2 files changed, 28 insertions(+) > > diff --git a/drivers/mux/core.c b/drivers/mux/core.c > index 775816112932..896f74b34eb8 100644 > --- a/drivers/mux/core.c > +++ b/drivers/mux/core.c > @@ -215,6 +215,33 @@ void mux_chip_free(struct mux_chip *mux_chip) > } > EXPORT_SYMBOL_GPL(mux_chip_free); > > +/** > + * mux_chip_resume() - restores the mux-chip state > + * @mux_chip: The mux-chip to resume. > + * > + * Restores the mux-chip state. > + * > + * Return: Zero on success or a negative errno on error. > + */ > +int mux_chip_resume(struct mux_chip *mux_chip) > +{ > + int ret, i; > + > + for (i = 0; i < mux_chip->controllers; ++i) { > + struct mux_control *mux = &mux_chip->mux[i]; > + > + if (mux->cached_state != MUX_CACHE_UNKNOWN) { > + ret = mux_control_set(mux, mux->cached_state); > + if (ret < 0) { > + dev_err(&mux_chip->dev, "unable to restore state\n"); > + return ret; I'm don't know what is expected of the core resume code on error, but is it ok to return on first failure? Is it not better to try to restore all muxes and return zero if all is well or the first failure when something is up? But maybe the resume is completely dead anyway if there is any failure? In that case the above early return is fine, I guess... Cheers, Peter > + } > + } > + } > + return 0; > +} > +EXPORT_SYMBOL_GPL(mux_chip_resume); > + > static void devm_mux_chip_release(struct device *dev, void *res) > { > struct mux_chip *mux_chip = *(struct mux_chip **)res; > diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h > index 18824064f8c0..2a7e5ec5d540 100644 > --- a/include/linux/mux/driver.h > +++ b/include/linux/mux/driver.h > @@ -88,6 +88,7 @@ struct mux_chip *mux_chip_alloc(struct device *dev, > int mux_chip_register(struct mux_chip *mux_chip); > void mux_chip_unregister(struct mux_chip *mux_chip); > void mux_chip_free(struct mux_chip *mux_chip); > +int mux_chip_resume(struct mux_chip *mux_chip); > > struct mux_chip *devm_mux_chip_alloc(struct device *dev, > unsigned int controllers, >
On 1/30/24 09:25, Peter Rosin wrote: > Hi! > > 2024-01-26 at 15:36, Thomas Richard wrote: >> The mux_chip_resume() function restores a mux_chip using the cached state >> of each mux. >> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> >> --- >> drivers/mux/core.c | 27 +++++++++++++++++++++++++++ >> include/linux/mux/driver.h | 1 + >> 2 files changed, 28 insertions(+) >> >> diff --git a/drivers/mux/core.c b/drivers/mux/core.c >> index 775816112932..896f74b34eb8 100644 >> --- a/drivers/mux/core.c >> +++ b/drivers/mux/core.c >> @@ -215,6 +215,33 @@ void mux_chip_free(struct mux_chip *mux_chip) >> } >> EXPORT_SYMBOL_GPL(mux_chip_free); >> >> +/** >> + * mux_chip_resume() - restores the mux-chip state >> + * @mux_chip: The mux-chip to resume. >> + * >> + * Restores the mux-chip state. >> + * >> + * Return: Zero on success or a negative errno on error. >> + */ >> +int mux_chip_resume(struct mux_chip *mux_chip) >> +{ >> + int ret, i; >> + >> + for (i = 0; i < mux_chip->controllers; ++i) { >> + struct mux_control *mux = &mux_chip->mux[i]; >> + >> + if (mux->cached_state != MUX_CACHE_UNKNOWN) { >> + ret = mux_control_set(mux, mux->cached_state); >> + if (ret < 0) { >> + dev_err(&mux_chip->dev, "unable to restore state\n"); >> + return ret; > > I'm don't know what is expected of the core resume code on error, > but is it ok to return on first failure? Is it not better to try > to restore all muxes and return zero if all is well or the first > failure when something is up? > > But maybe the resume is completely dead anyway if there is any > failure? In that case the above early return is fine, I guess... > In the first iteration of this series (when it was done in mmio driver), it restored all muxes and returned zero or the first failure. I don't know why I changed the behaviour. For me it's better to try to restores all muxes.
On 1/26/24 22:28, Andy Shevchenko wrote: > On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> >> The mux_chip_resume() function restores a mux_chip using the cached state >> of each mux. > > ... > >> +int mux_chip_resume(struct mux_chip *mux_chip) >> +{ >> + int ret, i; >> + >> + for (i = 0; i < mux_chip->controllers; ++i) { >> + struct mux_control *mux = &mux_chip->mux[i]; > >> + if (mux->cached_state != MUX_CACHE_UNKNOWN) { > > if (_state == ...) > continue; > > ? Yes it makes the code easier to read. Fixed in next iteration.
diff --git a/drivers/mux/core.c b/drivers/mux/core.c index 775816112932..896f74b34eb8 100644 --- a/drivers/mux/core.c +++ b/drivers/mux/core.c @@ -215,6 +215,33 @@ void mux_chip_free(struct mux_chip *mux_chip) } EXPORT_SYMBOL_GPL(mux_chip_free); +/** + * mux_chip_resume() - restores the mux-chip state + * @mux_chip: The mux-chip to resume. + * + * Restores the mux-chip state. + * + * Return: Zero on success or a negative errno on error. + */ +int mux_chip_resume(struct mux_chip *mux_chip) +{ + int ret, i; + + for (i = 0; i < mux_chip->controllers; ++i) { + struct mux_control *mux = &mux_chip->mux[i]; + + if (mux->cached_state != MUX_CACHE_UNKNOWN) { + ret = mux_control_set(mux, mux->cached_state); + if (ret < 0) { + dev_err(&mux_chip->dev, "unable to restore state\n"); + return ret; + } + } + } + return 0; +} +EXPORT_SYMBOL_GPL(mux_chip_resume); + static void devm_mux_chip_release(struct device *dev, void *res) { struct mux_chip *mux_chip = *(struct mux_chip **)res; diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h index 18824064f8c0..2a7e5ec5d540 100644 --- a/include/linux/mux/driver.h +++ b/include/linux/mux/driver.h @@ -88,6 +88,7 @@ struct mux_chip *mux_chip_alloc(struct device *dev, int mux_chip_register(struct mux_chip *mux_chip); void mux_chip_unregister(struct mux_chip *mux_chip); void mux_chip_free(struct mux_chip *mux_chip); +int mux_chip_resume(struct mux_chip *mux_chip); struct mux_chip *devm_mux_chip_alloc(struct device *dev, unsigned int controllers,
The mux_chip_resume() function restores a mux_chip using the cached state of each mux. Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- drivers/mux/core.c | 27 +++++++++++++++++++++++++++ include/linux/mux/driver.h | 1 + 2 files changed, 28 insertions(+)