On Fri, 8 Mar 2024 15:10:20 +0000
inv.git-commit@xxxxxxx wrote:
> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>
> WoM is a threshold test on accel value comparing actual sample
Very short wrap on this message. Wrap at 75 chars for commit messages.
> with previous one. It maps best to roc rising event.
> Add support of a new WOM sensor and functions for handling the
> corresponding roc_rising event. The event value is in SI units.
> Ensure WOM is stopped and restarted at suspend-resume, handle
> usage with buffer data ready interrupt, and handle change in
> sampling rate impacting already set roc value.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
Hi Jean-Baptiste
Some minor comments inline
Jonathan
> ---
> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 293 +++++++++++++++++-
> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 19 +-
> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 6 +-
> drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 14 +-
> 4 files changed, 319 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 0e94e5335e93..ad42be809f09 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -15,6 +15,7 @@
> +static u64 inv_mpu6050_convert_wom_to_roc(unsigned int threshold, unsigned int freq_div)
> +{
> + /* 4mg per LSB converted in m/s² in micro (1000000) */
> + const unsigned int convert = 4U * 9807U;
> + u64 value;
> +
> + value = threshold * convert;
> + /* compute the differential by multiplying by the frequency */
> + value = div_u64(value * INV_MPU6050_INTERNAL_FREQ_HZ, freq_div);
return div_u64();
> +
> + return value;
> +}
> +
> +static unsigned int inv_mpu6050_convert_roc_to_wom(u64 roc, unsigned int freq_div)
> +{
> + /* 4mg per LSB converted in m/s² in micro (1000000) */
> + const unsigned int convert = 4U * 9807U;
> + u64 value;
> + unsigned int threshold;
> +
> + /* return 0 only if roc is 0 */
> + if (roc == 0)
> + return 0;
> +
> + value = div_u64(roc * freq_div, convert * INV_MPU6050_INTERNAL_FREQ_HZ);
> + /* limit value to 8 bits and prevent 0*/
> + if (value == 0)
> + threshold = 1;
return min(255, max(1, value));
unless this open code flow is needed for later changes.
> + else if (value > 255)
> + threshold = 255;
> + else
> + threshold = value;
> +
> + return threshold;
> +}
> +
...
> +static int inv_mpu6050_set_wom_threshold(struct inv_mpu6050_state *st, u64 value,
> + unsigned int freq_div)
> +{
> + unsigned int threshold;
> + int result;
> +
> + /* convert roc to wom threshold and convert back to handle clipping */
> + threshold = inv_mpu6050_convert_roc_to_wom(value, freq_div);
> + value = inv_mpu6050_convert_wom_to_roc(threshold, freq_div);
> +
> + dev_dbg(regmap_get_device(st->map), "wom_threshold: 0x%x\n", threshold);
> +
> + switch (st->chip_type) {
> + case INV_ICM20609:
> + case INV_ICM20689:
> + case INV_ICM20600:
> + case INV_ICM20602:
> + case INV_ICM20690:
> + st->data[0] = threshold;
> + st->data[1] = threshold;
> + st->data[2] = threshold;
> + result = regmap_bulk_write(st->map, INV_ICM20609_REG_ACCEL_WOM_X_THR,
> + st->data, 3);
> + break;
> + default:
> + result = regmap_write(st->map, INV_MPU6500_REG_WOM_THRESHOLD, threshold);
> + break;
> + }
> + if (!result)
> + st->chip_config.roc_threshold = value;
if (result)
return result;
st->chip_config.roc_threshold = value;
return 0;
More code, but keeping all error handling out of line makes for easier code review.
> +
> + return result;
> +}
> +
> +static int inv_mpu6050_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct inv_mpu6050_state *st = iio_priv(indio_dev);
> + int enable;
> + int result;
> +
> + /* support only WoM (accel roc rising) event */
> + if (chan->type != IIO_ACCEL || type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
Long line - consider a line break. I don't think it will hurt readability
and I'd still prefer IIO mostly stays under 80 chars
> + return -EINVAL;
> +
> + enable = !!state;
> +
> + mutex_lock(&st->lock);
At some point we should switch to automated unlocking.
guard(mutex)(&st->lock);
as can do nice simple direct returns without the goto.
> +
> + if (st->chip_config.wom_en == enable) {
> + result = 0;
> + goto exit_unlock;
> + }
> +
> + result = inv_mpu6050_enable_wom(st, enable);
> +
> +exit_unlock:
> + mutex_unlock(&st->lock);
> + return result;
> +}
> +
> +static int inv_mpu6050_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int *val, int *val2)
> +{
> + struct inv_mpu6050_state *st = iio_priv(indio_dev);
> + u32 rem;
> +
> + /* support only WoM (accel roc rising) event value */
> + if (chan->type != IIO_ACCEL || type != IIO_EV_TYPE_ROC ||
> + dir != IIO_EV_DIR_RISING || info != IIO_EV_INFO_VALUE)
Odd alignment. dir should align with chan.
> + return -EINVAL;
> +
> + /* return value in micro */
> + *val = div_u64_rem(st->chip_config.roc_threshold, 1000000U, &rem);
> + *val2 = rem;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int inv_mpu6050_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int val, int val2)
> +{
> + struct inv_mpu6050_state *st = iio_priv(indio_dev);
> + struct device *pdev = regmap_get_device(st->map);
> + u64 value;
> + int result;
> +
> + /* support only WoM (accel roc rising) event value */
> + if (chan->type != IIO_ACCEL || type != IIO_EV_TYPE_ROC ||
> + dir != IIO_EV_DIR_RISING || info != IIO_EV_INFO_VALUE)
As above, odd alignment.
> + return -EINVAL;
> +
> + if (val < 0 || val2 < 0)
> + return -EINVAL;
> +
> + mutex_lock(&st->lock);
> +
> + result = pm_runtime_resume_and_get(pdev);
> + if (result)
> + goto exit_unlock;
> +
> + value = (u64)val * 1000000ULL + (u64)val2;
> + result = inv_mpu6050_set_wom_threshold(st, value, INV_MPU6050_FREQ_DIVIDER(st));
> +
> + pm_runtime_mark_last_busy(pdev);
> + pm_runtime_put_autosuspend(pdev);
> +
> +exit_unlock:
> + mutex_unlock(&st->lock);
> + return result;
> +}
> +
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 5950e2419ebb..19adccf388cf 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;
> + u64 roc_threshold;
No docs?
> };
>
