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

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

[Vasileios Amoiridis]

All the proposals were implemented, and 2 extra patches were added (Patch 2 and
patch 4) in order to have better logical split between patches.

Changes in v2:

Patch 1: Sorted and removed headers as per request.

Patch 2: *NEW* Patch, adds coefficients for IIO units in the chip_info
structure as per request, remove them from the read_* functions.

Patch 3: Patch 2 of v1. Added RAW values as well, as per request.

Patch 4: *NEW* Remove the temperature reading from inside read_* functions so
the addition of a buffer for userspace is facilitated + make the code much more 
intuitive.

Patch 5: Patch 3 of v1. No logical change, only minor typos as per request.

Patch 6: Patch 4 of v1. Previous commits allowed for much cleaner approach as
per request. Dropped filling of extra buffer in the read_* functions.
Patch 4 allows to put extra buffer in the union of the chip_info and fill
the buffer in the buffer_handler function.

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

Vasileios Amoiridis (6):
  iio: pressure: BMP280 core driver headers sorting
  iio: pressure: Simplify read_* functions
  iio: pressure: add SCALE and RAW values for channels
  iio: pressure: Simplify and make more clear temperature readings
  iio: pressure: Add timestamp and scan_masks for BM280 driver
  iio: pressure: Add triggered buffer support for BMP280 driver

 drivers/iio/pressure/Kconfig       |   2 +
 drivers/iio/pressure/bmp280-core.c | 431 +++++++++++++++++++++--------
 drivers/iio/pressure/bmp280.h      |  18 +-
 3 files changed, 326 insertions(+), 125 deletions(-)

-- 
2.25.1

[PATCH v2 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

[PATCH v2 2/6] iio: pressure: Simplify read_* functions

[Vasileios Amoiridis]

Add the coefficients for the IIO standard units inside the chip_info
structure.

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

Execute the calculations with the coefficients inside the read_raw
oneshot capture functions.

Also fix raw_* and comp_* values signs.

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

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 871b2214121b..dfd845acfa22 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -363,8 +363,7 @@ 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 s32 bmp280_read_temp(struct bmp280_data *data)
 {
 	s32 adc_temp, comp_temp;
 	int ret;
@@ -384,27 +383,17 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 	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;
-	}
-
-	return 0;
+	return comp_temp;
 }
 
-static int bmp280_read_press(struct bmp280_data *data,
-			     int *val, int *val2)
+static u32 bmp280_read_press(struct bmp280_data *data)
 {
 	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);
 	if (ret < 0)
 		return ret;
 
@@ -423,20 +412,17 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 	comp_press = bmp280_compensate_press(data, adc_press);
 
-	*val = comp_press;
-	*val2 = 256000;
-
-	return IIO_VAL_FRACTIONAL;
+	return comp_press;
 }
 
-static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
+static u32 bmp280_read_humid(struct bmp280_data *data)
 {
 	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);
 	if (ret < 0)
 		return ret;
 
@@ -455,9 +441,7 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 	}
 	comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
 
-	*val = comp_humidity * 1000 / 1024;
-
-	return IIO_VAL_INT;
+	return comp_humidity;
 }
 
 static int bmp280_read_raw(struct iio_dev *indio_dev,
@@ -474,13 +458,27 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
-			ret = data->chip_info->read_humid(data, val, val2);
+			ret = data->chip_info->read_humid(data);
+			*val = data->chip_info->humid_coeffs[0] * ret;
+			*val2 = data->chip_info->humid_coeffs[1];
+			ret = IIO_VAL_FRACTIONAL;
 			break;
 		case IIO_PRESSURE:
-			ret = data->chip_info->read_press(data, val, val2);
+			ret = data->chip_info->read_press(data);
+			*val = data->chip_info->press_coeffs[0] * ret;
+			*val2 = data->chip_info->press_coeffs[1];
+			ret = IIO_VAL_FRACTIONAL;
 			break;
 		case IIO_TEMP:
-			ret = data->chip_info->read_temp(data, val, val2);
+			ret = data->chip_info->read_temp(data);
+			*val = data->chip_info->temp_coeffs[0] * ret;
+			*val2 = data->chip_info->temp_coeffs[1];
+
+			if (!strcmp(indio_dev->name, "bmp580"))
+				ret = IIO_VAL_FRACTIONAL_LOG2;
+			else
+				ret = IIO_VAL_FRACTIONAL;
+
 			break;
 		default:
 			ret = -EINVAL;
@@ -796,6 +794,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,
@@ -824,6 +824,9 @@ 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,
+	.press_coeffs = bmp280_press_coeffs,
+
 	.chip_config = bmp280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
@@ -850,6 +853,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,
@@ -872,6 +876,10 @@ 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,
+	.press_coeffs = bmp280_press_coeffs,
+	.humid_coeffs = bme280_humid_coeffs,
+
 	.chip_config = bme280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
@@ -997,7 +1005,7 @@ 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 s32 bmp380_read_temp(struct bmp280_data *data)
 {
 	s32 comp_temp;
 	u32 adc_temp;
@@ -1017,27 +1025,17 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
 	}
 	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;
-	}
-
-	return 0;
+	return comp_temp;
 }
 
-static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
+static u32 bmp380_read_press(struct bmp280_data *data)
 {
-	s32 comp_press;
-	u32 adc_press;
+	u32 comp_press;
+	s32 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);
 	if (ret)
 		return ret;
 
@@ -1055,11 +1053,7 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
 	}
 	comp_press = bmp380_compensate_press(data, adc_press);
 
-	*val = comp_press;
-	/* Compensated pressure is in cPa (centipascals) */
-	*val2 = 100000;
-
-	return IIO_VAL_FRACTIONAL;
+	return comp_press;
 }
 
 static int bmp380_read_calib(struct bmp280_data *data)
@@ -1227,6 +1221,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,
@@ -1253,6 +1249,9 @@ 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,
+	.press_coeffs = bmp380_press_coeffs,
+
 	.chip_config = bmp380_chip_config,
 	.read_temp = bmp380_read_temp,
 	.read_press = bmp380_read_press,
@@ -1373,7 +1372,7 @@ 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 s32 bmp580_read_temp(struct bmp280_data *data)
 {
 	s32 raw_temp;
 	int ret;
@@ -1391,17 +1390,10 @@ static int bmp580_read_temp(struct bmp280_data *data, int *val, int *val2)
 		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 raw_temp;
 }
 
-static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
+static u32 bmp580_read_press(struct bmp280_data *data)
 {
 	u32 raw_press;
 	int ret;
@@ -1418,13 +1410,8 @@ static int bmp580_read_press(struct bmp280_data *data, int *val, int *val2)
 		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 raw_press;
 }
 
 static const int bmp580_odr_table[][2] = {
@@ -1729,6 +1716,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,
@@ -1755,6 +1744,9 @@ 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 = bmp580_temp_coeffs,
+	.press_coeffs = bmp580_press_coeffs,
+
 	.chip_config = bmp580_chip_config,
 	.read_temp = bmp580_read_temp,
 	.read_press = bmp580_read_press,
@@ -1882,7 +1874,7 @@ 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 s32 bmp180_read_temp(struct bmp280_data *data)
 {
 	s32 adc_temp, comp_temp;
 	int ret;
@@ -1893,16 +1885,7 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
 
 	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;
-	}
-
-	return 0;
+	return comp_temp;
 }
 
 static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
@@ -1962,15 +1945,14 @@ 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 u32 bmp180_read_press(struct bmp280_data *data)
 {
 	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);
 	if (ret)
 		return ret;
 
@@ -1980,10 +1962,7 @@ static int bmp180_read_press(struct bmp280_data *data,
 
 	comp_press = bmp180_compensate_press(data, adc_press);
 
-	*val = comp_press;
-	*val2 = 1000;
-
-	return IIO_VAL_FRACTIONAL;
+	return comp_press;
 }
 
 static int bmp180_chip_config(struct bmp280_data *data)
@@ -1994,6 +1973,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,
@@ -2014,6 +1995,9 @@ 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,
+	.press_coeffs = bmp180_press_coeffs,
+
 	.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..dde55b556ea2 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -448,10 +448,14 @@ struct bmp280_chip_info {
 	int num_sampling_freq_avail;
 	int sampling_freq_default;
 
+	const int *temp_coeffs;
+	const int *press_coeffs;
+	const int *humid_coeffs;
+
 	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 *);
+	s32 (*read_temp)(struct bmp280_data *);
+	u32 (*read_press)(struct bmp280_data *);
+	u32 (*read_humid)(struct bmp280_data *);
 	int (*read_calib)(struct bmp280_data *);
 	int (*preinit)(struct bmp280_data *);
 };
-- 
2.25.1

Re: [PATCH v2 2/6] iio: pressure: Simplify read_* functions

[Andy Shevchenko]

On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:

In the Subject: ... read_*() functions

> Add the coefficients for the IIO standard units inside the chip_info
> structure.
> 
> Remove the calculations with the coefficients for the IIO compatibility
> from inside the read_(temp/press/humid) functions and move it to the

read_{temp,press,humid}()

> read_raw function.

read_raw()

> Execute the calculations with the coefficients inside the read_raw

read_raw()

> oneshot capture functions.
> 
> Also fix raw_* and comp_* values signs.

...

>  		case IIO_TEMP:
> -			ret = data->chip_info->read_temp(data, val, val2);
> +			ret = data->chip_info->read_temp(data);
> +			*val = data->chip_info->temp_coeffs[0] * ret;
> +			*val2 = data->chip_info->temp_coeffs[1];

> +			if (!strcmp(indio_dev->name, "bmp580"))
> +				ret = IIO_VAL_FRACTIONAL_LOG2;
> +			else
> +				ret = IIO_VAL_FRACTIONAL;

I'm wondering if we may replace these strcmp():s by using enum and respective
values in chip_info.

>  			break;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] iio: pressure: Simplify read_* functions

[Vasileios Amoiridis]

On Wed, Mar 13, 2024 at 09:01:55PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:
> 
> In the Subject: ... read_*() functions
> 
> > Add the coefficients for the IIO standard units inside the chip_info
> > structure.
> > 
> > Remove the calculations with the coefficients for the IIO compatibility
> > from inside the read_(temp/press/humid) functions and move it to the
> 
> read_{temp,press,humid}()
> 
> > read_raw function.
> 
> read_raw()
> 
> > Execute the calculations with the coefficients inside the read_raw
> 
> read_raw()
> 
> > oneshot capture functions.
> > 
> > Also fix raw_* and comp_* values signs.
> 
Thank you very much for pointing these out, I should have thought it.

> ...
> 
> >  		case IIO_TEMP:
> > -			ret = data->chip_info->read_temp(data, val, val2);
> > +			ret = data->chip_info->read_temp(data);
> > +			*val = data->chip_info->temp_coeffs[0] * ret;
> > +			*val2 = data->chip_info->temp_coeffs[1];
> 
> > +			if (!strcmp(indio_dev->name, "bmp580"))
> > +				ret = IIO_VAL_FRACTIONAL_LOG2;
> > +			else
> > +				ret = IIO_VAL_FRACTIONAL;
> 
> I'm wondering if we may replace these strcmp():s by using enum and respective
> values in chip_info.
> 

The whole problem starts from the fact that all these BMPxxx_CHIP_ID defines are
not unique for the respective BMPxxx device. You mean to add a new variable
that could store some enum values that would be the actual chip_info IDs? Like:

enum chip_info_ids = {
	BMP085,
	BMP180,
	...
	BMP580,
};

and later for every chip_info struct to use it like this:

const struct bmp280_chip_info bmpxxx_chip_info = {
	...
	.chip_info_id = BIT(BMPxxx),
	...
}

And in the read_raw() function to just use the test_bit() function in the same
way that is done with the test_bit() and avail_scan_mask to test for the
enabled channels?

Best regards,
Vasilis Amoiridis 

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

Re: [PATCH v2 2/6] iio: pressure: Simplify read_* functions

[Andy Shevchenko]

On Wed, Mar 13, 2024 at 08:22:45PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 13, 2024 at 09:01:55PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:

...

> > >  		case IIO_TEMP:
> > > -			ret = data->chip_info->read_temp(data, val, val2);
> > > +			ret = data->chip_info->read_temp(data);
> > > +			*val = data->chip_info->temp_coeffs[0] * ret;
> > > +			*val2 = data->chip_info->temp_coeffs[1];
> > 
> > > +			if (!strcmp(indio_dev->name, "bmp580"))
> > > +				ret = IIO_VAL_FRACTIONAL_LOG2;
> > > +			else
> > > +				ret = IIO_VAL_FRACTIONAL;
> > 
> > I'm wondering if we may replace these strcmp():s by using enum and respective
> > values in chip_info.
> 
> The whole problem starts from the fact that all these BMPxxx_CHIP_ID defines are
> not unique for the respective BMPxxx device. You mean to add a new variable
> that could store some enum values that would be the actual chip_info IDs? Like:
> 
> enum chip_info_ids = {
> 	BMP085,
> 	BMP180,
> 	...
> 	BMP580,
> };
> 
> and later for every chip_info struct to use it like this:
> 
> const struct bmp280_chip_info bmpxxx_chip_info = {
> 	...
> 	.chip_info_id = BIT(BMPxxx),

No BIT(), but yes.

> 	...
> }
> 
> And in the read_raw() function to just use the test_bit() function in the same
> way that is done with the test_bit() and avail_scan_mask to test for the
> enabled channels?

