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

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

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

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

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

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

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

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

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

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

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

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

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

Jonathan


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

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

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

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

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

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

As above, I don't think it does. 

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

J

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

Hi Jonathan,

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

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

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

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

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

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

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

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

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

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

	return 0
}

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

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

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

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

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

read_{temp,press,humid}()

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

-- 
With Best Regards,
Andy Shevchenko