[PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm ops

[PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm ops

[Shuah Khan]

Convert the mwifiex/pci driver to use dev_pm_ops for power management and
remove Legacy PM handling. This change re-uses existing suspend and resume
interfaces for dev_pm_ops.

Signed-off-by: Shuah Khan <shuah.kh-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/wireless/mwifiex/pcie.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/pcie.c b/drivers/net/wireless/mwifiex/pcie.c
index 20c9c4c..b169318 100644
--- a/drivers/net/wireless/mwifiex/pcie.c
+++ b/drivers/net/wireless/mwifiex/pcie.c
@@ -85,7 +85,7 @@ static bool mwifiex_pcie_ok_to_access_hw(struct mwifiex_adapter *adapter)
  * If already not suspended, this function allocates and sends a host
  * sleep activate request to the firmware and turns off the traffic.
  */
-static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __mwifiex_pcie_suspend(struct pci_dev *pdev)
 {
 	struct mwifiex_adapter *adapter;
 	struct pcie_service_card *card;
@@ -112,6 +112,13 @@ static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
 	return 0;
 }
 
+static int mwifiex_pcie_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return __mwifiex_pcie_suspend(pdev);
+}
+
 /*
  * Kernel needs to suspend all functions separately. Therefore all
  * registered functions must have drivers with suspend and resume
@@ -120,7 +127,7 @@ static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
  * If already not resumed, this function turns on the traffic and
  * sends a host sleep cancel request to the firmware.
  */
-static int mwifiex_pcie_resume(struct pci_dev *pdev)
+static int __mwifiex_pcie_resume(struct pci_dev *pdev)
 {
 	struct mwifiex_adapter *adapter;
 	struct pcie_service_card *card;
@@ -150,6 +157,13 @@ static int mwifiex_pcie_resume(struct pci_dev *pdev)
 
 	return 0;
 }
+
+static int mwifiex_pcie_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return __mwifiex_pcie_resume(pdev);
+}
 #endif
 
 /*
@@ -213,7 +227,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 	if (user_rmmod) {
 #ifdef CONFIG_PM
 		if (adapter->is_suspended)
-			mwifiex_pcie_resume(pdev);
+			__mwifiex_pcie_resume(pdev);
 #endif
 
 		for (i = 0; i < adapter->priv_num; i++)
@@ -249,6 +263,14 @@ static DEFINE_PCI_DEVICE_TABLE(mwifiex_ids) = {
 
 MODULE_DEVICE_TABLE(pci, mwifiex_ids);
 
+#ifdef CONFIG_PM
+/* Power Management Hooks */
+static const struct dev_pm_ops mwifiex_pcie_pm_ops = {
+	.suspend = mwifiex_pcie_suspend,
+	.resume = mwifiex_pcie_resume,
+};
+#endif
+
 /* PCI Device Driver */
 static struct pci_driver __refdata mwifiex_pcie = {
 	.name     = "mwifiex_pcie",
@@ -256,9 +278,9 @@ static struct pci_driver __refdata mwifiex_pcie = {
 	.probe    = mwifiex_pcie_probe,
 	.remove   = mwifiex_pcie_remove,
 #ifdef CONFIG_PM
-	/* Power Management Hooks */
-	.suspend  = mwifiex_pcie_suspend,
-	.resume   = mwifiex_pcie_resume,
+	.driver   = {
+		.pm = &mwifiex_pcie_pm_ops,
+	},
 #endif
 };
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm ops

[Bing Zhao]

Hi Shuah,

Thanks for the patch.

> Convert the mwifiex/pci driver to use dev_pm_ops for power management and
> remove Legacy PM handling. This change re-uses existing suspend and resume
> interfaces for dev_pm_ops.
> 
> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>

Acked-by: Bing Zhao <bzhao@marvell.com>

Thanks,
Bing

Re: [PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm ops

[Hauke Mehrtens]

On 07/02/2013 05:24 PM, Shuah Khan wrote:
> Convert the mwifiex/pci driver to use dev_pm_ops for power management and
> remove Legacy PM handling. This change re-uses existing suspend and resume
> interfaces for dev_pm_ops.
> 
> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> ---
>  drivers/net/wireless/mwifiex/pcie.c |   34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/pcie.c b/drivers/net/wireless/mwifiex/pcie.c
> index 20c9c4c..b169318 100644
> --- a/drivers/net/wireless/mwifiex/pcie.c
> +++ b/drivers/net/wireless/mwifiex/pcie.c
> @@ -85,7 +85,7 @@ static bool mwifiex_pcie_ok_to_access_hw(struct mwifiex_adapter *adapter)
>   * If already not suspended, this function allocates and sends a host
>   * sleep activate request to the firmware and turns off the traffic.
>   */
> -static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __mwifiex_pcie_suspend(struct pci_dev *pdev)
>  {
>  	struct mwifiex_adapter *adapter;
>  	struct pcie_service_card *card;
> @@ -112,6 +112,13 @@ static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
>  	return 0;
>  }
>  
> +static int mwifiex_pcie_suspend(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return __mwifiex_pcie_suspend(pdev);
> +}
> +

For what do you need __mwifiex_pcie_suspend() ? Why not make one
function out of these two?

>  /*
>   * Kernel needs to suspend all functions separately. Therefore all
>   * registered functions must have drivers with suspend and resume
> @@ -120,7 +127,7 @@ static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
>   * If already not resumed, this function turns on the traffic and
>   * sends a host sleep cancel request to the firmware.
>   */
> -static int mwifiex_pcie_resume(struct pci_dev *pdev)
> +static int __mwifiex_pcie_resume(struct pci_dev *pdev)
>  {
>  	struct mwifiex_adapter *adapter;
>  	struct pcie_service_card *card;
> @@ -150,6 +157,13 @@ static int mwifiex_pcie_resume(struct pci_dev *pdev)
>  
>  	return 0;
>  }
> +
> +static int mwifiex_pcie_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return __mwifiex_pcie_resume(pdev);
> +}
>  #endif
>  
>  /*
> @@ -213,7 +227,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
>  	if (user_rmmod) {
>  #ifdef CONFIG_PM
>  		if (adapter->is_suspended)
> -			mwifiex_pcie_resume(pdev);
> +			__mwifiex_pcie_resume(pdev);

You could use mwifiex_pcie_resume(&pdev->dev) here and then the extra
function __mwifiex_pcie_resume() is not needed any more.

>  #endif
>  
>  		for (i = 0; i < adapter->priv_num; i++)
> @@ -249,6 +263,14 @@ static DEFINE_PCI_DEVICE_TABLE(mwifiex_ids) = {
>  
>  MODULE_DEVICE_TABLE(pci, mwifiex_ids);
>  
> +#ifdef CONFIG_PM
> +/* Power Management Hooks */
> +static const struct dev_pm_ops mwifiex_pcie_pm_ops = {
> +	.suspend = mwifiex_pcie_suspend,
> +	.resume = mwifiex_pcie_resume,
> +};
> +#endif
> +

Is it intended that you do not use SIMPLE_DEV_PM_OPS() like most of the
other wifi drivers?

>  /* PCI Device Driver */
>  static struct pci_driver __refdata mwifiex_pcie = {
>  	.name     = "mwifiex_pcie",
> @@ -256,9 +278,9 @@ static struct pci_driver __refdata mwifiex_pcie = {
>  	.probe    = mwifiex_pcie_probe,
>  	.remove   = mwifiex_pcie_remove,
>  #ifdef CONFIG_PM
> -	/* Power Management Hooks */
> -	.suspend  = mwifiex_pcie_suspend,
> -	.resume   = mwifiex_pcie_resume,
> +	.driver   = {
> +		.pm = &mwifiex_pcie_pm_ops,
> +	},
>  #endif
>  };
>  
> 

RE: [PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm ops

[Bing Zhao]

Hi Hauke,

Thanks for your comments.

> > @@ -213,7 +227,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
> >  	if (user_rmmod) {
> >  #ifdef CONFIG_PM
> >  		if (adapter->is_suspended)
> > -			mwifiex_pcie_resume(pdev);
> > +			__mwifiex_pcie_resume(pdev);
> 
> You could use mwifiex_pcie_resume(&pdev->dev) here and then the extra
> function __mwifiex_pcie_resume() is not needed any more.

That makes more sense.

Thanks,
Bing

Re: [PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm ops

[Shuah Khan]

On 07/02/2013 12:51 PM, Hauke Mehrtens wrote:
> On 07/02/2013 05:24 PM, Shuah Khan wrote:
>> Convert the mwifiex/pci driver to use dev_pm_ops for power management and
>> remove Legacy PM handling. This change re-uses existing suspend and resume
>> interfaces for dev_pm_ops.
>>
>> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
>> ---
>>   drivers/net/wireless/mwifiex/pcie.c |   34 ++++++++++++++++++++++++++++------
>>   1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/mwifiex/pcie.c b/drivers/net/wireless/mwifiex/pcie.c
>> index 20c9c4c..b169318 100644
>> --- a/drivers/net/wireless/mwifiex/pcie.c
>> +++ b/drivers/net/wireless/mwifiex/pcie.c
>> @@ -85,7 +85,7 @@ static bool mwifiex_pcie_ok_to_access_hw(struct mwifiex_adapter *adapter)
>>    * If already not suspended, this function allocates and sends a host
>>    * sleep activate request to the firmware and turns off the traffic.
>>    */
>> -static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
>> +static int __mwifiex_pcie_suspend(struct pci_dev *pdev)
>>   {
>>   	struct mwifiex_adapter *adapter;
>>   	struct pcie_service_card *card;
>> @@ -112,6 +112,13 @@ static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
>>   	return 0;
>>   }
>>
>> +static int mwifiex_pcie_suspend(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	return __mwifiex_pcie_suspend(pdev);
>> +}
>> +
>
> For what do you need __mwifiex_pcie_suspend() ? Why not make one
> function out of these two?

Originally, the reason is to keep __mwifiex_pcie_suspend() and 
__mwifiex_pcie_resume() parameters the same so there will be no 
confusion. However, I didn't think about using 
mwifiex_pcie_resume(&pdev->dev) as you mentioned below. Yes, I can make 
the change and not add two additional routines and send a v2 patch.

>
>>   /*
>>    * Kernel needs to suspend all functions separately. Therefore all
>>    * registered functions must have drivers with suspend and resume
>> @@ -120,7 +127,7 @@ static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
>>    * If already not resumed, this function turns on the traffic and
>>    * sends a host sleep cancel request to the firmware.
>>    */
>> -static int mwifiex_pcie_resume(struct pci_dev *pdev)
>> +static int __mwifiex_pcie_resume(struct pci_dev *pdev)
>>   {
>>   	struct mwifiex_adapter *adapter;
>>   	struct pcie_service_card *card;
>> @@ -150,6 +157,13 @@ static int mwifiex_pcie_resume(struct pci_dev *pdev)
>>
>>   	return 0;
>>   }
>> +
>> +static int mwifiex_pcie_resume(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	return __mwifiex_pcie_resume(pdev);
>> +}
>>   #endif
>>
>>   /*
>> @@ -213,7 +227,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
>>   	if (user_rmmod) {
>>   #ifdef CONFIG_PM
>>   		if (adapter->is_suspended)
>> -			mwifiex_pcie_resume(pdev);
>> +			__mwifiex_pcie_resume(pdev);
>
> You could use mwifiex_pcie_resume(&pdev->dev) here and then the extra
> function __mwifiex_pcie_resume() is not needed any more.

Yes. That would eliminate the need for additional routine. I didn't 
think about that option.

>
>>   #endif
>>
>>   		for (i = 0; i < adapter->priv_num; i++)
>> @@ -249,6 +263,14 @@ static DEFINE_PCI_DEVICE_TABLE(mwifiex_ids) = {
>>
>>   MODULE_DEVICE_TABLE(pci, mwifiex_ids);
>>
>> +#ifdef CONFIG_PM
>> +/* Power Management Hooks */
>> +static const struct dev_pm_ops mwifiex_pcie_pm_ops = {
>> +	.suspend = mwifiex_pcie_suspend,
>> +	.resume = mwifiex_pcie_resume,
>> +};
>> +#endif
>> +
>
> Is it intended that you do not use SIMPLE_DEV_PM_OPS() like most of the
> other wifi drivers?

So the reason for this is SIMPLE_DEV_PM_OPS() is defined only when 
CONFIG_PM_SLEEP is enabled. This driver doesn't defines suspend and 
resume when CONFIG_PM is enabled and I didn't want to change that. Hope 
that helps understand why I didn't use SIMPLE_DEV_PM_OPS()

>
>>   /* PCI Device Driver */
>>   static struct pci_driver __refdata mwifiex_pcie = {
>>   	.name     = "mwifiex_pcie",
>> @@ -256,9 +278,9 @@ static struct pci_driver __refdata mwifiex_pcie = {
>>   	.probe    = mwifiex_pcie_probe,
>>   	.remove   = mwifiex_pcie_remove,
>>   #ifdef CONFIG_PM
>> -	/* Power Management Hooks */
>> -	.suspend  = mwifiex_pcie_suspend,
>> -	.resume   = mwifiex_pcie_resume,
>> +	.driver   = {
>> +		.pm = &mwifiex_pcie_pm_ops,
>> +	},
>>   #endif
>>   };
>>
>>
>
>

-- Shuah

Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research 
America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658

Re: [PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm ops

[Lars-Peter Clausen]

On 07/02/2013 09:24 PM, Shuah Khan wrote:
[...]
>>> +#ifdef CONFIG_PM
>>> +/* Power Management Hooks */
>>> +static const struct dev_pm_ops mwifiex_pcie_pm_ops = {
>>> +	.suspend = mwifiex_pcie_suspend,
>>> +	.resume = mwifiex_pcie_resume,
>>> +};
>>> +#endif
>>> +
>>
>> Is it intended that you do not use SIMPLE_DEV_PM_OPS() like most of the
>> other wifi drivers?
> 
> So the reason for this is SIMPLE_DEV_PM_OPS() is defined only when 
> CONFIG_PM_SLEEP is enabled. This driver doesn't defines suspend and 
> resume when CONFIG_PM is enabled and I didn't want to change that. Hope 
> that helps understand why I didn't use SIMPLE_DEV_PM_OPS()

Change the ifdefs to CONFIG_PM_SLEEP and use SIMPLE_DEV_PM_OPS. The driver does
not support runtime PM.

- Lars

Re: [PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm ops

[Shuah Khan]

On 07/02/2013 01:49 PM, Lars-Peter Clausen wrote:
> On 07/02/2013 09:24 PM, Shuah Khan wrote:
> [...]
>>>> +#ifdef CONFIG_PM
>>>> +/* Power Management Hooks */
>>>> +static const struct dev_pm_ops mwifiex_pcie_pm_ops = {
>>>> +	.suspend = mwifiex_pcie_suspend,
>>>> +	.resume = mwifiex_pcie_resume,
>>>> +};
>>>> +#endif
>>>> +
>>>
>>> Is it intended that you do not use SIMPLE_DEV_PM_OPS() like most of the
>>> other wifi drivers?
>>
>> So the reason for this is SIMPLE_DEV_PM_OPS() is defined only when
>> CONFIG_PM_SLEEP is enabled. This driver doesn't defines suspend and
>> resume when CONFIG_PM is enabled and I didn't want to change that. Hope
>> that helps understand why I didn't use SIMPLE_DEV_PM_OPS()
>
> Change the ifdefs to CONFIG_PM_SLEEP and use SIMPLE_DEV_PM_OPS. The driver does
> not support runtime PM.
>
> - Lars
>
>


Thanks. I will update the patch to use SIMPLE_DEV_PM_OPS.

-- Shuah

Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research 
America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658

[PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm_ops

[Shuah Khan]

Convert the mwifiex/pci driver to use dev_pm_ops for power management and
remove Legacy PM handling. This change re-uses existing suspend and resume
interfaces for dev_pm_ops, and changes CONFIG_PM ifdefs to CONFIG_PM_SLEEP
as the driver does not support run-time PM.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/net/wireless/mwifiex/pcie.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/pcie.c b/drivers/net/wireless/mwifiex/pcie.c
index 20c9c4c..4a57eb4 100644
--- a/drivers/net/wireless/mwifiex/pcie.c
+++ b/drivers/net/wireless/mwifiex/pcie.c
@@ -76,7 +76,7 @@ static bool mwifiex_pcie_ok_to_access_hw(struct mwifiex_adapter *adapter)
 	return false;
 }
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 /*
  * Kernel needs to suspend all functions separately. Therefore all
  * registered functions must have drivers with suspend and resume
@@ -85,11 +85,12 @@ static bool mwifiex_pcie_ok_to_access_hw(struct mwifiex_adapter *adapter)
  * If already not suspended, this function allocates and sends a host
  * sleep activate request to the firmware and turns off the traffic.
  */
-static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
+static int mwifiex_pcie_suspend(struct device *dev)
 {
 	struct mwifiex_adapter *adapter;
 	struct pcie_service_card *card;
 	int hs_actived;
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	if (pdev) {
 		card = (struct pcie_service_card *) pci_get_drvdata(pdev);
@@ -120,10 +121,11 @@ static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t state)
  * If already not resumed, this function turns on the traffic and
  * sends a host sleep cancel request to the firmware.
  */
-static int mwifiex_pcie_resume(struct pci_dev *pdev)
+static int mwifiex_pcie_resume(struct device *dev)
 {
 	struct mwifiex_adapter *adapter;
 	struct pcie_service_card *card;
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	if (pdev) {
 		card = (struct pcie_service_card *) pci_get_drvdata(pdev);
@@ -211,9 +213,9 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 	wait_for_completion(&adapter->fw_load);
 
 	if (user_rmmod) {
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 		if (adapter->is_suspended)
-			mwifiex_pcie_resume(pdev);
+			mwifiex_pcie_resume(&pdev->dev);
 #endif
 
 		for (i = 0; i < adapter->priv_num; i++)
@@ -249,16 +251,22 @@ static DEFINE_PCI_DEVICE_TABLE(mwifiex_ids) = {
 
 MODULE_DEVICE_TABLE(pci, mwifiex_ids);
 
+#ifdef CONFIG_PM_SLEEP
+/* Power Management Hooks */
+static SIMPLE_DEV_PM_OPS(mwifiex_pcie_pm_ops, mwifiex_pcie_suspend,
+				mwifiex_pcie_resume);
+#endif
+
 /* PCI Device Driver */
 static struct pci_driver __refdata mwifiex_pcie = {
 	.name     = "mwifiex_pcie",
 	.id_table = mwifiex_ids,
 	.probe    = mwifiex_pcie_probe,
 	.remove   = mwifiex_pcie_remove,
-#ifdef CONFIG_PM
-	/* Power Management Hooks */
-	.suspend  = mwifiex_pcie_suspend,
-	.resume   = mwifiex_pcie_resume,
+#ifdef CONFIG_PM_SLEEP
+	.driver   = {
+		.pm = &mwifiex_pcie_pm_ops,
+	},
 #endif
 };
 
-- 
1.7.10.4

RE: [PATCH] wireless: Convert mwifiex/pcie to dev_pm_ops from legacy pm_ops

[Bing Zhao]

> Convert the mwifiex/pci driver to use dev_pm_ops for power management and
> remove Legacy PM handling. This change re-uses existing suspend and resume
> interfaces for dev_pm_ops, and changes CONFIG_PM ifdefs to CONFIG_PM_SLEEP
> as the driver does not support run-time PM.
> 
> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>

Acked-by: Bing Zhao <bzhao@marvell.com>

Thanks,
Bing