[alsa-devel] [PATCH v1 8/9] ASoC: hdac_hda: add asoc extension for legacy HDA codec drivers

[Pierre-Louis Bossart]

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 at suse.de]
>>> Sent: Friday, February 23, 2018 3:48 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: [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?