[PATCH v3 0/6] Series to add triggered buffer support to BMP280 driver

[PATCH v3 0/6] Series to add triggered buffer support to BMP280 driver

[Vasileios Amoiridis]

Changes in v3:

Patch 2: Add guard(mutex) as per request {read/write}_raw() functions.

Patch 3: Patch 2 of v2. Added IIO return value as per request.

Patch 4: Patch 3 of v2.

Patch 5: Patch 4 of v2.

Patch 6: Completely different approach from v2. Instead of leveraging the
functionality of the read_*() functions for the oneshot capture, burst reads
were used in order to read all the values in one single read operation. This
minimizes the number of accesses to the device to just 1 time, which all the
values are being read. Different buffer_handler() functions were implemented
for the different "families" of sensors because there were a lot of differences
in the register configuration and read operation for different sensors.

BMP085 and BMP180 have a very well defined read operation that is kept in the
buffer_handler(). There is no option for a burst read in these sensors.

BM(P/E)2xx, BMP3xx, and BMP5xx have their own buffer_handler() functions.
Registers, endianess and compensation formulas are different in each one of
those 3 categories which doesn't allow for a more generic buffer_handler().

[v2] https://lore.kernel.org/linux-iio/20240313174007.1934983-1-vassilisamir@gmail.com
[v1] https://lore.kernel.org/linux-iio/20240303165300.468011-1-vassilisamir@gmail.com

Vasileios Amoiridis (6):
  iio: pressure: BMP280 core driver headers sorting
  iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw()
    functions
  iio: pressure: Generalize read_{temp/press/humid}() functions
  iio: pressure: Add SCALE and RAW values for channels
  iio: pressure: Add timestamp and scan_masks for BMP280 driver
  iio: pressure: Add triggered buffer support for BMP280 driver

 drivers/iio/pressure/Kconfig       |   2 +
 drivers/iio/pressure/bmp280-core.c | 727 +++++++++++++++++++++--------
 drivers/iio/pressure/bmp280-spi.c  |   8 +-
 drivers/iio/pressure/bmp280.h      |  28 +-
 4 files changed, 570 insertions(+), 195 deletions(-)

-- 
2.25.1

[PATCH v3 1/6] iio: pressure: BMP280 core driver headers sorting

[Vasileios Amoiridis]

Sort headers in alphabetical order.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index fe8734468ed3..871b2214121b 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -27,20 +27,20 @@
 
 #include <linux/bitops.h>
 #include <linux/bitfield.h>
-#include <linux/device.h>
-#include <linux/module.h>
-#include <linux/nvmem-provider.h>
-#include <linux/regmap.h>
+#include <linux/completion.h>
 #include <linux/delay.h>
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
+#include <linux/device.h>
 #include <linux/gpio/consumer.h>
-#include <linux/regulator/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h> /* For irq_get_irq_data() */
-#include <linux/completion.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
 #include <linux/pm_runtime.h>
 #include <linux/random.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
 
 #include <asm/unaligned.h>
 
-- 
2.25.1

Re: [PATCH v3 1/6] iio: pressure: BMP280 core driver headers sorting

[Andy Shevchenko]

On Tue, Mar 19, 2024 at 01:29:20AM +0100, Vasileios Amoiridis wrote:
> Sort headers in alphabetical order.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/6] iio: pressure: BMP280 core driver headers sorting

[Jonathan Cameron]

On Wed, 20 Mar 2024 13:00:41 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Mar 19, 2024 at 01:29:20AM +0100, Vasileios Amoiridis wrote:
> > Sort headers in alphabetical order.  
> 
> FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 

Hi Vasileios.

To reduce the number of patches we need to consider if this goes
to a v4, I'm going to pick up what seems to be definitely ready to
go now.

Applied this one to the togreg-normal branch of iio.git and pushed
out for 0-day to see if it can find any problems.

Jonathan

[PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions

[Vasileios Amoiridis]

Introduce the new linux/cleanup.h with the guard(mutex) functionality
in the {read/write}_raw() functions

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 125 +++++++++++++----------------
 1 file changed, 58 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 871b2214121b..f7a13ff6f26c 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -460,77 +460,74 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 	return IIO_VAL_INT;
 }
 
-static int bmp280_read_raw(struct iio_dev *indio_dev,
-			   struct iio_chan_spec const *chan,
-			   int *val, int *val2, long mask)
+static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan,
+				   int *val, int *val2, long mask)
 {
 	struct bmp280_data *data = iio_priv(indio_dev);
-	int ret;
 
-	pm_runtime_get_sync(data->dev);
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
-			ret = data->chip_info->read_humid(data, val, val2);
-			break;
+			return data->chip_info->read_humid(data, val, val2);
 		case IIO_PRESSURE:
-			ret = data->chip_info->read_press(data, val, val2);
-			break;
+			return data->chip_info->read_press(data, val, val2);
 		case IIO_TEMP:
-			ret = data->chip_info->read_temp(data, val, val2);
-			break;
+			return data->chip_info->read_temp(data, val, val2);
 		default:
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
-		break;
+		return 0;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
 			*val = 1 << data->oversampling_humid;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		case IIO_PRESSURE:
 			*val = 1 << data->oversampling_press;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		case IIO_TEMP:
 			*val = 1 << data->oversampling_temp;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		default:
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
-		break;
+		return 0;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (!data->chip_info->sampling_freq_avail) {
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
 		*val = data->chip_info->sampling_freq_avail[data->sampling_freq][0];
 		*val2 = data->chip_info->sampling_freq_avail[data->sampling_freq][1];
-		ret = IIO_VAL_INT_PLUS_MICRO;
-		break;
+		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		if (!data->chip_info->iir_filter_coeffs_avail) {
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
 		*val = (1 << data->iir_filter_coeff) - 1;
-		ret = IIO_VAL_INT;
-		break;
+		return IIO_VAL_INT;
 	default:
-		ret = -EINVAL;
-		break;
+		return -EINVAL;
 	}
 
-	mutex_unlock(&data->lock);
+	return 0;
+}
+
+
+static int bmp280_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	int ret;
+
+	pm_runtime_get_sync(data->dev);
+	ret = bmp280_read_raw_guarded(indio_dev, chan, val, val2, mask);
 	pm_runtime_mark_last_busy(data->dev);
 	pm_runtime_put_autosuspend(data->dev);
 
@@ -662,13 +659,13 @@ static int bmp280_write_iir_filter_coeffs(struct bmp280_data *data, int val)
 	return -EINVAL;
 }
 
-static int bmp280_write_raw(struct iio_dev *indio_dev,
-			    struct iio_chan_spec const *chan,
-			    int val, int val2, long mask)
+static int bmp280_write_raw_guarded(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan,
+				    int val, int val2, long mask)
 {
 	struct bmp280_data *data = iio_priv(indio_dev);
-	int ret = 0;
 
+	guard(mutex)(&data->lock);
 	/*
 	 * Helper functions to update sensor running configuration.
 	 * If an error happens applying new settings, will try restore
@@ -677,46 +674,40 @@ static int bmp280_write_raw(struct iio_dev *indio_dev,
 	 */
 	switch (mask) {
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		pm_runtime_get_sync(data->dev);
-		mutex_lock(&data->lock);
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
-			ret = bmp280_write_oversampling_ratio_humid(data, val);
-			break;
+			return bmp280_write_oversampling_ratio_humid(data, val);
 		case IIO_PRESSURE:
-			ret = bmp280_write_oversampling_ratio_press(data, val);
-			break;
+			return bmp280_write_oversampling_ratio_press(data, val);
 		case IIO_TEMP:
-			ret = bmp280_write_oversampling_ratio_temp(data, val);
-			break;
+			return bmp280_write_oversampling_ratio_temp(data, val);
 		default:
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
-		mutex_unlock(&data->lock);
-		pm_runtime_mark_last_busy(data->dev);
-		pm_runtime_put_autosuspend(data->dev);
-		break;
+		return 0;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		pm_runtime_get_sync(data->dev);
-		mutex_lock(&data->lock);
-		ret = bmp280_write_sampling_frequency(data, val, val2);
-		mutex_unlock(&data->lock);
-		pm_runtime_mark_last_busy(data->dev);
-		pm_runtime_put_autosuspend(data->dev);
-		break;
+		return bmp280_write_sampling_frequency(data, val, val2);
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		pm_runtime_get_sync(data->dev);
-		mutex_lock(&data->lock);
-		ret = bmp280_write_iir_filter_coeffs(data, val);
-		mutex_unlock(&data->lock);
-		pm_runtime_mark_last_busy(data->dev);
-		pm_runtime_put_autosuspend(data->dev);
-		break;
+		return bmp280_write_iir_filter_coeffs(data, val);
 	default:
 		return -EINVAL;
 	}
 
+	return 0;
+}
+
+static int bmp280_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	pm_runtime_get_sync(data->dev);
+	ret = bmp280_write_raw_guarded(indio_dev, chan, val, val2, mask);
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions

[Andy Shevchenko]

On Tue, Mar 19, 2024 at 01:29:21AM +0100, Vasileios Amoiridis wrote:
> Introduce the new linux/cleanup.h with the guard(mutex) functionality
> in the {read/write}_raw() functions

Missing period at the end of the sentence.

...

>  	switch (mask) {
>  	case IIO_CHAN_INFO_PROCESSED:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> -			ret = data->chip_info->read_humid(data, val, val2);
> -			break;
> +			return data->chip_info->read_humid(data, val, val2);
>  		case IIO_PRESSURE:
> -			ret = data->chip_info->read_press(data, val, val2);
> -			break;
> +			return data->chip_info->read_press(data, val, val2);
>  		case IIO_TEMP:
> -			ret = data->chip_info->read_temp(data, val, val2);
> -			break;
> +			return data->chip_info->read_temp(data, val, val2);
>  		default:
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}

> -		break;
> +		return 0;

Now dead code.

>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
>  			*val = 1 << data->oversampling_humid;
> -			ret = IIO_VAL_INT;
> -			break;
> +			return IIO_VAL_INT;
>  		case IIO_PRESSURE:
>  			*val = 1 << data->oversampling_press;
> -			ret = IIO_VAL_INT;
> -			break;
> +			return IIO_VAL_INT;
>  		case IIO_TEMP:
>  			*val = 1 << data->oversampling_temp;
> -			ret = IIO_VAL_INT;
> -			break;
> +			return IIO_VAL_INT;
>  		default:
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}

> -		break;
> +		return 0;

Ditto.

>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (!data->chip_info->sampling_freq_avail) {
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}
>  
>  		*val = data->chip_info->sampling_freq_avail[data->sampling_freq][0];
>  		*val2 = data->chip_info->sampling_freq_avail[data->sampling_freq][1];
> -		ret = IIO_VAL_INT_PLUS_MICRO;
> -		break;
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>  		if (!data->chip_info->iir_filter_coeffs_avail) {
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}
>  
>  		*val = (1 << data->iir_filter_coeff) - 1;
> -		ret = IIO_VAL_INT;
> -		break;
> +		return IIO_VAL_INT;
>  	default:
> -		ret = -EINVAL;
> -		break;
> +		return -EINVAL;
>  	}

> -	mutex_unlock(&data->lock);
> +	return 0;

Ditto.

...

> +
> +

One blank line is enough.

> +static int bmp280_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)

...

