Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Jonathan Cameron]

On Thu, 20 Feb 2025 21:52:07 +0100
Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@xxxxxxxxxx> wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
> 
> When Wake-on-Motion is on, enable system wakeup and keep chip on for
> waking up system with interrupt.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
Hi Jean-Baptiste,

A few comments inline.

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h       |  2 +
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c |  3 +
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c  | 89 +++++++++++++++--------
>  3 files changed, 63 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -151,6 +151,7 @@ struct inv_icm42600_apex {
>   *  @map:		regmap pointer.
>   *  @vdd_supply:	VDD voltage regulator for the chip.
>   *  @vddio_supply:	I/O voltage regulator for the chip.
> + *  @irq:		chip irq.

Maybe say a little on what the variable is used for. It's not otherwise
obvious why we need it.  Also, does this chip really only have one irq line?
Looks like there are two. So the drivers should be fixed to cope with the
only one wired being irq2  unless I've found the wrong datasheet which is
certainly possible.


>   *  @orientation:	sensor chip orientation relative to main hardware.
>   *  @conf:		chip sensors configurations.
>   *  @suspended:		suspended sensors configuration.
> @@ -168,6 +169,7 @@ struct inv_icm42600_state {
>  	struct regmap *map;
>  	struct regulator *vdd_supply;
>  	struct regulator *vddio_supply;
> +	int irq;
>  	struct iio_mount_matrix orientation;
>  	struct inv_icm42600_conf conf;
>  	struct inv_icm42600_suspended suspended;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	/* accel events are wakeup capable */
> +	device_set_wakeup_capable(&indio_dev->dev, true);
> +
>  	return indio_dev;
>  }
>  
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
>  	mutex_init(&st->lock);
>  	st->chip = chip;
>  	st->map = regmap;
> +	st->irq = irq;
>  
>  	ret = iio_read_mount_matrix(dev, &st->orientation);
>  	if (ret) {
> @@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
>  static int inv_icm42600_suspend(struct device *dev)
>  {
>  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	struct device *accel_dev;
> +	bool wakeup;
> +	int accel_conf;
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);

As below. Pull these guard changes out as a precursor patch. That will make
it easier to spot the real changes here.

>  
>  	st->suspended.gyro = st->conf.gyro.mode;
>  	st->suspended.accel = st->conf.accel.mode;
>  	st->suspended.temp = st->conf.temp_en;
> -	if (pm_runtime_suspended(dev)) {
> -		ret = 0;
> -		goto out_unlock;
> -	}
> +	if (pm_runtime_suspended(dev))
> +		return 0;
>  
>  	/* disable FIFO data streaming */
>  	if (st->fifo.on) {
>  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  				   INV_ICM42600_FIFO_CONFIG_BYPASS);
>  		if (ret)
> -			goto out_unlock;
> +			return ret;
>  	}
>  
> -	/* disable APEX features */
> -	if (st->apex.wom.enable) {
> -		ret = inv_icm42600_set_wom(st, false);
> -		if (ret)
> -			goto out_unlock;
> +	/* keep chip on and wake-up capable if APEX and wakeup on */
> +	accel_dev = &st->indio_accel->dev;
> +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> +
> +	if (!wakeup) {
> +		/* disable APEX features and accel if wakeup disabled */
> +		if (st->apex.wom.enable) {
> +			ret = inv_icm42600_set_wom(st, false);
> +			if (ret)
> +				return ret;
> +		}
> +		accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> +	} else {
> +		/* keep accel on and setup irq for wakeup */
> +		accel_conf = st->conf.accel.mode;
> +		enable_irq_wake(st->irq);
> +		disable_irq(st->irq);
>  	}
>  
>  	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> -					 INV_ICM42600_SENSOR_MODE_OFF, false,
> -					 NULL);
> +					 accel_conf, false, NULL);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
> -	regulator_disable(st->vddio_supply);
> +	/* disable vddio regulator if chip is sleeping */
> +	if (!wakeup)
> +		regulator_disable(st->vddio_supply);
>  
> -out_unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /*
> @@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
>  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
>  	struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
>  	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> +	struct device *accel_dev;
> +	bool wakeup;
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);

Good change.  But separate patch as not related to most of what is going on here.


>  
> -	ret = inv_icm42600_enable_regulator_vddio(st);
> -	if (ret)
> -		goto out_unlock;
> +	/* check wakeup capability */
> +	accel_dev = &st->indio_accel->dev;
> +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> +
> +	/* restore vddio if cut off or irq state */
> +	if (!wakeup) {
> +		ret = inv_icm42600_enable_regulator_vddio(st);
> +		if (ret)
> +			return ret;
> +	} else {
> +		enable_irq(st->irq);
> +		disable_irq_wake(st->irq);
> +	}
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_active(dev);
> @@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
>  					 st->suspended.accel,
>  					 st->suspended.temp, NULL);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
> -	/* restore APEX features */
> -	if (st->apex.wom.enable) {
> -		ret = inv_icm42600_set_wom(st, true);
> -		if (ret)
> -			goto out_unlock;
> +	/* restore APEX features if disabled */
> +	if (!wakeup) {
> +		if (st->apex.wom.enable) {
> +			ret = inv_icm42600_set_wom(st, true);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	/* restore FIFO data streaming */
> @@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
>  		inv_sensors_timestamp_reset(&accel_st->ts);
>  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  				   INV_ICM42600_FIFO_CONFIG_STREAM);
> +		if (ret)
> +			return ret;
>  	}
>  
> -out_unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /* Runtime suspend will turn off sensors that are enabled by iio devices. */
>