cpufreq: i.MX6: -EPROBE_DEFER fixes

cpufreq: i.MX6: -EPROBE_DEFER fixes

[Sascha Hauer]

The i.MX6 cpufreq driver can't cope with -EPROBE_DEFER returned from
regulator_get/clk_get. This series fix that and some other small issues.

This series supersedes
[PATCH 1/2] cpufreq: imx: fix regulator_get error handling
[PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
from Heiner Kallweit.

Sascha

----------------------------------------------------------------
Sascha Hauer (4):
      cpufreq: imx6q: Fix goto wrong error label
      cpufreq: imx6q: Fix wrong device in devm_kzalloc
      cpufreq: imx6q: Fix regulator/clock error handling
      cpufreq: imx6q: check regulator_get_optional return value

 drivers/cpufreq/imx6q-cpufreq.c | 182 +++++++++++++++++++++++++---------------
 1 file changed, 113 insertions(+), 69 deletions(-)

[PATCH 1/4] cpufreq: imx6q: Fix goto wrong error label

[Sascha Hauer]

After the call to dev_pm_opp_init_cpufreq_table() we have to go to
out_free_opp because we have already allocated the opp.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/cpufreq/imx6q-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index ef1fa81..fabd144 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -256,7 +256,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
 		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
-		goto put_reg;
+		goto out_free_opp;
 	}
 
 	/* Make imx6_soc_volt array's size same as arm opp number */
-- 
2.6.1

[PATCH 2/4] cpufreq: imx6q: Fix wrong device in devm_kzalloc

[Sascha Hauer]

devm_kzalloc must be called with the device that is actually probed,
not with cpu_dev which resources are not freed when probe fails.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/cpufreq/imx6q-cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index fabd144..7557d1e 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -260,7 +260,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 	}
 
 	/* Make imx6_soc_volt array's size same as arm opp number */
