Re: [PATCH v4 09/10] thermal/of: Support cooling device ID in cooling-spec

[Lukasz Luba]

On 5/26/26 15:08, Daniel Lezcano wrote:
> Extend the cooling device specifier parsing to support an optional
> cooling device identifier (cdev_id).
>
> Two formats are now supported:
>
>    - Legacy format:
>          <&cdev lower upper>
>
>    - Indexed format:
>          <&cdev cdev_id lower upper>
>
> When the indexed format is used, both the device node and the
> cdev_id must match in order to bind a cooling device to a thermal
> zone. The legacy format continues to match on the device node only,
> preserving backward compatibility.
>
> Update the parsing logic accordingly to handle both formats and
> extract the mitigation limits from the appropriate arguments.
>
> This is a preparatory step for upcoming DT bindings describing
> cooling devices using (device node, id) tuples instead of child
> nodes.
>
> No functional change for existing device trees.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxxxxxxxx>
> ---
>   drivers/thermal/thermal_of.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 3584024b76f5..100fd8a0c8ce 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -259,16 +259,34 @@ static bool thermal_of_get_cooling_spec(struct device_node *map_np, int index,
>   
>   	of_node_put(cooling_spec.np);
>   
> -	if (cooling_spec.args_count < 2) {
> -		pr_err("wrong reference to cooling device, missing limits\n");
> +	/*
> +	 * There are two formats:
> +	 * - Legacy format :	<&cdev lower upper>
> +	 * - New format    :	<&cdev cdev_id lower upper>
> +	 *
> +	 * With the new format, along with the device node pointer,
> +	 * the cdev_id must match with the cooling device cdev_id in
> +	 * order to bind
> +	 */
> +	if (cooling_spec.args_count < 2 || cooling_spec.args_count > 3) {
> +		pr_err("Invalid number of cooling device parameters\n");
>   		return false;
>   	}
>   
>   	if (cooling_spec.np != cdev->np)
>   		return false;
>   
> -	c->lower = cooling_spec.args[0];
> -	c->upper = cooling_spec.args[1];
> +	if (cooling_spec.args_count == 3 &&
> +	    cooling_spec.args[0] != cdev->cdev_id)
> +		return false;
> +
> +	if (cooling_spec.args_count != 3) {

A bit odd to read the negation format while still having 'else'.

> +		c->lower = cooling_spec.args[0];
> +		c->upper = cooling_spec.args[1];
> +	} else {
> +		c->lower = cooling_spec.args[1];
> +		c->upper = cooling_spec.args[2];
> +	}
>   	c->weight = weight;
>   
>   	return true;

Just a minor comment, which can be ignored, fill free to do so

Reviewed-by: Lukasz Luba <lukasz.luba@xxxxxxx>