[PATCH v3 0/3] BMI270: Add support for step counter and motion events

[PATCH v3 0/3] BMI270: Add support for step counter and motion events

[Gustavo Silva]

This series adds support for step counter and motion events using
interrupts in the BMI270 driver.

The step counter can be enabled, disabled, and configured with a
watermark, all from userspace.

Any-motion and no-motion events are generated by detecting changes
in acceleration on each axis.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
Changes in v3:
- Avoid the usage of set_mask_bits() macro on 16-bit integers,
  use the bitwise operators instead
- Fix indentation, alignment and style issues
- Link to v2: https://lore.kernel.org/r/20250605-bmi270-events-v2-0-8b2c07d0c213@gmail.com

Changes in v2:
- Reduce the scope of mutex lock when clearing the step counter
- Change the type of the 'steps_enabled' variable from int to bool
- Add a new DMA safe variable to the device's private data to access the
  feature registers
- Remove unnecessary mutex lock
- Fix a build error found by the kernel test robot by initializing a
  local variable in the `bmi270_update_feature_reg()` function
- Remove dead code in the `bmi270_write_event_config()` function
- Add macro definitions and corresponding datasheet references for
  relevant constants: step counter maximum value, step counter factor,
  and threshold upper limit
- Remove the event bitmask from the device's private data. Read the
  registers directly to retrieve this information instead
- Use IIO_UNMOD_EVENT_CODE instead of IIO_MOD_EVENT_CODE where
  appropriate
- Fix shadowed error codes
- Change motion event to be enabled on a per-axis basis
- Create pseudo channel of type accel_x&y&z for the no-motion event
- Change no-motion event type to IIO_EV_TYPE_ROC
- Link to v1: https://lore.kernel.org/r/20250424-bmi270-events-v1-0-a6c722673e5f@gmail.com

---
Gustavo Silva (3):
      iio: imu: bmi270: add channel for step counter
      iio: imu: bmi270: add step counter watermark event
      iio: imu: bmi270: add support for motion events

 drivers/iio/imu/bmi270/bmi270_core.c | 603 ++++++++++++++++++++++++++++++++++-
 1 file changed, 600 insertions(+), 3 deletions(-)
---
base-commit: b475195fecc79a1a6e7fb0846aaaab0a1a4cb2e6
change-id: 20250424-bmi270-events-74c6ef5f4243

Best regards,
-- 
Gustavo Silva <gustavograzs@gmail.com>

[PATCH v3 1/3] iio: imu: bmi270: add channel for step counter

[Gustavo Silva]

Add a channel for enabling/disabling the step counter, reading the
number of steps and resetting the counter.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 drivers/iio/imu/bmi270/bmi270_core.c | 138 +++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index a86be5af5ccb1f010f2282ee31c47f284c1bcc86..c3cf65bb05fc2f6699e107110ee98f703aca3da3 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_1_REG				0x1d
 #define BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK		GENMASK(7, 6)
 
+#define BMI270_SC_OUT_0_REG				0x1e
+
 #define BMI270_INTERNAL_STATUS_REG			0x21
 #define BMI270_INTERNAL_STATUS_MSG_MSK			GENMASK(3, 0)
 #define BMI270_INTERNAL_STATUS_MSG_INIT_OK		0x01
@@ -39,6 +41,8 @@
 
 #define BMI270_TEMPERATURE_0_REG			0x22
 
+#define BMI270_FEAT_PAGE_REG				0x2f
+
 #define BMI270_ACC_CONF_REG				0x40
 #define BMI270_ACC_CONF_ODR_MSK				GENMASK(3, 0)
 #define BMI270_ACC_CONF_ODR_100HZ			0x08
@@ -90,6 +94,9 @@
 #define BMI270_PWR_CTRL_ACCEL_EN_MSK			BIT(2)
 #define BMI270_PWR_CTRL_TEMP_EN_MSK			BIT(3)
 
+#define BMI270_STEP_SC26_RST_CNT_MSK			BIT(10)
+#define BMI270_STEP_SC26_EN_CNT_MSK			BIT(12)
+
 /* See datasheet section 4.6.14, Temperature Sensor */
 #define BMI270_TEMP_OFFSET				11776
 #define BMI270_TEMP_SCALE				1953125
@@ -111,6 +118,7 @@ struct bmi270_data {
 	struct iio_trigger *trig;
 	 /* Protect device's private data from concurrent access */
 	struct mutex mutex;
+	bool steps_enabled;
 
 	/*
 	 * Where IIO_DMA_MINALIGN may be larger than 8 bytes, align to
@@ -120,6 +128,11 @@ struct bmi270_data {
 		__le16 channels[6];
 		aligned_s64 timestamp;
 	} buffer __aligned(IIO_DMA_MINALIGN);
+	/*
+	 * Variable to access feature registers. It can be accessed concurrently
+	 * with the 'buffer' variable
+	 */
+	__le16 regval __aligned(IIO_DMA_MINALIGN);
 };
 
 enum bmi270_scan {
@@ -282,6 +295,107 @@ static const struct  bmi270_odr_item bmi270_odr_table[] = {
 	},
 };
 
+enum bmi270_feature_reg_id {
+	BMI270_SC_26_REG,
+};
+
+struct bmi270_feature_reg {
+	u8 page;
+	u8 addr;
+};
+
+static const struct bmi270_feature_reg bmi270_feature_regs[] = {
+	[BMI270_SC_26_REG] = {
+		.page = 6,
+		.addr = 0x32,
+	},
+};
+
+static int bmi270_write_feature_reg(struct bmi270_data *data,
+				    enum bmi270_feature_reg_id id,
+				    u16 val)
+{
+	const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
+	int ret;
+
+	ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
+	if (ret)
+		return ret;
+
+	data->regval = cpu_to_le16(val);
+	return regmap_bulk_write(data->regmap, reg->addr, &data->regval,
+				 sizeof(data->regval));
+}
+
+static int bmi270_read_feature_reg(struct bmi270_data *data,
+				   enum bmi270_feature_reg_id id,
+				   u16 *val)
+{
+	const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
+	int ret;
+
+	ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap, reg->addr, &data->regval,
+			       sizeof(data->regval));
+	if (ret)
+		return ret;
+
+	*val = le16_to_cpu(data->regval);
+	return 0;
+}
+
+static int bmi270_update_feature_reg(struct bmi270_data *data,
+				     enum bmi270_feature_reg_id id,
+				     u16 mask, u16 val)
+{
+	u16 regval;
+	int ret;
+
+	ret = bmi270_read_feature_reg(data, id, &regval);
+	if (ret)
+		return ret;
+
+	regval = (regval & ~mask) | (val & mask);
+
+	return bmi270_write_feature_reg(data, id, regval);
+}
+
+static int bmi270_enable_steps(struct bmi270_data *data, int val)
+{
+	int ret;
+
+	guard(mutex)(&data->mutex);
+	if (data->steps_enabled)
+		return 0;
+
+	ret = bmi270_update_feature_reg(data, BMI270_SC_26_REG,
+					BMI270_STEP_SC26_EN_CNT_MSK,
+					FIELD_PREP(BMI270_STEP_SC26_EN_CNT_MSK,
+						   val ? 1 : 0));
+	if (ret)
+		return ret;
+
+	data->steps_enabled = true;
+	return 0;
+}
+
+static int bmi270_read_steps(struct bmi270_data *data, int *val)
+{
+	__le16 steps_count;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BMI270_SC_OUT_0_REG, &steps_count,
+			       sizeof(steps_count));
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(le16_to_cpu(steps_count), 15);
+	return IIO_VAL_INT;
+}
+
 static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
 {
 	int i;
@@ -551,6 +665,8 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
 	struct bmi270_data *data = iio_priv(indio_dev);
 
 	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		return bmi270_read_steps(data, val);
 	case IIO_CHAN_INFO_RAW:
 		if (!iio_device_claim_direct(indio_dev))
 			return -EBUSY;
@@ -571,6 +687,9 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		ret = bmi270_get_odr(data, chan->type, val, val2);
 		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_ENABLE:
+		*val = data->steps_enabled ? 1 : 0;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -596,6 +715,19 @@ static int bmi270_write_raw(struct iio_dev *indio_dev,
 		ret = bmi270_set_odr(data, chan->type, val, val2);
 		iio_device_release_direct(indio_dev);
 		return ret;
+	case IIO_CHAN_INFO_ENABLE:
+		return bmi270_enable_steps(data, val);
+	case IIO_CHAN_INFO_PROCESSED: {
+		if (val || !data->steps_enabled)
+			return -EINVAL;
+
+		guard(mutex)(&data->mutex);
+		/* Clear step counter value */
+		return bmi270_update_feature_reg(data, BMI270_SC_26_REG,
+						 BMI270_STEP_SC26_RST_CNT_MSK,
+						 FIELD_PREP(BMI270_STEP_SC26_RST_CNT_MSK,
+							    1));
+	}
 	default:
 		return -EINVAL;
 	}
@@ -698,6 +830,12 @@ static const struct iio_chan_spec bmi270_channels[] = {
 				      BIT(IIO_CHAN_INFO_OFFSET),
 		.scan_index = -1, /* No buffer support */
 	},
+	{
+		.type = IIO_STEPS,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) |
+				      BIT(IIO_CHAN_INFO_PROCESSED),
+		.scan_index = -1, /* No buffer support */
+	},
 	IIO_CHAN_SOFT_TIMESTAMP(BMI270_SCAN_TIMESTAMP),
 };
 

