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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 14 Mar 2024 16:44:23 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> Hello Jonathan,
> 
> I already issued a V3 of this patch.
> 
> I have switched to guard(mutex) in the V3, but too bad I didn't get your comments sooner.
> Certainly, we could use BIT() macro, but I didn't want to change the other defines or have 1 BIT() in the middle. But do as you prefer.

It would need them all tidied up in a separate patch I think.

Jonathan

> 
> Thanks for reviewing V3 of the series and for your comments,
> JB
> 
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Thursday, March 14, 2024 15:52
> To: INV Git Commit <INV.git-commit@tdk.com>
> Cc: lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> Subject: Re: [PATCH v2 4/4] iio: imu: inv_mpu6050: add WoM suspend wakeup with low-power mode
>  
> This Message Is From an External Sender
> This message came from outside your organization.
>  
> 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,  
>