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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel