[alsa-devel] [RFC v2 00/11] Enable HDA Codec support on Intel Platforms (Series2)

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Monday, December 11, 2017 8:51 PM
>To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>
>Cc: alsa-devel at alsa-project.org; broonie at kernel.org;
>liam.r.girdwood at linux.intel.com; pierre-louis.bossart at linux.intel.com; Koul, Vinod
><vinod.koul at intel.com>; Patches Audio <patches.audio at 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 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 ?