From: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Subject: Re: [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend
Date: Tue, 18 Feb 2020 16:44:36 +0000 [thread overview]
Message-ID: <CH2PR12MB418154CF0143B4DAC52E2C0DC4110@CH2PR12MB4181.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20200215182221.01f53e88@archlinux>
Hello,
responses are inline.
Hope text formatting is better.
JB
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, February 15, 2020 19:22
> To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
> Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
> Subject: Re: [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend
>
>
> On Wed, 12 Feb 2020 18:40:47 +0100
> Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
>
> > Use runtime power management for handling chip power and
> > sensor engines on/off. Simplifies things a lot since pm
> > runtime already has reference counter.
> > Usage of autosuspend reduces the number of power on/off. This
> > makes polling interface now usable to get data at low
> > frequency.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>
> Looks good. A couple of questions / suggestions inline.
>
> > ---
> > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 265 ++++++++++++------
> > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 5 +-
> > drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 29 +-
> > 3 files changed, 191 insertions(+), 108 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index f33fd04671cc..f698d2aa61f4 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > @@ -16,6 +16,8 @@
> > #include <linux/acpi.h>
> > #include <linux/platform_device.h>
> > #include <linux/regulator/consumer.h>
> > +#include <linux/pm.h>
> > +#include <linux/pm_runtime.h>
> > #include "inv_mpu_iio.h"
> > #include "inv_mpu_magn.h"
> >
> > @@ -400,26 +402,13 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
> > {
> > int result;
> >
> > - if (power_on) {
> > - if (!st->powerup_count) {
> > - result = inv_mpu6050_pwr_mgmt_1_write(st, false, -1, -1);
> > - if (result)
> > - return result;
> > - usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> > - INV_MPU6050_REG_UP_TIME_MAX);
> > - }
> > - st->powerup_count++;
> > - } else {
> > - if (st->powerup_count == 1) {
> > - result = inv_mpu6050_pwr_mgmt_1_write(st, true, -1, -1);
> > - if (result)
> > - return result;
> > - }
> > - st->powerup_count--;
> > - }
> > + result = inv_mpu6050_pwr_mgmt_1_write(st, !power_on, -1, -1);
> > + if (result)
> > + return result;
> >
> > - dev_dbg(regmap_get_device(st->map), "set power %d, count=%u\n",
> > - power_on, st->powerup_count);
> > + if (power_on)
> > + usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> > + INV_MPU6050_REG_UP_TIME_MAX);
> >
> > return 0;
> > }
> > @@ -563,6 +552,7 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
> > int *val)
> > {
> > struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > + struct device *pdev = regmap_get_device(st->map);
> > unsigned int freq_hz, period_us, min_sleep_us, max_sleep_us;
> > int result;
> > int ret;
> > @@ -571,92 +561,85 @@ 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 = inv_mpu6050_set_power_itg(st, true);
> > - if (result)
> > + result = pm_runtime_get_sync(pdev);
> > + if (result < 0) {
> > + pm_runtime_put_noidle(pdev);
> > return result;
> > + }
> >
> > switch (chan->type) {
> > case IIO_ANGL_VEL:
> > - result = inv_mpu6050_switch_engine(st, true,
> > - INV_MPU6050_SENSOR_GYRO);
> > - if (result)
> > - goto error_power_off;
> > - /* need to wait 2 periods to have first valid sample */
> > - min_sleep_us = 2 * period_us;
> > - max_sleep_us = 2 * (period_us + period_us / 2);
> > - usleep_range(min_sleep_us, max_sleep_us);
> > + if (!st->chip_config.gyro_en) {
>
> Are these changes due to runtime pm? Or do they make sense in general?
>
Yes, these changes are required for using efficiently the autosuspend feature
of runtime pm. We want to sample the data without turning the sensor on/off
each time. So if the sensor engine is already turned on, just read the sensor
data register. Without autosuspend it doesn't make sense, since the sensor are
turned off as soon as the data is read.
> > + result = inv_mpu6050_switch_engine(st, true,
> > + INV_MPU6050_SENSOR_GYRO);
> > + if (result)
> > + goto error_power_off;
> > + /* need to wait 2 periods to have first valid sample */
> > + min_sleep_us = 2 * period_us;
> > + max_sleep_us = 2 * (period_us + period_us / 2);
> > + usleep_range(min_sleep_us, max_sleep_us);
> > + }
> > ret = inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
> > chan->channel2, val);
> > - result = inv_mpu6050_switch_engine(st, false,
> > - INV_MPU6050_SENSOR_GYRO);
> > - if (result)
> > - goto error_power_off;
> > break;
> > case IIO_ACCEL:
> > - result = inv_mpu6050_switch_engine(st, true,
> > - INV_MPU6050_SENSOR_ACCL);
> > - if (result)
> > - goto error_power_off;
> > - /* wait 1 period for first sample availability */
> > - min_sleep_us = period_us;
> > - max_sleep_us = period_us + period_us / 2;
> > - usleep_range(min_sleep_us, max_sleep_us);
> > + if (!st->chip_config.accl_en) {
> > + result = inv_mpu6050_switch_engine(st, true,
> > + INV_MPU6050_SENSOR_ACCL);
> > + if (result)
> > + goto error_power_off;
> > + /* wait 1 period for first sample availability */
> > + min_sleep_us = period_us;
> > + max_sleep_us = period_us + period_us / 2;
> > + usleep_range(min_sleep_us, max_sleep_us);
> > + }
> > ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
> > chan->channel2, val);
> > - result = inv_mpu6050_switch_engine(st, false,
> > - INV_MPU6050_SENSOR_ACCL);
> > - if (result)
> > - goto error_power_off;
> > break;
> > case IIO_TEMP:
> > - result = inv_mpu6050_switch_engine(st, true,
> > - INV_MPU6050_SENSOR_TEMP);
> > - if (result)
> > - goto error_power_off;
> > - /* wait 1 period for first sample availability */
> > - min_sleep_us = period_us;
> > - max_sleep_us = period_us + period_us / 2;
> > - usleep_range(min_sleep_us, max_sleep_us);
> > + if (!st->chip_config.temp_en) {
> > + result = inv_mpu6050_switch_engine(st, true,
> > + INV_MPU6050_SENSOR_TEMP);
> > + if (result)
> > + goto error_power_off;
> > + /* wait 1 period for first sample availability */
> > + min_sleep_us = period_us;
> > + max_sleep_us = period_us + period_us / 2;
> > + usleep_range(min_sleep_us, max_sleep_us);
> > + }
> > ret = inv_mpu6050_sensor_show(st, st->reg->temperature,
> > IIO_MOD_X, val);
> > - result = inv_mpu6050_switch_engine(st, false,
> > - INV_MPU6050_SENSOR_TEMP);
> > - if (result)
> > - goto error_power_off;
> > break;
> > case IIO_MAGN:
> > - result = inv_mpu6050_switch_engine(st, true,
> > - INV_MPU6050_SENSOR_MAGN);
> > - if (result)
> > - goto error_power_off;
> > - /* frequency is limited for magnetometer */
> > - if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
> > - freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
> > - period_us = 1000000 / freq_hz;
> > + if (!st->chip_config.magn_en) {
> > + result = inv_mpu6050_switch_engine(st, true,
> > + INV_MPU6050_SENSOR_MAGN);
> > + if (result)
> > + goto error_power_off;
> > + /* frequency is limited for magnetometer */
> > + if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
> > + freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
> > + period_us = 1000000 / freq_hz;
> > + }
> > + /* need to wait 2 periods to have first valid sample */
> > + min_sleep_us = 2 * period_us;
> > + max_sleep_us = 2 * (period_us + period_us / 2);
> > + usleep_range(min_sleep_us, max_sleep_us);
> > }
> > - /* need to wait 2 periods to have first valid sample */
> > - min_sleep_us = 2 * period_us;
> > - max_sleep_us = 2 * (period_us + period_us / 2);
> > - usleep_range(min_sleep_us, max_sleep_us);
> > ret = inv_mpu_magn_read(st, chan->channel2, val);
> > - result = inv_mpu6050_switch_engine(st, false,
> > - INV_MPU6050_SENSOR_MAGN);
> > - if (result)
> > - goto error_power_off;
> > break;
> > default:
> > ret = -EINVAL;
> > break;
> > }
> >
> > - result = inv_mpu6050_set_power_itg(st, false);
> > - if (result)
> > - goto error_power_off;
> > + pm_runtime_mark_last_busy(pdev);
> > + pm_runtime_put_autosuspend(pdev);
> >
> > return ret;
> >
> > error_power_off:
> > - inv_mpu6050_set_power_itg(st, false);
> > + pm_runtime_put_autosuspend(pdev);
> > return result;
> > }
> >
> > @@ -795,6 +778,7 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> > int val, int val2, long mask)
> > {
> > struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > + struct device *pdev = regmap_get_device(st->map);
> > int result;
> >
> > /*
> > @@ -806,9 +790,11 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> > return result;
> >
> > mutex_lock(&st->lock);
> > - result = inv_mpu6050_set_power_itg(st, true);
> > - if (result)
> > + result = pm_runtime_get_sync(pdev);
> > + if (result < 0) {
> > + pm_runtime_put_noidle(pdev);
> > goto error_write_raw_unlock;
> > + }
> >
> > switch (mask) {
> > case IIO_CHAN_INFO_SCALE:
> > @@ -846,7 +832,8 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> > break;
> > }
> >
> > - result |= inv_mpu6050_set_power_itg(st, false);
> > + pm_runtime_mark_last_busy(pdev);
> > + pm_runtime_put_autosuspend(pdev);
> > error_write_raw_unlock:
> > mutex_unlock(&st->lock);
> > iio_device_release_direct_mode(indio_dev);
> > @@ -903,6 +890,7 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> > int result;
> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > + struct device *pdev = regmap_get_device(st->map);
> >
> > if (kstrtoint(buf, 10, &fifo_rate))
> > return -EINVAL;
> > @@ -920,9 +908,11 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> > result = 0;
> > goto fifo_rate_fail_unlock;
> > }
> > - result = inv_mpu6050_set_power_itg(st, true);
> > - if (result)
> > + result = pm_runtime_get_sync(pdev);
> > + if (result < 0) {
> > + pm_runtime_put_noidle(pdev);
> > goto fifo_rate_fail_unlock;
> > + }
> >
> > result = regmap_write(st->map, st->reg->sample_rate_div, d);
> > if (result)
> > @@ -938,8 +928,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> > if (result)
> > goto fifo_rate_fail_power_off;
> >
> > + pm_runtime_mark_last_busy(pdev);
> > fifo_rate_fail_power_off:
> > - result |= inv_mpu6050_set_power_itg(st, false);
> > + pm_runtime_put_autosuspend(pdev);
> > fifo_rate_fail_unlock:
> > mutex_unlock(&st->lock);
> > if (result)
> > @@ -1385,6 +1376,14 @@ static void inv_mpu_core_disable_regulator_action(void *_data)
> > inv_mpu_core_disable_regulator_vddio(st);
> > }
> >
> > +static void inv_mpu_pm_disable(void *data)
> > +{
> > + struct device *dev = data;
> > +
> > + pm_runtime_put_sync_suspend(dev);
> > + pm_runtime_disable(dev);
> > +}
> > +
> > int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> > int (*inv_mpu_bus_setup)(struct iio_dev *), int chip_type)
> > {
> > @@ -1409,7 +1408,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> > st = iio_priv(indio_dev);
> > mutex_init(&st->lock);
> > st->chip_type = chip_type;
> > - st->powerup_count = 0;
> > st->irq = irq;
> > st->map = regmap;
> >
> > @@ -1521,8 +1519,16 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> > goto error_power_off;
> > }
> >
> > - /* chip init is done, turning off */
> > - result = inv_mpu6050_set_power_itg(st, false);
> > + /* chip init is done, turning on runtime power management */
> > + result = pm_runtime_set_active(dev);
> > + if (result)
> > + goto error_power_off;
> > + pm_runtime_get_noresume(dev);
> > + pm_runtime_enable(dev);
> > + pm_runtime_set_autosuspend_delay(dev, INV_MPU6050_SUSPEND_DELAY_MS);
> > + pm_runtime_use_autosuspend(dev);
> > + pm_runtime_put(dev);
> > + result = devm_add_action_or_reset(dev, inv_mpu_pm_disable, dev);
> > if (result)
> > return result;
> >
> > @@ -1594,36 +1600,113 @@ EXPORT_SYMBOL_GPL(inv_mpu_core_probe);
> >
> > static int inv_mpu_resume(struct device *dev)
> > {
> > - struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > int result;
> >
> > mutex_lock(&st->lock);
> > +
> > result = inv_mpu_core_enable_regulator_vddio(st);
> > if (result)
> > goto out_unlock;
> >
> > result = inv_mpu6050_set_power_itg(st, true);
> > + if (result)
> > + goto out_unlock;
> > +
> > + result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);
> > + if (result)
> > + goto out_unlock;
> > +
> > + if (iio_buffer_enabled(indio_dev))
> > + result = inv_mpu6050_prepare_fifo(st, true);
> > +
> > out_unlock:
> > mutex_unlock(&st->lock);
> > -
>
> Not keen on random white space tweaks in a complex patch like this.
>
Oups, will be deleted in v2.
> > return result;
> > }
> >
> > static int inv_mpu_suspend(struct device *dev)
> > {
> > - struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > int result;
> >
> > mutex_lock(&st->lock);
> > +
> > + if (iio_buffer_enabled(indio_dev)) {
> > + result = inv_mpu6050_prepare_fifo(st, false);
> > + 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)
> > + st->suspended_sensors |= INV_MPU6050_SENSOR_GYRO;
> > + if (st->chip_config.temp_en)
> > + st->suspended_sensors |= INV_MPU6050_SENSOR_TEMP;
> > + if (st->chip_config.magn_en)
> > + st->suspended_sensors |= INV_MPU6050_SENSOR_MAGN;
> > + result = inv_mpu6050_switch_engine(st, false, st->suspended_sensors);
> > + if (result)
> > + goto out_unlock;
> > +
> > result = inv_mpu6050_set_power_itg(st, false);
> > + if (result)
> > + goto out_unlock;
> > +
> > inv_mpu_core_disable_regulator_vddio(st);
> > - mutex_unlock(&st->lock);
> >
> > +out_unlock:
> > + mutex_unlock(&st->lock);
> > return result;
> > }
> > #endif /* CONFIG_PM_SLEEP */
> >
> > -SIMPLE_DEV_PM_OPS(inv_mpu_pmops, inv_mpu_suspend, inv_mpu_resume);
> > +#ifdef CONFIG_PM
>
> Preferred to avoid the ifdef fun and mark the functions __maybe_unused
> instead. Avoids weird build issues that tend to happen around the
> CONFIG_PM options. Same for the CONFIG_PM_SLEEP cases above.
> Note this preference has changed over the years since this driver
> was introduced so entirely reasonable that we might want to update
> this now.
>
No problem. Will be fixed in v2.
> > +static int inv_mpu_runtime_suspend(struct device *dev)
> > +{
> > + struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > + unsigned int sensors;
> > + int ret;
> > +
> > + mutex_lock(&st->lock);
> > +
> > + sensors = INV_MPU6050_SENSOR_ACCL | INV_MPU6050_SENSOR_GYRO |
> > + INV_MPU6050_SENSOR_TEMP | INV_MPU6050_SENSOR_MAGN;
> > + ret = inv_mpu6050_switch_engine(st, false, sensors);
> > + if (ret)
> > + goto out_unlock;
> > +
> > + ret = inv_mpu6050_set_power_itg(st, false);
> > + if (ret)
> > + goto out_unlock;
> > +
> > + inv_mpu_core_disable_regulator_vddio(st);
> > +
> > +out_unlock:
> > + mutex_unlock(&st->lock);
> > + return ret;
> > +}
> > +
> > +static int inv_mpu_runtime_resume(struct device *dev)
> > +{
> > + struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > + int ret;
> > +
> > + ret = inv_mpu_core_enable_regulator_vddio(st);
> > + if (ret)
> > + return ret;
> > +
> > + return inv_mpu6050_set_power_itg(st, true);
> > +}
> > +#endif
> > +
> > +const struct dev_pm_ops inv_mpu_pmops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(inv_mpu_suspend, inv_mpu_resume)
> > + SET_RUNTIME_PM_OPS(inv_mpu_runtime_suspend, inv_mpu_runtime_resume, NULL)
> > +};
> > EXPORT_SYMBOL_GPL(inv_mpu_pmops);
> >
> > MODULE_AUTHOR("Invensense Corporation");
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > index e328c98e362c..cd38b3fccc7b 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > @@ -164,6 +164,7 @@ struct inv_mpu6050_hw {
> > * @magn_disabled: magnetometer disabled for backward compatibility reason.
> > * @magn_raw_to_gauss: coefficient to convert mag raw value to Gauss.
> > * @magn_orient: magnetometer sensor chip orientation if available.
> > + * @suspended_sensors: sensors mask of sensors turned off for suspend
> > */
> > struct inv_mpu6050_state {
> > struct mutex lock;
> > @@ -174,7 +175,6 @@ struct inv_mpu6050_state {
> > enum inv_devices chip_type;
> > struct i2c_mux_core *muxc;
> > struct i2c_client *mux_client;
> > - unsigned int powerup_count;
> > struct inv_mpu6050_platform_data plat_data;
> > struct iio_mount_matrix orientation;
> > struct regmap *map;
> > @@ -189,6 +189,7 @@ struct inv_mpu6050_state {
> > bool magn_disabled;
> > s32 magn_raw_to_gauss[3];
> > struct iio_mount_matrix magn_orient;
> > + unsigned int suspended_sensors;
> > };
> >
> > /*register and associated bit definition*/
> > @@ -312,6 +313,7 @@ struct inv_mpu6050_state {
> > #define INV_MPU6050_ACCEL_UP_TIME 20
> > #define INV_MPU6050_GYRO_UP_TIME 35
> > #define INV_MPU6050_GYRO_DOWN_TIME 150
> > +#define INV_MPU6050_SUSPEND_DELAY_MS 2000
> >
> > /* delay time in microseconds */
> > #define INV_MPU6050_REG_UP_TIME_MIN 5000
> > @@ -439,7 +441,6 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable);
> > int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
> > unsigned int mask);
> > int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 val);
> > -int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
> > int inv_mpu_acpi_create_mux_client(struct i2c_client *client);
> > void inv_mpu_acpi_delete_mux_client(struct i2c_client *client);
> > int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > index f53f50d08b9e..9eab39042443 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > @@ -3,6 +3,7 @@
> > * Copyright (C) 2012 Invensense, Inc.
> > */
> >
> > +#include <linux/pm_runtime.h>
> > #include "inv_mpu_iio.h"
> >
> > static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
> > @@ -156,41 +157,39 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
> > static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
> > {
> > struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > + struct device *pdev = regmap_get_device(st->map);
> > unsigned int scan;
> > int result;
> >
> > - scan = inv_scan_query(indio_dev);
> > -
> > if (enable) {
> > - result = inv_mpu6050_set_power_itg(st, true);
> > - if (result)
> > + scan = inv_scan_query(indio_dev);
> > + result = pm_runtime_get_sync(pdev);
> > + if (result < 0) {
> > + pm_runtime_put_noidle(pdev);
> > return result;
> > + }
> > + result = inv_mpu6050_switch_engine(st, false, ~scan);
> > + if (result)
> > + goto error_power_off;
>
> I'd like a comment on this block. I guess this off / on sequence is
> now necessary as we were previously relying on the power having been
> turned off?
>
Good guess. We need now to turn off sensor that we don't need in case
autosuspend didn't trigger. Will add a comment in v2.
> > result = inv_mpu6050_switch_engine(st, true, scan);
> > if (result)
> > goto error_power_off;
> > st->skip_samples = inv_compute_skip_samples(st);
> > result = inv_mpu6050_prepare_fifo(st, true);
> > if (result)
> > - goto error_sensors_off;
> > + goto error_power_off;
> > } else {
> > result = inv_mpu6050_prepare_fifo(st, false);
> > - if (result)
> > - goto error_sensors_off;
> > - result = inv_mpu6050_switch_engine(st, false, scan);
> > - if (result)
> > - goto error_power_off;
> > -
> > - result = inv_mpu6050_set_power_itg(st, false);
> > if (result)
> > goto error_power_off;
> > + pm_runtime_mark_last_busy(pdev);
> > + pm_runtime_put_autosuspend(pdev);
> > }
> >
> > return 0;
> >
> > -error_sensors_off:
> > - inv_mpu6050_switch_engine(st, false, scan);
> > error_power_off:
> > - inv_mpu6050_set_power_itg(st, false);
> > + pm_runtime_put_autosuspend(pdev);
> > return result;
> > }
> >
next prev parent reply other threads:[~2020-02-18 16:44 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once Jean-Baptiste Maneyrol
2020-02-15 17:30 ` Jonathan Cameron
2020-02-18 16:03 ` Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 02/13] iio: imu: inv_mpu6050: delete useless check Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 03/13] iio: imu: inv_mpu6050: set power on/off only once during all init Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 04/13] iio: imu: inv_mpu6050: simplify polling magnetometer Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 05/13] iio: imu: inv_mpu6050: early init of chip_config for use at setup Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 06/13] iio: imu: inv_mpu6050: add all signal path resets at init Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 07/13] iio: imu: inv_mpu6050: fix sleep time when turning regulators on Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 08/13] iio: imu: inv_mpu6050: rewrite power and engine management Jean-Baptiste Maneyrol
2020-02-15 18:09 ` Jonathan Cameron
2020-02-12 17:40 ` [PATCH 09/13] iio: imu: inv_mpu6050: fix data polling interface Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 10/13] iio: imu: inv_mpu6050: factorize fifo enable/disable Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 11/13] iio: imu: inv_mpu6050: dynamic sampling rate change Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend Jean-Baptiste Maneyrol
2020-02-15 18:22 ` Jonathan Cameron
2020-02-18 16:44 ` Jean-Baptiste Maneyrol [this message]
2020-02-12 17:40 ` [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro Jean-Baptiste Maneyrol
2020-02-15 18:23 ` Jonathan Cameron
2020-02-18 16:46 ` Jean-Baptiste Maneyrol
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CH2PR12MB418154CF0143B4DAC52E2C0DC4110@CH2PR12MB4181.namprd12.prod.outlook.com \
--to=jmaneyrol@invensense.com \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).