Re: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new hardware and update TMU interface

[Tudor Ambarus <tudor.ambarus@linaro.org>]

Hi, Shin Son,

Just trivial notes on registers description for now.

On 11/13/25 8:40 AM, Shin Son wrote:
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/ 
> thermal/ samsung/exynos_tmu.c index 47a99b3c5395..8fa188928b79 
> 100644 --- a/ drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/ 
> thermal/samsung/ exynos_tmu.c @@ -121,8 +121,51 @@
> 
> #define EXYNOS_NOISE_CANCEL_MODE		4
> 
> +/* ExynosAutov920 specific registers */ +#define 
> EXYNOSAUTOV920_SLOPE_COMP		25 +#define 
> EXYNOSAUTOV920_SLOPE_COMP_MASK		0xf

Register fields shall be named
SOC_REG_NAME_FIELD_NAME

If you include <linux/bits.h> you can substitute the above 2 definitions
with just one:
EXYNOSAUTOV920_TRIMINFO_SLOPE_COMP	GENMASK(28, 25)

and later on in the code, instead of doing the shift and the mask, you
can include <linux/bitfield.h> and do:

data->slope_comp = FIELD_GET(EXYNOSAUTOV920_TRIMINFO_SLOPE_COMP, val);

btw, above matches the GS101 definitions.

> +#define EXYNOSAUTOV920_CALIB_SEL_TEMP		30 +#define 
> EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK	0x2 +

is this BIT(31)?
EXYNOSAUTOV920_TRIMINFO2_CALIB_SEL_TEMP		BIT(31)

GS101 differs, it has this field defined at:
GS101_TRIMINFO_CALIB_SEL_TEMP			BIT(0)
where TRIMINFO is at Base Address + 0x0, not at Base Address + 0x4 as in
your case.

> +#define EXYNOSAUTOV920_SENSOR0_TRIM_INFO	0x10

GS101 does not have any SENSOR0 in the reg name, so maybe rename it to:
#define EXYNOSAUTOV920_TRIMINFO0		0x10

> +#define EXYNOSAUTOV920_TRIM_MASK		0x1ff +#define 
> EXYNOSAUTOV920_TRIMINFO_25_SHIFT	0 +#define 
> EXYNOSAUTOV920_TRIMINFO_85_SHIFT	9

#define EXYNOSAUTOV920_TRIMINFO_85_P0		GENMASK(17, 9)
#define EXYNOSAUTOV920_TRIMINFO_25_P0		GENMASK(8, 0)

> + +#define EXYNOSAUTOV920_TMU_REG_TRIMINFO2	0x04

Is this a TRIMINFO_CONFIG2 register? I don't have such thing on GS101.

> + +#define EXYNOSAUTOV920_TMU_REG_THRESHOLD(p)	(((p)) * 0x50 + 
> 0x00d0)

#define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6(p)	(((p)) * 0x50 + 0xd0)
and then:
#define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6_RISE7 	GENMASK(24, 16)
#define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6_RISE6 	GENMASK(8, 0)
you'll stop passing the shift and mask as function arguments :)


> +#define EXYNOSAUTOV920_TMU_REG_INTEN(p)		(((p)) * 0x50 + 0x00f0)

#define EXYNOSAUTOV920_INTEN(p)				(((p)) * 0x50 + 0xf0)

I see you use just BIT(7) from this register. Let's define it and stop passing
the bit offset as function argument:

#define EXYNOSAUTOV920_INTEN_RISE7			BIT(7)

> +#define EXYNOSAUTOV920_TMU_REG_INT_PEND(p)	(((p)) * 0x50 + 0x00f8)

#define EXYNOSAUTOV920_PEND(p)				(((p)) * 0x50 + 0xf8)

Are you using GENMASK(15, 0) for this register?

On GS101 GENMASK(15, 9) is reserved. Here's how the bits are defined for GS101:

#define EXYNOSAUTOV920_PEND_FALL(i)			BIT(16 + (i))
#define EXYNOSAUTOV920_PEND_RISE_MASK			GENMASK(23, 16)
#define EXYNOSAUTOV920_PEND_RISE(i)			BIT(i)
#define EXYNOSAUTOV920_PEND_RISE_MASK			GENMASK(8, 0)

Would you please verify and let me know if EXYNOSAUTOV920 differs or not?

> +#define EXYNOSAUTOV920_CURRENT_TEMP_P1_P0	0x084

no leading 0
#define EXYNOSAUTOV920_CURRENT_TEMP_P1_P0		0x84
then define the fields:
#define EXYNOSAUTOV920_CURRENT_TEMP_P1			GENMASK(24, 16)
#define EXYNOSAUTOV920_CURRENT_TEMP_P0			GENMASK(8, 0)


> +#define EXYNOSAUTOV920_TMU_REG_EMUL_CON		0x0b0

no TMU_REG in the name, no leading 0, define the fields as GENMASK
#define EXYNOSAUTOV920_EMUL_CON				0xb0
#define EXYNOSAUTOV920_EMUL_CON_EMUL_NEXT_TIME		GENMASK(31, 16)
#define EXYNOSAUTOV920_EMUL_CON_EMUL_NEXT_DATA		GENMASK(15, 7)
#define EXYNOSAUTOV920_EMUL_CON_EMUL_EN			BIT(0)


> +
> +#define EXYNOSAUTOV920_TMU_REG_CONTROL		0x50

no reg in the name, control0
#define EXYNOSAUTOV920_TMU_CONTROL0			0x50

define fields as GENMASK and BIT

> +#define EXYNOSAUTOV920_TMU_REG_CONTROL1		0x54

ditto

> +#define EXYNOSAUTOV920_TMU_REG_AVG_CONTROL	0x58

ditto

> +#define EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL	0x70

no TMU in the name, respect the registers name from the datasheet please.
define the full genmask
#define EXYNOSAUTOV920_SAMPLING_INTERVAL_MASK		GENMASK(31, 0)

> +#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0	0x74
> +#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1	0x78

no TMU_REG in the name, define fields> +
> +#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT		8
> +#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK		0x1f
> +#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT	3
> +#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK		0xf
> +#define EXYNOSAUTOV920_TMU_NUM_PROBE_MASK		0xf
> +#define EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT		16
> +#define EXYNOSAUTOV920_TMU_LPI_MODE_MASK		1
> +#define EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT		10

you won't need these if you define the register fields, isn't it?> +
> +#define EXYNOSAUTOV920_TMU_AVG_CON_UPDATE		0x0008011a

no leading zeros. You better construct the fields dynamically, by using bitfields,
no full register magic number, humans don't understand this.

> +#define EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE	0x030003c0
> +#define EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE	0x03c0004d

same for both

> +
>  #define MCELSIUS	1000
>  
> +#define EXYNOS_DEFAULT_SENSOR_COUNT			1
> +#define EXYNOS_MAX_SENSOR_COUNT	
would it make sense to have the tzd_array to fit just the sensor count that
we're using so that we don't waste memory? i.e. allocate tzd_array dynamically.

Looking at the exynosautov9 registers that you described and comparing them with
gs101 I see just 2 differences:
1/ exnosautov2 has a TRIMINFO_CONFIG2 register, while gs101 doesn't
2/ EXYNOSAUTOV920_PEND register fields differ from GS101

Given the similarities, and considering the EXYNOS9_ registers rename from:
https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-5-youngmin.nam@samsung.com/
would it make sense to use the SoC-era name instead of specific SoC, i.e.
s/EXYNOSAUTOV920_/EXYNOS9_ and use the latter for both exynosautov9 and gs101?

Cheers,
ta