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

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org, broonie@kernel.org
Subject: Re: [alsa-devel] [PATCH] ASoC: SOF: Intel: add PCI ID for CometLake-S
Date: Tue, 17 Dec 2019 18:50:42 -0600	[thread overview]
Message-ID: <0131b134-46dc-991f-230b-fe843f43f078@linux.intel.com> (raw)
In-Reply-To: <s5h4kyqzpge.wl-tiwai@suse.de>

>> 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?

> 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'.

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