[PATCH v5 0/4] BMI270: Add support for step counter and motion events

[PATCH v5 0/4] 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>
---
Sorry for the delay in sending v5.
As a reminder, patches 1 and 2 were already accepted in v3.

Changes in v5:
- BMI270_G_MACRO_M_S_2 -> BMI270_G_MICRO_M_S_2
- Add patch #4: ABI documentation for the exposed sysfs attributes
Link to v4: https://lore.kernel.org/r/20250711-bmi270-events-v4-3-53ec7da35046@gmail.com

Changes in v4:
- Reworked the threshold for the any-motion event so that threshold *
  accel_scale is given in m/s^2, in accordance with the ABI
  Also the range of available values for the threshold is now determined
  dynamically based on the value of the accelerometer scale
- Fixed alignment and styling of some statements and macros
- Simplified the return statement in the bmi270_read_event_config()
  function by returning the boolean expression directly
- Link to v3: https://lore.kernel.org/r/20250616-bmi270-events-v3-0-16e37588604f@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 (2):
  iio: imu: bmi270: add support for motion events
  iio: ABI: document accel and roc event attributes

 Documentation/ABI/testing/sysfs-bus-iio |  40 +++
 drivers/iio/imu/bmi270/bmi270_core.c    | 384 ++++++++++++++++++++++--
 2 files changed, 404 insertions(+), 20 deletions(-)


base-commit: 91812d3843409c235f336f32f1c37ddc790f1e03
-- 
2.51.0

[PATCH v5 3/4] 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 | 384 +++++++++++++++++++++++++--
 1 file changed, 364 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index 519f1c9d466d..714754160c2b 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,25 @@
 #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_FULL_SCALE			GENMASK(10, 0)
+#define BMI270_MOTION_DURAT_SCALE			50
+#define BMI270_MOTION_DURAT_MAX				162
+
+/* 9.81 * 1000000 m/s^2 */
+#define BMI270_G_MICRO_M_S_2				9810000
+
 /* See datasheet section 4.6.14, Temperature Sensor */
 #define BMI270_TEMP_OFFSET				11776
 #define BMI270_TEMP_SCALE				1953125
@@ -114,6 +137,11 @@
 #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 +337,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 +353,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 +490,121 @@ static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
 					     state));
 }
 
+static int bmi270_motion_reg(enum iio_event_type type, enum iio_event_info info)
+{
+	switch (info) {
+	case IIO_EV_INFO_PERIOD:
+		switch (type) {
+		case IIO_EV_TYPE_MAG_ADAPTIVE:
+			return BMI270_ANYMO1_REG;
+		case IIO_EV_TYPE_ROC:
+			return BMI270_NOMO1_REG;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_VALUE:
+		switch (type) {
+		case IIO_EV_TYPE_MAG_ADAPTIVE:
+			return BMI270_ANYMO2_REG;
+		case IIO_EV_TYPE_ROC:
+			return BMI270_NOMO2_REG;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+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;
@@ -479,8 +645,6 @@ static int bmi270_get_scale(struct bmi270_data *data, int chan_type, int *scale,
 	unsigned int val;
 	struct bmi270_scale_item bmi270_scale_item;
 
-	guard(mutex)(&data->mutex);
-
 	switch (chan_type) {
 	case IIO_ACCEL:
 		ret = regmap_read(data->regmap, BMI270_ACC_CONF_RANGE_REG, &val);
@@ -614,6 +778,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 +1005,40 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static ssize_t bmi270_show_accel_value_avail(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi270_data *data = iio_priv(indio_dev);
+	int ret, scale, uscale;
+	unsigned int step, max;
+
+	ret = bmi270_get_scale(data, IIO_ACCEL, &scale, &uscale);
+	if (ret)
+		return ret;
+
+	max = BMI270_G_MICRO_M_S_2 / uscale;
+	step = max / BMI270_MOTION_THRES_FULL_SCALE;
+
+	return sysfs_emit(buf, "[0 %u %u]\n", step, max);
+}
+
+static IIO_DEVICE_ATTR(in_accel_value_available, 0444,
+		       bmi270_show_accel_value_avail, NULL, 0);
+
+static IIO_CONST_ATTR(in_accel_period_available, "[0.0 0.02 162.0]");
+
+static struct attribute *bmi270_event_attributes[] = {
+	&iio_dev_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 +1047,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 +1064,57 @@ static int bmi270_read_event_config(struct iio_dev *indio_dev,
 				    enum iio_event_direction dir)
 {
 	struct bmi270_data *data = iio_priv(indio_dev);
+	bool feat_en, axis_en;
 	int ret, reg, regval;
+	u16 motion_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;
+		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;
 
-		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;
+			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;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -876,20 +1128,50 @@ 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 ret, reg, scale, uscale;
+	u64 tmp;
 
 	guard(mutex)(&data->mutex);
 
-	switch (type) {
-	case IIO_EV_TYPE_CHANGE:
+	if (type == 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));
+		mask = BMI270_STEP_SC26_WTRMRK_MSK;
+		regval = FIELD_PREP(BMI270_STEP_SC26_WTRMRK_MSK, raw);
+		return bmi270_update_feature_reg(data, BMI270_SC_26_REG, mask,
+						 regval);
+	}
+
+	reg = bmi270_motion_reg(type, info);
+	if (reg < 0)
+		return reg;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		ret = bmi270_get_scale(data, IIO_ACCEL, &scale, &uscale);
+		if (ret)
+			return ret;
+
+		if (!in_range(val, 0, (BMI270_G_MICRO_M_S_2 / uscale) + 1))
+			return -EINVAL;
+
+		tmp = (u64)val * BMI270_MOTION_THRES_FULL_SCALE * uscale;
+		raw = DIV_ROUND_CLOSEST_ULL(tmp, BMI270_G_MICRO_M_S_2);
+		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;
 	}
@@ -903,14 +1185,14 @@ static int bmi270_read_event_value(struct iio_dev *indio_dev,
 				   int *val, int *val2)
 {
 	struct bmi270_data *data = iio_priv(indio_dev);
+	int ret, reg, scale, uscale;
 	unsigned int raw;
 	u16 regval;
-	int ret;
+	u64 tmp;
 
 	guard(mutex)(&data->mutex);
 
-	switch (type) {
-	case IIO_EV_TYPE_CHANGE:
+	if (type == IIO_EV_TYPE_CHANGE) {
 		ret = bmi270_read_feature_reg(data, BMI270_SC_26_REG, &regval);
 		if (ret)
 			return ret;
@@ -918,6 +1200,36 @@ static int bmi270_read_event_value(struct iio_dev *indio_dev,
 		raw = FIELD_GET(BMI270_STEP_SC26_WTRMRK_MSK, regval);
 		*val = raw * BMI270_STEP_COUNTER_FACTOR;
 		return IIO_VAL_INT;
+	}
+
+	reg = bmi270_motion_reg(type, info);
+	if (reg < 0)
+		return reg;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		ret = bmi270_read_feature_reg(data, reg, &regval);
+		if (ret)
+			return ret;
+
+		ret = bmi270_get_scale(data, IIO_ACCEL, &scale, &uscale);
+		if (ret)
+			return ret;
+
+		raw = FIELD_GET(BMI270_FEAT_MOTION_THRESHOLD_MSK, regval);
+		tmp = (u64)raw * BMI270_G_MICRO_M_S_2;
+		*val = DIV_ROUND_CLOSEST_ULL(tmp,
+					     BMI270_MOTION_THRES_FULL_SCALE * uscale);
+		return IIO_VAL_INT;
+	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;
 	}
@@ -929,6 +1241,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 +1263,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 +1283,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 +1329,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 +1444,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.51.0

[PATCH v5 4/4] iio: ABI: document accel and roc event attributes

[Gustavo Silva]

Add accelerometer and rate of change event-related sysfs attributes
exposed by the bmi270 driver.

Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 40 +++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 2fb2cea4b192..75a88f0dc533 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -908,6 +908,7 @@ What:		/sys/.../iio:deviceX/events/in_accel_y_roc_rising_en
 What:		/sys/.../iio:deviceX/events/in_accel_y_roc_falling_en
 What:		/sys/.../iio:deviceX/events/in_accel_z_roc_rising_en
 What:		/sys/.../iio:deviceX/events/in_accel_z_roc_falling_en
+What:		/sys/.../iio:deviceX/events/in_accel_x&y&z_roc_rising_en
 What:		/sys/.../iio:deviceX/events/in_anglvel_x_roc_rising_en
 What:		/sys/.../iio:deviceX/events/in_anglvel_x_roc_falling_en
 What:		/sys/.../iio:deviceX/events/in_anglvel_y_roc_rising_en
@@ -1129,6 +1130,7 @@ Description:
 		will get activated once in_voltage0_raw goes above 1200 and will become
 		deactivated again once the value falls below 1150.
 
+What:		/sys/.../events/in_accel_roc_rising_value
 What:		/sys/.../events/in_accel_x_raw_roc_rising_value
 What:		/sys/.../events/in_accel_x_raw_roc_falling_value
 What:		/sys/.../events/in_accel_y_raw_roc_rising_value
@@ -1177,6 +1179,7 @@ Description:
 
 What:		/sys/.../events/in_accel_x_thresh_rising_period
 What:		/sys/.../events/in_accel_x_thresh_falling_period
+What:		/sys/.../events/in_accel_roc_rising_period
 What:		/sys/.../events/in_accel_x_roc_rising_period
 What:		/sys/.../events/in_accel_x_roc_falling_period
 What:		/sys/.../events/in_accel_y_thresh_rising_period
@@ -1187,6 +1190,7 @@ What:		/sys/.../events/in_accel_z_thresh_rising_period
 What:		/sys/.../events/in_accel_z_thresh_falling_period
 What:		/sys/.../events/in_accel_z_roc_rising_period
 What:		/sys/.../events/in_accel_z_roc_falling_period
+What:		/sys/.../events/in_accel_mag_adaptive_rising_period
 What:		/sys/.../events/in_anglvel_x_thresh_rising_period
 What:		/sys/.../events/in_anglvel_x_thresh_falling_period
 What:		/sys/.../events/in_anglvel_x_roc_rising_period
@@ -1344,6 +1348,23 @@ Description:
 		number or direction is not specified, applies to all channels of
 		this type.
 
+What:		/sys/.../iio:deviceX/events/in_accel_x_mag_adaptive_rising_en
+What:		/sys/.../iio:deviceX/events/in_accel_y_mag_adaptive_rising_en
+What:		/sys/.../iio:deviceX/events/in_accel_z_mag_adaptive_rising_en
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Similar to in_accel_x_thresh[_rising|_falling]_en, but here the
+		adaptive magnitude of the channel is compared to the threshold.
+
+What:		/sys/.../events/in_accel_mag_adaptive_rising_value
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The value to which the reference adaptive magnitude of the channel is
+		compared. If the axis is not specified, it applies to all channels
+		of this type.
+
 What:		/sys/.../iio:deviceX/events/in_accel_mag_referenced_en
 What:		/sys/.../iio:deviceX/events/in_accel_mag_referenced_rising_en
 What:		/sys/.../iio:deviceX/events/in_accel_mag_referenced_falling_en
@@ -2386,3 +2407,22 @@ Description:
 		Value representing the user's attention to the system expressed
 		in units as percentage. This usually means if the user is
 		looking at the screen or not.
+
+What:		/sys/.../events/in_accel_value_available
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		List of available acceleration threshold values that can be
+		configured for event generation. Units after application of
+		scale and offset are m/s^2. Expressed as:
+
+		- a range specified as "[min step max]"
+
+What:		/sys/.../events/in_accel_period_available
+KernelVersion:	6.18
+Contact:	linux-iio@vger.kernel.org
+Description:
+		List of available periods for accelerometer event detection in
+		seconds, expressed as:
+
+		- a range specified as "[min step max]"
-- 
2.51.0

Re: [PATCH v5 0/4] BMI270: Add support for step counter and motion events

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 2:58 PM Gustavo Silva <gustavograzs@gmail.com> wrote:
>
> 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.
>

> Sorry for the delay in sending v5.
> As a reminder, patches 1 and 2 were already accepted in v3.

Right, and this series has confusing patch numbering. Please, make
sure you send all the patches that are mentioned in the numbers, or
correct numbers. I even don't know if `b4` by default can handle this
(yes, it can if one manually passes the numbers of the patches to
take).

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 2:58 PM 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)
> +#define BMI270_RAW_TO_MICRO(raw, scale) \
> +       ((((raw) % (scale)) * MEGA) / scale)

In the macro names "MICRO" in the implementation "MEGA", please make
this consistent.

...

> +static ssize_t bmi270_show_accel_value_avail(struct device *dev,
> +                                            struct device_attribute *attr,
> +                                            char *buf)
> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct bmi270_data *data = iio_priv(indio_dev);
> +       int ret, scale, uscale;
> +       unsigned int step, max;
> +
> +       ret = bmi270_get_scale(data, IIO_ACCEL, &scale, &uscale);
> +       if (ret)
> +               return ret;
> +
> +       max = BMI270_G_MICRO_M_S_2 / uscale;
> +       step = max / BMI270_MOTION_THRES_FULL_SCALE;
> +
> +       return sysfs_emit(buf, "[0 %u %u]\n", step, max);

Do we need []? Is it common in IIO? The usual way in SW to have "$min
$max $step" (see `seq` in shell, range() in Python and so on).

> +}
> +
> +static IIO_DEVICE_ATTR(in_accel_value_available, 0444,
> +                      bmi270_show_accel_value_avail, NULL, 0);

IIO_DEVICE_ATTR_RO()

...

> +               return FIELD_GET(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, regval) ?
> +                                                                       1 : 0;

Make it one line by replacing ternary with !! (double exclamation mark).

...

> +                       return FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
> +                                        regval) ? 1 : 0;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 3:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Aug 30, 2025 at 2:58 PM Gustavo Silva <gustavograzs@gmail.com> wrote:

...

> > +       return sysfs_emit(buf, "[0 %u %u]\n", step, max);
>
> Do we need []? Is it common in IIO? The usual way in SW to have "$min
> $max $step" (see `seq` in shell, range() in Python and so on).

I stand corrected `seq` actually gives what you used. So, the first
question is still relevant. Use what is the common case in the other
IIO drivers / core.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 4/4] iio: ABI: document accel and roc event attributes

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 2:58 PM Gustavo Silva <gustavograzs@gmail.com> wrote:
>
> Add accelerometer and rate of change event-related sysfs attributes
> exposed by the bmi270 driver.

Seems to me like the absent attributes that are already in the kernel,
should be added in the separate patch.

...

> +What:          /sys/.../iio:deviceX/events/in_accel_x&y&z_roc_rising_en

Out of curiosity, is it for real? I mean & (ampersand) in the sysfs
attribute name? This is quite inconvenient for use in shells.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

Just picking out one thing to reply to...
> 
> > +static ssize_t bmi270_show_accel_value_avail(struct device *dev,
> > +                                            struct device_attribute *attr,
> > +                                            char *buf)
> > +{
> > +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +       struct bmi270_data *data = iio_priv(indio_dev);
> > +       int ret, scale, uscale;
> > +       unsigned int step, max;
> > +
> > +       ret = bmi270_get_scale(data, IIO_ACCEL, &scale, &uscale);
> > +       if (ret)
> > +               return ret;
> > +
> > +       max = BMI270_G_MICRO_M_S_2 / uscale;
> > +       step = max / BMI270_MOTION_THRES_FULL_SCALE;
> > +
> > +       return sysfs_emit(buf, "[0 %u %u]\n", step, max);  
> 
> Do we need []? Is it common in IIO? The usual way in SW to have "$min
> $max $step" (see `seq` in shell, range() in Python and so on).
> 
yes. See ABI docs for _available.  It comes in two forms.
space separted list of values and this one which is used for specifying
limits and step.  The [] is how we tell which one it is.

For anything other than events we handle all this via callbacks and
masks. I've never bothered adding the complexity of that to events
as _available attributes are fairly rare.

Re: [PATCH v5 4/4] iio: ABI: document accel and roc event attributes

[Jonathan Cameron]

On Sat, 30 Aug 2025 15:49:50 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Aug 30, 2025 at 2:58 PM Gustavo Silva <gustavograzs@gmail.com> wrote:
> >
> > Add accelerometer and rate of change event-related sysfs attributes
> > exposed by the bmi270 driver.  
> 
> Seems to me like the absent attributes that are already in the kernel,
> should be added in the separate patch.
Agreed that would be ideal.
> 
> ...
> 
> > +What:          /sys/.../iio:deviceX/events/in_accel_x&y&z_roc_rising_en  
> 
> Out of curiosity, is it for real? I mean & (ampersand) in the sysfs
> attribute name? This is quite inconvenient for use in shells.

Yup.

Easy enough to escape...

It's really wordy to express boolean relationships without using symbols.
This has been in the ABI all the way back to the beginning I think.

Jonathan

> 

Re: [PATCH v5 4/4] iio: ABI: document accel and roc event attributes

[Jonathan Cameron]

On Sat, 30 Aug 2025 18:05:34 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 30 Aug 2025 15:49:50 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Sat, Aug 30, 2025 at 2:58 PM Gustavo Silva <gustavograzs@gmail.com> wrote:  
> > >
> > > Add accelerometer and rate of change event-related sysfs attributes
> > > exposed by the bmi270 driver.    
> > 
> > Seems to me like the absent attributes that are already in the kernel,
> > should be added in the separate patch.  
> Agreed that would be ideal.

Actually what did you mean by absent attributes? 

This is documenting ABI that is part of the general 'scope' of the full
IIO ABI but which hasn't turned up before in this particular combination
(or possibly we missed updating docs when it did!)

Whether it is worth separating out any we know are in another driver is
an open question, but Gustavo hasn't called out any as being like that.
It's possible that these are all surfacing for the first time in this driver.

Jonathan

> > 
> > ...
> >   
> > > +What:          /sys/.../iio:deviceX/events/in_accel_x&y&z_roc_rising_en    
> > 
> > Out of curiosity, is it for real? I mean & (ampersand) in the sysfs
> > attribute name? This is quite inconvenient for use in shells.  
> 
> Yup.
> 
> Easy enough to escape...
> 
> It's really wordy to express boolean relationships without using symbols.
> This has been in the ABI all the way back to the beginning I think.
> 
> Jonathan
> 
> >   
> 

Re: [PATCH v5 4/4] iio: ABI: document accel and roc event attributes

[Jonathan Cameron]

On Sat, 30 Aug 2025 08:58:57 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> Add accelerometer and rate of change event-related sysfs attributes
> exposed by the bmi270 driver.
> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 40 +++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 2fb2cea4b192..75a88f0dc533 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -908,6 +908,7 @@ What:		/sys/.../iio:deviceX/events/in_accel_y_roc_rising_en
>  What:		/sys/.../iio:deviceX/events/in_accel_y_roc_falling_en
>  What:		/sys/.../iio:deviceX/events/in_accel_z_roc_rising_en
>  What:		/sys/.../iio:deviceX/events/in_accel_z_roc_falling_en
> +What:		/sys/.../iio:deviceX/events/in_accel_x&y&z_roc_rising_en
>  What:		/sys/.../iio:deviceX/events/in_anglvel_x_roc_rising_en
>  What:		/sys/.../iio:deviceX/events/in_anglvel_x_roc_falling_en
>  What:		/sys/.../iio:deviceX/events/in_anglvel_y_roc_rising_en
> @@ -1129,6 +1130,7 @@ Description:
>  		will get activated once in_voltage0_raw goes above 1200 and will become
>  		deactivated again once the value falls below 1150.
>  
> +What:		/sys/.../events/in_accel_roc_rising_value
>  What:		/sys/.../events/in_accel_x_raw_roc_rising_value
>  What:		/sys/.../events/in_accel_x_raw_roc_falling_value
>  What:		/sys/.../events/in_accel_y_raw_roc_rising_value
> @@ -1177,6 +1179,7 @@ Description:
>  
>  What:		/sys/.../events/in_accel_x_thresh_rising_period
>  What:		/sys/.../events/in_accel_x_thresh_falling_period
> +What:		/sys/.../events/in_accel_roc_rising_period
>  What:		/sys/.../events/in_accel_x_roc_rising_period
>  What:		/sys/.../events/in_accel_x_roc_falling_period
>  What:		/sys/.../events/in_accel_y_thresh_rising_period
> @@ -1187,6 +1190,7 @@ What:		/sys/.../events/in_accel_z_thresh_rising_period
>  What:		/sys/.../events/in_accel_z_thresh_falling_period
>  What:		/sys/.../events/in_accel_z_roc_rising_period
>  What:		/sys/.../events/in_accel_z_roc_falling_period
> +What:		/sys/.../events/in_accel_mag_adaptive_rising_period
>  What:		/sys/.../events/in_anglvel_x_thresh_rising_period
>  What:		/sys/.../events/in_anglvel_x_thresh_falling_period
>  What:		/sys/.../events/in_anglvel_x_roc_rising_period
> @@ -1344,6 +1348,23 @@ Description:
>  		number or direction is not specified, applies to all channels of
>  		this type.
>  
> +What:		/sys/.../iio:deviceX/events/in_accel_x_mag_adaptive_rising_en
> +What:		/sys/.../iio:deviceX/events/in_accel_y_mag_adaptive_rising_en
> +What:		/sys/.../iio:deviceX/events/in_accel_z_mag_adaptive_rising_en
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Similar to in_accel_x_thresh[_rising|_falling]_en, but here the
> +		adaptive magnitude of the channel is compared to the threshold.
That seems backwards.  I was thinking of this as

the magnitude of the channel is compared to an adaptive threshold. 

I'm not sure what 'adaptive magnitude of the channel' would mean.

> +
> +What:		/sys/.../events/in_accel_mag_adaptive_rising_value
I'm not sure this one needs it's own block.

For the nearest equivalent which is _adaptive_rising_value (which is only documented
for capacitive channels we just put it in the same block as the non _adaptive_ variant.

> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The value to which the reference adaptive magnitude of the channel is
> +		compared. If the axis is not specified, it applies to all channels
> +		of this type.
> +
>  What:		/sys/.../iio:deviceX/events/in_accel_mag_referenced_en
>  What:		/sys/.../iio:deviceX/events/in_accel_mag_referenced_rising_en
>  What:		/sys/.../iio:deviceX/events/in_accel_mag_referenced_falling_en
> @@ -2386,3 +2407,22 @@ Description:
>  		Value representing the user's attention to the system expressed
>  		in units as percentage. This usually means if the user is
>  		looking at the screen or not.
> +
> +What:		/sys/.../events/in_accel_value_available
> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		List of available acceleration threshold values that can be
> +		configured for event generation. Units after application of
> +		scale and offset are m/s^2. Expressed as:
> +

This is generic. What sort of event does it apply to?  Seems like it should
be in_accel_mag_adaptive_value or something along those lines? Might also need
in_acecl_mag_reference_value or even direction specific variants of each.

Whilst the ABI does allow for a broader 'available' to cover for
all channels for instance so this is sort of valid ABI, to me it just
feels non intuitive for a user.    Maybe just adding tot this document
to say that this applies to all forms of event for in_accel channels on
this device is sufficient.

> +		- a range specified as "[min step max]"
> +
> +What:		/sys/.../events/in_accel_period_available

This one feels more reasonable as the idea of all events sharing a period control
is more likely than the values that are accepted being the same for all.

> +KernelVersion:	6.18
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		List of available periods for accelerometer event detection in
> +		seconds, expressed as:
> +
> +		- a range specified as "[min step max]"

Re: [PATCH v5 0/4] BMI270: Add support for step counter and motion events

[Jonathan Cameron]

On Sat, 30 Aug 2025 15:36:52 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Aug 30, 2025 at 2:58 PM Gustavo Silva <gustavograzs@gmail.com> wrote:
> >
> > 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.
> >  
> 
> > Sorry for the delay in sending v5.
> > As a reminder, patches 1 and 2 were already accepted in v3.  
> 
> Right, and this series has confusing patch numbering. Please, make
> sure you send all the patches that are mentioned in the numbers, or
> correct numbers. I even don't know if `b4` by default can handle this
> (yes, it can if one manually passes the numbers of the patches to
> take).
> 
Absolutely agree. Don't worry about patch numbers matching cross
versions. It's common for them to change for this reason or because
things got split up, merged, spun around etc so normally we make no
attempt to keep them constant.

J

Re: [PATCH v5 4/4] iio: ABI: document accel and roc event attributes

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sat, 30 Aug 2025 18:05:34 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sat, 30 Aug 2025 15:49:50 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Sat, Aug 30, 2025 at 2:58 PM Gustavo Silva <gustavograzs@gmail.com> wrote:
> > > >
> > > > Add accelerometer and rate of change event-related sysfs attributes
> > > > exposed by the bmi270 driver.
> > >
> > > Seems to me like the absent attributes that are already in the kernel,
> > > should be added in the separate patch.
> > Agreed that would be ideal.
>
> Actually what did you mean by absent attributes?

Absent in the documentation, but present in the code. That's what this
patch adds mainly, no?

> This is documenting ABI that is part of the general 'scope' of the full
> IIO ABI but which hasn't turned up before in this particular combination
> (or possibly we missed updating docs when it did!)
>
> Whether it is worth separating out any we know are in another driver is
> an open question, but Gustavo hasn't called out any as being like that.
> It's possible that these are all surfacing for the first time in this driver.

Hmm... But if the code handles the attribute which is not documented,
that needs to be fixed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 4/4] iio: ABI: document accel and roc event attributes

[Jonathan Cameron]

On Sat, 30 Aug 2025 22:58:17 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Aug 30, 2025 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sat, 30 Aug 2025 18:05:34 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:  
> > > On Sat, 30 Aug 2025 15:49:50 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > On Sat, Aug 30, 2025 at 2:58 PM Gustavo Silva <gustavograzs@gmail.com> wrote:  
> > > > >
> > > > > Add accelerometer and rate of change event-related sysfs attributes
> > > > > exposed by the bmi270 driver.  
> > > >
> > > > Seems to me like the absent attributes that are already in the kernel,
> > > > should be added in the separate patch.  
> > > Agreed that would be ideal.  
> >
> > Actually what did you mean by absent attributes?  
> 
> Absent in the documentation, but present in the code. That's what this
> patch adds mainly, no?
> 
> > This is documenting ABI that is part of the general 'scope' of the full
> > IIO ABI but which hasn't turned up before in this particular combination
> > (or possibly we missed updating docs when it did!)
> >
> > Whether it is worth separating out any we know are in another driver is
> > an open question, but Gustavo hasn't called out any as being like that.
> > It's possible that these are all surfacing for the first time in this driver.  
> 
> Hmm... But if the code handles the attribute which is not documented,
> that needs to be fixed.
> 
The core code handles a massive number of combinations that make no sense
because the attributes are constructed from 5+ parameters, each with many
values. So what matters here is whether a combination is in use, not whether it
could be instantiate by a driver using standard IIO structures.

Sometimes we miss things in reviews so gaps occur. Those absolutely should
be fixed, but we shouldn't documents stuff on basis it 'might' be ABI in
future.

Jonathan