[PATCH v1 00/17] iio: chemical: bme680: Driver cleanup

[PATCH v1 00/17] iio: chemical: bme680: Driver cleanup

[Vasileios Amoiridis]

This started as a series to add support for buffers and the new
BME688 but it ended up being just a cleaning series. These might
be quite some patches for such a thing but I feel that they are
are well split, in order to allow for better review.

The patches are mostly small changes but essential for the correct use
of the driver. The first patches looked like fixes that should be
marked for the stable. Patches [11,17/17] might be a bit bigger but 11/17
is quite straightforward and 17/17 is basically a duplication of a
very similar commit coming from the BMP280 driver [1].

In general, the datasheet [2] of the driver is not very descriptive,
and it redirects the user to the BME68x Sensor API [3]. All the things
that were identified from the BME68x Sensor API have been marked with
links to the original locations of the GitHub code. If this is too much
and we don't want this type of information on the commit message, please
let me know and I will fix it.

[1]: https://lore.kernel.org/linux-iio/20240512230524.53990-1-vassilisamir@gmail.com/T/#mc6f814e9a4f8c2b39015909d174c7013b3648b9b
[2]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bme680-ds001.pdf
[3]: https://github.com/boschsensortec/BME68x_SensorAPI/tree/master

Vasileios Amoiridis (17):
  iio: chemical: bme680: Fix pressure value output
  iio: chemical: bme680: Fix calibration data variable
  iio: chemical: bme680: Fix overflows in compensate() functions
  iio: chemical: bme680: Fix sensor data read operation
  iio: chemical: bme680: Fix type in define
  iio: chemical: bme680: Add mutexes to guard read/write to device
  iio: chemical: bme680: Drop unnecessary casts and correct adc data
    types
  iio: chemical: bme680: Remove remaining ACPI-only stuff
  iio: chemical: bme680: Sort headers alphabetically
  iio: chemical: bme680: Remove duplicate register read
  iio: chemical: bme680: Use bulk reads for calibration data
  iio: chemical: bme680: Allocate IIO device before chip initialization
  iio: chemical: bme680: Add read buffers in DMA safe region
  iio: chemical: bme680: Modify startup procedure
  iio: chemical: bme680: Remove redundant gas configuration
  iio: chemical: bme680: Move forced mode setup in ->read_raw()
  iio: chemical: bme680: Refactorize reading functions

 drivers/iio/chemical/bme680.h      |  39 +-
 drivers/iio/chemical/bme680_core.c | 643 ++++++++++++++---------------
 2 files changed, 315 insertions(+), 367 deletions(-)


base-commit: 409b6d632f5078f3ae1018b6e43c32f2e12f6736
-- 
2.25.1

[PATCH v1 01/17] iio: chemical: bme680: Fix pressure value output

[Vasileios Amoiridis]

The IIO standard units are measured in kPa while the driver
is using hPa.

Apart from checking the userspace value itself, it is mentioned also
in the Bosch API [1] that the pressure value is in Pascal.

[1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x_defs.h#L742
Fixes: 1b3bd8592780 ("iio: chemical: Add support for Bosch BME680 sensor")
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index ef5e0e46fd34..2c40c13fe97a 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -678,7 +678,7 @@ static int bme680_read_press(struct bme680_data *data,
 	}
 
 	*val = bme680_compensate_press(data, adc_press);
-	*val2 = 100;
+	*val2 = 1000;
 	return IIO_VAL_FRACTIONAL;
 }
 
-- 
2.25.1

[PATCH v1 02/17] iio: chemical: bme680: Fix calibration data variable

[Vasileios Amoiridis]

According to the BME68x Sensor API [1], the h6 calibration
data variable should be an unsigned integer of size 8.

