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@gmail.com>
---
drivers/iio/imu/bmi270/bmi270_core.c | 316 +++++++++++++++++++++++++++++++++--
1 file changed, 306 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index 7b632ec5b0108328c216a815535502f0078eb151..e54fb5f7a87f6f88e7eba8f81a15f703e48031b5 100644
--- a/drivers/iio/imu/bmi270/bmi270_core.c
+++ b/drivers/iio/imu/bmi270/bmi270_core.c
@@ -31,6 +31,8 @@
#define BMI270_INT_STATUS_0_REG 0x1c
#define BMI270_INT_STATUS_0_STEP_CNT_MSK BIT(1)
+#define BMI270_INT_STATUS_0_NOMOTION_MSK BIT(5)
+#define BMI270_INT_STATUS_0_MOTION_MSK BIT(6)
#define BMI270_INT_STATUS_1_REG 0x1d
#define BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK GENMASK(7, 6)
@@ -81,6 +83,8 @@
#define BMI270_INT1_MAP_FEAT_REG 0x56
#define BMI270_INT2_MAP_FEAT_REG 0x57
#define BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK BIT(1)
+#define BMI270_INT_MAP_FEAT_NOMOTION_MSK BIT(5)
+#define BMI270_INT_MAP_FEAT_ANYMOTION_MSK BIT(6)
#define BMI270_INT_MAP_DATA_REG 0x58
#define BMI270_INT_MAP_DATA_DRDY_INT1_MSK BIT(2)
@@ -106,6 +110,22 @@
#define BMI270_STEP_SC26_RST_CNT_MSK BIT(10)
#define BMI270_STEP_SC26_EN_CNT_MSK BIT(12)
+#define BMI270_FEAT_MOTION_DURATION_MSK GENMASK(12, 0)
+#define BMI270_FEAT_MOTION_X_EN_MSK BIT(13)
+#define BMI270_FEAT_MOTION_Y_EN_MSK BIT(14)
+#define BMI270_FEAT_MOTION_Z_EN_MSK BIT(15)
+#define BMI270_FEAT_MOTION_XYZ_EN_MSK GENMASK(15, 13)
+#define BMI270_FEAT_MOTION_THRESHOLD_MSK GENMASK(10, 0)
+#define BMI270_FEAT_MOTION_OUT_CONF_MSK GENMASK(14, 11)
+#define BMI270_FEAT_MOTION_ENABLE_MSK BIT(15)
+
+#define BMI270_MOTION_XYZ_MSK GENMASK(2, 0)
+
+/* See pages 92 and 93 of the datasheet */
+#define BMI270_MOTION_THRES_SCALE GENMASK(10, 0)
+#define BMI270_MOTION_DURAT_SCALE 50
+#define BMI270_MOTION_DURAT_MAX 162
+
/* See datasheet section 4.6.14, Temperature Sensor */
#define BMI270_TEMP_OFFSET 11776
#define BMI270_TEMP_SCALE 1953125
@@ -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)
+
#define BMI260_INIT_DATA_FILE "bmi260-init-data.fw"
#define BMI270_INIT_DATA_FILE "bmi270-init-data.fw"
@@ -309,6 +333,13 @@ static const struct bmi270_odr_item bmi270_odr_table[] = {
};
enum bmi270_feature_reg_id {
+ /* Page 1 registers */
+ BMI270_ANYMO1_REG,
+ BMI270_ANYMO2_REG,
+ /* Page 2 registers */
+ BMI270_NOMO1_REG,
+ BMI270_NOMO2_REG,
+ /* Page 6 registers */
BMI270_SC_26_REG,
};
@@ -318,6 +349,22 @@ struct bmi270_feature_reg {
};
static const struct bmi270_feature_reg bmi270_feature_regs[] = {
+ [BMI270_ANYMO1_REG] = {
+ .page = 1,
+ .addr = 0x3c,
+ },
+ [BMI270_ANYMO2_REG] = {
+ .page = 1,
+ .addr = 0x3e,
+ },
+ [BMI270_NOMO1_REG] = {
+ .page = 2,
+ .addr = 0x30,
+ },
+ [BMI270_NOMO2_REG] = {
+ .page = 2,
+ .addr = 0x32,
+ },
[BMI270_SC_26_REG] = {
.page = 6,
.addr = 0x32,
@@ -439,6 +486,95 @@ static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
state));
}
+static int bmi270_anymotion_event_en(struct bmi270_data *data,
+ struct iio_chan_spec const *chan,
+ bool state)
+{
+ u16 axis_msk, axis_field_val, regval;
+ int ret, irq_reg;
+ bool axis_en;
+
+ irq_reg = bmi270_int_map_reg(data->irq_pin);
+ if (irq_reg < 0)
+ return irq_reg;
+
+ guard(mutex)(&data->mutex);
+
+ ret = bmi270_read_feature_reg(data, BMI270_ANYMO1_REG, ®val);
+ if (ret)
+ return ret;
+
+ switch (chan->channel2) {
+ case IIO_MOD_X:
+ axis_msk = BMI270_FEAT_MOTION_X_EN_MSK;
+ axis_field_val = FIELD_PREP(BMI270_FEAT_MOTION_X_EN_MSK, state);
+ axis_en = FIELD_GET(BMI270_FEAT_MOTION_Y_EN_MSK, regval) |
+ FIELD_GET(BMI270_FEAT_MOTION_Z_EN_MSK, regval);
+ break;
+ case IIO_MOD_Y:
+ axis_msk = BMI270_FEAT_MOTION_Y_EN_MSK;
+ axis_field_val = FIELD_PREP(BMI270_FEAT_MOTION_Y_EN_MSK, state);
+ axis_en = FIELD_GET(BMI270_FEAT_MOTION_X_EN_MSK, regval) |
+ FIELD_GET(BMI270_FEAT_MOTION_Z_EN_MSK, regval);
+ break;
+ case IIO_MOD_Z:
+ axis_msk = BMI270_FEAT_MOTION_Z_EN_MSK;
+ axis_field_val = FIELD_PREP(BMI270_FEAT_MOTION_Z_EN_MSK, state);
+ axis_en = FIELD_GET(BMI270_FEAT_MOTION_X_EN_MSK, regval) |
+ FIELD_GET(BMI270_FEAT_MOTION_Y_EN_MSK, regval);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = bmi270_update_feature_reg(data, BMI270_ANYMO1_REG, axis_msk,
+ axis_field_val);
+ if (ret)
+ return ret;
+
+ ret = bmi270_update_feature_reg(data, BMI270_ANYMO2_REG,
+ BMI270_FEAT_MOTION_ENABLE_MSK,
+ FIELD_PREP(BMI270_FEAT_MOTION_ENABLE_MSK,
+ state || axis_en));
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(data->regmap, irq_reg,
+ BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
+ FIELD_PREP(BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
+ state || axis_en));
+}
+
+static int bmi270_nomotion_event_en(struct bmi270_data *data, bool state)
+{
+ int ret, irq_reg;
+
+ irq_reg = bmi270_int_map_reg(data->irq_pin);
+ if (irq_reg < 0)
+ return irq_reg;
+
+ guard(mutex)(&data->mutex);
+
+ ret = bmi270_update_feature_reg(data, BMI270_NOMO1_REG,
+ BMI270_FEAT_MOTION_XYZ_EN_MSK,
+ FIELD_PREP(BMI270_FEAT_MOTION_XYZ_EN_MSK,
+ state ? BMI270_MOTION_XYZ_MSK : 0));
+ if (ret)
+ return ret;
+
+ ret = bmi270_update_feature_reg(data, BMI270_NOMO2_REG,
+ BMI270_FEAT_MOTION_ENABLE_MSK,
+ FIELD_PREP(BMI270_FEAT_MOTION_ENABLE_MSK,
+ state));
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(data->regmap, irq_reg,
+ BMI270_INT_MAP_FEAT_NOMOTION_MSK,
+ FIELD_PREP(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
+ state));
+}
+
static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
{
int i;
@@ -614,6 +750,20 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status1))
iio_trigger_poll_nested(data->trig);
+ if (FIELD_GET(BMI270_INT_STATUS_0_MOTION_MSK, status0))
+ iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_OR_Y_OR_Z,
+ IIO_EV_TYPE_MAG_ADAPTIVE,
+ IIO_EV_DIR_RISING),
+ timestamp);
+
+ if (FIELD_GET(BMI270_INT_STATUS_0_NOMOTION_MSK, status0))
+ iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_AND_Y_AND_Z,
+ IIO_EV_TYPE_ROC,
+ IIO_EV_DIR_RISING),
+ timestamp);
+
if (FIELD_GET(BMI270_INT_STATUS_0_STEP_CNT_MSK, status0))
iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_STEPS, 0,
IIO_EV_TYPE_CHANGE,
@@ -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]");
+
+static IIO_CONST_ATTR(in_accel_period_available, "[0.0 0.02 162.0]");
+
+static struct attribute *bmi270_event_attributes[] = {
+ &iio_const_attr_in_accel_value_available.dev_attr.attr,
+ &iio_const_attr_in_accel_period_available.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group bmi270_event_attribute_group = {
+ .attrs = bmi270_event_attributes,
+};
+
static int bmi270_write_event_config(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan,
enum iio_event_type type,
@@ -835,6 +999,10 @@ static int bmi270_write_event_config(struct iio_dev *indio_dev,
struct bmi270_data *data = iio_priv(indio_dev);
switch (type) {
+ case IIO_EV_TYPE_MAG_ADAPTIVE:
+ return bmi270_anymotion_event_en(data, chan, state);
+ case IIO_EV_TYPE_ROC:
+ return bmi270_nomotion_event_en(data, state);
case IIO_EV_TYPE_CHANGE:
return bmi270_step_wtrmrk_en(data, state);
default:
@@ -848,21 +1016,58 @@ static int bmi270_read_event_config(struct iio_dev *indio_dev,
enum iio_event_direction dir)
{
struct bmi270_data *data = iio_priv(indio_dev);
- int ret, reg, regval;
+ bool feat_en, axis_en;
+ unsigned int regval;
+ u16 motion_reg;
+ int ret, reg;
guard(mutex)(&data->mutex);
+ reg = bmi270_int_map_reg(data->irq_pin);
+ if (reg < 0)
+ return reg;
+
+ ret = regmap_read(data->regmap, reg, ®val);
+ if (ret)
+ return ret;
+
switch (chan->type) {
case IIO_STEPS:
- reg = bmi270_int_map_reg(data->irq_pin);
- if (reg)
- return reg;
-
- ret = regmap_read(data->regmap, reg, ®val);
- if (ret)
- return ret;
return FIELD_GET(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
regval) ? 1 : 0;
+ case IIO_ACCEL:
+ switch (type) {
+ case IIO_EV_TYPE_ROC:
+ return FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
+ regval) ? 1 : 0;
+ case IIO_EV_TYPE_MAG_ADAPTIVE:
+ ret = bmi270_read_feature_reg(data, BMI270_ANYMO1_REG,
+ &motion_reg);
+ if (ret)
+ return ret;
+
+ feat_en = FIELD_GET(BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
+ regval);
+ switch (chan->channel2) {
+ case IIO_MOD_X:
+ axis_en = FIELD_GET(BMI270_FEAT_MOTION_X_EN_MSK,
+ motion_reg);
+ break;
+ case IIO_MOD_Y:
+ axis_en = FIELD_GET(BMI270_FEAT_MOTION_Y_EN_MSK,
+ motion_reg);
+ break;
+ case IIO_MOD_Z:
+ axis_en = FIELD_GET(BMI270_FEAT_MOTION_Z_EN_MSK,
+ motion_reg);
+ break;
+ default:
+ return -EINVAL;
+ }
+ return (axis_en && feat_en) ? 1 : 0;
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
@@ -876,11 +1081,18 @@ static int bmi270_write_event_value(struct iio_dev *indio_dev,
int val, int val2)
{
struct bmi270_data *data = iio_priv(indio_dev);
- unsigned int raw;
+ unsigned int raw, mask, regval;
+ 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;
@@ -893,6 +1105,29 @@ 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);
+ mask = BMI270_FEAT_MOTION_THRESHOLD_MSK;
+ regval = FIELD_PREP(BMI270_FEAT_MOTION_THRESHOLD_MSK, raw);
+ return bmi270_update_feature_reg(data, reg, mask, regval);
+ 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);
+ mask = BMI270_FEAT_MOTION_DURATION_MSK;
+ regval = FIELD_PREP(BMI270_FEAT_MOTION_DURATION_MSK, raw);
+ return bmi270_update_feature_reg(data, reg, mask, regval);
+ default:
+ return -EINVAL;
+ }
}
static int bmi270_read_event_value(struct iio_dev *indio_dev,
@@ -904,12 +1139,18 @@ static int bmi270_read_event_value(struct iio_dev *indio_dev,
{
struct bmi270_data *data = iio_priv(indio_dev);
unsigned int raw;
+ int ret, reg;
u16 regval;
- int ret;
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:
ret = bmi270_read_feature_reg(data, BMI270_SC_26_REG, ®val);
if (ret)
@@ -921,6 +1162,29 @@ static int bmi270_read_event_value(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = bmi270_read_feature_reg(data, reg, ®val);
+ 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);
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_EV_INFO_PERIOD:
+ ret = bmi270_read_feature_reg(data, reg, ®val);
+ 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;
+ }
}
static const struct iio_event_spec bmi270_step_wtrmrk_event = {
@@ -929,6 +1193,20 @@ static const struct iio_event_spec bmi270_step_wtrmrk_event = {
.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) | 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),
+};
+
+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),
+};
+
static const struct iio_info bmi270_info = {
.read_raw = bmi270_read_raw,
.write_raw = bmi270_write_raw,
@@ -937,6 +1215,7 @@ static const struct iio_info bmi270_info = {
.read_event_config = bmi270_read_event_config,
.write_event_value = bmi270_write_event_value,
.read_event_value = bmi270_read_event_value,
+ .event_attrs = &bmi270_event_attribute_group,
};
#define BMI270_ACCEL_CHANNEL(_axis) { \
@@ -956,6 +1235,8 @@ static const struct iio_info bmi270_info = {
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
+ .event_spec = &bmi270_anymotion_event, \
+ .num_event_specs = 1, \
}
#define BMI270_ANG_VEL_CHANNEL(_axis) { \
@@ -1000,6 +1281,14 @@ static const struct iio_chan_spec bmi270_channels[] = {
.num_event_specs = 1,
},
IIO_CHAN_SOFT_TIMESTAMP(BMI270_SCAN_TIMESTAMP),
+ {
+ .type = IIO_ACCEL,
+ .modified = 1,
+ .channel2 = IIO_MOD_X_AND_Y_AND_Z,
+ .scan_index = -1, /* Fake channel */
+ .event_spec = &bmi270_nomotion_event,
+ .num_event_specs = 1,
+ },
};
static int bmi270_int_pin_config(struct bmi270_data *data,
@@ -1107,6 +1396,13 @@ static int bmi270_trigger_probe(struct bmi270_data *data,
return dev_err_probe(data->dev, ret,
"Trigger registration failed\n");
+ /* Disable axes for motion events */
+ ret = bmi270_update_feature_reg(data, BMI270_ANYMO1_REG,
+ BMI270_FEAT_MOTION_XYZ_EN_MSK,
+ FIELD_PREP(BMI270_FEAT_MOTION_XYZ_EN_MSK, 0));
+ if (ret)
+ return ret;
+
data->irq_pin = irq_pin;
return 0;
--
2.49.0On Mon, 16 Jun 2025 20:53:11 -0300
Gustavo Silva <gustavograzs@gmail.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@gmail.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, ®val);
> + 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, ®val);
> + 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;
> + }
On Sun, Jun 22, 2025 at 02:42:40PM +0100, Jonathan Cameron wrote:
> 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.
>
Hi Jonathan,
Thanks for the review.
>
> 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?
>
These macros convert register values to the range defined in
'in_accel_{period,value}_available' and vice versa.
The same macros are defined in the bmi323 driver, so I used them here
for the sake of consistency.
>
>
>
> > @@ -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, ®val);
> > + 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?
>
That's correct. In the bmi323 there's an actual scaling factor of 512 to
the register value. In the bmi270 the register value is not scaled.
> 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.
>
Thanks for the clarification. I'll work on that in v4.
>
On Tue, Jun 17, 2025 at 2:53 AM Gustavo Silva <gustavograzs@gmail.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.
...
> +#define BMI270_INT_MICRO_TO_RAW(val, val2, scale) ((val) * (scale) + \
> + ((val2) * (scale)) / MEGA)
Much easier to read and maintain when it's split logically, i.e.
#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)
Ditto for the sake of consistency with the above.
...
> + case IIO_ACCEL:
> + switch (type) {
> + case IIO_EV_TYPE_ROC:
> + return FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
> + regval) ? 1 : 0;
I would do it with logical split despite being longer (than 80) line
return
FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK, regval) ?
1 : 0;
Or even
return
!!FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK, regval);
if 1 is important here, or w/o !! if not.
> + case IIO_EV_TYPE_MAG_ADAPTIVE:
> + ret = bmi270_read_feature_reg(data, BMI270_ANYMO1_REG,
> + &motion_reg);
> + if (ret)
> + return ret;
> +
> + feat_en = FIELD_GET(BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
> + regval);
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + axis_en = FIELD_GET(BMI270_FEAT_MOTION_X_EN_MSK,
> + motion_reg);
> + break;
> + case IIO_MOD_Y:
> + axis_en = FIELD_GET(BMI270_FEAT_MOTION_Y_EN_MSK,
> + motion_reg);
> + break;
> + case IIO_MOD_Z:
> + axis_en = FIELD_GET(BMI270_FEAT_MOTION_Z_EN_MSK,
> + motion_reg);
> + break;
> + default:
> + return -EINVAL;
> + }
> + return (axis_en && feat_en) ? 1 : 0;
Do you expect boolean to be not 1 when it's true? IIRC this is part of
the C standard.
> + default:
> + return -EINVAL;
> + }
...
> + int ret, reg;
> u16 regval;
> - int ret;
Why? And why is reg signed? Also check other cases with reg and
semantically similar variables.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.