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

[Hans de Goede <hdegoede@redhat.com>]

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