Message-ID: <d09ca6f843f75be5c5726eeab95063ea0e7c99cc.camel@baylibre.com>
Date: Tue, 13 Jan 2026 10:48:39 +0100
From: Francesco Lavra <flavra@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>, David Lechner
 <dlechner@...libre.com>,  Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>,  linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation
 sensor

On Sun, 2026-01-11 at 16:46 +0000, Jonathan Cameron wrote:
> On Fri,  9 Jan 2026 19:15:28 +0100
> Francesco Lavra <flavra@...libre.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@...libre.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.


Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)

Powered by blists - more mailing lists