Re: [PATCH v8 6/6] iio: imu: st_lsm6dsx: Add support for rotation sensor

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 17 Mar 2026 16:04:26 +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_fusion_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>
> Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
Hi Francesco

Just a few really minor last (hopefully!) things from me. On the whole
this set is looking very nice.

Jonathan

> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 450cb5b47346..160cc6551853 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c

> 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..74a2cc5cab12
> --- /dev/null
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_fusion.c
> @@ -0,0 +1,235 @@

> +
> +int st_lsm6dsx_fusion_set_odr(struct st_lsm6dsx_sensor *sensor, bool enable)
> +{
> +	const struct st_lsm6dsx_fusion_settings *settings;
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +	u8 data;
> +	int err;
> +
> +	guard(mutex)(&hw->page_lock);

I missed this before, but mixing cleanup and gotos is generally not
seen in a good light even when it doesn't introduce bugs.  See the comments in
cleanup.h  Easiest solution here is probably a helper function doing
the stuff after the fusion page is enabled.  That can return directly
and we can then handle any error out here.

> +
> +	err = st_lsm6dsx_fusion_page_enable(hw);
> +	if (err)
> +		return err;
> +
> +	settings = &hw->settings->fusion_settings;
> +	if (enable) {
> +		const struct st_lsm6dsx_reg *reg = &settings->odr_reg;
> +		u8 odr_val;
> +
> +		st_lsm6dsx_fusion_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)
> +			goto out;
> +	}
> +
> +	err = regmap_assign_bits(hw->regmap, settings->fifo_enable.addr,
> +				 settings->fifo_enable.mask, enable);
> +	if (err)
> +		goto out;
> +
> +	return st_lsm6dsx_fusion_page_disable(hw);
> +
> +out:
> +	st_lsm6dsx_fusion_page_disable(hw);
> +
> +	return err;
> +}

> +int st_lsm6dsx_fusion_probe(struct st_lsm6dsx_hw *hw, const char *name)
> +{

> +
> +	/*
> +	 *  Put the IIO device pointer in the iio_devs array so that the caller

Really minor but why the leading double spaces?  1 should be enough I think.

> +	 *  can set up a buffer and register this IIO device.
> +	 */
> +	hw->iio_devs[ST_LSM6DSX_ID_FUSION] = iio_dev;
> +
> +	return 0;
> +}