Re: [PATCH v2 3/3] iio: imu: bmi270: add support for motion events

[Jonathan Cameron]

On Thu, 05 Jun 2025 19:05:03 -0300
Gustavo Silva <gustavograzs@xxxxxxxxx> wrote:

> Any-motion event can be enabled on a per-axis basis and triggers a
> combined event when motion is detected on any axis.
> 
> No-motion event is triggered if the rate of change on all axes falls
> below a specified threshold for a configurable duration. A fake channel
> is used to report this event.
> 
> Threshold and duration can be configured from userspace.
> 
> Signed-off-by: Gustavo Silva <gustavograzs@xxxxxxxxx>
Hi Gustavo,

A few minor comments inline.  +CC Lothar given they have been doing a lot of
work with similar events recently and might have time to take a look at this
and see if I'm missing anything wrt to consistency with our recent discussions.

> ---
>  drivers/iio/imu/bmi270/bmi270_core.c | 318 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 309 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index 0798eb1da3ecc3cecaf7d7d47214bb07f4ec294f..eb0cada50087ccecfd5624a531692873e396deb6 100644
> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> +++ b/drivers/iio/imu/bmi270/bmi270_core.c

...


> @@ -881,10 +1086,17 @@ static int bmi270_write_event_value(struct iio_dev *indio_dev,
>  {
>  	struct bmi270_data *data = iio_priv(indio_dev);
>  	unsigned int raw;
> +	int reg;
>  
>  	guard(mutex)(&data->mutex);
>  
>  	switch (type) {
> +	case IIO_EV_TYPE_MAG_ADAPTIVE:
> +		reg = BMI270_ANYMO1_REG;
> +		break;
> +	case IIO_EV_TYPE_ROC:
> +		reg = BMI270_NOMO1_REG;
> +		break;
>  	case IIO_EV_TYPE_CHANGE:
>  		if (!in_range(val, 0, BMI270_STEP_COUNTER_MAX + 1))
>  			return -EINVAL;
> @@ -897,6 +1109,31 @@ static int bmi270_write_event_value(struct iio_dev *indio_dev,
>  	default:
>  		return -EINVAL;
>  	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (!in_range(val, 0, 1))
> +			return -EINVAL;
> +
> +		raw = BMI270_INT_MICRO_TO_RAW(val, val2,
> +					      BMI270_MOTION_THRES_SCALE);
> +		return bmi270_update_feature_reg(data, reg,
> +					 BMI270_FEAT_MOTION_THRESHOLD_MSK,
> +				FIELD_PREP(BMI270_FEAT_MOTION_THRESHOLD_MSK,
> +					raw));

This is some usual indenting.   Use a local variable for the value and
maybe the mask as well just to keep it readable.


> +	case IIO_EV_INFO_PERIOD:
> +		if (!in_range(val, 0, BMI270_MOTION_DURAT_MAX + 1))
> +			return -EINVAL;
> +
> +		raw = BMI270_INT_MICRO_TO_RAW(val, val2,
> +					BMI270_MOTION_DURAT_SCALE);
Align after (

> +		return bmi270_update_feature_reg(data, reg,
> +						BMI270_FEAT_MOTION_DURATION_MSK,
> +				FIELD_PREP(BMI270_FEAT_MOTION_DURATION_MSK,
> +					raw));
As above.

> +	default:
> +		return -EINVAL;
> +	}
>  }

>  
>  static const struct iio_event_spec bmi270_step_wtrmrk_event = {
> @@ -934,6 +1200,22 @@ static const struct iio_event_spec bmi270_step_wtrmrk_event = {
>  			       BIT(IIO_EV_INFO_VALUE),
>  };
>  
> +static const struct iio_event_spec bmi270_anymotion_event = {
> +	.type = IIO_EV_TYPE_MAG_ADAPTIVE,
> +	.dir = IIO_EV_DIR_RISING,
> +	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> +			       BIT(IIO_EV_INFO_PERIOD),
As below.
> +};
> +
> +static const struct iio_event_spec bmi270_nomotion_event = {
> +	.type = IIO_EV_TYPE_ROC,
> +	.dir = IIO_EV_DIR_RISING,
> +	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> +			       BIT(IIO_EV_INFO_PERIOD),
Could make that one line as I think it's 80 chars exactly.
I don't mind that much though.