[PATCH v1 00/13]: chemical: bme680: 2nd set of clean and add

[PATCH v1 00/13]: chemical: bme680: 2nd set of clean and add

[vamoirid]

This patch series is continuing the work that started on [1] by
improving some small issues of the driver in the commits 1,2,3.

Commits 4,5 are refactorizing existing code.

Commits 6,7,8 are adding DT, regulator and PM support.

Commit 9 is refactorizing one macro to attribute.

Commit 10,11,12 are refactorizing the read/compensate functions
to become generic and add triggered buffer support.

Finally, commit 13 adds support for an *output* channel of type
IIO_CURRENT in order to preheat the plate that is used to measure the
quality of the air.

This and the previous series [1] started with the idea to add support
for the new bme688 device but due to the structure of the driver I
decided that it is better to restructure and improve some things before
adding extra funcitonalities.

[1]: https://lore.kernel.org/linux-iio/20240609233826.330516-1-vassilisamir@gmail.com

Vasileios Amoiridis (13):
  iio: chemical: bme680: Fix indentation and unnecessary spaces
  iio: chemical: bme680: avoid using camel case
  iio: chemical: bme680: fix startup time
  iio: chemical: bme680: move to fsleep()
  iio: chemical: bme680: refactorize set_mode() mode
  dt-bindings: iio: add binding for BME680 driver
  iio: chemical: bme680: add regulators
  iio: chemical: bme680: add power management
  iio: chemical: bme680: Move ambient temperature to attributes
  iio: chemical: bme680: generalize read_*() functions
  iio: chemical: bme680: Add SCALE and RAW channels
  iio: chemical: bme680: Add triggered buffer support
  iio: chemical: bme680: Add support for preheat current

 .../bindings/iio/chemical/bosch,bme680.yaml   |  64 ++
 drivers/iio/chemical/Kconfig                  |   2 +
 drivers/iio/chemical/bme680.h                 |   7 +-
 drivers/iio/chemical/bme680_core.c            | 554 +++++++++++++++---
 4 files changed, 556 insertions(+), 71 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml

-- 
2.43.0

[PATCH v1 01/13] iio: chemical: bme680: Fix indentation and unnecessary spaces

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Fix indentation issues, line breaking and unnecessary spaces
reported by checkpatch.pl.

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 5d2e750ca2b9..95a667d56527 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -224,7 +224,7 @@ static int bme680_read_calib(struct bme680_data *data,
 	calib->res_heat_val = data->bme680_cal_buf_3[RES_HEAT_VAL];
 
 	calib->res_heat_range = FIELD_GET(BME680_RHRANGE_MASK,
-					data->bme680_cal_buf_3[RES_HEAT_RANGE]);
+					  data->bme680_cal_buf_3[RES_HEAT_RANGE]);
 
 	calib->range_sw_err = FIELD_GET(BME680_RSERROR_MASK,
 					data->bme680_cal_buf_3[RANGE_SW_ERR]);
@@ -294,8 +294,7 @@ static int bme680_get_t_fine(struct bme680_data *data, s32 *t_fine)
 	return 0;
 }
 
-static s16 bme680_compensate_temp(struct bme680_data *data,
-				  u32 adc_temp)
+static s16 bme680_compensate_temp(struct bme680_data *data, u32 adc_temp)
 {
 	return (bme680_calc_t_fine(data, adc_temp) * 5 + 128) / 256;
 }
@@ -445,12 +444,12 @@ static u32 bme680_compensate_gas(struct bme680_data *data, u16 gas_res_adc,
 				2136746228u, 2147483647u, 2126008810u,
 				2147483647u, 2147483647u};
 
-	var1 = ((1340 + (5 * (s64) calib->range_sw_err)) *
-			((s64) lookupTable[gas_range])) >> 16;
+	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
+			((s64)lookupTable[gas_range])) >> 16;
 	var2 = ((gas_res_adc << 15) - 16777216) + var1;
 	var3 = ((125000 << (15 - gas_range)) * var1) >> 9;
 	var3 += (var2 >> 1);
-	calc_gas_res = div64_s64(var3, (s64) var2);
+	calc_gas_res = div64_s64(var3, (s64)var2);
 
 	return calc_gas_res;
 }
@@ -468,7 +467,7 @@ static u8 bme680_calc_heater_res(struct bme680_data *data, u16 temp)
 	if (temp > 400) /* Cap temperature */
 		temp = 400;
 
-	var1 = (((s32) BME680_AMB_TEMP * calib->par_gh3) / 1000) * 256;
+	var1 = (((s32)BME680_AMB_TEMP * calib->par_gh3) / 1000) * 256;
 	var2 = (calib->par_gh1 + 784) * (((((calib->par_gh2 + 154009) *
 						temp * 5) / 100)
 						+ 3276800) / 10);
@@ -571,9 +570,8 @@ static int bme680_chip_config(struct bme680_data *data)
 	int ret;
 	u8 osrs;
 
-	osrs = FIELD_PREP(
-		BME680_OSRS_HUMIDITY_MASK,
-		bme680_oversampling_to_reg(data->oversampling_humid));
+	osrs = FIELD_PREP(BME680_OSRS_HUMIDITY_MASK,
+			  bme680_oversampling_to_reg(data->oversampling_humid));
 	/*
 	 * Highly recommended to set oversampling of humidity before
 	 * temperature/pressure oversampling.
@@ -587,8 +585,7 @@ static int bme680_chip_config(struct bme680_data *data)
 
 	/* IIR filter settings */
 	ret = regmap_update_bits(data->regmap, BME680_REG_CONFIG,
-				 BME680_FILTER_MASK,
-				 BME680_FILTER_COEFF_VAL);
+				 BME680_FILTER_MASK, BME680_FILTER_COEFF_VAL);
 	if (ret < 0) {
 		dev_err(dev, "failed to write config register\n");
 		return ret;
@@ -664,8 +661,7 @@ static int bme680_read_temp(struct bme680_data *data, int *val)
 	return IIO_VAL_INT;
 }
 
-static int bme680_read_press(struct bme680_data *data,
-			     int *val, int *val2)
+static int bme680_read_press(struct bme680_data *data, int *val, int *val2)
 {
 	int ret;
 	u32 adc_press;
@@ -684,8 +680,7 @@ static int bme680_read_press(struct bme680_data *data,
 	return IIO_VAL_FRACTIONAL;
 }
 
-static int bme680_read_humid(struct bme680_data *data,
-			     int *val, int *val2)
+static int bme680_read_humid(struct bme680_data *data, int *val, int *val2)
 {
 	int ret;
 	u32 adc_humidity, comp_humidity;
@@ -706,8 +701,7 @@ static int bme680_read_humid(struct bme680_data *data,
 	return IIO_VAL_FRACTIONAL;
 }
 
-static int bme680_read_gas(struct bme680_data *data,
-			   int *val)
+static int bme680_read_gas(struct bme680_data *data, int *val)
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
@@ -889,8 +883,7 @@ 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);
+	ret = regmap_write(regmap, BME680_REG_SOFT_RESET, BME680_CMD_SOFTRESET);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to reset chip\n");
 
-- 
2.43.0

Re: [PATCH v1 01/13] iio: chemical: bme680: Fix indentation and unnecessary spaces

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:18PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Fix indentation issues, line breaking and unnecessary spaces
> reported by checkpatch.pl.

Can we move this to be the last (or close to the end) in the series?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/13] iio: chemical: bme680: Fix indentation and unnecessary spaces

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 12:55:00PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:18PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Fix indentation issues, line breaking and unnecessary spaces
> > reported by checkpatch.pl.
> 
> Can we move this to be the last (or close to the end) in the series?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Hi Andy,

Yes, will do it.

[PATCH v1 02/13] iio: chemical: bme680: avoid using camel case

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Rename camel case variable, as checkpatch.pl complains.

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 95a667d56527..3b4431998ca4 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -437,7 +437,7 @@ static u32 bme680_compensate_gas(struct bme680_data *data, u16 gas_res_adc,
 	u32 calc_gas_res;
 
 	/* Look up table for the possible gas range values */
-	static const u32 lookupTable[16] = {2147483647u, 2147483647u,
+	static const u32 lookup_table[16] = {2147483647u, 2147483647u,
 				2147483647u, 2147483647u, 2147483647u,
 				2126008810u, 2147483647u, 2130303777u,
 				2147483647u, 2147483647u, 2143188679u,
@@ -445,7 +445,7 @@ static u32 bme680_compensate_gas(struct bme680_data *data, u16 gas_res_adc,
 				2147483647u, 2147483647u};
 
 	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
-			((s64)lookupTable[gas_range])) >> 16;
+			((s64)lookup_table[gas_range])) >> 16;
 	var2 = ((gas_res_adc << 15) - 16777216) + var1;
 	var3 = ((125000 << (15 - gas_range)) * var1) >> 9;
 	var3 += (var2 >> 1);
-- 
2.43.0

Re: [PATCH v1 02/13] iio: chemical: bme680: avoid using camel case

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:19PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Rename camel case variable, as checkpatch.pl complains.

With given reply to the first patch...

...

>  	/* Look up table for the possible gas range values */
> -	static const u32 lookupTable[16] = {2147483647u, 2147483647u,
> +	static const u32 lookup_table[16] = {2147483647u, 2147483647u,
>  				2147483647u, 2147483647u, 2147483647u,
>  				2126008810u, 2147483647u, 2130303777u,
>  				2147483647u, 2147483647u, 2143188679u,

...here is the opportunity to fix indentation while at fixing the code.
I.o.w. I would reformat the entire table to be

	static const u32 lookup_table[16] = {
		2147483647u, 2147483647u, 2147483647u, 2147483647u,
		2147483647u, 2126008810u, 2147483647u, 2130303777u,
		2147483647u, 2147483647u, 2143188679u, ...

(also note power-of-2 number of items per line which much easier to read and
 find one you need).

...

>  	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
> -			((s64)lookupTable[gas_range])) >> 16;
> +			((s64)lookup_table[gas_range])) >> 16;

Also an opportunity to make this neater like

	var1 = (1340 + (5 * (s64)calib->range_sw_err)) * (s64)lookup_table[gas_range]);
	var1 >>= 16;

So, at bare minumym there are redundant parentheses. And looking at the table
and the first argument of multiplication I'm puzzled why casting is needed for
the second? Shouldn't s64 already be implied by the first one?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 02/13] iio: chemical: bme680: avoid using camel case

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:00:33PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:19PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Rename camel case variable, as checkpatch.pl complains.
> 
> With given reply to the first patch...
>

Hi Andy,

> ...
> 
> >  	/* Look up table for the possible gas range values */
> > -	static const u32 lookupTable[16] = {2147483647u, 2147483647u,
> > +	static const u32 lookup_table[16] = {2147483647u, 2147483647u,
> >  				2147483647u, 2147483647u, 2147483647u,
> >  				2126008810u, 2147483647u, 2130303777u,
> >  				2147483647u, 2147483647u, 2143188679u,
> 
> ...here is the opportunity to fix indentation while at fixing the code.
> I.o.w. I would reformat the entire table to be
> 
> 	static const u32 lookup_table[16] = {
> 		2147483647u, 2147483647u, 2147483647u, 2147483647u,
> 		2147483647u, 2126008810u, 2147483647u, 2130303777u,
> 		2147483647u, 2147483647u, 2143188679u, ...
> 
> (also note power-of-2 number of items per line which much easier to read and
>  find one you need).
> 

ACK.

> ...
> 
> >  	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
> > -			((s64)lookupTable[gas_range])) >> 16;
> > +			((s64)lookup_table[gas_range])) >> 16;
> 
> Also an opportunity to make this neater like
> 
> 	var1 = (1340 + (5 * (s64)calib->range_sw_err)) * (s64)lookup_table[gas_range]);
> 	var1 >>= 16;
> 
> So, at bare minumym there are redundant parentheses. And looking at the table
> and the first argument of multiplication I'm puzzled why casting is needed for
> the second? Shouldn't s64 already be implied by the first one?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

I think the 2nd cast indeed is not needed since the 1st one forces the
s64 so I can remove it.

Cheers,
Vasilis

Re: [PATCH v1 02/13] iio: chemical: bme680: avoid using camel case

[Andy Shevchenko]

Fri, Oct 11, 2024 at 08:50:27PM +0200, Vasileios Aoiridis kirjoitti:
> On Fri, Oct 11, 2024 at 01:00:33PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 11:00:19PM +0200, vamoirid wrote:

...

> > >  	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
> > > -			((s64)lookupTable[gas_range])) >> 16;
> > > +			((s64)lookup_table[gas_range])) >> 16;
> > 
> > Also an opportunity to make this neater like
> > 
> > 	var1 = (1340 + (5 * (s64)calib->range_sw_err)) * (s64)lookup_table[gas_range]);
> > 	var1 >>= 16;
> > 
> > So, at bare minumym there are redundant parentheses. And looking at the table
> > and the first argument of multiplication I'm puzzled why casting is needed for
> > the second? Shouldn't s64 already be implied by the first one?
> 
> I think the 2nd cast indeed is not needed since the 1st one forces the
> s64 so I can remove it.

