Received-SPF: pass (google.com: domain of linux-kernel+bounces-103758-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Vasileios Amoiridis <vassilisamir@gmail.com>
Date: Thu, 14 Mar 2024 21:14:31 +0100
To: Jonathan Cameron <jic23@kernel.org>
Cc: Vasileios Amoiridis <vassilisamir@gmail.com>, lars@metafoo.de,
	andriy.shevchenko@linux.intel.com, ang.iglesiasg@gmail.com,
	mazziesaccount@gmail.com, ak@it-klinger.de,
	petre.rodan@subdimension.ro, linus.walleij@linaro.org,
	phil@raspberrypi.com, 579lpy@gmail.com, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 6/6] iio: pressure: Add triggered buffer support for
 BMP280 driver
Message-ID: <20240314201431.GC1964894@vamoiridPC>
References: <20240313174007.1934983-1-vassilisamir@gmail.com>
 <20240313174007.1934983-7-vassilisamir@gmail.com>
 <20240314152525.51bcdcd6@jic23-huawei>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240314152525.51bcdcd6@jic23-huawei>

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@gmail.com> 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@gmail.com>
> > ---
> >  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];
>