[v8,1/3] iio: pressure: bmp280: Generalize read_*() functions

Commit Message

Vasileios Amoiridis June 17, 2024, 11:05 p.m. UTC

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

Move the calculations for the IIO unit compatibility from inside the
read_{temp,press,humid}() functions and move them to the general
read_raw() 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 <[e-mail]>
---
 drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------
 drivers/iio/pressure/bmp280.h      |  13 ++-
 2 files changed, 107 insertions(+), 73 deletions(-)

Comments

Jonathan Cameron June 22, 2024, 9:28 a.m. UTC | #1

On Tue, 18 Jun 2024 01:05:38 +0200
Vasileios Amoiridis <[e-mail]> wrote:

> Add the coefficients for the IIO standard units and the IIO value
> inside the chip_info structure.
> 
> Move the calculations for the IIO unit compatibility from inside the
> read_{temp,press,humid}() functions and move them to the general
> read_raw() 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 <[e-mail]>
Does this incorporate the fix?  I'm a little confused looking at
what is visible here, so I'd like Adam to take a look.

Btw, you missed cc'ing Adam.

> ---
>  drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------
>  drivers/iio/pressure/bmp280.h      |  13 ++-
>  2 files changed, 107 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 50d71ad83f37..27c00af060fa 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -445,10 +445,8 @@ 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 comp_temp;
>  	u32 adc_temp;
>  	int ret;
>  
> @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data,
>  	if (ret)
>  		return ret;
>  
> -	comp_temp = bmp280_compensate_temp(data, adc_temp);
> +	*comp_temp = bmp280_compensate_temp(data, adc_temp);
>  
> -	*val = comp_temp * 10;
> -	return IIO_VAL_INT;
> +	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, adc_press, t_fine;
> +	u32 adc_press;
> +	s32 t_fine;
>  	int ret;
>  
>  	ret = bmp280_get_t_fine(data, &t_fine);
> @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data,
>  	if (ret)
>  		return ret;
>  
> -	comp_press = bmp280_compensate_press(data, adc_press, t_fine);
> -
> -	*val = comp_press;
> -	*val2 = 256000;
> +	*comp_press = bmp280_compensate_press(data, adc_press, t_fine);
>  
> -	return IIO_VAL_FRACTIONAL;
> +	return 0;
>  }
>  
> -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
> +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
>  {
> -	u32 comp_humidity;
>  	u16 adc_humidity;
>  	s32 t_fine;
>  	int ret;
> @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
>  	if (ret)
>  		return ret;
>  
> -	comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
> -
> -	*val = comp_humidity * 1000 / 1024;
> +	*comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
>  
> -	return IIO_VAL_INT;
> +	return 0;
>  }
>  
>  static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
> @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(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);
>  
> @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(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] * (s64)chan_value;
> +			*val2 = data->chip_info->temp_coeffs[1];
> +			return data->chip_info->temp_coeffs_type;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -822,6 +833,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,
> @@ -850,6 +863,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,
> @@ -877,6 +895,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,
> @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = {
>  	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
>  	.oversampling_humid_default = BME280_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,
> @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data,
>  	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;
>  
> @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
>  	if (ret)
>  		return ret;
>  
> -	comp_temp = bmp380_compensate_temp(data, adc_temp);
> +	*comp_temp = bmp380_compensate_temp(data, adc_temp);
>  
> -	*val = comp_temp * 10;
> -	return IIO_VAL_INT;
> +	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)
>  {
> -	u32 adc_press, comp_press, t_fine;
> +	u32 adc_press, t_fine;
>  	int ret;
>  
>  	ret = bmp380_get_t_fine(data, &t_fine);
> @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
>  	if (ret)
>  		return ret;
>  
> -	comp_press = bmp380_compensate_press(data, adc_press, t_fine);
> -
> -	*val = comp_press;
> -	*val2 = 100000;
> +	*comp_press = bmp380_compensate_press(data, adc_press, t_fine);
>  
> -	return IIO_VAL_FRACTIONAL;
> +	return 0;
>  }
>  
>  static int bmp380_read_calib(struct bmp280_data *data)
> @@ -1296,6 +1317,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,
> @@ -1323,6 +1346,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,
> @@ -1443,9 +1471,9 @@ 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;
> +	s32 value_temp;
>  	int ret;
>  
>  	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
> @@ -1455,25 +1483,19 @@ 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) {
> +	value_temp = get_unaligned_le24(data->buf);
> +	if (value_temp == BMP580_TEMP_SKIPPED) {
>  		dev_err(data->dev, "reading temperature skipped\n");
>  		return -EIO;
>  	}
> +	*raw_temp = sign_extend32(value_temp, 23);
>  
> -	/*
> -	 * Temperature is returned in Celsius degrees in fractional
> -	 * form down 2^16. We rescale by x1000 to return millidegrees
> -	 * Celsius to respect IIO ABI.
> -	 */
> -	raw_temp = sign_extend32(raw_temp, 23);
> -	*val = ((s64)raw_temp * 1000) / (1 << 16);
> -	return IIO_VAL_INT;
> +	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;
> +	u32 value_press;
>  	int ret;
>  
>  	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
> @@ -1483,18 +1505,14 @@ 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) {
> +	value_press = get_unaligned_le24(data->buf);
> +	if (value_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;
> +	*raw_press = value_press;
> +
> +	return 0;
>  }
>  
>  static const int bmp580_odr_table[][2] = {
> @@ -1830,6 +1848,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,
> @@ -1856,6 +1876,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 = bmp580_temp_coeffs,
> +	.temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2,
> +	.press_coeffs = bmp580_press_coeffs,
> +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> +
>  	.chip_config = bmp580_chip_config,
>  	.read_temp = bmp580_read_temp,
>  	.read_press = bmp580_read_press,
> @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp)
>  	return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16;
>  }
>  
> -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 comp_temp;
>  	u32 adc_temp;
>  	int ret;
>  
> @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
>  	if (ret)
>  		return ret;
>  
> -	comp_temp = bmp180_compensate_temp(data, adc_temp);
> +	*comp_temp = bmp180_compensate_temp(data, adc_temp);
>  
> -	*val = comp_temp * 100;
> -	return IIO_VAL_INT;
> +	return 0;
>  }
>  
>  static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press)
> @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 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, adc_press;
> +	u32 adc_press;
>  	s32 t_fine;
>  	int ret;
>  
> @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2)
>  	if (ret)
>  		return ret;
>  
> -	comp_press = bmp180_compensate_press(data, adc_press, t_fine);
> -
> -	*val = comp_press;
> -	*val2 = 1000;
> +	*comp_press = bmp180_compensate_press(data, adc_press, t_fine);
>  
> -	return IIO_VAL_FRACTIONAL;
> +	return 0;
>  }
>  
>  static int bmp180_chip_config(struct bmp280_data *data)
> @@ -2117,6 +2137,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,
> @@ -2137,6 +2159,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 7c30e4d523be..a3d2cd722760 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -446,10 +446,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 *data);
> -	int (*read_temp)(struct bmp280_data *data, int *val, int *val2);
> -	int (*read_press)(struct bmp280_data *data, int *val, int *val2);
> -	int (*read_humid)(struct bmp280_data *data, int *val, int *val2);
> +	int (*read_temp)(struct bmp280_data *data, s32 *adc_temp);
> +	int (*read_press)(struct bmp280_data *data, u32 *adc_press);
> +	int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity);
>  	int (*read_calib)(struct bmp280_data *data);
>  	int (*preinit)(struct bmp280_data *data);
>  };

