Re: [PATCH v14] thermal/drivers/mediatek/auxadc_thermal: expose all thermal sensors

From: Daniel Lezcano
Date: Tue Nov 19 2024 - 16:41:01 EST


On 19/11/2024 08:38, Hsin-Te Yuan wrote:
On Fri, Nov 15, 2024 at 12:48 AM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:


Hi,

On 25/10/2024 14:05, Hsin-Te Yuan wrote:
From: James Lo <james.lo@xxxxxxxxxxxx>

Previously, the driver only supported reading the temperature from all
sensors and returning the maximum value. This update adds another
get_temp ops to support reading the temperature from each sensor
separately.

Especially, some thermal zones registered by this patch are needed by
MT8183 since those thermal zones are necessary for mtk-svs driver.

The DT for the mt8183 describes the sensor id = 0 as the CPU. On this,
there is a cooling device with trip points.

The driver registers the id=0 as an aggregator for the sensors which
overloads the CPU thermal zone above.

Why do you need to aggregate all the sensors to retrieve the max value ?

They are all contributing differently to the heat and they should be
tied with their proper cooling device.

I don't think the thermal configuration is correct and I suggest to fix
this aggregator by removing it.



As far as I know the thermal design of Mediatek's board is based on
the highest temperature of the whole board. Also, removing the
aggregator will break all the boards using this driver.

AFAICT, it is not a thermal design but a thermal configuration.

What is the rational of using power numbers related to the CPU but aggregate all temperatures as an input to the governor ?

And for example, the mt8173 has 4 banks and 4 sensors per banks, so 16 sensors. And they are all grouped together under the thermal zone "cpu-thermal" with the cpu cooling device.

So if the GPU is getting hot, we cool down the CPU ?


By the way, I heard that baylibre is working on multi-sensor
aggregation support, which can be the alternative solution for the
aggregator in this driver, but that should be another story and is
unrelated to this patch.

Right.


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog