On Wed, 18 Dec 2019 01:50:42 +0100, Pierre-Louis Bossart wrote:
diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c index bbeffd932de7..0b39ea6117cf 100644 --- a/sound/soc/sof/sof-pci-dev.c +++ b/sound/soc/sof/sof-pci-dev.c @@ -417,6 +417,10 @@ static const struct pci_device_id sof_pci_ids[] = { { PCI_DEVICE(0x8086, 0x06c8), .driver_data = (unsigned long)&cml_desc}, #endif +#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_S)
- { PCI_DEVICE(0x8086, 0xa3f0),
.driver_data = (unsigned long)&cml_desc},+#endif #if IS_ENABLED(CONFIG_SND_SOC_SOF_TIGERLAKE) { PCI_DEVICE(0x8086, 0xa0c8), .driver_data = (unsigned long)&tgl_desc},
Sorry Takashi, I was checking why this patch wasn't merged and only realized now that I missed your feedback (likely an effect of the Thanksgiving holiday).
I guess the change in ifdef for cml_desc definition is still missing.
Not sure what change you are referring to?
Take a look at the definition of cml_desc. It's wrapped with
#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_LP) || \ IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_H)
but this wasn't updated for the new COMETLAKE_S.
But, I wonder whether it'd be simpler to have Kconfigs only per sof_dev_desc? That is, have CONFIG_SND_SOC_SOF_COMETLAKE, and all CML model PCI entries are enabled by that as long as they use the same cml_desc definition.
it's difficult to have a classification that's complete and accurate, some PCH versions are reused in products that use a different family name. For example you will find the same PCI ID for CNL and WHL, and in others the -H, -U and -Y skews are not identical. Even Intel folks get lost at times, myself included.
For now we prefer having one Kconfig per PCI ID, and we try to provide a name for the Kconfig that is the most accurate without being cryptic. One of the reasons for having different Kconfigs is that we have multiple versions of the audio IP, and the ability to narrow the selection to a single device helps make sure the code is in the right place/module and dependencies are correct.
Also, can we reduce even the ifdef around sof_dev_desc definitions by __maybe_unused atttribute?
Sorry, I am not following your suggestion. I would really like to keep the ifdefs for now, and while it can be seen as overkill to have descriptors that are identical in some cases the past experience shows it's useful when we have to add quirks for specific 'hardware recommended programming sequences'.
What I suggested was simple, just dropping ifdef by something like
diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c index bbeffd932de7..297632a54f1b 100644 --- a/sound/soc/sof/sof-pci-dev.c +++ b/sound/soc/sof/sof-pci-dev.c @@ -36,8 +36,7 @@ MODULE_PARM_DESC(sof_pci_debug, "SOF PCI debug options (0x0 all off)");
#define SOF_PCI_DISABLE_PM_RUNTIME BIT(0)
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE) -static const struct sof_dev_desc bxt_desc = { +static const struct sof_dev_desc __maybe_unused bxt_desc = { .machines = snd_soc_acpi_intel_bxt_machines, .resindex_lpe_base = 0, .resindex_pcicfg_base = -1, @@ -52,10 +51,8 @@ static const struct sof_dev_desc bxt_desc = { .ops = &sof_apl_ops, .arch_ops = &sof_xtensa_arch_ops }; -#endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_GEMINILAKE) -static const struct sof_dev_desc glk_desc = { +static const struct sof_dev_desc __maybe_unused glk_desc = { .machines = snd_soc_acpi_intel_glk_machines, .resindex_lpe_base = 0, .resindex_pcicfg_base = -1, @@ -70,10 +67,8 @@ static const struct sof_dev_desc glk_desc = { .ops = &sof_apl_ops, .arch_ops = &sof_xtensa_arch_ops }; -#endif .....
Then the issue I pointed above can be solved as well.
thanks,
Takashi