Received-SPF: pass (google.com: domain of linux-kernel+bounces-198235-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sun, 2 Jun 2024 13:41:06 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Vasileios Amoiridis <vassilisamir@gmail.com>
Cc: lars@metafoo.de, himanshujha199640@gmail.com, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 04/17] iio: chemical: bme680: Fix sensor data read
 operation
Message-ID: <20240602134106.2538471a@jic23-huawei>
In-Reply-To: <20240527183805.311501-5-vassilisamir@gmail.com>
References: <20240527183805.311501-1-vassilisamir@gmail.com>
	<20240527183805.311501-5-vassilisamir@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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.

> +	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");