| Message ID | 20250216195850.5352-1-linux.amoon@gmail.com |
|---|---|
| Headers | show |
| Series | Exynos Thermal code improvement | expand |
Hi Anand, On 2/16/25 19:58, 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. So IIUC there was no need to init that clock for other types of SoCs... Do we know that for sure (e.g. from other TRMs)? If that was the case, then simplification here can go further, but after some fixes. > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > v3: improve the commit message > --- > drivers/thermal/samsung/exynos_tmu.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c > index 47a99b3c5395..9c138772d380 100644 > --- a/drivers/thermal/samsung/exynos_tmu.c > +++ b/drivers/thermal/samsung/exynos_tmu.c > @@ -1040,19 +1040,6 @@ static int exynos_tmu_probe(struct platform_device *pdev) > if (IS_ERR(data->clk)) > return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n"); > > - 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); > - if (ret) { > - dev_err(dev, "Failed to get clock\n"); > - return ret; > - } > - } > - > ret = clk_prepare(data->clk); Here the data->clk is now used in different order. > if (ret) { > dev_err(dev, "Failed to get clock\n"); > @@ -1060,6 +1047,19 @@ static int exynos_tmu_probe(struct platform_device *pdev) > } > > switch (data->soc) { > + case SOC_ARCH_EXYNOS5420_TRIMINFO: > + data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif"); > + if (IS_ERR(data->clk_sec)) { > + return dev_err_probe(dev, PTR_ERR(data->clk_sec), > + "Failed to get triminfo clock\n"); Then here you shouldn't simply copy the old code. Now the data->clk is first, so should be 'clk_unprepare()' before return of the function. > + } else { You can get rid of this 'else' above and still be safe in your refactoring. > + ret = clk_prepare(data->clk_sec); > + if (ret) { > + dev_err(dev, "Failed to get clock\n"); > + return ret; > + } Here you can further simplify this to something like: -----------------------8<------------------------------------- + case SOC_ARCH_EXYNOS5420_TRIMINFO: + data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif"); + if (IS_ERR(data->clk_sec)) { + clk_unprepare(data->clk); ///// <---- + return dev_err_probe(dev, PTR_ERR(data->clk_sec), + "Failed to get triminfo clock\n"); + } + ret = clk_prepare(data->clk_sec); + if (ret) { + dev_err(dev, "Failed to get clock\n"); + clk_unprepare(data->clk); ///// <---- + return ret; + } + + break; --------------------------->8--------------------------------- Or with better 'goto' flow. > + } > + break; > case SOC_ARCH_EXYNOS5433: > case SOC_ARCH_EXYNOS7: > data->sclk = devm_clk_get(dev, "tmu_sclk"); Also, you should revisit the 'goto' cleanup section at the bottom. Regards, Lukasz
Hi Lukasz, Thanks for your review comments On Fri, 28 Feb 2025 at 20:07, Lukasz Luba <lukasz.luba@arm.com> wrote: > > Hi Anand, > > On 2/16/25 19:58, 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. > > So IIUC there was no need to init that clock for other types of SoCs... > Do we know that for sure (e.g. from other TRMs)? This clock (clk) is utilized for the Thermal Management Unit (TMU) and the GPU in the Exynos 542x, as specified in the user manual. > > If that was the case, then simplification here can go further, but after > some fixes. > > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > v3: improve the commit message > > --- > > drivers/thermal/samsung/exynos_tmu.c | 26 +++++++++++++------------- > > 1 file changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c > > index 47a99b3c5395..9c138772d380 100644 > > --- a/drivers/thermal/samsung/exynos_tmu.c > > +++ b/drivers/thermal/samsung/exynos_tmu.c > > @@ -1040,19 +1040,6 @@ static int exynos_tmu_probe(struct platform_device *pdev) > > if (IS_ERR(data->clk)) > > return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n"); > > > > - 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); > > - if (ret) { > > - dev_err(dev, "Failed to get clock\n"); > > - return ret; > > - } > > - } > > - > > ret = clk_prepare(data->clk); > > Here the data->clk is now used in different order. Ok. > > > if (ret) { > > dev_err(dev, "Failed to get clock\n"); > > @@ -1060,6 +1047,19 @@ static int exynos_tmu_probe(struct platform_device *pdev) > > } > > > > switch (data->soc) { > > + case SOC_ARCH_EXYNOS5420_TRIMINFO: > > + data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif"); > > + if (IS_ERR(data->clk_sec)) { > > + return dev_err_probe(dev, PTR_ERR(data->clk_sec), > > + "Failed to get triminfo clock\n"); > > Then here you shouldn't simply copy the old code. Now the data->clk > is first, so should be 'clk_unprepare()' before return of the function. > > > + } else { > > You can get rid of this 'else' above and still be safe in your > refactoring. > > > + ret = clk_prepare(data->clk_sec); > > + if (ret) { > > + dev_err(dev, "Failed to get clock\n"); > > + return ret; > > + } > > Here you can further simplify this to something like: > -----------------------8<------------------------------------- > > + case SOC_ARCH_EXYNOS5420_TRIMINFO: > + data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif"); > + if (IS_ERR(data->clk_sec)) { > + clk_unprepare(data->clk); ///// <---- > + return dev_err_probe(dev, PTR_ERR(data->clk_sec), > + "Failed to get triminfo clock\n"); > + } > + ret = clk_prepare(data->clk_sec); > + if (ret) { > + dev_err(dev, "Failed to get clock\n"); > + clk_unprepare(data->clk); ///// <---- > + return ret; > + } > + > + break; Ok > > --------------------------->8--------------------------------- > > Or with better 'goto' flow. > > > + } > > + break; > > case SOC_ARCH_EXYNOS5433: > > case SOC_ARCH_EXYNOS7: > > data->sclk = devm_clk_get(dev, "tmu_sclk"); > > > Also, you should revisit the 'goto' cleanup section at the bottom. Thanks. I will recheck and update the code for the next version. > > Regards, > Lukasz Thanks -Anand
Hi All, This patch series is a rework of my previous patch series [1], where the code changes were not adequately justified. In this new series, I have improved the commit subject and commit message to better explain the changes. Tested on Odroid U3 amd XU4 SoC boards. [1] https://lore.kernel.org/all/20220515064126.1424-1-linux.amoon@gmail.com/ [0] https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#m77e57120d230d57f34c29e1422d7fc5f5587ac30 Thanks -Anand Anand Moon (4): drivers/thermal/exynos: Refactor clk_sec initialization inside SOC-specific case drivers/thermal/exymos: Remove redundant IS_ERR() checks for clk_sec clock drivers/thermal/exymos: Fixed the efuse min max value for exynos5422 drivers/thermal/exymos: Use guard notation when acquiring mutex drivers/thermal/samsung/exynos_tmu.c | 78 ++++++++++++++-------------- 1 file changed, 39 insertions(+), 39 deletions(-) base-commit: ba643b6d84409e8a9057d5bdd6dd99255b1a88fe