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

[Takashi Iwai <tiwai@suse.de>]

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