| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANAwSgS_OnEv=r7S4CetLkW4vo-QdFAKTbqRK9H-Q0dq5VDYLA@mail.gmail.com>
Date: Fri, 18 Apr 2025 19:01:29 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>, Krzysztof Kozlowski <krzk@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Zhang Rui <rui.zhang@...el.com>,
Lukasz Luba <lukasz.luba@....com>, Alim Akhtar <alim.akhtar@...sung.com>,
Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
"open list:SAMSUNG THERMAL DRIVER" <linux-pm@...r.kernel.org>,
"open list:SAMSUNG THERMAL DRIVER" <linux-samsung-soc@...r.kernel.org>,
"moderated list:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM ARCHITECTURES" <linux-arm-kernel@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>,
"open list:CLANG/LLVM BUILD SUPPORT:Keyword:b(?i:clang|llvm)b" <llvm@...ts.linux.dev>
Subject: Re: [PATCH v5 1/3] drivers/thermal/exynos: Refactor clk_sec
initialization inside SOC-specific case
Hi Daniel,
On Fri, 18 Apr 2025 at 13:36, Daniel Lezcano <daniel.lezcano@...aro.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@...il.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
Powered by blists - more mailing lists