-	imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
+	imx6_soc_volt = devm_kzalloc(&pdev->dev,
+			sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
 	if (imx6_soc_volt == NULL) {
 		ret = -ENOMEM;
 		goto free_freq_table;
-- 
2.6.1

[PATCH 3/4] cpufreq: imx6q: Fix regulator/clock error handling

[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>
---
 drivers/cpufreq/imx6q-cpufreq.c | 166 ++++++++++++++++++++++++----------------
 1 file changed, 102 insertions(+), 64 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 7557d1e..ce0c6bd 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -176,6 +176,99 @@ 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 void imx6q_cpufreq_put_resources(void)
+{
+	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 +291,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 +305,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 +414,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 +427,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

[PATCH 4/4] cpufreq: imx6q: check regulator_get_optional return value

[Sascha Hauer]

While pu_reg is an optional regulator we still have to check
the return value for -EPROBE_DEFER to properly detect the
case when we have a pu regulator but it is not yet present.

While at it, set pu_reg to NULL for an erroneous regulator
so that we can check for the regulator with if (pu_reg)
instead of the slightly less readable IS_ERR().

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/cpufreq/imx6q-cpufreq.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index ce0c6bd..b643614 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -71,7 +71,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);
@@ -148,7 +148,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);
@@ -217,6 +217,11 @@ static int imx6q_cpufreq_get_resources(struct platform_device *pdev)
 		return PTR_ERR(arm_reg);
 
 	pu_reg = regulator_get_optional(cpu_dev, "pu");
+	if (IS_ERR(pu_reg)) {
+		if (PTR_ERR(pu_reg) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		pu_reg = NULL;
+	}
 
 	soc_reg = regulator_get(cpu_dev, "soc");
 	if (IS_ERR(soc_reg))
@@ -377,7 +382,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;
-- 
2.6.1

[PATCH 1/4] cpufreq: imx6q: Fix goto wrong error label

[Lucas Stach]

Am Donnerstag, den 22.10.2015, 10:47 +0200 schrieb Sascha Hauer:
> After the call to dev_pm_opp_init_cpufreq_table() we have to go to
> out_free_opp because we have already allocated the opp.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index ef1fa81..fabd144 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -256,7 +256,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
>  	if (ret) {
>  		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
> -		goto put_reg;
> +		goto out_free_opp;
>  	}
>  
>  	/* Make imx6_soc_volt array's size same as arm opp number */

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH 2/4] cpufreq: imx6q: Fix wrong device in devm_kzalloc

[Lucas Stach]

Am Donnerstag, den 22.10.2015, 10:47 +0200 schrieb Sascha Hauer:
> devm_kzalloc must be called with the device that is actually probed,
> not with cpu_dev which resources are not freed when probe fails.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index fabd144..7557d1e 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -260,7 +260,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Make imx6_soc_volt array's size same as arm opp number */
> -	imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
> +	imx6_soc_volt = devm_kzalloc(&pdev->dev,
> +			sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
>  	if (imx6_soc_volt == NULL) {
>  		ret = -ENOMEM;
>  		goto free_freq_table;

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH 3/4] cpufreq: imx6q: Fix regulator/clock error handling

[Lucas Stach]

Am Donnerstag, den 22.10.2015, 10:47 +0200 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>

Nice cleanup overall, one nit below.

> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 166 ++++++++++++++++++++++++----------------
>  1 file changed, 102 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 7557d1e..ce0c6bd 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -176,6 +176,99 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
>  	.attr = cpufreq_generic_attr,
>  };
>  
> +static int imx6q_cpufreq_get_resources(struct platform_device *pdev)

Given that all the resources are attached to the cpu_dev anyway, do you
really see a use for this parameter?

> +{
> +	/*
> +	 * 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;
> +}
> +
[...]

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH 4/4] cpufreq: imx6q: check regulator_get_optional return value

[Lucas Stach]

Am Donnerstag, den 22.10.2015, 10:47 +0200 schrieb Sascha Hauer:
> While pu_reg is an optional regulator we still have to check
> the return value for -EPROBE_DEFER to properly detect the
> case when we have a pu regulator but it is not yet present.
> 
> While at it, set pu_reg to NULL for an erroneous regulator
> so that we can check for the regulator with if (pu_reg)
> instead of the slightly less readable IS_ERR().
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index ce0c6bd..b643614 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -71,7 +71,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);
> @@ -148,7 +148,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);
> @@ -217,6 +217,11 @@ static int imx6q_cpufreq_get_resources(struct platform_device *pdev)
>  		return PTR_ERR(arm_reg);
>  
>  	pu_reg = regulator_get_optional(cpu_dev, "pu");
> +	if (IS_ERR(pu_reg)) {
> +		if (PTR_ERR(pu_reg) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		pu_reg = NULL;
> +	}

The PU regulator isn't really optional for MX6 Q/DL (where it is present
in hardware) as the datasheet clearly specifies that it needs to be set
to the same voltage as SOC. Could you change this to request this
regulator as non-optional for those chip derivatives and skip the
request otherwise?

Regards,
Lucas

>  
>  	soc_reg = regulator_get(cpu_dev, "soc");
>  	if (IS_ERR(soc_reg))
> @@ -377,7 +382,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;

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH 2/4] cpufreq: imx6q: Fix wrong device in devm_kzalloc

[Viresh Kumar]

On 22-10-15, 10:47, Sascha Hauer wrote:
> devm_kzalloc must be called with the device that is actually probed,
> not with cpu_dev which resources are not freed when probe fails.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index fabd144..7557d1e 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -260,7 +260,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Make imx6_soc_volt array's size same as arm opp number */
> -	imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
> +	imx6_soc_volt = devm_kzalloc(&pdev->dev,
> +			sizeof(*imx6_soc_volt) * num, GFP_KERNEL);

Please rewrite it as:

	imx6_soc_volt = devm_kzalloc(&pdev->dev,
			             sizeof(*imx6_soc_volt) * num, GFP_KERNEL);

checkpatch --strict will warn against this.

-- 
viresh

[PATCH 3/4] cpufreq: imx6q: Fix regulator/clock error handling

[Viresh Kumar]

On 22-10-15, 10:47, Sascha Hauer wrote:
> +	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");

Yes, current code doesn't care about if this failed, but if that's the
intention please add a comment for it now.

> +	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)
> +{
> +	if (!IS_ERR_OR_NULL(arm_reg))
> +		regulator_put(arm_reg);
> +	arm_reg = NULL;

Can you please clarify (maybe again) why this is required to be set to
NULL again?

-- 
viresh

[PATCH 3/4] cpufreq: imx6q: Fix regulator/clock error handling

[Sascha Hauer]

On Thu, Oct 22, 2015 at 07:03:15PM +0530, Viresh Kumar wrote:
> On 22-10-15, 10:47, Sascha Hauer wrote:
> > +	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");
> 
> Yes, current code doesn't care about if this failed, but if that's the
> intention please add a comment for it now.

This is fixed right in the next patch.

> 
> > +	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)
> > +{
> > +	if (!IS_ERR_OR_NULL(arm_reg))
> > +		regulator_put(arm_reg);
> > +	arm_reg = NULL;
> 
> Can you please clarify (maybe again) why this is required to be set to
> NULL again?

Ok.

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 |

[PATCH 4/4] cpufreq: imx6q: check regulator_get_optional return value

[Sascha Hauer]

On Thu, Oct 22, 2015 at 12:23:15PM +0200, Lucas Stach wrote:
> Am Donnerstag, den 22.10.2015, 10:47 +0200 schrieb Sascha Hauer:
> > While pu_reg is an optional regulator we still have to check
> > the return value for -EPROBE_DEFER to properly detect the
> > case when we have a pu regulator but it is not yet present.
> > 
> > While at it, set pu_reg to NULL for an erroneous regulator
> > so that we can check for the regulator with if (pu_reg)
> > instead of the slightly less readable IS_ERR().
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/cpufreq/imx6q-cpufreq.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> > index ce0c6bd..b643614 100644
> > --- a/drivers/cpufreq/imx6q-cpufreq.c
> > +++ b/drivers/cpufreq/imx6q-cpufreq.c
> > @@ -71,7 +71,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);
> > @@ -148,7 +148,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);
> > @@ -217,6 +217,11 @@ static int imx6q_cpufreq_get_resources(struct platform_device *pdev)
> >  		return PTR_ERR(arm_reg);
> >  
> >  	pu_reg = regulator_get_optional(cpu_dev, "pu");
> > +	if (IS_ERR(pu_reg)) {
> > +		if (PTR_ERR(pu_reg) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +		pu_reg = NULL;
> > +	}
> 
> The PU regulator isn't really optional for MX6 Q/DL (where it is present
> in hardware) as the datasheet clearly specifies that it needs to be set
> to the same voltage as SOC. Could you change this to request this
> regulator as non-optional for those chip derivatives and skip the
> request otherwise?

i.MX6SL also has the pu-reg, so this would become

	if (of_machine_is_compatible("fsl,imx6q") ||
	    of_machine_is_compatible("fsl,imx6dl") ||
	    of_machine_is_compatible("fsl,imx6sl))

Not really nice. Also we already have a
of_machine_is_compatible("fsl,imx6ul") in the driver. It seems the
knowledge about (struct platform_device)->id_table is already lost
nowadays. I think this should be used here to distinguish between the
different devices.

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 |

cpufreq: i.MX6: -EPROBE_DEFER fixes

[Rafael J. Wysocki]

On Thursday, October 22, 2015 10:47:28 AM Sascha Hauer wrote:
> The i.MX6 cpufreq driver can't cope with -EPROBE_DEFER returned from
> regulator_get/clk_get. This series fix that and some other small issues.
> 
> This series supersedes
> [PATCH 1/2] cpufreq: imx: fix regulator_get error handling
> [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
> from Heiner Kallweit.

I'm expecting a new version of this addressing the review comments to be
submitted.

Thanks,
Rafael

cpufreq: i.MX6: -EPROBE_DEFER fixes

[Sascha Hauer]

On Wed, Oct 28, 2015 at 04:27:00AM +0100, Rafael J. Wysocki wrote:
> On Thursday, October 22, 2015 10:47:28 AM Sascha Hauer wrote:
> > The i.MX6 cpufreq driver can't cope with -EPROBE_DEFER returned from
> > regulator_get/clk_get. This series fix that and some other small issues.
> > 
> > This series supersedes
> > [PATCH 1/2] cpufreq: imx: fix regulator_get error handling
> > [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
> > from Heiner Kallweit.
> 
> I'm expecting a new version of this addressing the review comments to be
> submitted.

Sure, the new series is out.

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 |