| Message ID | 20241010210030.33309-9-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | : chemical: bme680: 2nd set of clean and add | expand |
On Thu, Oct 10, 2024 at 11:00:25PM +0200, vamoirid wrote: > From: Vasileios Amoiridis <vassilisamir@gmail.com> > > 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. ... > --- a/drivers/iio/chemical/bme680.h > +++ b/drivers/iio/chemical/bme680.h > @@ -75,6 +75,7 @@ > #define BME680_CALIB_RANGE_3_LEN 5 > > extern const struct regmap_config bme680_regmap_config; > +extern const struct dev_pm_ops bmp280_dev_pm_ops; Is pm.h being included already in this header? Otherwise you need to add it. ... > struct regmap *regmap; > struct bme680_calib bme680; > struct mutex lock; /* Protect multiple serial R/W ops to device. */ > + struct device *dev; Is it the same that you may get wia regmap_get_device()? > u8 oversampling_temp; > u8 oversampling_press; > u8 oversampling_humid; ... > + /* Enable runtime PM */ > + pm_runtime_get_noresume(dev); > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100); > + pm_runtime_use_autosuspend(dev); > + pm_runtime_put(dev); Can we use devm_pm_runtime_enable() for some of the above? > + ret = devm_add_action_or_reset(dev, bme680_pm_disable, dev); > + if (ret) > + return ret; ... > +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; > + ret = bme680_gas_config(data); > + if (ret) > + return ret; > + > + return 0; return bme680_gas_config(...); > +} ... > +EXPORT_RUNTIME_DEV_PM_OPS(bme680_dev_pm_ops, bme680_runtime_suspend, > + bme680_runtime_resume, NULL); You also need pm.h for the macro IIRC.
On Fri, Oct 11, 2024 at 01:10:20PM +0300, Andy Shevchenko wrote: > On Thu, Oct 10, 2024 at 11:00:25PM +0200, vamoirid wrote: > > From: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > 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. > > ... > > > --- a/drivers/iio/chemical/bme680.h > > +++ b/drivers/iio/chemical/bme680.h > > @@ -75,6 +75,7 @@ > > #define BME680_CALIB_RANGE_3_LEN 5 > > > > extern const struct regmap_config bme680_regmap_config; > > +extern const struct dev_pm_ops bmp280_dev_pm_ops; > > Is pm.h being included already in this header? Otherwise you need to add it. > No it is not, and indeed I need to add it. Probably because it was included by some other file I didn't get an error from gcc? > ... > > > struct regmap *regmap; > > struct bme680_calib bme680; > > struct mutex lock; /* Protect multiple serial R/W ops to device. */ > > + struct device *dev; > > Is it the same that you may get wia regmap_get_device()? > Yes it is the same. Maybe I can try and see if I can use the following regmap_get_device(data->regmap) in the places where the pm functions are used in order to not declare a new value inside the struct bme680_data. But in general, is this approach prefered? > > u8 oversampling_temp; > > u8 oversampling_press; > > u8 oversampling_humid; > > ... > > > + /* Enable runtime PM */ > > + pm_runtime_get_noresume(dev); > > + pm_runtime_set_active(dev); > > + pm_runtime_enable(dev); > > + pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100); > > + pm_runtime_use_autosuspend(dev); > > + pm_runtime_put(dev); > > Can we use devm_pm_runtime_enable() for some of the above? > I will have to check its use, and I will let you know. > > + ret = devm_add_action_or_reset(dev, bme680_pm_disable, dev); > > + if (ret) > > + return ret; > > ... > > > +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; > > > + ret = bme680_gas_config(data); > > + if (ret) > > + return ret; > > + > > + return 0; > > return bme680_gas_config(...); > Indeed, much cleaner, thanks! > > +} > > ... > > > +EXPORT_RUNTIME_DEV_PM_OPS(bme680_dev_pm_ops, bme680_runtime_suspend, > > + bme680_runtime_resume, NULL); > > You also need pm.h for the macro IIRC. > ACK. > -- > With Best Regards, > Andy Shevchenko > > Cheers, Vasilis
On Fri, 11 Oct 2024 21:02:32 +0200 Vasileios Aoiridis <vassilisamir@gmail.com> wrote: > On Fri, Oct 11, 2024 at 01:10:20PM +0300, Andy Shevchenko wrote: > > On Thu, Oct 10, 2024 at 11:00:25PM +0200, vamoirid wrote: > > > From: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > > > 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. > > > > ... > > > > > --- a/drivers/iio/chemical/bme680.h > > > +++ b/drivers/iio/chemical/bme680.h > > > @@ -75,6 +75,7 @@ > > > #define BME680_CALIB_RANGE_3_LEN 5 > > > > > > extern const struct regmap_config bme680_regmap_config; > > > +extern const struct dev_pm_ops bmp280_dev_pm_ops; > > > > Is pm.h being included already in this header? Otherwise you need to add it. > > > > No it is not, and indeed I need to add it. Probably because it was > included by some other file I didn't get an error from gcc? > > > ... > > > > > struct regmap *regmap; > > > struct bme680_calib bme680; > > > struct mutex lock; /* Protect multiple serial R/W ops to device. */ > > > + struct device *dev; > > > > Is it the same that you may get wia regmap_get_device()? > > > > Yes it is the same. Maybe I can try and see if I can use the following > > regmap_get_device(data->regmap) > > in the places where the pm functions are used in order to not declare a > new value inside the struct bme680_data. But in general, is this approach > prefered? slightly by me. I tend not to poke on that if people have chosen a local variable, but it is a little neater. This patch might get caught up in an effort to simplify the autosuspend handling but if it is we'll deal with that whilst merging. There 'should' be a clean path to transition from this style to the proposed new one where a simple pm_runtime_put() without the mark_last_busy stuff is enough for autosuspend cases. Jonathan
Fri, Oct 11, 2024 at 09:02:32PM +0200, Vasileios Aoiridis kirjoitti: > On Fri, Oct 11, 2024 at 01:10:20PM +0300, Andy Shevchenko wrote: > > On Thu, Oct 10, 2024 at 11:00:25PM +0200, vamoirid wrote: ... > > > +extern const struct dev_pm_ops bmp280_dev_pm_ops; > > > > Is pm.h being included already in this header? Otherwise you need to add it. > > No it is not, and indeed I need to add it. Probably because it was > included by some other file I didn't get an error from gcc? Yeah, it's called a "proxy" header in general meaning. We should try hard not to use such headers (meaning not to use them in a "proxy" mode). ... > > > struct regmap *regmap; > > > struct bme680_calib bme680; > > > struct mutex lock; /* Protect multiple serial R/W ops to device. */ > > > + struct device *dev; > > > > Is it the same that you may get wia regmap_get_device()? > > > > Yes it is the same. Maybe I can try and see if I can use the following > > regmap_get_device(data->regmap) > > in the places where the pm functions are used in order to not declare a > new value inside the struct bme680_data. But in general, is this approach > prefered? Since there is a getter already available, I prefer not to shortcut it via adding a duplicating information to the data structure. > > > u8 oversampling_temp; > > > u8 oversampling_press; > > > u8 oversampling_humid;
diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h index e55a48982b3e..e9e3e08fa366 100644 --- a/drivers/iio/chemical/bme680.h +++ b/drivers/iio/chemical/bme680.h @@ -75,6 +75,7 @@ #define BME680_CALIB_RANGE_3_LEN 5 extern const struct regmap_config bme680_regmap_config; +extern const struct dev_pm_ops bmp280_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 a2039b966f20..5fd5740bb7fe 100644 --- a/drivers/iio/chemical/bme680_core.c +++ b/drivers/iio/chemical/bme680_core.c @@ -14,6 +14,7 @@ #include <linux/device.h> #include <linux/log2.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> @@ -111,6 +112,7 @@ struct bme680_data { struct regmap *regmap; struct bme680_calib bme680; struct mutex lock; /* Protect multiple serial R/W ops to device. */ + struct device *dev; u8 oversampling_temp; u8 oversampling_press; u8 oversampling_humid; @@ -753,9 +755,9 @@ static int bme680_read_gas(struct bme680_data *data, int *val) return IIO_VAL_INT; } -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 ret; @@ -803,14 +805,29 @@ 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); + int ret; + + pm_runtime_get_sync(data->dev); + ret = __bme680_read_raw(indio_dev, chan, val, val2, mask); + pm_runtime_mark_last_busy(data->dev); + pm_runtime_put_autosuspend(data->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); @@ -846,6 +863,21 @@ 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); + int ret; + + pm_runtime_get_sync(data->dev); + ret = __bme680_write_raw(indio_dev, chan, val, val2, mask); + pm_runtime_mark_last_busy(data->dev); + pm_runtime_put_autosuspend(data->dev); + + return ret; +} + static const char bme680_oversampling_ratio_show[] = "1 2 4 8 16"; static IIO_CONST_ATTR(oversampling_ratio_available, @@ -873,6 +905,15 @@ static void bme680_regulators_disable(void *data) regulator_bulk_disable(BME680_NUM_SUPPLIES, supplies); } +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) { @@ -887,6 +928,7 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap, data = iio_priv(indio_dev); mutex_init(&data->lock); dev_set_drvdata(dev, indio_dev); + data->dev = dev; data->regmap = regmap; indio_dev->name = name; indio_dev->channels = bme680_channels; @@ -947,10 +989,56 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap, return dev_err_probe(dev, ret, "failed to set gas config data\n"); + /* Enable runtime PM */ + pm_runtime_get_noresume(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + pm_runtime_set_autosuspend_delay(dev, BME680_STARTUP_TIME_US * 100); + pm_runtime_use_autosuspend(dev); + 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; + + ret = bme680_gas_config(data); + if (ret) + return ret; + + return 0; +} + +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");