Thinking about this more, you don't need the first either,
if using 5LL instead of 5.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 03/13] iio: chemical: bme680: fix startup time

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

According to datasheet's Section 1.1, Table 1, the startup time for the
device is 2ms and not 5ms.

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

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index b2c547ac8d34..e55a48982b3e 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -63,7 +63,7 @@
 
 #define BME680_MEAS_TRIM_MASK			GENMASK(24, 4)
 
-#define BME680_STARTUP_TIME_US			5000
+#define BME680_STARTUP_TIME_US			2000
 
 /* Calibration Parameters */
 #define BME680_T2_LSB_REG	0x8A
-- 
2.43.0

Re: [PATCH v1 03/13] iio: chemical: bme680: fix startup time

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:20PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> According to datasheet's Section 1.1, Table 1, the startup time for the
> device is 2ms and not 5ms.

Fixes tag?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 03/13] iio: chemical: bme680: fix startup time

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:00:55PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:20PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > According to datasheet's Section 1.1, Table 1, the startup time for the
> > device is 2ms and not 5ms.
> 
> Fixes tag?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Hi Andy,

It is not affecting at all the operation of the driver so I was not sure
if it was worth it to be backported to the previous versions. This is
why I didn't put a fixes tag. You think for such a fix is necessary?

Cheers,
Vasilis

Re: [PATCH v1 03/13] iio: chemical: bme680: fix startup time

[Jonathan Cameron]

On Fri, 11 Oct 2024 20:51:41 +0200
Vasileios Aoiridis <vassilisamir@gmail.com> wrote:

> On Fri, Oct 11, 2024 at 01:00:55PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 11:00:20PM +0200, vamoirid wrote:  
> > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > 
> > > According to datasheet's Section 1.1, Table 1, the startup time for the
> > > device is 2ms and not 5ms.  
> > 
> > Fixes tag?
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> >  
> 
> Hi Andy,
> 
> It is not affecting at all the operation of the driver so I was not sure
> if it was worth it to be backported to the previous versions. This is
> why I didn't put a fixes tag. You think for such a fix is necessary?
> 
Not for a reduction in sleep and not a huge one at that.
It's an optimization, not a fix to my eyes!

> Cheers,
> Vasilis

Re: [PATCH v1 03/13] iio: chemical: bme680: fix startup time

[Andy Shevchenko]

Fri, Oct 11, 2024 at 08:51:41PM +0200, Vasileios Aoiridis kirjoitti:
> On Fri, Oct 11, 2024 at 01:00:55PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 11:00:20PM +0200, vamoirid wrote:
> > > 
> > > According to datasheet's Section 1.1, Table 1, the startup time for the
> > > device is 2ms and not 5ms.
> > 
> > Fixes tag?
> 
> It is not affecting at all the operation of the driver so I was not sure
> if it was worth it to be backported to the previous versions. This is
> why I didn't put a fixes tag. You think for such a fix is necessary?

The commit message siggests that this is a fix. If you want to make it clear,
that it shouldn't be considered as a such, perhaps you need to rephrase it.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 04/13] iio: chemical: bme680: move to fsleep()

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Use the new fsleep() function in the remaining driver instances.

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 3b4431998ca4..9e843e463502 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -545,7 +545,7 @@ static int bme680_wait_for_eoc(struct bme680_data *data)
 			   data->oversampling_humid) * 1936) + (477 * 4) +
 			   (477 * 5) + 1000 + (data->heater_dur * 1000);
 
-	usleep_range(wait_eoc_us, wait_eoc_us + 100);
+	fsleep(wait_eoc_us);
 
 	ret = regmap_read(data->regmap, BME680_REG_MEAS_STAT_0, &data->check);
 	if (ret) {
@@ -887,7 +887,7 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to reset chip\n");
 
-	usleep_range(BME680_STARTUP_TIME_US, BME680_STARTUP_TIME_US + 1000);
+	fsleep(BME680_STARTUP_TIME_US);
 
 	ret = regmap_read(regmap, BME680_REG_CHIP_ID, &data->check);
 	if (ret < 0)
-- 
2.43.0

[PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Refactorize the set_mode() function to use an external enum that
describes the possible modes of the BME680 device instead of using
true/false variables for selecting SLEEPING/FORCED mode.

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 9e843e463502..dedb7edaf43d 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -95,6 +95,11 @@ struct bme680_calib {
 	s8  range_sw_err;
 };
 
+enum bme680_op_mode {
+	BME680_SLEEP,
+	BME680_FORCED,
+};
+
 struct bme680_data {
 	struct regmap *regmap;
 	struct bme680_calib bme680;
@@ -501,25 +506,24 @@ static u8 bme680_calc_heater_dur(u16 dur)
 	return durval;
 }
 
-static int bme680_set_mode(struct bme680_data *data, bool mode)
+static int bme680_set_mode(struct bme680_data *data, enum bme680_op_mode mode)
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
-	if (mode) {
-		ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
-					BME680_MODE_MASK, BME680_MODE_FORCED);
-		if (ret < 0)
-			dev_err(dev, "failed to set forced mode\n");
-
-	} else {
-		ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
-					BME680_MODE_MASK, BME680_MODE_SLEEP);
-		if (ret < 0)
-			dev_err(dev, "failed to set sleep mode\n");
-
+	switch (mode) {
+	case BME680_SLEEP:
+	case BME680_FORCED:
+		break;
+	default:
+		return -EINVAL;
 	}
 
+	ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
+				BME680_MODE_MASK, mode);
+	if (ret < 0)
+		dev_err(dev, "failed to set ctrl_meas register\n");
+
 	return ret;
 }
 
@@ -612,8 +616,7 @@ 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);
+	ret = bme680_set_mode(data, BME680_SLEEP);
 	if (ret < 0)
 		return ret;
 
@@ -750,8 +753,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
 
 	guard(mutex)(&data->lock);
 
-	/* set forced mode to trigger measurement */
-	ret = bme680_set_mode(data, true);
+	ret = bme680_set_mode(data, BME680_FORCED);
 	if (ret < 0)
 		return ret;
 
-- 
2.43.0

Re: [PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:22PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Refactorize the set_mode() function to use an external enum that
> describes the possible modes of the BME680 device instead of using
> true/false variables for selecting SLEEPING/FORCED mode.

...

> -	if (mode) {
> -		ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> -					BME680_MODE_MASK, BME680_MODE_FORCED);
> -		if (ret < 0)
> -			dev_err(dev, "failed to set forced mode\n");
> -
> -	} else {
> -		ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> -					BME680_MODE_MASK, BME680_MODE_SLEEP);
> -		if (ret < 0)
> -			dev_err(dev, "failed to set sleep mode\n");
> -
> +	switch (mode) {
> +	case BME680_SLEEP:
> +	case BME680_FORCED:
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> +	ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> +				BME680_MODE_MASK, mode);
> +	if (ret < 0)
> +		dev_err(dev, "failed to set ctrl_meas register\n");

This is an information loss. You shuld probably still have a value of mode to
be printed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:02:28PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:22PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Refactorize the set_mode() function to use an external enum that
> > describes the possible modes of the BME680 device instead of using
> > true/false variables for selecting SLEEPING/FORCED mode.
> 
> ...
> 
> > -	if (mode) {
> > -		ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> > -					BME680_MODE_MASK, BME680_MODE_FORCED);
> > -		if (ret < 0)
> > -			dev_err(dev, "failed to set forced mode\n");
> > -
> > -	} else {
> > -		ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> > -					BME680_MODE_MASK, BME680_MODE_SLEEP);
> > -		if (ret < 0)
> > -			dev_err(dev, "failed to set sleep mode\n");
> > -
> > +	switch (mode) {
> > +	case BME680_SLEEP:
> > +	case BME680_FORCED:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> >  	}
> >  
> > +	ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> > +				BME680_MODE_MASK, mode);
> > +	if (ret < 0)
> > +		dev_err(dev, "failed to set ctrl_meas register\n");
> 
> This is an information loss. You shuld probably still have a value of mode to
> be printed.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

You are right, I missed a return here.

Cheers,
Vasilis

Re: [PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode

[Jonathan Cameron]

On Thu, 10 Oct 2024 23:00:22 +0200
vamoirid <vassilisamir@gmail.com> wrote:

> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Refactorize the set_mode() function to use an external enum that
> describes the possible modes of the BME680 device instead of using
> true/false variables for selecting SLEEPING/FORCED mode.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/chemical/bme680_core.c | 36 ++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 9e843e463502..dedb7edaf43d 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -95,6 +95,11 @@ struct bme680_calib {
>  	s8  range_sw_err;
>  };
>  
> +enum bme680_op_mode {
> +	BME680_SLEEP,
> +	BME680_FORCED,
> +};
> +
>  struct bme680_data {
>  	struct regmap *regmap;
>  	struct bme680_calib bme680;
> @@ -501,25 +506,24 @@ static u8 bme680_calc_heater_dur(u16 dur)
>  	return durval;
>  }
>  
> -static int bme680_set_mode(struct bme680_data *data, bool mode)
> +static int bme680_set_mode(struct bme680_data *data, enum bme680_op_mode mode)
>  {
>  	struct device *dev = regmap_get_device(data->regmap);
>  	int ret;
>  
> -	if (mode) {
> -		ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> -					BME680_MODE_MASK, BME680_MODE_FORCED);
> -		if (ret < 0)
> -			dev_err(dev, "failed to set forced mode\n");
> -
> -	} else {
> -		ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> -					BME680_MODE_MASK, BME680_MODE_SLEEP);
> -		if (ret < 0)
> -			dev_err(dev, "failed to set sleep mode\n");
> -
> +	switch (mode) {
> +	case BME680_SLEEP:
> +	case BME680_FORCED:
You are passing in an enum that currently has no other values.
The compiler should complain if it isn't one of these (and it can tell)

So unless I'm missing you adding another enum value later, this switch
should be unnecessary.


> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> +	ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> +				BME680_MODE_MASK, mode);
> +	if (ret < 0)
> +		dev_err(dev, "failed to set ctrl_meas register\n");
> +
>  	return ret;
>  }
>  
> @@ -612,8 +616,7 @@ 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);
> +	ret = bme680_set_mode(data, BME680_SLEEP);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -750,8 +753,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
>  
>  	guard(mutex)(&data->lock);
>  
> -	/* set forced mode to trigger measurement */
> -	ret = bme680_set_mode(data, true);
> +	ret = bme680_set_mode(data, BME680_FORCED);
>  	if (ret < 0)
>  		return ret;
>  

[PATCH v1 06/13] dt-bindings: iio: add binding for BME680 driver

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Add dt-binding for BME680 gas sensor device. The device incorporates as
well temperature, pressure and relative humidity sensors.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 .../bindings/iio/chemical/bosch,bme680.yaml   | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml

diff --git a/Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml b/Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml
new file mode 100644
index 000000000000..e54df3afa7b2
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/bosch,bme680.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bosch BME680 Gas sensor
+
+maintainers:
+  - Vasileios Amoiridis <vassilisamir@gmail.com>
+
+description:
+  BME680 is a gas sensor which combines relative humidity, barometric pressure,
+  ambient temperature and gas (VOC - Volatile Organic Compounds) measurements.
+
+  https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bme680-ds001.pdf
+
+properties:
+  compatible:
+    const: bosch,bme680
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+  vddio-supply: true
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddio-supply
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        bme680@77 {
+            compatible = "bosch,bme680";
+            reg = <0x77>;
+            vddio-supply = <&vddio>;
+            vdd-supply = <&vdd>;
+        };
+    };
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        bme680@0 {
+            compatible = "bosch,bme680";
+            reg = <0>;
+            spi-max-frequency = <500000>;
+            vddio-supply = <&vddio>;
+            vdd-supply = <&vdd>;
+        };
+    };
-- 
2.43.0

Re: [PATCH v1 06/13] dt-bindings: iio: add binding for BME680 driver

[Rob Herring (Arm)]

On Thu, 10 Oct 2024 23:00:23 +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Add dt-binding for BME680 gas sensor device. The device incorporates as
> well temperature, pressure and relative humidity sensors.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  .../bindings/iio/chemical/bosch,bme680.yaml   | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Warning: Duplicate compatible "bosch,bme680" found in schemas matching "$id":
	http://devicetree.org/schemas/iio/chemical/bosch,bme680.yaml#
	http://devicetree.org/schemas/trivial-devices.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/chemical/bosch,bme680.example.dtb: bme680@77: 'vdd-supply', 'vddio-supply' do not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/chemical/bosch,bme680.example.dtb: bme680@0: 'vdd-supply', 'vddio-supply' do not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241010210030.33309-7-vassilisamir@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 06/13] dt-bindings: iio: add binding for BME680 driver

[Krzysztof Kozlowski]

On 10/10/2024 23:00, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Add dt-binding for BME680 gas sensor device. The device incorporates as
> well temperature, pressure and relative humidity sensors.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  .../bindings/iio/chemical/bosch,bme680.yaml   | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml

The device is already documented. You need to move it from trivial devices.

Best regards,
Krzysztof

Re: [PATCH v1 06/13] dt-bindings: iio: add binding for BME680 driver

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 08:51:00AM +0200, Krzysztof Kozlowski wrote:
> On 10/10/2024 23:00, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Add dt-binding for BME680 gas sensor device. The device incorporates as
> > well temperature, pressure and relative humidity sensors.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  .../bindings/iio/chemical/bosch,bme680.yaml   | 64 +++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml
> 
> The device is already documented. You need to move it from trivial devices.
> 
> Best regards,
> Krzysztof
>

Hi Krzysztof!

Thanks for your time to have a look at this!

You mean to keep this new dt-binging and remove it from trivial devices?

Cheers,
Vasilis

Re: [PATCH v1 06/13] dt-bindings: iio: add binding for BME680 driver

[Jonathan Cameron]

On Fri, 11 Oct 2024 20:44:51 +0200
Vasileios Aoiridis <vassilisamir@gmail.com> wrote:

> On Fri, Oct 11, 2024 at 08:51:00AM +0200, Krzysztof Kozlowski wrote:
> > On 10/10/2024 23:00, vamoirid wrote:  
> > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > 
> > > Add dt-binding for BME680 gas sensor device. The device incorporates as
> > > well temperature, pressure and relative humidity sensors.
> > > 
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > ---
> > >  .../bindings/iio/chemical/bosch,bme680.yaml   | 64 +++++++++++++++++++
> > >  1 file changed, 64 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml  
> > 
> > The device is already documented. You need to move it from trivial devices.
> > 
> > Best regards,
> > Krzysztof
> >  
> 
> Hi Krzysztof!
> 
> Thanks for your time to have a look at this!
> 
> You mean to keep this new dt-binging and remove it from trivial devices?
> 
Yes

> Cheers,
> Vasilis
> 

[PATCH v1 07/13] iio: chemical: bme680: add regulators

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Add support for the regulators described in the dt-binding.

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index dedb7edaf43d..a2039b966f20 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -15,6 +15,7 @@
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -100,6 +101,12 @@ enum bme680_op_mode {
 	BME680_FORCED,
 };
 
