[PATCH 1/2] regulator: core: Use a bitfield for continuous_voltage_range

[PATCH 1/2] regulator: core: Use a bitfield for continuous_voltage_range

[Mark Brown]

Using a bitfield enables the compiler to lay out the structure more
efficiently when we have other boolean flags since multiple values can
be included in a single byte.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/regulator/driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 16ac9e108806..3ac0f306f033 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -281,7 +281,7 @@ struct regulator_desc {
 			    const struct regulator_desc *,
 			    struct regulator_config *);
 	int id;
-	bool continuous_voltage_range;
+	unsigned int continuous_voltage_range:1;
 	unsigned n_voltages;
 	const struct regulator_ops *ops;
 	int irq;
-- 
2.7.0.rc3

[PATCH 2/2] regulator: core: Provide per-regulator runtime PM support

[Mark Brown]

Provide a flag auto_runtime_pm in the regulator_desc which causes the
regulator core to take a runtime PM reference to a regulator while it
is enabled. This helps integration with chip wide power management for
auxiliary PMICs, they may be able to implement chip wide power savings
if nothing on the PMIC is in use.

Signed-off-by: Mark Brown <broonie@kernel.org>
---

Not tested at all yet, pushing out for testing by others who have
devices that could benefit from this.

 drivers/regulator/core.c         | 20 ++++++++++++++++++--
 include/linux/regulator/driver.h |  1 +
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3308c6bb83db..968ff3081e6c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -26,6 +26,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/consumer.h>
@@ -2059,17 +2060,23 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 		}
 	}
 
+	if (rdev->desc->auto_runtime_pm) {
+		ret = pm_runtime_get_sync(rdev->dev.parent);
+		if (ret < 0)
+			goto err;
+	}
+
 	if (rdev->ena_pin) {
 		if (!rdev->ena_gpio_state) {
 			ret = regulator_ena_gpio_ctrl(rdev, true);
 			if (ret < 0)
-				return ret;
+				goto err_pm;
 			rdev->ena_gpio_state = 1;
 		}
 	} else if (rdev->desc->ops->enable) {
 		ret = rdev->desc->ops->enable(rdev);
 		if (ret < 0)
-			return ret;
+			goto err_pm;
 	} else {
 		return -EINVAL;
 	}
@@ -2084,6 +2091,12 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 	trace_regulator_enable_complete(rdev_get_name(rdev));
 
 	return 0;
+
+err_pm:
+	if (rdev->desc->auto_runtime_pm)
+		pm_runtime_put_autosuspend(rdev->dev.parent);
+err:
+	return ret;
 }
 
 /* locks held by regulator_enable() */
@@ -2177,6 +2190,9 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
 			return ret;
 	}
 
+	if (rdev->desc->auto_runtime_pm)
+		pm_runtime_put_autosuspend(rdev->dev.parent);
+
 	/* cares about last_off_jiffy only if off_on_delay is required by
 	 * device.
 	 */
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 3ac0f306f033..dccea032a143 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -282,6 +282,7 @@ struct regulator_desc {
 			    struct regulator_config *);
 	int id;
 	unsigned int continuous_voltage_range:1;
+	unsigned int auto_runtime_pm:1;
 	unsigned n_voltages;
 	const struct regulator_ops *ops;
 	int irq;
-- 
2.7.0.rc3

Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 13685 bytes --]

Hi Mark,

[auto build test WARNING on regulator/for-next]
[also build test WARNING on v4.4 next-20160121]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Mark-Brown/regulator-core-Use-a-bitfield-for-continuous_voltage_range/20160122-042835
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/regulator/machine.h:151: warning: No description found for parameter 'over_current_protection'
   include/linux/regulator/driver.h:202: warning: No description found for parameter 'set_over_current_protection'
>> include/linux/regulator/driver.h:325: warning: No description found for parameter 'auto_runtime_pm'
   include/linux/regulator/driver.h:325: warning: No description found for parameter 'csel_reg'
   include/linux/regulator/driver.h:325: warning: No description found for parameter 'csel_mask'

vim +/auto_runtime_pm +325 include/linux/regulator/driver.h

