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

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

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

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

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

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