[1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x_defs.h#L789
Fixes: 1b3bd8592780 ("iio: chemical: Add support for Bosch BME680 sensor")
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 2c40c13fe97a..812829841733 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -38,7 +38,7 @@ struct bme680_calib {
 	s8  par_h3;
 	s8  par_h4;
 	s8  par_h5;
-	s8  par_h6;
+	u8  par_h6;
 	s8  par_h7;
 	s8  par_gh1;
 	s16 par_gh2;
-- 
2.25.1

[PATCH v1 03/17] iio: chemical: bme680: Fix overflows in compensate() functions

[Vasileios Amoiridis]

There are cases in the compensate functions of the driver that
there could be overflows of variables due to bit shifting ops.
These implications were initially discussed here [1] and they
were mentioned in log message of Commit 1b3bd8592780 ("iio:
chemical: Add support for Bosch BME680 sensor").

[1]: https://lore.kernel.org/linux-iio/20180728114028.3c1bbe81@archlinux/
Fixes: 1b3bd8592780 ("iio: chemical: Add support for Bosch BME680 sensor")
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 812829841733..5db48f6d646c 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -342,10 +342,10 @@ static s16 bme680_compensate_temp(struct bme680_data *data,
 	if (!calib->par_t2)
 		bme680_read_calib(data, calib);
 
-	var1 = (adc_temp >> 3) - (calib->par_t1 << 1);
+	var1 = (adc_temp >> 3) - ((s32)calib->par_t1 << 1);
 	var2 = (var1 * calib->par_t2) >> 11;
 	var3 = ((var1 >> 1) * (var1 >> 1)) >> 12;
-	var3 = (var3 * (calib->par_t3 << 4)) >> 14;
+	var3 = (var3 * ((s32)calib->par_t3 << 4)) >> 14;
 	data->t_fine = var2 + var3;
 	calc_temp = (data->t_fine * 5 + 128) >> 8;
 
@@ -368,9 +368,9 @@ static u32 bme680_compensate_press(struct bme680_data *data,
 	var1 = (data->t_fine >> 1) - 64000;
 	var2 = ((((var1 >> 2) * (var1 >> 2)) >> 11) * calib->par_p6) >> 2;
 	var2 = var2 + (var1 * calib->par_p5 << 1);
-	var2 = (var2 >> 2) + (calib->par_p4 << 16);
+	var2 = (var2 >> 2) + ((s32)calib->par_p4 << 16);
 	var1 = (((((var1 >> 2) * (var1 >> 2)) >> 13) *
-			(calib->par_p3 << 5)) >> 3) +
+			((s32)calib->par_p3 << 5)) >> 3) +
 			((calib->par_p2 * var1) >> 1);
 	var1 = var1 >> 18;
 	var1 = ((32768 + var1) * calib->par_p1) >> 15;
@@ -388,7 +388,7 @@ static u32 bme680_compensate_press(struct bme680_data *data,
 	var3 = ((press_comp >> 8) * (press_comp >> 8) *
 			(press_comp >> 8) * calib->par_p10) >> 17;
 
-	press_comp += (var1 + var2 + var3 + (calib->par_p7 << 7)) >> 4;
+	press_comp += (var1 + var2 + var3 + ((s32)calib->par_p7 << 7)) >> 4;
 
 	return press_comp;
 }
@@ -414,7 +414,7 @@ static u32 bme680_compensate_humid(struct bme680_data *data,
 		 (((temp_scaled * ((temp_scaled * calib->par_h5) / 100))
 		   >> 6) / 100) + (1 << 14))) >> 10;
 	var3 = var1 * var2;
-	var4 = calib->par_h6 << 7;
+	var4 = (s32)calib->par_h6 << 7;
 	var4 = (var4 + ((temp_scaled * calib->par_h7) / 100)) >> 4;
 	var5 = ((var3 >> 14) * (var3 >> 14)) >> 10;
 	var6 = (var4 * var5) >> 1;
-- 
2.25.1

[PATCH v1 04/17] iio: chemical: bme680: Fix sensor data read operation

[Vasileios Amoiridis]

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

Re: [PATCH v1 04/17] iio: chemical: bme680: Fix sensor data read operation

[Jonathan Cameron]

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

Re: [PATCH v1 04/17] iio: chemical: bme680: Fix sensor data read operation

[Vasileios Amoiridis]

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

Re: [PATCH v1 04/17] iio: chemical: bme680: Fix sensor data read operation

[Jonathan Cameron]

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

Re: [PATCH v1 04/17] iio: chemical: bme680: Fix sensor data read operation

[Vasileios Amoiridis]

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

[PATCH v1 05/17] iio: chemical: bme680: Fix type in define

[Vasileios Amoiridis]

Fix a define typo that instead of BME680_... it is
saying BM6880_...

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680.h      | 2 +-
 drivers/iio/chemical/bme680_core.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index 4edc5d21cb9f..3133d624270a 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -12,7 +12,7 @@
 
 #define BME680_REG_TEMP_MSB			0x22
 #define BME680_REG_PRESS_MSB			0x1F
-#define BM6880_REG_HUMIDITY_MSB			0x25
+#define BME680_REG_HUMIDITY_MSB			0x25
 #define BME680_REG_GAS_MSB			0x2A
 #define BME680_REG_GAS_R_LSB			0x2B
 #define   BME680_GAS_STAB_BIT			BIT(4)
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index dd2cd11b6dd3..8b42c4716412 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -719,7 +719,7 @@ static int bme680_read_humid(struct bme680_data *data,
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BM6880_REG_HUMIDITY_MSB,
+	ret = regmap_bulk_read(data->regmap, BME680_REG_HUMIDITY_MSB,
 			       &tmp, sizeof(tmp));
 	if (ret < 0) {
 		dev_err(dev, "failed to read humidity\n");
-- 
2.25.1

Re: [PATCH v1 05/17] iio: chemical: bme680: Fix type in define

[Jonathan Cameron]

On Mon, 27 May 2024 20:37:53 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
Patch title "type" should be "typo". :)

> Fix a define typo that instead of BME680_... it is
> saying BM6880_...
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/chemical/bme680.h      | 2 +-
>  drivers/iio/chemical/bme680_core.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
> index 4edc5d21cb9f..3133d624270a 100644
> --- a/drivers/iio/chemical/bme680.h
> +++ b/drivers/iio/chemical/bme680.h
> @@ -12,7 +12,7 @@
>  
>  #define BME680_REG_TEMP_MSB			0x22
>  #define BME680_REG_PRESS_MSB			0x1F
> -#define BM6880_REG_HUMIDITY_MSB			0x25
> +#define BME680_REG_HUMIDITY_MSB			0x25
>  #define BME680_REG_GAS_MSB			0x2A
>  #define BME680_REG_GAS_R_LSB			0x2B
>  #define   BME680_GAS_STAB_BIT			BIT(4)
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index dd2cd11b6dd3..8b42c4716412 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -719,7 +719,7 @@ static int bme680_read_humid(struct bme680_data *data,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = regmap_bulk_read(data->regmap, BM6880_REG_HUMIDITY_MSB,
> +	ret = regmap_bulk_read(data->regmap, BME680_REG_HUMIDITY_MSB,
>  			       &tmp, sizeof(tmp));
>  	if (ret < 0) {
>  		dev_err(dev, "failed to read humidity\n");

Re: [PATCH v1 05/17] iio: chemical: bme680: Fix type in define

[Vasileios Amoiridis]

On Sun, Jun 02, 2024 at 01:41:50PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 20:37:53 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> Patch title "type" should be "typo". :)
> 

Ah, you are right thanks for pointing this out!

Cheers,
Vasilis

> > Fix a define typo that instead of BME680_... it is
> > saying BM6880_...
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/chemical/bme680.h      | 2 +-
> >  drivers/iio/chemical/bme680_core.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
> > index 4edc5d21cb9f..3133d624270a 100644
> > --- a/drivers/iio/chemical/bme680.h
> > +++ b/drivers/iio/chemical/bme680.h
> > @@ -12,7 +12,7 @@
> >  
> >  #define BME680_REG_TEMP_MSB			0x22
> >  #define BME680_REG_PRESS_MSB			0x1F
> > -#define BM6880_REG_HUMIDITY_MSB			0x25
> > +#define BME680_REG_HUMIDITY_MSB			0x25
> >  #define BME680_REG_GAS_MSB			0x2A
> >  #define BME680_REG_GAS_R_LSB			0x2B
> >  #define   BME680_GAS_STAB_BIT			BIT(4)
> > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > index dd2cd11b6dd3..8b42c4716412 100644
> > --- a/drivers/iio/chemical/bme680_core.c
> > +++ b/drivers/iio/chemical/bme680_core.c
> > @@ -719,7 +719,7 @@ static int bme680_read_humid(struct bme680_data *data,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	ret = regmap_bulk_read(data->regmap, BM6880_REG_HUMIDITY_MSB,
> > +	ret = regmap_bulk_read(data->regmap, BME680_REG_HUMIDITY_MSB,
> >  			       &tmp, sizeof(tmp));
> >  	if (ret < 0) {
> >  		dev_err(dev, "failed to read humidity\n");
> 

[PATCH v1 06/17] iio: chemical: bme680: Add mutexes to guard read/write to device

[Vasileios Amoiridis]

Add mutexes in the {read/write}_raw() functions of the device to
guard the read/write of data from/to the device. This is necessary
because for any operation other than temperature, multiple reads
need to take place from the device. Even though regmap has a locking
by itself, it won't protect us from multiple applications trying to
read at the same time temperature and pressure since the pressure
reading includes an internal temperature reading and there is nothing
to ensure that this temperature+pressure reading will happen
sequentially without any other operation interfering in the meantime.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 8b42c4716412..2ef3fc7effc6 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/cleanup.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/module.h>
@@ -52,6 +53,7 @@ struct bme680_calib {
 struct bme680_data {
 	struct regmap *regmap;
 	struct bme680_calib bme680;
+	struct mutex lock;
 	u8 oversampling_temp;
 	u8 oversampling_press;
 	u8 oversampling_humid;
@@ -806,6 +808,8 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
 {
 	struct bme680_data *data = iio_priv(indio_dev);
 
+	guard(mutex)(&data->lock);
+
 	switch (mask) {
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
@@ -850,6 +854,8 @@ static int bme680_write_raw(struct iio_dev *indio_dev,
 {
 	struct bme680_data *data = iio_priv(indio_dev);
 
+	guard(mutex)(&data->lock);
+
 	if (val2 != 0)
 		return -EINVAL;
 
@@ -946,6 +952,7 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		name = bme680_match_acpi_device(dev);
 
 	data = iio_priv(indio_dev);
+	mutex_init(&data->lock);
 	dev_set_drvdata(dev, indio_dev);
 	data->regmap = regmap;
 	indio_dev->name = name;
-- 
2.25.1

[PATCH v1 07/17] iio: chemical: bme680: Drop unnecessary casts and correct adc data types

[Vasileios Amoiridis]

Delete any redundant casts in the code and use unsigned integers for
the raw adc values.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 2ef3fc7effc6..7607c3167e24 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -335,7 +335,7 @@ static int bme680_read_calib(struct bme680_data *data,
  * output value of "3233" represents 32.33 DegC.
  */
 static s16 bme680_compensate_temp(struct bme680_data *data,
-				  s32 adc_temp)
+				  u32 adc_temp)
 {
 	struct bme680_calib *calib = &data->bme680;
 	s64 var1, var2, var3;
@@ -345,7 +345,7 @@ static s16 bme680_compensate_temp(struct bme680_data *data,
 	if (!calib->par_t2)
 		bme680_read_calib(data, calib);
 
-	var1 = (adc_temp >> 3) - ((s32)calib->par_t1 << 1);
+	var1 = ((s32)adc_temp >> 3) - ((s32)calib->par_t1 << 1);
 	var2 = (var1 * calib->par_t2) >> 11;
 	var3 = ((var1 >> 1) * (var1 >> 1)) >> 12;
 	var3 = (var3 * ((s32)calib->par_t3 << 4)) >> 14;
@@ -410,9 +410,9 @@ static u32 bme680_compensate_humid(struct bme680_data *data,
 	s32 var1, var2, var3, var4, var5, var6, temp_scaled, calc_hum;
 
 	temp_scaled = (data->t_fine * 5 + 128) >> 8;
-	var1 = (adc_humid - ((s32) ((s32) calib->par_h1 * 16))) -
-		(((temp_scaled * (s32) calib->par_h3) / 100) >> 1);
-	var2 = ((s32) calib->par_h2 *
+	var1 = (adc_humid - (((s32)calib->par_h1 * 16))) -
+		(((temp_scaled * calib->par_h3) / 100) >> 1);
+	var2 = (calib->par_h2 *
 		(((temp_scaled * calib->par_h4) / 100) +
 		 (((temp_scaled * ((temp_scaled * calib->par_h5) / 100))
 		   >> 6) / 100) + (1 << 14))) >> 10;
@@ -637,7 +637,7 @@ static int bme680_read_temp(struct bme680_data *data, int *val)
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 	__be32 tmp = 0;
-	s32 adc_temp;
+	u32 adc_temp;
 	s16 comp_temp;
 
 	/* set forced mode to trigger measurement */
@@ -681,7 +681,7 @@ static int bme680_read_press(struct bme680_data *data,
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 	__be32 tmp = 0;
-	s32 adc_press;
+	u32 adc_press;
 
 	/* Read and compensate temperature to get a reading of t_fine */
 	ret = bme680_read_temp(data, NULL);
@@ -713,7 +713,7 @@ static int bme680_read_humid(struct bme680_data *data,
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 	__be16 tmp = 0;
-	s32 adc_humidity;
+	u16 adc_humidity;
 	u32 comp_humidity;
 
 	/* Read and compensate temperature to get a reading of t_fine */
-- 
2.25.1

[PATCH v1 08/17] iio: chemical: bme680: Remove remaining ACPI-only stuff

[Vasileios Amoiridis]

The ACPI ID table was removed with the following 2 commits:
Commit b73d21dccf68 ("iio: bme680_i2c: Remove acpi_device_id table")
Commit f0e4057e97c1 ("iio: bme680_spi: Remove acpi_device_id table")

Remove the remaining ACPI related stuff to this driver since they are
not directly used.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 7607c3167e24..e61d1f0b67c8 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -8,7 +8,6 @@
  * Datasheet:
  * https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf
  */
-#include <linux/acpi.h>
 #include <linux/bitfield.h>
 #include <linux/cleanup.h>
 #include <linux/delay.h>
@@ -906,17 +905,6 @@ static const struct iio_info bme680_info = {
 	.attrs = &bme680_attribute_group,
 };
 
-static const char *bme680_match_acpi_device(struct device *dev)
-{
-	const struct acpi_device_id *id;
-
-	id = acpi_match_device(dev->driver->acpi_match_table, dev);
-	if (!id)
-		return NULL;
-
-	return dev_name(dev);
-}
-
 int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		      const char *name)
 {
@@ -948,9 +936,6 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	if (!indio_dev)
 		return -ENOMEM;
 
-	if (!name && ACPI_HANDLE(dev))
-		name = bme680_match_acpi_device(dev);
-
 	data = iio_priv(indio_dev);
 	mutex_init(&data->lock);
 	dev_set_drvdata(dev, indio_dev);
-- 
2.25.1

[PATCH v1 09/17] iio: chemical: bme680: Sort headers alphabetically

[Vasileios Amoiridis]

Sort headers in alphabetical order and split the iio specific
functions with a blank line

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index e61d1f0b67c8..96423861c79a 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -12,9 +12,10 @@
 #include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/device.h>
-#include <linux/module.h>
 #include <linux/log2.h>
+#include <linux/module.h>
 #include <linux/regmap.h>
+
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-- 
2.25.1

[PATCH v1 10/17] iio: chemical: bme680: Remove duplicate register read

[Vasileios Amoiridis]

The LSB of the gas register was read first to check if the following
check was correct and then the MSB+LSB were read together. Simplify
this by reading together the MSB+LSB immediately.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 96423861c79a..681f271f9b06 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -748,7 +748,7 @@ static int bme680_read_gas(struct bme680_data *data,
 	int ret;
 	__be16 tmp = 0;
 	unsigned int check;
-	u16 adc_gas_res;
+	u16 adc_gas_res, gas_regs_val;
 	u8 gas_range;
 
 	/* Set heater settings */
@@ -771,11 +771,14 @@ static int bme680_read_gas(struct bme680_data *data,
 		return -EBUSY;
 	}
 
-	ret = regmap_read(data->regmap, BME680_REG_GAS_R_LSB, &check);
+	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
+			       &tmp, sizeof(tmp));
 	if (ret < 0) {
-		dev_err(dev, "failed to read gas_r_lsb register\n");
+		dev_err(dev, "failed to read gas resistance\n");
 		return ret;
 	}
+	gas_regs_val = be16_to_cpu(tmp);
+	adc_gas_res = gas_regs_val >> BME680_ADC_GAS_RES_SHIFT;
 
 	/*
 	 * occurs if either the gas heating duration was insuffient
@@ -783,20 +786,12 @@ static int bme680_read_gas(struct bme680_data *data,
 	 * heater temperature was too high for the heater sink to
 	 * reach.
 	 */
-	if ((check & BME680_GAS_STAB_BIT) == 0) {
+	if ((gas_regs_val & BME680_GAS_STAB_BIT) == 0) {
 		dev_err(dev, "heater failed to reach the target temperature\n");
 		return -EINVAL;
 	}
 
-	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
-			       &tmp, sizeof(tmp));
-	if (ret < 0) {
-		dev_err(dev, "failed to read gas resistance\n");
-		return ret;
-	}
-
-	gas_range = check & BME680_GAS_RANGE_MASK;
-	adc_gas_res = be16_to_cpu(tmp) >> BME680_ADC_GAS_RES_SHIFT;
+	gas_range = gas_regs_val & BME680_GAS_RANGE_MASK;
 
 	*val = bme680_compensate_gas(data, adc_gas_res, gas_range);
 	return IIO_VAL_INT;
-- 
2.25.1

Re: [PATCH v1 10/17] iio: chemical: bme680: Remove duplicate register read

[Jonathan Cameron]

On Mon, 27 May 2024 20:37:58 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> The LSB of the gas register was read first to check if the following
> check was correct and then the MSB+LSB were read together. Simplify
> this by reading together the MSB+LSB immediately.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
A few trivial things. I wrote a nice comment on dma safe buffers then
noticed this does a custom regmap using spi_write_then_read() so despite
not looking like it from this code, DMA safe buffers are used.
Just thought I'd mention it for any other reviewers!

> ---
>  drivers/iio/chemical/bme680_core.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 96423861c79a..681f271f9b06 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -748,7 +748,7 @@ static int bme680_read_gas(struct bme680_data *data,
>  	int ret;
>  	__be16 tmp = 0;
>  	unsigned int check;
> -	u16 adc_gas_res;
> +	u16 adc_gas_res, gas_regs_val;
>  	u8 gas_range;
>  
>  	/* Set heater settings */
> @@ -771,11 +771,14 @@ static int bme680_read_gas(struct bme680_data *data,
>  		return -EBUSY;
>  	}
>  
> -	ret = regmap_read(data->regmap, BME680_REG_GAS_R_LSB, &check);
> +	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
> +			       &tmp, sizeof(tmp)); 
>  	if (ret < 0) {
> -		dev_err(dev, "failed to read gas_r_lsb register\n");
> +		dev_err(dev, "failed to read gas resistance\n");
>  		return ret;
>  	}
> +	gas_regs_val = be16_to_cpu(tmp);
> +	adc_gas_res = gas_regs_val >> BME680_ADC_GAS_RES_SHIFT;
I'd rather see this as a FIELD_GET() but given this was what was originally
here I guess I can cope with keeping it a little longer!

>  
>  	/*
>  	 * occurs if either the gas heating duration was insuffient
> @@ -783,20 +786,12 @@ static int bme680_read_gas(struct bme680_data *data,
>  	 * heater temperature was too high for the heater sink to
>  	 * reach.
>  	 */
> -	if ((check & BME680_GAS_STAB_BIT) == 0) {
> +	if ((gas_regs_val & BME680_GAS_STAB_BIT) == 0) {
>  		dev_err(dev, "heater failed to reach the target temperature\n");
>  		return -EINVAL;
>  	}
>  
> -	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
> -			       &tmp, sizeof(tmp));
> -	if (ret < 0) {
> -		dev_err(dev, "failed to read gas resistance\n");
> -		return ret;
> -	}
> -
> -	gas_range = check & BME680_GAS_RANGE_MASK;
> -	adc_gas_res = be16_to_cpu(tmp) >> BME680_ADC_GAS_RES_SHIFT;
> +	gas_range = gas_regs_val & BME680_GAS_RANGE_MASK;

Whilst you are here, may this a FIELD_GET() so we don't have to go
check that it includes the LSB.

>  
>  	*val = bme680_compensate_gas(data, adc_gas_res, gas_range);
>  	return IIO_VAL_INT;

Re: [PATCH v1 10/17] iio: chemical: bme680: Remove duplicate register read

[Vasileios Amoiridis]

On Sun, Jun 02, 2024 at 01:50:36PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 20:37:58 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The LSB of the gas register was read first to check if the following
> > check was correct and then the MSB+LSB were read together. Simplify
> > this by reading together the MSB+LSB immediately.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> A few trivial things. I wrote a nice comment on dma safe buffers then
> noticed this does a custom regmap using spi_write_then_read() so despite
> not looking like it from this code, DMA safe buffers are used.
> Just thought I'd mention it for any other reviewers!
> 
> > ---
> >  drivers/iio/chemical/bme680_core.c | 21 ++++++++-------------
> >  1 file changed, 8 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > index 96423861c79a..681f271f9b06 100644
> > --- a/drivers/iio/chemical/bme680_core.c
> > +++ b/drivers/iio/chemical/bme680_core.c
> > @@ -748,7 +748,7 @@ static int bme680_read_gas(struct bme680_data *data,
> >  	int ret;
> >  	__be16 tmp = 0;
> >  	unsigned int check;
> > -	u16 adc_gas_res;
> > +	u16 adc_gas_res, gas_regs_val;
> >  	u8 gas_range;
> >  
> >  	/* Set heater settings */
> > @@ -771,11 +771,14 @@ static int bme680_read_gas(struct bme680_data *data,
> >  		return -EBUSY;
> >  	}
> >  
> > -	ret = regmap_read(data->regmap, BME680_REG_GAS_R_LSB, &check);
> > +	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
> > +			       &tmp, sizeof(tmp)); 
> >  	if (ret < 0) {
> > -		dev_err(dev, "failed to read gas_r_lsb register\n");
> > +		dev_err(dev, "failed to read gas resistance\n");
> >  		return ret;
> >  	}
> > +	gas_regs_val = be16_to_cpu(tmp);
> > +	adc_gas_res = gas_regs_val >> BME680_ADC_GAS_RES_SHIFT;
> I'd rather see this as a FIELD_GET() but given this was what was originally
> here I guess I can cope with keeping it a little longer!
> 

Well, it's not a big deal, I could put it in a FIELD_GET().

> >  
> >  	/*
> >  	 * occurs if either the gas heating duration was insuffient
> > @@ -783,20 +786,12 @@ static int bme680_read_gas(struct bme680_data *data,
> >  	 * heater temperature was too high for the heater sink to
> >  	 * reach.
> >  	 */
> > -	if ((check & BME680_GAS_STAB_BIT) == 0) {
> > +	if ((gas_regs_val & BME680_GAS_STAB_BIT) == 0) {
> >  		dev_err(dev, "heater failed to reach the target temperature\n");
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
> > -			       &tmp, sizeof(tmp));
> > -	if (ret < 0) {
> > -		dev_err(dev, "failed to read gas resistance\n");
> > -		return ret;
> > -	}
> > -
> > -	gas_range = check & BME680_GAS_RANGE_MASK;
> > -	adc_gas_res = be16_to_cpu(tmp) >> BME680_ADC_GAS_RES_SHIFT;
> > +	gas_range = gas_regs_val & BME680_GAS_RANGE_MASK;
> 
> Whilst you are here, may this a FIELD_GET() so we don't have to go
> check that it includes the LSB.
> 

I could do it here as well.

Cheers,
Vasilis
> >  
> >  	*val = bme680_compensate_gas(data, adc_gas_res, gas_range);
> >  	return IIO_VAL_INT;
> 

[PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for calibration data

[Vasileios Amoiridis]

Calibration data are located in contiguous-ish registers
inside the chip. For that reason we can use bulk reads as is
done as well in the BME68x Sensor API [1].

The arrays that are used for reading the data out of the sensor
are located inside DMA safe buffer.

[1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680.h      |  28 +--
 drivers/iio/chemical/bme680_core.c | 283 ++++++++++-------------------
 2 files changed, 101 insertions(+), 210 deletions(-)

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index 3133d624270a..5f602170a3af 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -39,10 +39,8 @@
 #define BME680_HUM_REG_SHIFT_VAL		4
 #define BME680_BIT_H1_DATA_MASK			GENMASK(3, 0)
 
-#define BME680_REG_RES_HEAT_RANGE		0x02
 #define   BME680_RHRANGE_MASK			GENMASK(5, 4)
 #define BME680_REG_RES_HEAT_VAL			0x00
-#define BME680_REG_RANGE_SW_ERR			0x04
 #define   BME680_RSERROR_MASK			GENMASK(7, 4)
 #define BME680_REG_RES_HEAT_0			0x5A
 #define BME680_REG_GAS_WAIT_0			0x64
@@ -58,31 +56,13 @@
 
 /* Calibration Parameters */
 #define BME680_T2_LSB_REG	0x8A
-#define BME680_T3_REG		0x8C
-#define BME680_P1_LSB_REG	0x8E
-#define BME680_P2_LSB_REG	0x90
-#define BME680_P3_REG		0x92
-#define BME680_P4_LSB_REG	0x94
-#define BME680_P5_LSB_REG	0x96
-#define BME680_P7_REG		0x98
-#define BME680_P6_REG		0x99
-#define BME680_P8_LSB_REG	0x9C
-#define BME680_P9_LSB_REG	0x9E
-#define BME680_P10_REG		0xA0
-#define BME680_H2_LSB_REG	0xE2
 #define BME680_H2_MSB_REG	0xE1
-#define BME680_H1_MSB_REG	0xE3
-#define BME680_H1_LSB_REG	0xE2
-#define BME680_H3_REG		0xE4
-#define BME680_H4_REG		0xE5
-#define BME680_H5_REG		0xE6
-#define BME680_H6_REG		0xE7
-#define BME680_H7_REG		0xE8
-#define BME680_T1_LSB_REG	0xE9
-#define BME680_GH2_LSB_REG	0xEB
-#define BME680_GH1_REG		0xED
 #define BME680_GH3_REG		0xEE
 
+#define BME680_CALIB_RANGE_1_LEN               23
+#define BME680_CALIB_RANGE_2_LEN               14
+#define BME680_CALIB_RANGE_3_LEN               5
+
 extern const struct regmap_config bme680_regmap_config;
 
 int bme680_core_probe(struct device *dev, struct regmap *regmap,
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 681f271f9b06..ed4cdb4d64af 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -19,8 +19,53 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
+#include <asm/unaligned.h>
+
 #include "bme680.h"
 
+/* 1st set of calibration data */
+enum {
+	/* Temperature calib indexes */
+	T2_LSB = 0,
+	T3 = 2,
+	/* Pressure calib indexes */
+	P1_LSB = 4,
+	P2_LSB = 6,
+	P3 = 8,
+	P4_LSB = 10,
+	P5_LSB = 12,
+	P7 = 14,
+	P6 = 15,
+	P8_LSB = 18,
+	P9_LSB = 20,
+	P10 = 22,
+};
+
+/* 2nd set of calibration data */
+enum {
+	/* Humidity calib indexes */
+	H2_MSB = 0,
+	H1_LSB = 1,
+	H3 = 3,
+	H4 = 4,
+	H5 = 5,
+	H6 = 6,
+	H7 = 7,
+	/* Stray T1 calib index */
+	T1_LSB = 8,
+	/* Gas heater calib indexes */
+	GH2_LSB = 10,
+	GH1 = 12,
+	GH3 = 13,
+};
+
+/* 3rd set of calibration data */
+enum {
+	RES_HEAT_VAL = 0,
+	RES_HEAT_RANGE = 2,
+	RANGE_SW_ERR = 4,
+};
+
 struct bme680_calib {
 	u16 par_t1;
 	s16 par_t2;
@@ -64,6 +109,16 @@ struct bme680_data {
 	 * and humidity compensation calculations.
 	 */
 	s32 t_fine;
+
+	/*
+	 * DMA (thus cache coherency maintenance) may require the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	union {
+		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
+		u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
+		u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
+	} __aligned(IIO_DMA_MINALIGN);
 };
 
 static const struct regmap_range bme680_volatile_ranges[] = {
@@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data,
 			     struct bme680_calib *calib)
 {
 	struct device *dev = regmap_get_device(data->regmap);
-	unsigned int tmp, tmp_msb, tmp_lsb;
+	unsigned int tmp_msb, tmp_lsb;
 	int ret;
-	__le16 buf;
-
-	/* Temperature related coefficients */
-	ret = regmap_bulk_read(data->regmap, BME680_T1_LSB_REG,
-			       &buf, sizeof(buf));
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_T1_LSB_REG\n");
-		return ret;
-	}
-	calib->par_t1 = le16_to_cpu(buf);
 
 	ret = regmap_bulk_read(data->regmap, BME680_T2_LSB_REG,
-			       &buf, sizeof(buf));
+			       &data->bme680_cal_buf_1[0],
+			       sizeof(data->bme680_cal_buf_1));
 	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_T2_LSB_REG\n");
+		dev_err(dev, "failed to read 1st set of calib data;\n");
 		return ret;
 	}
-	calib->par_t2 = le16_to_cpu(buf);
 
-	ret = regmap_read(data->regmap, BME680_T3_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_T3_REG\n");
-		return ret;
-	}
-	calib->par_t3 = tmp;
+	calib->par_t2 = get_unaligned_le16(&data->bme680_cal_buf_1[T2_LSB]);
+	calib->par_t3 = data->bme680_cal_buf_1[T3];
+	calib->par_p1 = get_unaligned_le16(&data->bme680_cal_buf_1[P1_LSB]);
+	calib->par_p2 = get_unaligned_le16(&data->bme680_cal_buf_1[P2_LSB]);
+	calib->par_p3 = data->bme680_cal_buf_1[P3];
+	calib->par_p4 = get_unaligned_le16(&data->bme680_cal_buf_1[P4_LSB]);
+	calib->par_p5 = get_unaligned_le16(&data->bme680_cal_buf_1[P5_LSB]);
+	calib->par_p7 = data->bme680_cal_buf_1[P7];
+	calib->par_p6 = data->bme680_cal_buf_1[P6];
+	calib->par_p8 = get_unaligned_le16(&data->bme680_cal_buf_1[P8_LSB]);
+	calib->par_p9 = get_unaligned_le16(&data->bme680_cal_buf_1[P9_LSB]);
+	calib->par_p10 = data->bme680_cal_buf_1[P10];
 
-	/* Pressure related coefficients */
-	ret = regmap_bulk_read(data->regmap, BME680_P1_LSB_REG,
-			       &buf, sizeof(buf));
+	ret = regmap_bulk_read(data->regmap, BME680_H2_MSB_REG,
+			       &data->bme680_cal_buf_2[0],
+			       sizeof(data->bme680_cal_buf_2));
 	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P1_LSB_REG\n");
+		dev_err(dev, "failed to read 2nd set of calib data;\n");
 		return ret;
 	}
-	calib->par_p1 = le16_to_cpu(buf);
 
-	ret = regmap_bulk_read(data->regmap, BME680_P2_LSB_REG,
-			       &buf, sizeof(buf));
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P2_LSB_REG\n");
-		return ret;
-	}
-	calib->par_p2 = le16_to_cpu(buf);
-
-	ret = regmap_read(data->regmap, BME680_P3_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P3_REG\n");
-		return ret;
-	}
-	calib->par_p3 = tmp;
-
-	ret = regmap_bulk_read(data->regmap, BME680_P4_LSB_REG,
-			       &buf, sizeof(buf));
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P4_LSB_REG\n");
-		return ret;
-	}
-	calib->par_p4 = le16_to_cpu(buf);
-
-	ret = regmap_bulk_read(data->regmap, BME680_P5_LSB_REG,
-			       &buf, sizeof(buf));
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P5_LSB_REG\n");
-		return ret;
-	}
-	calib->par_p5 = le16_to_cpu(buf);
-
-	ret = regmap_read(data->regmap, BME680_P6_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P6_REG\n");
-		return ret;
-	}
-	calib->par_p6 = tmp;
-
-	ret = regmap_read(data->regmap, BME680_P7_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P7_REG\n");
-		return ret;
-	}
-	calib->par_p7 = tmp;
-
-	ret = regmap_bulk_read(data->regmap, BME680_P8_LSB_REG,
-			       &buf, sizeof(buf));
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P8_LSB_REG\n");
-		return ret;
-	}
-	calib->par_p8 = le16_to_cpu(buf);
-
-	ret = regmap_bulk_read(data->regmap, BME680_P9_LSB_REG,
-			       &buf, sizeof(buf));
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P9_LSB_REG\n");
-		return ret;
-	}
-	calib->par_p9 = le16_to_cpu(buf);
-
-	ret = regmap_read(data->regmap, BME680_P10_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_P10_REG\n");
-		return ret;
-	}
-	calib->par_p10 = tmp;
-
-	/* Humidity related coefficients */
-	ret = regmap_read(data->regmap, BME680_H1_MSB_REG, &tmp_msb);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H1_MSB_REG\n");
-		return ret;
-	}
-	ret = regmap_read(data->regmap, BME680_H1_LSB_REG, &tmp_lsb);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H1_LSB_REG\n");
-		return ret;
-	}
+	tmp_lsb = data->bme680_cal_buf_2[H1_LSB];
+	tmp_msb = data->bme680_cal_buf_2[H1_LSB + 1];
 	calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
 			(tmp_lsb & BME680_BIT_H1_DATA_MASK);
 
-	ret = regmap_read(data->regmap, BME680_H2_MSB_REG, &tmp_msb);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H2_MSB_REG\n");
-		return ret;
-	}
-	ret = regmap_read(data->regmap, BME680_H2_LSB_REG, &tmp_lsb);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H2_LSB_REG\n");
-		return ret;
-	}
+	tmp_msb = data->bme680_cal_buf_2[H2_MSB];
+	tmp_lsb = data->bme680_cal_buf_2[H2_MSB + 1];
 	calib->par_h2 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
 			(tmp_lsb >> BME680_HUM_REG_SHIFT_VAL);
 
-	ret = regmap_read(data->regmap, BME680_H3_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H3_REG\n");
-		return ret;
-	}
-	calib->par_h3 = tmp;
-
-	ret = regmap_read(data->regmap, BME680_H4_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H4_REG\n");
-		return ret;
-	}
-	calib->par_h4 = tmp;
-
-	ret = regmap_read(data->regmap, BME680_H5_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H5_REG\n");
-		return ret;
-	}
-	calib->par_h5 = tmp;
-
-	ret = regmap_read(data->regmap, BME680_H6_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H6_REG\n");
-		return ret;
-	}
-	calib->par_h6 = tmp;
+	calib->par_h3 = data->bme680_cal_buf_2[H3];
+	calib->par_h4 = data->bme680_cal_buf_2[H4];
+	calib->par_h5 = data->bme680_cal_buf_2[H5];
+	calib->par_h6 = data->bme680_cal_buf_2[H6];
+	calib->par_h7 = data->bme680_cal_buf_2[H7];
+	calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]);
+	calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]);
+	calib->par_gh1 = data->bme680_cal_buf_2[GH1];
+	calib->par_gh3 = data->bme680_cal_buf_2[GH3];
 
-	ret = regmap_read(data->regmap, BME680_H7_REG, &tmp);
+	ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL,
+			       &data->bme680_cal_buf_3[0],
+			       sizeof(data->bme680_cal_buf_3));
 	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_H7_REG\n");
+		dev_err(dev, "failed to read 3rd set of calib data;\n");
 		return ret;
 	}
-	calib->par_h7 = tmp;
 
-	/* Gas heater related coefficients */
-	ret = regmap_read(data->regmap, BME680_GH1_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_GH1_REG\n");
-		return ret;
-	}
-	calib->par_gh1 = tmp;
+	calib->res_heat_val = data->bme680_cal_buf_3[RES_HEAT_VAL];
 
-	ret = regmap_bulk_read(data->regmap, BME680_GH2_LSB_REG,
-			       &buf, sizeof(buf));
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_GH2_LSB_REG\n");
-		return ret;
-	}
-	calib->par_gh2 = le16_to_cpu(buf);
+	calib->res_heat_range = FIELD_GET(BME680_RHRANGE_MASK,
+					data->bme680_cal_buf_3[RES_HEAT_RANGE]);
 
-	ret = regmap_read(data->regmap, BME680_GH3_REG, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read BME680_GH3_REG\n");
-		return ret;
-	}
-	calib->par_gh3 = tmp;
-
-	/* Other coefficients */
-	ret = regmap_read(data->regmap, BME680_REG_RES_HEAT_RANGE, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read resistance heat range\n");
-		return ret;
-	}
-	calib->res_heat_range = FIELD_GET(BME680_RHRANGE_MASK, tmp);
-
-	ret = regmap_read(data->regmap, BME680_REG_RES_HEAT_VAL, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read resistance heat value\n");
-		return ret;
-	}
-	calib->res_heat_val = tmp;
-
-	ret = regmap_read(data->regmap, BME680_REG_RANGE_SW_ERR, &tmp);
-	if (ret < 0) {
-		dev_err(dev, "failed to read range software error\n");
-		return ret;
-	}
-	calib->range_sw_err = FIELD_GET(BME680_RSERROR_MASK, tmp);
+	calib->range_sw_err = FIELD_GET(BME680_RSERROR_MASK,
+					data->bme680_cal_buf_3[RANGE_SW_ERR]);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for calibration data

[Jonathan Cameron]

On Mon, 27 May 2024 20:37:59 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Calibration data are located in contiguous-ish registers
> inside the chip. For that reason we can use bulk reads as is
> done as well in the BME68x Sensor API [1].
> 
> The arrays that are used for reading the data out of the sensor
> are located inside DMA safe buffer.

See below. I think in this case that isn't necessary.
However it's a quirk of how the custom regmap works. Whilst
we can't rely on regmap core spi implementations continuing to
bounce buffer, we can rely on one local to our particular driver.

> 
> [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>


> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 681f271f9b06..ed4cdb4d64af 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c

> +
>  struct bme680_calib {
>  	u16 par_t1;
>  	s16 par_t2;
> @@ -64,6 +109,16 @@ struct bme680_data {
>  	 * and humidity compensation calculations.
>  	 */
>  	s32 t_fine;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) may require the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	union {
> +		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
> +		u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
> +		u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
> +	} __aligned(IIO_DMA_MINALIGN);
Ah! I should have read ahead.  I don't think you need this alignment forcing
because bme680_regmap_spi_read uses spi_write_then_read() which always
bounces the data.

>  };
>  
>  static const struct regmap_range bme680_volatile_ranges[] = {
> @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data,
>  			     struct bme680_calib *calib)
>  {


> +	calib->par_h3 = data->bme680_cal_buf_2[H3];
> +	calib->par_h4 = data->bme680_cal_buf_2[H4];
> +	calib->par_h5 = data->bme680_cal_buf_2[H5];
> +	calib->par_h6 = data->bme680_cal_buf_2[H6];
> +	calib->par_h7 = data->bme680_cal_buf_2[H7];
> +	calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]);
> +	calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]);
> +	calib->par_gh1 = data->bme680_cal_buf_2[GH1];
> +	calib->par_gh3 = data->bme680_cal_buf_2[GH3];
>  
> -	ret = regmap_read(data->regmap, BME680_H7_REG, &tmp);
> +	ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL,
> +			       &data->bme680_cal_buf_3[0],
This one is always debated, but personally I'd prefer
				data->bme680_cal_buf_3,

for cases like this. Up to you though.
> +			       sizeof(data->bme680_cal_buf_3));
>  	if (ret < 0) {
> -		dev_err(dev, "failed to read BME680_H7_REG\n");
> +		dev_err(dev, "failed to read 3rd set of calib data;\n");
>  		return ret;
>  	}

Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for calibration data

[Vasileios Amoiridis]

On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 20:37:59 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Calibration data are located in contiguous-ish registers
> > inside the chip. For that reason we can use bulk reads as is
> > done as well in the BME68x Sensor API [1].
> > 
> > The arrays that are used for reading the data out of the sensor
> > are located inside DMA safe buffer.
> 
> See below. I think in this case that isn't necessary.
> However it's a quirk of how the custom regmap works. Whilst
> we can't rely on regmap core spi implementations continuing to
> bounce buffer, we can rely on one local to our particular driver.
> 

What about the I2C implementation though? I watched recently a video
from Wolfram Sang [1] and as far as I understood, the buffers are not
provided by the I2C API, but you have to provide them. In any case, I
should maybe check both SPI and I2C reads to understand the internals.

[1]: https://www.youtube.com/watch?v=JDwaMClvV-s

> > 
> > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> 
> > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > index 681f271f9b06..ed4cdb4d64af 100644
> > --- a/drivers/iio/chemical/bme680_core.c
> > +++ b/drivers/iio/chemical/bme680_core.c
> 
> > +
> >  struct bme680_calib {
> >  	u16 par_t1;
> >  	s16 par_t2;
> > @@ -64,6 +109,16 @@ struct bme680_data {
> >  	 * and humidity compensation calculations.
> >  	 */
> >  	s32 t_fine;
> > +
> > +	/*
> > +	 * DMA (thus cache coherency maintenance) may require the
> > +	 * transfer buffers to live in their own cache lines.
> > +	 */
> > +	union {
> > +		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
> > +		u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
> > +		u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
> > +	} __aligned(IIO_DMA_MINALIGN);
> Ah! I should have read ahead.  I don't think you need this alignment forcing
> because bme680_regmap_spi_read uses spi_write_then_read() which always
> bounces the data.
> 

Same comment. What about I2C?

> >  };
> >  
> >  static const struct regmap_range bme680_volatile_ranges[] = {
> > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data,
> >  			     struct bme680_calib *calib)
> >  {
> 
> 
> > +	calib->par_h3 = data->bme680_cal_buf_2[H3];
> > +	calib->par_h4 = data->bme680_cal_buf_2[H4];
> > +	calib->par_h5 = data->bme680_cal_buf_2[H5];
> > +	calib->par_h6 = data->bme680_cal_buf_2[H6];
> > +	calib->par_h7 = data->bme680_cal_buf_2[H7];
> > +	calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]);
> > +	calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]);
> > +	calib->par_gh1 = data->bme680_cal_buf_2[GH1];
> > +	calib->par_gh3 = data->bme680_cal_buf_2[GH3];
> >  
> > -	ret = regmap_read(data->regmap, BME680_H7_REG, &tmp);
> > +	ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL,
> > +			       &data->bme680_cal_buf_3[0],
> This one is always debated, but personally I'd prefer
> 				data->bme680_cal_buf_3,
> 

For me it's the same, I could change it to what you proposed, no problem!

Cheers,
Vasilis

> for cases like this. Up to you though.
> > +			       sizeof(data->bme680_cal_buf_3));
> >  	if (ret < 0) {
> > -		dev_err(dev, "failed to read BME680_H7_REG\n");
> > +		dev_err(dev, "failed to read 3rd set of calib data;\n");
> >  		return ret;
> >  	}
> 

Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for calibration data