571a354b15 Liam Girdwood            2008-04-30  196  	int (*set_suspend_disable) (struct regulator_dev *);
571a354b15 Liam Girdwood            2008-04-30  197  
fde297bb4d Kim, Milo                2012-02-16  198  	/* set regulator suspend operating mode (defined in consumer.h) */
571a354b15 Liam Girdwood            2008-04-30  199  	int (*set_suspend_mode) (struct regulator_dev *, unsigned int mode);
23c779b9f9 Stephen Boyd             2015-06-11  200  
23c779b9f9 Stephen Boyd             2015-06-11  201  	int (*set_pull_down) (struct regulator_dev *);
571a354b15 Liam Girdwood            2008-04-30 @202  };
571a354b15 Liam Girdwood            2008-04-30  203  
571a354b15 Liam Girdwood            2008-04-30  204  /*
571a354b15 Liam Girdwood            2008-04-30  205   * Regulators can either control voltage or current.
571a354b15 Liam Girdwood            2008-04-30  206   */
571a354b15 Liam Girdwood            2008-04-30  207  enum regulator_type {
571a354b15 Liam Girdwood            2008-04-30  208  	REGULATOR_VOLTAGE,
571a354b15 Liam Girdwood            2008-04-30  209  	REGULATOR_CURRENT,
571a354b15 Liam Girdwood            2008-04-30  210  };
571a354b15 Liam Girdwood            2008-04-30  211  
571a354b15 Liam Girdwood            2008-04-30  212  /**
c172708d38 Mark Brown               2012-04-04  213   * struct regulator_desc - Static regulator descriptor
571a354b15 Liam Girdwood            2008-04-30  214   *
c172708d38 Mark Brown               2012-04-04  215   * Each regulator registered with the core is described with a
c172708d38 Mark Brown               2012-04-04  216   * structure of this type and a struct regulator_config.  This
c172708d38 Mark Brown               2012-04-04  217   * structure contains the non-varying parts of the regulator
c172708d38 Mark Brown               2012-04-04  218   * description.
c8e7e4640f Mark Brown               2008-12-31  219   *
c8e7e4640f Mark Brown               2008-12-31  220   * @name: Identifying name for the regulator.
69511a452e Rajendra Nayak           2011-11-18  221   * @supply_name: Identifying the regulator supply
a0c7b164ad Mark Brown               2014-09-09  222   * @of_match: Name used to identify regulator in DT.
a0c7b164ad Mark Brown               2014-09-09  223   * @regulators_node: Name of node containing regulator definitions in DT.
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  224   * @of_parse_cb: Optional callback called only if of_match is present.
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  225   *               Will be called for each regulator parsed from DT, during
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  226   *               init_data parsing.
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  227   *               The regulator_config passed as argument to the callback will
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  228   *               be a copy of config passed to regulator_register, valid only
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  229   *               for this particular call. Callback may freely change the
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  230   *               config but it cannot store it for later usage.
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  231   *               Callback should return 0 on success or negative ERRNO
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  232   *               indicating failure.
c8e7e4640f Mark Brown               2008-12-31  233   * @id: Numerical identifier for the regulator.
c8e7e4640f Mark Brown               2008-12-31  234   * @ops: Regulator operations table.
0ba4887c63 Randy Dunlap             2009-01-08  235   * @irq: Interrupt number for the regulator.
c8e7e4640f Mark Brown               2008-12-31  236   * @type: Indicates if the regulator is a voltage or current regulator.
c8e7e4640f Mark Brown               2008-12-31  237   * @owner: Module providing the regulator, used for refcounting.
bca7bbfff3 Mark Brown               2012-05-09  238   *
bd7a2b600a Pawel Moll               2012-09-24  239   * @continuous_voltage_range: Indicates if the regulator can set any
bd7a2b600a Pawel Moll               2012-09-24  240   *                            voltage within constrains range.
bca7bbfff3 Mark Brown               2012-05-09  241   * @n_voltages: Number of selectors available for ops.list_voltage().
bca7bbfff3 Mark Brown               2012-05-09  242   *
bca7bbfff3 Mark Brown               2012-05-09  243   * @min_uV: Voltage given by the lowest selector (if linear mapping)
bca7bbfff3 Mark Brown               2012-05-09  244   * @uV_step: Voltage increase with each selector (if linear mapping)
33234e791d Axel Lin                 2012-11-27  245   * @linear_min_sel: Minimal selector for starting linear mapping
5a523605af Laxman Dewangan          2013-09-10  246   * @fixed_uV: Fixed voltage of rails.
ea38d13fd1 Axel Lin                 2012-06-18  247   * @ramp_delay: Time to settle down after voltage change (unit: uV/us)
5abe4f223e Sascha Hauer             2015-10-13  248   * @min_dropout_uV: The minimum dropout voltage this regulator can handle
a8dbfeedfe Randy Dunlap             2014-08-27  249   * @linear_ranges: A constant table of possible voltage ranges.
a8dbfeedfe Randy Dunlap             2014-08-27  250   * @n_linear_ranges: Number of entries in the @linear_ranges table.
cffc9592fd Axel Lin                 2012-05-20  251   * @volt_table: Voltage mapping table (if table based mapping)
bca7bbfff3 Mark Brown               2012-05-09  252   *
4ab5b3d92c Mark Brown               2012-04-15  253   * @vsel_reg: Register for selector when using regulator_regmap_X_voltage_
4ab5b3d92c Mark Brown               2012-04-15  254   * @vsel_mask: Mask for register bitfield used for selector
c8520b4c5d Axel Lin                 2012-12-18  255   * @apply_reg: Register for initiate voltage change on the output when
c8520b4c5d Axel Lin                 2012-12-18  256   *                using regulator_set_voltage_sel_regmap
c8520b4c5d Axel Lin                 2012-12-18  257   * @apply_bit: Register bitfield used for initiate voltage change on the
c8520b4c5d Axel Lin                 2012-12-18  258   *                output when using regulator_set_voltage_sel_regmap
cd6dffb4c6 Mark Brown               2012-04-15  259   * @enable_reg: Register for control when using regmap enable/disable ops
cd6dffb4c6 Mark Brown               2012-04-15  260   * @enable_mask: Mask for control when using regmap enable/disable ops
ca5d1b3524 Carlo Caione             2014-03-05  261   * @enable_val: Enabling value for control when using regmap enable/disable ops
ca5d1b3524 Carlo Caione             2014-03-05  262   * @disable_val: Disabling value for control when using regmap enable/disable ops
51dcdafcb7 Axel Lin                 2013-03-05  263   * @enable_is_inverted: A flag to indicate set enable_mask bits to disable
51dcdafcb7 Axel Lin                 2013-03-05  264   *                      when using regulator_enable_regmap and friends APIs.
5838b032fd Nishanth Menon           2013-02-28  265   * @bypass_reg: Register for control when using regmap set_bypass
5838b032fd Nishanth Menon           2013-02-28  266   * @bypass_mask: Mask for control when using regmap set_bypass
ca5d1b3524 Carlo Caione             2014-03-05  267   * @bypass_val_on: Enabling value for control when using regmap set_bypass
ca5d1b3524 Carlo Caione             2014-03-05  268   * @bypass_val_off: Disabling value for control when using regmap set_bypass
79511ed322 Mark Brown               2012-06-27  269   *
79511ed322 Mark Brown               2012-06-27  270   * @enable_time: Time taken for initial enable of regulator (in uS).
871f565055 Guodong Xu               2014-08-13  271   * @off_on_delay: guard time (in uS), before re-enabling a regulator
87e1e0f29f Javier Martinez Canillas 2014-11-10  272   *
87e1e0f29f Javier Martinez Canillas 2014-11-10  273   * @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
571a354b15 Liam Girdwood            2008-04-30  274   */
571a354b15 Liam Girdwood            2008-04-30  275  struct regulator_desc {
571a354b15 Liam Girdwood            2008-04-30  276  	const char *name;
69511a452e Rajendra Nayak           2011-11-18  277  	const char *supply_name;
a0c7b164ad Mark Brown               2014-09-09  278  	const char *of_match;
a0c7b164ad Mark Brown               2014-09-09  279  	const char *regulators_node;
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  280  	int (*of_parse_cb)(struct device_node *,
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  281  			    const struct regulator_desc *,
bfa21a0dfe Krzysztof Kozlowski      2015-01-05  282  			    struct regulator_config *);
571a354b15 Liam Girdwood            2008-04-30  283  	int id;
5105f4655a Mark Brown               2016-01-21  284  	unsigned int continuous_voltage_range:1;
cfe771af73 Mark Brown               2016-01-21  285  	unsigned int auto_runtime_pm:1;
4367cfdc7c David Brownell           2009-02-26  286  	unsigned n_voltages;
df11e506d3 Axel Lin                 2014-08-21  287  	const struct regulator_ops *ops;
571a354b15 Liam Girdwood            2008-04-30  288  	int irq;
571a354b15 Liam Girdwood            2008-04-30  289  	enum regulator_type type;
571a354b15 Liam Girdwood            2008-04-30  290  	struct module *owner;
4ab5b3d92c Mark Brown               2012-04-15  291  
bca7bbfff3 Mark Brown               2012-05-09  292  	unsigned int min_uV;
bca7bbfff3 Mark Brown               2012-05-09  293  	unsigned int uV_step;
33234e791d Axel Lin                 2012-11-27  294  	unsigned int linear_min_sel;
5a523605af Laxman Dewangan          2013-09-10  295  	int fixed_uV;
98a175b60f Yadwinder Singh Brar     2012-06-09  296  	unsigned int ramp_delay;
5abe4f223e Sascha Hauer             2015-10-13  297  	int min_dropout_uV;
bca7bbfff3 Mark Brown               2012-05-09  298  
94d33c02c7 Mark Brown               2013-07-02  299  	const struct regulator_linear_range *linear_ranges;
94d33c02c7 Mark Brown               2013-07-02  300  	int n_linear_ranges;
94d33c02c7 Mark Brown               2013-07-02  301  
cffc9592fd Axel Lin                 2012-05-20  302  	const unsigned int *volt_table;
cffc9592fd Axel Lin                 2012-05-20  303  
4ab5b3d92c Mark Brown               2012-04-15  304  	unsigned int vsel_reg;
4ab5b3d92c Mark Brown               2012-04-15  305  	unsigned int vsel_mask;
c0ea88b890 Nikita Kiryanov          2015-11-25  306  	unsigned int csel_reg;
c0ea88b890 Nikita Kiryanov          2015-11-25  307  	unsigned int csel_mask;
c8520b4c5d Axel Lin                 2012-12-18  308  	unsigned int apply_reg;
c8520b4c5d Axel Lin                 2012-12-18  309  	unsigned int apply_bit;
cd6dffb4c6 Mark Brown               2012-04-15  310  	unsigned int enable_reg;
cd6dffb4c6 Mark Brown               2012-04-15  311  	unsigned int enable_mask;
ca5d1b3524 Carlo Caione             2014-03-05  312  	unsigned int enable_val;
ca5d1b3524 Carlo Caione             2014-03-05  313  	unsigned int disable_val;
51dcdafcb7 Axel Lin                 2013-03-05  314  	bool enable_is_inverted;
df36793115 Mark Brown               2012-08-27  315  	unsigned int bypass_reg;
df36793115 Mark Brown               2012-08-27  316  	unsigned int bypass_mask;
ca5d1b3524 Carlo Caione             2014-03-05  317  	unsigned int bypass_val_on;
ca5d1b3524 Carlo Caione             2014-03-05  318  	unsigned int bypass_val_off;
79511ed322 Mark Brown               2012-06-27  319  
79511ed322 Mark Brown               2012-06-27  320  	unsigned int enable_time;
871f565055 Guodong Xu               2014-08-13  321  
871f565055 Guodong Xu               2014-08-13  322  	unsigned int off_on_delay;
87e1e0f29f Javier Martinez Canillas 2014-11-10  323  
87e1e0f29f Javier Martinez Canillas 2014-11-10  324  	unsigned int (*of_map_mode)(unsigned int mode);
571a354b15 Liam Girdwood            2008-04-30 @325  };
571a354b15 Liam Girdwood            2008-04-30  326  
c172708d38 Mark Brown               2012-04-04  327  /**
c172708d38 Mark Brown               2012-04-04  328   * struct regulator_config - Dynamic regulator descriptor

:::::: The code at line 325 was first introduced by commit
:::::: 571a354b1542a274d88617e1f6703f3fe7a517f1 regulator: regulator driver interface

:::::: TO: Liam Girdwood <lg@opensource.wolfsonmicro.com>
:::::: CC: Liam Girdwood <lg@opensource.wolfsonmicro.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6122 bytes --]

Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

Le jeudi 21 janvier 2016 à 20:24 +0000, Mark Brown a écrit :
> Provide a flag auto_runtime_pm in the regulator_desc which causes the
> regulator core to take a runtime PM reference to a regulator while it
> is enabled. This helps integration with chip wide power management
> for
> auxiliary PMICs, they may be able to implement chip wide power
> savings
> if nothing on the PMIC is in use.

Thanks for working on this!

I'm having a major drawback on my development unit (fried it
accidentally) so I'm unable to test this with the LP8720 regulator on
the Optimus Black, but I'll manage to get a working development unit
soon.

I'll let you know how it goes. Feel free to merge this before I do the
work on the lp8720 regulator, though.

> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> 
> Not tested at all yet, pushing out for testing by others who have
> devices that could benefit from this.
> 
>  drivers/regulator/core.c         | 20 ++++++++++++++++++--
>  include/linux/regulator/driver.h |  1 +
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 3308c6bb83db..968ff3081e6c 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -26,6 +26,7 @@
>  #include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/of_regulator.h>
>  #include <linux/regulator/consumer.h>
> @@ -2059,17 +2060,23 @@ static int _regulator_do_enable(struct
> regulator_dev *rdev)
>  		}
>  	}
>  
> +	if (rdev->desc->auto_runtime_pm) {
> +		ret = pm_runtime_get_sync(rdev->dev.parent);
> +		if (ret < 0)
> +			goto err;
> +	}
> +
>  	if (rdev->ena_pin) {
>  		if (!rdev->ena_gpio_state) {
>  			ret = regulator_ena_gpio_ctrl(rdev, true);
>  			if (ret < 0)
> -				return ret;
> +				goto err_pm;
>  			rdev->ena_gpio_state = 1;
>  		}
>  	} else if (rdev->desc->ops->enable) {
>  		ret = rdev->desc->ops->enable(rdev);
>  		if (ret < 0)
> -			return ret;
> +			goto err_pm;
>  	} else {
>  		return -EINVAL;
>  	}
> @@ -2084,6 +2091,12 @@ static int _regulator_do_enable(struct
> regulator_dev *rdev)
>  	trace_regulator_enable_complete(rdev_get_name(rdev));
>  
>  	return 0;
> +
> +err_pm:
> +	if (rdev->desc->auto_runtime_pm)
> +		pm_runtime_put_autosuspend(rdev->dev.parent);
> +err:
> +	return ret;
>  }
>  
>  /* locks held by regulator_enable() */
> @@ -2177,6 +2190,9 @@ static int _regulator_do_disable(struct
> regulator_dev *rdev)
>  			return ret;
>  	}
>  
> +	if (rdev->desc->auto_runtime_pm)
> +		pm_runtime_put_autosuspend(rdev->dev.parent);
> +
>  	/* cares about last_off_jiffy only if off_on_delay is
> required by
>  	 * device.
>  	 */
> diff --git a/include/linux/regulator/driver.h
> b/include/linux/regulator/driver.h
> index 3ac0f306f033..dccea032a143 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -282,6 +282,7 @@ struct regulator_desc {
>  			    struct regulator_config *);
>  	int id;
>  	unsigned int continuous_voltage_range:1;
> +	unsigned int auto_runtime_pm:1;
>  	unsigned n_voltages;
>  	const struct regulator_ops *ops;
>  	int irq;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 11141 bytes --]

