diff mbox series

PM / devfreq: mtk-cci: refactor error handling of probe and remove

Message ID 20230503092742.19404-1-jia-wei.chang@mediatek.com (mailing list archive)
State New, archived
Headers show
Series PM / devfreq: mtk-cci: refactor error handling of probe and remove | expand

Commit Message

Jia-wei Chang (張佳偉) May 3, 2023, 9:27 a.m. UTC 
From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

To refactor the regulator/clk handlers so it can follow the way of "Free
the Last Thing Style".

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
Fixes: 86d231b1db1b ("PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver")
---
 drivers/devfreq/mtk-cci-devfreq.c | 47 ++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Comments

AngeloGioacchino Del Regno May 3, 2023, 11:40 a.m. UTC | #1 
Il 03/05/23 11:27, jia-wei.chang ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> To refactor the regulator/clk handlers so it can follow the way of "Free
> the Last Thing Style".
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Fixes: 86d231b1db1b ("PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver")
> ---
>   drivers/devfreq/mtk-cci-devfreq.c | 47 ++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
> index e5458ada5197..d2f743774147 100644
> --- a/drivers/devfreq/mtk-cci-devfreq.c
> +++ b/drivers/devfreq/mtk-cci-devfreq.c
> @@ -294,14 +294,14 @@ static int mtk_ccifreq_probe(struct platform_device *pdev)
>   	if (IS_ERR(drv->sram_reg)) {
>   		ret = PTR_ERR(drv->sram_reg);
>   		if (ret == -EPROBE_DEFER)
> -			goto out_free_resources;
> +			goto out_disable_proc_reg;
>   
>   		drv->sram_reg = NULL;
>   	} else {
>   		ret = regulator_enable(drv->sram_reg);
>   		if (ret) {
>   			dev_err(dev, "failed to enable sram regulator\n");
> -			goto out_free_resources;
> +			goto out_disable_proc_reg;
>   		}
>   	}
>   
> @@ -316,12 +316,16 @@ static int mtk_ccifreq_probe(struct platform_device *pdev)
>   
>   	ret = clk_prepare_enable(drv->cci_clk);
>   	if (ret)
> -		goto out_free_resources;
> +		goto out_disable_sram_reg;
> +
> +	ret = clk_prepare_enable(drv->inter_clk);

Adding a clk_prepare_enable() call for a clock must be done in a separate commit.
Besides, there shouldn't be any need to do that, as when you call clk_set_parent()
(done in mtk_ccifreq_target()) on a clock that has flag CLK_OPS_PARENT_ENABLE, the
clk core will automatically call clk_core_prepare_enable() on the new parent.

If you're facing a bug for which the parent is not getting enabled, the solution
is to add CLK_OPS_PARENT_ENABLE to the interested clock.