-- 
2.49.0

[PATCH v3 2/3] iio: imu: bmi270: add step counter watermark event

[Gustavo Silva]

Add support for generating events when the step counter reaches the
configurable watermark.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 drivers/iio/imu/bmi270/bmi270_core.c | 169 ++++++++++++++++++++++++++++++++++-
 1 file changed, 166 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index c3cf65bb05fc2f6699e107110ee98f703aca3da3..7b632ec5b0108328c216a815535502f0078eb151 100644
--- a/drivers/iio/imu/bmi270/bmi270_core.c
+++ b/drivers/iio/imu/bmi270/bmi270_core.c
@@ -8,6 +8,7 @@
 #include <linux/regmap.h>
 #include <linux/units.h>
 
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger.h>
@@ -28,6 +29,9 @@
 #define BMI270_ACCEL_X_REG				0x0c
 #define BMI270_ANG_VEL_X_REG				0x12
 
+#define BMI270_INT_STATUS_0_REG				0x1c
+#define BMI270_INT_STATUS_0_STEP_CNT_MSK		BIT(1)
+
 #define BMI270_INT_STATUS_1_REG				0x1d
 #define BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK		GENMASK(7, 6)
 
@@ -74,6 +78,10 @@
 #define BMI270_INT_LATCH_REG				0x55
 #define BMI270_INT_LATCH_REG_MSK			BIT(0)
 
+#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_DATA_REG				0x58
 #define BMI270_INT_MAP_DATA_DRDY_INT1_MSK		BIT(2)
 #define BMI270_INT_MAP_DATA_DRDY_INT2_MSK		BIT(6)
@@ -94,6 +102,7 @@
 #define BMI270_PWR_CTRL_ACCEL_EN_MSK			BIT(2)
 #define BMI270_PWR_CTRL_TEMP_EN_MSK			BIT(3)
 
+#define BMI270_STEP_SC26_WTRMRK_MSK			GENMASK(9, 0)
 #define BMI270_STEP_SC26_RST_CNT_MSK			BIT(10)
 #define BMI270_STEP_SC26_EN_CNT_MSK			BIT(12)
 
@@ -101,6 +110,10 @@
 #define BMI270_TEMP_OFFSET				11776
 #define BMI270_TEMP_SCALE				1953125
 
+/* See page 90 of datasheet. The step counter "holds implicitly a 20x factor" */
+#define BMI270_STEP_COUNTER_FACTOR			20
+#define BMI270_STEP_COUNTER_MAX				20460
+
 #define BMI260_INIT_DATA_FILE "bmi260-init-data.fw"
 #define BMI270_INIT_DATA_FILE "bmi270-init-data.fw"
 
@@ -396,6 +409,36 @@ static int bmi270_read_steps(struct bmi270_data *data, int *val)
 	return IIO_VAL_INT;
 }
 
