Re: [PATCH V7 05/11] iio: imu: inv_icm42607: Add PM support for icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 15 May 2026 08:00:10 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add power management support for the ICM42607 device driver.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>

Sashiko had a comment but I'd ignore it as it's about the rather
complex magic that lets us do various forms of PM configs without
lots of __maybe_unused etc and it works so don't touch it :)
I'm too lazy to check what the dependency ordering is other than
pm_ptr() is true in cases where pm_sleep_ptr() whereas the
opposite is not true and that means the actual dead code removal
occurs at a different point in the code those macros are producing.

Anyhow a few really minor things inline. Otherwise LGTM

> ---
>  drivers/iio/imu/inv_icm42607/inv_icm42607.h   |  12 ++
>  .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 143 ++++++++++++++++++
>  .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |   1 +
>  .../iio/imu/inv_icm42607/inv_icm42607_spi.c   |   1 +
>  4 files changed, 157 insertions(+)
> 
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> index 2c20e95b237a..5f37999e39a5 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 1088c5c7076f..e27ad0319a12 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

>  static int inv_icm42607_set_conf(struct inv_icm42607_state *st,
>  				 const struct inv_icm42607_conf *conf)
>  {
> @@ -198,11 +255,15 @@ static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st)
>  {
>  	int ret;
>  
> +	if (st->vddio_en)
> +		return 0;
> +
>  	ret = regulator_enable(st->vddio_supply);
>  	if (ret)
>  		return ret;
>  
>  	fsleep(INV_ICM42607_POWER_UP_TIME_US);
> +	st->vddio_en = 1;

It's a bool (which is good) so = true

>  
>  	return 0;
>  }
> @@ -211,7 +272,10 @@ static void inv_icm42607_disable_vddio_reg(void *_data)
>  {
>  	struct inv_icm42607_state *st = _data;
>  
> +	if (!st->vddio_en)
> +		return;
>  	regulator_disable(st->vddio_supply);
> +	st->vddio_en = 0;

= false

>  }

>  
> +static int inv_icm42607_suspend(struct device *dev)
> +{
> +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	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;

Trivial but blank line here

> +	inv_icm42607_disable_vddio_reg(st);
> +
> +	return 0;
> +}

> +
> +static int inv_icm42607_runtime_suspend(struct device *dev)
> +{
> +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> +	int ret = 0;

Assigned in all paths I think?  If that stops being true I'd probably still
introduce the assignment when it becomes the case even though it's a little
more code churn.

> +
> +	guard(mutex)(&st->lock);
> +
> +	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;
> +}