[PATCH 0/3] imu: st_lsm6dsx: Add support for rotation sensor

[PATCH 0/3] imu: st_lsm6dsx: Add support for rotation sensor

[Francesco Lavra]

This series adds support for the rotation sensor functionality present in
some chips from the ST LSM6DSX IMU family.
The first commit is a fix to a previous commit of mine [1] which made it to
6.19-rcX; the fix technically changes userspace, but this should be OK as
long as it goes in during this release cycle.
Tested on LSM6DSV16X.

[1] https://lore.kernel.org/linux-iio/20251017164255.1251060-3-flavra@baylibre.com/

Francesco Lavra (3):
  iio: imu: st_lsm6dsx: set buffer sampling frequency for accelerometer
    only
  iio: imu: st_lsm6dsx: set FIFO ODR for accelerometer and magnetometer
    only
  iio: imu: st_lsm6dsx: add support for rotation sensor

 drivers/iio/imu/st_lsm6dsx/Makefile           |   2 +-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  26 +-
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    |  28 ++-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  |  58 +++++
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c    | 224 ++++++++++++++++++
 5 files changed, 331 insertions(+), 7 deletions(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c

-- 
2.39.5

[PATCH 1/3] iio: imu: st_lsm6dsx: set buffer sampling frequency for accelerometer only

[Francesco Lavra]

The st_lsm6dsx_hwfifo_odr_store() function, which is called when userspace
writes the buffer sampling frequency sysfs attribute, calls
st_lsm6dsx_check_odr(), which accesses the odr_table array at index
`sensor->id`; since this array is only 2 entries long, an access for any
sensor type other than accelerometer or gyroscope is an out-of-bounds
access.

To prevent userspace from triggering an out-of-bounds array access, and to
support the only use case for which FIFO sampling frequency values
different from the sensor sampling frequency may be needed (which is for
keeping FIFO data rate low while sampling acceleration data at high rates
for accurate event detection), do not create the buffer sampling frequency
attribute for sensor types other than the accelerometer.

Fixes: 6b648a36c200 ("iio: imu: st_lsm6dsx: Decouple sensor ODR from FIFO batch data rate")
Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 55d877745575..5ac45e6230b5 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -858,12 +858,21 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
 	int i, ret;
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
+		const struct iio_dev_attr **attrs;
+
 		if (!hw->iio_devs[i])
 			continue;
 
+		/*
+		 * For the accelerometer, allow setting FIFO sampling frequency
+		 * values different from the sensor sampling frequency, which
+		 * may be needed to keep FIFO data rate low while sampling
+		 * acceleration data at high rates for accurate event detection.
+		 */
+		attrs = (i == ST_LSM6DSX_ID_ACC) ? st_lsm6dsx_buffer_attrs : NULL;
 		ret = devm_iio_kfifo_buffer_setup_ext(hw->dev, hw->iio_devs[i],
 						      &st_lsm6dsx_buffer_ops,
-						      st_lsm6dsx_buffer_attrs);
+						      attrs);
 		if (ret)
 			return ret;
 	}
-- 
2.39.5

[PATCH 2/3] iio: imu: st_lsm6dsx: set FIFO ODR for accelerometer and magnetometer only

[Francesco Lavra]

