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

[PATCH v2 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 second commit is a fix to a previous commit of mine [*] 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.

Changes from v1 [1]:
- swapped patches 1 and 2 (Jonathan)
- miscellaneous stylistic changes (Andy)
- fixed usage of MICRO and MILLI constants in st_lsm6dsx_sf_read_raw and
  st_lsm6dsx_sf_write_raw (Andy)
- replaced scnprintf() with sysfs_emit_at() in
  st_lsm6dsx_sf_sampling_freq_avail (Andy)
- replaced scnprintf() with snprintf() in st_lsm6dsx_sf_probe (Andy)
- clarified in a comment in st_lsm6dsx_set_fifo_odr() that only internal
  sensors have a FIFO ODR configuration register (Jonathan)
- modified patch 3 description to explain justification for the extra IIO
  device (Jonathan)
- moved page lock from st_lsm6dsx_sf_set_page() to the callers (Jonathan)
- s/magnetometer/gyroscope/ in patch 2 description

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

Francesco Lavra (3):
  iio: imu: st_lsm6dsx: set FIFO ODR for accelerometer and gyroscope
    only
  iio: imu: st_lsm6dsx: set buffer sampling frequency for accelerometer
    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    |  29 ++-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  |  58 +++++
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c    | 212 ++++++++++++++++++
 5 files changed, 320 insertions(+), 7 deletions(-)
 create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c

-- 
2.39.5

[PATCH v2 1/3] iio: imu: st_lsm6dsx: Set FIFO ODR for accelerometer and gyroscope 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 gyroscope; 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 55d877745575..1ee2fc5f5f1f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -225,6 +225,10 @@ static int st_lsm6dsx_set_fifo_odr(struct st_lsm6dsx_sensor *sensor,
 	const struct st_lsm6dsx_reg *batch_reg;
 	u8 data;
 
+	/* 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;
-- 
2.39.5

[PATCH v2 2/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 1ee2fc5f5f1f..cde29b2e6f34 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -862,12 +862,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 v2 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 the rate at which sensor fusion data is
generated may not match the data rate from any of the existing devices.

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    | 212 ++++++++++++++++++
 5 files changed, 307 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_fifo_mode {
@@ -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 cde29b2e6f34..1846b9f84c29 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 internal sensors have a FIFO ODR configuration register. */
-	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) {
@@ -580,6 +584,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,				\
+}
+
 static const struct iio_event_spec st_lsm6dsx_ev_motion[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -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[] = {
 				},
 			},
 		},
