On Mon, 11 Dec 2017 19:20:41 +0100, Ughreja, Rakesh A wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, December 11, 2017 8:51 PM To: Ughreja, Rakesh A rakesh.a.ughreja@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org; liam.r.girdwood@linux.intel.com; pierre-louis.bossart@linux.intel.com; Koul, Vinod vinod.koul@intel.com; Patches Audio patches.audio@intel.com Subject: Re: [RFC v2 00/11] Enable HDA Codec support on Intel Platforms (Series2)
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.
So when you say "after all the components get ready" what exactly you are referring to ?
I am calling snd_hda_asoc_codec_new function from probe function of hdac_driver. So I was thinking that since the codec is already enumerated from the bus point of view, I can register. What am I missing here ?
The registration should be triggered from the top-level, i.e. the controller driver, once after all codecs get probed. Otherwise, it becomes racy. The user-space will start poking the device before all the stuff becomes ready.
That said, at the point when the codec is probed, the card shouldn't be registered yet. The card gets registered after the all components are bound and become ready -- which is done a single snd_card_register() call. At this point, the access points to user-space are provided.
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.
The generic hdac_hda driver uses the legacy driver APIs once the hdac_dev is enumerated on the hdac_bus. While the legacy driver APIs are used by controller driver.
Do you think it's a wrong path to go ? Should the legacy driver APIs be called directly from ASoC platform driver ?
The ideal situation would be that there is no difference in the codec driver level which controller driver to bind. It means we'd need some stuff from the legacy helper merged into the hda-core, and I'm not sure how difficult it would be.
Takashi