Vasileios Amoiridis June 22, 2024, 12:19 p.m. UTC | #2

On Sat, Jun 22, 2024 at 10:28:26AM +0100, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 01:05:38 +0200
> Vasileios Amoiridis <[e-mail]> wrote:
> 
> > Add the coefficients for the IIO standard units and the IIO value
> > inside the chip_info structure.
> > 
> > Move the calculations for the IIO unit compatibility from inside the
> > read_{temp,press,humid}() functions and move them to the general
> > read_raw() 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 <[e-mail]>
> Does this incorporate the fix?  I'm a little confused looking at
> what is visible here, so I'd like Adam to take a look.
> 
> Btw, you missed cc'ing Adam.
> 

Ah, I only used the output of get_maintainer...

> > ---
> >  drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------
> >  drivers/iio/pressure/bmp280.h      |  13 ++-
> >  2 files changed, 107 insertions(+), 73 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 50d71ad83f37..27c00af060fa 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -445,10 +445,8 @@ 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 comp_temp;
> >  	u32 adc_temp;
> >  	int ret;
> >  
> > @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data,
> >  	if (ret)
> >  		return ret;
> >  
> > -	comp_temp = bmp280_compensate_temp(data, adc_temp);
> > +	*comp_temp = bmp280_compensate_temp(data, adc_temp);
> >  
> > -	*val = comp_temp * 10;
> > -	return IIO_VAL_INT;
> > +	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, adc_press, t_fine;
> > +	u32 adc_press;
> > +	s32 t_fine;
> >  	int ret;
> >  
> >  	ret = bmp280_get_t_fine(data, &t_fine);
> > @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data,
> >  	if (ret)
> >  		return ret;
> >  
> > -	comp_press = bmp280_compensate_press(data, adc_press, t_fine);
> > -
> > -	*val = comp_press;
> > -	*val2 = 256000;
> > +	*comp_press = bmp280_compensate_press(data, adc_press, t_fine);
> >  
> > -	return IIO_VAL_FRACTIONAL;
> > +	return 0;
> >  }
> >  
> > -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
> >  {
> > -	u32 comp_humidity;
> >  	u16 adc_humidity;
> >  	s32 t_fine;
> >  	int ret;
> > @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
> >  	if (ret)
> >  		return ret;
> >  
> > -	comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
> > -
> > -	*val = comp_humidity * 1000 / 1024;
> > +	*comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
> >  
> > -	return IIO_VAL_INT;
> > +	return 0;
> >  }
> >  
> >  static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
> > @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(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);
> >

...
  
> > @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(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] * (s64)chan_value;

This is the first difference with the previous version where I incorporated
the typecasting to (s64).

> > +			*val2 = data->chip_info->temp_coeffs[1];
> > +			return data->chip_info->temp_coeffs_type;
> >  		default:
> >  			return -EINVAL;
> >  		}
> > @@ -822,6 +833,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,
> > @@ -850,6 +863,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,
> > @@ -877,6 +895,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,
> > @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = {
> >  	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> >  	.oversampling_humid_default = BME280_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,
> > @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data,
> >  	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;
> >  
> > @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> >  	if (ret)
> >  		return ret;
> >  
> > -	comp_temp = bmp380_compensate_temp(data, adc_temp);
> > +	*comp_temp = bmp380_compensate_temp(data, adc_temp);
> >  
> > -	*val = comp_temp * 10;
> > -	return IIO_VAL_INT;
> > +	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)
> >  {
> > -	u32 adc_press, comp_press, t_fine;
> > +	u32 adc_press, t_fine;
> >  	int ret;
> >  
> >  	ret = bmp380_get_t_fine(data, &t_fine);
> > @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> >  	if (ret)
> >  		return ret;
> >  
> > -	comp_press = bmp380_compensate_press(data, adc_press, t_fine);
> > -
> > -	*val = comp_press;
> > -	*val2 = 100000;
> > +	*comp_press = bmp380_compensate_press(data, adc_press, t_fine);
> >  
> > -	return IIO_VAL_FRACTIONAL;
> > +	return 0;
> >  }
> >  
> >  static int bmp380_read_calib(struct bmp280_data *data)
> > @@ -1296,6 +1317,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,
> > @@ -1323,6 +1346,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,
> > @@ -1443,9 +1471,9 @@ 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;
> > +	s32 value_temp;
> >  	int ret;
> >  
> >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
> > @@ -1455,25 +1483,19 @@ 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) {
> > +	value_temp = get_unaligned_le24(data->buf);
> > +	if (value_temp == BMP580_TEMP_SKIPPED) {
> >  		dev_err(data->dev, "reading temperature skipped\n");
> >  		return -EIO;
> >  	}
> > +	*raw_temp = sign_extend32(value_temp, 23);
> >  

Here I use Adam's correction with sign_extend32()

> > -	/*
> > -	 * Temperature is returned in Celsius degrees in fractional
> > -	 * form down 2^16. We rescale by x1000 to return millidegrees
> > -	 * Celsius to respect IIO ABI.
> > -	 */
> > -	raw_temp = sign_extend32(raw_temp, 23);
> > -	*val = ((s64)raw_temp * 1000) / (1 << 16);
> > -	return IIO_VAL_INT;
> > +	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;
> > +	u32 value_press;
> >  	int ret;
> >  
> >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
> > @@ -1483,18 +1505,14 @@ 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) {
> > +	value_press = get_unaligned_le24(data->buf);
> > +	if (value_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;
> > +	*raw_press = value_press;
> > +
> > +	return 0;
> >  }
> >  
> >  static const int bmp580_odr_table[][2] = {
> > @@ -1830,6 +1848,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,
> > @@ -1856,6 +1876,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 = bmp580_temp_coeffs,
> > +	.temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2,
> > +	.press_coeffs = bmp580_press_coeffs,
> > +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> > +
> >  	.chip_config = bmp580_chip_config,
> >  	.read_temp = bmp580_read_temp,
> >  	.read_press = bmp580_read_press,
> > @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp)
> >  	return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16;
> >  }
> >  
> > -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 comp_temp;
> >  	u32 adc_temp;
> >  	int ret;
> >  
> > @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
> >  	if (ret)
> >  		return ret;
> >  
> > -	comp_temp = bmp180_compensate_temp(data, adc_temp);
> > +	*comp_temp = bmp180_compensate_temp(data, adc_temp);
> >  
> > -	*val = comp_temp * 100;
> > -	return IIO_VAL_INT;
> > +	return 0;
> >  }
> >  
> >  static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press)
> > @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 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, adc_press;
> > +	u32 adc_press;
> >  	s32 t_fine;
> >  	int ret;
> >  
> > @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2)
> >  	if (ret)
> >  		return ret;
> >  
> > -	comp_press = bmp180_compensate_press(data, adc_press, t_fine);
> > -
> > -	*val = comp_press;
> > -	*val2 = 1000;
> > +	*comp_press = bmp180_compensate_press(data, adc_press, t_fine);
> >  
> > -	return IIO_VAL_FRACTIONAL;
> > +	return 0;
> >  }
> >  
> >  static int bmp180_chip_config(struct bmp280_data *data)
> > @@ -2117,6 +2137,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,
> > @@ -2137,6 +2159,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 7c30e4d523be..a3d2cd722760 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -446,10 +446,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 *data);
> > -	int (*read_temp)(struct bmp280_data *data, int *val, int *val2);
> > -	int (*read_press)(struct bmp280_data *data, int *val, int *val2);
> > -	int (*read_humid)(struct bmp280_data *data, int *val, int *val2);
> > +	int (*read_temp)(struct bmp280_data *data, s32 *adc_temp);
> > +	int (*read_press)(struct bmp280_data *data, u32 *adc_press);
> > +	int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity);
> >  	int (*read_calib)(struct bmp280_data *data);
> >  	int (*preinit)(struct bmp280_data *data);
> >  };
>