If BIT() is more suitable, than also yes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] iio: pressure: Simplify read_* functions

[Jonathan Cameron]

On Wed, 13 Mar 2024 21:28:47 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Mar 13, 2024 at 08:22:45PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Mar 13, 2024 at 09:01:55PM +0200, Andy Shevchenko wrote:  
> > > On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:  
> 
> ...
> 
> > > >  		case IIO_TEMP:
> > > > -			ret = data->chip_info->read_temp(data, val, val2);
> > > > +			ret = data->chip_info->read_temp(data);
> > > > +			*val = data->chip_info->temp_coeffs[0] * ret;
> > > > +			*val2 = data->chip_info->temp_coeffs[1];  
> > >   
> > > > +			if (!strcmp(indio_dev->name, "bmp580"))
> > > > +				ret = IIO_VAL_FRACTIONAL_LOG2;
> > > > +			else
> > > > +				ret = IIO_VAL_FRACTIONAL;  
> > > 
> > > I'm wondering if we may replace these strcmp():s by using enum and respective
> > > values in chip_info.  
> > 
> > The whole problem starts from the fact that all these BMPxxx_CHIP_ID defines are
> > not unique for the respective BMPxxx device. You mean to add a new variable
> > that could store some enum values that would be the actual chip_info IDs? Like:
> > 
> > enum chip_info_ids = {
> > 	BMP085,
> > 	BMP180,
> > 	...
> > 	BMP580,
> > };
> > 
> > and later for every chip_info struct to use it like this:
> > 
> > const struct bmp280_chip_info bmpxxx_chip_info = {
> > 	...
> > 	.chip_info_id = BIT(BMPxxx),  
> 
> No BIT(), but yes.
> 
Better to use something more meaningful such as just storing the
type you need to return alongside the values it refers to.
temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2 / IIO_VAL_FRACTIONAL as appropriate.
That way the data and what it is are found in one simple place.

Basic rule of thumb is that if there is a string comparison to identify
what to do in a driver (other than deep in the fw handling code) then
that is a bad design. Likewise any matching on an enum value that correlates
with that chip ID.  I want to see all the difference between chips in one place,
not scattered through the code.

Jonathan


> > 	...
> > }
> > 
> > And in the read_raw() function to just use the test_bit() function in the same
> > way that is done with the test_bit() and avail_scan_mask to test for the
> > enabled channels?  
> 
> If BIT() is more suitable, than also yes.
> 

Re: [PATCH v2 2/6] iio: pressure: Simplify read_* functions

[Vasileios Amoiridis]

On Thu, Mar 14, 2024 at 02:32:31PM +0000, Jonathan Cameron wrote:
> On Wed, 13 Mar 2024 21:28:47 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Wed, Mar 13, 2024 at 08:22:45PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 09:01:55PM +0200, Andy Shevchenko wrote:  
> > > > On Wed, Mar 13, 2024 at 06:40:03PM +0100, Vasileios Amoiridis wrote:  
> > 
> > ...
> > 
> > > > >  		case IIO_TEMP:
> > > > > -			ret = data->chip_info->read_temp(data, val, val2);
> > > > > +			ret = data->chip_info->read_temp(data);
> > > > > +			*val = data->chip_info->temp_coeffs[0] * ret;
> > > > > +			*val2 = data->chip_info->temp_coeffs[1];  
> > > >   
> > > > > +			if (!strcmp(indio_dev->name, "bmp580"))
> > > > > +				ret = IIO_VAL_FRACTIONAL_LOG2;
> > > > > +			else
> > > > > +				ret = IIO_VAL_FRACTIONAL;  
> > > > 
> > > > I'm wondering if we may replace these strcmp():s by using enum and respective
> > > > values in chip_info.  
> > > 
> > > The whole problem starts from the fact that all these BMPxxx_CHIP_ID defines are
> > > not unique for the respective BMPxxx device. You mean to add a new variable
> > > that could store some enum values that would be the actual chip_info IDs? Like:
> > > 
> > > enum chip_info_ids = {
> > > 	BMP085,
> > > 	BMP180,
> > > 	...
> > > 	BMP580,
> > > };
> > > 
> > > and later for every chip_info struct to use it like this:
> > > 
> > > const struct bmp280_chip_info bmpxxx_chip_info = {
> > > 	...
> > > 	.chip_info_id = BIT(BMPxxx),  
> > 
> > No BIT(), but yes.
> > 
> Better to use something more meaningful such as just storing the
> type you need to return alongside the values it refers to.
> temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2 / IIO_VAL_FRACTIONAL as appropriate.
> That way the data and what it is are found in one simple place.
> 
> Basic rule of thumb is that if there is a string comparison to identify
> what to do in a driver (other than deep in the fw handling code) then
> that is a bad design. Likewise any matching on an enum value that correlates
> with that chip ID.  I want to see all the difference between chips in one place,
> not scattered through the code.
> 
> Jonathan
> 

Yes, sounds totally right. I was just hesitating to add new variables in the
chip_info structure (as you probably noticed in the next commits as well).

Best regards,
Vasilis
> 
> > > 	...
> > > }
> > > 
> > > And in the read_raw() function to just use the test_bit() function in the same
> > > way that is done with the test_bit() and avail_scan_mask to test for the
> > > enabled channels?  
> > 
> > If BIT() is more suitable, than also yes.
> > 
> 

Re: [PATCH v2 2/6] iio: pressure: Simplify read_* functions

[Jonathan Cameron]

On Wed, 13 Mar 2024 18:40:03 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Add the coefficients for the IIO standard units inside the chip_info
> structure.
> 
> Remove the calculations with the coefficients for the IIO compatibility
> from inside the read_(temp/press/humid) functions and move it to the
> read_raw function.
> 
> Execute the calculations with the coefficients inside the read_raw
> oneshot capture functions.
> 
> Also fix raw_* and comp_* values signs.

That needs some more explanation.  Is this a fix that needs backporting?

> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-core.c | 150 +++++++++++++----------------
>  drivers/iio/pressure/bmp280.h      |  10 +-
>  2 files changed, 74 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 871b2214121b..dfd845acfa22 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -363,8 +363,7 @@ 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 s32 bmp280_read_temp(struct bmp280_data *data)

It's returning error codes through here. If you need to force a specific type,
don't use the return code.  Pass in a variable to put it in.

static int bm280_read_temp(struct bmp280_data *data, u32 *result)


>  {
>  	s32 adc_temp, comp_temp;
>  	int ret;
> @@ -384,27 +383,17 @@ static int bmp280_read_temp(struct bmp280_data *data,
>  	}
>  	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;
> -	}
> -
> -	return 0;
> +	return comp_temp;
>  }
>  
> -static int bmp280_read_press(struct bmp280_data *data,
> -			     int *val, int *val2)
> +static u32 bmp280_read_press(struct bmp280_data *data)
>  {
>  	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);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -423,20 +412,17 @@ static int bmp280_read_press(struct bmp280_data *data,
>  	}
>  	comp_press = bmp280_compensate_press(data, adc_press);
>  
> -	*val = comp_press;
> -	*val2 = 256000;
> -
> -	return IIO_VAL_FRACTIONAL;
> +	return comp_press;
>  }
>  
> -static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> +static u32 bmp280_read_humid(struct bmp280_data *data)
>  {
>  	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);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -455,9 +441,7 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
>  	}
>  	comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
>  
> -	*val = comp_humidity * 1000 / 1024;
> -
> -	return IIO_VAL_INT;
> +	return comp_humidity;
>  }
>  
>  static int bmp280_read_raw(struct iio_dev *indio_dev,
> @@ -474,13 +458,27 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_PROCESSED:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> -			ret = data->chip_info->read_humid(data, val, val2);
> +			ret = data->chip_info->read_humid(data);

You can still get an error code back from this and you must handle it.
Previously it was fine because both errors and IIO_VAL_ were returned by
the callback so error returns were handled, now they aren't.

> +			*val = data->chip_info->humid_coeffs[0] * ret;
> +			*val2 = data->chip_info->humid_coeffs[1];
> +			ret = IIO_VAL_FRACTIONAL;
>  			break;
Same applies in all the other cases.

Jonathan

[PATCH v3 3/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.

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

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index dfd845acfa22..6d7734f867bc 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -138,16 +138,22 @@ static const struct iio_chan_spec bmp280_channels[] = {
 	{
 		.type = IIO_PRESSURE,
 		.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,
 		.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,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 };
@@ -156,6 +162,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
 	{
 		.type = IIO_PRESSURE,
 		.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),
@@ -163,6 +171,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.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),
@@ -170,6 +180,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
 	{
 		.type = IIO_HUMIDITYRELATIVE,
 		.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),
@@ -485,6 +497,52 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
 			break;
 		}
 		break;
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_HUMIDITYRELATIVE:
+			*val = data->chip_info->read_humid(data);
+			ret = IIO_VAL_INT;
+			break;
+		case IIO_PRESSURE:
+			*val = data->chip_info->read_press(data);
+			ret = IIO_VAL_INT;
+			break;
+		case IIO_TEMP:
+			*val = data->chip_info->read_temp(data);
+			ret = IIO_VAL_INT;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
+	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];
+			ret = IIO_VAL_FRACTIONAL;
+			break;
+		case IIO_PRESSURE:
+			*val = data->chip_info->press_coeffs[0];
+			*val2 = data->chip_info->press_coeffs[1];
+			ret = IIO_VAL_FRACTIONAL;
+			break;
+		case IIO_TEMP:
+			*val = data->chip_info->temp_coeffs[0];
+			*val2 = data->chip_info->temp_coeffs[1];
+
+			if (!strcmp(indio_dev->name, "bmp580"))
+				ret = IIO_VAL_FRACTIONAL_LOG2;
+			else
+				ret = IIO_VAL_FRACTIONAL;
+
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
-- 
2.25.1

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

[Andy Shevchenko]

On Wed, Mar 13, 2024 at 06:40:04PM +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.

...

> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_HUMIDITYRELATIVE:
> +			*val = data->chip_info->read_humid(data);
> +			ret = IIO_VAL_INT;
> +			break;
> +		case IIO_PRESSURE:
> +			*val = data->chip_info->read_press(data);
> +			ret = IIO_VAL_INT;
> +			break;
> +		case IIO_TEMP:
> +			*val = data->chip_info->read_temp(data);
> +			ret = IIO_VAL_INT;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;

Is it mutex that prevents us from returning here?
If so, perhaps switching to use cleanup.h first?

> +		}
> +		break;


-- 
With Best Regards,
Andy Shevchenko

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

[Vasileios Amoiridis]

On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 06:40:04PM +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.
> 
> ...
> 
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_HUMIDITYRELATIVE:
> > +			*val = data->chip_info->read_humid(data);
> > +			ret = IIO_VAL_INT;
> > +			break;
> > +		case IIO_PRESSURE:
> > +			*val = data->chip_info->read_press(data);
> > +			ret = IIO_VAL_INT;
> > +			break;
> > +		case IIO_TEMP:
> > +			*val = data->chip_info->read_temp(data);
> > +			ret = IIO_VAL_INT;
> > +			break;
> > +		default:
> > +			ret = -EINVAL;
> > +			break;
> 
> Is it mutex that prevents us from returning here?
> If so, perhaps switching to use cleanup.h first?
> 

I haven't seen cleanup.h used in any file and now that I searched,
only 5-6 are including it. I am currently thinking if the mutex
that already exists is really needed since most of the drivers
don't have it + I feel like this is something that should be done
by IIO, thus maybe it's not even needed here.

> > +		}
> > +		break;
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Best regards,
Vasileios Amoiridis

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

[Andy Shevchenko]

On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 13, 2024 at 06:40:04PM +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.

...

