[PATCH v2 0/2] add device managed version of dev_pm_domain_attach|detach_list()

[PATCH v2 0/2] add device managed version of dev_pm_domain_attach|detach_list()

[Dikshita Agarwal]

These patches add the devres-enabled version of
dev_pm_domain_attach|detach_list in pm domain framework and
use the same APIs in venus driver.

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

Changes since v1:
 - fixed the warnings reported by kernel test robot.
 - added stub functions for !CONFIG_PM case.
 - removed un-necessasry type casting.
 - fixed the issue with remove sequence.

Dikshita Agarwal (2):
  PM: domains: add device managed version of
    dev_pm_domain_attach|detach_list()
  media: venus: use device managed APIs for power domains

 drivers/base/power/common.c                    | 44 ++++++++++++++++++++++++++
 drivers/media/platform/qcom/venus/pm_helpers.c |  5 +--
 include/linux/pm_domain.h                      | 13 ++++++++
 3 files changed, 58 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Dikshita Agarwal]

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;
+
+	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;
-- 
2.7.4

Re: [PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Dhruva Gole]

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.


-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

Re: [PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Dikshita Agarwal]

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
> 

Re: [PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Dikshita Agarwal]

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

Re: [PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Dhruva Gole]

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.

-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

Re: [PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Dikshita Agarwal]

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

Re: [PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Dhruva Gole]

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!


-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

Re: [PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Ulf Hansson]

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

Re: [PATCH v2 1/2] PM: domains: add device managed version of dev_pm_domain_attach|detach_list()

[Dikshita Agarwal]

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

[PATCH v2 2/2] media: venus: use device managed APIs for power domains

[Dikshita Agarwal]

Use devres-enabled version of power domain attach APIs.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 5028220..86221e0 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -869,7 +869,7 @@ static int vcodec_domains_get(struct venus_core *core)
 	if (!res->vcodec_pmdomains_num)
 		goto skip_pmdomains;
 
-	ret = dev_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
+	ret = devm_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
 	if (ret < 0)
 		return ret;
 
@@ -895,14 +895,11 @@ static int vcodec_domains_get(struct venus_core *core)
 	return 0;
 
 opp_attach_err:
-	dev_pm_domain_detach_list(core->pmdomains);
 	return ret;
 }
 
 static void vcodec_domains_put(struct venus_core *core)
 {
-	dev_pm_domain_detach_list(core->pmdomains);
-
 	if (!core->has_opp_table)
 		return;
 
-- 
2.7.4