[Jonathan Cameron]

On Sun, 2 Jun 2024 21:30:23 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote:
> > On Mon, 27 May 2024 20:37:59 +0200
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> >   
> > > Calibration data are located in contiguous-ish registers
> > > inside the chip. For that reason we can use bulk reads as is
> > > done as well in the BME68x Sensor API [1].
> > > 
> > > The arrays that are used for reading the data out of the sensor
> > > are located inside DMA safe buffer.  
> > 
> > See below. I think in this case that isn't necessary.
> > However it's a quirk of how the custom regmap works. Whilst
> > we can't rely on regmap core spi implementations continuing to
> > bounce buffer, we can rely on one local to our particular driver.
> >   
> 
> What about the I2C implementation though? I watched recently a video
> from Wolfram Sang [1] and as far as I understood, the buffers are not
> provided by the I2C API, but you have to provide them. In any case, I
> should maybe check both SPI and I2C reads to understand the internals.
> 
> [1]: https://www.youtube.com/watch?v=JDwaMClvV-s
> 

I'm not sure Wolfram got far with his desire for generally avoiding the
bounce buffers for i2c.  I think it's strictly opt in only so don't opt in
unless your code is safe for it and regmap never will by default as too
many drivers will be subtly broken.


> > > 
> > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > 
> >   
> > > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > > index 681f271f9b06..ed4cdb4d64af 100644
> > > --- a/drivers/iio/chemical/bme680_core.c
> > > +++ b/drivers/iio/chemical/bme680_core.c  
> >   
> > > +
> > >  struct bme680_calib {
> > >  	u16 par_t1;
> > >  	s16 par_t2;
> > > @@ -64,6 +109,16 @@ struct bme680_data {
> > >  	 * and humidity compensation calculations.
> > >  	 */
> > >  	s32 t_fine;
> > > +
> > > +	/*
> > > +	 * DMA (thus cache coherency maintenance) may require the
> > > +	 * transfer buffers to live in their own cache lines.
> > > +	 */
> > > +	union {
> > > +		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
> > > +		u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
> > > +		u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
> > > +	} __aligned(IIO_DMA_MINALIGN);  
> > Ah! I should have read ahead.  I don't think you need this alignment forcing
> > because bme680_regmap_spi_read uses spi_write_then_read() which always
> > bounces the data.
> >   
> 
> Same comment. What about I2C?
> 
> > >  };
> > >  
> > >  static const struct regmap_range bme680_volatile_ranges[] = {
> > > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data,
> > >  			     struct bme680_calib *calib)
> > >  {  
> > 
> >   
> > > +	calib->par_h3 = data->bme680_cal_buf_2[H3];
> > > +	calib->par_h4 = data->bme680_cal_buf_2[H4];
> > > +	calib->par_h5 = data->bme680_cal_buf_2[H5];
> > > +	calib->par_h6 = data->bme680_cal_buf_2[H6];
> > > +	calib->par_h7 = data->bme680_cal_buf_2[H7];
> > > +	calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]);
> > > +	calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]);
> > > +	calib->par_gh1 = data->bme680_cal_buf_2[GH1];
> > > +	calib->par_gh3 = data->bme680_cal_buf_2[GH3];
> > >  
> > > -	ret = regmap_read(data->regmap, BME680_H7_REG, &tmp);
> > > +	ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL,
> > > +			       &data->bme680_cal_buf_3[0],  
> > This one is always debated, but personally I'd prefer
> > 				data->bme680_cal_buf_3,
> >   
> 
> For me it's the same, I could change it to what you proposed, no problem!
> 
> Cheers,
> Vasilis
> 
> > for cases like this. Up to you though.  
> > > +			       sizeof(data->bme680_cal_buf_3));
> > >  	if (ret < 0) {
> > > -		dev_err(dev, "failed to read BME680_H7_REG\n");
> > > +		dev_err(dev, "failed to read 3rd set of calib data;\n");
> > >  		return ret;
> > >  	}  
> >   

Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for calibration data

