Re: [PATCH v3 3/6] iio: pressure: Generalize read_{temp/press/humid}() functions
From: Vasileios Amoiridis
Date: Tue Apr 02 2024 - 13:56:08 EST
On Sun, Mar 24, 2024 at 11:36:16AM +0000, Jonathan Cameron wrote:
> On Tue, 19 Mar 2024 01:29:22 +0100
> Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
>
> > Add the coefficients for the IIO standard units and the return
> > IIO value inside the chip_info structure.
> >
> > Remove the calculations with the coefficients for the IIO unit
> > compatibility from inside the read_{temp/press/humid}() functions
> > and move it to the general read_raw() function.
> >
> > Execute the calculations with the coefficients inside the read_raw()
> > oneshot capture function.
> >
> > In this way, all the data for the calculation of the value are
> > located in the chip_info structure of the respective sensor.
> >
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> Hi,
>
> Perhaps it's later in the series, but I still don't much like the hidden nature
> of t_fine. I'd much rather that was explicitly 'returned' by the function
> - by that I mean read_temp takes an s32 *t_fine and provides that if the pointer
> isn't NULL.
>
> Then drop the cached value in bmp280_data which I think just serves to make
> this code less readable than it could be.
>
> Then bmp280_compensate_pressure() can take a struct bmp280_calib, s32 adc_press and
> s32 t_fine so it just has the data it needs.
> Something similar for bmp280_compenstate_temp()
>
> Obviously this is cleaning up stuff that's been there a long time, but
> given you are generalizing these functions this seems like the time to
> make these other changes.
>
> As it stands, I don't think this code works as t_fine isn't updated
> everywhere it needs to be and that is hidden away by it being updated
> as a side effect of other calls.
>
> Jonathan
>
Hi Jonathan,
I am replying a bit late but I was off for quite some days.
In general the t_fine variable is calculated inside the bmpxxx_compensate_temp().
The t_fine variable is a function of the various varX variables and the adc_temp.
So by reading a new temp value from
the sensor and calculating its compensated value, the new t_fine variable is
calculated. So the combination of reading temp from sensor + compensating the
temp value = updated t_fine as a result of the compensated temperature. I agree that
this has a hidden nature. I can solve it by disintegrating the read()+compensate()
functions as follows:
bmpxxx_read_temp_adc(struct bmp280_data *data, s32 *adc_temp)
{
/* reads adc temperature from the sensor */
}
bmpxxx_calc_t_fine(struct bmp280_calib *calib, s32 *adc_temp)
{
/* calculate t_fine from adc_temp */
}
bmpxxx_get/update_t_fine(struct bmp280_data *data, ...)
{
u32 adc_temp;
s32 t_fine;
bmpxxx_read_temp_adc(adc_temp); //get adc_temp
if (ret)
return ret;
*t_fine = bmpxxx_calc_t_fine(&data->bmp280_calib.bmpxxx_calib, adc_temp);
}
bmpxxx_read_temp(struct bmp280_data *data, s32 *comp_temp)
{
int ret;
s32 t_fine;
ret = bmpxxx_get/update_t_fine(&data, &t_fine);
if (ret)
return ret;
*comp_temp = /* rest of the calculations to compensate temperature */
return 0
}
Another question is, should this be applied to the pressure/humidity readings as
well? Maybe, read_{humidity/press}_adc() functions could be introduced just to
have consistency with the temperature readings. Currently the adc_{humid/press}()
reading is done inside the read_{humid/press}() functions which already
incorporates the compensate_{humid/press}() functions.
>
> > ---
> > drivers/iio/pressure/bmp280-core.c | 189 +++++++++++++++--------------
> > drivers/iio/pressure/bmp280.h | 13 +-
> > 2 files changed, 106 insertions(+), 96 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f7a13ff6f26c..6d6173c4b744 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -363,10 +363,9 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
> > return (u32)p;
> > }
> >
> > -static int bmp280_read_temp(struct bmp280_data *data,
> > - int *val, int *val2)
> > +static int bmp280_read_temp(struct bmp280_data *data, s32 *comp_temp)
> > {
> > - s32 adc_temp, comp_temp;
> > + s32 adc_temp;
> > int ret;
> >
> > ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> > @@ -382,29 +381,20 @@ static int bmp280_read_temp(struct bmp280_data *data,
> > dev_err(data->dev, "reading temperature skipped\n");
> > return -EIO;
> > }
> > - comp_temp = bmp280_compensate_temp(data, adc_temp);
> >
> > - /*
> > - * val might be NULL if we're called by the read_press routine,
> > - * who only cares about the carry over t_fine value.
> > - */
> > - if (val) {
> > - *val = comp_temp * 10;
> > - return IIO_VAL_INT;
> > - }
> > + if (comp_temp)
> > + *comp_temp = bmp280_compensate_temp(data, adc_temp);
>
> As below, I don't think this is updating t_fine.
> Another reason to make that update very obvious rather than burried
> in this function call.
>
> >
> > return 0;
> > }
> >
> > -static int bmp280_read_press(struct bmp280_data *data,
> > - int *val, int *val2)
> > +static int bmp280_read_press(struct bmp280_data *data, u32 *comp_press)
> > {
> > - u32 comp_press;
> > s32 adc_press;
> > int ret;
> >
> > /* Read and compensate temperature so we get a reading of t_fine. */
> > - ret = bmp280_read_temp(data, NULL, NULL);
> > + ret = bmp280_read_temp(data, NULL);
> > if (ret < 0)
> > return ret;
> >
> > @@ -421,22 +411,19 @@ static int bmp280_read_press(struct bmp280_data *data,
> > dev_err(data->dev, "reading pressure skipped\n");
> > return -EIO;
> > }
> > - comp_press = bmp280_compensate_press(data, adc_press);
> >
> > - *val = comp_press;
> > - *val2 = 256000;
> > + *comp_press = bmp280_compensate_press(data, adc_press);
> >
> > - return IIO_VAL_FRACTIONAL;
> > + return 0;
> > }
> >
> > -static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > +static int bmp280_read_humid(struct bmp280_data *data, u32 *comp_humidity)
> > {
> > - u32 comp_humidity;
> > s32 adc_humidity;
> > int ret;
> >
> > /* Read and compensate temperature so we get a reading of t_fine. */
> > - ret = bmp280_read_temp(data, NULL, NULL);
> > + ret = bmp280_read_temp(data, NULL);
> > if (ret < 0)
> > return ret;
> >
> > @@ -453,11 +440,10 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > dev_err(data->dev, "reading humidity skipped\n");
> > return -EIO;
> > }
> > - comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
> >
> > - *val = comp_humidity * 1000 / 1024;
> > + *comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
> >
> > - return IIO_VAL_INT;
> > + return 0;
> > }
> >
> > static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> > @@ -465,6 +451,8 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> > int *val, int *val2, long mask)
> > {
> > struct bmp280_data *data = iio_priv(indio_dev);
> > + int chan_value;
> > + int ret;
> >
> > guard(mutex)(&data->lock);
> >
> > @@ -472,11 +460,29 @@ static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,
> > case IIO_CHAN_INFO_PROCESSED:
> > switch (chan->type) {
> > case IIO_HUMIDITYRELATIVE:
> > - return data->chip_info->read_humid(data, val, val2);
> > + ret = data->chip_info->read_humid(data, &chan_value);
> > + if (ret)
> > + return ret;
> > +
> > + *val = data->chip_info->humid_coeffs[0] * chan_value;
> > + *val2 = data->chip_info->humid_coeffs[1];
> > + return data->chip_info->humid_coeffs_type;
> > case IIO_PRESSURE:
> > - return data->chip_info->read_press(data, val, val2);
> > + ret = data->chip_info->read_press(data, &chan_value);
> > + if (ret)
> > + return ret;
> > +
> > + *val = data->chip_info->press_coeffs[0] * chan_value;
> > + *val2 = data->chip_info->press_coeffs[1];
> > + return data->chip_info->press_coeffs_type;
> > case IIO_TEMP:
> > - return data->chip_info->read_temp(data, val, val2);
> > + ret = data->chip_info->read_temp(data, &chan_value);
> > + if (ret)
> > + return ret;
> > +
> > + *val = data->chip_info->temp_coeffs[0] * chan_value;
> > + *val2 = data->chip_info->temp_coeffs[1];
> > + return data->chip_info->temp_coeffs_type;
> > default:
> > return -EINVAL;
> > }
> > @@ -787,6 +793,8 @@ static int bmp280_chip_config(struct bmp280_data *data)
> >
> > static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
> > static const u8 bmp280_chip_ids[] = { BMP280_CHIP_ID };
> > +static const int bmp280_temp_coeffs[] = { 10, 1 };
> > +static const int bmp280_press_coeffs[] = { 1, 256000 };
> >
> > const struct bmp280_chip_info bmp280_chip_info = {
> > .id_reg = BMP280_REG_ID,
> > @@ -815,6 +823,11 @@ const struct bmp280_chip_info bmp280_chip_info = {
> > .num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> > .oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
> >
> > + .temp_coeffs = bmp280_temp_coeffs,
> > + .temp_coeffs_type = IIO_VAL_FRACTIONAL,
> > + .press_coeffs = bmp280_press_coeffs,
> > + .press_coeffs_type = IIO_VAL_FRACTIONAL,
> > +
> > .chip_config = bmp280_chip_config,
> > .read_temp = bmp280_read_temp,
> > .read_press = bmp280_read_press,
> > @@ -841,6 +854,7 @@ static int bme280_chip_config(struct bmp280_data *data)
> > }
> >
> > static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
> > +static const int bme280_humid_coeffs[] = { 1000, 1024 };
> >
> > const struct bmp280_chip_info bme280_chip_info = {
> > .id_reg = BMP280_REG_ID,
> > @@ -863,6 +877,14 @@ const struct bmp280_chip_info bme280_chip_info = {
> > .num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> > .oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
> >
> > + .temp_coeffs = bmp280_temp_coeffs,
> > + .temp_coeffs_type = IIO_VAL_FRACTIONAL,
> > + .press_coeffs = bmp280_press_coeffs,
> > + .press_coeffs_type = IIO_VAL_FRACTIONAL,
> > + .humid_coeffs = bme280_humid_coeffs,
> > + .humid_coeffs_type = IIO_VAL_FRACTIONAL,
> > +
> > +
> One blank line is almost always enough.
>
> > .chip_config = bme280_chip_config,
> > .read_temp = bmp280_read_temp,
> > .read_press = bmp280_read_press,
> > @@ -988,9 +1010,8 @@ static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press)
> > return comp_press;
> > }
> >
> > -static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> > +static int bmp380_read_temp(struct bmp280_data *data, s32 *comp_temp)
> > {
> > - s32 comp_temp;
> > u32 adc_temp;
> > int ret;
> >
> > @@ -1006,29 +1027,20 @@ static int bmp380_read_temp(struct bmp280_data *data, int *val, int *val2)
> > dev_err(data->dev, "reading temperature skipped\n");
> > return -EIO;
> > }
> > - comp_temp = bmp380_compensate_temp(data, adc_temp);
> >
> > - /*
> > - * Val might be NULL if we're called by the read_press routine,
> > - * who only cares about the carry over t_fine value.
> > - */
> > - if (val) {
> > - /* IIO reports temperatures in milli Celsius */
> > - *val = comp_temp * 10;
> > - return IIO_VAL_INT;
> > - }
> > + if (comp_temp)
> > + *comp_temp = bmp380_compensate_temp(data, adc_temp);
> >
>
> I'm confused. If comp_temp is not provided then t_fine isn't updated
> so this function isn't doing anything?
>
> > return 0;
> > }
> >
> > -static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> > +static int bmp380_read_press(struct bmp280_data *data, u32 *comp_press)
> > {
> > - s32 comp_press;
> > u32 adc_press;
> > int ret;
> >
> > /* Read and compensate for temperature so we get a reading of t_fine */
>
> As above, I don't think it does.
>
> > - ret = bmp380_read_temp(data, NULL, NULL);
> > + ret = bmp380_read_temp(data, NULL);
> > if (ret)
> > return ret;
> >
> > @@ -1044,13 +1056,10 @@ static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> > dev_err(data->dev, "reading pressure skipped\n");
> > return -EIO;
> > }
> > - comp_press = bmp380_compensate_press(data, adc_press);
> >
> > - *val = comp_press;
> > - /* Compensated pressure is in cPa (centipascals) */
> > - *val2 = 100000;
> > + *comp_press = bmp380_compensate_press(data, adc_press);
> >
> > - return IIO_VAL_FRACTIONAL;
> > + return 0;
> > }
> >
>
> J