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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>");
> > MODULE_DESCRIPTION("Bosch BME680 Driver");
> > MODULE_LICENSE("GPL v2");
>
