[PATCH 1/2] ALSA: hda: intel-dsp-config: Add FLAG_BYT_FIRST / _SECOND defines

[PATCH 1/2] ALSA: hda: intel-dsp-config: Add FLAG_BYT_FIRST / _SECOND defines

[Hans de Goede]

Instead of hardcording the SST driver having the highest prio, add
FLAG_BYT_FIRST and FLAG_BYT_SECOND defines, which get set like this
when both drivers are enabled:

	#define FLAG_BYT_FIRST               FLAG_SST
	#define FLAG_BYT_SECOND              FLAG_SOF

And when only 1 driver is enabled then FLAG_BYT_FIRST gets set to
the flag for that driver.

This is a preparation patch for making which driver is preferred
configurable through Kconfig.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/hda/intel-dsp-config.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c
index c45686172517..2201a1e6944e 100644
--- a/sound/hda/intel-dsp-config.c
+++ b/sound/hda/intel-dsp-config.c
@@ -452,6 +452,16 @@ int snd_intel_dsp_driver_probe(struct pci_dev *pci)
 }
 EXPORT_SYMBOL_GPL(snd_intel_dsp_driver_probe);
 
+/* FLAG_BYT_FIRST / _SECOND determine which driver is preferred on BYT/CHT when both are enabled */
+#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
+#define FLAG_BYT_FIRST		FLAG_SST
+#define FLAG_BYT_SECOND		FLAG_SOF
+#elif IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
+#define FLAG_BYT_FIRST		FLAG_SST
+#elif IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
+#define FLAG_BYT_FIRST		FLAG_SOF
+#endif
+
 /*
  * configuration table
  * - the order of similar ACPI ID entries is important!
@@ -459,28 +469,28 @@ EXPORT_SYMBOL_GPL(snd_intel_dsp_driver_probe);
  */
 static const struct config_entry acpi_config_table[] = {
 /* BayTrail */
-#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
+#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) || IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
 	{
-		.flags = FLAG_SST,
+		.flags = FLAG_BYT_FIRST,
 		.acpi_hid = "80860F28",
 	},
 #endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
+#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
 	{
-		.flags = FLAG_SOF,
+		.flags = FLAG_BYT_SECOND,
 		.acpi_hid = "80860F28",
 	},
 #endif
 /* CherryTrail */
-#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
+#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) || IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
 	{
-		.flags = FLAG_SST,
+		.flags = FLAG_BYT_FIRST,
 		.acpi_hid = "808622A8",
 	},
 #endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
+#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
 	{
-		.flags = FLAG_SOF,
+		.flags = FLAG_BYT_SECOND,
 		.acpi_hid = "808622A8",
 	},
 #endif
-- 
2.30.0

[PATCH 2/2] ALSA: hda: intel-dsp-config: Add SND_INTEL_BYT_PREFER_SOF Kconfig option

[Hans de Goede]

The kernel has 2 drivers for the Low Power Engine audio-block on
Bay- and Cherry-Trail SoCs. The old SST driver and the new SOF
driver. If both drivers are enabled then the kernel will default
to using the old SST driver, unless told otherwise through the
snd_intel_dspcfg.dsp_driver module-parameter.

Add a boolean SND_INTEL_BYT_PREFER_SOF Kconfig option, which when set to Y
will make the kernel default to the new SOF driver instead.

Making this configurable will help distributions such as Fedora:
https://fedoraproject.org/w/index.php?title=Changes/SofDefaultForIntelLpe
to test using SOF on BYT/CHT during the transition phase where we
have both drivers (eventually the old driver and this option will
be removed).

Note the option defaults to n, preserving the current behavior.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/hda/Kconfig            | 14 ++++++++++++++
 sound/hda/intel-dsp-config.c |  5 +++++
 2 files changed, 19 insertions(+)

diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig
index 3bc9224d5e4f..9ed5cfa3c18c 100644
--- a/sound/hda/Kconfig
+++ b/sound/hda/Kconfig
@@ -46,3 +46,17 @@ config SND_INTEL_DSP_CONFIG
 	select SND_INTEL_NHLT if ACPI
 	# this config should be selected only for Intel DSP platforms.
 	# A fallback is provided so that the code compiles in all cases.
+
+config SND_INTEL_BYT_PREFER_SOF
+	bool "Prefer SOF driver over SST on BY/CHT platforms"
+	depends on SND_SST_ATOM_HIFI2_PLATFORM_ACPI && SND_SOC_SOF_BAYTRAIL
+	default n
+	help
+	  The kernel has 2 drivers for the Low Power Engine audio-block on
+	  Bay- and Cherry-Trail SoCs. The old SST driver and the new SOF
+	  driver. If both drivers are enabled then the kernel will default
+	  to using the old SST driver, unless told otherwise through the
+	  snd_intel_dspcfg.dsp_driver module-parameter.
+
+	  Set this option to Y to make the kernel default to the new SOF
+	  driver instead.
diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c
index 2201a1e6944e..fd288df7ede9 100644
--- a/sound/hda/intel-dsp-config.c
+++ b/sound/hda/intel-dsp-config.c
@@ -454,8 +454,13 @@ EXPORT_SYMBOL_GPL(snd_intel_dsp_driver_probe);
 
 /* FLAG_BYT_FIRST / _SECOND determine which driver is preferred on BYT/CHT when both are enabled */
 #if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
+#ifdef CONFIG_SND_INTEL_BYT_PREFER_SOF
+#define FLAG_BYT_FIRST		FLAG_SOF
+#define FLAG_BYT_SECOND		FLAG_SST
+#else
 #define FLAG_BYT_FIRST		FLAG_SST
 #define FLAG_BYT_SECOND		FLAG_SOF
+#endif
 #elif IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
 #define FLAG_BYT_FIRST		FLAG_SST
 #elif IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-- 
2.30.0

Re: [PATCH 1/2] ALSA: hda: intel-dsp-config: Add FLAG_BYT_FIRST / _SECOND defines

[Takashi Iwai]

On Mon, 08 Feb 2021 10:37:59 +0100,
Hans de Goede wrote:
> 
> Instead of hardcording the SST driver having the highest prio, add
> FLAG_BYT_FIRST and FLAG_BYT_SECOND defines, which get set like this
> when both drivers are enabled:
> 
> 	#define FLAG_BYT_FIRST               FLAG_SST
> 	#define FLAG_BYT_SECOND              FLAG_SOF
> 
> And when only 1 driver is enabled then FLAG_BYT_FIRST gets set to
> the flag for that driver.
> 
> This is a preparation patch for making which driver is preferred
> configurable through Kconfig.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I find the idea is fine, but the ifdef conditions become too complex
after this change.  It took minutes to check whether the ifdef changes
are really correct for me :)

