From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: Why open-coding in sof_hda_bus_init()? Date: Fri, 31 May 2019 23:20:31 +0200 Message-ID: References: <1f3059d6-b271-f612-c670-e7214674892f@linux.intel.com> <68aa2fa2-41fc-3dfb-c82f-1f88be5bd867@linux.intel.com> <13dce83f-a092-6d9f-4676-8e3fc46bdc23@linux.intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 9CF9DF80C1B for ; Fri, 31 May 2019 23:20:32 +0200 (CEST) In-Reply-To: <13dce83f-a092-6d9f-4676-8e3fc46bdc23@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: Pierre-Louis Bossart Cc: alsa-devel@alsa-project.org, Keyon Jie List-Id: alsa-devel@alsa-project.org 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