>  	switch (mask) {
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		pm_runtime_get_sync(data->dev);
> -		mutex_lock(&data->lock);
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> -			ret = bmp280_write_oversampling_ratio_humid(data, val);
> -			break;
> +			return bmp280_write_oversampling_ratio_humid(data, val);
>  		case IIO_PRESSURE:
> -			ret = bmp280_write_oversampling_ratio_press(data, val);
> -			break;
> +			return bmp280_write_oversampling_ratio_press(data, val);
>  		case IIO_TEMP:
> -			ret = bmp280_write_oversampling_ratio_temp(data, val);
> -			break;
> +			return bmp280_write_oversampling_ratio_temp(data, val);
>  		default:
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}
> -		mutex_unlock(&data->lock);
> -		pm_runtime_mark_last_busy(data->dev);
> -		pm_runtime_put_autosuspend(data->dev);
> -		break;
> +		return 0;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		pm_runtime_get_sync(data->dev);
> -		mutex_lock(&data->lock);
> -		ret = bmp280_write_sampling_frequency(data, val, val2);
> -		mutex_unlock(&data->lock);
> -		pm_runtime_mark_last_busy(data->dev);
> -		pm_runtime_put_autosuspend(data->dev);
> -		break;
> +		return bmp280_write_sampling_frequency(data, val, val2);
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		pm_runtime_get_sync(data->dev);
> -		mutex_lock(&data->lock);
> -		ret = bmp280_write_iir_filter_coeffs(data, val);
> -		mutex_unlock(&data->lock);
> -		pm_runtime_mark_last_busy(data->dev);
> -		pm_runtime_put_autosuspend(data->dev);
> -		break;
> +		return bmp280_write_iir_filter_coeffs(data, val);
>  	default:
>  		return -EINVAL;
>  	}
>  
> +	return 0;

As per above, dead code.

...

> +static int bmp280_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct bmp280_data *data = iio_priv(indio_dev);

> +	int ret = 0;

Useless assignment.

> +
> +	pm_runtime_get_sync(data->dev);
> +	ret = bmp280_write_raw_guarded(indio_dev, chan, val, val2, mask);
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +
>  	return ret;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions

[Andy Shevchenko]

On Wed, Mar 20, 2024 at 01:00:01PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 19, 2024 at 01:29:21AM +0100, Vasileios Amoiridis wrote:
> > Introduce the new linux/cleanup.h with the guard(mutex) functionality
> > in the {read/write}_raw() functions
> 
> Missing period at the end of the sentence.

And also

{read,write}_raw()

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions

[Jonathan Cameron]

On Tue, 19 Mar 2024 01:29:21 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Introduce the new linux/cleanup.h with the guard(mutex) functionality
> in the {read/write}_raw() functions
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-core.c | 125 +++++++++++++----------------
>  1 file changed, 58 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 871b2214121b..f7a13ff6f26c 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -460,77 +460,74 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
>  	return IIO_VAL_INT;
>  }
>  
> -static int bmp280_read_raw(struct iio_dev *indio_dev,
> -			   struct iio_chan_spec const *chan,
> -			   int *val, int *val2, long mask)
> +static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,

Why do we need the guarded naming?  It always took the lock, no
change just because we are doing that in a neater fashion.

I don't see a reason for that name.  Better to use something like
_impl, or _internal as the prefix.

I don't want to see people calling every function that uses guard
_guarded().


> +				   struct iio_chan_spec const *chan,
> +				   int *val, int *val2, long mask)
Otherwise, I didn't find anything beyond what Andy already pointed out.
So looks good in general.

Jonathan

[PATCH v3 3/6] iio: pressure: Generalize read_{temp/press/humid}() functions

[Vasileios Amoiridis]

Add the coefficients for the IIO standard units and the return
IIO value inside the chip_info structure.

Remove the calculations with the coefficients for the IIO unit
compatibility from inside the read_{temp/press/humid}() functions
and move it to the general read_raw() function.

Execute the calculations with the coefficients inside the read_raw()
oneshot capture function.

In this way, all the data for the calculation of the value are
located in the chip_info structure of the respective sensor.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 189 +++++++++++++++--------------
 drivers/iio/pressure/bmp280.h      |  13 +-
 2 files changed, 106 insertions(+), 96 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index f7a13ff6f26c..6d6173c4b744 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -363,10 +363,9 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 	return (u32)p;
 }
 
-static int bmp280_read_temp(struct bmp280_data *data,
-			    int *val, int *val2)
+static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp)
 {
-	s32 adc_temp, comp_temp;
+	s32 adc_temp;
 	int ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
@@ -382,29 +381,20 @@ static int bmp280_read_temp(struct bmp280_data *data,
 		dev_err(data->dev, "reading temperature skipped\n");
 		return -EIO;
 	}
-	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
-	/*
-	 * val might be NULL if we're called by the read_press routine,
-	 * who only cares about the carry over t_fine value.
-	 */
-	if (val) {
-		*val = comp_temp * 10;
-		return IIO_VAL_INT;
-	}
+	if (comp_temp)
+		*comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	return 0;
 }
 
-static int bmp280_read_press(struct bmp280_data *data,
-			     int *val, int *val2)
+static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press)
 {
-	u32 comp_press;
 	s32 adc_press;
 	int ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
-	ret = bmp280_read_temp(data, NULL, NULL);
+	ret = bmp280_read_temp(data, NULL);
 	if (ret < 0)
 		return ret;
 
@@ -421,22 +411,19 @@ static int bmp280_read_press(struct bmp280_data *data,
 		dev_err(data->dev, "reading pressure skipped\n");
 		return -EIO;
 	}
-	comp_press = bmp280_compensate_press(data, adc_press);
 
-	*val = comp_press;
-	*val2 = 256000;
+	*comp_press = bmp280_compensate_press(data, adc_press);
 
-	return IIO_VAL_FRACTIONAL;
+	return 0;
 }
 
-static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
+static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
 {
-	u32 comp_humidity;
 	s32 adc_humidity;
 	int ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
-	ret = bmp280_read_temp(data, NULL, NULL);
+	ret = bmp280_read_temp(data, NULL);
 	if (ret < 0)
 		return ret;
 
@@ -453,11 +440,10 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 		dev_err(data->dev, "reading humidity skipped\n");
 		return -EIO;
 	}
-	comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
 
-	*val = comp_humidity * 1000 / 1024;
+	*comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
 
-	return IIO_VAL_INT;
+	return 0;
 }
 
 static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
@@ -465,6 +451,8 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
 				   int *val, int *val2, long mask)
 {
 	struct bmp280_data *data = iio_priv(indio_dev);
+	int chan_value;
+	int ret;
 
 	guard(mutex)(&data->lock);
 
@@ -472,11 +460,29 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
-			return data->chip_info->read_humid(data, val, val2);
+			ret = data->chip_info->read_humid(data, &chan_value);
+			if (ret)
+				return ret;
+
+			*val = data->chip_info->humid_coeffs[0] * chan_value;
+			*val2 = data->chip_info->humid_coeffs[1];
+			return data->chip_info->humid_coeffs_type;
 		case IIO_PRESSURE:
-			return data->chip_info->read_press(data, val, val2);
+			ret = data->chip_info->read_press(data, &chan_value);
+			if (ret)
+				return ret;
+
+			*val = data->chip_info->press_coeffs[0] * chan_value;
+			*val2 = data->chip_info->press_coeffs[1];
+			return data->chip_info->press_coeffs_type;
 		case IIO_TEMP:
-			return data->chip_info->read_temp(data, val, val2);
+			ret = data->chip_info->read_temp(data, &chan_value);
+			if (ret)
+				return ret;
+
+			*val = data->chip_info->temp_coeffs[0] * chan_value;
+			*val2 = data->chip_info->temp_coeffs[1];
+			return data->chip_info->temp_coeffs_type;
 		default:
 			return -EINVAL;
 		}
@@ -787,6 +793,8 @@ static int bmp280_chip_config(struct bmp280_data *data)
 
 static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
 static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID };
+static const int bmp280_temp_coeffs[] = { 10, 1 };
+static const int bmp280_press_coeffs[] = { 1, 256000 };
 
 const struct bmp280_chip_info bmp280_chip_info = {
 	.id_reg = BMP280_REG_ID,
@@ -815,6 +823,11 @@ const struct bmp280_chip_info bmp280_chip_info = {
 	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
 	.oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
 
+	.temp_coeffs = bmp280_temp_coeffs,
+	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
+	.press_coeffs = bmp280_press_coeffs,
+	.press_coeffs_type = IIO_VAL_FRACTIONAL,
+
 	.chip_config = bmp280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
@@ -841,6 +854,7 @@ static int bme280_chip_config(struct bmp280_data *data)
 }
 
 static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
+static const int bme280_humid_coeffs[] = { 1000, 1024 };
 
 const struct bmp280_chip_info bme280_chip_info = {
 	.id_reg = BMP280_REG_ID,
@@ -863,6 +877,14 @@ const struct bmp280_chip_info bme280_chip_info = {
 	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
 	.oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
 
+	.temp_coeffs = bmp280_temp_coeffs,
+	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
+	.press_coeffs = bmp280_press_coeffs,
+	.press_coeffs_type = IIO_VAL_FRACTIONAL,
+	.humid_coeffs = bme280_humid_coeffs,
+	.humid_coeffs_type = IIO_VAL_FRACTIONAL,
+
+
 	.chip_config = bme280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
@@ -988,9 +1010,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press)
 	return comp_press;
 }
 
-static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
+static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp)
 {
-	s32 comp_temp;
 	u32 adc_temp;
 	int ret;
 
@@ -1006,29 +1027,20 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
 		dev_err(data->dev, "reading temperature skipped\n");
 		return -EIO;
 	}
-	comp_temp = bmp380_compensate_temp(data, adc_temp);
 
-	/*
-	 * Val might be NULL if we're called by the read_press routine,
-	 * who only cares about the carry over t_fine value.
-	 */
-	if (val) {
-		/* IIO reports temperatures in milli Celsius */
-		*val = comp_temp * 10;
-		return IIO_VAL_INT;
-	}
+	if (comp_temp)
+		*comp_temp = bmp380_compensate_temp(data, adc_temp);
 
 	return 0;
 }
 
-static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
+static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press)
 {
-	s32 comp_press;
 	u32 adc_press;
 	int ret;
 
 	/* Read and compensate for temperature so we get a reading of t_fine */
-	ret = bmp380_read_temp(data, NULL, NULL);
+	ret = bmp380_read_temp(data, NULL);
 	if (ret)
 		return ret;
 
@@ -1044,13 +1056,10 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
 		dev_err(data->dev, "reading pressure skipped\n");
 		return -EIO;
 	}
-	comp_press = bmp380_compensate_press(data, adc_press);
 
-	*val = comp_press;
-	/* Compensated pressure is in cPa (centipascals) */
-	*val2 = 100000;
+	*comp_press = bmp380_compensate_press(data, adc_press);
 
-	return IIO_VAL_FRACTIONAL;
+	return 0;
 }
 
 static int bmp380_read_calib(struct bmp280_data *data)
@@ -1218,6 +1227,8 @@ static int bmp380_chip_config(struct bmp280_data *data)
 static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 };
 static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128};
 static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID };
+static const int bmp380_temp_coeffs[] = { 10, 1 };
+static const int bmp380_press_coeffs[] = { 1, 100000 };
 
 const struct bmp280_chip_info bmp380_chip_info = {
 	.id_reg = BMP380_REG_ID,
@@ -1244,6 +1255,11 @@ const struct bmp280_chip_info bmp380_chip_info = {
 	.num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail),
 	.iir_filter_coeff_default = 2,
 
+	.temp_coeffs = bmp380_temp_coeffs,
+	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
+	.press_coeffs = bmp380_press_coeffs,
+	.press_coeffs_type = IIO_VAL_FRACTIONAL,
+
 	.chip_config = bmp380_chip_config,
 	.read_temp = bmp380_read_temp,
 	.read_press = bmp380_read_press,
@@ -1364,9 +1380,8 @@ static int bmp580_nvm_operation(struct bmp280_data *data, bool is_write)
  * for what is expected on IIO ABI.
  */
 
-static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2)
+static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp)
 {
-	s32 raw_temp;
 	int ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
@@ -1376,25 +1391,17 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2)
 		return ret;
 	}
 