So, it'd be appreciated if this can be re-designed and simplified...


Takashi

Re: [PATCH 1/2] ALSA: hda: intel-dsp-config: Add FLAG_BYT_FIRST / _SECOND defines

[Hans de Goede]

Hi,

On 2/8/21 11:04 AM, Takashi Iwai wrote:
> On Mon, 08 Feb 2021 10:37:59 +0100,
> Hans de Goede wrote:
>>
>> Instead of hardcording the SST driver having the highest prio, add
>> FLAG_BYT_FIRST and FLAG_BYT_SECOND defines, which get set like this
>> when both drivers are enabled:
>>
>> 	#define FLAG_BYT_FIRST               FLAG_SST
>> 	#define FLAG_BYT_SECOND              FLAG_SOF
>>
>> And when only 1 driver is enabled then FLAG_BYT_FIRST gets set to
>> the flag for that driver.
>>
>> This is a preparation patch for making which driver is preferred
>> configurable through Kconfig.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> I find the idea is fine, but the ifdef conditions become too complex
> after this change.  It took minutes to check whether the ifdef changes
> are really correct for me :)

I understand but...

> So, it'd be appreciated if this can be re-designed and simplified...

This was actually the cleanest which I could come up with, well maybe not the
cleanest, but the most "do not repeat yourself" option.

The alternative would be something like this:

