| Message ID | 20220807185618.1038812-5-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: PM macro rework continued. | expand |
On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > If CONFIG_PM is not set, the pm_ptr() will ensure that the struct > dev_pm_ops and callbacks are removed without the need for __maybe_unused > markings. > > In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS() because > that would provide suspend and resume functions without the > checks the driver is doing before calling runtime_pm functions > (whether the necessary GPIO is provided). It may be possible to > clean that up in future by moving the checks into the callbacks. ... > static const struct dev_pm_ops srf04_pm_ops = { > - SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend, > - srf04_pm_runtime_resume, NULL) > + RUNTIME_PM_OPS(srf04_pm_runtime_suspend, > + srf04_pm_runtime_resume, NULL) > }; static DEFINE_RUNTIME_DEV_PM_OPS(...); ?
Hi Andy, Le lun., août 8 2022 at 11:28:12 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron <jic23@kernel.org> > wrote: >> >> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> >> If CONFIG_PM is not set, the pm_ptr() will ensure that the struct >> dev_pm_ops and callbacks are removed without the need for >> __maybe_unused >> markings. >> >> In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS() because >> that would provide suspend and resume functions without the >> checks the driver is doing before calling runtime_pm functions >> (whether the necessary GPIO is provided). It may be possible to >> clean that up in future by moving the checks into the callbacks. > > ... > >> static const struct dev_pm_ops srf04_pm_ops = { >> - SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend, >> - srf04_pm_runtime_resume, NULL) >> + RUNTIME_PM_OPS(srf04_pm_runtime_suspend, >> + srf04_pm_runtime_resume, NULL) >> }; > > static DEFINE_RUNTIME_DEV_PM_OPS(...); > > ? Read the commit message? Cheers, -Paul
On Mon, Aug 8, 2022 at 11:35 AM Paul Cercueil <paul@crapouillou.net> wrote: > Le lun., août 8 2022 at 11:28:12 +0200, Andy Shevchenko > <andy.shevchenko@gmail.com> a écrit : > > On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron <jic23@kernel.org> > > wrote: ... > >> In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS() because > >> that would provide suspend and resume functions without the > >> checks the driver is doing before calling runtime_pm functions > >> (whether the necessary GPIO is provided). It may be possible to > >> clean that up in future by moving the checks into the callbacks. > > > > ... > > > >> static const struct dev_pm_ops srf04_pm_ops = { > >> - SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend, > >> - srf04_pm_runtime_resume, NULL) > >> + RUNTIME_PM_OPS(srf04_pm_runtime_suspend, > >> + srf04_pm_runtime_resume, NULL) > >> }; > > > > static DEFINE_RUNTIME_DEV_PM_OPS(...); > > > > ? > > Read the commit message? Yes, and I'm not sure how that part is relevant. The callbacks won't be called if pm_ptr() equals no-op, no?
Le lun., août 8 2022 at 11:39:56 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > On Mon, Aug 8, 2022 at 11:35 AM Paul Cercueil <paul@crapouillou.net> > wrote: >> Le lun., août 8 2022 at 11:28:12 +0200, Andy Shevchenko >> <andy.shevchenko@gmail.com> a écrit : >> > On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron <jic23@kernel.org> >> > wrote: > > ... > >> >> In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS() >> because >> >> that would provide suspend and resume functions without the >> >> checks the driver is doing before calling runtime_pm functions >> >> (whether the necessary GPIO is provided). It may be possible to >> >> clean that up in future by moving the checks into the callbacks. >> > >> > ... >> > >> >> static const struct dev_pm_ops srf04_pm_ops = { >> >> - SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend, >> >> - srf04_pm_runtime_resume, NULL) >> >> + RUNTIME_PM_OPS(srf04_pm_runtime_suspend, >> >> + srf04_pm_runtime_resume, NULL) >> >> }; >> > >> > static DEFINE_RUNTIME_DEV_PM_OPS(...); >> > >> > ? >> >> Read the commit message? > > Yes, and I'm not sure how that part is relevant. The callbacks won't > be called if pm_ptr() equals no-op, no? Have a look at the definition of DEFINE_RUNTIME_DEV_PM_OPS(). I believe it does not do what you think it does. What the commit message says is that using DEFINE_RUNTIME_DEV_PM_OPS() would add .suspend/.resume callbacks, which aren't provided with the current code. Cheers, -Paul
On Mon, Aug 8, 2022 at 11:49 AM Paul Cercueil <paul@crapouillou.net> wrote: > > > > Le lun., août 8 2022 at 11:39:56 +0200, Andy Shevchenko > <andy.shevchenko@gmail.com> a écrit : > > On Mon, Aug 8, 2022 at 11:35 AM Paul Cercueil <paul@crapouillou.net> > > wrote: > >> Le lun., août 8 2022 at 11:28:12 +0200, Andy Shevchenko > >> <andy.shevchenko@gmail.com> a écrit : > >> > On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron <jic23@kernel.org> > >> > wrote: > > > > ... > > > >> >> In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS() > >> because > >> >> that would provide suspend and resume functions without the > >> >> checks the driver is doing before calling runtime_pm functions > >> >> (whether the necessary GPIO is provided). It may be possible to > >> >> clean that up in future by moving the checks into the callbacks. > >> > > >> > ... > >> > > >> >> static const struct dev_pm_ops srf04_pm_ops = { > >> >> - SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend, > >> >> - srf04_pm_runtime_resume, NULL) > >> >> + RUNTIME_PM_OPS(srf04_pm_runtime_suspend, > >> >> + srf04_pm_runtime_resume, NULL) > >> >> }; > >> > > >> > static DEFINE_RUNTIME_DEV_PM_OPS(...); > >> > > >> > ? > >> > >> Read the commit message? > > > > Yes, and I'm not sure how that part is relevant. The callbacks won't > > be called if pm_ptr() equals no-op, no? > > Have a look at the definition of DEFINE_RUNTIME_DEV_PM_OPS(). I believe > it does not do what you think it does. > > What the commit message says is that using DEFINE_RUNTIME_DEV_PM_OPS() > would add .suspend/.resume callbacks, which aren't provided with the > current code. Effectively the use of DEFINE_RUNTIME_DEV_PM_OPS() enables system sleep with the same callbacks as runtime PM. I don't see any disadvantages of using it instead of keeping runtime PM only. That said, I don't see the commit message that describes that nuance. It rather says, as I said, something irrelevant.
Le lun., août 8 2022 at 12:09:35 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > On Mon, Aug 8, 2022 at 11:49 AM Paul Cercueil <paul@crapouillou.net> > wrote: >> >> >> >> Le lun., août 8 2022 at 11:39:56 +0200, Andy Shevchenko >> <andy.shevchenko@gmail.com> a écrit : >> > On Mon, Aug 8, 2022 at 11:35 AM Paul Cercueil >> <paul@crapouillou.net> >> > wrote: >> >> Le lun., août 8 2022 at 11:28:12 +0200, Andy Shevchenko >> >> <andy.shevchenko@gmail.com> a écrit : >> >> > On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron >> <jic23@kernel.org> >> >> > wrote: >> > >> > ... >> > >> >> >> In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS() >> >> because >> >> >> that would provide suspend and resume functions without the >> >> >> checks the driver is doing before calling runtime_pm >> functions >> >> >> (whether the necessary GPIO is provided). It may be >> possible to >> >> >> clean that up in future by moving the checks into the >> callbacks. >> >> > >> >> > ... >> >> > >> >> >> static const struct dev_pm_ops srf04_pm_ops = { >> >> >> - SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend, >> >> >> - srf04_pm_runtime_resume, >> NULL) >> >> >> + RUNTIME_PM_OPS(srf04_pm_runtime_suspend, >> >> >> + srf04_pm_runtime_resume, NULL) >> >> >> }; >> >> > >> >> > static DEFINE_RUNTIME_DEV_PM_OPS(...); >> >> > >> >> > ? >> >> >> >> Read the commit message? >> > >> > Yes, and I'm not sure how that part is relevant. The callbacks >> won't >> > be called if pm_ptr() equals no-op, no? >> >> Have a look at the definition of DEFINE_RUNTIME_DEV_PM_OPS(). I >> believe >> it does not do what you think it does. >> >> What the commit message says is that using >> DEFINE_RUNTIME_DEV_PM_OPS() >> would add .suspend/.resume callbacks, which aren't provided with the >> current code. > > Effectively the use of DEFINE_RUNTIME_DEV_PM_OPS() enables system > sleep with the same callbacks as runtime PM. I don't see any > disadvantages of using it instead of keeping runtime PM only. That > said, I don't see the commit message that describes that nuance. It > rather says, as I said, something irrelevant. Probably no disavantages, yes, but that would be a functional change, so I can understand why it's not done in this patchset. That doesn't mean it cannot be done later, though, but somebody would need to test it on hardware. Cheers, -Paul
On Mon, Aug 8, 2022 at 12:17 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lun., août 8 2022 at 12:09:35 +0200, Andy Shevchenko > <andy.shevchenko@gmail.com> a écrit : > > On Mon, Aug 8, 2022 at 11:49 AM Paul Cercueil <paul@crapouillou.net> > > wrote: > >> Le lun., août 8 2022 at 11:39:56 +0200, Andy Shevchenko > >> <andy.shevchenko@gmail.com> a écrit : > >> > On Mon, Aug 8, 2022 at 11:35 AM Paul Cercueil > >> <paul@crapouillou.net> > >> > wrote: > >> >> Le lun., août 8 2022 at 11:28:12 +0200, Andy Shevchenko > >> >> <andy.shevchenko@gmail.com> a écrit : > >> >> > On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron > >> <jic23@kernel.org> > >> >> > wrote: > >> > > >> > ... > >> > > >> >> >> In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS() > >> >> because > >> >> >> that would provide suspend and resume functions without the > >> >> >> checks the driver is doing before calling runtime_pm > >> functions > >> >> >> (whether the necessary GPIO is provided). It may be > >> possible to > >> >> >> clean that up in future by moving the checks into the > >> callbacks. > >> >> > > >> >> > ... > >> >> > > >> >> >> static const struct dev_pm_ops srf04_pm_ops = { > >> >> >> - SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend, > >> >> >> - srf04_pm_runtime_resume, > >> NULL) > >> >> >> + RUNTIME_PM_OPS(srf04_pm_runtime_suspend, > >> >> >> + srf04_pm_runtime_resume, NULL) > >> >> >> }; > >> >> > > >> >> > static DEFINE_RUNTIME_DEV_PM_OPS(...); > >> >> > > >> >> > ? > >> >> > >> >> Read the commit message? > >> > > >> > Yes, and I'm not sure how that part is relevant. The callbacks > >> won't > >> > be called if pm_ptr() equals no-op, no? > >> > >> Have a look at the definition of DEFINE_RUNTIME_DEV_PM_OPS(). I > >> believe > >> it does not do what you think it does. > >> > >> What the commit message says is that using > >> DEFINE_RUNTIME_DEV_PM_OPS() > >> would add .suspend/.resume callbacks, which aren't provided with the > >> current code. > > > > Effectively the use of DEFINE_RUNTIME_DEV_PM_OPS() enables system > > sleep with the same callbacks as runtime PM. I don't see any > > disadvantages of using it instead of keeping runtime PM only. That > > said, I don't see the commit message that describes that nuance. It > > rather says, as I said, something irrelevant. > > Probably no disavantages, yes, but that would be a functional change, > so I can understand why it's not done in this patchset. That doesn't > mean it cannot be done later, though, but somebody would need to test > it on hardware. This is a fair point.
On Sun, 7 Aug 2022 19:56:16 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > If CONFIG_PM is not set, the pm_ptr() will ensure that the struct > dev_pm_ops and callbacks are removed without the need for __maybe_unused > markings. > > In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS() because > that would provide suspend and resume functions without the > checks the driver is doing before calling runtime_pm functions > (whether the necessary GPIO is provided). It may be possible to > clean that up in future by moving the checks into the callbacks. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Andreas Klinger <ak@it-klinger.de> Some consensus reached in the discussion so even though no one gave a tag I feel comfortable taking this one. The suggested follow up change needs hardware to boost confidence that there are no side effects. Applied. Jonathan
diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c index 05015351a34a..faf2f806ce80 100644 --- a/drivers/iio/proximity/srf04.c +++ b/drivers/iio/proximity/srf04.c @@ -359,7 +359,7 @@ static int srf04_remove(struct platform_device *pdev) return 0; } -static int __maybe_unused srf04_pm_runtime_suspend(struct device *dev) +static int srf04_pm_runtime_suspend(struct device *dev) { struct platform_device *pdev = container_of(dev, struct platform_device, dev); @@ -371,7 +371,7 @@ static int __maybe_unused srf04_pm_runtime_suspend(struct device *dev) return 0; } -static int __maybe_unused srf04_pm_runtime_resume(struct device *dev) +static int srf04_pm_runtime_resume(struct device *dev) { struct platform_device *pdev = container_of(dev, struct platform_device, dev); @@ -385,8 +385,8 @@ static int __maybe_unused srf04_pm_runtime_resume(struct device *dev) } static const struct dev_pm_ops srf04_pm_ops = { - SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend, - srf04_pm_runtime_resume, NULL) + RUNTIME_PM_OPS(srf04_pm_runtime_suspend, + srf04_pm_runtime_resume, NULL) }; static struct platform_driver srf04_driver = { @@ -395,7 +395,7 @@ static struct platform_driver srf04_driver = { .driver = { .name = "srf04-gpio", .of_match_table = of_srf04_match, - .pm = &srf04_pm_ops, + .pm = pm_ptr(&srf04_pm_ops), }, };