On Fri, 31 May 2019 22:23:37 +0200, Pierre-Louis Bossart wrote:
On 5/31/19 2:44 PM, Takashi Iwai wrote:
On Fri, 31 May 2019 21:06:25 +0200, Takashi Iwai wrote:
On Fri, 31 May 2019 20:31:33 +0200, Pierre-Louis Bossart wrote:
On 5/31/19 1:22 PM, Takashi Iwai wrote:
On Fri, 31 May 2019 19:43:59 +0200, Pierre-Louis Bossart wrote:
On 5/31/19 12:11 PM, Takashi Iwai wrote: > Hi, > > while looking at SOF code due to the recent debugging session, I > noticed that sof_hda_bus_init() is basically an open-code of the > existing snd_hdac_ext_bus_init(). Why don't we simply call > snd_hdac_ext_bus_init() like below?
It's intentional. We've been asked since Day1 of SOF on ApolloLake to provide a 'self-contained' controller-only support that has no dependency on the snd_hdac library for solutions where HDaudio links+codecs are not used (typically IOT devices). This was driven by the lack of separation between layers in that library as well as a desire to have a dual-license. That's why you see the init and some of the basic utilities re-implemented for SOF.
However for cases where HDaudio+HDMI are required, we didn't want to reinvent the wheel - HDaudio is complicated enough - and do make use of this snd_hdac library.
We have a config SND_SOC_SOF_HDA that controls in which mode we operate, and it enables HDMI by default (for I2S+HDMI solutions). To get external HDaudio codecs you need the additional SOF_HDAUDIO_CODEC kconfig.
Does this help?
Well, what's wrong with the conditional build with Kconfig? You can just wrap the call snd_hdac_ext_bus_init() with #if/endif, e.g. in soc/sof/intel/hda.h,
static inline void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev, const struct hdac_ext_bus_ops *ext_ops) { #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) snd_hdac_ext_bus_init(bus, dev, NULL, NULL, ext_ops); #endif }
We still need initializations for some of the data structures when SOF_HDA is not defined.
Which data structure? The function above is only initializing the given struct hdac_bus object. I'm not suggesting to change the caller site, hda_init() of sound/soc/sof/intel/hda.c.
we need everything that was removed in your proposal :-)
- memset(bus, 0, sizeof(*bus));
- bus->dev = dev;
- bus->io_ops = &io_ops;
- INIT_LIST_HEAD(&bus->stream_list);
- bus->irq = -1;
- bus->ext_ops = ext_ops;
- /*
* There is only one HDA bus atm. keep the index as 0.* Need to fix when there are more than one HDA bus.*/- bus->idx = 0;
- spin_lock_init(&bus->reg_lock);
This is the smallest set of initialization needed when you don't need hdmi/hdaudio codec support.
I don't understand it... Why SOF core needs to initialize the content of HD-audio bus object even if you won't use it?
IOW, what's the merit of having hda-bus.c with the copy of snd-hda-core code? As far as I see, both hda.c and hda-bus.c are linked into the same snd-sof-intel-hda-common module. And, the former has the direct calls of HD-audio core API (with CONFIG_SND_SOC_SOF_HDA); i.e. snd-sof-intel-hda-common already depends on snd-hda-core if CONFIG_SND_SOC_SOF_HDA is on, no matter how you code hda-bus.c.
thanks,
Takashi