Re: [PATCH 2/2] iio: imu: st_lsm6dsx: Decouple sensor ODR from FIFO batch data rate

[Francesco Lavra <flavra@baylibre.com>]

On Fri, 2025-10-10 at 18:44 +0100, Jonathan Cameron wrote:
> On Thu,  9 Oct 2025 19:36:09 +0200
> Francesco Lavra <flavra@baylibre.com> wrote:
> 
> > The rate at which accelerometer or gyroscope sensor samples are fed
> > to the hardware FIFO (batch data rate, or BDR) does not have to
> > coincide with the sensor sampling frequency (output data rate, or
> > ODR); the only requirement is for the BDR to not be greater than
> > the ODR. Having a BDR lower than the ODR is useful in cases where
> > an application requires a high sampling rate for accurate detection
> > of motion events (e.g. wakeup events), but wants to read sensor
> > sample values from the device buffer at a lower data rate.
> > To support the above use case, add a sampling_frequency sysfs
> > attribute to the buffer directory of st_lsm6dsx IIO devices, which
> > controls the BDR for a given sensor independently from the "main"
> > sampling_frequency attribute (which controls the ODR); introduce a
> > new `bdr` field in struct st_lsm6dsx_sensor to keep track of the
> > current BDR value, and use this field instead of the `odr` field in
> > the code that deals with the FIFO data rate. In the sensor hub
> > driver, make the bdr value always mirror the odr value, since there
> > is no separate configuration setting to control the BDR for data
> > produced by the sensor hub functionality.
> > 
> > Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> 
> A few additional trivial things from me.  In general this looks fine.
> Whilst that buffer/sampling_frequency isn't common it's been part
> of the ABI for a while for this sort of thing.
> 
> My only slight concern is backwards compatibility. 
> Perhaps you can add something on what happens if main sampling_frequency
> is modified by a user who doesn't know anything about
> buffer/sampling_frequency?
> 
> Given that's a new interface and the ABI always allows a write to one
> value to change any other maybe we have to say the main sampling
> frequency
> write updates the buffer one and a write to the buffer one after that is
> needed
> to set it to a different value?
> 
> That is a bit ugly but it is backwards compatible I think.

Yes, for backwards compatibility it makes sense to update the buffer
frequency whenever the main frequency is set. Will do.

OK also for the cosmetic changes below.

> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > index 8a9d2593576a..5912ea76d493 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > @@ -56,6 +56,7 @@
> >  #include <linux/iio/kfifo_buf.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/buffer.h>
> > +#include <linux/iio/sysfs.h>
> >  #include <linux/regmap.h>
> >  #include <linux/bitfield.h>
> >  
> > @@ -105,7 +106,7 @@ static int
> >  st_lsm6dsx_get_decimator_val(struct st_lsm6dsx_sensor *sensor, u32
> > max_odr)
> >  {
> >         const int max_size = ARRAY_SIZE(st_lsm6dsx_decimator_table);
> > -       u32 decimator =  max_odr / sensor->odr;
> > +       u32 decimator =  max_odr / sensor->bdr;
> 
> No idea why there is a bonus space after = but good to cleanup whilst you
> are
> here.
> 
> >         int i;
> 
> > +static ssize_t st_lsm6dsx_bdr_store(struct device *dev,
> > +                                   struct device_attribute *attr,
> > +                                   const char *buf, size_t len)
> > +{
> > +       struct iio_dev *iio_dev = dev_to_iio_dev(dev);
> > +       struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > +       int integer, fract;
> > +       int ret;
> > +       u32 bdr;
> > +       u8 data;
> > +
> > +       ret = iio_str_to_fixpoint(buf, 100, &integer, &fract);
> > +       if (ret)
> > +               return ret;
> 
> Add blank line after this sort of error handling return.  Slightly helps
> with readability.
> 
> > +       bdr = integer * 1000 + fract;
> > +       ret = st_lsm6dsx_check_odr(sensor, bdr, &data);
> > +       if (ret < 0)
> > +               return ret;
> Here as well.
> > +       bdr = ret;
> 
> Probably here as well.
> 
> > +       if (!iio_device_claim_direct(iio_dev))
> > +               return -EBUSY;
> > +       /* the batch data rate must not exceed the sensor output data
> > rate */
> > +       if (bdr <= sensor->odr)
> > +               sensor->bdr = bdr;
> > +       else
> > +               ret = -EINVAL;
> > +       iio_device_release_direct(iio_dev);
> Add one before the final return.
> > +       return (ret < 0) ? ret : len;
> > +}