[PATCH v5 0/3] iio: chemical bme680: 2nd round of cleanup

[PATCH v5 0/3] iio: chemical bme680: 2nd round of cleanup

[Vasileios Amoiridis]

Changes in v5:

[PATCH v5 1/3]:
	- Changed name of sensor to "co2-sensor" as it is already used
	  by other chemical sensors.
	- Added reviewer tag.

[PATCH v5 2/3]:
	- Removed unnecessary macro
	- Added reviewer tag

[PATCH v5 3/3]:
	- Replaced pm_runtime_get_sync() with
	  pm_runtime_resume_and_get()

---
v4: https://lore.kernel.org/linux-iio/20241128193246.24572-1-vassilisamir@gmail.com/
Changes in v4:

[PATCH v4 1/3]:
	- Changed description to include the move out of trivial
	  devices.

---
v3: https://lore.kernel.org/linux-iio/20241102131311.36210-1-vassilisamir@gmail.com/
Changes in v3:

Removed applied patches 1,2,3,5,6,7,8

[PATCH v3 1/7]:
	- v2 4/13
	- Set mode of sensor with enum variable and remove macros

[PATCH v3 5/7]:
	- v2 11/13
	- removed regulators from being required, adjusted commit
	  message

[PATCH v3 7/7]:
	- v2 13/13
	- removed unecessary usage of runtime PM functions

---
v2: https://lore.kernel.org/linux-iio/20241021195316.58911-1-vassilisamir@gmail.com/
Changes in v2:

Generally, the patches were rearranged according to comments from Andy
in previous version in order to be more consistent. The refactoring of
the ambient temperature was dropped for now because it was a bit more
complicated than I thought and this series is already heavy enough.

[PATCH v2 01/13]:
	- New patch

[PATCH v2 02/13]:
	- v1 1/13
	- used "optimized" in commit message to not prompt for a fix.
	- added documentation of where this sleep comes from

[PATCH v2 03/13]:
	- v1 2/13
	- Fix indentation of array and removed extra whitespace.

[PATCH v2 04/13]:
	- v1 5/13
	- removed extra check inside the set_mode() function.

[PATCH v2 06/13]:
	- v1 1/13
	- removed indentation fixes which are fixed later since code is
	  changed in those lines in later commits.

[PATCH v2 09/13]:
	- v1 12/13
	- removed unnecessary debug messages
	- Used struture instead of buffer to push data to userspace

[PATCH v2 10/13]:
	- v1 13/13
	- used better naming
	- made channel index to -1

[PATCH v2 11/13]:
	- v1 06/13
	- removed device from trivial-devices

[PATCH v2 12/13]:
	- v1 07/13
	- use devm_regulator_bulk_get_enable()

[PATCH v2 13/13]:
	- v1 08/13
	- removed internal usage of dev structure
	- added missing header in both bme680_core.c and bme680.h
	- used devm_pm_runtime_enable

---
v1: https://lore.kernel.org/linux-iio/20241010210030.33309-1-vassilisamir@gmail.com

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 (3):

Vasileios Amoiridis (3):
  dt-bindings: iio: bosch,bme680: Move from trivial-devices and add
    supplies
  iio: chemical: bme680: add regulators
  iio: chemical: bme680: add power management

 .../bindings/iio/chemical/bosch,bme680.yaml   |  62 +++++++++
 .../devicetree/bindings/trivial-devices.yaml  |   2 -
 drivers/iio/chemical/bme680.h                 |   2 +
 drivers/iio/chemical/bme680_core.c            | 125 +++++++++++++++++-
 drivers/iio/chemical/bme680_i2c.c             |   1 +
 drivers/iio/chemical/bme680_spi.c             |   1 +
 6 files changed, 184 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml


base-commit: a61ff7eac77e86de828fe28c4e42b8ae9ec2b195
-- 
2.43.0

[PATCH v5 1/3] dt-bindings: iio: bosch,bme680: Move from trivial-devices and add supplies

[Vasileios Amoiridis]

