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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Jan 19, 2026 at 11:04:49AM +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 the rate at which sensor fusion data is
> generated may not match the data rate from any of the existing devices.
> 
> Tested on LSMDSV16X.

...

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

This looks confusing taking into account

	s16 val = le16_to_cpu(*(__le16 *)data);

(which actually should use le16_to_cpup() instead of the direct conversion).

> +		iio_dev = hw->iio_devs[ST_LSM6DSX_ID_SF];
> +		break;
>  	default:
>  		return -EINVAL;
>  	}

...

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

Do you need ' < 0' here? Why isn't it required at the end of the function?

> +		return err;
> +
> +	err = regmap_assign_bits(hw->regmap, en_reg->addr, en_reg->mask, enable);
> +	if (err < 0) {

' < 0' is not needed.

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

And IIRC I replied that these _sf_set_page() seems to be used only with
the explicit boolean value, which means a good hint that it needs to be
split just to two functions doing for true and false. It will increase
the readability of the code in both places (in the caller and callee).

...

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

Can we use reversed xmas tree ordering?

> +		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);
> +	if (err < 0)
> +		goto out;
> +
> +	return st_lsm6dsx_sf_set_page(hw, false);
> +
> +out:
> +	st_lsm6dsx_sf_set_page(hw, false);
> +
> +	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 = val * MILLI + val2 * MILLI / MICRO;

Probably "(MILLI / MICRO)" (with parentheses) to avoid potential overflows,

> +		u8 odr_val;
> +
> +		/* check that the requested frequency is supported */
> +		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;
> +	}
> +}

...

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

Does GCC complain on this (`make W=1` build)?
Since this can cut the string and we don't check the return value, the Q is:
is this okay to have a reduced string?

-- 
With Best Regards,
Andy Shevchenko