Hi,
Currently get_regulator returns -EPROBE_DEFER in the case of regulator supply
which have no device tree node or even if regulator which are depicted in device
tree node is defering it's registration for valid reasons.
This makes it impossible to use an regulator that registers itself after
cpufreq-cpu0 probe is complete. The reason for the same is regulator framework
fails to return appropriate error value when device tree binding is not actually
present as a node.
Once we fix that, we can then fix cpufreq-cpu0 to make intelligent decisions
based on return value.
Nishanth Menon (2):
regulator: core: return err value for regulator_get if there is no DT
binding
cpufreq: cpufreq-cpu0: defer probe when regulator is not ready
drivers/cpufreq/cpufreq-cpu0.c | 20 ++++++++++++++------
drivers/regulator/core.c | 4 ++--
2 files changed, 16 insertions(+), 8 deletions(-)
Series is based off tag v3.9-rc5 (also applies on rafael's bleeding-edge branch)
Series is also available at:
https://github.com/nmenon/linux-2.6-playground/commits/push/cpufreq-regulator-fixing-v1
git link: git://github.com/nmenon/linux-2.6-playground.git
branch: push/cpufreq-regulator-fixing-v1
Test scenarios(performed on 3.9-rc3 on beagle-XM platform):
test #1: cpu0-supply binding is not present:
http://pastebin.com/0SSC1HAw
test #2: cpu0-supply binding is present, but regulator defers probing:
http://pastebin.com/HCSJqtRK
test #3: cpu0-supply binding is present, but regulator never registers (bug in DT binding)
http://pastebin.com/guUwQcGW
test #4: cpu0-supply binding is present, regulator is available:
http://pastebin.com/hsbBdxiz
Sub Note: This series might not be important for 3.9, considering the regulator
bug has been around since last year, however, it might be nice to have it fixed
up in 3.10 sometime.
Regards,
Nishanth Menon
--
1.7.9.5
commit 6d191a5fc7a969d972f1681e1c23781aecb06a61
(regulator: core: Don't defer probe if there's no DT binding for a supply)
Attempted to differentiate between regulator_get() with an actual
DT binding for the supply and when there is none to avoid unnecessary
deferal.
In cases where a driver, such as cpufreq driver, need to handle (based
on platform) DT nodes which may or may not have regulator supply
binding, it is necessary to differentiate the return value. When there
is regulator supply node, defering the probe is beneficial
as sequence of dependent regulator probe may not be predictable.
However, where is no regulator supply binding, it can operate without
the same as needed.
So, to provide an appropriate return result, when we cannot find an
regulator in regulator_dev_lookup and we run out of options,
use the regulator_dev_lookup result. This helps the caller to help make
informed decision.
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/regulator/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e3661c2..139d86a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1229,7 +1229,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
struct regulator_dev *rdev;
struct regulator *regulator = ERR_PTR(-EPROBE_DEFER);
const char *devname = NULL;
- int ret;
+ int ret = 0;
if (id == NULL) {
pr_err("get() with no identifier\n");
@@ -1266,7 +1266,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
#endif
mutex_unlock(®ulator_list_mutex);
- return regulator;
+ return ret ? ERR_PTR(ret) : regulator;
found:
if (rdev->exclusive) {
--
1.7.9.5
regulator_get will now return -EPROBE_DEFER when the cpu0-supply
node is present, but the regulator is not yet registered.
It is possible for this to occur when the regulator registration
by itself might be defered due to some dependent interface not
yet instantiated. For example: an regulator which uses I2C and GPIO
might need both systems available before proceeding, in this case,
the regulator might defer it's registration.
However, the cpufreq-cpu0 driver assumes that any un-successful return
result is equivalent of failure.
When the regulator_get returns failure other than -EPROBE_DEFER, it
makes sense to assume that supply node is not present and proceed
with the assumption that only clock control is necessary in the
platform.
With this change, we can now handle the following conditions:
a) cpu0-supply binding is not present, regulator_get will return
appropriate error result, resulting in cpufreq-cpu0 driver controlling
just the clock.
b) cpu0-supply binding is present, regulator_get returns -EPROBE_DEFER,
we retry resulting in cpufreq-cpu0 driver registering later once the
regulator is available.
c) cpu0-supply binding is present, regulator_get returns -EPROBE_DEFER,
however, regulator never registers, we retry until cpufreq-cpu0
driver fails to register pointing at device tree information bug.
However, in this case, the fact that cpufreq-cpu0 operates with clock
only when the DT binding clearly indicates need of a supply is a bug of
it's own.
d) cpu0-supply gets an regulator at probe - cpufreq-cpu0 driver controls
both the clock and regulator
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Viresh Kumar <[email protected]>
Cc: Shawn Guo <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
drivers/cpufreq/cpufreq-cpu0.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
index 4e5b7fb..55fec7c 100644
--- a/drivers/cpufreq/cpufreq-cpu0.c
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -194,6 +194,20 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)
cpu_dev = &pdev->dev;
cpu_dev->of_node = np;
+ cpu_reg = devm_regulator_get(cpu_dev, "cpu0");
+ if (IS_ERR(cpu_reg)) {
+ /*
+ * If cpu0 regulator supply node is present, but regulator is
+ * not yet registered, we should try defering probe.
+ */
+ if (PTR_ERR(cpu_reg) == -EPROBE_DEFER) {
+ dev_err(cpu_dev, "cpu0 regulator not ready, retry\n");
+ return -EPROBE_DEFER;
+ }
+ pr_err("failed to get cpu0 regulator: %ld\n", PTR_ERR(cpu_reg));
+ cpu_reg = NULL;
+ }
+
cpu_clk = devm_clk_get(cpu_dev, NULL);
if (IS_ERR(cpu_clk)) {
ret = PTR_ERR(cpu_clk);
@@ -201,12 +215,6 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)
goto out_put_node;
}
- cpu_reg = devm_regulator_get(cpu_dev, "cpu0");
- if (IS_ERR(cpu_reg)) {
- pr_warn("failed to get cpu0 regulator\n");
- cpu_reg = NULL;
- }
-
ret = of_init_opp_table(cpu_dev);
if (ret) {
pr_err("failed to init OPP table: %d\n", ret);
--
1.7.9.5
On 5 April 2013 09:51, Nishanth Menon <[email protected]> wrote:
> Currently get_regulator returns -EPROBE_DEFER in the case of regulator supply
> which have no device tree node or even if regulator which are depicted in device
> tree node is defering it's registration for valid reasons.
>
> This makes it impossible to use an regulator that registers itself after
> cpufreq-cpu0 probe is complete. The reason for the same is regulator framework
> fails to return appropriate error value when device tree binding is not actually
> present as a node.
>
> Once we fix that, we can then fix cpufreq-cpu0 to make intelligent decisions
> based on return value.
>
> Nishanth Menon (2):
> regulator: core: return err value for regulator_get if there is no DT
> binding
> cpufreq: cpufreq-cpu0: defer probe when regulator is not ready
>
> drivers/cpufreq/cpufreq-cpu0.c | 20 ++++++++++++++------
> drivers/regulator/core.c | 4 ++--
> 2 files changed, 16 insertions(+), 8 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
On Thu, Apr 04, 2013 at 11:21:46PM -0500, Nishanth Menon wrote:
> Nishanth Menon (2):
> regulator: core: return err value for regulator_get if there is no DT
> binding
> cpufreq: cpufreq-cpu0: defer probe when regulator is not ready
For both,
Acked-by: Shawn Guo <[email protected]>
On Thu, Apr 04, 2013 at 11:21:47PM -0500, Nishanth Menon wrote:
> commit 6d191a5fc7a969d972f1681e1c23781aecb06a61
> (regulator: core: Don't defer probe if there's no DT binding for a supply)
>
> Attempted to differentiate between regulator_get() with an actual
> DT binding for the supply and when there is none to avoid unnecessary
> deferal.
So, this is an extremely long and hence difficult to understand and
follow commit message which manages to miss out mentioning the core
issue which is that we're ignoring the return value from lookup_dev().
I had to actually look at the code to understand.
What should be being said here is that the ret value supplied by
regulator_dev_lookup() is being ignored by _regulator_get().
> mutex_unlock(®ulator_list_mutex);
> - return regulator;
> + return ret ? ERR_PTR(ret) : regulator;
Please implement this so it looks like the rest of the function -
everywhere else in the function we just make regulator an ERR_PTR() and
goto out, we should do the same.
commit 6d191a5fc7a969d972f1681e1c23781aecb06a61
(regulator: core: Don't defer probe if there's no DT binding for a supply)
Attempted to differentiate between regulator_get() with an actual
DT binding for the supply and when there is none to avoid unnecessary
deferal.
However, ret value supplied by regulator_dev_lookup() is being
ignored by regulator_get(). So, exit with the appropriate return value.
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
based on v3.9-rc7
Changes in v2 (review comments update):
- simplified commit log
- error handling inline with rest of function
Available at: https://github.com/nmenon/linux-2.6-playground/commits/push/cpufreq-regulator-fixing-v2
V1:
https://patchwork.kernel.org/patch/2396571/
(original depending cpufreq patch: https://patchwork.kernel.org/patch/2396541/
- no change expected- so not reposting unless requested)
drivers/regulator/core.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e3661c2..cfe8b84 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1229,7 +1229,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
struct regulator_dev *rdev;
struct regulator *regulator = ERR_PTR(-EPROBE_DEFER);
const char *devname = NULL;
- int ret;
+ int ret = 0;
if (id == NULL) {
pr_err("get() with no identifier\n");
@@ -1245,6 +1245,15 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
if (rdev)
goto found;
+ /*
+ * If we have return value from dev_lookup fail, we do not expect to
+ * succeed, so, quit with appropriate error value
+ */
+ if (ret) {
+ regulator = ERR_PTR(ret);
+ goto out;
+ }
+
if (board_wants_dummy_regulator) {
rdev = dummy_regulator_rdev;
goto found;
--
1.7.9.5