diff mbox series

[3/4] cpufreq: s5pv210: Defer probe if getting regulators fail

Message ID 20190108200509.30494-4-pawel.mikolaj.chmiel@gmail.com (mailing list archive)
State New, archived
Headers show
Series ARM: dts: s5pv210: Enable cpufreq support | expand

Commit Message

Paweł Chmiel Jan. 8, 2019, 8:05 p.m. UTC
There is possibility, that when probing driver, regulators are not yet
initialized. In this case we should return EPROBE_DEFER and wait till
they're initialized, since they're required currently for cpufreq driver
to work. Also move regulator initialization code at beginning of probe,
so we can defer as fast as posibble.

Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/cpufreq/s5pv210-cpufreq.c | 33 +++++++++++++++++++------------
 1 file changed, 20 insertions(+), 13 deletions(-)

Comments

Viresh Kumar Jan. 10, 2019, 7:01 a.m. UTC | #1
On 08-01-19, 21:05, Paweł Chmiel wrote:
> There is possibility, that when probing driver, regulators are not yet
> initialized. In this case we should return EPROBE_DEFER and wait till
> they're initialized, since they're required currently for cpufreq driver
> to work. Also move regulator initialization code at beginning of probe,
> so we can defer as fast as posibble.
> 
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  drivers/cpufreq/s5pv210-cpufreq.c | 33 +++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> index f51697f1e0b3..2d0e4dc7ede7 100644
> --- a/drivers/cpufreq/s5pv210-cpufreq.c
> +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> @@ -594,6 +594,26 @@ static int s5pv210_cpufreq_probe(struct platform_device *pdev)
>  	 * this whole driver as soon as S5PV210 gets migrated to use
>  	 * cpufreq-dt driver.
>  	 */
> +	arm_regulator = regulator_get(NULL, "vddarm");
> +	if (PTR_ERR(arm_regulator) == -EPROBE_DEFER) {
> +		pr_dbg("vddarm regulator not ready, defer\n");
> +		return -EPROBE_DEFER;
> +	} else if (IS_ERR(arm_regulator)) {
> +		pr_err("failed to get regulator vddarm\n");
> +		return PTR_ERR(arm_regulator);
> +	}