+static const char *const bme680_supply_names[] = {
+	"vdd", "vddio"
+};
+
+#define BME680_NUM_SUPPLIES ARRAY_SIZE(bme680_supply_names)
+
 struct bme680_data {
 	struct regmap *regmap;
 	struct bme680_calib bme680;
@@ -110,6 +117,8 @@ struct bme680_data {
 	u16 heater_dur;
 	u16 heater_temp;
 
+	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
+
 	union {
 		u8 buf[3];
 		unsigned int check;
@@ -857,6 +866,13 @@ static const struct iio_info bme680_info = {
 	.attrs = &bme680_attribute_group,
 };
 
+static void bme680_regulators_disable(void *data)
+{
+	struct regulator_bulk_data *supplies = data;
+
+	regulator_bulk_disable(BME680_NUM_SUPPLIES, supplies);
+}
+
 int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		      const char *name)
 {
@@ -885,6 +901,20 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	data->heater_temp = 320; /* degree Celsius */
 	data->heater_dur = 150;  /* milliseconds */
 
+	regulator_bulk_set_supply_names(data->supplies, bme680_supply_names,
+					BME680_NUM_SUPPLIES);
+	ret = devm_regulator_bulk_get(dev, BME680_NUM_SUPPLIES, data->supplies);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get regulators\n");
+
+	ret = regulator_bulk_enable(BME680_NUM_SUPPLIES, data->supplies);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable regulators\n");
+
+	devm_add_action_or_reset(dev, bme680_regulators_disable, data->supplies);
+
+	fsleep(BME680_STARTUP_TIME_US);
+
 	ret = regmap_write(regmap, BME680_REG_SOFT_RESET, BME680_CMD_SOFTRESET);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to reset chip\n");
-- 
2.43.0

Re: [PATCH v1 07/13] iio: chemical: bme680: add regulators

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:24PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Add support for the regulators described in the dt-binding.

...

> +static const char *const bme680_supply_names[] = {
> +	"vdd", "vddio"

Leave trailing comma.

> +};

...

> +	devm_add_action_or_reset(dev, bme680_regulators_disable, data->supplies);

No error check?! Why?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 07/13] iio: chemical: bme680: add regulators

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:05:06PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:24PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Add support for the regulators described in the dt-binding.
> 
> ...
> 
> > +static const char *const bme680_supply_names[] = {
> > +	"vdd", "vddio"
> 
> Leave trailing comma.
>

ACK.

> > +};
> 
> ...
> 
> > +	devm_add_action_or_reset(dev, bme680_regulators_disable, data->supplies);
> 
> No error check?! Why?
>

You are right, I should put it, I probably didnit see it!

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Cheers,
Vasilis

Re: [PATCH v1 07/13] iio: chemical: bme680: add regulators

[Jonathan Cameron]

On Thu, 10 Oct 2024 23:00:24 +0200
vamoirid <vassilisamir@gmail.com> wrote:

> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Add support for the regulators described in the dt-binding.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/chemical/bme680_core.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index dedb7edaf43d..a2039b966f20 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -15,6 +15,7 @@
>  #include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -100,6 +101,12 @@ enum bme680_op_mode {
>  	BME680_FORCED,
>  };
>  
> +static const char *const bme680_supply_names[] = {
> +	"vdd", "vddio"
> +};
> +
> +#define BME680_NUM_SUPPLIES ARRAY_SIZE(bme680_supply_names)
> +
>  struct bme680_data {
>  	struct regmap *regmap;
>  	struct bme680_calib bme680;
> @@ -110,6 +117,8 @@ struct bme680_data {
>  	u16 heater_dur;
>  	u16 heater_temp;
>  
> +	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
> +
>  	union {
>  		u8 buf[3];
>  		unsigned int check;
> @@ -857,6 +866,13 @@ static const struct iio_info bme680_info = {
>  	.attrs = &bme680_attribute_group,
>  };
>  
> +static void bme680_regulators_disable(void *data)
> +{
> +	struct regulator_bulk_data *supplies = data;
> +
> +	regulator_bulk_disable(BME680_NUM_SUPPLIES, supplies);
> +}
> +
>  int bme680_core_probe(struct device *dev, struct regmap *regmap,
>  		      const char *name)
>  {
> @@ -885,6 +901,20 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
>  	data->heater_temp = 320; /* degree Celsius */
>  	data->heater_dur = 150;  /* milliseconds */
>  
> +	regulator_bulk_set_supply_names(data->supplies, bme680_supply_names,
> +					BME680_NUM_SUPPLIES);
> +	ret = devm_regulator_bulk_get(dev, BME680_NUM_SUPPLIES, data->supplies);

devm_regulator_bulk_get_enable() should replace all this with functionally
equivalent code.  You can also have the supplies list local as you don't
then need to hang on to a copy.


> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(BME680_NUM_SUPPLIES, data->supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable regulators\n");
> +
> +	devm_add_action_or_reset(dev, bme680_regulators_disable, data->supplies);

> +
> +	fsleep(BME680_STARTUP_TIME_US);
> +
>  	ret = regmap_write(regmap, BME680_REG_SOFT_RESET, BME680_CMD_SOFTRESET);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to reset chip\n");

[PATCH v1 08/13] iio: chemical: bme680: add power management

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Add runtime power management to the device. To facilitate this, add also
a struct dev * inside the bme680_data structure to have the device
accesible from the data structure.

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

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index e55a48982b3e..e9e3e08fa366 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -75,6 +75,7 @@
 #define BME680_CALIB_RANGE_3_LEN               5
 
 extern const struct regmap_config bme680_regmap_config;
+extern const struct dev_pm_ops bmp280_dev_pm_ops;
 
 int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		      const char *name);
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index a2039b966f20..5fd5740bb7fe 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -14,6 +14,7 @@
 #include <linux/device.h>
 #include <linux/log2.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -111,6 +112,7 @@ struct bme680_data {
 	struct regmap *regmap;
 	struct bme680_calib bme680;
 	struct mutex lock; /* Protect multiple serial R/W ops to device. */
+	struct device *dev;
 	u8 oversampling_temp;
 	u8 oversampling_press;
 	u8 oversampling_humid;
@@ -753,9 +755,9 @@ static int bme680_read_gas(struct bme680_data *data, int *val)
 	return IIO_VAL_INT;
 }
 
-static int bme680_read_raw(struct iio_dev *indio_dev,
-			   struct iio_chan_spec const *chan,
-			   int *val, int *val2, long mask)
+static int __bme680_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
 {
 	struct bme680_data *data = iio_priv(indio_dev);
 	int ret;
@@ -803,14 +805,29 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int bme680_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct bme680_data *data = iio_priv(indio_dev);
+	int ret;
+
+	pm_runtime_get_sync(data->dev);
+	ret = __bme680_read_raw(indio_dev, chan, val, val2, mask);
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
+	return ret;
+}
+
 static bool bme680_is_valid_oversampling(int rate)
 {
 	return (rate > 0 && rate <= 16 && is_power_of_2(rate));
 }
 
-static int bme680_write_raw(struct iio_dev *indio_dev,
-			    struct iio_chan_spec const *chan,
-			    int val, int val2, long mask)
+static int __bme680_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
 {
 	struct bme680_data *data = iio_priv(indio_dev);
 
@@ -846,6 +863,21 @@ static int bme680_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int bme680_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct bme680_data *data = iio_priv(indio_dev);
+	int ret;
+
+	pm_runtime_get_sync(data->dev);
+	ret = __bme680_write_raw(indio_dev, chan, val, val2, mask);
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
+	return ret;
+}
+
 static const char bme680_oversampling_ratio_show[] = "1 2 4 8 16";
 
 static IIO_CONST_ATTR(oversampling_ratio_available,
@@ -873,6 +905,15 @@ static void bme680_regulators_disable(void *data)
 	regulator_bulk_disable(BME680_NUM_SUPPLIES, supplies);
 }
 
+static void bme680_pm_disable(void *data)
+{
+	struct device *dev = data;
+
+	pm_runtime_get_sync(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_disable(dev);
+}
+
 int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		      const char *name)
 {
@@ -887,6 +928,7 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	data = iio_priv(indio_dev);
 	mutex_init(&data->lock);
 	dev_set_drvdata(dev, indio_dev);
+	data->dev = dev;
 	data->regmap = regmap;
 	indio_dev->name = name;
 	indio_dev->channels = bme680_channels;
@@ -947,10 +989,56 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		return dev_err_probe(dev, ret,
 				     "failed to set gas config data\n");
 
+	/* Enable runtime PM */
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_put(dev);
+
+	ret = devm_add_action_or_reset(dev, bme680_pm_disable, dev);
+	if (ret)
+		return ret;
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(bme680_core_probe, IIO_BME680);
 
+static int bme680_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct bme680_data *data = iio_priv(indio_dev);
+
+	return regulator_bulk_disable(BME680_NUM_SUPPLIES, data->supplies);
+}
+
+static int bme680_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct bme680_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regulator_bulk_enable(BME680_NUM_SUPPLIES, data->supplies);
+	if (ret)
+		return ret;
+
+	fsleep(BME680_STARTUP_TIME_US);
+
+	ret = bme680_chip_config(data);
+	if (ret)
+		return ret;
+
+	ret = bme680_gas_config(data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+EXPORT_RUNTIME_DEV_PM_OPS(bme680_dev_pm_ops, bme680_runtime_suspend,
+			  bme680_runtime_resume, NULL);
+
 MODULE_AUTHOR("Himanshu Jha <himanshujha199640@gmail.com>");
 MODULE_DESCRIPTION("Bosch BME680 Driver");
 MODULE_LICENSE("GPL v2");
-- 
2.43.0

Re: [PATCH v1 08/13] iio: chemical: bme680: add power management

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:25PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Add runtime power management to the device. To facilitate this, add also
> a struct dev * inside the bme680_data structure to have the device
> accesible from the data structure.

...

> --- a/drivers/iio/chemical/bme680.h
> +++ b/drivers/iio/chemical/bme680.h
> @@ -75,6 +75,7 @@
>  #define BME680_CALIB_RANGE_3_LEN               5
>  
>  extern const struct regmap_config bme680_regmap_config;
> +extern const struct dev_pm_ops bmp280_dev_pm_ops;

Is pm.h being included already in this header? Otherwise you need to add it.

...

>  	struct regmap *regmap;
>  	struct bme680_calib bme680;
>  	struct mutex lock; /* Protect multiple serial R/W ops to device. */
> +	struct device *dev;

Is it the same that you may get wia regmap_get_device()?

>  	u8 oversampling_temp;
>  	u8 oversampling_press;
>  	u8 oversampling_humid;

...

> +	/* Enable runtime PM */
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put(dev);

Can we use devm_pm_runtime_enable() for some of the above?

> +	ret = devm_add_action_or_reset(dev, bme680_pm_disable, dev);
> +	if (ret)
> +		return ret;

...

> +static int bme680_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct bme680_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(BME680_NUM_SUPPLIES, data->supplies);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(BME680_STARTUP_TIME_US);
> +
> +	ret = bme680_chip_config(data);
> +	if (ret)
> +		return ret;

> +	ret = bme680_gas_config(data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return bme680_gas_config(...);

> +}

...

> +EXPORT_RUNTIME_DEV_PM_OPS(bme680_dev_pm_ops, bme680_runtime_suspend,
> +			  bme680_runtime_resume, NULL);

You also need pm.h for the macro IIRC.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 08/13] iio: chemical: bme680: add power management

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:10:20PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:25PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Add runtime power management to the device. To facilitate this, add also
> > a struct dev * inside the bme680_data structure to have the device
> > accesible from the data structure.
> 
> ...
> 
> > --- a/drivers/iio/chemical/bme680.h
> > +++ b/drivers/iio/chemical/bme680.h
> > @@ -75,6 +75,7 @@
> >  #define BME680_CALIB_RANGE_3_LEN               5
> >  
> >  extern const struct regmap_config bme680_regmap_config;
> > +extern const struct dev_pm_ops bmp280_dev_pm_ops;
> 
> Is pm.h being included already in this header? Otherwise you need to add it.
>

No it is not, and indeed I need to add it. Probably because it was
included by some other file I didn't get an error from gcc?

> ...
> 
> >  	struct regmap *regmap;
> >  	struct bme680_calib bme680;
> >  	struct mutex lock; /* Protect multiple serial R/W ops to device. */
> > +	struct device *dev;
> 
> Is it the same that you may get wia regmap_get_device()?
> 

Yes it is the same. Maybe I can try and see if I can use the following

	regmap_get_device(data->regmap)

in the places where the pm functions are used in order to not declare a
new value inside the struct bme680_data. But in general, is this approach
prefered?

> >  	u8 oversampling_temp;
> >  	u8 oversampling_press;
> >  	u8 oversampling_humid;
> 
> ...
> 
> > +	/* Enable runtime PM */
> > +	pm_runtime_get_noresume(dev);
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100);
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_put(dev);
> 
> Can we use devm_pm_runtime_enable() for some of the above?
>

I will have to check its use, and I will let you know.

> > +	ret = devm_add_action_or_reset(dev, bme680_pm_disable, dev);
> > +	if (ret)
> > +		return ret;
> 
> ...
> 
> > +static int bme680_runtime_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct bme680_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = regulator_bulk_enable(BME680_NUM_SUPPLIES, data->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> > +	fsleep(BME680_STARTUP_TIME_US);
> > +
> > +	ret = bme680_chip_config(data);
> > +	if (ret)
> > +		return ret;
> 
> > +	ret = bme680_gas_config(data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> 	return bme680_gas_config(...);
> 

Indeed, much cleaner, thanks!

> > +}
> 
> ...
> 
> > +EXPORT_RUNTIME_DEV_PM_OPS(bme680_dev_pm_ops, bme680_runtime_suspend,
> > +			  bme680_runtime_resume, NULL);
> 
> You also need pm.h for the macro IIRC.
> 

ACK.

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Cheers,
Vasilis

Re: [PATCH v1 08/13] iio: chemical: bme680: add power management

[Jonathan Cameron]

On Fri, 11 Oct 2024 21:02:32 +0200
Vasileios Aoiridis <vassilisamir@gmail.com> wrote:

> On Fri, Oct 11, 2024 at 01:10:20PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 11:00:25PM +0200, vamoirid wrote:  
> > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > 
> > > Add runtime power management to the device. To facilitate this, add also
> > > a struct dev * inside the bme680_data structure to have the device
> > > accesible from the data structure.  
> > 
> > ...
> >   
> > > --- a/drivers/iio/chemical/bme680.h
> > > +++ b/drivers/iio/chemical/bme680.h
> > > @@ -75,6 +75,7 @@
> > >  #define BME680_CALIB_RANGE_3_LEN               5
> > >  
> > >  extern const struct regmap_config bme680_regmap_config;
> > > +extern const struct dev_pm_ops bmp280_dev_pm_ops;  
> > 
> > Is pm.h being included already in this header? Otherwise you need to add it.
> >  
> 
> No it is not, and indeed I need to add it. Probably because it was
> included by some other file I didn't get an error from gcc?
> 
> > ...
> >   
> > >  	struct regmap *regmap;
> > >  	struct bme680_calib bme680;
> > >  	struct mutex lock; /* Protect multiple serial R/W ops to device. */
> > > +	struct device *dev;  
> > 
> > Is it the same that you may get wia regmap_get_device()?
> >   
> 
> Yes it is the same. Maybe I can try and see if I can use the following
> 
> 	regmap_get_device(data->regmap)
> 
> in the places where the pm functions are used in order to not declare a
> new value inside the struct bme680_data. But in general, is this approach
> prefered?

slightly by me.  I tend not to poke on that if people have chosen a local
variable, but it is a little neater.

This patch might get caught up in an effort to simplify the
autosuspend handling but if it is we'll deal with that whilst merging.
There 'should' be a clean path to transition from this style to the proposed
new one where a simple pm_runtime_put() without the mark_last_busy stuff
is enough for autosuspend cases.

Jonathan

Re: [PATCH v1 08/13] iio: chemical: bme680: add power management

[Andy Shevchenko]

Fri, Oct 11, 2024 at 09:02:32PM +0200, Vasileios Aoiridis kirjoitti:
> On Fri, Oct 11, 2024 at 01:10:20PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 11:00:25PM +0200, vamoirid wrote:

...

> > > +extern const struct dev_pm_ops bmp280_dev_pm_ops;
> > 
> > Is pm.h being included already in this header? Otherwise you need to add it.
> 
> No it is not, and indeed I need to add it. Probably because it was
> included by some other file I didn't get an error from gcc?

Yeah, it's called a "proxy" header in general meaning. We should try hard not
to use such headers (meaning not to use them in a "proxy" mode).

...

> > >  	struct regmap *regmap;
> > >  	struct bme680_calib bme680;
> > >  	struct mutex lock; /* Protect multiple serial R/W ops to device. */
> > > +	struct device *dev;
> > 
> > Is it the same that you may get wia regmap_get_device()?
> > 
> 
> Yes it is the same. Maybe I can try and see if I can use the following
> 
> 	regmap_get_device(data->regmap)
> 
> in the places where the pm functions are used in order to not declare a
> new value inside the struct bme680_data. But in general, is this approach
> prefered?

Since there is a getter already available, I prefer not to shortcut it via
adding a duplicating information to the data structure.

> > >  	u8 oversampling_temp;
> > >  	u8 oversampling_press;
> > >  	u8 oversampling_humid;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 09/13] iio: chemical: bme680: Move ambient temperature to attributes

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Remove the ambient temperature from being a macro and implement it as
an attribute. This way, it is possible to dynamically configure the
ambient temperature of the environment to improve the accuracy of the
measurements.

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

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index e9e3e08fa366..95d39c154d59 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -45,7 +45,6 @@
 #define BME680_REG_RES_HEAT_0			0x5A
 #define BME680_REG_GAS_WAIT_0			0x64
 #define BME680_ADC_GAS_RES			GENMASK(15, 6)
-#define BME680_AMB_TEMP				25
 
 #define BME680_REG_CTRL_GAS_1			0x71
 #define   BME680_RUN_GAS_MASK			BIT(4)
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 5fd5740bb7fe..0979c8f0afcf 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -120,6 +120,7 @@ struct bme680_data {
 	u16 heater_temp;
 
 	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
+	int ambient_temp;
 
 	union {
 		u8 buf[3];
@@ -483,7 +484,7 @@ static u8 bme680_calc_heater_res(struct bme680_data *data, u16 temp)
 	if (temp > 400) /* Cap temperature */
 		temp = 400;
 
-	var1 = (((s32)BME680_AMB_TEMP * calib->par_gh3) / 1000) * 256;
+	var1 = (((s32)data->ambient_temp * calib->par_gh3) / 1000) * 256;
 	var2 = (calib->par_gh1 + 784) * (((((calib->par_gh2 + 154009) *
 						temp * 5) / 100)
 						+ 3276800) / 10);
@@ -878,6 +879,37 @@ static int bme680_write_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static ssize_t ambient_temp_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bme680_data *data = iio_priv(indio_dev);
+	int vals[2];
+
+	vals[0] = data->ambient_temp;
+	vals[1] = 1;
+
+	return iio_format_value(buf, IIO_VAL_INT, 1, vals);
+}
+
+static ssize_t ambient_temp_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bme680_data *data = iio_priv(indio_dev);
+	int ret, val_int, val_fract;
+
+	ret = iio_str_to_fixpoint(buf, 1, &val_int, &val_fract);
+	if (ret)
+		return ret;
+
+	data->ambient_temp = val_int;
+	return len;
+}
+
+static IIO_DEVICE_ATTR_RW(ambient_temp, 0);
+
 static const char bme680_oversampling_ratio_show[] = "1 2 4 8 16";
 
 static IIO_CONST_ATTR(oversampling_ratio_available,
@@ -885,6 +917,7 @@ static IIO_CONST_ATTR(oversampling_ratio_available,
 
 static struct attribute *bme680_attributes[] = {
 	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
+	&iio_dev_attr_ambient_temp.dev_attr.attr,
 	NULL,
 };
 
-- 
2.43.0

Re: [PATCH v1 09/13] iio: chemical: bme680: Move ambient temperature to attributes

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:26PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Remove the ambient temperature from being a macro and implement it as
> an attribute. This way, it is possible to dynamically configure the
> ambient temperature of the environment to improve the accuracy of the
> measurements.

...

> -	var1 = (((s32)BME680_AMB_TEMP * calib->par_gh3) / 1000) * 256;
> +	var1 = (((s32)data->ambient_temp * calib->par_gh3) / 1000) * 256;

MILLI? KILO?

...

>  static struct attribute *bme680_attributes[] = {
>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
> +	&iio_dev_attr_ambient_temp.dev_attr.attr,
>  	NULL,

Side note: Perhaps a patch or an additional change in the existing one to drop
the trailing comma here.

>  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 09/13] iio: chemical: bme680: Move ambient temperature to attributes

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:12:21PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:26PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Remove the ambient temperature from being a macro and implement it as
> > an attribute. This way, it is possible to dynamically configure the
> > ambient temperature of the environment to improve the accuracy of the
> > measurements.
> 
> ...
> 
> > -	var1 = (((s32)BME680_AMB_TEMP * calib->par_gh3) / 1000) * 256;
> > +	var1 = (((s32)data->ambient_temp * calib->par_gh3) / 1000) * 256;
> 
> MILLI? KILO?
>

I could do it yes.

> ...
> 
> >  static struct attribute *bme680_attributes[] = {
> >  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
> > +	&iio_dev_attr_ambient_temp.dev_attr.attr,
> >  	NULL,
> 
> Side note: Perhaps a patch or an additional change in the existing one to drop
> the trailing comma here.
> 

I could add an additional change if you think it is worth the churn.

> >  };
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Cheers,
Vasilis

Re: [PATCH v1 09/13] iio: chemical: bme680: Move ambient temperature to attributes

[Jonathan Cameron]

On Thu, 10 Oct 2024 23:00:26 +0200
vamoirid <vassilisamir@gmail.com> wrote:

> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Remove the ambient temperature from being a macro and implement it as
> an attribute. This way, it is possible to dynamically configure the
> ambient temperature of the environment to improve the accuracy of the
> measurements.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
New ABI? Would need docs.

However, I 'think' we have a few cases where we handle this via the slightly
odd interface of out_temp_processed / _raw with a label saying it's
ambient temperature.

The tenuous argument is that we have heaters that actually control the
temperature and the affect of either heating the thing or just happening
to know the external temperature ends up being the same. Hence use
an output channel for this control.

Jonathan

Re: [PATCH v1 09/13] iio: chemical: bme680: Move ambient temperature to attributes

[Vasileios Amoiridis]

On Sat, Oct 12, 2024 at 01:01:24PM +0100, Jonathan Cameron wrote:
> On Thu, 10 Oct 2024 23:00:26 +0200
> vamoirid <vassilisamir@gmail.com> wrote:
> 
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Remove the ambient temperature from being a macro and implement it as
> > an attribute. This way, it is possible to dynamically configure the
> > ambient temperature of the environment to improve the accuracy of the
> > measurements.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> New ABI? Would need docs.
> 
> However, I 'think' we have a few cases where we handle this via the slightly
> odd interface of out_temp_processed / _raw with a label saying it's
> ambient temperature.
> 
> The tenuous argument is that we have heaters that actually control the
> temperature and the affect of either heating the thing or just happening
> to know the external temperature ends up being the same. Hence use
> an output channel for this control.
> 
> Jonathan

Hi Jonathan,

Thanks for taking the time to review this. I saw your previous messages,
and I am not responding to all of them so as to not flood you with ACK
messages.

For this one though I have to ask. The last commit of this series is
adding support for an output current channel that controls the current
that is being inserted into an internal plate that is heated up in order
to have more precise acquisition of humidity and gas measurement. Does
it makes sense to add an ambient temp output channel as well?

Cheers,
Vasilis

Re: [PATCH v1 09/13] iio: chemical: bme680: Move ambient temperature to attributes

[Jonathan Cameron]

On Mon, 14 Oct 2024 22:14:23 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Sat, Oct 12, 2024 at 01:01:24PM +0100, Jonathan Cameron wrote:
> > On Thu, 10 Oct 2024 23:00:26 +0200
> > vamoirid <vassilisamir@gmail.com> wrote:
> >   
> > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > 
> > > Remove the ambient temperature from being a macro and implement it as
> > > an attribute. This way, it is possible to dynamically configure the
> > > ambient temperature of the environment to improve the accuracy of the
> > > measurements.
> > > 
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > New ABI? Would need docs.
> > 
> > However, I 'think' we have a few cases where we handle this via the slightly
> > odd interface of out_temp_processed / _raw with a label saying it's
> > ambient temperature.
> > 
> > The tenuous argument is that we have heaters that actually control the
> > temperature and the affect of either heating the thing or just happening
> > to know the external temperature ends up being the same. Hence use
> > an output channel for this control.
> > 
> > Jonathan  
> 
> Hi Jonathan,
> 
> Thanks for taking the time to review this. I saw your previous messages,
> and I am not responding to all of them so as to not flood you with ACK
> messages.
> 
> For this one though I have to ask. The last commit of this series is
> adding support for an output current channel that controls the current
> that is being inserted into an internal plate that is heated up in order
> to have more precise acquisition of humidity and gas measurement. Does
> it makes sense to add an ambient temp output channel as well?

If we need to know that temperature to calculate the meaning of the pressure
channels then I think it does.

I am a little confused though as this device measures the temperature.
Why isn't that the right value to use?  Is that because the heater
is confusing things?


> 
> Cheers,
> Vasilis
> 

Re: [PATCH v1 09/13] iio: chemical: bme680: Move ambient temperature to attributes

[Vasileios Amoiridis]

On Sat, Oct 19, 2024 at 02:59:25PM +0100, Jonathan Cameron wrote:
> On Mon, 14 Oct 2024 22:14:23 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > On Sat, Oct 12, 2024 at 01:01:24PM +0100, Jonathan Cameron wrote:
> > > On Thu, 10 Oct 2024 23:00:26 +0200
> > > vamoirid <vassilisamir@gmail.com> wrote:
> > >   
> > > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > 
> > > > Remove the ambient temperature from being a macro and implement it as
> > > > an attribute. This way, it is possible to dynamically configure the
> > > > ambient temperature of the environment to improve the accuracy of the
> > > > measurements.
> > > > 
> > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > > New ABI? Would need docs.
> > > 
> > > However, I 'think' we have a few cases where we handle this via the slightly
> > > odd interface of out_temp_processed / _raw with a label saying it's
> > > ambient temperature.
> > > 
> > > The tenuous argument is that we have heaters that actually control the
> > > temperature and the affect of either heating the thing or just happening
> > > to know the external temperature ends up being the same. Hence use
> > > an output channel for this control.
> > > 
> > > Jonathan  
> > 
> > Hi Jonathan,
> > 
> > Thanks for taking the time to review this. I saw your previous messages,
> > and I am not responding to all of them so as to not flood you with ACK
> > messages.
> > 
> > For this one though I have to ask. The last commit of this series is
> > adding support for an output current channel that controls the current
> > that is being inserted into an internal plate that is heated up in order
> > to have more precise acquisition of humidity and gas measurement. Does
> > it makes sense to add an ambient temp output channel as well?
> 
> If we need to know that temperature to calculate the meaning of the pressure
> channels then I think it does.
> 
> I am a little confused though as this device measures the temperature.
> Why isn't that the right value to use?  Is that because the heater
> is confusing things?
> 
>

Hi Jonathan,

Thank you very much for your message! So, I digged a bit more into the
device datasheet and I found out that the ambient temperature can
actually be taken directly from the measured temperature (p.22 of [1]),
I can use this one. This means, that the ambient temp can be fully
dropped since the temperature of the sensor is the information we need.
Is it ok to drop it fully since it is an ABI change? If not how should I
approach this?

Another thing that I had missed because of the way the code was written
is that actually we can (and should) use output channel for setting the
temperature of the internal heater plate. This sensor essentially has an
internal heater plate that allows it to measure the VOC. Currently if
you check the driver [2], the value of the requested temperature of the
heater is set only once in the probe function and stays all the time
like this. This doesn't allow for much flexibility. But it is something
that I will do in another series and not this one, since this one is
already quite heavy.

Cheers,
Vasilis

PS: I don't understand why Bosch designed the sensor in this way. Since
the value of the ambient temp can be either hardcoded or the actual
value of the temperature sensor, they could have had all this logic in
hardware. They could actually even implement the compensation functions
in hardware and just return RAW values to us. It's kind of the same
situation with the BME280 and BMP{1,2,3,5}80 drivers that we have.

[1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bme680-ds001.pdf
[2]: https://elixir.bootlin.com/linux/v6.11.4/source/drivers/iio/chemical/bme680_core.c#L989
> > 
> > Cheers,
> > Vasilis
> > 
>

Re: [PATCH v1 09/13] iio: chemical: bme680: Move ambient temperature to attributes

[Vasileios Amoiridis]

On Sat, Oct 19, 2024 at 07:51:32PM +0200, Vasileios Amoiridis wrote:
> On Sat, Oct 19, 2024 at 02:59:25PM +0100, Jonathan Cameron wrote:
> > On Mon, 14 Oct 2024 22:14:23 +0200
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > 
> > > On Sat, Oct 12, 2024 at 01:01:24PM +0100, Jonathan Cameron wrote:
> > > > On Thu, 10 Oct 2024 23:00:26 +0200
> > > > vamoirid <vassilisamir@gmail.com> wrote:
> > > >   
> > > > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > > 
> > > > > Remove the ambient temperature from being a macro and implement it as
> > > > > an attribute. This way, it is possible to dynamically configure the
> > > > > ambient temperature of the environment to improve the accuracy of the
> > > > > measurements.
> > > > > 
> > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > > > New ABI? Would need docs.
> > > > 
> > > > However, I 'think' we have a few cases where we handle this via the slightly
> > > > odd interface of out_temp_processed / _raw with a label saying it's
> > > > ambient temperature.
> > > > 
> > > > The tenuous argument is that we have heaters that actually control the
> > > > temperature and the affect of either heating the thing or just happening
> > > > to know the external temperature ends up being the same. Hence use
> > > > an output channel for this control.
> > > > 
> > > > Jonathan  
> > > 
> > > Hi Jonathan,
> > > 
> > > Thanks for taking the time to review this. I saw your previous messages,
> > > and I am not responding to all of them so as to not flood you with ACK
> > > messages.
> > > 
> > > For this one though I have to ask. The last commit of this series is
> > > adding support for an output current channel that controls the current
> > > that is being inserted into an internal plate that is heated up in order
> > > to have more precise acquisition of humidity and gas measurement. Does
> > > it makes sense to add an ambient temp output channel as well?
> > 
> > If we need to know that temperature to calculate the meaning of the pressure
> > channels then I think it does.
> > 
> > I am a little confused though as this device measures the temperature.
> > Why isn't that the right value to use?  Is that because the heater
> > is confusing things?
> > 
> >
> 
> Hi Jonathan,
> 
> Thank you very much for your message! So, I digged a bit more into the
> device datasheet and I found out that the ambient temperature can
> actually be taken directly from the measured temperature (p.22 of [1]),
> I can use this one. This means, that the ambient temp can be fully
> dropped since the temperature of the sensor is the information we need.
> Is it ok to drop it fully since it is an ABI change? If not how should I
> approach this?
> 

Actually in the end, no ABI change because the temperature will be
taken from the internal measurement so nothing that has to do with
userspace so there is no ABI change here, my mistake!

Vasilis

> Another thing that I had missed because of the way the code was written
> is that actually we can (and should) use output channel for setting the
> temperature of the internal heater plate. This sensor essentially has an
> internal heater plate that allows it to measure the VOC. Currently if
> you check the driver [2], the value of the requested temperature of the
> heater is set only once in the probe function and stays all the time
> like this. This doesn't allow for much flexibility. But it is something
> that I will do in another series and not this one, since this one is
> already quite heavy.
> 
> Cheers,
> Vasilis
> 
> PS: I don't understand why Bosch designed the sensor in this way. Since
> the value of the ambient temp can be either hardcoded or the actual
> value of the temperature sensor, they could have had all this logic in
> hardware. They could actually even implement the compensation functions
> in hardware and just return RAW values to us. It's kind of the same
> situation with the BME280 and BMP{1,2,3,5}80 drivers that we have.
> 
> [1]: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bme680-ds001.pdf
> [2]: https://elixir.bootlin.com/linux/v6.11.4/source/drivers/iio/chemical/bme680_core.c#L989
> > > 
> > > Cheers,
> > > Vasilis
> > > 
> >

[PATCH v1 10/13] iio: chemical: bme680: generalize read_*() functions

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Remove the IIO specific scaling measurement units from the read functions
and add them inside the ->read_raw() function to keep the read_*() generic.
This way they can be used in other parts of the driver.

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 0979c8f0afcf..4669338ec2e5 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -661,22 +661,20 @@ static int bme680_gas_config(struct bme680_data *data)
 	return ret;
 }
 
-static int bme680_read_temp(struct bme680_data *data, int *val)
+static int bme680_read_temp(struct bme680_data *data, s16 *comp_temp)
 {
 	int ret;
 	u32 adc_temp;
-	s16 comp_temp;
 
 	ret = bme680_read_temp_adc(data, &adc_temp);
 	if (ret)
 		return ret;
 
-	comp_temp = bme680_compensate_temp(data, adc_temp);
-	*val = comp_temp * 10; /* Centidegrees to millidegrees */
-	return IIO_VAL_INT;
+	*comp_temp = bme680_compensate_temp(data, adc_temp);
+	return 0;
 }
 
-static int bme680_read_press(struct bme680_data *data, int *val, int *val2)
+static int bme680_read_press(struct bme680_data *data, u32 *comp_press)
 {
 	int ret;
 	u32 adc_press;
@@ -690,15 +688,14 @@ static int bme680_read_press(struct bme680_data *data, int *val, int *val2)
 	if (ret)
 		return ret;
 
-	*val = bme680_compensate_press(data, adc_press, t_fine);
-	*val2 = 1000;
-	return IIO_VAL_FRACTIONAL;
+	*comp_press = bme680_compensate_press(data, adc_press, t_fine);
+	return 0;
 }
 
-static int bme680_read_humid(struct bme680_data *data, int *val, int *val2)
+static int bme680_read_humid(struct bme680_data *data, u32 *comp_humidity)
 {
 	int ret;
-	u32 adc_humidity, comp_humidity;
+	u32 adc_humidity;
 	s32 t_fine;
 
 	ret = bme680_get_t_fine(data, &t_fine);
@@ -709,14 +706,11 @@ static int bme680_read_humid(struct bme680_data *data, int *val, int *val2)
 	if (ret)
 		return ret;
 
-	comp_humidity = bme680_compensate_humid(data, adc_humidity, t_fine);
-
-	*val = comp_humidity;
-	*val2 = 1000;
-	return IIO_VAL_FRACTIONAL;
+	*comp_humidity = bme680_compensate_humid(data, adc_humidity, t_fine);
+	return 0;
 }
 
-static int bme680_read_gas(struct bme680_data *data, int *val)
+static int bme680_read_gas(struct bme680_data *data, int *comp_gas_res)
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
@@ -751,9 +745,8 @@ static int bme680_read_gas(struct bme680_data *data, int *val)
 	}
 
 	gas_range = FIELD_GET(BME680_GAS_RANGE_MASK, gas_regs_val);
-
-	*val = bme680_compensate_gas(data, adc_gas_res, gas_range);
-	return IIO_VAL_INT;
+	*comp_gas_res = bme680_compensate_gas(data, adc_gas_res, gas_range);
+	return 0;
 }
 
 static int __bme680_read_raw(struct iio_dev *indio_dev,
@@ -761,7 +754,7 @@ 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;
+	int chan_val, ret;
 
 	guard(mutex)(&data->lock);
 
@@ -777,13 +770,35 @@ static int __bme680_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
 		case IIO_TEMP:
-			return bme680_read_temp(data, val);
+			ret = bme680_read_temp(data, (s16 *)&chan_val);
+			if (ret)
+				return ret;
+
+			*val = chan_val * 10;
+			return IIO_VAL_INT;
 		case IIO_PRESSURE:
-			return bme680_read_press(data, val, val2);
+			ret = bme680_read_press(data, &chan_val);
+			if (ret)
+				return ret;
+
+			*val = chan_val;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_HUMIDITYRELATIVE:
-			return bme680_read_humid(data, val, val2);
+			ret = bme680_read_humid(data, &chan_val);
+			if (ret)
+				return ret;
+
+			*val = chan_val;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_RESISTANCE:
-			return bme680_read_gas(data, val);
+			ret = bme680_read_gas(data, &chan_val);
+			if (ret)
+				return ret;
+
+			*val = chan_val;
+			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
 		}
-- 
2.43.0

[PATCH v1 11/13] iio: chemical: bme680: Add SCALE and RAW channels

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Add SCALE,RAW channels to the device. Even though PROCESSED should be
kept for backwards compatibility add comment to avoid using it if the
value is not actually reported in IIO values.

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 4669338ec2e5..7d6f4d8c5fe4 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -155,17 +155,26 @@ EXPORT_SYMBOL_NS(bme680_regmap_config, IIO_BME680);
 static const struct iio_chan_spec bme680_channels[] = {
 	{
 		.type = IIO_TEMP,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 	{
 		.type = IIO_PRESSURE,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 	{
 		.type = IIO_HUMIDITYRELATIVE,
+		/* PROCESSED maintained for ABI backwards compatibility */
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 	{
@@ -802,6 +811,48 @@ static int __bme680_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_TEMP:
+			ret = bme680_read_temp(data, (s16 *)&chan_val);
+			if (ret)
+				return ret;
+
+			*val = chan_val;
+			return IIO_VAL_INT;
+		case IIO_PRESSURE:
+			ret = bme680_read_press(data, &chan_val);
+			if (ret)
+				return ret;
+
+			*val = chan_val;
+			return IIO_VAL_INT;
+		case IIO_HUMIDITYRELATIVE:
+			ret = bme680_read_humid(data, &chan_val);
+			if (ret)
+				return ret;
+
+			*val = chan_val;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_TEMP:
+			*val = 10;
+			return IIO_VAL_INT;
+		case IIO_PRESSURE:
+			*val = 1;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_HUMIDITYRELATIVE:
+			*val = 1;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (chan->type) {
 		case IIO_TEMP:
-- 
2.43.0

[PATCH v1 12/13] iio: chemical: bme680: Add triggered buffer support

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Add triggered buffer and soft timestamp support. The available scan mask
enables all the channels of the sensor in order to follow the operation of
the sensor. The sensor basically starts to capture from all channels
as long as it enters into FORCED mode.

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

diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 678a6adb9a75..447b205f57bd 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -50,6 +50,8 @@ config BME680
 	select REGMAP
 	select BME680_I2C if I2C
 	select BME680_SPI if SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Bosch Sensortec BME680 sensor with
 	  temperature, pressure, humidity and gas sensing capability.
diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index 95d39c154d59..e7eed2962baa 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -64,6 +64,8 @@
 
 #define BME680_STARTUP_TIME_US			2000
 
+#define BME680_NUM_CHANNELS			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 7d6f4d8c5fe4..df6ae4355902 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -18,8 +18,11 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #include <asm/unaligned.h>
 
@@ -108,6 +111,13 @@ static const char *const bme680_supply_names[] = {
 
 #define BME680_NUM_SUPPLIES ARRAY_SIZE(bme680_supply_names)
 
+enum bme680_scan {
+	BME680_TEMP,
+	BME680_PRESS,
+	BME680_HUMID,
+	BME680_GAS,
+};
+
 struct bme680_data {
 	struct regmap *regmap;
 	struct bme680_calib bme680;
@@ -122,8 +132,11 @@ struct bme680_data {
 	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
 	int ambient_temp;
 
+	u8 buffer[ALIGN(sizeof(s32) * BME680_NUM_CHANNELS, sizeof(s64))
+		  + sizeof(s64)] __aligned(sizeof(s64));
+
 	union {
-		u8 buf[3];
+		u8 buf[15];
 		unsigned int check;
 		__be16 be16;
 		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
@@ -160,6 +173,13 @@ static const struct iio_chan_spec bme680_channels[] = {
 				      BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
 		.type = IIO_PRESSURE,
@@ -168,6 +188,13 @@ static const struct iio_chan_spec bme680_channels[] = {
 				      BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
 		.type = IIO_HUMIDITYRELATIVE,
@@ -176,11 +203,26 @@ static const struct iio_chan_spec bme680_channels[] = {
 				      BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
 		.type = IIO_RESISTANCE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.scan_index = 3,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
 static int bme680_read_calib(struct bme680_data *data,
@@ -997,6 +1039,121 @@ static const struct iio_info bme680_info = {
 	.attrs = &bme680_attribute_group,
 };
 
+static const unsigned long bme680_avail_scan_masks[] = {
+	BIT(BME680_GAS) | BIT(BME680_HUMID) | BIT(BME680_PRESS) | BIT(BME680_TEMP),
+	0
+};
+
+static irqreturn_t bme680_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct bme680_data *data = iio_priv(indio_dev);
+	u32 adc_temp, adc_press, adc_humid, comp_press, comp_humid;
+	s32 *chans = (s32 *)data->buffer;
+	u16 adc_gas_res, gas_regs_val;
+	s32 t_fine, comp_gas_res;
+	s16 comp_temp;
+	u8 gas_range;
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	ret = bme680_set_mode(data, BME680_FORCED);
+	if (ret < 0)
+		goto out;
+
+	ret = bme680_wait_for_eoc(data);
+	if (ret)
+		goto out;
+
+	/* Burst read data regs */
+	ret = regmap_bulk_read(data->regmap, BME680_REG_MEAS_STAT_0,
+			       data->buf, sizeof(data->buf));
+	if (ret) {
+		dev_err(data->dev, "failed to burst read sensor data\n");
+		goto out;
+	}
+	if (data->buf[0] & BME680_GAS_MEAS_BIT) {
+		dev_err(data->dev, "gas measurement incomplete\n");
+		goto out;
+	}
+
+	/* Temperature calculations */
+	adc_temp = FIELD_GET(BME680_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[5]));
+	if (adc_temp == BME680_MEAS_SKIPPED) {
+		dev_err(data->dev, "reading temperature skipped\n");
+		goto out;
+	}
+	comp_temp = bme680_compensate_temp(data, adc_temp);
+	t_fine = bme680_calc_t_fine(data, adc_temp);
+
+	/* Pressure calculations */
+	adc_press = FIELD_GET(BME680_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[2]));
+	if (adc_press == BME680_MEAS_SKIPPED) {
+		dev_err(data->dev, "reading pressure skipped\n");
+		goto out;
+	}
+	comp_press = bme680_compensate_press(data, adc_press, t_fine);
+	pr_info("comp_press: %d\n", comp_press);
+
+	/* Humidity calculations */
+	adc_humid = get_unaligned_be16(&data->buf[8]);
+	if (adc_humid == BME680_MEAS_SKIPPED) {
+		dev_err(data->dev, "reading humidity skipped\n");
+		goto out;
+	}
+	comp_humid = bme680_compensate_humid(data, adc_humid, t_fine);
+	pr_info("comp_humid: %d\n", comp_humid);
+
+	/* Gas calculations */
+	gas_regs_val = get_unaligned_be16(&data->buf[13]);
+	adc_gas_res = FIELD_GET(BME680_ADC_GAS_RES, gas_regs_val);
+	if ((gas_regs_val & BME680_GAS_STAB_BIT) == 0) {
+		dev_err(data->dev, "heater failed to reach the target temperature\n");
+		goto out;
+	}
+	gas_range = FIELD_GET(BME680_GAS_RANGE_MASK, gas_regs_val);
+	comp_gas_res = bme680_compensate_gas(data, adc_gas_res, gas_range);
+	pr_info("comp_gas_res: %d\n", comp_gas_res);
+
+	chans[0] = comp_temp;
+	chans[1] = comp_press;
+	chans[2] = comp_humid;
+	chans[3] = comp_gas_res;
+
+	/* Push to buffer */
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+					   iio_get_time_ns(indio_dev));
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static int bme680_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct bme680_data *data = iio_priv(indio_dev);
+
+	pm_runtime_get_sync(data->dev);
+
+	return 0;
+}
+
+static int bme680_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct bme680_data *data = iio_priv(indio_dev);
+
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops bme680_buffer_setup_ops = {
+	.preenable = bme680_buffer_preenable,
+	.postdisable = bme680_buffer_postdisable,
+};
+
 static void bme680_regulators_disable(void *data)
 {
 	struct regulator_bulk_data *supplies = data;
@@ -1032,6 +1189,7 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->name = name;
 	indio_dev->channels = bme680_channels;
 	indio_dev->num_channels = ARRAY_SIZE(bme680_channels);
+	indio_dev->available_scan_masks = bme680_avail_scan_masks;
 	indio_dev->info = &bme680_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
@@ -1088,6 +1246,14 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		return dev_err_probe(dev, ret,
 				     "failed to set gas config data\n");
 
+	ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      bme680_trigger_handler,
+					      &bme680_buffer_setup_ops);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio triggered buffer setup failed\n");
+
 	/* Enable runtime PM */
 	pm_runtime_get_noresume(dev);
 	pm_runtime_set_active(dev);
-- 
2.43.0

Re: [PATCH v1 12/13] iio: chemical: bme680: Add triggered buffer support

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:29PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Add triggered buffer and soft timestamp support. The available scan mask
> enables all the channels of the sensor in order to follow the operation of
> the sensor. The sensor basically starts to capture from all channels
> as long as it enters into FORCED mode.

...

>  	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
>  	int ambient_temp;
>  
> +	u8 buffer[ALIGN(sizeof(s32) * BME680_NUM_CHANNELS, sizeof(s64))
> +		  + sizeof(s64)] __aligned(sizeof(s64));

Can it be represented as a structure?
We also have aligned_s64 for the timestamp.

>  	union {
> -		u8 buf[3];
> +		u8 buf[15];
>  		unsigned int check;
>  		__be16 be16;
>  		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];

...

> +static irqreturn_t bme680_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bme680_data *data = iio_priv(indio_dev);
> +	u32 adc_temp, adc_press, adc_humid, comp_press, comp_humid;

> +	s32 *chans = (s32 *)data->buffer;

With the structure in place this becomes much more readable.

> +	u16 adc_gas_res, gas_regs_val;
> +	s32 t_fine, comp_gas_res;
> +	s16 comp_temp;
> +	u8 gas_range;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = bme680_set_mode(data, BME680_FORCED);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = bme680_wait_for_eoc(data);
> +	if (ret)
> +		goto out;
> +
> +	/* Burst read data regs */
> +	ret = regmap_bulk_read(data->regmap, BME680_REG_MEAS_STAT_0,
> +			       data->buf, sizeof(data->buf));
> +	if (ret) {
> +		dev_err(data->dev, "failed to burst read sensor data\n");
> +		goto out;
> +	}
> +	if (data->buf[0] & BME680_GAS_MEAS_BIT) {
> +		dev_err(data->dev, "gas measurement incomplete\n");
> +		goto out;
> +	}
> +
> +	/* Temperature calculations */
> +	adc_temp = FIELD_GET(BME680_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[5]));
> +	if (adc_temp == BME680_MEAS_SKIPPED) {
> +		dev_err(data->dev, "reading temperature skipped\n");
> +		goto out;
> +	}
> +	comp_temp = bme680_compensate_temp(data, adc_temp);
> +	t_fine = bme680_calc_t_fine(data, adc_temp);
> +
> +	/* Pressure calculations */
> +	adc_press = FIELD_GET(BME680_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[2]));
> +	if (adc_press == BME680_MEAS_SKIPPED) {
> +		dev_err(data->dev, "reading pressure skipped\n");
> +		goto out;
> +	}
> +	comp_press = bme680_compensate_press(data, adc_press, t_fine);

> +	pr_info("comp_press: %d\n", comp_press);

No debugging in the production code. Or if you really need that, it should
use dev_dbg().

> +	/* Humidity calculations */
> +	adc_humid = get_unaligned_be16(&data->buf[8]);
> +	if (adc_humid == BME680_MEAS_SKIPPED) {
> +		dev_err(data->dev, "reading humidity skipped\n");
> +		goto out;
> +	}
> +	comp_humid = bme680_compensate_humid(data, adc_humid, t_fine);

> +	pr_info("comp_humid: %d\n", comp_humid);

Ditto.

> +
> +	/* Gas calculations */
> +	gas_regs_val = get_unaligned_be16(&data->buf[13]);
> +	adc_gas_res = FIELD_GET(BME680_ADC_GAS_RES, gas_regs_val);
> +	if ((gas_regs_val & BME680_GAS_STAB_BIT) == 0) {
> +		dev_err(data->dev, "heater failed to reach the target temperature\n");
> +		goto out;
> +	}
> +	gas_range = FIELD_GET(BME680_GAS_RANGE_MASK, gas_regs_val);
> +	comp_gas_res = bme680_compensate_gas(data, adc_gas_res, gas_range);
> +	pr_info("comp_gas_res: %d\n", comp_gas_res);
> +
> +	chans[0] = comp_temp;
> +	chans[1] = comp_press;
> +	chans[2] = comp_humid;
> +	chans[3] = comp_gas_res;
> +
> +	/* Push to buffer */
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> +					   iio_get_time_ns(indio_dev));
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 12/13] iio: chemical: bme680: Add triggered buffer support

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:37:56PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:29PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Add triggered buffer and soft timestamp support. The available scan mask
> > enables all the channels of the sensor in order to follow the operation of
> > the sensor. The sensor basically starts to capture from all channels
> > as long as it enters into FORCED mode.
> 
> ...
> 
> >  	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
> >  	int ambient_temp;
> >  
> > +	u8 buffer[ALIGN(sizeof(s32) * BME680_NUM_CHANNELS, sizeof(s64))
> > +		  + sizeof(s64)] __aligned(sizeof(s64));
> 
> Can it be represented as a structure?
> We also have aligned_s64 for the timestamp.
>

Hi Andy,

The same approach was used also for the bmp280 driver and since I was
working on the bmp280 as well, I did it here. You think the
representation as a struct would look better? Personally I like the
nature of this one because of the ALIGN() but I have no problem of using
a struct here.

> >  	union {
> > -		u8 buf[3];
> > +		u8 buf[15];
> >  		unsigned int check;
> >  		__be16 be16;
> >  		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
> 
> ...
> 
> > +static irqreturn_t bme680_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct bme680_data *data = iio_priv(indio_dev);
> > +	u32 adc_temp, adc_press, adc_humid, comp_press, comp_humid;
> 
> > +	s32 *chans = (s32 *)data->buffer;
> 
> With the structure in place this becomes much more readable.
> 
> > +	u16 adc_gas_res, gas_regs_val;
> > +	s32 t_fine, comp_gas_res;
> > +	s16 comp_temp;
> > +	u8 gas_range;
> > +	int ret;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	ret = bme680_set_mode(data, BME680_FORCED);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = bme680_wait_for_eoc(data);
> > +	if (ret)
> > +		goto out;
> > +
> > +	/* Burst read data regs */
> > +	ret = regmap_bulk_read(data->regmap, BME680_REG_MEAS_STAT_0,
> > +			       data->buf, sizeof(data->buf));
> > +	if (ret) {
> > +		dev_err(data->dev, "failed to burst read sensor data\n");
> > +		goto out;
> > +	}
> > +	if (data->buf[0] & BME680_GAS_MEAS_BIT) {
> > +		dev_err(data->dev, "gas measurement incomplete\n");
> > +		goto out;
> > +	}
> > +
> > +	/* Temperature calculations */
> > +	adc_temp = FIELD_GET(BME680_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[5]));
> > +	if (adc_temp == BME680_MEAS_SKIPPED) {
> > +		dev_err(data->dev, "reading temperature skipped\n");
> > +		goto out;
> > +	}
> > +	comp_temp = bme680_compensate_temp(data, adc_temp);
> > +	t_fine = bme680_calc_t_fine(data, adc_temp);
> > +
> > +	/* Pressure calculations */
> > +	adc_press = FIELD_GET(BME680_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[2]));
> > +	if (adc_press == BME680_MEAS_SKIPPED) {
> > +		dev_err(data->dev, "reading pressure skipped\n");
> > +		goto out;
> > +	}
> > +	comp_press = bme680_compensate_press(data, adc_press, t_fine);
> 
> > +	pr_info("comp_press: %d\n", comp_press);
> 
> No debugging in the production code. Or if you really need that, it should
> use dev_dbg().
> 

ACK. I shouldn't have forgotten those here..

> > +	/* Humidity calculations */
> > +	adc_humid = get_unaligned_be16(&data->buf[8]);
> > +	if (adc_humid == BME680_MEAS_SKIPPED) {
> > +		dev_err(data->dev, "reading humidity skipped\n");
> > +		goto out;
> > +	}
> > +	comp_humid = bme680_compensate_humid(data, adc_humid, t_fine);
> 
> > +	pr_info("comp_humid: %d\n", comp_humid);
> 
> Ditto.
> 

ACK.

> > +
> > +	/* Gas calculations */
> > +	gas_regs_val = get_unaligned_be16(&data->buf[13]);
> > +	adc_gas_res = FIELD_GET(BME680_ADC_GAS_RES, gas_regs_val);
> > +	if ((gas_regs_val & BME680_GAS_STAB_BIT) == 0) {
> > +		dev_err(data->dev, "heater failed to reach the target temperature\n");
> > +		goto out;
> > +	}
> > +	gas_range = FIELD_GET(BME680_GAS_RANGE_MASK, gas_regs_val);
> > +	comp_gas_res = bme680_compensate_gas(data, adc_gas_res, gas_range);
> > +	pr_info("comp_gas_res: %d\n", comp_gas_res);
> > +
> > +	chans[0] = comp_temp;
> > +	chans[1] = comp_press;
> > +	chans[2] = comp_humid;
> > +	chans[3] = comp_gas_res;
> > +
> > +	/* Push to buffer */
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> > +					   iio_get_time_ns(indio_dev));
> > +out:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +	return IRQ_HANDLED;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Cheers,
Vasilis

Re: [PATCH v1 12/13] iio: chemical: bme680: Add triggered buffer support

[Jonathan Cameron]

On Fri, 11 Oct 2024 21:07:20 +0200
Vasileios Aoiridis <vassilisamir@gmail.com> wrote:

> On Fri, Oct 11, 2024 at 01:37:56PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 11:00:29PM +0200, vamoirid wrote:  
> > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > 
> > > Add triggered buffer and soft timestamp support. The available scan mask
> > > enables all the channels of the sensor in order to follow the operation of
> > > the sensor. The sensor basically starts to capture from all channels
> > > as long as it enters into FORCED mode.  
> > 
> > ...
> >   
> > >  	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
> > >  	int ambient_temp;
> > >  
> > > +	u8 buffer[ALIGN(sizeof(s32) * BME680_NUM_CHANNELS, sizeof(s64))
> > > +		  + sizeof(s64)] __aligned(sizeof(s64));  
> > 
> > Can it be represented as a structure?
> > We also have aligned_s64 for the timestamp.
> >  
> 
> Hi Andy,
> 
> The same approach was used also for the bmp280 driver and since I was
> working on the bmp280 as well, I did it here. You think the
> representation as a struct would look better? Personally I like the
> nature of this one because of the ALIGN() but I have no problem of using
> a struct here.

Depends if you can enable sufficiently few channels that the timestamp
moves.  If that is the case, a structure is missleading as a representation
of this buffer so I prefer the above fun as it doesn't give the wrong
impression (by giving no impression at all of the data layout!)

Jonathan

Re: [PATCH v1 12/13] iio: chemical: bme680: Add triggered buffer support

[Jonathan Cameron]

On Sat, 12 Oct 2024 13:04:02 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 11 Oct 2024 21:07:20 +0200
> Vasileios Aoiridis <vassilisamir@gmail.com> wrote:
> 
> > On Fri, Oct 11, 2024 at 01:37:56PM +0300, Andy Shevchenko wrote:  
> > > On Thu, Oct 10, 2024 at 11:00:29PM +0200, vamoirid wrote:    
> > > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > 
> > > > Add triggered buffer and soft timestamp support. The available scan mask
> > > > enables all the channels of the sensor in order to follow the operation of
> > > > the sensor. The sensor basically starts to capture from all channels
> > > > as long as it enters into FORCED mode.    
> > > 
> > > ...
> > >     
> > > >  	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
> > > >  	int ambient_temp;
> > > >  
> > > > +	u8 buffer[ALIGN(sizeof(s32) * BME680_NUM_CHANNELS, sizeof(s64))
> > > > +		  + sizeof(s64)] __aligned(sizeof(s64));    
> > > 
> > > Can it be represented as a structure?
> > > We also have aligned_s64 for the timestamp.
> > >    
> > 
> > Hi Andy,
> > 
> > The same approach was used also for the bmp280 driver and since I was
> > working on the bmp280 as well, I did it here. You think the
> > representation as a struct would look better? Personally I like the
> > nature of this one because of the ALIGN() but I have no problem of using
> > a struct here.  
> 
> Depends if you can enable sufficiently few channels that the timestamp
> moves.  If that is the case, a structure is missleading as a representation
> of this buffer so I prefer the above fun as it doesn't give the wrong
> impression (by giving no impression at all of the data layout!)
Looks like you always write them all to the buffer. So structure indeed
appropriate here. (I should have read the code before commenting!)

J
> 
> Jonathan

[PATCH v1 13/13] iio: chemical: bme680: Add support for preheat current

[vamoirid]

From: Vasileios Amoiridis <vassilisamir@gmail.com>

Add functionality to inject a specified amount of current to the heating
plate before the start of the gas measurement to allow the sensor to reach
faster to the requested temperature.

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

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index e7eed2962baa..c658cb631b52 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -42,6 +42,7 @@
 #define   BME680_RHRANGE_MASK			GENMASK(5, 4)
 #define BME680_REG_RES_HEAT_VAL			0x00
 #define   BME680_RSERROR_MASK			GENMASK(7, 4)
+#define BME680_REG_IDAC_HEAT_0			0x50
 #define BME680_REG_RES_HEAT_0			0x5A
 #define BME680_REG_GAS_WAIT_0			0x64
 #define BME680_ADC_GAS_RES			GENMASK(15, 6)
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index df6ae4355902..6cdf8b9dbf2b 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -126,6 +126,7 @@ struct bme680_data {
 	u8 oversampling_temp;
 	u8 oversampling_press;
 	u8 oversampling_humid;
+	u8 preheat_curr;
 	u16 heater_dur;
 	u16 heater_temp;
 
@@ -223,6 +224,12 @@ static const struct iio_chan_spec bme680_channels[] = {
 		},
 	},
 	IIO_CHAN_SOFT_TIMESTAMP(4),
+	{
+		.type = IIO_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.output = 1,
+		.scan_index = 5,
+	},
 };
 
 static int bme680_read_calib(struct bme680_data *data,
@@ -569,6 +576,12 @@ static u8 bme680_calc_heater_dur(u16 dur)
 	return durval;
 }
 
+/* Taken from datasheet, section 5.3.3 */
+static u8 bme680_calc_heater_preheat_current(u8 curr)
+{
+	return 8 * curr - 1;
+}
+
 static int bme680_set_mode(struct bme680_data *data, enum bme680_op_mode mode)
 {
 	struct device *dev = regmap_get_device(data->regmap);
@@ -673,6 +686,19 @@ static int bme680_chip_config(struct bme680_data *data)
 	return 0;
 }
 
+static int bme680_preheat_curr_config(struct bme680_data *data, u8 val)
+{
+	u8 heatr_curr;
+	int ret;
+
+	heatr_curr = bme680_calc_heater_preheat_current(val);
+	ret = regmap_write(data->regmap, BME680_REG_IDAC_HEAT_0, heatr_curr);
+	if (ret < 0)
+		dev_err(data->dev, "failed to write idac_heat_0 register\n");
+
+	return ret;
+}
+
 static int bme680_gas_config(struct bme680_data *data)
 {
 	struct device *dev = regmap_get_device(data->regmap);
@@ -701,6 +727,10 @@ static int bme680_gas_config(struct bme680_data *data)
 		return ret;
 	}
 
+	ret = bme680_preheat_curr_config(data, data->preheat_curr);
+	if (ret)
+		return ret;
+
 	/* Enable the gas sensor and select heater profile set-point 0 */
 	ret = regmap_update_bits(data->regmap, BME680_REG_CTRL_GAS_1,
 				 BME680_RUN_GAS_MASK | BME680_NB_CONV_MASK,
@@ -967,6 +997,13 @@ static int __bme680_write_raw(struct iio_dev *indio_dev,
 
 		return bme680_chip_config(data);
 	}
+	case IIO_CHAN_INFO_RAW:
+	{
+		if (chan->type != IIO_CURRENT)
+			return -EINVAL;
+
+		return bme680_preheat_curr_config(data, (u8)val);
+	}
 	default:
 		return -EINVAL;
 	}
@@ -1199,6 +1236,7 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	data->oversampling_temp = 8;  /* 8X oversampling rate */
 	data->heater_temp = 320; /* degree Celsius */
 	data->heater_dur = 150;  /* milliseconds */
+	data->preheat_curr = 0; /* milliamps */
 
 	regulator_bulk_set_supply_names(data->supplies, bme680_supply_names,
 					BME680_NUM_SUPPLIES);
-- 
2.43.0

Re: [PATCH v1 13/13] iio: chemical: bme680: Add support for preheat current

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:30PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Add functionality to inject a specified amount of current to the heating
> plate before the start of the gas measurement to allow the sensor to reach
> faster to the requested temperature.

...

> +	data->preheat_curr = 0; /* milliamps */

Instead of the comment, make it better to any appearance of the variable in the
code by adding unit suffix.

	data->preheat_curr_mA = 0;

(yes, capital 'A').

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 13/13] iio: chemical: bme680: Add support for preheat current

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:39:29PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:30PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Add functionality to inject a specified amount of current to the heating
> > plate before the start of the gas measurement to allow the sensor to reach
> > faster to the requested temperature.
> 
> ...
> 
> > +	data->preheat_curr = 0; /* milliamps */
> 
> Instead of the comment, make it better to any appearance of the variable in the
> code by adding unit suffix.
> 
> 	data->preheat_curr_mA = 0;
> 
> (yes, capital 'A').
> 

Hi Andy,

I used the comment here because it is exactly like this above and I
though it is more consistent if I do the same. But I guess repeating a
not so good design choice for consistency might not be the best
decision.

Cheers,
Vasilis
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v1 13/13] iio: chemical: bme680: Add support for preheat current

[Andy Shevchenko]

Fri, Oct 11, 2024 at 09:08:51PM +0200, Vasileios Aoiridis kirjoitti:
> On Fri, Oct 11, 2024 at 01:39:29PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 11:00:30PM +0200, vamoirid wrote:

...

> > > +	data->preheat_curr = 0; /* milliamps */
> > 
> > Instead of the comment, make it better to any appearance of the variable in the
> > code by adding unit suffix.
> > 
> > 	data->preheat_curr_mA = 0;
> > 
> > (yes, capital 'A').
> 
> I used the comment here because it is exactly like this above and I
> though it is more consistent if I do the same. But I guess repeating a
> not so good design choice for consistency might not be the best
> decision.

Right. I would rather see an additional patch that adds unit suffixes here
and there...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 13/13] iio: chemical: bme680: Add support for preheat current

[Jonathan Cameron]

On Thu, 10 Oct 2024 23:00:30 +0200
vamoirid <vassilisamir@gmail.com> wrote:

> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Add functionality to inject a specified amount of current to the heating
> plate before the start of the gas measurement to allow the sensor to reach
> faster to the requested temperature.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/chemical/bme680.h      |  1 +
>  drivers/iio/chemical/bme680_core.c | 38 ++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
> index e7eed2962baa..c658cb631b52 100644
> --- a/drivers/iio/chemical/bme680.h
> +++ b/drivers/iio/chemical/bme680.h
> @@ -42,6 +42,7 @@
>  #define   BME680_RHRANGE_MASK			GENMASK(5, 4)
>  #define BME680_REG_RES_HEAT_VAL			0x00
>  #define   BME680_RSERROR_MASK			GENMASK(7, 4)
> +#define BME680_REG_IDAC_HEAT_0			0x50
>  #define BME680_REG_RES_HEAT_0			0x5A
>  #define BME680_REG_GAS_WAIT_0			0x64
>  #define BME680_ADC_GAS_RES			GENMASK(15, 6)
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index df6ae4355902..6cdf8b9dbf2b 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -126,6 +126,7 @@ struct bme680_data {
>  	u8 oversampling_temp;
>  	u8 oversampling_press;
>  	u8 oversampling_humid;
> +	u8 preheat_curr;
>  	u16 heater_dur;
>  	u16 heater_temp;
>  
> @@ -223,6 +224,12 @@ static const struct iio_chan_spec bme680_channels[] = {
>  		},
>  	},
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
> +	{
> +		.type = IIO_CURRENT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.output = 1,
> +		.scan_index = 5,

Set scan index to -1
you don't want to create the buffer related attrs for this one

> +	},

Re: [PATCH v1 00/13]: chemical: bme680: 2nd set of clean and add

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:17PM +0200, vamoirid wrote:
> This patch series is continuing the work that started on [1] by
> improving some small issues of the driver in the commits 1,2,3.
> 
> Commits 4,5 are refactorizing existing code.
> 
> Commits 6,7,8 are adding DT, regulator and PM support.
> 
> Commit 9 is refactorizing one macro to attribute.
> 
> Commit 10,11,12 are refactorizing the read/compensate functions
> to become generic and add triggered buffer support.
> 
> Finally, commit 13 adds support for an *output* channel of type
> IIO_CURRENT in order to preheat the plate that is used to measure the
> quality of the air.
> 
> This and the previous series [1] started with the idea to add support
> for the new bme688 device but due to the structure of the driver I
> decided that it is better to restructure and improve some things before
> adding extra funcitonalities.

Besides the small comments here and there I think you need to rearrange the
patches layout in the series.

In general it should go in these blocks:
1) bug fixes (if any);
2) cleanups (note, whitespace-cleanup-like are the last);
3) new feature.

