linux-kernel - Re: [PATCH v2 13/13] iio: chemical: bme680: add power management

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241027103013.06daac42@jic23-huawei>
Date: Sun, 27 Oct 2024 10:30:13 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, andriy.shevchenko@...ux.intel.com,
 anshulusr@...il.com, gustavograzs@...il.com, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 13/13] iio: chemical: bme680: add power management

On Mon, 21 Oct 2024 21:53:16 +0200
Vasileios Amoiridis <vassilisamir@...il.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@...il.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@...il.com>");
>  MODULE_DESCRIPTION("Bosch BME680 Driver");
>  MODULE_LICENSE("GPL v2");