From: Paul Kocialkowski <contact@paulk.fr>
To: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
linux-kernel@vger.kernel.org,
"Rafael J. Wysocki" <rjw@rjwysocki.net>
Subject: Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support
Date: Tue, 09 Feb 2016 21:51:56 +0100 [thread overview]
Message-ID: <1455051116.2757.15.camel@collins> (raw)
In-Reply-To: <1453407851-8039-2-git-send-email-broonie@kernel.org>
[-- 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 --]
next prev parent reply other threads:[~2016-02-09 20:52 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-21 20:24 [PATCH 1/2] regulator: core: Use a bitfield for continuous_voltage_range Mark Brown
2016-01-21 20:24 ` [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support Mark Brown
2016-01-21 21:49 ` kbuild test robot
2016-01-29 12:02 ` Paul Kocialkowski
2016-02-09 20:51 ` Paul Kocialkowski [this message]
2016-02-12 19:24 ` Paul Kocialkowski
2016-02-26 2:13 ` Mark Brown
2016-01-22 19:15 ` [PATCH 1/2] regulator: core: Use a bitfield for continuous_voltage_range Joe Perches
2016-01-22 21:31 ` Mark Brown
2016-01-22 21:42 ` Joe Perches
2016-01-23 15:16 ` Mark Brown
2016-04-21 16:11 ` Applied "regulator: core: Use a bitfield for continuous_voltage_range" to the regulator tree Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1455051116.2757.15.camel@collins \
--to=contact@paulk.fr \
--cc=broonie@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome