Re: [PATCH v3 1/8] thermal: core: Add mechanism for connecting trips with driver data
From: Daniel Lezcano
Date: Wed Aug 02 2023 - 08:34:13 EST
Hi Rafael,
On 01/08/2023 21:02, Rafael J. Wysocki wrote:
On Tue, Aug 1, 2023 at 8:29 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
On 25/07/2023 14:04, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
Some drivers need to update trip point data (temperature and/or
hysteresis) upon notifications from the platform firmware or they
may need to reprogram hardware when trip point parameters are changed
via sysfs. For those purposes, they need to connect struct thermal_trip
to a private data set associated with the trip or the other way around
and using a trip point index for that may not always work, because the
core may need to reorder the trips during thermal zone registration (in
particular, they may need to be sorted).
To allow that to be done without using a trip point index, introduce
a new field in struct thermal_trip that can be pointed by the driver
to its own data structure containing a trip pointer to be initialized
by the core during thermal zone registration. That pointer will then
have to be updated by the core every time the location of the given
trip point object in memory changes.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
---
v2 -> v3: No changes.
v1 -> v2: No changes.
---
drivers/thermal/thermal_core.c | 20 +++++++++++++++++---
include/linux/thermal.h | 13 +++++++++++++
2 files changed, 30 insertions(+), 3 deletions(-)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -76,16 +76,29 @@ struct thermal_zone_device_ops {
void (*critical)(struct thermal_zone_device *);
};
+struct thermal_trip_ref {
+ struct thermal_trip *trip;
+};
That introduces a circular dependency. That should be avoided.
Sorry, but this is an empty statement without any substance.
I'm just pointing that we have a struct A pointing to struct B and
struct B pointing to struct A.
[ ... ]
struct thermal_cooling_device_ops {
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1306,14 +1306,28 @@ thermal_zone_device_register_with_trips(
if (result)
goto release_device;
+ mutex_lock(&tz->lock);
+
for (count = 0; count < num_trips; count++) {
- struct thermal_trip trip;
+ int temperature = 0;
+
+ if (trips) {
+ temperature = trips[count].temperature;
+ if (trips[count].driver_ref)
+ trips[count].driver_ref->trip = &trips[count];
+ } else {
+ struct thermal_trip trip;
As mentioned above, that should not appear in the thermal core code.
Well, this is a matter of opinion to me. Clearly, I disagree with it.
Why? It is not an opinion. The thermal core code has been very very tied
with the ACPI implementation (which is logical given the history of the
changes). All the efforts have been made to cut these frictions and make
the thermal core code driver agnostic.
The changes put in place a mechanism for the ACPI driver.
The thermal zone lock wrapper is put in place for the ACPI driver.
Anyway, I want to be productive, so here's the thing: either something
like this is done, or drivers need to be allowed to walk the trips
table.
Which one is better?
None of them. I think we can find a third solution where the changes are
self contained in the ACPI driver. What do you think?
--
<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