Hi Krzysztof,
Thanks for the review.
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Tuesday, March 11, 2025 8:31 AM
> To: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>; geert+renesas@xxxxxxxxx;
> niklas.soderlund+renesas@xxxxxxxxxxxx; conor+dt@xxxxxxxxxx;
> krzk+dt@xxxxxxxxxx; robh@xxxxxxxxxx; rafael@xxxxxxxxxx;
> daniel.lezcano@xxxxxxxxxx
> Subject: Re: [RFC PATCH 1/3] thermal/cpuplog_cooling: Add CPU hotplug
> cooling driver
>
> On 09/03/2025 13:13, John Madieu wrote:
> > +
> > +/* Check if a trip point is of type "plug" */ static bool
> > +is_plug_trip_point(struct device_node *trip_node) {
> > + const char *trip_type_str;
> > +
> > + if (!trip_node) {
> > + pr_err("Trip node is NULL\n");
> > + return false;
> > + }
> > +
> > + if (of_property_read_string(trip_node, "type", &trip_type_str)) {
> > + pr_err("Trip node missing 'type' property\n");
> > + return false;
> > + }
> > +
> > + pr_info("Trip type: '%s'\n", trip_type_str);
> > +
> > + if (strcmp(trip_type_str, "plug") != 0) {
>
> type is object, not string. Where is ABI documented? For the type and its
> value?
I'll prepare it for v2.
>
>
> > + pr_debug("Trip type is '%s', not 'plug' - skipping\n",
> > + trip_type_str);
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +/* Init function */
> > +static int __init cpu_hotplug_cooling_init(void) {
> > + struct device_node *thermal_zones, *thermal_zone;
> > + int ret = 0;
> > + int count = 0;
> > +
> > + bitmap_zero(cpu_cooling_registered, NR_CPUS);
> > +
> > + thermal_zones = of_find_node_by_name(NULL, "thermal-zones");
> > + if (!thermal_zones) {
> > + pr_err("Missing thermal-zones node\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Process each thermal zone */
> > + for_each_child_of_node(thermal_zones, thermal_zone) {
> > + struct device_node *trips, *trip;
> > + struct device_node *maps, *map;
> > + bool found_plug = false;
> > +
> > + /* First find trips and get a specific plug trip */
> > + trips = of_find_node_by_name(thermal_zone, "trips");
> > + if (!trips)
> > + continue;
> > +
> > + /* Find the emergency trip with type="plug" */
> > + for_each_child_of_node(trips, trip) {
> > + if (is_plug_trip_point(trip)) {
> > + found_plug = true;
> > + break;
> > + }
> > + }
> > +
> > + /* If we didn't find a plug trip, no need to process this zone
> */
> > + if (!found_plug) {
> > + of_node_put(trips);
> > + continue;
> > + }
> > +
> > + maps = of_find_node_by_name(thermal_zone, "cooling-maps");
> > + if (!maps) {
> > + of_node_put(trip);
> > + of_node_put(trips);
> > + continue;
> > + }
> > +
> > + pr_info("Found 'plug' trip point, processing cooling
> devices\n");
>
> dev_info, or just drop. You are not supposed to print successes of
> standard DT parsing.
Noted. Thanks!
>
> > +
> > + /* Find the specific cooling map that references our plug trip
> */
> > + for_each_child_of_node(maps, map) {
> > + struct device_node *trip_ref;
> > + struct of_phandle_args cooling_spec;
> > + int idx = 0;
> > +
> > + trip_ref = of_parse_phandle(map, "trip", 0);
> > + if (!trip_ref || trip_ref != trip) {
> > + if (trip_ref)
> > + of_node_put(trip_ref);
> > + continue;
> > + }
> > + of_node_put(trip_ref);
> > +
> > + if (!of_find_property(map, "cooling-device", NULL)) {
> > + pr_err("Missing cooling-device property\n");
> > + continue;
> > + }
> > +
> > + /* Iterate through all cooling-device entries */
> > + while (of_parse_phandle_with_args(
> > + map, "cooling-device",
> > + "#cooling-cells", idx++,
> > + &cooling_spec) == 0) {
> > + struct device_node *cpu_node = cooling_spec.np;
> > + int cpu;
> > +
> > + if (!cpu_node) {
> > + pr_err("CPU node at index %d is NULL\n",
> > + idx - 1);
> > + continue;
> > + }
> > +
> > + cpu = of_cpu_node_to_id(cpu_node);
> > + if (cpu < 0) {
> > + pr_err("Failed to map CPU node %pOF to
> logical ID\n",
> > + cpu_node);
> > + of_node_put(cpu_node);
> > + continue;
> > + }
> > +
> > + if (cpu >= num_possible_cpus()) {
> > + pr_err("Invalid CPU ID %d (max %d)\n",
> > + cpu, num_possible_cpus() - 1);
> > + of_node_put(cpu_node);
> > + continue;
> > + }
> > +
> > + pr_info("Processing cooling device for CPU%d\n",
> cpu);
> > + ret = register_cpu_hotplug_cooling(cpu_node, cpu);
> > + if (ret == 0)
> > + count++;
> > +
> > + of_node_put(cpu_node);
> > + }
> > + break; /* Only process the first map that references our
> trip */
> > + }
> > + of_node_put(maps);
> > + of_node_put(trip);
> > + of_node_put(trips);
> > + }
> > + of_node_put(thermal_zones);
> > +
> > + if (count == 0) {
> > + pr_err("No cooling devices registered\n");
> > + return -ENODEV;
> > + }
> > +
> > + pr_info("CPU hotplug cooling driver initialized with %d devices\n",
> > +count);
>
> Drop. Why would you print this on MIPS machine which does not care about
> it, just because someone loaded a module?
>
Will remove this in v2.
> > + return 0;
> > +}
> > +
> > +/* Exit function */
> > +static void __exit cpu_hotplug_cooling_exit(void) {
> > + cleanup_cooling_devices();
> > + pr_info("CPU hotplug cooling driver removed\n");
>
> No, drop
>
Got it.
>
> > +}
> > +
> > +module_init(cpu_hotplug_cooling_init);
> > +module_exit(cpu_hotplug_cooling_exit);
> > +
> > +MODULE_AUTHOR("John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("CPU Hotplug Thermal Cooling Device");
> > +MODULE_LICENSE("GPL");
> > \ No newline at end of file
>
> Warning here
>
Will be fixed in v2.
> > diff --git a/drivers/thermal/thermal_of.c
> > b/drivers/thermal/thermal_of.c index 0eb92d57a1e2..41655af1e419 100644
> > --- a/drivers/thermal/thermal_of.c
> > +++ b/drivers/thermal/thermal_of.c
> > @@ -28,6 +28,7 @@ static const char * const trip_types[] = {
> > [THERMAL_TRIP_ACTIVE] = "active",
> > [THERMAL_TRIP_PASSIVE] = "passive",
> > [THERMAL_TRIP_HOT] = "hot",
> > + [THERMAL_TRIP_PLUG] = "plug",
> > [THERMAL_TRIP_CRITICAL] = "critical",
> > };
> >
> > diff --git a/drivers/thermal/thermal_trace.h
> > b/drivers/thermal/thermal_trace.h index df8f4edd6068..c26a3aa7de5f
> > 100644
> > --- a/drivers/thermal/thermal_trace.h
> > +++ b/drivers/thermal/thermal_trace.h
> > @@ -12,6 +12,7 @@
> > #include "thermal_core.h"
> >
> > TRACE_DEFINE_ENUM(THERMAL_TRIP_CRITICAL);
> > +TRACE_DEFINE_ENUM(THERMAL_TRIP_PLUG);
> > TRACE_DEFINE_ENUM(THERMAL_TRIP_HOT);
> > TRACE_DEFINE_ENUM(THERMAL_TRIP_PASSIVE);
> > TRACE_DEFINE_ENUM(THERMAL_TRIP_ACTIVE);
> > @@ -19,6 +20,7 @@ TRACE_DEFINE_ENUM(THERMAL_TRIP_ACTIVE);
> > #define show_tzt_type(type) \
> > __print_symbolic(type, \
> > { THERMAL_TRIP_CRITICAL, "CRITICAL"}, \
> > + { THERMAL_TRIP_PLUG, "PLUG"}, \
> > { THERMAL_TRIP_HOT, "HOT"}, \
> > { THERMAL_TRIP_PASSIVE, "PASSIVE"}, \
> > { THERMAL_TRIP_ACTIVE, "ACTIVE"})
> > diff --git a/drivers/thermal/thermal_trip.c
> > b/drivers/thermal/thermal_trip.c index 4b8238468b53..373f6aaaf0da
> > 100644
> > --- a/drivers/thermal/thermal_trip.c
> > +++ b/drivers/thermal/thermal_trip.c
> > @@ -13,6 +13,7 @@ static const char *trip_type_names[] = {
> > [THERMAL_TRIP_ACTIVE] = "active",
> > [THERMAL_TRIP_PASSIVE] = "passive",
> > [THERMAL_TRIP_HOT] = "hot",
> > + [THERMAL_TRIP_PLUG] = "plug",
> > [THERMAL_TRIP_CRITICAL] = "critical", };
> >
> > diff --git a/include/uapi/linux/thermal.h
> > b/include/uapi/linux/thermal.h index 46a2633d33aa..5f76360c6f69 100644
> > --- a/include/uapi/linux/thermal.h
> > +++ b/include/uapi/linux/thermal.h
> > @@ -15,6 +15,7 @@ enum thermal_trip_type {
> > THERMAL_TRIP_ACTIVE = 0,
> > THERMAL_TRIP_PASSIVE,
> > THERMAL_TRIP_HOT,
> > + THERMAL_TRIP_PLUG,
> > THERMAL_TRIP_CRITICAL,
> > };
> >
>
>
> Best regards,
> Krzysztof
Regards,
John
