Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0644aede-c281-3919-50e0-4466f6587d81@linaro.org>
Date:   Thu, 29 Dec 2022 16:49:13 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for
 thermal sensors
Content-Language: en-US
To:     Amjad Ouled-Ameur <aouledameur@baylibre.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Amit Kucheria <amitk@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Zhang Rui <rui.zhang@intel.com>
Cc:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Fabien Parent <fparent@baylibre.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Markus Schneider-Pargmann <msp@baylibre.com>,
        linux-pm@vger.kernel.org, Rob Herring <robh@kernel.org>,
        Michael Kao <michael.kao@mediatek.com>,
        linux-kernel@vger.kernel.org, Hsin-Yi Wang <hsinyi@chromium.org>,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org
References: <20221018-up-i350-thermal-bringup-v7-0-ebf08ff2eddb@baylibre.com>
 <20221018-up-i350-thermal-bringup-v7-4-ebf08ff2eddb@baylibre.com>
 <4121bb6b-30db-7a23-f4c8-40afdda7a0b5@linaro.org>
 <COTTJX635TNF.1WL2TEZN7VW9O@amjad-ThinkPad-T490>
 <adfe41f7-00e5-876b-7803-3127919fba13@linaro.org>
 <COUMF3IZ9Y63.LA3KFHJSUZIC@amjad-ThinkPad-T490>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <COUMF3IZ9Y63.LA3KFHJSUZIC@amjad-ThinkPad-T490>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@baylibre.com/
>>
>> 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