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

[syed saba kareem]

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@xxxxxxx>
> > Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
> > ---
> >   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: