Hi Mateusz,

Thanks for your review comments..

On Tue, 19 Aug 2025 at 18:47, Mateusz Majewski [off-list ref] wrote:

Hello :)

quoted

+/* Map Rise and Falling edges for IRQ Clean */
+struct tmu_irq_map {
+     u32 fall[3];
+     u32 rise[3];
+};

Ops, this is an overkill approach that could be simplified further.

static void exynos4412_tmu_clear_irqs(struct exynos_tmu_data *data)
{
        unsigned int val_irq, clear_irq = 0;
        u32 tmu_intstat = data->tmu_intstat;
        u32 tmu_intclear = data->tmu_intclear;
        static const u32 irq_bits[] = {
                BIT(0), BIT(4), BIT(8),   /* Rising edge */
                BIT(12), BIT(16), BIT(20) /* Falling edge */
        };

        val_irq = readl(data->base + tmu_intstat);

        /* Set SoC-specific interrupt bit mappings */
        for (int i = 0; i < ARRAY_SIZE(irq_bits); i++) {
                if (val_irq & irq_bits[i])
                        clear_irq |= irq_bits[i];
        }

        if (clear_irq)
                writel(clear_irq, data->base + tmu_intclear);
}

Hmm, we can probably get away with less interrupts. We actually only
enable one fall interrupt in tmu_set_low_temp and one rise interrupt in
tmu_set_high_temp.

Got it — we need to enable INTEN for both rising and falling edges,
specific to each SoC.
So each SoC has a one-to-one mapping of INTEN with INTSTAT and INTCLEAR bits
for rising and falling edge detection:.

Exynos4412
   Falling edge: bits 20, 16, 12
   Rising edge: bits 8, 4, 0
Exynos5422
  Falling edge: bits 24, 20, 16
  Rising edge: bits 8, 4, 0
Exynos5433
  Falling edge: bits 23, 17, 16
  Rising edge: bits 7, 1, 0

So we need to enable INTEN like below.

 static void exynos4412_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
 {
        exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_FALL, 0, temp);
-       exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
-                             EXYNOS_TMU_INTEN_FALL0_SHIFT, true);
+       for (int i = 0; i < 3; i++)
+               exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+                                     EXYNOS_TMU_INTEN_FALL0_SHIFT + i
* 4, true);
 }

 static void exynos4412_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
 {
        exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_RISE, 8, temp);
-       exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
-                             EXYNOS_TMU_INTEN_RISE0_SHIFT + 4, true);
+       for (int i = 0; i < 3; i++)
+               exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+                             EXYNOS_TMU_INTEN_RISE0_SHIFT + 1 * 4, true);
 }

Regarding tmu_set_crit_temp, on SoCs that have hardware thermal tripping
there is nothing to clear. On others we will reboot immediately anyway,
though maybe there is nothing wrong with clearing the interrupt
beforehand? Regardless of this, there is only a rise critical
temperature interrupt, we never set a matching fall interrupt.

As per my understanding, it depends on the calibration consists of reading the
measured data from e-fuse and wrote the calibrated threshold temperature to
generate interrupts into trigger level 0-3

THRES_TEMP_RISE and THRES_TEMP_FALL.

When the current temperature exceeds a threshold rise temperature,
 then it generates the corresponding interrupt (INTREQ_RISE[2:0]).
When the current temperature goes below a threshold fall temperature, t
hen it generates a corresponding interrupt (INTREQ_FALL[2:0].

It also depends on the sample interval

Maybe it would also be good to add a bool to this struct containing
information about whether a fall interrupt is in use, and reuse
the same logic for 4210?

(Nitpick: I am not a native speaker of English, but I think "clean" and
"clear" have slightly different meanings, and the rest of the code
consistently uses "clear", so it would be worthwhile to also use "clear"
here.)

quoted

+             break;

Maybe put irq_map inside exynos_tmu_data? exynos_map_dt_data has a
switch block that is quite similar, in that it also matches on the SoC
type. This way also there is no need to have a fallback.

I want to avoid this If you have any design feedback, plz let me know.

I had attempted to refactor exynos_tmu_data by splitting it per-SoC,
assigning dedicated callback functions for each variant. The goal was
to eliminate
the need for data->soc checks and make the code more modular.
However, the approach didn’t work as expected—devm_thermal_of_zone_register()
wouldn’t bind the sensor-specific callbacks correctly.

static const struct thermal_zone_device_ops exynos_sensor_ops = {
.get_temp = exynos_get_temp,
.set_emul_temp = exynos_tmu_set_emulation,
.set_trips = exynos_set_trips,
};

Kind regards,
Mateusz Majewski

Thanks
-Anand