+		.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..3594d97a98ff
--- /dev/null
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
@@ -0,0 +1,212 @@
+// 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.
+ *
+ * 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 = &hw->settings->sf_settings.page_mux;
+
+	return regmap_assign_bits(hw->regmap, mux->addr, mux->mask, enable);
+}
+
+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 *en_reg;
+	int err;
+
+	guard(mutex)(&hw->page_lock);
+
+	en_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, en_reg->addr, en_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;
+
+	guard(mutex)(&hw->page_lock);
+
+	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) * (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 / MICRO;
+		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
+		if (err)
+			return err;
+
+		sensor->hwfifo_odr_mHz = odr_mHz;
+		return 0;
+	}
+	default:
+		return -EINVAL;
+	}
+}
+
+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 len = 0;
+
+	settings = &sensor->hw->settings->sf_settings;
+	for (unsigned int i = 0; i < settings->odr_table.odr_len; i++) {
+		u32 val = settings->odr_table.odr_avl[i].milli_hz;
+
+		len += sysfs_emit_at(buf, 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;
+	snprintf(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 v2 1/3] iio: imu: st_lsm6dsx: Set FIFO ODR for accelerometer and gyroscope only

[Lorenzo Bianconi]

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

> 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 gyroscope; 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 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index 55d877745575..1ee2fc5f5f1f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -225,6 +225,10 @@ static int st_lsm6dsx_set_fifo_odr(struct st_lsm6dsx_sensor *sensor,
>  	const struct st_lsm6dsx_reg *batch_reg;
>  	u8 data;
>  
> +	/* Only internal sensors have a FIFO ODR configuration register. */
> +	if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
> +		return 0;

I guess it is more clear to check if the sensor is acc or gyro here.
What do you think? Something like:

	if (sensor->id != ST_LSM6DSX_ID_GYRO &&
	    sensor->id != ST_LSM6DSX_ID_ACC)
	    return 0;

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/3] iio: imu: st_lsm6dsx: Set buffer sampling frequency for accelerometer only

[Lorenzo Bianconi]

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

> 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 1ee2fc5f5f1f..cde29b2e6f34 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -862,12 +862,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;

Nit: you do not need brackets here:
		attrs = i == ST_LSM6DSX_ID_ACC ? st_lsm6dsx_buffer_attrs : NULL;

Regards,
Lorenzo

>  		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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Lorenzo Bianconi]

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

> 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 the rate at which sensor fusion data is
> generated may not match the data rate from any of the existing devices.
> 
> Tested on LSMDSV16X.

Nice feature, thx for working on this. I think the patch is fine,
just a couple of Nits inline.

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> 
> 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    | 212 ++++++++++++++++++
>  5 files changed, 307 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_fifo_mode {
> @@ -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 cde29b2e6f34..1846b9f84c29 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 internal sensors have a FIFO ODR configuration register. */
> -	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);

Nit: please align this to the comment in patch 1/3

>  		return 0;
> +	}
>  
>  	batch_reg = &hw->settings->batch[sensor->id];
>  	if (batch_reg->addr) {
> @@ -580,6 +584,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,				\
> +}
> +
>  static const struct iio_event_spec st_lsm6dsx_ev_motion[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -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[] = {
>  				},
>  			},
>  		},
> +		.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..3594d97a98ff
> --- /dev/null
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
> @@ -0,0 +1,212 @@
> +// 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.
> + *
> + * 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 = &hw->settings->sf_settings.page_mux;
> +
> +	return regmap_assign_bits(hw->regmap, mux->addr, mux->mask, enable);
> +}
> +
> +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 *en_reg;
> +	int err;
> +
> +	guard(mutex)(&hw->page_lock);
> +
> +	en_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, en_reg->addr, en_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;
> +
> +	guard(mutex)(&hw->page_lock);
> +
> +	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;
> +	}

Nit: new-line here.

> +	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) * (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;

I guess err = -EINVAL;

> +
> +	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 / MICRO;
> +		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
> +		if (err)
> +			return err;
> +
> +		sensor->hwfifo_odr_mHz = odr_mHz;
> +		return 0;

break;

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

break;

> +	}

return err;

> +}
> +
> +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 len = 0;
> +
> +	settings = &sensor->hw->settings->sf_settings;
> +	for (unsigned int i = 0; i < settings->odr_table.odr_len; i++) {
> +		u32 val = settings->odr_table.odr_avl[i].milli_hz;
> +
> +		len += sysfs_emit_at(buf, 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;
> +	snprintf(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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Andy Shevchenko]

On Thu, Jan 15, 2026 at 02:32:11PM +0100, Lorenzo Bianconi 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 the rate at which sensor fusion data is
> > generated may not match the data rate from any of the existing devices.
> > 
> > Tested on LSMDSV16X.

...

> > +	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 / MICRO;
> > +		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
> > +		if (err)
> > +			return err;
> > +
> > +		sensor->hwfifo_odr_mHz = odr_mHz;
> > +		return 0;
> 
> break;
> 
> > +	}
> > +	default:
> > +		return -EINVAL;
> 
> break;
> 
> > +	}
> 
> return err;

Why?

-- 
With Best Regards,
Andy Shevchenko

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

[Lorenzo Bianconi]

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

