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

[Jonathan Cameron]

> 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>
Looking good.  Just a few small things.

Jonathan

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



>  enum dps310_scan_index {
>  	DPS310_SCAN_TEMP,
>  	DPS310_SCAN_PRESSURE,
> +	DPS310_SCAN_TIMESTAMP,
>  };
>  
>  static const struct iio_chan_spec dps310_channels[] = {
> @@ -140,7 +168,7 @@ static const struct iio_chan_spec dps310_channels[] = {
>  			.endianness = IIO_CPU,
>  		},
>  	},
> -	IIO_CHAN_SOFT_TIMESTAMP(2),
> +	IIO_CHAN_SOFT_TIMESTAMP(DPS310_SCAN_TIMESTAMP),

Why not in previous patch?

>  };


> +/* Pressure seen before any temperature, kept until one turns up */
> +static void dps310_fifo_hold(struct dps310_data *data, s32 pressure_raw)
> +	__must_hold(&data->lock)
> +{
> +	if (data->fifo_held < data->fifo_hold_max)
> +		data->fifo_hold[data->fifo_held++] = pressure_raw;
> +}
> +
> +/* Returns scans pushed */
> +static int dps310_fifo_push_held(struct dps310_data *data)
> +	__must_hold(&data->lock)
> +{
> +	unsigned int i, held = data->fifo_held;

As below, I'd separate those two declartions onto different lines.

> +	int rc = 0;
> +
> +	for (i = 0; i < held; i++) {
> +		rc = dps310_fifo_push_scan(data, data->fifo_temp_raw,
> +					   data->fifo_hold[i]);
> +		if (rc)
> +			break;
> +	}
> +
> +	/* What did not go out stays for the next drain */
> +	data->fifo_held = held - i;
> +	memmove(data->fifo_hold, &data->fifo_hold[i],
> +		data->fifo_held * sizeof(*data->fifo_hold));
> +
> +	return rc ? rc : i;
> +}
> +
> +/*
> + * 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 i, n = 0, pushed = 0;

Please use separate lines for declerations with assignment and
those without.

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

Given n is used in a couple of places, I'd keep the handling simple
by doing it as
	for (i = 0, n = 0; i < DPS310_FIFO_DEPTH; i++, n++)

That makes it visisble that it is a loop variable rather than
one tha is only sometime incremented.

Maybe rename it to something like cnt to make it more obvious
what it is.

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

> +
> +static void dps310_fifo_hold_free(struct dps310_data *data)
> +	__must_hold(&data->lock)
> +{
> +	kfree(data->fifo_hold);
> +	data->fifo_hold = NULL;

Why clear fifo_hold?  Nothing ever checks if it is NULL.

> +	data->fifo_hold_max = 0;

Similar for this.  If you are checking it you are in a path
where we are assuming the fifo is valid. I'd hope the
code is structured so that isn't the case.

> +}
> +
Thanks,

Jonathan

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>