-----Original Message----- From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] Sent: Tuesday, February 27, 2018 12:49 AM To: Takashi Iwai tiwai@suse.de; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Cc: alsa-devel@alsa-project.org; Koul, Vinod vinod.koul@intel.com; liam.r.girdwood@linux.intel.com; Patches Audio patches.audio@intel.com; broonie@kernel.org Subject: Re: [alsa-devel] [PATCH v1 8/9] ASoC: hdac_hda: add asoc extension for legacy HDA codec drivers
On 2/26/18 3:25 AM, Takashi Iwai wrote:
On Mon, 26 Feb 2018 11:11:44 +0100, Ughreja, Rakesh A wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Friday, February 23, 2018 3:48 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: [PATCH v1 8/9] ASoC: hdac_hda: add asoc extension for legacy
HDA
codec drivers
On Fri, 23 Feb 2018 09:12:29 +0100, Rakesh Ughreja wrote:
+static int hdac_hda_codec_probe(struct snd_soc_codec *codec) +{
- struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
- struct snd_soc_dapm_context *dapm =
snd_soc_component_get_dapm(&codec-component);
- struct hdac_device *hdev = &hda_pvt->codec.core;
- struct hda_codec *hcodec = &hda_pvt->codec;
- struct hdac_ext_link *hlink = NULL;
- hda_codec_patch_t patch;
- int ret, i = 0;
- u16 codec_mask;
- hda_pvt->scodec = codec;
- hlink = snd_hdac_ext_bus_get_link(hdev->bus, dev_name(&hdev->dev));
- if (!hlink) {
dev_err(&hdev->dev, "hdac link not found\n");return -EIO;- }
- ret = snd_hdac_ext_bus_link_get(hdev->bus, hlink);
So this is the essential part for the ext-hda stuff. But...
- udelay(1000);
- do {
codec_mask = snd_hdac_chip_readw(hdev->bus, STATESTS);if (codec_mask)break;i++;udelay(100);- } while (i < 100);
- if (!codec_mask) {
dev_err(&hdev->dev, "No codecs found after link reset\n");return -EIO;- }
- snd_hdac_chip_writew(hdev->bus, STATESTS, STATESTS_INT_MASK);
Why do you need this? The callback gets called by the HD-audio controller driver and it already should have checked the codec mask bits.
With the multiple link support on the same HDA controller, when the link gets turned ON immediately codec may not indicate its presence. As per the HDA spec, it may take up to 521uSec before the codec responds.
In the legacy HDA the link was turned ON/OFF only during CRST time, but now it can happen anytime.
This must be documented. Otherwise no one understands it.
And I still don't think such stuff belonging to the codec driver. It's rather a job of the controller driver to assure the codec access. If any, we should fix that side instead.
I missed what the 'multiple-link' notion means or which part of the spec this is referring to? The code here assumes one analog codec and one iDisp, there isn't any support for additional codecs. What am I missing?
HDA Spec section 4.3 - Codec discovery. In SKL onwards there are two links and both are individually powered ON/OFF.
Regards, Rakesh