Re: [PATCH v1 1/8] thermal: Add Remote Proc cooling driver

[Lukasz Luba]

On 2/9/26 05:28, Gaurav Kohli wrote:
> On 2/2/2026 4:29 PM, Lukasz Luba wrote:
> > Hi Gaurav,
> >
> > On 12/23/25 12:32, Gaurav Kohli wrote:
> > > Add a new generic driver for thermal cooling devices that control
> > > remote processors (modem, DSP, etc.) through various communication
> > > channels.
> > >
> > > This driver provides an abstraction layer between the thermal
> > > subsystem and vendor-specific remote processor communication
> > > mechanisms.
> >
> > Is this the patch about proposing the new cooling
> > device type at last LPC2025 conference (what we've discussed with Amit)?
>
> thanks Lukasz for review, yes this is the same.
> sorry for late reply, was on leave last week.
>
> > There was some feedback asking you to add a bit more description
> > into this patch header, please do that (with some background as well).
>
> Sure, will update.
>
> > > Suggested-by: Amit Kucheria <amit.kucheria@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Gaurav Kohli <gaurav.kohli@xxxxxxxxxxxxxxxx>
> > > ---


[snip]


> > > +struct remoteproc_cooling_ops {
> > > +    int (*get_max_level)(void *devdata, unsigned long *level);
> > > +    int (*get_cur_level)(void *devdata, unsigned long *level);
> > > +    int (*set_cur_level)(void *devdata, unsigned long level);
> > > +};
> >
> > 1. There is no comment for struct and the functions like you did below.
> > 2. Why you need those 3 callbacks?
> >     It looks like they are simple wrappers on stuff in
> >     'struct thermal_cooling_device_ops'.
> >     Please try to get rid of them and re-use the existing fwk callbacks.
>
> thanks for this suggestion, i will use thermal_cooling_device_ops directly.
>
> > > +
> > > +/**
> > > + * struct remoteproc_cdev - Remote processor cooling device
> > > + * @cdev: Thermal cooling device handle
> > > + * @ops: Vendor-specific operation callbacks
> > > + * @devdata: Private data for vendor implementation
> > > + * @np: Device tree node associated with this cooling device
> > > + * @lock: Mutex to protect cooling device operations
> > > + */
> > > +struct remoteproc_cdev {
> >
> > Please use the full naming:
> > remoteproc_cooling_device
> >
> > > +    struct thermal_cooling_device *cdev;
> >
> > You don't need to keep it here. AFAICS it's only
> > used in the 'unregister' function. Please check my
> > comment here and then remove this pointer.
> > (It creates uneseccery linkage between those devices).
> >
> > > +    const struct remoteproc_cooling_ops *ops;
> >
> > So here it can be simply:
> > struct thermal_cooling_device_ops cooling_ops;
>
> yes, i will use this as part of remoteproc_cooling_device struct.
>
> > > +    void *devdata;
> > > +    struct device_node *np;
> >
> > This 'np' is also not used, remove it please.
> >
> > > +    struct mutex lock;
> > > +};
> > > +
> > > +
> > > +/* Thermal cooling device callbacks */
> > > +
> > > +static int remoteproc_get_max_state(struct thermal_cooling_device 
> > > *cdev,
> > > +                    unsigned long *state)
> > > +{
> > > +    struct remoteproc_cdev *rproc_cdev = cdev->devdata;
> > > +    int ret;
> > > +
> > > +    if (!rproc_cdev || !rproc_cdev->ops)
> > > +        return -EINVAL;
> >
> > This mustn't be changed in runtime accidenly. We don't guard in
> > cpufreq-/devfreq- cooling these callbacks that way. Please drop them.
>
> Sure, let me rewrite this and update in next version.
>
> >

[snip]

> > > +
> > > +    if (!name || !ops) {
> >
> > IMO you should check the '!np' here, not the lines below.
> > We can simply bail out very early.
>
> thanks will put explicit check for np, but please let me know for non 
> np, do we have to add support for non np also.

If your code doesn't use the non-np then let's not implement it.
When there will be a new client, we can refactor slightly the existing
code and make two interfaces for the registration (similat to cpufreq
cooling).

> so they can directly register with thermal_cooling_device_register.
>
> > > +        return ERR_PTR(-EINVAL);
> > > +    }
> > > +

[snip]

> > > +
> > > +void remoteproc_cooling_unregister(struct remoteproc_cdev *rproc_cdev)
> >
> > Change the API to be alined with cpufreq-cooling and devfreq-cooling
> > types of devices, so:
> >
> > void remoteproc_cooling_unregister(struct thermal_cooling_device *cdev)
> >
> > You still should be able to get the rptoc_cdev like:
> >
> > rproc_cdev = cdev->devdata;
> >
> > and free it.
>
> thanks, will change something like below
> +       rproc_cdev = cdev->devdata;
> +       thermal_cooling_device_unregister(cdev);

Should work, let see in the new code.


[snip]


> > > +struct remoteproc_cooling_ops {
> > > +    int (*get_max_level)(void *devdata, unsigned long *level);
> > > +    int (*get_cur_level)(void *devdata, unsigned long *level);
> > > +    int (*set_cur_level)(void *devdata, unsigned long level);
> > > +};
> >
> > That duplicate w/ .c file content.
> > We don't need this in the header, please follow the cpufreq-/devfreq-
> > design.
>
> Yes, with new approach of using thermal_cooling_device_ops directly can
> save this.

Great, looking for the the v2

> > > +
> > > +struct remoteproc_cdev;
> > > +
> > > +#ifdef CONFIG_REMOTEPROC_THERMAL
> > > +
> > > +struct remoteproc_cdev *
> > > +remoteproc_cooling_register(struct device_node *np,
> > > +                 const char *name,
> > > +                 const struct remoteproc_cooling_ops *ops,
> > > +                 void *devdata);
> > > +
> > > +void remoteproc_cooling_unregister(struct remoteproc_cdev *rproc_cdev);
> > > +
> > > +#else /* !CONFIG_REMOTEPROC_THERMAL */
> > > +
> > > +static inline struct remoteproc_cdev *
> > > +remoteproc_cooling_register(struct device_node *np,
> > > +                 const char *name,
> > > +                 const struct remoteproc_cooling_ops *ops,
> > > +                 void *devdata)
> > > +{
> > > +    return ERR_PTR(-EINVAL);
> > > +}
> >
> > Function naming convention here as well
>
> thanks a lot, let me rewrite as per suggestion and update in newer version.

you're welcome