The st_lsm6dsx_set_fifo_odr() function, which is called when enabling and
disabling the hardware FIFO, checks the contents of the hw->settings->batch
array at index sensor->id, and then sets the current ODR value in sensor
registers that depend on whether the register address is set in the above
array element. This logic is valid for internal sensors only, i.e. the
accelerometer and magnetometer; however, since commit c91c1c844ebd ("iio:
imu: st_lsm6dsx: add i2c embedded controller support"), this function is
called also when configuring the hardware FIFO for external sensors (i.e.
sensors accessed through the sensor hub functionality), which can result in
unrelated device registers being written.

Add a check to the beginning of st_lsm6dsx_set_fifo_odr() so that it does
not touch any registers unless it is called for internal sensors.

Fixes: c91c1c844ebd ("iio: imu: st_lsm6dsx: add i2c embedded controller support")
Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 5ac45e6230b5..9db48e835d4f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -225,6 +225,9 @@ static int st_lsm6dsx_set_fifo_odr(struct st_lsm6dsx_sensor *sensor,
 	const struct st_lsm6dsx_reg *batch_reg;
 	u8 data;
 
+	/* Only accel and gyro have batch registers. */
+	if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
+		return 0;
 	batch_reg = &hw->settings->batch[sensor->id];
 	if (batch_reg->addr) {
 		int val;
-- 
2.39.5

[PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor

[Francesco Lavra]

Some IMU chips in the LSM6DSX family have sensor fusion features that
combine data from the accelerometer and gyroscope. One of these features
generates rotation vector data and makes it available in the hardware
FIFO as a quaternion (more specifically, the X, Y and Z components of the
quaternion vector, expressed as 16-bit half-precision floating-point
numbers).

Add support for a new sensor instance that allows receiving sensor fusion
data, by defining a new struct st_lsm6dsx_sf_settings (which contains
chip-specific details for the sensor fusion functionality), and adding this
struct as a new field in struct st_lsm6dsx_settings. In st_lsm6dsx_core.c,
populate this new struct for the LSM6DSV and LSM6DSV16X chips, and add the
logic to initialize an additional IIO device if this struct is populated
for the hardware type being probed.
Note: a new IIO device is being defined (as opposed to adding channels to
an existing device) because each of the existing devices handles data
coming from a single sensor, while sensor fusion data comes from multiple
sensors.

Tested on LSMDSV16X.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/Makefile           |   2 +-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  26 +-
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    |  16 +-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  |  58 +++++
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c    | 224 ++++++++++++++++++
 5 files changed, 319 insertions(+), 7 deletions(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c

diff --git a/drivers/iio/imu/st_lsm6dsx/Makefile b/drivers/iio/imu/st_lsm6dsx/Makefile
index 57cbcd67d64f..19a488254de3 100644
--- a/drivers/iio/imu/st_lsm6dsx/Makefile
+++ b/drivers/iio/imu/st_lsm6dsx/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 st_lsm6dsx-y := st_lsm6dsx_core.o st_lsm6dsx_buffer.o \
-		st_lsm6dsx_shub.o
+		st_lsm6dsx_shub.o st_lsm6dsx_fusion.o
 
 obj-$(CONFIG_IIO_ST_LSM6DSX) += st_lsm6dsx.o
 obj-$(CONFIG_IIO_ST_LSM6DSX_I2C) += st_lsm6dsx_i2c.o
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 07b1773c87bd..4173f670f7af 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -294,6 +294,7 @@ enum st_lsm6dsx_sensor_id {
 	ST_LSM6DSX_ID_EXT0,
 	ST_LSM6DSX_ID_EXT1,
 	ST_LSM6DSX_ID_EXT2,
+	ST_LSM6DSX_ID_SF,
 	ST_LSM6DSX_ID_MAX,
 };
 
@@ -301,6 +302,15 @@ enum st_lsm6dsx_ext_sensor_id {
 	ST_LSM6DSX_ID_MAGN,
 };
 
+struct st_lsm6dsx_sf_settings {
+	const struct iio_chan_spec *chan;
+	int chan_len;
+	struct st_lsm6dsx_odr_table_entry odr_table;
+	struct st_lsm6dsx_reg fifo_enable;
+	struct st_lsm6dsx_reg page_mux;
+	struct st_lsm6dsx_reg enable;
+};
+
 /**
  * struct st_lsm6dsx_ext_dev_settings - i2c controller slave settings
  * @i2c_addr: I2c slave address list.
@@ -388,6 +398,7 @@ struct st_lsm6dsx_settings {
 	struct st_lsm6dsx_hw_ts_settings ts_settings;
 	struct st_lsm6dsx_shub_settings shub_settings;
 	struct st_lsm6dsx_event_settings event_settings;
+	struct st_lsm6dsx_sf_settings sf_settings;
 };
 
 enum st_lsm6dsx_sensor_id {
@@ -510,6 +521,9 @@ int st_lsm6dsx_check_odr(struct st_lsm6dsx_sensor *sensor, u32 odr, u8 *val);
 int st_lsm6dsx_shub_probe(struct st_lsm6dsx_hw *hw, const char *name);
 int st_lsm6dsx_shub_set_enable(struct st_lsm6dsx_sensor *sensor, bool enable);
 int st_lsm6dsx_shub_read_output(struct st_lsm6dsx_hw *hw, u8 *data, int len);
+int st_lsm6dsx_sf_probe(struct st_lsm6dsx_hw *hw, const char *name);
+int st_lsm6dsx_sf_set_enable(struct st_lsm6dsx_sensor *sensor, bool enable);
+int st_lsm6dsx_sf_set_odr(struct st_lsm6dsx_sensor *sensor, bool enable);
 int st_lsm6dsx_set_page(struct st_lsm6dsx_hw *hw, bool enable);
 
 static inline int
@@ -564,12 +578,14 @@ st_lsm6dsx_get_mount_matrix(const struct iio_dev *iio_dev,
 static inline int
 st_lsm6dsx_device_set_enable(struct st_lsm6dsx_sensor *sensor, bool enable)
 {
-	if (sensor->id == ST_LSM6DSX_ID_EXT0 ||
-	    sensor->id == ST_LSM6DSX_ID_EXT1 ||
-	    sensor->id == ST_LSM6DSX_ID_EXT2)
+	switch (sensor->id) {
+	case ST_LSM6DSX_ID_EXT0 ... ST_LSM6DSX_ID_EXT2:
 		return st_lsm6dsx_shub_set_enable(sensor, enable);
-
-	return st_lsm6dsx_sensor_set_enable(sensor, enable);
+	case ST_LSM6DSX_ID_SF:
+		return st_lsm6dsx_sf_set_enable(sensor, enable);
+	default:
+		return st_lsm6dsx_sensor_set_enable(sensor, enable);
+	}
 }
 
 static const
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 9db48e835d4f..a689c081c186 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -88,6 +88,7 @@ enum st_lsm6dsx_fifo_tag {
 	ST_LSM6DSX_EXT0_TAG = 0x0f,
 	ST_LSM6DSX_EXT1_TAG = 0x10,
 	ST_LSM6DSX_EXT2_TAG = 0x11,
+	ST_LSM6DSX_ROT_TAG = 0x13,
 };
 
 static const
@@ -226,8 +227,11 @@ static int st_lsm6dsx_set_fifo_odr(struct st_lsm6dsx_sensor *sensor,
 	u8 data;
 
 	/* Only accel and gyro have batch registers. */
-	if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
+	if (sensor->id >= ARRAY_SIZE(hw->settings->batch)) {
+		if (sensor->id == ST_LSM6DSX_ID_SF)
+			return st_lsm6dsx_sf_set_odr(sensor, enable);
 		return 0;
+	}
 	batch_reg = &hw->settings->batch[sensor->id];
 	if (batch_reg->addr) {
 		int val;
@@ -579,6 +583,16 @@ st_lsm6dsx_push_tagged_data(struct st_lsm6dsx_hw *hw, u8 tag,
 	case ST_LSM6DSX_EXT2_TAG:
 		iio_dev = hw->iio_devs[ST_LSM6DSX_ID_EXT2];
 		break;
+	case ST_LSM6DSX_ROT_TAG:
+		/**
+		 * The sensor reports only the {X, Y, Z} elements of the
+		 * quaternion vector; set the W value to 0 (it can be derived
+		 * from the {X, Y, Z} values due to the property that the vector
+		 * is normalized).
+		 */
+		*(u16 *)(data + ST_LSM6DSX_SAMPLE_SIZE) = 0;
+		iio_dev = hw->iio_devs[ST_LSM6DSX_ID_SF];
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index dc0ae0e488ce..c21163a06a71 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -94,6 +94,24 @@
 
 #define ST_LSM6DSX_REG_WHOAMI_ADDR		0x0f
 
+/* Raw values from the IMU are 16-bit half-precision floating-point numbers. */
+#define ST_LSM6DSX_CHANNEL_ROT						\
+{									\
+	.type = IIO_ROT,						\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_QUATERNION,					\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = 0,						\
+	.scan_type = {							\
+		.sign = 'u',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+		.repeat = 4,						\
+	},								\
+	.ext_info = st_lsm6dsx_ext_info,				\
+}
+
 #define ST_LSM6DSX_TS_SENSITIVITY		25000UL /* 25us */
 
 static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
@@ -153,6 +171,11 @@ static const struct iio_chan_spec st_lsm6ds0_gyro_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
+static const struct iio_chan_spec st_lsm6dsx_sf_channels[] = {
+	ST_LSM6DSX_CHANNEL_ROT,
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
 static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	{
 		.reset = {
@@ -1492,6 +1515,35 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 			.wakeup_src_y_mask = BIT(1),
 			.wakeup_src_x_mask = BIT(2),
 		},
+		.sf_settings = {
+			.chan = st_lsm6dsx_sf_channels,
+			.chan_len = ARRAY_SIZE(st_lsm6dsx_sf_channels),
+			.odr_table = {
+				.reg = {
+					.addr = 0x5e,
+					.mask = GENMASK(5, 3),
+				},
+				.odr_avl[0] = {  15000, 0x00 },
+				.odr_avl[1] = {  30000, 0x01 },
+				.odr_avl[2] = {  60000, 0x02 },
+				.odr_avl[3] = { 120000, 0x03 },
+				.odr_avl[4] = { 240000, 0x04 },
+				.odr_avl[5] = { 480000, 0x05 },
+				.odr_len = 6,
+			},
+			.fifo_enable = {
+				.addr = 0x44,
+				.mask = BIT(1),
+			},
+			.page_mux = {
+				.addr = 0x01,
+				.mask = BIT(7),
+			},
+			.enable = {
+				.addr = 0x04,
+				.mask = BIT(1),
+			},
+		},
 	},
 	{
 		.reset = {
@@ -2899,6 +2951,12 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 			return err;
 	}
 
+	if (hw->settings->sf_settings.chan) {
+		err = st_lsm6dsx_sf_probe(hw, name);
+		if (err)
+			return err;
+	}
+
 	if (hw->irq > 0) {
 		err = st_lsm6dsx_irq_setup(hw);
 		if (err < 0)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
new file mode 100644
index 000000000000..7c78f14cbb91
--- /dev/null
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * STMicroelectronics st_lsm6dsx IMU sensor fusion
+ */
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/sprintf.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include "st_lsm6dsx.h"
+
+static int
+st_lsm6dsx_sf_get_odr_val(const struct st_lsm6dsx_sf_settings *settings,
+			  u32 odr, u8 *val)
+{
+	int i;
+
+	for (i = 0; i < settings->odr_table.odr_len; i++) {
+		if (settings->odr_table.odr_avl[i].milli_hz == odr)
+			break;
+	}
+	if (i == settings->odr_table.odr_len)
+		return -EINVAL;
+
+	*val = settings->odr_table.odr_avl[i].val;
+	return 0;
+}
+
+/**
+ * st_lsm6dsx_sf_set_page - Enable or disable access to sensor fusion
+ * configuration registers.
+ * @hw: Sensor hardware instance.
+ * @enable: True to enable access, false to disable access.
+ *
+ * The register page lock is acquired when enabling access, and released when
+ * disabling access. Therefore, a function call with @enable set to true must be
+ * followed by a call with @enable set to false (unless the first call returns
+ * an error value).
+ *
+ * Return: 0 on success, negative value on error.
+ */
+static int st_lsm6dsx_sf_set_page(struct st_lsm6dsx_hw *hw, bool enable)
+{
+	const struct st_lsm6dsx_reg *mux;
+	int err;
+
+	mux = &hw->settings->sf_settings.page_mux;
+	if (enable)
+		mutex_lock(&hw->page_lock);
+	err = regmap_assign_bits(hw->regmap, mux->addr, mux->mask, enable);
+	if (!enable || err < 0)
+		mutex_unlock(&hw->page_lock);
+
+	return err;
+}
+
+int st_lsm6dsx_sf_set_enable(struct st_lsm6dsx_sensor *sensor, bool enable)
+{
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+	const struct st_lsm6dsx_reg *enable_reg;
+	int err;
+
+	enable_reg = &hw->settings->sf_settings.enable;
+	err = st_lsm6dsx_sf_set_page(hw, true);
+	if (err < 0)
+		return err;
+
+	err = regmap_assign_bits(hw->regmap, enable_reg->addr, enable_reg->mask,
+				 enable);
+	st_lsm6dsx_sf_set_page(hw, false);
+
+	return err;
+}
+
+int st_lsm6dsx_sf_set_odr(struct st_lsm6dsx_sensor *sensor, bool enable)
+{
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+	const struct st_lsm6dsx_sf_settings *settings;
+	u8 data;
+	int err;
+
+	err = st_lsm6dsx_sf_set_page(hw, true);
+	if (err < 0)
+		return err;
+
+	settings = &hw->settings->sf_settings;
+	if (enable) {
+		u8 odr_val;
+		const struct st_lsm6dsx_reg *reg = &settings->odr_table.reg;
+
+		st_lsm6dsx_sf_get_odr_val(settings, sensor->hwfifo_odr_mHz,
+					  &odr_val);
+		data = ST_LSM6DSX_SHIFT_VAL(odr_val, reg->mask);
+		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
+					 data);
+		if (err < 0)
+			goto out;
+	}
+	err = regmap_assign_bits(hw->regmap, settings->fifo_enable.addr,
+				 settings->fifo_enable.mask, enable);
+
+out:
+	st_lsm6dsx_sf_set_page(hw, false);
+
+	return err;
+}
+
+static int st_lsm6dsx_sf_read_raw(struct iio_dev *iio_dev,
+				  struct iio_chan_spec const *ch,
+				  int *val, int *val2, long mask)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = sensor->hwfifo_odr_mHz / MILLI;
+		*val2 = (sensor->hwfifo_odr_mHz % MILLI) * MILLI;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int st_lsm6dsx_sf_write_raw(struct iio_dev *iio_dev,
+				   struct iio_chan_spec const *chan,
+				   int val, int val2, long mask)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+	const struct st_lsm6dsx_sf_settings *settings;
+	int err;
+
+	settings = &sensor->hw->settings->sf_settings;
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		u32 odr_mHz;
+		u8 odr_val;
+
+		odr_mHz = val * MILLI + val2 / MILLI;
+		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
+		if (err)
+			return err;
+
+		sensor->hwfifo_odr_mHz = odr_mHz;
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t st_lsm6dsx_sf_sampling_freq_avail(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_to_iio_dev(dev));
+	const struct st_lsm6dsx_sf_settings *settings;
+	int i, len = 0;
+
+	settings = &sensor->hw->settings->sf_settings;
+	for (i = 0; i < settings->odr_table.odr_len; i++) {
+		u32 val = settings->odr_table.odr_avl[i].milli_hz;
+
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%lu.%03lu ",
+				 val / MILLI, val % MILLI);
+	}
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sf_sampling_freq_avail);
+static struct attribute *st_lsm6dsx_sf_attributes[] = {
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group st_lsm6dsx_sf_attribute_group = {
+	.attrs = st_lsm6dsx_sf_attributes,
+};
+
+static const struct iio_info st_lsm6dsx_sf_info = {
+	.attrs = &st_lsm6dsx_sf_attribute_group,
+	.read_raw = st_lsm6dsx_sf_read_raw,
+	.write_raw = st_lsm6dsx_sf_write_raw,
+	.hwfifo_set_watermark = st_lsm6dsx_set_watermark,
+};
+
+int st_lsm6dsx_sf_probe(struct st_lsm6dsx_hw *hw, const char *name)
+{
+	const struct st_lsm6dsx_sf_settings *settings;
+	struct st_lsm6dsx_sensor *sensor;
+	struct iio_dev *iio_dev;
+
+	iio_dev = devm_iio_device_alloc(hw->dev, sizeof(*sensor));
+	if (!iio_dev)
+		return -ENOMEM;
+
+	settings = &hw->settings->sf_settings;
+	sensor = iio_priv(iio_dev);
+	sensor->id = ST_LSM6DSX_ID_SF;
+	sensor->hw = hw;
+	sensor->hwfifo_odr_mHz = settings->odr_table.odr_avl[0].milli_hz;
+	sensor->watermark = 1;
+	iio_dev->modes = INDIO_DIRECT_MODE;
+	iio_dev->info = &st_lsm6dsx_sf_info;
+	iio_dev->channels = settings->chan;
+	iio_dev->num_channels = settings->chan_len;
+	scnprintf(sensor->name, sizeof(sensor->name), "%s_sf", name);
+	iio_dev->name = sensor->name;
+
+	/**
+	 *  Put the IIO device pointer in the iio_devs array so that the caller
+	 *  can set up a buffer and register this IIO device.
+	 */
+	hw->iio_devs[ST_LSM6DSX_ID_SF] = iio_dev;
+
+	return 0;
+}
-- 
2.39.5

Re: [PATCH 2/3] iio: imu: st_lsm6dsx: set FIFO ODR for accelerometer and magnetometer only

[Andy Shevchenko]

On Fri, Jan 09, 2026 at 07:15:27PM +0100, Francesco Lavra wrote:
> The st_lsm6dsx_set_fifo_odr() function, which is called when enabling and
> disabling the hardware FIFO, checks the contents of the hw->settings->batch
> array at index sensor->id, and then sets the current ODR value in sensor
> registers that depend on whether the register address is set in the above
> array element. This logic is valid for internal sensors only, i.e. the
> accelerometer and magnetometer; however, since commit c91c1c844ebd ("iio:
> imu: st_lsm6dsx: add i2c embedded controller support"), this function is
> called also when configuring the hardware FIFO for external sensors (i.e.
> sensors accessed through the sensor hub functionality), which can result in
> unrelated device registers being written.
> 
> Add a check to the beginning of st_lsm6dsx_set_fifo_odr() so that it does
> not touch any registers unless it is called for internal sensors.

...

>  	const struct st_lsm6dsx_reg *batch_reg;
>  	u8 data;
>  
> +	/* Only accel and gyro have batch registers. */
> +	if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
> +		return 0;

+ Blank line.

>  	batch_reg = &hw->settings->batch[sensor->id];

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor

[Andy Shevchenko]

On Fri, Jan 09, 2026 at 07:15:28PM +0100, Francesco Lavra wrote:
> Some IMU chips in the LSM6DSX family have sensor fusion features that
> combine data from the accelerometer and gyroscope. One of these features
> generates rotation vector data and makes it available in the hardware
> FIFO as a quaternion (more specifically, the X, Y and Z components of the
> quaternion vector, expressed as 16-bit half-precision floating-point
> numbers).
> 
> Add support for a new sensor instance that allows receiving sensor fusion
> data, by defining a new struct st_lsm6dsx_sf_settings (which contains
> chip-specific details for the sensor fusion functionality), and adding this
> struct as a new field in struct st_lsm6dsx_settings. In st_lsm6dsx_core.c,
> populate this new struct for the LSM6DSV and LSM6DSV16X chips, and add the
> logic to initialize an additional IIO device if this struct is populated
> for the hardware type being probed.
> Note: a new IIO device is being defined (as opposed to adding channels to
> an existing device) because each of the existing devices handles data
> coming from a single sensor, while sensor fusion data comes from multiple
> sensors.
> 
> Tested on LSMDSV16X.

...

> enum st_lsm6dsx_sensor_id {

>  	ST_LSM6DSX_ID_EXT0,
>  	ST_LSM6DSX_ID_EXT1,
>  	ST_LSM6DSX_ID_EXT2,
> +	ST_LSM6DSX_ID_SF,
>  	ST_LSM6DSX_ID_MAX,

At some point please either get rid of _ID_MAX, or drop the trailing comma
(maybe some other places also need the same treatment).

...

> +static int st_lsm6dsx_sf_set_page(struct st_lsm6dsx_hw *hw, bool enable)
> +{
> +	const struct st_lsm6dsx_reg *mux;
> +	int err;
> +
> +	mux = &hw->settings->sf_settings.page_mux;
> +	if (enable)
> +		mutex_lock(&hw->page_lock);
> +	err = regmap_assign_bits(hw->regmap, mux->addr, mux->mask, enable);
> +	if (!enable || err < 0)
> +		mutex_unlock(&hw->page_lock);
> +
> +	return err;
> +}

Why not having properly made functions with the respective sparse annotations?
And drop this "enable" parameter.

...

> +int st_lsm6dsx_sf_set_enable(struct st_lsm6dsx_sensor *sensor, bool enable)
> +{
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +	const struct st_lsm6dsx_reg *enable_reg;
> +	int err;
> +
> +	enable_reg = &hw->settings->sf_settings.enable;
> +	err = st_lsm6dsx_sf_set_page(hw, true);
> +	if (err < 0)
> +		return err;
> +
> +	err = regmap_assign_bits(hw->regmap, enable_reg->addr, enable_reg->mask,
> +				 enable);

One line? The variable name can be shortened as well.

> +	st_lsm6dsx_sf_set_page(hw, false);
> +
> +	return err;
> +}

...

> +static int st_lsm6dsx_sf_read_raw(struct iio_dev *iio_dev,
> +				  struct iio_chan_spec const *ch,
> +				  int *val, int *val2, long mask)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = sensor->hwfifo_odr_mHz / MILLI;
> +		*val2 = (sensor->hwfifo_odr_mHz % MILLI) * MILLI;

Strictly speaking the multiplier in the second one should be "(MICRO / MILLI)".

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

...

> +static int st_lsm6dsx_sf_write_raw(struct iio_dev *iio_dev,
> +				   struct iio_chan_spec const *chan,
> +				   int val, int val2, long mask)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +	const struct st_lsm6dsx_sf_settings *settings;
> +	int err;
> +
> +	settings = &sensor->hw->settings->sf_settings;
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		u32 odr_mHz;
> +		u8 odr_val;
> +
> +		odr_mHz = val * MILLI + val2 / MILLI;


Ditto.

> +		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
> +		if (err)
> +			return err;
> +
> +		sensor->hwfifo_odr_mHz = odr_mHz;
> +		break;
> +	}
> +	default:
> +		return -EINVAL;
> +	}

> +	return 0;

Perhaps move it to the only case that needs it?

> +}

...

> +static ssize_t st_lsm6dsx_sf_sampling_freq_avail(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_to_iio_dev(dev));
> +	const struct st_lsm6dsx_sf_settings *settings;
> +	int i, len = 0;
> +
> +	settings = &sensor->hw->settings->sf_settings;
> +	for (i = 0; i < settings->odr_table.odr_len; i++) {

	for (unsigned int i ...) {

> +		u32 val = settings->odr_table.odr_avl[i].milli_hz;
> +
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%lu.%03lu ",
> +				 val / MILLI, val % MILLI);

Hmm... I think this has to be sysfs_emit_at().

> +	}
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}

...

> +static struct attribute *st_lsm6dsx_sf_attributes[] = {
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,

> +	NULL,

No comma here.

> +};


...

> +int st_lsm6dsx_sf_probe(struct st_lsm6dsx_hw *hw, const char *name)
> +{
> +	const struct st_lsm6dsx_sf_settings *settings;
> +	struct st_lsm6dsx_sensor *sensor;
> +	struct iio_dev *iio_dev;
> +
> +	iio_dev = devm_iio_device_alloc(hw->dev, sizeof(*sensor));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	settings = &hw->settings->sf_settings;
> +	sensor = iio_priv(iio_dev);
> +	sensor->id = ST_LSM6DSX_ID_SF;
> +	sensor->hw = hw;
> +	sensor->hwfifo_odr_mHz = settings->odr_table.odr_avl[0].milli_hz;
> +	sensor->watermark = 1;
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +	iio_dev->info = &st_lsm6dsx_sf_info;
> +	iio_dev->channels = settings->chan;
> +	iio_dev->num_channels = settings->chan_len;

> +	scnprintf(sensor->name, sizeof(sensor->name), "%s_sf", name);

What's the point of "c" here, please?

> +	iio_dev->name = sensor->name;
> +
> +	/**

Huh?!

> +	 *  Put the IIO device pointer in the iio_devs array so that the caller
> +	 *  can set up a buffer and register this IIO device.
> +	 */
> +	hw->iio_devs[ST_LSM6DSX_ID_SF] = iio_dev;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] iio: imu: st_lsm6dsx: set buffer sampling frequency for accelerometer only

[Jonathan Cameron]

On Fri,  9 Jan 2026 19:15:26 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> The st_lsm6dsx_hwfifo_odr_store() function, which is called when userspace
> writes the buffer sampling frequency sysfs attribute, calls
> st_lsm6dsx_check_odr(), which accesses the odr_table array at index
> `sensor->id`; since this array is only 2 entries long, an access for any
> sensor type other than accelerometer or gyroscope is an out-of-bounds
> access.
> 
> To prevent userspace from triggering an out-of-bounds array access, and to
> support the only use case for which FIFO sampling frequency values
> different from the sensor sampling frequency may be needed (which is for
> keeping FIFO data rate low while sampling acceleration data at high rates
> for accurate event detection), do not create the buffer sampling frequency
> attribute for sensor types other than the accelerometer.

I'm not following why we need to drop this attribute for the gyroscope.
Perhaps lay out what the combinations of controls are and the attributes
we end up with.

As you note in the cover letter we can change this now with ABI issues as
it is just in my tree, so I don't mind the change, just want to understand
it a little better than I currently do!

> 
> Fixes: 6b648a36c200 ("iio: imu: st_lsm6dsx: Decouple sensor ODR from FIFO batch data rate")
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index 55d877745575..5ac45e6230b5 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -858,12 +858,21 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
>  	int i, ret;
>  
>  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> +		const struct iio_dev_attr **attrs;
> +
>  		if (!hw->iio_devs[i])
>  			continue;
>  
> +		/*
> +		 * For the accelerometer, allow setting FIFO sampling frequency
> +		 * values different from the sensor sampling frequency, which
> +		 * may be needed to keep FIFO data rate low while sampling
> +		 * acceleration data at high rates for accurate event detection.
> +		 */
> +		attrs = (i == ST_LSM6DSX_ID_ACC) ? st_lsm6dsx_buffer_attrs : NULL;
>  		ret = devm_iio_kfifo_buffer_setup_ext(hw->dev, hw->iio_devs[i],
>  						      &st_lsm6dsx_buffer_ops,
> -						      st_lsm6dsx_buffer_attrs);
> +						      attrs);
>  		if (ret)
>  			return ret;
>  	}

Re: [PATCH 2/3] iio: imu: st_lsm6dsx: set FIFO ODR for accelerometer and magnetometer only

[Jonathan Cameron]

On Fri,  9 Jan 2026 19:15:27 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> The st_lsm6dsx_set_fifo_odr() function, which is called when enabling and
> disabling the hardware FIFO, checks the contents of the hw->settings->batch
> array at index sensor->id, and then sets the current ODR value in sensor
> registers that depend on whether the register address is set in the above
> array element. This logic is valid for internal sensors only, i.e. the
> accelerometer and magnetometer; however, since commit c91c1c844ebd ("iio:
> imu: st_lsm6dsx: add i2c embedded controller support"), this function is
> called also when configuring the hardware FIFO for external sensors (i.e.
> sensors accessed through the sensor hub functionality), which can result in
> unrelated device registers being written.
> 
> Add a check to the beginning of st_lsm6dsx_set_fifo_odr() so that it does
> not touch any registers unless it is called for internal sensors.
> 
> Fixes: c91c1c844ebd ("iio: imu: st_lsm6dsx: add i2c embedded controller support")
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>
This seems fine to me. Ideally it would have been first patch in the series
as this is one we want to backport.  I'll leave it on list little while
though to see if Lorenzo or anyone else has time to take a look.

Thanks,

Jonathan


> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index 5ac45e6230b5..9db48e835d4f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -225,6 +225,9 @@ static int st_lsm6dsx_set_fifo_odr(struct st_lsm6dsx_sensor *sensor,
>  	const struct st_lsm6dsx_reg *batch_reg;
>  	u8 data;
>  
> +	/* Only accel and gyro have batch registers. */
> +	if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
> +		return 0;
>  	batch_reg = &hw->settings->batch[sensor->id];
>  	if (batch_reg->addr) {
>  		int val;

Re: [PATCH 2/3] iio: imu: st_lsm6dsx: set FIFO ODR for accelerometer and magnetometer only

[Jonathan Cameron]

On Sun, 11 Jan 2026 16:23:51 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri,  9 Jan 2026 19:15:27 +0100
> Francesco Lavra <flavra@baylibre.com> wrote:
> 
> > The st_lsm6dsx_set_fifo_odr() function, which is called when enabling and
> > disabling the hardware FIFO, checks the contents of the hw->settings->batch
> > array at index sensor->id, and then sets the current ODR value in sensor
> > registers that depend on whether the register address is set in the above
> > array element. This logic is valid for internal sensors only, i.e. the
> > accelerometer and magnetometer; however, since commit c91c1c844ebd ("iio:
> > imu: st_lsm6dsx: add i2c embedded controller support"), this function is
> > called also when configuring the hardware FIFO for external sensors (i.e.
> > sensors accessed through the sensor hub functionality), which can result in
> > unrelated device registers being written.
> > 
> > Add a check to the beginning of st_lsm6dsx_set_fifo_odr() so that it does
> > not touch any registers unless it is called for internal sensors.
> > 
> > Fixes: c91c1c844ebd ("iio: imu: st_lsm6dsx: add i2c embedded controller support")
> > Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> This seems fine to me. Ideally it would have been first patch in the series
> as this is one we want to backport.  I'll leave it on list little while
> though to see if Lorenzo or anyone else has time to take a look.
> 
One thing...

> Thanks,
> 
> Jonathan
> 
> 
> > ---
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > index 5ac45e6230b5..9db48e835d4f 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > @@ -225,6 +225,9 @@ static int st_lsm6dsx_set_fifo_odr(struct st_lsm6dsx_sensor *sensor,
> >  	const struct st_lsm6dsx_reg *batch_reg;
> >  	u8 data;
> >  
> > +	/* Only accel and gyro have batch registers. */

It's true we can't use the batch register, but not clear from this comment
that the else path below is inappropriate.  Is it the absence of patch register
or just that the set_fifo_odr is meaningless for other sensors that matters?
I think this comment needs to provide more detail.

> > +	if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
> > +		return 0;
> >  	batch_reg = &hw->settings->batch[sensor->id];
> >  	if (batch_reg->addr) {
> >  		int val;
> 

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor

[Jonathan Cameron]

On Fri,  9 Jan 2026 19:15:28 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> Some IMU chips in the LSM6DSX family have sensor fusion features that
> combine data from the accelerometer and gyroscope. One of these features
> generates rotation vector data and makes it available in the hardware
> FIFO as a quaternion (more specifically, the X, Y and Z components of the
> quaternion vector, expressed as 16-bit half-precision floating-point
> numbers).
> 
> Add support for a new sensor instance that allows receiving sensor fusion
> data, by defining a new struct st_lsm6dsx_sf_settings (which contains
> chip-specific details for the sensor fusion functionality), and adding this
> struct as a new field in struct st_lsm6dsx_settings. In st_lsm6dsx_core.c,
> populate this new struct for the LSM6DSV and LSM6DSV16X chips, and add the
> logic to initialize an additional IIO device if this struct is populated
> for the hardware type being probed.
> Note: a new IIO device is being defined (as opposed to adding channels to
> an existing device) because each of the existing devices handles data
> coming from a single sensor, while sensor fusion data comes from multiple
> sensors.
That doesn't really justify the extra IIO device.  We used to do this
because the IIO buffers aren't tagged with channel info
(unlike say driver/input) and so if the data for different channels
/sub parts of the sensor can come out at different rates, we have to
split them up.  So real question here is what data rate is this generated
at?  If it matches an existing sensor we should add the channel there,
if not fine to have yet another IIO device. All this changes is
the explanation in this patch description. Code is fine.

Note as an FYI, we now support multiple buffers per IIO device. I think
there are probably still some corners that need ironing out, but in theory
that was to handle this sort of sensor in a single IIO device.

> 
> Tested on LSMDSV16X.
> 
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>


> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
> new file mode 100644
> index 000000000000..7c78f14cbb91
> --- /dev/null
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * STMicroelectronics st_lsm6dsx IMU sensor fusion
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/sprintf.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include "st_lsm6dsx.h"

> +/**
> + * st_lsm6dsx_sf_set_page - Enable or disable access to sensor fusion
> + * configuration registers.
> + * @hw: Sensor hardware instance.
> + * @enable: True to enable access, false to disable access.
> + *
> + * The register page lock is acquired when enabling access, and released when
> + * disabling access. Therefore, a function call with @enable set to true must be
> + * followed by a call with @enable set to false (unless the first call returns
> + * an error value).
> + *
> + * Return: 0 on success, negative value on error.
> + */
> +static int st_lsm6dsx_sf_set_page(struct st_lsm6dsx_hw *hw, bool enable)
> +{

I'm curious what sparse thinks of this.  Try a make C=1 build.
I'd expect it to be fussy that it can't figure out the acquires and releases.
Maybe it can see enough to figure it out.  If not you might need to break
it into two functions so you can add the markings __acquires() etc

Personally I think I'd just take the page lock out of here and use guard() in the
callers.


> +	const struct st_lsm6dsx_reg *mux;
> +	int err;
> +
> +	mux = &hw->settings->sf_settings.page_mux;

Just to check, can you use regmaps support for paging instead of doing this by hand?
That would avoid future problems with enabling caching or similar


> +	if (enable)
> +		mutex_lock(&hw->page_lock);
> +	err = regmap_assign_bits(hw->regmap, mux->addr, mux->mask, enable);
> +	if (!enable || err < 0)
> +		mutex_unlock(&hw->page_lock);
> +
> +	return err;
> +}
> +

> +
> +static int st_lsm6dsx_sf_write_raw(struct iio_dev *iio_dev,
> +				   struct iio_chan_spec const *chan,
> +				   int val, int val2, long mask)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +	const struct st_lsm6dsx_sf_settings *settings;
> +	int err;
> +
> +	settings = &sensor->hw->settings->sf_settings;
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		u32 odr_mHz;
> +		u8 odr_val;
> +
> +		odr_mHz = val * MILLI + val2 / MILLI;
> +		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
> +		if (err)
> +			return err;
> +
> +		sensor->hwfifo_odr_mHz = odr_mHz;
> +		break;
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
Might as well return instead of break above and save a couple of lines + avoid
anyone having to go see what happens after the switch statement.

> +}

> +
> +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sf_sampling_freq_avail);
> +static struct attribute *st_lsm6dsx_sf_attributes[] = {
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL,

No comma as nothing should come after this.  Ideally would be replaced with
code using the read_avail callback and appropriate bit set in the
info mask avail bitmaps.


> +};

Thanks

Jonathan

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor

[Jonathan Cameron]

On Fri, 9 Jan 2026 21:22:09 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Fri, Jan 09, 2026 at 07:15:28PM +0100, Francesco Lavra wrote:
> > Some IMU chips in the LSM6DSX family have sensor fusion features that
> > combine data from the accelerometer and gyroscope. One of these features
> > generates rotation vector data and makes it available in the hardware
> > FIFO as a quaternion (more specifically, the X, Y and Z components of the
> > quaternion vector, expressed as 16-bit half-precision floating-point
> > numbers).
> > 
> > Add support for a new sensor instance that allows receiving sensor fusion
> > data, by defining a new struct st_lsm6dsx_sf_settings (which contains
> > chip-specific details for the sensor fusion functionality), and adding this
> > struct as a new field in struct st_lsm6dsx_settings. In st_lsm6dsx_core.c,
> > populate this new struct for the LSM6DSV and LSM6DSV16X chips, and add the
> > logic to initialize an additional IIO device if this struct is populated
> > for the hardware type being probed.
> > Note: a new IIO device is being defined (as opposed to adding channels to
> > an existing device) because each of the existing devices handles data
> > coming from a single sensor, while sensor fusion data comes from multiple
> > sensors.
> > 
> > Tested on LSMDSV16X.  
> 
> ...
> 
> > enum st_lsm6dsx_sensor_id {  
> 
> >  	ST_LSM6DSX_ID_EXT0,
> >  	ST_LSM6DSX_ID_EXT1,
> >  	ST_LSM6DSX_ID_EXT2,
> > +	ST_LSM6DSX_ID_SF,
> >  	ST_LSM6DSX_ID_MAX,  
> 
> At some point please either get rid of _ID_MAX, or drop the trailing comma
> (maybe some other places also need the same treatment).
It's already gone, this needs a rebase on the upstream
tree.

Thanks,

Jonathan

Re: [PATCH 1/3] iio: imu: st_lsm6dsx: set buffer sampling frequency for accelerometer only

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]

On Sun, 2026-01-11 at 16:18 +0000, Jonathan Cameron wrote:
> On Fri,  9 Jan 2026 19:15:26 +0100
> Francesco Lavra <flavra@baylibre.com> wrote:
> 
> > The st_lsm6dsx_hwfifo_odr_store() function, which is called when
> > userspace
> > writes the buffer sampling frequency sysfs attribute, calls
> > st_lsm6dsx_check_odr(), which accesses the odr_table array at index
> > `sensor->id`; since this array is only 2 entries long, an access for
> > any
> > sensor type other than accelerometer or gyroscope is an out-of-bounds
> > access.
> > 
> > To prevent userspace from triggering an out-of-bounds array access, and
> > to
> > support the only use case for which FIFO sampling frequency values
> > different from the sensor sampling frequency may be needed (which is
> > for
> > keeping FIFO data rate low while sampling acceleration data at high
> > rates
> > for accurate event detection), do not create the buffer sampling
> > frequency
> > attribute for sensor types other than the accelerometer.
> 
> I'm not following why we need to drop this attribute for the gyroscope.
> Perhaps lay out what the combinations of controls are and the attributes
> we end up with.

It's not like we need to drop this attribute, it's just that I don't see a
need for it. The only reason I added this attribute was to be able to
control (e.g. lower) the rate of data coming from the sensor while
maintaining a high accuracy for event detection; and accurate event
detection requires a high sampling rate for the accelerometer. So the
gyroscope is not involved here, and the attribute is only needed for the
accelerometer.

Before this change, we have:
- accel IIO device with separate samp_freq and buffer/samp_freq
- gyro IIO device with separate samp_freq and buffer/samp_freq
- (optionally) external sensor IIO devices with separate samp_freq and
buffer/samp_freq (and trying to set buffer/samp_freq for these triggers an
out-of-bounds array access)

After this change, we have the accel IIO device with separate samp_freq and
buffer/samp_freq, while the other IIO devices have only a single samp_freq
attribute.

> As you note in the cover letter we can change this now with ABI issues as
> it is just in my tree, so I don't mind the change, just want to
> understand
> it a little better than I currently do!

It's not just in your tree, it has been pulled into Linus's tree for 6.19.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] iio: imu: st_lsm6dsx: set FIFO ODR for accelerometer and magnetometer only

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 3318 bytes --]

On Sun, 2026-01-11 at 16:26 +0000, Jonathan Cameron wrote:
> On Sun, 11 Jan 2026 16:23:51 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri,  9 Jan 2026 19:15:27 +0100
> > Francesco Lavra <flavra@baylibre.com> wrote:
> > 
> > > The st_lsm6dsx_set_fifo_odr() function, which is called when enabling
> > > and
> > > disabling the hardware FIFO, checks the contents of the hw->settings-
> > > >batch
> > > array at index sensor->id, and then sets the current ODR value in
> > > sensor
> > > registers that depend on whether the register address is set in the
> > > above
> > > array element. This logic is valid for internal sensors only, i.e.
> > > the
> > > accelerometer and magnetometer; however, since commit c91c1c844ebd

Oops: s/magnetometer/gyroscope/ (will fix this in the next iteration)

> > > ("iio:
> > > imu: st_lsm6dsx: add i2c embedded controller support"), this function
> > > is
> > > called also when configuring the hardware FIFO for external sensors
> > > (i.e.
> > > sensors accessed through the sensor hub functionality), which can
> > > result in
> > > unrelated device registers being written.
> > > 
> > > Add a check to the beginning of st_lsm6dsx_set_fifo_odr() so that it
> > > does
> > > not touch any registers unless it is called for internal sensors.
> > > 
> > > Fixes: c91c1c844ebd ("iio: imu: st_lsm6dsx: add i2c embedded
> > > controller support")
> > > Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> > This seems fine to me. Ideally it would have been first patch in the
> > series
> > as this is one we want to backport.  I'll leave it on list little while
> > though to see if Lorenzo or anyone else has time to take a look.
> > 
> One thing...
> 
> > Thanks,
> > 
> > Jonathan
> > 
> > 
> > > ---
> > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > index 5ac45e6230b5..9db48e835d4f 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > @@ -225,6 +225,9 @@ static int st_lsm6dsx_set_fifo_odr(struct
> > > st_lsm6dsx_sensor *sensor,
> > >         const struct st_lsm6dsx_reg *batch_reg;
> > >         u8 data;
> > >  
> > > +       /* Only accel and gyro have batch registers. */
> 
> It's true we can't use the batch register, but not clear from this
> comment
> that the else path below is inappropriate.  Is it the absence of patch
> register
> or just that the set_fifo_odr is meaningless for other sensors that
> matters?
> I think this comment needs to provide more detail.

The set_fifo_odr is meaningless for external sensors, so the else path
below is also inappropriate for external sensors; I will amend the comment
to clarify that only internal sensors have a FIFO ODR configuration
register.

> > > +       if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
> > > +               return 0;
> > >         batch_reg = &hw->settings->batch[sensor->id];
> > >         if (batch_reg->addr) {
> > >                 int val;


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 5936 bytes --]

On Sun, 2026-01-11 at 16:46 +0000, Jonathan Cameron wrote:
> On Fri,  9 Jan 2026 19:15:28 +0100
> Francesco Lavra <flavra@baylibre.com> wrote:
> 
> > Some IMU chips in the LSM6DSX family have sensor fusion features that
> > combine data from the accelerometer and gyroscope. One of these
> > features
> > generates rotation vector data and makes it available in the hardware
> > FIFO as a quaternion (more specifically, the X, Y and Z components of
> > the
> > quaternion vector, expressed as 16-bit half-precision floating-point
> > numbers).
> > 
> > Add support for a new sensor instance that allows receiving sensor
> > fusion
> > data, by defining a new struct st_lsm6dsx_sf_settings (which contains
> > chip-specific details for the sensor fusion functionality), and adding
> > this
> > struct as a new field in struct st_lsm6dsx_settings. In
> > st_lsm6dsx_core.c,
> > populate this new struct for the LSM6DSV and LSM6DSV16X chips, and add
> > the
> > logic to initialize an additional IIO device if this struct is
> > populated
> > for the hardware type being probed.
> > Note: a new IIO device is being defined (as opposed to adding channels
> > to
> > an existing device) because each of the existing devices handles data
> > coming from a single sensor, while sensor fusion data comes from
> > multiple
> > sensors.
> That doesn't really justify the extra IIO device.  We used to do this
> because the IIO buffers aren't tagged with channel info
> (unlike say driver/input) and so if the data for different channels
> /sub parts of the sensor can come out at different rates, we have to
> split them up.  So real question here is what data rate is this generated
> at?  If it matches an existing sensor we should add the channel there,
> if not fine to have yet another IIO device. All this changes is
> the explanation in this patch description. Code is fine.

The data rate from the rotation sensor may not match the rate from any of
the existing sensors. I changed the explanation in the patch description.

> Note as an FYI, we now support multiple buffers per IIO device. I think
> there are probably still some corners that need ironing out, but in
> theory
> that was to handle this sort of sensor in a single IIO device.
> 
> > 
> > Tested on LSMDSV16X.
> > 
> > Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> 
> 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
> > b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
> > new file mode 100644
> > index 000000000000..7c78f14cbb91
> > --- /dev/null
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * STMicroelectronics st_lsm6dsx IMU sensor fusion
> > + */
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/sprintf.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +
> > +#include "st_lsm6dsx.h"
> 
> > +/**
> > + * st_lsm6dsx_sf_set_page - Enable or disable access to sensor fusion
> > + * configuration registers.
> > + * @hw: Sensor hardware instance.
> > + * @enable: True to enable access, false to disable access.
> > + *
> > + * The register page lock is acquired when enabling access, and
> > released when
> > + * disabling access. Therefore, a function call with @enable set to
> > true must be
> > + * followed by a call with @enable set to false (unless the first call
> > returns
> > + * an error value).
> > + *
> > + * Return: 0 on success, negative value on error.
> > + */
> > +static int st_lsm6dsx_sf_set_page(struct st_lsm6dsx_hw *hw, bool
> > enable)
> > +{
> 
> I'm curious what sparse thinks of this.  Try a make C=1 build.
> I'd expect it to be fussy that it can't figure out the acquires and
> releases.
> Maybe it can see enough to figure it out.  If not you might need to break
> it into two functions so you can add the markings __acquires() etc
> 
> Personally I think I'd just take the page lock out of here and use
> guard() in the
> callers.

Building with make C=1 doesn't generate any sparse warnings (sparse is not
invoked for this source file). On the other hand, if I break this function
into two functions with the __acquires() and __releases() annotations,
sparse generates a "context imbalance - wrong count at exit" warning for
both functions (and their callers).
I ended up just taking the page lock out of here and using guard() in the
callers.

> > +       const struct st_lsm6dsx_reg *mux;
> > +       int err;
> > +
> > +       mux = &hw->settings->sf_settings.page_mux;
> 
> Just to check, can you use regmaps support for paging instead of doing
> this by hand?
> That would avoid future problems with enabling caching or similar

No, the chip has multiple register sets with overlapping addresses, and
struct regmap_range_cfg is not usable here.

> 
> > +
> > +static
> > IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sf_sampling_freq_avail);
> > +static struct attribute *st_lsm6dsx_sf_attributes[] = {
> > +       &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > +       NULL,
> 
> No comma as nothing should come after this.  Ideally would be replaced
> with
> code using the read_avail callback and appropriate bit set in the
> info mask avail bitmaps.

The read_avail callback would require available values to be in an int
array, with one of the available IIO_VAL_* formats, but the driver uses a
table (similarly to other parts of the existing code) where frequency
values are expressed in mHz (which does not match any IIO_VAL_* format) and
are interleaved with corresponding hardware register values. So we can't
use the existing available frequency values in a read_avail callback.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/3] iio: imu: st_lsm6dsx: set buffer sampling frequency for accelerometer only

[Jonathan Cameron]

On Mon, 12 Jan 2026 18:10:32 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> On Sun, 2026-01-11 at 16:18 +0000, Jonathan Cameron wrote:
> > On Fri,  9 Jan 2026 19:15:26 +0100
> > Francesco Lavra <flavra@baylibre.com> wrote:
> >   
> > > The st_lsm6dsx_hwfifo_odr_store() function, which is called when
> > > userspace
> > > writes the buffer sampling frequency sysfs attribute, calls
> > > st_lsm6dsx_check_odr(), which accesses the odr_table array at index
> > > `sensor->id`; since this array is only 2 entries long, an access for
> > > any
> > > sensor type other than accelerometer or gyroscope is an out-of-bounds
> > > access.
> > > 
> > > To prevent userspace from triggering an out-of-bounds array access, and
> > > to
> > > support the only use case for which FIFO sampling frequency values
> > > different from the sensor sampling frequency may be needed (which is
> > > for
> > > keeping FIFO data rate low while sampling acceleration data at high
> > > rates
> > > for accurate event detection), do not create the buffer sampling
> > > frequency
> > > attribute for sensor types other than the accelerometer.  
> > 
> > I'm not following why we need to drop this attribute for the gyroscope.
> > Perhaps lay out what the combinations of controls are and the attributes
> > we end up with.  
> 
> It's not like we need to drop this attribute, it's just that I don't see a
> need for it. The only reason I added this attribute was to be able to
> control (e.g. lower) the rate of data coming from the sensor while
> maintaining a high accuracy for event detection; and accurate event
> detection requires a high sampling rate for the accelerometer.

Ok. So key here is for accelerations we are looking at impacts as a typical
use case, whereas gyroscope tends to be slow orientation change stuff.
That sounds a bit usecase specific. If someone is using these to detect shaft rotation
issues they are going to care about sampling rates on the gyro as well,
or is there something inherent in the gyroscope events (i.e. maybe there
aren't any gyro events?) that makes this not relevant?

> So the
> gyroscope is not involved here, and the attribute is only needed for the
> accelerometer.
> 
> Before this change, we have:
> - accel IIO device with separate samp_freq and buffer/samp_freq
> - gyro IIO device with separate samp_freq and buffer/samp_freq
> - (optionally) external sensor IIO devices with separate samp_freq and
> buffer/samp_freq (and trying to set buffer/samp_freq for these triggers an
> out-of-bounds array access)
> 
> After this change, we have the accel IIO device with separate samp_freq and
> buffer/samp_freq, while the other IIO devices have only a single samp_freq
> attribute.
> 
> > As you note in the cover letter we can change this now with ABI issues as
> > it is just in my tree, so I don't mind the change, just want to
> > understand
> > it a little better than I currently do!  
> 
> It's not just in your tree, it has been pulled into Linus's tree for 6.19.
Bit tight to get a fix in, though I gather we are going to rc8 this time
so maybe.

Jonathan

> 

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor

[Jonathan Cameron]

> 
> > > +       const struct st_lsm6dsx_reg *mux;
> > > +       int err;
> > > +
> > > +       mux = &hw->settings->sf_settings.page_mux;  
> > 
> > Just to check, can you use regmaps support for paging instead of doing
> > this by hand?
> > That would avoid future problems with enabling caching or similar  
> 
> No, the chip has multiple register sets with overlapping addresses, and
> struct regmap_range_cfg is not usable here.

Paged addressing always has overlapping addresses, so I'm not following.
The trick this does is to map those higher pages to a fake set of addresses.
An example is the ICM42600:
https://invensense.tdk.com/wp-content/uploads/2020/04/ds-000347_icm-42688-p-datasheet.pdf
for a suitable datasheet (some of the newer parts have a much more complex scheme)

I took a look at the datasheet and seems there are multiple types of paging
going on and effectively ends up with two nested paging controls. So fair enough it
doesn't fit here.

> 
> >   
> > > +
> > > +static
> > > IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sf_sampling_freq_avail);
> > > +static struct attribute *st_lsm6dsx_sf_attributes[] = {
> > > +       &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > > +       NULL,  
> > 
> > No comma as nothing should come after this.  Ideally would be replaced
> > with
> > code using the read_avail callback and appropriate bit set in the
> > info mask avail bitmaps.  
> 
> The read_avail callback would require available values to be in an int
> array, with one of the available IIO_VAL_* formats, but the driver uses a
> table (similarly to other parts of the existing code) where frequency
> values are expressed in mHz (which does not match any IIO_VAL_* format) and
> are interleaved with corresponding hardware register values. So we can't
> use the existing available frequency values in a read_avail callback.

You would need to do some data mangling to create the relevant table in order
to use that callback. We can always do that later I suppose if we find
that it matters for some inkernel consumer or other reason.

Jonathan

> 

Re: [PATCH 1/3] iio: imu: st_lsm6dsx: set buffer sampling frequency for accelerometer only

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

On Fri, 2026-01-16 at 18:03 +0000, Jonathan Cameron wrote:
> On Mon, 12 Jan 2026 18:10:32 +0100
> Francesco Lavra <flavra@baylibre.com> wrote:
> 
> > On Sun, 2026-01-11 at 16:18 +0000, Jonathan Cameron wrote:
> > > On Fri,  9 Jan 2026 19:15:26 +0100
> > > Francesco Lavra <flavra@baylibre.com> wrote:
> > >   
> > > > The st_lsm6dsx_hwfifo_odr_store() function, which is called when
> > > > userspace
> > > > writes the buffer sampling frequency sysfs attribute, calls
> > > > st_lsm6dsx_check_odr(), which accesses the odr_table array at index
> > > > `sensor->id`; since this array is only 2 entries long, an access
> > > > for
> > > > any
> > > > sensor type other than accelerometer or gyroscope is an out-of-
> > > > bounds
> > > > access.
> > > > 
> > > > To prevent userspace from triggering an out-of-bounds array access,
> > > > and
> > > > to
> > > > support the only use case for which FIFO sampling frequency values
> > > > different from the sensor sampling frequency may be needed (which
> > > > is
> > > > for
> > > > keeping FIFO data rate low while sampling acceleration data at high
> > > > rates
> > > > for accurate event detection), do not create the buffer sampling
> > > > frequency
> > > > attribute for sensor types other than the accelerometer.  
> > > 
> > > I'm not following why we need to drop this attribute for the
> > > gyroscope.
> > > Perhaps lay out what the combinations of controls are and the
> > > attributes
> > > we end up with.  
> > 
> > It's not like we need to drop this attribute, it's just that I don't
> > see a
> > need for it. The only reason I added this attribute was to be able to
> > control (e.g. lower) the rate of data coming from the sensor while
> > maintaining a high accuracy for event detection; and accurate event
> > detection requires a high sampling rate for the accelerometer.
> 
> Ok. So key here is for accelerations we are looking at impacts as a
> typical
> use case, whereas gyroscope tends to be slow orientation change stuff.
> That sounds a bit usecase specific. If someone is using these to detect
> shaft rotation
> issues they are going to care about sampling rates on the gyro as well,
> or is there something inherent in the gyroscope events (i.e. maybe there
> aren't any gyro events?) that makes this not relevant?

All the events supported by this driver (motion detection and tap
detection) use acceleration data only.
Some chip variants (e.g LSM6DSV) have more advanced features such as
configurable finite state machines that can take inputs from both the
accelerometer and the gyroscope and generate event interrupts; but I don't
think these events would map cleanly to standard IIO event types.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

On Fri, 2026-01-16 at 18:27 +0000, Jonathan Cameron wrote:
> 
> > 
> > > > +       const struct st_lsm6dsx_reg *mux;
> > > > +       int err;
> > > > +
> > > > +       mux = &hw->settings->sf_settings.page_mux;  
> > > 
> > > Just to check, can you use regmaps support for paging instead of
> > > doing
> > > this by hand?
> > > That would avoid future problems with enabling caching or similar  
> > 
> > No, the chip has multiple register sets with overlapping addresses, and
> > struct regmap_range_cfg is not usable here.
> 
> Paged addressing always has overlapping addresses, so I'm not following.
> The trick this does is to map those higher pages to a fake set of
> addresses.

I thought having a fake set of addresses in the driver would be frowned
upon since it would create a mismatch between the code and the device
datasheet. But I guess it wouldn't be so bad if appropriate offsets between
register pages are chosen.
Anyway, as you noted in this case the different pages cannot be selected
via a simple bank selector register.

> An example is the ICM42600:
> https://invensense.tdk.com/wp-content/uploads/2020/04/ds-000347_icm-42688-p-datasheet.pdf
> for a suitable datasheet (some of the newer parts have a much more
> complex scheme)
> 
> I took a look at the datasheet and seems there are multiple types of
> paging
> going on and effectively ends up with two nested paging controls. So fair
> enough it
> doesn't fit here.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/3] iio: imu: st_lsm6dsx: set buffer sampling frequency for accelerometer only

[Jonathan Cameron]

On Fri, 16 Jan 2026 20:06:46 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> On Fri, 2026-01-16 at 18:03 +0000, Jonathan Cameron wrote:
> > On Mon, 12 Jan 2026 18:10:32 +0100
> > Francesco Lavra <flavra@baylibre.com> wrote:
> >   
> > > On Sun, 2026-01-11 at 16:18 +0000, Jonathan Cameron wrote:  
> > > > On Fri,  9 Jan 2026 19:15:26 +0100
> > > > Francesco Lavra <flavra@baylibre.com> wrote:
> > > >     
> > > > > The st_lsm6dsx_hwfifo_odr_store() function, which is called when
> > > > > userspace
> > > > > writes the buffer sampling frequency sysfs attribute, calls
> > > > > st_lsm6dsx_check_odr(), which accesses the odr_table array at index
> > > > > `sensor->id`; since this array is only 2 entries long, an access
> > > > > for
> > > > > any
> > > > > sensor type other than accelerometer or gyroscope is an out-of-
> > > > > bounds
> > > > > access.
> > > > > 
> > > > > To prevent userspace from triggering an out-of-bounds array access,
> > > > > and
> > > > > to
> > > > > support the only use case for which FIFO sampling frequency values
> > > > > different from the sensor sampling frequency may be needed (which
> > > > > is
> > > > > for
> > > > > keeping FIFO data rate low while sampling acceleration data at high
> > > > > rates
> > > > > for accurate event detection), do not create the buffer sampling
> > > > > frequency
> > > > > attribute for sensor types other than the accelerometer.    
> > > > 
> > > > I'm not following why we need to drop this attribute for the
> > > > gyroscope.
> > > > Perhaps lay out what the combinations of controls are and the
> > > > attributes
> > > > we end up with.    
> > > 
> > > It's not like we need to drop this attribute, it's just that I don't
> > > see a
> > > need for it. The only reason I added this attribute was to be able to
> > > control (e.g. lower) the rate of data coming from the sensor while
> > > maintaining a high accuracy for event detection; and accurate event
> > > detection requires a high sampling rate for the accelerometer.  
> > 
> > Ok. So key here is for accelerations we are looking at impacts as a
> > typical
> > use case, whereas gyroscope tends to be slow orientation change stuff.
> > That sounds a bit usecase specific. If someone is using these to detect
> > shaft rotation
> > issues they are going to care about sampling rates on the gyro as well,
> > or is there something inherent in the gyroscope events (i.e. maybe there
> > aren't any gyro events?) that makes this not relevant?  
> 
> All the events supported by this driver (motion detection and tap
> detection) use acceleration data only.
> Some chip variants (e.g LSM6DSV) have more advanced features such as
> configurable finite state machines that can take inputs from both the
> accelerometer and the gyroscope and generate event interrupts; but I don't
> think these events would map cleanly to standard IIO event types.
> 
Doh! The fact we don't support gyro events was the detail I was completely missing ;(

Please add that to the patch description.

Thanks

Jonathan