[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <99c1fd8f-4b17-4d4a-87a5-6a65745632fe@collabora.com>
Date: Fri, 1 Dec 2023 10:52:54 +0100
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>, 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
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.
> 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.
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.
>
> 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?
> 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)?
Cheers,
Angelo
> -- Daniel
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/thermal_of.c?h=thermal%2Fbleeding-edge#n514
>
>> Example:
>> smart-scaling-driver@...00000 {
>> [...]
>>
>> thermal-zones = <&cluster_big_tz>, <&gpu_tz>, <&vpu_tz>;
>> thermal-zone-names = "cpu", "gpu", "vpu";
>>
>> [...]
>> }
>>
>> battery-charger@...00000 {
>> [...]
>>
>> thermal-zones = <&battery_temp>, <&device_skin_temp>;
>> thermal-zone-names = "batt-ext-sensor", "skin";
>>
>> [...]
>> }
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> ---
>>
>> Changes in v2:
>> - Added missing static inline for !CONFIG_OF fallback functions
>>
>> Background story: while I was cleaning up the MediaTek Smart Voltage Scaling
>> (SVS) driver, I've found out that there's a lot of commonization to be done.
>> After a rewrite of "this and that" in that driver, I came across a barrier
>> that didn't allow me to remove another ~100 lines of code, and that was also
>> anyway breaking the driver, because the thermal zone names are different
>> from what was originally intended.
>>
>> I've been looking for thermal zone handle retrieval around the kernel and
>> found that there currently are at least four other drivers that could make
>> use this as a cleanup: charger-manager, which is retrieving a thermal zone
>> to look for with a "cm-thermal-zone" string property, gpu/drm/tiny/repaper.c
>> that does the same by checking a "pervasive,thermal-zone" string property,
>> and ab8500_temp and sdhci-omap which are simply hardcoding a "cpu_thermal"
>> and "battery-thermal" thermal zone names respectively.
>>
>> There are a number of other devices (mostly embedded, mostly smartphones)
>> that don't have an upstream driver and that could make use of this as well.
>>
>> Cheers!
>>
>>
>> drivers/thermal/thermal_of.c | 91 ++++++++++++++++++++++++++++++++++++
>> include/linux/thermal.h | 15 ++++++
>> 2 files changed, 106 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
>> index 1e0655b63259..d8ead456993e 100644
>> --- a/drivers/thermal/thermal_of.c
>> +++ b/drivers/thermal/thermal_of.c
>> @@ -538,6 +538,97 @@ static struct thermal_zone_device
>> *thermal_of_zone_register(struct device_node *
>> return ERR_PTR(ret);
>> }
>> +/**
>> + * __thermal_of_get_zone_by_index() - Get thermal zone handle from the DT
>> + * thermal-zones index
>> + * @dev: Pointer to the consumer device
>> + * @index: Index of thermal-zones
>> + *
>> + * This function will search for a thermal zone in the thermal-zones phandle
>> + * array corresponding to the specified index, then will search for its name
>> + * into the registered thermal zones through thermal_zone_get_zone_by_name()
>> + *
>> + * Please note that this function is for internal use only and expects that
>> + * all of the sanity checks are performed by its caller.
>> + *
>> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
>> + * when the API is disabled or the "thermal-zones" DT property is missing.
>> + */
>> +static struct thermal_zone_device
>> +*__thermal_of_get_zone_by_index(struct device *dev, int index)
>> +{
>> + struct thermal_zone_device *tzd;
>> + struct device_node *np;
>> +
>> + np = of_parse_phandle(dev->of_node, "thermal-zones", index);
>> + if (!np)
>> + return NULL;
>> +
>> + tzd = thermal_zone_get_zone_by_name(np->name);
>> + of_node_put(np);
>> +
>> + return tzd;
>> +}
>> +
>> +/**
>> + * thermal_of_get_zone_by_index() - Get thermal zone handle from a DT node
>> + * based on index
>> + * @dev: Pointer to the consumer device
>> + * @index: Index of thermal-zones
>> + *
>> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
>> + * when the API is disabled or the "thermal-zones" DT property is missing.
>> + */
>> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int
>> index)
>> +{
>> + if (!dev || !dev->of_node)
>> + return ERR_PTR(-ENODEV);
>> +
>> + if (!of_property_present(dev->of_node, "thermal-zones"))
>> + return NULL;
>> +
>> + return __thermal_of_get_zone_by_index(dev, index);
>> +}
>> +
>> +/**
>> + * thermal_of_get_zone() - Get thermal zone handle from a DT node based
>> + * on name, or the first handle in list
>> + * @dev: Pointer to the consumer device
>> + * @name: Name as found in thermal-zone-names or NULL
>> + *
>> + * This function will search for a thermal zone in the thermal-zones phandle
>> + * array corresponding to the index of that in the thermal-zone-names array.
>> + * If the name is not specified (NULL), it will return the first thermal zone
>> + * in the thermal-zones phandle array.
>> + *
>> + * Return: thermal_zone_device pointer on success, ERR_PTR() on error or NULL
>> + * when the API is disabled or the "thermal-zones" DT property is missing.
>> + */
>> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char
>> *name)
>> +{
>> + int index;
>> +
>> + if (!dev || !dev->of_node)
>> + return ERR_PTR(-ENODEV);
>> +
>> + if (!of_property_present(dev->of_node, "thermal-zones")) {
>> + pr_err("thermal zones property not present\n");
>> + return NULL;
>> + }
>> +
>> + if (name) {
>> + index = of_property_match_string(dev->of_node, "thermal-zone-names", name);
>> + if (index < 0) {
>> + pr_err("thermal zone names property not present\n");
>> + return ERR_PTR(index);
>> + }
>> + } else {
>> + index = 0;
>> + }
>> +
>> + return __thermal_of_get_zone_by_index(dev, index);
>> +}
>> +
>> static void devm_thermal_of_zone_release(struct device *dev, void *res)
>> {
>> thermal_of_zone_unregister(*(struct thermal_zone_device **)res);
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index cee814d5d1ac..0fceeb7ed08a 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -261,6 +261,9 @@ struct thermal_zone_device
>> *devm_thermal_of_zone_register(struct device *dev, in
>> void devm_thermal_of_zone_unregister(struct device *dev, struct
>> thermal_zone_device *tz);
>> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int
>> index);
>> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char
>> *name);
>> +
>> #else
>> static inline
>> @@ -274,6 +277,18 @@ static inline void devm_thermal_of_zone_unregister(struct
>> device *dev,
>> struct thermal_zone_device *tz)
>> {
>> }
>> +
>> +static inline
>> +struct thermal_zone_device *thermal_of_get_zone_by_index(struct device *dev, int
>> index)
>> +{
>> + return ERR_PTR(-ENOTSUPP);
>> +}
>> +
>> +static inline
>> +struct thermal_zone_device *thermal_of_get_zone(struct device *dev, const char
>> *name)
>> +{
>> + return ERR_PTR(-ENOTSUPP);
>> +}
>> #endif
>> int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
>
Powered by blists - more mailing lists