| Message ID | 20210208093800.62099-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ALSA: hda: intel-dsp-config: Add FLAG_BYT_FIRST / _SECOND defines | expand |
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 :) So, it'd be appreciated if this can be re-designed and simplified... Takashi
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
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@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. 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 > > >
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@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 >> >> >> >
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@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
diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index c45686172517..2201a1e6944e 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -452,6 +452,16 @@ int snd_intel_dsp_driver_probe(struct pci_dev *pci) } EXPORT_SYMBOL_GPL(snd_intel_dsp_driver_probe); +/* FLAG_BYT_FIRST / _SECOND determine which driver is preferred on BYT/CHT when both are enabled */ +#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) +#define FLAG_BYT_FIRST FLAG_SST +#define FLAG_BYT_SECOND FLAG_SOF +#elif IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) +#define FLAG_BYT_FIRST FLAG_SST +#elif IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) +#define FLAG_BYT_FIRST FLAG_SOF +#endif + /* * configuration table * - the order of similar ACPI ID entries is important! @@ -459,28 +469,28 @@ 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, + .flags = FLAG_BYT_FIRST, .acpi_hid = "80860F28", }, #endif -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) +#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) { - .flags = FLAG_SOF, + .flags = FLAG_BYT_SECOND, .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, + .flags = FLAG_BYT_FIRST, .acpi_hid = "808622A8", }, #endif -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) +#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) { - .flags = FLAG_SOF, + .flags = FLAG_BYT_SECOND, .acpi_hid = "808622A8", }, #endif
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> --- sound/hda/intel-dsp-config.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)