| Message ID | 20241202192341.33187-4-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: chemical bme680: 2nd round of cleanup | expand |
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). > +}
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!
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
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,
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(-)