[PATCH] iio: adc: rzg2l: Use RUNTIME_PM_OPS() instead of SET_*

[PATCH] iio: adc: rzg2l: Use RUNTIME_PM_OPS() instead of SET_*

[Jonathan Cameron]

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>
---
 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),
 	},
 };
 
-- 
2.37.1

Re: [PATCH] iio: adc: rzg2l: Use RUNTIME_PM_OPS() instead of SET_*

[Andy Shevchenko]

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() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: rzg2l: Use RUNTIME_PM_OPS() instead of SET_*

[Jonathan Cameron]

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

Re: [PATCH] iio: adc: rzg2l: Use RUNTIME_PM_OPS() instead of SET_*

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: rzg2l: Use RUNTIME_PM_OPS() instead of SET_*

[Jonathan Cameron]

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

Re: [PATCH] iio: adc: rzg2l: Use RUNTIME_PM_OPS() instead of SET_*

[Jonathan Cameron]

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