Jonathan Cameron June 23, 2024, 4:23 p.m. UTC | #3

On Sat, 22 Jun 2024 14:19:18 +0200
Vasileios Amoiridis <[e-mail]> wrote:

> On Sat, Jun 22, 2024 at 10:28:26AM +0100, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2024 01:05:38 +0200
> > Vasileios Amoiridis <[e-mail]> wrote:
> >   
> > > Add the coefficients for the IIO standard units and the IIO value
> > > inside the chip_info structure.
> > > 
> > > Move the calculations for the IIO unit compatibility from inside the
> > > read_{temp,press,humid}() functions and move them to the general
> > > read_raw() 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 <[e-mail]>  
> > Does this incorporate the fix?  I'm a little confused looking at
> > what is visible here, so I'd like Adam to take a look.
> > 
> > Btw, you missed cc'ing Adam.
> >   
> 
> Ah, I only used the output of get_maintainer...

always be careful to sanity check that :)

> ...
>   
> > > @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(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] * (s64)chan_value;  
> 
> This is the first difference with the previous version where I incorporated
> the typecasting to (s64).

On a 32 bit platform that will then get pushed into a 32 bit int and overflow
I think.  Back when IIO got started everything was 32 bit so it didn't make sense
to make these 64 bit or indeed to worry about forcing the size.

Jonathan

Vasileios Amoiridis June 23, 2024, 5:18 p.m. UTC | #4

On Sun, Jun 23, 2024 at 05:23:30PM +0100, Jonathan Cameron wrote:
> On Sat, 22 Jun 2024 14:19:18 +0200
> Vasileios Amoiridis <[e-mail]> wrote:
> 
> > On Sat, Jun 22, 2024 at 10:28:26AM +0100, Jonathan Cameron wrote:
> > > On Tue, 18 Jun 2024 01:05:38 +0200
> > > Vasileios Amoiridis <[e-mail]> wrote:
> > >   
> > > > Add the coefficients for the IIO standard units and the IIO value
> > > > inside the chip_info structure.
> > > > 
> > > > Move the calculations for the IIO unit compatibility from inside the
> > > > read_{temp,press,humid}() functions and move them to the general
> > > > read_raw() 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 <[e-mail]>  
> > > Does this incorporate the fix?  I'm a little confused looking at
> > > what is visible here, so I'd like Adam to take a look.
> > > 
> > > Btw, you missed cc'ing Adam.
> > >   
> > 
> > Ah, I only used the output of get_maintainer...
> 
> always be careful to sanity check that :)
> 
> > ...
> >   
> > > > @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(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] * (s64)chan_value;  
> > 
> > This is the first difference with the previous version where I incorporated
> > the typecasting to (s64).
> 
> On a 32 bit platform that will then get pushed into a 32 bit int and overflow
> I think.  Back when IIO got started everything was 32 bit so it didn't make sense
> to make these 64 bit or indeed to worry about forcing the size.
> 
> Jonathan
> 

Well, I use a 32-bit platform, (BeagleBone Black) and the negative values are
handled gracefully with this code. Also, how is that different from the previous
code? Instead of doing the typecasting to (s64) in the bmp580_read_temp()
function, we do it here. I feel that it is the same piece of code, just in
different places. What I can do though, for your peace of mind is the following:

Since the problem with overflow comes when we multiply by 1000 in order to have
milli-Celsius, what I can do, is to "force" the division with 2^16 to happen
first. In this way, they divided value cannot be overflowed. The division will
happen inside the bmp580_read_temp() function and let the multiplication happen
later by the IIO code. Then we can drop the (s64) from here.

How does that sound to you?

Cheers,
Vasilis

Adam Rizkalla June 24, 2024, 4:26 a.m. UTC | #5

