[alsa-devel] [PATCH] ASoC: SOF: Intel: add PCI ID for CometLake-S
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},
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) platformsusing 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
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
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
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.
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.
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.
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
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).
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
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.
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai