[v1,9/9] ASoC: Intel: Boards: add support for HDA codecs
diff mbox

Message ID 1519373550-2545-10-git-send-email-rakesh.a.ughreja@intel.com
State New
Headers show

Commit Message

Ughreja, Rakesh A Feb. 23, 2018, 8:12 a.m. UTC
Add support for HDA codecs. add required widgets, controls, routes
and dai links for the same.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/intel/Kconfig                      |  1 +
 sound/soc/intel/boards/skl_hda_dsp_common.c  | 24 +++++++++++++++++++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h  |  2 +-
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 29 ++++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl.c                | 27 ++++++++++++++++++++++----
 5 files changed, 78 insertions(+), 5 deletions(-)

Comments

Pierre-Louis Bossart Feb. 23, 2018, 5:04 p.m. UTC | #1
On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
> Add support for HDA codecs. add required widgets, controls, routes
> and dai links for the same.
> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>   sound/soc/intel/Kconfig                      |  1 +
>   sound/soc/intel/boards/skl_hda_dsp_common.c  | 24 +++++++++++++++++++++++
>   sound/soc/intel/boards/skl_hda_dsp_common.h  |  2 +-
>   sound/soc/intel/boards/skl_hda_dsp_generic.c | 29 ++++++++++++++++++++++++++++
>   sound/soc/intel/skylake/skl.c                | 27 ++++++++++++++++++++++----
>   5 files changed, 78 insertions(+), 5 deletions(-)
> 
> 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

