>>>> @@ -809,6 +814,10 @@ void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops) >>>> if (!hda_use_tplg_nhlt) >>>> ipc4_data->nhlt = intel_nhlt_init(sdev->dev); >>>> >>>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE) >>>> + sdw_callback.trigger = ipc4_be_dai_common_trigger; >>>> +#endif >>> >>> #if should not be in .c files if at all possible. Surely there's a >>> better way here... >> >> we could use >> >> if (IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)) >> sdw_callback.trigger = ipc4_be_dai_common_trigger; >> >> would that work? > > It's more readable, right? Also easier to maintain over time. yes, it's more readable, and that's no problem to update. There's another #if IS_ENABLED( that we can replace by a if (IS_ENABLED in the same routine, that would make this routine less of an eyesore. I am not sure why we added all these #if, we can cleanup. case SOF_INTEL_IPC4: { struct sof_ipc4_fw_data *ipc4_data = sdev->private; for (i = 0; i < ops->num_drv; i++) { if (strstr(ops->drv[i].name, "DMIC")) { ops->drv[i].ops = &ipc4_dmic_dai_ops; continue; } if (strstr(ops->drv[i].name, "SSP")) { ops->drv[i].ops = &ipc4_ssp_dai_ops; continue; } #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) if (strstr(ops->drv[i].name, "iDisp") || strstr(ops->drv[i].name, "Analog") || strstr(ops->drv[i].name, "Digital")) ops->drv[i].ops = &ipc4_hda_dai_ops; #endif } if (!hda_use_tplg_nhlt) ipc4_data->nhlt = intel_nhlt_init(sdev->dev); #if IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE) sdw_callback.trigger = ipc4_be_dai_common_trigger; #endif break; } default: break; } } >> We try to keep this driver configurable, not all platforms require >> SoundWire or HDaudio, and that 'sdw_callback' ops structure is >> conditionally declared. > > Perhaps don't conditionally declare that? How much does it really save > to do that? It would force us to expose additional things that are only relevant for SoundWire, see the large block of code in hda.c https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/intel/hda.c#L108 We've been burned in the past by having too many things in a single configuration, so we try to allow for minimal builds that don't depend on other modules in sound/soc/codecs/, sound/pci/hda and drivers/soundwire - it also forces us to get the Kconfig dependencies right. if (IS_ENABLED()) is less invasive in that case. Thanks for your feedback -Pierre