| Message ID | 20240429190046.24252-7-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: pressure: bmp280: Driver cleanup and add triggered buffer support | expand |
On Mon, 29 Apr 2024 21:00:42 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > For BMP18x, BMP28x, BME280, BMP38x the reading of the pressure > value requires an update of the t_fine variable which happens > through reading the temperature value. > > So all the bmpxxx_read_press() functions of the above sensors > are internally calling the equivalent bmpxxx_read_temp() function > in order to update the t_fine value. By just looking at the code > this functionality is a bit hidden and is not easy to understand > why those channels are not independent. > > This commit tries to clear these things a bit by splitting the > bmpxxx_{read/compensate}_{temp/press/humid}() to the following: > > i. bmpxxx_read_{temp/press/humid}_adc(): read the raw value from > the sensor. > > ii. bmpxx_calc_t_fine(): calculate the t_fine variable. > > iii. bmpxxx_get_t_fine(): get the t_fine variable. > > iv. bmpxxx_compensate_{temp/press/humid}(): compensate the adc > values and return the calculated value. > > v. bmpxxx_read_{temp/press/humid}(): combine calls of the > aforementioned functions to return the requested value. > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> In general looks good, but a few details to consider inline. Jonathan > --- > drivers/iio/pressure/bmp280-core.c | 351 ++++++++++++++++++----------- > drivers/iio/pressure/bmp280.h | 6 - > 2 files changed, 221 insertions(+), 136 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index 8290028824e9..5ebce38e99f6 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -288,13 +288,33 @@ static int bme280_read_calib(struct bmp280_data *data) > * > * Taken from BME280 datasheet, Section 4.2.3, "Compensation formula". > */ > +static int bme280_read_humid_adc(struct bmp280_data *data, s32 *adc_humidity) It's an u16, so why use an s32? I can see using a signed value avoids a cast later, but it makes this more confusing to read, so I'd push that cast up to the user. > +{ > + int ret; > + > + ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, > + &data->be16, sizeof(data->be16)); > + if (ret < 0) { > + dev_err(data->dev, "failed to read humidity\n"); > + return ret; > + } > + > + *adc_humidity = be16_to_cpu(data->be16); Trivial, but on error return we normally aim for side effect free. To do that here use an internal variable first. s16 value; ... value = be16_to_cpu(data->be16) if (value == BMP280_HUMIDITY_SKIPPED) { dev_err(data->dev, "... return -EIO; } This is the odd bit due to using an s32 to store a u16. Have to rely on that size mismatch to avoid the sign accidentally mattering. Which is ugly! *adc_humidity = value; return 0; > + if (*adc_humidity == BMP280_HUMIDITY_SKIPPED) { > + dev_err(data->dev, "reading humidity skipped\n"); > + return -EIO; > + } > + > + return 0; > +} > + > static u32 bme280_compensate_humidity(struct bmp280_data *data, > - s32 adc_humidity) > + s32 adc_humidity, s32 t_fine) > { > struct bmp280_calib *calib = &data->calib.bmp280; > s32 var; > > - var = ((s32)data->t_fine) - (s32)76800; > + var = ((s32)t_fine) - (s32)76800; Casting an s32 to an s32. For the const it shouldn't matter as it'll be at least 32 bits and we don't care about the sign. > var = ((((adc_humidity << 14) - (calib->H4 << 20) - (calib->H5 * var)) > + (s32)16384) >> 15) * (((((((var * calib->H6) >> 10) > * (((var * (s32)calib->H3) >> 11) + (s32)32768)) >> 10) > @@ -313,8 +333,27 @@ static u32 bme280_compensate_humidity(struct bmp280_data *data, > * > * Taken from datasheet, Section 3.11.3, "Compensation formula". > */ > -static s32 bmp280_compensate_temp(struct bmp280_data *data, > - s32 adc_temp) > +static int bmp280_read_temp_adc(struct bmp280_data *data, s32 *adc_temp) As before, sign of the extra variable is confusing. It's not signed as it's a raw ADC value. So I'd use a u32 for it. > +{ > + int ret; > + > + ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > + data->buf, sizeof(data->buf)); > + if (ret < 0) { > + dev_err(data->dev, "failed to read temperature\n"); > + return ret; > + } > + > + *adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); > + if (*adc_temp == BMP280_TEMP_SKIPPED) { > + dev_err(data->dev, "reading temperature skipped\n"); > + return -EIO; Same thing on side effects. Best to avoid them if it is easy to do (like here!) > + } > + > + return 0; > +} > + > +static s32 bmp280_calc_t_fine(struct bmp280_data *data, s32 adc_temp) > { > struct bmp280_calib *calib = &data->calib.bmp280; > s32 var1, var2; > @@ -324,9 +363,26 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data, > var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) * > ((adc_temp >> 4) - ((s32)calib->T1))) >> 12) * > ((s32)calib->T3)) >> 14; > - data->t_fine = var1 + var2; > + return var1 + var2; /* t_fine = var1 + var2 */ > +} > + > +static int bmp280_get_t_fine(struct bmp280_data *data, s32 *t_fine) > +{ > + s32 adc_temp; > + int ret; > + > + ret = bmp280_read_temp_adc(data, &adc_temp); > + if (ret < 0) > + return ret; > + > + *t_fine = bmp280_calc_t_fine(data, adc_temp); > > - return (data->t_fine * 5 + 128) >> 8; > + return 0; > +} > + > +static s32 bmp280_compensate_temp(struct bmp280_data *data, s32 adc_temp) > +{ > + return (bmp280_calc_t_fine(data, adc_temp) * 5 + 128) / 256; > } > > /* > @@ -336,13 +392,33 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data, > * > * Taken from datasheet, Section 3.11.3, "Compensation formula". > */ > +static int bmp280_read_press_adc(struct bmp280_data *data, s32 *adc_press) > +{ > + int ret; > + > + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > + data->buf, sizeof(data->buf)); > + if (ret < 0) { > + dev_err(data->dev, "failed to read pressure\n"); > + return ret; > + } > + > + *adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); > + if (*adc_press == BMP280_PRESS_SKIPPED) { > + dev_err(data->dev, "reading pressure skipped\n"); > + return -EIO; As above; avoid side effects. > + } > + > + return 0; > +} > + > static u32 bmp280_compensate_press(struct bmp280_data *data, > - s32 adc_press) > + s32 adc_press, s32 t_fine) > { > struct bmp280_calib *calib = &data->calib.bmp280; > s64 var1, var2, p; > > - var1 = ((s64)data->t_fine) - 128000; > + var1 = ((s64)t_fine) - 128000; > var2 = var1 * var1 * (s64)calib->P6; > var2 += (var1 * (s64)calib->P5) << 17; > var2 += ((s64)calib->P4) << 35; > @@ -368,60 +444,35 @@ static int bmp280_read_temp(struct bmp280_data *data, > s32 adc_temp, comp_temp; > int ret; > > - ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > - data->buf, sizeof(data->buf)); > - if (ret < 0) { > - dev_err(data->dev, "failed to read temperature\n"); > + ret = bmp280_read_temp_adc(data, &adc_temp); > + if (ret < 0) > return ret; > - } > > - adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); > - if (adc_temp == BMP280_TEMP_SKIPPED) { > - /* reading was skipped */ > - dev_err(data->dev, "reading temperature skipped\n"); > - return -EIO; > - } > comp_temp = bmp280_compensate_temp(data, adc_temp); > > - /* > - * val might be NULL if we're called by the read_press routine, > - * who only cares about the carry over t_fine value. > - */ > - if (val) { > - *val = comp_temp * 10; > - return IIO_VAL_INT; > - } > - > - return 0; > + /* IIO units are in milli Celsius */ > + *val = comp_temp * 10; > + return IIO_VAL_INT; > } > > static int bmp280_read_press(struct bmp280_data *data, > int *val, int *val2) > { > + s32 adc_press, t_fine; > u32 comp_press; > - s32 adc_press; > int ret; > > - /* Read and compensate temperature so we get a reading of t_fine. */ > - ret = bmp280_read_temp(data, NULL, NULL); > + ret = bmp280_get_t_fine(data, &t_fine); > if (ret < 0) > return ret; > > - ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > - data->buf, sizeof(data->buf)); > - if (ret < 0) { > - dev_err(data->dev, "failed to read pressure\n"); > + ret = bmp280_read_press_adc(data, &adc_press); > + if (ret < 0) > return ret; > - } > > - adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); > - if (adc_press == BMP280_PRESS_SKIPPED) { > - /* reading was skipped */ > - dev_err(data->dev, "reading pressure skipped\n"); > - return -EIO; > - } > - comp_press = bmp280_compensate_press(data, adc_press); > + comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > + /* IIO units are in kPa */ > *val = comp_press; > *val2 = 256000; > > @@ -430,30 +481,21 @@ static int bmp280_read_press(struct bmp280_data *data, > > static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > { > + s32 adc_humidity, t_fine; > u32 comp_humidity; > - s32 adc_humidity; > int ret; > > - /* Read and compensate temperature so we get a reading of t_fine. */ > - ret = bmp280_read_temp(data, NULL, NULL); > + ret = bmp280_get_t_fine(data, &t_fine); > if (ret < 0) > return ret; > > - ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, > - &data->be16, sizeof(data->be16)); > - if (ret < 0) { > - dev_err(data->dev, "failed to read humidity\n"); > + ret = bme280_read_humid_adc(data, &adc_humidity); > + if (ret < 0) > return ret; > - } > > - adc_humidity = be16_to_cpu(data->be16); > - if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { > - /* reading was skipped */ > - dev_err(data->dev, "reading humidity skipped\n"); > - return -EIO; > - } > - comp_humidity = bme280_compensate_humidity(data, adc_humidity); > + comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > + /* IIO units are in 1000 * % */ > *val = comp_humidity * 1000 / 1024; > > return IIO_VAL_INT; > @@ -930,9 +972,29 @@ static int bmp380_cmd(struct bmp280_data *data, u8 cmd) > * Taken from datasheet, Section Appendix 9, "Compensation formula" and repo > * https://github.com/BoschSensortec/BMP3-Sensor-API. > */ > -static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > +static int bmp380_read_temp_adc(struct bmp280_data *data, u32 *adc_temp) Interesting this one is unsigned. > +{ > + int ret; > + > + ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, > + data->buf, sizeof(data->buf)); > + if (ret < 0) { > + dev_err(data->dev, "failed to read temperature\n"); > + return ret; > + } > + > + *adc_temp = get_unaligned_le24(data->buf); > + if (*adc_temp == BMP380_TEMP_SKIPPED) { Same as above. > + dev_err(data->dev, "reading temperature skipped\n"); > + return -EIO; > + } > + > + return 0; > +} > + > +static s32 bmp380_calc_t_fine(struct bmp280_data *data, u32 adc_temp) > { > - s64 var1, var2, var3, var4, var5, var6, comp_temp; > + s64 var1, var2, var3, var4, var5, var6; > struct bmp380_calib *calib = &data->calib.bmp380; > > var1 = ((s64) adc_temp) - (((s64) calib->T1) << 8); > @@ -941,7 +1003,29 @@ static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > var4 = var3 * ((s64) calib->T3); > var5 = (var2 << 18) + var4; > var6 = var5 >> 32; > - data->t_fine = (s32) var6; > + return (s32) var6; /* t_fine = var6 */ > +} > + > +static int bmp380_get_t_fine(struct bmp280_data *data, s32 *t_fine) > +{ > + s32 adc_temp; > + int ret; > + > + ret = bmp380_read_temp_adc(data, &adc_temp); > + if (ret < 0) > + return ret; > + > + *t_fine = bmp380_calc_t_fine(data, adc_temp); > + > + return 0; > +} > + > +static int bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > +{ > + s64 comp_temp; > + s32 var6; > + > + var6 = bmp380_calc_t_fine(data, adc_temp); > comp_temp = (var6 * 25) >> 14; > > comp_temp = clamp_val(comp_temp, BMP380_MIN_TEMP, BMP380_MAX_TEMP); > @@ -955,27 +1039,48 @@ static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > * Taken from datasheet, Section 9.3. "Pressure compensation" and repository > * https://github.com/BoschSensortec/BMP3-Sensor-API. > */ > -static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press) > +static int bmp380_read_press_adc(struct bmp280_data *data, u32 *adc_press) > +{ > + int ret; > + > + ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, > + data->buf, sizeof(data->buf)); > + if (ret < 0) { > + dev_err(data->dev, "failed to read pressure\n"); > + return ret; > + } > + > + *adc_press = get_unaligned_le24(data->buf); As above > + if (*adc_press == BMP380_PRESS_SKIPPED) { > + dev_err(data->dev, "reading pressure skipped\n"); > + return -EIO; > + } > + > + return 0; > +} Jonathan
On Sun, May 05, 2024 at 08:21:06PM +0100, Jonathan Cameron wrote: > On Mon, 29 Apr 2024 21:00:42 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > For BMP18x, BMP28x, BME280, BMP38x the reading of the pressure > > value requires an update of the t_fine variable which happens > > through reading the temperature value. > > > > So all the bmpxxx_read_press() functions of the above sensors > > are internally calling the equivalent bmpxxx_read_temp() function > > in order to update the t_fine value. By just looking at the code > > this functionality is a bit hidden and is not easy to understand > > why those channels are not independent. > > > > This commit tries to clear these things a bit by splitting the > > bmpxxx_{read/compensate}_{temp/press/humid}() to the following: > > > > i. bmpxxx_read_{temp/press/humid}_adc(): read the raw value from > > the sensor. > > > > ii. bmpxx_calc_t_fine(): calculate the t_fine variable. > > > > iii. bmpxxx_get_t_fine(): get the t_fine variable. > > > > iv. bmpxxx_compensate_{temp/press/humid}(): compensate the adc > > values and return the calculated value. > > > > v. bmpxxx_read_{temp/press/humid}(): combine calls of the > > aforementioned functions to return the requested value. > > > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > In general looks good, but a few details to consider inline. > > Jonathan > > > --- > > drivers/iio/pressure/bmp280-core.c | 351 ++++++++++++++++++----------- > > drivers/iio/pressure/bmp280.h | 6 - > > 2 files changed, 221 insertions(+), 136 deletions(-) > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > index 8290028824e9..5ebce38e99f6 100644 > > --- a/drivers/iio/pressure/bmp280-core.c > > +++ b/drivers/iio/pressure/bmp280-core.c > > @@ -288,13 +288,33 @@ static int bme280_read_calib(struct bmp280_data *data) > > * > > * Taken from BME280 datasheet, Section 4.2.3, "Compensation formula". > > */ > > +static int bme280_read_humid_adc(struct bmp280_data *data, s32 *adc_humidity) > > It's an u16, so why use an s32? I can see using a signed value avoids a cast later, > but it makes this more confusing to read, so I'd push that cast up to the user. > Bosch in general has messed up a bit with the signs on their raw values on all of those sensors that we use in this driver. I totally agree with you, that this value does not make any sense to be anything else apart from u16 but in the datasheet [1] in pages 25-26 you can clearly see that they use an s32 for this value... [1]: https://www.mouser.com/datasheet/2/783/BST-BME280-DS002-1509607.pdf > > +{ > > + int ret; > > + > > + ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, > > + &data->be16, sizeof(data->be16)); > > + if (ret < 0) { > > + dev_err(data->dev, "failed to read humidity\n"); > > + return ret; > > + } > > + > > + *adc_humidity = be16_to_cpu(data->be16); > > Trivial, but on error return we normally aim for side effect free. > To do that here use an internal variable first. I am sorry, but in this part, I cannot fully understand what you mean by side effect free. I can understand the issue of storing a u16 to an s32 might make accidentally the sign to matter but how is this thing that you proposed no side effect free? You also made this comment in various other places in this patch (because the same principle with the SKIPPED is used) and in the other places the values are 20-bit and 24-bit long which confuses me a bit more on what you mean exactly. > s16 value; > > ... > > value = be16_to_cpu(data->be16) > > if (value == BMP280_HUMIDITY_SKIPPED) { > dev_err(data->dev, "... > return -EIO; > } > This is the odd bit due to using an s32 to store a u16. > Have to rely on that size mismatch to avoid the sign accidentally mattering. > Which is ugly! > > *adc_humidity = value; > > return 0; > > > > + if (*adc_humidity == BMP280_HUMIDITY_SKIPPED) { > > + dev_err(data->dev, "reading humidity skipped\n"); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > + > > static u32 bme280_compensate_humidity(struct bmp280_data *data, > > - s32 adc_humidity) > > + s32 adc_humidity, s32 t_fine) > > { > > struct bmp280_calib *calib = &data->calib.bmp280; > > s32 var; > > > > - var = ((s32)data->t_fine) - (s32)76800; > > + var = ((s32)t_fine) - (s32)76800; > > Casting an s32 to an s32. For the const it shouldn't matter as it'll be at least > 32 bits and we don't care about the sign. > In general, I kept the casting for the t_fine because it was used from before, but I don't see the point since it is already an s32 value. The casting in front of the const, I see it is used in the datasheet [1] in pages 25-26 so maybe they kept it for this reason. Since it will be again a 32 bit value, I don't see the point of casting it but I still kept it as it was, there originally. [1]: https://www.mouser.com/datasheet/2/783/BST-BME280-DS002-1509607.pdf > > var = ((((adc_humidity << 14) - (calib->H4 << 20) - (calib->H5 * var)) > > + (s32)16384) >> 15) * (((((((var * calib->H6) >> 10) > > * (((var * (s32)calib->H3) >> 11) + (s32)32768)) >> 10) > > @@ -313,8 +333,27 @@ static u32 bme280_compensate_humidity(struct bmp280_data *data, > > * > > * Taken from datasheet, Section 3.11.3, "Compensation formula". > > */ > > -static s32 bmp280_compensate_temp(struct bmp280_data *data, > > - s32 adc_temp) > > +static int bmp280_read_temp_adc(struct bmp280_data *data, s32 *adc_temp) > > As before, sign of the extra variable is confusing. It's not signed > as it's a raw ADC value. So I'd use a u32 for it. > Again, as I said before, Bosch has messed this up. I agree (again) with you that this should have been a u16 but according to the datasheet [2] in pages 45-46 it is an s32... [2]: https://cdn-shop.adafruit.com/datasheets/BST-BMP280-DS001-11.pdf > > +{ > > + int ret; > > + > > + ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > > + data->buf, sizeof(data->buf)); > > + if (ret < 0) { > > + dev_err(data->dev, "failed to read temperature\n"); > > + return ret; > > + } > > + > > + *adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); > > + if (*adc_temp == BMP280_TEMP_SKIPPED) { > > + dev_err(data->dev, "reading temperature skipped\n"); > > + return -EIO; > Same thing on side effects. Best to avoid them if it is easy to do (like here!) > > + } > > + > > + return 0; > > +} > > + > > +static s32 bmp280_calc_t_fine(struct bmp280_data *data, s32 adc_temp) > > { > > struct bmp280_calib *calib = &data->calib.bmp280; > > s32 var1, var2; > > @@ -324,9 +363,26 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data, > > var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) * > > ((adc_temp >> 4) - ((s32)calib->T1))) >> 12) * > > ((s32)calib->T3)) >> 14; > > - data->t_fine = var1 + var2; > > + return var1 + var2; /* t_fine = var1 + var2 */ > > +} > > + > > +static int bmp280_get_t_fine(struct bmp280_data *data, s32 *t_fine) > > +{ > > + s32 adc_temp; > > + int ret; > > + > > + ret = bmp280_read_temp_adc(data, &adc_temp); > > + if (ret < 0) > > + return ret; > > + > > + *t_fine = bmp280_calc_t_fine(data, adc_temp); > > > > - return (data->t_fine * 5 + 128) >> 8; > > + return 0; > > +} > > + > > +static s32 bmp280_compensate_temp(struct bmp280_data *data, s32 adc_temp) > > +{ > > + return (bmp280_calc_t_fine(data, adc_temp) * 5 + 128) / 256; > > } > > > > /* > > @@ -336,13 +392,33 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data, > > * > > * Taken from datasheet, Section 3.11.3, "Compensation formula". > > */ > > +static int bmp280_read_press_adc(struct bmp280_data *data, s32 *adc_press) > > +{ > > + int ret; > > + > > + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > > + data->buf, sizeof(data->buf)); > > + if (ret < 0) { > > + dev_err(data->dev, "failed to read pressure\n"); > > + return ret; > > + } > > + > > + *adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); > > + if (*adc_press == BMP280_PRESS_SKIPPED) { > > + dev_err(data->dev, "reading pressure skipped\n"); > > + return -EIO; > > As above; avoid side effects. > > > + } > > + > > + return 0; > > +} > > + > > static u32 bmp280_compensate_press(struct bmp280_data *data, > > - s32 adc_press) > > + s32 adc_press, s32 t_fine) > > { > > struct bmp280_calib *calib = &data->calib.bmp280; > > s64 var1, var2, p; > > > > - var1 = ((s64)data->t_fine) - 128000; > > + var1 = ((s64)t_fine) - 128000; > > var2 = var1 * var1 * (s64)calib->P6; > > var2 += (var1 * (s64)calib->P5) << 17; > > var2 += ((s64)calib->P4) << 35; > > @@ -368,60 +444,35 @@ static int bmp280_read_temp(struct bmp280_data *data, > > s32 adc_temp, comp_temp; > > int ret; > > > > - ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, > > - data->buf, sizeof(data->buf)); > > - if (ret < 0) { > > - dev_err(data->dev, "failed to read temperature\n"); > > + ret = bmp280_read_temp_adc(data, &adc_temp); > > + if (ret < 0) > > return ret; > > - } > > > > - adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); > > - if (adc_temp == BMP280_TEMP_SKIPPED) { > > - /* reading was skipped */ > > - dev_err(data->dev, "reading temperature skipped\n"); > > - return -EIO; > > - } > > comp_temp = bmp280_compensate_temp(data, adc_temp); > > > > - /* > > - * val might be NULL if we're called by the read_press routine, > > - * who only cares about the carry over t_fine value. > > - */ > > - if (val) { > > - *val = comp_temp * 10; > > - return IIO_VAL_INT; > > - } > > - > > - return 0; > > + /* IIO units are in milli Celsius */ > > + *val = comp_temp * 10; > > + return IIO_VAL_INT; > > } > > > > static int bmp280_read_press(struct bmp280_data *data, > > int *val, int *val2) > > { > > + s32 adc_press, t_fine; > > u32 comp_press; > > - s32 adc_press; > > int ret; > > > > - /* Read and compensate temperature so we get a reading of t_fine. */ > > - ret = bmp280_read_temp(data, NULL, NULL); > > + ret = bmp280_get_t_fine(data, &t_fine); > > if (ret < 0) > > return ret; > > > > - ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, > > - data->buf, sizeof(data->buf)); > > - if (ret < 0) { > > - dev_err(data->dev, "failed to read pressure\n"); > > + ret = bmp280_read_press_adc(data, &adc_press); > > + if (ret < 0) > > return ret; > > - } > > > > - adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); > > - if (adc_press == BMP280_PRESS_SKIPPED) { > > - /* reading was skipped */ > > - dev_err(data->dev, "reading pressure skipped\n"); > > - return -EIO; > > - } > > - comp_press = bmp280_compensate_press(data, adc_press); > > + comp_press = bmp280_compensate_press(data, adc_press, t_fine); > > > > + /* IIO units are in kPa */ > > *val = comp_press; > > *val2 = 256000; > > > > @@ -430,30 +481,21 @@ static int bmp280_read_press(struct bmp280_data *data, > > > > static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > > { > > + s32 adc_humidity, t_fine; > > u32 comp_humidity; > > - s32 adc_humidity; > > int ret; > > > > - /* Read and compensate temperature so we get a reading of t_fine. */ > > - ret = bmp280_read_temp(data, NULL, NULL); > > + ret = bmp280_get_t_fine(data, &t_fine); > > if (ret < 0) > > return ret; > > > > - ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, > > - &data->be16, sizeof(data->be16)); > > - if (ret < 0) { > > - dev_err(data->dev, "failed to read humidity\n"); > > + ret = bme280_read_humid_adc(data, &adc_humidity); > > + if (ret < 0) > > return ret; > > - } > > > > - adc_humidity = be16_to_cpu(data->be16); > > - if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { > > - /* reading was skipped */ > > - dev_err(data->dev, "reading humidity skipped\n"); > > - return -EIO; > > - } > > - comp_humidity = bme280_compensate_humidity(data, adc_humidity); > > + comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > > > + /* IIO units are in 1000 * % */ > > *val = comp_humidity * 1000 / 1024; > > > > return IIO_VAL_INT; > > @@ -930,9 +972,29 @@ static int bmp380_cmd(struct bmp280_data *data, u8 cmd) > > * Taken from datasheet, Section Appendix 9, "Compensation formula" and repo > > * https://github.com/BoschSensortec/BMP3-Sensor-API. > > */ > > -static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > > +static int bmp380_read_temp_adc(struct bmp280_data *data, u32 *adc_temp) > > Interesting this one is unsigned. > Yes, and it is also mentioned in the datasheet [3] in page 26. [3]: https://www.mouser.com/pdfdocs/BST-BMP388-DS001-01.pdf Cheers, Vasilis > > +{ > > + int ret; > > + > > + ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, > > + data->buf, sizeof(data->buf)); > > + if (ret < 0) { > > + dev_err(data->dev, "failed to read temperature\n"); > > + return ret; > > + } > > + > > + *adc_temp = get_unaligned_le24(data->buf); > > + if (*adc_temp == BMP380_TEMP_SKIPPED) { > Same as above. > > + dev_err(data->dev, "reading temperature skipped\n"); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > + > > +static s32 bmp380_calc_t_fine(struct bmp280_data *data, u32 adc_temp) > > { > > - s64 var1, var2, var3, var4, var5, var6, comp_temp; > > + s64 var1, var2, var3, var4, var5, var6; > > struct bmp380_calib *calib = &data->calib.bmp380; > > > > var1 = ((s64) adc_temp) - (((s64) calib->T1) << 8); > > @@ -941,7 +1003,29 @@ static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > > var4 = var3 * ((s64) calib->T3); > > var5 = (var2 << 18) + var4; > > var6 = var5 >> 32; > > - data->t_fine = (s32) var6; > > + return (s32) var6; /* t_fine = var6 */ > > +} > > + > > +static int bmp380_get_t_fine(struct bmp280_data *data, s32 *t_fine) > > +{ > > + s32 adc_temp; > > + int ret; > > + > > + ret = bmp380_read_temp_adc(data, &adc_temp); > > + if (ret < 0) > > + return ret; > > + > > + *t_fine = bmp380_calc_t_fine(data, adc_temp); > > + > > + return 0; > > +} > > + > > +static int bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > > +{ > > + s64 comp_temp; > > + s32 var6; > > + > > + var6 = bmp380_calc_t_fine(data, adc_temp); > > comp_temp = (var6 * 25) >> 14; > > > > comp_temp = clamp_val(comp_temp, BMP380_MIN_TEMP, BMP380_MAX_TEMP); > > @@ -955,27 +1039,48 @@ static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > > * Taken from datasheet, Section 9.3. "Pressure compensation" and repository > > * https://github.com/BoschSensortec/BMP3-Sensor-API. > > */ > > -static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press) > > +static int bmp380_read_press_adc(struct bmp280_data *data, u32 *adc_press) > > +{ > > + int ret; > > + > > + ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, > > + data->buf, sizeof(data->buf)); > > + if (ret < 0) { > > + dev_err(data->dev, "failed to read pressure\n"); > > + return ret; > > + } > > + > > + *adc_press = get_unaligned_le24(data->buf); > > As above > > > + if (*adc_press == BMP380_PRESS_SKIPPED) { > > + dev_err(data->dev, "reading pressure skipped\n"); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > Jonathan >
On Mon, 6 May 2024 01:47:51 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > On Sun, May 05, 2024 at 08:21:06PM +0100, Jonathan Cameron wrote: > > On Mon, 29 Apr 2024 21:00:42 +0200 > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > For BMP18x, BMP28x, BME280, BMP38x the reading of the pressure > > > value requires an update of the t_fine variable which happens > > > through reading the temperature value. > > > > > > So all the bmpxxx_read_press() functions of the above sensors > > > are internally calling the equivalent bmpxxx_read_temp() function > > > in order to update the t_fine value. By just looking at the code > > > this functionality is a bit hidden and is not easy to understand > > > why those channels are not independent. > > > > > > This commit tries to clear these things a bit by splitting the > > > bmpxxx_{read/compensate}_{temp/press/humid}() to the following: > > > > > > i. bmpxxx_read_{temp/press/humid}_adc(): read the raw value from > > > the sensor. > > > > > > ii. bmpxx_calc_t_fine(): calculate the t_fine variable. > > > > > > iii. bmpxxx_get_t_fine(): get the t_fine variable. > > > > > > iv. bmpxxx_compensate_{temp/press/humid}(): compensate the adc > > > values and return the calculated value. > > > > > > v. bmpxxx_read_{temp/press/humid}(): combine calls of the > > > aforementioned functions to return the requested value. > > > > > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > In general looks good, but a few details to consider inline. > > > > Jonathan > > > > > --- > > > drivers/iio/pressure/bmp280-core.c | 351 ++++++++++++++++++----------- > > > drivers/iio/pressure/bmp280.h | 6 - > > > 2 files changed, 221 insertions(+), 136 deletions(-) > > > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > > index 8290028824e9..5ebce38e99f6 100644 > > > --- a/drivers/iio/pressure/bmp280-core.c > > > +++ b/drivers/iio/pressure/bmp280-core.c > > > @@ -288,13 +288,33 @@ static int bme280_read_calib(struct bmp280_data *data) > > > * > > > * Taken from BME280 datasheet, Section 4.2.3, "Compensation formula". > > > */ > > > +static int bme280_read_humid_adc(struct bmp280_data *data, s32 *adc_humidity) > > > > It's an u16, so why use an s32? I can see using a signed value avoids a cast later, > > but it makes this more confusing to read, so I'd push that cast up to the user. > > > > Bosch in general has messed up a bit with the signs on their raw values on all > of those sensors that we use in this driver. I totally agree with you, that this > value does not make any sense to be anything else apart from u16 but in the > datasheet [1] in pages 25-26 you can clearly see that they use an s32 for this > value... > > [1]: https://www.mouser.com/datasheet/2/783/BST-BME280-DS002-1509607.pdf I would be tempted to ignore that and use the more appropriate size, but be careful that any necessary casts are in place when you use the value. > > > > +{ > > > + int ret; > > > + > > > + ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, > > > + &data->be16, sizeof(data->be16)); > > > + if (ret < 0) { > > > + dev_err(data->dev, "failed to read humidity\n"); > > > + return ret; > > > + } > > > + > > > + *adc_humidity = be16_to_cpu(data->be16); > > > > Trivial, but on error return we normally aim for side effect free. > > To do that here use an internal variable first. > > I am sorry, but in this part, I cannot fully understand what you mean > by side effect free. I can understand the issue of storing a u16 to an > s32 might make accidentally the sign to matter but how is this thing > that you proposed no side effect free? You also made this comment > in various other places in this patch (because the same principle > with the SKIPPED is used) and in the other places the values are > 20-bit and 24-bit long which confuses me a bit more on what you mean > exactly. If you get an error, e.g. if (*adc_humidty == BMP280_HUMIDITY_SKIPPED) then you should not set *adc_humidity. Setting it is the side effect. Normal aim is that a function that returns an error should ensure it leave no other effects in data it can access. This make reasoning about error paths much simpler because you only have to deal with the return values. Hence the code example I included though I make it more confusing by commenting on the types in the middle of the code. Key is I don't want *adc_humidity to be modified if we aren't going to return 0. > > > s16 value; > > > > ... > > > > value = be16_to_cpu(data->be16) > > > > if (value == BMP280_HUMIDITY_SKIPPED) { > > dev_err(data->dev, "... > > return -EIO; > > } > > This is the odd bit due to using an s32 to store a u16. > > Have to rely on that size mismatch to avoid the sign accidentally mattering. > > Which is ugly! > > > > *adc_humidity = value; > > > > return 0; > > > > > > > + if (*adc_humidity == BMP280_HUMIDITY_SKIPPED) { > > > + dev_err(data->dev, "reading humidity skipped\n"); > > > + return -EIO; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static u32 bme280_compensate_humidity(struct bmp280_data *data, > > > - s32 adc_humidity) > > > + s32 adc_humidity, s32 t_fine) > > > { > > > struct bmp280_calib *calib = &data->calib.bmp280; > > > s32 var; > > > > > > - var = ((s32)data->t_fine) - (s32)76800; > > > + var = ((s32)t_fine) - (s32)76800; > > > > Casting an s32 to an s32. For the const it shouldn't matter as it'll be at least > > 32 bits and we don't care about the sign. > > > > In general, I kept the casting for the t_fine because it was used from before, > but I don't see the point since it is already an s32 value. The casting in front > of the const, I see it is used in the datasheet [1] in pages 25-26 so maybe they > kept it for this reason. Since it will be again a 32 bit value, I don't see the > point of casting it but I still kept it as it was, there originally. > > [1]: https://www.mouser.com/datasheet/2/783/BST-BME280-DS002-1509607.pdf As you are tidying up the code, drop the unnecessary cast. Good to mention it in the patch description though. > > > > var = ((((adc_humidity << 14) - (calib->H4 << 20) - (calib->H5 * var)) > > > + (s32)16384) >> 15) * (((((((var * calib->H6) >> 10) > > > * (((var * (s32)calib->H3) >> 11) + (s32)32768)) >> 10) > > > @@ -313,8 +333,27 @@ static u32 bme280_compensate_humidity(struct bmp280_data *data, > > > * > > > * Taken from datasheet, Section 3.11.3, "Compensation formula". > > > */ > > > -static s32 bmp280_compensate_temp(struct bmp280_data *data, > > > - s32 adc_temp) > > > +static int bmp280_read_temp_adc(struct bmp280_data *data, s32 *adc_temp) > > > > As before, sign of the extra variable is confusing. It's not signed > > as it's a raw ADC value. So I'd use a u32 for it. > > > > Again, as I said before, Bosch has messed this up. I agree (again) with you > that this should have been a u16 but according to the datasheet [2] in pages > 45-46 it is an s32... > > [2]: https://cdn-shop.adafruit.com/datasheets/BST-BMP280-DS001-11.pdf > What they have is not incorrect, but it is relaxed in a rather lazy fashion! ... > > > @@ -430,30 +481,21 @@ static int bmp280_read_press(struct bmp280_data *data, > > > > > > static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) > > > { > > > + s32 adc_humidity, t_fine; > > > u32 comp_humidity; > > > - s32 adc_humidity; > > > int ret; > > > > > > - /* Read and compensate temperature so we get a reading of t_fine. */ > > > - ret = bmp280_read_temp(data, NULL, NULL); > > > + ret = bmp280_get_t_fine(data, &t_fine); > > > if (ret < 0) > > > return ret; > > > > > > - ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, > > > - &data->be16, sizeof(data->be16)); > > > - if (ret < 0) { > > > - dev_err(data->dev, "failed to read humidity\n"); > > > + ret = bme280_read_humid_adc(data, &adc_humidity); > > > + if (ret < 0) > > > return ret; > > > - } > > > > > > - adc_humidity = be16_to_cpu(data->be16); > > > - if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { > > > - /* reading was skipped */ > > > - dev_err(data->dev, "reading humidity skipped\n"); > > > - return -EIO; > > > - } > > > - comp_humidity = bme280_compensate_humidity(data, adc_humidity); > > > + comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); > > > > > > + /* IIO units are in 1000 * % */ > > > *val = comp_humidity * 1000 / 1024; > > > > > > return IIO_VAL_INT; > > > @@ -930,9 +972,29 @@ static int bmp380_cmd(struct bmp280_data *data, u8 cmd) > > > * Taken from datasheet, Section Appendix 9, "Compensation formula" and repo > > > * https://github.com/BoschSensortec/BMP3-Sensor-API. > > > */ > > > -static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) > > > +static int bmp380_read_temp_adc(struct bmp280_data *data, u32 *adc_temp) > > > > Interesting this one is unsigned. > > > > Yes, and it is also mentioned in the datasheet [3] in page 26. > > [3]: https://www.mouser.com/pdfdocs/BST-BMP388-DS001-01.pdf I'd take the datasheet maths as 'an implementation' rather than something we have to match. So go for types that make sense, not what they used! Jonathan > > Cheers, > Vasilis
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 8290028824e9..5ebce38e99f6 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -288,13 +288,33 @@ static int bme280_read_calib(struct bmp280_data *data) * * Taken from BME280 datasheet, Section 4.2.3, "Compensation formula". */ +static int bme280_read_humid_adc(struct bmp280_data *data, s32 *adc_humidity) +{ + int ret; + + ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, + &data->be16, sizeof(data->be16)); + if (ret < 0) { + dev_err(data->dev, "failed to read humidity\n"); + return ret; + } + + *adc_humidity = be16_to_cpu(data->be16); + if (*adc_humidity == BMP280_HUMIDITY_SKIPPED) { + dev_err(data->dev, "reading humidity skipped\n"); + return -EIO; + } + + return 0; +} + static u32 bme280_compensate_humidity(struct bmp280_data *data, - s32 adc_humidity) + s32 adc_humidity, s32 t_fine) { struct bmp280_calib *calib = &data->calib.bmp280; s32 var; - var = ((s32)data->t_fine) - (s32)76800; + var = ((s32)t_fine) - (s32)76800; var = ((((adc_humidity << 14) - (calib->H4 << 20) - (calib->H5 * var)) + (s32)16384) >> 15) * (((((((var * calib->H6) >> 10) * (((var * (s32)calib->H3) >> 11) + (s32)32768)) >> 10) @@ -313,8 +333,27 @@ static u32 bme280_compensate_humidity(struct bmp280_data *data, * * Taken from datasheet, Section 3.11.3, "Compensation formula". */ -static s32 bmp280_compensate_temp(struct bmp280_data *data, - s32 adc_temp) +static int bmp280_read_temp_adc(struct bmp280_data *data, s32 *adc_temp) +{ + int ret; + + ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, + data->buf, sizeof(data->buf)); + if (ret < 0) { + dev_err(data->dev, "failed to read temperature\n"); + return ret; + } + + *adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); + if (*adc_temp == BMP280_TEMP_SKIPPED) { + dev_err(data->dev, "reading temperature skipped\n"); + return -EIO; + } + + return 0; +} + +static s32 bmp280_calc_t_fine(struct bmp280_data *data, s32 adc_temp) { struct bmp280_calib *calib = &data->calib.bmp280; s32 var1, var2; @@ -324,9 +363,26 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data, var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) * ((adc_temp >> 4) - ((s32)calib->T1))) >> 12) * ((s32)calib->T3)) >> 14; - data->t_fine = var1 + var2; + return var1 + var2; /* t_fine = var1 + var2 */ +} + +static int bmp280_get_t_fine(struct bmp280_data *data, s32 *t_fine) +{ + s32 adc_temp; + int ret; + + ret = bmp280_read_temp_adc(data, &adc_temp); + if (ret < 0) + return ret; + + *t_fine = bmp280_calc_t_fine(data, adc_temp); - return (data->t_fine * 5 + 128) >> 8; + return 0; +} + +static s32 bmp280_compensate_temp(struct bmp280_data *data, s32 adc_temp) +{ + return (bmp280_calc_t_fine(data, adc_temp) * 5 + 128) / 256; } /* @@ -336,13 +392,33 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data, * * Taken from datasheet, Section 3.11.3, "Compensation formula". */ +static int bmp280_read_press_adc(struct bmp280_data *data, s32 *adc_press) +{ + int ret; + + ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, + data->buf, sizeof(data->buf)); + if (ret < 0) { + dev_err(data->dev, "failed to read pressure\n"); + return ret; + } + + *adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); + if (*adc_press == BMP280_PRESS_SKIPPED) { + dev_err(data->dev, "reading pressure skipped\n"); + return -EIO; + } + + return 0; +} + static u32 bmp280_compensate_press(struct bmp280_data *data, - s32 adc_press) + s32 adc_press, s32 t_fine) { struct bmp280_calib *calib = &data->calib.bmp280; s64 var1, var2, p; - var1 = ((s64)data->t_fine) - 128000; + var1 = ((s64)t_fine) - 128000; var2 = var1 * var1 * (s64)calib->P6; var2 += (var1 * (s64)calib->P5) << 17; var2 += ((s64)calib->P4) << 35; @@ -368,60 +444,35 @@ static int bmp280_read_temp(struct bmp280_data *data, s32 adc_temp, comp_temp; int ret; - ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, - data->buf, sizeof(data->buf)); - if (ret < 0) { - dev_err(data->dev, "failed to read temperature\n"); + ret = bmp280_read_temp_adc(data, &adc_temp); + if (ret < 0) return ret; - } - adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); - if (adc_temp == BMP280_TEMP_SKIPPED) { - /* reading was skipped */ - dev_err(data->dev, "reading temperature skipped\n"); - return -EIO; - } comp_temp = bmp280_compensate_temp(data, adc_temp); - /* - * val might be NULL if we're called by the read_press routine, - * who only cares about the carry over t_fine value. - */ - if (val) { - *val = comp_temp * 10; - return IIO_VAL_INT; - } - - return 0; + /* IIO units are in milli Celsius */ + *val = comp_temp * 10; + return IIO_VAL_INT; } static int bmp280_read_press(struct bmp280_data *data, int *val, int *val2) { + s32 adc_press, t_fine; u32 comp_press; - s32 adc_press; int ret; - /* Read and compensate temperature so we get a reading of t_fine. */ - ret = bmp280_read_temp(data, NULL, NULL); + ret = bmp280_get_t_fine(data, &t_fine); if (ret < 0) return ret; - ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, - data->buf, sizeof(data->buf)); - if (ret < 0) { - dev_err(data->dev, "failed to read pressure\n"); + ret = bmp280_read_press_adc(data, &adc_press); + if (ret < 0) return ret; - } - adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf)); - if (adc_press == BMP280_PRESS_SKIPPED) { - /* reading was skipped */ - dev_err(data->dev, "reading pressure skipped\n"); - return -EIO; - } - comp_press = bmp280_compensate_press(data, adc_press); + comp_press = bmp280_compensate_press(data, adc_press, t_fine); + /* IIO units are in kPa */ *val = comp_press; *val2 = 256000; @@ -430,30 +481,21 @@ static int bmp280_read_press(struct bmp280_data *data, static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2) { + s32 adc_humidity, t_fine; u32 comp_humidity; - s32 adc_humidity; int ret; - /* Read and compensate temperature so we get a reading of t_fine. */ - ret = bmp280_read_temp(data, NULL, NULL); + ret = bmp280_get_t_fine(data, &t_fine); if (ret < 0) return ret; - ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB, - &data->be16, sizeof(data->be16)); - if (ret < 0) { - dev_err(data->dev, "failed to read humidity\n"); + ret = bme280_read_humid_adc(data, &adc_humidity); + if (ret < 0) return ret; - } - adc_humidity = be16_to_cpu(data->be16); - if (adc_humidity == BMP280_HUMIDITY_SKIPPED) { - /* reading was skipped */ - dev_err(data->dev, "reading humidity skipped\n"); - return -EIO; - } - comp_humidity = bme280_compensate_humidity(data, adc_humidity); + comp_humidity = bme280_compensate_humidity(data, adc_humidity, t_fine); + /* IIO units are in 1000 * % */ *val = comp_humidity * 1000 / 1024; return IIO_VAL_INT; @@ -930,9 +972,29 @@ static int bmp380_cmd(struct bmp280_data *data, u8 cmd) * Taken from datasheet, Section Appendix 9, "Compensation formula" and repo * https://github.com/BoschSensortec/BMP3-Sensor-API. */ -static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) +static int bmp380_read_temp_adc(struct bmp280_data *data, u32 *adc_temp) +{ + int ret; + + ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, + data->buf, sizeof(data->buf)); + if (ret < 0) { + dev_err(data->dev, "failed to read temperature\n"); + return ret; + } + + *adc_temp = get_unaligned_le24(data->buf); + if (*adc_temp == BMP380_TEMP_SKIPPED) { + dev_err(data->dev, "reading temperature skipped\n"); + return -EIO; + } + + return 0; +} + +static s32 bmp380_calc_t_fine(struct bmp280_data *data, u32 adc_temp) { - s64 var1, var2, var3, var4, var5, var6, comp_temp; + s64 var1, var2, var3, var4, var5, var6; struct bmp380_calib *calib = &data->calib.bmp380; var1 = ((s64) adc_temp) - (((s64) calib->T1) << 8); @@ -941,7 +1003,29 @@ static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) var4 = var3 * ((s64) calib->T3); var5 = (var2 << 18) + var4; var6 = var5 >> 32; - data->t_fine = (s32) var6; + return (s32) var6; /* t_fine = var6 */ +} + +static int bmp380_get_t_fine(struct bmp280_data *data, s32 *t_fine) +{ + s32 adc_temp; + int ret; + + ret = bmp380_read_temp_adc(data, &adc_temp); + if (ret < 0) + return ret; + + *t_fine = bmp380_calc_t_fine(data, adc_temp); + + return 0; +} + +static int bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) +{ + s64 comp_temp; + s32 var6; + + var6 = bmp380_calc_t_fine(data, adc_temp); comp_temp = (var6 * 25) >> 14; comp_temp = clamp_val(comp_temp, BMP380_MIN_TEMP, BMP380_MAX_TEMP); @@ -955,27 +1039,48 @@ static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp) * Taken from datasheet, Section 9.3. "Pressure compensation" and repository * https://github.com/BoschSensortec/BMP3-Sensor-API. */ -static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press) +static int bmp380_read_press_adc(struct bmp280_data *data, u32 *adc_press) +{ + int ret; + + ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, + data->buf, sizeof(data->buf)); + if (ret < 0) { + dev_err(data->dev, "failed to read pressure\n"); + return ret; + } + + *adc_press = get_unaligned_le24(data->buf); + if (*adc_press == BMP380_PRESS_SKIPPED) { + dev_err(data->dev, "reading pressure skipped\n"); + return -EIO; + } + + return 0; +} + +static u32 bmp380_compensate_press(struct bmp280_data *data, + u32 adc_press, s32 t_fine) { s64 var1, var2, var3, var4, var5, var6, offset, sensitivity; struct bmp380_calib *calib = &data->calib.bmp380; u32 comp_press; - var1 = (s64)data->t_fine * (s64)data->t_fine; + var1 = (s64)t_fine * (s64)t_fine; var2 = var1 >> 6; - var3 = (var2 * ((s64) data->t_fine)) >> 8; + var3 = (var2 * ((s64) t_fine)) >> 8; var4 = ((s64)calib->P8 * var3) >> 5; var5 = ((s64)calib->P7 * var1) << 4; - var6 = ((s64)calib->P6 * (s64)data->t_fine) << 22; + var6 = ((s64)calib->P6 * (s64)t_fine) << 22; offset = ((s64)calib->P5 << 47) + var4 + var5 + var6; var2 = ((s64)calib->P4 * var3) >> 5; var4 = ((s64)calib->P3 * var1) << 2; var5 = ((s64)calib->P2 - ((s64)1 << 14)) * - ((s64)data->t_fine << 21); + ((s64)t_fine << 21); sensitivity = (((s64) calib->P1 - ((s64) 1 << 14)) << 46) + var2 + var4 + var5; var1 = (sensitivity >> 24) * (s64)adc_press; - var2 = (s64)calib->P10 * (s64)data->t_fine; + var2 = (s64)calib->P10 * (s64)t_fine; var3 = var2 + ((s64)calib->P9 << 16); var4 = (var3 * (s64)adc_press) >> 13; @@ -1001,60 +1106,34 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2) u32 adc_temp; int ret; - ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, - data->buf, sizeof(data->buf)); - if (ret < 0) { - dev_err(data->dev, "failed to read temperature\n"); + ret = bmp380_read_temp_adc(data, &adc_temp); + if (ret < 0) return ret; - } - adc_temp = get_unaligned_le24(data->buf); - if (adc_temp == BMP380_TEMP_SKIPPED) { - dev_err(data->dev, "reading temperature skipped\n"); - return -EIO; - } comp_temp = bmp380_compensate_temp(data, adc_temp); - /* - * Val might be NULL if we're called by the read_press routine, - * who only cares about the carry over t_fine value. - */ - if (val) { - /* IIO reports temperatures in milli Celsius */ - *val = comp_temp * 10; - return IIO_VAL_INT; - } - - return 0; + /* IIO units are in milli Celsius */ + *val = comp_temp * 10; + return IIO_VAL_INT; } static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2) { - s32 comp_press; - u32 adc_press; + u32 adc_press, comp_press, t_fine; int ret; - /* Read and compensate for temperature so we get a reading of t_fine */ - ret = bmp380_read_temp(data, NULL, NULL); + ret = bmp380_get_t_fine(data, &t_fine); if (ret < 0) return ret; - ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, - data->buf, sizeof(data->buf)); - if (ret < 0) { - dev_err(data->dev, "failed to read pressure\n"); + ret = bmp380_read_press_adc(data, &adc_press); + if (ret < 0) return ret; - } - adc_press = get_unaligned_le24(data->buf); - if (adc_press == BMP380_PRESS_SKIPPED) { - dev_err(data->dev, "reading pressure skipped\n"); - return -EIO; - } - comp_press = bmp380_compensate_press(data, adc_press); + comp_press = bmp380_compensate_press(data, adc_press, t_fine); + /* IIO units are in kPa */ *val = comp_press; - /* Compensated pressure is in cPa (centipascals) */ *val2 = 100000; return IIO_VAL_FRACTIONAL; @@ -1826,7 +1905,7 @@ static int bmp180_wait_for_eoc(struct bmp280_data *data, u8 ctrl_meas) return 0; } -static int bmp180_read_adc_temp(struct bmp280_data *data, int *val) +static int bmp180_read_temp_adc(struct bmp280_data *data, s32 *adc_temp) { int ret; @@ -1843,7 +1922,7 @@ static int bmp180_read_adc_temp(struct bmp280_data *data, int *val) return ret; } - *val = be16_to_cpu(data->be16); + *adc_temp = be16_to_cpu(data->be16); return 0; } @@ -1893,16 +1972,34 @@ static int bmp180_read_calib(struct bmp280_data *data) * * Taken from datasheet, Section 3.5, "Calculating pressure and temperature". */ -static s32 bmp180_compensate_temp(struct bmp280_data *data, s32 adc_temp) + +static s32 bmp180_calc_t_fine(struct bmp280_data *data, s32 adc_temp) { struct bmp180_calib *calib = &data->calib.bmp180; s32 x1, x2; x1 = ((adc_temp - calib->AC6) * calib->AC5) >> 15; x2 = (calib->MC << 11) / (x1 + calib->MD); - data->t_fine = x1 + x2; + return x1 + x2; /* t_fine = x1 + x2; */ +} + +static int bmp180_get_t_fine(struct bmp280_data *data, s32 *t_fine) +{ + s32 adc_temp; + int ret; + + ret = bmp180_read_temp_adc(data, &adc_temp); + if (ret < 0) + return ret; + + *t_fine = bmp180_calc_t_fine(data, adc_temp); - return (data->t_fine + 8) >> 4; + return 0; +} + +static s32 bmp180_compensate_temp(struct bmp280_data *data, s32 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) @@ -1910,25 +2007,18 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val, int *val2) s32 adc_temp, comp_temp; int ret; - ret = bmp180_read_adc_temp(data, &adc_temp); + ret = bmp180_read_temp_adc(data, &adc_temp); if (ret < 0) return ret; comp_temp = bmp180_compensate_temp(data, adc_temp); - /* - * val might be NULL if we're called by the read_press routine, - * who only cares about the carry over t_fine value. - */ - if (val) { - *val = comp_temp * 100; - return IIO_VAL_INT; - } - - return 0; + /* IIO units are in milli Celsius */ + *val = comp_temp * 100; + return IIO_VAL_INT; } -static int bmp180_read_adc_press(struct bmp280_data *data, int *val) +static int bmp180_read_press_adc(struct bmp280_data *data, s32 *adc_press) { u8 oss = data->oversampling_press; int ret; @@ -1947,7 +2037,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val) return ret; } - *val = get_unaligned_be24(data->buf) >> (8 - oss); + *adc_press = get_unaligned_be24(data->buf) >> (8 - oss); return 0; } @@ -1957,7 +2047,8 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val) * * Taken from datasheet, Section 3.5, "Calculating pressure and temperature". */ -static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press) +static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press, + s32 t_fine) { struct bmp180_calib *calib = &data->calib.bmp180; s32 oss = data->oversampling_press; @@ -1965,7 +2056,7 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press) s32 b3, b6; u32 b4, b7; - b6 = data->t_fine - 4000; + b6 = t_fine - 4000; x1 = (calib->B2 * (b6 * b6 >> 12)) >> 11; x2 = calib->AC2 * b6 >> 11; x3 = x1 + x2; @@ -1989,21 +2080,21 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press) static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2) { + s32 adc_press, t_fine; u32 comp_press; - s32 adc_press; int ret; - /* Read and compensate temperature so we get a reading of t_fine. */ - ret = bmp180_read_temp(data, NULL, NULL); + ret = bmp180_get_t_fine(data, &t_fine); if (ret < 0) return ret; - ret = bmp180_read_adc_press(data, &adc_press); + ret = bmp180_read_press_adc(data, &adc_press); if (ret < 0) return ret; - comp_press = bmp180_compensate_press(data, adc_press); + comp_press = bmp180_compensate_press(data, adc_press, t_fine); + /* IIO units are in kPa */ *val = comp_press; *val2 = 1000; diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index fe4d3f127954..7c30e4d523be 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -397,12 +397,6 @@ struct bmp280_data { */ int sampling_freq; - /* - * Carryover value from temperature conversion, used in pressure - * calculation. - */ - s32 t_fine; - /* * DMA (thus cache coherency maintenance) may require the * transfer buffers to live in their own cache lines.
For BMP18x, BMP28x, BME280, BMP38x the reading of the pressure value requires an update of the t_fine variable which happens through reading the temperature value. So all the bmpxxx_read_press() functions of the above sensors are internally calling the equivalent bmpxxx_read_temp() function in order to update the t_fine value. By just looking at the code this functionality is a bit hidden and is not easy to understand why those channels are not independent. This commit tries to clear these things a bit by splitting the bmpxxx_{read/compensate}_{temp/press/humid}() to the following: i. bmpxxx_read_{temp/press/humid}_adc(): read the raw value from the sensor. ii. bmpxx_calc_t_fine(): calculate the t_fine variable. iii. bmpxxx_get_t_fine(): get the t_fine variable. iv. bmpxxx_compensate_{temp/press/humid}(): compensate the adc values and return the calculated value. v. bmpxxx_read_{temp/press/humid}(): combine calls of the aforementioned functions to return the requested value. Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 351 ++++++++++++++++++----------- drivers/iio/pressure/bmp280.h | 6 - 2 files changed, 221 insertions(+), 136 deletions(-)