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

[Daniel Lezcano]

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