[PATCH v5 1/3] drivers/thermal/exynos: Refactor clk_sec initialization inside SOC-specific case

[Anand Moon]

Hi Daniel,

On Fri, 18 Apr 2025 at 13:36, Daniel Lezcano <daniel.lezcano at linaro.org> wrote:
>
> On Thu, Apr 10, 2025 at 12:07:48PM +0530, Anand Moon wrote:
> > Refactor the initialization of the clk_sec clock to be inside the
> > SOC_ARCH_EXYNOS5420_TRIMINFO case. It ensures that the clk_sec clock
> > is only initialized for the specified SOC and not for other SOCs,
> > thereby simplifying the code. The clk_sec clock is used by the TMU
> > for GPU on the Exynos 542x platform.
> >
> > Removed redundant IS_ERR() checks for the clk_sec clock since error
> > handling is already managed internally by clk_unprepare() functions.
> >
> > Signed-off-by: Anand Moon <linux.amoon at gmail.com>
> > ---
> > v5: None
> > v4: Fix the aligment of code clk for clk_prepare in proper if/else block.
> >     update the commit for clk_sec used.
> >     checked to goto clean up all the clks are proper.
> >     drop IS_ERR() check for clk_sec.
> > v3: improve the commit message.
> > ---
> >  drivers/thermal/samsung/exynos_tmu.c | 37 ++++++++++++++--------------
> >  1 file changed, 18 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > index 47a99b3c5395..3657920de000 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -1037,29 +1037,30 @@ static int exynos_tmu_probe(struct platform_device *pdev)
> >               return ret;
> >
> >       data->clk = devm_clk_get(dev, "tmu_apbif");
> > -     if (IS_ERR(data->clk))
> > +     if (IS_ERR(data->clk)) {
> >               return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n");
>
> As this branch returns, the else block can be removed.
>
>         if (IS_ERR(data->clk))
>                 return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n");
>
>         ret = clk_prepare(data->clk);
>         if (ret) {
>                 ...
>         }
>
Earlier I got this review comment on this.
[0] https://patchwork.kernel.org/project/linux-samsung-soc/patch/20250216195850.5352-2-linux.amoon@gmail.com/
I will try to fix this in next vrsion.

> May be worth to group both calls with devm_clk_get_enabled()

Earlier, I attempted to change the clock ABI, but it didn't work.

[1] https://lore.kernel.org/all/20220515064126.1424-2-linux.amoon@gmail.com/
If you're okay with changing this, I'll update it in the next version.

> > -
> > -     data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
> > -     if (IS_ERR(data->clk_sec)) {
> > -             if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO)
> > -                     return dev_err_probe(dev, PTR_ERR(data->clk_sec),
> > -                                          "Failed to get triminfo clock\n");
> >       } else {
> > -             ret = clk_prepare(data->clk_sec);
> > +             ret = clk_prepare(data->clk);
> >               if (ret) {
> >                       dev_err(dev, "Failed to get clock\n");
> >                       return ret;
> >               }
> >       }
> >
> > -     ret = clk_prepare(data->clk);
> > -     if (ret) {
> > -             dev_err(dev, "Failed to get clock\n");
> > -             goto err_clk_sec;
> > -     }
> > -
> >       switch (data->soc) {
> > +     case SOC_ARCH_EXYNOS5420_TRIMINFO:
> > +             data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
> > +             if (IS_ERR(data->clk_sec)) {
> > +                     ret = dev_err_probe(dev, PTR_ERR(data->clk_sec),
> > +                                         "Failed to get clk_sec clock\n");
> > +                     goto err_clk;
> > +             }
> > +             ret = clk_prepare(data->clk_sec);
>
> Same comment, devm_clk_get_enabled()
If you're okay with changing this, I'll update it in the next version.
>
> > +             if (ret) {
> > +                     dev_err(dev, "Failed to prepare clk_sec clock\n");
> > +                     goto err_clk_sec;
> > +             }
> > +             break;
> >       case SOC_ARCH_EXYNOS5433:
> >       case SOC_ARCH_EXYNOS7:
> >               data->sclk = devm_clk_get(dev, "tmu_sclk");
> > @@ -1112,11 +1113,10 @@ static int exynos_tmu_probe(struct platform_device *pdev)
> >
> >  err_sclk:
> >       clk_disable_unprepare(data->sclk);
> > +err_clk_sec:
> > +     clk_unprepare(data->clk_sec);
> >  err_clk:
> >       clk_unprepare(data->clk);
> > -err_clk_sec:
> > -     if (!IS_ERR(data->clk_sec))
> > -             clk_unprepare(data->clk_sec);
>
> With devm_ variant those labels should go away
Correct.

Thanks
-Anand
>
> >       return ret;
> >  }
> >
> > @@ -1128,8 +1128,7 @@ static void exynos_tmu_remove(struct platform_device *pdev)
> >
> >       clk_disable_unprepare(data->sclk);
> >       clk_unprepare(data->clk);
> > -     if (!IS_ERR(data->clk_sec))
> > -             clk_unprepare(data->clk_sec);
> > +     clk_unprepare(data->clk_sec);
> >  }
> >
> >  #ifdef CONFIG_PM_SLEEP
> > --
> > 2.49.0
> >
>
> --
>
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog