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

[Takashi Iwai <tiwai@suse.de>]

On Wed, 18 Dec 2019 17:42:24 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 12/18/19 10:28 AM, Takashi Iwai wrote:
> > 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 :)
> 
> I wish it was true :-)
> 
> In reality having buildbots play with kconfig options does help
> identify issues at the code level, just like the namespace use helped
> identify the .arch_ops just above did not belong here.
> I find it's a constant battle to avoid accumulated crud in the wrong
> places when dealing with multiple platforms, and when looking at
> patches it's very hard (at least for me) to realize where the code
> gets added and the implications.

But how it can be worse than ifdef...?  From the resultant code POV,
it's same, the redundant objects are dropped automatically, while you
can avoid a pitfall like this case to forget the counter-part ifdef,
which could be identified at first by some randconfig tests.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel