Re: [PATCH v3] iio: pressure: dps310: add triggered buffer support

[Rupert Zoone]

On Mon, 20 Jul 2026, Jonathan Cameron wrote:
  > Ah... This wasn't what I meant.  I was expecting to see the fifo
  > stuff but coupled with use of buffers.
  > ...
  > I think what would make sense for this device would be
  > that fifo mode is used when a trigger is not provided, but disabled when
  > one is.

  Thanks for the detailed review, and sorry for taking the FIFO suggestion
  in the wrong direction. I'm happy to carry the triggered-buffer support
  forward and build toward the design you describe, FIFO used when no
  trigger is attached, disabled when one is.

  Before I rework, one scope question :) would you prefer the FIFO and
  external-trigger support in a single series (with the fifo-vs-trigger
  selection as one patch), or is it OK to land the triggered-buffer support
  first -> reworked per your comments -> and add the FIFO/watermark path as a
  follow-up? Either is fine by me, just wanted to structure easiest
for you to review.

  Either way I'll fold in the review comments:

  > Just have local instances on the stack.
  > + the discussion with Andy, that __aligned(8) isn't doing anything.
  Will move the scan buffer to a stack local and drop the __aligned(8).

  > Given temperature and pressure are effectively independent sequences I
  > think you would be better off not providing available_scan_masks()
  Makes sense - I'll drop available_scan_masks and push only the enabled
  channels (still reading temperature internally for pressure compensation).

  > Look at the ACQUIRE stuff in iio.h
  > And similar ACQUIRE stuff for this one.
  Will switch claim_direct and the mutex to the ACQUIRE()/guard() scoped
  helpers, so the release-in-every-path handling goes away.

  > Maybe wrap this up in a little helper given sharing with PROCESSED.
  Will factor the shared read-and-calculate into a helper for the RAW and
  PROCESSED cases.

  > Would it make sense to provide a version of these calls that doesn't take
  > the lock then take it here to avoid bouncing it?
  Sure will add lock-free inner helpers and take the lock once in the
  trigger handler rather than per call.

  > Can you make sure to add some comments around the chan spec about this.
  > I don't want it copied into other drivers!
  Will add a comment on the pressure channel explaining the raw+scale choice
  and it's relevancy to this driver.

  Thanks,
  Rupesh

