Hi Krzysztof,
On Wed, 18 May 2022 at 12:58, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 17/05/2022 20:43, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > On Sun, 15 May 2022 at 15:11, Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>
> >> On 15/05/2022 08:41, Anand Moon wrote:
> >>> Reorder the tmu_gpu clock initialization for exynos5422 SoC.
> >>
> >> Why?
> > It just code reorder
>
> I know what it is. I asked why. The answer in English to question "Why"
> is starting with "Because".
>
> You repeated again the argument what are you doing to my question "Why
> are you doing it".
>
tmu_triminfo_apbif is not a core driver to all the Exynos SOC board
it is only used by the Exynos542x SOC family
If we look into the original code its place in between
the devm_clk_get(data->clk) and clk_prepare(data->clk)
after this change, the code is in the correct order of initialization
of the clock.
> It was the same before, many, many times. It's a waste of reviewers
> time, because you receive a review and you do not implement the feedback.
>
> >>
> >>>
> >>> Cc: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> >>> Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
> >>> ---
> >>> v1: split the changes and improve the commit messages
> >>> ---
> >>> drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
> >>> 1 file changed, 21 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> >>> index 75b3afadb5be..1ef90dc52c08 100644
> >>> --- a/drivers/thermal/samsung/exynos_tmu.c
> >>> +++ b/drivers/thermal/samsung/exynos_tmu.c
> >>> @@ -1044,42 +1044,41 @@ static int exynos_tmu_probe(struct platform_device *pdev)
> >>> dev_err(&pdev->dev, "Failed to get clock\n");
> >>> ret = PTR_ERR(data->clk);
> >>> goto err_sensor;
> >>> - }
> >>> -
> >>> - data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
> >>> - if (IS_ERR(data->clk_sec)) {
> >>> - if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
> >>> - dev_err(&pdev->dev, "Failed to get triminfo clock\n");
> >>> - ret = PTR_ERR(data->clk_sec);
> >>> - goto err_sensor;
> >>> - }
> >>> } else {
> >>> - ret = clk_prepare_enable(data->clk_sec);
> >>> + ret = clk_prepare_enable(data->clk);
> >>
> >> This looks a bit odd. The clock was before taken unconditionally, not
> >> within "else" branch...
> >
> > The whole *clk_sec* ie tmu_triminfo_apbif clock enable is being moved
> > down to the switch case.
> > tmu_triminfo_apbif clock is not used by Exynos4412 and Exynos5433 and
> > Exynos7 SoC.
>
> This is not the answer. Why are you preparing data->clk within else{}
> branch?
>
After cleanly applying the patches I see the below changes.
if you want me to remove the else part below and keep
the original code I am ok.
data->clk = devm_clk_get(&pdev->dev, "tmu_apbif");
if (IS_ERR(data->clk)) {
dev_err(&pdev->dev, "Failed to get clock\n");
ret = PTR_ERR(data->clk);
goto err_sensor;
} else {
ret = clk_prepare_enable(data->clk);
if (ret) {
dev_err(&pdev->dev, "Failed to get clock\n");
goto err_sensor;
}
}
switch (data->soc) {
case SOC_ARCH_EXYNOS5420_TRIMINFO:
data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
if (IS_ERR(data->clk_sec)) {
dev_err(&pdev->dev, "Failed to get triminfo clock\n");
ret = PTR_ERR(data->clk_sec);
goto err_clk_apbif;
} else {
ret = clk_prepare_enable(data->clk_sec);
if (ret) {
dev_err(&pdev->dev, "Failed to get clock\n");
goto err_clk_apbif;
}
}
break;
case SOC_ARCH_EXYNOS5433:
case SOC_ARCH_EXYNOS7:
data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk");
if (IS_ERR(data->sclk)) {
dev_err(&pdev->dev, "Failed to get sclk\n");
ret = PTR_ERR(data->sclk);
goto err_clk_sec;
} else {
ret = clk_prepare_enable(data->sclk);
if (ret) {
dev_err(&pdev->dev, "Failed to enable sclk\n");
goto err_clk_sec;
}
}
break;
default:
break;
}
>
> Best regards,
> Krzysztof
Thanks and Regards
-Anand
