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

[Lukasz Luba <lukasz.luba@arm.com>]

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@oss.qualcomm.com>
>>> Signed-off-by: Gaurav Kohli <gaurav.kohli@oss.qualcomm.com>
>>> ---


[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