On Mon, Jul 20, 2026 at 1:11 AM Jonathan Cameron
<jonathan.cameron@xxxxxxxxxxxxxxxx> wrote:
>
> On Sun, 19 Jul 2026 02:51:58 +0300
> Rupesh Majhi <zoone.rupert@xxxxxxxxx> wrote:
>
> > Add triggered buffer support so pressure and temperature can be captured
> > into a buffer instead of only through one-shot sysfs reads.
> >
> > Pressure is a processed value in kPa computed from a non-linear
> > calibration polynomial. To keep full resolution in the buffer without
> > disagreeing with the sysfs unit, add raw and scale attributes for
> > pressure (raw in Pa, scale 1/1000 to kPa), following bme680; the existing
> > processed attribute is kept for ABI compatibility.
>
> Hmm. That is unfortunate.  Ideally in original driver we'd have only
> output RAW (even though somewhat processed) then this would be the
> same.   Agreed that given the constraints, this is the way we'll have to go.
> Can you make sure to add some comments around the chan spec about this.
> I don't want it copied into other drivers!
>
> > Temperature is already
> > a full-resolution value in its base unit (millidegrees Celsius) and stays
> > a processed channel.
> >
> > Pressure compensation depends on a temperature reading, so both channels
> > are always captured together. The device already runs in continuous
> > background mode, so no buffer setup ops are needed. Sysfs reads and
> > reconfiguration return -EBUSY while the buffer is enabled, as they share
> > the capture path's raw values and configuration.
> >
> > Signed-off-by: Rupesh Majhi <zoone.rupert@xxxxxxxxx>
> https://sashiko.dev/#/patchset/20260718211727.39752-1-zoone.rupert%40gmail.com
>
> Some stuff in there to check. I haven't looked closely beyond checking
> for usual false positives (none of those).
>
> > ---
> > Changes in v3:
> >  - Rework from just enabling the FIFO into proper IIO triggered buffer
> >    support, as suggested by Jonathan.
> >  - Buffer the pressure and temperature values; add raw + scale for
> >    pressure (bme680-style) so buffered pressure keeps full resolution
> >    and stays consistent with the sysfs unit.
> >  - Drop the unused FIFO enable/flush/read helpers.
>
> Ah... This wasn't what I meant.  I was expecting to see the fifo
> stuff but coupled with use of buffers.  It may not make sense to support
> triggered buffer for that case.  Often doesn't for FIFO equiped parts where
> the read out of a fifo is not reading just one 'scan' but many and where
> timestamps are either not provided or are the subject of non trivial
> interpolation of interrupt timings.
>
> So does supporting an external trigger (what this does) actually make
> sense for this device?  There are drivers where we do both but I would
> expect to see that in one patch set as the mechanism for choosing fifo
> (and watermarks interrupt) vs direct reads and a trigger is the complex
> corner.   I think what would make sense for this device would be
> that fifo mode is used when a trigger is not provided, but disabled when
> one is.
>
> Anyhow I've given this a quick review based on the fact we might
> want to carry forward triggered buffer support as part of the solution.
>
> >
> > v2 was a resend that still contained the v1 FIFO-enable code.
> >
> > Note: this touches dps310_probe() near the separately-sent fix "iio:
> > pressure: dps310: fix NULL pointer deref on ACPI probe". It was generated
> > on a plain base tree, so applying it after that fix needs a trivial
> > 3-way merge/rebase - happy to resend in whatever order or branch you
> > prefer.
> >
> >  drivers/iio/pressure/Kconfig  |   2 +
> >  drivers/iio/pressure/dps310.c | 141 ++++++++++++++++++++++++++++++++--
> >  2 files changed, 136 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> > index 838a8340c4c0..cef8b90b9ae7 100644
> > --- a/drivers/iio/pressure/Kconfig
> > +++ b/drivers/iio/pressure/Kconfig
> > @@ -112,6 +112,8 @@ config DPS310
> >       tristate "Infineon DPS310 pressure and temperature sensor"
> >       depends on I2C
> >       select REGMAP_I2C
> > +     select IIO_BUFFER
> > +     select IIO_TRIGGERED_BUFFER
> >       help
> >         Support for the Infineon DPS310 digital barometric pressure sensor.
> >         It can be accessed over I2C bus.
> > diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> > index f45af72a0554..967a2043550b 100644
> > --- a/drivers/iio/pressure/dps310.c
> > +++ b/drivers/iio/pressure/dps310.c
> > @@ -20,8 +20,11 @@
> >  #include <linux/module.h>
> >  #include <linux/regmap.h>
> >
> > +#include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> >
> >  #define DPS310_DEV_NAME              "dps310"
> >
> > @@ -90,6 +93,12 @@ struct dps310_data {
> >       s32 pressure_raw;
> >       s32 temp_raw;
> >       bool timeout_recovery_failed;
> > +
> > +     /* Buffer to hold a scan; timestamp is naturally aligned */
> > +     struct {
> > +             s32 chan[2];
> > +             aligned_s64 timestamp;
> > +     } scan __aligned(8);
>
> See below.  No reason to have this in this structure. Just have local
> instances on the stack.  + the discussion with Andy, that __aligned(8)
> isn't doing anything.
>
> >  };
> >
> >  static const struct iio_chan_spec dps310_channels[] = {
> > @@ -98,15 +107,38 @@ static const struct iio_chan_spec dps310_channels[] = {
> >               .info_mask_separate = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
> >                       BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> >                       BIT(IIO_CHAN_INFO_PROCESSED),
> > +             .scan_index = 0,
>
> Andy mentioned this.  I'd generally prefer an enum used for this
> as then anywhere else we can clearly see which scan_index we are working with.
>
> > +             .scan_type = {
> > +                     .sign = 's',
> > +                     .realbits = 32,
> > +                     .storagebits = 32,
> > +                     .endianness = IIO_CPU,
> > +             },
> >       },
> >       {
> >               .type = IIO_PRESSURE,
> >               .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) |
> > +                     BIT(IIO_CHAN_INFO_RAW) |
> > +                     BIT(IIO_CHAN_INFO_SCALE),
> > +             .scan_index = 1,
> > +             .scan_type = {
> > +                     .sign = 's',
> > +                     .realbits = 32,
> > +                     .storagebits = 32,
> > +                     .endianness = IIO_CPU,
> > +             },
> >       },
> > +     IIO_CHAN_SOFT_TIMESTAMP(2),
> >  };
> >
> > +/*
> > + * Pressure compensation needs a temperature reading, so the trigger
> > + * handler always captures both channels; keep them enabled together.
> > + */
> > +static const unsigned long dps310_scan_masks[] = { GENMASK(1, 0), 0 };
> > +
> >  /* To be called after checking the COEF_RDY bit in MEAS_CFG */
> >  static int dps310_get_coefs(struct dps310_data *data)
> >  {
> > @@ -583,8 +615,14 @@ static int dps310_write_raw(struct iio_dev *iio,
> >       int rc;
> >       struct dps310_data *data = iio_priv(iio);
> >
> > -     if (mutex_lock_interruptible(&data->lock))
> > +     /* Don't reconfigure the sensor while a buffered capture is running */
> > +     if (!iio_device_claim_direct(iio))
> > +             return -EBUSY;
>
> Look at the ACQUIRE stuff in iio.h
> > +
> > +     if (mutex_lock_interruptible(&data->lock)) {
>
> And similar ACQUIRE stuff for this one.  Given both are
> release in all paths just before return it should simplify the flow a little.
>
> > +             iio_device_release_direct(iio);
> >               return -EINTR;
> > +     }
> >
> >       switch (mask) {
> >       case IIO_CHAN_INFO_SAMP_FREQ:
> > @@ -625,6 +663,7 @@ static int dps310_write_raw(struct iio_dev *iio,
> >       }
> >
> >       mutex_unlock(&data->lock);
> > +     iio_device_release_direct(iio);
> >       return rc;
> >  }
> >
> > @@ -735,6 +774,23 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
> >               *val2 = 1000; /* Convert Pa to KPa per IIO ABI */
> >               return IIO_VAL_FRACTIONAL;
> >
> > +     case IIO_CHAN_INFO_RAW:
> > +             rc = dps310_read_pres_raw(data);
> > +             if (rc)
> > +                     return rc;
> > +
> > +             rc = dps310_calculate_pressure(data, val);
> > +             if (rc)
> > +                     return rc;
>
> Maybe wrap this up in a little helper given sharing with PROCESSED.
> Will make that a little more visually obvious.
>
> > +
> > +             return IIO_VAL_INT;
> > +
> > +     case IIO_CHAN_INFO_SCALE:
> > +             /* Raw pressure is in Pa; scale to kPa per IIO ABI */
> > +             *val = 1;
> > +             *val2 = 1000;
> > +             return IIO_VAL_FRACTIONAL;
> > +
> >       case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >               rc = dps310_get_pres_precision(data, val);
> >               if (rc)
> > @@ -804,12 +860,10 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
>
> ...
>
> > +static int dps310_read_raw(struct iio_dev *iio,
> > +                        struct iio_chan_spec const *chan,
> > +                        int *val, int *val2, long mask)
> > +{
> > +     struct dps310_data *data = iio_priv(iio);
> > +     int rc;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +     case IIO_CHAN_INFO_PROCESSED:
> > +             /*
> > +              * Reading a sample uses the same raw values as the buffered
> > +              * capture path, so only allow it outside of buffered mode.
> > +              */
> > +             if (!iio_device_claim_direct(iio))
> > +                     return -EBUSY;
>
> Maybe add scope and use an ACQUIRE here similar to suggested above.
> It is more marginal but might make sense to use it for consistency.
>
> > +
> > +             rc = dps310_read_channel(data, chan, val, val2, mask);
> > +             iio_device_release_direct(iio);
> > +             return rc;
> > +
> > +     default:
> > +             return dps310_read_channel(data, chan, val, val2, mask);
> > +     }
> > +}
> > +
> >  static void dps310_reset(void *action_data)
> >  {
> >       struct dps310_data *data = action_data;
> > @@ -843,6 +923,46 @@ static const struct iio_info dps310_info = {
> >       .write_raw = dps310_write_raw,
> >  };
> >
> > +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);
> > +     int rc, pressure, temp;
> > +
> > +     /*
> > +      * Don't hold data->lock across these calls: the read helpers take it
> > +      * themselves and the mutex is not recursive (dps310_calculate_pressure
> > +      * also grabs it with mutex_trylock to refresh the temperature).
>
> Would it make sense to provide a version of these calls that doesn't take the
> lock then take it here to avoid bouncing it?
>
> > +      */
> > +     rc = dps310_read_pres_raw(data);
> > +     if (rc)
> > +             goto out;
> > +
> > +     rc = dps310_read_temp_raw(data);
>
> Given temperature and pressure are effectively independent sequences I think
> you would be better off not providing available_scan_masks() but instead reading
> only the ones that are enabled. Lots of examples of how to do that
> in tree.
>
> > +     if (rc)
> > +             goto out;
> > +
> > +     rc = dps310_calculate_pressure(data, &pressure);
> > +     if (rc)
> > +             goto out;
> > +
> > +     rc = dps310_calculate_temp(data, &temp);
> > +     if (rc)
> > +             goto out;
> > +
> > +     data->scan.chan[0] = temp;      /* millidegrees Celsius */
> > +     data->scan.chan[1] = pressure;  /* Pascals */
> > +
>
> Drag the scan in here as a local variable. It's fairly small and there
> is nothing stopping you having it on the stack given no DMA to it
> or anything like that.
>
> > +     iio_push_to_buffers_with_ts(iio, &data->scan, sizeof(data->scan),
> > +                                 pf->timestamp);
> > +
> > +out:
> > +     iio_trigger_notify_done(iio->trig);
> > +
> > +     return IRQ_HANDLED;
> > +}