Re: [PATCH 5/5] ASoC: amd: acp7x: add system and runtime PM ops

[Mario Limonciello]

On 5/7/26 13:11, Vijendar Mukunda wrote:
> Add ACP7.x PM callbacks and hook up system sleep and runtime PM ops for
> the PCI driver.
>
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
> ---
>   sound/soc/amd/acp7x/acp7x-common.c | 43 ++++++++++++++++++++++++++++++
>   sound/soc/amd/acp7x/acp7x.h        |  3 +++
>   sound/soc/amd/acp7x/pci-acp7x.c    | 31 +++++++++++++++++++++
>   3 files changed, 77 insertions(+)
>
> diff --git a/sound/soc/amd/acp7x/acp7x-common.c b/sound/soc/amd/acp7x/acp7x-common.c
> index 68f9b47766c4..df94864a1693 100644
> --- a/sound/soc/amd/acp7x/acp7x-common.c
> +++ b/sound/soc/amd/acp7x/acp7x-common.c
> @@ -75,8 +75,51 @@ static int acp7x_deinit(void __iomem *acp_base, struct device *dev)
>   	return 0;
>   }
>   
> +static int __maybe_unused snd_acp7x_suspend(struct device *dev)
> +{
> +	struct acp7x_dev_data *adata;
> +	int ret;
> +
> +	adata = dev_get_drvdata(dev);
> +	ret = acp_hw_deinit(adata, dev);
> +	if (ret)
> +		dev_err(dev, "ACP de-init failed\n");
> +	return ret;
> +}
> +
> +static int __maybe_unused snd_acp7x_runtime_resume(struct device *dev)
> +{
> +	struct acp7x_dev_data *adata;
> +	int ret;
> +
> +	adata = dev_get_drvdata(dev);
> +	ret = acp_hw_init(adata, dev);
> +	if (ret) {
> +		dev_err(dev, "ACP init failed\n");
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static int __maybe_unused snd_acp7x_resume(struct device *dev)
> +{
> +	struct acp7x_dev_data *adata;
> +	int ret;
> +
> +	adata = dev_get_drvdata(dev);
> +	ret = acp_hw_init(adata, dev);
> +	if (ret)
> +		dev_err(dev, "ACP init failed\n");
> +
> +	return ret;
> +}

Both of snd_acp7x_runtime_resume and snd_acp7x_resume look identical. 
Will there be later code (when machine driver and IO enablement come) 
that makes them different?

If not; then I think you can drop snd_acp7x_runtime_resume().

If so; this makes sense to keep separately.

> +
>   void acp7x_hw_init_ops(struct acp_hw_ops *hw_ops)
>   {
>   	hw_ops->acp_init = acp7x_init;
>   	hw_ops->acp_deinit = acp7x_deinit;
> +	hw_ops->acp_suspend = snd_acp7x_suspend;
> +	hw_ops->acp_resume = snd_acp7x_resume;
> +	hw_ops->acp_suspend_runtime = snd_acp7x_suspend;
> +	hw_ops->acp_resume_runtime = snd_acp7x_runtime_resume;
>   }
> diff --git a/sound/soc/amd/acp7x/acp7x.h b/sound/soc/amd/acp7x/acp7x.h
> index d3a57705385a..ddb5eabf6828 100644
> --- a/sound/soc/amd/acp7x/acp7x.h
> +++ b/sound/soc/amd/acp7x/acp7x.h
> @@ -33,6 +33,9 @@
>   #define ACP7X_PGFSM_CNTL_POWER_ON_MASK		7
>   #define ACP7X_PGFSM_STATUS_MASK			0x3F
>   
> +/* time in ms for runtime suspend delay */
> +#define ACP_SUSPEND_DELAY_MS			2000
> +
>   struct acp_hw_ops {
>   	int (*acp_init)(void __iomem *acp_base, struct device *dev);
>   	int (*acp_deinit)(void __iomem *acp_base, struct device *dev);
> diff --git a/sound/soc/amd/acp7x/pci-acp7x.c b/sound/soc/amd/acp7x/pci-acp7x.c
> index 237eb06e3cad..d29a149cf175 100644
> --- a/sound/soc/amd/acp7x/pci-acp7x.c
> +++ b/sound/soc/amd/acp7x/pci-acp7x.c
> @@ -11,6 +11,7 @@
>   #include <linux/io.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
>   
> @@ -92,6 +93,11 @@ static int snd_acp7x_probe(struct pci_dev *pci,
>   	ret = acp_hw_init(adata, &pci->dev);
>   	if (ret)
>   		goto release_regions;
> +
> +	pm_runtime_set_autosuspend_delay(&pci->dev, ACP_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(&pci->dev);
> +	pm_runtime_put_noidle(&pci->dev);
> +	pm_runtime_allow(&pci->dev);
>   	return 0;
>   
>   release_regions:
> @@ -102,6 +108,26 @@ static int snd_acp7x_probe(struct pci_dev *pci,
>   	return ret;
>   }
>   
> +static int __maybe_unused snd_acp_suspend(struct device *dev)
> +{
> +	return acp_hw_suspend(dev);
> +}
> +
> +static int __maybe_unused snd_acp_runtime_resume(struct device *dev)
> +{
> +	return acp_hw_runtime_resume(dev);
> +}
> +
> +static int __maybe_unused snd_acp_resume(struct device *dev)
> +{
> +	return acp_hw_resume(dev);
> +}
> +
> +static const struct dev_pm_ops acp7x_pm_ops = {
> +	SET_RUNTIME_PM_OPS(snd_acp_suspend, snd_acp_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(snd_acp_suspend, snd_acp_resume)
> +};
> +
>   static void snd_acp7x_remove(struct pci_dev *pci)
>   {
>   	struct acp7x_dev_data *adata;
> @@ -111,6 +137,8 @@ static void snd_acp7x_remove(struct pci_dev *pci)
>   	ret = acp_hw_deinit(adata, &pci->dev);
>   	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);
>   }
> @@ -128,6 +156,9 @@ static struct pci_driver acp7x_pci_driver  = {
>   	.id_table = snd_acp7x_ids,
>   	.probe = snd_acp7x_probe,
>   	.remove = snd_acp7x_remove,
> +	.driver = {
> +		.pm = &acp7x_pm_ops,
> +	}
>   };
>   
>   module_pci_driver(acp7x_pci_driver);