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.
Also, if the size matters: we can split hda-core code between the thin
bus accessor and the rest. Basically what you need unconditionally is
the functions in sound/hda/hdac_bus.c, and they are fairly independent
from other HD-audio functios, so it can be its own module. And the
recent ext_bus init and exit implementations are very close to the
bare init/exit code, too, so they can be simply moved into the
hdac-core-bus or provided as a static inline, too.
In anyway, my point is that there are tons better way than the open
code of such a complex object initialization.
It's actually not open-coding based on copy/paste, it took us a long
time to figure out what was strictly necessary.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
