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