A read operation is happening as follows:
a) Set sensor to forced mode
b) Sensor measures values and update data registers and sleeps again
c) Read data registers
In the current implementation the read operation happens immediately
after the sensor is set to forced mode so the sensor does not have
the time to update properly the registers. This leads to the following
2 problems:
1) The first ever value which is read by the register is always wrong
2) Every read operation, puts the register into forced mode and reads
the data that were calculated in the previous conversion.
This behaviour was tested in 2 ways:
1) The internal meas_status_0 register was read before and after every
read operation in order to verify that the data were ready even before
the register was set to forced mode and also to check that after the
forced mode was set the new data were not yet ready.
2) Physically changing the temperature and measuring the temperature
This commit adds the waiting time in between the set of the forced mode
and the read of the data. The function is taken from the Bosch BME68x
Sensor API [1].
[1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L490
Fixes: 1b3bd8592780 ("iio: chemical: Add support for Bosch BME680 sensor")
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
drivers/iio/chemical/bme680_core.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 5db48f6d646c..dd2cd11b6dd3 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -10,6 +10,7 @@
*/
#include <linux/acpi.h>
#include <linux/bitfield.h>
+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/module.h>
#include <linux/log2.h>
@@ -532,6 +533,26 @@ static u8 bme680_oversampling_to_reg(u8 val)
return ilog2(val) + 1;
}
+/*
+ * Taken from Bosch BME680 API:
+ * https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L490
+ */
+static int bme680_conversion_time_us(u8 meas, u8 dur)
+{
+ /* Oversampling + TPH meas + Gas meas + Forced mode + heater duration */
+ return (meas * 1936) + (477 * 4) + (477 * 5) + 1000 + (dur * 1000);
+}
+
+static void bme680_wait_for_eoc(struct bme680_data *data)
+{
+ int wait_eoc = bme680_conversion_time_us(data->oversampling_temp +
+ data->oversampling_press +
+ data->oversampling_press,
+ data->heater_dur);
+
+ usleep_range(wait_eoc, wait_eoc + 100);
+}
+
static int bme680_chip_config(struct bme680_data *data)
{
struct device *dev = regmap_get_device(data->regmap);
@@ -622,6 +643,8 @@ static int bme680_read_temp(struct bme680_data *data, int *val)
if (ret < 0)
return ret;
+ bme680_wait_for_eoc(data);
+
ret = regmap_bulk_read(data->regmap, BME680_REG_TEMP_MSB,
&tmp, 3);
if (ret < 0) {
@@ -738,6 +761,8 @@ static int bme680_read_gas(struct bme680_data *data,
if (ret < 0)
return ret;
+ bme680_wait_for_eoc(data);
+
ret = regmap_read(data->regmap, BME680_REG_MEAS_STAT_0, &check);
if (check & BME680_GAS_MEAS_BIT) {
dev_err(dev, "gas measurement incomplete\n");
--
2.25.1On Mon, 27 May 2024 20:37:52 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> A read operation is happening as follows:
>
> a) Set sensor to forced mode
> b) Sensor measures values and update data registers and sleeps again
> c) Read data registers
>
> In the current implementation the read operation happens immediately
> after the sensor is set to forced mode so the sensor does not have
> the time to update properly the registers. This leads to the following
> 2 problems:
>
> 1) The first ever value which is read by the register is always wrong
> 2) Every read operation, puts the register into forced mode and reads
> the data that were calculated in the previous conversion.
>
> This behaviour was tested in 2 ways:
>
> 1) The internal meas_status_0 register was read before and after every
> read operation in order to verify that the data were ready even before
> the register was set to forced mode and also to check that after the
> forced mode was set the new data were not yet ready.
>
> 2) Physically changing the temperature and measuring the temperature
>
> This commit adds the waiting time in between the set of the forced mode
> and the read of the data. The function is taken from the Bosch BME68x
> Sensor API [1].
>
> [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L490
> Fixes: 1b3bd8592780 ("iio: chemical: Add support for Bosch BME680 sensor")
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
> drivers/iio/chemical/bme680_core.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 5db48f6d646c..dd2cd11b6dd3 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -10,6 +10,7 @@
> */
> #include <linux/acpi.h>
> #include <linux/bitfield.h>
> +#include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/module.h>
> #include <linux/log2.h>
> @@ -532,6 +533,26 @@ static u8 bme680_oversampling_to_reg(u8 val)
> return ilog2(val) + 1;
> }
>
> +/*
> + * Taken from Bosch BME680 API:
> + * https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L490
> + */
> +static int bme680_conversion_time_us(u8 meas, u8 dur)
> +{
> + /* Oversampling + TPH meas + Gas meas + Forced mode + heater duration */
I'd break oversampling up
/* (Oversampling ratio * time per reading) ...
or something along those lines because it's related to oversampling but isn't
of itself oversampling.
> + return (meas * 1936) + (477 * 4) + (477 * 5) + 1000 + (dur * 1000);
Trivial but I think we can rely on precedence both for correctness and readability
and hence don't need the brackets
> +}
> +
> +static void bme680_wait_for_eoc(struct bme680_data *data)
Don't call it wait as that implies something is being checked.
bme680_conversion_sleep() or something like that.
> +{
> + int wait_eoc = bme680_conversion_time_us(data->oversampling_temp +
> + data->oversampling_press +
> + data->oversampling_press,
> + data->heater_dur);
I'd pull the calculation inline in here unless you are going to use it elsewhere
in later patches.
> +
> + usleep_range(wait_eoc, wait_eoc + 100);
> +}
> +
> static int bme680_chip_config(struct bme680_data *data)
> {
> struct device *dev = regmap_get_device(data->regmap);
> @@ -622,6 +643,8 @@ static int bme680_read_temp(struct bme680_data *data, int *val)
> if (ret < 0)
> return ret;
>
> + bme680_wait_for_eoc(data);
> +
> ret = regmap_bulk_read(data->regmap, BME680_REG_TEMP_MSB,
> &tmp, 3);
> if (ret < 0) {
> @@ -738,6 +761,8 @@ static int bme680_read_gas(struct bme680_data *data,
> if (ret < 0)
> return ret;
>
> + bme680_wait_for_eoc(data);
> +
> ret = regmap_read(data->regmap, BME680_REG_MEAS_STAT_0, &check);
> if (check & BME680_GAS_MEAS_BIT) {
> dev_err(dev, "gas measurement incomplete\n");
On Sun, Jun 02, 2024 at 01:41:06PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 20:37:52 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
>
> > A read operation is happening as follows:
> >
> > a) Set sensor to forced mode
> > b) Sensor measures values and update data registers and sleeps again
> > c) Read data registers
> >
> > In the current implementation the read operation happens immediately
> > after the sensor is set to forced mode so the sensor does not have
> > the time to update properly the registers. This leads to the following
> > 2 problems:
> >
> > 1) The first ever value which is read by the register is always wrong
> > 2) Every read operation, puts the register into forced mode and reads
> > the data that were calculated in the previous conversion.
> >
> > This behaviour was tested in 2 ways:
> >
> > 1) The internal meas_status_0 register was read before and after every
> > read operation in order to verify that the data were ready even before
> > the register was set to forced mode and also to check that after the
> > forced mode was set the new data were not yet ready.
> >
> > 2) Physically changing the temperature and measuring the temperature
> >
> > This commit adds the waiting time in between the set of the forced mode
> > and the read of the data. The function is taken from the Bosch BME68x
> > Sensor API [1].
> >
> > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L490
> > Fixes: 1b3bd8592780 ("iio: chemical: Add support for Bosch BME680 sensor")
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> > drivers/iio/chemical/bme680_core.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > index 5db48f6d646c..dd2cd11b6dd3 100644
> > --- a/drivers/iio/chemical/bme680_core.c
> > +++ b/drivers/iio/chemical/bme680_core.c
> > @@ -10,6 +10,7 @@
> > */
> > #include <linux/acpi.h>
> > #include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > #include <linux/device.h>
> > #include <linux/module.h>
> > #include <linux/log2.h>
> > @@ -532,6 +533,26 @@ static u8 bme680_oversampling_to_reg(u8 val)
> > return ilog2(val) + 1;
> > }
> >
> > +/*
> > + * Taken from Bosch BME680 API:
> > + * https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L490
> > + */
> > +static int bme680_conversion_time_us(u8 meas, u8 dur)
> > +{
> > + /* Oversampling + TPH meas + Gas meas + Forced mode + heater duration */
> I'd break oversampling up
>
> /* (Oversampling ratio * time per reading) ...
> or something along those lines because it's related to oversampling but isn't
> of itself oversampling.
>
Ok I see , makes sense.
> > + return (meas * 1936) + (477 * 4) + (477 * 5) + 1000 + (dur * 1000);
>
> Trivial but I think we can rely on precedence both for correctness and readability
> and hence don't need the brackets
>
The reason I used the parentheses was because in my eyes it's easier to read what's
happening exactly. But I can also remove them, there is no problem.
> > +}
> > +
> > +static void bme680_wait_for_eoc(struct bme680_data *data)
> Don't call it wait as that implies something is being checked.
>
> bme680_conversion_sleep() or something like that.
>
This sesnor has a sleep mode, so I think calling a function like that might
make it a bit confusing, since we are not putting the sensor into sleeping
mode but rather actually wait for the eoc.
> > +{
> > + int wait_eoc = bme680_conversion_time_us(data->oversampling_temp +
> > + data->oversampling_press +
> > + data->oversampling_press,
> > + data->heater_dur);
>
> I'd pull the calculation inline in here unless you are going to use it elsewhere
> in later patches.
>
Ok, I could merge them into one, I don't think there is a problem.
> > +
> > + usleep_range(wait_eoc, wait_eoc + 100);
> > +}
> > +
> > static int bme680_chip_config(struct bme680_data *data)
> > {
> > struct device *dev = regmap_get_device(data->regmap);
> > @@ -622,6 +643,8 @@ static int bme680_read_temp(struct bme680_data *data, int *val)
> > if (ret < 0)
> > return ret;
> >
> > + bme680_wait_for_eoc(data);
> > +
> > ret = regmap_bulk_read(data->regmap, BME680_REG_TEMP_MSB,
> > &tmp, 3);
> > if (ret < 0) {
> > @@ -738,6 +761,8 @@ static int bme680_read_gas(struct bme680_data *data,
> > if (ret < 0)
> > return ret;
> >
> > + bme680_wait_for_eoc(data);
> > +
> > ret = regmap_read(data->regmap, BME680_REG_MEAS_STAT_0, &check);
> > if (check & BME680_GAS_MEAS_BIT) {
> > dev_err(dev, "gas measurement incomplete\n");
>
> > > +}
> > > +
> > > +static void bme680_wait_for_eoc(struct bme680_data *data)
> > Don't call it wait as that implies something is being checked.
> >
> > bme680_conversion_sleep() or something like that.
> >
>
> This sesnor has a sleep mode, so I think calling a function like that might
> make it a bit confusing, since we are not putting the sensor into sleeping
> mode but rather actually wait for the eoc.
Hmm. Bikesheding time. I don't like wait because it generally implies a
condition check.
Maybe just go more explicit
bme680_suspend_execution_until_conversion_done()
bme680_suspend_execution_whilst_converting()
or similar might be overkill but something along those lines.
The suspend execution terminology lifted from man usleep
>
> > > +{
> > > + int wait_eoc = bme680_conversion_time_us(data->oversampling_temp +
> > > + data->oversampling_press +
> > > + data->oversampling_press,
> > > + data->heater_dur);
On Mon, Jun 03, 2024 at 08:23:03PM +0100, Jonathan Cameron wrote:
>
> > > > +}
> > > > +
> > > > +static void bme680_wait_for_eoc(struct bme680_data *data)
> > > Don't call it wait as that implies something is being checked.
> > >
> > > bme680_conversion_sleep() or something like that.
> > >
> >
> > This sesnor has a sleep mode, so I think calling a function like that might
> > make it a bit confusing, since we are not putting the sensor into sleeping
> > mode but rather actually wait for the eoc.
>
> Hmm. Bikesheding time. I don't like wait because it generally implies a
> condition check.
>
> Maybe just go more explicit
> bme680_suspend_execution_until_conversion_done()
> bme680_suspend_execution_whilst_converting()
> or similar might be overkill but something along those lines.
>
> The suspend execution terminology lifted from man usleep
>
Well actually I realised that I can do some condition checks so I will resubmit
and we discuss again with the new feature.
Cheers,
Vasilis
> >
> > > > +{
> > > > + int wait_eoc = bme680_conversion_time_us(data->oversampling_temp +
> > > > + data->oversampling_press +
> > > > + data->oversampling_press,
> > > > + data->heater_dur);
© 2016 - 2026 Red Hat, Inc.