[alsa-devel] [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA codecs

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>Sent: Tuesday, February 27, 2018 1:07 AM
>To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>; alsa-devel at alsa-
>project.org; broonie at kernel.org; tiwai at suse.de;
>liam.r.girdwood at linux.intel.com
>Cc: Koul, Vinod <vinod.koul at intel.com>; Patches Audio
><patches.audio at intel.com>
>Subject: Re: [alsa-devel] [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA
>codecs
>
>
>>>> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
>>>> index ceb105c..d44cf1e 100644
>>>> --- a/sound/soc/intel/Kconfig
>>>> +++ b/sound/soc/intel/Kconfig
>>>> @@ -107,6 +107,7 @@ config SND_SOC_INTEL_SKYLAKE
>>>>    	select SND_HDA_DSP_LOADER
>>>>    	select SND_SOC_TOPOLOGY
>>>>    	select SND_SOC_INTEL_SST
>>>> +	select SND_SOC_HDAC_HDA if
>>> SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
>>>
>>> This looks odd, it beats the purpose of the clean split between platform
>>> and machine Kconfig added in 2017. You should select SND_SOC_HDAC_HDA
>in
>>> the config for SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
>>
>> If I move into the machine driver Kconfig, then it will be loaded after
>> the SKL platform driver loads and so symbol dependency will create an issue.
>>
>> hdac_hda needs to be loaded before the SKL driver, so that it can
>> get the ops from library. So I will have to load it manually in the platform
>> driver code. Let me try that, if it does not work, I will come back on this
>> topic.
>
>I don't believe the Kconfig selection has any bearing on how the codecs
>are loaded. In fact the existing I2S-based machine drivers are a
>counter-example, the machine drivers depend on a codec driver in
>Kconfig, but the latter needs to be loaded first so that the machine
>driver finds the relevant DAIs.

Let me come back on this after trying out few things.

>
>>>> +static const struct snd_kcontrol_new skl_hda_controls[] = {
>>>> +	SOC_DAPM_PIN_SWITCH("Headphone"),
>>>> +	SOC_DAPM_PIN_SWITCH("Headset Mic"),
>>>> +};
>>>> +
>>>> +static const struct snd_soc_dapm_widget skl_hda_widgets[] = {
>>>> +	SND_SOC_DAPM_HP("Headphone", NULL),
>>>> +	SND_SOC_DAPM_MIC("Headset Mic", NULL),
>>>> +	SND_SOC_DAPM_SPK("Codec Speaker", NULL),
>>>> +	SND_SOC_DAPM_MIC("Codec Mic", NULL),
>>>> +};
>>>
>>> what about all the other outputs, e.g. line out? And how does this work
>>> with pin retasking?
>>
>> Good point.
>>
>> The hdac_hda is just a wrapper around the legacy codec driver
>> and so it relies on the functionality of the legacy HDA codec driver
>> for all the functionality including pin re-tasking.
>>
>> The widget names that you see above is just to complete the
>> DAPM route. Based on your comment I am planning to rename it as
>> following
>>
>> Analog In Endpoint
>> Analog Output Endpoint
>> Digital In Endpoint
>> Digital Out Endpoint
>>
>> and will connect it to the Codec Pins.
>>
>> Also I think it makes sense to rename the codec Pin names accordingly
>>
>> Codec Analog Input Pin
>> Codec Analog Output Pin
>> Codec Digital Input Pin
>> Codec Digital Output Pin
>
>Humm, what if you have more than one analog input? It's almost as if
>this list should be created dynamically based on what is exposed by the
>codec, I don't see how a static list will cover all configurations.

If it is really required it can be done, the codec->pcm_list_head has got
entries stored.

But I am not sure what is the behavior of the legacy HDA codec driver
when it sees more than one Analog inputs.

Takashi, will I see two Analog entries in the pcm_list_head ?

>
>
>>>> +	err = snd_hdac_ext_bus_device_init(bus, addr, hdev);
>>>> +	if (err < 0)
>>>> +		return err;
>>>> +
>>>> +	if ((res & 0xFFFF0000) != 0x80860000)
>>>> +		hdev->type = HDA_DEV_LEGACY;
>>>
>>> I don't get what this test does?
>>
>> I am using the legacy HDA codec drivers only for the HDA codecs.
>> For iDisp I will continue to use the ASoC hdac_hdmi driver, which
>> is using ASOC BUS.
>>
>> To make it more clear I will convert the if condition into inline
>> function with proper name.
>
>As written in my previous email, this is fine for now but long term we'd
>want to avoid having duplication of functionality. Fixing issues in two
>locations is not efficient, I vote to deprecate hdac_hdmi at some point.

I have no objection. But that work won't be part of this patch series.

Regards,
Rakesh