Re: [PATCH 2/2] iio: pressure: dps310: add a data-ready trigger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] iio: pressure: dps310: blocking temp wait in
  dps310_fill_channels() on IRQ
--

commit f0cb91c380b85ec52615026366938a6f2c5145ae
Author: Rupesh Majhi <zoone.rupert@gmail.com>

iio: pressure: dps310: add a data-ready trigger

This commit registers an IIO trigger backed by the DPS310 pressure
data-ready interrupt on SDO and uses the interrupt timestamp when
capturing scans with the device's own trigger.

> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index 5cee23986f307..76738360e528e 100644
> --- a/drivers/iio/pressure/dps310.c
> +++ b/drivers/iio/pressure/dps310.c

[ ... ]

> @@ -1375,6 +1391,7 @@ static irqreturn_t dps310_trigger_handler(int irq, void *p)
>  		s32 channels[2];
>  		aligned_s64 timestamp;
>  	} scan = { };
> +	s64 timestamp;
>  	int rc;
>  
>  	mutex_lock(&data->lock);
> @@ -1383,7 +1400,12 @@ static irqreturn_t dps310_trigger_handler(int irq, void *p)
>  	if (rc)
>  		goto err;
>  
> -	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), iio_get_time_ns(iio));
> +	if (iio_trigger_using_own(iio))
> +		timestamp = data->irq_timestamp;
> +	else
> +		timestamp = iio_get_time_ns(iio);
> +
> +	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), timestamp);
>  
>  err:
>  	iio_trigger_notify_done(iio->trig);
> @@ -1391,6 +1413,97 @@ static irqreturn_t dps310_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t dps310_irq_handler(int irq, void *private)
> +{
> +	struct dps310_data *data = private;
> +
> +	data->irq_timestamp = iio_get_time_ns(data->iio);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t dps310_irq_thread(int irq, void *private)
> +{
> +	struct dps310_data *data = private;
> +	unsigned int status;
> +
> +	if (regmap_read(data->regmap, DPS310_INT_STS, &status))
> +		return IRQ_NONE;
> +
> +	if (!(status & DPS310_INT_STS_PRS))
> +		return IRQ_NONE;
> +
> +	iio_trigger_poll_nested(data->trig);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
When the pressure sampling rate is higher than the temperature rate (for
example, any pressure rate above the 1 Hz default temperature rate), does
dps310_trigger_handler() block waiting for the next temperature conversion
before reading the pressure sample that fired DPS310_INT_PRS?

dps310_irq_thread() clears DPS310_INT_STS and calls
iio_trigger_poll_nested() -> dps310_trigger_handler() ->
dps310_fill_channels():

drivers/iio/pressure/dps310.c:dps310_fill_channels() {
	/* Compensation needs a temperature, so it is sampled either way */
	rc = dps310_read_temp_raw_locked(data);
	if (rc)
		return rc;
    ...
	if (test_bit(DPS310_SCAN_PRESSURE, scan_mask)) {
		rc = dps310_read_pres_raw_locked(data);

Reading DPS310_TMP_BASE clears DPS310_TMP_RDY, so on the next pressure
interrupt dps310_read_temp_raw_locked() sleeps until another temperature
measurement finishes.  While it sleeps, pressure capture is throttled to
the temperature rate, new pressure conversions overwrite DPS310_PRS_B0..B2
so the newer sample is pushed with the earlier data->irq_timestamp, and
DPS310_INT_STS_PRS is re-latched without being cleared by
dps310_read_pres_raw_locked() (leaving SDO asserted, which causes an
immediate spurious interrupt on level-triggered IRQs or stalls
edge-triggered IRQs).

Should dps310_fill_channels() read pressure first and use
dps310_refresh_temp_locked() like dps310_read_pressure_value() does?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110154254.39092-1-zoone.rupert@gmail.com?part=2