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

[Takashi Iwai]

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