On Sat, Jun 22, 2024 at 02:19:18PM +0200, Vasileios Amoiridis wrote:
> On Sat, Jun 22, 2024 at 10:28:26AM +0100, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2024 01:05:38 +0200
> > Vasileios Amoiridis <[e-mail]> wrote:
> > 
> > > Add the coefficients for the IIO standard units and the IIO value
> > > inside the chip_info structure.
> > > 
> > > Move the calculations for the IIO unit compatibility from inside the
> > > read_{temp,press,humid}() functions and move them to the general
> > > read_raw() 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 <[e-mail]>
> > Does this incorporate the fix?  I'm a little confused looking at
> > what is visible here, so I'd like Adam to take a look.
> > 
> > Btw, you missed cc'ing Adam.
> > 
> 
> Ah, I only used the output of get_maintainer...
> 
> > > ---
> > >  drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------
> > >  drivers/iio/pressure/bmp280.h      |  13 ++-
> > >  2 files changed, 107 insertions(+), 73 deletions(-)
> > > 
> > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > > index 50d71ad83f37..27c00af060fa 100644
> > > --- a/drivers/iio/pressure/bmp280-core.c
> > > +++ b/drivers/iio/pressure/bmp280-core.c
> > > @@ -445,10 +445,8 @@ 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 comp_temp;
> > >  	u32 adc_temp;
> > >  	int ret;
> > >  
> > > @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	comp_temp = bmp280_compensate_temp(data, adc_temp);
> > > +	*comp_temp = bmp280_compensate_temp(data, adc_temp);
> > >  
> > > -	*val = comp_temp * 10;
> > > -	return IIO_VAL_INT;
> > > +	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, adc_press, t_fine;
> > > +	u32 adc_press;
> > > +	s32 t_fine;
> > >  	int ret;
> > >  
> > >  	ret = bmp280_get_t_fine(data, &t_fine);
> > > @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	comp_press = bmp280_compensate_press(data, adc_press, t_fine);
> > > -
> > > -	*val = comp_press;
> > > -	*val2 = 256000;
> > > +	*comp_press = bmp280_compensate_press(data, adc_press, t_fine);
> > >  
> > > -	return IIO_VAL_FRACTIONAL;
> > > +	return 0;
> > >  }
> > >  
> > > -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > > +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
> > >  {
> > > -	u32 comp_humidity;
> > >  	u16 adc_humidity;
> > >  	s32 t_fine;
> > >  	int ret;
> > > @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
> > > -
> > > -	*val = comp_humidity * 1000 / 1024;
> > > +	*comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
> > >  
> > > -	return IIO_VAL_INT;
> > > +	return 0;
> > >  }
> > >  
> > >  static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
> > > @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(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);
> > >
> 
> ...
>   
> > > @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(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] * (s64)chan_value;
> 
> This is the first difference with the previous version where I incorporated
> the typecasting to (s64).
> 
> > > +			*val2 = data->chip_info->temp_coeffs[1];
> > > +			return data->chip_info->temp_coeffs_type;
> > >  		default:
> > >  			return -EINVAL;
> > >  		}
> > > @@ -822,6 +833,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,
> > > @@ -850,6 +863,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,
> > > @@ -877,6 +895,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,
> > > @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = {
> > >  	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> > >  	.oversampling_humid_default = BME280_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,
> > > @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data,
> > >  	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;
> > >  
> > > @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	comp_temp = bmp380_compensate_temp(data, adc_temp);
> > > +	*comp_temp = bmp380_compensate_temp(data, adc_temp);
> > >  
> > > -	*val = comp_temp * 10;
> > > -	return IIO_VAL_INT;
> > > +	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)
> > >  {
> > > -	u32 adc_press, comp_press, t_fine;
> > > +	u32 adc_press, t_fine;
> > >  	int ret;
> > >  
> > >  	ret = bmp380_get_t_fine(data, &t_fine);
> > > @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	comp_press = bmp380_compensate_press(data, adc_press, t_fine);
> > > -
> > > -	*val = comp_press;
> > > -	*val2 = 100000;
> > > +	*comp_press = bmp380_compensate_press(data, adc_press, t_fine);
> > >  
> > > -	return IIO_VAL_FRACTIONAL;
> > > +	return 0;
> > >  }
> > >  
> > >  static int bmp380_read_calib(struct bmp280_data *data)
> > > @@ -1296,6 +1317,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,
> > > @@ -1323,6 +1346,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,
> > > @@ -1443,9 +1471,9 @@ 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;
> > > +	s32 value_temp;
> > >  	int ret;
> > >  
> > >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
> > > @@ -1455,25 +1483,19 @@ 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) {
> > > +	value_temp = get_unaligned_le24(data->buf);
> > > +	if (value_temp == BMP580_TEMP_SKIPPED) {
> > >  		dev_err(data->dev, "reading temperature skipped\n");
> > >  		return -EIO;
> > >  	}
> > > +	*raw_temp = sign_extend32(value_temp, 23);
> > >  
> 
> Here I use Adam's correction with sign_extend32()
> 
> > > -	/*
> > > -	 * Temperature is returned in Celsius degrees in fractional
> > > -	 * form down 2^16. We rescale by x1000 to return millidegrees
> > > -	 * Celsius to respect IIO ABI.
> > > -	 */
> > > -	raw_temp = sign_extend32(raw_temp, 23);
> > > -	*val = ((s64)raw_temp * 1000) / (1 << 16);
> > > -	return IIO_VAL_INT;
> > > +	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;
> > > +	u32 value_press;
> > >  	int ret;
> > >  
> > >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
> > > @@ -1483,18 +1505,14 @@ 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) {
> > > +	value_press = get_unaligned_le24(data->buf);
> > > +	if (value_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;
> > > +	*raw_press = value_press;
> > > +
> > > +	return 0;
> > >  }
> > >  
> > >  static const int bmp580_odr_table[][2] = {
> > > @@ -1830,6 +1848,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 };

I believe we can do something similar to what Jonathan mentioned in [1]
to avoid using a 64-bit signed int by changing these coefficients to
something like:

static const int bmp580_temp_coeffs[] = { 250, 14 };

This would allow the result for the max temperature value (85) to still
fit in a 32-bit signed integer and would give a bit more resolution in
the result. The bit shift of 14 would achieve the same effect with the
new scaling.

85 * 2^16 * 250 = 1,392,640,000 (can be stored as a s32)

Then, 1,392,640,000 / 2^14 = 85000


[1] https://lore.kernel.org/all/[e-mail]/

> > > +static const int bmp580_press_coeffs[] = { 1, 64000};
> > >  
> > >  const struct bmp280_chip_info bmp580_chip_info = {
> > >  	.id_reg = BMP580_REG_CHIP_ID,
> > > @@ -1856,6 +1876,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 = bmp580_temp_coeffs,
> > > +	.temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2,
> > > +	.press_coeffs = bmp580_press_coeffs,
> > > +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> > > +
> > >  	.chip_config = bmp580_chip_config,
> > >  	.read_temp = bmp580_read_temp,
> > >  	.read_press = bmp580_read_press,
> > > @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp)
> > >  	return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16;
> > >  }
> > >  
> > > -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 comp_temp;
> > >  	u32 adc_temp;
> > >  	int ret;
> > >  
> > > @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	comp_temp = bmp180_compensate_temp(data, adc_temp);
> > > +	*comp_temp = bmp180_compensate_temp(data, adc_temp);
> > >  
> > > -	*val = comp_temp * 100;
> > > -	return IIO_VAL_INT;
> > > +	return 0;
> > >  }
> > >  
> > >  static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press)
> > > @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 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, adc_press;
> > > +	u32 adc_press;
> > >  	s32 t_fine;
> > >  	int ret;
> > >  
> > > @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	comp_press = bmp180_compensate_press(data, adc_press, t_fine);
> > > -
> > > -	*val = comp_press;
> > > -	*val2 = 1000;
> > > +	*comp_press = bmp180_compensate_press(data, adc_press, t_fine);
> > >  
> > > -	return IIO_VAL_FRACTIONAL;
> > > +	return 0;
> > >  }
> > >  
> > >  static int bmp180_chip_config(struct bmp280_data *data)
> > > @@ -2117,6 +2137,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,
> > > @@ -2137,6 +2159,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 7c30e4d523be..a3d2cd722760 100644
> > > --- a/drivers/iio/pressure/bmp280.h
> > > +++ b/drivers/iio/pressure/bmp280.h
> > > @@ -446,10 +446,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 *data);
> > > -	int (*read_temp)(struct bmp280_data *data, int *val, int *val2);
> > > -	int (*read_press)(struct bmp280_data *data, int *val, int *val2);
> > > -	int (*read_humid)(struct bmp280_data *data, int *val, int *val2);
> > > +	int (*read_temp)(struct bmp280_data *data, s32 *adc_temp);
> > > +	int (*read_press)(struct bmp280_data *data, u32 *adc_press);
> > > +	int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity);
> > >  	int (*read_calib)(struct bmp280_data *data);
> > >  	int (*preinit)(struct bmp280_data *data);
> > >  };
> >

