| Message ID | 20240313174007.1934983-5-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Series to add triggered buffer support to BMP280 driver | expand |
On Wed, Mar 13, 2024 at 06:40:05PM +0100, Vasileios Amoiridis wrote: > The read_press/read_humid functions need the updated t_fine value read_press()/read_humid() > in order to calculate the current pressure/humidity. Temperature > reads should be removed from the read_press/read_humid functions read_press()/read_humid() > and should be placed in the oneshot captures before the pressure > and humidity reads. This makes the code more intuitive. ... > + if (strcmp(indio_dev->name, "bmp580")) > + data->chip_info->read_temp(data); > + > + if (strcmp(indio_dev->name, "bmp580")) > + data->chip_info->read_temp(data); Yeah, not a fan of the strcmp().
On Wed, 13 Mar 2024 21:04:46 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Mar 13, 2024 at 06:40:05PM +0100, Vasileios Amoiridis wrote: > > The read_press/read_humid functions need the updated t_fine value > > read_press()/read_humid() > > > > in order to calculate the current pressure/humidity. Temperature > > reads should be removed from the read_press/read_humid functions > > read_press()/read_humid() > > > and should be placed in the oneshot captures before the pressure > > and humidity reads. This makes the code more intuitive. > > ... > > > + if (strcmp(indio_dev->name, "bmp580")) > > + data->chip_info->read_temp(data); > > + > > > + if (strcmp(indio_dev->name, "bmp580")) > > + data->chip_info->read_temp(data); > > Yeah, not a fan of the strcmp(). > Yes - that's a non starter. Add a flag to say it is necessary to chip_info that doesn't rely on name matching. If you do it the way you have here, you have to add another strcmp for each new device supported that needs this code to run. Add a flag and you just set that in the chip_info structure. Much more flexible and extensible. Usual description of this is "when you can do things with data rather than code, do it with data". Jonathan
On Wed, 13 Mar 2024 18:40:05 +0100 Vasileios Amoiridis <vassilisamir@gmail.com> 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@gmail.com> 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;
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@gmail.com> 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@gmail.com> > > 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; >
On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote: > 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@gmail.com> 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@gmail.com> > > > > 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! > Hi, just a side note reflecting on this. Depending on your sampling frequency and registers data shadowing, to avoid compensating with different samples between readings, we should be doing burst readings to get a bundle of the temperature+pressure and/or humidity. On the bmp/bme280 and bmp380 this can be done as registers are contiguous on the memory. On the bmp580 this is not a problem as the values are already compensated, you`ll get always the latest reading. Kind regard, Angel
On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote: > On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote: > > 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@gmail.com> 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@gmail.com> > > > > > > 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! > > > > Hi, just a side note reflecting on this. Depending on your sampling frequency > and registers data shadowing, to avoid compensating with different samples > between readings, we should be doing burst readings to get a bundle of the > temperature+pressure and/or humidity. > On the bmp/bme280 and bmp380 this can be done as registers are contiguous on the > memory. On the bmp580 this is not a problem as the values are already > compensated, you`ll get always the latest reading. > > Kind regard, > Angel Hi Angel, Thank you for pointing this out! Indeed that's true but I noticed that this is not the case for the BMP{085/180} devices. I just feel that some changes might make data acquisition/processing faster for a device (like the one you proposed) but it might make the code much more unreadable and unmaintanable. I will try and see if something could be done in that sense but I feel that keeping it simple will be good for everyone! Cheers, Vasilis
On Fri, 2024-03-15 at 10:05 +0100, Vasileios Amoiridis wrote: > On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote: > > On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote: > > > 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@gmail.com> 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@gmail.com> > > > > > > > > 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! > > > > > > > Hi, just a side note reflecting on this. Depending on your sampling > > frequency > > and registers data shadowing, to avoid compensating with different samples > > between readings, we should be doing burst readings to get a bundle of the > > temperature+pressure and/or humidity. > > On the bmp/bme280 and bmp380 this can be done as registers are contiguous on > > the > > memory. On the bmp580 this is not a problem as the values are already > > compensated, you`ll get always the latest reading. > > > > Kind regard, > > Angel > > Hi Angel, > > Thank you for pointing this out! Indeed that's true but I noticed that this is > not > the case for the BMP{085/180} devices. I just feel that some changes might > make > data acquisition/processing faster for a device (like the one you proposed) > but > it might make the code much more unreadable and unmaintanable. I will try and > see if something could be done in that sense but I feel that keeping it simple > will > be good for everyone! > > Cheers, > Vasilis Yeah, data adquisition on bmp085/180 is already different as they don't support continuous mode as the newer models and you have to warm-up the sensor and do one-shot readings. There's already a different code path in place for that models. I guess that is the price to pay to support that wide range of sensors... Anyway, this patches are already big and you're doing quite a lot of heavy- lifting right now, so don't pay much attention to my ramblings! Regardless, happy to help with tasks polishing and updating this driver :) Kind regards, Angel
On Fri, 15 Mar 2024 14:28:30 +0100 Angel Iglesias <ang.iglesiasg@gmail.com> wrote: > On Fri, 2024-03-15 at 10:05 +0100, Vasileios Amoiridis wrote: > > On Fri, Mar 15, 2024 at 12:22:50AM +0100, Angel Iglesias wrote: > > > On Thu, 2024-03-14 at 21:17 +0100, Vasileios Amoiridis wrote: > > > > 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@gmail.com> 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@gmail.com> > > > > > > > > > > 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! > > > > > > > > > > Hi, just a side note reflecting on this. Depending on your sampling > > > frequency > > > and registers data shadowing, to avoid compensating with different samples > > > between readings, we should be doing burst readings to get a bundle of the > > > temperature+pressure and/or humidity. > > > On the bmp/bme280 and bmp380 this can be done as registers are contiguous on > > > the > > > memory. On the bmp580 this is not a problem as the values are already > > > compensated, you`ll get always the latest reading. > > > > > > Kind regard, > > > Angel > > > > Hi Angel, > > > > Thank you for pointing this out! Indeed that's true but I noticed that this is > > not > > the case for the BMP{085/180} devices. I just feel that some changes might > > make > > data acquisition/processing faster for a device (like the one you proposed) > > but > > it might make the code much more unreadable and unmaintanable. I will try and > > see if something could be done in that sense but I feel that keeping it simple > > will > > be good for everyone! > > > > Cheers, > > Vasilis > > Yeah, data adquisition on bmp085/180 is already different as they don't support > continuous mode as the newer models and you have to warm-up the sensor and do > one-shot readings. There's already a different code path in place for that > models. I guess that is the price to pay to support that wide range of > sensors... > Anyway, this patches are already big and you're doing quite a lot of heavy- > lifting right now, so don't pay much attention to my ramblings! Regardless, > happy to help with tasks polishing and updating this driver :) If burst readings do make sense: We can reasonably assume anyone who is using this sensor and is using buffered mode probably wants to 'mostly' grab all the channels, then a specific function that always grabs them all, plus use of available_scan_masks = { ALL BITS, 0 }; will let the IIO core deal with any cases where only some channels are requested. This is something we do in drivers where there is some interaction between the channels (like here) or where burst reads are much more efficient than single channels (possibly also true here) and complexity is significant for switching between burst and single channels reads. That covers a lot of devices and is part of why we have the core code do channel de-multiplexing rather than leaving it for the drivers. The other reason that drove that complexity was unrelated to this driver (SoC ADCs with some channels used for touchscreens, and others for unrelated purposes). You may just want to reduce how much code you are reusing from the oneshot single channel sysfs reads so that you can just do a single set of readings and use them as needed for compensation etc. Jonathan > > Kind regards, > Angel >
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;
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@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 38 ++++++++++++++---------------- 1 file changed, 18 insertions(+), 20 deletions(-)