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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Aug 24, 2026 at 11:12:01PM +0300, Rupesh Majhi wrote:
> 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.

...

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

Wondering if using macro here would make sense...

...

>  static const struct iio_chan_spec dps310_channels[] = {
>  	{
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>  			BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  			BIT(IIO_CHAN_INFO_PROCESSED),
> +		.scan_index = DPS310_SCAN_TEMP,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 32,
> +			.storagebits = 32,
> +			.endianness = IIO_CPU,
> +		},
>  	},
>  	{
>  		.type = IIO_PRESSURE,
> +		/*
> +		 * Pressure is only meaningful once the raw register value has
> +		 * been run through the compensation polynomial in section 4.9.1
> +		 * of the datasheet, which needs a temperature reading as well.
> +		 * So what is reported as _raw here is already compensated, in
> +		 * Pa, and _scale converts it to the kPa the ABI asks for. The
> +		 * _processed attribute reports the same value and predates
> +		 * buffer support, so it has to stay.
> +		 *
> +		 * Do not copy this pattern into other drivers. A raw attribute
> +		 * that is not the raw register value is only tolerable here
> +		 * because the alternative is either losing resolution in the
> +		 * buffer or breaking existing users of _processed.
> +		 */
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>  			BIT(IIO_CHAN_INFO_SAMP_FREQ) |

> -			BIT(IIO_CHAN_INFO_PROCESSED),
> +			BIT(IIO_CHAN_INFO_PROCESSED) |

Leave it untouched, squeeze the new ones above this.

> +			BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = DPS310_SCAN_PRESSURE,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 32,
> +			.storagebits = 32,
> +			.endianness = IIO_CPU,
> +		},
>  	},
> +	IIO_CHAN_SOFT_TIMESTAMP(2),
>  };

...

> static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,

>  		*val2 = 1000; /* Convert Pa to KPa per IIO ABI */

Side note, at some point makes sense to move from 1000 to KILO.

>  		return IIO_VAL_FRACTIONAL;
>  
> +	case IIO_CHAN_INFO_SCALE:
> +		/* The raw value is in Pa, the ABI wants kPa */
> +		*val = 1;
> +		*val2 = 1000;

Ditto.

> +		return IIO_VAL_FRACTIONAL;

...

> +static int dps310_fill_scan(struct dps310_data *data,
> +			    const unsigned long *scan_mask,
> +			    struct dps310_scan *scan)
> +	__must_hold(&data->lock)
> +{
> +	int rc;

> +	int i = 0;

Why is 'i' signed? Also, move assignment closer to its first user.

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

Wouldn't

		rc = dps310_calculate_temp(data, &scan->channels[i++]);

work?

> +	}
> +
> +	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]);

For the consistency's sake

		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 = { };
> +	int rc;
> +
> +	mutex_lock(&data->lock);
> +	rc = dps310_fill_scan(data, iio->active_scan_mask, &scan);
> +	mutex_unlock(&data->lock);

Hmm... Is scoped_guard() too bad here?

> +	if (rc)
> +		goto err;

> +	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan),
> +				    iio_get_time_ns(iio));

I would go with a single line.

> +err:
> +	iio_trigger_notify_done(iio->trig);

> +	return IRQ_HANDLED;
> +}

-- 
With Best Regards,
Andy Shevchenko