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

[Rafael J. Wysocki]

On Tue, May 5, 2026 at 12:07 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxxxxxxxx> wrote:
>
> 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.

They don't need to be exported outside the thermal subsystem though
and they don't need to be exported to modules.

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

That still would be done by the core code.

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

Well, you want the logic to be duplicate, kind of, if you want both
the OF and non-OF variants to be there.

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

I don't think that adding the init callback is a good idea, sorry.