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

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>Sent: Tuesday, February 27, 2018 12:49 AM
>To: Takashi Iwai <tiwai at suse.de>; Ughreja, Rakesh A
><rakesh.a.ughreja at intel.com>
>Cc: alsa-devel at alsa-project.org; Koul, Vinod <vinod.koul at intel.com>;
>liam.r.girdwood at linux.intel.com; Patches Audio <patches.audio at intel.com>;
>broonie at 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 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?

HDA Spec section 4.3 - Codec discovery.
In SKL onwards there are two links and both are individually
powered ON/OFF.

Regards,
Rakesh