| Message ID | 20241021195316.58911-14-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | : chemical: bme680: 2nd set of cleanup | expand |
On Mon, 21 Oct 2024 21:53:16 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > 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. Needs an update as you are now getting that from the regmap. > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > --- > drivers/iio/chemical/bme680.h | 2 + > drivers/iio/chemical/bme680_core.c | 126 +++++++++++++++++++++++++++-- > 2 files changed, 121 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h > index e5d82a6d5b59..74e97e35e35a 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 > @@ -82,6 +83,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; You seem to have missed the changes that use this in the i2c and spi drivers. > static const char bme680_oversampling_ratio_show[] = "1 2 4 8 16"; > > static IIO_CONST_ATTR(oversampling_ratio_available, > @@ -1091,6 +1125,39 @@ 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); > + > + pm_runtime_get_sync(dev); > + 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, > +}; > + > +static void bme680_pm_disable(void *data) > +{ > + struct device *dev = data; > + > + pm_runtime_get_sync(dev); > + pm_runtime_put_noidle(dev); This dance is to get the device powered up on runtime pm tear down I think? Whilst we sometimes do this, why is it needed in this particular driver? > + pm_runtime_disable(dev); > +} > + > int bme680_core_probe(struct device *dev, struct regmap *regmap, > const char *name) > { > @@ -1164,15 +1231,60 @@ 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_get_noresume(dev); > + pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100); > + pm_runtime_use_autosuspend(dev); > + pm_runtime_set_active(dev); > + ret = devm_pm_runtime_enable(dev); Take a look at what this unwinds in the devm handler. You don't need to do as much (or possibly anything) yourself. > + if (ret) > + return ret; > + > + 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; > + > + 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");
On Sun, Oct 27, 2024 at 10:30:13AM +0000, Jonathan Cameron wrote: > On Mon, 21 Oct 2024 21:53:16 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > 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. > > Needs an update as you are now getting that from the regmap. > > Hi Jonathan, Thanks for noticing, I will fix it for next version. > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > --- > > drivers/iio/chemical/bme680.h | 2 + > > drivers/iio/chemical/bme680_core.c | 126 +++++++++++++++++++++++++++-- > > 2 files changed, 121 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h > > index e5d82a6d5b59..74e97e35e35a 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 > > @@ -82,6 +83,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; > > You seem to have missed the changes that use this in the i2c and spi drivers. > > You are totally right. > > static const char bme680_oversampling_ratio_show[] = "1 2 4 8 16"; > > > > static IIO_CONST_ATTR(oversampling_ratio_available, > > @@ -1091,6 +1125,39 @@ 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); > > + > > + pm_runtime_get_sync(dev); > > + 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, > > +}; > > + > > +static void bme680_pm_disable(void *data) > > +{ > > + struct device *dev = data; > > + > > + pm_runtime_get_sync(dev); > > + pm_runtime_put_noidle(dev); > This dance is to get the device powered up on runtime pm tear down > I think? Whilst we sometimes do this, why is it needed in this particular driver? > Actually this one is not needed indeed, I was using it to test that the device was coming up but it is not really needed. And the whole disable sequence is perfectly handled by the devm_pm_runtime_enable so it is not even needed at all actually. > > + pm_runtime_disable(dev); > > +} > > + > > int bme680_core_probe(struct device *dev, struct regmap *regmap, > > const char *name) > > { > > @@ -1164,15 +1231,60 @@ 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_get_noresume(dev); > > + pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100); > > + pm_runtime_use_autosuspend(dev); > > + pm_runtime_set_active(dev); > > + ret = devm_pm_runtime_enable(dev); > > Take a look at what this unwinds in the devm handler. You don't need to do > as much (or possibly anything) yourself. > > Well, in general what I see that could probably be dropped here is the extra add_action_or_reset because of the devm_*. About the functions get_noresume() and put(), I feel that they could be dropped as well because the device registration will happen well before the autosuspend delay, but does it make sense to depend on something like this? Cheers, Vasilis > > + if (ret) > > + return ret; > > + > > + 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; > > + > > + 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"); >
> > > + > > > int bme680_core_probe(struct device *dev, struct regmap *regmap, > > > const char *name) > > > { > > > @@ -1164,15 +1231,60 @@ 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_get_noresume(dev); > > > + pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100); > > > + pm_runtime_use_autosuspend(dev); > > > + pm_runtime_set_active(dev); > > > + ret = devm_pm_runtime_enable(dev); > > > > Take a look at what this unwinds in the devm handler. You don't need to do > > as much (or possibly anything) yourself. > > > > > > Well, in general what I see that could probably be dropped here is the > extra add_action_or_reset because of the devm_*. About the functions > get_noresume() and put(), I feel that they could be dropped as well > because the device registration will happen well before the autosuspend > delay, but does it make sense to depend on something like this? If you haven't enabled runtime pm yet all you need to do is set up the current state Shouldn't need the get etc as you don't care if it powers down here anyway as you aren't talking to the device. > > Cheers, > Vasilis > > > > + if (ret) > > > + return ret; > > > + > > > + 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; > > > + > > > + 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.h b/drivers/iio/chemical/bme680.h index e5d82a6d5b59..74e97e35e35a 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 @@ -82,6 +83,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 2d9d20f203aa..803aa4f14b37 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> @@ -823,9 +825,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; @@ -937,14 +939,30 @@ 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; + + pm_runtime_get_sync(dev); + 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); @@ -989,6 +1007,22 @@ 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; + + pm_runtime_get_sync(dev); + 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, @@ -1091,6 +1125,39 @@ 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); + + pm_runtime_get_sync(dev); + 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, +}; + +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) { @@ -1164,15 +1231,60 @@ 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_get_noresume(dev); + pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100); + pm_runtime_use_autosuspend(dev); + pm_runtime_set_active(dev); + ret = devm_pm_runtime_enable(dev); + if (ret) + return ret; + + 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; + + 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");
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 | 2 + drivers/iio/chemical/bme680_core.c | 126 +++++++++++++++++++++++++++-- 2 files changed, 121 insertions(+), 7 deletions(-)