Re: Why open-coding in sof_hda_bus_init()?

[Takashi Iwai <tiwai@suse.de>]

On Fri, 31 May 2019 22:59:17 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >> 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?
> 
> we do use it left and right, but we only use the 'controller/DMA'
> parts of that structure. we have zero use for CORB/RIRB and
> codec-specific stuff when I2S and DMIC are the only connections to 3rd
> party chips

So you want to avoid the dependency on snd-hda-core if the system is
with only I2S/DMIC?

But, snd-sof-intel-hda-common already has the hard dependency on
snd-hda-core if CONFIG_SND_SOC_SOF_HDA is enabled.  So the attempt
makes little sense as long as CONFIG_SND_SOC_SOF_HDA is set.

And, if CONFIG_SND_SOC_SOF_HDA isn't set -- i.e.
CONFIG_SND_SOC_HF_HDA_LINK isn't set either; it implies that there can
be no user of HD-audio bus.  So, I see no big reason we need to care
about the stuff...


> > 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.
> 
> I agree we could implement hda-bus in a cleaner way - but it's a very
> small file.

Yes, but this is a kind of layer violation.  Imagine you do initialize
the struct pci_dev or whatever else device object because you want to
reduce the dependency on other core helpers; it would be nightmare
from the maintenance POV.

I've had already hard time to figure out why SOF HDA initializes the
HD-audio bus, because it misses the explicit snd_hdac_*_bus_init()
call.  That's the starting point of this thread.

So, let's avoid ugly hacks.  Make it more straightforward.  Again, if
the module size matters, we can split and reduce the part of HD-audio
core stuff that is directly linked to SOF core.

> A larger core repartitioning would take quite a bit of
> time, and in the mean time we already have to sort out all the deltas
> between legacy driver and hdac library.
> 
> Anyways, that's it for me this week, enjoy your vacation!

Thanks!


Takashi