Re: [RFC PATCH 1/3] thermal/cpuplog_cooling: Add CPU hotplug cooling driver

[Krzysztof Kozlowski <krzk@kernel.org>]

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?


> +		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.

> +
> +		/* 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?

> +	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


> +}
> +
> +module_init(cpu_hotplug_cooling_init);
> +module_exit(cpu_hotplug_cooling_exit);
> +
> +MODULE_AUTHOR("John Madieu <john.madieu.xa@bp.renesas.com>");
> +MODULE_DESCRIPTION("CPU Hotplug Thermal Cooling Device");
> +MODULE_LICENSE("GPL");
> \ No newline at end of file

Warning here

> 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