cpufreq: imx: fix regulator_get_optional error handling

PATCH In review Heiner Kallweit 5 participants 6 messages Oct 18, 2015 – Oct 21, 2015

Subsystems (2)

Arm i.MX and Layerscape CPU frequency scaling framework

lore

Share & tools

lore

Heiner Kallweit

author

2015-10-18 11:26 UTC #0 lore raw

PATCH 2/2 cpufreq: imx: fix regulator_get_optional error handling

Properly handle the case that regulator_get_optional returns -EPROBE_DEFER and ignore other errors.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

drivers/cpufreq/imx6q-cpufreq.c +12 -5

index 0bb33dc..67dc81c 100644
@@ -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);

Signature · 1 line

2.6.1

Sascha Hauer

maintainer

2015-10-20 13:31 UTC #1 lore raw

Quoted 54 lines · Heiner Kallweit

Properly handle the case that regulator_get_optional returns -EPROBE_DEFER and ignore other errors.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

drivers/cpufreq/imx6q-cpufreq.c +10 -3

index 0bb33dc..67dc81c 100644
@@ -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;

drivers/cpufreq/imx6q-cpufreq.c out_free_opp: · line 338

Quoted 8 lines

drivers/cpufreq/imx6q-cpufreq.c +1 -1

@@ -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

Signature · 4 lines

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 |

Sascha Hauer

maintainer

2015-10-20 13:44 UTC #2 lore raw

Quoted 62 lines · Sascha Hauer

Quoted 61 lines · Heiner Kallweit

Properly handle the case that regulator_get_optional returns -EPROBE_DEFER and ignore other errors.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

drivers/cpufreq/imx6q-cpufreq.c +11 -4

index 0bb33dc..67dc81c 100644
@@ -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))

Quoted 8 lines

drivers/cpufreq/imx6q-cpufreq.c out_free_opp: · line 338
Quoted 1 line
drivers/cpufreq/imx6q-cpufreq.c +0 -0
 		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<--------------------------------

Quoted 1 line

From 7661b5b5407bfa4d29ba48059819937daaa2fa32 Mon Sep 17 00:00:00 2001

From: Sascha Hauer <[email protected]>
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 <[email protected]>

---
 drivers/cpufreq/imx6q-cpufreq.c | 168 +++++++++++++++++++++++++---------------
 1 file changed, 104 insertions(+), 64 deletions(-)

drivers/cpufreq/imx6q-cpufreq.c +104 ~~-64~~

index ef1fa81..a049d43 100644
@@ -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;
 }

Signature · 7 lines

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 |

Heiner Kallweit

2015-10-20 14:36 UTC #3 lore raw

Quoted 72 lines · Sascha Hauer

Quoted 62 lines · Sascha Hauer

Quoted 61 lines · Heiner Kallweit

Properly handle the case that regulator_get_optional returns -EPROBE_DEFER and ignore other errors.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

drivers/cpufreq/imx6q-cpufreq.c +11 -4

index 0bb33dc..67dc81c 100644
@@ -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))

Quoted 8 lines

drivers/cpufreq/imx6q-cpufreq.c out_free_opp: · line 338
Quoted 1 line
drivers/cpufreq/imx6q-cpufreq.c +0 -0
            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.

Quoted 4 lines

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.

Quoted 58 lines

Sascha

------------------------------------8<--------------------------------

From 7661b5b5407bfa4d29ba48059819937daaa2fa32 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <[email protected]>
Date: Tue, 20 Oct 2015 15:39:05 +0200
Subject: [PATCH] cpufreq: imx6q: cleanup resource allocation

Signed-off-by Sascha Hauer <[email protected]>

---
 drivers/cpufreq/imx6q-cpufreq.c | 168 +++++++++++++++++++++++++---------------
 1 file changed, 104 insertions(+), 64 deletions(-)

drivers/cpufreq/imx6q-cpufreq.c +31 -0

index ef1fa81..a049d43 100644
@@ -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);
+

drivers/cpufreq/imx6q-cpufreq.c static struct cpufreq_driver imx6q_cpufreq_driver = { · line 219

Quoted 10 lines

drivers/cpufreq/imx6q-cpufreq.c +10 -0

+               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.

Quoted 170 lines

drivers/cpufreq/imx6q-cpufreq.c +63 ~~-65~~

+
+       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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

kbuild test robot

build bot

2015-10-20 15:57 UTC #4 lore raw

Re: [PATCH] cpufreq: imx6q: cleanup resource allocation

In reply to Sascha Hauer

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…

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

Viresh Kumar

maintainer

2015-10-21 11:02 UTC #5 lore raw

In reply to Sascha Hauer

drivers/cpufreq/imx6q-cpufreq.c static struct cpufreq_driver imx6q_cpufreq_driver = { · line 232

Quoted 4 lines · Sascha Hauer

drivers/cpufreq/imx6q-cpufreq.c +4 -0

+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.

Signature · 1 line

viresh