Re: [RFC v2 00/11] Enable HDA Codec support on Intel Platforms (Series2)

From: Takashi Iwai <tiwai@suse.de>
To: "Ughreja, Rakesh A" <rakesh.a.ughreja@intel.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"Koul, Vinod" <vinod.koul@intel.com>,
	"pierre-louis.bossart@linux.intel.com"
	<pierre-louis.bossart@linux.intel.com>,
	"liam.r.girdwood@linux.intel.com"
	<liam.r.girdwood@linux.intel.com>,
	Patches Audio <patches.audio@intel.com>,
	"broonie@kernel.org" <broonie@kernel.org>
Subject: Re: [RFC v2 00/11] Enable HDA Codec support on Intel Platforms (Series2)
Date: Mon, 11 Dec 2017 16:21:14 +0100	[thread overview]
Message-ID: <s5hinddl28l.wl-tiwai@suse.de> (raw)
In-Reply-To: <85DFEED57DC57344B2483EF7BF8CB60579AD2978@BGSMSX104.gar.corp.intel.com>

On Mon, 11 Dec 2017 16:10:32 +0100,
Ughreja, Rakesh A wrote:
> 
> >good in one side.  OTOH, it is unacceptably messy, in special, because
> >the device creation and registration phases are mixed up.
> 
> I am not sure if I understand this fully. "Device creation and registration
> phases are mixed up". If you can explain bit more, I can try to improve
> it.

e.g. in the patch 7, snd_hda_asoc_codec_new() has the code like:

+int snd_hda_asoc_codec_new(struct hda_bus *bus, struct snd_card *card,
+			unsigned int codec_addr, struct hda_codec **codecp)
+{
+	struct hda_codec *codec = NULL;
....
+	err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);

which means that the device is being created in this function.
At the same time, however, this function tries to register that codec
device itself soon after the above:

+	list_for_each_entry(snd_dev, &card->devices, list) {
+		if (snd_dev->type == SNDRV_DEV_CODEC) {
+			void *device_data = snd_dev->device_data;
+
+			err = snd_device_register(card, device_data);
+			if (err < 0)

This is wrong.  The registration must happen at a later stage once
after all components get ready.  After all, that's the reason we have
the dev_register individual callback in snd_dev_ops.

The exception is a case with multiple probe-bindings like USB-audio
that matches multiple times per interface.  But it's an exception, and
not applied to the normal probe.  (And if any, you should just
register the whole via snd_card_register() call.)

> >And exporting each codec table doesn't look nice.  We need to find a
> >better way...
> 
> Do you think accessing the codec table via some EXPORT function
> is a better way ?

We need more consideration.  I don't think we have done it fully
enough.  At least, exporting each table sounds like a bad idea.
Ideally, we should re-use the whole codec driver as is; i.e. the
single codec probe should work for both legacy and ext controllers.

Takashi