Re: [PATCH v8 07/10] iio: pressure: dps310: read buffered samples from the hardware FIFO

[Jonathan Cameron]

On Mon, 21 Sep 2026 21:31:29 +0300
Rupesh Majhi <zoone.rupert@xxxxxxxxx> wrote:

> The DPS310 has a 32-entry FIFO shared by both measurements. Drain it from
> a work item and push what it held, so a buffered capture needs no
> trigger. Nothing in tree wires the interrupt pin, so the work rearms
> itself at half the time the FIFO takes to fill.
> 
> Entries carry one measurement each, so a pressure entry is compensated
> with the temperature ahead of it. Pressure read before the first
> temperature of a session is held until one arrives rather than dropped,
> so the first push can wait a temperature period.
> 
> FIFO entries are not timestamped, so postenable refuses the timestamp
> channel unless a trigger is attached.
> 
> Tested on a DPS310 on a BeagleBone Black.
> 
> Assisted-by: LLM
> Signed-off-by: Rupesh Majhi <zoone.rupert@xxxxxxxxx>
Take a look at what Sashiko had for this patch.
It raises the question on whether we can ever have a value that
is out of range in the fifo and as such fail to flush it out.

https://sashiko.dev/#/patchset/20260921183132.233136-1-zoone.rupert%40gmail.com

It also correctly raises that guard() and gotos should never be mixed in a
function.  See the comments in cleanup.h on this.


> ---
>  drivers/iio/pressure/dps310.c | 364 +++++++++++++++++++++++++++++++++-
>  1 file changed, 354 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index 85df58ac1809..ba59413d8e11 100644
> --- a/drivers/iio/pressure/dps310.c
> +++ b/drivers/iio/pressure/dps310.c


> +
> +static int dps310_buffer_postenable(struct iio_dev *iio)
> +{
> +	struct dps310_data *data = iio_priv(iio);
> +	int rc, prs_rate, tmp_rate;
> +
> +	/* An attached trigger drives the capture instead, FIFO stays off */
> +	if (iio_device_get_current_mode(iio) == INDIO_BUFFER_TRIGGERED)
> +		return 0;
> +
> +	/* Entries are not timestamped and the drain timer is no substitute */
> +	if (iio_scan_timestamp_enabled(iio))
> +		return -EINVAL;
> +
> +	guard(mutex)(&data->lock);

Sashiko spotted this one.  The rules are no combining this
with a goto in the same function.  In this particular case I think it
isn't a bug as such, but it is fragile code.

Here I think best option is to do the mutex_lock()/unlock() by
hand.


> +
> +	rc = dps310_get_pres_samp_freq(data, &prs_rate);
> +	if (rc)
> +		return rc;
> +
> +	rc = dps310_get_temp_samp_freq(data, &tmp_rate);
> +	if (rc)
> +		return rc;
> +
> +	data->drain_interval_ms = dps310_fifo_interval(prs_rate, tmp_rate);
> +
> +	rc = dps310_fifo_hold_alloc(data, prs_rate, tmp_rate);
> +	if (rc)
> +		return rc;
> +
> +	/* Drop whatever accumulated before enable */
> +	rc = dps310_fifo_hw_flush(data);
> +	if (rc)
> +		goto err_hold;
> +
> +	rc = dps310_fifo_set_enable(data, true);
> +	if (rc)
> +		goto err_hold;
> +
> +	schedule_delayed_work(&data->fifo_work,
> +			      msecs_to_jiffies(data->drain_interval_ms));
> +
> +	return 0;
> +
> +err_hold:
> +	kfree(data->fifo_hold);
> +
> +	return rc;
> +}