Move dt-binding for BME680 out of trivial-devices.yaml and extend it by
adding the missing supplies.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 .../bindings/iio/chemical/bosch,bme680.yaml   | 62 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |  2 -
 2 files changed, 62 insertions(+), 2 deletions(-)
 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..fe98ec44f081
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/bosch,bme680.yaml
@@ -0,0 +1,62 @@
+# 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
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        co2-sensor@77 {
+            compatible = "bosch,bme680";
+            reg = <0x77>;
+            vddio-supply = <&vddio>;
+            vdd-supply = <&vdd>;
+        };
+    };
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        co2-sensor@0 {
+            compatible = "bosch,bme680";
+            reg = <0>;
+            spi-max-frequency = <500000>;
+            vddio-supply = <&vddio>;
+            vdd-supply = <&vdd>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 9bf0fb17a05e..b651826e2d21 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -55,8 +55,6 @@ properties:
           - atmel,atsha204a
             # BPA-RS600: Power Supply
           - blutek,bpa-rs600
-            # Bosch Sensortec pressure, temperature, humididty and VOC sensor
-          - bosch,bme680
             # CM32181: Ambient Light Sensor
           - capella,cm32181
             # CM3232: Ambient Light Sensor
-- 
2.43.0

[PATCH v5 2/3] iio: chemical: bme680: add regulators

[Vasileios Amoiridis]

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

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 9783953e64e0..bcf84c0a1a59 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/buffer.h>
 #include <linux/iio/iio.h>
@@ -111,6 +112,8 @@ enum bme680_scan {
 	BME680_GAS,
 };
 
+static const char *const bme680_supply_names[] = { "vdd", "vddio" };
+
 struct bme680_data {
 	struct regmap *regmap;
 	struct bme680_calib bme680;
@@ -1114,6 +1117,14 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	data->heater_dur = 150;  /* milliseconds */
 	data->preheat_curr_mA = 0;
 
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(bme680_supply_names),
+					     bme680_supply_names);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to get and enable supplies.\n");
+
+	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

[PATCH v5 3/3] iio: chemical: bme680: add power management

[Vasileios Amoiridis]

Add runtime power management to the device.

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

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index 00ab89b3138b..7d86ed8b02e6 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -2,6 +2,7 @@
 #ifndef BME680_H_
 #define BME680_H_
 
+#include <linux/pm.h>
 #include <linux/regmap.h>
 
 #define BME680_REG_CHIP_ID			0xD0
@@ -80,6 +81,7 @@
 #define BME680_CALIB_RANGE_3_LEN               5
 
 extern const struct regmap_config bme680_regmap_config;
+extern const struct dev_pm_ops bme680_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 bcf84c0a1a59..f5f22a83ad5b 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -14,6 +14,8 @@
 #include <linux/device.h>
 #include <linux/log2.h>
 #include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -820,9 +822,9 @@ static int bme680_read_gas(struct bme680_data *data, int *comp_gas_res)
 	return 0;
 }
 
-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 chan_val, ret;
@@ -935,14 +937,33 @@ 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);
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	ret = __bme680_read_raw(indio_dev, chan, val, val2, mask);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(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);
 
@@ -987,6 +1008,25 @@ 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);
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	ret = __bme680_write_raw(indio_dev, chan, val, val2, mask);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
 static const char bme680_oversampling_ratio_show[] = "1 2 4 8 16";
 
 static IIO_CONST_ATTR(oversampling_ratio_available,
@@ -1087,6 +1127,34 @@ static irqreturn_t bme680_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static int bme680_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct bme680_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int bme680_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct bme680_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops bme680_buffer_setup_ops = {
+	.preenable = bme680_buffer_preenable,
+	.postdisable = bme680_buffer_postdisable,
+};
+
 int bme680_core_probe(struct device *dev, struct regmap *regmap,
 		      const char *name)
 {
@@ -1160,15 +1228,47 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
 					      iio_pollfunc_store_time,
 					      bme680_trigger_handler,
-					      NULL);
+					      &bme680_buffer_setup_ops);
 	if (ret)
 		return dev_err_probe(dev, ret,
 				     "iio triggered buffer setup failed\n");
 
+	/* Enable runtime PM */
+	pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_active(dev);
+	ret = devm_pm_runtime_enable(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 bme680_set_mode(data, BME680_MODE_SLEEP);
+}
+
+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 = bme680_chip_config(data);
+	if (ret)
+		return ret;
+
+	return bme680_gas_config(data);
+}
+
+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");
diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
index 7c4224d75955..9998d7fa3e98 100644
--- a/drivers/iio/chemical/bme680_i2c.c
+++ b/drivers/iio/chemical/bme680_i2c.c
@@ -51,6 +51,7 @@ static struct i2c_driver bme680_i2c_driver = {
 	.driver = {
 		.name			= "bme680_i2c",
 		.of_match_table		= bme680_of_i2c_match,
+		.pm = pm_ptr(&bme680_dev_pm_ops),
 	},
 	.probe = bme680_i2c_probe,
 	.id_table = bme680_i2c_id,
diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
index 7c54bd17d4b0..43d59544d903 100644
--- a/drivers/iio/chemical/bme680_spi.c
+++ b/drivers/iio/chemical/bme680_spi.c
@@ -154,6 +154,7 @@ static struct spi_driver bme680_spi_driver = {
 	.driver = {
 		.name			= "bme680_spi",
 		.of_match_table		= bme680_of_spi_match,
+		.pm = pm_ptr(&bme680_dev_pm_ops),
 	},
 	.probe = bme680_spi_probe,
 	.id_table = bme680_spi_id,
-- 
2.43.0

Re: [PATCH v5 3/3] iio: chemical: bme680: add power management

[Andy Shevchenko]

On Mon, Dec 02, 2024 at 08:23:41PM +0100, Vasileios Amoiridis wrote:
> Add runtime power management to the device.

...

> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = __bme680_read_raw(indio_dev, chan, val, val2, mask);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);

Side note: as long as idle method is not defined (NULL) the above dance is
already taken into account in the regular put.

...

> +static int bme680_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct bme680_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;

> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Either this is broken (if the above can return positive codes), or can be
replaced with direct return:

	return pm_...

(but I believe it's the former and you wanted something like if (ret < 0)
 there).

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/3] iio: chemical: bme680: add power management

