Message-ID: <ce110f25-5431-4c80-b037-add7fd7461bd@linaro.org>
Date:   Fri, 1 Dec 2023 15:18:39 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>, rafael@...nel.org
Cc:     rui.zhang@...el.com, lukasz.luba@....com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...labora.com,
        wenst@...omium.org
Subject: Re: [PATCH v2] thermal: Add support for device tree thermal zones
 consumers


Hi Angelo,

On 01/12/2023 10:52, AngeloGioacchino Del Regno wrote:
> Il 30/11/23 14:22, Daniel Lezcano ha scritto:
>>
>> Hi Angelo,
>>
>> thanks for your proposal
>>
>> On 15/11/2023 15:48, AngeloGioacchino Del Regno wrote:
>>> Add helpers to support retrieving thermal zones from device tree nodes:
>>> this will allow a device tree consumer to specify phandles to specific
>>> thermal zone(s), including support for specifying thermal-zone-names.
>>> This is useful, for example, for smart voltage scaling drivers that
>>> need to adjust CPU/GPU/other voltages based on temperature, and for
>>> battery charging drivers that need to scale current based on various
>>> aggregated temperature sensor readings which are board-dependant.
>>
>> IMO these changes are trying to solve something from the DT 
>> perspective adding more confusion between phandle, names, types etc 
>> ... and it does not really help AFAICT.
>>
> 
> I honestly don't see how can assigning thermal zones (like we're doing 
> for other
> consumers like clocks, etc) to a node can be confusing?
> To me, it looks like a pattern that is repeating over and over in device 
> tree, for
> multiple types of consumers.

Because there is no need to add anything. Everything is already available.

Add a phandle in the device node wanting to access the thermal zone, get 
the thermal zone device node pointer name and use 
thermal_zone_device_get_by_name(), but see below ...


>> Overall I'm a bit reluctant to add more API in the thermal.h. From my 
>> POV, we should try to remove as much as possible functions from there.
>>
> 
> Cleaning up the API is always something that makes sense, but I don't 
> see why this
> should prevent useful additions...
> 
>> That said, the name of a thermal zone does not really exists and there 
>> is confusion in the code between a name and a type. (type being 
>> assumed to be a name).
> 
> That depends on how you see it. What my brain ticks around is:
> A thermal zone is a physical zone on the PCB, or a physical zone on a chip,
> which has its own "real name", as in, it can be physically identified.

What I meant the thermal framework does not really have a thermal zone 
name, just a type. So it is possible to find several thermal zone with 
the same type like "acpitz"

> Example: The "Skin area" of a laptop is something "reachable" from the 
> user as an
> externally exposed part. This area's temperature is read by thermistor 
> EXTERNAL_1,
> not by thermistor "SKIN0".
> 
> Same goes for "big cluster area", "little cluster area", "cpu complex 
> area", etc.

Today that is solved with a configuration file mapping a specific 
thermal zone to a name but still fragile as we can have duplicate 
thermal zone types.

>> There could be several thermal zones with the same types for non-DT 
>> description. However, the documentation says we should create an 
>> unique type in the DT and that is what is happening when registering a 
>> thermal zone from the DT [1] as we use the node name.
>>
>>  From an external driver, it possible to get the np->name from the 
>> phandles and call thermal_zone_get_by_name(np->name).
>>
> 
> That'd still require you to pass a thermal zone phandle to the 
> node(driver) though?

Yes

>> The hardening change which may make sense is to check a thermal zone 
>> with the same name is not already registered in thermal_of.c by 
>> checking thermal_zone_get_by_name() fails before registering it.
>>
> 
> Yes we can harden that, but I don't see how is this relevant to thermal 
> zones
> device tree consumers (proposed in this patch)?

Putting apart the fact the change you propose is not relevant as there 
is already everything in. My comment is about the current state of the 
thermal framework.

  - A thermal zone does not have a name but a type

  - We use the thermal zone DT node name to register as a name but it is 
a type from the thermal framework point of view

  - We can register several thermal zones with the same type (so we can 
have duplicate names if we use type as name)

  - We use thermal_zone_device_get_by_name() but actually it checks 
against the type and as we can have multiple identical types, the 
function returns the first one found

All this is a bit fuzzy and confusing. So if you add these mapping 
between thermal zone nodes and names, that will be even more confusing.

Ideally, it would make more sense to cleanup this in order to have 
something like an enum type describing the thermal zone (battery, cpu, 
npu, gpu, dsp, ...) which would be used as a type of thermal zone and 
then an unique name (cpu0, cpu1, modem0, modem1, gpu-bottom, gpu-top, 
gpu-center, skin, ...).




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

Powered by blists - more mailing lists