[Vasileios Amoiridis]

On Mon, Jun 03, 2024 at 08:25:37PM +0100, Jonathan Cameron wrote:
> On Sun, 2 Jun 2024 21:30:23 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote:
> > > On Mon, 27 May 2024 20:37:59 +0200
> > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > >   
> > > > Calibration data are located in contiguous-ish registers
> > > > inside the chip. For that reason we can use bulk reads as is
> > > > done as well in the BME68x Sensor API [1].
> > > > 
> > > > The arrays that are used for reading the data out of the sensor
> > > > are located inside DMA safe buffer.  
> > > 
> > > See below. I think in this case that isn't necessary.
> > > However it's a quirk of how the custom regmap works. Whilst
> > > we can't rely on regmap core spi implementations continuing to
> > > bounce buffer, we can rely on one local to our particular driver.
> > >   
> > 
> > What about the I2C implementation though? I watched recently a video
> > from Wolfram Sang [1] and as far as I understood, the buffers are not
> > provided by the I2C API, but you have to provide them. In any case, I
> > should maybe check both SPI and I2C reads to understand the internals.
> > 
> > [1]: https://www.youtube.com/watch?v=JDwaMClvV-s
> > 
> 
> I'm not sure Wolfram got far with his desire for generally avoiding the
> bounce buffers for i2c.  I think it's strictly opt in only so don't opt in
> unless your code is safe for it and regmap never will by default as too
> many drivers will be subtly broken.
> 

