On Thu, Mar 14, 2024 at 03:25:25PM +0000, Jonathan Cameron wrote:
> On Wed, 13 Mar 2024 18:40:07 +0100
> Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
>
> > Add a buffer struct that will hold the values of the measurements
> > and will be pushed to userspace and a buffer_handler function to
> > read the data and push them.
> >
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> > ---
> > drivers/iio/pressure/Kconfig | 2 +
> > drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
> > drivers/iio/pressure/bmp280.h | 7 ++++
> > 3 files changed, 70 insertions(+)
> >
> > diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> > index 79adfd059c3a..5145b94b4679 100644
> > --- a/drivers/iio/pressure/Kconfig
> > +++ b/drivers/iio/pressure/Kconfig
> > @@ -31,6 +31,8 @@ config BMP280
> > select REGMAP
> > select BMP280_I2C if (I2C)
> > select BMP280_SPI if (SPI_MASTER)
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > help
> > Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
> > and BMP580 pressure and temperature sensors. Also supports the BME280 with
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f2cf9bef522c..7c889cda396a 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -40,7 +40,10 @@
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> >
> > +#include <linux/iio/buffer.h>
> > #include <linux/iio/iio.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> >
> > #include <asm/unaligned.h>
> >
> > @@ -2188,6 +2191,57 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
> > return 0;
> > }
> >
> > +static irqreturn_t bmp280_buffer_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct bmp280_data *data = iio_priv(indio_dev);
> > + int ret, temp;
> > +
> > + /*
> > + * data->buf[3] is used to transfer data from the device. Whenever a
> > + * pressure or a humidity reading takes place, the data written in the
> > + * data->buf[3] overwrites the iio_buf.temperature value. Keep the
> > + * temperature value and apply it after the readings.
>
> See comment below. Given you saw this problem did you not think maybe you
> were doing something a little unusual / wrong? Should have rung alarm
> bells beyond just putting a comment here to explain you needed to work around
> the issue.
>
> > + */
> > + mutex_lock(&data->lock);
> > +
> > + if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
> > + ret = data->chip_info->read_temp(data);
> > + if (ret < 0)
> > + goto done;
> > +
> > + temp = ret;
> > + }
> > +
> > + if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
> > + ret = data->chip_info->read_press(data);
> > + if (ret < 0)
> > + goto done;
> > +
> > + data->iio_buf.pressure = ret;
> > + data->iio_buf.temperature = temp;
> Try running this with the tooling in tools/iio and you'll see that
> you are getting the wrong output if you have just humidity and
> temperature enabled - IIO packs channels, so disable an early
> one and everything moves down in address.
>
> If you an this device without timestamps and only a single channel
> the buffer used will have one s32 per scan for example.
>
> > + }
> > +
> > + if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> > + ret = data->chip_info->read_humid(data);
> > + if (ret < 0)
> > + goto done;
> > +
> > + data->iio_buf.humidity = ret;
> > + data->iio_buf.temperature = temp;
> > + }
> > +
> > + iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > + iio_get_time_ns(indio_dev));
> > +
> > +done:
> > + mutex_unlock(&data->lock);
> > + iio_trigger_notify_done(indio_dev->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static void bmp280_pm_disable(void *data)
> > {
> > struct device *dev = data;
> > @@ -2329,6 +2383,13 @@ int bmp280_common_probe(struct device *dev,
> > return ret;
> > }
> >
> > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> > + iio_pollfunc_store_time,
> > + &bmp280_buffer_handler, NULL);
> > + if (ret)
> > + return dev_err_probe(data->dev, ret,
> > + "iio triggered buffer setup failed\n");
> > +
> > /* Enable runtime PM */
> > pm_runtime_get_noresume(dev);
> > pm_runtime_set_active(dev);
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index c8cb7c417dab..b5369dd496ba 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -407,6 +407,13 @@ struct bmp280_data {
> > union {
> > /* Sensor data buffer */
> > u8 buf[3];
> > + /* Data buffer to push to userspace */
>
> Why is this in the union? This union is to ensure cache safe buffers for
> DMAing directly into. That's not applicable here. Even though it may
> be safe to do this (I was reading backwards so wrote a comment here
> on you corrupting temperature before seeing the comment above)
> it is giving a misleading impression of how this struct is used.
> Pull the struct to after t_fine - It only needs to
> be 8 byte aligned and the aligned marking you have will ensure that
>
> > + struct {
> > + s32 temperature;
> > + u32 pressure;
> > + u32 humidity;
>
> As above, this is 3 4 byte buffers but they don't have these meanings
> unless all channels are enabled.
>
> You could set available mask to enforce that, but don't as it makes
> limited sense for this hardware. Just make these
> u32 chans[3];
> and fill them in with an index incremented for each channel that is
> enabled.
>
> Jonathan
>
I wanted to put it inside the union to save some space but you are right
that it is quite misleading. I was just trying in general, along with the
previous patches to avoid reading the temperature twice. Along with your
comments in the previous patch, if a user has enabled both temperature
and pressure and humidity we could save ourselves from reading the
temperature 3 times instead of 1. But in any case, your previous proposal
with a separate get_t_fine structure looks good.
Best regards,
Vasilis
> > + s64 timestamp __aligned(8);
> > + } iio_buf;
> > /* Calibration data buffers */
> > __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> > __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
>
