* [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
@ 2015-10-18 11:23 Heiner Kallweit
2015-10-20 13:31 ` Sascha Hauer
0 siblings, 1 reply; 6+ messages in thread
From: Heiner Kallweit @ 2015-10-18 11:23 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar; +Cc: linux-pm
Properly handle the case that regulator_get_optional returns -EPROBE_DEFER
and ignore other errors.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 0bb33dc..67dc81c 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -67,7 +67,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
/* scaling up? scale voltage before frequency */
if (new_freq > old_freq) {
- if (!IS_ERR(pu_reg)) {
+ if (pu_reg) {
ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
if (ret) {
dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
@@ -124,7 +124,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
ret = 0;
}
- if (!IS_ERR(pu_reg)) {
+ if (pu_reg) {
ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
if (ret) {
dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
@@ -195,6 +195,13 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
}
pu_reg = regulator_get_optional(cpu_dev, "pu");
+ if (IS_ERR(pu_reg)) {
+ if (PTR_ERR(pu_reg) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto put_reg;
+ } else
+ pu_reg = NULL;
+ }
soc_reg = regulator_get(cpu_dev, "soc");
if (IS_ERR(soc_reg)) {
@@ -285,7 +292,7 @@ soc_opp_out:
ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
if (ret > 0)
transition_latency += ret * 1000;
- if (!IS_ERR(pu_reg)) {
+ if (pu_reg) {
ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
if (ret > 0)
transition_latency += ret * 1000;
@@ -325,7 +332,7 @@ out_free_opp:
put_reg:
if (!IS_ERR(arm_reg))
regulator_put(arm_reg);
- if (!IS_ERR(pu_reg))
+ if (!IS_ERR_OR_NULL(pu_reg))
regulator_put(pu_reg);
if (!IS_ERR(soc_reg))
regulator_put(soc_reg);
@@ -351,7 +358,7 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
if (free_opp)
of_free_opp_table(cpu_dev);
regulator_put(arm_reg);
- if (!IS_ERR(pu_reg))
+ if (pu_reg)
regulator_put(pu_reg);
regulator_put(soc_reg);
clk_put(arm_clk);
--
2.6.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
2015-10-18 11:23 [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling Heiner Kallweit
@ 2015-10-20 13:31 ` Sascha Hauer
2015-10-20 13:44 ` Sascha Hauer
0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2015-10-20 13:31 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Rafael J. Wysocki, Viresh Kumar, linux-pm
On Tue, Oct 20, 2015 at 03:12:24PM +0200, Heiner Kallweit wrote:
> Properly handle the case that regulator_get_optional returns -EPROBE_DEFER
> and ignore other errors.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 0bb33dc..67dc81c 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -67,7 +67,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>
> /* scaling up? scale voltage before frequency */
> if (new_freq > old_freq) {
> - if (!IS_ERR(pu_reg)) {
> + if (pu_reg) {
> ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> if (ret) {
> dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
> @@ -124,7 +124,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
> ret = 0;
> }
> - if (!IS_ERR(pu_reg)) {
> + if (pu_reg) {
> ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> if (ret) {
> dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
> @@ -195,6 +195,13 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> }
>
> pu_reg = regulator_get_optional(cpu_dev, "pu");
> + if (IS_ERR(pu_reg)) {
> + if (PTR_ERR(pu_reg) == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto put_reg;
> + } else
> + pu_reg = NULL;
> + }
>
> soc_reg = regulator_get(cpu_dev, "soc");
> if (IS_ERR(soc_reg)) {
> @@ -285,7 +292,7 @@ soc_opp_out:
> ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> if (ret > 0)
> transition_latency += ret * 1000;
> - if (!IS_ERR(pu_reg)) {
> + if (pu_reg) {
> ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> if (ret > 0)
> transition_latency += ret * 1000;
> @@ -325,7 +332,7 @@ out_free_opp:
> put_reg:
> if (!IS_ERR(arm_reg))
> regulator_put(arm_reg);
> - if (!IS_ERR(pu_reg))
> + if (!IS_ERR_OR_NULL(pu_reg))
> regulator_put(pu_reg);
> if (!IS_ERR(soc_reg))
> regulator_put(soc_reg);
These checks do not work properly. We assume here that the pointers are
correctly initialized, but this is only true for the first probe. After
a -EPROBE_DEFER the pointers are still initialized from the previous probe.
We either have to jump between the above IS_ERR checks when the middle
regulator failed or explicitly set the *_reg variables to some error
value on probe enter.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
2015-10-20 13:31 ` Sascha Hauer
@ 2015-10-20 13:44 ` Sascha Hauer
2015-10-20 14:36 ` Heiner Kallweit
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Sascha Hauer @ 2015-10-20 13:44 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Rafael J. Wysocki, Viresh Kumar, linux-pm
On Tue, Oct 20, 2015 at 03:31:02PM +0200, Sascha Hauer wrote:
> On Tue, Oct 20, 2015 at 03:12:24PM +0200, Heiner Kallweit wrote:
> > Properly handle the case that regulator_get_optional returns -EPROBE_DEFER
> > and ignore other errors.
> >
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> > drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
> > 1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> > index 0bb33dc..67dc81c 100644
> > --- a/drivers/cpufreq/imx6q-cpufreq.c
> > +++ b/drivers/cpufreq/imx6q-cpufreq.c
> > @@ -67,7 +67,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> >
> > /* scaling up? scale voltage before frequency */
> > if (new_freq > old_freq) {
> > - if (!IS_ERR(pu_reg)) {
> > + if (pu_reg) {
> > ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> > if (ret) {
> > dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
> > @@ -124,7 +124,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> > dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
> > ret = 0;
> > }
> > - if (!IS_ERR(pu_reg)) {
> > + if (pu_reg) {
> > ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> > if (ret) {
> > dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
> > @@ -195,6 +195,13 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> > }
> >
> > pu_reg = regulator_get_optional(cpu_dev, "pu");
> > + if (IS_ERR(pu_reg)) {
> > + if (PTR_ERR(pu_reg) == -EPROBE_DEFER) {
> > + ret = -EPROBE_DEFER;
> > + goto put_reg;
> > + } else
> > + pu_reg = NULL;
> > + }
> >
> > soc_reg = regulator_get(cpu_dev, "soc");
> > if (IS_ERR(soc_reg)) {
> > @@ -285,7 +292,7 @@ soc_opp_out:
> > ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> > if (ret > 0)
> > transition_latency += ret * 1000;
> > - if (!IS_ERR(pu_reg)) {
> > + if (pu_reg) {
> > ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> > if (ret > 0)
> > transition_latency += ret * 1000;
> > @@ -325,7 +332,7 @@ out_free_opp:
> > put_reg:
> > if (!IS_ERR(arm_reg))
> > regulator_put(arm_reg);
> > - if (!IS_ERR(pu_reg))
> > + if (!IS_ERR_OR_NULL(pu_reg))
> > regulator_put(pu_reg);
> > if (!IS_ERR(soc_reg))
> > regulator_put(soc_reg);
>
> These checks do not work properly. We assume here that the pointers are
> correctly initialized, but this is only true for the first probe. After
> a -EPROBE_DEFER the pointers are still initialized from the previous probe.
> We either have to jump between the above IS_ERR checks when the middle
> regulator failed or explicitly set the *_reg variables to some error
> value on probe enter.
I think my favourite approach would be to go in the direction as
indicated below. At least it would be easier to review for correctness
than the current mix of reverse-cleanup and !IS_ERR checks at the end of probe().
Sascha
------------------------------------8<--------------------------------
>From 7661b5b5407bfa4d29ba48059819937daaa2fa32 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Tue, 20 Oct 2015 15:39:05 +0200
Subject: [PATCH] cpufreq: imx6q: cleanup resource allocation
Put all regulator and clock allocation into a separate function and
create a corresponding cleanup function. This is easier to review for
correctness than the current mix of reverse-cleanup and !IS_ERR
checks at the end of probe().
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/cpufreq/imx6q-cpufreq.c | 168 +++++++++++++++++++++++++---------------
1 file changed, 104 insertions(+), 64 deletions(-)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index ef1fa81..a049d43 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -176,6 +176,101 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
.attr = cpufreq_generic_attr,
};
+static int imx6q_cpufreq_get_resources(struct platform_device *pdev)
+{
+ /*
+ * Do not use devm_* here. The resources are bound to the
+ * cpu_dev and not to this drivers platform_device.
+ */
+ arm_clk = clk_get(cpu_dev, "arm");
+ if (IS_ERR(arm_clk))
+ return PTR_ERR(arm_clk);
+
+ pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
+ if (IS_ERR(pll1_sys_clk))
+ return PTR_ERR(pll1_sys_clk);
+
+ pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
+ if (IS_ERR(pll1_sw_clk))
+ return PTR_ERR(pll1_sw_clk);
+
+ step_clk = clk_get(cpu_dev, "step");
+ if (IS_ERR(step_clk))
+ return PTR_ERR(step_clk);
+
+ pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
+ if (IS_ERR(pll2_pfd2_396m_clk))
+ return PTR_ERR(pll2_pfd2_396m_clk);
+
+ if (of_machine_is_compatible("fsl,imx6ul")) {
+ pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
+ if (IS_ERR(pll2_bus_clk))
+ return PTR_ERR(pll2_bus_clk);
+
+ secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
+ if (IS_ERR(secondary_sel_clk))
+ return PTR_ERR(secondary_sel_clk);
+ }
+
+ arm_reg = regulator_get(cpu_dev, "arm");
+ if (IS_ERR(arm_reg))
+ return PTR_ERR(arm_reg);
+
+ pu_reg = regulator_get_optional(cpu_dev, "pu");
+
+ soc_reg = regulator_get(cpu_dev, "soc");
+ if (IS_ERR(soc_reg))
+ return PTR_ERR(soc_reg);
+
+ return 0;
+}
+
+
+static int imx6q_cpufreq_put_resources(struct platform_device *pdev)
+{
+ if (!IS_ERR_OR_NULL(arm_reg))
+ regulator_put(arm_reg);
+ arm_reg = NULL;
+
+ if (!IS_ERR_OR_NULL(pu_reg))
+ regulator_put(pu_reg);
+ pu_reg = NULL;
+
+ if (!IS_ERR_OR_NULL(soc_reg))
+ regulator_put(soc_reg);
+ soc_reg = NULL;
+
+ if (!IS_ERR_OR_NULL(arm_clk))
+ clk_put(arm_clk);
+ arm_clk = NULL;
+
+ if (!IS_ERR_OR_NULL(pll1_sys_clk))
+ clk_put(pll1_sys_clk);
+ pll1_sys_clk = NULL;
+
+ if (!IS_ERR_OR_NULL(pll1_sw_clk))
+ clk_put(pll1_sw_clk);
+ pll1_sw_clk = NULL;
+
+ if (!IS_ERR_OR_NULL(step_clk))
+ clk_put(step_clk);
+ step_clk = NULL;
+
+ if (!IS_ERR_OR_NULL(pll2_pfd2_396m_clk))
+ clk_put(pll2_pfd2_396m_clk);
+ pll2_pfd2_396m_clk = NULL;
+
+ if (!IS_ERR_OR_NULL(pll2_bus_clk))
+ clk_put(pll2_bus_clk);
+ pll2_bus_clk = NULL;
+
+ if (!IS_ERR_OR_NULL(secondary_sel_clk))
+ clk_put(secondary_sel_clk);
+ secondary_sel_clk = NULL;
+
+
+}
+
static int imx6q_cpufreq_probe(struct platform_device *pdev)
{
struct device_node *np;
@@ -198,36 +293,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
return -ENOENT;
}
- arm_clk = clk_get(cpu_dev, "arm");
- pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
- pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
- step_clk = clk_get(cpu_dev, "step");
- pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
- if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
- IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
- dev_err(cpu_dev, "failed to get clocks\n");
- ret = -ENOENT;
- goto put_clk;
- }
-
- if (of_machine_is_compatible("fsl,imx6ul")) {
- pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
- secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
- if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
- dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
- ret = -ENOENT;
- goto put_clk;
- }
- }
-
- arm_reg = regulator_get(cpu_dev, "arm");
- pu_reg = regulator_get_optional(cpu_dev, "pu");
- soc_reg = regulator_get(cpu_dev, "soc");
- if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
- dev_err(cpu_dev, "failed to get regulators\n");
- ret = -ENOENT;
- goto put_reg;
- }
+ ret = imx6q_cpufreq_get_resources(pdev);
+ if (ret)
+ goto drop_resources;
/*
* We expect an OPP table supplied by platform.
@@ -239,7 +307,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
ret = dev_pm_opp_of_add_table(cpu_dev);
if (ret < 0) {
dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
- goto put_reg;
+ goto drop_resources;
}
/* Because we have added the OPPs here, we must free them */
@@ -347,28 +415,9 @@ free_freq_table:
out_free_opp:
if (free_opp)
dev_pm_opp_of_remove_table(cpu_dev);
-put_reg:
- if (!IS_ERR(arm_reg))
- regulator_put(arm_reg);
- if (!IS_ERR(pu_reg))
- regulator_put(pu_reg);
- if (!IS_ERR(soc_reg))
- regulator_put(soc_reg);
-put_clk:
- if (!IS_ERR(arm_clk))
- clk_put(arm_clk);
- if (!IS_ERR(pll1_sys_clk))
- clk_put(pll1_sys_clk);
- if (!IS_ERR(pll1_sw_clk))
- clk_put(pll1_sw_clk);
- if (!IS_ERR(step_clk))
- clk_put(step_clk);
- if (!IS_ERR(pll2_pfd2_396m_clk))
- clk_put(pll2_pfd2_396m_clk);
- if (!IS_ERR(pll2_bus_clk))
- clk_put(pll2_bus_clk);
- if (!IS_ERR(secondary_sel_clk))
- clk_put(secondary_sel_clk);
+drop_resources:
+ imx6q_cpufreq_put_resources();
+
of_node_put(np);
return ret;
}
@@ -379,17 +428,8 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
if (free_opp)
dev_pm_opp_of_remove_table(cpu_dev);
- regulator_put(arm_reg);
- if (!IS_ERR(pu_reg))
- regulator_put(pu_reg);
- regulator_put(soc_reg);
- clk_put(arm_clk);
- clk_put(pll1_sys_clk);
- clk_put(pll1_sw_clk);
- clk_put(step_clk);
- clk_put(pll2_pfd2_396m_clk);
- clk_put(pll2_bus_clk);
- clk_put(secondary_sel_clk);
+
+ imx6q_cpufreq_put_resources();
return 0;
}
--
2.6.1
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
2015-10-20 13:44 ` Sascha Hauer
@ 2015-10-20 14:36 ` Heiner Kallweit
2015-10-20 15:55 ` [PATCH] cpufreq: imx6q: cleanup resource allocation kbuild test robot
2015-10-21 11:02 ` [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling Viresh Kumar
2 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2015-10-20 14:36 UTC (permalink / raw)
To: Sascha Hauer; +Cc: Heiner Kallweit, Rafael J. Wysocki, Viresh Kumar, linux-pm
On Tue, Oct 20, 2015 at 3:44 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Oct 20, 2015 at 03:31:02PM +0200, Sascha Hauer wrote:
>> On Tue, Oct 20, 2015 at 03:12:24PM +0200, Heiner Kallweit wrote:
>> > Properly handle the case that regulator_get_optional returns -EPROBE_DEFER
>> > and ignore other errors.
>> >
>> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> > ---
>> > drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
>> > 1 file changed, 12 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
>> > index 0bb33dc..67dc81c 100644
>> > --- a/drivers/cpufreq/imx6q-cpufreq.c
>> > +++ b/drivers/cpufreq/imx6q-cpufreq.c
>> > @@ -67,7 +67,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>> >
>> > /* scaling up? scale voltage before frequency */
>> > if (new_freq > old_freq) {
>> > - if (!IS_ERR(pu_reg)) {
>> > + if (pu_reg) {
>> > ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
>> > if (ret) {
>> > dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
>> > @@ -124,7 +124,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>> > dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
>> > ret = 0;
>> > }
>> > - if (!IS_ERR(pu_reg)) {
>> > + if (pu_reg) {
>> > ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
>> > if (ret) {
>> > dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
>> > @@ -195,6 +195,13 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>> > }
>> >
>> > pu_reg = regulator_get_optional(cpu_dev, "pu");
>> > + if (IS_ERR(pu_reg)) {
>> > + if (PTR_ERR(pu_reg) == -EPROBE_DEFER) {
>> > + ret = -EPROBE_DEFER;
>> > + goto put_reg;
>> > + } else
>> > + pu_reg = NULL;
>> > + }
>> >
>> > soc_reg = regulator_get(cpu_dev, "soc");
>> > if (IS_ERR(soc_reg)) {
>> > @@ -285,7 +292,7 @@ soc_opp_out:
>> > ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
>> > if (ret > 0)
>> > transition_latency += ret * 1000;
>> > - if (!IS_ERR(pu_reg)) {
>> > + if (pu_reg) {
>> > ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
>> > if (ret > 0)
>> > transition_latency += ret * 1000;
>> > @@ -325,7 +332,7 @@ out_free_opp:
>> > put_reg:
>> > if (!IS_ERR(arm_reg))
>> > regulator_put(arm_reg);
>> > - if (!IS_ERR(pu_reg))
>> > + if (!IS_ERR_OR_NULL(pu_reg))
>> > regulator_put(pu_reg);
>> > if (!IS_ERR(soc_reg))
>> > regulator_put(soc_reg);
>>
>> These checks do not work properly. We assume here that the pointers are
>> correctly initialized, but this is only true for the first probe. After
>> a -EPROBE_DEFER the pointers are still initialized from the previous probe.
>> We either have to jump between the above IS_ERR checks when the middle
>> regulator failed or explicitly set the *_reg variables to some error
>> value on probe enter.
>
> I think my favourite approach would be to go in the direction as
> indicated below. At least it would be easier to review for correctness
> than the current mix of reverse-cleanup and !IS_ERR checks at the end of probe().
>
Agree. Just two remarks regarding pu_reg:
- IMHO the call to regulator_get_optional can return -EPROBE_DEFER as
well and that
would need to be checked.
- I don't like this "!IS_ERR()" too much every time pu_reg is used.
That's what I addressed
in my second patch. If you share the opinion this could be addressed
in a separate patch
to separate the fix from the improvement.
> Sascha
>
> ------------------------------------8<--------------------------------
>
> From 7661b5b5407bfa4d29ba48059819937daaa2fa32 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Tue, 20 Oct 2015 15:39:05 +0200
> Subject: [PATCH] cpufreq: imx6q: cleanup resource allocation
>
> Put all regulator and clock allocation into a separate function and
> create a corresponding cleanup function. This is easier to review for
> correctness than the current mix of reverse-cleanup and !IS_ERR
> checks at the end of probe().
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> drivers/cpufreq/imx6q-cpufreq.c | 168 +++++++++++++++++++++++++---------------
> 1 file changed, 104 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index ef1fa81..a049d43 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -176,6 +176,101 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
> .attr = cpufreq_generic_attr,
> };
>
> +static int imx6q_cpufreq_get_resources(struct platform_device *pdev)
> +{
> + /*
> + * Do not use devm_* here. The resources are bound to the
> + * cpu_dev and not to this drivers platform_device.
> + */
> + arm_clk = clk_get(cpu_dev, "arm");
> + if (IS_ERR(arm_clk))
> + return PTR_ERR(arm_clk);
> +
> + pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> + if (IS_ERR(pll1_sys_clk))
> + return PTR_ERR(pll1_sys_clk);
> +
> + pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> + if (IS_ERR(pll1_sw_clk))
> + return PTR_ERR(pll1_sw_clk);
> +
> + step_clk = clk_get(cpu_dev, "step");
> + if (IS_ERR(step_clk))
> + return PTR_ERR(step_clk);
> +
> + pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> + if (IS_ERR(pll2_pfd2_396m_clk))
> + return PTR_ERR(pll2_pfd2_396m_clk);
> +
> + if (of_machine_is_compatible("fsl,imx6ul")) {
> + pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
> + if (IS_ERR(pll2_bus_clk))
> + return PTR_ERR(pll2_bus_clk);
> +
> + secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
> + if (IS_ERR(secondary_sel_clk))
> + return PTR_ERR(secondary_sel_clk);
> + }
> +
> + arm_reg = regulator_get(cpu_dev, "arm");
> + if (IS_ERR(arm_reg))
> + return PTR_ERR(arm_reg);
> +
> + pu_reg = regulator_get_optional(cpu_dev, "pu");
I think this call can return -EPROBE_DEFER as well.
> +
> + soc_reg = regulator_get(cpu_dev, "soc");
> + if (IS_ERR(soc_reg))
> + return PTR_ERR(soc_reg);
> +
> + return 0;
> +}
> +
> +
> +static int imx6q_cpufreq_put_resources(struct platform_device *pdev)
> +{
> + if (!IS_ERR_OR_NULL(arm_reg))
> + regulator_put(arm_reg);
> + arm_reg = NULL;
> +
> + if (!IS_ERR_OR_NULL(pu_reg))
> + regulator_put(pu_reg);
> + pu_reg = NULL;
> +
> + if (!IS_ERR_OR_NULL(soc_reg))
> + regulator_put(soc_reg);
> + soc_reg = NULL;
> +
> + if (!IS_ERR_OR_NULL(arm_clk))
> + clk_put(arm_clk);
> + arm_clk = NULL;
> +
> + if (!IS_ERR_OR_NULL(pll1_sys_clk))
> + clk_put(pll1_sys_clk);
> + pll1_sys_clk = NULL;
> +
> + if (!IS_ERR_OR_NULL(pll1_sw_clk))
> + clk_put(pll1_sw_clk);
> + pll1_sw_clk = NULL;
> +
> + if (!IS_ERR_OR_NULL(step_clk))
> + clk_put(step_clk);
> + step_clk = NULL;
> +
> + if (!IS_ERR_OR_NULL(pll2_pfd2_396m_clk))
> + clk_put(pll2_pfd2_396m_clk);
> + pll2_pfd2_396m_clk = NULL;
> +
> + if (!IS_ERR_OR_NULL(pll2_bus_clk))
> + clk_put(pll2_bus_clk);
> + pll2_bus_clk = NULL;
> +
> + if (!IS_ERR_OR_NULL(secondary_sel_clk))
> + clk_put(secondary_sel_clk);
> + secondary_sel_clk = NULL;
> +
> +
> +}
> +
> static int imx6q_cpufreq_probe(struct platform_device *pdev)
> {
> struct device_node *np;
> @@ -198,36 +293,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> return -ENOENT;
> }
>
> - arm_clk = clk_get(cpu_dev, "arm");
> - pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> - pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> - step_clk = clk_get(cpu_dev, "step");
> - pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> - if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
> - IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
> - dev_err(cpu_dev, "failed to get clocks\n");
> - ret = -ENOENT;
> - goto put_clk;
> - }
> -
> - if (of_machine_is_compatible("fsl,imx6ul")) {
> - pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
> - secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
> - if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
> - dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
> - ret = -ENOENT;
> - goto put_clk;
> - }
> - }
> -
> - arm_reg = regulator_get(cpu_dev, "arm");
> - pu_reg = regulator_get_optional(cpu_dev, "pu");
> - soc_reg = regulator_get(cpu_dev, "soc");
> - if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
> - dev_err(cpu_dev, "failed to get regulators\n");
> - ret = -ENOENT;
> - goto put_reg;
> - }
> + ret = imx6q_cpufreq_get_resources(pdev);
> + if (ret)
> + goto drop_resources;
>
> /*
> * We expect an OPP table supplied by platform.
> @@ -239,7 +307,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> ret = dev_pm_opp_of_add_table(cpu_dev);
> if (ret < 0) {
> dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
> - goto put_reg;
> + goto drop_resources;
> }
>
> /* Because we have added the OPPs here, we must free them */
> @@ -347,28 +415,9 @@ free_freq_table:
> out_free_opp:
> if (free_opp)
> dev_pm_opp_of_remove_table(cpu_dev);
> -put_reg:
> - if (!IS_ERR(arm_reg))
> - regulator_put(arm_reg);
> - if (!IS_ERR(pu_reg))
> - regulator_put(pu_reg);
> - if (!IS_ERR(soc_reg))
> - regulator_put(soc_reg);
> -put_clk:
> - if (!IS_ERR(arm_clk))
> - clk_put(arm_clk);
> - if (!IS_ERR(pll1_sys_clk))
> - clk_put(pll1_sys_clk);
> - if (!IS_ERR(pll1_sw_clk))
> - clk_put(pll1_sw_clk);
> - if (!IS_ERR(step_clk))
> - clk_put(step_clk);
> - if (!IS_ERR(pll2_pfd2_396m_clk))
> - clk_put(pll2_pfd2_396m_clk);
> - if (!IS_ERR(pll2_bus_clk))
> - clk_put(pll2_bus_clk);
> - if (!IS_ERR(secondary_sel_clk))
> - clk_put(secondary_sel_clk);
> +drop_resources:
> + imx6q_cpufreq_put_resources();
> +
> of_node_put(np);
> return ret;
> }
> @@ -379,17 +428,8 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
> dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
> if (free_opp)
> dev_pm_opp_of_remove_table(cpu_dev);
> - regulator_put(arm_reg);
> - if (!IS_ERR(pu_reg))
> - regulator_put(pu_reg);
> - regulator_put(soc_reg);
> - clk_put(arm_clk);
> - clk_put(pll1_sys_clk);
> - clk_put(pll1_sw_clk);
> - clk_put(step_clk);
> - clk_put(pll2_pfd2_396m_clk);
> - clk_put(pll2_bus_clk);
> - clk_put(secondary_sel_clk);
> +
> + imx6q_cpufreq_put_resources();
>
> return 0;
> }
> --
> 2.6.1
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpufreq: imx6q: cleanup resource allocation
2015-10-20 13:44 ` Sascha Hauer
2015-10-20 14:36 ` Heiner Kallweit
@ 2015-10-20 15:55 ` kbuild test robot
2015-10-21 11:02 ` [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling Viresh Kumar
2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2015-10-20 15:55 UTC (permalink / raw)
To: Sascha Hauer
Cc: kbuild-all, Heiner Kallweit, Rafael J. Wysocki, Viresh Kumar,
linux-pm
[-- Attachment #1: Type: text/plain, Size: 7095 bytes --]
Hi Sascha,
[auto build test ERROR on pm/linux-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
url: https://github.com/0day-ci/linux/commits/Sascha-Hauer/cpufreq-imx6q-cleanup-resource-allocation/20151020-214609
config: arm-imx_v6_v7_defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All error/warnings (new ones prefixed by >>):
drivers/cpufreq/imx6q-cpufreq.c: In function 'imx6q_cpufreq_put_resources':
>> drivers/cpufreq/imx6q-cpufreq.c:272:1: warning: no return statement in function returning non-void [-Wreturn-type]
}
^
drivers/cpufreq/imx6q-cpufreq.c: In function 'imx6q_cpufreq_probe':
>> drivers/cpufreq/imx6q-cpufreq.c:419:2: error: too few arguments to function 'imx6q_cpufreq_put_resources'
imx6q_cpufreq_put_resources();
^
drivers/cpufreq/imx6q-cpufreq.c:229:12: note: declared here
static int imx6q_cpufreq_put_resources(struct platform_device *pdev)
^
>> drivers/cpufreq/imx6q-cpufreq.c:327:3: error: label 'put_reg' used but not defined
goto put_reg;
^
drivers/cpufreq/imx6q-cpufreq.c: In function 'imx6q_cpufreq_remove':
drivers/cpufreq/imx6q-cpufreq.c:432:2: error: too few arguments to function 'imx6q_cpufreq_put_resources'
imx6q_cpufreq_put_resources();
^
drivers/cpufreq/imx6q-cpufreq.c:229:12: note: declared here
static int imx6q_cpufreq_put_resources(struct platform_device *pdev)
^
vim +/imx6q_cpufreq_put_resources +419 drivers/cpufreq/imx6q-cpufreq.c
266
267 if (!IS_ERR_OR_NULL(secondary_sel_clk))
268 clk_put(secondary_sel_clk);
269 secondary_sel_clk = NULL;
270
271
> 272 }
273
274 static int imx6q_cpufreq_probe(struct platform_device *pdev)
275 {
276 struct device_node *np;
277 struct dev_pm_opp *opp;
278 unsigned long min_volt, max_volt;
279 int num, ret;
280 const struct property *prop;
281 const __be32 *val;
282 u32 nr, i, j;
283
284 cpu_dev = get_cpu_device(0);
285 if (!cpu_dev) {
286 pr_err("failed to get cpu0 device\n");
287 return -ENODEV;
288 }
289
290 np = of_node_get(cpu_dev->of_node);
291 if (!np) {
292 dev_err(cpu_dev, "failed to find cpu0 node\n");
293 return -ENOENT;
294 }
295
296 ret = imx6q_cpufreq_get_resources(pdev);
297 if (ret)
298 goto drop_resources;
299
300 /*
301 * We expect an OPP table supplied by platform.
302 * Just, incase the platform did not supply the OPP
303 * table, it will try to get it.
304 */
305 num = dev_pm_opp_get_opp_count(cpu_dev);
306 if (num < 0) {
307 ret = dev_pm_opp_of_add_table(cpu_dev);
308 if (ret < 0) {
309 dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
310 goto drop_resources;
311 }
312
313 /* Because we have added the OPPs here, we must free them */
314 free_opp = true;
315
316 num = dev_pm_opp_get_opp_count(cpu_dev);
317 if (num < 0) {
318 ret = num;
319 dev_err(cpu_dev, "no OPP table is found: %d\n", ret);
320 goto out_free_opp;
321 }
322 }
323
324 ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
325 if (ret) {
326 dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
> 327 goto put_reg;
328 }
329
330 /* Make imx6_soc_volt array's size same as arm opp number */
331 imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
332 if (imx6_soc_volt == NULL) {
333 ret = -ENOMEM;
334 goto free_freq_table;
335 }
336
337 prop = of_find_property(np, "fsl,soc-operating-points", NULL);
338 if (!prop || !prop->value)
339 goto soc_opp_out;
340
341 /*
342 * Each OPP is a set of tuples consisting of frequency and
343 * voltage like <freq-kHz vol-uV>.
344 */
345 nr = prop->length / sizeof(u32);
346 if (nr % 2 || (nr / 2) < num)
347 goto soc_opp_out;
348
349 for (j = 0; j < num; j++) {
350 val = prop->value;
351 for (i = 0; i < nr / 2; i++) {
352 unsigned long freq = be32_to_cpup(val++);
353 unsigned long volt = be32_to_cpup(val++);
354 if (freq_table[j].frequency == freq) {
355 imx6_soc_volt[soc_opp_count++] = volt;
356 break;
357 }
358 }
359 }
360
361 soc_opp_out:
362 /* use fixed soc opp volt if no valid soc opp info found in dtb */
363 if (soc_opp_count != num) {
364 dev_warn(cpu_dev, "can NOT find valid fsl,soc-operating-points property in dtb, use default value!\n");
365 for (j = 0; j < num; j++)
366 imx6_soc_volt[j] = PU_SOC_VOLTAGE_NORMAL;
367 if (freq_table[num - 1].frequency * 1000 == FREQ_1P2_GHZ)
368 imx6_soc_volt[num - 1] = PU_SOC_VOLTAGE_HIGH;
369 }
370
371 if (of_property_read_u32(np, "clock-latency", &transition_latency))
372 transition_latency = CPUFREQ_ETERNAL;
373
374 /*
375 * Calculate the ramp time for max voltage change in the
376 * VDDSOC and VDDPU regulators.
377 */
378 ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
379 if (ret > 0)
380 transition_latency += ret * 1000;
381 if (!IS_ERR(pu_reg)) {
382 ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
383 if (ret > 0)
384 transition_latency += ret * 1000;
385 }
386
387 /*
388 * OPP is maintained in order of increasing frequency, and
389 * freq_table initialised from OPP is therefore sorted in the
390 * same order.
391 */
392 rcu_read_lock();
393 opp = dev_pm_opp_find_freq_exact(cpu_dev,
394 freq_table[0].frequency * 1000, true);
395 min_volt = dev_pm_opp_get_voltage(opp);
396 opp = dev_pm_opp_find_freq_exact(cpu_dev,
397 freq_table[--num].frequency * 1000, true);
398 max_volt = dev_pm_opp_get_voltage(opp);
399 rcu_read_unlock();
400 ret = regulator_set_voltage_time(arm_reg, min_volt, max_volt);
401 if (ret > 0)
402 transition_latency += ret * 1000;
403
404 ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
405 if (ret) {
406 dev_err(cpu_dev, "failed register driver: %d\n", ret);
407 goto free_freq_table;
408 }
409
410 of_node_put(np);
411 return 0;
412
413 free_freq_table:
414 dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
415 out_free_opp:
416 if (free_opp)
417 dev_pm_opp_of_remove_table(cpu_dev);
418 drop_resources:
> 419 imx6q_cpufreq_put_resources();
420
421 of_node_put(np);
422 return ret;
---
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: 26987 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
2015-10-20 13:44 ` Sascha Hauer
2015-10-20 14:36 ` Heiner Kallweit
2015-10-20 15:55 ` [PATCH] cpufreq: imx6q: cleanup resource allocation kbuild test robot
@ 2015-10-21 11:02 ` Viresh Kumar
2 siblings, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2015-10-21 11:02 UTC (permalink / raw)
To: Sascha Hauer; +Cc: Heiner Kallweit, Rafael J. Wysocki, linux-pm
On 20-10-15, 15:44, Sascha Hauer wrote:
> +static int imx6q_cpufreq_put_resources(struct platform_device *pdev)
> +{
> + if (!IS_ERR_OR_NULL(arm_reg))
> + regulator_put(arm_reg);
You can simply put them, regulator core takes care of this.
--
viresh
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-21 11:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-18 11:23 [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling Heiner Kallweit
2015-10-20 13:31 ` Sascha Hauer
2015-10-20 13:44 ` Sascha Hauer
2015-10-20 14:36 ` Heiner Kallweit
2015-10-20 15:55 ` [PATCH] cpufreq: imx6q: cleanup resource allocation kbuild test robot
2015-10-21 11:02 ` [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling Viresh Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.