static const struct config_entry acpi_config_table[] = {
/* BayTrail */
#ifdef CONFIG_SND_INTEL_BYT_PREFER_SOF /* implies both drivers are enabled */
        {
                .flags = FLAG_SOF,
                .acpi_hid = "80860F28",
        },
        {
                .flags = FLAG_SST,
                .acpi_hid = "80860F28",
        },
#else
#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
        {
                .flags = FLAG_SST,
                .acpi_hid = "80860F28",
        },
#endif
#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL
        {
                .flags = FLAG_SOF,
                .acpi_hid = "80860F28",
        },
#endif
#endif

With the same thing repeating for the Cherry Trail case, now that
I actually have written this out I guess it is not too bad, but it
does mean repeating all the BYT/CHT entries once, visually
leading to 4 extra entries (but the #ifdef #else #endif
will always include only 2/4 for each of BYT and CHT.

If you like this better I can do a v2 with this approach, that
would also reduce the set to a single patch.

Regards,

Hans

Re: [PATCH 1/2] ALSA: hda: intel-dsp-config: Add FLAG_BYT_FIRST / _SECOND defines

[Takashi Iwai]

On Mon, 08 Feb 2021 11:24:53 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 2/8/21 11:04 AM, Takashi Iwai wrote:
> > On Mon, 08 Feb 2021 10:37:59 +0100,
> > Hans de Goede wrote:
> >>
> >> Instead of hardcording the SST driver having the highest prio, add
> >> FLAG_BYT_FIRST and FLAG_BYT_SECOND defines, which get set like this
> >> when both drivers are enabled:
> >>
> >> 	#define FLAG_BYT_FIRST               FLAG_SST
> >> 	#define FLAG_BYT_SECOND              FLAG_SOF
> >>
> >> And when only 1 driver is enabled then FLAG_BYT_FIRST gets set to
> >> the flag for that driver.
> >>
> >> This is a preparation patch for making which driver is preferred
> >> configurable through Kconfig.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > I find the idea is fine, but the ifdef conditions become too complex
> > after this change.  It took minutes to check whether the ifdef changes
> > are really correct for me :)
> 
> I understand but...
> 
> > So, it'd be appreciated if this can be re-designed and simplified...
> 
> This was actually the cleanest which I could come up with, well maybe not the
> cleanest, but the most "do not repeat yourself" option.
> 
> The alternative would be something like this:
> 
> static const struct config_entry acpi_config_table[] = {
> /* BayTrail */
> #ifdef CONFIG_SND_INTEL_BYT_PREFER_SOF /* implies both drivers are enabled */
>         {
>                 .flags = FLAG_SOF,
>                 .acpi_hid = "80860F28",
>         },
>         {
>                 .flags = FLAG_SST,
>                 .acpi_hid = "80860F28",
>         },
> #else
> #if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
>         {
>                 .flags = FLAG_SST,
>                 .acpi_hid = "80860F28",
>         },
> #endif
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL
>         {
>                 .flags = FLAG_SOF,
>                 .acpi_hid = "80860F28",
>         },
> #endif
> #endif
> 
> With the same thing repeating for the Cherry Trail case, now that
> I actually have written this out I guess it is not too bad, but it
> does mean repeating all the BYT/CHT entries once, visually
> leading to 4 extra entries (but the #ifdef #else #endif
> will always include only 2/4 for each of BYT and CHT.
> 
> If you like this better I can do a v2 with this approach, that
> would also reduce the set to a single patch.

If I understand correctly, we don't need to have two entries since the
first matching always wins.

So it could be something like below?


thanks,

Takashi

--- a/sound/hda/intel-dsp-config.c
+++ b/sound/hda/intel-dsp-config.c
@@ -26,6 +26,12 @@ MODULE_PARM_DESC(dsp_driver, "Force the DSP driver for Intel DSP (0=auto, 1=lega
 #define FLAG_SOF_ONLY_IF_DMIC_OR_SOUNDWIRE (FLAG_SOF_ONLY_IF_DMIC | \
 					    FLAG_SOF_ONLY_IF_SOUNDWIRE)
 
+#if IS_ENABLED(CONFIG_SND_SOC_PREFER_SOF_BAYTRAIL)
+#define FLAG_SST_OR_SOF_BYT		FLAG_SOF
+#else
+#define FLAG_SST_OR_SOF_BYT		FLAG_SST
+#endif
+
 struct config_entry {
 	u32 flags;
 	u16 device;
@@ -459,28 +465,18 @@ EXPORT_SYMBOL_GPL(snd_intel_dsp_driver_probe);
  */
 static const struct config_entry acpi_config_table[] = {
 /* BayTrail */
-#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
+#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) || \
+    IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
 	{
-		.flags = FLAG_SST,
-		.acpi_hid = "80860F28",
-	},
-#endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-	{
-		.flags = FLAG_SOF,
+		.flags = FLAG_SST_OR_SOF_BYT,
 		.acpi_hid = "80860F28",
 	},
 #endif
 /* CherryTrail */
-#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
+#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) || \
+    IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
 	{
-		.flags = FLAG_SST,
-		.acpi_hid = "808622A8",
-	},
-#endif
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-	{
-		.flags = FLAG_SOF,
+		.flags = FLAG_SST_OR_SOF_BYT,
 		.acpi_hid = "808622A8",
 	},
 #endif



> 
> Regards,
> 
> Hans
> 
> 
> 

Re: [PATCH 1/2] ALSA: hda: intel-dsp-config: Add FLAG_BYT_FIRST / _SECOND defines

[Hans de Goede]

Hi,

On 2/8/21 12:02 PM, Takashi Iwai wrote:
> On Mon, 08 Feb 2021 11:24:53 +0100,
> Hans de Goede wrote:
>>
>> Hi,
>>
>> On 2/8/21 11:04 AM, Takashi Iwai wrote:
>>> On Mon, 08 Feb 2021 10:37:59 +0100,
>>> Hans de Goede wrote:
>>>>
>>>> Instead of hardcording the SST driver having the highest prio, add
>>>> FLAG_BYT_FIRST and FLAG_BYT_SECOND defines, which get set like this
>>>> when both drivers are enabled:
>>>>
>>>> 	#define FLAG_BYT_FIRST               FLAG_SST
>>>> 	#define FLAG_BYT_SECOND              FLAG_SOF
>>>>
>>>> And when only 1 driver is enabled then FLAG_BYT_FIRST gets set to
>>>> the flag for that driver.
>>>>
>>>> This is a preparation patch for making which driver is preferred
>>>> configurable through Kconfig.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> I find the idea is fine, but the ifdef conditions become too complex
>>> after this change.  It took minutes to check whether the ifdef changes
>>> are really correct for me :)
>>
>> I understand but...
>>
>>> So, it'd be appreciated if this can be re-designed and simplified...
>>
>> This was actually the cleanest which I could come up with, well maybe not the
>> cleanest, but the most "do not repeat yourself" option.
>>
>> The alternative would be something like this:
>>
>> static const struct config_entry acpi_config_table[] = {
>> /* BayTrail */
>> #ifdef CONFIG_SND_INTEL_BYT_PREFER_SOF /* implies both drivers are enabled */
>>         {
>>                 .flags = FLAG_SOF,
>>                 .acpi_hid = "80860F28",
>>         },
>>         {
>>                 .flags = FLAG_SST,
>>                 .acpi_hid = "80860F28",
>>         },
>> #else
>> #if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
>>         {
>>                 .flags = FLAG_SST,
>>                 .acpi_hid = "80860F28",
>>         },
>> #endif
>> #if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL
>>         {
>>                 .flags = FLAG_SOF,
>>                 .acpi_hid = "80860F28",
>>         },
>> #endif
>> #endif
>>
>> With the same thing repeating for the Cherry Trail case, now that
>> I actually have written this out I guess it is not too bad, but it
>> does mean repeating all the BYT/CHT entries once, visually
>> leading to 4 extra entries (but the #ifdef #else #endif
>> will always include only 2/4 for each of BYT and CHT.
>>
>> If you like this better I can do a v2 with this approach, that
>> would also reduce the set to a single patch.
> 
> If I understand correctly, we don't need to have two entries since the
> first matching always wins.

Yes that is true,

> So it could be something like below?

> --- a/sound/hda/intel-dsp-config.c
> +++ b/sound/hda/intel-dsp-config.c
> @@ -26,6 +26,12 @@ MODULE_PARM_DESC(dsp_driver, "Force the DSP driver for Intel DSP (0=auto, 1=lega
>  #define FLAG_SOF_ONLY_IF_DMIC_OR_SOUNDWIRE (FLAG_SOF_ONLY_IF_DMIC | \
>  					    FLAG_SOF_ONLY_IF_SOUNDWIRE)
>  
> +#if IS_ENABLED(CONFIG_SND_SOC_PREFER_SOF_BAYTRAIL)

This condition would need to be changed to:

#if IS_ENABLED(CONFIG_SND_SOC_PREFER_SOF_BAYTRAIL) || !IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)

In case only the SOF driver is enabled.

With that changed I believe that your suggestion should work.

Shall I prepare a new patch going this route ?

Regards,

Hans



> +#define FLAG_SST_OR_SOF_BYT		FLAG_SOF
> +#else
> +#define FLAG_SST_OR_SOF_BYT		FLAG_SST
> +#endif
> +
>  struct config_entry {
>  	u32 flags;
>  	u16 device;
> @@ -459,28 +465,18 @@ EXPORT_SYMBOL_GPL(snd_intel_dsp_driver_probe);
>   */
>  static const struct config_entry acpi_config_table[] = {
>  /* BayTrail */
> -#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
> +#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) || \
> +    IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
>  	{
> -		.flags = FLAG_SST,
> -		.acpi_hid = "80860F28",
> -	},
> -#endif
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -	{
> -		.flags = FLAG_SOF,
> +		.flags = FLAG_SST_OR_SOF_BYT,
>  		.acpi_hid = "80860F28",
>  	},
>  #endif
>  /* CherryTrail */
> -#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
> +#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) || \
> +    IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
>  	{
> -		.flags = FLAG_SST,
> -		.acpi_hid = "808622A8",
> -	},
> -#endif
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -	{
> -		.flags = FLAG_SOF,
> +		.flags = FLAG_SST_OR_SOF_BYT,
>  		.acpi_hid = "808622A8",
>  	},
>  #endif
> 
> 
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
> 

Re: [PATCH 1/2] ALSA: hda: intel-dsp-config: Add FLAG_BYT_FIRST / _SECOND defines

[Takashi Iwai]

On Mon, 08 Feb 2021 12:08:52 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 2/8/21 12:02 PM, Takashi Iwai wrote:
> > On Mon, 08 Feb 2021 11:24:53 +0100,
> > Hans de Goede wrote:
> >>
> >> Hi,
> >>
> >> On 2/8/21 11:04 AM, Takashi Iwai wrote:
> >>> On Mon, 08 Feb 2021 10:37:59 +0100,
> >>> Hans de Goede wrote:
> >>>>
> >>>> Instead of hardcording the SST driver having the highest prio, add
> >>>> FLAG_BYT_FIRST and FLAG_BYT_SECOND defines, which get set like this
> >>>> when both drivers are enabled:
> >>>>
> >>>> 	#define FLAG_BYT_FIRST               FLAG_SST
> >>>> 	#define FLAG_BYT_SECOND              FLAG_SOF
> >>>>
> >>>> And when only 1 driver is enabled then FLAG_BYT_FIRST gets set to
> >>>> the flag for that driver.
> >>>>
> >>>> This is a preparation patch for making which driver is preferred
> >>>> configurable through Kconfig.
> >>>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>
> >>> I find the idea is fine, but the ifdef conditions become too complex
> >>> after this change.  It took minutes to check whether the ifdef changes
> >>> are really correct for me :)
> >>
> >> I understand but...
> >>
> >>> So, it'd be appreciated if this can be re-designed and simplified...
> >>
> >> This was actually the cleanest which I could come up with, well maybe not the
> >> cleanest, but the most "do not repeat yourself" option.
> >>
> >> The alternative would be something like this:
> >>
> >> static const struct config_entry acpi_config_table[] = {
> >> /* BayTrail */
> >> #ifdef CONFIG_SND_INTEL_BYT_PREFER_SOF /* implies both drivers are enabled */
> >>         {
> >>                 .flags = FLAG_SOF,
> >>                 .acpi_hid = "80860F28",
> >>         },
> >>         {
> >>                 .flags = FLAG_SST,
> >>                 .acpi_hid = "80860F28",
> >>         },
> >> #else
> >> #if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
> >>         {
> >>                 .flags = FLAG_SST,
> >>                 .acpi_hid = "80860F28",
> >>         },
> >> #endif
> >> #if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL
> >>         {
> >>                 .flags = FLAG_SOF,
> >>                 .acpi_hid = "80860F28",
> >>         },
> >> #endif
> >> #endif
> >>
> >> With the same thing repeating for the Cherry Trail case, now that
> >> I actually have written this out I guess it is not too bad, but it
> >> does mean repeating all the BYT/CHT entries once, visually
> >> leading to 4 extra entries (but the #ifdef #else #endif
> >> will always include only 2/4 for each of BYT and CHT.
> >>
> >> If you like this better I can do a v2 with this approach, that
> >> would also reduce the set to a single patch.
> > 
> > If I understand correctly, we don't need to have two entries since the
> > first matching always wins.
> 
> Yes that is true,
> 
> > So it could be something like below?
> 
> > --- a/sound/hda/intel-dsp-config.c
> > +++ b/sound/hda/intel-dsp-config.c
> > @@ -26,6 +26,12 @@ MODULE_PARM_DESC(dsp_driver, "Force the DSP driver for Intel DSP (0=auto, 1=lega
> >  #define FLAG_SOF_ONLY_IF_DMIC_OR_SOUNDWIRE (FLAG_SOF_ONLY_IF_DMIC | \
> >  					    FLAG_SOF_ONLY_IF_SOUNDWIRE)
> >  
> > +#if IS_ENABLED(CONFIG_SND_SOC_PREFER_SOF_BAYTRAIL)
> 
> This condition would need to be changed to:
> 
> #if IS_ENABLED(CONFIG_SND_SOC_PREFER_SOF_BAYTRAIL) || !IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI)
> 
> In case only the SOF driver is enabled.
> 
> With that changed I believe that your suggestion should work.
> 
> Shall I prepare a new patch going this route ?

Yes, please.  It's easier to understand (to my eyes).


thanks,

Takashi