diff mbox series

[10/12] ASoC: amd: add vangogh pci driver pm ops

Message ID 20210707055623.27371-11-vijendar.mukunda@amd.com
State Superseded
Headers show
Series Add Vangogh ACP ASoC driver | expand

Commit Message

Vijendar Mukunda July 7, 2021, 5:56 a.m. UTC
Add Vangogh acp pci driver pm ops.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/vangogh/pci-acp5x.c | 45 +++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Vijendar Mukunda July 8, 2021, 11:41 a.m. UTC | #1
On 7/7/21 10:04 PM, Pierre-Louis Bossart wrote:
> 
>> +static int snd_acp5x_suspend(struct device *dev)
>> +{
>> +	int ret;
>> +	struct acp5x_dev_data *adata;
>> +
>> +	adata = dev_get_drvdata(dev);
>> +	ret = acp5x_deinit(adata->acp5x_base);
>> +	if (ret)
>> +		dev_err(dev, "ACP de-init failed\n");
>> +	else
>> +		dev_dbg(dev, "ACP de-initialized\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int snd_acp5x_resume(struct device *dev)
>> +{
>> +	int ret;
>> +	struct acp5x_dev_data *adata;
>> +
>> +	adata = dev_get_drvdata(dev);
>> +	ret = acp5x_init(adata->acp5x_base);
>> +	if (ret) {
>> +		dev_err(dev, "ACP init failed\n");
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops acp5x_pm = {
>> +	.runtime_suspend = snd_acp5x_suspend,
>> +	.runtime_resume =  snd_acp5x_resume,
>> +	.resume =	snd_acp5x_resume,
> 
> use SET_SYSTEM_SLEEP_PM_OPS and SET_RUNTIME_PM_OPS?

 We will modify the code.
> 
> also not clear why you don't have a .suspend here
It was a miss. we will add .suspend callback which invokes same callback
"snd_acp5x_suspend".
> 
> And to avoid warnings use __maybe_unused for those callbacks when PM is disabled?
> 
Agreed. We will modify the code and post the new version.
>
Vijendar Mukunda July 13, 2021, 6:36 a.m. UTC | #2
On 7/8/21 5:11 PM, Mukunda,Vijendar wrote:
> On 7/7/21 10:04 PM, Pierre-Louis Bossart wrote:
>>
>>> +static int snd_acp5x_suspend(struct device *dev)
>>> +{
>>> +	int ret;
>>> +	struct acp5x_dev_data *adata;
>>> +
>>> +	adata = dev_get_drvdata(dev);
>>> +	ret = acp5x_deinit(adata->acp5x_base);
>>> +	if (ret)
>>> +		dev_err(dev, "ACP de-init failed\n");
>>> +	else
>>> +		dev_dbg(dev, "ACP de-initialized\n");
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int snd_acp5x_resume(struct device *dev)
>>> +{
>>> +	int ret;
>>> +	struct acp5x_dev_data *adata;
>>> +
>>> +	adata = dev_get_drvdata(dev);
>>> +	ret = acp5x_init(adata->acp5x_base);
>>> +	if (ret) {
>>> +		dev_err(dev, "ACP init failed\n");
>>> +		return ret;
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops acp5x_pm = {
>>> +	.runtime_suspend = snd_acp5x_suspend,
>>> +	.runtime_resume =  snd_acp5x_resume,
>>> +	.resume =	snd_acp5x_resume,
>>
>> use SET_SYSTEM_SLEEP_PM_OPS and SET_RUNTIME_PM_OPS?
> 
suspend and resume callbacks implementation is same for runtime pm ops
and system level pm ops in ACP PCI driver i.e in suspend callback acp
de-init sequence will be invoked and in resume callback acp init
sequence will be invoked.
As per our understanding if we safeguard code with CONFIG_PM_SLEEP
macro, then runtime pm ops won't work.

Do we need to duplicate the same code as mentioned below?

static const struct dev_pm_ops acp5x_pm = {
        SET_RUNTIME_PM_OPS(snd_acp5x_runtime_suspend,
                           snd_acp5x_runtime_resume, NULL)
        SET_SYSTEM_SLEEP_PM_OPS(snd_acp5x_suspend, snd_acp5x_resume)
};

where snd_acp5x_runtime_suspend() & snd_acp5x_suspend() API
implementation is same. Similarly snd_acp5x_runtime_resume() &
snd_acp5x_resume() implementation is same.
>  We will modify the code.
>>
>> also not clear why you don't have a .suspend here
> It was a miss. we will add .suspend callback which invokes same callback
> "snd_acp5x_suspend".
>>
>> And to avoid warnings use __maybe_unused for those callbacks when PM is disabled?
>>
> Agreed. We will modify the code and post the new version.
>>
>
Mark Brown July 14, 2021, 4:23 p.m. UTC | #3
On Tue, Jul 13, 2021 at 12:06:38PM +0530, Mukunda,Vijendar wrote:
> On 7/8/21 5:11 PM, Mukunda,Vijendar wrote:
> > On 7/7/21 10:04 PM, Pierre-Louis Bossart wrote:

> >>> +static const struct dev_pm_ops acp5x_pm = {
> >>> +	.runtime_suspend = snd_acp5x_suspend,
> >>> +	.runtime_resume =  snd_acp5x_resume,
> >>> +	.resume =	snd_acp5x_resume,

> >> use SET_SYSTEM_SLEEP_PM_OPS and SET_RUNTIME_PM_OPS?

> suspend and resume callbacks implementation is same for runtime pm ops
> and system level pm ops in ACP PCI driver i.e in suspend callback acp
> de-init sequence will be invoked and in resume callback acp init
> sequence will be invoked.

> As per our understanding if we safeguard code with CONFIG_PM_SLEEP
> macro, then runtime pm ops won't work.

That's not what Pierre is suggesting though?

> Do we need to duplicate the same code as mentioned below?

> static const struct dev_pm_ops acp5x_pm = {
>         SET_RUNTIME_PM_OPS(snd_acp5x_runtime_suspend,
>                            snd_acp5x_runtime_resume, NULL)
>         SET_SYSTEM_SLEEP_PM_OPS(snd_acp5x_suspend, snd_acp5x_resume)
> };

Using the SET_ macros doesn't require that you duplicate the functions,
it literally just means changing the way the ops are assigned.
diff mbox series

Patch

diff --git a/sound/soc/amd/vangogh/pci-acp5x.c b/sound/soc/amd/vangogh/pci-acp5x.c
index 2779423f7cd3..c95e2212188f 100644
--- a/sound/soc/amd/vangogh/pci-acp5x.c
+++ b/sound/soc/amd/vangogh/pci-acp5x.c
@@ -10,6 +10,7 @@ 
 #include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
+#include <linux/pm_runtime.h>
 
 #include "acp5x.h"
 
@@ -255,6 +256,10 @@  static int snd_acp5x_probe(struct pci_dev *pci,
 	default:
 		dev_info(&pci->dev, "ACP audio mode : %d\n", val);
 	}
+	pm_runtime_set_autosuspend_delay(&pci->dev, 2000);
+	pm_runtime_use_autosuspend(&pci->dev);
+	pm_runtime_put_noidle(&pci->dev);
+	pm_runtime_allow(&pci->dev);
 	return 0;
 
 unregister_devs:
@@ -272,6 +277,41 @@  static int snd_acp5x_probe(struct pci_dev *pci,
 	return ret;
 }
 
+static int snd_acp5x_suspend(struct device *dev)
+{
+	int ret;
+	struct acp5x_dev_data *adata;
+
+	adata = dev_get_drvdata(dev);
+	ret = acp5x_deinit(adata->acp5x_base);
+	if (ret)
+		dev_err(dev, "ACP de-init failed\n");
+	else
+		dev_dbg(dev, "ACP de-initialized\n");
+
+	return ret;
+}
+
+static int snd_acp5x_resume(struct device *dev)
+{
+	int ret;
+	struct acp5x_dev_data *adata;
+
+	adata = dev_get_drvdata(dev);
+	ret = acp5x_init(adata->acp5x_base);
+	if (ret) {
+		dev_err(dev, "ACP init failed\n");
+		return ret;
+	}
+	return 0;
+}
+
+static const struct dev_pm_ops acp5x_pm = {
+	.runtime_suspend = snd_acp5x_suspend,
+	.runtime_resume =  snd_acp5x_resume,
+	.resume =	snd_acp5x_resume,
+};
+
 static void snd_acp5x_remove(struct pci_dev *pci)
 {
 	struct acp5x_dev_data *adata;
@@ -285,6 +325,8 @@  static void snd_acp5x_remove(struct pci_dev *pci)
 	ret = acp5x_deinit(adata->acp5x_base);
 	if (ret)
 		dev_err(&pci->dev, "ACP de-init failed\n");
+	pm_runtime_forbid(&pci->dev);
+	pm_runtime_get_noresume(&pci->dev);
 	pci_release_regions(pci);
 	pci_disable_device(pci);
 }
@@ -302,6 +344,9 @@  static struct pci_driver acp5x_driver  = {
 	.id_table = snd_acp5x_ids,
 	.probe = snd_acp5x_probe,
 	.remove = snd_acp5x_remove,
+	.driver = {
+		.pm = &acp5x_pm,
+	}
 };
 
 module_pci_driver(acp5x_driver);