Hi Daniel,
On Fri, 18 Apr 2025 at 13:36, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx/
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@xxxxxxxxx/
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
