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

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

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