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

[Mukunda,Vijendar]

On 08/05/26 01:03, Mario Limonciello wrote:
>
>
> 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? 

Yes, when IO enablement code is updated, both the functions are going to be
different.
We need to keep it separately.

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