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

[Jonathan Cameron]

On Thu,  9 Oct 2025 19:36:09 +0200
Francesco Lavra <flavra@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>

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