+static int bmi270_int_map_reg(enum bmi270_irq_pin pin)
+{
+	switch (pin) {
+	case BMI270_IRQ_INT1:
+		return BMI270_INT1_MAP_FEAT_REG;
+	case BMI270_IRQ_INT2:
+		return BMI270_INT2_MAP_FEAT_REG;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
+{
+	int reg;
+
+	guard(mutex)(&data->mutex);
+	if (!data->steps_enabled)
+		return -EINVAL;
+
+	reg = bmi270_int_map_reg(data->irq_pin);
+	if (reg < 0)
+		return reg;
+
+	return regmap_update_bits(data->regmap, reg,
+				  BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
+				  FIELD_PREP(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
+					     state));
+}
+
 static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
 {
 	int i;
@@ -552,19 +595,31 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
 {
 	struct iio_dev *indio_dev = private;
 	struct bmi270_data *data = iio_priv(indio_dev);
-	unsigned int status;
+	unsigned int status0, status1;
+	s64 timestamp = iio_get_time_ns(indio_dev);
 	int ret;
 
 	scoped_guard(mutex, &data->mutex) {
+		ret = regmap_read(data->regmap, BMI270_INT_STATUS_0_REG,
+				  &status0);
+		if (ret)
+			return IRQ_NONE;
+
 		ret = regmap_read(data->regmap, BMI270_INT_STATUS_1_REG,
-				  &status);
+				  &status1);
 		if (ret)
 			return IRQ_NONE;
 	}
 
-	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status))
+	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_STEP_CNT_MSK, status0))
+		iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_STEPS, 0,
+							       IIO_EV_TYPE_CHANGE,
+							       IIO_EV_DIR_NONE),
+			       timestamp);
+
 	return IRQ_HANDLED;
 }
 
@@ -772,10 +827,116 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static int bmi270_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir, bool state)
+{
+	struct bmi270_data *data = iio_priv(indio_dev);
+
+	switch (type) {
+	case IIO_EV_TYPE_CHANGE:
+		return bmi270_step_wtrmrk_en(data, state);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi270_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	struct bmi270_data *data = iio_priv(indio_dev);
+	int ret, reg, regval;
+
+	guard(mutex)(&data->mutex);
+
+	switch (chan->type) {
+	case IIO_STEPS:
+		reg = bmi270_int_map_reg(data->irq_pin);
+		if (reg)
+			return reg;
+
+		ret = regmap_read(data->regmap, reg, &regval);
+		if (ret)
+			return ret;
+		return FIELD_GET(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
+				 regval) ? 1 : 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi270_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 bmi270_data *data = iio_priv(indio_dev);
+	unsigned int raw;
+
+	guard(mutex)(&data->mutex);
+
+	switch (type) {
+	case IIO_EV_TYPE_CHANGE:
+		if (!in_range(val, 0, BMI270_STEP_COUNTER_MAX + 1))
+			return -EINVAL;
+
+		raw = val / BMI270_STEP_COUNTER_FACTOR;
+		return bmi270_update_feature_reg(data, BMI270_SC_26_REG,
+						 BMI270_STEP_SC26_WTRMRK_MSK,
+						 FIELD_PREP(BMI270_STEP_SC26_WTRMRK_MSK,
+							    raw));
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi270_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 bmi270_data *data = iio_priv(indio_dev);
+	unsigned int raw;
+	u16 regval;
+	int ret;
+
+	guard(mutex)(&data->mutex);
+
+	switch (type) {
+	case IIO_EV_TYPE_CHANGE:
+		ret = bmi270_read_feature_reg(data, BMI270_SC_26_REG, &regval);
+		if (ret)
+			return ret;
+
+		raw = FIELD_GET(BMI270_STEP_SC26_WTRMRK_MSK, regval);
+		*val = raw * BMI270_STEP_COUNTER_FACTOR;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_event_spec bmi270_step_wtrmrk_event = {
+	.type = IIO_EV_TYPE_CHANGE,
+	.dir = IIO_EV_DIR_NONE,
+	.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) | BIT(IIO_EV_INFO_VALUE),
+};
+
 static const struct iio_info bmi270_info = {
 	.read_raw = bmi270_read_raw,
 	.write_raw = bmi270_write_raw,
 	.read_avail = bmi270_read_avail,
+	.write_event_config = bmi270_write_event_config,
+	.read_event_config = bmi270_read_event_config,
+	.write_event_value = bmi270_write_event_value,
+	.read_event_value = bmi270_read_event_value,
 };
 
 #define BMI270_ACCEL_CHANNEL(_axis) {				\
@@ -835,6 +996,8 @@ static const struct iio_chan_spec bmi270_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) |
 				      BIT(IIO_CHAN_INFO_PROCESSED),
 		.scan_index = -1, /* No buffer support */
+		.event_spec = &bmi270_step_wtrmrk_event,
+		.num_event_specs = 1,
 	},
 	IIO_CHAN_SOFT_TIMESTAMP(BMI270_SCAN_TIMESTAMP),
 };

-- 
2.49.0

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

[Gustavo Silva]

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, &regval);
+	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, &regval);
+	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, &regval);
-		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, &regval);
 		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, &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);