-	raw_temp = get_unaligned_le24(data->buf);
-	if (raw_temp == BMP580_TEMP_SKIPPED) {
+	*raw_temp = get_unaligned_le24(data->buf);
+	if (*raw_temp == BMP580_TEMP_SKIPPED) {
 		dev_err(data->dev, "reading temperature skipped\n");
 		return -EIO;
 	}
 
-	/*
-	 * Temperature is returned in Celsius degrees in fractional
-	 * form down 2^16. We rescale by x1000 to return milli Celsius
-	 * to respect IIO ABI.
-	 */
-	*val = raw_temp * 1000;
-	*val2 = 16;
-	return IIO_VAL_FRACTIONAL_LOG2;
+	return 0;
 }
 
-static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
+static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press)
 {
-	u32 raw_press;
 	int ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
@@ -1404,18 +1411,13 @@ static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
 		return ret;
 	}
 
-	raw_press = get_unaligned_le24(data->buf);
-	if (raw_press == BMP580_PRESS_SKIPPED) {
+	*raw_press = get_unaligned_le24(data->buf);
+	if (*raw_press == BMP580_PRESS_SKIPPED) {
 		dev_err(data->dev, "reading pressure skipped\n");
 		return -EIO;
 	}
-	/*
-	 * Pressure is returned in Pascals in fractional form down 2^16.
-	 * We rescale /1000 to convert to kilopascal to respect IIO ABI.
-	 */
-	*val = raw_press;
-	*val2 = 64000; /* 2^6 * 1000 */
-	return IIO_VAL_FRACTIONAL;
+
+	return 0;
 }
 
 static const int bmp580_odr_table[][2] = {
@@ -1720,6 +1722,8 @@ static int bmp580_chip_config(struct bmp280_data *data)
 
 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 };
+static const int bmp580_press_coeffs[] = { 1, 64000 };
 
 const struct bmp280_chip_info bmp580_chip_info = {
 	.id_reg = BMP580_REG_CHIP_ID,
@@ -1746,6 +1750,11 @@ const struct bmp280_chip_info bmp580_chip_info = {
 	.num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail),
 	.iir_filter_coeff_default = 2,
 
+	.temp_coeffs = bmp280_temp_coeffs,
+	.temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2,
+	.press_coeffs = bmp280_press_coeffs,
+	.press_coeffs_type = IIO_VAL_FRACTIONAL,
+
 	.chip_config = bmp580_chip_config,
 	.read_temp = bmp580_read_temp,
 	.read_press = bmp580_read_press,
@@ -1873,25 +1882,17 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, s32 adc_temp)
 	return (data->t_fine + 8) >> 4;
 }
 
-static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
+static int bmp180_read_temp(struct bmp280_data *data, s32 *comp_temp)
 {
-	s32 adc_temp, comp_temp;
+	s32 adc_temp;
 	int ret;
 
 	ret = bmp180_read_adc_temp(data, &adc_temp);
 	if (ret)
 		return ret;
 
-	comp_temp = bmp180_compensate_temp(data, adc_temp);
-
-	/*
-	 * val might be NULL if we're called by the read_press routine,
-	 * who only cares about the carry over t_fine value.
-	 */
-	if (val) {
-		*val = comp_temp * 100;
-		return IIO_VAL_INT;
-	}
+	if (comp_temp)
+		*comp_temp = bmp180_compensate_temp(data, adc_temp);
 
 	return 0;
 }
@@ -1953,15 +1954,13 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press)
 	return p + ((x1 + x2 + 3791) >> 4);
 }
 
-static int bmp180_read_press(struct bmp280_data *data,
-			     int *val, int *val2)
+static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press)
 {
-	u32 comp_press;
 	s32 adc_press;
 	int ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
-	ret = bmp180_read_temp(data, NULL, NULL);
+	ret = bmp180_read_temp(data, NULL);
 	if (ret)
 		return ret;
 
@@ -1969,12 +1968,9 @@ static int bmp180_read_press(struct bmp280_data *data,
 	if (ret)
 		return ret;
 
-	comp_press = bmp180_compensate_press(data, adc_press);
-
-	*val = comp_press;
-	*val2 = 1000;
+	*comp_press = bmp180_compensate_press(data, adc_press);
 
-	return IIO_VAL_FRACTIONAL;
+	return 0;
 }
 
 static int bmp180_chip_config(struct bmp280_data *data)
@@ -1985,6 +1981,8 @@ static int bmp180_chip_config(struct bmp280_data *data)
 static const int bmp180_oversampling_temp_avail[] = { 1 };
 static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
 static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID };
+static const int bmp180_temp_coeffs[] = { 100, 1 };
+static const int bmp180_press_coeffs[] = { 1, 1000 };
 
 const struct bmp280_chip_info bmp180_chip_info = {
 	.id_reg = BMP280_REG_ID,
@@ -2005,6 +2003,11 @@ const struct bmp280_chip_info bmp180_chip_info = {
 		ARRAY_SIZE(bmp180_oversampling_press_avail),
 	.oversampling_press_default = BMP180_MEAS_PRESS_8X,
 
+	.temp_coeffs = bmp180_temp_coeffs,
+	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
+	.press_coeffs = bmp180_press_coeffs,
+	.press_coeffs_type = IIO_VAL_FRACTIONAL,
+
 	.chip_config = bmp180_chip_config,
 	.read_temp = bmp180_read_temp,
 	.read_press = bmp180_read_press,
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 4012387d7956..6d1dca31dd52 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -448,10 +448,17 @@ struct bmp280_chip_info {
 	int num_sampling_freq_avail;
 	int sampling_freq_default;
 
+	const int *temp_coeffs;
+	const int temp_coeffs_type;
+	const int *press_coeffs;
+	const int press_coeffs_type;
+	const int *humid_coeffs;
+	const int humid_coeffs_type;
+
 	int (*chip_config)(struct bmp280_data *);
-	int (*read_temp)(struct bmp280_data *, int *, int *);
-	int (*read_press)(struct bmp280_data *, int *, int *);
-	int (*read_humid)(struct bmp280_data *, int *, int *);
+	int (*read_temp)(struct bmp280_data *, s32 *);
+	int (*read_press)(struct bmp280_data *, u32 *);
+	int (*read_humid)(struct bmp280_data *, u32 *);
 	int (*read_calib)(struct bmp280_data *);
 	int (*preinit)(struct bmp280_data *);
 };
-- 
2.25.1

Re: [PATCH v3 3/6] iio: pressure: Generalize read_{temp/press/humid}() functions

[Andy Shevchenko]

On Tue, Mar 19, 2024 at 01:29:22AM +0100, Vasileios Amoiridis wrote:
> Add the coefficients for the IIO standard units and the return
> IIO value inside the chip_info structure.
> 
> Remove the calculations with the coefficients for the IIO unit
> compatibility from inside the read_{temp/press/humid}() functions

read_{temp,press,humid}()

> and move it to the general read_raw() function.
> 
> Execute the calculations with the coefficients inside the read_raw()
> oneshot capture function.
> 
> In this way, all the data for the calculation of the value are
> located in the chip_info structure of the respective sensor.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/6] iio: pressure: Generalize read_{temp/press/humid}() functions

[Jonathan Cameron]

On Tue, 19 Mar 2024 01:29:22 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Add the coefficients for the IIO standard units and the return
> IIO value inside the chip_info structure.
> 
> Remove the calculations with the coefficients for the IIO unit
> compatibility from inside the read_{temp/press/humid}() functions
> and move it to the general read_raw() function.
> 
> Execute the calculations with the coefficients inside the read_raw()
> oneshot capture function.
> 
> In this way, all the data for the calculation of the value are
> located in the chip_info structure of the respective sensor.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
Hi,

Perhaps it's later in the series, but I still don't much like the hidden nature
of t_fine. I'd much rather that was explicitly 'returned' by the function
- by that I mean read_temp takes an s32 *t_fine and provides that if the pointer
isn't NULL.

Then drop the cached value in bmp280_data which I think just serves to make
this code less readable than it could be.

Then bmp280_compensate_pressure() can take a struct bmp280_calib, s32 adc_press and
s32 t_fine so it just has the data it needs.
Something similar for bmp280_compenstate_temp()

Obviously this is cleaning up stuff that's been there a long time, but
given you are generalizing these functions this seems like the time to
make these other changes.

As it stands, I don't think this code works as t_fine isn't updated
everywhere it needs to be and that is hidden away by it being updated
as a side effect of other calls.

Jonathan


> ---
>  drivers/iio/pressure/bmp280-core.c | 189 +++++++++++++++--------------
>  drivers/iio/pressure/bmp280.h      |  13 +-
>  2 files changed, 106 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index f7a13ff6f26c..6d6173c4b744 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -363,10 +363,9 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
>  	return (u32)p;
>  }
>  
> -static int bmp280_read_temp(struct bmp280_data *data,
> -			    int *val, int *val2)
> +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp)
>  {
> -	s32 adc_temp, comp_temp;
> +	s32 adc_temp;
>  	int ret;
>  
>  	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> @@ -382,29 +381,20 @@ static int bmp280_read_temp(struct bmp280_data *data,
>  		dev_err(data->dev, "reading temperature skipped\n");
>  		return -EIO;
>  	}
> -	comp_temp = bmp280_compensate_temp(data, adc_temp);
>  
> -	/*
> -	 * val might be NULL if we're called by the read_press routine,
> -	 * who only cares about the carry over t_fine value.
> -	 */
> -	if (val) {
> -		*val = comp_temp * 10;
> -		return IIO_VAL_INT;
> -	}
> +	if (comp_temp)
> +		*comp_temp = bmp280_compensate_temp(data, adc_temp);

As below, I don't think this is updating t_fine.
Another reason to make that update very obvious rather than burried
in this function call.

>  
>  	return 0;
>  }
>  
> -static int bmp280_read_press(struct bmp280_data *data,
> -			     int *val, int *val2)
> +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press)
>  {
> -	u32 comp_press;
>  	s32 adc_press;
>  	int ret;
>  
>  	/* Read and compensate temperature so we get a reading of t_fine. */
> -	ret = bmp280_read_temp(data, NULL, NULL);
> +	ret = bmp280_read_temp(data, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -421,22 +411,19 @@ static int bmp280_read_press(struct bmp280_data *data,
>  		dev_err(data->dev, "reading pressure skipped\n");
>  		return -EIO;
>  	}
> -	comp_press = bmp280_compensate_press(data, adc_press);
>  
> -	*val = comp_press;
> -	*val2 = 256000;
> +	*comp_press = bmp280_compensate_press(data, adc_press);
>  
> -	return IIO_VAL_FRACTIONAL;
> +	return 0;
>  }
>  
> -static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> +static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
>  {
> -	u32 comp_humidity;
>  	s32 adc_humidity;
>  	int ret;
>  
>  	/* Read and compensate temperature so we get a reading of t_fine. */
> -	ret = bmp280_read_temp(data, NULL, NULL);
> +	ret = bmp280_read_temp(data, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -453,11 +440,10 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
>  		dev_err(data->dev, "reading humidity skipped\n");
>  		return -EIO;
>  	}
> -	comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
>  
> -	*val = comp_humidity * 1000 / 1024;
> +	*comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
>  
> -	return IIO_VAL_INT;
> +	return 0;
>  }
>  
>  static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> @@ -465,6 +451,8 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
>  				   int *val, int *val2, long mask)
>  {
>  	struct bmp280_data *data = iio_priv(indio_dev);
> +	int chan_value;
> +	int ret;
>  
>  	guard(mutex)(&data->lock);
>  
> @@ -472,11 +460,29 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_PROCESSED:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> -			return data->chip_info->read_humid(data, val, val2);
> +			ret = data->chip_info->read_humid(data, &chan_value);
> +			if (ret)
> +				return ret;
> +
> +			*val = data->chip_info->humid_coeffs[0] * chan_value;
> +			*val2 = data->chip_info->humid_coeffs[1];
> +			return data->chip_info->humid_coeffs_type;
>  		case IIO_PRESSURE:
> -			return data->chip_info->read_press(data, val, val2);
> +			ret = data->chip_info->read_press(data, &chan_value);
> +			if (ret)
> +				return ret;
> +
> +			*val = data->chip_info->press_coeffs[0] * chan_value;
> +			*val2 = data->chip_info->press_coeffs[1];
> +			return data->chip_info->press_coeffs_type;
>  		case IIO_TEMP:
> -			return data->chip_info->read_temp(data, val, val2);
> +			ret = data->chip_info->read_temp(data, &chan_value);
> +			if (ret)
> +				return ret;
> +
> +			*val = data->chip_info->temp_coeffs[0] * chan_value;
> +			*val2 = data->chip_info->temp_coeffs[1];
> +			return data->chip_info->temp_coeffs_type;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -787,6 +793,8 @@ static int bmp280_chip_config(struct bmp280_data *data)
>  
>  static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
>  static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID };
> +static const int bmp280_temp_coeffs[] = { 10, 1 };
> +static const int bmp280_press_coeffs[] = { 1, 256000 };
>  
>  const struct bmp280_chip_info bmp280_chip_info = {
>  	.id_reg = BMP280_REG_ID,
> @@ -815,6 +823,11 @@ const struct bmp280_chip_info bmp280_chip_info = {
>  	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
>  	.oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
>  
> +	.temp_coeffs = bmp280_temp_coeffs,
> +	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
> +	.press_coeffs = bmp280_press_coeffs,
> +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> +
>  	.chip_config = bmp280_chip_config,
>  	.read_temp = bmp280_read_temp,
>  	.read_press = bmp280_read_press,
> @@ -841,6 +854,7 @@ static int bme280_chip_config(struct bmp280_data *data)
>  }
>  
>  static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
> +static const int bme280_humid_coeffs[] = { 1000, 1024 };
>  
>  const struct bmp280_chip_info bme280_chip_info = {
>  	.id_reg = BMP280_REG_ID,
> @@ -863,6 +877,14 @@ const struct bmp280_chip_info bme280_chip_info = {
>  	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
>  	.oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
>  
> +	.temp_coeffs = bmp280_temp_coeffs,
> +	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
> +	.press_coeffs = bmp280_press_coeffs,
> +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> +	.humid_coeffs = bme280_humid_coeffs,
> +	.humid_coeffs_type = IIO_VAL_FRACTIONAL,
> +
> +
One blank line is almost always enough.

>  	.chip_config = bme280_chip_config,
>  	.read_temp = bmp280_read_temp,
>  	.read_press = bmp280_read_press,
> @@ -988,9 +1010,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press)
>  	return comp_press;
>  }
>  
> -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp)
>  {
> -	s32 comp_temp;
>  	u32 adc_temp;
>  	int ret;
>  
> @@ -1006,29 +1027,20 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
>  		dev_err(data->dev, "reading temperature skipped\n");
>  		return -EIO;
>  	}
> -	comp_temp = bmp380_compensate_temp(data, adc_temp);
>  
> -	/*
> -	 * Val might be NULL if we're called by the read_press routine,
> -	 * who only cares about the carry over t_fine value.
> -	 */
> -	if (val) {
> -		/* IIO reports temperatures in milli Celsius */
> -		*val = comp_temp * 10;
> -		return IIO_VAL_INT;
> -	}
> +	if (comp_temp)
> +		*comp_temp = bmp380_compensate_temp(data, adc_temp);
>  

