On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> 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.
>
This was mainly driven by the fact that I wanted to avoid reading
the temperature 3 times in case temp, press and humid are enabled
and there are consecutive buffer readings. But thank you for the
proposal I really appreciate it!
Best regards,
Vasilis
> 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;
>