The only difference between the two cases is pr_dbg vs pr_err, its
ugly that we have to add special code for that :(

Maybe write it as:

        if (IS_ERR(arm_regulator)) {
                if (PTR_ERR(arm_regulator) == -EPROBE_DEFER)
                        pr_dbg...
                else
                        pr_err
                return PTR_ERR(arm_regulator);
        }

> +
> +	int_regulator = regulator_get(NULL, "vddint");
> +	if (PTR_ERR(int_regulator == -EPROBE_DEFER) {

Does this even compile ?

> +		regulator_put(arm_regulator);
> +		pr_dbg("vddint regulator not ready, defer\n");
> +		return -EPROBE_DEFER;
> +	} else if (IS_ERR(int_regulator)) {
> +		regulator_put(arm_regulator);
> +		pr_err("failed to get regulator vddint\n");
> +		return PTR_ERR(int_regulator);
> +	}
> +
>  	np = of_find_compatible_node(NULL, NULL, "samsung,s5pv210-clock");
>  	if (!np) {
>  		pr_err("%s: failed to find clock controller DT node\n",
> @@ -633,19 +653,6 @@ static int s5pv210_cpufreq_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	arm_regulator = regulator_get(NULL, "vddarm");
> -	if (IS_ERR(arm_regulator)) {
> -		pr_err("failed to get regulator vddarm\n");
> -		return PTR_ERR(arm_regulator);
> -	}
> -
> -	int_regulator = regulator_get(NULL, "vddint");
> -	if (IS_ERR(int_regulator)) {
> -		pr_err("failed to get regulator vddint\n");
> -		regulator_put(arm_regulator);
> -		return PTR_ERR(int_regulator);
> -	}
> -
>  	register_reboot_notifier(&s5pv210_cpufreq_reboot_notifier);
>  
>  	return cpufreq_register_driver(&s5pv210_driver);
> -- 
> 2.17.1
Paweł Chmiel Jan. 10, 2019, 8:03 a.m. UTC | #2
czw., 10 sty 2019 o 08:01 Viresh Kumar <viresh.kumar@linaro.org> napisał(a):
>
> On 08-01-19, 21:05, Paweł Chmiel wrote:
> > There is possibility, that when probing driver, regulators are not yet
> > initialized. In this case we should return EPROBE_DEFER and wait till
> > they're initialized, since they're required currently for cpufreq driver
> > to work. Also move regulator initialization code at beginning of probe,
> > so we can defer as fast as posibble.
> >
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> >  drivers/cpufreq/s5pv210-cpufreq.c | 33 +++++++++++++++++++------------
> >  1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> > index f51697f1e0b3..2d0e4dc7ede7 100644
> > --- a/drivers/cpufreq/s5pv210-cpufreq.c
> > +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> > @@ -594,6 +594,26 @@ static int s5pv210_cpufreq_probe(struct platform_device *pdev)
> >        * this whole driver as soon as S5PV210 gets migrated to use
> >        * cpufreq-dt driver.
> >        */
> > +     arm_regulator = regulator_get(NULL, "vddarm");
> > +     if (PTR_ERR(arm_regulator) == -EPROBE_DEFER) {
> > +             pr_dbg("vddarm regulator not ready, defer\n");
> > +             return -EPROBE_DEFER;
> > +     } else if (IS_ERR(arm_regulator)) {
> > +             pr_err("failed to get regulator vddarm\n");
> > +             return PTR_ERR(arm_regulator);
> > +     }
>
> The only difference between the two cases is pr_dbg vs pr_err, its
> ugly that we have to add special code for that :(
>
> Maybe write it as:
>
>         if (IS_ERR(arm_regulator)) {
>                 if (PTR_ERR(arm_regulator) == -EPROBE_DEFER)
>                         pr_dbg...
>                 else
>                         pr_err
>                 return PTR_ERR(arm_regulator);
>         }
>
> > +
> > +     int_regulator = regulator_get(NULL, "vddint");
> > +     if (PTR_ERR(int_regulator == -EPROBE_DEFER) {
>
> Does this even compile ?
Looks like i broke it after testing (where it was working fine - was
able to change freq and gov),
and didn't catch this before sending patch for review :/.
Sorry, this (and first issue) will be fixed in v2.

Thanks for review
>
> > +             regulator_put(arm_regulator);
> > +             pr_dbg("vddint regulator not ready, defer\n");
> > +             return -EPROBE_DEFER;
> > +     } else if (IS_ERR(int_regulator)) {
> > +             regulator_put(arm_regulator);
> > +             pr_err("failed to get regulator vddint\n");
> > +             return PTR_ERR(int_regulator);
> > +     }
> > +
> >       np = of_find_compatible_node(NULL, NULL, "samsung,s5pv210-clock");
> >       if (!np) {
> >               pr_err("%s: failed to find clock controller DT node\n",
> > @@ -633,19 +653,6 @@ static int s5pv210_cpufreq_probe(struct platform_device *pdev)
> >               }
> >       }
> >
> > -     arm_regulator = regulator_get(NULL, "vddarm");
> > -     if (IS_ERR(arm_regulator)) {
> > -             pr_err("failed to get regulator vddarm\n");
> > -             return PTR_ERR(arm_regulator);
> > -     }
> > -
> > -     int_regulator = regulator_get(NULL, "vddint");
> > -     if (IS_ERR(int_regulator)) {
> > -             pr_err("failed to get regulator vddint\n");
> > -             regulator_put(arm_regulator);
> > -             return PTR_ERR(int_regulator);
> > -     }
> > -
> >       register_reboot_notifier(&s5pv210_cpufreq_reboot_notifier);
> >
> >       return cpufreq_register_driver(&s5pv210_driver);
> > --
> > 2.17.1
>
> --
> viresh
diff mbox series

Patch

diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index f51697f1e0b3..2d0e4dc7ede7 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -594,6 +594,26 @@  static int s5pv210_cpufreq_probe(struct platform_device *pdev)
 	 * this whole driver as soon as S5PV210 gets migrated to use
 	 * cpufreq-dt driver.
 	 */
+	arm_regulator = regulator_get(NULL, "vddarm");
+	if (PTR_ERR(arm_regulator) == -EPROBE_DEFER) {
+		pr_dbg("vddarm regulator not ready, defer\n");
+		return -EPROBE_DEFER;
+	} else if (IS_ERR(arm_regulator)) {
+		pr_err("failed to get regulator vddarm\n");
+		return PTR_ERR(arm_regulator);
+	}
+
+	int_regulator = regulator_get(NULL, "vddint");
+	if (PTR_ERR(int_regulator == -EPROBE_DEFER) {
+		regulator_put(arm_regulator);
+		pr_dbg("vddint regulator not ready, defer\n");
+		return -EPROBE_DEFER;
+	} else if (IS_ERR(int_regulator)) {
+		regulator_put(arm_regulator);
+		pr_err("failed to get regulator vddint\n");
+		return PTR_ERR(int_regulator);
+	}
+
 	np = of_find_compatible_node(NULL, NULL, "samsung,s5pv210-clock");
 	if (!np) {
 		pr_err("%s: failed to find clock controller DT node\n",
@@ -633,19 +653,6 @@  static int s5pv210_cpufreq_probe(struct platform_device *pdev)
 		}
 	}
 
-	arm_regulator = regulator_get(NULL, "vddarm");
-	if (IS_ERR(arm_regulator)) {
-		pr_err("failed to get regulator vddarm\n");
-		return PTR_ERR(arm_regulator);
-	}
-
-	int_regulator = regulator_get(NULL, "vddint");
-	if (IS_ERR(int_regulator)) {
-		pr_err("failed to get regulator vddint\n");
-		regulator_put(arm_regulator);
-		return PTR_ERR(int_regulator);
-	}
-
 	register_reboot_notifier(&s5pv210_cpufreq_reboot_notifier);
 
 	return cpufreq_register_driver(&s5pv210_driver);