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@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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"); >