The things that I found about DMA "safety" in I2C are [1] and [2] so I think
that the IIO_DMA_MINALIGN should remain because in the future, in case it's
needed for triggered buffers to do buffer reads from the volatile registers
of the device, then it might be a problem for I2C. 

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L2627
[2]: https://elixir.bootlin.com/linux/latest/source/include/linux/i2c.h#L92

> 
> > > > 
> > > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769
> > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > > 
> > >   
> > > > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > > > index 681f271f9b06..ed4cdb4d64af 100644
> > > > --- a/drivers/iio/chemical/bme680_core.c
> > > > +++ b/drivers/iio/chemical/bme680_core.c  
> > >   
> > > > +
> > > >  struct bme680_calib {
> > > >  	u16 par_t1;
> > > >  	s16 par_t2;
> > > > @@ -64,6 +109,16 @@ struct bme680_data {
> > > >  	 * and humidity compensation calculations.
> > > >  	 */
> > > >  	s32 t_fine;
> > > > +
> > > > +	/*
> > > > +	 * DMA (thus cache coherency maintenance) may require the
> > > > +	 * transfer buffers to live in their own cache lines.
> > > > +	 */
> > > > +	union {
> > > > +		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
> > > > +		u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
> > > > +		u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
> > > > +	} __aligned(IIO_DMA_MINALIGN);  
> > > Ah! I should have read ahead.  I don't think you need this alignment forcing
> > > because bme680_regmap_spi_read uses spi_write_then_read() which always
> > > bounces the data.
> > >   
> > 
> > Same comment. What about I2C?
> > 
> > > >  };
> > > >  
> > > >  static const struct regmap_range bme680_volatile_ranges[] = {
> > > > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data,
> > > >  			     struct bme680_calib *calib)
> > > >  {  
> > > 
> > >   
> > > > +	calib->par_h3 = data->bme680_cal_buf_2[H3];
> > > > +	calib->par_h4 = data->bme680_cal_buf_2[H4];
> > > > +	calib->par_h5 = data->bme680_cal_buf_2[H5];
> > > > +	calib->par_h6 = data->bme680_cal_buf_2[H6];
> > > > +	calib->par_h7 = data->bme680_cal_buf_2[H7];
> > > > +	calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]);
> > > > +	calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]);
> > > > +	calib->par_gh1 = data->bme680_cal_buf_2[GH1];
> > > > +	calib->par_gh3 = data->bme680_cal_buf_2[GH3];
> > > >  
> > > > -	ret = regmap_read(data->regmap, BME680_H7_REG, &tmp);
> > > > +	ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL,
> > > > +			       &data->bme680_cal_buf_3[0],  
> > > This one is always debated, but personally I'd prefer
> > > 				data->bme680_cal_buf_3,
> > >   
> > 
> > For me it's the same, I could change it to what you proposed, no problem!
> > 
> > Cheers,
> > Vasilis
> > 
> > > for cases like this. Up to you though.  
> > > > +			       sizeof(data->bme680_cal_buf_3));
> > > >  	if (ret < 0) {
> > > > -		dev_err(dev, "failed to read BME680_H7_REG\n");
> > > > +		dev_err(dev, "failed to read 3rd set of calib data;\n");
> > > >  		return ret;
> > > >  	}  
> > >   
> 

Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for calibration data

[Jonathan Cameron]

On Mon, 3 Jun 2024 22:30:03 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Mon, Jun 03, 2024 at 08:25:37PM +0100, Jonathan Cameron wrote:
> > On Sun, 2 Jun 2024 21:30:23 +0200
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> >   
> > > On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote:  
> > > > On Mon, 27 May 2024 20:37:59 +0200
> > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > > >     
> > > > > Calibration data are located in contiguous-ish registers
> > > > > inside the chip. For that reason we can use bulk reads as is
> > > > > done as well in the BME68x Sensor API [1].
> > > > > 
> > > > > The arrays that are used for reading the data out of the sensor
> > > > > are located inside DMA safe buffer.    
> > > > 
> > > > See below. I think in this case that isn't necessary.
> > > > However it's a quirk of how the custom regmap works. Whilst
> > > > we can't rely on regmap core spi implementations continuing to
> > > > bounce buffer, we can rely on one local to our particular driver.
> > > >     
> > > 
> > > What about the I2C implementation though? I watched recently a video
> > > from Wolfram Sang [1] and as far as I understood, the buffers are not
> > > provided by the I2C API, but you have to provide them. In any case, I
> > > should maybe check both SPI and I2C reads to understand the internals.
> > > 
> > > [1]: https://www.youtube.com/watch?v=JDwaMClvV-s
> > >   
> > 
> > I'm not sure Wolfram got far with his desire for generally avoiding the
> > bounce buffers for i2c.  I think it's strictly opt in only so don't opt in
> > unless your code is safe for it and regmap never will by default as too
> > many drivers will be subtly broken.
> >   
> 
> The things that I found about DMA "safety" in I2C are [1] and [2] so I think
> that the IIO_DMA_MINALIGN should remain because in the future, in case it's
> needed for triggered buffers to do buffer reads from the volatile registers
> of the device, then it might be a problem for I2C. 
> 
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L2627
> [2]: https://elixir.bootlin.com/linux/latest/source/include/linux/i2c.h#L92
> 

Those will remain opt in for a very long time if not for ever.

I suspect they will actually go away as a result of Catalin's 
https://lore.kernel.org/linux-arm-kernel/20230612153201.554742-15-catalin.marinas@arm.com/
which bounces small or unaligned buffers and will apply to an annoying number of
IIO buffers even though they are actually safe because it's a heuristic
on the size part.

Today only a few architectures that do non coherent DMA use it though.
Will take the others opting in to the point where the config variable
goes away before we can stop this dance.

I don't know if anyone has yet looked at the impact on performance of that
change on the many drivers that have to work whether that is in place
or not and do small dma mappings.  Maybe we need to teach bus drivers
when they need to map padding around an actually safe buffer that
doesn't look like it.

Jonathan


> >   
> > > > > 
> > > > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769
> > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>    
> > > > 
> > > >     
> > > > > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > > > > index 681f271f9b06..ed4cdb4d64af 100644
> > > > > --- a/drivers/iio/chemical/bme680_core.c
> > > > > +++ b/drivers/iio/chemical/bme680_core.c    
> > > >     
> > > > > +
> > > > >  struct bme680_calib {
> > > > >  	u16 par_t1;
> > > > >  	s16 par_t2;
> > > > > @@ -64,6 +109,16 @@ struct bme680_data {
> > > > >  	 * and humidity compensation calculations.
> > > > >  	 */
> > > > >  	s32 t_fine;
> > > > > +
> > > > > +	/*
> > > > > +	 * DMA (thus cache coherency maintenance) may require the
> > > > > +	 * transfer buffers to live in their own cache lines.
> > > > > +	 */
> > > > > +	union {
> > > > > +		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
> > > > > +		u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
> > > > > +		u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
> > > > > +	} __aligned(IIO_DMA_MINALIGN);    
> > > > Ah! I should have read ahead.  I don't think you need this alignment forcing
> > > > because bme680_regmap_spi_read uses spi_write_then_read() which always
> > > > bounces the data.
> > > >     
> > > 
> > > Same comment. What about I2C?
> > >   
> > > > >  };
> > > > >  
> > > > >  static const struct regmap_range bme680_volatile_ranges[] = {
> > > > > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data,
> > > > >  			     struct bme680_calib *calib)
> > > > >  {    
> > > > 
> > > >     
> > > > > +	calib->par_h3 = data->bme680_cal_buf_2[H3];
> > > > > +	calib->par_h4 = data->bme680_cal_buf_2[H4];
> > > > > +	calib->par_h5 = data->bme680_cal_buf_2[H5];
> > > > > +	calib->par_h6 = data->bme680_cal_buf_2[H6];
> > > > > +	calib->par_h7 = data->bme680_cal_buf_2[H7];
> > > > > +	calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]);
> > > > > +	calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]);
> > > > > +	calib->par_gh1 = data->bme680_cal_buf_2[GH1];
> > > > > +	calib->par_gh3 = data->bme680_cal_buf_2[GH3];
> > > > >  
> > > > > -	ret = regmap_read(data->regmap, BME680_H7_REG, &tmp);
> > > > > +	ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL,
> > > > > +			       &data->bme680_cal_buf_3[0],    
> > > > This one is always debated, but personally I'd prefer
> > > > 				data->bme680_cal_buf_3,
> > > >     
> > > 
> > > For me it's the same, I could change it to what you proposed, no problem!
> > > 
> > > Cheers,
> > > Vasilis
> > >   
> > > > for cases like this. Up to you though.    
> > > > > +			       sizeof(data->bme680_cal_buf_3));
> > > > >  	if (ret < 0) {
> > > > > -		dev_err(dev, "failed to read BME680_H7_REG\n");
> > > > > +		dev_err(dev, "failed to read 3rd set of calib data;\n");
> > > > >  		return ret;
> > > > >  	}    
> > > >     
> >   

[PATCH v1 12/17] iio: chemical: bme680: Allocate IIO device before chip initialization

[Vasileios Amoiridis]

Move the IIO device allocation before the actual initialization
of the chip to be more consistent with most IIO drivers and also
to have the ability to use any driver specific data for the chip
initialization.

While at it, fix also a misaligned line.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 38 +++++++++++++++---------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index ed4cdb4d64af..a6f425076d36 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -820,25 +820,6 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	unsigned int val;
 	int ret;
 
-	ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
-			   BME680_CMD_SOFTRESET);
-	if (ret < 0) {
-		dev_err(dev, "Failed to reset chip\n");
-		return ret;
-	}
-
-	ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
-	if (ret < 0) {
-		dev_err(dev, "Error reading chip ID\n");
-		return ret;
-	}
-
-	if (val != BME680_CHIP_ID_VAL) {
-		dev_err(dev, "Wrong chip ID, got %x expected %x\n",
-				val, BME680_CHIP_ID_VAL);
-		return -ENODEV;
-	}
-
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
@@ -860,6 +841,25 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	data->heater_temp = 320; /* degree Celsius */
 	data->heater_dur = 150;  /* milliseconds */
 
