Message-ID: <000001dc5d2a$0697bf10$13c73d30$@samsung.com>
Date: Mon, 24 Nov 2025 19:06:33 +0900
From: 손신 <shin.son@...sung.com>
To: "'Daniel Lezcano'" <daniel.lezcano@...aro.org>, "'Bartlomiej
 Zolnierkiewicz'" <bzolnier@...il.com>, "'Krzysztof Kozlowski'"
	<krzk@...nel.org>, "'Rafael J . Wysocki'" <rafael@...nel.org>, "'Zhang Rui'"
	<rui.zhang@...el.com>, "'Lukasz Luba'" <lukasz.luba@....com>, "'Rob
	Herring'" <robh@...nel.org>, "'Conor Dooley'" <conor+dt@...nel.org>, "'Alim
	Akhtar'" <alim.akhtar@...sung.com>
Cc: "'Henrik Grimler'" <henrik@...mler.se>, <linux-pm@...r.kernel.org>,
	<linux-samsung-soc@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<shin.son@...sung.com>
Subject: RE: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new hardware
 and update TMU interface

Hello, Daniel Lezcano

> On 11/13/25 07:40, Shin Son wrote:
> > +	if (data->soc == SOC_ARCH_EXYNOSAUTOV920 && code_diff < 0)
> > +		temp = temp * 65 / (57 + data->slope_comp);
> 
> No litterals, comments, etc ...

I'll move those fomulas into the variant data via the .data field in the of_device_id match table.

> > +static void update_con_reg(struct exynos_tmu_data *data) {
> > +	u32 val, t_buf_vref_sel, t_buf_slope_sel;
> > +
> > +	val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> > +	t_buf_vref_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT)
> > +				& EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK;
> > +	t_buf_slope_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT)
> > +				& EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK;
> > +
> > +	val = readl(data->base +  EXYNOSAUTOV920_TMU_REG_CONTROL);
> > +	val &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK <<
> EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> > +	val |= (t_buf_vref_sel << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> > +	val &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK <<
> EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> > +	val |= (t_buf_slope_sel << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> > +	writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> > +
> > +	val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> > +	val &= ~(EXYNOSAUTOV920_TMU_NUM_PROBE_MASK <<
> EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> > +	val &= ~(EXYNOSAUTOV920_TMU_LPI_MODE_MASK <<
> EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT);
> > +	val |= (data->sensor_count << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> > +	writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> > +
> > +	writel(1, data->base + EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL);
> > +	writel(EXYNOSAUTOV920_TMU_AVG_CON_UPDATE, data->base +
> EXYNOSAUTOV920_TMU_REG_AVG_CONTROL);
> > +	writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE,
> > +	       data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0);
> > +	writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE,
> > +	       data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1);
> > +}
> > +
> 
> This is unreadable; please make it understandable for those who don’t have
> the documentation (explicit static inline functions, comments, etc ...).

I'll restructure this code by introducing explicit static inline helper functions and proper comments to improve readability.

> > +static void exynosautov920_tmu_disable_high(struct exynos_tmu_data
> > +*data) {
> > +	/* Again, this is handled by polling. */ }
> 
> The driver would deserve some cleanups. Instead of having empty callbacks,
> check in exynos_set_trips() if the ops is !NULL. Then remove all no-op ops.

Ok, I'll update exynos_set_trips() to check for NULL ops and remove the no-op callbacks accordingly.

> > +static void exynosautov920_tmu_set_crit_temp(struct exynos_tmu_data
> > +*data, u8 temp) {
> > +	unsigned int idx;
> > +
> > +	for (idx = 0; idx < data->sensor_count; idx++) {
> > +		if (!data->tzd_array[idx])
> > +			continue;
> > +
> > +		exynos_tmu_update_temp(data,
> EXYNOSAUTOV920_TMU_REG_THRESHOLD(idx), 16, temp);
> > +		exynos_tmu_update_bit(data,
> EXYNOSAUTOV920_TMU_REG_INTEN(idx), 7, true);
> > +	}
> > +}
> 
> There is something wrong in the driver design.
> 
> exynosautov920_tmu_set_crit_temp() is called from
> exynos_thermal_zone_configure() and the routine above sets the temperature
> on all the thermal zone while this one is retrieved from one thermal zone.
> 
> Which results in:
> 
> 	for all tz do;
> 		for all tz do;
> 			if !tz then continue;
> 			set_crit_temp(tz)
> 
> No, this driver needs to be revisited and cleanup before sending changes
> for multiple sensors support.
> 
> What percentage of code sharing is there with the existing driver ?

Overall, I would say that roughly 60% of the logic can be shared.
The temperature reading and emulation paths are similar, but the initialization sequence differs significantly.

Given this level of divergence, would introducing a separate driver 
(instead of extending the current one with many special-case paths) be acceptable?

> > +static void exynosautov920_tmu_initialize(struct platform_device
> > +*pdev) {
> > +	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > +	unsigned int val;
> > +
> > +	data->tmu_control(pdev, false);
> > +
> > +	update_con_reg(data);
> > +
> > +	val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> > +	data->cal_type = TYPE_TWO_POINT_TRIMMING;
> > +	data->slope_comp = (val >> EXYNOSAUTOV920_SLOPE_COMP) &
> > +EXYNOSAUTOV920_SLOPE_COMP_MASK;
> > +
> > +	val = readl(data->base + EXYNOSAUTOV920_SENSOR0_TRIM_INFO);
> > +	data->temp_error1 = (val >> EXYNOSAUTOV920_TRIMINFO_25_SHIFT) &
> EXYNOSAUTOV920_TRIM_MASK;
> > +	data->temp_error2 = (val >> EXYNOSAUTOV920_TRIMINFO_85_SHIFT) &
> > +EXYNOSAUTOV920_TRIM_MASK;
> > +
> > +	val = readl(data->base + EXYNOSAUTOV920_TMU_REG_TRIMINFO2);
> > +	val = (val >> EXYNOSAUTOV920_CALIB_SEL_TEMP) &
> > +EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK;
> > +
> > +	data->calib_temp = (EXYNOS_SECOND_POINT_TRIM + (20 * val)); }
> > +
> 
> This is unreadable; please make it understandable for those who don’t have
> the documentation (explicit static inline functions, comments, etc ...).

Ok, I'll refactor this code using explicit static inline helpers and comments.

> > +static void exynosautov920_tmu_control(struct platform_device *pdev,
> > +bool on) {
> > +	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > +	unsigned int con;
> > +
> > +	con = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> > +
> > +	if (on) {
> > +		con |= BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
> > +		con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT);
> > +	} else {
> > +		con &= ~BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
> > +		con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT);
> > +	}
> > +
> > +	writel(con, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL); }
> 
> Document a bit the code please.

