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

[Keyon Jie]

On 2019/6/1 上午5:20, Takashi Iwai wrote:
> 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?

Hi Takashi, yes, that's true. We want to reuse the hdac registers access 
part(bus->io_ops, bus->stream_list), for host DMA stream management(e.g. 
in sof/intel/hda-stream.c), without the dependency on snd-hda-core.

> 
> 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.

If we can split HD-audio core stuff to provide a host DMA/stream only 
module for I2S/DMIC, that would be great.

Thanks,
~Keyon

> 
>> 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
>