Re: [PATCH v6 4/6] iio: pressure: dps310: add triggered buffer support

[Jonathan Cameron]

> Add a triggered buffer in order to capture continuously on both channels
> instead of one sysfs read at a time.
> 
> Raw register value is not useful on its own, pressure has to go through
> the compensation polynomial and needs a temperature reading. Report raw
> in Pa with 1/1000 scale to keep full resolution in the buffer without
> changing what the existing processed attribute reports.
> 
> Raw and processed reads return -EBUSY while buffer is on, so does any
> reconfiguration.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Rupesh Majhi <zoone.rupert@xxxxxxxxx>
>

A few comments inline on how to simplify things by
choosing carefully what parameters to pass around.


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

...

> +
> +struct dps310_scan {
> +	s32 channels[2];
> +	aligned_s64 ts;
> +};

See below - we normally define this locally where possible
and I think that makes sense here.

>  
> +
> +static int dps310_fill_scan(struct dps310_data *data,
> +			    const unsigned long *scan_mask,
> +			    struct dps310_scan *scan)

As this isn't filling in the timestamp, I think you'd be better
passing s32 channels[__at_least 2] as that final parameter.

> +	__must_hold(&data->lock)
> +{
> +	int rc;
> +	int i = 0;
> +
> +	/*
> +	 * The pressure compensation needs a temperature reading, so temperature
> +	 * is sampled even when only the pressure channel is enabled.
> +	 */
> +	rc = dps310_read_temp_raw_locked(data);
> +	if (rc)
> +		return rc;
> +
> +	if (test_bit(DPS310_SCAN_TEMP, scan_mask)) {
> +		/* Millidegrees Celsius */
> +		rc = dps310_calculate_temp(data, &scan->channels[i]);
> +		if (rc)
> +			return rc;
> +
> +		i++;
> +	}
> +
> +	if (test_bit(DPS310_SCAN_PRESSURE, scan_mask)) {
> +		rc = dps310_read_pres_raw_locked(data);
> +		if (rc)
> +			return rc;
> +
> +		/* Pascals, see the comment on the channel definition */
> +		rc = dps310_calculate_pressure(data, &scan->channels[i]);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t dps310_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct dps310_data *data = iio_priv(iio);
> +	struct dps310_scan scan = { };

I'd pull the definition down here as
	struct {
		s32 channels[2];
		aligned_s64 timestamp;
	} scan;
	
> +	int rc;
> +
> +	mutex_lock(&data->lock);
> +	rc = dps310_fill_scan(data, iio->active_scan_mask, &scan);

Then make this *fill_channels() with only the relevant part of
the structure passed in.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>