Received-SPF: pass (google.com: domain of linux-kernel+bounces-198246-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 2 Jun 2024 14:01:23 +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 14/17] iio: chemical: bme680: Modify startup
 procedure
Message-ID: <20240602140123.3dd1b793@jic23-huawei>
In-Reply-To: <20240527183805.311501-15-vassilisamir@gmail.com>
References: <20240527183805.311501-1-vassilisamir@gmail.com>
	<20240527183805.311501-15-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:38:02 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Modify the startup procedure to reflect the procedure of
> the Bosch BME68x Sensor API. The initial readings and
> configuration of the sensor need to happen in the
> following order:
> 
> 1) Read calibration data [1,2]
> 2) Chip general configuration [3]
> 3) Gas configuration [4]
> 
> After the chip configuration it is necessary to ensure that
> the sensor is in sleeping mode, in order to apply the gas
> configuration settings [5].
> 
> Also, after the soft reset, it is advised to wait for 5ms [6].
> 
> [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L162
> [2]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/examples/forced_mode/forced_mode.c#L44
> [3]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/examples/forced_mode/forced_mode.c#L53
> [4]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/examples/forced_mode/forced_mode.c#L60
> [5]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L640
> [6]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L294
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/chemical/bme680.h      |  2 ++
>  drivers/iio/chemical/bme680_core.c | 27 ++++++++++++++++++---------
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
> index 17ea59253923..3be2f76a5bfb 100644
> --- a/drivers/iio/chemical/bme680.h
> +++ b/drivers/iio/chemical/bme680.h
> @@ -61,6 +61,8 @@
>  
>  #define BME680_MEAS_TRIM_MASK			GENMASK(24, 4)
>  
> +#define BME680_STARTUP_TIME_US			5000
> +
>  /* Calibration Parameters */
>  #define BME680_T2_LSB_REG	0x8A
>  #define BME680_H2_MSB_REG	0xE1
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index b055eeee8f1c..afaa43ec3241 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -505,10 +505,12 @@ static int bme680_chip_config(struct bme680_data *data)
>  	ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
>  				BME680_OSRS_TEMP_MASK | BME680_OSRS_PRESS_MASK,
>  				osrs);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(dev, "failed to write ctrl_meas register\n");
> +		return ret;
> +	}
>  
> -	return ret;
> +	return 0;
>  }

I think this is an unrelated change so if you want to do this - different patch.

>  
>  static int bme680_gas_config(struct bme680_data *data)
> @@ -517,6 +519,11 @@ static int bme680_gas_config(struct bme680_data *data)
>  	int ret;
>  	u8 heatr_res, heatr_dur;
>  
> +	/* Go to sleep */
> +	ret = bme680_set_mode(data, false);
> +	if (ret < 0)
> +		return ret;
> +
>  	heatr_res = bme680_calc_heater_res(data, data->heater_temp);
>  
>  	/* set target heater temperature */
> @@ -847,6 +854,8 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
>  		return ret;
>  	}
>  
> +	usleep_range(BME680_STARTUP_TIME_US, BME680_STARTUP_TIME_US + 1000);
> +
>  	ret = regmap_read(regmap, BME680_REG_CHIP_ID, &data->check);
>  	if (ret < 0) {
>  		dev_err(dev, "Error reading chip ID\n");
> @@ -859,22 +868,22 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
>  		return -ENODEV;
>  	}
>  
> -	ret = bme680_chip_config(data);
> +	ret = bme680_read_calib(data, &data->bme680);
>  	if (ret < 0) {
> -		dev_err(dev, "failed to set chip_config data\n");
> +		dev_err(dev,
> +			"failed to read calibration coefficients at probe\n");
>  		return ret;

Maybe you have it in a later patch (it definitely wants to be a different patch from
this one as different issue), but feels like a bunch of places where
dev_err_probe() would be good.

>  	}
>  
> -	ret = bme680_gas_config(data);
> +	ret = bme680_chip_config(data);
>  	if (ret < 0) {
> -		dev_err(dev, "failed to set gas config data\n");
> +		dev_err(dev, "failed to set chip_config data\n");
>  		return ret;
>  	}
>  
> -	ret = bme680_read_calib(data, &data->bme680);
> +	ret = bme680_gas_config(data);
>  	if (ret < 0) {
> -		dev_err(dev,
> -			"failed to read calibration coefficients at probe\n");
> +		dev_err(dev, "failed to set gas config data\n");
>  		return ret;
>  	}
>