linux-kernel - Re: [PATCH v1 14/17] iio: chemical: bme680: Modify startup procedure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 2 Jun 2024 21:40:42 +0200
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Vasileios Amoiridis <vassilisamir@...il.com>, lars@...afoo.de,
	himanshujha199640@...il.com, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 14/17] iio: chemical: bme680: Modify startup procedure

On Sun, Jun 02, 2024 at 02:01:23PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 20:38:02 +0200
> Vasileios Amoiridis <vassilisamir@...il.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@...il.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.
> 

Well, it is not completely unrelated. This function is only doing regmap_reads() 
and after every regmap in case of error it returns in the if statement that exists
after the regmap_read(). In the last check though, instead of exiting inside the if
statement it just sends a dev_err() message, exits the if() and then exits from
the last return. Functionality is the same, it is just not consistent. But I could
split it in 2 commits, no problem!

> >  
> >  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.
> 

Well, since they are in the probe function I guess I could also change those to
dev_err_probe() in a separate commit.

Cheers,
Vasilis

> >  	}
> >  
> > -	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;
> >  	}
> >  
>