On 8/9/2024 9:49 AM, Dhruva Gole wrote: > Hi, > > On Aug 08, 2024 at 16:29:12 +0530, Dikshita Agarwal wrote: >> >> >> On 8/8/2024 4:25 PM, Dikshita Agarwal wrote: >>> >>> >>> On 8/8/2024 4:11 PM, Dhruva Gole wrote: >>>> On Aug 07, 2024 at 12:45:46 +0530, Dikshita Agarwal wrote: >>>>> Add the devres-enabled version of dev_pm_domain_attach|detach_list. >>>>> If client drivers use devm_pm_domain_attach_list() to attach the >>>>> PM domains, devm_pm_domain_detach_list() will be invoked implicitly >>>>> during remove phase. >>>>> >>>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx> >>>>> --- >>>>> drivers/base/power/common.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/pm_domain.h | 13 +++++++++++++ >>>>> 2 files changed, 57 insertions(+) >>>>> >>>>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c >>>>> index 327d168..729d6c2 100644 >>>>> --- a/drivers/base/power/common.c >>>>> +++ b/drivers/base/power/common.c >>>>> @@ -277,6 +277,50 @@ int dev_pm_domain_attach_list(struct device *dev, >>>>> EXPORT_SYMBOL_GPL(dev_pm_domain_attach_list); >>>>> >>>>> /** >>>>> + * devm_pm_domain_detach_list - devres-enabled version of dev_pm_domain_detach_list. >>>>> + * @_list: The list of PM domains to detach. >>>>> + * >>>>> + * This function reverse the actions from devm_pm_domain_attach_list(). >>>>> + * it will be invoked during the remove phase from drivers implicitly if driver >>>>> + * uses devm_pm_domain_attach_list() to attach the PM domains. >>>>> + */ >>>>> +void devm_pm_domain_detach_list(void *_list) > > My problem is with the type of parameter used being void, why void? > Why not be explicit about it and call it dev_pm_domain_list *list like > the non-devres version of the API? > devm_add_action_or_reset API expects the argument as void (*)(void *). Below are code references following the same way: https://elixir.bootlin.com/linux/v6.11-rc2/source/drivers/devfreq/devfreq.c#L1332 https://elixir.bootlin.com/linux/v6.11-rc2/source/drivers/clk/clk.c#L1033 If I change the type of argument as you are suggesting, it will throw compilation error. "expected 'void (*)(void *)' but argument is of type 'void (*)(struct dev_pm_domain_list *)'" >>>>> +{ >>>>> + struct dev_pm_domain_list *list = _list; >>>>> + >>>>> + dev_pm_domain_detach_list(list); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(devm_pm_domain_detach_list); >>>>> + >>>>> +/** >>>>> + * devm_pm_domain_attach_list - devres-enabled version of dev_pm_domain_attach_list >>>>> + * @dev: The device used to lookup the PM domains for. >>>>> + * @data: The data used for attaching to the PM domains. >>>>> + * @list: An out-parameter with an allocated list of attached PM domains. >>>>> + * >>>>> + * NOTE: this will also handle calling devm_pm_domain_detach_list() for >>>>> + * you during remove phase. >>>>> + * >>>>> + * Returns the number of attached PM domains or a negative error code in case of >>>>> + * a failure. >>>>> + */ >>>>> +int devm_pm_domain_attach_list(struct device *dev, >>>>> + const struct dev_pm_domain_attach_data *data, >>>>> + struct dev_pm_domain_list **list) >>>>> +{ >>>>> + int ret, num_pds = 0; >>>> >>>> Do we require this =0? In the very next line you're initing this anyway. >>>> >>> That's correct, will fix this. Thanks. >>>>> + >>>>> + num_pds = dev_pm_domain_attach_list(dev, data, list); >>>>> + >>>>> + ret = devm_add_action_or_reset(dev, devm_pm_domain_detach_list, *list); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return num_pds; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(devm_pm_domain_attach_list); >>>>> + >>>>> +/** >>>>> * dev_pm_domain_detach - Detach a device from its PM domain. >>>>> * @dev: Device to detach. >>>>> * @power_off: Used to indicate whether we should power off the device. >>>>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >>>>> index 772d328..efd517017 100644 >>>>> --- a/include/linux/pm_domain.h >>>>> +++ b/include/linux/pm_domain.h >>>>> @@ -450,8 +450,12 @@ struct device *dev_pm_domain_attach_by_name(struct device *dev, >>>>> int dev_pm_domain_attach_list(struct device *dev, >>>>> const struct dev_pm_domain_attach_data *data, >>>>> struct dev_pm_domain_list **list); >>>>> +int devm_pm_domain_attach_list(struct device *dev, >>>>> + const struct dev_pm_domain_attach_data *data, >>>>> + struct dev_pm_domain_list **list); >>>>> void dev_pm_domain_detach(struct device *dev, bool power_off); >>>>> void dev_pm_domain_detach_list(struct dev_pm_domain_list *list); >>>>> +void devm_pm_domain_detach_list(void *list); >>>> >>>> Why not just call it dev_pm_domain_list *list? Why make it void? I am a >>>> bit confused. >>>> >>> This comment is not clear to me, could you pls elaborate? >> Ah! Sorry, pls ignore my below comment. But can you still explain the >> concern here? > > I have explained above near the func definition. >