I'm confused. If comp_temp is not provided then t_fine isn't updated
so this function isn't doing anything?

>  	return 0;
>  }
>  
> -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press)
>  {
> -	s32 comp_press;
>  	u32 adc_press;
>  	int ret;
>  
>  	/* Read and compensate for temperature so we get a reading of t_fine */

As above, I don't think it does. 

> -	ret = bmp380_read_temp(data, NULL, NULL);
> +	ret = bmp380_read_temp(data, NULL);
>  	if (ret)
>  		return ret;
>  
> @@ -1044,13 +1056,10 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
>  		dev_err(data->dev, "reading pressure skipped\n");
>  		return -EIO;
>  	}
> -	comp_press = bmp380_compensate_press(data, adc_press);
>  
> -	*val = comp_press;
> -	/* Compensated pressure is in cPa (centipascals) */
> -	*val2 = 100000;
> +	*comp_press = bmp380_compensate_press(data, adc_press);
>  
> -	return IIO_VAL_FRACTIONAL;
> +	return 0;
>  }
>  

J

Re: [PATCH v3 3/6] iio: pressure: Generalize read_{temp/press/humid}() functions

[Vasileios Amoiridis]

On Sun, Mar 24, 2024 at 11:36:16AM +0000, Jonathan Cameron wrote:
> On Tue, 19 Mar 2024 01:29:22 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Add the coefficients for the IIO standard units and the return
> > IIO value inside the chip_info structure.
> > 
> > Remove the calculations with the coefficients for the IIO unit
> > compatibility from inside the read_{temp/press/humid}() functions
> > and move it to the general read_raw() function.
> > 
> > Execute the calculations with the coefficients inside the read_raw()
> > oneshot capture function.
> > 
> > In this way, all the data for the calculation of the value are
> > located in the chip_info structure of the respective sensor.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> Hi,
> 
> Perhaps it's later in the series, but I still don't much like the hidden nature
> of t_fine. I'd much rather that was explicitly 'returned' by the function
> - by that I mean read_temp takes an s32 *t_fine and provides that if the pointer
> isn't NULL.
> 
> Then drop the cached value in bmp280_data which I think just serves to make
> this code less readable than it could be.
> 
> Then bmp280_compensate_pressure() can take a struct bmp280_calib, s32 adc_press and
> s32 t_fine so it just has the data it needs.
> Something similar for bmp280_compenstate_temp()
> 
> Obviously this is cleaning up stuff that's been there a long time, but
> given you are generalizing these functions this seems like the time to
> make these other changes.
> 
> As it stands, I don't think this code works as t_fine isn't updated
> everywhere it needs to be and that is hidden away by it being updated
> as a side effect of other calls.
> 
> Jonathan
> 

Hi Jonathan,

I am replying a bit late but I was off for quite some days.

In general the t_fine variable is calculated inside the bmpxxx_compensate_temp().
The t_fine variable is a function of the various varX variables and the adc_temp.
So by reading a new temp value from
the sensor and calculating its compensated value, the new t_fine variable is
calculated. So the combination of reading temp from sensor + compensating the
temp value = updated t_fine as a result of the compensated temperature. I agree that
this has a hidden nature. I can solve it by disintegrating the read()+compensate()
functions as follows:

bmpxxx_read_temp_adc(struct bmp280_data *data, s32 *adc_temp)
{
	/* reads adc temperature from the sensor */
}

bmpxxx_calc_t_fine(struct bmp280_calib *calib, s32 *adc_temp)
{
	/* calculate t_fine from adc_temp */
}

bmpxxx_get/update_t_fine(struct bmp280_data *data, ...)
{
	u32 adc_temp;
	s32 t_fine;

	bmpxxx_read_temp_adc(adc_temp); //get adc_temp
	if (ret)
		return ret;

	*t_fine = bmpxxx_calc_t_fine(&data->bmp280_calib.bmpxxx_calib, adc_temp);
}

bmpxxx_read_temp(struct bmp280_data *data, s32 *comp_temp)
{
	int ret;
	s32 t_fine;

	ret = bmpxxx_get/update_t_fine(&data, &t_fine);
	if (ret)
		return ret;

	*comp_temp = /* rest of the calculations to compensate temperature */

	return 0
}

Another question is, should this be applied to the pressure/humidity readings as 
well? Maybe, read_{humidity/press}_adc() functions could be introduced just to
have consistency with the temperature readings. Currently the adc_{humid/press}()
reading is done inside the read_{humid/press}() functions which already
incorporates the compensate_{humid/press}() functions.
 
