Re: [PATCH v6 5/6] iio: pressure: dps310: add hardware FIFO support

[Jonathan Cameron]

> Use the 32 entry FIFO for buffered capture, so a reader wakes once per
> batch of samples instead of once per sample.
> 
> FIFO runs when no trigger is attached and stays off when one is.
> iio_verify_update() already picks the mode, so buffer setup ops just
> branch on iio_device_get_current_mode(), as rohm-bm1390.c does.
> 
> Drain is on a timer because there is no interrupt to use and nothing in
> tree wires the INT pin. hwfifo_flush_to_buffer is not enough on its own:
> iio_buffer_read() sleeps until something is pushed, so a blocking reader
> would hang with samples still sitting in the FIFO. Hook is kept for
> poll() and non-blocking reads.
> 
> Pressure entries drive the scans and reuse last temperature, so the two
> configured rates stay independent. FIFO does not timestamp entries, they
> are estimated from the sample rate.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Rupesh Majhi <zoone.rupert@xxxxxxxxx>

Various comments inline.  I'd definitely drop the timestamp code from
this patch - maybe more than that.  Need to make it easier to review.
Timestamp + FIFO is hard to get right - see the iterations the invense
handler has gone through.  Even more so when there are no actual
interrupts to provide meaningful information!

>
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index f122acd72b0c..d05aa05d6222 100644
> --- a/drivers/iio/pressure/dps310.c
> +++ b/drivers/iio/pressure/dps310.c


...

> +
> +static int dps310_fifo_set_enable(struct dps310_data *data, bool enable)
> +	__must_hold(&data->lock)
> +{
> +	return regmap_write_bits(data->regmap, DPS310_CFG_REG, DPS310_FIFO_EN,
> +				 enable ? DPS310_FIFO_EN : 0);
r
egmap_assign_bits

> +}
> +
> +/*
> + * There is no interrupt wired on any in-tree platform and the binding has no
> + * interrupts property, so the FIFO is drained on a timer, at an interval below
> + * the time it takes to fill. See DPS310_FIFO_DEPTH for why late is bad.
> + */

Just as a passing comment.  This is very unusual. Fifos are normally
coupled with interrupts.

> +static int dps310_fifo_interval(struct dps310_data *data, unsigned int *ms)
> +	__must_hold(&data->lock)
> +{
> +	bool pressure_enabled = test_bit(DPS310_SCAN_PRESSURE,
> +					 data->iio->active_scan_mask);
> +	unsigned int fill_ms, want_ms;
> +	int rc, prs_rate, tmp_rate;
> +
> +	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;
> +
> +	/* Both streams share the same entries, so they fill it together. */
> +	fill_ms = MSEC_PER_SEC * DPS310_FIFO_DEPTH / (prs_rate + tmp_rate);
> +
> +	/*
> +	 * The DPS310 has no configurable hardware watermark, only a FIFO-full
> +	 * condition, so the watermark is taken as the number of scans the user
> +	 * is prepared to wait for and drives the drain interval instead. Scans
> +	 * come at the rate of whichever measurement drives them, which is not
> +	 * the pressure rate when only the temperature channel is enabled.
> +	 */

I'd not mention the full condition. It is of no use other than
detecting overflow (and we have no way to report that).


> +	want_ms = data->watermark * MSEC_PER_SEC /
> +		  (pressure_enabled ? prs_rate : tmp_rate);
> +
> +	*ms = clamp(min(want_ms, fill_ms / 2), DPS310_DRAIN_MIN_MS,
> +		    DPS310_DRAIN_MAX_MS);
> +
> +	return 0;
> +}
> +

...

