On Thu, 14 Mar 2024 16:44:23 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx> 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@xxxxxxxxxx>
> Sent: Thursday, March 14, 2024 15:52
> To: INV Git Commit <INV.git-commit@xxxxxxx>
> Cc: lars@xxxxxxxxxx <lars@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx>
> 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@xxxxxxx wrote:
>
> > From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
> >
> > 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@xxxxxxx>
> 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,
>