Hi,

Le jeudi 21 janvier 2016 à 20:24 +0000, Mark Brown a écrit :
> Provide a flag auto_runtime_pm in the regulator_desc which causes the
> regulator core to take a runtime PM reference to a regulator while it
> is enabled. This helps integration with chip wide power management for
> auxiliary PMICs, they may be able to implement chip wide power savings
> if nothing on the PMIC is in use.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> 
> Not tested at all yet, pushing out for testing by others who have
> devices that could benefit from this.

Thanks for pushing this, I didn't think it would be that easy to
implement.

I have tried using those patches on top of my Optimus Black support
series (as of v2), on top of 4.5.0-rc2. As-is, it fails at run-time, due
to badly-ordered calls to runtime pm functions. Here is the relevant
part of the kernel log:

[    1.943359] regulator disable, pm autosuspend
[    1.949615] regulator enable, pm sync
[    1.953491] lp872x_runtime_resume() 37
[    1.957519] lp872x_runtime_suspend() 37

Despite having the regulator disabled and enabled in that precise order,
the runtime functions are called in the opposite order, which causes the
enable pin (and thus the whole regulator chip) to be disabled.

Is that behavior intended? I suppose this is not an issue at the
regulator core level.

The diff I used to produce this is as follows. Perhaps I'm doing
something wrong?

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e39c75d..416701d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2061,6 +2061,7 @@ static int _regulator_do_enable(struct
regulator_dev *rdev)
        }
 
        if (rdev->desc->auto_runtime_pm) {
+               printk(KERN_ERR "regulator enable, pm sync\n");
                ret = pm_runtime_get_sync(rdev->dev.parent);
                if (ret < 0)
                        goto err;
@@ -2190,8 +2191,10 @@ static int _regulator_do_disable(struct
regulator_dev *rdev)
                        return ret;
        }
 