> +static int dps310_fifo_push_scan(struct dps310_data *data, s32 temp_raw,
> +				 s32 pressure_raw, s64 timestamp)
> +	__must_hold(&data->lock)
> +{
> +	struct iio_dev *iio = data->iio;
> +	struct dps310_scan scan = { };

As before I'd have the structure locally here. I really want to be
able to see what it looks like.  Don't worry about the small amount
of duplication this will entail.

> +	int i = 0;
> +	int rc;
> +
> +	/*
> +	 * The compensation helpers read the cached raw values. Sysfs reads take
> +	 * the direct-mode claim, so they cannot be looking at these while a
> +	 * buffered capture is running.
> +	 */
> +	data->temp_raw = temp_raw;
> +	data->pressure_raw = pressure_raw;
> +
> +	if (test_bit(DPS310_SCAN_TEMP, iio->active_scan_mask)) {
> +		rc = dps310_calculate_temp(data, &scan.channels[i]);
> +		if (rc)
> +			return rc;
> +
> +		i++;
> +	}
> +
> +	if (test_bit(DPS310_SCAN_PRESSURE, iio->active_scan_mask)) {
> +		rc = dps310_calculate_pressure(data, &scan.channels[i]);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), timestamp);
> +
> +	return 0;
> +}
> +
> +/*
> + * Drain the FIFO and push the samples it held, stopping once max_scans scans
> + * are in hand or draining everything when max_scans is zero. Stopping at the
> + * read rather than after it matters: entries leave the hardware as they are
> + * read, so any collected beyond the caller's limit would have to be discarded.
> + *
> + * Returns the number of scans pushed.
> + */
> +static int dps310_fifo_drain(struct dps310_data *data, s64 now,
> +			     unsigned int max_scans)
> +	__must_hold(&data->lock)
> +{
> +	bool pressure_enabled = test_bit(DPS310_SCAN_PRESSURE,
> +					 data->iio->active_scan_mask);
> +	bool temp_valid = data->fifo_temp_valid;
> +	bool is_pressure[DPS310_FIFO_DEPTH];
> +	s32 raw[DPS310_FIFO_DEPTH];
> +	unsigned int i, n = 0, scans = 0, pushed = 0;
> +	s64 interval, first;
> +	int rc, rate;
> +
> +	/*
> +	 * Empty the hardware first and compensate afterwards, so the time spent
> +	 * in the polynomial is not time the FIFO spends filling.

This seems backwards.  We are aiming that thie fifo does fill (with
new data) whilst doing the maths.

> +	 *
> +	 * Pressure entries drive the scans and reuse the most recent
> +	 * temperature, so the two rates stay independent; entries arriving
> +	 * before any temperature cannot be compensated and are dropped. With
> +	 * only the temperature channel enabled there is nothing to pair with
> +	 * and temperature drives the scans itself.

Dropping data rather defeats the point of the fifo.  Can we avoid it
by holding them locally until we will definitely have seen a useable
temperature?

> +	 */
> +	for (i = 0; i < DPS310_FIFO_DEPTH; i++) {
> +		rc = dps310_fifo_read_entry(data, &raw[n], &is_pressure[n]);
> +		if (rc < 0)
> +			return rc;
> +		if (!rc)
> +			break;
> +
> +		if (!is_pressure[n]) {
> +			temp_valid = true;
> +			if (!pressure_enabled)
> +				scans++;
> +		} else if (pressure_enabled && temp_valid) {
> +			scans++;
> +		}
> +		n++;
> +
> +		if (max_scans && scans >= max_scans)
> +			break;
> +	}
> +
> +	if (!scans)
> +		return 0;
> +
> +	/*
> +	 * FIFO entries carry no timestamps. They are synthesised by working
> +	 * back from the drain with the configured period of whichever
> +	 * measurement drives the scans, so the spacing matches the sampling
> +	 * frequency the user asked for instead of varying with how much each
> +	 * drain happened to collect. These are estimates, not hardware
> +	 * timestamps.
> +	 */

Do this as a follow up patch if at all.  Fifo timestamp estimation is very
hard to get right.  We often just don't provide timestamps for this
reason.

For initial patch just reject starting the buffer if timestamps are
set to be captured.



> +	rc = pressure_enabled ? dps310_get_pres_samp_freq(data, &rate) :
> +				dps310_get_temp_samp_freq(data, &rate);
> +	if (rc)
> +		return rc;
> +
> +	interval = div_s64(NSEC_PER_SEC, rate);
> +	first = now - (s64)(scans - 1) * interval;
> +
> +	/*
> +	 * A batch must not start before the previous one ended, or the buffer
> +	 * would carry timestamps that go backwards. If this drain collected
> +	 * more than the configured rate accounts for, spread it across the
> +	 * window since the last sample instead.
> +	 */
> +	if (data->fifo_timestamp && first <= data->fifo_timestamp) {
> +		interval = max_t(s64, div_s64(now - data->fifo_timestamp, scans), 1);
> +		first = data->fifo_timestamp + interval;
> +	}
> +
> +	for (i = 0; i < n; i++) {
> +		s64 timestamp;
> +
> +		if (!is_pressure[i]) {
> +			data->fifo_temp_raw = raw[i];
> +			data->fifo_temp_valid = true;
> +
> +			if (pressure_enabled)
> +				continue;
> +		} else if (!pressure_enabled || !data->fifo_temp_valid) {
> +			continue;
> +		}
> +
> +		timestamp = first + (s64)pushed * interval;
> +
> +		rc = dps310_fifo_push_scan(data,
> +					   is_pressure[i] ? data->fifo_temp_raw
> +							  : raw[i],
> +					   is_pressure[i] ? raw[i] : 0,
> +					   timestamp);
> +		if (rc)
> +			return rc;
> +
> +		data->fifo_timestamp = timestamp;
> +		pushed++;
> +	}
> +
> +	return pushed;
> +}
> +

>  
>  static int dps310_probe(struct i2c_client *client)
> @@ -1018,13 +1392,22 @@ static int dps310_probe(struct i2c_client *client)
>  
>  	data = iio_priv(iio);
>  	data->client = client;
> +	data->iio = iio;
> +	data->watermark = 1;
>  	mutex_init(&data->lock);
> +	INIT_DELAYED_WORK(&data->fifo_work, dps310_fifo_work);
>  
>  	iio->name = id->name;
>  	iio->channels = dps310_channels;
>  	iio->num_channels = ARRAY_SIZE(dps310_channels);
>  	iio->info = &dps310_info;
> -	iio->modes = INDIO_DIRECT_MODE;
> +	/*
> +	 * Both buffer modes are advertised so that iio_verify_update() picks
> +	 * INDIO_BUFFER_TRIGGERED when a trigger is attached and falls back to
> +	 * INDIO_BUFFER_SOFTWARE, which the FIFO path uses, when one is not.
> +	 */

Hmm.  One of thes is set automatically (hence why it wasn't here
before).  I suppose there is some value in making the list explicit
here for documentation purposes rather thn just adding the other bits.
I'd talk less about what a particular core function does and rather
refer to the 'IIO core' or something like that.  Avoids it becoming
wrong if we change the function name or implementation in future.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>