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