+		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;
+	}
 }
 
 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.0

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

[Andy Shevchenko]

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

Re: [PATCH v3 1/3] iio: imu: bmi270: add channel for step counter

[Jonathan Cameron]

On Mon, 16 Jun 2025 20:53:09 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> Add a channel for enabling/disabling the step counter, reading the
> number of steps and resetting the counter.
> 
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>

Hi.

Given this is ready to go and independent of later patches:
Applied to the togreg branch of iio.git and pushed out as testing
for 0-day to take a poke at it and see what we missed.

Thanks,

Jonathan

Re: [PATCH v3 2/3] iio: imu: bmi270: add step counter watermark event

[Jonathan Cameron]

On Mon, 16 Jun 2025 20:53:10 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> Add support for generating events when the step counter reaches the
> configurable watermark.
> 
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to see what we missed.

Thanks,

Jonathan

Re: [PATCH v3 1/3] iio: imu: bmi270: add channel for step counter

[Lothar Rubusch]

Hi Jonathan, thank you so much, but.. actually, I still prepared
actually still another v6. I was about to send it in today (just
finished tests). Might be still some bugfixes, and I guess if any
possible, I'd prefer v6 patches over v5.

Let me know, or I send it in anyway and you might consider v6? BTW
adxl345 also should follow later today. Already prepared, but going
over the tests..

Best,
L

On Sun, Jun 22, 2025 at 1:44 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 16 Jun 2025 20:53:09 -0300
> Gustavo Silva <gustavograzs@gmail.com> wrote:
>
> > Add a channel for enabling/disabling the step counter, reading the
> > number of steps and resetting the counter.
> >
> > Reviewed-by: Andy Shevchenko <andy@kernel.org>
> > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
>
> Hi.
>
> Given this is ready to go and independent of later patches:
> Applied to the togreg branch of iio.git and pushed out as testing
> for 0-day to take a poke at it and see what we missed.
>
> Thanks,
>
> Jonathan
>

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

[Jonathan Cameron]

On 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, &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;
> +	}

Re: [PATCH v3 1/3] iio: imu: bmi270: add channel for step counter

[Jonathan Cameron]

On Sun, 22 Jun 2025 14:11:22 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan, thank you so much, but.. actually, I still prepared
> actually still another v6. I was about to send it in today (just
> finished tests). Might be still some bugfixes, and I guess if any
> possible, I'd prefer v6 patches over v5.
> 
> Let me know, or I send it in anyway and you might consider v6? BTW
> adxl345 also should follow later today. Already prepared, but going
> over the tests..
> 
Series confusion?  This particular thread is about Gustavo's series
for the bmi270.

> Best,
> L
> 
> On Sun, Jun 22, 2025 at 1:44 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 16 Jun 2025 20:53:09 -0300
> > Gustavo Silva <gustavograzs@gmail.com> wrote:
> >  
> > > Add a channel for enabling/disabling the step counter, reading the
> > > number of steps and resetting the counter.
> > >
> > > Reviewed-by: Andy Shevchenko <andy@kernel.org>
> > > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>  
> >
> > Hi.
> >
> > Given this is ready to go and independent of later patches:
> > Applied to the togreg branch of iio.git and pushed out as testing
> > for 0-day to take a poke at it and see what we missed.
> >
> > Thanks,
> >
> > Jonathan
> >  
> 

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

[Gustavo Silva]

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, &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?
> 
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.
>