Re: [PATCH 1/4] iio: imu: inv_mpu6050: add WoM (Wake-on-Motion) sensor

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 25 Feb 2024 16:00:24 +0000
inv.git-commit@tdk.com wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> WoM is a threshold test on accel value comparing actual sample
> with previous one. It maps best to magnitude adaptive rising
> event.
> Add support of a new WOM sensor and functions for handling the
> corresponding mag_adaptive_rising event. The event value is in
> SI units. Ensure WOM is stopped and restarted at suspend-resume
> and handle usage with buffer data ready interrupt.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>

A few questions inline. Things don't seem to align perfectly with the
few datasheets I opened.

Jonathan



> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 5950e2419ebb..519c1eee96ad 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -88,11 +88,12 @@ enum inv_devices {
>  	INV_NUM_PARTS
>  };
>  
> -/* chip sensors mask: accelerometer, gyroscope, temperature, magnetometer */
> +/* chip sensors mask: accelerometer, gyroscope, temperature, magnetometer, WoM */
>  #define INV_MPU6050_SENSOR_ACCL		BIT(0)
>  #define INV_MPU6050_SENSOR_GYRO		BIT(1)
>  #define INV_MPU6050_SENSOR_TEMP		BIT(2)
>  #define INV_MPU6050_SENSOR_MAGN		BIT(3)
> +#define INV_MPU6050_SENSOR_WOM		BIT(4)
>  
>  /**
>   *  struct inv_mpu6050_chip_config - Cached chip configuration data.
> @@ -104,6 +105,7 @@ enum inv_devices {
>   *  @gyro_en:		gyro engine enabled
>   *  @temp_en:		temperature sensor enabled
>   *  @magn_en:		magn engine (i2c master) enabled
> + *  @wom_en:		Wake-on-Motion enabled
>   *  @accl_fifo_enable:	enable accel data output
>   *  @gyro_fifo_enable:	enable gyro data output
>   *  @temp_fifo_enable:	enable temp data output
> @@ -119,12 +121,14 @@ struct inv_mpu6050_chip_config {
>  	unsigned int gyro_en:1;
>  	unsigned int temp_en:1;
>  	unsigned int magn_en:1;
> +	unsigned int wom_en:1;
>  	unsigned int accl_fifo_enable:1;
>  	unsigned int gyro_fifo_enable:1;
>  	unsigned int temp_fifo_enable:1;
>  	unsigned int magn_fifo_enable:1;
>  	u8 divider;
>  	u8 user_ctrl;
> +	u8 wom_threshold;
>  };
>  
>  /*
> @@ -256,12 +260,14 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_REG_INT_ENABLE          0x38
>  #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>  #define INV_MPU6050_BIT_DMP_INT_EN          0x02
> +#define INV_MPU6500_BIT_WOM_INT_EN          (BIT(7) | BIT(6) | BIT(5))

GENMASK?  or build it up as I'm guessing those are the 3 axis?
However I opened the datasheet from the tdk website and only bit(6) is mentioned.

>  
>  #define INV_MPU6050_REG_RAW_ACCEL           0x3B
>  #define INV_MPU6050_REG_TEMPERATURE         0x41
>  #define INV_MPU6050_REG_RAW_GYRO            0x43
>  
>  #define INV_MPU6050_REG_INT_STATUS          0x3A
> +#define INV_MPU6500_BIT_WOM_INT             (BIT(7) | BIT(6) | BIT(5))

Likewise on this, the mpu6500 datasheet only mentions bit(6)

>  #define INV_MPU6050_BIT_FIFO_OVERFLOW_INT   0x10
>  #define INV_MPU6050_BIT_RAW_DATA_RDY_INT    0x01
>  
> @@ -301,6 +307,11 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_BIT_PWR_ACCL_STBY       0x38
>  #define INV_MPU6050_BIT_PWR_GYRO_STBY       0x07
>  
> +/* ICM20609 registers */
> +#define INV_ICM20609_REG_ACCEL_WOM_X_THR    0x20
> +#define INV_ICM20609_REG_ACCEL_WOM_Y_THR    0x21
> +#define INV_ICM20609_REG_ACCEL_WOM_Z_THR    0x22
This one does mention all 3 bits for the enable and status registers.
Perhaps there is more inter chip variation than currently covered?
I don't like writing reserved bits unless we have a clear statement
(and a comment here) that it is correct thing to do.


> +
>  /* ICM20602 register */
>  #define INV_ICM20602_REG_I2C_IF             0x70
>  #define INV_ICM20602_BIT_I2C_IF_DIS         0x40
> @@ -320,6 +331,10 @@ 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_WOM_THRESHOLD       0x1F
> +#define INV_MPU6500_REG_ACCEL_INTEL_CTRL    0x69
> +#define INV_MPU6500_BIT_ACCEL_INTEL_EN      BIT(7)
> +#define INV_MPU6500_BIT_ACCEL_INTEL_MODE    BIT(6)
>  #define INV_MPU6500_REG_ACCEL_OFFSET        0x77
>  
>  /* delay time in milliseconds */
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 66d4ba088e70..13da6f523ca2 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -33,10 +33,8 @@ static int inv_reset_fifo(struct iio_dev *indio_dev)
>  
>  reset_fifo_fail:
>  	dev_err(regmap_get_device(st->map), "reset fifo failed %d\n", result);
> -	result = regmap_write(st->map, st->reg->int_enable,
> -			      INV_MPU6050_BIT_DATA_RDY_EN);
> -
> -	return result;
> +	return regmap_update_bits(st->map, st->reg->int_enable,
> +			INV_MPU6050_BIT_DATA_RDY_EN, INV_MPU6050_BIT_DATA_RDY_EN);
>  }
>  
>  /*
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 676704f9151f..ec2398a87f45 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -134,11 +134,13 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
>  		ret = regmap_write(st->map, st->reg->user_ctrl, d);
>  		if (ret)
>  			return ret;
> -		/* enable interrupt */
> -		ret = regmap_write(st->map, st->reg->int_enable,
> -				   INV_MPU6050_BIT_DATA_RDY_EN);
> +		/* enable data interrupt */
> +		ret = regmap_update_bits(st->map, st->reg->int_enable,
> +				INV_MPU6050_BIT_DATA_RDY_EN, INV_MPU6050_BIT_DATA_RDY_EN);
>  	} else {
> -		ret = regmap_write(st->map, st->reg->int_enable, 0);
> +		/* disable data interrupt */
> +		ret = regmap_update_bits(st->map, st->reg->int_enable,
> +				INV_MPU6050_BIT_DATA_RDY_EN, 0);
>  		if (ret)
>  			return ret;
>  		ret = regmap_write(st->map, st->reg->fifo_en, 0);
> @@ -171,9 +173,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  			return result;
>  		/*
>  		 * In case autosuspend didn't trigger, turn off first not
> -		 * required sensors.
> +		 * required sensors excepted WoM
>  		 */
> -		result = inv_mpu6050_switch_engine(st, false, ~scan);
> +		result = inv_mpu6050_switch_engine(st, false, ~scan & ~INV_MPU6050_SENSOR_WOM);
>  		if (result)
>  			goto error_power_off;
>  		result = inv_mpu6050_switch_engine(st, true, scan);