Re: [PATCH v3 03/11] thermal/of: Move the node pointer assignation in the OF code file
From: Daniel Lezcano
Date: Tue May 05 2026 - 06:25:47 EST
On 5/1/26 14:50, Rafael J. Wysocki wrote:
On Thu, Apr 30, 2026 at 10:12 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
On Wed, Apr 29, 2026 at 6:14 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx/