Re: [PATCH v2 4/4] iio: imu: inv_mpu6050: add WoM suspend wakeup with low-power mode

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  8 Mar 2024 15:10:23 +0000
inv.git-commit@tdk.com wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> Add wakeup from suspend for WoM when enabled and put accel in
> low-power mode when suspended. Requires rewriting pwr_mgmt_1
> register handling and factorize out accel LPF settings.
> Use a low-power rate similar to the chip sampling rate but always
> lower for a best match of the sampling rate while saving power
> and adjust threshold to follow the required roc value.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
A few comments inline, but nothing significant that needs changes.
Not sure why this didn't send the other day - just found it still open :(

Jonathan

> ---

> +
> +static int inv_mpu6050_set_wom_lp(struct inv_mpu6050_state *st, bool on)

You could just split this given almost nothing shared between the two branches.

> +{
> +	unsigned int lp_div;
> +	int result;
> +
> +	if (on) {
> +		/* set low power ODR */
> +		result = inv_mpu6050_set_lp_odr(st, INV_MPU6050_FREQ_DIVIDER(st), &lp_div);
> +		if (result)
> +			return result;
> +		/* disable accel low pass filter */
> +		result = inv_mpu6050_set_accel_lpf_regs(st, INV_MPU6050_FILTER_NOLPF);
> +		if (result)
> +			return result;
> +		/* update wom threshold with new low-power frequency divider */
> +		result = inv_mpu6050_set_wom_threshold(st, st->chip_config.roc_threshold, lp_div);
> +		if (result)
> +			return result;
> +		/* set cycle mode */
> +		result = inv_mpu6050_pwr_mgmt_1_write(st, false, true, -1, -1);
> +	} else {
> +		/* disable cycle mode */
> +		result = inv_mpu6050_pwr_mgmt_1_write(st, false, false, -1, -1);
> +		if (result)
> +			return result;
> +		/* restore wom threshold */
> +		result = inv_mpu6050_set_wom_threshold(st, st->chip_config.roc_threshold,
> +						       INV_MPU6050_FREQ_DIVIDER(st));
> +		if (result)
> +			return result;
> +		/* restore accel low pass filter */
> +		result = inv_mpu6050_set_accel_lpf_regs(st, st->chip_config.lpf);
> +	}
> +
> +	return result;
> +}
> +
>  static int inv_mpu6050_enable_wom(struct inv_mpu6050_state *st, bool en)
>  {
>  	struct device *pdev = regmap_get_device(st->map);
> @@ -1847,6 +1933,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  			irq_type);
>  		return -EINVAL;
>  	}
> +	device_set_wakeup_capable(dev, true);
>  
>  	st->vdd_supply = devm_regulator_get(dev, "vdd");
>  	if (IS_ERR(st->vdd_supply))
> @@ -2012,16 +2099,27 @@ static int inv_mpu_resume(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	bool wakeup;
>  	int result;
>  
>  	mutex_lock(&st->lock);
A very good case for using guard(mutex)(&st->lock); but that can be a future series.

> -	result = inv_mpu_core_enable_regulator_vddio(st);
> -	if (result)
> -		goto out_unlock;
>  
> -	result = inv_mpu6050_set_power_itg(st, true);
> -	if (result)
> -		goto out_unlock;
> +	wakeup = device_may_wakeup(dev) && st->chip_config.wom_en;
> +
> +	if (wakeup) {
> +		enable_irq(st->irq);
> +		disable_irq_wake(st->irq);
> +		result = inv_mpu6050_set_wom_lp(st, false);
> +		if (result)
> +			goto out_unlock;
> +	} else {
> +		result = inv_mpu_core_enable_regulator_vddio(st);
> +		if (result)
> +			goto out_unlock;
> +		result = inv_mpu6050_set_power_itg(st, true);
> +		if (result)
> +			goto out_unlock;
> +	}
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_active(dev);
> @@ -2031,7 +2129,7 @@ static int inv_mpu_resume(struct device *dev)
>  	if (result)
>  		goto out_unlock;
>  
> -	if (st->chip_config.wom_en) {
> +	if (st->chip_config.wom_en && !wakeup) {
>  		result = inv_mpu6050_set_wom_int(st, true);
>  		if (result)
>  			goto out_unlock;
...

> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index e97a63ad2c31..6ba9d42b2537 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -304,6 +304,7 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_REG_PWR_MGMT_1          0x6B
>  #define INV_MPU6050_BIT_H_RESET             0x80
>  #define INV_MPU6050_BIT_SLEEP               0x40
> +#define INV_MPU6050_BIT_CYCLE               0x20
>  #define INV_MPU6050_BIT_TEMP_DIS            0x08

Side note but why don't we use BIT(x) for these?

>  #define INV_MPU6050_BIT_CLK_MASK            0x7
>  
> @@ -335,6 +336,7 @@ struct inv_mpu6050_state {
>  /* mpu6500 registers */
>  #define INV_MPU6500_REG_ACCEL_CONFIG_2      0x1D
>  #define INV_ICM20689_BITS_FIFO_SIZE_MAX     0xC0
> +#define INV_MPU6500_REG_LP_ODR              0x1E
>  #define INV_MPU6500_REG_WOM_THRESHOLD       0x1F
>  #define INV_MPU6500_REG_ACCEL_INTEL_CTRL    0x69
>  #define INV_MPU6500_BIT_ACCEL_INTEL_EN      BIT(7)
> @@ -451,6 +453,18 @@ enum inv_mpu6050_filter_e {
>  	NUM_MPU6050_FILTER
>  };
>  
> +enum inv_mpu6050_lposc_e {
> +	INV_MPU6050_LPOSC_4HZ = 4,
> +	INV_MPU6050_LPOSC_8HZ,
> +	INV_MPU6050_LPOSC_16HZ,
> +	INV_MPU6050_LPOSC_31HZ,
> +	INV_MPU6050_LPOSC_62HZ,
> +	INV_MPU6050_LPOSC_125HZ,
> +	INV_MPU6050_LPOSC_250HZ,
> +	INV_MPU6050_LPOSC_500HZ,
> +	NUM_MPU6050_LPOSC,

Trivial but no comma needed on a NUM type last entry as we'll never
add anything after it.

> +};
> +
>  /* IIO attribute address */
>  enum INV_MPU6050_IIO_ATTR_ADDR {
>  	ATTR_GYRO_MATRIX,