On Thu, 14 Mar 2024 21:14:31 +0100
Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
> 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.
It may not save any space. The force alignment here tends to lead to some
room between the first chunk of this structure and the __aligned(IIO_DMA_MINALIGN) part.
This may well fit in the hole - I've not checked though.
> 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.
If it gets complex, just implement a buffered read that always gets all the
channels and set available_scan_masks to express that. The core code
has channel data shuffling that will result in your userspace seeing
whatever subset of channels it requests.
Jonathan
>
> 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];
> >
