[PATCH V13 5/9] iio: imu: inv_icm42607: Add PM support for icm42607
Chris Morgan
macromorgan at hotmail.com
Wed Jun 24 10:04:42 PDT 2026
On Wed, Jun 24, 2026 at 04:06:51PM +0100, Jonathan Cameron wrote:
> On Tue, 23 Jun 2026 13:08:02 -0500
> Chris Morgan <macromorgan at hotmail.com> wrote:
>
> > On Mon, Jun 22, 2026 at 09:34:10PM -0500, Chris Morgan wrote:
> > > On Sun, Jun 21, 2026 at 06:19:48PM +0100, Jonathan Cameron wrote:
> > > > On Mon, 15 Jun 2026 12:25:48 -0500
> > > > Chris Morgan <macroalpha82 at gmail.com> wrote:
> > > >
> > > > > From: Chris Morgan <macromorgan at hotmail.com>
> > > > >
> > > > > Add power management support for the ICM42607 device driver.
> > > > >
> > > > > Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
> > > > A few things from taking a look at the sashiko report:
> > > > https://sashiko.dev/#/patchset/20260615172554.160910-1-macroalpha82%40gmail.com
> > > >
> > > > > ---
> > > > > drivers/iio/imu/inv_icm42607/inv_icm42607.h | 18 +++
> > > > > .../iio/imu/inv_icm42607/inv_icm42607_core.c | 139 ++++++++++++++++++
> > > > > .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 1 +
> > > > > .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 1 +
> > > > > 4 files changed, 159 insertions(+)
> > > > >
> > > > > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > > > > index a6a58571935f..4f4f541027dc 100644
> > > > > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > > > > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > > >
> > > > > @@ -334,11 +345,18 @@ struct inv_icm42607_state {
> > > > > #define INV_ICM42607_GYRO_STOP_TIME_MS 45
> > > > > #define INV_ICM42607_TEMP_STARTUP_TIME_MS 77
> > > > >
> > > > > +/*
> > > > > + * Suspend delay assumed from other icm42600 series device, not
> > > > > + * documented in datasheet.
> > > > > + */
> > > > > +#define INV_ICM42607_SUSPEND_DELAY_MS (2 * USEC_PER_MSEC)
> > > >
> > > > Sashiko had a valid comment on this. MSEC_PER_SEC seems more
> > > > appropriate given this is 2 seconds in milli seconds.
> > > >
> > > > > +
> > > > > typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
> > > > >
> > > > > extern const struct regmap_config inv_icm42607_regmap_config;
> > > > > extern const struct inv_icm42607_hw inv_icm42607_hw_data;
> > > > > extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
> > > > > +extern const struct dev_pm_ops inv_icm42607_pm_ops;
> > > > >
> > > > > int inv_icm42607_core_probe(struct regmap *regmap,
> > > > > const struct inv_icm42607_hw *hw,
> > > > > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > > index 4b8e19091786..64f5d263de4f 100644
> > > > > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > > @@ -4,6 +4,7 @@
> > > > > */
> > > > >
> > > > > #include <linux/bitfield.h>
> > > > > +#include <linux/cleanup.h>
> > > > > #include <linux/delay.h>
> > > > > #include <linux/dev_printk.h>
> > > > > #include <linux/device/devres.h>
> > > > > @@ -11,6 +12,7 @@
> > > > > #include <linux/iio/iio.h>
> > > > > #include <linux/module.h>
> > > > > #include <linux/mutex.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > > #include <linux/regmap.h>
> > > > > #include <linux/regulator/consumer.h>
> > > > > #include <linux/time.h>
> > > > > @@ -103,6 +105,63 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = {
> > > > > };
> > > > > EXPORT_SYMBOL_NS_GPL(inv_icm42607p_hw_data, "IIO_ICM42607");
> > > > >
> > > > > +static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
> > > > > + enum inv_icm42607_sensor_mode gyro,
> > > > > + enum inv_icm42607_sensor_mode accel,
> > > > > + bool temp, unsigned int *sleep_ms)
> > > > > +{
> > > > > + enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
> > > > > + enum inv_icm42607_sensor_mode oldgyro = st->conf.gyro.mode;
> > > > > + bool oldtemp = st->conf.temp_en;
> > > > > + unsigned int sleepval_ms;
> > > > > + unsigned int val;
> > > > > + int ret;
> > > > > +
> > > > > + if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
> > > > > + return 0;
> > > > > +
> > > > > + /*
> > > > > + * Datasheet on page 14.26 says we need to ensure the gyro sensor is on
> > > > > + * for a minimum of 45ms. So if we transition from an on state to an
> > > > > + * off state wait 45ms to ensure a sufficient pause before power off.
> > > >
> > > > Sashiko commented on this.. I think what we could do with adding to the
> > > > comment is what the path is that didn't pass through this function which would
> > > > ensure we have been on for 30 of this msecs already.
> > >
> > > I'm going to track whatever time the gyro started, and then if less
> > > than 45ms has elapsed just pause the remaining amount of time.
> > >
> > > >
> > > > > + */
> > > > > + if (!gyro && oldgyro)
> > > > > + fsleep(INV_ICM42607_GYRO_STOP_TIME_MS * USEC_PER_MSEC);
> > > > > +
> > > > > + val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> > > > > + val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
> > > > > + ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + st->conf.gyro.mode = gyro;
> > > > > + st->conf.accel.mode = accel;
> > > > > + st->conf.temp_en = temp;
> > > > > +
> > > > > + /*
> > > > > + * If a state change occurs from off to on, sleep for the startup
> > > > > + * time of the sensor, unless a sleep_ms is specified. Since more
> > > > > + * than one sensor can be transitioned from off to on, select the
> > > > > + * maximum time from each of the sensors changing from off to on.
> > > > > + */
> > > > > + sleepval_ms = 0;
> > > > > + if (temp && !oldtemp)
> > > > > + sleepval_ms = max(sleepval_ms, INV_ICM42607_TEMP_STARTUP_TIME_MS);
> > > > > +
> > > > > + if (accel && !oldaccel)
> > > > > + sleepval_ms = max(sleepval_ms, INV_ICM42607_ACCEL_STARTUP_TIME_MS);
> > > > > +
> > > > > + if (gyro && !oldgyro)
> > > > > + sleepval_ms = max(sleepval_ms, INV_ICM42607_GYRO_STARTUP_TIME_MS);
> > > > > +
> > > > > + if (sleep_ms)
> > > > > + *sleep_ms = sleepval_ms;
> > > > > + else if (sleepval_ms)
> > > > > + fsleep(sleepval_ms * USEC_PER_MSEC);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > >
> > > > > int inv_icm42607_core_probe(struct regmap *regmap,
> > > > > @@ -236,6 +305,8 @@ int inv_icm42607_core_probe(struct regmap *regmap,
> > > > > if (!st)
> > > > > return -ENOMEM;
> > > > >
> > > > > + dev_set_drvdata(dev, st);
> > > > > +
> > > > > ret = devm_mutex_init(dev, &st->lock);
> > > > > if (ret)
> > > > > return ret;
> > > > > @@ -271,10 +342,78 @@ int inv_icm42607_core_probe(struct regmap *regmap,
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > + ret = devm_pm_runtime_set_active_enabled(dev);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
> > > > > + pm_runtime_use_autosuspend(dev);
> > > > Sashiko does put out some stuff here. Please take a look and work out or
> > > > test if it is right (I think not but haven't checked that carefully!)
> > > > From a quick look I think that the auto disabling of autosuspend does a
> > > > rpm_idle() that should result in it suspending...
> > > >
> > >
> > > I see a few other drivers adding one more call to
> > > devm_pm_runtime_enable() so I'm going to see how that works out.
> >
> > Obviously that didn't work, but digging in much deeper into the PM code
> > I see that devm_pm_runtime_set_active_enabled() returns
> > devm_pm_runtime_enable(), which sets devm_add_action_or_reset() on
> > pm_runtime_disable_action(), which calls
> > pm_runtime_dont_use_autosuspend() and pm_runtime_disable(). Shouldn't
> > this work, or am I missing something? Basically when the driver
> > detaches shouldn't this chain end up calling
> > pm_runtime_dont_use_autosuspend()?
> Agreed. That was how I read things. The only thing I didn't entirely
> confirm (states involved were fiddly enough I'd want to check it
> with the actual driver!) was that pm_runtime_dont_use_autosuspend()
> ends up calling rpm_idle() in this case.
>
> Jonathan
I dropped in a dev_warn on the rpm_idle() and can confirm when I
unbound the driver it called rpm_idle(). So... yes?
Chris
>
> >
> > Thank you,
> > Chris
> >
> > >
> > > >
> > > > > +
> > > > > return 0;
> > > > > }
> > > > > EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");
> > >
> > > Thank you,
> > > Chris
>
More information about the Linux-rockchip
mailing list