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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
