[bardliao]

snd_hda_codec_device_new() is used by both legacy HDA and ASoC driver. However, we will call snd_hdac_device_unregister() in snd_hdac_ext_bus_device_remove() for ASoC device. The first patch uses
the type flag in hdac_device struct to determine is it a ASoC device or legacy HDA device and call snd_hdac_device_unregister() in snd_hda_codec_dev_free() only if it is a legacy HDA device.
And the second patch overwrite the hdev->type flag to identify that is a ASoC device.

fix #837

[ranj063]

@bardliao Im a bit confused by this piece of code in the SOF driver in hda-codec.c
/* use legacy bus only for HDA codecs, idisp uses ext bus */
if ((resp & 0xFFFF0000) != IDISP_VID_INTEL) {
hdev->type = HDA_DEV_LEGACY;
hda_codec_load_module(&hda_priv->codec);
}

Why do we set it to HDA_DEV_LEGACY here explicitly only to be overwritten later?

[bardliao]

@bardliao Im a bit confused by this piece of code in the SOF driver in hda-codec.c
/* use legacy bus only for HDA codecs, idisp uses ext bus */
if ((resp & 0xFFFF0000) != IDISP_VID_INTEL) {
hdev->type = HDA_DEV_LEGACY;
hda_codec_load_module(&hda_priv->codec);
}

Why do we set it to HDA_DEV_LEGACY here explicitly only to be overwritten later?

@ranj063 Good question. That was also my question when I made the PR. The reason of the piece of code is that we set .api_version = HDA_DEV_LEGACY for hda codecs and .api_version = HDA_DEV_ASOC for hdmi only. So we must set hdev->type = HDA_DEV_LEGACY to load driver for hda codecs. And the type flag will never be used after that. That's why it can be overwrite and used for legacy HDA/ASoC mode checking.

see also snd_hda_id_realtek[] and hdmi_list[] for reference

[RanderWang]

Sorry, I also have a question: sof invokes snd_hdac_ext_bus_device_remove to remove all the codec devices, but codec devices are not directly created by sof and it is created by hdac_hda driver. So why sof needs to release these codecs? let hdac_hda do it.

[bardliao]

@RanderWang In fact, sof will call snd_hdac_ext_bus_device_init() to invoke snd_hdac_device_register(). So I think it is even to call snd_hdac_device_unregister() in snd_hdac_ext_bus_device_remove()

[RanderWang]

In legacy hda, snd_hdac_device_register() is invoked by hda codec probe, and snd_hdac_device_unregister() is invoked by hda codec remove. Now in sof, snd_hdac_ext_bus_device_init is used to probe the hda codec, so it is correct to invoke snd_hdac_device_register() in snd_hdac_ext_bus_device_init. But now we keep the snd_hdac_device_unregister in hda codec remove and add it in another function snd_hdac_ext_bus_device_remove. My idea is deleting snd_hdac_ext_bus_device_remove in sof.

[bardliao]

@RanderWang

snd_hdac_device_unregister() is invoked by hda codec remove

No, actually, it is done when hda codec is unbind.

My idea is deleting snd_hdac_ext_bus_device_remove in sof.

That is the other direction. But note that we don't have a similar unregister function on hdac_hdmi codec driver. i.e. Currently, snd_hdac_ext_bus_device_remove is required for hdac_hdmi codec driver. Also, I think it is even to call hda_codec_probe() -> snd_hdac_ext_bus_device_init() -> snd_hdac_device_register() and hda_dsp_remove() -> snd_hdac_ext_bus_device_remove() -> snd_hdac_device_unregister() in sof. I don't see a reason to call snd_hdac_device_register() in sof but snd_hdac_device_unregister() in another place.

[RanderWang]

@RanderWang

snd_hdac_device_unregister() is invoked by hda codec remove

No, actually, it is done when hda codec is unbind.

My idea is deleting snd_hdac_ext_bus_device_remove in sof.

That is the other direction. But note that we don't have a similar unregister function on hdac_hdmi codec driver. i.e. Currently, snd_hdac_ext_bus_device_remove is required for hdac_hdmi codec driver.

ok, I get it. The flow is complex for me to make it clear in short time.

[plbossart]

I am not hot on this. The flow is very complicated and I wonder if we can do a better job.

[plbossart]

it's really a bit ugly to do this. You first use HDA_DEV_LEGACY to load the codec, and have to use HDA_DEV_ASOC to prevent it being released twice.
Is this the best we can do?

[ranj063]

@bardliao I think my suggestion #837 (comment) might be a better solution. CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC is only enabled when there are HDA codecs, so we dont need to call snd_hdac_ext_bus_device_remove in SOF.

In the case of HDMI only as in APL, this will get called because we dont enable this right?

[bardliao]

@ranj063 I tried it, but the result is not good. rmmod can't finish if I don't call snd_hdac_ext_bus_device_remove in SOF or snd_hdac_device_unregister in snd_hdac_ext_bus_device_remove. So we need to do more if we decide to go on this direction. Actually, I don't know if it is the right direction. My question is why we have to call snd_hdac_device_register in SOF, but snd_hdac_device_unregister in somewhere else?

@plbossart Yes, I agree that the code is ugly :). However, the problem is that we set hdev->type = HDA_DEV_ASOC; in snd_hdac_ext_bus_device_init at first , but overwrite it to hdev->type = HDA_DEV_LEGACY latter. So, in my opinion, set hdev->type = HDA_DEV_ASOC in hda_hdac.c is to correct hdev->type to as what it should be.

[plbossart]

@bardliao I'll have to look at this next week when I have a bigger screen and can look at the code in details. Thanks for your patience.

[wenqingfu]

this can go to alsa-dev for more brains to look into @bardliao

[bardliao]

@wenqingfu Sure, I will do it.

[plbossart]

this patch was merged upstream.

[bardliao]

@plbossart Seems I am a little bit too late. I just cherry-pick -x from Takashi's git and force update the PR after you merged it :)