Received-SPF: pass (google.com: domain of linux-kernel+bounces-128485-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Vasileios Amoiridis <vassilisamir@gmail.com>
Date: Tue, 2 Apr 2024 20:08:49 +0200
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, phil@raspberrypi.com, 579lpy@gmail.com,
	linus.walleij@linaro.org, semen.protsenko@linaro.org,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 6/6] iio: pressure: Add triggered buffer support for
 BMP280 driver
Message-ID: <20240402180849.GB18068@vamoiridPC>
References: <20240319002925.2121016-1-vassilisamir@gmail.com>
 <20240319002925.2121016-7-vassilisamir@gmail.com>
 <20240324121418.570857a9@jic23-huawei>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240324121418.570857a9@jic23-huawei>

On Sun, Mar 24, 2024 at 12:14:18PM +0000, Jonathan Cameron wrote:
> On Tue, 19 Mar 2024 01:29:25 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > BMP2xx, BMP3xx, and BMP5xx use consecutive buffers for their
> > temperature, pressure and humidity readings. This facilitates
> > the use of burst reads in order to acquire data much faster
> > and in a different way from the one used in oneshot captures.
> > 
> > BMP085 and BMP180 use a completely different measurement
> > process that is well defined and is used in their buffer_handler().
> > 
> > Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Various comments inline,
> 
> Thanks,
> Jonathan
> 
> > ---
> >  drivers/iio/pressure/Kconfig       |   2 +
> >  drivers/iio/pressure/bmp280-core.c | 231 +++++++++++++++++++++++++++--
> >  drivers/iio/pressure/bmp280-spi.c  |   8 +-
> >  drivers/iio/pressure/bmp280.h      |  14 +-
> >  4 files changed, 241 insertions(+), 14 deletions(-)
> > 
> > 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 ddfcd23f29a0..ffcd17807cf5 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>
> >  
> > @@ -454,7 +457,7 @@ static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp)
> >  	int ret;
> >  
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> > -			       data->buf, sizeof(data->buf));
> > +			       data->buf, BMP280_NUM_TEMP_BYTES);
> >  	if (ret < 0) {
> >  		dev_err(data->dev, "failed to read temperature\n");
> >  		return ret;
> > @@ -484,7 +487,7 @@ static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press)
> >  		return ret;
> >  
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> > -			       data->buf, sizeof(data->buf));
> > +			       data->buf, BMP280_NUM_PRESS_BYTES);
> >  	if (ret < 0) {
> >  		dev_err(data->dev, "failed to read pressure\n");
> >  		return ret;
> > @@ -513,13 +516,13 @@ static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
> >  		return ret;
> >  
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> > -			       &data->be16, sizeof(data->be16));
> > +			       &data->be16, BME280_NUM_HUMIDITY_BYTES);
> >  	if (ret < 0) {
> >  		dev_err(data->dev, "failed to read humidity\n");
> >  		return ret;
> >  	}
> >  
> > -	adc_humidity = be16_to_cpu(data->be16);
> > +	adc_humidity = get_unaligned_be16(&data->be16);
> 
> If it's a be16, how did it become unaligned?
> 
> >  	if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
> >  		/* reading was skipped */
> >  		dev_err(data->dev, "reading humidity skipped\n");
> > @@ -931,6 +934,73 @@ static int bmp280_chip_config(struct bmp280_data *data)
> >  	return ret;
> >  }
> >  
> > +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);
> > +	s32 adc_temp, adc_press, adc_humidity;
> > +	u8 size_of_burst_read;
> > +	int ret, chan_value;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	if (test_bit(BME280_HUMID, indio_dev->active_scan_mask))
> 
> This confuses me a little. Is it allowing reuse of this function for
> multiple devices or aiming to optimise the read in the case of
> the humidity channel being disabled (in which case I don't think
> it works because you aren't providing that combination in avail_scan_masks.)
> 
> Add a comment to explain.
> 

Hi Jonathan,

It is aimed to reuse the function both for BMP280 and BME280 so that's why is
there, it's not in case humidity channel is disabled. I can add a comment it
is definitely not obvious. Thanks for pointing this out.

By applying the changes that you pointed out + by implementing the changes
that you proposed in a previous patch to split the t_fine calculation this
patch will become much cleaner, thanks a lot!

Cheers,
Vasilis
> > +		size_of_burst_read = 8;
> > +	else
> > +		size_of_burst_read = 6;
> > +
> > +	/* Burst read data registers */
> > +	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> > +			       data->buf, 8);
> > +	if (ret) {
> > +		dev_err(data->dev, "failed to read sensor data\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Temperature calculations */
> > +	memcpy(&chan_value, &data->buf[3], 3);
> > +
> > +	adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&chan_value));
> > +	if (adc_temp == BMP280_TEMP_SKIPPED) {
> > +		/* reading was skipped */
> Similar comments on this to below (I read backwards as normally code makes
> more sense that way :)
> > +		dev_err(data->dev, "reading temperature skipped\n");
> > +		return -EIO;
> > +	}
> > +	data->iio_buf.sensor_data[0] = bmp280_compensate_temp(data, adc_temp);
> > +
> > +	/* Pressure calculations */
> > +	memcpy(&chan_value, &data->buf[0], 3);
> > +
> > +	adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&chan_value));
> > +	if (adc_press == BMP280_PRESS_SKIPPED) {
> > +		/* reading was skipped */
> > +		dev_err(data->dev, "reading pressure skipped\n");
> > +		return -EIO;
> > +	}
> > +	data->iio_buf.sensor_data[1] = bmp280_compensate_press(data, adc_press);
> > +
> > +	/* Humidity calculations */
> > +	if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> > +		memcpy(&chan_value, &data->buf[6], 2);
> > +
> > +		adc_humidity = get_unaligned_be16(&chan_value);
> > +		if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
> > +			/* reading was skipped */
> > +			dev_err(data->dev, "reading humidity skipped\n");
> > +			return -EIO;
> > +		}
> > +	data->iio_buf.sensor_data[2] = bmp280_compensate_humidity(data, adc_humidity);
> Alignment of code looks wrong.
> 
> > +	}
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> > +static irqreturn_t bmp380_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);
> > +	s32 adc_temp, adc_press;
> > +	int ret, chan_value;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	/* Burst read data registers */
> > +	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> > +			       data->buf, 6);
> > +	if (ret) {
> > +		dev_err(data->dev, "failed to read sensor data\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Temperature calculations */
> > +	memcpy(&chan_value, &data->buf[3], 3);
> > +
> > +	adc_temp = get_unaligned_le24(&chan_value);
> 
> Same comments as below.
> 
> > +	if (adc_temp == BMP380_TEMP_SKIPPED) {
> > +		/* reading was skipped */
> > +		dev_err(data->dev, "reading temperature skipped\n");
> > +		return -EIO;
> > +	}
> > +	data->iio_buf.sensor_data[0] = bmp380_compensate_temp(data, adc_temp);
> > +
> > +	/* Pressure calculations */
> > +	memcpy(&chan_value, &data->buf[0], 3);
> > +
> > +	adc_press = get_unaligned_le24(&chan_value);
> > +	if (adc_press == BMP380_PRESS_SKIPPED) {
> > +		/* reading was skipped */
> > +		dev_err(data->dev, "reading pressure skipped\n");
> > +		return -EIO;
> > +	}
> > +	data->iio_buf.sensor_data[1] = bmp380_compensate_press(data, adc_press);
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> > +static irqreturn_t bmp580_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);
> > +	s32 adc_temp, adc_press;
> > +	int ret, chan_value;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	/* Burst read data registers */
> > +	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB,
> > +			       data->buf, 6);
> > +	if (ret) {
> > +		dev_err(data->dev, "failed to read sensor data\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Temperature calculations */
> > +	memcpy(&chan_value, &data->buf[3], 3);
> > +
> > +	adc_temp = get_unaligned_le24(&chan_value);
> 
> As in other thread branch, get it directly from &data->buf[3]
> rather than bouncing it via another buffer.
> 
> > +	if (adc_temp == BMP580_TEMP_SKIPPED) {
> > +		/* reading was skipped */
> > +		dev_err(data->dev, "reading temperature skipped\n");
> > +		return -EIO;
> -EIO isn't irqreturn_t 
> 
> There isn't a good way to return errors from interrupt handlers,
> so just return IRQ_HANDLED after printing the error message.
> 
> > +	}
> > +	data->iio_buf.sensor_data[0] = adc_temp;
> > +
> > +	/* Pressure calculations */
> > +	memcpy(&chan_value, &data->buf[0], 3);
> > +
> > +	adc_press = get_unaligned_le24(&chan_value);
> As above, get it directly.
> 
> > +	if (adc_press == BMP580_PRESS_SKIPPED) {
> > +		/* reading was skipped */
> > +		dev_err(data->dev, "reading pressure skipped\n");
> > +		return -EIO;
> return IRQ_HANDLED;
> 
> > +	}
> > +	data->iio_buf.sensor_data[1] =  adc_press;
> Extra space.
> 
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
> >  static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT };
> >  static const int bmp580_temp_coeffs[] = { 1000, 16 };
> > @@ -1903,6 +2075,8 @@ const struct bmp280_chip_info bmp580_chip_info = {
> >  	.read_temp = bmp580_read_temp,
> >  	.read_press = bmp580_read_press,
> >  	.preinit = bmp580_preinit,
> > +
> > +	.buffer_handler = bmp580_buffer_handler
> >  };
> >  EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280);
> >  
> > @@ -2054,7 +2228,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
> >  		return ret;
> >  
> >  	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB,
> > -			       data->buf, sizeof(data->buf));
> > +			       data->buf, BMP280_NUM_PRESS_BYTES);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -2122,6 +2296,35 @@ static int bmp180_chip_config(struct bmp280_data *data)
> >  	return 0;
> >  }
> >  
> > +static irqreturn_t bmp180_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, chan_value;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	ret = data->chip_info->read_temp(data, &chan_value);
> > +	if (ret)
> > +		return ret;
> > +
> > +	data->iio_buf.sensor_data[0] = chan_value;
> > +
> > +	ret = data->chip_info->read_press(data, &chan_value);
> 
> Given my earlier suggestion to stop hiding t_fine in the
> structure, these callbacks will need to bring it all the way out
> here (from the temperature read) and pass it back into the
> pressure read.
> 
> That will have the pleasing side effect of making it obvious why
> we aren't handling subsets of channels (as they aren't
> independent)
> 
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	data->iio_buf.sensor_data[1] = chan_value;
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> 
> > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> > index a444d4b2978b..dc297583cac1 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -40,14 +40,14 @@ static int bmp380_regmap_spi_read(void *context, const void *reg,
> >  				  size_t reg_size, void *val, size_t val_size)
> >  {
> >  	struct spi_device *spi = to_spi_device(context);
> > -	u8 rx_buf[4];
> > +	u8 rx_buf[9];
> >  	ssize_t status;
> >  
> >  	/*
> > -	 * Maximum number of consecutive bytes read for a temperature or
> > -	 * pressure measurement is 3.
> > +	 * Maximum number of a burst read for temperature, pressure, humidity
> > +	 * is 8 bytes.
> 
> Once this 8 is expressed as the sum of the 3 types, you can drop the comment
> as it will add nothing useful.
> 
> >  	 */
> > -	if (val_size > 3)
> > +	if (val_size > 8)
> >  		return -EINVAL;
> >  
> >  	/*
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index 8cc3eed70c18..32155567faf6 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -301,6 +301,10 @@
> >  #define BMP280_PRESS_SKIPPED		0x80000
> >  #define BMP280_HUMIDITY_SKIPPED		0x8000
> >  
> > +#define BMP280_NUM_TEMP_BYTES		3
> > +#define BMP280_NUM_PRESS_BYTES		3
> > +#define BME280_NUM_HUMIDITY_BYTES	2
> > +
> >  /* Core exported structs */
> >  
> >  static const char *const bmp280_supply_names[] = {
> > @@ -400,13 +404,19 @@ struct bmp280_data {
> >  	 */
> >  	s32 t_fine;
> >  
> > +	/* Data to push to userspace */
> > +	struct {
> > +		s32 sensor_data[3];
> 
> This doesn't work (as explanation of data) for all cases.
>  If you only have 2 channels, the packing needs to be.
> 
> 		s32 sensor_data[2];
> //no padding
> 		s64 timestamp __aligned(8);
> > +		s64 timestamp __aligned(8);
> > +	} iio_buf;
> > +
> So using a structure for this definition isn't a bug as such as the core
> doesn't care that you provide a bigger buffer than needed, but it is misleading.
> Use something along lines of
> 
> 	/* Up to 3 channels and aligned s64 timestamp */
> 	s32 sensor_data[6] __aligned(8);
> 
> or use a union of two structures to cover the two layouts making
> sure to write and read from the correct one in each callback.
> 
> >  	/*
> >  	 * DMA (thus cache coherency maintenance) may require the
> >  	 * transfer buffers to live in their own cache lines.
> >  	 */
> >  	union {
> >  		/* Sensor data buffer */
> > -		u8 buf[3];
> > +		u8 buf[8];
> As in previous discussion - build this up as a sum of what can go in it.
> Maybe via a define for BMP280_BURST_READ_MAX 
>