[PATCH v4 0/4] Exynos Thermal code improvement

[PATCH v4 0/4] Exynos Thermal code improvement

[Anand Moon]

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.

v4: Tried to address Lukasz review coments.

Tested on Odroid U3 amd XU4 SoC boards.

[2] https://lore.kernel.org/all/20250216195850.5352-2-linux.amoon@gmail.com/
[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 | 87 +++++++++++++++-------------
 1 file changed, 46 insertions(+), 41 deletions(-)


base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
-- 
2.48.1

[PATCH v4 1/4] drivers/thermal/exynos: Refactor clk_sec initialization inside SOC-specific case

[Anand Moon]

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@gmail.com>
---
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 47a99b3c53958..3657920de0004 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");
-
-	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);
+		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);
 	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.48.1

Re: [PATCH v4 1/4] drivers/thermal/exynos: Refactor clk_sec initialization inside SOC-specific case

[Anand Moon]

Hi All,

On Mon, 10 Mar 2025 at 20:05, Anand Moon <linux.amoon@gmail.com> 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@gmail.com>

On Exynos4412 Odroid U3 uses the clocks name
      clock-names = "tmu_apbif";

On Exynos5422 Odroid XU4 uses the clocks name
       clock-names = "tmu_apbif", "tmu_triminfo_apbif";

So Exynos 5433 and Exynos7  SoC use the clocks name
      clock-names = "tmu_apbif", "tmu_sclk";

As per my understanding, there could be a common case for GPU clock in
TMU driver
which could simplify the code, any thoughts

-----------------------8<-------------------------------------
switch (data->soc) {
        case SOC_ARCH_EXYNOS5420_TRIMINFO:
        case SOC_ARCH_EXYNOS5433:
        case SOC_ARCH_EXYNOS7:

Thanks
-Anand

[PATCH v4 2/4] drivers/thermal/exymos: Remove redundant IS_ERR() checks for clk_sec clock

[Anand Moon]

Remove unnecessary IS_ERR() checks for the clk_sec clock,
the clk_enable() and clk_disable() functions can handle NULL clock
pointers, so the additional checks are redundant and have been removed
to simplify the code.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v4: drop IE_ERR() for clk_unprepare() as its handle in earlier code.
v3: improve the commit message.
---
 drivers/thermal/samsung/exynos_tmu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 3657920de0004..ac3b9d2c900ca 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -258,8 +258,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 
 	mutex_lock(&data->lock);
 	clk_enable(data->clk);
-	if (!IS_ERR(data->clk_sec))
-		clk_enable(data->clk_sec);
+	clk_enable(data->clk_sec);
 
 	status = readb(data->base + EXYNOS_TMU_REG_STATUS);
 	if (!status) {
@@ -269,8 +268,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 		data->tmu_clear_irqs(data);
 	}
 
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
+	clk_disable(data->clk_sec);
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
-- 
2.48.1

[PATCH v4 3/4] drivers/thermal/exymos: Fixed the efuse min max value for exynos5422

[Anand Moon]

As per Exynos5422 user manual e-Fuse range min~max range is 16~76.
if e-Fuse value is out of this range, then thermal sensor may not
sense thermal data properly. Refactors the efuse value
initialization logic within exynos_map_dt_data function by
replacing the nested if-else statements with a switch statement.
Ensures proper initialization of efuse values based on the SOC type.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index ac3b9d2c900ca..a71cde0a4b17e 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -899,12 +899,23 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 		data->gain = 8;
 		data->reference_voltage = 16;
 		data->efuse_value = 55;
-		if (data->soc != SOC_ARCH_EXYNOS5420 &&
-		    data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO)
+		data->max_efuse_value = 100;
+		switch (data->soc) {
+		case SOC_ARCH_EXYNOS3250:
+		case SOC_ARCH_EXYNOS4412:
+		case SOC_ARCH_EXYNOS5250:
+		case SOC_ARCH_EXYNOS5260:
 			data->min_efuse_value = 40;
-		else
+			break;
+		case SOC_ARCH_EXYNOS5420:
+		case SOC_ARCH_EXYNOS5420_TRIMINFO:
+			data->min_efuse_value = 16;
+			data->max_efuse_value = 76;
+			break;
+		default:
 			data->min_efuse_value = 0;
-		data->max_efuse_value = 100;
+			break;
+		}
 		break;
 	case SOC_ARCH_EXYNOS5433:
 		data->tmu_set_low_temp = exynos5433_tmu_set_low_temp;
-- 
2.48.1

[PATCH v4 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Anand Moon]

Using guard notation makes the code more compact and error handling
more robust by ensuring that mutexes are released in all code paths
when control leaves critical section.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v4: used DEFINE_GUARD macro to guard exynos_tmu_data structure.
    However, incorporating guard(exynos_tmu_data)(data); results
    in a recursive deadlock with the mutex during initialization, as this
    data structure is common to all the code configurations of Exynos TMU
v3: New patch
---
 drivers/thermal/samsung/exynos_tmu.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index a71cde0a4b17e..85f88c5e0f11c 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/cleanup.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
@@ -199,6 +200,9 @@ struct exynos_tmu_data {
 	void (*tmu_clear_irqs)(struct exynos_tmu_data *data);
 };
 
+DEFINE_GUARD(exynos_tmu_data, struct exynos_tmu_data *,
+	     mutex_lock(&_T->lock), mutex_unlock(&_T->lock))
+
 /*
  * TMU treats temperature as a mapped temperature code.
  * The temperature is converted differently depending on the calibration type.
@@ -256,7 +260,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 	unsigned int status;
 	int ret = 0;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 	clk_enable(data->clk_sec);
 
@@ -270,7 +274,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 
 	clk_disable(data->clk_sec);
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return ret;
 }
@@ -292,13 +295,12 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
 		return ret;
 	}
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 
 	data->tmu_set_crit_temp(data, temp / MCELSIUS);
 
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return 0;
 }
@@ -325,12 +327,11 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 	data->tmu_control(pdev, on);
 	data->enabled = on;
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 }
 
 static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
@@ -645,7 +646,7 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
 		 */
 		return -EAGAIN;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 
 	value = data->tmu_read(data);
@@ -655,7 +656,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
 		*temp = code_to_temp(data, value) * MCELSIUS;
 
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return ret;
 }
@@ -720,11 +720,10 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
 	if (temp && temp < MCELSIUS)
 		goto out;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 	data->tmu_set_emulation(data, temp);
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 	return 0;
 out:
 	return ret;
@@ -760,14 +759,13 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
 
 	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 
 	/* TODO: take action based on particular interrupt */
 	data->tmu_clear_irqs(data);
 
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return IRQ_HANDLED;
 }
@@ -987,7 +985,7 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
 {
 	struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 
 	if (low > INT_MIN)
@@ -1000,7 +998,6 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
 		data->tmu_disable_high(data);
 
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return 0;
 }
-- 
2.48.1

Re: [PATCH v4 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Krzysztof Kozlowski]

On 10/03/2025 15:34, Anand Moon wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that mutexes are released in all code paths
> when control leaves critical section.
> 

Subject: typo, exynos

> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v4: used DEFINE_GUARD macro to guard exynos_tmu_data structure.
>     However, incorporating guard(exynos_tmu_data)(data); results
>     in a recursive deadlock with the mutex during initialization, as this
>     data structure is common to all the code configurations of Exynos TMU
> v3: New patch

If you ever use cleanup or guards, you must build your code with recent
clang and W=1. Failure to do so means you ask reviewers manually to spot
issues not visible in the context, instead of using tools. It's a NAK
for me.

