[PATCH V12 5/9] iio: imu: inv_icm42607: Add PM support for icm42607

[Chris Morgan]

On Mon, Jun 15, 2026 at 01:28:29PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 11, 2026 at 03:26:02PM -0500, Chris Morgan wrote:
> 
> > Add power management support for the ICM42607 device driver.
> 
> ...
> 
> > +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;
> 
> This validation seems weak, see below why.
> 
> > +	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> > +	val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
> > +	/*
> > +	 * Note that temp being enabled here doesn't affect PM since
> > +	 * per 10.25 of the datasheet the clock will be off by default
> > +	 * if both the gyro and accel modes are off.
> > +	 */
> > +	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_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);
> 
> Does it mean that it might be more than a single conditional to become true?
> If so, the last code wins, which is most likely buggy approach. Can you put
> a comment, and possible convert this to if-else-if, to clarify what's going
> on here?

I've changed this logic in the next version, but basically what needs
to be done is ensure if any sensor changes from off to on we wait the
startup delay amount of time for that sensor. If more than one sensor
changes from off to on, we wait the maximum of the two sensors delay.

I'm going to change this instead to just (sensor && !oldsensor) for
each of the three. Note that I also have to insert a shutdown delay
for the gyro to ensure if the state goes from on to off the sensor is
on a minimum of 45ms per the datasheet.

So I do expect it to be possible for more than one condition to be true
here, and when it is I want to select the maximum value for the sleep.
If the sleep_ms is specified however, we will use that value.

> 
> > +	if (sleep_ms)
> > +		*sleep_ms = sleepval_ms;
> > +	else if (sleepval_ms)
> > +		fsleep(sleepval_ms * USEC_PER_MSEC);
> > +
> > +	return 0;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Thank you,
Chris