| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aPFUDr59szl_-Ckq@lore-desk>
Date: Thu, 16 Oct 2025 22:22:38 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Francesco Lavra <flavra@...libre.com>
Cc: Jonathan Cameron <jic23@...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 2/2] iio: imu: st_lsm6dsx: Decouple sensor ODR from FIFO
batch data rate
> 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 hardware FIFO at a lower data rate (e.g. to
> minimize the amount of I2C or SPI traffic and the rate of periodic
> interrupts).
> 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 `hwfifo_odr_mHz` 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 hwfifo_odr_mHz 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.
> For backwards compatibility, set the buffer frequency equal to the
> main frequency whenever the latter is updated via sysfs; if
> userspace wants a different buffer frequency, it has to write to
> the relevant sysfs attribute after any writes to the main frequency
> attribute.
>
> Signed-off-by: Francesco Lavra <flavra@...libre.com>
Hi Francesco,
I think the patch is fine, just few nits inline. Addressing them:
Acked-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 2 +
> .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 71 ++++++++++++++++---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 3 +-
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c | 3 +-
> 4 files changed, 69 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index bd366c6e282a..3cd520bdec46 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -366,6 +366,7 @@ enum st_lsm6dsx_fifo_mode {
> * @hw: Pointer to instance of struct st_lsm6dsx_hw.
> * @gain: Configured sensor sensitivity.
> * @odr: Output data rate of the sensor [mHz].
> + * hwfifo_odr_mHz: Batch data rate for hardware FIFO [mHz]
> * @samples_to_discard: Number of samples to discard for filters settling time.
> * @watermark: Sensor watermark level.
> * @decimator: Sensor decimation factor.
> @@ -380,6 +381,7 @@ struct st_lsm6dsx_sensor {
>
> u32 gain;
> u32 odr;
> + u32 hwfifo_odr_mHz;
>
> u16 samples_to_discard;
> u16 watermark;
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index 8a9d2593576a..7a66f06522c4 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->hwfifo_odr_mHz;
> int i;
>
> if (decimator > 1)
> @@ -136,14 +137,14 @@ static void st_lsm6dsx_get_max_min_odr(struct st_lsm6dsx_hw *hw,
> if (!(hw->enable_mask & BIT(sensor->id)))
> continue;
>
> - *max_odr = max_t(u32, *max_odr, sensor->odr);
> - *min_odr = min_t(u32, *min_odr, sensor->odr);
> + *max_odr = max(*max_odr, sensor->hwfifo_odr_mHz);
> + *min_odr = min(*min_odr, sensor->hwfifo_odr_mHz);
> }
> }
>
> static u8 st_lsm6dsx_get_sip(struct st_lsm6dsx_sensor *sensor, u32 min_odr)
> {
> - u8 sip = sensor->odr / min_odr;
> + u8 sip = sensor->hwfifo_odr_mHz / min_odr;
>
> return sip > 1 ? round_down(sip, 2) : sip;
> }
> @@ -231,7 +232,7 @@ static int st_lsm6dsx_set_fifo_odr(struct st_lsm6dsx_sensor *sensor,
> if (enable) {
> int err;
>
> - err = st_lsm6dsx_check_odr(sensor, sensor->odr,
> + err = st_lsm6dsx_check_odr(sensor, sensor->hwfifo_odr_mHz,
> &data);
> if (err < 0)
> return err;
> @@ -713,7 +714,7 @@ st_lsm6dsx_update_samples_to_discard(struct st_lsm6dsx_sensor *sensor)
>
> data = &hw->settings->samples_to_discard[sensor->id];
> for (i = 0; i < ST_LSM6DSX_ODR_LIST_SIZE; i++) {
> - if (data->val[i].milli_hz == sensor->odr) {
> + if (data->val[i].milli_hz == sensor->hwfifo_odr_mHz) {
> sensor->samples_to_discard = data->val[i].samples;
> return;
> }
> @@ -799,6 +800,59 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
> .postdisable = st_lsm6dsx_buffer_postdisable,
> };
>
> +static ssize_t st_lsm6dsx_hwfifo_odr_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct st_lsm6dsx_sensor *sensor = iio_priv(dev_to_iio_dev(dev));
> + u32 hwfifo_odr = sensor->hwfifo_odr_mHz;
nit: you can rid of this hwfifo_odr variable here and use
sensor->hwfifo_odr_mHz directly.
> +
> + return sysfs_emit(buf, "%d.%03d\n", hwfifo_odr / 1000, hwfifo_odr % 1000);
> +}
> +
> +static ssize_t st_lsm6dsx_hwfifo_odr_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, milli;
> + int ret;
> + u32 hwfifo_odr;
> + u8 data;
> +
> + if (!iio_device_claim_direct(iio_dev))
> + return -EBUSY;
> +
> + ret = iio_str_to_fixpoint(buf, 100, &integer, &milli);
> + if (ret)
> + goto out;
> +
> + hwfifo_odr = integer * 1000 + milli;
> + ret = st_lsm6dsx_check_odr(sensor, hwfifo_odr, &data);
> + if (ret < 0)
> + goto out;
> +
> + hwfifo_odr = ret;
> +
> + /* the batch data rate must not exceed the sensor output data rate */
> + if (hwfifo_odr <= sensor->odr)
> + sensor->hwfifo_odr_mHz = hwfifo_odr;
> + else
> + ret = -EINVAL;
> +
> +out:
> + iio_device_release_direct(iio_dev);
> +
> + return ret < 0 ? ret : len;
> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ(0664, st_lsm6dsx_hwfifo_odr_show, st_lsm6dsx_hwfifo_odr_store);
> +
> +static const struct iio_dev_attr *st_lsm6dsx_buffer_attrs[] = {
> + &iio_dev_attr_sampling_frequency,
> + NULL
> +};
> +
> int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
> {
> int i, ret;
> @@ -807,8 +861,9 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
> if (!hw->iio_devs[i])
> continue;
>
> - ret = devm_iio_kfifo_buffer_setup(hw->dev, hw->iio_devs[i],
> - &st_lsm6dsx_buffer_ops);
> + ret = devm_iio_kfifo_buffer_setup_ext(hw->dev, hw->iio_devs[i],
> + &st_lsm6dsx_buffer_ops,
> + st_lsm6dsx_buffer_attrs);
> if (ret)
> return ret;
> }
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index c65ad49829e7..055424303a4f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1850,7 +1850,7 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
> if (val < 0)
> err = val;
> else
> - sensor->odr = val;
> + sensor->odr = sensor->hwfifo_odr_mHz = val;
please avoid multi-assignment here.
> break;
> }
> default:
> @@ -2384,6 +2384,7 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> sensor->id = id;
> sensor->hw = hw;
> sensor->odr = hw->settings->odr_table[id].odr_avl[0].milli_hz;
> + sensor->hwfifo_odr_mHz = sensor->odr;
> sensor->gain = hw->settings->fs_table[id].fs_avl[0].gain;
> sensor->watermark = 1;
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> index 3c5e65dc0f97..039f588ef806 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> @@ -639,7 +639,7 @@ __st_lsm6dsx_shub_write_raw(struct iio_dev *iio_dev,
> return odr;
>
> sensor->ext_info.slv_odr = val;
> - sensor->odr = odr;
> + sensor->odr = sensor->hwfifo_odr_mHz = odr;
same here, one assignment per line.
> return 0;
> }
> case IIO_CHAN_INFO_SCALE:
> @@ -746,6 +746,7 @@ st_lsm6dsx_shub_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> sensor->id = id;
> sensor->hw = hw;
> sensor->odr = hw->settings->odr_table[ref_id].odr_avl[0].milli_hz;
> + sensor->hwfifo_odr_mHz = sensor->odr;
> sensor->ext_info.slv_odr = info->odr_table.odr_avl[0].milli_hz;
> sensor->gain = info->fs_table.fs_avl[0].gain;
> sensor->ext_info.settings = info;
> --
> 2.39.5
>
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists