| Message ID | 20240319002925.2121016-4-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Series to add triggered buffer support to BMP280 driver | expand |
On Tue, Mar 19, 2024 at 01:29:22AM +0100, Vasileios Amoiridis 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 read_{temp,press,humid}() > 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.
On Tue, 19 Mar 2024 01:29:22 +0100 Vasileios Amoiridis <vassilisamir@gmail.com> 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@gmail.com> 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 > --- > 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
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@gmail.com> 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@gmail.com> > 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
On Tue, 2 Apr 2024 19:55:53 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > 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@gmail.com> 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@gmail.com> > > 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. From a quick look there isn't the same issue with hidden data, but if it makes sense from the point of view of consistency that is fine. > > > > > > --- > > > 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
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); 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, + + .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); 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 */ - 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; } static int bmp380_read_calib(struct bmp280_data *data) @@ -1218,6 +1227,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, @@ -1244,6 +1255,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, @@ -1364,9 +1380,8 @@ 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; int ret; ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf, @@ -1376,25 +1391,17 @@ 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) { + *raw_temp = get_unaligned_le24(data->buf); + if (*raw_temp == BMP580_TEMP_SKIPPED) { dev_err(data->dev, "reading temperature skipped\n"); return -EIO; } - /* - * Temperature is returned in Celsius degrees in fractional - * form down 2^16. We rescale by x1000 to return milli Celsius - * to respect IIO ABI. - */ - *val = raw_temp * 1000; - *val2 = 16; - return IIO_VAL_FRACTIONAL_LOG2; + 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; int ret; ret = regmap_bulk_read(data->regmap, BMP580_REG_PRESS_XLSB, data->buf, @@ -1404,18 +1411,13 @@ 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) { + *raw_press = get_unaligned_le24(data->buf); + if (*raw_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; + + return 0; } static const int bmp580_odr_table[][2] = { @@ -1720,6 +1722,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, @@ -1746,6 +1750,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 = bmp280_temp_coeffs, + .temp_coeffs_type = IIO_VAL_FRACTIONAL_LOG2, + .press_coeffs = bmp280_press_coeffs, + .press_coeffs_type = IIO_VAL_FRACTIONAL, + .chip_config = bmp580_chip_config, .read_temp = bmp580_read_temp, .read_press = bmp580_read_press, @@ -1873,25 +1882,17 @@ static s32 bmp180_compensate_temp(struct bmp280_data *data, s32 adc_temp) return (data->t_fine + 8) >> 4; } -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 adc_temp, comp_temp; + s32 adc_temp; int ret; ret = bmp180_read_adc_temp(data, &adc_temp); if (ret) 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; - } + if (comp_temp) + *comp_temp = bmp180_compensate_temp(data, adc_temp); return 0; } @@ -1953,15 +1954,13 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, s32 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; 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_read_temp(data, NULL); if (ret) return ret; @@ -1969,12 +1968,9 @@ static int bmp180_read_press(struct bmp280_data *data, if (ret) return ret; - comp_press = bmp180_compensate_press(data, adc_press); - - *val = comp_press; - *val2 = 1000; + *comp_press = bmp180_compensate_press(data, adc_press); - return IIO_VAL_FRACTIONAL; + return 0; } static int bmp180_chip_config(struct bmp280_data *data) @@ -1985,6 +1981,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, @@ -2005,6 +2003,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 4012387d7956..6d1dca31dd52 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -448,10 +448,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 *); - int (*read_temp)(struct bmp280_data *, int *, int *); - int (*read_press)(struct bmp280_data *, int *, int *); - int (*read_humid)(struct bmp280_data *, int *, int *); + int (*read_temp)(struct bmp280_data *, s32 *); + int (*read_press)(struct bmp280_data *, u32 *); + int (*read_humid)(struct bmp280_data *, u32 *); int (*read_calib)(struct bmp280_data *); int (*preinit)(struct bmp280_data *); };
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@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 189 +++++++++++++++-------------- drivers/iio/pressure/bmp280.h | 13 +- 2 files changed, 106 insertions(+), 96 deletions(-)