Re: [PATCH 07/21] ASoC: amd: Add module to determine ACP configuration

[Geert Uytterhoeven]

Hi Daniel,

On Wed, Nov 17, 2021 at 12:39 PM Daniel Baluta
<daniel.baluta@xxxxxxxxxxx> wrote:
> From: Ajit Kumar Pandey <AjitKumar.Pandey@xxxxxxx>
>
> ACP hw block configuration differs across various distributions
> and hence it's required to register different drivers module for
> distributions. For now we support three ACP drivers:
>
> * ACP without SOF use case
> * ACP with SOF use case
> * ACP with SOF use case for DMIC and non SOF for I2S endpoints
>
> As all above driver registers with common PCI ID for ACP hw block
> we need code to determine ACP configuration and auto select driver
> module. This patch expose function that return configuration flag
> based on dmi checks for a system. ACP driver module probe register
> platform device based on such configuration flag to avoid conflict
> with other ACP drivers probed for same PCI ID.
>
> Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@xxxxxxx>
> Reviewed-by: Bard Liao <bard.liao@xxxxxxxxx>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Baluta <daniel.baluta@xxxxxxx>

Thanks for your patch, which is now commit f1bdd8d385a80356 ("ASoC:
amd: Add module to determine ACP configuration") in sound-asoc/for-next.

> --- a/sound/soc/amd/Kconfig
> +++ b/sound/soc/amd/Kconfig
> @@ -96,4 +96,10 @@ config SND_SOC_AMD_YC_MACH
>           Say m if you have such a device.
>           If unsure select "N".
>
> +config SND_AMD_ACP_CONFIG
> +       tristate "AMD ACP configuration selection"

This definitely needs proper dependencies, to prevent asking the user
about this when configuring a kernel without AMD Audio ACP support.

I would have sent a patch, but...

> +       help
> +        This option adds an auto detection to determine which ACP
> +        driver modules to use
> +
>  source "sound/soc/amd/acp/Kconfig"

> --- /dev/null
> +++ b/sound/soc/amd/acp-config.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> +//
> +// This file is provided under a dual BSD/GPLv2 license. When using or
> +// redistributing this file, you may do so under either license.
> +//
> +// Copyright(c) 2021 Advanced Micro Devices, Inc.
> +//
> +// Authors: Ajit Kumar Pandey <AjitKumar.Pandey@xxxxxxx>
> +//
> +
> +/* ACP machine configuration module */
> +
> +#include <linux/acpi.h>
> +#include <linux/bits.h>
> +#include <linux/dmi.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +
> +#include "../sof/amd/acp.h"

This doesn't seem to use anything from this header file?

> +#include "mach-config.h"
> +
> +static int acp_quirk_data;
> +
> +static const struct config_entry config_table[] = {
> +       {
> +               .flags = FLAG_AMD_SOF,
> +               .device = ACP_PCI_DEV_ID,
> +               .dmi_table = (const struct dmi_system_id []) {
> +                       {
> +                               .matches = {
> +                                       DMI_MATCH(DMI_SYS_VENDOR, "AMD"),
> +                                       DMI_MATCH(DMI_PRODUCT_NAME, "Majolica-CZN"),
> +                               },
> +                       },
> +                       {}
> +               },
> +       },
> +};
> +
> +int snd_amd_acp_find_config(struct pci_dev *pci)
> +{
> +       const struct config_entry *table = config_table;
> +       u16 device = pci->device;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(config_table); i++, table++) {
> +               if (table->device != device)
> +                       continue;
> +               if (table->dmi_table && !dmi_check_system(table->dmi_table))
> +                       continue;
> +               acp_quirk_data = table->flags;
> +               return table->flags;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(snd_amd_acp_find_config);

> +struct snd_soc_acpi_mach snd_soc_acpi_amd_acp_machines[] = {
> +       {
> +               .id = "AMDI1019",
> +               .drv_name = "renoir-acp",
> +               .pdata = (void *)&acp_quirk_data,
> +       },
> +       {},
> +};
> +EXPORT_SYMBOL(snd_soc_acpi_amd_acp_machines);

These symbols are only used from sound/soc/sof/amd/pci-rn.c.

Why is this code living under sound/soc/amd/, while the ACP code
is under sound/soc/amd/acp/?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds