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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Jun 26, 2026 at 11:12:26AM -0500, Chris Morgan wrote:

> Add power management support for the ICM42607 device driver.

...

> +/*
> + * Suspend delay assumed from other icm42600 series device, not
> + * documented in datasheet.
> + */
> +#define INV_ICM42607_SUSPEND_DELAY_MS			2000

Perhaps (2 * MSEC_PER_SEC) ?

...

> +static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
> +				      enum inv_icm42607_sensor_mode gyro,
> +				      enum inv_icm42607_sensor_mode accel)
> +{
> +	unsigned int oldaccel, oldgyro;

> +	unsigned int sleepval_us = 0;

It's discouraged to have assignment here, as it's too far from the actual use
and makes code harder to maintain and prone to subtle errors. See also below.

> +	unsigned int val;
> +	s64 disable_wait;
> +	int ret;
> +
> +	ret = regmap_read(st->map, INV_ICM42607_REG_PWR_MGMT0, &val);
> +	if (ret)
> +		return ret;
> +
> +	oldaccel = FIELD_GET(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, val);
> +	oldgyro = FIELD_GET(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, val);
> +
> +	if (gyro == oldgyro && accel == oldaccel)
> +		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 make sure at least 45ms have passed before power off and
> +	 * wait if it hasn't.
> +	 */
> +	if (!gyro && oldgyro) {
> +		disable_wait = ktime_us_delta(st->conf.gyro_stop,
> +					      ktime_get());

It's perfectly a single line.

		disable_wait = ktime_us_delta(st->conf.gyro_stop, ktime_get());

> +		disable_wait = clamp(disable_wait, 0,
> +				     INV_ICM42607_GYRO_STOP_TIME_US);

I would leave on a single line, or split logically, meaning moving 0 to the
next line:

		disable_wait = clamp(disable_wait,
				     0, INV_ICM42607_GYRO_STOP_TIME_US);

> +		fsleep(disable_wait);
> +	}

> +	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> +	val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);


Perhaps

	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro) |
	      FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);

which is slightly better to read in my opinion.

> +	ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * 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.
> +	 * The startup time for the temp sensor is considerably smaller
> +	 * than the startup time for the other sensors and one or more are
> +	 * required to be on for the temp sensor to function, so any start
> +	 * delay should be enough.
> +	 */

	sleepval_us = 0;

> +	if (accel && !oldaccel)
> +		sleepval_us = max(sleepval_us, INV_ICM42607_ACCEL_STARTUP_TIME_US);
> +
> +	if (gyro && !oldgyro) {
> +		sleepval_us = max(sleepval_us, INV_ICM42607_GYRO_STARTUP_TIME_US);
> +		/* Track the earliest we can turn off the gyroscope. */
> +		st->conf.gyro_stop = ktime_add_us(ktime_get(),
> +						  INV_ICM42607_GYRO_STOP_TIME_US);
> +	}
> +
> +	fsleep(sleepval_us);

The problem is that the reaction to 0 is a gray area. Some architecture
implementations might even complain on that.

On x86, for instance, udelay(0) actually does some (supposed to be small) delay.

But I think due to above check for gyro == oldgyro && accel == oldaccel this
won't happen. Please, add a comment on top of fsleep().

> +	return 0;
> +}

...

> +	dev_set_drvdata(dev, st);

This will require device.h to be included and hence the rest related headers
can be dropped at the same time (like dev_printk.h).

-- 
With Best Regards,
Andy Shevchenko