Re: [PATCH v11 2/5] iio: pressure: dps310: read buffered samples from the hardware FIFO

[Andy Shevchenko]

On Fri, Oct 02, 2026 at 10:25:23AM +0300, Rupesh Majhi wrote:
> DPS310 has a 32-entry FIFO shared by both measurements. Drain it from a
> work item and push what it held, so buffered capture needs no trigger.
> Nothing in tree wires the interrupt pin, so the work rearms itself at
> half the FIFO fill time.
> 
> Each entry carries one measurement, so a pressure entry is compensated
> with the temperature ahead of it. Pressure read before a session's first
> temperature 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.

...

> +{
> +	u8 val[3];
> +	s32 raw;
> +	int rc;
> +
> +	/* Entries come out of the pressure registers whichever made them */
> +	rc = regmap_bulk_read(data->regmap, DPS310_PRS_BASE, val, sizeof(val));
> +	if (rc < 0)
> +		return rc;
> +
> +	raw = get_unaligned_be24(val);
> +	if (raw == DPS310_FIFO_EMPTY_VAL)
> +		return DPS310_FIFO_EMPTY;
> +
> +	*value = sign_extend32(raw, 23);
> +
> +	return raw & DPS310_FIFO_TAG_PRS ? DPS310_FIFO_PRESSURE :
> +					   DPS310_FIFO_TEMP;

Make it a single line (yes, 83 characters).

> +}

...

> +static unsigned int dps310_fifo_push_held(struct dps310_data *data)
> +	__must_hold(&data->lock)
> +{
> +	unsigned int pushed = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < data->fifo_held; i++) {

	for (unsigned int i = 0; i < data->fifo_held; i++) {

> +		if (!dps310_fifo_push_scan(data, data->fifo_temp_raw,
> +					   data->fifo_hold[i]))
> +			pushed++;
> +	}
> +
> +	data->fifo_held = 0;
> +
> +	return pushed;
> +}

...

> +/*
> + * Read the batch out before compensating it, so a pressure entry pairs with
> + * the temperature preceding it rather than the last one in the batch.
> + *
> + * Returns scans pushed.
> + */
> +static int dps310_fifo_drain(struct dps310_data *data)
> +	__must_hold(&data->lock)
> +{
> +	bool pressure_enabled = test_bit(DPS310_SCAN_PRESSURE,
> +					 data->iio->active_scan_mask);
> +	u8 kind[DPS310_FIFO_DEPTH];
> +	s32 raw[DPS310_FIFO_DEPTH];
> +	unsigned int pushed = 0;
> +	unsigned int cnt, i;
> +	int rc;
> +
> +	for (cnt = 0; cnt < DPS310_FIFO_DEPTH; cnt++) {

Ditto.

> +		rc = dps310_fifo_read_entry(data, &raw[cnt]);
> +		if (rc < 0)
> +			return rc;
> +
> +		if (rc == DPS310_FIFO_EMPTY)
> +			break;
> +
> +		kind[cnt] = rc;
> +	}
> +
> +	for (i = 0; i < cnt; i++) {

Ditto.

> +		if (kind[i] == DPS310_FIFO_TEMP) {
> +			data->fifo_temp_raw = raw[i];
> +			data->fifo_temp_valid = true;
> +
> +			if (pressure_enabled)
> +				pushed += dps310_fifo_push_held(data);
> +			else if (!dps310_fifo_push_scan(data, raw[i], 0))
> +				pushed++;
> +			continue;
> +		}
> +
> +		if (!pressure_enabled)
> +			continue;
> +
> +		if (!data->fifo_temp_valid) {
> +			dps310_fifo_hold(data, raw[i]);
> +			continue;
> +		}
> +
> +		if (!dps310_fifo_push_scan(data, data->fifo_temp_raw, raw[i]))
> +			pushed++;
> +	}
> +
> +	return pushed;
> +}

...

> +static void dps310_fifo_work(struct work_struct *work)
> +{
> +	struct dps310_data *data = container_of(to_delayed_work(work),
> +						struct dps310_data, fifo_work);
> +	int rc;
> +
> +	mutex_lock(&data->lock);
> +	rc = dps310_fifo_drain(data);
> +	mutex_unlock(&data->lock);

What's wrong with scoped_guard()?

> +	if (rc < 0)
> +		dev_dbg(&data->client->dev, "FIFO drain failed: %d\n", rc);
> +
> +	schedule_delayed_work(&data->fifo_work,
> +			      msecs_to_jiffies(data->drain_interval_ms));
> +}

...

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

> +	mutex_lock(&data->lock);

Refactor this to make one use guard()() and the inner one to use kfree().

> +	rc = dps310_get_pres_samp_freq(data, &prs_rate);
> +	if (rc)
> +		goto err_unlock;
> +
> +	rc = dps310_get_temp_samp_freq(data, &tmp_rate);
> +	if (rc)
> +		goto err_unlock;
> +
> +	data->drain_interval_ms = dps310_fifo_interval(prs_rate, tmp_rate);
> +
> +	rc = dps310_fifo_hold_alloc(data, prs_rate, tmp_rate);
> +	if (rc)
> +		goto err_unlock;
> +
> +	/* 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));
> +
> +	mutex_unlock(&data->lock);
> +
> +	return 0;
> +
> +err_hold:
> +	kfree(data->fifo_hold);
> +err_unlock:
> +	mutex_unlock(&data->lock);
> +
> +	return rc;
> +}

-- 
With Best Regards,
Andy Shevchenko