[Vasileios Amoiridis]

On Mon, Dec 02, 2024 at 09:43:36PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 02, 2024 at 08:23:41PM +0100, Vasileios Amoiridis wrote:
> > Add runtime power management to the device.
> 
> ...
> 
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __bme680_read_raw(indio_dev, chan, val, val2, mask);
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_runtime_put_autosuspend(dev);
> 
> Side note: as long as idle method is not defined (NULL) the above dance is
> already taken into account in the regular put.
> 
> ...
> 

Hi Andy,

Thanks again for the review! Indeed by looking at the code a bit, it
looks like the suspend callback is being called if the idle one is not
found. But I have seen this dance that you mention much more often in
the IIO that's why I used it. We can see what Jonathan has to say as
well, I think what you propose, simplifies things.

> > +static int bme680_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> > +	struct bme680_data *data = iio_priv(indio_dev);
> > +	struct device *dev = regmap_get_device(data->regmap);
> > +	int ret;
> 
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> Either this is broken (if the above can return positive codes), or can be
> replaced with direct return:
> 
> 	return pm_...
> 
> (but I believe it's the former and you wanted something like if (ret < 0)
>  there).
> 
> > +}
> 

Well, pm_runtime_resume_and_get() looks like it returns 0 on success and
negative value on error so I think the if (ret) is correct, no? But I
agree with you that it can be simplified as you proposed.

Cheers,
Vasilis

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

Re: [PATCH v5 3/3] iio: chemical: bme680: add power management

[Andy Shevchenko]

On Mon, Dec 02, 2024 at 09:35:50PM +0100, Vasileios Amoiridis wrote:
> On Mon, Dec 02, 2024 at 09:43:36PM +0200, Andy Shevchenko wrote:
> > On Mon, Dec 02, 2024 at 08:23:41PM +0100, Vasileios Amoiridis wrote:
> > > Add runtime power management to the device.

...

> > > +	ret = pm_runtime_resume_and_get(dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = __bme680_read_raw(indio_dev, chan, val, val2, mask);
> > > +	pm_runtime_mark_last_busy(dev);
> > > +	pm_runtime_put_autosuspend(dev);
> > 
> > Side note: as long as idle method is not defined (NULL) the above dance is
> > already taken into account in the regular put.

> Thanks again for the review! Indeed by looking at the code a bit, it
> looks like the suspend callback is being called if the idle one is not
> found. But I have seen this dance that you mention much more often in
> the IIO that's why I used it. We can see what Jonathan has to say as
> well, I think what you propose, simplifies things.

Yeah, this is cargo cult by many people (including me :-) who missed that
detail. If any, this can be addressed in a different series.

...

> > > +static int bme680_buffer_preenable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct bme680_data *data = iio_priv(indio_dev);
> > > +	struct device *dev = regmap_get_device(data->regmap);
> > > +	int ret;
> > 
> > > +	ret = pm_runtime_resume_and_get(dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > 
> > Either this is broken (if the above can return positive codes), or can be
> > replaced with direct return:
> > 
> > 	return pm_...
> > 
> > (but I believe it's the former and you wanted something like if (ret < 0)
> >  there).
> > 
> > > +}
> 
> Well, pm_runtime_resume_and_get() looks like it returns 0 on success and
> negative value on error so I think the if (ret) is correct, no? But I
> agree with you that it can be simplified as you proposed.

Please, go ahead with the simplification!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/3] iio: chemical: bme680: add power management

[Jonathan Cameron]

