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