[alsa-devel] Why open-coding in sof_hda_bus_init()?

[Takashi Iwai]

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.

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.


thanks,

Takashi