> > > +	case IIO_CHAN_INFO_RAW:
> > > +		switch (chan->type) {
> > > +		case IIO_HUMIDITYRELATIVE:
> > > +			*val = data->chip_info->read_humid(data);
> > > +			ret = IIO_VAL_INT;
> > > +			break;
> > > +		case IIO_PRESSURE:
> > > +			*val = data->chip_info->read_press(data);
> > > +			ret = IIO_VAL_INT;
> > > +			break;
> > > +		case IIO_TEMP:
> > > +			*val = data->chip_info->read_temp(data);
> > > +			ret = IIO_VAL_INT;
> > > +			break;
> > > +		default:
> > > +			ret = -EINVAL;
> > > +			break;
> > 
> > Is it mutex that prevents us from returning here?
> > If so, perhaps switching to use cleanup.h first?
> 
> I haven't seen cleanup.h used in any file and now that I searched,
> only 5-6 are including it.

Hmm... Which repository you are checking with?

$ git grep -lw cleanup.h -- drivers/ | wc -l
47

(Today's Linux Next)

> I am currently thinking if the mutex
> that already exists is really needed since most of the drivers
> don't have it + I feel like this is something that should be done
> by IIO, thus maybe it's not even needed here.

> > > +		}
> > > +		break;

-- 
With Best Regards,
Andy Shevchenko

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

[Vasileios Amoiridis]

On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 13, 2024 at 06:40:04PM +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.
> 
> ...
> 
> > > > +	case IIO_CHAN_INFO_RAW:
> > > > +		switch (chan->type) {
> > > > +		case IIO_HUMIDITYRELATIVE:
> > > > +			*val = data->chip_info->read_humid(data);
> > > > +			ret = IIO_VAL_INT;
> > > > +			break;
> > > > +		case IIO_PRESSURE:
> > > > +			*val = data->chip_info->read_press(data);
> > > > +			ret = IIO_VAL_INT;
> > > > +			break;
> > > > +		case IIO_TEMP:
> > > > +			*val = data->chip_info->read_temp(data);
> > > > +			ret = IIO_VAL_INT;
> > > > +			break;
> > > > +		default:
> > > > +			ret = -EINVAL;
> > > > +			break;
> > > 
> > > Is it mutex that prevents us from returning here?
> > > If so, perhaps switching to use cleanup.h first?
> > 
> > I haven't seen cleanup.h used in any file and now that I searched,
> > only 5-6 are including it.
> 
> Hmm... Which repository you are checking with?
> 
> $ git grep -lw cleanup.h -- drivers/ | wc -l
> 47
> 
> (Today's Linux Next)
>

I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
drivers that use it.

> > I am currently thinking if the mutex
> > that already exists is really needed since most of the drivers
> > don't have it + I feel like this is something that should be done
> > by IIO, thus maybe it's not even needed here.
> 
> > > > +		}
> > > > +		break;
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[vamoirid]

On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 13, 2024 at 06:40:04PM +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.
> > 
> > ...
> > 
> > > > > +	case IIO_CHAN_INFO_RAW:
> > > > > +		switch (chan->type) {
> > > > > +		case IIO_HUMIDITYRELATIVE:
> > > > > +			*val = data->chip_info->read_humid(data);
> > > > > +			ret = IIO_VAL_INT;
> > > > > +			break;
> > > > > +		case IIO_PRESSURE:
> > > > > +			*val = data->chip_info->read_press(data);
> > > > > +			ret = IIO_VAL_INT;
> > > > > +			break;
> > > > > +		case IIO_TEMP:
> > > > > +			*val = data->chip_info->read_temp(data);
> > > > > +			ret = IIO_VAL_INT;
> > > > > +			break;
> > > > > +		default:
> > > > > +			ret = -EINVAL;
> > > > > +			break;
> > > > 
> > > > Is it mutex that prevents us from returning here?
> > > > If so, perhaps switching to use cleanup.h first?
> > > 
> > > I haven't seen cleanup.h used in any file and now that I searched,
> > > only 5-6 are including it.
> > 
> > Hmm... Which repository you are checking with?
> > 
> > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > 47
> > 
> > (Today's Linux Next)
> >
> 
> I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> drivers that use it.
> 
> > > I am currently thinking if the mutex
> > > that already exists is really needed since most of the drivers
> > > don't have it + I feel like this is something that should be done
> > > by IIO, thus maybe it's not even needed here.
> >

After some researching today, I realized that all the                           
{read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
for channel mapping in the kernel and it looks like they are guarded by
the mutex_{un}lock(&iio_dev_opaque->info_exist_lock). So I feel that the
mutexes in the aforementioned functions can be dropped. When you have the
time please have a look, maybe the could be dropped.

In general, there is quite some cleaning that can be done in this driver
but is it wise to include it in the triggered buffer support series??? I
have noticed quite some things that could be improved but I am hesitating
to do it now in order to not "pollute" this series with many cleanups and
leave it for another cleanup series for example.

Best regards,
Vasilis Amoiridis

> > > > > +		}
> > > > > +		break;
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 

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

[Jonathan Cameron]

On Thu, 14 Mar 2024 11:57:28 +0100
vamoirid <vassilisamir@gmail.com> wrote:

> On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:  
> > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:  
> > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:  
> > > > > On Wed, Mar 13, 2024 at 06:40:04PM +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.  
> > > 
> > > ...
> > >   
> > > > > > +	case IIO_CHAN_INFO_RAW:
> > > > > > +		switch (chan->type) {
> > > > > > +		case IIO_HUMIDITYRELATIVE:
> > > > > > +			*val = data->chip_info->read_humid(data);
> > > > > > +			ret = IIO_VAL_INT;
> > > > > > +			break;
> > > > > > +		case IIO_PRESSURE:
> > > > > > +			*val = data->chip_info->read_press(data);
> > > > > > +			ret = IIO_VAL_INT;
> > > > > > +			break;
> > > > > > +		case IIO_TEMP:
> > > > > > +			*val = data->chip_info->read_temp(data);
> > > > > > +			ret = IIO_VAL_INT;
> > > > > > +			break;
> > > > > > +		default:
> > > > > > +			ret = -EINVAL;
> > > > > > +			break;  
> > > > > 
> > > > > Is it mutex that prevents us from returning here?
> > > > > If so, perhaps switching to use cleanup.h first?  
> > > > 
> > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > only 5-6 are including it.  
> > > 
> > > Hmm... Which repository you are checking with?
> > > 
> > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > 47
> > > 
> > > (Today's Linux Next)
> > >  
> > 
> > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > drivers that use it.

Yes - but that's because it's new - most of the stuff in 6.8 was the proof
points for the patches originally introducing support for autocleanup (so typically
one or two cases for each type of handling) That doesn't mean we don't want it
in drivers that are being worked upon if it gives a significant advantage.
Some features we need will merge shortly, and a great deal more usage
of this autocleanup will occur.

> >   
> > > > I am currently thinking if the mutex
> > > > that already exists is really needed since most of the drivers
> > > > don't have it + I feel like this is something that should be done
> > > > by IIO, thus maybe it's not even needed here.  
> > >  
> 
> After some researching today, I realized that all the                           
> {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> for channel mapping in the kernel and it looks like they are guarded by
> the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).

Why is that relevant to this patch which isn't using that interface at all?
Those protections are to ensure that a consumer driver doesn't access a removed
IIO device, not accesses directly from userspace.

>so I feel that the
> mutexes in the aforementioned functions can be dropped. When you have the
> time please have a look, maybe the could be dropped.

Identify what your locks are protecting.  Those existence locks have
very specific purpose and should not be relied on for anything else.

If this driver is protecting state known only to itself, then it must
be responsible for appropriate locking.

> 
> In general, there is quite some cleaning that can be done in this driver
> but is it wise to include it in the triggered buffer support series??? 

Generally if working on a driver and you see cleanup that you think should
be done, it belongs before any series adding new features, precisely because
that code can typically end up simpler as a result.  This sounds like one
of those cases.  Normally that only includes things that are directly related
to resulting code for new features (or applying the same cleanup across a driver)
as we don't want to make people do a full scrub of a driver before adding
anything as it will just create too much noise.

So for this case, it does look like a quick use of guard(mutex) in
a precursor patch will simplify what you add here - hence that's a reasonable
request for Andy to make.

Jonathan


> I
> have noticed quite some things that could be improved but I am hesitating
> to do it now in order to not "pollute" this series with many cleanups and
> leave it for another cleanup series for example.
> 
> Best regards,
> Vasilis Amoiridis
> 
> > > > > > +		}
> > > > > > +		break;  
> > > 
> > > -- 
> > > With Best Regards,
> > > Andy Shevchenko
> > > 
> > >   

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

[Vasileios Amoiridis]

On Thu, Mar 14, 2024 at 02:46:47PM +0000, Jonathan Cameron wrote:
> On Thu, 14 Mar 2024 11:57:28 +0100
> vamoirid <vassilisamir@gmail.com> wrote:
> 
> > On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:  
> > > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:  
> > > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:  
> > > > > > On Wed, Mar 13, 2024 at 06:40:04PM +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.  
> > > > 
> > > > ...
> > > >   
> > > > > > > +	case IIO_CHAN_INFO_RAW:
> > > > > > > +		switch (chan->type) {
> > > > > > > +		case IIO_HUMIDITYRELATIVE:
> > > > > > > +			*val = data->chip_info->read_humid(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		case IIO_PRESSURE:
> > > > > > > +			*val = data->chip_info->read_press(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		case IIO_TEMP:
> > > > > > > +			*val = data->chip_info->read_temp(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		default:
> > > > > > > +			ret = -EINVAL;
> > > > > > > +			break;  
> > > > > > 
> > > > > > Is it mutex that prevents us from returning here?
> > > > > > If so, perhaps switching to use cleanup.h first?  
> > > > > 
> > > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > > only 5-6 are including it.  
> > > > 
> > > > Hmm... Which repository you are checking with?
> > > > 
> > > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > > 47
> > > > 
> > > > (Today's Linux Next)
> > > >  
> > > 
> > > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > > drivers that use it.
> 
> Yes - but that's because it's new - most of the stuff in 6.8 was the proof
> points for the patches originally introducing support for autocleanup (so typically
> one or two cases for each type of handling) That doesn't mean we don't want it
> in drivers that are being worked upon if it gives a significant advantage.
> Some features we need will merge shortly, and a great deal more usage
> of this autocleanup will occur.
> 
> > >   
> > > > > I am currently thinking if the mutex
> > > > > that already exists is really needed since most of the drivers
> > > > > don't have it + I feel like this is something that should be done
> > > > > by IIO, thus maybe it's not even needed here.  
> > > >  
> > 
> > After some researching today, I realized that all the                           
> > {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> > for channel mapping in the kernel and it looks like they are guarded by
> > the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).
> 
> Why is that relevant to this patch which isn't using that interface at all?
> Those protections are to ensure that a consumer driver doesn't access a removed
> IIO device, not accesses directly from userspace.
> 
> >so I feel that the
> > mutexes in the aforementioned functions can be dropped. When you have the
> > time please have a look, maybe the could be dropped.
> 
> Identify what your locks are protecting.  Those existence locks have
> very specific purpose and should not be relied on for anything else.
> 
> If this driver is protecting state known only to itself, then it must
> be responsible for appropriate locking.
> 
> > 
> > In general, there is quite some cleaning that can be done in this driver
> > but is it wise to include it in the triggered buffer support series??? 
> 
> Generally if working on a driver and you see cleanup that you think should
> be done, it belongs before any series adding new features, precisely because
> that code can typically end up simpler as a result.  This sounds like one
> of those cases.  Normally that only includes things that are directly related
> to resulting code for new features (or applying the same cleanup across a driver)
> as we don't want to make people do a full scrub of a driver before adding
> anything as it will just create too much noise.
> 
> So for this case, it does look like a quick use of guard(mutex) in
> a precursor patch will simplify what you add here - hence that's a reasonable
> request for Andy to make.
> 
> Jonathan
> 

Hi Jonathan.

Thank you very much for the feedback once again. I didn't know that cleanup.h
was a new thing. I also didn't understand it when Andy mentioned it. Now that
I saw it better and I read about it, it certainly looks like a very good thing
to add.

I don't know if I sounded like I didn't like that request, but just to clarify,
I see it as a very good thing all the proposals that you do because first I
get to learn and understand how to write better code and second the users will
use a better driver! So please, the more requests, the better.

So a precursor patch adding the new functionality of the guard(mutex) in this
and possibly other places in the driver will be good indeed, thank you!

Best regards,
Vasilis
> 
> > I
> > have noticed quite some things that could be improved but I am hesitating
> > to do it now in order to not "pollute" this series with many cleanups and
> > leave it for another cleanup series for example.
> > 
> > Best regards,
> > Vasilis Amoiridis
> > 
> > > > > > > +		}
> > > > > > > +		break;  
> > > > 
> > > > -- 
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > > 
> > > >   
> 

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

[Vasileios Amoiridis]

On Thu, Mar 14, 2024 at 02:46:47PM +0000, Jonathan Cameron wrote:
> On Thu, 14 Mar 2024 11:57:28 +0100
> vamoirid <vassilisamir@gmail.com> wrote:
> 
> > On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:  
> > > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:  
> > > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:  
> > > > > > On Wed, Mar 13, 2024 at 06:40:04PM +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.  
> > > > 
> > > > ...
> > > >   
> > > > > > > +	case IIO_CHAN_INFO_RAW:
> > > > > > > +		switch (chan->type) {
> > > > > > > +		case IIO_HUMIDITYRELATIVE:
> > > > > > > +			*val = data->chip_info->read_humid(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		case IIO_PRESSURE:
> > > > > > > +			*val = data->chip_info->read_press(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		case IIO_TEMP:
> > > > > > > +			*val = data->chip_info->read_temp(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		default:
> > > > > > > +			ret = -EINVAL;
> > > > > > > +			break;  
> > > > > > 
> > > > > > Is it mutex that prevents us from returning here?
> > > > > > If so, perhaps switching to use cleanup.h first?  
> > > > > 
> > > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > > only 5-6 are including it.  
> > > > 
> > > > Hmm... Which repository you are checking with?
> > > > 
> > > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > > 47
> > > > 
> > > > (Today's Linux Next)
> > > >  
> > > 
> > > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > > drivers that use it.
> 
> Yes - but that's because it's new - most of the stuff in 6.8 was the proof
> points for the patches originally introducing support for autocleanup (so typically
> one or two cases for each type of handling) That doesn't mean we don't want it
> in drivers that are being worked upon if it gives a significant advantage.
> Some features we need will merge shortly, and a great deal more usage
> of this autocleanup will occur.
> 
> > >   
> > > > > I am currently thinking if the mutex
> > > > > that already exists is really needed since most of the drivers
> > > > > don't have it + I feel like this is something that should be done
> > > > > by IIO, thus maybe it's not even needed here.  
> > > >  
> > 
> > After some researching today, I realized that all the                           
> > {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> > for channel mapping in the kernel and it looks like they are guarded by
> > the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).
> 
> Why is that relevant to this patch which isn't using that interface at all?
> Those protections are to ensure that a consumer driver doesn't access a removed
> IIO device, not accesses directly from userspace.
> 
> >so I feel that the
> > mutexes in the aforementioned functions can be dropped. When you have the
> > time please have a look, maybe the could be dropped.
> 
> Identify what your locks are protecting.  Those existence locks have
> very specific purpose and should not be relied on for anything else.
> 
> If this driver is protecting state known only to itself, then it must
> be responsible for appropriate locking.
> 
> > 
> > In general, there is quite some cleaning that can be done in this driver
> > but is it wise to include it in the triggered buffer support series??? 
> 
> Generally if working on a driver and you see cleanup that you think should
> be done, it belongs before any series adding new features, precisely because
> that code can typically end up simpler as a result.  This sounds like one
> of those cases.  Normally that only includes things that are directly related
> to resulting code for new features (or applying the same cleanup across a driver)
> as we don't want to make people do a full scrub of a driver before adding
> anything as it will just create too much noise.
> 
> So for this case, it does look like a quick use of guard(mutex) in
> a precursor patch will simplify what you add here - hence that's a reasonable
> request for Andy to make.
> 
> Jonathan
> 

After looking into how to change the code to introduce the new guard(mutex)
I encountered the following situation.

In general, with the new guard(mutex) functinality you can remove most of the
goto statements and return immediately without doing the cleanup yourself. 
In the case of this driver, in the read_raw() call, apart from the mutex, 
the power management functions are also used. This means that in each case, 
before returning, the pm functions will need to be called, which I don't
know if it will actually make the code cleaner. Have a look below with
an example.

----- Current Implementation -----

static int bmp280_read_raw( ... )
{
	...

	pm_runtime_get_sync_data(data->dev);
	mutex_lock(&data->lock);

	switch (mask) {
	case 1:
		switch (channel) {
		case TEMP:
			ret = read_temp();
			break;
		case PRESS:
			ret = read_press();
			break;
		...
	case 2:
		switch (channel) {
		...

	case 3:
		...
	default:
		ret = -EINVAL;
		break;
	}

	mutex_unlock(&data->lock);
	pm_runtime_mark_last_busy(data->dev);
	pm_runtime_put_autosuspend(data->dev);

	return ret;
}

----- End of Current Implementation -----

With the use of the guard(mutex)(&data->lock) you could immediately
return without all the break statements. But we still need to call
the pm functions. So the code, as far as I can understand will look
like this:

----- Guard Mutex Implementation -----

static int bmp280_read_raw( ... )
{
	...

	pm_runtime_get_sync_data(data->dev);
	guard(mutex)(&data->lock);

	switch (mask) {
	case 1:
		switch (channel {
		case TEMP:
			ret = read_temp();
			pm_runtime_mark_last_busy(data->dev);
			pm_runtime_put_autosuspend(data->dev);
			return ret;
		case PRESS:
			ret = read_press();
			pm_runtime_mark_last_busy(data->dev);
			pm_runtime_put_autosuspend(data->dev);
			return ret;
		...
	case 2:
		switch (channel) {
		...
	case 3:
		...
	default:
		return -EINVAL;
	}

	return 0;
}

----- End of Guard Mutex Implementation -----

Have I completely misunderstood something? If what I explain
above is correct, you think that this is a better implementation
and I should move forward becasue we want to use the guard(mutex)
functionality? 

Maybe it is necessary to create some new type of guard to call
also the pm functions before exiting?

Cheers,
Vasilis

> 
> > I
> > have noticed quite some things that could be improved but I am hesitating
> > to do it now in order to not "pollute" this series with many cleanups and
> > leave it for another cleanup series for example.
> > 
> > Best regards,
> > Vasilis Amoiridis
> > 
> > > > > > > +		}
> > > > > > > +		break;  
> > > > 
> > > > -- 
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > > 
> > > >   
> 

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

[Jonathan Cameron]

On Fri, 15 Mar 2024 14:11:06 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Thu, Mar 14, 2024 at 02:46:47PM +0000, Jonathan Cameron wrote:
> > On Thu, 14 Mar 2024 11:57:28 +0100
> > vamoirid <vassilisamir@gmail.com> wrote:
> >   
> > > On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:  
> > > > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:    
> > > > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:    
> > > > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:    
> > > > > > > On Wed, Mar 13, 2024 at 06:40:04PM +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.    
> > > > > 
> > > > > ...
> > > > >     
> > > > > > > > +	case IIO_CHAN_INFO_RAW:
> > > > > > > > +		switch (chan->type) {
> > > > > > > > +		case IIO_HUMIDITYRELATIVE:
> > > > > > > > +			*val = data->chip_info->read_humid(data);
> > > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > > +			break;
> > > > > > > > +		case IIO_PRESSURE:
> > > > > > > > +			*val = data->chip_info->read_press(data);
> > > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > > +			break;
> > > > > > > > +		case IIO_TEMP:
> > > > > > > > +			*val = data->chip_info->read_temp(data);
> > > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > > +			break;
> > > > > > > > +		default:
> > > > > > > > +			ret = -EINVAL;
> > > > > > > > +			break;    
> > > > > > > 
> > > > > > > Is it mutex that prevents us from returning here?
> > > > > > > If so, perhaps switching to use cleanup.h first?    
> > > > > > 
> > > > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > > > only 5-6 are including it.    
> > > > > 
> > > > > Hmm... Which repository you are checking with?
> > > > > 
> > > > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > > > 47
> > > > > 
> > > > > (Today's Linux Next)
> > > > >    
> > > > 
> > > > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > > > drivers that use it.  
> > 
> > Yes - but that's because it's new - most of the stuff in 6.8 was the proof
> > points for the patches originally introducing support for autocleanup (so typically
> > one or two cases for each type of handling) That doesn't mean we don't want it
> > in drivers that are being worked upon if it gives a significant advantage.
> > Some features we need will merge shortly, and a great deal more usage
> > of this autocleanup will occur.
> >   
> > > >     
> > > > > > I am currently thinking if the mutex
> > > > > > that already exists is really needed since most of the drivers
> > > > > > don't have it + I feel like this is something that should be done
> > > > > > by IIO, thus maybe it's not even needed here.    
> > > > >    
> > > 
> > > After some researching today, I realized that all the                           
> > > {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> > > for channel mapping in the kernel and it looks like they are guarded by
> > > the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).  
> > 
> > Why is that relevant to this patch which isn't using that interface at all?
> > Those protections are to ensure that a consumer driver doesn't access a removed
> > IIO device, not accesses directly from userspace.
> >   
> > >so I feel that the
> > > mutexes in the aforementioned functions can be dropped. When you have the
> > > time please have a look, maybe the could be dropped.  
> > 
> > Identify what your locks are protecting.  Those existence locks have
> > very specific purpose and should not be relied on for anything else.
> > 
> > If this driver is protecting state known only to itself, then it must
> > be responsible for appropriate locking.
> >   
> > > 
> > > In general, there is quite some cleaning that can be done in this driver
> > > but is it wise to include it in the triggered buffer support series???   
> > 
> > Generally if working on a driver and you see cleanup that you think should
> > be done, it belongs before any series adding new features, precisely because
> > that code can typically end up simpler as a result.  This sounds like one
> > of those cases.  Normally that only includes things that are directly related
> > to resulting code for new features (or applying the same cleanup across a driver)
> > as we don't want to make people do a full scrub of a driver before adding
> > anything as it will just create too much noise.
> > 
> > So for this case, it does look like a quick use of guard(mutex) in
> > a precursor patch will simplify what you add here - hence that's a reasonable
> > request for Andy to make.
> > 
> > Jonathan
> >   
> 
> After looking into how to change the code to introduce the new guard(mutex)
> I encountered the following situation

No problem. We are all getting used to how to use this stuff. There
have been a few 'comments' from Linus Torvalds on people doing it wrong.
One of those (it was about cond_guard() proposal if you want to find it)
is applicable here (note that Linus was rather abrupt in that thread and
got called out for it, not in my view a good example of kernel process).

Make more use of helper functions to avoid gotos.

>
> In general, with the new guard(mutex) functinality you can remove most of the
> goto statements and return immediately without doing the cleanup yourself. 
> In the case of this driver, in the read_raw() call, apart from the mutex, 
> the power management functions are also used. This means that in each case, 
> before returning, the pm functions will need to be called, which I don't
> know if it will actually make the code cleaner. Have a look below with
> an example.
> 
> ----- Current Implementation -----
> 
> static int bmp280_read_raw( ... )
> {
> 	...
> 
> 	pm_runtime_get_sync_data(data->dev);

Pull from here...
> 	mutex_lock(&data->lock);
> 
> 	switch (mask) {
> 	case 1:
> 		switch (channel) {
> 		case TEMP:
> 			ret = read_temp();
> 			break;
> 		case PRESS:
> 			ret = read_press();
> 			break;
> 		...
> 	case 2:
> 		switch (channel) {
> 		...
> 
> 	case 3:
> 		...
> 	default:
> 		ret = -EINVAL;
> 		break;
> 	}
> 
> 	mutex_unlock(&data->lock);

... to here out as a separate little function - somethimg like
bmp280_read_raw_impl() which can use guard() and direct returns
internally.

Then this block will be

	pm_runtime_get_sync_data(data->dev);
	ret = bmp280_read_raw_impl(...);
	pm_runtime_mark_last_busy(data->dev);
	pm_runtime_put_autosuspend(data->dev);

	return ret;

> 	pm_runtime_mark_last_busy(data->dev);
> 	pm_runtime_put_autosuspend(data->dev);
> 
> 	return ret;
> }
> 
> ----- End of Current Implementation -----
> 
> With the use of the guard(mutex)(&data->lock) you could immediately
> return without all the break statements. But we still need to call
> the pm functions. So the code, as far as I can understand will look
> like this:
> 
> ----- Guard Mutex Implementation -----
> 
> static int bmp280_read_raw( ... )
> {
> 	...
> 
> 	pm_runtime_get_sync_data(data->dev);
> 	guard(mutex)(&data->lock);
> 
> 	switch (mask) {
> 	case 1:
> 		switch (channel {
> 		case TEMP:
> 			ret = read_temp();

This inverts ordering of pm and the guard mutex, so not a good idea.

> 			pm_runtime_mark_last_busy(data->dev);
> 			pm_runtime_put_autosuspend(data->dev);
> 			return ret;
> 		case PRESS:
> 			ret = read_press();
> 			pm_runtime_mark_last_busy(data->dev);
> 			pm_runtime_put_autosuspend(data->dev);
> 			return ret;
> 		...
> 	case 2:
> 		switch (channel) {
> 		...
> 	case 3:
> 		...
> 	default:
> 		return -EINVAL;
> 	}
> 
> 	return 0;
> }
> 
> ----- End of Guard Mutex Implementation -----
> 
> Have I completely misunderstood something? If what I explain
> above is correct, you think that this is a better implementation
> and I should move forward becasue we want to use the guard(mutex)
> functionality? 
> 
> Maybe it is necessary to create some new type of guard to call
> also the pm functions before exiting?

Don't try that approach - it's complexity that will get a response
you don't want from Linus.  Helper functions solve this one
for us nicely.

At somepoint maybe generic infrastructure for runtime pm handling
will be added, but that stuff is complex enough already so I suspect
not or not until people are in general much more confident with the
cleanup.h infrastructure and where it is appropriate.

What you are doing here should all be standard usage at the simpler
end of the scale, so not a risky as a few things I'm trying to get in :)

Jonathan

> 
> Cheers,
> Vasilis
> 
> >   
> > > I
> > > have noticed quite some things that could be improved but I am hesitating
> > > to do it now in order to not "pollute" this series with many cleanups and
> > > leave it for another cleanup series for example.
> > > 
> > > Best regards,
> > > Vasilis Amoiridis
> > >   
> > > > > > > > +		}
> > > > > > > > +		break;    
> > > > > 
> > > > > -- 
> > > > > With Best Regards,
> > > > > Andy Shevchenko
> > > > > 
> > > > >     
> >   
> 

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

[Jonathan Cameron]

On Wed, 13 Mar 2024 18:40:04 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> 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.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-core.c | 58 ++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index dfd845acfa22..6d7734f867bc 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -138,16 +138,22 @@ static const struct iio_chan_spec bmp280_channels[] = {
>  	{
>  		.type = IIO_PRESSURE,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |

Next to each (or at least the first) existing entry for PROCESSED add a
comment that says it is maintained for ABI backwards compatibility reasons.
I really don't want people copying the result of this patch into new drivers
- we've ended up here because of a less than ideal decision in the past, that
history doesn't apply to other drivers.

> +				      BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
>  				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>  	},
>  	{
>  		.type = IIO_TEMP,
>  		.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,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				      BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
>  				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>  	},
>  };
> @@ -156,6 +162,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
>  	{
>  		.type = IIO_PRESSURE,
>  		.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),
> @@ -163,6 +171,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
>  	{
>  		.type = IIO_TEMP,
>  		.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),
> @@ -170,6 +180,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
>  	{
>  		.type = IIO_HUMIDITYRELATIVE,
>  		.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),
> @@ -485,6 +497,52 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  			break;
>  		}
>  		break;
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_HUMIDITYRELATIVE:
> +			*val = data->chip_info->read_humid(data);
> +			ret = IIO_VAL_INT;
> +			break;
> +		case IIO_PRESSURE:
> +			*val = data->chip_info->read_press(data);
> +			ret = IIO_VAL_INT;
> +			break;
> +		case IIO_TEMP:
> +			*val = data->chip_info->read_temp(data);
> +			ret = IIO_VAL_INT;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +		break;
> +	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];
> +			ret = IIO_VAL_FRACTIONAL;
> +			break;
> +		case IIO_PRESSURE:
> +			*val = data->chip_info->press_coeffs[0];
> +			*val2 = data->chip_info->press_coeffs[1];
> +			ret = IIO_VAL_FRACTIONAL;
> +			break;
> +		case IIO_TEMP:
> +			*val = data->chip_info->temp_coeffs[0];
> +			*val2 = data->chip_info->temp_coeffs[1];
> +
> +			if (!strcmp(indio_dev->name, "bmp580"))
> +				ret = IIO_VAL_FRACTIONAL_LOG2;
> +			else
> +				ret = IIO_VAL_FRACTIONAL;
> +
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +		break;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:

[PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Vasileios Amoiridis]

The read_press/read_humid functions need the updated t_fine value
in order to calculate the current pressure/humidity. Temperature
reads should be removed from the read_press/read_humid functions
and should be placed in the oneshot captures before the pressure
and humidity reads. This makes the code more intuitive.

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

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 6d7734f867bc..377e90d9e5a2 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -404,11 +404,6 @@ static u32 bmp280_read_press(struct bmp280_data *data)
 	s32 adc_press;
 	int ret;
 
-	/* Read and compensate temperature so we get a reading of t_fine. */
-	ret = bmp280_read_temp(data);
-	if (ret < 0)
-		return ret;
-
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
 			       data->buf, sizeof(data->buf));
 	if (ret < 0) {
@@ -433,11 +428,6 @@ static u32 bmp280_read_humid(struct bmp280_data *data)
 	s32 adc_humidity;
 	int ret;
 
-	/* Read and compensate temperature so we get a reading of t_fine. */
-	ret = bmp280_read_temp(data);
-	if (ret < 0)
-		return ret;
-
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
 			       &data->be16, sizeof(data->be16));
 	if (ret < 0) {
@@ -470,12 +460,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
+			/* Read temperature to update the t_fine value */
+			data->chip_info->read_temp(data);
 			ret = data->chip_info->read_humid(data);
 			*val = data->chip_info->humid_coeffs[0] * ret;
 			*val2 = data->chip_info->humid_coeffs[1];
 			ret = IIO_VAL_FRACTIONAL;
 			break;
 		case IIO_PRESSURE:
+			/*
+			 * Read temperature to update the t_fine value.
+			 * BMP5xx devices do this in hardware, so skip it.
+			 */
+			if (strcmp(indio_dev->name, "bmp580"))
+				data->chip_info->read_temp(data);
+
 			ret = data->chip_info->read_press(data);
 			*val = data->chip_info->press_coeffs[0] * ret;
 			*val2 = data->chip_info->press_coeffs[1];
@@ -500,10 +499,19 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
+			/* Read temperature to update the t_fine value */
+			data->chip_info->read_temp(data);
 			*val = data->chip_info->read_humid(data);
 			ret = IIO_VAL_INT;
 			break;
 		case IIO_PRESSURE:
+			/*
+			 * Read temperature to update the t_fine value.
+			 * BMP5xx devices do this in hardware, so skip it.
+			 */
+			if (strcmp(indio_dev->name, "bmp580"))
+				data->chip_info->read_temp(data);
+
 			*val = data->chip_info->read_press(data);
 			ret = IIO_VAL_INT;
 			break;
@@ -1092,11 +1100,6 @@ static u32 bmp380_read_press(struct bmp280_data *data)
 	s32 adc_press;
 	int ret;
 
-	/* Read and compensate for temperature so we get a reading of t_fine */
-	ret = bmp380_read_temp(data);
-	if (ret)
-		return ret;
-
 	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
 			       data->buf, sizeof(data->buf));
 	if (ret) {
@@ -2009,11 +2012,6 @@ static u32 bmp180_read_press(struct bmp280_data *data)
 	s32 adc_press;
 	int ret;
 
-	/* Read and compensate temperature so we get a reading of t_fine. */
-	ret = bmp180_read_temp(data);
-	if (ret)
-		return ret;
-
 	ret = bmp180_read_adc_press(data, &adc_press);
 	if (ret)
 		return ret;
-- 
2.25.1

Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Andy Shevchenko]

On Wed, Mar 13, 2024 at 06:40:05PM +0100, Vasileios Amoiridis wrote:
> The read_press/read_humid functions need the updated t_fine value

read_press()/read_humid()


> in order to calculate the current pressure/humidity. Temperature
> reads should be removed from the read_press/read_humid functions

read_press()/read_humid()

> and should be placed in the oneshot captures before the pressure
> and humidity reads. This makes the code more intuitive.

...

> +			if (strcmp(indio_dev->name, "bmp580"))
> +				data->chip_info->read_temp(data);
> +

> +			if (strcmp(indio_dev->name, "bmp580"))
> +				data->chip_info->read_temp(data);

Yeah, not a fan of the strcmp().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Jonathan Cameron]

On Wed, 13 Mar 2024 21:04:46 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Mar 13, 2024 at 06:40:05PM +0100, Vasileios Amoiridis wrote:
> > The read_press/read_humid functions need the updated t_fine value  
> 
> read_press()/read_humid()
> 
> 
> > in order to calculate the current pressure/humidity. Temperature
> > reads should be removed from the read_press/read_humid functions  
> 
> read_press()/read_humid()
> 
> > and should be placed in the oneshot captures before the pressure
> > and humidity reads. This makes the code more intuitive.  
> 
> ...
> 
> > +			if (strcmp(indio_dev->name, "bmp580"))
> > +				data->chip_info->read_temp(data);
> > +  
> 
> > +			if (strcmp(indio_dev->name, "bmp580"))
> > +				data->chip_info->read_temp(data);  
> 
> Yeah, not a fan of the strcmp().
> 
Yes - that's a non starter.  Add a flag to say it is necessary to
chip_info that doesn't rely on name matching.  If you do it the way
you have here, you have to add another strcmp for each new device
supported that needs this code to run.  Add a flag and you just
set that in the chip_info structure.  Much more flexible and extensible.
Usual description of this is "when you can do things with data rather than
code, do it with data".

Jonathan

Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Jonathan Cameron]

On Wed, 13 Mar 2024 18:40:05 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> The read_press/read_humid functions need the updated t_fine value
> in order to calculate the current pressure/humidity. Temperature
> reads should be removed from the read_press/read_humid functions
> and should be placed in the oneshot captures before the pressure
> and humidity reads. This makes the code more intuitive.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>

To me this makes the use of these calls less obvious than they were
previously.  The calls are made close to where t_fine is used and
don't have to go via the indirection of chip_info.

So I disagree. I think this change makes the code a lot less
clear.

The only improvement I can readily see would be to move the
temperature read into the compensation functions themselves, possibly
removing t_fine from data and having a function that reads everything
relevant to computing it directly but doesn't do the maths to get
a temperature reading.  That can be reused in bmp280_compensate_temp()

Something along lines of.

static s32 bmp280_calc_tfine(struct bmp280_calib *calib, s32 adc_temp) 
{
	s32 var1, var2;

	var1 = (((adc_temp >> 3) - ((s32)calib->T1 << 1)) *
		((s32)calib->T2)) >> 11;
	var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) *
		  ((adc_temp >> 4) - ((s32)calib->T1))) >> 12) *
		((s32)calib->T3)) >> 14;
	return var1 + var2;
}

static int bmp280_read_temp_raw(struct bmp280_data *data,
			    	s32 *raw)
{
	s32 adc_temp;
	int ret;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
			       data->buf, sizeof(data->buf));
	if (ret < 0) {
		dev_err(data->dev, "failed to read temperature\n");
		return ret;
	}

	adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf));
	if (adc_temp == BMP280_TEMP_SKIPPED) {
		/* reading was skipped */
		dev_err(data->dev, "reading temperature skipped\n");
		return -EIO;
	}
	*raw = adc_temp;

	return 0;
}
static int bmp280_get_t_fine(.., s32 *t_fine)
{
	s32 adc_temp, comp_temp;
	s32 t_fine;
	int ret;

	ret = bmp280_read_temp_raw(data, &adc_temp;
	if (ret)
		return ret;

	*t_fine = bmp280_calc_tfine(&data->calib.bmp280, adc_temp);
	return 0;
}

static int bmp280_read_temp(struct bmp280_data *data, s32 *temp)
{
	int ret;
	s32 t_fine;

	ret = bmp280_get_t_fine(data, &t_fine);
	if (ret)
		return ret;

	*temp = (t_fine * 5 + 128) / 256;
//division rather than shift as then it's obvious what the 128 is there for
	return 0;
}

Now you have a nice function to get you t_fine which is all you want in some
of these paths.  Call it directly where it is needed instead of as
a side effect of a temperature read.



> ---
>  drivers/iio/pressure/bmp280-core.c | 38 ++++++++++++++----------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 6d7734f867bc..377e90d9e5a2 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -404,11 +404,6 @@ static u32 bmp280_read_press(struct bmp280_data *data)
>  	s32 adc_press;
>  	int ret;
>  
> -	/* Read and compensate temperature so we get a reading of t_fine. */
> -	ret = bmp280_read_temp(data);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
>  			       data->buf, sizeof(data->buf));
>  	if (ret < 0) {
> @@ -433,11 +428,6 @@ static u32 bmp280_read_humid(struct bmp280_data *data)
>  	s32 adc_humidity;
>  	int ret;
>  
> -	/* Read and compensate temperature so we get a reading of t_fine. */
> -	ret = bmp280_read_temp(data);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
>  			       &data->be16, sizeof(data->be16));
>  	if (ret < 0) {
> @@ -470,12 +460,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_PROCESSED:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> +			/* Read temperature to update the t_fine value */
> +			data->chip_info->read_temp(data);
>  			ret = data->chip_info->read_humid(data);
>  			*val = data->chip_info->humid_coeffs[0] * ret;
>  			*val2 = data->chip_info->humid_coeffs[1];
>  			ret = IIO_VAL_FRACTIONAL;
>  			break;
>  		case IIO_PRESSURE:
> +			/*
> +			 * Read temperature to update the t_fine value.
> +			 * BMP5xx devices do this in hardware, so skip it.
> +			 */
> +			if (strcmp(indio_dev->name, "bmp580"))
> +				data->chip_info->read_temp(data);
> +
>  			ret = data->chip_info->read_press(data);
>  			*val = data->chip_info->press_coeffs[0] * ret;
>  			*val2 = data->chip_info->press_coeffs[1];
> @@ -500,10 +499,19 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> +			/* Read temperature to update the t_fine value */
> +			data->chip_info->read_temp(data);
>  			*val = data->chip_info->read_humid(data);
>  			ret = IIO_VAL_INT;
>  			break;
>  		case IIO_PRESSURE:
> +			/*
> +			 * Read temperature to update the t_fine value.
> +			 * BMP5xx devices do this in hardware, so skip it.
> +			 */
> +			if (strcmp(indio_dev->name, "bmp580"))
> +				data->chip_info->read_temp(data);
> +
>  			*val = data->chip_info->read_press(data);
>  			ret = IIO_VAL_INT;
>  			break;
> @@ -1092,11 +1100,6 @@ static u32 bmp380_read_press(struct bmp280_data *data)
>  	s32 adc_press;
>  	int ret;
>  
> -	/* Read and compensate for temperature so we get a reading of t_fine */
> -	ret = bmp380_read_temp(data);
> -	if (ret)
> -		return ret;
> -
>  	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
>  			       data->buf, sizeof(data->buf));
>  	if (ret) {
> @@ -2009,11 +2012,6 @@ static u32 bmp180_read_press(struct bmp280_data *data)
>  	s32 adc_press;
>  	int ret;
>  
> -	/* Read and compensate temperature so we get a reading of t_fine. */
> -	ret = bmp180_read_temp(data);
> -	if (ret)
> -		return ret;
> -
>  	ret = bmp180_read_adc_press(data, &adc_press);
>  	if (ret)
>  		return ret;

Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Vasileios Amoiridis]

On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> On Wed, 13 Mar 2024 18:40:05 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The read_press/read_humid functions need the updated t_fine value
> > in order to calculate the current pressure/humidity. Temperature
> > reads should be removed from the read_press/read_humid functions
> > and should be placed in the oneshot captures before the pressure
> > and humidity reads. This makes the code more intuitive.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> To me this makes the use of these calls less obvious than they were
> previously.  The calls are made close to where t_fine is used and
> don't have to go via the indirection of chip_info.
> 
> So I disagree. I think this change makes the code a lot less
> clear.
> 

This was mainly driven by the fact that I wanted to avoid reading
the temperature 3 times in case temp, press and humid are enabled
and there are consecutive buffer readings. But thank you for the
proposal I really appreciate it!

Best regards,
Vasilis

> The only improvement I can readily see would be to move the
> temperature read into the compensation functions themselves, possibly
> removing t_fine from data and having a function that reads everything
> relevant to computing it directly but doesn't do the maths to get
> a temperature reading.  That can be reused in bmp280_compensate_temp()
> 
> Something along lines of.
> 
> static s32 bmp280_calc_tfine(struct bmp280_calib *calib, s32 adc_temp) 
> {
> 	s32 var1, var2;
> 
> 	var1 = (((adc_temp >> 3) - ((s32)calib->T1 << 1)) *
> 		((s32)calib->T2)) >> 11;
> 	var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) *
> 		  ((adc_temp >> 4) - ((s32)calib->T1))) >> 12) *
> 		((s32)calib->T3)) >> 14;
> 	return var1 + var2;
> }
> 
> static int bmp280_read_temp_raw(struct bmp280_data *data,
> 			    	s32 *raw)
> {
> 	s32 adc_temp;
> 	int ret;
> 
> 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> 			       data->buf, sizeof(data->buf));
> 	if (ret < 0) {
> 		dev_err(data->dev, "failed to read temperature\n");
> 		return ret;
> 	}
> 
> 	adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf));
> 	if (adc_temp == BMP280_TEMP_SKIPPED) {
> 		/* reading was skipped */
> 		dev_err(data->dev, "reading temperature skipped\n");
> 		return -EIO;
> 	}
> 	*raw = adc_temp;
> 
> 	return 0;
> }
> static int bmp280_get_t_fine(.., s32 *t_fine)
> {
> 	s32 adc_temp, comp_temp;
> 	s32 t_fine;
> 	int ret;
> 
> 	ret = bmp280_read_temp_raw(data, &adc_temp;
> 	if (ret)
> 		return ret;
> 
> 	*t_fine = bmp280_calc_tfine(&data->calib.bmp280, adc_temp);
> 	return 0;
> }
> 
> static int bmp280_read_temp(struct bmp280_data *data, s32 *temp)
> {
> 	int ret;
> 	s32 t_fine;
> 
> 	ret = bmp280_get_t_fine(data, &t_fine);
> 	if (ret)
> 		return ret;
> 
> 	*temp = (t_fine * 5 + 128) / 256;
> //division rather than shift as then it's obvious what the 128 is there for
> 	return 0;
> }
> 
> Now you have a nice function to get you t_fine which is all you want in some
> of these paths.  Call it directly where it is needed instead of as
> a side effect of a temperature read.
> 
> 
> 
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 38 ++++++++++++++----------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 6d7734f867bc..377e90d9e5a2 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -404,11 +404,6 @@ static u32 bmp280_read_press(struct bmp280_data *data)
> >  	s32 adc_press;
> >  	int ret;
> >  
> > -	/* Read and compensate temperature so we get a reading of t_fine. */
> > -	ret = bmp280_read_temp(data);
> > -	if (ret < 0)
> > -		return ret;
> > -
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> >  			       data->buf, sizeof(data->buf));
> >  	if (ret < 0) {
> > @@ -433,11 +428,6 @@ static u32 bmp280_read_humid(struct bmp280_data *data)
> >  	s32 adc_humidity;
> >  	int ret;
> >  
> > -	/* Read and compensate temperature so we get a reading of t_fine. */
> > -	ret = bmp280_read_temp(data);
> > -	if (ret < 0)
> > -		return ret;
> > -
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> >  			       &data->be16, sizeof(data->be16));
> >  	if (ret < 0) {
> > @@ -470,12 +460,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_PROCESSED:
> >  		switch (chan->type) {
> >  		case IIO_HUMIDITYRELATIVE:
> > +			/* Read temperature to update the t_fine value */
> > +			data->chip_info->read_temp(data);
> >  			ret = data->chip_info->read_humid(data);
> >  			*val = data->chip_info->humid_coeffs[0] * ret;
> >  			*val2 = data->chip_info->humid_coeffs[1];
> >  			ret = IIO_VAL_FRACTIONAL;
> >  			break;
> >  		case IIO_PRESSURE:
> > +			/*
> > +			 * Read temperature to update the t_fine value.
> > +			 * BMP5xx devices do this in hardware, so skip it.
> > +			 */
> > +			if (strcmp(indio_dev->name, "bmp580"))
> > +				data->chip_info->read_temp(data);
> > +
> >  			ret = data->chip_info->read_press(data);
> >  			*val = data->chip_info->press_coeffs[0] * ret;
> >  			*val2 = data->chip_info->press_coeffs[1];
> > @@ -500,10 +499,19 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_RAW:
> >  		switch (chan->type) {
> >  		case IIO_HUMIDITYRELATIVE:
> > +			/* Read temperature to update the t_fine value */
> > +			data->chip_info->read_temp(data);
> >  			*val = data->chip_info->read_humid(data);
> >  			ret = IIO_VAL_INT;
> >  			break;
> >  		case IIO_PRESSURE:
> > +			/*
> > +			 * Read temperature to update the t_fine value.
> > +			 * BMP5xx devices do this in hardware, so skip it.
> > +			 */
> > +			if (strcmp(indio_dev->name, "bmp580"))
> > +				data->chip_info->read_temp(data);
> > +
> >  			*val = data->chip_info->read_press(data);
> >  			ret = IIO_VAL_INT;
> >  			break;
> > @@ -1092,11 +1100,6 @@ static u32 bmp380_read_press(struct bmp280_data *data)
> >  	s32 adc_press;
> >  	int ret;
> >  
> > -	/* Read and compensate for temperature so we get a reading of t_fine */
> > -	ret = bmp380_read_temp(data);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> >  			       data->buf, sizeof(data->buf));
> >  	if (ret) {
> > @@ -2009,11 +2012,6 @@ static u32 bmp180_read_press(struct bmp280_data *data)
> >  	s32 adc_press;
> >  	int ret;
> >  
> > -	/* Read and compensate temperature so we get a reading of t_fine. */
> > -	ret = bmp180_read_temp(data);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ret = bmp180_read_adc_press(data, &adc_press);
> >  	if (ret)
> >  		return ret;
> 

Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Angel Iglesias]

On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:
> On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> > On Wed, 13 Mar 2024 18:40:05 +0100
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > 
> > > The read_press/read_humid functions need the updated t_fine value
> > > in order to calculate the current pressure/humidity. Temperature
> > > reads should be removed from the read_press/read_humid functions
> > > and should be placed in the oneshot captures before the pressure
> > > and humidity reads. This makes the code more intuitive.
> > > 
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > To me this makes the use of these calls less obvious than they were
> > previously.  The calls are made close to where t_fine is used and
> > don't have to go via the indirection of chip_info.
> > 
> > So I disagree. I think this change makes the code a lot less
> > clear.
> > 
> 
> This was mainly driven by the fact that I wanted to avoid reading
> the temperature 3 times in case temp, press and humid are enabled
> and there are consecutive buffer readings. But thank you for the
> proposal I really appreciate it!
> 

Hi, just a side note reflecting on this. Depending on your sampling frequency
and registers data shadowing, to avoid compensating with different samples
between readings, we should be doing burst readings to get a bundle of the
temperature+pressure and/or humidity.
On the bmp/bme280 and bmp380 this can be done as registers are contiguous on the
memory. On the bmp580 this is not a problem as the values are already
compensated, you`ll get always the latest reading.

Kind regard,
Angel

Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Vasileios Amoiridis]

On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote:
> On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:
> > On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> > > On Wed, 13 Mar 2024 18:40:05 +0100
> > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > > 
> > > > The read_press/read_humid functions need the updated t_fine value
> > > > in order to calculate the current pressure/humidity. Temperature
> > > > reads should be removed from the read_press/read_humid functions
> > > > and should be placed in the oneshot captures before the pressure
> > > > and humidity reads. This makes the code more intuitive.
> > > > 
> > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > 
> > > To me this makes the use of these calls less obvious than they were
> > > previously.  The calls are made close to where t_fine is used and
> > > don't have to go via the indirection of chip_info.
> > > 
> > > So I disagree. I think this change makes the code a lot less
> > > clear.
> > > 
> > 
> > This was mainly driven by the fact that I wanted to avoid reading
> > the temperature 3 times in case temp, press and humid are enabled
> > and there are consecutive buffer readings. But thank you for the
> > proposal I really appreciate it!
> > 
> 
> Hi, just a side note reflecting on this. Depending on your sampling frequency
> and registers data shadowing, to avoid compensating with different samples
> between readings, we should be doing burst readings to get a bundle of the
> temperature+pressure and/or humidity.
> On the bmp/bme280 and bmp380 this can be done as registers are contiguous on the
> memory. On the bmp580 this is not a problem as the values are already
> compensated, you`ll get always the latest reading.
> 
> Kind regard,
> Angel

Hi Angel,

Thank you for pointing this out! Indeed that's true but I noticed that this is not
the case for the BMP{085/180} devices. I just feel that some changes might make
data acquisition/processing faster for a device (like the one you proposed) but
it might make the code much more unreadable and unmaintanable. I will try and
see if something could be done in that sense but I feel that keeping it simple will
be good for everyone!

Cheers,
Vasilis

Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Angel Iglesias]

On Fri, 2024-03-15 at 10:05 +0100, Vasileios Amoiridis wrote:
> On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote:
> > On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:
> > > On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> > > > On Wed, 13 Mar 2024 18:40:05 +0100
> > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > > > 
> > > > > The read_press/read_humid functions need the updated t_fine value
> > > > > in order to calculate the current pressure/humidity. Temperature
> > > > > reads should be removed from the read_press/read_humid functions
> > > > > and should be placed in the oneshot captures before the pressure
> > > > > and humidity reads. This makes the code more intuitive.
> > > > > 
> > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > 
> > > > To me this makes the use of these calls less obvious than they were
> > > > previously.  The calls are made close to where t_fine is used and
> > > > don't have to go via the indirection of chip_info.
> > > > 
> > > > So I disagree. I think this change makes the code a lot less
> > > > clear.
> > > > 
> > > 
> > > This was mainly driven by the fact that I wanted to avoid reading
> > > the temperature 3 times in case temp, press and humid are enabled
> > > and there are consecutive buffer readings. But thank you for the
> > > proposal I really appreciate it!
> > > 
> > 
> > Hi, just a side note reflecting on this. Depending on your sampling
> > frequency
> > and registers data shadowing, to avoid compensating with different samples
> > between readings, we should be doing burst readings to get a bundle of the
> > temperature+pressure and/or humidity.
> > On the bmp/bme280 and bmp380 this can be done as registers are contiguous on
> > the
> > memory. On the bmp580 this is not a problem as the values are already
> > compensated, you`ll get always the latest reading.
> > 
> > Kind regard,
> > Angel
> 
> Hi Angel,
> 
> Thank you for pointing this out! Indeed that's true but I noticed that this is
> not
> the case for the BMP{085/180} devices. I just feel that some changes might
> make
> data acquisition/processing faster for a device (like the one you proposed)
> but
> it might make the code much more unreadable and unmaintanable. I will try and
> see if something could be done in that sense but I feel that keeping it simple
> will
> be good for everyone!
> 
> Cheers,
> Vasilis

Yeah, data adquisition on bmp085/180 is already different as they don't support
continuous mode as the newer models and you have to warm-up the sensor and do
one-shot readings. There's already a different code path in place for that
models. I guess that is the price to pay to support that wide range of
sensors...
Anyway, this patches are already big and you're doing quite a lot of heavy-
lifting right now, so don't pay much attention to my ramblings! Regardless,
happy to help with tasks polishing and updating this driver :)

Kind regards,
Angel

Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings

[Jonathan Cameron]

On Fri, 15 Mar 2024 14:28:30 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> On Fri, 2024-03-15 at 10:05 +0100, Vasileios Amoiridis wrote:
> > On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote:  
> > > On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote:  
> > > > On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:  
> > > > > On Wed, 13 Mar 2024 18:40:05 +0100
> > > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > > > >   
> > > > > > The read_press/read_humid functions need the updated t_fine value
> > > > > > in order to calculate the current pressure/humidity. Temperature
> > > > > > reads should be removed from the read_press/read_humid functions
> > > > > > and should be placed in the oneshot captures before the pressure
> > > > > > and humidity reads. This makes the code more intuitive.
> > > > > > 
> > > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > > > > 
> > > > > To me this makes the use of these calls less obvious than they were
> > > > > previously.  The calls are made close to where t_fine is used and
> > > > > don't have to go via the indirection of chip_info.
> > > > > 
> > > > > So I disagree. I think this change makes the code a lot less
> > > > > clear.
> > > > >   
> > > > 
> > > > This was mainly driven by the fact that I wanted to avoid reading
> > > > the temperature 3 times in case temp, press and humid are enabled
> > > > and there are consecutive buffer readings. But thank you for the
> > > > proposal I really appreciate it!
> > > >   
> > > 
> > > Hi, just a side note reflecting on this. Depending on your sampling
> > > frequency
> > > and registers data shadowing, to avoid compensating with different samples
> > > between readings, we should be doing burst readings to get a bundle of the
> > > temperature+pressure and/or humidity.
> > > On the bmp/bme280 and bmp380 this can be done as registers are contiguous on
> > > the
> > > memory. On the bmp580 this is not a problem as the values are already
> > > compensated, you`ll get always the latest reading.
> > > 
> > > Kind regard,
> > > Angel  
> > 
> > Hi Angel,
> > 
> > Thank you for pointing this out! Indeed that's true but I noticed that this is
> > not
> > the case for the BMP{085/180} devices. I just feel that some changes might
> > make
> > data acquisition/processing faster for a device (like the one you proposed)
> > but
> > it might make the code much more unreadable and unmaintanable. I will try and
> > see if something could be done in that sense but I feel that keeping it simple
> > will
> > be good for everyone!
> > 
> > Cheers,
> > Vasilis  
> 
> Yeah, data adquisition on bmp085/180 is already different as they don't support
> continuous mode as the newer models and you have to warm-up the sensor and do
> one-shot readings. There's already a different code path in place for that
> models. I guess that is the price to pay to support that wide range of
> sensors...
> Anyway, this patches are already big and you're doing quite a lot of heavy-
> lifting right now, so don't pay much attention to my ramblings! Regardless,
> happy to help with tasks polishing and updating this driver :)

If burst readings do make sense: We can reasonably assume anyone who is using
this sensor and is using buffered mode probably wants to 'mostly' grab all the
channels, then a specific function that always grabs them all, plus use of
available_scan_masks = { ALL BITS, 0 }; will let the IIO core deal with any
cases where only some channels are requested.  This is something we
do in drivers where there is some interaction between the channels (like here)
or where burst reads are much more efficient than single channels (possibly
also true here) and complexity is significant for switching between burst
and single channels reads.

That covers a lot of devices and is part of why we have the core code
do channel de-multiplexing rather than leaving it for the drivers. The other
reason that drove that complexity was unrelated to this driver (SoC ADCs with
some channels used for touchscreens, and others for unrelated purposes).

You may just want to reduce how much code you are reusing from
the oneshot single channel sysfs reads so that you can just do a single set
of readings and use them as needed for compensation etc.

Jonathan

 
> 
> Kind regards,
> Angel
> 

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

[Vasileios Amoiridis]

The scan mask for the BME280 supports humidity measurement needs
to be distinguished from the rest in order for the timestamp to
be able to work. Scan masks are added for different combinations
of measurements. The temperature measurement is always needed for
pressure and humidity measurements.

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

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 377e90d9e5a2..f2cf9bef522c 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -134,40 +134,91 @@ enum {
 	BMP380_P11 = 20,
 };
 
+enum bmp280_scan {
+	BMP280_TEMP,
+	BMP280_PRESS,
+	BME280_HUMID,
+};
+
 static const struct iio_chan_spec bmp280_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.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,
 		.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 = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+static const struct iio_chan_spec bme280_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.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_HUMIDITYRELATIVE,
+		.type = IIO_PRESSURE,
 		.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,
 		.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,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
@@ -176,16 +227,31 @@ 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,
 		.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) |
+	.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)
@@ -829,6 +895,20 @@ static const struct iio_info bmp280_info = {
 	.write_raw = &bmp280_write_raw,
 };
 
+static const unsigned long bmp280_avail_scan_masks[] = {
+	BIT(BMP280_TEMP),
+	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+	0
+};
+
+static const unsigned long bme280_avail_scan_masks[] = {
+	BIT(BMP280_TEMP),
+	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+	BIT(BME280_HUMID) | BIT(BMP280_TEMP),
+	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) |
@@ -870,7 +950,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),
@@ -927,8 +1008,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),
@@ -1292,7 +1374,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),
@@ -1787,7 +1870,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),
@@ -2039,7 +2123,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 =
@@ -2149,6 +2234,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 dde55b556ea2..c8cb7c417dab 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 v2 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver

