Re: [alsa-devel] [PATCH] ASoC: SOF: Intel: add PCI ID for CometLake-S

[Takashi Iwai <tiwai@suse.de>]

On Wed, 18 Dec 2019 16:21:22 +0100,
Pierre-Louis Bossart wrote:
> 
> >>> 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.
> 
> The ifdefs are still needed in the PCI IDs tables

Yes, but it halves the messes :)


Takashi

> 
> static const struct pci_device_id sof_pci_ids[] = {
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_MERRIFIELD)
> 	{ PCI_DEVICE(0x8086, 0x119a),
> 		.driver_data = (unsigned long)&tng_desc},
> #endif
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE)
> 	/* BXT-P & Apollolake */
> 	{ PCI_DEVICE(0x8086, 0x5a98),
> 		.driver_data = (unsigned long)&bxt_desc},
> 	{ PCI_DEVICE(0x8086, 0x1a98),
> 		.driver_data = (unsigned long)&bxt_desc},
> #endif
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_GEMINILAKE)
> 	{ PCI_DEVICE(0x8086, 0x3198),
> 		.driver_data = (unsigned long)&glk_desc},
> #endif
> 
> so for consistency I personally prefer the matching ifdef for the
> descriptors.
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel