[RESEND PATCH 00/28] IIO: Runtime PM related cleanups.

[RESEND PATCH 00/28] IIO: Runtime PM related cleanups.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Resend because I managed to misstype kernel.org in the first set of
email addresses. Sorry all who hence get this twice.

Two basic types of tidying up in here.
1) Cases where pm_runtime_put_noidle() is called in remove() path
   but not matched with any form of get
2) Using pm_runtime_resume_and_get() to either replace existing error
   handling or to introduce it where errors were previously ignored.

Note the unbalanced put is not a bug as such, because the runtime pm core
protects against the reference count going negative.  However, it is
a bad pattern to have copied into new drivers (as it confuses me)
so I'd like to clear it out.

Inspired by Mauro's work on similar issues in media and Julia's
coccicheck script.

There will be at least one more set of these once this first set
have flushed out any mistakes I may have made.

I checked my assumptions around the excess pm_runtime_put_noidle
calls in QEMU but may well be missing something even so.

Cc: Andreas Klinger <ak@it-klinger.de>
Cc: Brian Masney <masneyb@onstation.org>
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Mathieu Othacehe <m.othacehe@gmail.com>
Cc: Mike Looijmans <mike.looijmans@topic.nl>
Cc: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
Cc: Parthiban Nallathambi <pn@denx.de>
Cc: Rui Miguel Silva <rui.silva@linaro.org>
Cc: Sean Nyekjaer <sean@geanix.com>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: jic23@kernel.org

Jonathan Cameron (28):
  iio: gyro: fxa210002c: Balance runtime pm + use
    pm_runtime_resume_and_get().
  iio: light: isl29028: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: light: tsl2583: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: accel: bmc150-accel: Balanced runtime pm + use
    pm_runtime_resume_and_get()
  iio: accel: mma8452: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: accel: mma9551/3: Balance untime pm + use
    pm_runtime_resume_and_get()
  iio: accel: kxcjk-1013: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: hid: trigger: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: imu: kmx61: Balance runtime pm + use pm_runtime_resume_and_get()
  iio: light: rpr0521: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: magn: bmc150: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: accel: bmi088: Balance runtime pm + use
    pm_runtime_resume_and_get()
  iio: imu: mpu6050: Use pm_runtime_resume_and_get() to replace open
    coding.
  iio: adc: stm32-adc: Use pm_runtime_resume_and_get() to replace open
    coding.
  iio: adc: stm32-dfsdm: Use pm_runtime_resume_and_get() to replace open
    coding.
  iio: dac: stm32-dac: Use pm_runtime_resume_and_get() to replace open
    coding.
  iio: adc: ads1015: Use pm_runtime_resume_and_get() to replace open
    coding.
  iio: chemical: atlas-sensor: Use pm_runtime_resume_and_get() to
    replace open coding.
  iio: light: pa12203001: Use pm_runtime_resume_and_get() to replace
    open coding.
  iio: light: us5182: Use pm_runtime_resume_and_get() to replace open
    coding.
  iio: light: vcnl4000: Use pm_runtime_resume_and_get() to replace open
    coding.
  iio: light: vcnl4035: Use pm_runtime_resume_and_get() to replace open
    coding.
  iio: pressure: zpa2326: fix potential extra call of runtime suspend.
  iio: temp: mlx90614: Handle failure in pm_runtime_resume_and_get()
  iio: proximity: srf04: Use pm_runtime_resume_and_get() and handle
    error
  iio: prox: pulsed-light-v2: Switch pm_runtime_resume_and_get()
  iio: pressure: icp10100: Use pm_runtime_resume_and_get() + handle
    errors
  iio: adc: rcar-gyroadc: Use pm_runtime_resume_and_get() and check in
    probe()

 drivers/iio/accel/bmc150-accel-core.c         |  6 +----
 drivers/iio/accel/bmi088-accel-core.c         | 26 ++++++++++++++-----
 drivers/iio/accel/kxcjk-1013.c                |  5 +---
 drivers/iio/accel/mma8452.c                   |  5 +---
 drivers/iio/accel/mma9551.c                   |  1 -
 drivers/iio/accel/mma9551_core.c              |  4 +--
 drivers/iio/accel/mma9553.c                   |  1 -
 drivers/iio/adc/rcar-gyroadc.c                | 15 +++++------
 drivers/iio/adc/stm32-adc.c                   | 24 ++++++-----------
 drivers/iio/adc/stm32-dfsdm-core.c            |  6 ++---
 drivers/iio/adc/ti-ads1015.c                  |  4 +--
 drivers/iio/chemical/atlas-sensor.c           | 12 +++------
 .../common/hid-sensors/hid-sensor-trigger.c   |  8 ++----
 drivers/iio/dac/stm32-dac.c                   |  3 +--
 drivers/iio/gyro/fxas21002c_core.c            | 11 +-------
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 18 +++++--------
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  6 ++---
 drivers/iio/imu/kmx61.c                       |  5 +---
 drivers/iio/light/isl29028.c                  |  5 +---
 drivers/iio/light/pa12203001.c                |  4 +--
 drivers/iio/light/rpr0521.c                   |  7 +----
 drivers/iio/light/tsl2583.c                   | 13 +++++-----
 drivers/iio/light/us5182d.c                   |  4 +--
 drivers/iio/light/vcnl4000.c                  |  4 +--
 drivers/iio/light/vcnl4035.c                  |  4 +--
 drivers/iio/magnetometer/bmc150_magn.c        | 10 +++----
 drivers/iio/pressure/icp10100.c               |  4 ++-
 drivers/iio/pressure/zpa2326.c                |  6 ++---
 .../iio/proximity/pulsedlight-lidar-lite-v2.c |  4 ++-
 drivers/iio/proximity/srf04.c                 |  8 +++---
 drivers/iio/temperature/mlx90614.c            | 25 ++++++++++++++----
 31 files changed, 109 insertions(+), 149 deletions(-)

-- 
2.31.1

[PATCH 01/28] iio: gyro: fxa210002c: Balance runtime pm + use pm_runtime_resume_and_get().

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

In both the probe() error path and remove() pm_runtime_put_noidle()
is called which will decrement the runtime pm reference count.
However, there is no matching function to have raised the reference count.
Not this isn't a fix as the runtime pm core will stop the reference count
going negative anyway.

An alternative would have been to raise the count in these paths, but
it is not clear why that would be necessary.

Whilst we are here replace some boilerplate with pm_runtime_resume_and_get()
Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Rui Miguel Silva <rui.silva@linaro.org>
---
 drivers/iio/gyro/fxas21002c_core.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
index 5af7b48ff01a..539585b0d300 100644
--- a/drivers/iio/gyro/fxas21002c_core.c
+++ b/drivers/iio/gyro/fxas21002c_core.c
@@ -366,14 +366,7 @@ static int fxas21002c_write(struct fxas21002c_data *data,
 
 static int  fxas21002c_pm_get(struct fxas21002c_data *data)
 {
-	struct device *dev = regmap_get_device(data->regmap);
-	int ret;
-
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		pm_runtime_put_noidle(dev);
-
-	return ret;
+	return pm_runtime_resume_and_get(regmap_get_device(data->regmap));
 }
 
 static int  fxas21002c_pm_put(struct fxas21002c_data *data)
@@ -1002,7 +995,6 @@ int fxas21002c_core_probe(struct device *dev, struct regmap *regmap, int irq,
 pm_disable:
 	pm_runtime_disable(dev);
 	pm_runtime_set_suspended(dev);
-	pm_runtime_put_noidle(dev);
 
 	return ret;
 }
@@ -1016,7 +1008,6 @@ void fxas21002c_core_remove(struct device *dev)
 
 	pm_runtime_disable(dev);
 	pm_runtime_set_suspended(dev);
-	pm_runtime_put_noidle(dev);
 }
 EXPORT_SYMBOL_GPL(fxas21002c_core_remove);
 
-- 
2.31.1

Re: [PATCH 01/28] iio: gyro: fxa210002c: Balance runtime pm + use pm_runtime_resume_and_get().

[Rui Miguel Silva]

Hi Jonathan,
thanks for the patch.

On Sun May 9, 2021 at 12:33 PM WEST, Jonathan Cameron wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> In both the probe() error path and remove() pm_runtime_put_noidle()
> is called which will decrement the runtime pm reference count.
> However, there is no matching function to have raised the reference count.
> Not this isn't a fix as the runtime pm core will stop the reference count
> going negative anyway.
>
> An alternative would have been to raise the count in these paths, but
> it is not clear why that would be necessary.
>
> Whilst we are here replace some boilerplate with pm_runtime_resume_and_get()
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Rui Miguel Silva <rui.silva@linaro.org>

With s/fxa210002c:/fxa21002c:/ in the patch subject:

Reviewed-by: Rui Miguel Silva <rui.silva@linaro.org>

Cheers,
   Rui

> ---
>  drivers/iio/gyro/fxas21002c_core.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> index 5af7b48ff01a..539585b0d300 100644
> --- a/drivers/iio/gyro/fxas21002c_core.c
> +++ b/drivers/iio/gyro/fxas21002c_core.c
> @@ -366,14 +366,7 @@ static int fxas21002c_write(struct fxas21002c_data *data,
>  
>  static int  fxas21002c_pm_get(struct fxas21002c_data *data)
>  {
> -	struct device *dev = regmap_get_device(data->regmap);
> -	int ret;
> -
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> -		pm_runtime_put_noidle(dev);
> -
> -	return ret;
> +	return pm_runtime_resume_and_get(regmap_get_device(data->regmap));
>  }
>  
>  static int  fxas21002c_pm_put(struct fxas21002c_data *data)
> @@ -1002,7 +995,6 @@ int fxas21002c_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  pm_disable:
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
>  
>  	return ret;
>  }
> @@ -1016,7 +1008,6 @@ void fxas21002c_core_remove(struct device *dev)
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
>  }
>  EXPORT_SYMBOL_GPL(fxas21002c_core_remove);
>  
> -- 
> 2.31.1

Re: [PATCH 01/28] iio: gyro: fxa210002c: Balance runtime pm + use pm_runtime_resume_and_get().

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:27 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> In both the probe() error path and remove() pm_runtime_put_noidle()
> is called which will decrement the runtime pm reference count.
> However, there is no matching function to have raised the reference count.
> Not this isn't a fix as the runtime pm core will stop the reference count
> going negative anyway.
> 
> An alternative would have been to raise the count in these paths, but
> it is not clear why that would be necessary.
> 
> Whilst we are here replace some boilerplate with pm_runtime_resume_and_get()
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Rui Miguel Silva <rui.silva@linaro.org>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/gyro/fxas21002c_core.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> index 5af7b48ff01a..539585b0d300 100644
> --- a/drivers/iio/gyro/fxas21002c_core.c
> +++ b/drivers/iio/gyro/fxas21002c_core.c
> @@ -366,14 +366,7 @@ static int fxas21002c_write(struct fxas21002c_data *data,
>  
>  static int  fxas21002c_pm_get(struct fxas21002c_data *data)
>  {
> -	struct device *dev = regmap_get_device(data->regmap);
> -	int ret;
> -
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> -		pm_runtime_put_noidle(dev);
> -
> -	return ret;
> +	return pm_runtime_resume_and_get(regmap_get_device(data->regmap));
>  }
>  
>  static int  fxas21002c_pm_put(struct fxas21002c_data *data)
> @@ -1002,7 +995,6 @@ int fxas21002c_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  pm_disable:
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
>  
>  	return ret;
>  }
> @@ -1016,7 +1008,6 @@ void fxas21002c_core_remove(struct device *dev)
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
>  }
>  EXPORT_SYMBOL_GPL(fxas21002c_core_remove);
>  



Thanks,
Mauro

Re: [PATCH 01/28] iio: gyro: fxa210002c: Balance runtime pm + use pm_runtime_resume_and_get().

[Mauro Carvalho Chehab]

Em Wed, 12 May 2021 15:22:43 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Sun,  9 May 2021 12:33:27 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > In both the probe() error path and remove() pm_runtime_put_noidle()
> > is called which will decrement the runtime pm reference count.
> > However, there is no matching function to have raised the reference count.
> > Not this isn't a fix as the runtime pm core will stop the reference count
> > going negative anyway.
> > 
> > An alternative would have been to raise the count in these paths, but
> > it is not clear why that would be necessary.
> > 
> > Whilst we are here replace some boilerplate with pm_runtime_resume_and_get()
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Rui Miguel Silva <rui.silva@linaro.org>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Hmm...not quite...

> > diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> > index 5af7b48ff01a..539585b0d300 100644
> > --- a/drivers/iio/gyro/fxas21002c_core.c
> > +++ b/drivers/iio/gyro/fxas21002c_core.c
> > @@ -366,14 +366,7 @@ static int fxas21002c_write(struct fxas21002c_data *data,
> >  
> >  static int  fxas21002c_pm_get(struct fxas21002c_data *data)
> >  {
> > -	struct device *dev = regmap_get_device(data->regmap);
> > -	int ret;
> > -
> > -	ret = pm_runtime_get_sync(dev);
> > -	if (ret < 0)
> > -		pm_runtime_put_noidle(dev);
> > -
> > -	return ret;
> > +	return pm_runtime_resume_and_get(regmap_get_device(data->regmap));
> >  }

fxas21002c_temp_get() and fxas21002c_axis_get() seem to be
missing a pm_runtime_put*() if something gets wrong at
regmap_field_read(), e. g.:

static int fxas21002c_axis_get(struct fxas21002c_data *data,
                               int index, int *val)
{
        struct device *dev = regmap_get_device(data->regmap);
        __be16 axis_be;
        int ret;

        mutex_lock(&data->lock);
        ret = fxas21002c_pm_get(data);
        if (ret < 0)
                goto data_unlock;

        ret = regmap_bulk_read(data->regmap, FXAS21002C_AXIS_TO_REG(index),
                               &axis_be, sizeof(axis_be));
        if (ret < 0) {
                dev_err(dev, "failed to read axis: %d: %d\n", index, ret);
+		fxas21002c_pm_put(data);
                goto data_unlock;
        }

	*val = sign_extend32(be16_to_cpu(axis_be), 15);

        ret = fxas21002c_pm_put(data);
        if (ret < 0)
                goto data_unlock;

        ret = IIO_VAL_INT;

data_unlock:
	mutex_unlock(&data->lock);

        return ret;
}

Thanks,
Mauro

Re: [PATCH 01/28] iio: gyro: fxa210002c: Balance runtime pm + use pm_runtime_resume_and_get().

[Rui Miguel Silva]

Hi Mauro,
On Wed May 12, 2021 at 2:30 PM WEST, Mauro Carvalho Chehab wrote:

> Em Wed, 12 May 2021 15:22:43 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
>
> > Em Sun,  9 May 2021 12:33:27 +0100
> > Jonathan Cameron <jic23@kernel.org> escreveu:
> > 
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > In both the probe() error path and remove() pm_runtime_put_noidle()
> > > is called which will decrement the runtime pm reference count.
> > > However, there is no matching function to have raised the reference count.
> > > Not this isn't a fix as the runtime pm core will stop the reference count
> > > going negative anyway.
> > > 
> > > An alternative would have been to raise the count in these paths, but
> > > it is not clear why that would be necessary.
> > > 
> > > Whilst we are here replace some boilerplate with pm_runtime_resume_and_get()
> > > Found using coccicheck script under review at:
> > > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Rui Miguel Silva <rui.silva@linaro.org>  
> > 
> > LGTM.
> > 
> > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>
> Hmm...not quite...
>
> > > diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> > > index 5af7b48ff01a..539585b0d300 100644
> > > --- a/drivers/iio/gyro/fxas21002c_core.c
> > > +++ b/drivers/iio/gyro/fxas21002c_core.c
> > > @@ -366,14 +366,7 @@ static int fxas21002c_write(struct fxas21002c_data *data,
> > >  
> > >  static int  fxas21002c_pm_get(struct fxas21002c_data *data)
> > >  {
> > > -	struct device *dev = regmap_get_device(data->regmap);
> > > -	int ret;
> > > -
> > > -	ret = pm_runtime_get_sync(dev);
> > > -	if (ret < 0)
> > > -		pm_runtime_put_noidle(dev);
> > > -
> > > -	return ret;
> > > +	return pm_runtime_resume_and_get(regmap_get_device(data->regmap));
> > >  }
>
> fxas21002c_temp_get() and fxas21002c_axis_get() seem to be
> missing a pm_runtime_put*() if something gets wrong at
> regmap_field_read(), e. g.:

Thanks for catching this, you are right. However I think it should
have is own patch, since it is a real fix and this one is not.

Jonathan, bellow goes a proper patch, please feel free to squash this
on top of yours, or just apply it separately, your call.

Cheers,
     Rui

8<-------------------------------------------------------------------

From 314a56247515c05c0c5504c1e305127878af0c9b Mon Sep 17 00:00:00 2001
From: Rui Miguel Silva <rui.silva@linaro.org>
Date: Wed, 12 May 2021 23:39:29 +0100
Subject: [PATCH] iio: gyro: fxas21002c: balance runtime power in error path

If we fail to read temperature or axis we need to decrement the
runtime pm reference count to trigger autosuspend.

Add the call to pm_put to do that in case of error.

Fixes: a0701b6263ae ("iio: gyro: add core driver for fxas21002c")
Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 drivers/iio/gyro/fxas21002c_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
index 1a20c6b88e7d..645461c70454 100644
--- a/drivers/iio/gyro/fxas21002c_core.c
+++ b/drivers/iio/gyro/fxas21002c_core.c
@@ -399,6 +399,7 @@ static int fxas21002c_temp_get(struct fxas21002c_data *data, int *val)
 	ret = regmap_field_read(data->regmap_fields[F_TEMP], &temp);
 	if (ret < 0) {
 		dev_err(dev, "failed to read temp: %d\n", ret);
+		fxas21002c_pm_put(data);
 		goto data_unlock;
 	}
 
@@ -432,6 +433,7 @@ static int fxas21002c_axis_get(struct fxas21002c_data *data,
 			       &axis_be, sizeof(axis_be));
 	if (ret < 0) {
 		dev_err(dev, "failed to read axis: %d: %d\n", index, ret);
+		fxas21002c_pm_put(data);
 		goto data_unlock;
 	}
 
-- 
2.31.1

Re: [PATCH 01/28] iio: gyro: fxa210002c: Balance runtime pm + use pm_runtime_resume_and_get().

[Jonathan Cameron]

On Thu, 13 May 2021 09:34:52 +0100
"Rui Miguel Silva" <rui.silva@linaro.org> wrote:

> Hi Mauro,
> On Wed May 12, 2021 at 2:30 PM WEST, Mauro Carvalho Chehab wrote:
> 
> > Em Wed, 12 May 2021 15:22:43 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> >  
> > > Em Sun,  9 May 2021 12:33:27 +0100
> > > Jonathan Cameron <jic23@kernel.org> escreveu:
> > >   
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > In both the probe() error path and remove() pm_runtime_put_noidle()
> > > > is called which will decrement the runtime pm reference count.
> > > > However, there is no matching function to have raised the reference count.
> > > > Not this isn't a fix as the runtime pm core will stop the reference count
> > > > going negative anyway.
> > > > 
> > > > An alternative would have been to raise the count in these paths, but
> > > > it is not clear why that would be necessary.
> > > > 
> > > > Whilst we are here replace some boilerplate with pm_runtime_resume_and_get()
> > > > Found using coccicheck script under review at:
> > > > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > > > 
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Cc: Rui Miguel Silva <rui.silva@linaro.org>    
> > > 
> > > LGTM.
> > > 
> > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> >
> > Hmm...not quite...
> >  
> > > > diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> > > > index 5af7b48ff01a..539585b0d300 100644
> > > > --- a/drivers/iio/gyro/fxas21002c_core.c
> > > > +++ b/drivers/iio/gyro/fxas21002c_core.c
> > > > @@ -366,14 +366,7 @@ static int fxas21002c_write(struct fxas21002c_data *data,
> > > >  
> > > >  static int  fxas21002c_pm_get(struct fxas21002c_data *data)
> > > >  {
> > > > -	struct device *dev = regmap_get_device(data->regmap);
> > > > -	int ret;
> > > > -
> > > > -	ret = pm_runtime_get_sync(dev);
> > > > -	if (ret < 0)
> > > > -		pm_runtime_put_noidle(dev);
> > > > -
> > > > -	return ret;
> > > > +	return pm_runtime_resume_and_get(regmap_get_device(data->regmap));
> > > >  }  
> >
> > fxas21002c_temp_get() and fxas21002c_axis_get() seem to be
> > missing a pm_runtime_put*() if something gets wrong at
> > regmap_field_read(), e. g.:  
> 
> Thanks for catching this, you are right. However I think it should
> have is own patch, since it is a real fix and this one is not.
> 
> Jonathan, bellow goes a proper patch, please feel free to squash this
> on top of yours, or just apply it separately, your call.
> 
> Cheers,
>      Rui
> 
> 8<-------------------------------------------------------------------
> 
> From 314a56247515c05c0c5504c1e305127878af0c9b Mon Sep 17 00:00:00 2001
> From: Rui Miguel Silva <rui.silva@linaro.org>
> Date: Wed, 12 May 2021 23:39:29 +0100
> Subject: [PATCH] iio: gyro: fxas21002c: balance runtime power in error path
> 
> If we fail to read temperature or axis we need to decrement the
> runtime pm reference count to trigger autosuspend.
> 
> Add the call to pm_put to do that in case of error.
> 
> Fixes: a0701b6263ae ("iio: gyro: add core driver for fxas21002c")
> Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>

I've queued this as a fix in my fixes-togreg branch of iio.git and marked
it for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/gyro/fxas21002c_core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> index 1a20c6b88e7d..645461c70454 100644
> --- a/drivers/iio/gyro/fxas21002c_core.c
> +++ b/drivers/iio/gyro/fxas21002c_core.c
> @@ -399,6 +399,7 @@ static int fxas21002c_temp_get(struct fxas21002c_data *data, int *val)
>  	ret = regmap_field_read(data->regmap_fields[F_TEMP], &temp);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to read temp: %d\n", ret);
> +		fxas21002c_pm_put(data);
>  		goto data_unlock;
>  	}
>  
> @@ -432,6 +433,7 @@ static int fxas21002c_axis_get(struct fxas21002c_data *data,
>  			       &axis_be, sizeof(axis_be));
>  	if (ret < 0) {
>  		dev_err(dev, "failed to read axis: %d: %d\n", index, ret);
> +		fxas21002c_pm_put(data);
>  		goto data_unlock;
>  	}
>  

Re: [PATCH 01/28] iio: gyro: fxa210002c: Balance runtime pm + use pm_runtime_resume_and_get().

[Jonathan Cameron]

On Thu, 13 May 2021 17:31:31 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 13 May 2021 09:34:52 +0100
> "Rui Miguel Silva" <rui.silva@linaro.org> wrote:
> 
> > Hi Mauro,
> > On Wed May 12, 2021 at 2:30 PM WEST, Mauro Carvalho Chehab wrote:
> >   
> > > Em Wed, 12 May 2021 15:22:43 +0200
> > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> > >    
> > > > Em Sun,  9 May 2021 12:33:27 +0100
> > > > Jonathan Cameron <jic23@kernel.org> escreveu:
> > > >     
> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > 
> > > > > In both the probe() error path and remove() pm_runtime_put_noidle()
> > > > > is called which will decrement the runtime pm reference count.
> > > > > However, there is no matching function to have raised the reference count.
> > > > > Not this isn't a fix as the runtime pm core will stop the reference count
> > > > > going negative anyway.
> > > > > 
> > > > > An alternative would have been to raise the count in these paths, but
> > > > > it is not clear why that would be necessary.
> > > > > 
> > > > > Whilst we are here replace some boilerplate with pm_runtime_resume_and_get()
> > > > > Found using coccicheck script under review at:
> > > > > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > > > > 
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > Cc: Rui Miguel Silva <rui.silva@linaro.org>      
> > > > 
> > > > LGTM.
> > > > 
> > > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>    
> > >
> > > Hmm...not quite...

Because the fix and the cleanup are in substantially different places in the
driver I'm going to assume git will sort it out and apply the original patch to my
togreg branch - initially pushed out as testing for bots to poke at.

Thanks,

Jonathan
> > >    
> > > > > diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> > > > > index 5af7b48ff01a..539585b0d300 100644
> > > > > --- a/drivers/iio/gyro/fxas21002c_core.c
> > > > > +++ b/drivers/iio/gyro/fxas21002c_core.c
> > > > > @@ -366,14 +366,7 @@ static int fxas21002c_write(struct fxas21002c_data *data,
> > > > >  
> > > > >  static int  fxas21002c_pm_get(struct fxas21002c_data *data)
> > > > >  {
> > > > > -	struct device *dev = regmap_get_device(data->regmap);
> > > > > -	int ret;
> > > > > -
> > > > > -	ret = pm_runtime_get_sync(dev);
> > > > > -	if (ret < 0)
> > > > > -		pm_runtime_put_noidle(dev);
> > > > > -
> > > > > -	return ret;
> > > > > +	return pm_runtime_resume_and_get(regmap_get_device(data->regmap));
> > > > >  }    
> > >
> > > fxas21002c_temp_get() and fxas21002c_axis_get() seem to be
> > > missing a pm_runtime_put*() if something gets wrong at
> > > regmap_field_read(), e. g.:    
> > 
> > Thanks for catching this, you are right. However I think it should
> > have is own patch, since it is a real fix and this one is not.
> > 
> > Jonathan, bellow goes a proper patch, please feel free to squash this
> > on top of yours, or just apply it separately, your call.
> > 
> > Cheers,
> >      Rui
> > 
> > 8<-------------------------------------------------------------------
> > 
> > From 314a56247515c05c0c5504c1e305127878af0c9b Mon Sep 17 00:00:00 2001
> > From: Rui Miguel Silva <rui.silva@linaro.org>
> > Date: Wed, 12 May 2021 23:39:29 +0100
> > Subject: [PATCH] iio: gyro: fxas21002c: balance runtime power in error path
> > 
> > If we fail to read temperature or axis we need to decrement the
> > runtime pm reference count to trigger autosuspend.
> > 
> > Add the call to pm_put to do that in case of error.
> > 
> > Fixes: a0701b6263ae ("iio: gyro: add core driver for fxas21002c")
> > Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>  
> 
> I've queued this as a fix in my fixes-togreg branch of iio.git and marked
> it for stable.
> 


> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/gyro/fxas21002c_core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/gyro/fxas21002c_core.c b/drivers/iio/gyro/fxas21002c_core.c
> > index 1a20c6b88e7d..645461c70454 100644
> > --- a/drivers/iio/gyro/fxas21002c_core.c
> > +++ b/drivers/iio/gyro/fxas21002c_core.c
> > @@ -399,6 +399,7 @@ static int fxas21002c_temp_get(struct fxas21002c_data *data, int *val)
> >  	ret = regmap_field_read(data->regmap_fields[F_TEMP], &temp);
> >  	if (ret < 0) {
> >  		dev_err(dev, "failed to read temp: %d\n", ret);
> > +		fxas21002c_pm_put(data);
> >  		goto data_unlock;
> >  	}
> >  
> > @@ -432,6 +433,7 @@ static int fxas21002c_axis_get(struct fxas21002c_data *data,
> >  			       &axis_be, sizeof(axis_be));
> >  	if (ret < 0) {
> >  		dev_err(dev, "failed to read axis: %d: %d\n", index, ret);
> > +		fxas21002c_pm_put(data);
> >  		goto data_unlock;
> >  	}
> >    
> 

[PATCH 02/28] iio: light: isl29028: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

In remove this driver called pm_runtime_put_noidle() but there is
no matching get operation.  This does not cause any problems because
the reference counter will not change if already zero, but it
does make the code harder to reason about so should be dropped.

Whilst we are here, use pm_runtime_resume_and_get() to replace open
coded version.
Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/iio/light/isl29028.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/light/isl29028.c b/drivers/iio/light/isl29028.c
index 2f8b494f3e08..9de3262aa688 100644
--- a/drivers/iio/light/isl29028.c
+++ b/drivers/iio/light/isl29028.c
@@ -339,9 +339,7 @@ static int isl29028_set_pm_runtime_busy(struct isl29028_chip *chip, bool on)
 	int ret;
 
 	if (on) {
-		ret = pm_runtime_get_sync(dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(dev);
+		ret = pm_runtime_resume_and_get(dev);
 	} else {
 		pm_runtime_mark_last_busy(dev);
 		ret = pm_runtime_put_autosuspend(dev);
@@ -647,7 +645,6 @@ static int isl29028_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	return isl29028_clear_configure_reg(chip);
 }
-- 
2.31.1

Re: [PATCH 02/28] iio: light: isl29028: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:28 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> In remove this driver called pm_runtime_put_noidle() but there is
> no matching get operation.  This does not cause any problems because
> the reference counter will not change if already zero, but it
> does make the code harder to reason about so should be dropped.
> 
> Whilst we are here, use pm_runtime_resume_and_get() to replace open
> coded version.
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/light/isl29028.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/isl29028.c b/drivers/iio/light/isl29028.c
> index 2f8b494f3e08..9de3262aa688 100644
> --- a/drivers/iio/light/isl29028.c
> +++ b/drivers/iio/light/isl29028.c
> @@ -339,9 +339,7 @@ static int isl29028_set_pm_runtime_busy(struct isl29028_chip *chip, bool on)
>  	int ret;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
>  		ret = pm_runtime_put_autosuspend(dev);
> @@ -647,7 +645,6 @@ static int isl29028_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	return isl29028_clear_configure_reg(chip);
>  }



Thanks,
Mauro

Re: [PATCH 02/28] iio: light: isl29028: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 15:33:35 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:28 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > In remove this driver called pm_runtime_put_noidle() but there is
> > no matching get operation.  This does not cause any problems because
> > the reference counter will not change if already zero, but it
> > does make the code harder to reason about so should be dropped.
> > 
> > Whilst we are here, use pm_runtime_resume_and_get() to replace open
> > coded version.
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied to the togreg branch of iio.git

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/light/isl29028.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/light/isl29028.c b/drivers/iio/light/isl29028.c
> > index 2f8b494f3e08..9de3262aa688 100644
> > --- a/drivers/iio/light/isl29028.c
> > +++ b/drivers/iio/light/isl29028.c
> > @@ -339,9 +339,7 @@ static int isl29028_set_pm_runtime_busy(struct isl29028_chip *chip, bool on)
> >  	int ret;
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(dev);
> > -		if (ret < 0)
> > -			pm_runtime_put_noidle(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(dev);
> >  		ret = pm_runtime_put_autosuspend(dev);
> > @@ -647,7 +645,6 @@ static int isl29028_remove(struct i2c_client *client)
> >  
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  
> >  	return isl29028_clear_configure_reg(chip);
> >  }  
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH 02/28] iio: light: isl29028: Balance runtime pm + use pm_runtime_resume_and_get()

[Linus Walleij]

On Sun, May 9, 2021 at 1:36 PM Jonathan Cameron <jic23@kernel.org> wrote:

> Whilst we are here, use pm_runtime_resume_and_get() to replace open
> coded version.

Wow that is a new API. Why didn't I know about it...
Oh it is only from november last year that is why.
I guess we should switch to this everywhere.

> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 03/28] iio: light: tsl2583: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Error paths in read_raw() and write_raw() callbacks failed to perform and
type of runtime pm put().  Remove called pm_runtime_put_noidle()
but there is no equivalent get (this is safe because the reference
count is protected against going below zero, but it is misleading.

Whilst here use pm_runtime_resume_and_get() to replace boilerplate.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Brian Masney <masneyb@onstation.org>
---
 drivers/iio/light/tsl2583.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c
index 0f787bfc88fc..1f4ad4047a86 100644
--- a/drivers/iio/light/tsl2583.c
+++ b/drivers/iio/light/tsl2583.c
@@ -636,9 +636,7 @@ static int tsl2583_set_pm_runtime_busy(struct tsl2583_chip *chip, bool on)
 	int ret;
 
 	if (on) {
-		ret = pm_runtime_get_sync(&chip->client->dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(&chip->client->dev);
+		ret = pm_runtime_resume_and_get(&chip->client->dev);
 	} else {
 		pm_runtime_mark_last_busy(&chip->client->dev);
 		ret = pm_runtime_put_autosuspend(&chip->client->dev);
@@ -721,8 +719,10 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev,
 read_done:
 	mutex_unlock(&chip->als_mutex);
 
-	if (ret < 0)
+	if (ret < 0) {
+		tsl2583_set_pm_runtime_busy(chip, false);
 		return ret;
+	}
 
 	/*
 	 * Preserve the ret variable if the call to
@@ -783,8 +783,10 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,
 
 	mutex_unlock(&chip->als_mutex);
 
-	if (ret < 0)
+	if (ret < 0) {
+		tsl2583_set_pm_runtime_busy(chip, false);
 		return ret;
+	}
 
 	ret = tsl2583_set_pm_runtime_busy(chip, false);
 	if (ret < 0)
@@ -872,7 +874,6 @@ static int tsl2583_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	return tsl2583_set_power_state(chip, TSL2583_CNTL_PWR_OFF);
 }
-- 
2.31.1

Re: [PATCH 03/28] iio: light: tsl2583: Balance runtime pm + use pm_runtime_resume_and_get()

[Brian Masney]

On Sun, May 09, 2021 at 12:33:29PM +0100, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Error paths in read_raw() and write_raw() callbacks failed to perform and
> type of runtime pm put().  Remove called pm_runtime_put_noidle()
> but there is no equivalent get (this is safe because the reference
> count is protected against going below zero, but it is misleading.
> 
> Whilst here use pm_runtime_resume_and_get() to replace boilerplate.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Brian Masney <masneyb@onstation.org>

Reviewed-by: Brian Masney <masneyb@onstation.org>

Re: [PATCH 03/28] iio: light: tsl2583: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Mon, 10 May 2021 06:46:01 -0400
Brian Masney <masneyb@onstation.org> wrote:

> On Sun, May 09, 2021 at 12:33:29PM +0100, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Error paths in read_raw() and write_raw() callbacks failed to perform and
> > type of runtime pm put().  Remove called pm_runtime_put_noidle()
> > but there is no equivalent get (this is safe because the reference
> > count is protected against going below zero, but it is misleading.
> > 
> > Whilst here use pm_runtime_resume_and_get() to replace boilerplate.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Brian Masney <masneyb@onstation.org>  
> 
> Reviewed-by: Brian Masney <masneyb@onstation.org>
> 
Applied.  Thanks,

Jonathan

Re: [PATCH 03/28] iio: light: tsl2583: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:29 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Error paths in read_raw() and write_raw() callbacks failed to perform and
> type of runtime pm put().  Remove called pm_runtime_put_noidle()
> but there is no equivalent get (this is safe because the reference
> count is protected against going below zero, but it is misleading.
> 
> Whilst here use pm_runtime_resume_and_get() to replace boilerplate.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Brian Masney <masneyb@onstation.org>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>


> ---
>  drivers/iio/light/tsl2583.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c
> index 0f787bfc88fc..1f4ad4047a86 100644
> --- a/drivers/iio/light/tsl2583.c
> +++ b/drivers/iio/light/tsl2583.c
> @@ -636,9 +636,7 @@ static int tsl2583_set_pm_runtime_busy(struct tsl2583_chip *chip, bool on)
>  	int ret;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(&chip->client->dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(&chip->client->dev);
> +		ret = pm_runtime_resume_and_get(&chip->client->dev);
>  	} else {
>  		pm_runtime_mark_last_busy(&chip->client->dev);
>  		ret = pm_runtime_put_autosuspend(&chip->client->dev);
> @@ -721,8 +719,10 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev,
>  read_done:
>  	mutex_unlock(&chip->als_mutex);
>  
> -	if (ret < 0)
> +	if (ret < 0) {
> +		tsl2583_set_pm_runtime_busy(chip, false);
>  		return ret;
> +	}
>  
>  	/*
>  	 * Preserve the ret variable if the call to
> @@ -783,8 +783,10 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,
>  
>  	mutex_unlock(&chip->als_mutex);
>  
> -	if (ret < 0)
> +	if (ret < 0) {
> +		tsl2583_set_pm_runtime_busy(chip, false);
>  		return ret;
> +	}
>  
>  	ret = tsl2583_set_pm_runtime_busy(chip, false);
>  	if (ret < 0)
> @@ -872,7 +874,6 @@ static int tsl2583_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	return tsl2583_set_power_state(chip, TSL2583_CNTL_PWR_OFF);
>  }



Thanks,
Mauro

[PATCH 04/28] iio: accel: bmc150-accel: Balanced runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

A call to pm_runtime_put_noidle() doesn't match any call that would
result in a get().  It is safe because runtime pm core protects against
the reference counter going 0, but it makes it harder to understand the
code.

Whilst here use pm_runtime_resume_and_get() to tidy things up.
The Coccinelle script didn't get this one due to more complex code
structure.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 62a164a7b852..43cfadf8f6b7 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -389,7 +389,7 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
 	int ret;
 
 	if (on) {
-		ret = pm_runtime_get_sync(dev);
+		ret = pm_runtime_resume_and_get(dev);
 	} else {
 		pm_runtime_mark_last_busy(dev);
 		ret = pm_runtime_put_autosuspend(dev);
@@ -398,9 +398,6 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
 	if (ret < 0) {
 		dev_err(dev,
 			"Failed: %s for %d\n", __func__, on);
-		if (on)
-			pm_runtime_put_noidle(dev);
-
 		return ret;
 	}
 
@@ -1836,7 +1833,6 @@ int bmc150_accel_core_remove(struct device *dev)
 
 	pm_runtime_disable(dev);
 	pm_runtime_set_suspended(dev);
-	pm_runtime_put_noidle(dev);
 
 	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
 
-- 
2.31.1

Re: [PATCH 04/28] iio: accel: bmc150-accel: Balanced runtime pm + use pm_runtime_resume_and_get()

[Hans de Goede]

Hi,

On 5/9/21 1:33 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> A call to pm_runtime_put_noidle() doesn't match any call that would
> result in a get().  It is safe because runtime pm core protects against
> the reference counter going 0, but it makes it harder to understand the
> code.
> 
> Whilst here use pm_runtime_resume_and_get() to tidy things up.
> The Coccinelle script didn't get this one due to more complex code
> structure.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/iio/accel/bmc150-accel-core.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 62a164a7b852..43cfadf8f6b7 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -389,7 +389,7 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
>  	int ret;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
>  		ret = pm_runtime_put_autosuspend(dev);
> @@ -398,9 +398,6 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
>  	if (ret < 0) {
>  		dev_err(dev,
>  			"Failed: %s for %d\n", __func__, on);
> -		if (on)
> -			pm_runtime_put_noidle(dev);
> -
>  		return ret;
>  	}
>  
> @@ -1836,7 +1833,6 @@ int bmc150_accel_core_remove(struct device *dev)
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
>  
>  	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
>  
> 

Re: [PATCH 04/28] iio: accel: bmc150-accel: Balanced runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Sun, 9 May 2021 15:52:44 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 5/9/21 1:33 PM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > A call to pm_runtime_put_noidle() doesn't match any call that would
> > result in a get().  It is safe because runtime pm core protects against
> > the reference counter going 0, but it makes it harder to understand the
> > code.
> > 
> > Whilst here use pm_runtime_resume_and_get() to tidy things up.
> > The Coccinelle script didn't get this one due to more complex code
> > structure.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>  
> 
> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Applied to the togreg branch of iio.git, pushed out as testing for all the
normal reasons.

Thanks,

Jonathan

> 
> Regards,
> 
> Hans
> 
> 
> > ---
> >  drivers/iio/accel/bmc150-accel-core.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> > index 62a164a7b852..43cfadf8f6b7 100644
> > --- a/drivers/iio/accel/bmc150-accel-core.c
> > +++ b/drivers/iio/accel/bmc150-accel-core.c
> > @@ -389,7 +389,7 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
> >  	int ret;
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(dev);
> >  		ret = pm_runtime_put_autosuspend(dev);
> > @@ -398,9 +398,6 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
> >  	if (ret < 0) {
> >  		dev_err(dev,
> >  			"Failed: %s for %d\n", __func__, on);
> > -		if (on)
> > -			pm_runtime_put_noidle(dev);
> > -
> >  		return ret;
> >  	}
> >  
> > @@ -1836,7 +1833,6 @@ int bmc150_accel_core_remove(struct device *dev)
> >  
> >  	pm_runtime_disable(dev);
> >  	pm_runtime_set_suspended(dev);
> > -	pm_runtime_put_noidle(dev);
> >  
> >  	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
> >  
> >   
> 

Re: [PATCH 04/28] iio: accel: bmc150-accel: Balanced runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:30 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> A call to pm_runtime_put_noidle() doesn't match any call that would
> result in a get().  It is safe because runtime pm core protects against
> the reference counter going 0, but it makes it harder to understand the
> code.
> 
> Whilst here use pm_runtime_resume_and_get() to tidy things up.
> The Coccinelle script didn't get this one due to more complex code
> structure.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>


> ---
>  drivers/iio/accel/bmc150-accel-core.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 62a164a7b852..43cfadf8f6b7 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -389,7 +389,7 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
>  	int ret;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
>  		ret = pm_runtime_put_autosuspend(dev);
> @@ -398,9 +398,6 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
>  	if (ret < 0) {
>  		dev_err(dev,
>  			"Failed: %s for %d\n", __func__, on);
> -		if (on)
> -			pm_runtime_put_noidle(dev);
> -
>  		return ret;
>  	}
>  
> @@ -1836,7 +1833,6 @@ int bmc150_accel_core_remove(struct device *dev)
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
>  
>  	bmc150_accel_unregister_triggers(data, BMC150_ACCEL_TRIGGERS - 1);
>  



Thanks,
Mauro

[PATCH 05/28] iio: accel: mma8452: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Remove() callback calls pm_runtime_put_noidle() but there it is not
balancing a get.  No actual affect because the runtime pm core prevents
the reference count going negative.

Whilst here use pm_runtime_resume_and_get() rather than open coded version.
Again, coccinelle script missed this one due to more complex code
structure.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/accel/mma8452.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 464a6bfe6746..715b8138fb71 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -221,7 +221,7 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
 	int ret;
 
 	if (on) {
-		ret = pm_runtime_get_sync(&client->dev);
+		ret = pm_runtime_resume_and_get(&client->dev);
 	} else {
 		pm_runtime_mark_last_busy(&client->dev);
 		ret = pm_runtime_put_autosuspend(&client->dev);
@@ -230,8 +230,6 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
 	if (ret < 0) {
 		dev_err(&client->dev,
 			"failed to change power state to %d\n", on);
-		if (on)
-			pm_runtime_put_noidle(&client->dev);
 
 		return ret;
 	}
@@ -1711,7 +1709,6 @@ static int mma8452_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	iio_triggered_buffer_cleanup(indio_dev);
 	mma8452_trigger_cleanup(indio_dev);
-- 
2.31.1

Re: [PATCH 05/28] iio: accel: mma8452: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:31 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Remove() callback calls pm_runtime_put_noidle() but there it is not
> balancing a get.  No actual affect because the runtime pm core prevents
> the reference count going negative.
> 
> Whilst here use pm_runtime_resume_and_get() rather than open coded version.
> Again, coccinelle script missed this one due to more complex code
> structure.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Sean Nyekjaer <sean@geanix.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/accel/mma8452.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 464a6bfe6746..715b8138fb71 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -221,7 +221,7 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
>  	int ret;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(&client->dev);
> +		ret = pm_runtime_resume_and_get(&client->dev);
>  	} else {
>  		pm_runtime_mark_last_busy(&client->dev);
>  		ret = pm_runtime_put_autosuspend(&client->dev);
> @@ -230,8 +230,6 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
>  	if (ret < 0) {
>  		dev_err(&client->dev,
>  			"failed to change power state to %d\n", on);
> -		if (on)
> -			pm_runtime_put_noidle(&client->dev);
>  
>  		return ret;
>  	}
> @@ -1711,7 +1709,6 @@ static int mma8452_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	mma8452_trigger_cleanup(indio_dev);



Thanks,
Mauro

Re: [PATCH 05/28] iio: accel: mma8452: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 15:37:10 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:31 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Remove() callback calls pm_runtime_put_noidle() but there it is not
> > balancing a get.  No actual affect because the runtime pm core prevents
> > the reference count going negative.
> > 
> > Whilst here use pm_runtime_resume_and_get() rather than open coded version.
> > Again, coccinelle script missed this one due to more complex code
> > structure.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Sean Nyekjaer <sean@geanix.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it.

Note additional reviews / tags are find for now as I'm happy to rebase this
tree as needed until it goes out as togreg (when I try not to!)

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/accel/mma8452.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> > index 464a6bfe6746..715b8138fb71 100644
> > --- a/drivers/iio/accel/mma8452.c
> > +++ b/drivers/iio/accel/mma8452.c
> > @@ -221,7 +221,7 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
> >  	int ret;
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(&client->dev);
> > +		ret = pm_runtime_resume_and_get(&client->dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(&client->dev);
> >  		ret = pm_runtime_put_autosuspend(&client->dev);
> > @@ -230,8 +230,6 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
> >  	if (ret < 0) {
> >  		dev_err(&client->dev,
> >  			"failed to change power state to %d\n", on);
> > -		if (on)
> > -			pm_runtime_put_noidle(&client->dev);
> >  
> >  		return ret;
> >  	}
> > @@ -1711,7 +1709,6 @@ static int mma8452_remove(struct i2c_client *client)
> >  
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  
> >  	iio_triggered_buffer_cleanup(indio_dev);
> >  	mma8452_trigger_cleanup(indio_dev);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 06/28] iio: accel: mma9551/3: Balance untime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Both these drivers call pm_runtime_put_no_idle() when the reference
count should already be zero as there is no matching get()

Whilst here use pm_runtime_resume_and_get() rather than open coding.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/mma9551.c      | 1 -
 drivers/iio/accel/mma9551_core.c | 4 +---
 drivers/iio/accel/mma9553.c      | 1 -
 3 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
index 08a2303cc9df..4c359fb05480 100644
--- a/drivers/iio/accel/mma9551.c
+++ b/drivers/iio/accel/mma9551.c
@@ -515,7 +515,6 @@ static int mma9551_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	mutex_lock(&data->mutex);
 	mma9551_set_device_state(data->client, false);
diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
index 666e7a04a7d7..fbf2e2c45678 100644
--- a/drivers/iio/accel/mma9551_core.c
+++ b/drivers/iio/accel/mma9551_core.c
@@ -664,7 +664,7 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)
 	int ret;
 
 	if (on)
-		ret = pm_runtime_get_sync(&client->dev);
+		ret = pm_runtime_resume_and_get(&client->dev);
 	else {
 		pm_runtime_mark_last_busy(&client->dev);
 		ret = pm_runtime_put_autosuspend(&client->dev);
@@ -673,8 +673,6 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)
 	if (ret < 0) {
 		dev_err(&client->dev,
 			"failed to change power state to %d\n", on);
-		if (on)
-			pm_runtime_put_noidle(&client->dev);
 
 		return ret;
 	}
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index c15908faa381..ba3ecb3b57dc 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -1154,7 +1154,6 @@ static int mma9553_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	mutex_lock(&data->mutex);
 	mma9551_set_device_state(data->client, false);
-- 
2.31.1

Re: [PATCH 06/28] iio: accel: mma9551/3: Balance untime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:32 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Both these drivers call pm_runtime_put_no_idle() when the reference
> count should already be zero as there is no matching get()
> 
> Whilst here use pm_runtime_resume_and_get() rather than open coding.

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/accel/mma9551.c      | 1 -
>  drivers/iio/accel/mma9551_core.c | 4 +---
>  drivers/iio/accel/mma9553.c      | 1 -
>  3 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 08a2303cc9df..4c359fb05480 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -515,7 +515,6 @@ static int mma9551_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	mutex_lock(&data->mutex);
>  	mma9551_set_device_state(data->client, false);
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 666e7a04a7d7..fbf2e2c45678 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -664,7 +664,7 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)
>  	int ret;
>  
>  	if (on)
> -		ret = pm_runtime_get_sync(&client->dev);
> +		ret = pm_runtime_resume_and_get(&client->dev);
>  	else {
>  		pm_runtime_mark_last_busy(&client->dev);
>  		ret = pm_runtime_put_autosuspend(&client->dev);
> @@ -673,8 +673,6 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)
>  	if (ret < 0) {
>  		dev_err(&client->dev,
>  			"failed to change power state to %d\n", on);
> -		if (on)
> -			pm_runtime_put_noidle(&client->dev);
>  
>  		return ret;
>  	}
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index c15908faa381..ba3ecb3b57dc 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -1154,7 +1154,6 @@ static int mma9553_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	mutex_lock(&data->mutex);
>  	mma9551_set_device_state(data->client, false);



Thanks,
Mauro

Re: [PATCH 06/28] iio: accel: mma9551/3: Balance untime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 15:37:57 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:32 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Both these drivers call pm_runtime_put_no_idle() when the reference
> > count should already be zero as there is no matching get()
> > 
> > Whilst here use pm_runtime_resume_and_get() rather than open coding.  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Applied. Thanks,
> > ---
> >  drivers/iio/accel/mma9551.c      | 1 -
> >  drivers/iio/accel/mma9551_core.c | 4 +---
> >  drivers/iio/accel/mma9553.c      | 1 -
> >  3 files changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> > index 08a2303cc9df..4c359fb05480 100644
> > --- a/drivers/iio/accel/mma9551.c
> > +++ b/drivers/iio/accel/mma9551.c
> > @@ -515,7 +515,6 @@ static int mma9551_remove(struct i2c_client *client)
> >  
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  
> >  	mutex_lock(&data->mutex);
> >  	mma9551_set_device_state(data->client, false);
> > diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> > index 666e7a04a7d7..fbf2e2c45678 100644
> > --- a/drivers/iio/accel/mma9551_core.c
> > +++ b/drivers/iio/accel/mma9551_core.c
> > @@ -664,7 +664,7 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)
> >  	int ret;
> >  
> >  	if (on)
> > -		ret = pm_runtime_get_sync(&client->dev);
> > +		ret = pm_runtime_resume_and_get(&client->dev);
> >  	else {
> >  		pm_runtime_mark_last_busy(&client->dev);
> >  		ret = pm_runtime_put_autosuspend(&client->dev);
> > @@ -673,8 +673,6 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)
> >  	if (ret < 0) {
> >  		dev_err(&client->dev,
> >  			"failed to change power state to %d\n", on);
> > -		if (on)
> > -			pm_runtime_put_noidle(&client->dev);
> >  
> >  		return ret;
> >  	}
> > diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> > index c15908faa381..ba3ecb3b57dc 100644
> > --- a/drivers/iio/accel/mma9553.c
> > +++ b/drivers/iio/accel/mma9553.c
> > @@ -1154,7 +1154,6 @@ static int mma9553_remove(struct i2c_client *client)
> >  
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  
> >  	mutex_lock(&data->mutex);
> >  	mma9551_set_device_state(data->client, false);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 07/28] iio: accel: kxcjk-1013: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This driver alls pm_runtime_put_noidle() in it's remove function, but there
is no matching get call.  This isn't a bug as runtime pm will not allow
the reference counter to go negative, but it is missleading so lets remove
it.

Whilst here use pm_runtime_resume_and_get() to tidy up some boilerplate.
The coccicheck script didn't get this one due to the less obvious
structure. Found by inspection.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/kxcjk-1013.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 283e6a3feffc..a9e3dae43ac5 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -478,7 +478,7 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
 	int ret;
 
 	if (on)
-		ret = pm_runtime_get_sync(&data->client->dev);
+		ret = pm_runtime_resume_and_get(&data->client->dev);
 	else {
 		pm_runtime_mark_last_busy(&data->client->dev);
 		ret = pm_runtime_put_autosuspend(&data->client->dev);
@@ -486,8 +486,6 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
 	if (ret < 0) {
 		dev_err(&data->client->dev,
 			"Failed: %s for %d\n", __func__, on);
-		if (on)
-			pm_runtime_put_noidle(&data->client->dev);
 		return ret;
 	}
 #endif
@@ -1485,7 +1483,6 @@ static int kxcjk1013_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	if (data->dready_trig) {
 		iio_triggered_buffer_cleanup(indio_dev);
-- 
2.31.1

Re: [PATCH 07/28] iio: accel: kxcjk-1013: Balance runtime pm + use pm_runtime_resume_and_get()

[Hans de Goede]

Hi,

On 5/9/21 1:33 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This driver alls pm_runtime_put_noidle() in it's remove function, but there
> is no matching get call.  This isn't a bug as runtime pm will not allow
> the reference counter to go negative, but it is missleading so lets remove
> it.
> 
> Whilst here use pm_runtime_resume_and_get() to tidy up some boilerplate.
> The coccicheck script didn't get this one due to the less obvious
> structure. Found by inspection.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/iio/accel/kxcjk-1013.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 283e6a3feffc..a9e3dae43ac5 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -478,7 +478,7 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>  	int ret;
>  
>  	if (on)
> -		ret = pm_runtime_get_sync(&data->client->dev);
> +		ret = pm_runtime_resume_and_get(&data->client->dev);
>  	else {
>  		pm_runtime_mark_last_busy(&data->client->dev);
>  		ret = pm_runtime_put_autosuspend(&data->client->dev);
> @@ -486,8 +486,6 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>  	if (ret < 0) {
>  		dev_err(&data->client->dev,
>  			"Failed: %s for %d\n", __func__, on);
> -		if (on)
> -			pm_runtime_put_noidle(&data->client->dev);
>  		return ret;
>  	}
>  #endif
> @@ -1485,7 +1483,6 @@ static int kxcjk1013_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	if (data->dready_trig) {
>  		iio_triggered_buffer_cleanup(indio_dev);
> 

Re: [PATCH 07/28] iio: accel: kxcjk-1013: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:33 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This driver alls pm_runtime_put_noidle() in it's remove function, but there
> is no matching get call.  This isn't a bug as runtime pm will not allow
> the reference counter to go negative, but it is missleading so lets remove
> it.
> 
> Whilst here use pm_runtime_resume_and_get() to tidy up some boilerplate.
> The coccicheck script didn't get this one due to the less obvious
> structure. Found by inspection.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/accel/kxcjk-1013.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 283e6a3feffc..a9e3dae43ac5 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -478,7 +478,7 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>  	int ret;
>  
>  	if (on)
> -		ret = pm_runtime_get_sync(&data->client->dev);
> +		ret = pm_runtime_resume_and_get(&data->client->dev);
>  	else {
>  		pm_runtime_mark_last_busy(&data->client->dev);
>  		ret = pm_runtime_put_autosuspend(&data->client->dev);
> @@ -486,8 +486,6 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>  	if (ret < 0) {
>  		dev_err(&data->client->dev,
>  			"Failed: %s for %d\n", __func__, on);
> -		if (on)
> -			pm_runtime_put_noidle(&data->client->dev);
>  		return ret;
>  	}
>  #endif
> @@ -1485,7 +1483,6 @@ static int kxcjk1013_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	if (data->dready_trig) {
>  		iio_triggered_buffer_cleanup(indio_dev);



Thanks,
Mauro

[PATCH 08/28] iio: hid: trigger: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The call to pm_runtime_put_noidle() in remove() callback is not
balanced by any gets

Note this doesn't cause any problems beyond reader confusion as the runtime
pm core protects against the reference counter going negative.

Whilst here, use pm_runtiem_resume_and_get() to simplify code a little.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 5a7b3e253e58..c06537e106e9 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -163,18 +163,15 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
 
 	if (state) {
 		atomic_inc(&st->user_requested_state);
-		ret = pm_runtime_get_sync(&st->pdev->dev);
+		ret = pm_runtime_resume_and_get(&st->pdev->dev);
 	} else {
 		atomic_dec(&st->user_requested_state);
 		pm_runtime_mark_last_busy(&st->pdev->dev);
 		pm_runtime_use_autosuspend(&st->pdev->dev);
 		ret = pm_runtime_put_autosuspend(&st->pdev->dev);
 	}
-	if (ret < 0) {
-		if (state)
-			pm_runtime_put_noidle(&st->pdev->dev);
+	if (ret < 0)
 		return ret;
-	}
 
 	return 0;
 #else
@@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
 		pm_runtime_disable(&attrb->pdev->dev);
 
 	pm_runtime_set_suspended(&attrb->pdev->dev);
-	pm_runtime_put_noidle(&attrb->pdev->dev);
 
 	cancel_work_sync(&attrb->work);
 	iio_trigger_unregister(attrb->trigger);
-- 
2.31.1

Re: [PATCH 08/28] iio: hid: trigger: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:34 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> The call to pm_runtime_put_noidle() in remove() callback is not
> balanced by any gets
> 
> Note this doesn't cause any problems beyond reader confusion as the runtime
> pm core protects against the reference counter going negative.
> 
> Whilst here, use pm_runtiem_resume_and_get() to simplify code a little.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

RPM get/put logic LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 5a7b3e253e58..c06537e106e9 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
>  
>  	if (state) {
>  		atomic_inc(&st->user_requested_state);
> -		ret = pm_runtime_get_sync(&st->pdev->dev);
> +		ret = pm_runtime_resume_and_get(&st->pdev->dev);
>  	} else {
>  		atomic_dec(&st->user_requested_state);
>  		pm_runtime_mark_last_busy(&st->pdev->dev);
>  		pm_runtime_use_autosuspend(&st->pdev->dev);
>  		ret = pm_runtime_put_autosuspend(&st->pdev->dev);
>  	}
> -	if (ret < 0) {
> -		if (state)
> -			pm_runtime_put_noidle(&st->pdev->dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	return 0;
>  #else
> @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
>  		pm_runtime_disable(&attrb->pdev->dev);
>  
>  	pm_runtime_set_suspended(&attrb->pdev->dev);
> -	pm_runtime_put_noidle(&attrb->pdev->dev);
>  
>  	cancel_work_sync(&attrb->work);
>  	iio_trigger_unregister(attrb->trigger);



Thanks,
Mauro

Re: [PATCH 08/28] iio: hid: trigger: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 15:44:30 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:34 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > The call to pm_runtime_put_noidle() in remove() callback is not
> > balanced by any gets
> > 
> > Note this doesn't cause any problems beyond reader confusion as the runtime
> > pm core protects against the reference counter going negative.
> > 
> > Whilst here, use pm_runtiem_resume_and_get() to simplify code a little.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>  
> 
> RPM get/put logic LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

After our side discussion on this one I wanted to take another look.
I'm not sure I follow the logic behind the runtime_enable() path
and why we don't want to enable runtime_pm until first use.  However
it's not directly related to this patch and as far as I can tell
it's just unsuual rather than broken.  As such, applied this one to the
togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > index 5a7b3e253e58..c06537e106e9 100644
> > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
> >  
> >  	if (state) {
> >  		atomic_inc(&st->user_requested_state);
> > -		ret = pm_runtime_get_sync(&st->pdev->dev);
> > +		ret = pm_runtime_resume_and_get(&st->pdev->dev);
> >  	} else {
> >  		atomic_dec(&st->user_requested_state);
> >  		pm_runtime_mark_last_busy(&st->pdev->dev);
> >  		pm_runtime_use_autosuspend(&st->pdev->dev);
> >  		ret = pm_runtime_put_autosuspend(&st->pdev->dev);
> >  	}
> > -	if (ret < 0) {
> > -		if (state)
> > -			pm_runtime_put_noidle(&st->pdev->dev);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> >  
> >  	return 0;
> >  #else
> > @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
> >  		pm_runtime_disable(&attrb->pdev->dev);
> >  
> >  	pm_runtime_set_suspended(&attrb->pdev->dev);
> > -	pm_runtime_put_noidle(&attrb->pdev->dev);
> >  
> >  	cancel_work_sync(&attrb->work);
> >  	iio_trigger_unregister(attrb->trigger);  
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH 08/28] iio: hid: trigger: Balance runtime pm + use pm_runtime_resume_and_get()

[Srinivas Pandruvada]

On Sun, 2021-05-16 at 16:19 +0100, Jonathan Cameron wrote:
> On Wed, 12 May 2021 15:44:30 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Em Sun,  9 May 2021 12:33:34 +0100
> > Jonathan Cameron <jic23@kernel.org> escreveu:
> > 
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > The call to pm_runtime_put_noidle() in remove() callback is not
> > > balanced by any gets
> > > 
> > > Note this doesn't cause any problems beyond reader confusion as
> > > the runtime
> > > pm core protects against the reference counter going negative.
> > > 
> > > Whilst here, use pm_runtiem_resume_and_get() to simplify code a
> > > little.
> > > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>  
> > 
> > RPM get/put logic LGTM.
> > 
> > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> After our side discussion on this one I wanted to take another look.
> I'm not sure I follow the logic behind the runtime_enable() path
> and why we don't want to enable runtime_pm until first use.  However
> it's not directly related to this patch and as far as I can tell
> it's just unsuual rather than broken.  As such, applied this one to
> the
> togreg branch of iio.git and pushed out as testing.
> 

This was enabled in init path but caused issue with USB sensor hub in
Thinkpasd Yoga S1, where we can never power off sensor and it wakes up
device immediately after suspend.

commit
ad7532cefd11d11a0814a75fb814c205ee3d9d4c


Thanks,
Srinivas

> Thanks,
> 
> Jonathan
> 
> > 
> > > ---
> > >  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------
> > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > index 5a7b3e253e58..c06537e106e9 100644
> > > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct
> > > hid_sensor_common *st, bool state)
> > >  
> > >         if (state) {
> > >                 atomic_inc(&st->user_requested_state);
> > > -               ret = pm_runtime_get_sync(&st->pdev->dev);
> > > +               ret = pm_runtime_resume_and_get(&st->pdev->dev);
> > >         } else {
> > >                 atomic_dec(&st->user_requested_state);
> > >                 pm_runtime_mark_last_busy(&st->pdev->dev);
> > >                 pm_runtime_use_autosuspend(&st->pdev->dev);
> > >                 ret = pm_runtime_put_autosuspend(&st->pdev->dev);
> > >         }
> > > -       if (ret < 0) {
> > > -               if (state)
> > > -                       pm_runtime_put_noidle(&st->pdev->dev);
> > > +       if (ret < 0)
> > >                 return ret;
> > > -       }
> > >  
> > >         return 0;
> > >  #else
> > > @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev
> > > *indio_dev,
> > >                 pm_runtime_disable(&attrb->pdev->dev);
> > >  
> > >         pm_runtime_set_suspended(&attrb->pdev->dev);
> > > -       pm_runtime_put_noidle(&attrb->pdev->dev);
> > >  
> > >         cancel_work_sync(&attrb->work);
> > >         iio_trigger_unregister(attrb->trigger);  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> 

Re: [PATCH 08/28] iio: hid: trigger: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Sun, 16 May 2021 19:19:40 -0700
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> On Sun, 2021-05-16 at 16:19 +0100, Jonathan Cameron wrote:
> > On Wed, 12 May 2021 15:44:30 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >   
> > > Em Sun,  9 May 2021 12:33:34 +0100
> > > Jonathan Cameron <jic23@kernel.org> escreveu:
> > >   
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > The call to pm_runtime_put_noidle() in remove() callback is not
> > > > balanced by any gets
> > > > 
> > > > Note this doesn't cause any problems beyond reader confusion as
> > > > the runtime
> > > > pm core protects against the reference counter going negative.
> > > > 
> > > > Whilst here, use pm_runtiem_resume_and_get() to simplify code a
> > > > little.
> > > > 
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>    
> > > 
> > > RPM get/put logic LGTM.
> > > 
> > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> > 
> > After our side discussion on this one I wanted to take another look.
> > I'm not sure I follow the logic behind the runtime_enable() path
> > and why we don't want to enable runtime_pm until first use.  However
> > it's not directly related to this patch and as far as I can tell
> > it's just unsuual rather than broken.  As such, applied this one to
> > the
> > togreg branch of iio.git and pushed out as testing.
> >   
> 
> This was enabled in init path but caused issue with USB sensor hub in
> Thinkpasd Yoga S1, where we can never power off sensor and it wakes up
> device immediately after suspend.
> 
> commit
> ad7532cefd11d11a0814a75fb814c205ee3d9d4c

Ah.  Thanks for the history.  Thanks makes a horrible kind of sense ;)

Jonathan

> 
> 
> Thanks,
> Srinivas
> 
> > Thanks,
> > 
> > Jonathan
> >   
> > >   
> > > > ---
> > > >  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------
> > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > index 5a7b3e253e58..c06537e106e9 100644
> > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct
> > > > hid_sensor_common *st, bool state)
> > > >  
> > > >         if (state) {
> > > >                 atomic_inc(&st->user_requested_state);
> > > > -               ret = pm_runtime_get_sync(&st->pdev->dev);
> > > > +               ret = pm_runtime_resume_and_get(&st->pdev->dev);
> > > >         } else {
> > > >                 atomic_dec(&st->user_requested_state);
> > > >                 pm_runtime_mark_last_busy(&st->pdev->dev);
> > > >                 pm_runtime_use_autosuspend(&st->pdev->dev);
> > > >                 ret = pm_runtime_put_autosuspend(&st->pdev->dev);
> > > >         }
> > > > -       if (ret < 0) {
> > > > -               if (state)
> > > > -                       pm_runtime_put_noidle(&st->pdev->dev);
> > > > +       if (ret < 0)
> > > >                 return ret;
> > > > -       }
> > > >  
> > > >         return 0;
> > > >  #else
> > > > @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev
> > > > *indio_dev,
> > > >                 pm_runtime_disable(&attrb->pdev->dev);
> > > >  
> > > >         pm_runtime_set_suspended(&attrb->pdev->dev);
> > > > -       pm_runtime_put_noidle(&attrb->pdev->dev);
> > > >  
> > > >         cancel_work_sync(&attrb->work);
> > > >         iio_trigger_unregister(attrb->trigger);    
> > > 
> > > 
> > > 
> > > Thanks,
> > > Mauro  
> >   
> 
> 

[PATCH 09/28] iio: imu: kmx61: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

No point in calling pm_runtime_put_noidle() that isn't balancing a get.
Note no actual impact because the runtime pm core protects against
a negative reference counter.

For the pm_runtime_resume_and_get() main interest is in clearing
out this old pattern to avoid it getting coppied into new submissions.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/imu/kmx61.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
index d3e06ce99c1e..1dabfd615dab 100644
--- a/drivers/iio/imu/kmx61.c
+++ b/drivers/iio/imu/kmx61.c
@@ -750,7 +750,7 @@ static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
 	}
 
 	if (on) {
-		ret = pm_runtime_get_sync(&data->client->dev);
+		ret = pm_runtime_resume_and_get(&data->client->dev);
 	} else {
 		pm_runtime_mark_last_busy(&data->client->dev);
 		ret = pm_runtime_put_autosuspend(&data->client->dev);
@@ -759,8 +759,6 @@ static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
 		dev_err(&data->client->dev,
 			"Failed: kmx61_set_power_state for %d, ret %d\n",
 			on, ret);
-		if (on)
-			pm_runtime_put_noidle(&data->client->dev);
 
 		return ret;
 	}
@@ -1426,7 +1424,6 @@ static int kmx61_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	if (client->irq > 0) {
 		iio_triggered_buffer_cleanup(data->acc_indio_dev);
-- 
2.31.1

Re: [PATCH 09/28] iio: imu: kmx61: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:35 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> No point in calling pm_runtime_put_noidle() that isn't balancing a get.
> Note no actual impact because the runtime pm core protects against
> a negative reference counter.
> 
> For the pm_runtime_resume_and_get() main interest is in clearing
> out this old pattern to avoid it getting coppied into new submissions.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/imu/kmx61.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> index d3e06ce99c1e..1dabfd615dab 100644
> --- a/drivers/iio/imu/kmx61.c
> +++ b/drivers/iio/imu/kmx61.c
> @@ -750,7 +750,7 @@ static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
>  	}
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(&data->client->dev);
> +		ret = pm_runtime_resume_and_get(&data->client->dev);
>  	} else {
>  		pm_runtime_mark_last_busy(&data->client->dev);
>  		ret = pm_runtime_put_autosuspend(&data->client->dev);
> @@ -759,8 +759,6 @@ static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
>  		dev_err(&data->client->dev,
>  			"Failed: kmx61_set_power_state for %d, ret %d\n",
>  			on, ret);
> -		if (on)
> -			pm_runtime_put_noidle(&data->client->dev);
>  
>  		return ret;
>  	}
> @@ -1426,7 +1424,6 @@ static int kmx61_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	if (client->irq > 0) {
>  		iio_triggered_buffer_cleanup(data->acc_indio_dev);



Thanks,
Mauro

Re: [PATCH 09/28] iio: imu: kmx61: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 15:45:11 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:35 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > No point in calling pm_runtime_put_noidle() that isn't balancing a get.
> > Note no actual impact because the runtime pm core protects against
> > a negative reference counter.
> > 
> > For the pm_runtime_resume_and_get() main interest is in clearing
> > out this old pattern to avoid it getting coppied into new submissions.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied. Thanks, J
> 
> > ---
> >  drivers/iio/imu/kmx61.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> > index d3e06ce99c1e..1dabfd615dab 100644
> > --- a/drivers/iio/imu/kmx61.c
> > +++ b/drivers/iio/imu/kmx61.c
> > @@ -750,7 +750,7 @@ static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
> >  	}
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(&data->client->dev);
> > +		ret = pm_runtime_resume_and_get(&data->client->dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(&data->client->dev);
> >  		ret = pm_runtime_put_autosuspend(&data->client->dev);
> > @@ -759,8 +759,6 @@ static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
> >  		dev_err(&data->client->dev,
> >  			"Failed: kmx61_set_power_state for %d, ret %d\n",
> >  			on, ret);
> > -		if (on)
> > -			pm_runtime_put_noidle(&data->client->dev);
> >  
> >  		return ret;
> >  	}
> > @@ -1426,7 +1424,6 @@ static int kmx61_remove(struct i2c_client *client)
> >  
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  
> >  	if (client->irq > 0) {
> >  		iio_triggered_buffer_cleanup(data->acc_indio_dev);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 10/28] iio: light: rpr0521: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Calls to pm_runtime_put_noidle in probe() error path and remove() are
not match to any get() calls.

The runtime pm core protects against negative reference counts, so this
doesn't have any visible impact beyond confusing the reader.

Whilst here use pm_runtime_resume_and_get() to replace boilerplate.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 7e332de0e6a5..c2dd8a3d4217 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -360,7 +360,7 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
 	 * both stay enabled until _suspend().
 	 */
 	if (on) {
-		ret = pm_runtime_get_sync(&data->client->dev);
+		ret = pm_runtime_resume_and_get(&data->client->dev);
 	} else {
 		pm_runtime_mark_last_busy(&data->client->dev);
 		ret = pm_runtime_put_autosuspend(&data->client->dev);
@@ -369,9 +369,6 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
 		dev_err(&data->client->dev,
 			"Failed: rpr0521_set_power_state for %d, ret %d\n",
 			on, ret);
-		if (on)
-			pm_runtime_put_noidle(&data->client->dev);
-
 		return ret;
 	}
 
@@ -1038,7 +1035,6 @@ static int rpr0521_probe(struct i2c_client *client,
 err_pm_disable:
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 err_poweroff:
 	rpr0521_poweroff(data);
 
@@ -1053,7 +1049,6 @@ static int rpr0521_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-	pm_runtime_put_noidle(&client->dev);
 
 	rpr0521_poweroff(iio_priv(indio_dev));
 
-- 
2.31.1

Re: [PATCH 10/28] iio: light: rpr0521: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:36 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Calls to pm_runtime_put_noidle in probe() error path and remove() are
> not match to any get() calls.
> 
> The runtime pm core protects against negative reference counts, so this
> doesn't have any visible impact beyond confusing the reader.
> 
> Whilst here use pm_runtime_resume_and_get() to replace boilerplate.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/light/rpr0521.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 7e332de0e6a5..c2dd8a3d4217 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -360,7 +360,7 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>  	 * both stay enabled until _suspend().
>  	 */
>  	if (on) {
> -		ret = pm_runtime_get_sync(&data->client->dev);
> +		ret = pm_runtime_resume_and_get(&data->client->dev);
>  	} else {
>  		pm_runtime_mark_last_busy(&data->client->dev);
>  		ret = pm_runtime_put_autosuspend(&data->client->dev);
> @@ -369,9 +369,6 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>  		dev_err(&data->client->dev,
>  			"Failed: rpr0521_set_power_state for %d, ret %d\n",
>  			on, ret);
> -		if (on)
> -			pm_runtime_put_noidle(&data->client->dev);
> -
>  		return ret;
>  	}
>  
> @@ -1038,7 +1035,6 @@ static int rpr0521_probe(struct i2c_client *client,
>  err_pm_disable:
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  err_poweroff:
>  	rpr0521_poweroff(data);
>  
> @@ -1053,7 +1049,6 @@ static int rpr0521_remove(struct i2c_client *client)
>  
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> -	pm_runtime_put_noidle(&client->dev);
>  
>  	rpr0521_poweroff(iio_priv(indio_dev));
>  



Thanks,
Mauro

Re: [PATCH 10/28] iio: light: rpr0521: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 15:46:32 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:36 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Calls to pm_runtime_put_noidle in probe() error path and remove() are
> > not match to any get() calls.
> > 
> > The runtime pm core protects against negative reference counts, so this
> > doesn't have any visible impact beyond confusing the reader.
> > 
> > Whilst here use pm_runtime_resume_and_get() to replace boilerplate.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to take a swing at it before I push it in something linux-next
will pick up.   Note still time for additional reviews as I'm happy to rebase
the tree for now.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/light/rpr0521.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> > index 7e332de0e6a5..c2dd8a3d4217 100644
> > --- a/drivers/iio/light/rpr0521.c
> > +++ b/drivers/iio/light/rpr0521.c
> > @@ -360,7 +360,7 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
> >  	 * both stay enabled until _suspend().
> >  	 */
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(&data->client->dev);
> > +		ret = pm_runtime_resume_and_get(&data->client->dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(&data->client->dev);
> >  		ret = pm_runtime_put_autosuspend(&data->client->dev);
> > @@ -369,9 +369,6 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
> >  		dev_err(&data->client->dev,
> >  			"Failed: rpr0521_set_power_state for %d, ret %d\n",
> >  			on, ret);
> > -		if (on)
> > -			pm_runtime_put_noidle(&data->client->dev);
> > -
> >  		return ret;
> >  	}
> >  
> > @@ -1038,7 +1035,6 @@ static int rpr0521_probe(struct i2c_client *client,
> >  err_pm_disable:
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  err_poweroff:
> >  	rpr0521_poweroff(data);
> >  
> > @@ -1053,7 +1049,6 @@ static int rpr0521_remove(struct i2c_client *client)
> >  
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> > -	pm_runtime_put_noidle(&client->dev);
> >  
> >  	rpr0521_poweroff(iio_priv(indio_dev));
> >    
> 
> 
> 
> Thanks,
> Mauro

[PATCH 11/28] iio: magn: bmc150: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

probe() error paths after runtime pm is enabled, should disable it.
remove() should not call pm_runtime_put_noidle() as there is no
matching get() to have raised the reference count.  This case
has no affect a the runtime pm core protects against going negative.

Whilst here use pm_runtime_resume_and_get() to tidy things up a little.
coccicheck script didn't get this one due to complex code structure so
found by inspection.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/magnetometer/bmc150_magn.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index d75b437a43f2..39920cb764bf 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -262,7 +262,7 @@ static int bmc150_magn_set_power_state(struct bmc150_magn_data *data, bool on)
 	int ret;
 
 	if (on) {
-		ret = pm_runtime_get_sync(data->dev);
+		ret = pm_runtime_resume_and_get(data->dev);
 	} else {
 		pm_runtime_mark_last_busy(data->dev);
 		ret = pm_runtime_put_autosuspend(data->dev);
@@ -271,9 +271,6 @@ static int bmc150_magn_set_power_state(struct bmc150_magn_data *data, bool on)
 	if (ret < 0) {
 		dev_err(data->dev,
 			"failed to change power state to %d\n", on);
-		if (on)
-			pm_runtime_put_noidle(data->dev);
-
 		return ret;
 	}
 #endif
@@ -963,12 +960,14 @@ int bmc150_magn_probe(struct device *dev, struct regmap *regmap,
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(dev, "unable to register iio device\n");
-		goto err_buffer_cleanup;
+		goto err_disable_runtime_pm;
 	}
 
 	dev_dbg(dev, "Registered device %s\n", name);
 	return 0;
 
+err_disable_runtime_pm:
+	pm_runtime_disable(dev);
 err_buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
 err_free_irq:
@@ -992,7 +991,6 @@ int bmc150_magn_remove(struct device *dev)
 
 	pm_runtime_disable(dev);
 	pm_runtime_set_suspended(dev);
-	pm_runtime_put_noidle(dev);
 
 	iio_triggered_buffer_cleanup(indio_dev);
 
-- 
2.31.1

Re: [PATCH 11/28] iio: magn: bmc150: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:37 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> probe() error paths after runtime pm is enabled, should disable it.
> remove() should not call pm_runtime_put_noidle() as there is no
> matching get() to have raised the reference count.  This case
> has no affect a the runtime pm core protects against going negative.
> 
> Whilst here use pm_runtime_resume_and_get() to tidy things up a little.
> coccicheck script didn't get this one due to complex code structure so
> found by inspection.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index d75b437a43f2..39920cb764bf 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -262,7 +262,7 @@ static int bmc150_magn_set_power_state(struct bmc150_magn_data *data, bool on)
>  	int ret;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(data->dev);
> +		ret = pm_runtime_resume_and_get(data->dev);
>  	} else {
>  		pm_runtime_mark_last_busy(data->dev);
>  		ret = pm_runtime_put_autosuspend(data->dev);
> @@ -271,9 +271,6 @@ static int bmc150_magn_set_power_state(struct bmc150_magn_data *data, bool on)
>  	if (ret < 0) {
>  		dev_err(data->dev,
>  			"failed to change power state to %d\n", on);
> -		if (on)
> -			pm_runtime_put_noidle(data->dev);
> -
>  		return ret;
>  	}
>  #endif
> @@ -963,12 +960,14 @@ int bmc150_magn_probe(struct device *dev, struct regmap *regmap,
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(dev, "unable to register iio device\n");
> -		goto err_buffer_cleanup;
> +		goto err_disable_runtime_pm;
>  	}
>  
>  	dev_dbg(dev, "Registered device %s\n", name);
>  	return 0;
>  
> +err_disable_runtime_pm:
> +	pm_runtime_disable(dev);
>  err_buffer_cleanup:
>  	iio_triggered_buffer_cleanup(indio_dev);
>  err_free_irq:
> @@ -992,7 +991,6 @@ int bmc150_magn_remove(struct device *dev)
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
>  
>  	iio_triggered_buffer_cleanup(indio_dev);
>  



Thanks,
Mauro

Re: [PATCH 11/28] iio: magn: bmc150: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 15:48:57 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:37 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > probe() error paths after runtime pm is enabled, should disable it.
> > remove() should not call pm_runtime_put_noidle() as there is no
> > matching get() to have raised the reference count.  This case
> > has no affect a the runtime pm core protects against going negative.
> > 
> > Whilst here use pm_runtime_resume_and_get() to tidy things up a little.
> > coccicheck script didn't get this one due to complex code structure so
> > found by inspection.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders
to take a poke at it.

Still time for additional reviews as this won't go out in a non rebasing
tree for a few days yet.

Thanks,

Jonathan
> 
> > ---
> >  drivers/iio/magnetometer/bmc150_magn.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> > index d75b437a43f2..39920cb764bf 100644
> > --- a/drivers/iio/magnetometer/bmc150_magn.c
> > +++ b/drivers/iio/magnetometer/bmc150_magn.c
> > @@ -262,7 +262,7 @@ static int bmc150_magn_set_power_state(struct bmc150_magn_data *data, bool on)
> >  	int ret;
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(data->dev);
> > +		ret = pm_runtime_resume_and_get(data->dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(data->dev);
> >  		ret = pm_runtime_put_autosuspend(data->dev);
> > @@ -271,9 +271,6 @@ static int bmc150_magn_set_power_state(struct bmc150_magn_data *data, bool on)
> >  	if (ret < 0) {
> >  		dev_err(data->dev,
> >  			"failed to change power state to %d\n", on);
> > -		if (on)
> > -			pm_runtime_put_noidle(data->dev);
> > -
> >  		return ret;
> >  	}
> >  #endif
> > @@ -963,12 +960,14 @@ int bmc150_magn_probe(struct device *dev, struct regmap *regmap,
> >  	ret = iio_device_register(indio_dev);
> >  	if (ret < 0) {
> >  		dev_err(dev, "unable to register iio device\n");
> > -		goto err_buffer_cleanup;
> > +		goto err_disable_runtime_pm;
> >  	}
> >  
> >  	dev_dbg(dev, "Registered device %s\n", name);
> >  	return 0;
> >  
> > +err_disable_runtime_pm:
> > +	pm_runtime_disable(dev);
> >  err_buffer_cleanup:
> >  	iio_triggered_buffer_cleanup(indio_dev);
> >  err_free_irq:
> > @@ -992,7 +991,6 @@ int bmc150_magn_remove(struct device *dev)
> >  
> >  	pm_runtime_disable(dev);
> >  	pm_runtime_set_suspended(dev);
> > -	pm_runtime_put_noidle(dev);
> >  
> >  	iio_triggered_buffer_cleanup(indio_dev);
> >    
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH 11/28] iio: magn: bmc150: Balance runtime pm + use pm_runtime_resume_and_get()

[Linus Walleij]

On Sun, May 9, 2021 at 1:37 PM Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> probe() error paths after runtime pm is enabled, should disable it.
> remove() should not call pm_runtime_put_noidle() as there is no
> matching get() to have raised the reference count.  This case
> has no affect a the runtime pm core protects against going negative.
>
> Whilst here use pm_runtime_resume_and_get() to tidy things up a little.
> coccicheck script didn't get this one due to complex code structure so
> found by inspection.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 12/28] iio: accel: bmi088: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The call to pm_runtime_put_noidle() in remove() is not balanced by a get so
drop it.

Using pm_runtime_resume_and_get() allows for simple introduction of
error handling to allow for runtime resume failing.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/iio/accel/bmi088-accel-core.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
index 61aaaf48c040..a06dae5c971d 100644
--- a/drivers/iio/accel/bmi088-accel-core.c
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -285,11 +285,17 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		switch (chan->type) {
 		case IIO_TEMP:
-			pm_runtime_get_sync(dev);
+			ret = pm_runtime_resume_and_get(dev);
+			if (ret)
+				return ret;
+
 			ret = bmi088_accel_get_temp(data, val);
 			goto out_read_raw_pm_put;
 		case IIO_ACCEL:
-			pm_runtime_get_sync(dev);
+			ret = pm_runtime_resume_and_get(dev);
+			if (ret)
+				return ret;
+
 			ret = iio_device_claim_direct_mode(indio_dev);
 			if (ret)
 				goto out_read_raw_pm_put;
@@ -319,7 +325,10 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
 			*val = BMI088_ACCEL_TEMP_UNIT;
 			return IIO_VAL_INT;
 		case IIO_ACCEL:
-			pm_runtime_get_sync(dev);
+			ret = pm_runtime_resume_and_get(dev);
+			if (ret)
+				return ret;
+
 			ret = regmap_read(data->regmap,
 					  BMI088_ACCEL_REG_ACC_RANGE, val);
 			if (ret)
@@ -334,7 +343,10 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		pm_runtime_get_sync(dev);
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret)
+			return ret;
+
 		ret = bmi088_accel_get_sample_freq(data, val, val2);
 		goto out_read_raw_pm_put;
 	default:
@@ -376,7 +388,10 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		pm_runtime_get_sync(dev);
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret)
+			return ret;
+
 		ret = bmi088_accel_set_sample_freq(data, val);
 		pm_runtime_mark_last_busy(dev);
 		pm_runtime_put_autosuspend(dev);
@@ -530,7 +545,6 @@ int bmi088_accel_core_remove(struct device *dev)
 
 	pm_runtime_disable(dev);
 	pm_runtime_set_suspended(dev);
-	pm_runtime_put_noidle(dev);
 	bmi088_accel_power_down(data);
 
 	return 0;
-- 
2.31.1

Re: [PATCH 12/28] iio: accel: bmi088: Balance runtime pm + use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:38 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> The call to pm_runtime_put_noidle() in remove() is not balanced by a get so
> drop it.
> 
> Using pm_runtime_resume_and_get() allows for simple introduction of
> error handling to allow for runtime resume failing.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Mike Looijmans <mike.looijmans@topic.nl>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/accel/bmi088-accel-core.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index 61aaaf48c040..a06dae5c971d 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -285,11 +285,17 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		switch (chan->type) {
>  		case IIO_TEMP:
> -			pm_runtime_get_sync(dev);
> +			ret = pm_runtime_resume_and_get(dev);
> +			if (ret)
> +				return ret;
> +
>  			ret = bmi088_accel_get_temp(data, val);
>  			goto out_read_raw_pm_put;
>  		case IIO_ACCEL:
> -			pm_runtime_get_sync(dev);
> +			ret = pm_runtime_resume_and_get(dev);
> +			if (ret)
> +				return ret;
> +
>  			ret = iio_device_claim_direct_mode(indio_dev);
>  			if (ret)
>  				goto out_read_raw_pm_put;
> @@ -319,7 +325,10 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
>  			*val = BMI088_ACCEL_TEMP_UNIT;
>  			return IIO_VAL_INT;
>  		case IIO_ACCEL:
> -			pm_runtime_get_sync(dev);
> +			ret = pm_runtime_resume_and_get(dev);
> +			if (ret)
> +				return ret;
> +
>  			ret = regmap_read(data->regmap,
>  					  BMI088_ACCEL_REG_ACC_RANGE, val);
>  			if (ret)
> @@ -334,7 +343,10 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		pm_runtime_get_sync(dev);
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			return ret;
> +
>  		ret = bmi088_accel_get_sample_freq(data, val, val2);
>  		goto out_read_raw_pm_put;
>  	default:
> @@ -376,7 +388,10 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		pm_runtime_get_sync(dev);
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			return ret;
> +
>  		ret = bmi088_accel_set_sample_freq(data, val);
>  		pm_runtime_mark_last_busy(dev);
>  		pm_runtime_put_autosuspend(dev);
> @@ -530,7 +545,6 @@ int bmi088_accel_core_remove(struct device *dev)
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
> -	pm_runtime_put_noidle(dev);
>  	bmi088_accel_power_down(data);
>  
>  	return 0;



Thanks,
Mauro

Re: [PATCH 12/28] iio: accel: bmi088: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 15:51:39 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:38 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > The call to pm_runtime_put_noidle() in remove() is not balanced by a get so
> > drop it.
> > 
> > Using pm_runtime_resume_and_get() allows for simple introduction of
> > error handling to allow for runtime resume failing.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Mike Looijmans <mike.looijmans@topic.nl>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to see if they can find anything we missed.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/accel/bmi088-accel-core.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> > index 61aaaf48c040..a06dae5c971d 100644
> > --- a/drivers/iio/accel/bmi088-accel-core.c
> > +++ b/drivers/iio/accel/bmi088-accel-core.c
> > @@ -285,11 +285,17 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_RAW:
> >  		switch (chan->type) {
> >  		case IIO_TEMP:
> > -			pm_runtime_get_sync(dev);
> > +			ret = pm_runtime_resume_and_get(dev);
> > +			if (ret)
> > +				return ret;
> > +
> >  			ret = bmi088_accel_get_temp(data, val);
> >  			goto out_read_raw_pm_put;
> >  		case IIO_ACCEL:
> > -			pm_runtime_get_sync(dev);
> > +			ret = pm_runtime_resume_and_get(dev);
> > +			if (ret)
> > +				return ret;
> > +
> >  			ret = iio_device_claim_direct_mode(indio_dev);
> >  			if (ret)
> >  				goto out_read_raw_pm_put;
> > @@ -319,7 +325,10 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> >  			*val = BMI088_ACCEL_TEMP_UNIT;
> >  			return IIO_VAL_INT;
> >  		case IIO_ACCEL:
> > -			pm_runtime_get_sync(dev);
> > +			ret = pm_runtime_resume_and_get(dev);
> > +			if (ret)
> > +				return ret;
> > +
> >  			ret = regmap_read(data->regmap,
> >  					  BMI088_ACCEL_REG_ACC_RANGE, val);
> >  			if (ret)
> > @@ -334,7 +343,10 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> >  			return -EINVAL;
> >  		}
> >  	case IIO_CHAN_INFO_SAMP_FREQ:
> > -		pm_runtime_get_sync(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> > +		if (ret)
> > +			return ret;
> > +
> >  		ret = bmi088_accel_get_sample_freq(data, val, val2);
> >  		goto out_read_raw_pm_put;
> >  	default:
> > @@ -376,7 +388,10 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_SAMP_FREQ:
> > -		pm_runtime_get_sync(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> > +		if (ret)
> > +			return ret;
> > +
> >  		ret = bmi088_accel_set_sample_freq(data, val);
> >  		pm_runtime_mark_last_busy(dev);
> >  		pm_runtime_put_autosuspend(dev);
> > @@ -530,7 +545,6 @@ int bmi088_accel_core_remove(struct device *dev)
> >  
> >  	pm_runtime_disable(dev);
> >  	pm_runtime_set_suspended(dev);
> > -	pm_runtime_put_noidle(dev);
> >  	bmi088_accel_power_down(data);
> >  
> >  	return 0;  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 13/28] iio: imu: mpu6050: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

pm_runtime_resume_and_get() returns <= 0 only so simplify related checks
to bring this more inline with nearby calls.

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 18 ++++++------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  6 ++----
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 6244a07048df..06b9c84f6bfb 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -570,11 +570,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
 	period_us = 1000000 / freq_hz;
 
-	result = pm_runtime_get_sync(pdev);
-	if (result < 0) {
-		pm_runtime_put_noidle(pdev);
+	result = pm_runtime_resume_and_get(pdev);
+	if (result)
 		return result;
-	}
 
 	switch (chan->type) {
 	case IIO_ANGL_VEL:
@@ -812,11 +810,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
 		return result;
 
 	mutex_lock(&st->lock);
-	result = pm_runtime_get_sync(pdev);
-	if (result < 0) {
-		pm_runtime_put_noidle(pdev);
+	result = pm_runtime_resume_and_get(pdev);
+	if (result)
 		goto error_write_raw_unlock;
-	}
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -930,11 +926,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 		result = 0;
 		goto fifo_rate_fail_unlock;
 	}
-	result = pm_runtime_get_sync(pdev);
-	if (result < 0) {
-		pm_runtime_put_noidle(pdev);
+	result = pm_runtime_resume_and_get(pdev);
+	if (result)
 		goto fifo_rate_fail_unlock;
-	}
 
 	result = regmap_write(st->map, st->reg->sample_rate_div, d);
 	if (result)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index e21ba778595a..2d0e8cdd4848 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -173,11 +173,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 
 	if (enable) {
 		scan = inv_scan_query(indio_dev);
-		result = pm_runtime_get_sync(pdev);
-		if (result < 0) {
-			pm_runtime_put_noidle(pdev);
+		result = pm_runtime_resume_and_get(pdev);
+		if (result)
 			return result;
-		}
 		/*
 		 * In case autosuspend didn't trigger, turn off first not
 		 * required sensors.
-- 
2.31.1

Re: [PATCH 13/28] iio: imu: mpu6050: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:39 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> pm_runtime_resume_and_get() returns <= 0 only so simplify related checks
> to bring this more inline with nearby calls.
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.

Hmm... I guess there are some weird things happening there at
inv_mpu_core_probe(). The code there does:

...
        pm_runtime_put(dev); // Already mentioned above
        result = devm_add_action_or_reset(dev, inv_mpu_pm_disable, dev);
        if (result)
                return result;

and inv_mpu_pm_disable, in turn, calls:

	pm_runtime_put_sync_suspend(dev);

I failed to see where the corresponding get for both 
pm_runtime_put() and pm_runtime_put_sync_suspend(), as the
remaining RPM get/put logic seemed balanced.



> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 18 ++++++------------
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  6 ++----
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 6244a07048df..06b9c84f6bfb 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -570,11 +570,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
>  	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
>  	period_us = 1000000 / freq_hz;
>  
> -	result = pm_runtime_get_sync(pdev);
> -	if (result < 0) {
> -		pm_runtime_put_noidle(pdev);
> +	result = pm_runtime_resume_and_get(pdev);
> +	if (result)
>  		return result;
> -	}
>  
>  	switch (chan->type) {
>  	case IIO_ANGL_VEL:
> @@ -812,11 +810,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
>  		return result;
>  
>  	mutex_lock(&st->lock);
> -	result = pm_runtime_get_sync(pdev);
> -	if (result < 0) {
> -		pm_runtime_put_noidle(pdev);
> +	result = pm_runtime_resume_and_get(pdev);
> +	if (result)
>  		goto error_write_raw_unlock;
> -	}
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -930,11 +926,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
>  		result = 0;
>  		goto fifo_rate_fail_unlock;
>  	}
> -	result = pm_runtime_get_sync(pdev);
> -	if (result < 0) {
> -		pm_runtime_put_noidle(pdev);
> +	result = pm_runtime_resume_and_get(pdev);
> +	if (result)
>  		goto fifo_rate_fail_unlock;
> -	}
>  
>  	result = regmap_write(st->map, st->reg->sample_rate_div, d);
>  	if (result)
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index e21ba778595a..2d0e8cdd4848 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -173,11 +173,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  
>  	if (enable) {
>  		scan = inv_scan_query(indio_dev);
> -		result = pm_runtime_get_sync(pdev);
> -		if (result < 0) {
> -			pm_runtime_put_noidle(pdev);
> +		result = pm_runtime_resume_and_get(pdev);
> +		if (result)
>  			return result;
> -		}
>  		/*
>  		 * In case autosuspend didn't trigger, turn off first not
>  		 * required sensors.



Thanks,
Mauro

Re: [PATCH 13/28] iio: imu: mpu6050: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 16:11:02 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:39 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > pm_runtime_resume_and_get() returns <= 0 only so simplify related checks
> > to bring this more inline with nearby calls.
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.  
> 
> Hmm... I guess there are some weird things happening there at
> inv_mpu_core_probe(). The code there does:
> 
> ...
>         pm_runtime_put(dev); // Already mentioned above

That one is ok I think as it matches the pm_runtime_get_noresume() a couple
of lines further up.

That will increment the counter, but not carry out a resume (which make sense
as the device is already powered up).  The pm_runtime_put() will cause
the device to suspend which is fine and somewhat logical.

>         result = devm_add_action_or_reset(dev, inv_mpu_pm_disable, dev);
>         if (result)
>                 return result;
> 
> and inv_mpu_pm_disable, in turn, calls:
> 
> 	pm_runtime_put_sync_suspend(dev);

Good spot on that one.  I'll drop it in v2.

> 
> I failed to see where the corresponding get for both 
> pm_runtime_put() and pm_runtime_put_sync_suspend(), as the
> remaining RPM get/put logic seemed balanced.

> 
> 
> 
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 18 ++++++------------
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  6 ++----
> >  2 files changed, 8 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index 6244a07048df..06b9c84f6bfb 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > @@ -570,11 +570,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
> >  	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
> >  	period_us = 1000000 / freq_hz;
> >  
> > -	result = pm_runtime_get_sync(pdev);
> > -	if (result < 0) {
> > -		pm_runtime_put_noidle(pdev);
> > +	result = pm_runtime_resume_and_get(pdev);
> > +	if (result)
> >  		return result;
> > -	}
> >  
> >  	switch (chan->type) {
> >  	case IIO_ANGL_VEL:
> > @@ -812,11 +810,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> >  		return result;
> >  
> >  	mutex_lock(&st->lock);
> > -	result = pm_runtime_get_sync(pdev);
> > -	if (result < 0) {
> > -		pm_runtime_put_noidle(pdev);
> > +	result = pm_runtime_resume_and_get(pdev);
> > +	if (result)
> >  		goto error_write_raw_unlock;
> > -	}
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_SCALE:
> > @@ -930,11 +926,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> >  		result = 0;
> >  		goto fifo_rate_fail_unlock;
> >  	}
> > -	result = pm_runtime_get_sync(pdev);
> > -	if (result < 0) {
> > -		pm_runtime_put_noidle(pdev);
> > +	result = pm_runtime_resume_and_get(pdev);
> > +	if (result)
> >  		goto fifo_rate_fail_unlock;
> > -	}
> >  
> >  	result = regmap_write(st->map, st->reg->sample_rate_div, d);
> >  	if (result)
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > index e21ba778595a..2d0e8cdd4848 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > @@ -173,11 +173,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
> >  
> >  	if (enable) {
> >  		scan = inv_scan_query(indio_dev);
> > -		result = pm_runtime_get_sync(pdev);
> > -		if (result < 0) {
> > -			pm_runtime_put_noidle(pdev);
> > +		result = pm_runtime_resume_and_get(pdev);
> > +		if (result)
> >  			return result;
> > -		}
> >  		/*
> >  		 * In case autosuspend didn't trigger, turn off first not
> >  		 * required sensors.  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 14/28] iio: adc: stm32-adc: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/iio/adc/stm32-adc.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index c9d36c88ee6a..5088de835bb1 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -1158,11 +1158,9 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
 
 	adc->bufi = 0;
 
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
 		return ret;
-	}
 
 	/* Apply sampling time settings */
 	stm32_adc_writel(adc, regs->smpr[0], adc->smpr_val[0]);
@@ -1364,11 +1362,9 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev,
 	struct device *dev = indio_dev->dev.parent;
 	int ret;
 
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
 		return ret;
-	}
 
 	adc->num_conv = bitmap_weight(scan_mask, indio_dev->masklength);
 
@@ -1413,11 +1409,9 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
 	struct device *dev = indio_dev->dev.parent;
 	int ret;
 
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
 		return ret;
-	}
 
 	if (!readval)
 		stm32_adc_writel(adc, reg, writeval);
@@ -1537,11 +1531,9 @@ static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
 	struct device *dev = indio_dev->dev.parent;
 	int ret;
 
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = stm32_adc_set_trig(indio_dev, indio_dev->trig);
 	if (ret) {
-- 
2.31.1

Re: [PATCH 14/28] iio: adc: stm32-adc: Use pm_runtime_resume_and_get() to replace open coding.

[Fabrice Gasnier]

On 5/9/21 1:33 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
>  drivers/iio/adc/stm32-adc.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)

Hi Jonathan,

Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks for the patch,
Regards,
Fabrice

> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index c9d36c88ee6a..5088de835bb1 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -1158,11 +1158,9 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
>  
>  	adc->bufi = 0;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	/* Apply sampling time settings */
>  	stm32_adc_writel(adc, regs->smpr[0], adc->smpr_val[0]);
> @@ -1364,11 +1362,9 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev,
>  	struct device *dev = indio_dev->dev.parent;
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	adc->num_conv = bitmap_weight(scan_mask, indio_dev->masklength);
>  
> @@ -1413,11 +1409,9 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
>  	struct device *dev = indio_dev->dev.parent;
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	if (!readval)
>  		stm32_adc_writel(adc, reg, writeval);
> @@ -1537,11 +1531,9 @@ static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
>  	struct device *dev = indio_dev->dev.parent;
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	ret = stm32_adc_set_trig(indio_dev, indio_dev->trig);
>  	if (ret) {
> 

Re: [PATCH 14/28] iio: adc: stm32-adc: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 15:30:14 +0200
Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote:

> On 5/9/21 1:33 PM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> > ---
> >  drivers/iio/adc/stm32-adc.c | 24 ++++++++----------------
> >  1 file changed, 8 insertions(+), 16 deletions(-)  
> 
> Hi Jonathan,
> 
> Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to see if they can find anything we missed.

Thanks,

Jonathan

> 
> Thanks for the patch,
> Regards,
> Fabrice
> 
> > 
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index c9d36c88ee6a..5088de835bb1 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -1158,11 +1158,9 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
> >  
> >  	adc->bufi = 0;
> >  
> > -	ret = pm_runtime_get_sync(dev);
> > -	if (ret < 0) {
> > -		pm_runtime_put_noidle(dev);
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> >  
> >  	/* Apply sampling time settings */
> >  	stm32_adc_writel(adc, regs->smpr[0], adc->smpr_val[0]);
> > @@ -1364,11 +1362,9 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev,
> >  	struct device *dev = indio_dev->dev.parent;
> >  	int ret;
> >  
> > -	ret = pm_runtime_get_sync(dev);
> > -	if (ret < 0) {
> > -		pm_runtime_put_noidle(dev);
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> >  
> >  	adc->num_conv = bitmap_weight(scan_mask, indio_dev->masklength);
> >  
> > @@ -1413,11 +1409,9 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
> >  	struct device *dev = indio_dev->dev.parent;
> >  	int ret;
> >  
> > -	ret = pm_runtime_get_sync(dev);
> > -	if (ret < 0) {
> > -		pm_runtime_put_noidle(dev);
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> >  
> >  	if (!readval)
> >  		stm32_adc_writel(adc, reg, writeval);
> > @@ -1537,11 +1531,9 @@ static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
> >  	struct device *dev = indio_dev->dev.parent;
> >  	int ret;
> >  
> > -	ret = pm_runtime_get_sync(dev);
> > -	if (ret < 0) {
> > -		pm_runtime_put_noidle(dev);
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> >  
> >  	ret = stm32_adc_set_trig(indio_dev, indio_dev->trig);
> >  	if (ret) {
> >   

Re: [PATCH 14/28] iio: adc: stm32-adc: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:40 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/adc/stm32-adc.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index c9d36c88ee6a..5088de835bb1 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -1158,11 +1158,9 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
>  
>  	adc->bufi = 0;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	/* Apply sampling time settings */
>  	stm32_adc_writel(adc, regs->smpr[0], adc->smpr_val[0]);
> @@ -1364,11 +1362,9 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev,
>  	struct device *dev = indio_dev->dev.parent;
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	adc->num_conv = bitmap_weight(scan_mask, indio_dev->masklength);
>  
> @@ -1413,11 +1409,9 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev,
>  	struct device *dev = indio_dev->dev.parent;
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	if (!readval)
>  		stm32_adc_writel(adc, reg, writeval);
> @@ -1537,11 +1531,9 @@ static int stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
>  	struct device *dev = indio_dev->dev.parent;
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	ret = stm32_adc_set_trig(indio_dev, indio_dev->trig);
>  	if (ret) {



Thanks,
Mauro

[PATCH 15/28] iio: adc: stm32-dfsdm: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/iio/adc/stm32-dfsdm-core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c
index bb925a11c8ae..a627af9a825e 100644
--- a/drivers/iio/adc/stm32-dfsdm-core.c
+++ b/drivers/iio/adc/stm32-dfsdm-core.c
@@ -135,11 +135,9 @@ int stm32_dfsdm_start_dfsdm(struct stm32_dfsdm *dfsdm)
 	int ret;
 
 	if (atomic_inc_return(&priv->n_active_ch) == 1) {
-		ret = pm_runtime_get_sync(dev);
-		if (ret < 0) {
-			pm_runtime_put_noidle(dev);
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret < 0)
 			goto error_ret;
-		}
 
 		/* select clock source, e.g. 0 for "dfsdm" or 1 for "audio" */
 		clk_src = priv->aclk ? 1 : 0;
-- 
2.31.1

Re: [PATCH 15/28] iio: adc: stm32-dfsdm: Use pm_runtime_resume_and_get() to replace open coding.

[Fabrice Gasnier]

On 5/9/21 1:33 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
>  drivers/iio/adc/stm32-dfsdm-core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)


Hi Jonathan,

Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks for the patch,
Regards,
Fabrice

> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c
> index bb925a11c8ae..a627af9a825e 100644
> --- a/drivers/iio/adc/stm32-dfsdm-core.c
> +++ b/drivers/iio/adc/stm32-dfsdm-core.c
> @@ -135,11 +135,9 @@ int stm32_dfsdm_start_dfsdm(struct stm32_dfsdm *dfsdm)
>  	int ret;
>  
>  	if (atomic_inc_return(&priv->n_active_ch) == 1) {
> -		ret = pm_runtime_get_sync(dev);
> -		if (ret < 0) {
> -			pm_runtime_put_noidle(dev);
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0)
>  			goto error_ret;
> -		}
>  
>  		/* select clock source, e.g. 0 for "dfsdm" or 1 for "audio" */
>  		clk_src = priv->aclk ? 1 : 0;
> 

Re: [PATCH 15/28] iio: adc: stm32-dfsdm: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 15:31:33 +0200
Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote:

> On 5/9/21 1:33 PM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> > ---
> >  drivers/iio/adc/stm32-dfsdm-core.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)  
> 
> 
> Hi Jonathan,
> 
> Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Applied, thanks

> 
> Thanks for the patch,
> Regards,
> Fabrice
> 
> > 
> > diff --git a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c
> > index bb925a11c8ae..a627af9a825e 100644
> > --- a/drivers/iio/adc/stm32-dfsdm-core.c
> > +++ b/drivers/iio/adc/stm32-dfsdm-core.c
> > @@ -135,11 +135,9 @@ int stm32_dfsdm_start_dfsdm(struct stm32_dfsdm *dfsdm)
> >  	int ret;
> >  
> >  	if (atomic_inc_return(&priv->n_active_ch) == 1) {
> > -		ret = pm_runtime_get_sync(dev);
> > -		if (ret < 0) {
> > -			pm_runtime_put_noidle(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> > +		if (ret < 0)
> >  			goto error_ret;
> > -		}
> >  
> >  		/* select clock source, e.g. 0 for "dfsdm" or 1 for "audio" */
> >  		clk_src = priv->aclk ? 1 : 0;
> >   

Re: [PATCH 15/28] iio: adc: stm32-dfsdm: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:41 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/adc/stm32-dfsdm-core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c
> index bb925a11c8ae..a627af9a825e 100644
> --- a/drivers/iio/adc/stm32-dfsdm-core.c
> +++ b/drivers/iio/adc/stm32-dfsdm-core.c
> @@ -135,11 +135,9 @@ int stm32_dfsdm_start_dfsdm(struct stm32_dfsdm *dfsdm)
>  	int ret;
>  
>  	if (atomic_inc_return(&priv->n_active_ch) == 1) {
> -		ret = pm_runtime_get_sync(dev);
> -		if (ret < 0) {
> -			pm_runtime_put_noidle(dev);
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0)
>  			goto error_ret;
> -		}
>  
>  		/* select clock source, e.g. 0 for "dfsdm" or 1 for "audio" */
>  		clk_src = priv->aclk ? 1 : 0;



Thanks,
Mauro

[PATCH 16/28] iio: dac: stm32-dac: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/iio/dac/stm32-dac.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
index a5b0a52bf86e..dd2e306824e7 100644
--- a/drivers/iio/dac/stm32-dac.c
+++ b/drivers/iio/dac/stm32-dac.c
@@ -69,9 +69,8 @@ static int stm32_dac_set_enable_state(struct iio_dev *indio_dev, int ch,
 	}
 
 	if (enable) {
-		ret = pm_runtime_get_sync(dev);
+		ret = pm_runtime_resume_and_get(dev);
 		if (ret < 0) {
-			pm_runtime_put_noidle(dev);
 			mutex_unlock(&dac->lock);
 			return ret;
 		}
-- 
2.31.1

Re: [PATCH 16/28] iio: dac: stm32-dac: Use pm_runtime_resume_and_get() to replace open coding.

[Fabrice Gasnier]

On 5/9/21 1:33 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
>  drivers/iio/dac/stm32-dac.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Hi Jonathan,

Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks for the patch,
Regards,
Fabrice

> 
> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> index a5b0a52bf86e..dd2e306824e7 100644
> --- a/drivers/iio/dac/stm32-dac.c
> +++ b/drivers/iio/dac/stm32-dac.c
> @@ -69,9 +69,8 @@ static int stm32_dac_set_enable_state(struct iio_dev *indio_dev, int ch,
>  	}
>  
>  	if (enable) {
> -		ret = pm_runtime_get_sync(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  		if (ret < 0) {
> -			pm_runtime_put_noidle(dev);
>  			mutex_unlock(&dac->lock);
>  			return ret;
>  		}
> 

Re: [PATCH 16/28] iio: dac: stm32-dac: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:42 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/dac/stm32-dac.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> index a5b0a52bf86e..dd2e306824e7 100644
> --- a/drivers/iio/dac/stm32-dac.c
> +++ b/drivers/iio/dac/stm32-dac.c
> @@ -69,9 +69,8 @@ static int stm32_dac_set_enable_state(struct iio_dev *indio_dev, int ch,
>  	}
>  
>  	if (enable) {
> -		ret = pm_runtime_get_sync(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  		if (ret < 0) {
> -			pm_runtime_put_noidle(dev);
>  			mutex_unlock(&dac->lock);
>  			return ret;
>  		}



Thanks,
Mauro

Re: [PATCH 16/28] iio: dac: stm32-dac: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 16:20:26 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:42 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Applied.  Thanks

Jonathan

> 
> > ---
> >  drivers/iio/dac/stm32-dac.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
> > index a5b0a52bf86e..dd2e306824e7 100644
> > --- a/drivers/iio/dac/stm32-dac.c
> > +++ b/drivers/iio/dac/stm32-dac.c
> > @@ -69,9 +69,8 @@ static int stm32_dac_set_enable_state(struct iio_dev *indio_dev, int ch,
> >  	}
> >  
> >  	if (enable) {
> > -		ret = pm_runtime_get_sync(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> >  		if (ret < 0) {
> > -			pm_runtime_put_noidle(dev);
> >  			mutex_unlock(&dac->lock);
> >  			return ret;
> >  		}  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 17/28] iio: adc: ads1015: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti-ads1015.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 9fef39bcf997..4b61ee3d2501 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -323,9 +323,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
 	struct device *dev = regmap_get_device(data->regmap);
 
 	if (on) {
-		ret = pm_runtime_get_sync(dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(dev);
+		ret = pm_runtime_resume_and_get(dev);
 	} else {
 		pm_runtime_mark_last_busy(dev);
 		ret = pm_runtime_put_autosuspend(dev);
-- 
2.31.1

Re: [PATCH 17/28] iio: adc: ads1015: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:43 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This driver has an extra (unbalanced?) call to pm_runtime_put_noidle() at
ads1015_remove().

> ---
>  drivers/iio/adc/ti-ads1015.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 9fef39bcf997..4b61ee3d2501 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -323,9 +323,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
>  	struct device *dev = regmap_get_device(data->regmap);
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
>  		ret = pm_runtime_put_autosuspend(dev);



Thanks,
Mauro

Re: [PATCH 17/28] iio: adc: ads1015: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 16:22:12 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:43 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> This driver has an extra (unbalanced?) call to pm_runtime_put_noidle() at
> ads1015_remove().
Good point. Dropped that as well for v2.

> 
> > ---
> >  drivers/iio/adc/ti-ads1015.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> > index 9fef39bcf997..4b61ee3d2501 100644
> > --- a/drivers/iio/adc/ti-ads1015.c
> > +++ b/drivers/iio/adc/ti-ads1015.c
> > @@ -323,9 +323,7 @@ static int ads1015_set_power_state(struct ads1015_data *data, bool on)
> >  	struct device *dev = regmap_get_device(data->regmap);
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(dev);
> > -		if (ret < 0)
> > -			pm_runtime_put_noidle(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(dev);
> >  		ret = pm_runtime_put_autosuspend(dev);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 18/28] iio: chemical: atlas-sensor: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

As pm_runtime_resume_and_get() returns <= 0 take advantage of that to
change the error checking to if (ret) which is more in keeping with the
rest of this driver.

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/iio/chemical/atlas-sensor.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
index d10f921b233a..90bb3f5bff19 100644
--- a/drivers/iio/chemical/atlas-sensor.c
+++ b/drivers/iio/chemical/atlas-sensor.c
@@ -410,11 +410,9 @@ static int atlas_buffer_postenable(struct iio_dev *indio_dev)
 	struct atlas_data *data = iio_priv(indio_dev);
 	int ret;
 
-	ret = pm_runtime_get_sync(&data->client->dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(&data->client->dev);
+	ret = pm_runtime_resume_and_get(&data->client->dev);
+	if (ret)
 		return ret;
-	}
 
 	return atlas_set_interrupt(data, true);
 }
@@ -487,11 +485,9 @@ static int atlas_read_measurement(struct atlas_data *data, int reg, __be32 *val)
 	int suspended = pm_runtime_suspended(dev);
 	int ret;
 
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
 		return ret;
-	}
 
 	if (suspended)
 		msleep(data->chip->delay);
-- 
2.31.1

Re: [PATCH 18/28] iio: chemical: atlas-sensor: Use pm_runtime_resume_and_get() to replace open coding.

[Matt Ranostay]

On Sun, May 9, 2021 at 4:37 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
>
> As pm_runtime_resume_and_get() returns <= 0 take advantage of that to
> change the error checking to if (ret) which is more in keeping with the
> rest of this driver.
>
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
>
Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/iio/chemical/atlas-sensor.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
> index d10f921b233a..90bb3f5bff19 100644
> --- a/drivers/iio/chemical/atlas-sensor.c
> +++ b/drivers/iio/chemical/atlas-sensor.c
> @@ -410,11 +410,9 @@ static int atlas_buffer_postenable(struct iio_dev *indio_dev)
>         struct atlas_data *data = iio_priv(indio_dev);
>         int ret;
>
> -       ret = pm_runtime_get_sync(&data->client->dev);
> -       if (ret < 0) {
> -               pm_runtime_put_noidle(&data->client->dev);
> +       ret = pm_runtime_resume_and_get(&data->client->dev);
> +       if (ret)
>                 return ret;
> -       }
>
>         return atlas_set_interrupt(data, true);
>  }
> @@ -487,11 +485,9 @@ static int atlas_read_measurement(struct atlas_data *data, int reg, __be32 *val)
>         int suspended = pm_runtime_suspended(dev);
>         int ret;
>
> -       ret = pm_runtime_get_sync(dev);
> -       if (ret < 0) {
> -               pm_runtime_put_noidle(dev);
> +       ret = pm_runtime_resume_and_get(dev);
> +       if (ret)
>                 return ret;
> -       }
>
>         if (suspended)
>                 msleep(data->chip->delay);
> --
> 2.31.1
>

Re: [PATCH 18/28] iio: chemical: atlas-sensor: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:44 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> As pm_runtime_resume_and_get() returns <= 0 take advantage of that to
> change the error checking to if (ret) which is more in keeping with the
> rest of this driver.
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>

Same case as patch 17: this driver also seems to have an extra
(unbalanced?) call to pm_runtime_disable() at remove().

> ---
>  drivers/iio/chemical/atlas-sensor.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
> index d10f921b233a..90bb3f5bff19 100644
> --- a/drivers/iio/chemical/atlas-sensor.c
> +++ b/drivers/iio/chemical/atlas-sensor.c
> @@ -410,11 +410,9 @@ static int atlas_buffer_postenable(struct iio_dev *indio_dev)
>  	struct atlas_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(&data->client->dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(&data->client->dev);
> +	ret = pm_runtime_resume_and_get(&data->client->dev);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	return atlas_set_interrupt(data, true);
>  }
> @@ -487,11 +485,9 @@ static int atlas_read_measurement(struct atlas_data *data, int reg, __be32 *val)
>  	int suspended = pm_runtime_suspended(dev);
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		pm_runtime_put_noidle(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	if (suspended)
>  		msleep(data->chip->delay);



Thanks,
Mauro

Re: [PATCH 18/28] iio: chemical: atlas-sensor: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 16:25:41 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:44 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > As pm_runtime_resume_and_get() returns <= 0 take advantage of that to
> > change the error checking to if (ret) which is more in keeping with the
> > rest of this driver.
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>  
> 
> Same case as patch 17: this driver also seems to have an extra
> (unbalanced?) call to pm_runtime_disable() at remove().

Good point again.  Matt, given this is a material change I'm going to
drop your tag, please take another look.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/chemical/atlas-sensor.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
> > index d10f921b233a..90bb3f5bff19 100644
> > --- a/drivers/iio/chemical/atlas-sensor.c
> > +++ b/drivers/iio/chemical/atlas-sensor.c
> > @@ -410,11 +410,9 @@ static int atlas_buffer_postenable(struct iio_dev *indio_dev)
> >  	struct atlas_data *data = iio_priv(indio_dev);
> >  	int ret;
> >  
> > -	ret = pm_runtime_get_sync(&data->client->dev);
> > -	if (ret < 0) {
> > -		pm_runtime_put_noidle(&data->client->dev);
> > +	ret = pm_runtime_resume_and_get(&data->client->dev);
> > +	if (ret)
> >  		return ret;
> > -	}
> >  
> >  	return atlas_set_interrupt(data, true);
> >  }
> > @@ -487,11 +485,9 @@ static int atlas_read_measurement(struct atlas_data *data, int reg, __be32 *val)
> >  	int suspended = pm_runtime_suspended(dev);
> >  	int ret;
> >  
> > -	ret = pm_runtime_get_sync(dev);
> > -	if (ret < 0) {
> > -		pm_runtime_put_noidle(dev);
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> >  		return ret;
> > -	}
> >  
> >  	if (suspended)
> >  		msleep(data->chip->delay);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 19/28] iio: light: pa12203001: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/light/pa12203001.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/light/pa12203001.c b/drivers/iio/light/pa12203001.c
index bfade6577a38..a52b2c788540 100644
--- a/drivers/iio/light/pa12203001.c
+++ b/drivers/iio/light/pa12203001.c
@@ -186,9 +186,7 @@ static int pa12203001_set_power_state(struct pa12203001_data *data, bool on,
 	}
 
 	if (on) {
-		ret = pm_runtime_get_sync(&data->client->dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(&data->client->dev);
+		ret = pm_runtime_resume_and_get(&data->client->dev);
 
 	} else {
 		pm_runtime_mark_last_busy(&data->client->dev);
-- 
2.31.1

Re: [PATCH 19/28] iio: light: pa12203001: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:45 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/light/pa12203001.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/pa12203001.c b/drivers/iio/light/pa12203001.c
> index bfade6577a38..a52b2c788540 100644
> --- a/drivers/iio/light/pa12203001.c
> +++ b/drivers/iio/light/pa12203001.c
> @@ -186,9 +186,7 @@ static int pa12203001_set_power_state(struct pa12203001_data *data, bool on,
>  	}
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(&data->client->dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(&data->client->dev);
> +		ret = pm_runtime_resume_and_get(&data->client->dev);
>  
>  	} else {
>  		pm_runtime_mark_last_busy(&data->client->dev);



Thanks,
Mauro

Re: [PATCH 19/28] iio: light: pa12203001: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Fri, 14 May 2021 09:35:13 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:45 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Thanks,

Applied to the togreg branch of iio.git.

Jonathan

> 
> > ---
> >  drivers/iio/light/pa12203001.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/pa12203001.c b/drivers/iio/light/pa12203001.c
> > index bfade6577a38..a52b2c788540 100644
> > --- a/drivers/iio/light/pa12203001.c
> > +++ b/drivers/iio/light/pa12203001.c
> > @@ -186,9 +186,7 @@ static int pa12203001_set_power_state(struct pa12203001_data *data, bool on,
> >  	}
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(&data->client->dev);
> > -		if (ret < 0)
> > -			pm_runtime_put_noidle(&data->client->dev);
> > +		ret = pm_runtime_resume_and_get(&data->client->dev);
> >  
> >  	} else {
> >  		pm_runtime_mark_last_busy(&data->client->dev);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 20/28] iio: light: us5182: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/light/us5182d.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 393f27b75c75..96e4a66ddf28 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -367,9 +367,7 @@ static int us5182d_set_power_state(struct us5182d_data *data, bool on)
 		return 0;
 
 	if (on) {
-		ret = pm_runtime_get_sync(&data->client->dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(&data->client->dev);
+		ret = pm_runtime_resume_and_get(&data->client->dev);
 	} else {
 		pm_runtime_mark_last_busy(&data->client->dev);
 		ret = pm_runtime_put_autosuspend(&data->client->dev);
-- 
2.31.1

Re: [PATCH 20/28] iio: light: us5182: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:46 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>


> ---
>  drivers/iio/light/us5182d.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
> index 393f27b75c75..96e4a66ddf28 100644
> --- a/drivers/iio/light/us5182d.c
> +++ b/drivers/iio/light/us5182d.c
> @@ -367,9 +367,7 @@ static int us5182d_set_power_state(struct us5182d_data *data, bool on)
>  		return 0;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(&data->client->dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(&data->client->dev);
> +		ret = pm_runtime_resume_and_get(&data->client->dev);
>  	} else {
>  		pm_runtime_mark_last_busy(&data->client->dev);
>  		ret = pm_runtime_put_autosuspend(&data->client->dev);



Thanks,
Mauro

Re: [PATCH 20/28] iio: light: us5182: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 16:35:33 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:46 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
Applied.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/light/us5182d.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
> > index 393f27b75c75..96e4a66ddf28 100644
> > --- a/drivers/iio/light/us5182d.c
> > +++ b/drivers/iio/light/us5182d.c
> > @@ -367,9 +367,7 @@ static int us5182d_set_power_state(struct us5182d_data *data, bool on)
> >  		return 0;
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(&data->client->dev);
> > -		if (ret < 0)
> > -			pm_runtime_put_noidle(&data->client->dev);
> > +		ret = pm_runtime_resume_and_get(&data->client->dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(&data->client->dev);
> >  		ret = pm_runtime_put_autosuspend(&data->client->dev);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 21/28] iio: light: vcnl4000: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Mathieu Othacehe <m.othacehe@gmail.com>
---
 drivers/iio/light/vcnl4000.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 4a61865d01cd..01772327a947 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -413,9 +413,7 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
 	int ret;
 
 	if (on) {
-		ret = pm_runtime_get_sync(dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(dev);
+		ret = pm_runtime_resume_and_get(dev);
 	} else {
 		pm_runtime_mark_last_busy(dev);
 		ret = pm_runtime_put_autosuspend(dev);
-- 
2.31.1

Re: [PATCH 21/28] iio: light: vcnl4000: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:47 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Mathieu Othacehe <m.othacehe@gmail.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/light/vcnl4000.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 4a61865d01cd..01772327a947 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -413,9 +413,7 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
>  	int ret;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
>  		ret = pm_runtime_put_autosuspend(dev);



Thanks,
Mauro

Re: [PATCH 21/28] iio: light: vcnl4000: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 16:36:36 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:47 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Mathieu Othacehe <m.othacehe@gmail.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to poke at.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/light/vcnl4000.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 4a61865d01cd..01772327a947 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -413,9 +413,7 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
> >  	int ret;
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(dev);
> > -		if (ret < 0)
> > -			pm_runtime_put_noidle(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(dev);
> >  		ret = pm_runtime_put_autosuspend(dev);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 22/28] iio: light: vcnl4035: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Parthiban Nallathambi <pn@denx.de>
---
 drivers/iio/light/vcnl4035.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c
index 691a54b763e1..fd2f181b16db 100644
--- a/drivers/iio/light/vcnl4035.c
+++ b/drivers/iio/light/vcnl4035.c
@@ -144,9 +144,7 @@ static int vcnl4035_set_pm_runtime_state(struct vcnl4035_data *data, bool on)
 	struct device *dev = &data->client->dev;
 
 	if (on) {
-		ret = pm_runtime_get_sync(dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(dev);
+		ret = pm_runtime_resume_and_get(dev);
 	} else {
 		pm_runtime_mark_last_busy(dev);
 		ret = pm_runtime_put_autosuspend(dev);
-- 
2.31.1

Re: [PATCH 22/28] iio: light: vcnl4035: Use pm_runtime_resume_and_get() to replace open coding.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:48 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Parthiban Nallathambi <pn@denx.de>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>


> ---
>  drivers/iio/light/vcnl4035.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c
> index 691a54b763e1..fd2f181b16db 100644
> --- a/drivers/iio/light/vcnl4035.c
> +++ b/drivers/iio/light/vcnl4035.c
> @@ -144,9 +144,7 @@ static int vcnl4035_set_pm_runtime_state(struct vcnl4035_data *data, bool on)
>  	struct device *dev = &data->client->dev;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(dev);
> +		ret = pm_runtime_resume_and_get(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
>  		ret = pm_runtime_put_autosuspend(dev);



Thanks,
Mauro

Re: [PATCH 22/28] iio: light: vcnl4035: Use pm_runtime_resume_and_get() to replace open coding.

[Jonathan Cameron]

On Wed, 12 May 2021 16:39:54 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:48 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Parthiban Nallathambi <pn@denx.de>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to see if they can find anything we missed.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/light/vcnl4035.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c
> > index 691a54b763e1..fd2f181b16db 100644
> > --- a/drivers/iio/light/vcnl4035.c
> > +++ b/drivers/iio/light/vcnl4035.c
> > @@ -144,9 +144,7 @@ static int vcnl4035_set_pm_runtime_state(struct vcnl4035_data *data, bool on)
> >  	struct device *dev = &data->client->dev;
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(dev);
> > -		if (ret < 0)
> > -			pm_runtime_put_noidle(dev);
> > +		ret = pm_runtime_resume_and_get(dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(dev);
> >  		ret = pm_runtime_put_autosuspend(dev);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 23/28] iio: pressure: zpa2326: fix potential extra call of runtime suspend.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This case illustrates why the new pm_runtime_sync_and_get() is good
in that it makes clear the correct way to handle errors.

Calling pm_runtime_put() on failure of the _get() in pm_runtime_get_sync()
will potentially result in powering down an already powered down device
(as we never successfully powered it up.  Unlikely to cause any problems
in reality.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/pressure/zpa2326.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/zpa2326.c b/drivers/iio/pressure/zpa2326.c
index 89295c90f801..97ac3ba399f7 100644
--- a/drivers/iio/pressure/zpa2326.c
+++ b/drivers/iio/pressure/zpa2326.c
@@ -664,11 +664,9 @@ static int zpa2326_resume(const struct iio_dev *indio_dev)
 {
 	int err;
 
-	err = pm_runtime_get_sync(indio_dev->dev.parent);
-	if (err < 0) {
-		pm_runtime_put(indio_dev->dev.parent);
+	err = pm_runtime_resume_and_get(indio_dev->dev.parent);
+	if (err < 0)
 		return err;
-	}
 
 	if (err > 0) {
 		/*
-- 
2.31.1

Re: [PATCH 23/28] iio: pressure: zpa2326: fix potential extra call of runtime suspend.

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:49 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This case illustrates why the new pm_runtime_sync_and_get() is good
> in that it makes clear the correct way to handle errors.
> 
> Calling pm_runtime_put() on failure of the _get() in pm_runtime_get_sync()
> will potentially result in powering down an already powered down device
> (as we never successfully powered it up.  Unlikely to cause any problems
> in reality.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This one seems a little odd on my eyes, although I don't know much about
IIO, as it calls RPM get only at:

	zpa2326_init_runtime()

on a balanced way, as the routine starts with RPM get() ends with RPM put().

Then it does a put at zpa2326_suspend() and a get at zpa2326_resume().

Can RPM usage_count be zero at suspend (or are there some other part
of IIO core that increments it?). Because, if after resume, usage_count
would be equal to 1, as I guess RPM core prevent negative values.

Regards,
Mauro



> ---
>  drivers/iio/pressure/zpa2326.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/pressure/zpa2326.c b/drivers/iio/pressure/zpa2326.c
> index 89295c90f801..97ac3ba399f7 100644
> --- a/drivers/iio/pressure/zpa2326.c
> +++ b/drivers/iio/pressure/zpa2326.c
> @@ -664,11 +664,9 @@ static int zpa2326_resume(const struct iio_dev *indio_dev)
>  {
>  	int err;
>  
> -	err = pm_runtime_get_sync(indio_dev->dev.parent);
> -	if (err < 0) {
> -		pm_runtime_put(indio_dev->dev.parent);
> +	err = pm_runtime_resume_and_get(indio_dev->dev.parent);
> +	if (err < 0)
>  		return err;
> -	}
>  
>  	if (err > 0) {
>  		/*



Thanks,
Mauro

Re: [PATCH 23/28] iio: pressure: zpa2326: fix potential extra call of runtime suspend.

[Jonathan Cameron]

On Wed, 12 May 2021 16:49:58 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:49 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This case illustrates why the new pm_runtime_sync_and_get() is good
> > in that it makes clear the correct way to handle errors.
> > 
> > Calling pm_runtime_put() on failure of the _get() in pm_runtime_get_sync()
> > will potentially result in powering down an already powered down device
> > (as we never successfully powered it up.  Unlikely to cause any problems
> > in reality.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> This one seems a little odd on my eyes, although I don't know much about
> IIO, as it calls RPM get only at:
> 
> 	zpa2326_init_runtime()
> 
> on a balanced way, as the routine starts with RPM get() ends with RPM put().
> 
> Then it does a put at zpa2326_suspend() and a get at zpa2326_resume().
> 
> Can RPM usage_count be zero at suspend (or are there some other part
> of IIO core that increments it?). Because, if after resume, usage_count
> would be equal to 1, as I guess RPM core prevent negative values.

I've stared at this for a few minutes and gotten nowhere in figuring out
the intent.   As such I'm going to drop the patch for now and revisit
at a later date.

Jonathan



> 
> Regards,
> Mauro
> 
> 
> 
> > ---
> >  drivers/iio/pressure/zpa2326.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/zpa2326.c b/drivers/iio/pressure/zpa2326.c
> > index 89295c90f801..97ac3ba399f7 100644
> > --- a/drivers/iio/pressure/zpa2326.c
> > +++ b/drivers/iio/pressure/zpa2326.c
> > @@ -664,11 +664,9 @@ static int zpa2326_resume(const struct iio_dev *indio_dev)
> >  {
> >  	int err;
> >  
> > -	err = pm_runtime_get_sync(indio_dev->dev.parent);
> > -	if (err < 0) {
> > -		pm_runtime_put(indio_dev->dev.parent);
> > +	err = pm_runtime_resume_and_get(indio_dev->dev.parent);
> > +	if (err < 0)
> >  		return err;
> > -	}
> >  
> >  	if (err > 0) {
> >  		/*  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 24/28] iio: temp: mlx90614: Handle failure in pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Converts from using pm_runtime_get_sync() with no error handling over
to pm_runtime_resume_and_get() which will ensure we don't end up
holding a reference.  Ensure this error return is then handled at
calls to mlx90614_power_get(). These are all direct returns.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/temperature/mlx90614.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index ef0fec94d269..afcb10ea7c44 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -176,11 +176,14 @@ static inline s32 mlx90614_iir_search(const struct i2c_client *client,
 static int mlx90614_power_get(struct mlx90614_data *data, bool startup)
 {
 	unsigned long now;
+	int ret;
 
 	if (!data->wakeup_gpio)
 		return 0;
 
-	pm_runtime_get_sync(&data->client->dev);
+	ret = pm_runtime_resume_and_get(&data->client->dev);
+	if (ret < 0)
+		return ret;
 
 	if (startup) {
 		now = jiffies;
@@ -267,7 +270,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 		*val = MLX90614_CONST_SCALE;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_CALIBEMISSIVITY: /* 1/65535 / LSB */
-		mlx90614_power_get(data, false);
+		ret = mlx90614_power_get(data, false);
+		if (ret < 0)
+			return ret;
+
 		mutex_lock(&data->lock);
 		ret = i2c_smbus_read_word_data(data->client,
 					       MLX90614_EMISSIVITY);
@@ -287,7 +293,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT_PLUS_NANO;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: /* IIR setting with
 							     FIR = 1024 */
-		mlx90614_power_get(data, false);
+		ret = mlx90614_power_get(data, false);
+		if (ret < 0)
+			return ret;
+
 		mutex_lock(&data->lock);
 		ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
 		mutex_unlock(&data->lock);
@@ -319,7 +328,10 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
 		val = val * MLX90614_CONST_RAW_EMISSIVITY_MAX +
 			val2 / MLX90614_CONST_EMISSIVITY_RESOLUTION;
 
-		mlx90614_power_get(data, false);
+		ret = mlx90614_power_get(data, false);
+		if (ret < 0)
+			return ret;
+
 		mutex_lock(&data->lock);
 		ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY,
 					  val);
@@ -331,7 +343,10 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
 		if (val < 0 || val2 < 0)
 			return -EINVAL;
 
-		mlx90614_power_get(data, false);
+		ret = mlx90614_power_get(data, false);
+		if (ret < 0)
+			return ret;
+
 		mutex_lock(&data->lock);
 		ret = mlx90614_iir_search(data->client,
 					  val * 100 + val2 / 10000);
-- 
2.31.1

Re: [PATCH 24/28] iio: temp: mlx90614: Handle failure in pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:50 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Converts from using pm_runtime_get_sync() with no error handling over
> to pm_runtime_resume_and_get() which will ensure we don't end up
> holding a reference.  Ensure this error return is then handled at
> calls to mlx90614_power_get(). These are all direct returns.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/temperature/mlx90614.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index ef0fec94d269..afcb10ea7c44 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -176,11 +176,14 @@ static inline s32 mlx90614_iir_search(const struct i2c_client *client,
>  static int mlx90614_power_get(struct mlx90614_data *data, bool startup)
>  {
>  	unsigned long now;
> +	int ret;
>  
>  	if (!data->wakeup_gpio)
>  		return 0;
>  
> -	pm_runtime_get_sync(&data->client->dev);
> +	ret = pm_runtime_resume_and_get(&data->client->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (startup) {
>  		now = jiffies;
> @@ -267,7 +270,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  		*val = MLX90614_CONST_SCALE;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_CALIBEMISSIVITY: /* 1/65535 / LSB */
> -		mlx90614_power_get(data, false);
> +		ret = mlx90614_power_get(data, false);
> +		if (ret < 0)
> +			return ret;
> +
>  		mutex_lock(&data->lock);
>  		ret = i2c_smbus_read_word_data(data->client,
>  					       MLX90614_EMISSIVITY);
> @@ -287,7 +293,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT_PLUS_NANO;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: /* IIR setting with
>  							     FIR = 1024 */
> -		mlx90614_power_get(data, false);
> +		ret = mlx90614_power_get(data, false);
> +		if (ret < 0)
> +			return ret;
> +
>  		mutex_lock(&data->lock);
>  		ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
>  		mutex_unlock(&data->lock);
> @@ -319,7 +328,10 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
>  		val = val * MLX90614_CONST_RAW_EMISSIVITY_MAX +
>  			val2 / MLX90614_CONST_EMISSIVITY_RESOLUTION;
>  
> -		mlx90614_power_get(data, false);
> +		ret = mlx90614_power_get(data, false);
> +		if (ret < 0)
> +			return ret;
> +
>  		mutex_lock(&data->lock);
>  		ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY,
>  					  val);
> @@ -331,7 +343,10 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
>  		if (val < 0 || val2 < 0)
>  			return -EINVAL;
>  
> -		mlx90614_power_get(data, false);
> +		ret = mlx90614_power_get(data, false);
> +		if (ret < 0)
> +			return ret;
> +
>  		mutex_lock(&data->lock);
>  		ret = mlx90614_iir_search(data->client,
>  					  val * 100 + val2 / 10000);



Thanks,
Mauro

Re: [PATCH 24/28] iio: temp: mlx90614: Handle failure in pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 16:51:57 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:50 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Converts from using pm_runtime_get_sync() with no error handling over
> > to pm_runtime_resume_and_get() which will ensure we don't end up
> > holding a reference.  Ensure this error return is then handled at
> > calls to mlx90614_power_get(). These are all direct returns.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Applied,

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/temperature/mlx90614.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> > index ef0fec94d269..afcb10ea7c44 100644
> > --- a/drivers/iio/temperature/mlx90614.c
> > +++ b/drivers/iio/temperature/mlx90614.c
> > @@ -176,11 +176,14 @@ static inline s32 mlx90614_iir_search(const struct i2c_client *client,
> >  static int mlx90614_power_get(struct mlx90614_data *data, bool startup)
> >  {
> >  	unsigned long now;
> > +	int ret;
> >  
> >  	if (!data->wakeup_gpio)
> >  		return 0;
> >  
> > -	pm_runtime_get_sync(&data->client->dev);
> > +	ret = pm_runtime_resume_and_get(&data->client->dev);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	if (startup) {
> >  		now = jiffies;
> > @@ -267,7 +270,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
> >  		*val = MLX90614_CONST_SCALE;
> >  		return IIO_VAL_INT;
> >  	case IIO_CHAN_INFO_CALIBEMISSIVITY: /* 1/65535 / LSB */
> > -		mlx90614_power_get(data, false);
> > +		ret = mlx90614_power_get(data, false);
> > +		if (ret < 0)
> > +			return ret;
> > +
> >  		mutex_lock(&data->lock);
> >  		ret = i2c_smbus_read_word_data(data->client,
> >  					       MLX90614_EMISSIVITY);
> > @@ -287,7 +293,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
> >  		return IIO_VAL_INT_PLUS_NANO;
> >  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: /* IIR setting with
> >  							     FIR = 1024 */
> > -		mlx90614_power_get(data, false);
> > +		ret = mlx90614_power_get(data, false);
> > +		if (ret < 0)
> > +			return ret;
> > +
> >  		mutex_lock(&data->lock);
> >  		ret = i2c_smbus_read_word_data(data->client, MLX90614_CONFIG);
> >  		mutex_unlock(&data->lock);
> > @@ -319,7 +328,10 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
> >  		val = val * MLX90614_CONST_RAW_EMISSIVITY_MAX +
> >  			val2 / MLX90614_CONST_EMISSIVITY_RESOLUTION;
> >  
> > -		mlx90614_power_get(data, false);
> > +		ret = mlx90614_power_get(data, false);
> > +		if (ret < 0)
> > +			return ret;
> > +
> >  		mutex_lock(&data->lock);
> >  		ret = mlx90614_write_word(data->client, MLX90614_EMISSIVITY,
> >  					  val);
> > @@ -331,7 +343,10 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
> >  		if (val < 0 || val2 < 0)
> >  			return -EINVAL;
> >  
> > -		mlx90614_power_get(data, false);
> > +		ret = mlx90614_power_get(data, false);
> > +		if (ret < 0)
> > +			return ret;
> > +
> >  		mutex_lock(&data->lock);
> >  		ret = mlx90614_iir_search(data->client,
> >  					  val * 100 + val2 / 10000);  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 25/28] iio: proximity: srf04: Use pm_runtime_resume_and_get() and handle error

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Previously this driver used pm_runtime_sync_get() and did not handle any
errors that occurred.  Moving to the new pm_runtime_resume_and_get() +
handle errors.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/proximity/srf04.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c
index 420c37c72de4..fe88b2bb60bc 100644
--- a/drivers/iio/proximity/srf04.c
+++ b/drivers/iio/proximity/srf04.c
@@ -100,9 +100,11 @@ static int srf04_read(struct srf04_data *data)
 	u64 dt_ns;
 	u32 time_ns, distance_mm;
 
-	if (data->gpiod_power)
-		pm_runtime_get_sync(data->dev);
-
+	if (data->gpiod_power) {
+		ret = pm_runtime_resume_and_get(data->dev);
+		if (ret < 0)
+			return ret;
+	}
 	/*
 	 * just one read-echo-cycle can take place at a time
 	 * ==> lock against concurrent reading calls
-- 
2.31.1

Re: [PATCH 25/28] iio: proximity: srf04: Use pm_runtime_resume_and_get() and handle error

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:51 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Previously this driver used pm_runtime_sync_get() and did not handle any
> errors that occurred.  Moving to the new pm_runtime_resume_and_get() +
> handle errors.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Andreas Klinger <ak@it-klinger.de>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/proximity/srf04.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c
> index 420c37c72de4..fe88b2bb60bc 100644
> --- a/drivers/iio/proximity/srf04.c
> +++ b/drivers/iio/proximity/srf04.c
> @@ -100,9 +100,11 @@ static int srf04_read(struct srf04_data *data)
>  	u64 dt_ns;
>  	u32 time_ns, distance_mm;
>  
> -	if (data->gpiod_power)
> -		pm_runtime_get_sync(data->dev);
> -
> +	if (data->gpiod_power) {
> +		ret = pm_runtime_resume_and_get(data->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>  	/*
>  	 * just one read-echo-cycle can take place at a time
>  	 * ==> lock against concurrent reading calls



Thanks,
Mauro

Re: [PATCH 25/28] iio: proximity: srf04: Use pm_runtime_resume_and_get() and handle error

[Jonathan Cameron]

On Wed, 12 May 2021 16:53:26 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:51 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Previously this driver used pm_runtime_sync_get() and did not handle any
> > errors that occurred.  Moving to the new pm_runtime_resume_and_get() +
> > handle errors.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Andreas Klinger <ak@it-klinger.de>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to see if we missed anything.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/proximity/srf04.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c
> > index 420c37c72de4..fe88b2bb60bc 100644
> > --- a/drivers/iio/proximity/srf04.c
> > +++ b/drivers/iio/proximity/srf04.c
> > @@ -100,9 +100,11 @@ static int srf04_read(struct srf04_data *data)
> >  	u64 dt_ns;
> >  	u32 time_ns, distance_mm;
> >  
> > -	if (data->gpiod_power)
> > -		pm_runtime_get_sync(data->dev);
> > -
> > +	if (data->gpiod_power) {
> > +		ret = pm_runtime_resume_and_get(data->dev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >  	/*
> >  	 * just one read-echo-cycle can take place at a time
> >  	 * ==> lock against concurrent reading calls  
> 
> 
> 
> Thanks,
> Mauro

[PATCH 26/28] iio: prox: pulsed-light-v2: Switch pm_runtime_resume_and_get()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Using this new call makes it easy to handle any errors as a result
of runtime resume as it exits without leaving the reference count
elevated.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
index c685f10b5ae4..ca8dc5cc209a 100644
--- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
+++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
@@ -154,7 +154,9 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
 	int tries = 10;
 	int ret;
 
-	pm_runtime_get_sync(&client->dev);
+	ret = pm_runtime_resume_and_get(&client->dev);
+	if (ret < 0)
+		return ret;
 
 	/* start sample */
 	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
-- 
2.31.1

Re: [PATCH 26/28] iio: prox: pulsed-light-v2: Switch pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:52 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Using this new call makes it easy to handle any errors as a result
> of runtime resume as it exits without leaving the reference count
> elevated.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> index c685f10b5ae4..ca8dc5cc209a 100644
> --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> @@ -154,7 +154,9 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>  	int tries = 10;
>  	int ret;
>  
> -	pm_runtime_get_sync(&client->dev);
> +	ret = pm_runtime_resume_and_get(&client->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	/* start sample */
>  	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);

There's a return just after this that it is missing a call to
pm_runtime_put_autosuspend():

	if (ret < 0) {
                dev_err(&client->dev, "cannot send start measurement command");
                return ret;
        }

Perhaps the best here would be to add a goto and a label before
the return logic.


Thanks,
Mauro

Re: [PATCH 26/28] iio: prox: pulsed-light-v2: Switch pm_runtime_resume_and_get()

[Jonathan Cameron]

On Wed, 12 May 2021 16:56:57 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:52 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Using this new call makes it easy to handle any errors as a result
> > of runtime resume as it exits without leaving the reference count
> > elevated.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > index c685f10b5ae4..ca8dc5cc209a 100644
> > --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > @@ -154,7 +154,9 @@ static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
> >  	int tries = 10;
> >  	int ret;
> >  
> > -	pm_runtime_get_sync(&client->dev);
> > +	ret = pm_runtime_resume_and_get(&client->dev);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	/* start sample */
> >  	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);  
> 
> There's a return just after this that it is missing a call to
> pm_runtime_put_autosuspend():
> 
> 	if (ret < 0) {
>                 dev_err(&client->dev, "cannot send start measurement command");
>                 return ret;
>         }
> 
> Perhaps the best here would be to add a goto and a label before
> the return logic.
Agreed.  I've added a precursor fix patch that does exactly that.

> 
> 
> Thanks,
> Mauro

[PATCH 27/28] iio: pressure: icp10100: Use pm_runtime_resume_and_get() + handle errors

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Using this new call makes it easy to handle failures in resume as it
doesn't hold a reference count if it exits with an error.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/pressure/icp10100.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/icp10100.c b/drivers/iio/pressure/icp10100.c
index 48759fc4bf18..80cd66688731 100644
--- a/drivers/iio/pressure/icp10100.c
+++ b/drivers/iio/pressure/icp10100.c
@@ -250,7 +250,9 @@ static int icp10100_get_measures(struct icp10100_state *st,
 	__be16 measures[3];
 	int ret;
 
-	pm_runtime_get_sync(&st->client->dev);
+	ret = pm_runtime_resume_and_get(&st->client->dev);
+	if (ret < 0)
+		return ret;
 
 	mutex_lock(&st->lock);
 	cmd = &icp10100_cmd_measure[st->mode];
-- 
2.31.1

Re: [PATCH 27/28] iio: pressure: icp10100: Use pm_runtime_resume_and_get() + handle errors

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:53 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Using this new call makes it easy to handle failures in resume as it
> doesn't hold a reference count if it exits with an error.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
>  drivers/iio/pressure/icp10100.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/icp10100.c b/drivers/iio/pressure/icp10100.c
> index 48759fc4bf18..80cd66688731 100644
> --- a/drivers/iio/pressure/icp10100.c
> +++ b/drivers/iio/pressure/icp10100.c
> @@ -250,7 +250,9 @@ static int icp10100_get_measures(struct icp10100_state *st,
>  	__be16 measures[3];
>  	int ret;
>  
> -	pm_runtime_get_sync(&st->client->dev);
> +	ret = pm_runtime_resume_and_get(&st->client->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	mutex_lock(&st->lock);
>  	cmd = &icp10100_cmd_measure[st->mode];


This one has a logic similar to a patch I commented earlier in this
series. At probing time, it does:

        pm_runtime_get_noresume(&client->dev);
        pm_runtime_set_active(&client->dev);
        pm_runtime_enable(&client->dev);
        pm_runtime_set_autosuspend_delay(&client->dev, 2000);
        pm_runtime_use_autosuspend(&client->dev);
        pm_runtime_put(&client->dev);
        ret = devm_add_action_or_reset(&client->dev, icp10100_pm_disable,
                                       &client->dev);


Here, get/put are balanced, but the icp10100_pm_disable logic is:

static void icp10100_pm_disable(void *data)
{
        struct device *dev = data;

        pm_runtime_put_sync_suspend(dev);
        pm_runtime_disable(dev);
}

Not sure what RPM get() it is trying to balance there.

Thanks,
Mauro

Re: [PATCH 27/28] iio: pressure: icp10100: Use pm_runtime_resume_and_get() + handle errors

[Jonathan Cameron]

On Wed, 12 May 2021 17:00:04 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:53 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Using this new call makes it easy to handle failures in resume as it
> > doesn't hold a reference count if it exits with an error.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> > ---
> >  drivers/iio/pressure/icp10100.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/pressure/icp10100.c b/drivers/iio/pressure/icp10100.c
> > index 48759fc4bf18..80cd66688731 100644
> > --- a/drivers/iio/pressure/icp10100.c
> > +++ b/drivers/iio/pressure/icp10100.c
> > @@ -250,7 +250,9 @@ static int icp10100_get_measures(struct icp10100_state *st,
> >  	__be16 measures[3];
> >  	int ret;
> >  
> > -	pm_runtime_get_sync(&st->client->dev);
> > +	ret = pm_runtime_resume_and_get(&st->client->dev);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	mutex_lock(&st->lock);
> >  	cmd = &icp10100_cmd_measure[st->mode];  
> 
> 
> This one has a logic similar to a patch I commented earlier in this
> series. At probing time, it does:
> 
>         pm_runtime_get_noresume(&client->dev);
>         pm_runtime_set_active(&client->dev);
>         pm_runtime_enable(&client->dev);
>         pm_runtime_set_autosuspend_delay(&client->dev, 2000);
>         pm_runtime_use_autosuspend(&client->dev);
>         pm_runtime_put(&client->dev);
>         ret = devm_add_action_or_reset(&client->dev, icp10100_pm_disable,
>                                        &client->dev);
> 
> 
> Here, get/put are balanced, but the icp10100_pm_disable logic is:
> 
> static void icp10100_pm_disable(void *data)
> {
>         struct device *dev = data;
> 
>         pm_runtime_put_sync_suspend(dev);
I'll drop this in v2.

Thanks,

Jonathan

>         pm_runtime_disable(dev);
> }
> 
> Not sure what RPM get() it is trying to balance there.
> 
> Thanks,
> Mauro

[PATCH 28/28] iio: adc: rcar-gyroadc: Use pm_runtime_resume_and_get() and check in probe()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

1 instance found using coccicheck script under review at:
https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
The other instance changed did not check for failure of the
pm_runtime_get_sync() so that is added.

Note the remaining pm_runtime_get_sync() call is left alone because it
is not obvious what to do on failure to power up in remove()

This is a prequel to taking a closer look at the runtime pm in IIO drivers
in general.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/iio/adc/rcar-gyroadc.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
index 9f38cf3c7dc2..a48895046408 100644
--- a/drivers/iio/adc/rcar-gyroadc.c
+++ b/drivers/iio/adc/rcar-gyroadc.c
@@ -162,18 +162,13 @@ static const struct iio_chan_spec rcar_gyroadc_iio_channels_3[] = {
 static int rcar_gyroadc_set_power(struct rcar_gyroadc *priv, bool on)
 {
 	struct device *dev = priv->dev;
-	int ret;
 
 	if (on) {
-		ret = pm_runtime_get_sync(dev);
-		if (ret < 0)
-			pm_runtime_put_noidle(dev);
+		return pm_runtime_resume_and_get(dev);
 	} else {
 		pm_runtime_mark_last_busy(dev);
-		ret = pm_runtime_put_autosuspend(dev);
+		return pm_runtime_put_autosuspend(dev);
 	}
-
-	return ret;
 }
 
 static int rcar_gyroadc_read_raw(struct iio_dev *indio_dev,
@@ -535,7 +530,10 @@ static int rcar_gyroadc_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		goto err_power_up;
+
 	rcar_gyroadc_hw_init(priv);
 	rcar_gyroadc_hw_start(priv);
 
@@ -552,6 +550,7 @@ static int rcar_gyroadc_probe(struct platform_device *pdev)
 err_iio_device_register:
 	rcar_gyroadc_hw_stop(priv);
 	pm_runtime_put_sync(dev);
+err_power_up:
 	pm_runtime_disable(dev);
 	pm_runtime_set_suspended(dev);
 	clk_disable_unprepare(priv->clk);
-- 
2.31.1

Re: [PATCH 28/28] iio: adc: rcar-gyroadc: Use pm_runtime_resume_and_get() and check in probe()

[Mauro Carvalho Chehab]

Em Sun,  9 May 2021 12:33:54 +0100
Jonathan Cameron <jic23@kernel.org> escreveu:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 1 instance found using coccicheck script under review at:
> https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> The other instance changed did not check for failure of the
> pm_runtime_get_sync() so that is added.
> 
> Note the remaining pm_runtime_get_sync() call is left alone because it
> is not obvious what to do on failure to power up in remove()
> 
> This is a prequel to taking a closer look at the runtime pm in IIO drivers
> in general.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>

LGTM.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  drivers/iio/adc/rcar-gyroadc.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
> index 9f38cf3c7dc2..a48895046408 100644
> --- a/drivers/iio/adc/rcar-gyroadc.c
> +++ b/drivers/iio/adc/rcar-gyroadc.c
> @@ -162,18 +162,13 @@ static const struct iio_chan_spec rcar_gyroadc_iio_channels_3[] = {
>  static int rcar_gyroadc_set_power(struct rcar_gyroadc *priv, bool on)
>  {
>  	struct device *dev = priv->dev;
> -	int ret;
>  
>  	if (on) {
> -		ret = pm_runtime_get_sync(dev);
> -		if (ret < 0)
> -			pm_runtime_put_noidle(dev);
> +		return pm_runtime_resume_and_get(dev);
>  	} else {
>  		pm_runtime_mark_last_busy(dev);
> -		ret = pm_runtime_put_autosuspend(dev);
> +		return pm_runtime_put_autosuspend(dev);
>  	}
> -
> -	return ret;
>  }
>  
>  static int rcar_gyroadc_read_raw(struct iio_dev *indio_dev,
> @@ -535,7 +530,10 @@ static int rcar_gyroadc_probe(struct platform_device *pdev)
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_enable(dev);
>  
> -	pm_runtime_get_sync(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		goto err_power_up;
> +
>  	rcar_gyroadc_hw_init(priv);
>  	rcar_gyroadc_hw_start(priv);
>  
> @@ -552,6 +550,7 @@ static int rcar_gyroadc_probe(struct platform_device *pdev)
>  err_iio_device_register:
>  	rcar_gyroadc_hw_stop(priv);
>  	pm_runtime_put_sync(dev);
> +err_power_up:
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
>  	clk_disable_unprepare(priv->clk);



Thanks,
Mauro

Re: [PATCH 28/28] iio: adc: rcar-gyroadc: Use pm_runtime_resume_and_get() and check in probe()

[Jonathan Cameron]

On Wed, 12 May 2021 17:01:40 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> wrote:

> Em Sun,  9 May 2021 12:33:54 +0100
> Jonathan Cameron <jic23@kernel.org> escreveu:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > 1 instance found using coccicheck script under review at:
> > https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
> > The other instance changed did not check for failure of the
> > pm_runtime_get_sync() so that is added.
> > 
> > Note the remaining pm_runtime_get_sync() call is left alone because it
> > is not obvious what to do on failure to power up in remove()
> > 
> > This is a prequel to taking a closer look at the runtime pm in IIO drivers
> > in general.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Marek Vasut <marek.vasut@gmail.com>  
> 
> LGTM.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Applied. Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/adc/rcar-gyroadc.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
> > index 9f38cf3c7dc2..a48895046408 100644
> > --- a/drivers/iio/adc/rcar-gyroadc.c
> > +++ b/drivers/iio/adc/rcar-gyroadc.c
> > @@ -162,18 +162,13 @@ static const struct iio_chan_spec rcar_gyroadc_iio_channels_3[] = {
> >  static int rcar_gyroadc_set_power(struct rcar_gyroadc *priv, bool on)
> >  {
> >  	struct device *dev = priv->dev;
> > -	int ret;
> >  
> >  	if (on) {
> > -		ret = pm_runtime_get_sync(dev);
> > -		if (ret < 0)
> > -			pm_runtime_put_noidle(dev);
> > +		return pm_runtime_resume_and_get(dev);
> >  	} else {
> >  		pm_runtime_mark_last_busy(dev);
> > -		ret = pm_runtime_put_autosuspend(dev);
> > +		return pm_runtime_put_autosuspend(dev);
> >  	}
> > -
> > -	return ret;
> >  }
> >  
> >  static int rcar_gyroadc_read_raw(struct iio_dev *indio_dev,
> > @@ -535,7 +530,10 @@ static int rcar_gyroadc_probe(struct platform_device *pdev)
> >  	pm_runtime_use_autosuspend(dev);
> >  	pm_runtime_enable(dev);
> >  
> > -	pm_runtime_get_sync(dev);
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> > +		goto err_power_up;
> > +
> >  	rcar_gyroadc_hw_init(priv);
> >  	rcar_gyroadc_hw_start(priv);
> >  
> > @@ -552,6 +550,7 @@ static int rcar_gyroadc_probe(struct platform_device *pdev)
> >  err_iio_device_register:
> >  	rcar_gyroadc_hw_stop(priv);
> >  	pm_runtime_put_sync(dev);
> > +err_power_up:
> >  	pm_runtime_disable(dev);
> >  	pm_runtime_set_suspended(dev);
> >  	clk_disable_unprepare(priv->clk);  
> 
> 
> 
> Thanks,
> Mauro