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

Jonathan Cameron
2025-10-10T17:44:42Z

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.

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