Jonathan Cameron June 25, 2024, 8:44 p.m. UTC | #6

On Sun, 23 Jun 2024 19:18:41 +0200
Vasileios Amoiridis <[e-mail]> wrote:

> On Sun, Jun 23, 2024 at 05:23:30PM +0100, Jonathan Cameron wrote:
> > On Sat, 22 Jun 2024 14:19:18 +0200
> > Vasileios Amoiridis <[e-mail]> wrote:
> >   
> > > On Sat, Jun 22, 2024 at 10:28:26AM +0100, Jonathan Cameron wrote:  
> > > > On Tue, 18 Jun 2024 01:05:38 +0200
> > > > Vasileios Amoiridis <[e-mail]> wrote:
> > > >     
> > > > > Add the coefficients for the IIO standard units and the IIO value
> > > > > inside the chip_info structure.
> > > > > 
> > > > > Move the calculations for the IIO unit compatibility from inside the
> > > > > read_{temp,press,humid}() functions and move them to the general
> > > > > read_raw() 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 <[e-mail]>    
> > > > Does this incorporate the fix?  I'm a little confused looking at
> > > > what is visible here, so I'd like Adam to take a look.
> > > > 
> > > > Btw, you missed cc'ing Adam.
> > > >     
> > > 
> > > Ah, I only used the output of get_maintainer...  
> > 
> > always be careful to sanity check that :)
> >   
> > > ...
> > >     
> > > > > @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(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] * (s64)chan_value;    
> > > 
> > > This is the first difference with the previous version where I incorporated
> > > the typecasting to (s64).  
> > 
> > On a 32 bit platform that will then get pushed into a 32 bit int and overflow
> > I think.  Back when IIO got started everything was 32 bit so it didn't make sense
> > to make these 64 bit or indeed to worry about forcing the size.
> > 
> > Jonathan
> >   
> 
> Well, I use a 32-bit platform, (BeagleBone Black) and the negative values are
> handled gracefully with this code. Also, how is that different from the previous
> code? Instead of doing the typecasting to (s64) in the bmp580_read_temp()
> function, we do it here. I feel that it is the same piece of code, just in
> different places.

The problem, I think, is the storage between those two places is too small.
It's not about negatives, but rather use that in the extremely large value
case it might not fit in the 32 bit int behind val so casting that back up
to 64 bits later won't recover the lost most significant bits.

> What I can do though, for your peace of mind is the following:
> 
> Since the problem with overflow comes when we multiply by 1000 in order to have
> milli-Celsius, what I can do, is to "force" the division with 2^16 to happen
> first. In this way, they divided value cannot be overflowed. The division will
> happen inside the bmp580_read_temp() function and let the multiplication happen
> later by the IIO code. Then we can drop the (s64) from here.

See Adam's reply - that should avoid overflow and preserve accuracy which you
tend to loose with divide then multiply.

Jonathan

> 
> How does that sound to you?
> 
> Cheers,
> Vasilis

Vasileios Amoiridis June 26, 2024, 1:38 p.m. UTC | #7

On Sun, Jun 23, 2024 at 11:26:23PM -0500, Adam Rizkalla wrote:
> On Sat, Jun 22, 2024 at 02:19:18PM +0200, Vasileios Amoiridis wrote:
> > On Sat, Jun 22, 2024 at 10:28:26AM +0100, Jonathan Cameron wrote:
> > > On Tue, 18 Jun 2024 01:05:38 +0200
> > > Vasileios Amoiridis <[e-mail]> wrote:
> > > 
> > > > Add the coefficients for the IIO standard units and the IIO value
> > > > inside the chip_info structure.
> > > > 
> > > > Move the calculations for the IIO unit compatibility from inside the
> > > > read_{temp,press,humid}() functions and move them to the general
> > > > read_raw() 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 <[e-mail]>
> > > Does this incorporate the fix?  I'm a little confused looking at
> > > what is visible here, so I'd like Adam to take a look.
> > > 
> > > Btw, you missed cc'ing Adam.
> > > 
> > 
> > Ah, I only used the output of get_maintainer...
> > 
> > > > ---
> > > >  drivers/iio/pressure/bmp280-core.c | 167 +++++++++++++++++------------
> > > >  drivers/iio/pressure/bmp280.h      |  13 ++-
> > > >  2 files changed, 107 insertions(+), 73 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > > > index 50d71ad83f37..27c00af060fa 100644
> > > > --- a/drivers/iio/pressure/bmp280-core.c
> > > > +++ b/drivers/iio/pressure/bmp280-core.c
> > > > @@ -445,10 +445,8 @@ 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 comp_temp;
> > > >  	u32 adc_temp;
> > > >  	int ret;
> > > >  
> > > > @@ -456,16 +454,15 @@ static int bmp280_read_temp(struct bmp280_data *data,
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	comp_temp = bmp280_compensate_temp(data, adc_temp);
> > > > +	*comp_temp = bmp280_compensate_temp(data, adc_temp);
> > > >  
> > > > -	*val = comp_temp * 10;
> > > > -	return IIO_VAL_INT;
> > > > +	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, adc_press, t_fine;
> > > > +	u32 adc_press;
> > > > +	s32 t_fine;
> > > >  	int ret;
> > > >  
> > > >  	ret = bmp280_get_t_fine(data, &t_fine);
> > > > @@ -476,17 +473,13 @@ static int bmp280_read_press(struct bmp280_data *data,
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	comp_press = bmp280_compensate_press(data, adc_press, t_fine);
> > > > -
> > > > -	*val = comp_press;
> > > > -	*val2 = 256000;
> > > > +	*comp_press = bmp280_compensate_press(data, adc_press, t_fine);
> > > >  
> > > > -	return IIO_VAL_FRACTIONAL;
> > > > +	return 0;
> > > >  }
> > > >  
> > > > -static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > > > +static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
> > > >  {
> > > > -	u32 comp_humidity;
> > > >  	u16 adc_humidity;
> > > >  	s32 t_fine;
> > > >  	int ret;
> > > > @@ -499,11 +492,9 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
> > > > -
> > > > -	*val = comp_humidity * 1000 / 1024;
> > > > +	*comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
> > > >  
> > > > -	return IIO_VAL_INT;
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
> > > > @@ -511,6 +502,8 @@ static int bmp280_read_raw_impl(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);
> > > >
> > 
> > ...
> >   
> > > > @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(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] * (s64)chan_value;
> > 
> > This is the first difference with the previous version where I incorporated
> > the typecasting to (s64).
> > 
> > > > +			*val2 = data->chip_info->temp_coeffs[1];
> > > > +			return data->chip_info->temp_coeffs_type;
> > > >  		default:
> > > >  			return -EINVAL;
> > > >  		}
> > > > @@ -822,6 +833,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,
> > > > @@ -850,6 +863,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,
> > > > @@ -877,6 +895,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,
> > > > @@ -899,6 +918,13 @@ const struct bmp280_chip_info bme280_chip_info = {
> > > >  	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> > > >  	.oversampling_humid_default = BME280_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,
> > > > @@ -1091,9 +1117,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data,
> > > >  	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;
> > > >  
> > > > @@ -1101,15 +1126,14 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	comp_temp = bmp380_compensate_temp(data, adc_temp);
> > > > +	*comp_temp = bmp380_compensate_temp(data, adc_temp);
> > > >  
> > > > -	*val = comp_temp * 10;
> > > > -	return IIO_VAL_INT;
> > > > +	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)
> > > >  {
> > > > -	u32 adc_press, comp_press, t_fine;
> > > > +	u32 adc_press, t_fine;
> > > >  	int ret;
> > > >  
> > > >  	ret = bmp380_get_t_fine(data, &t_fine);
> > > > @@ -1120,12 +1144,9 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	comp_press = bmp380_compensate_press(data, adc_press, t_fine);
> > > > -
> > > > -	*val = comp_press;
> > > > -	*val2 = 100000;
> > > > +	*comp_press = bmp380_compensate_press(data, adc_press, t_fine);
> > > >  
> > > > -	return IIO_VAL_FRACTIONAL;
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  static int bmp380_read_calib(struct bmp280_data *data)
> > > > @@ -1296,6 +1317,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,
> > > > @@ -1323,6 +1346,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,
> > > > @@ -1443,9 +1471,9 @@ 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;
> > > > +	s32 value_temp;
> > > >  	int ret;
> > > >  
> > > >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
> > > > @@ -1455,25 +1483,19 @@ 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) {
> > > > +	value_temp = get_unaligned_le24(data->buf);
> > > > +	if (value_temp == BMP580_TEMP_SKIPPED) {
> > > >  		dev_err(data->dev, "reading temperature skipped\n");
> > > >  		return -EIO;
> > > >  	}
> > > > +	*raw_temp = sign_extend32(value_temp, 23);
> > > >  
> > 
> > Here I use Adam's correction with sign_extend32()
> > 
> > > > -	/*
> > > > -	 * Temperature is returned in Celsius degrees in fractional
> > > > -	 * form down 2^16. We rescale by x1000 to return millidegrees
> > > > -	 * Celsius to respect IIO ABI.
> > > > -	 */
> > > > -	raw_temp = sign_extend32(raw_temp, 23);
> > > > -	*val = ((s64)raw_temp * 1000) / (1 << 16);
> > > > -	return IIO_VAL_INT;
> > > > +	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;
> > > > +	u32 value_press;
> > > >  	int ret;
> > > >  
> > > >  	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
> > > > @@ -1483,18 +1505,14 @@ 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) {
> > > > +	value_press = get_unaligned_le24(data->buf);
> > > > +	if (value_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;
> > > > +	*raw_press = value_press;
> > > > +
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  static const int bmp580_odr_table[][2] = {
> > > > @@ -1830,6 +1848,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 };
> 
> I believe we can do something similar to what Jonathan mentioned in [1]
> to avoid using a 64-bit signed int by changing these coefficients to
> something like:
> 
> static const int bmp580_temp_coeffs[] = { 250, 14 };
> 
> This would allow the result for the max temperature value (85) to still
> fit in a 32-bit signed integer and would give a bit more resolution in
> the result. The bit shift of 14 would achieve the same effect with the
> new scaling.
> 
> 85 * 2^16 * 250 = 1,392,640,000 (can be stored as a s32)
> 
> Then, 1,392,640,000 / 2^14 = 85000
> 
> 
> [1] https://lore.kernel.org/all/[e-mail]/
> 

Hi Adam,

This is such a good and simple idea. I don't know how it didn't even
came up to my mind. I could even push a bit further to go to 125 and 19 maybe.
Thanks a lot for the feedback.

Cheers,
Vasilis

> > > > +static const int bmp580_press_coeffs[] = { 1, 64000};
> > > >  
> > > >  const struct bmp280_chip_info bmp580_chip_info = {
> > > >  	.id_reg = BMP580_REG_CHIP_ID,
> > > > @@ -1856,6 +1876,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 = bmp580_temp_coeffs,
> > > > +	.temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2,
> > > > +	.press_coeffs = bmp580_press_coeffs,
> > > > +	.press_coeffs_type = IIO_VAL_FRACTIONAL,
> > > > +
> > > >  	.chip_config = bmp580_chip_config,
> > > >  	.read_temp = bmp580_read_temp,
> > > >  	.read_press = bmp580_read_press,
> > > > @@ -2011,9 +2036,8 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp)
> > > >  	return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16;
> > > >  }
> > > >  
> > > > -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 comp_temp;
> > > >  	u32 adc_temp;
> > > >  	int ret;
> > > >  
> > > > @@ -2021,10 +2045,9 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	comp_temp = bmp180_compensate_temp(data, adc_temp);
> > > > +	*comp_temp = bmp180_compensate_temp(data, adc_temp);
> > > >  
> > > > -	*val = comp_temp * 100;
> > > > -	return IIO_VAL_INT;
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press)
> > > > @@ -2087,9 +2110,9 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 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, adc_press;
> > > > +	u32 adc_press;
> > > >  	s32 t_fine;
> > > >  	int ret;
> > > >  
> > > > @@ -2101,12 +2124,9 @@ static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	comp_press = bmp180_compensate_press(data, adc_press, t_fine);
> > > > -
> > > > -	*val = comp_press;
> > > > -	*val2 = 1000;
> > > > +	*comp_press = bmp180_compensate_press(data, adc_press, t_fine);
> > > >  
> > > > -	return IIO_VAL_FRACTIONAL;
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  static int bmp180_chip_config(struct bmp280_data *data)
> > > > @@ -2117,6 +2137,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,
> > > > @@ -2137,6 +2159,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 7c30e4d523be..a3d2cd722760 100644
> > > > --- a/drivers/iio/pressure/bmp280.h
> > > > +++ b/drivers/iio/pressure/bmp280.h
> > > > @@ -446,10 +446,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 *data);
> > > > -	int (*read_temp)(struct bmp280_data *data, int *val, int *val2);
> > > > -	int (*read_press)(struct bmp280_data *data, int *val, int *val2);
> > > > -	int (*read_humid)(struct bmp280_data *data, int *val, int *val2);
> > > > +	int (*read_temp)(struct bmp280_data *data, s32 *adc_temp);
> > > > +	int (*read_press)(struct bmp280_data *data, u32 *adc_press);
> > > > +	int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity);
> > > >  	int (*read_calib)(struct bmp280_data *data);
> > > >  	int (*preinit)(struct bmp280_data *data);
> > > >  };
> > >