On Tue, 3 Dec 2024 14:42:36 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Dec 02, 2024 at 09:35:50PM +0100, Vasileios Amoiridis wrote:
> > On Mon, Dec 02, 2024 at 09:43:36PM +0200, Andy Shevchenko wrote:  
> > > On Mon, Dec 02, 2024 at 08:23:41PM +0100, Vasileios Amoiridis wrote:  
> > > > Add runtime power management to the device.  
> 
> ...
> 
> > > > +	ret = pm_runtime_resume_and_get(dev);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = __bme680_read_raw(indio_dev, chan, val, val2, mask);
> > > > +	pm_runtime_mark_last_busy(dev);
> > > > +	pm_runtime_put_autosuspend(dev);  
> > > 
> > > Side note: as long as idle method is not defined (NULL) the above dance is
> > > already taken into account in the regular put.  
> 
> > Thanks again for the review! Indeed by looking at the code a bit, it
> > looks like the suspend callback is being called if the idle one is not
> > found. But I have seen this dance that you mention much more often in
> > the IIO that's why I used it. We can see what Jonathan has to say as
> > well, I think what you propose, simplifies things.  
> 
> Yeah, this is cargo cult by many people (including me :-) who missed that
> detail. If any, this can be addressed in a different series.
> 
> ...
> 
> > > > +static int bme680_buffer_preenable(struct iio_dev *indio_dev)
> > > > +{
> > > > +	struct bme680_data *data = iio_priv(indio_dev);
> > > > +	struct device *dev = regmap_get_device(data->regmap);
> > > > +	int ret;  
> > >   
> > > > +	ret = pm_runtime_resume_and_get(dev);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return 0;  
> > > 
> > > Either this is broken (if the above can return positive codes), or can be
> > > replaced with direct return:
> > > 
> > > 	return pm_...
> > > 
> > > (but I believe it's the former and you wanted something like if (ret < 0)
> > >  there).
> > >   
> > > > +}  
> > 
> > Well, pm_runtime_resume_and_get() looks like it returns 0 on success and
> > negative value on error so I think the if (ret) is correct, no? But I
> > agree with you that it can be simplified as you proposed.  
> 
> Please, go ahead with the simplification!
> 
I tweaked it and applied the series to the togreg branch of iio.git and
pushed out as testing for all the normal reasons.

There was some mess because of the EXPORT_SYMBOL() macro changes this raced
against.  Please sanity check I didn't mess it up.

Jonathan

Re: [PATCH v5 3/3] iio: chemical: bme680: add power management

[Vasileios Amoiridis]

On Sun, Dec 08, 2024 at 05:33:36PM +0000, Jonathan Cameron wrote:
> On Tue, 3 Dec 2024 14:42:36 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Mon, Dec 02, 2024 at 09:35:50PM +0100, Vasileios Amoiridis wrote:
> > > On Mon, Dec 02, 2024 at 09:43:36PM +0200, Andy Shevchenko wrote:  
> > > > On Mon, Dec 02, 2024 at 08:23:41PM +0100, Vasileios Amoiridis wrote:  
> > > > > Add runtime power management to the device.  
> > 
> > ...
> > 
> > > > > +	ret = pm_runtime_resume_and_get(dev);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = __bme680_read_raw(indio_dev, chan, val, val2, mask);
> > > > > +	pm_runtime_mark_last_busy(dev);
> > > > > +	pm_runtime_put_autosuspend(dev);  
> > > > 
> > > > Side note: as long as idle method is not defined (NULL) the above dance is
> > > > already taken into account in the regular put.  
> > 
> > > Thanks again for the review! Indeed by looking at the code a bit, it
> > > looks like the suspend callback is being called if the idle one is not
> > > found. But I have seen this dance that you mention much more often in
> > > the IIO that's why I used it. We can see what Jonathan has to say as
> > > well, I think what you propose, simplifies things.  
> > 
> > Yeah, this is cargo cult by many people (including me :-) who missed that
> > detail. If any, this can be addressed in a different series.
> > 
> > ...
> > 
> > > > > +static int bme680_buffer_preenable(struct iio_dev *indio_dev)
> > > > > +{
> > > > > +	struct bme680_data *data = iio_priv(indio_dev);
> > > > > +	struct device *dev = regmap_get_device(data->regmap);
> > > > > +	int ret;  
> > > >   
> > > > > +	ret = pm_runtime_resume_and_get(dev);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	return 0;  
> > > > 
> > > > Either this is broken (if the above can return positive codes), or can be
> > > > replaced with direct return:
> > > > 
> > > > 	return pm_...
> > > > 
> > > > (but I believe it's the former and you wanted something like if (ret < 0)
> > > >  there).
> > > >   
> > > > > +}  
> > > 
> > > Well, pm_runtime_resume_and_get() looks like it returns 0 on success and
> > > negative value on error so I think the if (ret) is correct, no? But I
> > > agree with you that it can be simplified as you proposed.  
> > 
> > Please, go ahead with the simplification!
> > 
> I tweaked it and applied the series to the togreg branch of iio.git and
> pushed out as testing for all the normal reasons.
> 
> There was some mess because of the EXPORT_SYMBOL() macro changes this raced
> against.  Please sanity check I didn't mess it up.
> 
> Jonathan
> 
>

Hi Jonathan,

Thank you for this! It looks good in the tree. 

Cheers,
Vasilis