Re: [PATCH] ASoC: amd: Add check for acp config flags

[syed saba kareem <ssabakar@amd.com>]

On 4/12/23 20:30, Limonciello, Mario wrote:
> On 4/12/2023 04:16, Syed Saba Kareem wrote:
>> We have SOF and generic ACP support enabled for Rembrandt and
>> pheonix platforms on some machines. Since we have same PCI id
>
> s,pheonix,Phoenix,
> s,have same,have the same,
>
>> used for probing, add check for machine configuration flag to
>
> You should mention here that the PCI ID is the same but the revision
> is different to make this clearer for the commit message.
>
>> avoid conflict with newer pci drivers. Such machine flag has
>> been initialized via dmi match on few Chrome machines. If no
>> flag is specified probe and register older platform device.
>>
>> Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
>> Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> ---
>>   sound/soc/amd/Kconfig        | 2 ++
>>   sound/soc/amd/ps/acp63.h     | 2 ++
>>   sound/soc/amd/ps/pci-ps.c    | 8 +++++++-
>>   sound/soc/amd/yc/acp6x.h     | 3 +++
>>   sound/soc/amd/yc/pci-acp6x.c | 8 +++++++-
>>   5 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/amd/Kconfig b/sound/soc/amd/Kconfig
>> index c88ebd84bdd5..08e42082f5e9 100644
>> --- a/sound/soc/amd/Kconfig
>> +++ b/sound/soc/amd/Kconfig
>> @@ -90,6 +90,7 @@ config SND_SOC_AMD_VANGOGH_MACH
>>     config SND_SOC_AMD_ACP6x
>>       tristate "AMD Audio Coprocessor-v6.x Yellow Carp support"
>> +    select SND_AMD_ACP_CONFIG
>>       depends on X86 && PCI
>>       help
>>         This option enables Audio Coprocessor i.e ACP v6.x support on
>> @@ -130,6 +131,7 @@ config SND_SOC_AMD_RPL_ACP6x
>>     config SND_SOC_AMD_PS
>>           tristate "AMD Audio Coprocessor-v6.3 Pink Sardine support"
>> +    select SND_AMD_ACP_CONFIG
>
> Whitespace looks not aligned here.  I think it's a mix of tabs and 
> spaces in this section.
> It is aligned ,may be due to addition of plus symbol in the patch it 
> is looking like misaligned.
>>           depends on X86 && PCI && ACPI
>>           help
>>             This option enables Audio Coprocessor i.e ACP v6.3 
>> support on
>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>> index 6bf29b520511..dd36790b25ae 100644
>> --- a/sound/soc/amd/ps/acp63.h
>> +++ b/sound/soc/amd/ps/acp63.h
>> @@ -111,3 +111,5 @@ struct acp63_dev_data {
>>       u16 pdev_count;
>>       u16 pdm_dev_index;
>>   };
>> +
>> +int snd_amd_acp_find_config(struct pci_dev *pci);
>> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
>> index 688a1d4643d9..afddb9a77ba4 100644
>> --- a/sound/soc/amd/ps/pci-ps.c
>> +++ b/sound/soc/amd/ps/pci-ps.c
>> @@ -247,11 +247,17 @@ static int snd_acp63_probe(struct pci_dev *pci,
>>   {
>>       struct acp63_dev_data *adata;
>>       u32 addr;
>> -    u32 irqflags;
>> +    u32 irqflags, flag; >       int val;
>>       int ret;
>>         irqflags = IRQF_SHARED;
>> +
>> +    /* Return if acp config flag is defined */
>> +    flag = snd_amd_acp_find_config(pci);
>
> This 'flag' variable seems unnecessary if it's just used in one place 
> and never evaluated.
>
>> +    if (flag)
>> +        return -ENODEV;
>> +
>>       /* Pink Sardine device check */
>>       switch (pci->revision) {
>>       case 0x63:
>> diff --git a/sound/soc/amd/yc/acp6x.h b/sound/soc/amd/yc/acp6x.h
>> index 036207568c04..2de7d1edf00b 100644
>> --- a/sound/soc/amd/yc/acp6x.h
>> +++ b/sound/soc/amd/yc/acp6x.h
>> @@ -105,3 +105,6 @@ static inline void acp6x_writel(u32 val, void 
>> __iomem *base_addr)
>>   {
>>       writel(val, base_addr - ACP6x_PHY_BASE_ADDRESS);
>>   }
>> +
>> +int snd_amd_acp_find_config(struct pci_dev *pci);
>> +
>> diff --git a/sound/soc/amd/yc/pci-acp6x.c b/sound/soc/amd/yc/pci-acp6x.c
>> index 77c5fa1f7af1..7af6a349b1d4 100644
>> --- a/sound/soc/amd/yc/pci-acp6x.c
>> +++ b/sound/soc/amd/yc/pci-acp6x.c
>> @@ -149,10 +149,16 @@ static int snd_acp6x_probe(struct pci_dev *pci,
>>       int index = 0;
>>       int val = 0x00;
>>       u32 addr;
>> -    unsigned int irqflags;
>> +    unsigned int irqflags, flag;
>>       int ret;
>>         irqflags = IRQF_SHARED;
>> +
>> +    /* Return if acp config flag is defined */
>> +    flag = snd_amd_acp_find_config(pci);
>
> This 'flag' variable seems unnecessary if it's just used in one place 
> and never evaluated.
>
>> +    if (flag)
>> +        return -ENODEV;
>> +
>>       /* Yellow Carp device check */
>>       switch (pci->revision) {
>>       case 0x60:
>