Re: [PATCH v2] thermal: Add support for device tree thermal zones consumers
From: Daniel Lezcano
Date: Tue Dec 05 2023 - 13:48:02 EST
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