+	ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
+			   BME680_CMD_SOFTRESET);
+	if (ret < 0) {
+		dev_err(dev, "Failed to reset chip\n");
+		return ret;
+	}
+
+	ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
+	if (ret < 0) {
+		dev_err(dev, "Error reading chip ID\n");
+		return ret;
+	}
+
+	if (val != BME680_CHIP_ID_VAL) {
+		dev_err(dev, "Wrong chip ID, got %x expected %x\n",
+			val, BME680_CHIP_ID_VAL);
+		return -ENODEV;
+	}
+
 	ret = bme680_chip_config(data);
 	if (ret < 0) {
 		dev_err(dev, "failed to set chip_config data\n");
-- 
2.25.1

[PATCH v1 13/17] iio: chemical: bme680: Add read buffers in DMA safe region

[Vasileios Amoiridis]

Move the buffers that are used in order to read data from the
device in a DMA-safe region. Also create defines for the number
of bytes that are being read from the device and don't use
magic numbers.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680.h      |  7 ++++++
 drivers/iio/chemical/bme680_core.c | 37 +++++++++++++++---------------
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index 5f602170a3af..17ea59253923 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -54,6 +54,13 @@
 #define BME680_REG_MEAS_STAT_0			0x1D
 #define   BME680_GAS_MEAS_BIT			BIT(6)
 
+#define BME680_TEMP_NUM_BYTES			3
+#define BME680_PRESS_NUM_BYTES			3
+#define BME680_HUMID_NUM_BYTES			2
+#define BME680_GAS_NUM_BYTES			2
+
+#define BME680_MEAS_TRIM_MASK			GENMASK(24, 4)
+
 /* 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 a6f425076d36..b055eeee8f1c 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -115,6 +115,9 @@ struct bme680_data {
 	 * transfer buffers to live in their own cache lines.
 	 */
 	union {
+		u8 buf[3];
+		unsigned int check;
+		__be16 be16;
 		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
 		u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
 		u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
@@ -547,7 +550,6 @@ static int bme680_read_temp(struct bme680_data *data, int *val)
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
-	__be32 tmp = 0;
 	u32 adc_temp;
 	s16 comp_temp;
 
@@ -559,13 +561,14 @@ static int bme680_read_temp(struct bme680_data *data, int *val)
 	bme680_wait_for_eoc(data);
 
 	ret = regmap_bulk_read(data->regmap, BME680_REG_TEMP_MSB,
-			       &tmp, 3);
+			       data->buf, BME680_TEMP_NUM_BYTES);
 	if (ret < 0) {
 		dev_err(dev, "failed to read temperature\n");
 		return ret;
 	}
 
-	adc_temp = be32_to_cpu(tmp) >> 12;
+	adc_temp = FIELD_GET(BME680_MEAS_TRIM_MASK,
+			     get_unaligned_be24(data->buf));
 	if (adc_temp == BME680_MEAS_SKIPPED) {
 		/* reading was skipped */
 		dev_err(dev, "reading temperature skipped\n");
@@ -591,7 +594,6 @@ static int bme680_read_press(struct bme680_data *data,
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
-	__be32 tmp = 0;
 	u32 adc_press;
 
 	/* Read and compensate temperature to get a reading of t_fine */
@@ -600,13 +602,14 @@ static int bme680_read_press(struct bme680_data *data,
 		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BME680_REG_PRESS_MSB,
-			       &tmp, 3);
+			       data->buf, BME680_PRESS_NUM_BYTES);
 	if (ret < 0) {
 		dev_err(dev, "failed to read pressure\n");
 		return ret;
 	}
 
-	adc_press = be32_to_cpu(tmp) >> 12;
+	adc_press = FIELD_GET(BME680_MEAS_TRIM_MASK,
+			      get_unaligned_be24(data->buf));
 	if (adc_press == BME680_MEAS_SKIPPED) {
 		/* reading was skipped */
 		dev_err(dev, "reading pressure skipped\n");
@@ -623,7 +626,6 @@ static int bme680_read_humid(struct bme680_data *data,
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
-	__be16 tmp = 0;
 	u16 adc_humidity;
 	u32 comp_humidity;
 
@@ -633,13 +635,13 @@ static int bme680_read_humid(struct bme680_data *data,
 		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BME680_REG_HUMIDITY_MSB,
-			       &tmp, sizeof(tmp));
+			       &data->be16, BME680_HUMID_NUM_BYTES);
 	if (ret < 0) {
 		dev_err(dev, "failed to read humidity\n");
 		return ret;
 	}
 
-	adc_humidity = be16_to_cpu(tmp);
+	adc_humidity = be16_to_cpu(data->be16);
 	if (adc_humidity == BME680_MEAS_SKIPPED) {
 		/* reading was skipped */
 		dev_err(dev, "reading humidity skipped\n");
@@ -657,8 +659,6 @@ static int bme680_read_gas(struct bme680_data *data,
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
-	__be16 tmp = 0;
-	unsigned int check;
 	u16 adc_gas_res, gas_regs_val;
 	u8 gas_range;
 
@@ -676,19 +676,19 @@ static int bme680_read_gas(struct bme680_data *data,
 
 	bme680_wait_for_eoc(data);
 
-	ret = regmap_read(data->regmap, BME680_REG_MEAS_STAT_0, &check);
-	if (check & BME680_GAS_MEAS_BIT) {
+	ret = regmap_read(data->regmap, BME680_REG_MEAS_STAT_0, &data->check);
+	if (data->check & BME680_GAS_MEAS_BIT) {
 		dev_err(dev, "gas measurement incomplete\n");
 		return -EBUSY;
 	}
 
 	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
-			       &tmp, sizeof(tmp));
+			       &data->be16, BME680_GAS_NUM_BYTES);
 	if (ret < 0) {
 		dev_err(dev, "failed to read gas resistance\n");
 		return ret;
 	}
-	gas_regs_val = be16_to_cpu(tmp);
+	gas_regs_val = be16_to_cpu(data->be16);
 	adc_gas_res = gas_regs_val >> BME680_ADC_GAS_RES_SHIFT;
 
 	/*
@@ -817,7 +817,6 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 {
 	struct iio_dev *indio_dev;
 	struct bme680_data *data;
-	unsigned int val;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -848,15 +847,15 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		return ret;
 	}
 
-	ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
+	ret = regmap_read(regmap, BME680_REG_CHIP_ID, &data->check);
 	if (ret < 0) {
 		dev_err(dev, "Error reading chip ID\n");
 		return ret;
 	}
 
-	if (val != BME680_CHIP_ID_VAL) {
+	if (data->check != BME680_CHIP_ID_VAL) {
 		dev_err(dev, "Wrong chip ID, got %x expected %x\n",
-			val, BME680_CHIP_ID_VAL);
+			data->check, BME680_CHIP_ID_VAL);
 		return -ENODEV;
 	}
 
-- 
2.25.1

Re: [PATCH v1 13/17] iio: chemical: bme680: Add read buffers in DMA safe region

[Jonathan Cameron]

On Mon, 27 May 2024 20:38:01 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Move the buffers that are used in order to read data from the
> device in a DMA-safe region. Also create defines for the number
> of bytes that are being read from the device and don't use
> magic numbers.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>

Same response as previous.  I don't think it's necessary because
of the custom regmap implementation.

My first instinct was the same as yours though!

Jonathan

Re: [PATCH v1 13/17] iio: chemical: bme680: Add read buffers in DMA safe region

[Vasileios Amoiridis]

On Sun, Jun 02, 2024 at 01:59:08PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 20:38:01 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Move the buffers that are used in order to read data from the
> > device in a DMA-safe region. Also create defines for the number
> > of bytes that are being read from the device and don't use
> > magic numbers.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Same response as previous.  I don't think it's necessary because
> of the custom regmap implementation.
> 
> My first instinct was the same as yours though!
> 
> Jonathan
> 
> 
Well, even if we end up not needing it, I would keep the values inside
the union just becasue it saves some space, and it keeps all the read
buffers in the same place. What do you think?

Cheers,
Vasilis

Re: [PATCH v1 13/17] iio: chemical: bme680: Add read buffers in DMA safe region

[Jonathan Cameron]

On Sun, 2 Jun 2024 21:33:10 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Sun, Jun 02, 2024 at 01:59:08PM +0100, Jonathan Cameron wrote:
> > On Mon, 27 May 2024 20:38:01 +0200
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> >   
> > > Move the buffers that are used in order to read data from the
> > > device in a DMA-safe region. Also create defines for the number
> > > of bytes that are being read from the device and don't use
> > > magic numbers.
> > > 
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > 
> > Same response as previous.  I don't think it's necessary because
> > of the custom regmap implementation.
> > 
> > My first instinct was the same as yours though!
> > 
> > Jonathan
> > 
> >   
> Well, even if we end up not needing it, I would keep the values inside
> the union just becasue it saves some space, and it keeps all the read
> buffers in the same place. What do you think?
> 
Sure. The union is fine as long as you have made sure there can't be
concurrent access to the different members. It will save space on x86_64
at least as that has very low IIO_DMA_MINALIGN.  Less likely to make
a difference on arm64.

[PATCH v1 14/17] iio: chemical: bme680: Modify startup procedure

[Vasileios Amoiridis]

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

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

[Jonathan Cameron]

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

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

[Vasileios Amoiridis]

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

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

[PATCH v1 15/17] iio: chemical: bme680: Remove redundant gas configuration

[Vasileios Amoiridis]

There is no need to explicitly configure the gas measurement
registers every time a gas measurement takes place. These are
initial configurations which are written in the beginning and
they are not changed throughout the lifetime of the driver.

If in the future, the device starts to support multiple
configuration profiles with variable heater duration and
heater temperature, then they could become members of
the ->read_avail().

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index afaa43ec3241..a91b15626ec8 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -669,13 +669,6 @@ static int bme680_read_gas(struct bme680_data *data,
 	u16 adc_gas_res, gas_regs_val;
 	u8 gas_range;
 
-	/* Set heater settings */
-	ret = bme680_gas_config(data);
-	if (ret < 0) {
-		dev_err(dev, "failed to set gas config\n");
-		return ret;
-	}
-
 	/* set forced mode to trigger measurement */
 	ret = bme680_set_mode(data, true);
 	if (ret < 0)
-- 
2.25.1

[PATCH v1 16/17] iio: chemical: bme680: Move forced mode setup in ->read_raw()

[Vasileios Amoiridis]

Whenever the sensor is set to forced mode, a TPHG cycle is
triggered and the values of temperature, pressure, humidity
and gas become ready to be read.

The setup of the forced mode to trigger measurements was located
inside the read_{temp/gas}() functions. This was not posing a
functional problem since read_{humid/press}() are internally
calling read_temp() so the forced mode is set through this call.

This is not very clear and it is kind of hidden that regardless
of the measurement, the setup of the forced mode needs to happen
before any measurement.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index a91b15626ec8..8f977667249b 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -560,13 +560,6 @@ static int bme680_read_temp(struct bme680_data *data, int *val)
 	u32 adc_temp;
 	s16 comp_temp;
 
-	/* set forced mode to trigger measurement */
-	ret = bme680_set_mode(data, true);
-	if (ret < 0)
-		return ret;
-
-	bme680_wait_for_eoc(data);
-
 	ret = regmap_bulk_read(data->regmap, BME680_REG_TEMP_MSB,
 			       data->buf, BME680_TEMP_NUM_BYTES);
 	if (ret < 0) {
@@ -669,13 +662,6 @@ static int bme680_read_gas(struct bme680_data *data,
 	u16 adc_gas_res, gas_regs_val;
 	u8 gas_range;
 
-	/* set forced mode to trigger measurement */
-	ret = bme680_set_mode(data, true);
-	if (ret < 0)
-		return ret;
-
-	bme680_wait_for_eoc(data);
-
 	ret = regmap_read(data->regmap, BME680_REG_MEAS_STAT_0, &data->check);
 	if (data->check & BME680_GAS_MEAS_BIT) {
 		dev_err(dev, "gas measurement incomplete\n");
@@ -713,9 +699,17 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct bme680_data *data = iio_priv(indio_dev);
+	int ret;
 
 	guard(mutex)(&data->lock);
 
+	/* set forced mode to trigger measurement */
+	ret = bme680_set_mode(data, true);
+	if (ret < 0)
+		return ret;
+
+	bme680_wait_for_eoc(data);
+
 	switch (mask) {
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
-- 
2.25.1

[PATCH v1 17/17] iio: chemical: bme680: Refactorize reading functions

[Vasileios Amoiridis]

The reading of the pressure and humidity value, requires an update
of the t_fine variable which happens by reading the temperature
value.

So the bme680_read_{press/humid}() functions of the above sensors
are internally calling the equivalent bme680_read_temp() function
in order to update the t_fine value. By just looking at the code
this relation is a bit hidden and is not easy to understand why
those channels are not independent.

This commit tries to clear these thing a bit by splitting the
bme680_{read/compensate}_{temp/press/humid}() to the following:

i. bme680_read_{temp/press/humid}_adc(): read the raw value from
the sensor.

ii. bme680_calc_t_fine(): calculate the t_fine variable.

iii. bme680_get_t_fine(): get the t_fine variable.

iv. bme680_compensate_{temp/press/humid}(): compensate the adc
values and return the calculated value.

v. bme680_read_{temp/press/humid}(): combine calls of the
aforementioned functions to return the requested value.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 192 +++++++++++++++++------------
 1 file changed, 116 insertions(+), 76 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 8f977667249b..cfcb6f791517 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -104,11 +104,6 @@ struct bme680_data {
 	u8 oversampling_humid;
 	u16 heater_dur;
 	u16 heater_temp;
-	/*
-	 * Carryover value from temperature conversion, used in pressure
-	 * and humidity compensation calculations.
-	 */
-	s32 t_fine;
 
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
@@ -241,6 +236,31 @@ static int bme680_read_calib(struct bme680_data *data,
 	return 0;
 }
 
+static int bme680_read_temp_adc(struct bme680_data *data, u32 *adc_temp)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	u32 value_temp;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BME680_REG_TEMP_MSB,
+			       data->buf, BME680_TEMP_NUM_BYTES);
+	if (ret < 0) {
+		dev_err(dev, "failed to read temperature\n");
+		return ret;
+	}
+
+	value_temp = FIELD_GET(BME680_MEAS_TRIM_MASK,
+			       get_unaligned_be24(data->buf));
+	if (value_temp == BME680_MEAS_SKIPPED) {
+		/* reading was skipped */
+		dev_err(dev, "reading temperature skipped\n");
+		return -EINVAL;
+	}
+	*adc_temp = value_temp;
+
+	return 0;
+}
+
 /*
  * Taken from Bosch BME680 API:
  * https://github.com/BoschSensortec/BME680_driver/blob/63bb5336/bme680.c#L876
@@ -248,12 +268,10 @@ static int bme680_read_calib(struct bme680_data *data,
  * Returns temperature measurement in DegC, resolutions is 0.01 DegC. Therefore,
  * output value of "3233" represents 32.33 DegC.
  */
-static s16 bme680_compensate_temp(struct bme680_data *data,
-				  u32 adc_temp)
+static s32 bme680_calc_t_fine(struct bme680_data *data, u32 adc_temp)
 {
 	struct bme680_calib *calib = &data->bme680;
 	s64 var1, var2, var3;
-	s16 calc_temp;
 
 	/* If the calibration is invalid, attempt to reload it */
 	if (!calib->par_t2)
@@ -263,10 +281,52 @@ static s16 bme680_compensate_temp(struct bme680_data *data,
 	var2 = (var1 * calib->par_t2) >> 11;
 	var3 = ((var1 >> 1) * (var1 >> 1)) >> 12;
 	var3 = (var3 * ((s32)calib->par_t3 << 4)) >> 14;
-	data->t_fine = var2 + var3;
-	calc_temp = (data->t_fine * 5 + 128) >> 8;
+	return var2 + var3; /* t_fine = var2 + var3 */
+}
 
-	return calc_temp;
+static int bme680_get_t_fine(struct bme680_data *data, s32 *t_fine)
+{
+	u32 adc_temp;
+	int ret;
+
+	ret = bme680_read_temp_adc(data, &adc_temp);
+	if (ret)
+		return ret;
+
+	*t_fine = bme680_calc_t_fine(data, adc_temp);
+
+	return 0;
+}
+
+static s16 bme680_compensate_temp(struct bme680_data *data,
+				  u32 adc_temp)
+{
+	return (bme680_calc_t_fine(data, adc_temp) * 5 + 128) / 256;
+}
+
+static int bme680_read_press_adc(struct bme680_data *data, u32 *adc_press)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	u32 value_press;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BME680_REG_PRESS_MSB,
+			       data->buf, BME680_PRESS_NUM_BYTES);
+	if (ret < 0) {
+		dev_err(dev, "failed to read pressure\n");
+		return ret;
+	}
+
+	value_press = FIELD_GET(BME680_MEAS_TRIM_MASK,
+				get_unaligned_be24(data->buf));
+	if (value_press == BME680_MEAS_SKIPPED) {
+		/* reading was skipped */
+		dev_err(dev, "reading pressure skipped\n");
+		return -EINVAL;
+	}
+	*adc_press = value_press;
+
+	return 0;
 }
 
 /*
@@ -277,12 +337,12 @@ static s16 bme680_compensate_temp(struct bme680_data *data,
  * 97356 Pa = 973.56 hPa.
  */
 static u32 bme680_compensate_press(struct bme680_data *data,
-				   u32 adc_press)
+				   u32 adc_press, s32 t_fine)
 {
 	struct bme680_calib *calib = &data->bme680;
 	s32 var1, var2, var3, press_comp;
 
-	var1 = (data->t_fine >> 1) - 64000;
+	var1 = (t_fine >> 1) - 64000;
 	var2 = ((((var1 >> 2) * (var1 >> 2)) >> 11) * calib->par_p6) >> 2;
 	var2 = var2 + (var1 * calib->par_p5 << 1);
 	var2 = (var2 >> 2) + ((s32)calib->par_p4 << 16);
@@ -310,6 +370,30 @@ static u32 bme680_compensate_press(struct bme680_data *data,
 	return press_comp;
 }
 
+static int bme680_read_humid_adc(struct bme680_data *data, u32 *adc_humidity)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	u32 value_humidity;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BME680_REG_HUMIDITY_MSB,
+			       &data->be16, BME680_HUMID_NUM_BYTES);
+	if (ret < 0) {
+		dev_err(dev, "failed to read humidity\n");
+		return ret;
+	}
+
+	value_humidity = be16_to_cpu(data->be16);
+	if (value_humidity == BME680_MEAS_SKIPPED) {
+		/* reading was skipped */
+		dev_err(dev, "reading humidity skipped\n");
+		return -EINVAL;
+	}
+	*adc_humidity = value_humidity;
+
+	return 0;
+}
+
 /*
  * Taken from Bosch BME680 API:
  * https://github.com/BoschSensortec/BME680_driver/blob/63bb5336/bme680.c#L937
@@ -318,12 +402,12 @@ static u32 bme680_compensate_press(struct bme680_data *data,
  * value of "43215" represents 43.215 %rH.
  */
 static u32 bme680_compensate_humid(struct bme680_data *data,
-				   u16 adc_humid)
+				   u16 adc_humid, s32 t_fine)
 {
 	struct bme680_calib *calib = &data->bme680;
 	s32 var1, var2, var3, var4, var5, var6, temp_scaled, calc_hum;
 
-	temp_scaled = (data->t_fine * 5 + 128) >> 8;
+	temp_scaled = (t_fine * 5 + 128) >> 8;
 	var1 = (adc_humid - (((s32)calib->par_h1 * 16))) -
 		(((temp_scaled * calib->par_h3) / 100) >> 1);
 	var2 = (calib->par_h2 *
@@ -555,68 +639,35 @@ static int bme680_gas_config(struct bme680_data *data)
 
 static int bme680_read_temp(struct bme680_data *data, int *val)
 {
-	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 	u32 adc_temp;
 	s16 comp_temp;
 
-	ret = regmap_bulk_read(data->regmap, BME680_REG_TEMP_MSB,
-			       data->buf, BME680_TEMP_NUM_BYTES);
-	if (ret < 0) {
-		dev_err(dev, "failed to read temperature\n");
+	ret = bme680_read_temp_adc(data, &adc_temp);
+	if (ret)
 		return ret;
-	}
 
-	adc_temp = FIELD_GET(BME680_MEAS_TRIM_MASK,
-			     get_unaligned_be24(data->buf));
-	if (adc_temp == BME680_MEAS_SKIPPED) {
-		/* reading was skipped */
-		dev_err(dev, "reading temperature skipped\n");
-		return -EINVAL;
-	}
 	comp_temp = bme680_compensate_temp(data, adc_temp);
-	/*
-	 * val might be NULL if we're called by the read_press/read_humid
-	 * routine which is called to get t_fine value used in
-	 * compensate_press/compensate_humid to get compensated
-	 * pressure/humidity readings.
-	 */
-	if (val) {
-		*val = comp_temp * 10; /* Centidegrees to millidegrees */
-		return IIO_VAL_INT;
-	}
-
-	return ret;
+	*val = comp_temp * 10; /* Centidegrees to millidegrees */
+	return IIO_VAL_INT;
 }
 
 static int bme680_read_press(struct bme680_data *data,
 			     int *val, int *val2)
 {
-	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 	u32 adc_press;
+	s32 t_fine;
 
-	/* Read and compensate temperature to get a reading of t_fine */
-	ret = bme680_read_temp(data, NULL);
-	if (ret < 0)
+	ret = bme680_get_t_fine(data, &t_fine);
+	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BME680_REG_PRESS_MSB,
-			       data->buf, BME680_PRESS_NUM_BYTES);
-	if (ret < 0) {
-		dev_err(dev, "failed to read pressure\n");
+	ret = bme680_read_press_adc(data, &adc_press);
+	if (ret)
 		return ret;
-	}
 
-	adc_press = FIELD_GET(BME680_MEAS_TRIM_MASK,
-			      get_unaligned_be24(data->buf));
-	if (adc_press == BME680_MEAS_SKIPPED) {
-		/* reading was skipped */
-		dev_err(dev, "reading pressure skipped\n");
-		return -EINVAL;
-	}
-
-	*val = bme680_compensate_press(data, adc_press);
+	*val = bme680_compensate_press(data, adc_press, t_fine);
 	*val2 = 1000;
 	return IIO_VAL_FRACTIONAL;
 }
@@ -624,30 +675,19 @@ static int bme680_read_press(struct bme680_data *data,
 static int bme680_read_humid(struct bme680_data *data,
 			     int *val, int *val2)
 {
-	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
-	u16 adc_humidity;
-	u32 comp_humidity;
+	u32 adc_humidity, comp_humidity;
+	s32 t_fine;
 
-	/* Read and compensate temperature to get a reading of t_fine */
-	ret = bme680_read_temp(data, NULL);
-	if (ret < 0)
+	ret = bme680_get_t_fine(data, &t_fine);
+	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BME680_REG_HUMIDITY_MSB,
-			       &data->be16, BME680_HUMID_NUM_BYTES);
-	if (ret < 0) {
-		dev_err(dev, "failed to read humidity\n");
+	ret = bme680_read_humid_adc(data, &adc_humidity);
+	if (ret)
 		return ret;
-	}
 
-	adc_humidity = be16_to_cpu(data->be16);
-	if (adc_humidity == BME680_MEAS_SKIPPED) {
-		/* reading was skipped */
-		dev_err(dev, "reading humidity skipped\n");
-		return -EINVAL;
-	}
-	comp_humidity = bme680_compensate_humid(data, adc_humidity);
+	comp_humidity = bme680_compensate_humid(data, adc_humidity, t_fine);
 
 	*val = comp_humidity;
 	*val2 = 1000;
-- 
2.25.1

Re: [PATCH v1 17/17] iio: chemical: bme680: Refactorize reading functions

[Jonathan Cameron]

On Mon, 27 May 2024 20:38:05 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> The reading of the pressure and humidity value, requires an update
> of the t_fine variable which happens by reading the temperature
> value.
> 
> So the bme680_read_{press/humid}() functions of the above sensors
> are internally calling the equivalent bme680_read_temp() function
> in order to update the t_fine value. By just looking at the code
> this relation is a bit hidden and is not easy to understand why
> those channels are not independent.
> 
> This commit tries to clear these thing a bit by splitting the
> bme680_{read/compensate}_{temp/press/humid}() to the following:
> 
> i. bme680_read_{temp/press/humid}_adc(): read the raw value from
> the sensor.
> 
> ii. bme680_calc_t_fine(): calculate the t_fine variable.
> 
> iii. bme680_get_t_fine(): get the t_fine variable.
> 
> iv. bme680_compensate_{temp/press/humid}(): compensate the adc
> values and return the calculated value.
> 
> v. bme680_read_{temp/press/humid}(): combine calls of the
> aforementioned functions to return the requested value.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
This and patches I didn't comment on (1-3, 6-9, 12, 15-17)
all look good to me. 

Thanks,

Jonathan

Re: [PATCH v1 17/17] iio: chemical: bme680: Refactorize reading functions

[Vasileios Amoiridis]

On Sun, Jun 02, 2024 at 02:04:05PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 20:38:05 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The reading of the pressure and humidity value, requires an update
> > of the t_fine variable which happens by reading the temperature
> > value.
> > 
> > So the bme680_read_{press/humid}() functions of the above sensors
> > are internally calling the equivalent bme680_read_temp() function
> > in order to update the t_fine value. By just looking at the code
> > this relation is a bit hidden and is not easy to understand why
> > those channels are not independent.
> > 
> > This commit tries to clear these thing a bit by splitting the
> > bme680_{read/compensate}_{temp/press/humid}() to the following:
> > 
> > i. bme680_read_{temp/press/humid}_adc(): read the raw value from
> > the sensor.
> > 
> > ii. bme680_calc_t_fine(): calculate the t_fine variable.
> > 
> > iii. bme680_get_t_fine(): get the t_fine variable.
> > 
> > iv. bme680_compensate_{temp/press/humid}(): compensate the adc
> > values and return the calculated value.
> > 
> > v. bme680_read_{temp/press/humid}(): combine calls of the
> > aforementioned functions to return the requested value.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> This and patches I didn't comment on (1-3, 6-9, 12, 15-17)
> all look good to me. 
> 
> Thanks,
> 
> Jonathan

Hi Jonathan,

Thank you very much for the quick and thorough review, it is
always extremely helpful. I will check a bit more on the
alignment thing internally in the I2C and SPI busses and I
will wait for your answer as well.

Cheers,
Vasilis

Re: [PATCH v1 00/17] iio: chemical: bme680: Driver cleanup

[Jonathan Cameron]

On Mon, 27 May 2024 20:37:48 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> This started as a series to add support for buffers and the new
> BME688 but it ended up being just a cleaning series. These might
> be quite some patches for such a thing but I feel that they are
> are well split, in order to allow for better review.
> 
> The patches are mostly small changes but essential for the correct use
> of the driver. The first patches looked like fixes that should be
> marked for the stable. Patches [11,17/17] might be a bit bigger but 11/17
> is quite straightforward and 17/17 is basically a duplication of a
> very similar commit coming from the BMP280 driver [1].
> 
> In general, the datasheet [2] of the driver is not very descriptive,
> and it redirects the user to the BME68x Sensor API [3]. All the things
> that were identified from the BME68x Sensor API have been marked with
> links to the original locations of the GitHub code. If this is too much
> and we don't want this type of information on the commit message, please
> let me know and I will fix it.
> 
> [1]: https://lore.kernel.org/linux-iio/20240512230524.53990-1-vassilisamir@gmail.com/T/#mc6f814e9a4f8c2b39015909d174c7013b3648b9b
> [2]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bme680-ds001.pdf
> [3]: https://github.com/boschsensortec/BME68x_SensorAPI/tree/master

+CC Mike and David who have been active around this driver in the past.


> 
> Vasileios Amoiridis (17):
>   iio: chemical: bme680: Fix pressure value output
>   iio: chemical: bme680: Fix calibration data variable
>   iio: chemical: bme680: Fix overflows in compensate() functions
>   iio: chemical: bme680: Fix sensor data read operation
>   iio: chemical: bme680: Fix type in define
>   iio: chemical: bme680: Add mutexes to guard read/write to device
>   iio: chemical: bme680: Drop unnecessary casts and correct adc data
>     types
>   iio: chemical: bme680: Remove remaining ACPI-only stuff
>   iio: chemical: bme680: Sort headers alphabetically
>   iio: chemical: bme680: Remove duplicate register read
>   iio: chemical: bme680: Use bulk reads for calibration data
>   iio: chemical: bme680: Allocate IIO device before chip initialization
>   iio: chemical: bme680: Add read buffers in DMA safe region
>   iio: chemical: bme680: Modify startup procedure
>   iio: chemical: bme680: Remove redundant gas configuration
>   iio: chemical: bme680: Move forced mode setup in ->read_raw()
>   iio: chemical: bme680: Refactorize reading functions
> 
>  drivers/iio/chemical/bme680.h      |  39 +-
>  drivers/iio/chemical/bme680_core.c | 643 ++++++++++++++---------------
>  2 files changed, 315 insertions(+), 367 deletions(-)
> 
> 
> base-commit: 409b6d632f5078f3ae1018b6e43c32f2e12f6736