Sure, I’ll document this part properly by adding clear comments and splitting the register options into explicit helper functions.

> >   static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
> >   {
> >   	struct exynos_tmu_data *data = id;
> > +	int idx;
> >
> > -	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
> > +	for (idx = 0; idx < data->sensor_count; idx++) {
> > +		if (!data->tzd_array[idx])
> > +			continue;
> > +
> > +		thermal_zone_device_update(data->tzd_array[idx],
> > +THERMAL_EVENT_UNSPECIFIED);
> I understand the main reason is to keep a common isr but you should
> *not* update all the thermal zones. There is an amount of processing
> behind this function adding a significant overhead.
> 
> So somehow readl(data->base + EXYNOSAUTOV920_TMU_REG_INT_PEND(idx));
> should be used here to know if the thermal zone has to be updated or not.

OK, I'll update the ISR so that it checks the pending register before calling 'thermal_zone_device_update()',
And only update the relevant thermal zones. 

> >   static const struct of_device_id exynos_tmu_match[] = {
> >   	{
> >   		.compatible = "samsung,exynos3250-tmu", @@ -833,6 +1044,9 @@
> > static const struct of_device_id exynos_tmu_match[] = {
> >   	}, {
> >   		.compatible = "samsung,exynos7-tmu",
> >   		.data = (const void *)SOC_ARCH_EXYNOS7,
> > +	}, {
> > +		.compatible = "samsung,exynosautov920-tmu",
> > +		.data = (const void *)SOC_ARCH_EXYNOSAUTOV920,
> 
> Time to do cleanups in the driver. Use at your advantage the .data to
> store the relevant info instead of a awful else-if in the different
> functions above.

OK, I'll refactor this by using the .data field.
However, since ExynosAutov920 diverges significantly from the existing driver,
Would introducing a separate driver instead of unifying everything be acceptable?

> >   	},
> >   	{ },
> >   };
> > @@ -865,6 +1079,10 @@ static int exynos_map_dt_data(struct
> > platform_device *pdev)
> >
> >   	data->soc = (uintptr_t)of_device_get_match_data(&pdev->dev);
> >
> > +	data->sensor_count = EXYNOS_DEFAULT_SENSOR_COUNT;
> > +
> > +	data->calib_temp = EXYNOS_SECOND_POINT_TRIM;
> > +
> >   	switch (data->soc) {
> >   	case SOC_ARCH_EXYNOS4210:
> >   		data->tmu_set_low_temp = exynos4210_tmu_set_low_temp; @@ -
> 945,6
> > +1163,19 @@ static int exynos_map_dt_data(struct platform_device *pdev)
> >   		data->min_efuse_value = 15;
> >   		data->max_efuse_value = 100;
> >   		break;
> > +	case SOC_ARCH_EXYNOSAUTOV920:
> > +		data->tmu_set_low_temp = exynosautov920_tmu_set_low_temp;
> > +		data->tmu_set_high_temp = exynosautov920_tmu_set_high_temp;
> > +		data->tmu_disable_low = exynosautov920_tmu_disable_low;
> > +		data->tmu_disable_high = exynosautov920_tmu_disable_high;
> > +		data->tmu_set_crit_temp = exynosautov920_tmu_set_crit_temp;
> > +		data->tmu_initialize = exynosautov920_tmu_initialize;
> > +		data->tmu_control = exynosautov920_tmu_control;
> > +		data->tmu_read = exynosautov920_tmu_read;
> > +		data->tmu_set_emulation = exynos4412_tmu_set_emulation;
> > +		data->tmu_clear_irqs = exynosautov920_tmu_clear_irqs;
> > +		data->sensor_count = EXYNOS_MAX_SENSOR_COUNT;
> > +		break;
> 
> Same comment as above.

Ok, I'll refactor this by using the .data field to move the SoC-specific callbacks into a proper
variant structure.

> --

Thank you for your detailed feedback. I appreciate it.

Powered by blists - more mailing lists