linux-kernel - Re: [PATCH v3 3/3] iio: imu: bmi270: add support for motion events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250622144240.02845c0a@jic23-huawei>
Date: Sun, 22 Jun 2025 14:42:40 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Gustavo Silva <gustavograzs@...il.com>
Cc: Alex Lanzano <lanzano.alex@...il.com>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, Lothar Rubusch <l.rubusch@...il.com>
Subject: Re: [PATCH v3 3/3] iio: imu: bmi270: add support for motion events

On Mon, 16 Jun 2025 20:53:11 -0300
Gustavo Silva <gustavograzs@...il.com> 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@...il.com>

Hi Gustavo,

Main question in here is about the scaling of the motion threshold.
It seems to be based on units of g.  We use m/s^2 for IIO acceleration channels
and also the expectation is that threshold events match scaling of the main channels
(_scale applies to them).  Hence here we will need to do something a little
fiddly to maintain the threshold scale if the main channel scaling changes.

I'd missed this on earlier reviews but came to see that we have rubbish documentation
on this aspect when looking at some other changes recently.

I need to find time to both fix up the ABI docs and write a lot more explanatory
text around events. Sadly neither is appropriate activity for a sleepy Sunday afternoon!

Jonathan

> @@ -114,6 +134,10 @@
>  #define BMI270_STEP_COUNTER_FACTOR			20
>  #define BMI270_STEP_COUNTER_MAX				20460
>  
> +#define BMI270_INT_MICRO_TO_RAW(val, val2, scale) ((val) * (scale) + \
> +						  ((val2) * (scale)) / MEGA)
> +#define BMI270_RAW_TO_MICRO(raw, scale) ((((raw) % (scale)) * MEGA) / scale)

I'm struggling a bit with what this is doing.  Perhaps a comment?

> @@ -827,6 +977,20 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static IIO_CONST_ATTR(in_accel_value_available, "[0.0 0.00049 1.0]");
This aligns with below.  Scaling definitely shouldn't be in g and is likely more
complex because of the relationship expected with the overall channels scaling
controls.

> @@ -848,21 +1016,58 @@ static int bmi270_read_event_config(struct iio_dev *indio_dev,
>  				    enum iio_event_direction dir)
>  {

> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = bmi270_read_feature_reg(data, reg, &regval);
> +		if (ret)
> +			return ret;
> +
> +		raw = FIELD_GET(BMI270_FEAT_MOTION_THRESHOLD_MSK, regval);
> +		*val = raw / BMI270_MOTION_THRES_SCALE;
> +		*val2 = BMI270_RAW_TO_MICRO(raw, BMI270_MOTION_THRES_SCALE);

Why this particular scaling? Is effectively just dividing 1g / number of
values and hence providing a scaling to g?

Full scale is described as being 1G.
As this device is providing _RAW data for the relevant channels I'd
expect this to be scaled to match that. That will be a little fiddly here as
this is apparently always 0-1g whereas the scaling of the channel varies.
Thus irritatingly we'd need to adjust the scaling of this so it remains
consistent as the full scale changes.

The events ABI documentation is rather weak / wrong on this as it refers to
both _input_ and _raw_ thresholds whereas the true ABI just has one with
the scaling always being to match _raw if that is present. So _threshold * _scale
should be in the standards base units of m/s^2.

> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_EV_INFO_PERIOD:
> +		ret = bmi270_read_feature_reg(data, reg, &regval);
> +		if (ret)
> +			return ret;
> +
> +		raw = FIELD_GET(BMI270_FEAT_MOTION_DURATION_MSK, regval);
> +		*val = raw / BMI270_MOTION_DURAT_SCALE;
> +		*val2 = BMI270_RAW_TO_MICRO(raw, BMI270_MOTION_DURAT_SCALE);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}