Re: [PATCH v3 03/11] thermal/of: Move the node pointer assignation in the OF code file

[Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>]

On 5/1/26 14:50, Rafael J. Wysocki wrote:
> On Thu, Apr 30, 2026 at 10:12 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Wed, Apr 29, 2026 at 6:14 PM Daniel Lezcano
>> <daniel.lezcano@oss.qualcomm.com> wrote:
>>>
>>> The node pointer being assigned to the cooling device structure is an
>>> action done by the thermal OF only and does not belong to the core
>>> framework code. Move the node pointer assignation in the thermal OF
>>> code. Consequently, the devm_thermal_of_cooling_device_register() can
>>> call its non-devm version resulting in a more intuitive design of the
>>> API.
>>
>> I wouldn't make this change.
>>
>> It adds overhead to the OF case that's not really necessary and
>> complicates the code just to avoid using struct device_node pointers
>> in the core and I'm not really convinced that passing a function
>> pointer to __thermal_cooling_device_register() is so much better.
> 
> I would start with splitting __thermal_cooling_device_register() so
> that it becomes (sorry for the white space breakage induced by GMail)
> 
> static struct thermal_cooling_device *
> __thermal_cooling_device_register(struct device_node *np,
>                    const char *type, void *devdata,
>                    const struct thermal_cooling_device_ops *ops)
> {
>      struct thermal_cooling_device *cdev;
> 
>      cdev = thermal_cooling_device_alloc(ops);
>      if (IS_ERR(cdev))
>          return cdev;
> 
>      cdev->np = np;
> 
>      return thermal_cooling_device_add(cdev, type, devdata, ops);
> }
> 
> where thermal_cooling_device_alloc() does all of the ops and other
> checks and the cdev struct allocation, and
> thermal_cooling_device_add() does everything else previously done by
> __thermal_cooling_device_register() itself.
> 
> Then, it could be renamed to __thermal_of_cooling_device_register()
> and the non-of variant would simply skip the np assignment (and it
> would not take np as an argument).
> 
> You can deal with the devm_ variants of the above analogously.

So we will end up with:

static struct thermal_cooling_device *
__thermal_of_cooling_device_register(struct device_node *np,
		const char *type, void *devdata,
		const struct thermal_cooling_device_ops *ops)
{
	struct thermal_cooling_device *cdev;

	cdev = thermal_cooling_device_alloc(ops);
	if (IS_ERR(cdev))
		return cdev;

	cdev->np = np;

	return thermal_cooling_device_add(cdev, type, devdata, ops);
}

and

static struct thermal_cooling_device *
__thermal_cooling_device_register(const char *type, void *devdata,
		const struct thermal_cooling_device_ops *ops)
{
	struct thermal_cooling_device *cdev;

	cdev = thermal_cooling_device_alloc(ops);
	if (IS_ERR(cdev))
		return cdev;

	return thermal_cooling_device_add(cdev, type, devdata, ops);
}

Right ?

That is what I did more or less initially [1]. It resulted into 
exporting thermal_cooling_device_init_complete(). Here it is similar, 
with other functions.

The reason why I added an init callback in the 
thermal_cooling_device_register() function is to centralize the cooling 
device register logic into the core code only.

By exporting the thermal_cooling_device_add() and 
thermal_cooling_device_alloc() we duplicate the logic and IMO it is not 
desirable.

By introducing a init callback, the core code gives the opportunity to 
any extra layers to initialize some private data in the cooling device 
before the init completion

[1] 
https://lore.kernel.org/all/20260422174305.2899095-4-daniel.lezcano@oss.qualcomm.com/