Vasileios Amoiridis June 26, 2024, 1:41 p.m. UTC | #8

On Tue, Jun 25, 2024 at 09:44:39PM +0100, Jonathan Cameron wrote:
> On Sun, 23 Jun 2024 19:18:41 +0200
> Vasileios Amoiridis <[e-mail]> wrote:
> 
> > On Sun, Jun 23, 2024 at 05:23:30PM +0100, Jonathan Cameron wrote:
> > > On Sat, 22 Jun 2024 14:19:18 +0200
> > > Vasileios Amoiridis <[e-mail]> wrote:
> > >   
> > > > On Sat, Jun 22, 2024 at 10:28:26AM +0100, Jonathan Cameron wrote:  
> > > > > On Tue, 18 Jun 2024 01:05:38 +0200
> > > > > Vasileios Amoiridis <[e-mail]> wrote:
> > > > >     
> > > > > > Add the coefficients for the IIO standard units and the IIO value
> > > > > > inside the chip_info structure.
> > > > > > 
> > > > > > Move the calculations for the IIO unit compatibility from inside the
> > > > > > read_{temp,press,humid}() functions and move them to the general
> > > > > > read_raw() 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 <[e-mail]>    
> > > > > Does this incorporate the fix?  I'm a little confused looking at
> > > > > what is visible here, so I'd like Adam to take a look.
> > > > > 
> > > > > Btw, you missed cc'ing Adam.
> > > > >     
> > > > 
> > > > Ah, I only used the output of get_maintainer...  
> > > 
> > > always be careful to sanity check that :)
> > >   
> > > > ...
> > > >     
> > > > > > @@ -518,11 +511,29 @@ static int bmp280_read_raw_impl(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] * (s64)chan_value;    
> > > > 
> > > > This is the first difference with the previous version where I incorporated
> > > > the typecasting to (s64).  
> > > 
> > > On a 32 bit platform that will then get pushed into a 32 bit int and overflow
> > > I think.  Back when IIO got started everything was 32 bit so it didn't make sense
> > > to make these 64 bit or indeed to worry about forcing the size.
> > > 
> > > Jonathan
> > >   
> > 
> > Well, I use a 32-bit platform, (BeagleBone Black) and the negative values are
> > handled gracefully with this code. Also, how is that different from the previous
> > code? Instead of doing the typecasting to (s64) in the bmp580_read_temp()
> > function, we do it here. I feel that it is the same piece of code, just in
> > different places.
> 
> The problem, I think, is the storage between those two places is too small.
> It's not about negatives, but rather use that in the extremely large value
> case it might not fit in the 32 bit int behind val so casting that back up
> to 64 bits later won't recover the lost most significant bits.
> 

Well, that is tricky, I didn't think about it. Thanks for noticing.

> > What I can do though, for your peace of mind is the following:
> > 
> > Since the problem with overflow comes when we multiply by 1000 in order to have
> > milli-Celsius, what I can do, is to "force" the division with 2^16 to happen
> > first. In this way, they divided value cannot be overflowed. The division will
> > happen inside the bmp580_read_temp() function and let the multiplication happen
> > later by the IIO code. Then we can drop the (s64) from here.
> 
> See Adam's reply - that should avoid overflow and preserve accuracy which you
> tend to loose with divide then multiply.
> 
> Jonathan
> 

I saw it, and I will apply it, thanks to both of you!

Cheers,
Vasilis

Patch

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 50d71ad83f37..27c00af060fa 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -445,10 +445,8 @@  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 comp_temp;
 	u32 adc_temp;
 	int ret;
 
@@ -456,16 +454,15 @@  static int bmp280_read_temp(struct bmp280_data *data,
 	if (ret)
 		return ret;
 
-	comp_temp = bmp280_compensate_temp(data, adc_temp);
+	*comp_temp = bmp280_compensate_temp(data, adc_temp);
 
-	*val = comp_temp * 10;
-	return IIO_VAL_INT;
+	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, adc_press, t_fine;
+	u32 adc_press;
+	s32 t_fine;
 	int ret;
 
 	ret = bmp280_get_t_fine(data, &t_fine);
@@ -476,17 +473,13 @@  static int bmp280_read_press(struct bmp280_data *data,
 	if (ret)
 		return ret;
 
-	comp_press = bmp280_compensate_press(data, adc_press, t_fine);
-
-	*val = comp_press;
-	*val2 = 256000;
+	*comp_press = bmp280_compensate_press(data, adc_press, t_fine);
 
-	return IIO_VAL_FRACTIONAL;
+	return 0;
 }
 
-static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
+static int bme280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
 {
-	u32 comp_humidity;
 	u16 adc_humidity;
 	s32 t_fine;
 	int ret;
@@ -499,11 +492,9 @@  static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
 	if (ret)
 		return ret;
 
-	comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
-
-	*val = comp_humidity * 1000 / 1024;
+	*comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine);
 
-	return IIO_VAL_INT;
+	return 0;
 }
 
 static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
@@ -511,6 +502,8 @@  static int bmp280_read_raw_impl(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);
 
@@ -518,11 +511,29 @@  static int bmp280_read_raw_impl(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] * (s64)chan_value;
+			*val2 = data->chip_info->temp_coeffs[1];
+			return data->chip_info->temp_coeffs_type;
 		default:
 			return -EINVAL;
 		}
@@ -822,6 +833,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,
@@ -850,6 +863,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,
@@ -877,6 +895,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,
@@ -899,6 +918,13 @@  const struct bmp280_chip_info bme280_chip_info = {
 	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
 	.oversampling_humid_default = BME280_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,
@@ -1091,9 +1117,8 @@  static u32 bmp380_compensate_press(struct bmp280_data *data,
 	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;
 
@@ -1101,15 +1126,14 @@  static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
 	if (ret)
 		return ret;
 
-	comp_temp = bmp380_compensate_temp(data, adc_temp);
+	*comp_temp = bmp380_compensate_temp(data, adc_temp);
 
-	*val = comp_temp * 10;
-	return IIO_VAL_INT;
+	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)
 {
-	u32 adc_press, comp_press, t_fine;
+	u32 adc_press, t_fine;
 	int ret;
 
 	ret = bmp380_get_t_fine(data, &t_fine);
@@ -1120,12 +1144,9 @@  static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
 	if (ret)
 		return ret;
 
-	comp_press = bmp380_compensate_press(data, adc_press, t_fine);
-
-	*val = comp_press;
-	*val2 = 100000;
+	*comp_press = bmp380_compensate_press(data, adc_press, t_fine);
 
-	return IIO_VAL_FRACTIONAL;
+	return 0;
 }
 
 static int bmp380_read_calib(struct bmp280_data *data)
@@ -1296,6 +1317,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,
@@ -1323,6 +1346,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,
@@ -1443,9 +1471,9 @@  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;
+	s32 value_temp;
 	int ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
@@ -1455,25 +1483,19 @@  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) {
+	value_temp = get_unaligned_le24(data->buf);
+	if (value_temp == BMP580_TEMP_SKIPPED) {
 		dev_err(data->dev, "reading temperature skipped\n");
 		return -EIO;
 	}
+	*raw_temp = sign_extend32(value_temp, 23);
 
-	/*
-	 * Temperature is returned in Celsius degrees in fractional
-	 * form down 2^16. We rescale by x1000 to return millidegrees
-	 * Celsius to respect IIO ABI.
-	 */
-	raw_temp = sign_extend32(raw_temp, 23);
-	*val = ((s64)raw_temp * 1000) / (1 << 16);
-	return IIO_VAL_INT;
+	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;
+	u32 value_press;
 	int ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf,
@@ -1483,18 +1505,14 @@  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) {
+	value_press = get_unaligned_le24(data->buf);
+	if (value_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;
+	*raw_press = value_press;
+
+	return 0;
 }
 
 static const int bmp580_odr_table[][2] = {
@@ -1830,6 +1848,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,
@@ -1856,6 +1876,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 = bmp580_temp_coeffs,
+	.temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2,
+	.press_coeffs = bmp580_press_coeffs,
+	.press_coeffs_type = IIO_VAL_FRACTIONAL,
+
 	.chip_config = bmp580_chip_config,
 	.read_temp = bmp580_read_temp,
 	.read_press = bmp580_read_press,
@@ -2011,9 +2036,8 @@  static s32 bmp180_compensate_temp(struct bmp280_data *data, u32 adc_temp)
 	return (bmp180_calc_t_fine(data, adc_temp) + 8) / 16;
 }
 
-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 comp_temp;
 	u32 adc_temp;
 	int ret;
 
@@ -2021,10 +2045,9 @@  static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2)
 	if (ret)
 		return ret;
 
-	comp_temp = bmp180_compensate_temp(data, adc_temp);
+	*comp_temp = bmp180_compensate_temp(data, adc_temp);
 
-	*val = comp_temp * 100;
-	return IIO_VAL_INT;
+	return 0;
 }
 
 static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press)
@@ -2087,9 +2110,9 @@  static u32 bmp180_compensate_press(struct bmp280_data *data, u32 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, adc_press;
+	u32 adc_press;
 	s32 t_fine;
 	int ret;
 
@@ -2101,12 +2124,9 @@  static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2)
 	if (ret)
 		return ret;
 
-	comp_press = bmp180_compensate_press(data, adc_press, t_fine);
-
-	*val = comp_press;
-	*val2 = 1000;
+	*comp_press = bmp180_compensate_press(data, adc_press, t_fine);
 
-	return IIO_VAL_FRACTIONAL;
+	return 0;
 }
 
 static int bmp180_chip_config(struct bmp280_data *data)
@@ -2117,6 +2137,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,
@@ -2137,6 +2159,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 7c30e4d523be..a3d2cd722760 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -446,10 +446,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 *data);
-	int (*read_temp)(struct bmp280_data *data, int *val, int *val2);
-	int (*read_press)(struct bmp280_data *data, int *val, int *val2);
-	int (*read_humid)(struct bmp280_data *data, int *val, int *val2);
+	int (*read_temp)(struct bmp280_data *data, s32 *adc_temp);
+	int (*read_press)(struct bmp280_data *data, u32 *adc_press);
+	int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity);
 	int (*read_calib)(struct bmp280_data *data);
 	int (*preinit)(struct bmp280_data *data);
 };