> ---
>  drivers/thermal/samsung/exynos_tmu.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index a71cde0a4b17e..85f88c5e0f11c 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/cleanup.h>
>  #include <linux/io.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> @@ -199,6 +200,9 @@ struct exynos_tmu_data {
>  	void (*tmu_clear_irqs)(struct exynos_tmu_data *data);
>  };
>  
> +DEFINE_GUARD(exynos_tmu_data, struct exynos_tmu_data *,

I do not understand why do you need custom guard.

> +	     mutex_lock(&_T->lock), mutex_unlock(&_T->lock))
> +
>  /*
>   * TMU treats temperature as a mapped temperature code.
>   * The temperature is converted differently depending on the calibration type.
> @@ -256,7 +260,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>  	unsigned int status;
>  	int ret = 0;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);

Which you do not use... Please don't use cleanup.h if you do not know
it. It leads to bugs.


Best regards,
Krzysztof

Re: [PATCH v4 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Anand Moon]

Hi Krzysztof,

Thanks for your review comments.

On Tue, 11 Mar 2025 at 23:00, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 10/03/2025 15:34, Anand Moon wrote:
> > Using guard notation makes the code more compact and error handling
> > more robust by ensuring that mutexes are released in all code paths
> > when control leaves critical section.
> >
>
> Subject: typo, exynos
Ok.
>
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v4: used DEFINE_GUARD macro to guard exynos_tmu_data structure.
> >     However, incorporating guard(exynos_tmu_data)(data); results
> >     in a recursive deadlock with the mutex during initialization, as this
> >     data structure is common to all the code configurations of Exynos TMU
> > v3: New patch
>
> If you ever use cleanup or guards, you must build your code with recent
> clang and W=1. Failure to do so means you ask reviewers manually to spot
> issues not visible in the context, instead of using tools. It's a NAK
> for me.
 Ok, I will check this next time before submitting the changes.
>
> > ---
> >  drivers/thermal/samsung/exynos_tmu.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > index a71cde0a4b17e..85f88c5e0f11c 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -12,6 +12,7 @@
> >   */
> >
> >  #include <linux/clk.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/io.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/module.h>
> > @@ -199,6 +200,9 @@ struct exynos_tmu_data {
> >       void (*tmu_clear_irqs)(struct exynos_tmu_data *data);
> >  };
> >
> > +DEFINE_GUARD(exynos_tmu_data, struct exynos_tmu_data *,
>
> I do not understand why do you need custom guard.

I thought this should add a global guard to exynos_tmu_data using
mutex_lock and mutex_unlock.
I'll drop this if it turns out to be unnecessary.
>
> > +          mutex_lock(&_T->lock), mutex_unlock(&_T->lock))
> > +
> >  /*
> >   * TMU treats temperature as a mapped temperature code.
> >   * The temperature is converted differently depending on the calibration type.
> > @@ -256,7 +260,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >       unsigned int status;
> >       int ret = 0;
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
>
> Which you do not use... Please don't use cleanup.h if you do not know
> it. It leads to bugs.
>
Ok, I will drop this include of cleanup.h.
>
> Best regards,
> Krzysztof

Thanks
-Anand

Re: [PATCH v4 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Krzysztof Kozlowski]

On 12/03/2025 15:59, Anand Moon wrote:
>>
>>> +          mutex_lock(&_T->lock), mutex_unlock(&_T->lock))
>>> +
>>>  /*
>>>   * TMU treats temperature as a mapped temperature code.
>>>   * The temperature is converted differently depending on the calibration type.
>>> @@ -256,7 +260,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>       unsigned int status;
>>>       int ret = 0;
>>>
>>> -     mutex_lock(&data->lock);
>>> +     guard(mutex)(&data->lock);
>>
>> Which you do not use... Please don't use cleanup.h if you do not know
>> it. It leads to bugs.
>>
> Ok, I will drop this include of cleanup.h.

So the guards as well...

Best regards,
Krzysztof

Re: [PATCH v4 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Anand Moon]

Hi Krzysztof,

On Thu, 13 Mar 2025 at 13:19, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 12/03/2025 15:59, Anand Moon wrote:
> >>
> >>> +          mutex_lock(&_T->lock), mutex_unlock(&_T->lock))
> >>> +
> >>>  /*
> >>>   * TMU treats temperature as a mapped temperature code.
> >>>   * The temperature is converted differently depending on the calibration type.
> >>> @@ -256,7 +260,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >>>       unsigned int status;
> >>>       int ret = 0;
> >>>
> >>> -     mutex_lock(&data->lock);
> >>> +     guard(mutex)(&data->lock);
> >>
> >> Which you do not use... Please don't use cleanup.h if you do not know
> >> it. It leads to bugs.
> >>
> > Ok, I will drop this include of cleanup.h.
>
> So the guards as well...
>
Ok I will drop this patch thanks.

> Best regards,
> Krzysztof

Thanks
-Anand