Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear temperature readings
From: Jonathan Cameron
Date: Thu Mar 14 2024 - 11:10:24 EST
On Wed, 13 Mar 2024 18:40:05 +0100
Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
> The read_press/read_humid functions need the updated t_fine value
> in order to calculate the current pressure/humidity. Temperature
> reads should be removed from the read_press/read_humid functions
> and should be placed in the oneshot captures before the pressure
> and humidity reads. This makes the code more intuitive.
>
> Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
To me this makes the use of these calls less obvious than they were
previously. The calls are made close to where t_fine is used and
don't have to go via the indirection of chip_info.
So I disagree. I think this change makes the code a lot less
clear.
The only improvement I can readily see would be to move the
temperature read into the compensation functions themselves, possibly
removing t_fine from data and having a function that reads everything
relevant to computing it directly but doesn't do the maths to get
a temperature reading. That can be reused in bmp280_compensate_temp()
Something along lines of.
static s32 bmp280_calc_tfine(struct bmp280_calib *calib, s32 adc_temp)
{
s32 var1, var2;
var1 = (((adc_temp >> 3) - ((s32)calib->T1 << 1)) *
((s32)calib->T2)) >> 11;
var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) *
((adc_temp >> 4) - ((s32)calib->T1))) >> 12) *
((s32)calib->T3)) >> 14;
return var1 + var2;
}
static int bmp280_read_temp_raw(struct bmp280_data *data,
s32 *raw)
{
s32 adc_temp;
int ret;
ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
data->buf, sizeof(data->buf));
if (ret < 0) {
dev_err(data->dev, "failed to read temperature\n");
return ret;
}
adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf));
if (adc_temp == BMP280_TEMP_SKIPPED) {
/* reading was skipped */
dev_err(data->dev, "reading temperature skipped\n");
return -EIO;
}
*raw = adc_temp;
return 0;
}
static int bmp280_get_t_fine(.., s32 *t_fine)
{
s32 adc_temp, comp_temp;
s32 t_fine;
int ret;
ret = bmp280_read_temp_raw(data, &adc_temp;
if (ret)
return ret;
*t_fine = bmp280_calc_tfine(&data->calib.bmp280, adc_temp);
return 0;
}
static int bmp280_read_temp(struct bmp280_data *data, s32 *temp)
{
int ret;
s32 t_fine;
ret = bmp280_get_t_fine(data, &t_fine);
if (ret)
return ret;
*temp = (t_fine * 5 + 128) / 256;
//division rather than shift as then it's obvious what the 128 is there for
return 0;
}
Now you have a nice function to get you t_fine which is all you want in some
of these paths. Call it directly where it is needed instead of as
a side effect of a temperature read.
> ---
> drivers/iio/pressure/bmp280-core.c | 38 ++++++++++++++----------------
> 1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 6d7734f867bc..377e90d9e5a2 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -404,11 +404,6 @@ static u32 bmp280_read_press(struct bmp280_data *data)
> s32 adc_press;
> int ret;
>
> - /* Read and compensate temperature so we get a reading of t_fine. */
> - ret = bmp280_read_temp(data);
> - if (ret < 0)
> - return ret;
> -
> ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> data->buf, sizeof(data->buf));
> if (ret < 0) {
> @@ -433,11 +428,6 @@ static u32 bmp280_read_humid(struct bmp280_data *data)
> s32 adc_humidity;
> int ret;
>
> - /* Read and compensate temperature so we get a reading of t_fine. */
> - ret = bmp280_read_temp(data);
> - if (ret < 0)
> - return ret;
> -
> ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> &data->be16, sizeof(data->be16));
> if (ret < 0) {
> @@ -470,12 +460,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_HUMIDITYRELATIVE:
> + /* Read temperature to update the t_fine value */
> + data->chip_info->read_temp(data);
> ret = data->chip_info->read_humid(data);
> *val = data->chip_info->humid_coeffs[0] * ret;
> *val2 = data->chip_info->humid_coeffs[1];
> ret = IIO_VAL_FRACTIONAL;
> break;
> case IIO_PRESSURE:
> + /*
> + * Read temperature to update the t_fine value.
> + * BMP5xx devices do this in hardware, so skip it.
> + */
> + if (strcmp(indio_dev->name, "bmp580"))
> + data->chip_info->read_temp(data);
> +
> ret = data->chip_info->read_press(data);
> *val = data->chip_info->press_coeffs[0] * ret;
> *val2 = data->chip_info->press_coeffs[1];
> @@ -500,10 +499,19 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_RAW:
> switch (chan->type) {
> case IIO_HUMIDITYRELATIVE:
> + /* Read temperature to update the t_fine value */
> + data->chip_info->read_temp(data);
> *val = data->chip_info->read_humid(data);
> ret = IIO_VAL_INT;
> break;
> case IIO_PRESSURE:
> + /*
> + * Read temperature to update the t_fine value.
> + * BMP5xx devices do this in hardware, so skip it.
> + */
> + if (strcmp(indio_dev->name, "bmp580"))
> + data->chip_info->read_temp(data);
> +
> *val = data->chip_info->read_press(data);
> ret = IIO_VAL_INT;
> break;
> @@ -1092,11 +1100,6 @@ static u32 bmp380_read_press(struct bmp280_data *data)
> s32 adc_press;
> int ret;
>
> - /* Read and compensate for temperature so we get a reading of t_fine */
> - ret = bmp380_read_temp(data);
> - if (ret)
> - return ret;
> -
> ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> data->buf, sizeof(data->buf));
> if (ret) {
> @@ -2009,11 +2012,6 @@ static u32 bmp180_read_press(struct bmp280_data *data)
> s32 adc_press;
> int ret;
>
> - /* Read and compensate temperature so we get a reading of t_fine. */
> - ret = bmp180_read_temp(data);
> - if (ret)
> - return ret;
> -
> ret = bmp180_read_adc_press(data, &adc_press);
> if (ret)
> return ret;