Message-ID: <3428b2af-5522-4090-995a-10eaee90c28e@linaro.org>
Date:   Tue, 5 Dec 2023 19:47:54 +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 05/12/2023 14:48, AngeloGioacchino Del Regno wrote:
> Il 01/12/23 15:18, Daniel Lezcano ha scritto:

[ ... ]

>> 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.
>>
> 
> I don't really understand this assertion, and I'm afraid that I'm 
> underestimating
> something so, in case, please help me to understand what am I missing here.

Sure. Let me clarify my understanding of you proposal and my assertion.

  - A driver needs a thermal zone device structure pointer in order to 
read its temperature. But as it is not the creator, it does not have 
this pointer.

  - As a solution, several drivers are using a specific DT bindings to 
map a thermal zone "name/type' with a string to refer from the driver a 
specific thermal zone node name. Then the function 
thermal_zone_device_get_by_name() is used to retrieve the pointer.

  - Your proposal is to provide that mechanism in the thermal_of code 
directly, so the code can be factored out for all these drivers.

Is my understanding correct?

My point is:

  - The driver are mapping a thermal zone with a name but a node name is 
supposed to be unique on DT (but that is implicit)

  - A phandle is enough to get the node name, no need to add a thermal 
zone name to get the node and then get the thermal zone. It is duplicate 
information as the DT uses the node name as a thermal zone name.

We could have:

         thermal-zones {
                 display {
                         polling-delay-passive = <0>;
                         polling-delay = <0>;
                         thermal-sensors = <&display_temp>;
                 };
         };

         papirus27@0{

		[ ... ]

---             pervasive,thermal-zone = "display";
+++             pervasive,thermal-zone = <&display>;
         };

The ux500 is directly calling thermal_zone_device_get_by_name() with the 
thermal zone node name.

> For how I see it, in the thermal framewoek I don't see any "somewhat 
> standardized"
> helper like the one(s) that I'm introducing with this patch 
> (thermal_of_get_zone(),
> thermal_of_get_zone_by_index()), and this is the exact reason why I'm 
> proposing
> this patch.
> 
> Then again - I mean no disrespect - it's just that I don't understand 
> (yet) why you
> are saying that "everything is already available", because I really 
> don't see it.

Ok said differently, thermal zone name and type are messy.

Let's clarify that and then let's see with the result if adding this 
thermal zone node/name mapping still makes sense.

>>   - 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
> 
> This is something that I didn't realize before. Thanks for that.
> 
> ...and yes, we're registering a "name" from DT as a "type" in the 
> framework, this
> is highly confusing and needs to be cleaned up.
> 
>>
>>   - We can register several thermal zones with the same type (so we 
>> can have duplicate names if we use type as name)
>>
> 
> ...which makes sense, after realizing that we're registering a TYPE and 
> not a NAME,
> and I agree about the logic for which that multiple zones can be of the 
> same type.
> 
>>   - 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
>>
> 
> The first thing that comes to mind is to rename 
> thermal_zone_device_get_by_name()
> to thermal_zone_device_get_by_type(), but then I'd be reintroducing the 
> former and
> this gives me concerns about OOT drivers using that and developers 
> getting highly
> confused (name->type, but name exists again, so they might erroneously 
> just fix the
> call to xxx_by_name() instead of changing it to xxx_by_type()).


> Should I *not* be concerned about that?

Not really :)

TBH, OOT drivers do not exist from upstream POV.

 > Any suggestion?

Yes, let's introduce a thermal zone name in the tzd.

  - There are now too many parameters to the function 
thermal_zone_device_register*(), so we can't add a new 'name' parameter. 
Introduce a thermal_zone_device_parameters structure. This structure 
will contain all thermal_zone_device_register_* parameter function. 
There won't be any functional changes, just how the parameters are 
passed. Perhaps, you should use an intermediate function to not have the 
change impacting everywhere.

  - then add a const char *name field in this structure and in the 
thermal_zone_device structure. So we can assign the name to the thermal 
zone. The name must be checked against duplicate. If no name is 
specified when creating a thermal zone, then name = type.

  - In thermal_of, use the node name for the type and the name, that 
will be duplicate but we will sort it out later.

  - Add the name in sysfs

Second step, track down users of thermal_zone_device_get_by_name() and 
check if they can use the name instead of the type. I'm pretty sure it 
is the case for most of the callers.

With that, there will be a nice clarification IMHO.

Then we will be able to restate the 'type' with something different 
without breaking the existing ABI.

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