| Message ID | 1723014947-15571-2-git-send-email-quic_dikshita@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add device managed version of dev_pm_domain_attach|detach_list() | expand |
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@quicinc.com> > --- > 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) > +{ > + 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. > + > + 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.
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@quicinc.com> >> --- >> 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) >> +{ >> + 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? This is just a stub API like others in this file for !CONFIG_PM case. Thanks, Dikshita >
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@quicinc.com> >>> --- >>> 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) >>> +{ >>> + 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? > This is just a stub API like others in this file for !CONFIG_PM case. > > Thanks, > Dikshita >> >
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@quicinc.com> > >>> --- > >>> 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? > >>> +{ > >>> + 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.
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@quicinc.com> >>>>> --- >>>>> 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. >
Hi, On Aug 09, 2024 at 14:57:11 +0530, Dikshita Agarwal wrote: > > > 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@quicinc.com> > >>>>> --- > >>>>> 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 *)'" Ah yes sorry I missed the devm_add_action_or_reset part. Thanks for clarifying!
On Wed, 7 Aug 2024 at 09:16, Dikshita Agarwal <quic_dikshita@quicinc.com> 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@quicinc.com> > --- > 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) > +{ > + struct dev_pm_domain_list *list = _list; > + > + dev_pm_domain_detach_list(list); > +} > +EXPORT_SYMBOL_GPL(devm_pm_domain_detach_list); I think this function should be internal and hence made static - unless there is a good reason to export it? > + > +/** > + * 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; There is no need to initialize num_pds to 0 here, as the below calls take care of it. > + > + num_pds = dev_pm_domain_attach_list(dev, data, list); > + We should add a check if num_pds is zero here, as in that case there is no reason to add a devres callback for it. > + 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); [...] Kind regards Uffe
On 8/13/2024 4:22 PM, Ulf Hansson wrote: > On Wed, 7 Aug 2024 at 09:16, Dikshita Agarwal <quic_dikshita@quicinc.com> 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@quicinc.com> >> --- >> 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) >> +{ >> + struct dev_pm_domain_list *list = _list; >> + >> + dev_pm_domain_detach_list(list); >> +} >> +EXPORT_SYMBOL_GPL(devm_pm_domain_detach_list); > > I think this function should be internal and hence made static - > unless there is a good reason to export it? Yeah, it should be static and no need of exporting it. Will make the changes. > >> + >> +/** >> + * 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; > > There is no need to initialize num_pds to 0 here, as the below calls > take care of it. > Right, will be fixed in next revision. >> + >> + num_pds = dev_pm_domain_attach_list(dev, data, list); >> + > > We should add a check if num_pds is zero here, as in that case there > is no reason to add a devres callback for it. > Sure, will add the below check. if (!num_pds) return 0; Thanks, Dikshita >> + 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); > > [...] > > Kind regards > Uffe
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) +{ + 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; + + 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); int dev_pm_domain_start(struct device *dev); void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd); int dev_pm_domain_set_performance_state(struct device *dev, unsigned int state); @@ -476,8 +480,17 @@ static inline int dev_pm_domain_attach_list(struct device *dev, { return 0; } + +static inline int devm_pm_domain_attach_list(struct device *dev, + const struct dev_pm_domain_attach_data *data, + struct dev_pm_domain_list **list) +{ + return 0; +} + static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {} static inline void dev_pm_domain_detach_list(struct dev_pm_domain_list *list) {} +static inline void devm_pm_domain_detach_list(void *list) {} static inline int dev_pm_domain_start(struct device *dev) { return 0;
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@quicinc.com> --- drivers/base/power/common.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_domain.h | 13 +++++++++++++ 2 files changed, 57 insertions(+)