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

From: Paul Kocialkowski
Date: Tue Feb 09 2016 - 15:52:10 EST

Next message: Andrew Morton: "Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous"
Previous message: Christoffer Dall: "Re: [PATCH 3/5] irqchip/gic-v2: Parse and export virtual GIC information"
Next in thread: Paul Kocialkowski: "Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> ---
>
> 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: signature.asc
Description: This is a digitally signed message part

Next message: Andrew Morton: "Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous"
Previous message: Christoffer Dall: "Re: [PATCH 3/5] irqchip/gic-v2: Parse and export virtual GIC information"
Next in thread: Paul Kocialkowski: "Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]