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

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

[Pierre-Louis Bossart]

Mirror ID added for legacy HDaudio

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/Kconfig | 16 ++++++++++++++++
 sound/soc/sof/sof-pci-dev.c |  4 ++++
 2 files changed, 20 insertions(+)

diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
index cc09bb606f7d..91c8bbcc015a 100644
--- a/sound/soc/sof/intel/Kconfig
+++ b/sound/soc/sof/intel/Kconfig
@@ -27,6 +27,7 @@ config SND_SOC_SOF_INTEL_PCI
 	select SND_SOC_SOF_ICELAKE     if SND_SOC_SOF_ICELAKE_SUPPORT
 	select SND_SOC_SOF_COMETLAKE_LP if SND_SOC_SOF_COMETLAKE_LP_SUPPORT
 	select SND_SOC_SOF_COMETLAKE_H if SND_SOC_SOF_COMETLAKE_H_SUPPORT
+	select SND_SOC_SOF_COMETLAKE_S if SND_SOC_SOF_COMETLAKE_S_SUPPORT
 	select SND_SOC_SOF_TIGERLAKE   if SND_SOC_SOF_TIGERLAKE_SUPPORT
 	select SND_SOC_SOF_ELKHARTLAKE if SND_SOC_SOF_ELKHARTLAKE_SUPPORT
 	select SND_SOC_SOF_JASPERLAKE  if SND_SOC_SOF_JASPERLAKE_SUPPORT
@@ -231,6 +232,21 @@ config SND_SOC_SOF_COMETLAKE_H_SUPPORT
 	  Say Y if you have such a device.
 	  If unsure select "N".
 
+config SND_SOC_SOF_COMETLAKE_S
+	tristate
+	select SND_SOC_SOF_HDA_COMMON
+	help
+	  This option is not user-selectable but automagically handled by
+	  'select' statements at a higher level
+
+config SND_SOC_SOF_COMETLAKE_S_SUPPORT
+	bool "SOF support for CometLake-S"
+	help
+	  This adds support for Sound Open Firmware for Intel(R) platforms
+	  using the Cometlake-H processors.
+	  Say Y if you have such a device.
+	  If unsure select "N".
+
 config SND_SOC_SOF_TIGERLAKE_SUPPORT
 	bool "SOF support for Tigerlake"
 	help
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},
-- 
2.20.1

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

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

[Takashi Iwai]

On Tue, 26 Nov 2019 15:14:23 +0100,
Pierre-Louis Bossart wrote:
> 
> Mirror ID added for legacy HDaudio
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/sof/intel/Kconfig | 16 ++++++++++++++++
>  sound/soc/sof/sof-pci-dev.c |  4 ++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
> index cc09bb606f7d..91c8bbcc015a 100644
> --- a/sound/soc/sof/intel/Kconfig
> +++ b/sound/soc/sof/intel/Kconfig
> @@ -27,6 +27,7 @@ config SND_SOC_SOF_INTEL_PCI
>  	select SND_SOC_SOF_ICELAKE     if SND_SOC_SOF_ICELAKE_SUPPORT
>  	select SND_SOC_SOF_COMETLAKE_LP if SND_SOC_SOF_COMETLAKE_LP_SUPPORT
>  	select SND_SOC_SOF_COMETLAKE_H if SND_SOC_SOF_COMETLAKE_H_SUPPORT
> +	select SND_SOC_SOF_COMETLAKE_S if SND_SOC_SOF_COMETLAKE_S_SUPPORT
>  	select SND_SOC_SOF_TIGERLAKE   if SND_SOC_SOF_TIGERLAKE_SUPPORT
>  	select SND_SOC_SOF_ELKHARTLAKE if SND_SOC_SOF_ELKHARTLAKE_SUPPORT
>  	select SND_SOC_SOF_JASPERLAKE  if SND_SOC_SOF_JASPERLAKE_SUPPORT
> @@ -231,6 +232,21 @@ config SND_SOC_SOF_COMETLAKE_H_SUPPORT
>  	  Say Y if you have such a device.
>  	  If unsure select "N".
>  
> +config SND_SOC_SOF_COMETLAKE_S
> +	tristate
> +	select SND_SOC_SOF_HDA_COMMON
> +	help
> +	  This option is not user-selectable but automagically handled by
> +	  'select' statements at a higher level
> +
> +config SND_SOC_SOF_COMETLAKE_S_SUPPORT
> +	bool "SOF support for CometLake-S"
> +	help
> +	  This adds support for Sound Open Firmware for Intel(R) platforms
> +	  using the Cometlake-H processors.
> +	  Say Y if you have such a device.
> +	  If unsure select "N".
> +
>  config SND_SOC_SOF_TIGERLAKE_SUPPORT
>  	bool "SOF support for Tigerlake"
>  	help
> 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},

I guess the change in ifdef for cml_desc definition is still missing.

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.

Also, can we reduce even the ifdef around sof_dev_desc definitions by
__maybe_unused atttribute?


thanks,

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

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

[Pierre-Louis Bossart]

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

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

[Takashi Iwai]

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@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Pierre-Louis Bossart]

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

Ah yes, that's a problem. will fix.

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

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

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

[Takashi Iwai]

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

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

[Pierre-Louis Bossart]

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

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

[Takashi Iwai]

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

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

[Pierre-Louis Bossart]

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

In a perfect world it'd be fine.
But the reviews are not perfect and it happens that we let things go 
through.
With the _maybe_unused proposal, I would not know which objects are not 
necessary for a specific config, they would be silently removed by a 
tool. Issues reported by randconfig or 'unused variable' warnings are 
painful but at least they do provide a clear hint that something's not 
right (including in my own code).
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

On Wed, 18 Dec 2019 20:01:55 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>>>> 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.
> 
> In a perfect world it'd be fine.
> But the reviews are not perfect and it happens that we let things go
> through.
> With the _maybe_unused proposal, I would not know which objects are
> not necessary for a specific config, they would be silently removed by
> a tool. Issues reported by randconfig or 'unused variable' warnings
> are painful but at least they do provide a clear hint that something's
> not right (including in my own code).

Well, it's another side of coin.  With the current massive Kconfig and
ifdef, something can be overlooked pretty easily.  A randconfig may
catch it up, but this would be often after the commit.

But it's basically just a minor bikeshed thing.  However, the main
concern is: unless Intel stops producing a newer model, we'll go soon
beyond the amount of Kconfigs we can manage manually.

Usually splitting Kconfig items makes sense if -

- Functionality needs to be selective,
- It reduces the resultant code significantly, or
- Modules to be built can be chosen (and reduces memory footprint)

When looking at the current code, for example, I'm not sure which
category hits the case of Cometlake-S vs Cometlake-H vs
Cometlake-LP...

In short: I'd love to see if Kconfigs can be reduced as much as
possible.  It'll make our life easier for users, including distros, in
the end.


thanks,

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

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 341 bytes --]

On Wed, Dec 18, 2019 at 09:02:30PM +0100, Takashi Iwai wrote:

> When looking at the current code, for example, I'm not sure which
> category hits the case of Cometlake-S vs Cometlake-H vs
> Cometlake-LP...

Yeah, it's a bit much at the minute - there's enormous resolution there
with the Intel systems and I'm not 100% sure who's using it.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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