[Andy Shevchenko]

On Wed, Mar 13, 2024 at 06:40:06PM +0100, Vasileios Amoiridis wrote:
> The scan mask for the BME280 supports humidity measurement needs
> to be distinguished from the rest in order for the timestamp to
> be able to work. Scan masks are added for different combinations
> of measurements. The temperature measurement is always needed for
> pressure and humidity measurements.

(Just to make sure if you used --histogram diff algo when preparing the series)

...

>  	{
> -		.type = IIO_HUMIDITYRELATIVE,
> +		.type = IIO_PRESSURE,
>  		.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) |
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |

Stray change

>  					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 32,
> +			.storagebits = 32,
> +			.endianness = IIO_CPU,
> +		},
>  	},

-- 
With Best Regards,
Andy Shevchenko

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

[Vasileios Amoiridis]

On Wed, Mar 13, 2024 at 08:54:47PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 06:40:06PM +0100, Vasileios Amoiridis wrote:
> > The scan mask for the BME280 supports humidity measurement needs
> > to be distinguished from the rest in order for the timestamp to
> > be able to work. Scan masks are added for different combinations
> > of measurements. The temperature measurement is always needed for
> > pressure and humidity measurements.
> 
> (Just to make sure if you used --histogram diff algo when preparing the series)
> 
> ...
> 
> >  	{
> > -		.type = IIO_HUMIDITYRELATIVE,
> > +		.type = IIO_PRESSURE,
> >  		.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) |
> > +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> 
> Stray change
> 
I didn't notice that, and the checkpatch.pl didn't actually say something,
thanks for pointing out.
> >  					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> > +		.scan_index = 1,
> > +		.scan_type = {
> > +			.sign = 'u',
> > +			.realbits = 32,
> > +			.storagebits = 32,
> > +			.endianness = IIO_CPU,
> > +		},
> >  	},
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Best regards,
Vasilis Amoiridis

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