>   	select SND_SOC_ACPI_INTEL_MATCH
>   	help
>   	  If you have a Intel Skylake/Broxton/ApolloLake/KabyLake/
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
> index 7066bed..f5fa475 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_common.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
> @@ -73,6 +73,30 @@ struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>   		.dpcm_playback = 1,
>   		.no_pcm = 1,
>   	},
> +	{
> +		.name = "Analog Playback and Capture",
> +		.id = 4,
> +		.cpu_dai_name = "Analog CPU DAI",
> +		.codec_name = "ehdaudio0D0",
> +		.codec_dai_name = "Analog Codec DAI",
> +		.platform_name = "0000:00:1f.3",
> +		.dpcm_playback = 1,
> +		.dpcm_capture = 1,
> +		.init = NULL,
> +		.no_pcm = 1,
> +	},
> +	{
> +		.name = "Digital Playback and Capture",
> +		.id = 5,
> +		.cpu_dai_name = "Digital CPU DAI",
> +		.codec_name = "ehdaudio0D0",
> +		.codec_dai_name = "Digital Codec DAI",
> +		.platform_name = "0000:00:1f.3",
> +		.dpcm_playback = 1,
> +		.dpcm_capture = 1,
> +		.init = NULL,
> +		.no_pcm = 1,
> +	},
>   };
>   
>   int skl_hda_hdmi_jack_init(struct snd_soc_card *card)
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h b/sound/soc/intel/boards/skl_hda_dsp_common.h
> index adbf552..a9bc92b 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_common.h
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
> @@ -13,7 +13,7 @@
>   #include <sound/core.h>
>   #include <sound/jack.h>
>   
> -#define HDA_DSP_MAX_BE_DAI_LINKS 3
> +#define HDA_DSP_MAX_BE_DAI_LINKS 5
>   
>   struct skl_hda_hdmi_pcm {
>   	struct list_head head;
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> index 9e925ba..009683d 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> @@ -18,8 +18,33 @@
>   
>   static const char *platform_name = "0000:00:1f.3";
>   
> +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?

> +
>   static const struct snd_soc_dapm_route skl_hda_map[] = {
>   
> +	/* HP jack connectors - unknown if we have jack detection */
> +	{ "Headphone", NULL, "Codec Output Pin1" },
> +	{ "Codec Speaker", NULL, "Codec Output Pin2" },
> +	{ "Codec Input Pin2", NULL, "Codec Mic" },
> +	{ "Codec Input Pin1", NULL, "Headset Mic" },
> +
> +	/* CODEC BE connections */
> +	{ "Analog Codec Playback", NULL, "Analog CPU Playback" },
> +	{ "Analog CPU Playback", NULL, "codec0_out" },
> +
> +	{ "codec0_in", NULL, "Analog CPU Capture" },
> +	{ "Analog CPU Capture", NULL, "Analog Codec Capture" },
> +
>   	{ "hifi3", NULL, "iDisp3 Tx"},
>   	{ "iDisp3 Tx", NULL, "iDisp3_out"},
>   	{ "hifi2", NULL, "iDisp2 Tx"},
> @@ -56,6 +81,10 @@ static struct snd_soc_card hda_soc_card = {
>   	.owner = THIS_MODULE,
>   	.dai_link = skl_hda_be_dai_links,
>   	.num_links = ARRAY_SIZE(skl_hda_be_dai_links),
> +	.controls = skl_hda_controls,
> +	.num_controls = ARRAY_SIZE(skl_hda_controls),
> +	.dapm_widgets = skl_hda_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(skl_hda_widgets),
>   	.dapm_routes = skl_hda_map,
>   	.num_dapm_routes = ARRAY_SIZE(skl_hda_map),
>   	.add_dai_link = skl_hda_add_dai_link,
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 1a5ac1b..d6a008b 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -36,6 +36,7 @@
>   #include "skl-sst-dsp.h"
>   #include "skl-sst-ipc.h"
>   #include "../../../pci/hda/hda_codec.h"
> +#include "../../../soc/codecs/hdac_hda.h"
>   
>   static struct skl_machine_pdata skl_dmic_data;
>   
> @@ -632,7 +633,9 @@ static int probe_codec(struct hdac_bus *bus, int addr)
>   		(AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
>   	unsigned int res = -1;
>   	struct skl *skl = bus_to_skl(bus);
> +	struct hdac_hda_priv *hda_codec;
>   	struct hdac_device *hdev;
> +	int err;
>   
>   	mutex_lock(&bus->cmd_mutex);
>   	snd_hdac_bus_send_cmd(bus, cmd);
> @@ -642,11 +645,22 @@ static int probe_codec(struct hdac_bus *bus, int addr)
>   		return -EIO;
>   	dev_dbg(bus->dev, "codec #%d probed OK: %x\n", addr, res);
>   
> -	hdev = devm_kzalloc(&skl->pci->dev, sizeof(*hdev), GFP_KERNEL);
> -	if (!hdev)
> +	hda_codec = devm_kzalloc(&skl->pci->dev, sizeof(*hda_codec),
> +								GFP_KERNEL);
> +	if (!hda_codec)
>   		return -ENOMEM;
>   
> -	return snd_hdac_ext_bus_device_init(bus, addr, hdev);
> +	hda_codec->codec.bus = skl_to_hbus(skl);
> +	hdev = &hda_codec->codec.core;
> +
> +	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?

> +
> +	return 0;
>   }
>   
>   /* Codec initialization */
> @@ -784,6 +798,7 @@ static int skl_create(struct pci_dev *pci,
>   	struct skl *skl;
>   	struct hdac_bus *bus;
>   	struct hda_bus *hbus;
> +	struct hdac_ext_bus_ops *ext_ops = NULL;
>   
>   	int err;
>   
> @@ -801,7 +816,11 @@ static int skl_create(struct pci_dev *pci,
>   
>   	hbus = skl_to_hbus(skl);
>   	bus = skl_to_bus(skl);
> -	snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops, NULL);
> +
> +#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
> +	ext_ops = snd_soc_hdac_hda_get_ops();
> +#endif
> +	snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops, ext_ops);
>   	bus->use_posbuf = 1;
>   	skl->pci = pci;
>   	INIT_WORK(&skl->probe_work, skl_probe_work);
>
Ughreja, Rakesh A Feb. 26, 2018, 4:01 p.m. UTC | #2
>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Friday, February 23, 2018 10:34 PM
>To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa-
>project.org; broonie@kernel.org; tiwai@suse.de;
>liam.r.girdwood@linux.intel.com
>Cc: Koul, Vinod <vinod.koul@intel.com>; Patches Audio
><patches.audio@intel.com>
>Subject: Re: [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA codecs
>
>On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>> Add support for HDA codecs. add required widgets, controls, routes
>> and dai links for the same.
>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>> ---
>>   sound/soc/intel/Kconfig                      |  1 +
>>   sound/soc/intel/boards/skl_hda_dsp_common.c  | 24
>+++++++++++++++++++++++
>>   sound/soc/intel/boards/skl_hda_dsp_common.h  |  2 +-
>>   sound/soc/intel/boards/skl_hda_dsp_generic.c | 29
>++++++++++++++++++++++++++++
>>   sound/soc/intel/skylake/skl.c                | 27 ++++++++++++++++++++++----
>>   5 files changed, 78 insertions(+), 5 deletions(-)
>>
>> 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.

>
>>   	select SND_SOC_ACPI_INTEL_MATCH
>>   	help
>>   	  If you have a Intel Skylake/Broxton/ApolloLake/KabyLake/
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c
>b/sound/soc/intel/boards/skl_hda_dsp_common.c
>> index 7066bed..f5fa475 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_common.c
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
>> @@ -73,6 +73,30 @@ struct snd_soc_dai_link
>skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>>   		.dpcm_playback = 1,
>>   		.no_pcm = 1,
>>   	},
>> +	{
>> +		.name = "Analog Playback and Capture",
>> +		.id = 4,
>> +		.cpu_dai_name = "Analog CPU DAI",
>> +		.codec_name = "ehdaudio0D0",
>> +		.codec_dai_name = "Analog Codec DAI",
>> +		.platform_name = "0000:00:1f.3",
>> +		.dpcm_playback = 1,
>> +		.dpcm_capture = 1,
>> +		.init = NULL,
>> +		.no_pcm = 1,
>> +	},
>> +	{
>> +		.name = "Digital Playback and Capture",
>> +		.id = 5,
>> +		.cpu_dai_name = "Digital CPU DAI",
>> +		.codec_name = "ehdaudio0D0",
>> +		.codec_dai_name = "Digital Codec DAI",
>> +		.platform_name = "0000:00:1f.3",
>> +		.dpcm_playback = 1,
>> +		.dpcm_capture = 1,
>> +		.init = NULL,
>> +		.no_pcm = 1,
>> +	},
>>   };
>>
>>   int skl_hda_hdmi_jack_init(struct snd_soc_card *card)
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h
>b/sound/soc/intel/boards/skl_hda_dsp_common.h
>> index adbf552..a9bc92b 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_common.h
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
>> @@ -13,7 +13,7 @@
>>   #include <sound/core.h>
>>   #include <sound/jack.h>
>>
>> -#define HDA_DSP_MAX_BE_DAI_LINKS 3
>> +#define HDA_DSP_MAX_BE_DAI_LINKS 5
>>
>>   struct skl_hda_hdmi_pcm {
>>   	struct list_head head;
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c
>b/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> index 9e925ba..009683d 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> @@ -18,8 +18,33 @@
>>
>>   static const char *platform_name = "0000:00:1f.3";
>>
>> +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

>
>> +
>>   static const struct snd_soc_dapm_route skl_hda_map[] = {
>>
>> +	/* HP jack connectors - unknown if we have jack detection */
>> +	{ "Headphone", NULL, "Codec Output Pin1" },
>> +	{ "Codec Speaker", NULL, "Codec Output Pin2" },
>> +	{ "Codec Input Pin2", NULL, "Codec Mic" },
>> +	{ "Codec Input Pin1", NULL, "Headset Mic" },
>> +
>> +	/* CODEC BE connections */
>> +	{ "Analog Codec Playback", NULL, "Analog CPU Playback" },
>> +	{ "Analog CPU Playback", NULL, "codec0_out" },
>> +
>> +	{ "codec0_in", NULL, "Analog CPU Capture" },
>> +	{ "Analog CPU Capture", NULL, "Analog Codec Capture" },
>> +
>>   	{ "hifi3", NULL, "iDisp3 Tx"},
>>   	{ "iDisp3 Tx", NULL, "iDisp3_out"},
>>   	{ "hifi2", NULL, "iDisp2 Tx"},
>> @@ -56,6 +81,10 @@ static struct snd_soc_card hda_soc_card = {
>>   	.owner = THIS_MODULE,
>>   	.dai_link = skl_hda_be_dai_links,
>>   	.num_links = ARRAY_SIZE(skl_hda_be_dai_links),
>> +	.controls = skl_hda_controls,
>> +	.num_controls = ARRAY_SIZE(skl_hda_controls),
>> +	.dapm_widgets = skl_hda_widgets,
>> +	.num_dapm_widgets = ARRAY_SIZE(skl_hda_widgets),
>>   	.dapm_routes = skl_hda_map,
>>   	.num_dapm_routes = ARRAY_SIZE(skl_hda_map),
>>   	.add_dai_link = skl_hda_add_dai_link,
>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>> index 1a5ac1b..d6a008b 100644
>> --- a/sound/soc/intel/skylake/skl.c
>> +++ b/sound/soc/intel/skylake/skl.c
>> @@ -36,6 +36,7 @@
>>   #include "skl-sst-dsp.h"
>>   #include "skl-sst-ipc.h"
>>   #include "../../../pci/hda/hda_codec.h"
>> +#include "../../../soc/codecs/hdac_hda.h"
>>
>>   static struct skl_machine_pdata skl_dmic_data;
>>
>> @@ -632,7 +633,9 @@ static int probe_codec(struct hdac_bus *bus, int addr)
>>   		(AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
>>   	unsigned int res = -1;
>>   	struct skl *skl = bus_to_skl(bus);
>> +	struct hdac_hda_priv *hda_codec;
>>   	struct hdac_device *hdev;
>> +	int err;
>>
>>   	mutex_lock(&bus->cmd_mutex);
>>   	snd_hdac_bus_send_cmd(bus, cmd);
>> @@ -642,11 +645,22 @@ static int probe_codec(struct hdac_bus *bus, int
>addr)
>>   		return -EIO;
>>   	dev_dbg(bus->dev, "codec #%d probed OK: %x\n", addr, res);
>>
>> -	hdev = devm_kzalloc(&skl->pci->dev, sizeof(*hdev), GFP_KERNEL);
>> -	if (!hdev)
>> +	hda_codec = devm_kzalloc(&skl->pci->dev, sizeof(*hda_codec),
>> +
>	GFP_KERNEL);
>> +	if (!hda_codec)
>>   		return -ENOMEM;
>>
>> -	return snd_hdac_ext_bus_device_init(bus, addr, hdev);
>> +	hda_codec->codec.bus = skl_to_hbus(skl);
>> +	hdev = &hda_codec->codec.core;
>> +
>> +	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.

Regards,
Rakesh
Pierre-Louis Bossart Feb. 26, 2018, 7:37 p.m. UTC | #3
>>> 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.

>>> +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.


>>> +	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.
Ughreja, Rakesh A Feb. 27, 2018, 4:20 p.m. UTC | #4
>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Tuesday, February 27, 2018 1:07 AM
>To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa-
>project.org; broonie@kernel.org; tiwai@suse.de;
>liam.r.girdwood@linux.intel.com
>Cc: Koul, Vinod <vinod.koul@intel.com>; Patches Audio
><patches.audio@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
Takashi Iwai Feb. 27, 2018, 4:55 p.m. UTC | #5
On Tue, 27 Feb 2018 17:20:05 +0100,
Ughreja, Rakesh A wrote:
> 
> >> 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 ?

Yes, in a few cases, the generic parser creates another PCM for analog
I/O as "Alt Analog":

- When a DAC is available for the headphone independently from others
  ("independent HP" stream)

- When there are multiple ADCs and neither dynamic ADC switch nor
  auto-mic selection feature is used


Takashi
Ughreja, Rakesh A Feb. 27, 2018, 5:06 p.m. UTC | #6
>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Tuesday, February 27, 2018 10:25 PM
>To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
>Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; alsa-devel@alsa-
>project.org; broonie@kernel.org; liam.r.girdwood@linux.intel.com; Koul, Vinod
><vinod.koul@intel.com>; Patches Audio <patches.audio@intel.com>
>Subject: Re: [alsa-devel] [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA
>codecs
>
>On Tue, 27 Feb 2018 17:20:05 +0100,
>Ughreja, Rakesh A wrote:
>>
>> >> 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 ?
>
>Yes, in a few cases, the generic parser creates another PCM for analog
>I/O as "Alt Analog":

But this again is static and its named as "Alt Analog". 
So, in the current code if I just create one more as "Alt Analog" DAI
it should be fine ?

I do map them by searching the pcm_list_head when the application
opens them.

Will I see more than one "Analog" and "Alt Analog" ever in the
pcm_list_head ? or at max I am going to see one "Analog" and an
additional "Alt Analog" PCM.

Regards,
Rakesh
Takashi Iwai Feb. 27, 2018, 5:16 p.m. UTC | #7
On Tue, 27 Feb 2018 18:06:50 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Tuesday, February 27, 2018 10:25 PM
> >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> >Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; alsa-devel@alsa-
> >project.org; broonie@kernel.org; liam.r.girdwood@linux.intel.com; Koul, Vinod
> ><vinod.koul@intel.com>; Patches Audio <patches.audio@intel.com>
> >Subject: Re: [alsa-devel] [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA
> >codecs
> >
> >On Tue, 27 Feb 2018 17:20:05 +0100,
> >Ughreja, Rakesh A wrote:
> >>
> >> >> 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 ?
> >
> >Yes, in a few cases, the generic parser creates another PCM for analog
> >I/O as "Alt Analog":
> 
> But this again is static and its named as "Alt Analog". 
> So, in the current code if I just create one more as "Alt Analog" DAI
> it should be fine ?

For the generic parser, it should be fine to up to three PCMs, yes.

> I do map them by searching the pcm_list_head when the application
> opens them.
> 
> Will I see more than one "Analog" and "Alt Analog" ever in the
> pcm_list_head ? or at max I am going to see one "Analog" and an
> additional "Alt Analog" PCM.

At least, not done by the generic parser.  If the codec has own parser
and handles differently, it's a different story.  For example, the
HDMI has quite different mechanisms, and it can give far more streams
for DP MST.  But you're not dealing with these types for now, so it
should be OK for Intel machines.


Takashi

Patch
diff mbox

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
 	select SND_SOC_ACPI_INTEL_MATCH
 	help
 	  If you have a Intel Skylake/Broxton/ApolloLake/KabyLake/
diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
index 7066bed..f5fa475 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
@@ -73,6 +73,30 @@  struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
 		.dpcm_playback = 1,
 		.no_pcm = 1,
 	},
+	{
+		.name = "Analog Playback and Capture",
+		.id = 4,
+		.cpu_dai_name = "Analog CPU DAI",
+		.codec_name = "ehdaudio0D0",
+		.codec_dai_name = "Analog Codec DAI",
+		.platform_name = "0000:00:1f.3",
+		.dpcm_playback = 1,
+		.dpcm_capture = 1,
+		.init = NULL,
+		.no_pcm = 1,
+	},
+	{
+		.name = "Digital Playback and Capture",
+		.id = 5,
+		.cpu_dai_name = "Digital CPU DAI",
+		.codec_name = "ehdaudio0D0",
+		.codec_dai_name = "Digital Codec DAI",
+		.platform_name = "0000:00:1f.3",
+		.dpcm_playback = 1,
+		.dpcm_capture = 1,
+		.init = NULL,
+		.no_pcm = 1,
+	},
 };
 
 int skl_hda_hdmi_jack_init(struct snd_soc_card *card)
diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h b/sound/soc/intel/boards/skl_hda_dsp_common.h
index adbf552..a9bc92b 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.h
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
@@ -13,7 +13,7 @@ 
 #include <sound/core.h>
 #include <sound/jack.h>
 
-#define HDA_DSP_MAX_BE_DAI_LINKS 3
+#define HDA_DSP_MAX_BE_DAI_LINKS 5
 
 struct skl_hda_hdmi_pcm {
 	struct list_head head;
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
index 9e925ba..009683d 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -18,8 +18,33 @@ 
 
 static const char *platform_name = "0000:00:1f.3";
 
+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),
+};
+
 static const struct snd_soc_dapm_route skl_hda_map[] = {
 
+	/* HP jack connectors - unknown if we have jack detection */
+	{ "Headphone", NULL, "Codec Output Pin1" },
+	{ "Codec Speaker", NULL, "Codec Output Pin2" },
+	{ "Codec Input Pin2", NULL, "Codec Mic" },
+	{ "Codec Input Pin1", NULL, "Headset Mic" },
+
+	/* CODEC BE connections */
+	{ "Analog Codec Playback", NULL, "Analog CPU Playback" },
+	{ "Analog CPU Playback", NULL, "codec0_out" },
+
+	{ "codec0_in", NULL, "Analog CPU Capture" },
+	{ "Analog CPU Capture", NULL, "Analog Codec Capture" },
+
 	{ "hifi3", NULL, "iDisp3 Tx"},
 	{ "iDisp3 Tx", NULL, "iDisp3_out"},
 	{ "hifi2", NULL, "iDisp2 Tx"},
@@ -56,6 +81,10 @@  static struct snd_soc_card hda_soc_card = {
 	.owner = THIS_MODULE,
 	.dai_link = skl_hda_be_dai_links,
 	.num_links = ARRAY_SIZE(skl_hda_be_dai_links),
+	.controls = skl_hda_controls,
+	.num_controls = ARRAY_SIZE(skl_hda_controls),
+	.dapm_widgets = skl_hda_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(skl_hda_widgets),
 	.dapm_routes = skl_hda_map,
 	.num_dapm_routes = ARRAY_SIZE(skl_hda_map),
 	.add_dai_link = skl_hda_add_dai_link,
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 1a5ac1b..d6a008b 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -36,6 +36,7 @@ 
 #include "skl-sst-dsp.h"
 #include "skl-sst-ipc.h"
 #include "../../../pci/hda/hda_codec.h"
+#include "../../../soc/codecs/hdac_hda.h"
 
 static struct skl_machine_pdata skl_dmic_data;
 
@@ -632,7 +633,9 @@  static int probe_codec(struct hdac_bus *bus, int addr)
 		(AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
 	unsigned int res = -1;
 	struct skl *skl = bus_to_skl(bus);
+	struct hdac_hda_priv *hda_codec;
 	struct hdac_device *hdev;
+	int err;
 
 	mutex_lock(&bus->cmd_mutex);
 	snd_hdac_bus_send_cmd(bus, cmd);
@@ -642,11 +645,22 @@  static int probe_codec(struct hdac_bus *bus, int addr)
 		return -EIO;
 	dev_dbg(bus->dev, "codec #%d probed OK: %x\n", addr, res);
 
-	hdev = devm_kzalloc(&skl->pci->dev, sizeof(*hdev), GFP_KERNEL);
-	if (!hdev)
+	hda_codec = devm_kzalloc(&skl->pci->dev, sizeof(*hda_codec),
+								GFP_KERNEL);
+	if (!hda_codec)
 		return -ENOMEM;
 
-	return snd_hdac_ext_bus_device_init(bus, addr, hdev);
+	hda_codec->codec.bus = skl_to_hbus(skl);
+	hdev = &hda_codec->codec.core;
+
+	err = snd_hdac_ext_bus_device_init(bus, addr, hdev);
+	if (err < 0)
+		return err;
+
+	if ((res & 0xFFFF0000) != 0x80860000)
+		hdev->type = HDA_DEV_LEGACY;
+
+	return 0;
 }
 
 /* Codec initialization */
@@ -784,6 +798,7 @@  static int skl_create(struct pci_dev *pci,
 	struct skl *skl;
 	struct hdac_bus *bus;
 	struct hda_bus *hbus;
+	struct hdac_ext_bus_ops *ext_ops = NULL;
 
 	int err;
 
@@ -801,7 +816,11 @@  static int skl_create(struct pci_dev *pci,
 
 	hbus = skl_to_hbus(skl);
 	bus = skl_to_bus(skl);
-	snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops, NULL);
+
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
+	ext_ops = snd_soc_hdac_hda_get_ops();
+#endif
+	snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops, ext_ops);
 	bus->use_posbuf = 1;
 	skl->pci = pci;
 	INIT_WORK(&skl->probe_work, skl_probe_work);