Re: [PATCH v3 6/8] iio: imu: inv_icm42607: Implement MREGx register access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Resource leak: The RC oscillator is forced on for MREG access but never explicitly turned off.
- [Medium] Unintended power state regression: default accelerometer mode for ICM42370 changed to LOW_POWER.
--

commit 5e054b5c4e067c471755e5889343bfd30cf9d57a
Author: Kanak Shilledar <kanak.shilledar@axis.com>

iio: imu: inv_icm42607: Implement MREGx register access

This commit implements indirect register access to different banks (MREGx)
for the inv_icm42607 IMU driver. This allows accessing registers configured
via the user and implementing buffer support. It also alters the default
accelerometer configuration for the ICM42370.

Datasheet: https://www.invensense.tdk.com/en-us/products/3-axis/icm-42370-p
Datasheet: https://www.lcsc.com/product-detail/C5129967.html

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index e77d72e0f7bc6..6e514b8682a0e 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -96,7 +99,7 @@ static const struct inv_icm42607_conf inv_icm42607_default_conf = {
>  static const struct inv_icm42607_conf inv_icm42370_default_conf = {
>  	.gyro = { },
>  	.accel = {
> -		.mode = INV_ICM42607_SENSOR_MODE_OFF,
> +		.mode = INV_ICM42607_SENSOR_MODE_LOW_POWER,

[Severity: Medium]
Does this unintentionally cause the accelerometer to consume power immediately
upon probe?

During driver probe, inv_icm42607_set_init_conf() writes this initial state
to the hardware. This undocumented change forces the accelerometer to turn on
immediately at boot. 

While runtime PM will eventually suspend the device and turn it off after a
2-second delay, it introduces an unnecessary power spike at boot and leaves
the software defaults in an inconsistent state.

>  		.fs = INV_ICM42607_ACCEL_FS_4G,
>  		.odr = INV_ICM42607_ODR_100HZ,
>  		.filter = INV_ICM42607_FILTER_BW_25HZ,
[ ... ]
> @@ -352,6 +355,107 @@ int inv_icm42607_set_sensor_conf(struct inv_icm42607_state *st,
>  	}
>  }
>  
> +static int inv_icm42607_mreg_check(struct inv_icm42607_state *st)
> +{
> +	struct regmap *map = st->map;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(map, INV_ICM42607_REG_MCLK_RDY, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & INV_ICM42607_MCLK_RDY_BIT)
> +		return 0;
> +
> +	/*
> +	 * Clock isn't running: we're either in Sleep mode or Accel LP
> +	 * mode with WUOSC. Force the RC oscillator on via IDLE, then
> +	 * wait for MCLK_RDY.
> +	 */
> +	ret = regmap_set_bits(map, INV_ICM42607_REG_PWR_MGMT0,
> +			       INV_ICM42607_PWR_MGMT0_IDLE);

[Severity: Medium]
Does this code leak power by leaving the RC oscillator running indefinitely?

inv_icm42607_mreg_check() sets the INV_ICM42607_PWR_MGMT0_IDLE bit to force
the RC oscillator on when accessing MREGs while the sensor is in OFF or
LOW_POWER mode. The datasheet explicitly requires this bit to be cleared
after MREG access is complete to save power.

Neither inv_icm42607_mreg_check() nor the read/write sequences clear this
bit. Although runtime PM autosuspend (after a 2-second delay) will
coincidentally overwrite PWR_MGMT0 and clear the bit, this still leaves the
oscillator running needlessly for 2 seconds per access, or indefinitely if
runtime PM is disabled, causing a persistent power leak.

> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * After setting the IDLE bit to 1 in PWR_MGMT0 register, wait for anywhere between
> +	 * 10us to 200us which are the ACCEL_STARTUP time and accelerometer transition time
> +	 * from OFF respectively.
> +	 */
> +	return regmap_read_poll_timeout(map, INV_ICM42607_REG_MCLK_RDY, val,
> +			val & INV_ICM42607_MCLK_RDY_BIT, 10, 200);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-inv_icm42370p-v3-0-77cc31642115@axis.com?part=6