[PATCH V8 05/10] iio: imu: inv_icm42607: Add PM support for icm42607
Chris Morgan
macromorgan at hotmail.com
Thu May 21 13:42:38 PDT 2026
On Wed, May 20, 2026 at 06:13:53PM +0100, Jonathan Cameron wrote:
> On Mon, 18 May 2026 15:05:20 -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>
>
> Hi Chris, runtime PM is my current place to look closest
> for bugs because we've had a lot of them recently.
>
> Anyhow, took another look and here I think we can optimize
> things a little more.
>
> > ---
> > drivers/iio/imu/inv_icm42607/inv_icm42607.h | 18 +++
> > .../iio/imu/inv_icm42607/inv_icm42607_core.c | 147 ++++++++++++++++++
> > .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 1 +
> > .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 1 +
> > 4 files changed, 167 insertions(+)
> >
> > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > index 74d8d3d7c890..b05828415053 100644
> > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > @@ -10,6 +10,7 @@
> > #include <linux/bitops.h>
> > #include <linux/iio/iio.h>
> > #include <linux/mutex.h>
> > +#include <linux/pm.h>
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> >
> > @@ -96,24 +97,34 @@ struct inv_icm42607_hw {
> > const struct inv_icm42607_conf *conf;
> > };
> >
> > +struct inv_icm42607_suspended {
> > + enum inv_icm42607_sensor_mode gyro;
> > + enum inv_icm42607_sensor_mode accel;
> > + bool temp;
> > +};
> > +
> > /**
> > * struct inv_icm42607_state - driver state variables
> > * @lock: lock for serializing multiple registers access.
> > * @hw: Hardware specific data.
> > * @map: regmap pointer.
> > * @vddio_supply: I/O voltage regulator for the chip.
> > + * @vddio_en: I/O voltage status for runtime PM.
> > * @irq: chip irq, required to enable/disable and set wakeup
> > * @orientation: sensor chip orientation relative to main hardware.
> > * @conf: chip sensors configurations.
> > + * @suspended: suspended sensors configuration.
> > */
> > struct inv_icm42607_state {
> > struct mutex lock;
> > const struct inv_icm42607_hw *hw;
> > struct regmap *map;
> > struct regulator *vddio_supply;
> > + bool vddio_en;
> > int irq;
> > struct iio_mount_matrix orientation;
> > struct inv_icm42607_conf conf;
> > + struct inv_icm42607_suspended suspended;
>
> Is this used? If not bring it in only when needed.
>
> > };
>
> > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > index e9c81b52f9ef..bc0cefa2fb77 100644
> > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > @@ -9,6 +9,7 @@
> > #include <linux/irq.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > +#include <linux/pm_runtime.h>
> > #include <linux/property.h>
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> > @@ -72,6 +73,62 @@ 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 oldgyro = st->conf.gyro.mode;
> > + enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
> > + bool oldtemp = st->conf.temp_en;
> > + unsigned int sleepval;
> > + unsigned int val;
> > + int ret;
> > +
> > + if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
> > + return 0;
> > +
> > + val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> > + val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
> > + if (!temp)
> > + val |= INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL;
> > + 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;
> > +
> > + sleepval = 0;
> > + if (temp && !oldtemp) {
> > + if (sleepval < INV_ICM42607_TEMP_STARTUP_TIME_MS)
> > + sleepval = INV_ICM42607_TEMP_STARTUP_TIME_MS;
> sleepval = max(sleepval,)
> or just assign it here if not later patches add stuff in between
> the assignment to 0 and here.
I'm going to assign it to 0 here (unless you think I should define it
at the beginning as 0) and then tweak as needed. I think this code
here can be further optimized, especially if we make the assumption
that START and STOP time for each sensor is comparable (the datasheet
doesn't say, so I'm going to go with yes since that greatly simplifies
things).
> > + }
> > + if (accel != oldaccel && oldaccel == INV_ICM42607_SENSOR_MODE_OFF) {
> > + usleep_range(200, 300);
>
> fsleep() + add a comment on why we are sleeping in here.
>
Just going to delete these usleep_ranges since 1) I can't find them
documented in the datasheet and 2) we sleep later on anyway if this
condition is met.
> > + if (sleepval < INV_ICM42607_ACCEL_STARTUP_TIME_MS)
> > + sleepval = INV_ICM42607_ACCEL_STARTUP_TIME_MS;
> sleepval = max(sleepval, INV...);
>
> > + }
> > + if (gyro != oldgyro) {
> > + if (oldgyro == INV_ICM42607_SENSOR_MODE_OFF) {
> > + usleep_range(200, 300);
>
> fsleep()
>
> > + if (sleepval < INV_ICM42607_GYRO_STARTUP_TIME_MS)
> > + sleepval = INV_ICM42607_GYRO_STARTUP_TIME_MS;
> I'd use max for these as well.
>
> > + } else if (gyro == INV_ICM42607_SENSOR_MODE_OFF) {
> > + if (sleepval < INV_ICM42607_GYRO_STARTUP_TIME_MS)
> > + sleepval = INV_ICM42607_GYRO_STARTUP_TIME_MS;
> > + }
> > + }
> > +
> > + if (sleep_ms)
> > + *sleep_ms = sleepval;
> > + else if (sleepval)
> > + msleep(sleepval);
> > +
> > + return 0;
> > +}
>
> >
> > +static int inv_icm42607_suspend(struct device *dev)
> > +{
> > + struct inv_icm42607_state *st = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + guard(mutex)(&st->lock);
> > +
> Could you use pm_runtime_force_suspend()?
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> > + INV_ICM42607_SENSOR_MODE_OFF,
> > + false, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + inv_icm42607_disable_vddio_reg(st);
> > +
> > + return 0;
> > +}
> > +
> > +static int inv_icm42607_resume(struct device *dev)
> > +{
> > + struct inv_icm42607_state *st = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + guard(mutex)(&st->lock);
> > +
> Given the bunch of stuff we've run into recently around these
> I'm getting more paranoid.
> Similar to above, could you use pm_runtime_force_resume()
> You would need to gate stuff added later to not occur
> though if it wasn't runtime suspended.
This I'm having trouble understanding. If I use
pm_force_runtime_resume() I'm assuming that either I got an error (in
which case I'd return the error) or the device is runtime resumed
after the call completes. If that's the case, wouldn't my suspend and
resume steps just be pm_force_runtime_suspend/resume, and enabling the
regulator (first for resume) or disabling the regulator (last for
suspend) as needed?
>
>
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + ret = inv_icm42607_enable_vddio_reg(st);
> > + if (ret)
> > + return ret;
> > +
> > + /* Nothing else to restore at this time. */
> > +
> > + return 0;
> > +}
> > +
> > +static int inv_icm42607_runtime_suspend(struct device *dev)
> > +{
> > + struct inv_icm42607_state *st = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> > + INV_ICM42607_SENSOR_MODE_OFF, false,
> > + NULL);
> Different alignment to above - aim for consistent choices on this.
>
> > + if (ret)
> > + return ret;
> > +
> > + inv_icm42607_disable_vddio_reg(st);
> > +
> > + return 0;
> > +}
> > +
> > +static int inv_icm42607_runtime_resume(struct device *dev)
> > +{
> > + struct inv_icm42607_state *st = dev_get_drvdata(dev);
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + return inv_icm42607_enable_vddio_reg(st);
> > +}
> > +
> > +EXPORT_NS_GPL_DEV_PM_OPS(inv_icm42607_pm_ops, IIO_ICM42607) = {
> > + SYSTEM_SLEEP_PM_OPS(inv_icm42607_suspend, inv_icm42607_resume)
> > + RUNTIME_PM_OPS(inv_icm42607_runtime_suspend,
> > + inv_icm42607_runtime_resume, NULL)
> > +};
> > +
Thank you,
Chris
More information about the Linux-rockchip
mailing list