On Jan 15, Andy Shevchenko wrote:
> On Thu, Jan 15, 2026 at 02:32:11PM +0100, Lorenzo Bianconi 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 the rate at which sensor fusion data is
> > > generated may not match the data rate from any of the existing devices.
> > > 
> > > Tested on LSMDSV16X.
> 
> ...
> 
> > > +	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 / MICRO;
> > > +		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
> > > +		if (err)
> > > +			return err;
> > > +
> > > +		sensor->hwfifo_odr_mHz = odr_mHz;
> > > +		return 0;
> > 
> > break;
> > 
> > > +	}
> > > +	default:
> > > +		return -EINVAL;
> > 
> > break;
> > 
> > > +	}
> > 
> > return err;
> 
> Why?

I guess it is more clear, but just a Nit, I can live with the current implementation.

Regards,
Lorenzo

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Andy Shevchenko]

On Thu, Jan 15, 2026 at 02:51:43PM +0100, Lorenzo Bianconi wrote:
> On Jan 15, Andy Shevchenko wrote:
> > On Thu, Jan 15, 2026 at 02:32:11PM +0100, Lorenzo Bianconi wrote:

...

> > > > +	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 / MICRO;
> > > > +		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
> > > > +		if (err)
> > > > +			return err;
> > > > +
> > > > +		sensor->hwfifo_odr_mHz = odr_mHz;
> > > > +		return 0;
> > > 
> > > break;
> > > 
> > > > +	}
> > > > +	default:
> > > > +		return -EINVAL;
> > > 
> > > break;
> > > 
> > > > +	}
> > > 
> > > return err;
> > 
> > Why?
> 
> I guess it is more clear, but just a Nit, I can live with the current implementation.

At least what you proposed is the opposite to the style that is accepted
in IIO.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] iio: imu: st_lsm6dsx: Set FIFO ODR for accelerometer and gyroscope only

[Jonathan Cameron]

On Thu, 15 Jan 2026 14:13:01 +0100
Lorenzo Bianconi <lorenzo@kernel.org> 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 gyroscope; 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 | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > index 55d877745575..1ee2fc5f5f1f 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > @@ -225,6 +225,10 @@ static int st_lsm6dsx_set_fifo_odr(struct st_lsm6dsx_sensor *sensor,
> >  	const struct st_lsm6dsx_reg *batch_reg;
> >  	u8 data;
> >  
> > +	/* Only internal sensors have a FIFO ODR configuration register. */
> > +	if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
> > +		return 0;  
> 
> I guess it is more clear to check if the sensor is acc or gyro here.
> What do you think? Something like:
> 
> 	if (sensor->id != ST_LSM6DSX_ID_GYRO &&
> 	    sensor->id != ST_LSM6DSX_ID_ACC)
> 	    return 0;

Disadvantage is that to check for overflow we have to know those are 0 and 1.
I'm not sure which is better of the two here. One is more logically correct
the other is easier to review :)

> 
> Regards,
> Lorenzo
> 
> > +
> >  	batch_reg = &hw->settings->batch[sensor->id];
> >  	if (batch_reg->addr) {
> >  		int val;
> > -- 
> > 2.39.5
> >   

Re: [PATCH v2 2/3] iio: imu: st_lsm6dsx: Set buffer sampling frequency for accelerometer only

[Jonathan Cameron]

On Thu, 15 Jan 2026 13:24:30 +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.

If we are going to do this, I'd like a little more discussion of why this
matters for accelerometer events and not gyroscope ones.
(see follow up on v1 discussion I posted earlier today).

Thanks,

Jonathan

> 
> 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 1ee2fc5f5f1f..cde29b2e6f34 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -862,12 +862,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 v2 3/3] iio: imu: st_lsm6dsx: Add support for rotation sensor

[Jonathan Cameron]