Note that PM runtime belongs to the last group quite close to the patches
where you more or less start using it more.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/13]: chemical: bme680: 2nd set of clean and add

[Vasileios Aoiridis]

On Fri, Oct 11, 2024 at 01:42:04PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:17PM +0200, vamoirid wrote:
> > This patch series is continuing the work that started on [1] by
> > improving some small issues of the driver in the commits 1,2,3.
> > 
> > Commits 4,5 are refactorizing existing code.
> > 
> > Commits 6,7,8 are adding DT, regulator and PM support.
> > 
> > Commit 9 is refactorizing one macro to attribute.
> > 
> > Commit 10,11,12 are refactorizing the read/compensate functions
> > to become generic and add triggered buffer support.
> > 
> > Finally, commit 13 adds support for an *output* channel of type
> > IIO_CURRENT in order to preheat the plate that is used to measure the
> > quality of the air.
> > 
> > This and the previous series [1] started with the idea to add support
> > for the new bme688 device but due to the structure of the driver I
> > decided that it is better to restructure and improve some things before
> > adding extra funcitonalities.
> 
> Besides the small comments here and there I think you need to rearrange the
> patches layout in the series.
> 
> In general it should go in these blocks:
> 1) bug fixes (if any);
> 2) cleanups (note, whitespace-cleanup-like are the last);
> 3) new feature.
> 
> Note that PM runtime belongs to the last group quite close to the patches
> where you more or less start using it more.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Hello Andy,

Thank you very much for taking the time to review this, I appreciate it
a lot. I was not sure about the order of the patches but what you
explained here makes total sense. I will rework the order and address
the rest of the comments.

Cheers,
Vasilis