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

[Francesco Lavra <flavra@baylibre.com>]

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

On Mon, 2026-01-19 at 12:33 +0200, Andy Shevchenko wrote:
> 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).

The above val variable is used to do a (val >= ST_LSM6DSX_INVALID_SAMPLE)
check, which actually should be done only if (tag == ST_LSM6DSX_ACC_TAG ||
tag == ST_LSM6DSX_GYRO_TAG).
I will add a patch to fix this (it will fix commit 960506ed2c69 "iio: imu:
st_lsm6dsx: enable drdy-mask if available")

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

Will drop ' < 0'

> > +               return err;
> > +
> > +       err = regmap_assign_bits(hw->regmap, en_reg->addr, en_reg-
> > >mask, enable);
> > +       if (err < 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).

OK

> ...
> 
> > +       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?

gcc does not complain with W=1. sensor->name is appropriately sized to
accommodate the longest possible name; if it wasn't, the string would be
cut in the accel and gyro IIO devices too (which use a longer suffix than
"_sf").


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