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

[Takashi Iwai]

On Mon, 11 Dec 2017 19:20:41 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----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 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