| Message ID | 20210707055623.27371-11-vijendar.mukunda@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Vangogh ACP ASoC driver | expand |
> +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? also not clear why you don't have a .suspend here? And to avoid warnings use __maybe_unused for those callbacks when PM is disabled?
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. >
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. >> >
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.
On 7/14/21 9:53 PM, Mark Brown wrote: > 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. > Will make the changes and post the new version.
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);
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(+)