2023-06-06 06:07:04

by Vijendar Mukunda

Subject: [PATCH V3 9/9] ASoC: amd: ps: Add SoundWire specific checks in pci driver in pm ops.

AMD SoundWire manager supports different power modes.
In case of SoundWire Power off Mode, ACP pci parent driver
should invoke acp de-init and init sequence during suspend/resume
callbacks.

Signed-off-by: Vijendar Mukunda <[email protected]>
---
sound/soc/amd/ps/pci-ps.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index daf54fe9cafd..5802a701b3b1 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -662,10 +662,15 @@ static int snd_acp63_probe(struct pci_dev *pci,
static int __maybe_unused snd_acp63_suspend(struct device *dev)
{
struct acp63_dev_data *adata;
- int ret;
+ int ret = 0;

adata = dev_get_drvdata(dev);
- ret = acp63_deinit(adata->acp63_base, dev);
+ if (adata->pdev_mask & ACP63_SDW_DEV_MASK) {
+ if (adata->acp_reset)
+ ret = acp63_deinit(adata->acp63_base, dev);
+ } else {
+ ret = acp63_deinit(adata->acp63_base, dev);
+ }
if (ret)
dev_err(dev, "ACP de-init failed\n");
return ret;
@@ -674,10 +679,15 @@ static int __maybe_unused snd_acp63_suspend(struct device *dev)
static int __maybe_unused snd_acp63_resume(struct device *dev)
{
struct acp63_dev_data *adata;
- int ret;
+ int ret = 0;

adata = dev_get_drvdata(dev);
- ret = acp63_init(adata->acp63_base, dev);
+ if (adata->pdev_mask & ACP63_SDW_DEV_MASK) {
+ if (adata->acp_reset)
+ ret = acp63_init(adata->acp63_base, dev);
+ } else {
+ ret = acp63_init(adata->acp63_base, dev);
+ }
if (ret)
dev_err(dev, "ACP init failed\n");
return ret;
--
2.34.1

2023-06-06 15:50:28

by Pierre-Louis Bossart

Subject: Re: [PATCH V3 9/9] ASoC: amd: ps: Add SoundWire specific checks in pci driver in pm ops.

On 6/6/23 01:07, Vijendar Mukunda wrote:
> AMD SoundWire manager supports different power modes.
> In case of SoundWire Power off Mode, ACP pci parent driver
> should invoke acp de-init and init sequence during suspend/resume
> callbacks.
>
> Signed-off-by: Vijendar Mukunda <[email protected]>
> ---
> sound/soc/amd/ps/pci-ps.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index daf54fe9cafd..5802a701b3b1 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -662,10 +662,15 @@ static int snd_acp63_probe(struct pci_dev *pci,
> static int __maybe_unused snd_acp63_suspend(struct device *dev)
> {
> struct acp63_dev_data *adata;
> - int ret;
> + int ret = 0;
>
> adata = dev_get_drvdata(dev);
> - ret = acp63_deinit(adata->acp63_base, dev);
> + if (adata->pdev_mask & ACP63_SDW_DEV_MASK) {

And now back to my comment from the first patch, you are using a
bit-wise operation with an enum which is not explicitly defined as a
mask. This test would be true for ACP63_SDW_PDM_DEV_MASK as well.

+enum acp_pdev_mask {
+ ACP63_PDM_DEV_MASK = 1,
+ ACP63_SDW_DEV_MASK,
+ ACP63_SDW_PDM_DEV_MASK,
+};

> + if (adata->acp_reset)
> + ret = acp63_deinit(adata->acp63_base, dev);
> + } else {
> + ret = acp63_deinit(adata->acp63_base, dev);
> + }
> if (ret)
> dev_err(dev, "ACP de-init failed\n");
> return ret;
> @@ -674,10 +679,15 @@ static int __maybe_unused snd_acp63_suspend(struct device *dev)
> static int __maybe_unused snd_acp63_resume(struct device *dev)
> {
> struct acp63_dev_data *adata;
> - int ret;
> + int ret = 0;
>
> adata = dev_get_drvdata(dev);
> - ret = acp63_init(adata->acp63_base, dev);
> + if (adata->pdev_mask & ACP63_SDW_DEV_MASK) {
> + if (adata->acp_reset)
> + ret = acp63_init(adata->acp63_base, dev);
> + } else {
> + ret = acp63_init(adata->acp63_base, dev);
> + }
> if (ret)
> dev_err(dev, "ACP init failed\n");
> return ret;

2023-06-07 07:26:06

by Vijendar Mukunda

Subject: Re: [PATCH V3 9/9] ASoC: amd: ps: Add SoundWire specific checks in pci driver in pm ops.

On 06/06/23 20:36, Pierre-Louis Bossart wrote:
>
> On 6/6/23 01:07, Vijendar Mukunda wrote:
>> AMD SoundWire manager supports different power modes.
>> In case of SoundWire Power off Mode, ACP pci parent driver
>> should invoke acp de-init and init sequence during suspend/resume
>> callbacks.
>>
>> Signed-off-by: Vijendar Mukunda <[email protected]>
>> ---
>> sound/soc/amd/ps/pci-ps.c | 18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
>> index daf54fe9cafd..5802a701b3b1 100644
>> --- a/sound/soc/amd/ps/pci-ps.c
>> +++ b/sound/soc/amd/ps/pci-ps.c
>> @@ -662,10 +662,15 @@ static int snd_acp63_probe(struct pci_dev *pci,
>> static int __maybe_unused snd_acp63_suspend(struct device *dev)
>> {
>> struct acp63_dev_data *adata;
>> - int ret;
>> + int ret = 0;
>>
>> adata = dev_get_drvdata(dev);
>> - ret = acp63_deinit(adata->acp63_base, dev);
>> + if (adata->pdev_mask & ACP63_SDW_DEV_MASK) {
> And now back to my comment from the first patch, you are using a
> bit-wise operation with an enum which is not explicitly defined as a
> mask. This test would be true for ACP63_SDW_PDM_DEV_MASK as well.
Already provided explanation for the same , in patch 1 review
comments. Please refer that one.
>
> +enum acp_pdev_mask {
> + ACP63_PDM_DEV_MASK = 1,
> + ACP63_SDW_DEV_MASK,
> + ACP63_SDW_PDM_DEV_MASK,
> +};
>
>> + if (adata->acp_reset)
>> + ret = acp63_deinit(adata->acp63_base, dev);
>> + } else {
>> + ret = acp63_deinit(adata->acp63_base, dev);
>> + }
>> if (ret)
>> dev_err(dev, "ACP de-init failed\n");
>> return ret;
>> @@ -674,10 +679,15 @@ static int __maybe_unused snd_acp63_suspend(struct device *dev)
>> static int __maybe_unused snd_acp63_resume(struct device *dev)
>> {
>> struct acp63_dev_data *adata;
>> - int ret;
>> + int ret = 0;
>>
>> adata = dev_get_drvdata(dev);
>> - ret = acp63_init(adata->acp63_base, dev);
>> + if (adata->pdev_mask & ACP63_SDW_DEV_MASK) {
>> + if (adata->acp_reset)
>> + ret = acp63_init(adata->acp63_base, dev);
>> + } else {
>> + ret = acp63_init(adata->acp63_base, dev);
>> + }
>> if (ret)
>> dev_err(dev, "ACP init failed\n");
>> return ret;