| Message ID | 20240102-j7200-pcie-s2r-v1-4-84e55da52400@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
Hi! 2024-01-15 at 17:14, Thomas Richard wrote: > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Implement resume support What Andy said, and please don't omit punctuation. Try to make it a pleasure to read your patches! > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > --- > drivers/mux/mmio.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c > index fd1d121a584b..ab4ef195fc0d 100644 > --- a/drivers/mux/mmio.c > +++ b/drivers/mux/mmio.c > @@ -125,13 +125,47 @@ static int mux_mmio_probe(struct platform_device *pdev) > > mux_chip->ops = &mux_mmio_ops; > > + dev_set_drvdata(dev, mux_chip); > + > return devm_mux_chip_register(dev, mux_chip); > } > > +#ifdef CONFIG_PM > +static int mux_mmio_resume_noirq(struct device *dev) > +{ > + struct mux_chip *mux_chip = dev_get_drvdata(dev); > + int global_ret = 0; > + unsigned int i; > + > + for (i = 0; i < mux_chip->controllers; i++) { > + struct mux_control *mux = &mux_chip->mux[i]; > + int val = mux->cached_state; You are not supposed to look at (or change) cached_state outside the mux core. > + int ret; > + > + if (val == MUX_IDLE_AS_IS) The cached_state can never be MUX_IDLE_AS_IS. Sure, it happens to have the same actual value as the correct MUX_CACHE_UNKNOWN, but abusing that is all kinds of wrong. > + continue; > + > + ret = mux_mmio_set(mux, val); > + if (ret) { If mux_mmio_set() fails, cached_state ends up wrong as it should be set to MUX_CACHE_UNKNOWN on failure. Low-level stuff like this needs to be done by the mux core, or things becomes a maintenance hazard... So, the meat of this function belongs in the mux core since none of it looks mmio specific. It could probably be named mux_chip_resume() or something such. That makes it simple to use the correct constant, and the mux_control_set() helper makes it easy to get the handling of cached_state right. Cheers, Peter > + dev_err(dev, "control %u: error restoring mux: %d\n", i, ret); > + if (!global_ret) > + global_ret = ret; > + } > + } > + > + return global_ret; > +} > +#endif > + > +static const struct dev_pm_ops mux_mmio_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, mux_mmio_resume_noirq) > +}; > + > static struct platform_driver mux_mmio_driver = { > .driver = { > .name = "mmio-mux", > .of_match_table = mux_mmio_dt_ids, > + .pm = &mux_mmio_pm_ops, > }, > .probe = mux_mmio_probe, > }; >
diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c index fd1d121a584b..ab4ef195fc0d 100644 --- a/drivers/mux/mmio.c +++ b/drivers/mux/mmio.c @@ -125,13 +125,47 @@ static int mux_mmio_probe(struct platform_device *pdev) mux_chip->ops = &mux_mmio_ops; + dev_set_drvdata(dev, mux_chip); + return devm_mux_chip_register(dev, mux_chip); } +#ifdef CONFIG_PM +static int mux_mmio_resume_noirq(struct device *dev) +{ + struct mux_chip *mux_chip = dev_get_drvdata(dev); + int global_ret = 0; + unsigned int i; + + for (i = 0; i < mux_chip->controllers; i++) { + struct mux_control *mux = &mux_chip->mux[i]; + int val = mux->cached_state; + int ret; + + if (val == MUX_IDLE_AS_IS) + continue; + + ret = mux_mmio_set(mux, val); + if (ret) { + dev_err(dev, "control %u: error restoring mux: %d\n", i, ret); + if (!global_ret) + global_ret = ret; + } + } + + return global_ret; +} +#endif + +static const struct dev_pm_ops mux_mmio_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, mux_mmio_resume_noirq) +}; + static struct platform_driver mux_mmio_driver = { .driver = { .name = "mmio-mux", .of_match_table = mux_mmio_dt_ids, + .pm = &mux_mmio_pm_ops, }, .probe = mux_mmio_probe, };