[PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

Hi Krzysztof,

On Wed, 18 May 2022 at 12:58, Krzysztof Kozlowski
<krzysztof.kozlowski at linaro.org> wrote:
>
> On 17/05/2022 20:43, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > On Sun, 15 May 2022 at 15:11, Krzysztof Kozlowski
> > <krzysztof.kozlowski at linaro.org> 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 at gmail.com>
> >>> Signed-off-by: Anand Moon <linux.amoon at gmail.com>
> >>> ---
> >>> 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