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