Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

[Jean-Baptiste Maneyrol <JManeyrol@invensense.com>]

Hello Jonathan,

there were 2 issues with suspend/resume when the device was already suspended by runtime pm.

When entering suspend, there was an error in logs because we were disabling vddio regulator although it was already disabled.
And when resuming, the chip was pull back to full power but the pm_runtime state was not updated. So it was believing it was still suspended.

Do you need a new patch with full description?

Thanks,
JB


From: Jonathan Cameron <jic23@kernel.org>

Sent: Sunday, April 5, 2020 15:15

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>

Subject: Re: [PATCH] iio: imu: inv_mpu6050: fix suspend/resume with runtime power

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Tue, 31 Mar 2020 15:38:50 +0200

Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:



> Suspend/resume were not working correctly with pm runtime.



Need more info than that!

Anyhow, when you say "not working correctly" what is happening that

is wrong?



Jonathan





> Now suspend check if the chip is already suspended, and

> resume put runtime pm in the correct state.

> 

> Fixes: 4599cac84614 ("iio: imu: inv_mpu6050: use runtime pm with autosuspend")

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

> ---

>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 11 ++++++++++-

>  1 file changed, 10 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> index e4b0d368c2f9..a58bab03f0b0 100644

> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> @@ -1636,6 +1636,10 @@ static int __maybe_unused inv_mpu_resume(struct device *dev)

>        if (result)

>                goto out_unlock;

>  

> +     pm_runtime_disable(dev);

> +     pm_runtime_set_active(dev);

> +     pm_runtime_enable(dev);

> +



Looking at the docs, we should do this if we were previously suspended and no longer

are.  Not sure we should do it we weren't previously in runtime suspend?



I guess it is idempotent anyway so if we were previously enabled we just set it again.

So probably fine.



Jonathan



>        result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);

>        if (result)

>                goto out_unlock;

> @@ -1657,13 +1661,18 @@ static int __maybe_unused inv_mpu_suspend(struct device *dev)

>  

>        mutex_lock(&st->lock);

>  

> +     st->suspended_sensors = 0;

> +     if (pm_runtime_suspended(dev)) {

> +             result = 0;

> +             goto out_unlock;

> +     }

> +

>        if (iio_buffer_enabled(indio_dev)) {

>                result = inv_mpu6050_prepare_fifo(st, false);

>                if (result)

>                        goto out_unlock;

>        }

>  

> -     st->suspended_sensors = 0;

>        if (st->chip_config.accl_en)

>                st->suspended_sensors |= INV_MPU6050_SENSOR_ACCL;

>        if (st->chip_config.gyro_en)