-       if (rdev->desc->auto_runtime_pm)
+       if (rdev->desc->auto_runtime_pm) {
+               printk(KERN_ERR "regulator disable, pm autosuspend\n");
                pm_runtime_put_autosuspend(rdev->dev.parent);
+       }
 
        /* cares about last_off_jiffy only if off_on_delay is required
by
         * device.
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 3e74861..5b99a34 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -15,6 +15,9 @@
 #include <linux/regmap.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/lp872x.h>
 #include <linux/regulator/driver.h>
 #include <linux/platform_device.h>
@@ -522,6 +525,7 @@ static struct regulator_desc lp8720_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo1"),
                .id = LP8720_ID_LDO1,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -536,6 +540,7 @@ static struct regulator_desc lp8720_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo2"),
                .id = LP8720_ID_LDO2,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -550,6 +555,7 @@ static struct regulator_desc lp8720_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo3"),
                .id = LP8720_ID_LDO3,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -564,6 +570,7 @@ static struct regulator_desc lp8720_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo4"),
                .id = LP8720_ID_LDO4,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp8720_ldo4_vtbl),
                .volt_table = lp8720_ldo4_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -578,6 +585,7 @@ static struct regulator_desc lp8720_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo5"),
                .id = LP8720_ID_LDO5,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -592,6 +600,7 @@ static struct regulator_desc lp8720_regulator_desc[]
= {
                .of_match = of_match_ptr("buck"),
                .id = LP8720_ID_BUCK,
                .ops = &lp8720_buck_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp8720_buck_vtbl),
                .volt_table = lp8720_buck_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -607,6 +616,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo1"),
                .id = LP8725_ID_LDO1,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -621,6 +631,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo2"),
                .id = LP8725_ID_LDO2,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -635,6 +646,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo3"),
                .id = LP8725_ID_LDO3,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -649,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo4"),
                .id = LP8725_ID_LDO4,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -663,6 +676,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("ldo5"),
                .id = LP8725_ID_LDO5,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
                .volt_table = lp872x_ldo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -677,6 +691,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("lilo1"),
                .id = LP8725_ID_LILO1,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
                .volt_table = lp8725_lilo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -691,6 +706,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("lilo2"),
                .id = LP8725_ID_LILO2,
                .ops = &lp872x_ldo_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
                .volt_table = lp8725_lilo_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -705,6 +721,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("buck1"),
                .id = LP8725_ID_BUCK1,
                .ops = &lp8725_buck_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
                .volt_table = lp8725_buck_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -717,6 +734,7 @@ static struct regulator_desc lp8725_regulator_desc[]
= {
                .of_match = of_match_ptr("buck2"),
                .id = LP8725_ID_BUCK2,
                .ops = &lp8725_buck_ops,
+               .auto_runtime_pm = 1,
                .n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
                .volt_table = lp8725_buck_vtbl,
                .type = REGULATOR_VOLTAGE,
@@ -985,9 +1003,63 @@ static int lp872x_probe(struct i2c_client *cl,
const struct i2c_device_id *id)
        if (ret)
                return ret;
 
-       return lp872x_regulator_register(lp);
+       pm_runtime_enable(&cl->dev);
+
+       ret = lp872x_regulator_register(lp);
+       if (ret) {
+               pm_runtime_disable(&cl->dev);
+               return ret;
+       }
+
+       return 0;
+}
+
+static int lp872x_runtime_suspend(struct device *dev)
+{
+       struct lp872x *lp = i2c_get_clientdata(to_i2c_client(dev));
+       struct gpio_desc *gpiod;
+       int gpio;
+
+       printk(KERN_ERR "%s() %d\n", __func__, lp->pdata->enable_gpio);
+
+       gpio = lp->pdata->enable_gpio;
+       if (!gpio_is_valid(gpio))
+               return 0;
+
+       gpiod = gpio_to_desc(gpio);
+       gpiod_set_value(gpiod, 0);
+
+       return 0;
+}
+
+static int lp872x_runtime_resume(struct device *dev)
+{
+       struct lp872x *lp = i2c_get_clientdata(to_i2c_client(dev));
+       struct gpio_desc *gpiod;
+       int gpio;
+
+       printk(KERN_ERR "%s() %d\n", __func__, lp->pdata->enable_gpio);
+
+       gpio = lp->pdata->enable_gpio;
+       if (!gpio_is_valid(gpio))
+               return 0;
+
+       gpiod = gpio_to_desc(gpio);
+       gpiod_set_value(gpiod, 0);
+
+       /* Each chip has a different enable delay. */
+       if (lp->chipid == LP8720)
+               usleep_range(LP8720_ENABLE_DELAY, 1.5 *
LP8720_ENABLE_DELAY);
+       else
+               usleep_range(LP8725_ENABLE_DELAY, 1.5 *
LP8725_ENABLE_DELAY);
+
+       return 0;
 }
 
+static const struct dev_pm_ops lp872x_pm_ops = {
+       SET_RUNTIME_PM_OPS(lp872x_runtime_suspend,
lp872x_runtime_resume, NULL)
+};
+
 static const struct of_device_id lp872x_dt_ids[] = {
        { .compatible = "ti,lp8720", },
        { .compatible = "ti,lp8725", },
@@ -1006,6 +1078,7 @@ static struct i2c_driver lp872x_driver = {
        .driver = {
                .name = "lp872x",
                .of_match_table = of_match_ptr(lp872x_dt_ids),
+               .pm = &lp872x_pm_ops,
        },
        .probe = lp872x_probe,
        .id_table = lp872x_ids,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 13257 bytes --]

Hi,

Le mardi 09 février 2016 à 21:51 +0100, Paul Kocialkowski a écrit :
> Le jeudi 21 janvier 2016 à 20:24 +0000, Mark Brown a écrit :
> > Provide a flag auto_runtime_pm in the regulator_desc which causes the
> > regulator core to take a runtime PM reference to a regulator while it
> > is enabled. This helps integration with chip wide power management for
> > auxiliary PMICs, they may be able to implement chip wide power savings
> > if nothing on the PMIC is in use.
> > 
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > ---
> > 
> > Not tested at all yet, pushing out for testing by others who have
> > devices that could benefit from this.
> 
> Thanks for pushing this, I didn't think it would be that easy to
> implement.
> 
> I have tried using those patches on top of my Optimus Black support
> series (as of v2), on top of 4.5.0-rc2. As-is, it fails at run-time, due
> to badly-ordered calls to runtime pm functions. Here is the relevant
> part of the kernel log:
> 
> [    1.943359] regulator disable, pm autosuspend
> [    1.949615] regulator enable, pm sync
> [    1.953491] lp872x_runtime_resume() 37
> [    1.957519] lp872x_runtime_suspend() 37
> 
> Despite having the regulator disabled and enabled in that precise order,
> the runtime functions are called in the opposite order, which causes the
> enable pin (and thus the whole regulator chip) to be disabled.
> 
> Is that behavior intended? I suppose this is not an issue at the
> regulator core level.

I have added more tracing today. Here is some more context:
[    2.016510] omap_hsmmc 4809c000.mmc: Got CD GPIO
[    2.021697] omap_hsmmc 4809c000.mmc: omap_device:
omap_device_enable() called from invalid state 1
[    2.031616] omap_hsmmc_disable_boot_regulators() vmmc disable
[    2.038177] omap_hsmmc_disable_boot_regulator(): regulator enable
[    2.045104] omap_hsmmc_disable_boot_regulator(): regulator disable
[    2.052368] regulator disable, pm autosuspend
[    2.056976] omap_hsmmc_disable_boot_regulators() vqmmc disable
[    2.063110] omap_hsmmc_disable_boot_regulators() pbias disable
[    2.069244] omap_hsmmc_disable_boot_regulator(): regulator enable
[    2.075653] omap_hsmmc_disable_boot_regulator(): regulator disable
[    2.082397] omap_hsmmc_enable_supply(): vmmc regulator enable
[    2.088958] mmc_regulator_set_ocr(): regulator enable
[    2.095764] regulator enable, pm sync
[    2.099639] lp872x_runtime_resume() 37
[    2.104309] lp872x_runtime_suspend() 37

This is mostly the omap_hsmmc driver enabling and disabling the
regulator to sort out the boot state (in
omap_hsmmc_disable_boot_regulators). There is a comment in that function
that explains this behavior.

Apparently, the first enable is not acted upon by the regulator core as
the regulator is enabled at boot.

So I think the question here is why runtime pm is calling the device
functions in reverse order. Any clue? Is that intended behavior?

> The diff I used to produce this is as follows. Perhaps I'm doing
> something wrong?
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e39c75d..416701d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2061,6 +2061,7 @@ static int _regulator_do_enable(struct
> regulator_dev *rdev)
>         }
>  
>         if (rdev->desc->auto_runtime_pm) {
> +               printk(KERN_ERR "regulator enable, pm sync\n");
>                 ret = pm_runtime_get_sync(rdev->dev.parent);
>                 if (ret < 0)
>                         goto err;
> @@ -2190,8 +2191,10 @@ static int _regulator_do_disable(struct
> regulator_dev *rdev)
>                         return ret;
>         }
>  
> -       if (rdev->desc->auto_runtime_pm)
> +       if (rdev->desc->auto_runtime_pm) {
> +               printk(KERN_ERR "regulator disable, pm autosuspend\n");
>                 pm_runtime_put_autosuspend(rdev->dev.parent);
> +       }
>  
>         /* cares about last_off_jiffy only if off_on_delay is required
> by
>          * device.
> diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
> index 3e74861..5b99a34 100644
> --- a/drivers/regulator/lp872x.c
> +++ b/drivers/regulator/lp872x.c
> @@ -15,6 +15,9 @@
>  #include <linux/regmap.h>
>  #include <linux/err.h>
>  #include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/lp872x.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/platform_device.h>
> @@ -522,6 +525,7 @@ static struct regulator_desc lp8720_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo1"),
>                 .id = LP8720_ID_LDO1,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -536,6 +540,7 @@ static struct regulator_desc lp8720_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo2"),
>                 .id = LP8720_ID_LDO2,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -550,6 +555,7 @@ static struct regulator_desc lp8720_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo3"),
>                 .id = LP8720_ID_LDO3,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -564,6 +570,7 @@ static struct regulator_desc lp8720_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo4"),
>                 .id = LP8720_ID_LDO4,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp8720_ldo4_vtbl),
>                 .volt_table = lp8720_ldo4_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -578,6 +585,7 @@ static struct regulator_desc lp8720_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo5"),
>                 .id = LP8720_ID_LDO5,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -592,6 +600,7 @@ static struct regulator_desc lp8720_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("buck"),
>                 .id = LP8720_ID_BUCK,
>                 .ops = &lp8720_buck_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp8720_buck_vtbl),
>                 .volt_table = lp8720_buck_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -607,6 +616,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo1"),
>                 .id = LP8725_ID_LDO1,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -621,6 +631,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo2"),
>                 .id = LP8725_ID_LDO2,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -635,6 +646,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo3"),
>                 .id = LP8725_ID_LDO3,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -649,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo4"),
>                 .id = LP8725_ID_LDO4,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -663,6 +676,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("ldo5"),
>                 .id = LP8725_ID_LDO5,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
>                 .volt_table = lp872x_ldo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -677,6 +691,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("lilo1"),
>                 .id = LP8725_ID_LILO1,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
>                 .volt_table = lp8725_lilo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -691,6 +706,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("lilo2"),
>                 .id = LP8725_ID_LILO2,
>                 .ops = &lp872x_ldo_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
>                 .volt_table = lp8725_lilo_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -705,6 +721,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("buck1"),
>                 .id = LP8725_ID_BUCK1,
>                 .ops = &lp8725_buck_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
>                 .volt_table = lp8725_buck_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -717,6 +734,7 @@ static struct regulator_desc lp8725_regulator_desc[]
> = {
>                 .of_match = of_match_ptr("buck2"),
>                 .id = LP8725_ID_BUCK2,
>                 .ops = &lp8725_buck_ops,
> +               .auto_runtime_pm = 1,
>                 .n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
>                 .volt_table = lp8725_buck_vtbl,
>                 .type = REGULATOR_VOLTAGE,
> @@ -985,9 +1003,63 @@ static int lp872x_probe(struct i2c_client *cl,
> const struct i2c_device_id *id)
>         if (ret)
>                 return ret;
>  
> -       return lp872x_regulator_register(lp);
> +       pm_runtime_enable(&cl->dev);
> +
> +       ret = lp872x_regulator_register(lp);
> +       if (ret) {
> +               pm_runtime_disable(&cl->dev);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int lp872x_runtime_suspend(struct device *dev)
> +{
> +       struct lp872x *lp = i2c_get_clientdata(to_i2c_client(dev));
> +       struct gpio_desc *gpiod;
> +       int gpio;
> +
> +       printk(KERN_ERR "%s() %d\n", __func__, lp->pdata->enable_gpio);
> +
> +       gpio = lp->pdata->enable_gpio;
> +       if (!gpio_is_valid(gpio))
> +               return 0;
> +
> +       gpiod = gpio_to_desc(gpio);
> +       gpiod_set_value(gpiod, 0);
> +
> +       return 0;
> +}
> +
> +static int lp872x_runtime_resume(struct device *dev)
> +{
> +       struct lp872x *lp = i2c_get_clientdata(to_i2c_client(dev));
> +       struct gpio_desc *gpiod;
> +       int gpio;
> +
> +       printk(KERN_ERR "%s() %d\n", __func__, lp->pdata->enable_gpio);
> +
> +       gpio = lp->pdata->enable_gpio;
> +       if (!gpio_is_valid(gpio))
> +               return 0;
> +
> +       gpiod = gpio_to_desc(gpio);
> +       gpiod_set_value(gpiod, 0);
> +
> +       /* Each chip has a different enable delay. */
> +       if (lp->chipid == LP8720)
> +               usleep_range(LP8720_ENABLE_DELAY, 1.5 *
> LP8720_ENABLE_DELAY);
> +       else
> +               usleep_range(LP8725_ENABLE_DELAY, 1.5 *
> LP8725_ENABLE_DELAY);
> +
> +       return 0;
>  }
>  
> +static const struct dev_pm_ops lp872x_pm_ops = {
> +       SET_RUNTIME_PM_OPS(lp872x_runtime_suspend,
> lp872x_runtime_resume, NULL)
> +};
> +
>  static const struct of_device_id lp872x_dt_ids[] = {
>         { .compatible = "ti,lp8720", },
>         { .compatible = "ti,lp8725", },
> @@ -1006,6 +1078,7 @@ static struct i2c_driver lp872x_driver = {
>         .driver = {
>                 .name = "lp872x",
>                 .of_match_table = of_match_ptr(lp872x_dt_ids),
> +               .pm = &lp872x_pm_ops,
>         },
>         .probe = lp872x_probe,
>         .id_table = lp872x_ids,


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

On Fri, Feb 12, 2016 at 08:24:11PM +0100, Paul Kocialkowski wrote:

> So I think the question here is why runtime pm is calling the device
> functions in reverse order. Any clue? Is that intended behavior?

Definitely not intended behaviour...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: core: Use a bitfield for continuous_voltage_range

[Joe Perches]

On Thu, 2016-01-21 at 20:24 +0000, Mark Brown wrote:
> Using a bitfield enables the compiler to lay out the structure more
> efficiently when we have other boolean flags since multiple values can
> be included in a single byte.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  include/linux/regulator/driver.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
> index 16ac9e108806..3ac0f306f033 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -281,7 +281,7 @@ struct regulator_desc {
>  			    const struct regulator_desc *,
>  			    struct regulator_config *);
>  	int id;
> -	bool continuous_voltage_range;
> +	unsigned int continuous_voltage_range:1;

Is this really valuable?

There are already padding bytes that are unused
and adding a couple more bools would be space
cost-free and more readable.

I believe that read/write of bytes is also more
efficient on some architectures than bit field
read/modify/write uses.

>  	unsigned n_voltages;
>  	const struct regulator_ops *ops;
>  	int irq;

Re: [PATCH 1/2] regulator: core: Use a bitfield for continuous_voltage_range

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

On Fri, Jan 22, 2016 at 11:15:28AM -0800, Joe Perches wrote:
> On Thu, 2016-01-21 at 20:24 +0000, Mark Brown wrote:

> > -	bool continuous_voltage_range;
> > +	unsigned int continuous_voltage_range:1;

> Is this really valuable?

> There are already padding bytes that are unused
> and adding a couple more bools would be space
> cost-free and more readable.

> I believe that read/write of bytes is also more
> efficient on some architectures than bit field
> read/modify/write uses.

It adds up when you get more flags and these are not in the least bit
performance sensitive.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: core: Use a bitfield for continuous_voltage_range

[Joe Perches]

On Fri, 2016-01-22 at 21:31 +0000, Mark Brown wrote:
> On Fri, Jan 22, 2016 at 11:15:28AM -0800, Joe Perches wrote:
> > On Thu, 2016-01-21 at 20:24 +0000, Mark Brown wrote:
> 
> > > -	bool continuous_voltage_range;
> > > +	unsigned int continuous_voltage_range:1;
> 
> > Is this really valuable?
> 
> > There are already padding bytes that are unused
> > and adding a couple more bools would be space
> > cost-free and more readable.
> 
> > I believe that read/write of bytes is also more
> > efficient on some architectures than bit field
> > read/modify/write uses.
> 
> It adds up when you get more flags and these are not in the least bit
> performance sensitive.

Sure, but intelligibility is useful too.
Do you expect to have more than 4 of these flags?

Re: [PATCH 1/2] regulator: core: Use a bitfield for continuous_voltage_range

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

On Fri, Jan 22, 2016 at 01:42:33PM -0800, Joe Perches wrote:
> On Fri, 2016-01-22 at 21:31 +0000, Mark Brown wrote:

> > It adds up when you get more flags and these are not in the least bit
> > performance sensitive.

> Sure, but intelligibility is useful too.

I'm really not sure the use of small integers for boolean flags is going
to be an especially troubling barrier for people.

> Do you expect to have more than 4 of these flags?

It's plausible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Applied "regulator: core: Use a bitfield for continuous_voltage_range" to the regulator tree

[Mark Brown]

The patch

   regulator: core: Use a bitfield for continuous_voltage_range

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From de4a54c4dfaed0604565c1b27488dce56997acc0 Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@kernel.org>
Date: Thu, 21 Jan 2016 20:19:41 +0000
Subject: [PATCH] regulator: core: Use a bitfield for continuous_voltage_range

Using a bitfield enables the compiler to lay out the structure more
efficiently when we have other boolean flags since multiple values can
be included in a single byte.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/regulator/driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index cd271e89a7e6..9ac3f9879576 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -292,7 +292,7 @@ struct regulator_desc {
 			    const struct regulator_desc *,
 			    struct regulator_config *);
 	int id;
-	bool continuous_voltage_range;
+	unsigned int continuous_voltage_range:1;
 	unsigned n_voltages;
 	const struct regulator_ops *ops;
 	int irq;
-- 
2.8.0.rc3