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

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