[Vasileios Amoiridis]

Add a buffer struct that will hold the values of the measurements
and will be pushed to userspace and a buffer_handler function to
read the data and push them.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/Kconfig       |  2 +
 drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
 drivers/iio/pressure/bmp280.h      |  7 ++++
 3 files changed, 70 insertions(+)

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 79adfd059c3a..5145b94b4679 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -31,6 +31,8 @@ config BMP280
 	select REGMAP
 	select BMP280_I2C if (I2C)
 	select BMP280_SPI if (SPI_MASTER)
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
 	  and BMP580 pressure and temperature sensors. Also supports the BME280 with
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index f2cf9bef522c..7c889cda396a 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -40,7 +40,10 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #include <asm/unaligned.h>
 
@@ -2188,6 +2191,57 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
 	return 0;
 }
 
+static irqreturn_t bmp280_buffer_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct bmp280_data *data = iio_priv(indio_dev);
+	int ret, temp;
+
+	/*
+	 * data->buf[3] is used to transfer data from the device. Whenever a
+	 * pressure or a humidity reading takes place, the data written in the
+	 * data->buf[3] overwrites the iio_buf.temperature value. Keep the
+	 * temperature value and apply it after the readings.
+	 */
+	mutex_lock(&data->lock);
+
+	if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
+		ret = data->chip_info->read_temp(data);
+		if (ret < 0)
+			goto done;
+
+		temp = ret;
+	}
+
+	if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
+		ret = data->chip_info->read_press(data);
+		if (ret < 0)
+			goto done;
+
+		data->iio_buf.pressure = ret;
+		data->iio_buf.temperature = temp;
+	}
+
+	if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
+		ret = data->chip_info->read_humid(data);
+		if (ret < 0)
+			goto done;
+
+		data->iio_buf.humidity = ret;
+		data->iio_buf.temperature = temp;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
+					   iio_get_time_ns(indio_dev));
+
+done:
+	mutex_unlock(&data->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static void bmp280_pm_disable(void *data)
 {
 	struct device *dev = data;
@@ -2329,6 +2383,13 @@ int bmp280_common_probe(struct device *dev,
 			return ret;
 	}
 
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      &bmp280_buffer_handler, NULL);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio triggered buffer setup failed\n");
+
 	/* Enable runtime PM */
 	pm_runtime_get_noresume(dev);
 	pm_runtime_set_active(dev);
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index c8cb7c417dab..b5369dd496ba 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -407,6 +407,13 @@ struct bmp280_data {
 	union {
 		/* Sensor data buffer */
 		u8 buf[3];
+		/* Data buffer to push to userspace */
+		struct {
+			s32 temperature;
+			u32 pressure;
+			u32 humidity;
+			s64 timestamp __aligned(8);
+		} iio_buf;
 		/* Calibration data buffers */
 		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
 		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
-- 
2.25.1

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

[Andy Shevchenko]

On Wed, Mar 13, 2024 at 06:40:07PM +0100, Vasileios Amoiridis wrote:
> Add a buffer struct that will hold the values of the measurements
> and will be pushed to userspace and a buffer_handler function to
> read the data and push them.

...

> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,

dev here

> +					      iio_pollfunc_store_time,
> +					      &bmp280_buffer_handler, NULL);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,

data->dev here

Are they the same? If not, why this difference?

> +				     "iio triggered buffer setup failed\n");

-- 
With Best Regards,
Andy Shevchenko

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

[Vasileios Amoiridis]

On Wed, Mar 13, 2024 at 08:58:07PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 06:40:07PM +0100, Vasileios Amoiridis wrote:
> > Add a buffer struct that will hold the values of the measurements
> > and will be pushed to userspace and a buffer_handler function to
> > read the data and push them.
> 
> ...
> 
> > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> 
> dev here
> 
> > +					      iio_pollfunc_store_time,
> > +					      &bmp280_buffer_handler, NULL);
> > +	if (ret)
> > +		return dev_err_probe(data->dev, ret,
> 
> data->dev here
> 
> Are they the same? If not, why this difference?
> 
They are the same. I didn't notice it, but I will fix it for consistency.

> > +				     "iio triggered buffer setup failed\n");
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Best regards,
Vasileios Amoiridis

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

