| Message ID | 20220807190712.1039403-1-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adc: rzg2l: Use RUNTIME_PM_OPS() instead of SET_* | expand |
On Sun, Aug 7, 2022 at 9:11 PM Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Here we could use DEFINE_RUNTIME_DEV_PM_OPS() but that would have the > side effect of providing suspend and resume support. That would be > harmless but also of little purpose as this driver does very simplistic > power management with synchronous power up and down around individual > channel reads. > > In general these new PM macros avoid the need to mark functions > __maybe_unused, whilst still allowing the compiler to remove them > if they are unused. ... > static const struct dev_pm_ops rzg2l_adc_pm_ops = { > - SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > - rzg2l_adc_pm_runtime_resume, > - NULL) > + RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > + rzg2l_adc_pm_runtime_resume, > + NULL) > }; DEFINE_RUNTIME_DEV_PM_OPS() ?
On Mon, 8 Aug 2022 11:34:23 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Aug 7, 2022 at 9:11 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Here we could use DEFINE_RUNTIME_DEV_PM_OPS() but that would have the > > side effect of providing suspend and resume support. That would be > > harmless but also of little purpose as this driver does very simplistic > > power management with synchronous power up and down around individual > > channel reads. > > > > In general these new PM macros avoid the need to mark functions > > __maybe_unused, whilst still allowing the compiler to remove them > > if they are unused. > > ... > > > static const struct dev_pm_ops rzg2l_adc_pm_ops = { > > - SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > > - rzg2l_adc_pm_runtime_resume, > > - NULL) > > + RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > > + rzg2l_adc_pm_runtime_resume, > > + NULL) > > }; > > DEFINE_RUNTIME_DEV_PM_OPS() ? > Disagreeing with the patch description argument on why I didn't do that? The extra ops set will never have anything to do... Mostly harmless, but kind of gives the wrong impression of what is going on in this driver. Jonathan
On Sat, Aug 13, 2022 at 7:02 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 8 Aug 2022 11:34:23 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Aug 7, 2022 at 9:11 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > Here we could use DEFINE_RUNTIME_DEV_PM_OPS() but that would have the > > > side effect of providing suspend and resume support. That would be > > > harmless but also of little purpose as this driver does very simplistic > > > power management with synchronous power up and down around individual > > > channel reads. > > > > > > In general these new PM macros avoid the need to mark functions > > > __maybe_unused, whilst still allowing the compiler to remove them > > > if they are unused. > > > > ... > > > > > static const struct dev_pm_ops rzg2l_adc_pm_ops = { > > > - SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > > > - rzg2l_adc_pm_runtime_resume, > > > - NULL) > > > + RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > > > + rzg2l_adc_pm_runtime_resume, > > > + NULL) > > > }; > > > > DEFINE_RUNTIME_DEV_PM_OPS() ? > > > Disagreeing with the patch description argument on why I didn't do that? > The extra ops set will never have anything to do... Mostly harmless, > but kind of gives the wrong impression of what is going on in this > driver. As per thread with Paul, this patch has no function change intentions, but also, if tested on hardware, enabling system sleep states shouldn't be harmful.
On Sun, 14 Aug 2022 22:12:59 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Aug 13, 2022 at 7:02 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 8 Aug 2022 11:34:23 +0200 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Sun, Aug 7, 2022 at 9:11 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > Here we could use DEFINE_RUNTIME_DEV_PM_OPS() but that would have the > > > > side effect of providing suspend and resume support. That would be > > > > harmless but also of little purpose as this driver does very simplistic > > > > power management with synchronous power up and down around individual > > > > channel reads. > > > > > > > > In general these new PM macros avoid the need to mark functions > > > > __maybe_unused, whilst still allowing the compiler to remove them > > > > if they are unused. > > > > > > ... > > > > > > > static const struct dev_pm_ops rzg2l_adc_pm_ops = { > > > > - SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > > > > - rzg2l_adc_pm_runtime_resume, > > > > - NULL) > > > > + RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > > > > + rzg2l_adc_pm_runtime_resume, > > > > + NULL) > > > > }; > > > > > > DEFINE_RUNTIME_DEV_PM_OPS() ? > > > > > Disagreeing with the patch description argument on why I didn't do that? > > The extra ops set will never have anything to do... Mostly harmless, > > but kind of gives the wrong impression of what is going on in this > > driver. > > As per thread with Paul, this patch has no function change intentions, > but also, if tested on hardware, enabling system sleep states > shouldn't be harmful. > This one is different from that case where there might be side effects. Here the suspend and resume are (I think) guaranteed to have nothing to do in all cases - because the driver does synchronous power up and power down in all paths. So in all cases we are already in runtime suspended state on a call to suspend. Joanthan
On Sun, 7 Aug 2022 20:07:12 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Here we could use DEFINE_RUNTIME_DEV_PM_OPS() but that would have the > side effect of providing suspend and resume support. That would be > harmless but also of little purpose as this driver does very simplistic > power management with synchronous power up and down around individual > channel reads. > > In general these new PM macros avoid the need to mark functions > __maybe_unused, whilst still allowing the compiler to remove them > if they are unused. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> +CC Prabhakar on basis of recent DT binding patch might have hardware to test changing this to DEFINE_RUNTIME_DEV_PM_OPS. If not I'm tempted to just pick this one up on basis it does no harm and we can revisit later. Thanks, Jonathan > --- > drivers/iio/adc/rzg2l_adc.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c > index 0921ff2d9b3a..b859a2db6b13 100644 > --- a/drivers/iio/adc/rzg2l_adc.c > +++ b/drivers/iio/adc/rzg2l_adc.c > @@ -547,7 +547,7 @@ static const struct of_device_id rzg2l_adc_match[] = { > }; > MODULE_DEVICE_TABLE(of, rzg2l_adc_match); > > -static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev) > +static int rzg2l_adc_pm_runtime_suspend(struct device *dev) > { > struct iio_dev *indio_dev = dev_get_drvdata(dev); > struct rzg2l_adc *adc = iio_priv(indio_dev); > @@ -559,7 +559,7 @@ static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev) > return 0; > } > > -static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev) > +static int rzg2l_adc_pm_runtime_resume(struct device *dev) > { > struct iio_dev *indio_dev = dev_get_drvdata(dev); > struct rzg2l_adc *adc = iio_priv(indio_dev); > @@ -581,9 +581,9 @@ static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev) > } > > static const struct dev_pm_ops rzg2l_adc_pm_ops = { > - SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > - rzg2l_adc_pm_runtime_resume, > - NULL) > + RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, > + rzg2l_adc_pm_runtime_resume, > + NULL) > }; > > static struct platform_driver rzg2l_adc_driver = { > @@ -591,7 +591,7 @@ static struct platform_driver rzg2l_adc_driver = { > .driver = { > .name = DRIVER_NAME, > .of_match_table = rzg2l_adc_match, > - .pm = &rzg2l_adc_pm_ops, > + .pm = pm_ptr(&rzg2l_adc_pm_ops), > }, > }; >
diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c index 0921ff2d9b3a..b859a2db6b13 100644 --- a/drivers/iio/adc/rzg2l_adc.c +++ b/drivers/iio/adc/rzg2l_adc.c @@ -547,7 +547,7 @@ static const struct of_device_id rzg2l_adc_match[] = { }; MODULE_DEVICE_TABLE(of, rzg2l_adc_match); -static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev) +static int rzg2l_adc_pm_runtime_suspend(struct device *dev) { struct iio_dev *indio_dev = dev_get_drvdata(dev); struct rzg2l_adc *adc = iio_priv(indio_dev); @@ -559,7 +559,7 @@ static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev) return 0; } -static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev) +static int rzg2l_adc_pm_runtime_resume(struct device *dev) { struct iio_dev *indio_dev = dev_get_drvdata(dev); struct rzg2l_adc *adc = iio_priv(indio_dev); @@ -581,9 +581,9 @@ static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev) } static const struct dev_pm_ops rzg2l_adc_pm_ops = { - SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, - rzg2l_adc_pm_runtime_resume, - NULL) + RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, + rzg2l_adc_pm_runtime_resume, + NULL) }; static struct platform_driver rzg2l_adc_driver = { @@ -591,7 +591,7 @@ static struct platform_driver rzg2l_adc_driver = { .driver = { .name = DRIVER_NAME, .of_match_table = rzg2l_adc_match, - .pm = &rzg2l_adc_pm_ops, + .pm = pm_ptr(&rzg2l_adc_pm_ops), }, };