Re: [PATCH v2] thermal: Add support for device tree thermal zones consumers

Daniel Lezcano
2023-12-05T18:47:57Z

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 |