[Jonathan Cameron]

On Wed, 13 Mar 2024 18:40:07 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Add a buffer struct that will hold the values of the measurements
> and will be pushed to userspace and a buffer_handler function to
> read the data and push them.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/Kconfig       |  2 +
>  drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
>  drivers/iio/pressure/bmp280.h      |  7 ++++
>  3 files changed, 70 insertions(+)
> 
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index 79adfd059c3a..5145b94b4679 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -31,6 +31,8 @@ config BMP280
>  	select REGMAP
>  	select BMP280_I2C if (I2C)
>  	select BMP280_SPI if (SPI_MASTER)
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
>  	  and BMP580 pressure and temperature sensors. Also supports the BME280 with
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index f2cf9bef522c..7c889cda396a 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -40,7 +40,10 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  #include <asm/unaligned.h>
>  
> @@ -2188,6 +2191,57 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
>  	return 0;
>  }
>  
> +static irqreturn_t bmp280_buffer_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	int ret, temp;
> +
> +	/*
> +	 * data->buf[3] is used to transfer data from the device. Whenever a
> +	 * pressure or a humidity reading takes place, the data written in the
> +	 * data->buf[3] overwrites the iio_buf.temperature value. Keep the
> +	 * temperature value and apply it after the readings.

See comment below. Given you saw this problem did you not think maybe you
were doing something a little unusual / wrong? Should have rung alarm
bells beyond just putting a comment here to explain you needed to work around
the issue.

> +	 */
> +	mutex_lock(&data->lock);
> +
> +	if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
> +		ret = data->chip_info->read_temp(data);
> +		if (ret < 0)
> +			goto done;
> +
> +		temp = ret;
> +	}
> +
> +	if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
> +		ret = data->chip_info->read_press(data);
> +		if (ret < 0)
> +			goto done;
> +
> +		data->iio_buf.pressure = ret;
> +		data->iio_buf.temperature = temp;
Try running this with the tooling in tools/iio and you'll see that
you are getting the wrong output if you have just humidity and
temperature enabled - IIO packs channels, so disable an early
one and everything moves down in address.

If you an this device without timestamps and only a single channel
the buffer used will have one s32 per scan for example.

> +	}
> +
> +	if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> +		ret = data->chip_info->read_humid(data);
> +		if (ret < 0)
> +			goto done;
> +
> +		data->iio_buf.humidity = ret;
> +		data->iio_buf.temperature = temp;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> +					   iio_get_time_ns(indio_dev));
> +
> +done:
> +	mutex_unlock(&data->lock);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static void bmp280_pm_disable(void *data)
>  {
>  	struct device *dev = data;
> @@ -2329,6 +2383,13 @@ int bmp280_common_probe(struct device *dev,
>  			return ret;
>  	}
>  
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      &bmp280_buffer_handler, NULL);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "iio triggered buffer setup failed\n");
> +
>  	/* Enable runtime PM */
>  	pm_runtime_get_noresume(dev);
>  	pm_runtime_set_active(dev);
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index c8cb7c417dab..b5369dd496ba 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -407,6 +407,13 @@ struct bmp280_data {
>  	union {
>  		/* Sensor data buffer */
>  		u8 buf[3];
> +		/* Data buffer to push to userspace */

Why is this in the union?  This union is to ensure cache safe buffers for
DMAing directly into.  That's not applicable here.  Even though it may
be safe to do this (I was reading backwards so wrote a comment here
on you corrupting temperature before seeing the comment above)
it is giving a misleading impression of how this struct is used.
Pull the struct to after t_fine - It only needs to
be 8 byte aligned and the aligned marking you have will ensure that
 
> +		struct {
> +			s32 temperature;
> +			u32 pressure;
> +			u32 humidity;

As above, this is 3 4 byte buffers but they don't have these meanings
unless all channels are enabled.

You could set available mask to enforce that, but don't as it makes
limited sense for this hardware.  Just make these
			u32 chans[3];
and fill them in with an index incremented for each channel that is
enabled.

Jonathan

> +			s64 timestamp __aligned(8);
> +		} iio_buf;
>  		/* Calibration data buffers */
>  		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
>  		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];

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

[Vasileios Amoiridis]

On Thu, Mar 14, 2024 at 03:25:25PM +0000, Jonathan Cameron wrote:
> On Wed, 13 Mar 2024 18:40:07 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Add a buffer struct that will hold the values of the measurements
> > and will be pushed to userspace and a buffer_handler function to
> > read the data and push them.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/pressure/Kconfig       |  2 +
> >  drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
> >  drivers/iio/pressure/bmp280.h      |  7 ++++
> >  3 files changed, 70 insertions(+)
> > 
> > diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> > index 79adfd059c3a..5145b94b4679 100644
> > --- a/drivers/iio/pressure/Kconfig
> > +++ b/drivers/iio/pressure/Kconfig
> > @@ -31,6 +31,8 @@ config BMP280
> >  	select REGMAP
> >  	select BMP280_I2C if (I2C)
> >  	select BMP280_SPI if (SPI_MASTER)
> > +	select IIO_BUFFER
> > +	select IIO_TRIGGERED_BUFFER
> >  	help
> >  	  Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
> >  	  and BMP580 pressure and temperature sensors. Also supports the BME280 with
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f2cf9bef522c..7c889cda396a 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -40,7 +40,10 @@
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> >  
> > +#include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> >  
> >  #include <asm/unaligned.h>
> >  
> > @@ -2188,6 +2191,57 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
> >  	return 0;
> >  }
> >  
> > +static irqreturn_t bmp280_buffer_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +	int ret, temp;
> > +
> > +	/*
> > +	 * data->buf[3] is used to transfer data from the device. Whenever a
> > +	 * pressure or a humidity reading takes place, the data written in the
> > +	 * data->buf[3] overwrites the iio_buf.temperature value. Keep the
> > +	 * temperature value and apply it after the readings.
> 
> See comment below. Given you saw this problem did you not think maybe you
> were doing something a little unusual / wrong? Should have rung alarm
> bells beyond just putting a comment here to explain you needed to work around
> the issue.
> 
> > +	 */
> > +	mutex_lock(&data->lock);
> > +
> > +	if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
> > +		ret = data->chip_info->read_temp(data);
> > +		if (ret < 0)
> > +			goto done;
> > +
> > +		temp = ret;
> > +	}
> > +
> > +	if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
> > +		ret = data->chip_info->read_press(data);
> > +		if (ret < 0)
> > +			goto done;
> > +
> > +		data->iio_buf.pressure = ret;
> > +		data->iio_buf.temperature = temp;
> Try running this with the tooling in tools/iio and you'll see that
> you are getting the wrong output if you have just humidity and
> temperature enabled - IIO packs channels, so disable an early
> one and everything moves down in address.
> 
> If you an this device without timestamps and only a single channel
> the buffer used will have one s32 per scan for example.
> 
> > +	}
> > +
> > +	if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> > +		ret = data->chip_info->read_humid(data);
> > +		if (ret < 0)
> > +			goto done;
> > +
> > +		data->iio_buf.humidity = ret;
> > +		data->iio_buf.temperature = temp;
> > +	}
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +done:
> > +	mutex_unlock(&data->lock);
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static void bmp280_pm_disable(void *data)
> >  {
> >  	struct device *dev = data;
> > @@ -2329,6 +2383,13 @@ int bmp280_common_probe(struct device *dev,
> >  			return ret;
> >  	}
> >  
> > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> > +					      iio_pollfunc_store_time,
> > +					      &bmp280_buffer_handler, NULL);
> > +	if (ret)
> > +		return dev_err_probe(data->dev, ret,
> > +				     "iio triggered buffer setup failed\n");
> > +
> >  	/* Enable runtime PM */
> >  	pm_runtime_get_noresume(dev);
> >  	pm_runtime_set_active(dev);
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index c8cb7c417dab..b5369dd496ba 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -407,6 +407,13 @@ struct bmp280_data {
> >  	union {
> >  		/* Sensor data buffer */
> >  		u8 buf[3];
> > +		/* Data buffer to push to userspace */
> 
> Why is this in the union?  This union is to ensure cache safe buffers for
> DMAing directly into.  That's not applicable here.  Even though it may
> be safe to do this (I was reading backwards so wrote a comment here
> on you corrupting temperature before seeing the comment above)
> it is giving a misleading impression of how this struct is used.
> Pull the struct to after t_fine - It only needs to
> be 8 byte aligned and the aligned marking you have will ensure that
>  
> > +		struct {
> > +			s32 temperature;
> > +			u32 pressure;
> > +			u32 humidity;
> 
> As above, this is 3 4 byte buffers but they don't have these meanings
> unless all channels are enabled.
> 
> You could set available mask to enforce that, but don't as it makes
> limited sense for this hardware.  Just make these
> 			u32 chans[3];
> and fill them in with an index incremented for each channel that is
> enabled.
> 
> Jonathan
> 

I wanted to put it inside the union to save some space but you are right
that it is quite misleading. I was just trying in general, along with the
previous patches to avoid reading the temperature twice. Along with your
comments in the previous patch, if a user has enabled both temperature
and pressure and humidity we could save ourselves from reading the
temperature 3 times instead of 1. But in any case, your previous proposal
with a separate get_t_fine structure looks good.

Best regards,
Vasilis
> > +			s64 timestamp __aligned(8);
> > +		} iio_buf;
> >  		/* Calibration data buffers */
> >  		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> >  		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
> 

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

[Jonathan Cameron]

On Thu, 14 Mar 2024 21:14:31 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Thu, Mar 14, 2024 at 03:25:25PM +0000, Jonathan Cameron wrote:
> > On Wed, 13 Mar 2024 18:40:07 +0100
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> >   
> > > Add a buffer struct that will hold the values of the measurements
> > > and will be pushed to userspace and a buffer_handler function to
> > > read the data and push them.
> > > 
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > ---
> > >  drivers/iio/pressure/Kconfig       |  2 +
> > >  drivers/iio/pressure/bmp280-core.c | 61 ++++++++++++++++++++++++++++++
> > >  drivers/iio/pressure/bmp280.h      |  7 ++++
> > >  3 files changed, 70 insertions(+)
> > > 
> > > diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> > > index 79adfd059c3a..5145b94b4679 100644
> > > --- a/drivers/iio/pressure/Kconfig
> > > +++ b/drivers/iio/pressure/Kconfig
> > > @@ -31,6 +31,8 @@ config BMP280
> > >  	select REGMAP
> > >  	select BMP280_I2C if (I2C)
> > >  	select BMP280_SPI if (SPI_MASTER)
> > > +	select IIO_BUFFER
> > > +	select IIO_TRIGGERED_BUFFER
> > >  	help
> > >  	  Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
> > >  	  and BMP580 pressure and temperature sensors. Also supports the BME280 with
> > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > > index f2cf9bef522c..7c889cda396a 100644
> > > --- a/drivers/iio/pressure/bmp280-core.c
> > > +++ b/drivers/iio/pressure/bmp280-core.c
> > > @@ -40,7 +40,10 @@
> > >  #include <linux/regmap.h>
> > >  #include <linux/regulator/consumer.h>
> > >  
> > > +#include <linux/iio/buffer.h>
> > >  #include <linux/iio/iio.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > >  
> > >  #include <asm/unaligned.h>
> > >  
> > > @@ -2188,6 +2191,57 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
> > >  	return 0;
> > >  }
> > >  
> > > +static irqreturn_t bmp280_buffer_handler(int irq, void *p)
> > > +{
> > > +	struct iio_poll_func *pf = p;
> > > +	struct iio_dev *indio_dev = pf->indio_dev;
> > > +	struct bmp280_data *data = iio_priv(indio_dev);
> > > +	int ret, temp;
> > > +
> > > +	/*
> > > +	 * data->buf[3] is used to transfer data from the device. Whenever a
> > > +	 * pressure or a humidity reading takes place, the data written in the
> > > +	 * data->buf[3] overwrites the iio_buf.temperature value. Keep the
> > > +	 * temperature value and apply it after the readings.  
> > 
> > See comment below. Given you saw this problem did you not think maybe you
> > were doing something a little unusual / wrong? Should have rung alarm
> > bells beyond just putting a comment here to explain you needed to work around
> > the issue.
> >   
> > > +	 */
> > > +	mutex_lock(&data->lock);
> > > +
> > > +	if (test_bit(BMP280_TEMP, indio_dev->active_scan_mask)) {
> > > +		ret = data->chip_info->read_temp(data);
> > > +		if (ret < 0)
> > > +			goto done;
> > > +
> > > +		temp = ret;
> > > +	}
> > > +
> > > +	if (test_bit(BMP280_PRESS, indio_dev->active_scan_mask)) {
> > > +		ret = data->chip_info->read_press(data);
> > > +		if (ret < 0)
> > > +			goto done;
> > > +
> > > +		data->iio_buf.pressure = ret;
> > > +		data->iio_buf.temperature = temp;  
> > Try running this with the tooling in tools/iio and you'll see that
> > you are getting the wrong output if you have just humidity and
> > temperature enabled - IIO packs channels, so disable an early
> > one and everything moves down in address.
> > 
> > If you an this device without timestamps and only a single channel
> > the buffer used will have one s32 per scan for example.
> >   
> > > +	}
> > > +
> > > +	if (test_bit(BME280_HUMID, indio_dev->active_scan_mask)) {
> > > +		ret = data->chip_info->read_humid(data);
> > > +		if (ret < 0)
> > > +			goto done;
> > > +
> > > +		data->iio_buf.humidity = ret;
> > > +		data->iio_buf.temperature = temp;
> > > +	}
> > > +
> > > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->iio_buf,
> > > +					   iio_get_time_ns(indio_dev));
> > > +
> > > +done:
> > > +	mutex_unlock(&data->lock);
> > > +	iio_trigger_notify_done(indio_dev->trig);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > >  static void bmp280_pm_disable(void *data)
> > >  {
> > >  	struct device *dev = data;
> > > @@ -2329,6 +2383,13 @@ int bmp280_common_probe(struct device *dev,
> > >  			return ret;
> > >  	}
> > >  
> > > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> > > +					      iio_pollfunc_store_time,
> > > +					      &bmp280_buffer_handler, NULL);
> > > +	if (ret)
> > > +		return dev_err_probe(data->dev, ret,
> > > +				     "iio triggered buffer setup failed\n");
> > > +
> > >  	/* Enable runtime PM */
> > >  	pm_runtime_get_noresume(dev);
> > >  	pm_runtime_set_active(dev);
> > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > > index c8cb7c417dab..b5369dd496ba 100644
> > > --- a/drivers/iio/pressure/bmp280.h
> > > +++ b/drivers/iio/pressure/bmp280.h
> > > @@ -407,6 +407,13 @@ struct bmp280_data {
> > >  	union {
> > >  		/* Sensor data buffer */
> > >  		u8 buf[3];
> > > +		/* Data buffer to push to userspace */  
> > 
> > Why is this in the union?  This union is to ensure cache safe buffers for
> > DMAing directly into.  That's not applicable here.  Even though it may
> > be safe to do this (I was reading backwards so wrote a comment here
> > on you corrupting temperature before seeing the comment above)
> > it is giving a misleading impression of how this struct is used.
> > Pull the struct to after t_fine - It only needs to
> > be 8 byte aligned and the aligned marking you have will ensure that
> >    
> > > +		struct {
> > > +			s32 temperature;
> > > +			u32 pressure;
> > > +			u32 humidity;  
> > 
> > As above, this is 3 4 byte buffers but they don't have these meanings
> > unless all channels are enabled.
> > 
> > You could set available mask to enforce that, but don't as it makes
> > limited sense for this hardware.  Just make these
> > 			u32 chans[3];
> > and fill them in with an index incremented for each channel that is
> > enabled.
> > 
> > Jonathan
> >   
> 
> I wanted to put it inside the union to save some space but you are right
> that it is quite misleading.

It may not save any space.  The force alignment here tends to lead to some
room between the first chunk of this structure and the __aligned(IIO_DMA_MINALIGN) part.
This may well fit in the hole - I've not checked though.

> I was just trying in general, along with the
> previous patches to avoid reading the temperature twice. Along with your
> comments in the previous patch, if a user has enabled both temperature
> and pressure and humidity we could save ourselves from reading the
> temperature 3 times instead of 1. But in any case, your previous proposal
> with a separate get_t_fine structure looks good.

If it gets complex, just implement a buffered read that always gets all the
channels and set available_scan_masks to express that.  The core code
has channel data shuffling that will result in your userspace seeing
whatever subset of channels it requests.

Jonathan

> 
> Best regards,
> Vasilis
> > > +			s64 timestamp __aligned(8);
> > > +		} iio_buf;
> > >  		/* Calibration data buffers */
> > >  		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> > >  		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];  
> >