| Message ID | 1446019154-16384-4-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer: > When one of the regulator_get/clk_get calls fails the driver just > returns -ENOENT. To properly handle the -EPROBE_DEFER case we have > to forward the actual error value instead. This patch fixes that. > > Since all clocks/regulators are global static variables the IS_ERR() > check in the error path in probe() may erroneously work on the values > assigned in the previous probe run. This means we have to reinitialize > the variables between two probe runs(). > > The probe function already is quite overloaded, so this patch moves > the regulator and clock allocation into a separate function and the > dropping of these resourced into 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(). > > While at it leave a comment that these resource allocations can't use > devm_* to prevent people from trying to convert it. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> Looks good to me now, Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/cpufreq/imx6q-cpufreq.c | 170 +++++++++++++++++++++++++--------------- > 1 file changed, 106 insertions(+), 64 deletions(-) > > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c > index 6fefd19..f81cf8e 100644 > --- a/drivers/cpufreq/imx6q-cpufreq.c > +++ b/drivers/cpufreq/imx6q-cpufreq.c > @@ -176,6 +176,103 @@ static struct cpufreq_driver imx6q_cpufreq_driver = { > .attr = cpufreq_generic_attr, > }; > > +static int imx6q_cpufreq_get_resources(void) > +{ > + /* > + * 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 void imx6q_cpufreq_put_resources(void) > +{ > + /* > + * Set all resources to NULL here so that they are correctly > + * initialized next time we enter probe() > + */ > + 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 +295,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(); > + if (ret) > + goto drop_resources; > > /* > * We expect an OPP table supplied by platform. > @@ -239,7 +309,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 */ > @@ -348,28 +418,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; > } > @@ -380,17 +431,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; > }
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c index 6fefd19..f81cf8e 100644 --- a/drivers/cpufreq/imx6q-cpufreq.c +++ b/drivers/cpufreq/imx6q-cpufreq.c @@ -176,6 +176,103 @@ static struct cpufreq_driver imx6q_cpufreq_driver = { .attr = cpufreq_generic_attr, }; +static int imx6q_cpufreq_get_resources(void) +{ + /* + * 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 void imx6q_cpufreq_put_resources(void) +{ + /* + * Set all resources to NULL here so that they are correctly + * initialized next time we enter probe() + */ + 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 +295,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(); + if (ret) + goto drop_resources; /* * We expect an OPP table supplied by platform. @@ -239,7 +309,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 */ @@ -348,28 +418,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; } @@ -380,17 +431,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; }
When one of the regulator_get/clk_get calls fails the driver just returns -ENOENT. To properly handle the -EPROBE_DEFER case we have to forward the actual error value instead. This patch fixes that. Since all clocks/regulators are global static variables the IS_ERR() check in the error path in probe() may erroneously work on the values assigned in the previous probe run. This means we have to reinitialize the variables between two probe runs(). The probe function already is quite overloaded, so this patch moves the regulator and clock allocation into a separate function and the dropping of these resourced into 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(). While at it leave a comment that these resource allocations can't use devm_* to prevent people from trying to convert it. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/cpufreq/imx6q-cpufreq.c | 170 +++++++++++++++++++++++++--------------- 1 file changed, 106 insertions(+), 64 deletions(-)