Regards,
Angelo
Jia-wei Chang (張佳偉) May 4, 2023, 3 a.m. UTC | #2 
On Wed, 2023-05-03 at 13:40 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/05/23 11:27, jia-wei.chang ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > To refactor the regulator/clk handlers so it can follow the way of
> > "Free
> > the Last Thing Style".
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > Fixes: 86d231b1db1b ("PM / devfreq: mediatek: Introduce MediaTek
> > CCI devfreq driver")
> > ---
> >   drivers/devfreq/mtk-cci-devfreq.c | 47 ++++++++++++++++++------
> > -------
> >   1 file changed, 28 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/devfreq/mtk-cci-devfreq.c
> > b/drivers/devfreq/mtk-cci-devfreq.c
> > index e5458ada5197..d2f743774147 100644
> > --- a/drivers/devfreq/mtk-cci-devfreq.c
> > +++ b/drivers/devfreq/mtk-cci-devfreq.c
> > @@ -294,14 +294,14 @@ static int mtk_ccifreq_probe(struct
> > platform_device *pdev)
> >       if (IS_ERR(drv->sram_reg)) {
> >               ret = PTR_ERR(drv->sram_reg);
> >               if (ret == -EPROBE_DEFER)
> > -                     goto out_free_resources;
> > +                     goto out_disable_proc_reg;
> > 
> >               drv->sram_reg = NULL;
> >       } else {
> >               ret = regulator_enable(drv->sram_reg);
> >               if (ret) {
> >                       dev_err(dev, "failed to enable sram
> > regulator\n");
> > -                     goto out_free_resources;
> > +                     goto out_disable_proc_reg;
> >               }
> >       }
> > 
> > @@ -316,12 +316,16 @@ static int mtk_ccifreq_probe(struct
> > platform_device *pdev)
> > 
> >       ret = clk_prepare_enable(drv->cci_clk);
> >       if (ret)
> > -             goto out_free_resources;
> > +             goto out_disable_sram_reg;
> > +
> > +     ret = clk_prepare_enable(drv->inter_clk);
> 
> Adding a clk_prepare_enable() call for a clock must be done in a
> separate commit.
> Besides, there shouldn't be any need to do that, as when you call
> clk_set_parent()
> (done in mtk_ccifreq_target()) on a clock that has flag
> CLK_OPS_PARENT_ENABLE, the
> clk core will automatically call clk_core_prepare_enable() on the new
> parent.
> 
> If you're facing a bug for which the parent is not getting enabled,
> the solution
> is to add CLK_OPS_PARENT_ENABLE to the interested clock.
> 
> Regards,
> Angelo
> 

Hi Angelo Sir,

Thanks for your suggestion.
I will send a new patch to remove clk_prepare_enable() call on the
parent clock, since it is not directly related to this commit purpose.

As you suggested, clock parent enable will be handled by flag if
necessary.
diff mbox series

Patch

diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
index e5458ada5197..d2f743774147 100644
--- a/drivers/devfreq/mtk-cci-devfreq.c
+++ b/drivers/devfreq/mtk-cci-devfreq.c
@@ -294,14 +294,14 @@  static int mtk_ccifreq_probe(struct platform_device *pdev)
 	if (IS_ERR(drv->sram_reg)) {
 		ret = PTR_ERR(drv->sram_reg);
 		if (ret == -EPROBE_DEFER)
-			goto out_free_resources;
+			goto out_disable_proc_reg;
 
 		drv->sram_reg = NULL;
 	} else {
 		ret = regulator_enable(drv->sram_reg);
 		if (ret) {
 			dev_err(dev, "failed to enable sram regulator\n");
-			goto out_free_resources;
+			goto out_disable_proc_reg;
 		}
 	}
 
@@ -316,12 +316,16 @@  static int mtk_ccifreq_probe(struct platform_device *pdev)
 
 	ret = clk_prepare_enable(drv->cci_clk);
 	if (ret)
-		goto out_free_resources;
+		goto out_disable_sram_reg;
+
+	ret = clk_prepare_enable(drv->inter_clk);
+	if (ret)
+		goto out_disable_cci_clock;
 
 	ret = dev_pm_opp_of_add_table(dev);
 	if (ret) {
 		dev_err(dev, "failed to add opp table: %d\n", ret);
-		goto out_disable_cci_clk;
+		goto out_disable_inter_clock;
 	}
 
 	rate = clk_get_rate(drv->inter_clk);
@@ -329,7 +333,7 @@  static int mtk_ccifreq_probe(struct platform_device *pdev)
 	if (IS_ERR(opp)) {
 		ret = PTR_ERR(opp);
 		dev_err(dev, "failed to get intermediate opp: %d\n", ret);
-		goto out_remove_opp_table;
+		goto out_free_opp_table;
 	}
 	drv->inter_voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
@@ -339,7 +343,7 @@  static int mtk_ccifreq_probe(struct platform_device *pdev)
 	if (IS_ERR(opp)) {
 		dev_err(dev, "failed to get opp\n");
 		ret = PTR_ERR(opp);
-		goto out_remove_opp_table;
+		goto out_free_opp_table;
 	}
 
 	opp_volt = dev_pm_opp_get_voltage(opp);
@@ -348,13 +352,13 @@  static int mtk_ccifreq_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(dev, "failed to scale to highest voltage %lu in proc_reg\n",
 			opp_volt);
-		goto out_remove_opp_table;
+		goto out_free_opp_table;
 	}
 
 	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
 	if (!passive_data) {
 		ret = -ENOMEM;
-		goto out_remove_opp_table;
+		goto out_free_opp_table;
 	}
 
 	passive_data->parent_type = CPUFREQ_PARENT_DEV;
@@ -365,29 +369,33 @@  static int mtk_ccifreq_probe(struct platform_device *pdev)
 		ret = -EPROBE_DEFER;
 		dev_err(dev, "failed to add devfreq device: %ld\n",
 			PTR_ERR(drv->devfreq));
-		goto out_remove_opp_table;
+		goto out_free_opp_table;
 	}
 
 	drv->opp_nb.notifier_call = mtk_ccifreq_opp_notifier;
 	ret = dev_pm_opp_register_notifier(dev, &drv->opp_nb);
 	if (ret) {
 		dev_err(dev, "failed to register opp notifier: %d\n", ret);
-		goto out_remove_opp_table;
+		goto out_free_opp_table;
 	}
 	return 0;
 
-out_remove_opp_table:
+out_free_opp_table:
 	dev_pm_opp_of_remove_table(dev);
 
-out_disable_cci_clk:
+out_disable_inter_clock:
+	clk_disable_unprepare(drv->inter_clk);
+
+out_disable_cci_clock:
 	clk_disable_unprepare(drv->cci_clk);
 
-out_free_resources:
-	if (regulator_is_enabled(drv->proc_reg))
-		regulator_disable(drv->proc_reg);
-	if (drv->sram_reg && regulator_is_enabled(drv->sram_reg))
+out_disable_sram_reg:
+	if (drv->sram_reg)
 		regulator_disable(drv->sram_reg);
 
+out_disable_proc_reg:
+	regulator_disable(drv->proc_reg);
+
 	return ret;
 }
 
@@ -398,12 +406,13 @@  static int mtk_ccifreq_remove(struct platform_device *pdev)
 
 	drv = platform_get_drvdata(pdev);
 
-	dev_pm_opp_unregister_notifier(dev, &drv->opp_nb);
-	dev_pm_opp_of_remove_table(dev);
-	clk_disable_unprepare(drv->cci_clk);
 	regulator_disable(drv->proc_reg);
 	if (drv->sram_reg)
 		regulator_disable(drv->sram_reg);
+	clk_disable_unprepare(drv->cci_clk);
+	clk_disable_unprepare(drv->inter_clk);
+	dev_pm_opp_of_remove_table(dev);
+	dev_pm_opp_unregister_notifier(dev, &drv->opp_nb);
 
 	return 0;
 }