Re: [PATCH v2 6/6] iio: pressure: Add triggered buffer support for BMP280 driver

[Jonathan Cameron]

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];  
> >