Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

From: Daniel Lezcano
Date: Thu Dec 29 2022 - 10:49:22 EST


On 06/12/2022 10:18, Amjad Ouled-Ameur wrote:
Hi Daniel,
On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:

Hi Amjad,


On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:

[ ... ]

@@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, mt);
- tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
- &mtk_thermal_ops);
- if (IS_ERR(tzdev)) {
- ret = PTR_ERR(tzdev);
- goto err_disable_clk_peri_therm;
+ for (i = 0; i < mt->conf->num_sensors + 1; i++) {
+ tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
+ if (!tz)
+ return -ENOMEM;
+
+ tz->mt = mt;
+ tz->id = i;
+
+ tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
+ &mtk_thermal_ops :
+ &mtk_thermal_sensor_ops);

Here you use again the aggregation
I addressed this concern in V6, could you please take a look and let me
know what you think [0].

[0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@xxxxxxxxxxxx/

May I misunderstanding but AFAICS, this patch is setting the
mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
max temperature in this ops which is what we don't want to do.

Correct, but I think that is out of scope of this patchset, as the current
driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
is to add support for the other sensors.

Besides, what do you suggest as a clean implementation if the current one
no longer meets thermal core requirements ?

IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?

If it is the case, then a thermal zone per sensor with the trip points and a cooling device for each of them.

The two thermal zones for the big will share the same cooling device. The little thermal zone will have its own cooling device.

If there is the GPU, then its own cooling device also with devfreq.


<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


--
<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