> 
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 189 +++++++++++++++--------------
> >  drivers/iio/pressure/bmp280.h      |  13 +-
> >  2 files changed, 106 insertions(+), 96 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f7a13ff6f26c..6d6173c4b744 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -363,10 +363,9 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
> >  	return (u32)p;
> >  }
> >  
> > -static int bmp280_read_temp(struct bmp280_data *data,
> > -			    int *val, int *val2)
> > +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp)
> >  {
> > -	s32 adc_temp, comp_temp;
> > +	s32 adc_temp;
> >  	int ret;
> >  
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> > @@ -382,29 +381,20 @@ static int bmp280_read_temp(struct bmp280_data *data,
> >  		dev_err(data->dev, "reading temperature skipped\n");
> >  		return -EIO;
> >  	}
> > -	comp_temp = bmp280_compensate_temp(data, adc_temp);
> >  
> > -	/*
> > -	 * val might be NULL if we're called by the read_press routine,
> > -	 * who only cares about the carry over t_fine value.
> > -	 */
> > -	if (val) {
> > -		*val = comp_temp * 10;
> > -		return IIO_VAL_INT;
> > -	}
> > +	if (comp_temp)
> > +		*comp_temp = bmp280_compensate_temp(data, adc_temp);
> 
> As below, I don't think this is updating t_fine.
> Another reason to make that update very obvious rather than burried
> in this function call.
> 
> >  
> >  	return 0;
> >  }
> >  
> > -static int bmp280_read_press(struct bmp280_data *data,
> > -			     int *val, int *val2)
> > +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press)
> >  {
> > -	u32 comp_press;
> >  	s32 adc_press;
> >  	int ret;
> >  
> >  	/* Read and compensate temperature so we get a reading of t_fine. */
> > -	ret = bmp280_read_temp(data, NULL, NULL);
> > +	ret = bmp280_read_temp(data, NULL);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -421,22 +411,19 @@ static int bmp280_read_press(struct bmp280_data *data,
> >  		dev_err(data->dev, "reading pressure skipped\n");
> >  		return -EIO;
> >  	}
> > -	comp_press = bmp280_compensate_press(data, adc_press);
> >  
> > -	*val = comp_press;
> > -	*val2 = 256000;
> > +	*comp_press = bmp280_compensate_press(data, adc_press);
> >  
> > -	return IIO_VAL_FRACTIONAL;
> > +	return 0;
> >  }
> >  
> > -static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > +static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
> >  {
> > -	u32 comp_humidity;
> >  	s32 adc_humidity;
> >  	int ret;
> >  
> >  	/* Read and compensate temperature so we get a reading of t_fine. */
> > -	ret = bmp280_read_temp(data, NULL, NULL);
> > +	ret = bmp280_read_temp(data, NULL);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -453,11 +440,10 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> >  		dev_err(data->dev, "reading humidity skipped\n");
> >  		return -EIO;
> >  	}
> > -	comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
> >  
> > -	*val = comp_humidity * 1000 / 1024;
> > +	*comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
> >  
> > -	return IIO_VAL_INT;
> > +	return 0;
> >  }
> >  
> >  static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> > @@ -465,6 +451,8 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> >  				   int *val, int *val2, long mask)
> >  {
> >  	struct bmp280_data *data = iio_priv(indio_dev);
> > +	int chan_value;
> > +	int ret;
> >  
> >  	guard(mutex)(&data->lock);
> >  
> > @@ -472,11 +460,29 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_PROCESSED:
> >  		switch (chan->type) {
> >  		case IIO_HUMIDITYRELATIVE:
> > -			return data->chip_info->read_humid(data, val, val2);
> > +			ret = data->chip_info->read_humid(data, &chan_value);
> > +			if (ret)
> > +				return ret;
> > +
> > +			*val = data->chip_info->humid_coeffs[0] * chan_value;
> > +			*val2 = data->chip_info->humid_coeffs[1];
> > +			return data->chip_info->humid_coeffs_type;
> >  		case IIO_PRESSURE:
> > -			return data->chip_info->read_press(data, val, val2);
> > +			ret = data->chip_info->read_press(data, &chan_value);
> > +			if (ret)
> > +				return ret;
> > +
> > +			*val = data->chip_info->press_coeffs[0] * chan_value;
> > +			*val2 = data->chip_info->press_coeffs[1];
> > +			return data->chip_info->press_coeffs_type;
> >  		case IIO_TEMP:
> > -			return data->chip_info->read_temp(data, val, val2);
> > +			ret = data->chip_info->read_temp(data, &chan_value);
> > +			if (ret)
> > +				return ret;
> > +
> > +			*val = data->chip_info->temp_coeffs[0] * chan_value;
> > +			*val2 = data->chip_info->temp_coeffs[1];
> > +			return data->chip_info->temp_coeffs_type;
> >  		default:
> >  			return -EINVAL;
> >  		}
> > @@ -787,6 +793,8 @@ static int bmp280_chip_config(struct bmp280_data *data)
> >  
> >  static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
> >  static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID };
> > +static const int bmp280_temp_coeffs[] = { 10, 1 };
> > +static const int bmp280_press_coeffs[] = { 1, 256000 };
> >  
> >  const struct bmp280_chip_info bmp280_chip_info = {
> >  	.id_reg = BMP280_REG_ID,
> > @@ -815,6 +823,11 @@ const struct bmp280_chip_info bmp280_chip_info = {
> >  	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> >  	.oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
> >  
> > +	.temp_coeffs = bmp280_temp_coeffs,
> > +	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
> > +	.press_coeffs = bmp280_press_coeffs,
> > +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> > +
> >  	.chip_config = bmp280_chip_config,
> >  	.read_temp = bmp280_read_temp,
> >  	.read_press = bmp280_read_press,
> > @@ -841,6 +854,7 @@ static int bme280_chip_config(struct bmp280_data *data)
> >  }
> >  
> >  static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
> > +static const int bme280_humid_coeffs[] = { 1000, 1024 };
> >  
> >  const struct bmp280_chip_info bme280_chip_info = {
> >  	.id_reg = BMP280_REG_ID,
> > @@ -863,6 +877,14 @@ const struct bmp280_chip_info bme280_chip_info = {
> >  	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> >  	.oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
> >  
> > +	.temp_coeffs = bmp280_temp_coeffs,
> > +	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
> > +	.press_coeffs = bmp280_press_coeffs,
> > +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> > +	.humid_coeffs = bme280_humid_coeffs,
> > +	.humid_coeffs_type = IIO_VAL_FRACTIONAL,
> > +
> > +
> One blank line is almost always enough.
> 
> >  	.chip_config = bme280_chip_config,
> >  	.read_temp = bmp280_read_temp,
> >  	.read_press = bmp280_read_press,
> > @@ -988,9 +1010,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press)
> >  	return comp_press;
> >  }
> >  
> > -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> > +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp)
> >  {
> > -	s32 comp_temp;
> >  	u32 adc_temp;
> >  	int ret;
> >  
> > @@ -1006,29 +1027,20 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> >  		dev_err(data->dev, "reading temperature skipped\n");
> >  		return -EIO;
> >  	}
> > -	comp_temp = bmp380_compensate_temp(data, adc_temp);
> >  
> > -	/*
> > -	 * Val might be NULL if we're called by the read_press routine,
> > -	 * who only cares about the carry over t_fine value.
> > -	 */
> > -	if (val) {
> > -		/* IIO reports temperatures in milli Celsius */
> > -		*val = comp_temp * 10;
> > -		return IIO_VAL_INT;
> > -	}
> > +	if (comp_temp)
> > +		*comp_temp = bmp380_compensate_temp(data, adc_temp);
> >  
> 
> I'm confused. If comp_temp is not provided then t_fine isn't updated
> so this function isn't doing anything?
> 
> >  	return 0;
> >  }
> >  
> > -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> > +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press)
> >  {
> > -	s32 comp_press;
> >  	u32 adc_press;
> >  	int ret;
> >  
> >  	/* Read and compensate for temperature so we get a reading of t_fine */
> 
> As above, I don't think it does. 
> 
> > -	ret = bmp380_read_temp(data, NULL, NULL);
> > +	ret = bmp380_read_temp(data, NULL);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1044,13 +1056,10 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> >  		dev_err(data->dev, "reading pressure skipped\n");
> >  		return -EIO;
> >  	}
> > -	comp_press = bmp380_compensate_press(data, adc_press);
> >  
> > -	*val = comp_press;
> > -	/* Compensated pressure is in cPa (centipascals) */
> > -	*val2 = 100000;
> > +	*comp_press = bmp380_compensate_press(data, adc_press);
> >  
> > -	return IIO_VAL_FRACTIONAL;
> > +	return 0;
> >  }
> >  
> 
> J

Re: [PATCH v3 3/6] iio: pressure: Generalize read_{temp/press/humid}() functions

[Jonathan Cameron]

On Tue, 2 Apr 2024 19:55:53 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Sun, Mar 24, 2024 at 11:36:16AM +0000, Jonathan Cameron wrote:
> > On Tue, 19 Mar 2024 01:29:22 +0100
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> >   
> > > Add the coefficients for the IIO standard units and the return
> > > IIO value inside the chip_info structure.
> > > 
> > > Remove the calculations with the coefficients for the IIO unit
> > > compatibility from inside the read_{temp/press/humid}() functions
> > > and move it to the general read_raw() function.
> > > 
> > > Execute the calculations with the coefficients inside the read_raw()
> > > oneshot capture function.
> > > 
> > > In this way, all the data for the calculation of the value are
> > > located in the chip_info structure of the respective sensor.
> > > 
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > Hi,
> > 
> > Perhaps it's later in the series, but I still don't much like the hidden nature
> > of t_fine. I'd much rather that was explicitly 'returned' by the function
> > - by that I mean read_temp takes an s32 *t_fine and provides that if the pointer
> > isn't NULL.
> > 
> > Then drop the cached value in bmp280_data which I think just serves to make
> > this code less readable than it could be.
> > 
> > Then bmp280_compensate_pressure() can take a struct bmp280_calib, s32 adc_press and
> > s32 t_fine so it just has the data it needs.
> > Something similar for bmp280_compenstate_temp()
> > 
> > Obviously this is cleaning up stuff that's been there a long time, but
> > given you are generalizing these functions this seems like the time to
> > make these other changes.
> > 
> > As it stands, I don't think this code works as t_fine isn't updated
> > everywhere it needs to be and that is hidden away by it being updated
> > as a side effect of other calls.
> > 
> > Jonathan
> >   
> 
> Hi Jonathan,
> 
> I am replying a bit late but I was off for quite some days.
> 
> In general the t_fine variable is calculated inside the bmpxxx_compensate_temp().
> The t_fine variable is a function of the various varX variables and the adc_temp.
> So by reading a new temp value from
> the sensor and calculating its compensated value, the new t_fine variable is
> calculated. So the combination of reading temp from sensor + compensating the
> temp value = updated t_fine as a result of the compensated temperature. I agree that
> this has a hidden nature. I can solve it by disintegrating the read()+compensate()
> functions as follows:
> 
> bmpxxx_read_temp_adc(struct bmp280_data *data, s32 *adc_temp)
> {
> 	/* reads adc temperature from the sensor */
> }
> 
> bmpxxx_calc_t_fine(struct bmp280_calib *calib, s32 *adc_temp)
> {
> 	/* calculate t_fine from adc_temp */
> }
> 
> bmpxxx_get/update_t_fine(struct bmp280_data *data, ...)
> {
> 	u32 adc_temp;
> 	s32 t_fine;
> 
> 	bmpxxx_read_temp_adc(adc_temp); //get adc_temp
> 	if (ret)
> 		return ret;
> 
> 	*t_fine = bmpxxx_calc_t_fine(&data->bmp280_calib.bmpxxx_calib, adc_temp);
> }
> 
> bmpxxx_read_temp(struct bmp280_data *data, s32 *comp_temp)
> {
> 	int ret;
> 	s32 t_fine;
> 
> 	ret = bmpxxx_get/update_t_fine(&data, &t_fine);
> 	if (ret)
> 		return ret;
> 
> 	*comp_temp = /* rest of the calculations to compensate temperature */
> 
> 	return 0
> }
> 
> Another question is, should this be applied to the pressure/humidity readings as 
> well? Maybe, read_{humidity/press}_adc() functions could be introduced just to
> have consistency with the temperature readings. Currently the adc_{humid/press}()
> reading is done inside the read_{humid/press}() functions which already
> incorporates the compensate_{humid/press}() functions.

From a quick look there isn't the same issue with hidden data, but if it makes
sense from the point of view of consistency that is fine.
>  
> >   
> > > ---
> > >  drivers/iio/pressure/bmp280-core.c | 189 +++++++++++++++--------------
> > >  drivers/iio/pressure/bmp280.h      |  13 +-
> > >  2 files changed, 106 insertions(+), 96 deletions(-)
> > > 
> > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > > index f7a13ff6f26c..6d6173c4b744 100644
> > > --- a/drivers/iio/pressure/bmp280-core.c
> > > +++ b/drivers/iio/pressure/bmp280-core.c
> > > @@ -363,10 +363,9 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
> > >  	return (u32)p;
> > >  }
> > >  
> > > -static int bmp280_read_temp(struct bmp280_data *data,
> > > -			    int *val, int *val2)
> > > +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp)
> > >  {
> > > -	s32 adc_temp, comp_temp;
> > > +	s32 adc_temp;
> > >  	int ret;
> > >  
> > >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> > > @@ -382,29 +381,20 @@ static int bmp280_read_temp(struct bmp280_data *data,
> > >  		dev_err(data->dev, "reading temperature skipped\n");
> > >  		return -EIO;
> > >  	}
> > > -	comp_temp = bmp280_compensate_temp(data, adc_temp);
> > >  
> > > -	/*
> > > -	 * val might be NULL if we're called by the read_press routine,
> > > -	 * who only cares about the carry over t_fine value.
> > > -	 */
> > > -	if (val) {
> > > -		*val = comp_temp * 10;
> > > -		return IIO_VAL_INT;
> > > -	}
> > > +	if (comp_temp)
> > > +		*comp_temp = bmp280_compensate_temp(data, adc_temp);  
> > 
> > As below, I don't think this is updating t_fine.
> > Another reason to make that update very obvious rather than burried
> > in this function call.
> >   
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > -static int bmp280_read_press(struct bmp280_data *data,
> > > -			     int *val, int *val2)
> > > +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press)
> > >  {
> > > -	u32 comp_press;
> > >  	s32 adc_press;
> > >  	int ret;
> > >  
> > >  	/* Read and compensate temperature so we get a reading of t_fine. */
> > > -	ret = bmp280_read_temp(data, NULL, NULL);
> > > +	ret = bmp280_read_temp(data, NULL);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > @@ -421,22 +411,19 @@ static int bmp280_read_press(struct bmp280_data *data,
> > >  		dev_err(data->dev, "reading pressure skipped\n");
> > >  		return -EIO;
> > >  	}
> > > -	comp_press = bmp280_compensate_press(data, adc_press);
> > >  
> > > -	*val = comp_press;
> > > -	*val2 = 256000;
> > > +	*comp_press = bmp280_compensate_press(data, adc_press);
> > >  
> > > -	return IIO_VAL_FRACTIONAL;
> > > +	return 0;
> > >  }
> > >  
> > > -static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > > +static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
> > >  {
> > > -	u32 comp_humidity;
> > >  	s32 adc_humidity;
> > >  	int ret;
> > >  
> > >  	/* Read and compensate temperature so we get a reading of t_fine. */
> > > -	ret = bmp280_read_temp(data, NULL, NULL);
> > > +	ret = bmp280_read_temp(data, NULL);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > @@ -453,11 +440,10 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > >  		dev_err(data->dev, "reading humidity skipped\n");
> > >  		return -EIO;
> > >  	}
> > > -	comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
> > >  
> > > -	*val = comp_humidity * 1000 / 1024;
> > > +	*comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
> > >  
> > > -	return IIO_VAL_INT;
> > > +	return 0;
> > >  }
> > >  
> > >  static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> > > @@ -465,6 +451,8 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> > >  				   int *val, int *val2, long mask)
> > >  {
> > >  	struct bmp280_data *data = iio_priv(indio_dev);
> > > +	int chan_value;
> > > +	int ret;
> > >  
> > >  	guard(mutex)(&data->lock);
> > >  
> > > @@ -472,11 +460,29 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> > >  	case IIO_CHAN_INFO_PROCESSED:
> > >  		switch (chan->type) {
> > >  		case IIO_HUMIDITYRELATIVE:
> > > -			return data->chip_info->read_humid(data, val, val2);
> > > +			ret = data->chip_info->read_humid(data, &chan_value);
> > > +			if (ret)
> > > +				return ret;
> > > +
> > > +			*val = data->chip_info->humid_coeffs[0] * chan_value;
> > > +			*val2 = data->chip_info->humid_coeffs[1];
> > > +			return data->chip_info->humid_coeffs_type;
> > >  		case IIO_PRESSURE:
> > > -			return data->chip_info->read_press(data, val, val2);
> > > +			ret = data->chip_info->read_press(data, &chan_value);
> > > +			if (ret)
> > > +				return ret;
> > > +
> > > +			*val = data->chip_info->press_coeffs[0] * chan_value;
> > > +			*val2 = data->chip_info->press_coeffs[1];
> > > +			return data->chip_info->press_coeffs_type;
> > >  		case IIO_TEMP:
> > > -			return data->chip_info->read_temp(data, val, val2);
> > > +			ret = data->chip_info->read_temp(data, &chan_value);
> > > +			if (ret)
> > > +				return ret;
> > > +
> > > +			*val = data->chip_info->temp_coeffs[0] * chan_value;
> > > +			*val2 = data->chip_info->temp_coeffs[1];
> > > +			return data->chip_info->temp_coeffs_type;
> > >  		default:
> > >  			return -EINVAL;
> > >  		}
> > > @@ -787,6 +793,8 @@ static int bmp280_chip_config(struct bmp280_data *data)
> > >  
> > >  static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
> > >  static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID };
> > > +static const int bmp280_temp_coeffs[] = { 10, 1 };
> > > +static const int bmp280_press_coeffs[] = { 1, 256000 };
> > >  
> > >  const struct bmp280_chip_info bmp280_chip_info = {
> > >  	.id_reg = BMP280_REG_ID,
> > > @@ -815,6 +823,11 @@ const struct bmp280_chip_info bmp280_chip_info = {
> > >  	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> > >  	.oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
> > >  
> > > +	.temp_coeffs = bmp280_temp_coeffs,
> > > +	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
> > > +	.press_coeffs = bmp280_press_coeffs,
> > > +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> > > +
> > >  	.chip_config = bmp280_chip_config,
> > >  	.read_temp = bmp280_read_temp,
> > >  	.read_press = bmp280_read_press,
> > > @@ -841,6 +854,7 @@ static int bme280_chip_config(struct bmp280_data *data)
> > >  }
> > >  
> > >  static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
> > > +static const int bme280_humid_coeffs[] = { 1000, 1024 };
> > >  
> > >  const struct bmp280_chip_info bme280_chip_info = {
> > >  	.id_reg = BMP280_REG_ID,
> > > @@ -863,6 +877,14 @@ const struct bmp280_chip_info bme280_chip_info = {
> > >  	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> > >  	.oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
> > >  
> > > +	.temp_coeffs = bmp280_temp_coeffs,
> > > +	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
> > > +	.press_coeffs = bmp280_press_coeffs,
> > > +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> > > +	.humid_coeffs = bme280_humid_coeffs,
> > > +	.humid_coeffs_type = IIO_VAL_FRACTIONAL,
> > > +
> > > +  
> > One blank line is almost always enough.
> >   
> > >  	.chip_config = bme280_chip_config,
> > >  	.read_temp = bmp280_read_temp,
> > >  	.read_press = bmp280_read_press,
> > > @@ -988,9 +1010,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press)
> > >  	return comp_press;
> > >  }
> > >  
> > > -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> > > +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp)
> > >  {
> > > -	s32 comp_temp;
> > >  	u32 adc_temp;
> > >  	int ret;
> > >  
> > > @@ -1006,29 +1027,20 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> > >  		dev_err(data->dev, "reading temperature skipped\n");
> > >  		return -EIO;
> > >  	}
> > > -	comp_temp = bmp380_compensate_temp(data, adc_temp);
> > >  
> > > -	/*
> > > -	 * Val might be NULL if we're called by the read_press routine,
> > > -	 * who only cares about the carry over t_fine value.
> > > -	 */
> > > -	if (val) {
> > > -		/* IIO reports temperatures in milli Celsius */
> > > -		*val = comp_temp * 10;
> > > -		return IIO_VAL_INT;
> > > -	}
> > > +	if (comp_temp)
> > > +		*comp_temp = bmp380_compensate_temp(data, adc_temp);
> > >    
> > 
> > I'm confused. If comp_temp is not provided then t_fine isn't updated
> > so this function isn't doing anything?
> >   
> > >  	return 0;
> > >  }
> > >  
> > > -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> > > +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press)
> > >  {
> > > -	s32 comp_press;
> > >  	u32 adc_press;
> > >  	int ret;
> > >  
> > >  	/* Read and compensate for temperature so we get a reading of t_fine */  
> > 
> > As above, I don't think it does. 
> >   
> > > -	ret = bmp380_read_temp(data, NULL, NULL);
> > > +	ret = bmp380_read_temp(data, NULL);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > @@ -1044,13 +1056,10 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> > >  		dev_err(data->dev, "reading pressure skipped\n");
> > >  		return -EIO;
> > >  	}
> > > -	comp_press = bmp380_compensate_press(data, adc_press);
> > >  
> > > -	*val = comp_press;
> > > -	/* Compensated pressure is in cPa (centipascals) */
> > > -	*val2 = 100000;
> > > +	*comp_press = bmp380_compensate_press(data, adc_press);
> > >  
> > > -	return IIO_VAL_FRACTIONAL;
> > > +	return 0;
> > >  }
> > >    
> > 
> > J  

[PATCH v3 4/6] iio: pressure: Add SCALE and RAW values for channels

[Vasileios Amoiridis]

Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
able to calculate the processed value with standard userspace IIO
tools. Can be used for triggered buffers as well.

Even though it is not a good design choice to have SCALE, RAW and
PROCESSED together, the PROCESSED channel is kept for ABI compatibility.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 63 ++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6d6173c4b744..312bc2617583 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -137,17 +137,26 @@ enum {
 static const struct iio_chan_spec bmp280_channels[] = {
 	{
 		.type = IIO_PRESSURE,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 	{
 		.type = IIO_TEMP,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 	{
 		.type = IIO_HUMIDITYRELATIVE,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 };
@@ -155,21 +164,30 @@ static const struct iio_chan_spec bmp280_channels[] = {
 static const struct iio_chan_spec bmp380_channels[] = {
 	{
 		.type = IIO_PRESSURE,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
 	},
 	{
 		.type = IIO_TEMP,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
 	},
 	{
 		.type = IIO_HUMIDITYRELATIVE,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
@@ -487,6 +505,51 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 		return 0;
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_HUMIDITYRELATIVE:
+			ret = data->chip_info->read_humid(data, &chan_value);
+			if (ret)
+				return ret;
+
+			*val = chan_value;
+			return IIO_VAL_INT;
+		case IIO_PRESSURE:
+			ret = data->chip_info->read_press(data, &chan_value);
+			if (ret)
+				return ret;
+
+			*val = chan_value;
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			ret = data->chip_info->read_press(data, &chan_value);
+			if (ret)
+				return ret;
+
+			*val = chan_value;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+		return 0;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_HUMIDITYRELATIVE:
+			*val = data->chip_info->humid_coeffs[0];
+			*val2 = data->chip_info->humid_coeffs[1];
+			return data->chip_info->humid_coeffs_type;
+		case IIO_PRESSURE:
+			*val = data->chip_info->press_coeffs[0];
+			*val2 = data->chip_info->press_coeffs[1];
+			return data->chip_info->press_coeffs_type;
+		case IIO_TEMP:
+			*val = data->chip_info->temp_coeffs[0];
+			*val2 = data->chip_info->temp_coeffs[1];
+			return data->chip_info->temp_coeffs_type;
+		default:
+			return -EINVAL;
+		}
+		return 0;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
-- 
2.25.1

Re: [PATCH v3 4/6] iio: pressure: Add SCALE and RAW values for channels

[Andy Shevchenko]

On Tue, Mar 19, 2024 at 01:29:23AM +0100, Vasileios Amoiridis wrote:
> Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> able to calculate the processed value with standard userspace IIO
> tools. Can be used for triggered buffers as well.
> 
> Even though it is not a good design choice to have SCALE, RAW and
> PROCESSED together, the PROCESSED channel is kept for ABI compatibility.

...

> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_HUMIDITYRELATIVE:
> +			ret = data->chip_info->read_humid(data, &chan_value);
> +			if (ret)
> +				return ret;
> +
> +			*val = chan_value;
> +			return IIO_VAL_INT;
> +		case IIO_PRESSURE:
> +			ret = data->chip_info->read_press(data, &chan_value);
> +			if (ret)
> +				return ret;
> +
> +			*val = chan_value;
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			ret = data->chip_info->read_press(data, &chan_value);
> +			if (ret)
> +				return ret;
> +
> +			*val = chan_value;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}

> +		return 0;

Dead code.

> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_HUMIDITYRELATIVE:
> +			*val = data->chip_info->humid_coeffs[0];
> +			*val2 = data->chip_info->humid_coeffs[1];
> +			return data->chip_info->humid_coeffs_type;
> +		case IIO_PRESSURE:
> +			*val = data->chip_info->press_coeffs[0];
> +			*val2 = data->chip_info->press_coeffs[1];
> +			return data->chip_info->press_coeffs_type;
> +		case IIO_TEMP:
> +			*val = data->chip_info->temp_coeffs[0];
> +			*val2 = data->chip_info->temp_coeffs[1];
> +			return data->chip_info->temp_coeffs_type;
> +		default:
> +			return -EINVAL;
> +		}

> +		return 0;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver

[Vasileios Amoiridis]

The scan mask for the BME280 supports humidity measurement and
needs to be distinguished from the rest in order for the timestamp
to be able to work.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 111 +++++++++++++++++++++++++----
 drivers/iio/pressure/bmp280.h      |   1 +
 2 files changed, 98 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 312bc2617583..ddfcd23f29a0 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -134,7 +134,28 @@ enum {
 	BMP380_P11 = 20,
 };
 
+enum bmp280_scan {
+	BMP280_TEMP,
+	BMP280_PRESS,
+	BME280_HUMID
+};
+
 static const struct iio_chan_spec bmp280_channels[] = {
+	{
+		.type = IIO_TEMP,
+		/* PROCESSED maintained for ABI backwards compatibility */
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
+	},
 	{
 		.type = IIO_PRESSURE,
 		/* PROCESSED maintained for ABI backwards compatibility */
@@ -142,7 +163,18 @@ static const struct iio_chan_spec bmp280_channels[] = {
 				      BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+static const struct iio_chan_spec bme280_channels[] = {
 	{
 		.type = IIO_TEMP,
 		/* PROCESSED maintained for ABI backwards compatibility */
@@ -150,28 +182,48 @@ static const struct iio_chan_spec bmp280_channels[] = {
 				      BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
-		.type = IIO_HUMIDITYRELATIVE,
+		.type = IIO_PRESSURE,
 		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
-};
-
-static const struct iio_chan_spec bmp380_channels[] = {
 	{
-		.type = IIO_PRESSURE,
+		.type = IIO_HUMIDITYRELATIVE,
 		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
-		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
-					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+		.scan_index = 2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static const struct iio_chan_spec bmp380_channels[] = {
 	{
 		.type = IIO_TEMP,
 		/* PROCESSED maintained for ABI backwards compatibility */
@@ -181,9 +233,16 @@ static const struct iio_chan_spec bmp380_channels[] = {
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
-		.type = IIO_HUMIDITYRELATIVE,
+		.type = IIO_PRESSURE,
 		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_RAW) |
@@ -191,7 +250,15 @@ static const struct iio_chan_spec bmp380_channels[] = {
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
 };
 
 static int bmp280_read_calib(struct bmp280_data *data)
@@ -825,6 +892,16 @@ static const struct iio_info bmp280_info = {
 	.write_raw = &bmp280_write_raw,
 };
 
+static const unsigned long bmp280_avail_scan_masks[] = {
+	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+	0
+};
+
+static const unsigned long bme280_avail_scan_masks[] = {
+	BIT(BME280_HUMID) | BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+	0
+};
+
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
@@ -866,7 +943,8 @@ const struct bmp280_chip_info bmp280_chip_info = {
 	.regmap_config = &bmp280_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
-	.num_channels = 2,
+	.num_channels = 3,
+	.avail_scan_masks = bmp280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp280_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
@@ -925,8 +1003,9 @@ const struct bmp280_chip_info bme280_chip_info = {
 	.num_chip_id = ARRAY_SIZE(bme280_chip_ids),
 	.regmap_config = &bmp280_regmap_config,
 	.start_up_time = 2000,
-	.channels = bmp280_channels,
-	.num_channels = 3,
+	.channels = bme280_channels,
+	.num_channels = 4,
+	.avail_scan_masks = bme280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp280_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
@@ -1300,7 +1379,8 @@ const struct bmp280_chip_info bmp380_chip_info = {
 	.regmap_config = &bmp380_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
-	.num_channels = 2,
+	.num_channels = 3,
+	.avail_scan_masks = bmp280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp380_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp380_oversampling_avail),
@@ -1795,7 +1875,8 @@ const struct bmp280_chip_info bmp580_chip_info = {
 	.regmap_config = &bmp580_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
-	.num_channels = 2,
+	.num_channels = 3,
+	.avail_scan_masks = bmp280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp580_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail),
@@ -2054,7 +2135,8 @@ const struct bmp280_chip_info bmp180_chip_info = {
 	.regmap_config = &bmp180_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
-	.num_channels = 2,
+	.num_channels = 3,
+	.avail_scan_masks = bmp280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp180_oversampling_temp_avail,
 	.num_oversampling_temp_avail =
@@ -2166,6 +2248,7 @@ int bmp280_common_probe(struct device *dev,
 	/* Apply initial values from chip info structure */
 	indio_dev->channels = chip_info->channels;
 	indio_dev->num_channels = chip_info->num_channels;
+	indio_dev->available_scan_masks = chip_info->avail_scan_masks;
 	data->oversampling_press = chip_info->oversampling_press_default;
 	data->oversampling_humid = chip_info->oversampling_humid_default;
 	data->oversampling_temp = chip_info->oversampling_temp_default;
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 6d1dca31dd52..8cc3eed70c18 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -427,6 +427,7 @@ struct bmp280_chip_info {
 	const struct iio_chan_spec *channels;
 	int num_channels;
 	unsigned int start_up_time;
+	const unsigned long *avail_scan_masks;
 
 	const int *oversampling_temp_avail;
 	int num_oversampling_temp_avail;
-- 
2.25.1

Re: [PATCH v3 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver

[Andy Shevchenko]

On Tue, Mar 19, 2024 at 01:29:24AM +0100, Vasileios Amoiridis wrote:
> The scan mask for the BME280 supports humidity measurement and
> needs to be distinguished from the rest in order for the timestamp
> to be able to work.

...

> +enum bmp280_scan {
> +	BMP280_TEMP,
> +	BMP280_PRESS,
> +	BME280_HUMID

The last is not a terminator, please leave trailing comma.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver

[Vasileios Amoiridis]

On Wed, Mar 20, 2024 at 01:07:07PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 19, 2024 at 01:29:24AM +0100, Vasileios Amoiridis wrote:
> > The scan mask for the BME280 supports humidity measurement and
> > needs to be distinguished from the rest in order for the timestamp
> > to be able to work.
> 
> ...
> 
> > +enum bmp280_scan {
> > +	BMP280_TEMP,
> > +	BMP280_PRESS,
> > +	BME280_HUMID
> 
> The last is not a terminator, please leave trailing comma.
> 
> > +};
> 

What do you mean it is not a terminator? In general with the enum
variables I would write:

	enum var { a, b, c };

Why in this case there is a comma needed after the BME280_HUMID element?

Cheers,
Vasilis
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v3 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver

[Andy Shevchenko]

On Wed, Mar 20, 2024 at 07:45:16PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 20, 2024 at 01:07:07PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 19, 2024 at 01:29:24AM +0100, Vasileios Amoiridis wrote:

...

> > > +enum bmp280_scan {
> > > +	BMP280_TEMP,
> > > +	BMP280_PRESS,
> > > +	BME280_HUMID
> > 
> > The last is not a terminator, please leave trailing comma.
> > 
> > > +};
> 
> What do you mean it is not a terminator? In general with the enum
> variables I would write:
> 
> 	enum var { a, b, c };

This example is different to what you used. I.o.w. _this_ example is okay.

> Why in this case there is a comma needed after the BME280_HUMID element?

It's pure style issue that helps to avoid the unneeded churn in the future in
case the list is getting expanded. You can easily imagine what I mean.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver

[Vasileios Amoiridis]

On Wed, Mar 20, 2024 at 10:38:03PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 20, 2024 at 07:45:16PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Mar 20, 2024 at 01:07:07PM +0200, Andy Shevchenko wrote:
> > > On Tue, Mar 19, 2024 at 01:29:24AM +0100, Vasileios Amoiridis wrote:
> 
> ...
> 
> > > > +enum bmp280_scan {
> > > > +	BMP280_TEMP,
> > > > +	BMP280_PRESS,
> > > > +	BME280_HUMID
> > > 
> > > The last is not a terminator, please leave trailing comma.
> > > 
> > > > +};
> > 
> > What do you mean it is not a terminator? In general with the enum
> > variables I would write:
> > 
> > 	enum var { a, b, c };
> 
> This example is different to what you used. I.o.w. _this_ example is okay.
> 
> > Why in this case there is a comma needed after the BME280_HUMID element?
> 
> It's pure style issue that helps to avoid the unneeded churn in the future in
> case the list is getting expanded. You can easily imagine what I mean.
> 

Ok, that definitely makes sense, thank you! In general, should this be applied
to structs as well?

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v3 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver

[Andy Shevchenko]

On Wed, Mar 20, 2024 at 10:31:39PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 20, 2024 at 10:38:03PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 20, 2024 at 07:45:16PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 20, 2024 at 01:07:07PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Mar 19, 2024 at 01:29:24AM +0100, Vasileios Amoiridis wrote:

...

> > > > > +enum bmp280_scan {
> > > > > +	BMP280_TEMP,
> > > > > +	BMP280_PRESS,
> > > > > +	BME280_HUMID
> > > > 
> > > > The last is not a terminator, please leave trailing comma.
> > > > 
> > > > > +};
> > > 
> > > What do you mean it is not a terminator? In general with the enum
> > > variables I would write:
> > > 
> > > 	enum var { a, b, c };
> > 
> > This example is different to what you used. I.o.w. _this_ example is okay.
> > 
> > > Why in this case there is a comma needed after the BME280_HUMID element?
> > 
> > It's pure style issue that helps to avoid the unneeded churn in the future in
> > case the list is getting expanded. You can easily imagine what I mean.
> > 
> 
> Ok, that definitely makes sense, thank you! In general, should this be applied
> to structs as well?

Yes, to structs and/or arrays initializers when the list has a potential
expanding. We don't have trailing comma when:
1) it's a terminator entry (nothing must be after);
2) it's on the one line (as in your above example).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver

[Jonathan Cameron]

On Tue, 19 Mar 2024 01:29:24 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> The scan mask for the BME280 supports humidity measurement and
> needs to be distinguished from the rest in order for the timestamp
> to be able to work.

This needs a rewrite.  I read that as the measurement needed to be
distinguished from other measurements, not the device needs to be
distinguished from other devices.

It's also unusual to update the scan masks to add timestamps or
scan_index / scan_type before they are actually used.

Hence this first patch should just split the definitions but
not add the additional elements until the next patch that adds
buffered support (where these get used).

So resulting code is fine, but the patch breakup can be improved
so all the bits we need to look at for a given feature are
in a single patch (but not the refactoring).

Jonathan

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

[Vasileios Amoiridis]

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>
---
 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 (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))
+		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 */
+		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);
+	}
+
+	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 bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
 static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID };
 static const int bmp280_temp_coeffs[] = { 10, 1 };
@@ -973,6 +1043,8 @@ const struct bmp280_chip_info bmp280_chip_info = {
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
 	.read_calib = bmp280_read_calib,
+
+	.buffer_handler = bmp280_buffer_handler
 };
 EXPORT_SYMBOL_NS(bmp280_chip_info, IIO_BMP280);
 
@@ -1032,6 +1104,8 @@ const struct bmp280_chip_info bme280_chip_info = {
 	.read_press = bmp280_read_press,
 	.read_humid = bmp280_read_humid,
 	.read_calib = bme280_read_calib,
+
+	.buffer_handler = bmp280_buffer_handler
 };
 EXPORT_SYMBOL_NS(bme280_chip_info, IIO_BMP280);
 
@@ -1158,7 +1232,7 @@ static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp)
 	int ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB,
-			       data->buf, sizeof(data->buf));
+			       data->buf, BMP280_NUM_TEMP_BYTES);
 	if (ret) {
 		dev_err(data->dev, "failed to read temperature\n");
 		return ret;
@@ -1187,7 +1261,7 @@ static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press)
 		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
-			       data->buf, sizeof(data->buf));
+			       data->buf, BMP280_NUM_PRESS_BYTES);
 	if (ret) {
 		dev_err(data->dev, "failed to read pressure\n");
 		return ret;
@@ -1366,6 +1440,54 @@ static int bmp380_chip_config(struct bmp280_data *data)
 	return 0;
 }
 
+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);
+	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 const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 };
 static const int bmp380_iir_filter_coeffs_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128};
 static const u8 bmp380_chip_ids[] = { BMP380_CHIP_ID, BMP390_CHIP_ID };
@@ -1408,6 +1530,8 @@ const struct bmp280_chip_info bmp380_chip_info = {
 	.read_press = bmp380_read_press,
 	.read_calib = bmp380_read_calib,
 	.preinit = bmp380_preinit,
+
+	.buffer_handler = bmp380_buffer_handler
 };
 EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
 
@@ -1528,7 +1652,7 @@ static int bmp580_read_temp(struct bmp280_data *data, s32 *raw_temp)
 	int ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
-			       sizeof(data->buf));
+			       BMP280_NUM_TEMP_BYTES);
 	if (ret) {
 		dev_err(data->dev, "failed to read temperature\n");
 		return ret;
@@ -1548,7 +1672,7 @@ static int bmp580_read_press(struct bmp280_data *data, u32 *raw_press)
 	int ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
-			       sizeof(data->buf));
+			       BMP280_NUM_PRESS_BYTES);
 	if (ret) {
 		dev_err(data->dev, "failed to read pressure\n");
 		return ret;
@@ -1863,6 +1987,54 @@ static int bmp580_chip_config(struct bmp280_data *data)
 	return 0;
 }
 
+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);
+	if (adc_temp == BMP580_TEMP_SKIPPED) {
+		/* reading was skipped */
+		dev_err(data->dev, "reading temperature skipped\n");
+		return -EIO;
+	}
+	data->iio_buf.sensor_data[0] = adc_temp;
+
+	/* Pressure calculations */
+	memcpy(&chan_value, &data->buf[0], 3);
+
+	adc_press = get_unaligned_le24(&chan_value);
+	if (adc_press == BMP580_PRESS_SKIPPED) {
+		/* reading was skipped */
+		dev_err(data->dev, "reading pressure skipped\n");
+		return -EIO;
+	}
+	data->iio_buf.sensor_data[1] =  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 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);
+	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;
+}
+
 static const int bmp180_oversampling_temp_avail[] = { 1 };
 static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
 static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID };
@@ -2157,6 +2360,8 @@ const struct bmp280_chip_info bmp180_chip_info = {
 	.read_temp = bmp180_read_temp,
 	.read_press = bmp180_read_press,
 	.read_calib = bmp180_read_calib,
+
+	.buffer_handler = bmp180_buffer_handler
 };
 EXPORT_SYMBOL_NS(bmp180_chip_info, IIO_BMP280);
 
@@ -2332,6 +2537,14 @@ int bmp280_common_probe(struct device *dev,
 					     "failed to read calibration coefficients\n");
 	}
 
+	ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      data->chip_info->buffer_handler,
+					      NULL);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio triggered buffer setup failed\n");
+
 	/*
 	 * Attempt to grab an optional EOC IRQ - only the BMP085 has this
 	 * however as it happens, the BMP085 shares the chip ID of BMP180
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.
 	 */
-	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];
+		s64 timestamp __aligned(8);
+	} iio_buf;
+
 	/*
 	 * 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];
 		/* Calibration data buffers */
 		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
 		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
@@ -462,6 +472,8 @@ struct bmp280_chip_info {
 	int (*read_humid)(struct bmp280_data *, u32 *);
 	int (*read_calib)(struct bmp280_data *);
 	int (*preinit)(struct bmp280_data *);
+
+	irqreturn_t (*buffer_handler)(int, void *);
 };
 
 /* Chip infos for each variant */
-- 
2.25.1

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

[Andy Shevchenko]

On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis 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().

...

>  	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> -			       data->buf, sizeof(data->buf));
> +			       data->buf, BMP280_NUM_TEMP_BYTES);

>  	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> -			       data->buf, sizeof(data->buf));
> +			       data->buf, BMP280_NUM_PRESS_BYTES);

>  	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> -			       &data->be16, sizeof(data->be16));
> +			       &data->be16, BME280_NUM_HUMIDITY_BYTES);

> -	adc_humidity = be16_to_cpu(data->be16);
> +	adc_humidity = get_unaligned_be16(&data->be16);

>  	ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB,
> -			       data->buf, sizeof(data->buf));
> +			       data->buf, BMP280_NUM_TEMP_BYTES);

>  	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> -			       data->buf, sizeof(data->buf));
> +			       data->buf, BMP280_NUM_PRESS_BYTES);

>  	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
> -			       sizeof(data->buf));
> +			       BMP280_NUM_TEMP_BYTES);

>  	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
> -			       sizeof(data->buf));
> +			       BMP280_NUM_PRESS_BYTES);

These smell to me as candidates to a separate patch with more explanation why.
(Yes, with the definitions you introduced.) But I leave it to Jonathan to
decide if we need to split.

...

The below are applicable to the bmp280_buffer_handler(),
bmp380_buffer_handler() implementations as well.

...

> +	/* Burst read data registers */
> +	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB,
> +			       data->buf, 6);

Magic size.

...

> +	/* Temperature calculations */
> +	memcpy(&chan_value, &data->buf[3], 3);

_le24() + sign_extend32()?

...

> +	/* Pressure calculations */
> +	memcpy(&chan_value, &data->buf[0], 3);

_le24() + sign_extend32()?

...

>  	/*
> -	 * 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.
>  	 */
> -	if (val_size > 3)
> +	if (val_size > 8)

sizeof() / new definition for the buf[] size?

>  		return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

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

[Vasileios Amoiridis]

On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis 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().
> 
> ...
> 
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> > -			       data->buf, sizeof(data->buf));
> > +			       data->buf, BMP280_NUM_TEMP_BYTES);
> 
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> > -			       data->buf, sizeof(data->buf));
> > +			       data->buf, BMP280_NUM_PRESS_BYTES);
> 
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> > -			       &data->be16, sizeof(data->be16));
> > +			       &data->be16, BME280_NUM_HUMIDITY_BYTES);
> 
> > -	adc_humidity = be16_to_cpu(data->be16);
> > +	adc_humidity = get_unaligned_be16(&data->be16);
> 
> >  	ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB,
> > -			       data->buf, sizeof(data->buf));
> > +			       data->buf, BMP280_NUM_TEMP_BYTES);
> 
> >  	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> > -			       data->buf, sizeof(data->buf));
> > +			       data->buf, BMP280_NUM_PRESS_BYTES);
> 
> >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
> > -			       sizeof(data->buf));
> > +			       BMP280_NUM_TEMP_BYTES);
> 
> >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
> > -			       sizeof(data->buf));
> > +			       BMP280_NUM_PRESS_BYTES);
> 
> These smell to me as candidates to a separate patch with more explanation why.
> (Yes, with the definitions you introduced.) But I leave it to Jonathan to
> decide if we need to split.
> 
> ...
> 
> The below are applicable to the bmp280_buffer_handler(),
> bmp380_buffer_handler() implementations as well.
> 
> ...
> 
> > +	/* Burst read data registers */
> > +	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB,
> > +			       data->buf, 6);
> 
> Magic size.
> 

Hi Andy,

Thank you again for your feedback. When I was writing it, it was 
looking as a magic number to me as well but then I though that 
since I put the comment above it could be obvious. Now that I see 
it again, I think it was not a good idea and maybe some type of 
definition like

	#define BMP280_BURST_READ_NUM_BYTES 6
	#define BME280_BURST_READ_NUM_BYTES 8

could look better and be more intuitive.

> ...
> 
> > +	/* Temperature calculations */
> > +	memcpy(&chan_value, &data->buf[3], 3);
> 
> _le24() + sign_extend32()?
> 

In the next line from your comment the _le24 or _be24 takes place.
If the sign_extend32() is needed here, shouldn't it also be used
in all the oneshot captures as well?

> ...
> 
> > +	/* Pressure calculations */
> > +	memcpy(&chan_value, &data->buf[0], 3);
> 
> _le24() + sign_extend32()?
> 
> ...
> 
> >  	/*
> > -	 * 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.
> >  	 */
> > -	if (val_size > 3)
> > +	if (val_size > 8)
> 
> sizeof() / new definition for the buf[] size?
> 

In a previous commit that I was fixing this SPI driver, Jonathan had mentioned
that there is no need for a specific definition since it will only be used
here so that's why I kept it as is.

Cheers,
Vasilis
> >  		return -EINVAL;
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[Andy Shevchenko]

On Wed, Mar 20, 2024 at 06:46:02PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis wrote:

...

> > >  	/*
> > > -	 * 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.
> > >  	 */
> > > -	if (val_size > 3)
> > > +	if (val_size > 8)
> > 
> > sizeof() / new definition for the buf[] size?
> 
> In a previous commit that I was fixing this SPI driver, Jonathan had mentioned
> that there is no need for a specific definition since it will only be used
> here so that's why I kept it as is.

It seems not only here, but also in the buf[] definition in the struct.

-- 
With Best Regards,
Andy Shevchenko

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

[Vasileios Amoiridis]

On Wed, Mar 20, 2024 at 11:25:46PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 20, 2024 at 06:46:02PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote:
> > > On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis wrote:
> 
> ...
> 
> > > >  	/*
> > > > -	 * 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.
> > > >  	 */
> > > > -	if (val_size > 3)
> > > > +	if (val_size > 8)
> > > 
> > > sizeof() / new definition for the buf[] size?
> > 
> > In a previous commit that I was fixing this SPI driver, Jonathan had mentioned
> > that there is no need for a specific definition since it will only be used
> > here so that's why I kept it as is.
> 
> It seems not only here, but also in the buf[] definition in the struct.
> 

You are totally right, I didn't think of that!
It makes sense to use a definition.

Cheers,
Vasilis

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[Jonathan Cameron]

On Wed, 20 Mar 2024 18:46:02 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Wed, Mar 20, 2024 at 01:16:03PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 19, 2024 at 01:29:25AM +0100, Vasileios Amoiridis 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().  
> > 
> > ...
> >   
> > >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> > > -			       data->buf, sizeof(data->buf));
> > > +			       data->buf, BMP280_NUM_TEMP_BYTES);  
> >   
> > >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> > > -			       data->buf, sizeof(data->buf));
> > > +			       data->buf, BMP280_NUM_PRESS_BYTES);  
> >   
> > >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> > > -			       &data->be16, sizeof(data->be16));
> > > +			       &data->be16, BME280_NUM_HUMIDITY_BYTES);  
> >   
> > > -	adc_humidity = be16_to_cpu(data->be16);
> > > +	adc_humidity = get_unaligned_be16(&data->be16);  
> >   
> > >  	ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB,
> > > -			       data->buf, sizeof(data->buf));
> > > +			       data->buf, BMP280_NUM_TEMP_BYTES);  
> >   
> > >  	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> > > -			       data->buf, sizeof(data->buf));
> > > +			       data->buf, BMP280_NUM_PRESS_BYTES);  
> >   
> > >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
> > > -			       sizeof(data->buf));
> > > +			       BMP280_NUM_TEMP_BYTES);  
> >   
> > >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
> > > -			       sizeof(data->buf));
> > > +			       BMP280_NUM_PRESS_BYTES);  
> > 
> > These smell to me as candidates to a separate patch with more explanation why.
> > (Yes, with the definitions you introduced.) But I leave it to Jonathan to
> > decide if we need to split.

The are somewhat confusing, though only when you start doing bulk reads
to do these makes sense - so I'm not sure how to do it as 2 patches.

Pity we don't have sizeof(be24) available.

> > 
> > ...
> > 
> > The below are applicable to the bmp280_buffer_handler(),
> > bmp380_buffer_handler() implementations as well.
> > 
> > ...
> >   
> > > +	/* Burst read data registers */
> > > +	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB,
> > > +			       data->buf, 6);  
> > 
> > Magic size.
> >   
> 
> Hi Andy,
> 
> Thank you again for your feedback. When I was writing it, it was 
> looking as a magic number to me as well but then I though that 
> since I put the comment above it could be obvious. Now that I see 
> it again, I think it was not a good idea and maybe some type of 
> definition like
> 
> 	#define BMP280_BURST_READ_NUM_BYTES 6
> 	#define BME280_BURST_READ_NUM_BYTES 8
I think these are sums of the other quantities.  Better to
express them as such if possible.

> 
> could look better and be more intuitive.
> 
> > ...
> >   
> > > +	/* Temperature calculations */
> > > +	memcpy(&chan_value, &data->buf[3], 3);  
> > 
> > _le24() + sign_extend32()?
> >   
> 
> In the next line from your comment the _le24 or _be24 takes place.
I think you can get that data directly rather than bouncing it via
a memcpy.
e.g.
adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[3]);

> If the sign_extend32() is needed here, shouldn't it also be used
> in all the oneshot captures as well?

Is this actually signed?  Compensated values are, but at this point
it's just a raw adc count.  So I'm not seeing why we'd sign extend it
(unless the device is returning a signed be24 - I haven't checked.)


> 
> > ...
> >   
> > > +	/* Pressure calculations */
> > > +	memcpy(&chan_value, &data->buf[0], 3);  
> > 
> > _le24() + sign_extend32()?
> > 
> > ...
> >   
> > >  	/*
> > > -	 * 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.
> > >  	 */
> > > -	if (val_size > 3)
> > > +	if (val_size > 8)  
> > 
> > sizeof() / new definition for the buf[] size?
> >   
> 
> In a previous commit that I was fixing this SPI driver, Jonathan had mentioned
> that there is no need for a specific definition since it will only be used
> here so that's why I kept it as is.

Never trust me ;)  Size of the buf is sensible here.

Jonathan

> 
> Cheers,
> Vasilis
> > >  		return -EINVAL;  
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> >   

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

[Jonathan Cameron]

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.

> +		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 

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

[Vasileios Amoiridis]

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 
> 

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

[Jonathan Cameron]

> > >  
> > > +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!

A comment would do the job nicely. Thanks,

J