| Message ID | 20220515064126.1424-3-linux.amoon@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Exynos Thermal code inprovement | expand |
Hi Krzysztof, On Wed, 18 May 2022 at 12:58, Krzysztof Kozlowski <krzysztof.kozlowski@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@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@gmail.com> > >>> Signed-off-by: Anand Moon <linux.amoon@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
On 21/05/2022 11:51, Anand Moon wrote: > Hi Krzysztof, > > On Wed, 18 May 2022 at 12:58, Krzysztof Kozlowski > <krzysztof.kozlowski@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@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. What was wrong with original order? You still did not explain it. > >> 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@gmail.com> >>>>> Signed-off-by: Anand Moon <linux.amoon@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); Which is wrong and does not make any sense. This is third question - why the main clock is prepared within 'else' branch? Best regards, Krzysztof
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); if (ret) { dev_err(&pdev->dev, "Failed to get clock\n"); goto err_sensor; } } - ret = clk_prepare_enable(data->clk); - if (ret) { - dev_err(&pdev->dev, "Failed to get clock\n"); - goto err_clk_sec; - } - 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; + 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; + goto err_clk_sec; } } break; @@ -1119,13 +1118,13 @@ static int exynos_tmu_probe(struct platform_device *pdev) err_thermal: thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd); -err_sclk: - clk_disable_unprepare(data->sclk); -err_clk: - clk_disable_unprepare(data->clk); err_clk_sec: if (!IS_ERR(data->clk_sec)) clk_disable_unprepare(data->clk_sec); +err_sclk: + clk_disable_unprepare(data->sclk); +err_clk_apbif: + clk_disable_unprepare(data->clk); err_sensor: if (!IS_ERR(data->regulator)) regulator_disable(data->regulator);
Reorder the tmu_gpu clock initialization for exynos5422 SoC. Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Signed-off-by: Anand Moon <linux.amoon@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(-)