On Thu, 15 Jan 2026 15:58:36 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Jan 15, 2026 at 02:51:43PM +0100, Lorenzo Bianconi wrote:
> > On Jan 15, Andy Shevchenko wrote:  
> > > On Thu, Jan 15, 2026 at 02:32:11PM +0100, Lorenzo Bianconi wrote:  
> 
> ...
> 
> > > > > +	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 / MICRO;
> > > > > +		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
> > > > > +		if (err)
> > > > > +			return err;
> > > > > +
> > > > > +		sensor->hwfifo_odr_mHz = odr_mHz;
> > > > > +		return 0;  
> > > > 
> > > > break;
> > > >   
> > > > > +	}
> > > > > +	default:
> > > > > +		return -EINVAL;  
> > > > 
> > > > break;
> > > >   
> > > > > +	}  
> > > > 
> > > > return err;  
> > > 
> > > Why?  
> > 
> > I guess it is more clear, but just a Nit, I can live with the current implementation.  
> 
> At least what you proposed is the opposite to the style that is accepted
> in IIO.
> 

My preference is always to minimize flow I need to follow.
If there is a break, I expect to see something to do after the switch
statement and will scroll down to check what it is. 

Whilst not the only use, it's one of the reasons cleanup.h stuff is
really handy as it often means there is nothing common to do by
hand before leaving a function.

Jonathan

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

[Jonathan Cameron]

On Thu, 15 Jan 2026 13:24:31 +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 the rate at which sensor fusion data is
> generated may not match the data rate from any of the existing devices.
> 
> Tested on LSMDSV16X.
> 
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>

Nice. A couple of really minor comments inline.

Jonathan

> 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..3594d97a98ff
> --- /dev/null
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c

> +
> +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 *en_reg;
> +	int err;
> +
> +	guard(mutex)(&hw->page_lock);
> +
> +	en_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, en_reg->addr, en_reg->mask, enable);
> +	st_lsm6dsx_sf_set_page(hw, false);

Similar to below. Feels odd to check all the other accesses but not the one
setting the page back to normal page.

> +
> +	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;
> +
> +	guard(mutex)(&hw->page_lock);
> +
> +	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);

Whilst there isn't much we can do about it, might be worth the dance
to check return value of this (but not overwrite the original one if
we came here via an error.

Maybe simply duplicate the call
	return st_lsm6dsx_sf_set_page(hw, false);

out:
	st_lsm6dsx_sf_set_page(hw, false);

	return err;


> +
> +	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 / MICRO;
> +		err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);

Why are we getting something we don't use in here? I guess this is checking
what was asked for is supported. I'd add a comment to make that clear
as it's not that obvious (I got confused anyway!)

> +		if (err)
> +			return err;
> +
> +		sensor->hwfifo_odr_mHz = odr_mHz;
> +		return 0;
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}

Re: [PATCH v2 1/3] iio: imu: st_lsm6dsx: Set FIFO ODR for accelerometer and gyroscope only

[Francesco Lavra]

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

On Fri, 2026-01-16 at 19:47 +0000, Jonathan Cameron wrote:
> On Thu, 15 Jan 2026 14:13:01 +0100
> Lorenzo Bianconi <lorenzo@kernel.org> 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 gyroscope; 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 | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > index 55d877745575..1ee2fc5f5f1f 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > @@ -225,6 +225,10 @@ static int st_lsm6dsx_set_fifo_odr(struct
> > > st_lsm6dsx_sensor *sensor,
> > >         const struct st_lsm6dsx_reg *batch_reg;
> > >         u8 data;
> > >  
> > > +       /* Only internal sensors have a FIFO ODR configuration
> > > register. */
> > > +       if (sensor->id >= ARRAY_SIZE(hw->settings->batch))
> > > +               return 0;  
> > 
> > I guess it is more clear to check if the sensor is acc or gyro here.
> > What do you think? Something like:
> > 
> >         if (sensor->id != ST_LSM6DSX_ID_GYRO &&
> >             sensor->id != ST_LSM6DSX_ID_ACC)
> >             return 0;
> 
> Disadvantage is that to check for overflow we have to know those are 0
> and 1.
> I'm not sure which is better of the two here. One is more logically
> correct
> the other is easier to review :)

I'm keeping this as is, since there are pros and cons to changing it

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