[PATCH v1 0/9] Enable HDA Codec support on Intel Platforms (Series2)

[PATCH v1 0/9] Enable HDA Codec support on Intel Platforms (Series2)

[Rakesh Ughreja]

Many Intel platforms (SKL, KBL) etc. in the market supports enhanced 
audio capabilities which also includes DSP processing. This patch carry
forwards the work that is done in the previous series to enable HD Audio
codecs on such platforms.

This patch series adds ASoC HDA codec driver for Intel platforms. It is
written by reusing the legacy HDA ALSA codec driver. Intention is to
maximize the reuse and minimize the changes in the legacy HDA codec driver.

Rakesh Ughreja (9):
  ASoC: Intel: Boards: Machine driver for Intel platforms
  ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs
  ASoC: Intel: Skylake: add HDA BE DAIs
  ASoC: Intel: Skylake: use hda_bus instead of hdac_bus
  ALSA: hda - split snd_hda_codec_new function
  ALSA: hdac: remove memory allocation from snd_hdac_ext_bus_device_init
  ALSA: hdac: add extended ops in the hdac_bus
  ASoC: hdac_hda: add asoc extension for legacy HDA codec drivers
  ASoC: Intel: Boards: add support for HDA codecs

 include/sound/hdaudio.h                      |   9 +
 include/sound/hdaudio_ext.h                  |   6 +-
 sound/hda/ext/hdac_ext_bus.c                 |  12 +-
 sound/pci/hda/hda_bind.c                     |   6 +
 sound/pci/hda/hda_codec.c                    |  67 +++-
 sound/pci/hda/hda_codec.h                    |   2 +
 sound/soc/codecs/Kconfig                     |   5 +
 sound/soc/codecs/Makefile                    |   2 +
 sound/soc/codecs/hdac_hda.c                  | 448 +++++++++++++++++++++++++++
 sound/soc/codecs/hdac_hda.h                  |  23 ++
 sound/soc/intel/Kconfig                      |   1 +
 sound/soc/intel/boards/Kconfig               |  10 +
 sound/soc/intel/boards/Makefile              |   2 +
 sound/soc/intel/boards/skl_hda_dsp_common.c  | 131 ++++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h  |  34 ++
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 143 +++++++++
 sound/soc/intel/skylake/skl-pcm.c            |  50 ++-
 sound/soc/intel/skylake/skl.c                |  98 +++++-
 sound/soc/intel/skylake/skl.h                |  11 +-
 19 files changed, 1011 insertions(+), 49 deletions(-)
 create mode 100644 sound/soc/codecs/hdac_hda.c
 create mode 100644 sound/soc/codecs/hdac_hda.h
 create mode 100644 sound/soc/intel/boards/skl_hda_dsp_common.c
 create mode 100644 sound/soc/intel/boards/skl_hda_dsp_common.h
 create mode 100644 sound/soc/intel/boards/skl_hda_dsp_generic.c

-- 
2.7.4

[PATCH v1 1/9] ASoC: Intel: Boards: Machine driver for Intel platforms

[Rakesh Ughreja]

Add machine driver for Intel platforms (SKL/KBL/BXT) with HDA and iDisp
codecs. This patch adds support for only iDisp (HDMI/DP) codec. In the
following patch support for HDA codec will be added.

This should work for other Intel platforms as well e.g. GLK,CNL
however this series is not tested on all the platforms.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/intel/boards/Kconfig               |  10 +++
 sound/soc/intel/boards/Makefile              |   2 +
 sound/soc/intel/boards/skl_hda_dsp_common.c  | 107 +++++++++++++++++++++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h  |  34 ++++++++
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 114 +++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl.h                |   1 +
 6 files changed, 268 insertions(+)
 create mode 100644 sound/soc/intel/boards/skl_hda_dsp_common.c
 create mode 100644 sound/soc/intel/boards/skl_hda_dsp_common.h
 create mode 100644 sound/soc/intel/boards/skl_hda_dsp_generic.c

diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index eedc83b..3b20601 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -268,6 +268,16 @@ config SND_SOC_INTEL_KBL_DA7219_MAX98357A_MACH
 	  This adds support for ASoC Onboard Codec I2S machine driver. This will
 	  create an alsa sound card for DA7219 + MAX98357A I2S audio codec.
 	  Say Y if you have such a device.
+
+config SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
+	tristate "SKL/KBL/BXT with HDA Codecs"
+	select SND_SOC_INTEL_SST
+	depends on SND_SOC_INTEL_SKYLAKE
+	select SND_SOC_HDAC_HDMI
+	help
+	  This adds support for ASoC Onboard Codec HDA machine driver. This will
+	  create an alsa sound card for HDA Codecs.
+          Say Y or m if you have such a device. This is a recommended option.
 	  If unsure select "N".
 
 endif ## SND_SOC_INTEL_SKYLAKE
diff --git a/sound/soc/intel/boards/Makefile b/sound/soc/intel/boards/Makefile
index 6fae506..5d65481 100644
--- a/sound/soc/intel/boards/Makefile
+++ b/sound/soc/intel/boards/Makefile
@@ -18,6 +18,7 @@ snd-soc-kbl_da7219_max98357a-objs := kbl_da7219_max98357a.o
 snd-soc-kbl_rt5663_max98927-objs := kbl_rt5663_max98927.o
 snd-soc-kbl_rt5663_rt5514_max98927-objs := kbl_rt5663_rt5514_max98927.o
 snd-soc-skl_rt286-objs := skl_rt286.o
+snd-soc-skl_hda_dsp-objs := skl_hda_dsp_generic.o skl_hda_dsp_common.o
 snd-skl_nau88l25_max98357a-objs := skl_nau88l25_max98357a.o
 snd-soc-skl_nau88l25_ssm4567-objs := skl_nau88l25_ssm4567.o
 
@@ -42,3 +43,4 @@ obj-$(CONFIG_SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH) += snd-soc-kbl_rt566
 obj-$(CONFIG_SND_SOC_INTEL_SKL_RT286_MACH) += snd-soc-skl_rt286.o
 obj-$(CONFIG_SND_SOC_INTEL_SKL_NAU88L25_MAX98357A_MACH) += snd-skl_nau88l25_max98357a.o
 obj-$(CONFIG_SND_SOC_INTEL_SKL_NAU88L25_SSM4567_MACH) += snd-soc-skl_nau88l25_ssm4567.o
+obj-$(CONFIG_SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH) += snd-soc-skl_hda_dsp.o
diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
new file mode 100644
index 0000000..7066bed
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright(c) 2015-17 Intel Corporation.
+
+/*
+ * Common functions used in different Intel machine drivers
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include "../../codecs/hdac_hdmi.h"
+#include "../skylake/skl.h"
+#include "skl_hda_dsp_common.h"
+
+#define NAME_SIZE	32
+
+int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
+{
+	char dai_name[NAME_SIZE];
+	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
+	struct skl_hda_hdmi_pcm *pcm;
+	static int i = 1;	/* hdmi codec dai name starts from index 1 */
+
+	pcm = devm_kzalloc(card->dev, sizeof(*pcm), GFP_KERNEL);
+	if (!pcm)
+		return -ENOMEM;
+
+	snprintf(dai_name, sizeof(dai_name), "intel-hdmi-hifi%d", i++);
+	pcm->codec_dai = snd_soc_card_get_codec_dai(card, dai_name);
+	if (!pcm->codec_dai)
+		return -EINVAL;
+
+	pcm->device = device;
+	list_add_tail(&pcm->head, &ctx->hdmi_pcm_list);
+
+	return 0;
+}
+
+/* skl_hda_digital audio interface glue - connects codec <--> CPU */
+struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
+
+	/* Back End DAI links */
+	{
+		.name = "iDisp1",
+		.id = 1,
+		.cpu_dai_name = "iDisp1 Pin",
+		.codec_name = "ehdaudio0D2",
+		.codec_dai_name = "intel-hdmi-hifi1",
+		.platform_name = "0000:00:1f.3",
+		.dpcm_playback = 1,
+		.no_pcm = 1,
+	},
+	{
+		.name = "iDisp2",
+		.id = 2,
+		.cpu_dai_name = "iDisp2 Pin",
+		.codec_name = "ehdaudio0D2",
+		.codec_dai_name = "intel-hdmi-hifi2",
+		.platform_name = "0000:00:1f.3",
+		.dpcm_playback = 1,
+		.no_pcm = 1,
+	},
+	{
+		.name = "iDisp3",
+		.id = 3,
+		.cpu_dai_name = "iDisp3 Pin",
+		.codec_name = "ehdaudio0D2",
+		.codec_dai_name = "intel-hdmi-hifi3",
+		.platform_name = "0000:00:1f.3",
+		.dpcm_playback = 1,
+		.no_pcm = 1,
+	},
+};
+
+int skl_hda_hdmi_jack_init(struct snd_soc_card *card)
+{
+	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
+	struct skl_hda_hdmi_pcm *pcm;
+	struct snd_soc_component *component = NULL;
+	int err;
+	char jack_name[NAME_SIZE];
+
+	list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) {
+		component = pcm->codec_dai->component;
+		snprintf(jack_name, sizeof(jack_name),
+			"HDMI/DP, pcm=%d Jack", pcm->device);
+		err = snd_soc_card_jack_new(card, jack_name,
+					SND_JACK_AVOUT, &pcm->hdmi_jack,
+					NULL, 0);
+
+		if (err)
+			return err;
+
+		err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device,
+						&pcm->hdmi_jack);
+		if (err < 0)
+			return err;
+	}
+
+	if (!component)
+		return -EINVAL;
+
+	return hdac_hdmi_jack_port_init(component, &card->dapm);
+}
diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h b/sound/soc/intel/boards/skl_hda_dsp_common.h
new file mode 100644
index 0000000..adbf552
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright(c) 2015-17 Intel Corporation.
+
+/*
+ * This file defines data structures used in Machine Driver for Intel
+ * platforms with HDA Codecs.
+ */
+
+#ifndef __SOUND_SOC_HDA_DSP_COMMON_H
+#define __SOUND_SOC_HDA_DSP_COMMON_H
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+
+#define HDA_DSP_MAX_BE_DAI_LINKS 3
+
+struct skl_hda_hdmi_pcm {
+	struct list_head head;
+	struct snd_soc_dai *codec_dai;
+	struct snd_soc_jack hdmi_jack;
+	int device;
+};
+
+struct skl_hda_private {
+	struct list_head hdmi_pcm_list;
+	int pcm_count;
+};
+
+extern struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
+int skl_hda_hdmi_jack_init(struct snd_soc_card *card);
+int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device);
+
+#endif /* __SOUND_SOC_HDA_DSP_COMMON_H */
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
new file mode 100644
index 0000000..9e925ba
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright(c) 2015-17 Intel Corporation.
+
+/*
+ * Machine Driver for SKL/KBL platforms with HDA Codecs
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include "../../codecs/hdac_hdmi.h"
+#include "../skylake/skl.h"
+#include "skl_hda_dsp_common.h"
+
+static const char *platform_name = "0000:00:1f.3";
+
+static const struct snd_soc_dapm_route skl_hda_map[] = {
+
+	{ "hifi3", NULL, "iDisp3 Tx"},
+	{ "iDisp3 Tx", NULL, "iDisp3_out"},
+	{ "hifi2", NULL, "iDisp2 Tx"},
+	{ "iDisp2 Tx", NULL, "iDisp2_out"},
+	{ "hifi1", NULL, "iDisp1 Tx"},
+	{ "iDisp1 Tx", NULL, "iDisp1_out"},
+
+};
+
+static int skl_hda_card_late_probe(struct snd_soc_card *card)
+{
+	return skl_hda_hdmi_jack_init(card);
+}
+
+static int
+skl_hda_add_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *link)
+{
+	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
+	int ret = 0;
+
+	dev_dbg(card->dev, "%s: dai link name - %s\n", __func__, link->name);
+	link->platform_name = platform_name;
+	link->nonatomic = 1;
+
+	if (strstr(link->name, "HDMI"))
+		ret = skl_hda_hdmi_add_pcm(card, ctx->pcm_count);
+	ctx->pcm_count++;
+
+	return ret;
+}
+
+static struct snd_soc_card hda_soc_card = {
+	.name = "skl_hda_card",
+	.owner = THIS_MODULE,
+	.dai_link = skl_hda_be_dai_links,
+	.num_links = ARRAY_SIZE(skl_hda_be_dai_links),
+	.dapm_routes = skl_hda_map,
+	.num_dapm_routes = ARRAY_SIZE(skl_hda_map),
+	.add_dai_link = skl_hda_add_dai_link,
+	.fully_routed = true,
+	.late_probe = skl_hda_card_late_probe,
+};
+
+static int skl_hda_audio_probe(struct platform_device *pdev)
+{
+	struct skl_machine_pdata *pdata;
+	struct skl_hda_private *ctx;
+	int i;
+
+	dev_dbg(&pdev->dev, "%s: machine driver probe got called\n", __func__);
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_ATOMIC);
+	if (!ctx)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&ctx->hdmi_pcm_list);
+	ctx->pcm_count = ARRAY_SIZE(skl_hda_be_dai_links);
+
+	pdata = dev_get_drvdata(&pdev->dev);
+	if (pdata && pdata->platform) {
+		platform_name = pdata->platform;
+		for (i = 0; i < ctx->pcm_count; i++)
+			skl_hda_be_dai_links[i].platform_name = platform_name;
+	}
+
+	hda_soc_card.dev = &pdev->dev;
+	snd_soc_card_set_drvdata(&hda_soc_card, ctx);
+
+	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);
+}
+
+static const struct platform_device_id skl_hda_board_ids[] = {
+	{ .name = "skl_hda_generic" },
+	{ }
+};
+
+static struct platform_driver skl_hda_audio = {
+	.probe = skl_hda_audio_probe,
+	.driver = {
+		.name = "skl_hda_generic",
+		.pm = &snd_soc_pm_ops,
+	},
+	.id_table = skl_hda_board_ids,
+};
+
+module_platform_driver(skl_hda_audio)
+
+/* Module information */
+MODULE_DESCRIPTION("SKL/KBL HDA Generic Machine driver");
+MODULE_AUTHOR("Rakesh Ughreja <rakesh.a.ughreja@intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:skl_hda_generic");
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index 75adce8..8a63626 100644
--- a/sound/soc/intel/skylake/skl.h
+++ b/sound/soc/intel/skylake/skl.h
@@ -113,6 +113,7 @@ struct skl_dma_params {
 struct skl_machine_pdata {
 	u32 dmic_num;
 	bool use_tplg_pcm; /* use dais and dai links from topology */
+	const char *platform;
 };
 
 struct skl_dsp_ops {
-- 
2.7.4

Re: [PATCH v1 1/9] ASoC: Intel: Boards: Machine driver for Intel platforms

[Pierre-Louis Bossart]

On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
> Add machine driver for Intel platforms (SKL/KBL/BXT) with HDA and iDisp
> codecs. This patch adds support for only iDisp (HDMI/DP) codec. In the
> following patch support for HDA codec will be added.
> 
> This should work for other Intel platforms as well e.g. GLK,CNL
> however this series is not tested on all the platforms.
> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>   sound/soc/intel/boards/Kconfig               |  10 +++
>   sound/soc/intel/boards/Makefile              |   2 +
>   sound/soc/intel/boards/skl_hda_dsp_common.c  | 107 +++++++++++++++++++++++++
>   sound/soc/intel/boards/skl_hda_dsp_common.h  |  34 ++++++++
>   sound/soc/intel/boards/skl_hda_dsp_generic.c | 114 +++++++++++++++++++++++++++
>   sound/soc/intel/skylake/skl.h                |   1 +
>   6 files changed, 268 insertions(+)
>   create mode 100644 sound/soc/intel/boards/skl_hda_dsp_common.c
>   create mode 100644 sound/soc/intel/boards/skl_hda_dsp_common.h
>   create mode 100644 sound/soc/intel/boards/skl_hda_dsp_generic.c
> 
> diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
> index eedc83b..3b20601 100644
> --- a/sound/soc/intel/boards/Kconfig
> +++ b/sound/soc/intel/boards/Kconfig
> @@ -268,6 +268,16 @@ config SND_SOC_INTEL_KBL_DA7219_MAX98357A_MACH
>   	  This adds support for ASoC Onboard Codec I2S machine driver. This will
>   	  create an alsa sound card for DA7219 + MAX98357A I2S audio codec.
>   	  Say Y if you have such a device.
> +
> +config SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
> +	tristate "SKL/KBL/BXT with HDA Codecs"
> +	select SND_SOC_INTEL_SST
> +	depends on SND_SOC_INTEL_SKYLAKE

There two lines are no longer necessary (done at the platform level for 
SST and you already in an if SKYLAKE block

> +	select SND_SOC_HDAC_HDMI
> +	help
> +	  This adds support for ASoC Onboard Codec HDA machine driver. This will
> +	  create an alsa sound card for HDA Codecs.

does this handle idisp as well? If yes, does this option conflict with 
the enablement of the other machine drivers which use the hdac_hdmi codec?


> +          Say Y or m if you have such a device. This is a recommended option.
>   	  If unsure select "N".
>   
>   endif ## SND_SOC_INTEL_SKYLAKE
> diff --git a/sound/soc/intel/boards/Makefile b/sound/soc/intel/boards/Makefile
> index 6fae506..5d65481 100644
> --- a/sound/soc/intel/boards/Makefile
> +++ b/sound/soc/intel/boards/Makefile
> @@ -18,6 +18,7 @@ snd-soc-kbl_da7219_max98357a-objs := kbl_da7219_max98357a.o
>   snd-soc-kbl_rt5663_max98927-objs := kbl_rt5663_max98927.o
>   snd-soc-kbl_rt5663_rt5514_max98927-objs := kbl_rt5663_rt5514_max98927.o
>   snd-soc-skl_rt286-objs := skl_rt286.o
> +snd-soc-skl_hda_dsp-objs := skl_hda_dsp_generic.o skl_hda_dsp_common.o
>   snd-skl_nau88l25_max98357a-objs := skl_nau88l25_max98357a.o
>   snd-soc-skl_nau88l25_ssm4567-objs := skl_nau88l25_ssm4567.o
>   
> @@ -42,3 +43,4 @@ obj-$(CONFIG_SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH) += snd-soc-kbl_rt566
>   obj-$(CONFIG_SND_SOC_INTEL_SKL_RT286_MACH) += snd-soc-skl_rt286.o
>   obj-$(CONFIG_SND_SOC_INTEL_SKL_NAU88L25_MAX98357A_MACH) += snd-skl_nau88l25_max98357a.o
>   obj-$(CONFIG_SND_SOC_INTEL_SKL_NAU88L25_SSM4567_MACH) += snd-soc-skl_nau88l25_ssm4567.o
> +obj-$(CONFIG_SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH) += snd-soc-skl_hda_dsp.o
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
> new file mode 100644
> index 0000000..7066bed
> --- /dev/null
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2015-17 Intel Corporation.
> +
> +/*
> + * Common functions used in different Intel machine drivers
> + */
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <sound/core.h>
> +#include <sound/jack.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include "../../codecs/hdac_hdmi.h"
> +#include "../skylake/skl.h"
> +#include "skl_hda_dsp_common.h"
> +
> +#define NAME_SIZE	32
> +
> +int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
> +{
> +	char dai_name[NAME_SIZE];
> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> +	struct skl_hda_hdmi_pcm *pcm;
> +	static int i = 1;	/* hdmi codec dai name starts from index 1 */
> +
> +	pcm = devm_kzalloc(card->dev, sizeof(*pcm), GFP_KERNEL);
> +	if (!pcm)
> +		return -ENOMEM;
> +
> +	snprintf(dai_name, sizeof(dai_name), "intel-hdmi-hifi%d", i++);
> +	pcm->codec_dai = snd_soc_card_get_codec_dai(card, dai_name);
> +	if (!pcm->codec_dai)
> +		return -EINVAL;
> +
> +	pcm->device = device;
> +	list_add_tail(&pcm->head, &ctx->hdmi_pcm_list);
> +
> +	return 0;
> +}
> +
> +/* skl_hda_digital audio interface glue - connects codec <--> CPU */
> +struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
> +
> +	/* Back End DAI links */
> +	{
> +		.name = "iDisp1",
> +		.id = 1,
> +		.cpu_dai_name = "iDisp1 Pin",
> +		.codec_name = "ehdaudio0D2",
> +		.codec_dai_name = "intel-hdmi-hifi1",
> +		.platform_name = "0000:00:1f.3",

what happens if you run on a device with a different ID, e.g. APL or GLK?

> +		.dpcm_playback = 1,
> +		.no_pcm = 1,
> +	},
> +	{
> +		.name = "iDisp2",
> +		.id = 2,
> +		.cpu_dai_name = "iDisp2 Pin",
> +		.codec_name = "ehdaudio0D2",
> +		.codec_dai_name = "intel-hdmi-hifi2",
> +		.platform_name = "0000:00:1f.3",
> +		.dpcm_playback = 1,
> +		.no_pcm = 1,
> +	},
> +	{
> +		.name = "iDisp3",
> +		.id = 3,
> +		.cpu_dai_name = "iDisp3 Pin",
> +		.codec_name = "ehdaudio0D2",
> +		.codec_dai_name = "intel-hdmi-hifi3",
> +		.platform_name = "0000:00:1f.3",
> +		.dpcm_playback = 1,
> +		.no_pcm = 1,
> +	},
> +};
> +
> +int skl_hda_hdmi_jack_init(struct snd_soc_card *card)
> +{
> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> +	struct skl_hda_hdmi_pcm *pcm;
> +	struct snd_soc_component *component = NULL;
> +	int err;
> +	char jack_name[NAME_SIZE];
> +
> +	list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) {
> +		component = pcm->codec_dai->component;
> +		snprintf(jack_name, sizeof(jack_name),
> +			"HDMI/DP, pcm=%d Jack", pcm->device);
> +		err = snd_soc_card_jack_new(card, jack_name,
> +					SND_JACK_AVOUT, &pcm->hdmi_jack,
> +					NULL, 0);
> +
> +		if (err)
> +			return err;
> +
> +		err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device,
> +						&pcm->hdmi_jack);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	if (!component)
> +		return -EINVAL;
> +
> +	return hdac_hdmi_jack_port_init(component, &card->dapm);
> +}
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h b/sound/soc/intel/boards/skl_hda_dsp_common.h
> new file mode 100644
> index 0000000..adbf552
> --- /dev/null
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2015-17 Intel Corporation.
> +
> +/*
> + * This file defines data structures used in Machine Driver for Intel
> + * platforms with HDA Codecs.
> + */
> +
> +#ifndef __SOUND_SOC_HDA_DSP_COMMON_H
> +#define __SOUND_SOC_HDA_DSP_COMMON_H
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <sound/core.h>
> +#include <sound/jack.h>
> +
> +#define HDA_DSP_MAX_BE_DAI_LINKS 3
> +
> +struct skl_hda_hdmi_pcm {
> +	struct list_head head;
> +	struct snd_soc_dai *codec_dai;
> +	struct snd_soc_jack hdmi_jack;
> +	int device;
> +};
> +
> +struct skl_hda_private {
> +	struct list_head hdmi_pcm_list;
> +	int pcm_count;
> +};
> +
> +extern struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
> +int skl_hda_hdmi_jack_init(struct snd_soc_card *card);
> +int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device);
> +
> +#endif /* __SOUND_SOC_HDA_DSP_COMMON_H */
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> new file mode 100644
> index 0000000..9e925ba
> --- /dev/null
> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2015-17 Intel Corporation.
> +
> +/*
> + * Machine Driver for SKL/KBL platforms with HDA Codecs

should this be SKL/KBL/APL/GLK....

> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <sound/core.h>
> +#include <sound/jack.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include "../../codecs/hdac_hdmi.h"
> +#include "../skylake/skl.h"
> +#include "skl_hda_dsp_common.h"
> +
> +static const char *platform_name = "0000:00:1f.3";

same comment as above, this is not constant across all devices

> +
> +static const struct snd_soc_dapm_route skl_hda_map[] = {
> +
> +	{ "hifi3", NULL, "iDisp3 Tx"},
> +	{ "iDisp3 Tx", NULL, "iDisp3_out"},
> +	{ "hifi2", NULL, "iDisp2 Tx"},
> +	{ "iDisp2 Tx", NULL, "iDisp2_out"},
> +	{ "hifi1", NULL, "iDisp1 Tx"},
> +	{ "iDisp1 Tx", NULL, "iDisp1_out"},
> +
> +};
> +
> +static int skl_hda_card_late_probe(struct snd_soc_card *card)
> +{
> +	return skl_hda_hdmi_jack_init(card);
> +}
> +
> +static int
> +skl_hda_add_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *link)
> +{
> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> +	int ret = 0;
> +
> +	dev_dbg(card->dev, "%s: dai link name - %s\n", __func__, link->name);
> +	link->platform_name = platform_name;
> +	link->nonatomic = 1;
> +
> +	if (strstr(link->name, "HDMI"))
> +		ret = skl_hda_hdmi_add_pcm(card, ctx->pcm_count);
> +	ctx->pcm_count++;
> +
> +	return ret;
> +}
> +
> +static struct snd_soc_card hda_soc_card = {
> +	.name = "skl_hda_card",
> +	.owner = THIS_MODULE,
> +	.dai_link = skl_hda_be_dai_links,
> +	.num_links = ARRAY_SIZE(skl_hda_be_dai_links),
> +	.dapm_routes = skl_hda_map,
> +	.num_dapm_routes = ARRAY_SIZE(skl_hda_map),
> +	.add_dai_link = skl_hda_add_dai_link,
> +	.fully_routed = true,
> +	.late_probe = skl_hda_card_late_probe,
> +};
> +
> +static int skl_hda_audio_probe(struct platform_device *pdev)
> +{
> +	struct skl_machine_pdata *pdata;
> +	struct skl_hda_private *ctx;
> +	int i;
> +
> +	dev_dbg(&pdev->dev, "%s: machine driver probe got called\n", __func__);
> +
> +	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_ATOMIC);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&ctx->hdmi_pcm_list);
> +	ctx->pcm_count = ARRAY_SIZE(skl_hda_be_dai_links);
> +
> +	pdata = dev_get_drvdata(&pdev->dev);
> +	if (pdata && pdata->platform) {
> +		platform_name = pdata->platform;
> +		for (i = 0; i < ctx->pcm_count; i++)
> +			skl_hda_be_dai_links[i].platform_name = platform_name;
> +	}
> +
> +	hda_soc_card.dev = &pdev->dev;
> +	snd_soc_card_set_drvdata(&hda_soc_card, ctx);
> +
> +	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);
> +}
> +
> +static const struct platform_device_id skl_hda_board_ids[] = {
> +	{ .name = "skl_hda_generic" },
> +	{ }
> +};
> +
> +static struct platform_driver skl_hda_audio = {
> +	.probe = skl_hda_audio_probe,
> +	.driver = {
> +		.name = "skl_hda_generic",
> +		.pm = &snd_soc_pm_ops,
> +	},
> +	.id_table = skl_hda_board_ids,

is this needed if you have a single id?

> +};
> +
> +module_platform_driver(skl_hda_audio)
> +
> +/* Module information */
> +MODULE_DESCRIPTION("SKL/KBL HDA Generic Machine driver");

I don't see how this handles HDAudio codecs in general, is this limited 
to iDISP/HDMI support?

> +MODULE_AUTHOR("Rakesh Ughreja <rakesh.a.ughreja@intel.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:skl_hda_generic");
> diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
> index 75adce8..8a63626 100644
> --- a/sound/soc/intel/skylake/skl.h
> +++ b/sound/soc/intel/skylake/skl.h
> @@ -113,6 +113,7 @@ struct skl_dma_params {
>   struct skl_machine_pdata {
>   	u32 dmic_num;
>   	bool use_tplg_pcm; /* use dais and dai links from topology */
> +	const char *platform;
>   };
>   
>   struct skl_dsp_ops {
> 

Re: [PATCH v1 1/9] ASoC: Intel: Boards: Machine driver for Intel platforms

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Friday, February 23, 2018 10:03 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 1/9] ASoC: Intel: Boards: Machine driver for Intel
>platforms
>
>On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>> Add machine driver for Intel platforms (SKL/KBL/BXT) with HDA and iDisp
>> codecs. This patch adds support for only iDisp (HDMI/DP) codec. In the
>> following patch support for HDA codec will be added.
>>
>> This should work for other Intel platforms as well e.g. GLK,CNL
>> however this series is not tested on all the platforms.
>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>> ---
>>   sound/soc/intel/boards/Kconfig               |  10 +++
>>   sound/soc/intel/boards/Makefile              |   2 +
>>   sound/soc/intel/boards/skl_hda_dsp_common.c  | 107
>+++++++++++++++++++++++++
>>   sound/soc/intel/boards/skl_hda_dsp_common.h  |  34 ++++++++
>>   sound/soc/intel/boards/skl_hda_dsp_generic.c | 114
>+++++++++++++++++++++++++++
>>   sound/soc/intel/skylake/skl.h                |   1 +
>>   6 files changed, 268 insertions(+)
>>   create mode 100644 sound/soc/intel/boards/skl_hda_dsp_common.c
>>   create mode 100644 sound/soc/intel/boards/skl_hda_dsp_common.h
>>   create mode 100644 sound/soc/intel/boards/skl_hda_dsp_generic.c
>>
>> diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
>> index eedc83b..3b20601 100644
>> --- a/sound/soc/intel/boards/Kconfig
>> +++ b/sound/soc/intel/boards/Kconfig
>> @@ -268,6 +268,16 @@ config
>SND_SOC_INTEL_KBL_DA7219_MAX98357A_MACH
>>   	  This adds support for ASoC Onboard Codec I2S machine driver. This will
>>   	  create an alsa sound card for DA7219 + MAX98357A I2S audio codec.
>>   	  Say Y if you have such a device.
>> +
>> +config SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
>> +	tristate "SKL/KBL/BXT with HDA Codecs"
>> +	select SND_SOC_INTEL_SST
>> +	depends on SND_SOC_INTEL_SKYLAKE
>
>There two lines are no longer necessary (done at the platform level for
>SST and you already in an if SKYLAKE block

Good point. Now, removed these two lines.

>
>> +	select SND_SOC_HDAC_HDMI
>> +	help
>> +	  This adds support for ASoC Onboard Codec HDA machine driver. This
>will
>> +	  create an alsa sound card for HDA Codecs.
>
>does this handle idisp as well? If yes, does this option conflict with
>the enablement of the other machine drivers which use the hdac_hdmi codec?

Yes, the machine driver does handle iDisp codec. I didn't understand your
comment about conflict and other machine driver using the hdac_hdmi
codec. There is only one iDisp codec and so what is the reason for having
two machine drivers accessing the same codec ? 

>
>
>> +          Say Y or m if you have such a device. This is a recommended option.
>>   	  If unsure select "N".
>>
>>   endif ## SND_SOC_INTEL_SKYLAKE
>> diff --git a/sound/soc/intel/boards/Makefile
>b/sound/soc/intel/boards/Makefile
>> index 6fae506..5d65481 100644
>> --- a/sound/soc/intel/boards/Makefile
>> +++ b/sound/soc/intel/boards/Makefile
>> @@ -18,6 +18,7 @@ snd-soc-kbl_da7219_max98357a-objs :=
>kbl_da7219_max98357a.o
>>   snd-soc-kbl_rt5663_max98927-objs := kbl_rt5663_max98927.o
>>   snd-soc-kbl_rt5663_rt5514_max98927-objs := kbl_rt5663_rt5514_max98927.o
>>   snd-soc-skl_rt286-objs := skl_rt286.o
>> +snd-soc-skl_hda_dsp-objs := skl_hda_dsp_generic.o skl_hda_dsp_common.o
>>   snd-skl_nau88l25_max98357a-objs := skl_nau88l25_max98357a.o
>>   snd-soc-skl_nau88l25_ssm4567-objs := skl_nau88l25_ssm4567.o
>>
>> @@ -42,3 +43,4 @@ obj-
>$(CONFIG_SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH) += snd-
>soc-kbl_rt566
>>   obj-$(CONFIG_SND_SOC_INTEL_SKL_RT286_MACH) += snd-soc-skl_rt286.o
>>   obj-$(CONFIG_SND_SOC_INTEL_SKL_NAU88L25_MAX98357A_MACH) += snd-
>skl_nau88l25_max98357a.o
>>   obj-$(CONFIG_SND_SOC_INTEL_SKL_NAU88L25_SSM4567_MACH) += snd-
>soc-skl_nau88l25_ssm4567.o
>> +obj-$(CONFIG_SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH) += snd-soc-
>skl_hda_dsp.o
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c
>b/sound/soc/intel/boards/skl_hda_dsp_common.c
>> new file mode 100644
>> index 0000000..7066bed
>> --- /dev/null
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
>> @@ -0,0 +1,107 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright(c) 2015-17 Intel Corporation.
>> +
>> +/*
>> + * Common functions used in different Intel machine drivers
>> + */
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <sound/core.h>
>> +#include <sound/jack.h>
>> +#include <sound/pcm.h>
>> +#include <sound/pcm_params.h>
>> +#include <sound/soc.h>
>> +#include "../../codecs/hdac_hdmi.h"
>> +#include "../skylake/skl.h"
>> +#include "skl_hda_dsp_common.h"
>> +
>> +#define NAME_SIZE	32
>> +
>> +int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
>> +{
>> +	char dai_name[NAME_SIZE];
>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
>> +	struct skl_hda_hdmi_pcm *pcm;
>> +	static int i = 1;	/* hdmi codec dai name starts from index 1 */
>> +
>> +	pcm = devm_kzalloc(card->dev, sizeof(*pcm), GFP_KERNEL);
>> +	if (!pcm)
>> +		return -ENOMEM;
>> +
>> +	snprintf(dai_name, sizeof(dai_name), "intel-hdmi-hifi%d", i++);
>> +	pcm->codec_dai = snd_soc_card_get_codec_dai(card, dai_name);
>> +	if (!pcm->codec_dai)
>> +		return -EINVAL;
>> +
>> +	pcm->device = device;
>> +	list_add_tail(&pcm->head, &ctx->hdmi_pcm_list);
>> +
>> +	return 0;
>> +}
>> +
>> +/* skl_hda_digital audio interface glue - connects codec <--> CPU */
>> +struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS]
>= {
>> +
>> +	/* Back End DAI links */
>> +	{
>> +		.name = "iDisp1",
>> +		.id = 1,
>> +		.cpu_dai_name = "iDisp1 Pin",
>> +		.codec_name = "ehdaudio0D2",
>> +		.codec_dai_name = "intel-hdmi-hifi1",
>> +		.platform_name = "0000:00:1f.3",
>
>what happens if you run on a device with a different ID, e.g. APL or GLK?

This is just a default ID. The same machine driver should work. 
The platform name (ID) is passed from platform driver using platform_data. 
You can see that in the later part of the code. So this name gets overwritten. 

>
>> +		.dpcm_playback = 1,
>> +		.no_pcm = 1,
>> +	},
>> +	{
>> +		.name = "iDisp2",
>> +		.id = 2,
>> +		.cpu_dai_name = "iDisp2 Pin",
>> +		.codec_name = "ehdaudio0D2",
>> +		.codec_dai_name = "intel-hdmi-hifi2",
>> +		.platform_name = "0000:00:1f.3",
>> +		.dpcm_playback = 1,
>> +		.no_pcm = 1,
>> +	},
>> +	{
>> +		.name = "iDisp3",
>> +		.id = 3,
>> +		.cpu_dai_name = "iDisp3 Pin",
>> +		.codec_name = "ehdaudio0D2",
>> +		.codec_dai_name = "intel-hdmi-hifi3",
>> +		.platform_name = "0000:00:1f.3",
>> +		.dpcm_playback = 1,
>> +		.no_pcm = 1,
>> +	},
>> +};
>> +
>> +int skl_hda_hdmi_jack_init(struct snd_soc_card *card)
>> +{
>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
>> +	struct skl_hda_hdmi_pcm *pcm;
>> +	struct snd_soc_component *component = NULL;
>> +	int err;
>> +	char jack_name[NAME_SIZE];
>> +
>> +	list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) {
>> +		component = pcm->codec_dai->component;
>> +		snprintf(jack_name, sizeof(jack_name),
>> +			"HDMI/DP, pcm=%d Jack", pcm->device);
>> +		err = snd_soc_card_jack_new(card, jack_name,
>> +					SND_JACK_AVOUT, &pcm-
>>hdmi_jack,
>> +					NULL, 0);
>> +
>> +		if (err)
>> +			return err;
>> +
>> +		err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device,
>> +						&pcm->hdmi_jack);
>> +		if (err < 0)
>> +			return err;
>> +	}
>> +
>> +	if (!component)
>> +		return -EINVAL;
>> +
>> +	return hdac_hdmi_jack_port_init(component, &card->dapm);
>> +}
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h
>b/sound/soc/intel/boards/skl_hda_dsp_common.h
>> new file mode 100644
>> index 0000000..adbf552
>> --- /dev/null
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
>> @@ -0,0 +1,34 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright(c) 2015-17 Intel Corporation.
>> +
>> +/*
>> + * This file defines data structures used in Machine Driver for Intel
>> + * platforms with HDA Codecs.
>> + */
>> +
>> +#ifndef __SOUND_SOC_HDA_DSP_COMMON_H
>> +#define __SOUND_SOC_HDA_DSP_COMMON_H
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <sound/core.h>
>> +#include <sound/jack.h>
>> +
>> +#define HDA_DSP_MAX_BE_DAI_LINKS 3
>> +
>> +struct skl_hda_hdmi_pcm {
>> +	struct list_head head;
>> +	struct snd_soc_dai *codec_dai;
>> +	struct snd_soc_jack hdmi_jack;
>> +	int device;
>> +};
>> +
>> +struct skl_hda_private {
>> +	struct list_head hdmi_pcm_list;
>> +	int pcm_count;
>> +};
>> +
>> +extern struct snd_soc_dai_link
>skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
>> +int skl_hda_hdmi_jack_init(struct snd_soc_card *card);
>> +int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device);
>> +
>> +#endif /* __SOUND_SOC_HDA_DSP_COMMON_H */
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c
>b/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> new file mode 100644
>> index 0000000..9e925ba
>> --- /dev/null
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> @@ -0,0 +1,114 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright(c) 2015-17 Intel Corporation.
>> +
>> +/*
>> + * Machine Driver for SKL/KBL platforms with HDA Codecs
>
>should this be SKL/KBL/APL/GLK....

Yes true. Will change it.

>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <sound/core.h>
>> +#include <sound/jack.h>
>> +#include <sound/pcm.h>
>> +#include <sound/pcm_params.h>
>> +#include <sound/soc.h>
>> +#include "../../codecs/hdac_hdmi.h"
>> +#include "../skylake/skl.h"
>> +#include "skl_hda_dsp_common.h"
>> +
>> +static const char *platform_name = "0000:00:1f.3";
>
>same comment as above, this is not constant across all devices

Yes and its overwritten when it is received from platform driver.
You can see in the later part of the code.

Copy/pasting the code here

>> +	pdata = dev_get_drvdata(&pdev->dev);
>> +	if (pdata && pdata->platform) {
>> +		platform_name = pdata->platform;
>> +		for (i = 0; i < ctx->pcm_count; i++)
>> +			skl_hda_be_dai_links[i].platform_name =
>platform_name;
>> +	}



>
>> +
>> +static const struct snd_soc_dapm_route skl_hda_map[] = {
>> +
>> +	{ "hifi3", NULL, "iDisp3 Tx"},
>> +	{ "iDisp3 Tx", NULL, "iDisp3_out"},
>> +	{ "hifi2", NULL, "iDisp2 Tx"},
>> +	{ "iDisp2 Tx", NULL, "iDisp2_out"},
>> +	{ "hifi1", NULL, "iDisp1 Tx"},
>> +	{ "iDisp1 Tx", NULL, "iDisp1_out"},
>> +
>> +};
>> +
>> +static int skl_hda_card_late_probe(struct snd_soc_card *card)
>> +{
>> +	return skl_hda_hdmi_jack_init(card);
>> +}
>> +
>> +static int
>> +skl_hda_add_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *link)
>> +{
>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
>> +	int ret = 0;
>> +
>> +	dev_dbg(card->dev, "%s: dai link name - %s\n", __func__, link->name);
>> +	link->platform_name = platform_name;
>> +	link->nonatomic = 1;
>> +
>> +	if (strstr(link->name, "HDMI"))
>> +		ret = skl_hda_hdmi_add_pcm(card, ctx->pcm_count);
>> +	ctx->pcm_count++;
>> +
>> +	return ret;
>> +}
>> +
>> +static struct snd_soc_card hda_soc_card = {
>> +	.name = "skl_hda_card",
>> +	.owner = THIS_MODULE,
>> +	.dai_link = skl_hda_be_dai_links,
>> +	.num_links = ARRAY_SIZE(skl_hda_be_dai_links),
>> +	.dapm_routes = skl_hda_map,
>> +	.num_dapm_routes = ARRAY_SIZE(skl_hda_map),
>> +	.add_dai_link = skl_hda_add_dai_link,
>> +	.fully_routed = true,
>> +	.late_probe = skl_hda_card_late_probe,
>> +};
>> +
>> +static int skl_hda_audio_probe(struct platform_device *pdev)
>> +{
>> +	struct skl_machine_pdata *pdata;
>> +	struct skl_hda_private *ctx;
>> +	int i;
>> +
>> +	dev_dbg(&pdev->dev, "%s: machine driver probe got called\n",
>__func__);
>> +
>> +	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_ATOMIC);
>> +	if (!ctx)
>> +		return -ENOMEM;
>> +
>> +	INIT_LIST_HEAD(&ctx->hdmi_pcm_list);
>> +	ctx->pcm_count = ARRAY_SIZE(skl_hda_be_dai_links);
>> +
>> +	pdata = dev_get_drvdata(&pdev->dev);
>> +	if (pdata && pdata->platform) {
>> +		platform_name = pdata->platform;
>> +		for (i = 0; i < ctx->pcm_count; i++)
>> +			skl_hda_be_dai_links[i].platform_name =
>platform_name;
>> +	}
>> +
>> +	hda_soc_card.dev = &pdev->dev;
>> +	snd_soc_card_set_drvdata(&hda_soc_card, ctx);
>> +
>> +	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);
>> +}
>> +
>> +static const struct platform_device_id skl_hda_board_ids[] = {
>> +	{ .name = "skl_hda_generic" },
>> +	{ }
>> +};
>> +
>> +static struct platform_driver skl_hda_audio = {
>> +	.probe = skl_hda_audio_probe,
>> +	.driver = {
>> +		.name = "skl_hda_generic",
>> +		.pm = &snd_soc_pm_ops,
>> +	},
>> +	.id_table = skl_hda_board_ids,
>
>is this needed if you have a single id?

I did the way other machine drivers are doing.
But I think it's not needed for single ID. But I think it's this way so that
more IDs can be added ? So I will keep it as it is.

>
>> +};
>> +
>> +module_platform_driver(skl_hda_audio)
>> +
>> +/* Module information */
>> +MODULE_DESCRIPTION("SKL/KBL HDA Generic Machine driver");
>
>I don't see how this handles HDAudio codecs in general, is this limited
>to iDISP/HDMI support?

In this patch only the iDisp/HDMI support is added. In the later patches
[Patch 9/9] support for HDA codec is added.

Regards,
Rakesh

Re: [PATCH v1 1/9] ASoC: Intel: Boards: Machine driver for Intel platforms

[Pierre-Louis Bossart]

>>> +	select SND_SOC_HDAC_HDMI
>>> +	help
>>> +	  This adds support for ASoC Onboard Codec HDA machine driver. This
>> will
>>> +	  create an alsa sound card for HDA Codecs.
>>
>> does this handle idisp as well? If yes, does this option conflict with
>> the enablement of the other machine drivers which use the hdac_hdmi codec?
> 
> Yes, the machine driver does handle iDisp codec. I didn't understand your
> comment about conflict and other machine driver using the hdac_hdmi
> codec. There is only one iDisp codec and so what is the reason for having
> two machine drivers accessing the same codec ?

thinking a bit more, all the existing machine drivers which provide 
support for HDMI/iDisp also cater to I2S codecs. Since you deal with 
HDaudio only if you haven't found a known I2S codec there is no real 
source of conflict.


>>> +/* skl_hda_digital audio interface glue - connects codec <--> CPU */
>>> +struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS]
>> = {
>>> +
>>> +	/* Back End DAI links */
>>> +	{
>>> +		.name = "iDisp1",
>>> +		.id = 1,
>>> +		.cpu_dai_name = "iDisp1 Pin",
>>> +		.codec_name = "ehdaudio0D2",
>>> +		.codec_dai_name = "intel-hdmi-hifi1",
>>> +		.platform_name = "0000:00:1f.3",
>>
>> what happens if you run on a device with a different ID, e.g. APL or GLK?
> 
> This is just a default ID. The same machine driver should work.
> The platform name (ID) is passed from platform driver using platform_data.
> You can see that in the later part of the code. So this name gets overwritten.

i don't see the point of having a default then? It could just as well be 
"deadbeef" or not initialized to avoid storing useless strings.


>>> +static const char *platform_name = "0000:00:1f.3";
>>
>> same comment as above, this is not constant across all devices
> 
> Yes and its overwritten when it is received from platform driver.
> You can see in the later part of the code.
> 
> Copy/pasting the code here
> 
>>> +	pdata = dev_get_drvdata(&pdev->dev);
>>> +	if (pdata && pdata->platform) {
>>> +		platform_name = pdata->platform;

can be be a const char * then?

>>> +		for (i = 0; i < ctx->pcm_count; i++)
>>> +			skl_hda_be_dai_links[i].platform_name =
>> platform_name;

and why do you need a static variable in the first place, wouldn't

skl_hda_be_dai_links[i].platform_name = pdata->platform work just as well?


>>> +static const struct platform_device_id skl_hda_board_ids[] = {
>>> +	{ .name = "skl_hda_generic" },
>>> +	{ }
>>> +};
>>> +
>>> +static struct platform_driver skl_hda_audio = {
>>> +	.probe = skl_hda_audio_probe,
>>> +	.driver = {
>>> +		.name = "skl_hda_generic",
>>> +		.pm = &snd_soc_pm_ops,
>>> +	},
>>> +	.id_table = skl_hda_board_ids,
>>
>> is this needed if you have a single id?
> 
> I did the way other machine drivers are doing.
> But I think it's not needed for single ID. But I think it's this way so that
> more IDs can be added ? So I will keep it as it is.

what other IDS would you use?

> 
>>
>>> +};
>>> +
>>> +module_platform_driver(skl_hda_audio)
>>> +
>>> +/* Module information */
>>> +MODULE_DESCRIPTION("SKL/KBL HDA Generic Machine driver");
>>
>> I don't see how this handles HDAudio codecs in general, is this limited
>> to iDISP/HDMI support?
> 
> In this patch only the iDisp/HDMI support is added. In the later patches
> [Patch 9/9] support for HDA codec is added.

yes, but I have to go read 8 patches before finding it out... I comment 
patches one by one.

Re: [PATCH v1 1/9] ASoC: Intel: Boards: Machine driver for Intel platforms

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Tuesday, February 27, 2018 12:41 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 1/9] ASoC: Intel: Boards: Machine driver for
>Intel platforms
>
>
>>>> +	select SND_SOC_HDAC_HDMI
>>>> +	help
>>>> +	  This adds support for ASoC Onboard Codec HDA machine driver. This
>>> will
>>>> +	  create an alsa sound card for HDA Codecs.
>>>
>>> does this handle idisp as well? If yes, does this option conflict with
>>> the enablement of the other machine drivers which use the hdac_hdmi codec?
>>
>> Yes, the machine driver does handle iDisp codec. I didn't understand your
>> comment about conflict and other machine driver using the hdac_hdmi
>> codec. There is only one iDisp codec and so what is the reason for having
>> two machine drivers accessing the same codec ?
>
>thinking a bit more, all the existing machine drivers which provide
>support for HDMI/iDisp also cater to I2S codecs. Since you deal with
>HDaudio only if you haven't found a known I2S codec there is no real
>source of conflict.
>

So is it clear now ? Do I need to change anything ?

>
>>>> +/* skl_hda_digital audio interface glue - connects codec <--> CPU */
>>>> +struct snd_soc_dai_link
>skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS]
>>> = {
>>>> +
>>>> +	/* Back End DAI links */
>>>> +	{
>>>> +		.name = "iDisp1",
>>>> +		.id = 1,
>>>> +		.cpu_dai_name = "iDisp1 Pin",
>>>> +		.codec_name = "ehdaudio0D2",
>>>> +		.codec_dai_name = "intel-hdmi-hifi1",
>>>> +		.platform_name = "0000:00:1f.3",
>>>
>>> what happens if you run on a device with a different ID, e.g. APL or GLK?
>>
>> This is just a default ID. The same machine driver should work.
>> The platform name (ID) is passed from platform driver using platform_data.
>> You can see that in the later part of the code. So this name gets overwritten.
>
>i don't see the point of having a default then? It could just as well be
>"deadbeef" or not initialized to avoid storing useless strings.
>

Yes, I can make default as NULL.

>
>>>> +static const char *platform_name = "0000:00:1f.3";
>>>
>>> same comment as above, this is not constant across all devices
>>
>> Yes and its overwritten when it is received from platform driver.
>> You can see in the later part of the code.
>>
>> Copy/pasting the code here
>>
>>>> +	pdata = dev_get_drvdata(&pdev->dev);
>>>> +	if (pdata && pdata->platform) {
>>>> +		platform_name = pdata->platform;
>
>can be be a const char * then?

Since its used only within the file, I marked it as 
static const char *

>
>>>> +		for (i = 0; i < ctx->pcm_count; i++)
>>>> +			skl_hda_be_dai_links[i].platform_name =
>>> platform_name;
>
>and why do you need a static variable in the first place, wouldn't
>
>skl_hda_be_dai_links[i].platform_name = pdata->platform work just as well?

platform_name variable is used in skl_hda_add_dai_link as well.

>
>
>>>> +static const struct platform_device_id skl_hda_board_ids[] = {
>>>> +	{ .name = "skl_hda_generic" },
>>>> +	{ }
>>>> +};
>>>> +
>>>> +static struct platform_driver skl_hda_audio = {
>>>> +	.probe = skl_hda_audio_probe,
>>>> +	.driver = {
>>>> +		.name = "skl_hda_generic",
>>>> +		.pm = &snd_soc_pm_ops,
>>>> +	},
>>>> +	.id_table = skl_hda_board_ids,
>>>
>>> is this needed if you have a single id?
>>
>> I did the way other machine drivers are doing.
>> But I think it's not needed for single ID. But I think it's this way so that
>> more IDs can be added ? So I will keep it as it is.
>
>what other IDS would you use?

I would like to use different IDs for different combination of the codecs.
e.g. HDMI Only, HDA+HDMI to begin with.

>
>>
>>>
>>>> +};
>>>> +
>>>> +module_platform_driver(skl_hda_audio)
>>>> +
>>>> +/* Module information */
>>>> +MODULE_DESCRIPTION("SKL/KBL HDA Generic Machine driver");
>>>
>>> I don't see how this handles HDAudio codecs in general, is this limited
>>> to iDISP/HDMI support?
>>
>> In this patch only the iDisp/HDMI support is added. In the later patches
>> [Patch 9/9] support for HDA codec is added.
>
>yes, but I have to go read 8 patches before finding it out... I comment
>patches one by one.

This is mentioned in the commit message.

[PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs

[Rakesh Ughreja]

When no I2S based codec entries are found in the BIOS, check if there are
any HDA codecs detected on the bus. If there are two (i.e. iDisp + HDA)
HDA codecs found on the bus, load the HDA machine driver.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/intel/skylake/skl.c | 59 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index f948f29..ac64416 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -442,6 +442,24 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
 						{.name = "ssp5_sclkfs"},
 };
 
+static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl,
+					struct snd_soc_acpi_mach *machines)
+{
+
+	struct snd_soc_acpi_mach *mach;
+	struct hdac_bus *bus = skl_to_bus(skl);
+
+	/* check if we have two HDA codecs */
+	if (hweight_long(bus->codec_mask) != 2)
+		return NULL;
+
+	for (mach = machines; mach->id[0]; mach++) {
+		if (!strcmp(mach->id, "HDA_GEN"))
+			return mach;
+	}
+	return NULL;
+}
+
 static int skl_find_machine(struct skl *skl, void *driver_data)
 {
 	struct hdac_bus *bus = skl_to_bus(skl);
@@ -450,8 +468,12 @@ static int skl_find_machine(struct skl *skl, void *driver_data)
 
 	mach = snd_soc_acpi_find_machine(mach);
 	if (mach == NULL) {
-		dev_err(bus->dev, "No matching machine driver found\n");
-		return -ENODEV;
+		dev_dbg(bus->dev, "No matching I2S machine driver found\n");
+		mach = skl_find_hda_machine(skl, driver_data);
+		if (mach == NULL) {
+			dev_err(bus->dev, "No matching machine driver found\n");
+			return -ENODEV;
+		}
 	}
 
 	skl->mach = mach;
@@ -466,8 +488,9 @@ static int skl_find_machine(struct skl *skl, void *driver_data)
 
 static int skl_machine_device_register(struct skl *skl)
 {
-	struct hdac_bus *bus = skl_to_bus(skl);
 	struct snd_soc_acpi_mach *mach = skl->mach;
+	struct hdac_bus *bus = skl_to_bus(skl);
+	struct skl_machine_pdata *pdata;
 	struct platform_device *pdev;
 	int ret;
 
@@ -484,8 +507,11 @@ static int skl_machine_device_register(struct skl *skl)
 		return -EIO;
 	}
 
-	if (mach->pdata)
+	if (mach->pdata) {
+		pdata = (struct skl_machine_pdata *)mach->pdata;
+		pdata->platform = dev_name(bus->dev);
 		dev_set_drvdata(&pdev->dev, mach->pdata);
+	}
 
 	skl->i2s_dev = pdev;
 
@@ -1030,6 +1056,14 @@ static struct snd_soc_acpi_mach sst_skl_devdata[] = {
 		.quirk_data = &skl_codecs,
 		.pdata = &skl_dmic_data
 	},
+	{
+		.id = "HDA_GEN",
+		.drv_name = "skl_hda_generic",
+		.fw_filename = "intel/dsp_fw_release.bin",
+		.machine_quirk = NULL,
+		.quirk_data = NULL,
+		.pdata = &cnl_pdata,
+	},
 	{}
 };
 
@@ -1046,6 +1080,14 @@ static struct snd_soc_acpi_mach sst_bxtp_devdata[] = {
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &bxt_codecs,
 	},
+	{
+		.id = "HDA_GEN",
+		.drv_name = "skl_hda_generic",
+		.fw_filename = "intel/dsp_fw_bxtn.bin",
+		.machine_quirk = NULL,
+		.quirk_data = NULL,
+		.pdata = &cnl_pdata,
+	},
 	{}
 };
 
@@ -1100,7 +1142,14 @@ static struct snd_soc_acpi_mach sst_kbl_devdata[] = {
 		.quirk_data = &kbl_7219_98357_codecs,
 		.pdata = &skl_dmic_data
 	},
-
+	{
+		.id = "HDA_GEN",
+		.drv_name = "skl_hda_generic",
+		.fw_filename = "intel/dsp_fw_kbl.bin",
+		.machine_quirk = NULL,
+		.quirk_data = NULL,
+		.pdata = &cnl_pdata,
+	},
 	{}
 };
 
-- 
2.7.4

Re: [PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs

[Pierre-Louis Bossart]

On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
> When no I2S based codec entries are found in the BIOS, check if there are
> any HDA codecs detected on the bus. If there are two (i.e. iDisp + HDA)
> HDA codecs found on the bus, load the HDA machine driver.

What if you have a headless device with no codec, that means no HDMI 
support? Why is this restriction necessary?

> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>   sound/soc/intel/skylake/skl.c | 59 +++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index f948f29..ac64416 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -442,6 +442,24 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
>   						{.name = "ssp5_sclkfs"},
>   };
>   
> +static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl,
> +					struct snd_soc_acpi_mach *machines)
> +{
> +
> +	struct snd_soc_acpi_mach *mach;
> +	struct hdac_bus *bus = skl_to_bus(skl);
> +
> +	/* check if we have two HDA codecs */
> +	if (hweight_long(bus->codec_mask) != 2)
> +		return NULL;
> +
> +	for (mach = machines; mach->id[0]; mach++) {
> +		if (!strcmp(mach->id, "HDA_GEN"))
> +			return mach;
> +	}

that's not really testing if there are actual HDaudio devices, is this 
loop just there to point to a firmware file?

> +	return NULL;
> +}
> +
>   static int skl_find_machine(struct skl *skl, void *driver_data)
>   {
>   	struct hdac_bus *bus = skl_to_bus(skl);
> @@ -450,8 +468,12 @@ static int skl_find_machine(struct skl *skl, void *driver_data)
>   
>   	mach = snd_soc_acpi_find_machine(mach);
>   	if (mach == NULL) {
> -		dev_err(bus->dev, "No matching machine driver found\n");
> -		return -ENODEV;
> +		dev_dbg(bus->dev, "No matching I2S machine driver found\n");
> +		mach = skl_find_hda_machine(skl, driver_data);
> +		if (mach == NULL) {
> +			dev_err(bus->dev, "No matching machine driver found\n");
> +			return -ENODEV;
> +		}
>   	}
>   
>   	skl->mach = mach;
> @@ -466,8 +488,9 @@ static int skl_find_machine(struct skl *skl, void *driver_data)
>   
>   static int skl_machine_device_register(struct skl *skl)
>   {
> -	struct hdac_bus *bus = skl_to_bus(skl);
>   	struct snd_soc_acpi_mach *mach = skl->mach;
> +	struct hdac_bus *bus = skl_to_bus(skl);
> +	struct skl_machine_pdata *pdata;
>   	struct platform_device *pdev;
>   	int ret;
>   
> @@ -484,8 +507,11 @@ static int skl_machine_device_register(struct skl *skl)
>   		return -EIO;
>   	}
>   
> -	if (mach->pdata)
> +	if (mach->pdata) {
> +		pdata = (struct skl_machine_pdata *)mach->pdata;
> +		pdata->platform = dev_name(bus->dev);
>   		dev_set_drvdata(&pdev->dev, mach->pdata);
> +	}
>   
>   	skl->i2s_dev = pdev;
>   
> @@ -1030,6 +1056,14 @@ static struct snd_soc_acpi_mach sst_skl_devdata[] = {
>   		.quirk_data = &skl_codecs,
>   		.pdata = &skl_dmic_data
>   	},
> +	{
> +		.id = "HDA_GEN",
> +		.drv_name = "skl_hda_generic",
> +		.fw_filename = "intel/dsp_fw_release.bin",
> +		.machine_quirk = NULL,
> +		.quirk_data = NULL,
> +		.pdata = &cnl_pdata,

this is odd, the cnl_pdata says the topology_pcm is used, I thought this 
was not applicable for SKL/KBL. Or put differently, why is this used for 
the hda case only?

> +	},
>   	{}
>   };
>   
> @@ -1046,6 +1080,14 @@ static struct snd_soc_acpi_mach sst_bxtp_devdata[] = {
>   		.machine_quirk = snd_soc_acpi_codec_list,
>   		.quirk_data = &bxt_codecs,
>   	},
> +	{
> +		.id = "HDA_GEN",
> +		.drv_name = "skl_hda_generic",
> +		.fw_filename = "intel/dsp_fw_bxtn.bin",
> +		.machine_quirk = NULL,
> +		.quirk_data = NULL,
> +		.pdata = &cnl_pdata,
> +	},
>   	{}
>   };
>   
> @@ -1100,7 +1142,14 @@ static struct snd_soc_acpi_mach sst_kbl_devdata[] = {
>   		.quirk_data = &kbl_7219_98357_codecs,
>   		.pdata = &skl_dmic_data
>   	},
> -
> +	{
> +		.id = "HDA_GEN",
> +		.drv_name = "skl_hda_generic",
> +		.fw_filename = "intel/dsp_fw_kbl.bin",
> +		.machine_quirk = NULL,
> +		.quirk_data = NULL,
> +		.pdata = &cnl_pdata,
> +	},
>   	{}
>   };
>   
> 

Re: [PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Friday, February 23, 2018 10:13 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 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for
>HDA codecs
>
>On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>> When no I2S based codec entries are found in the BIOS, check if there are
>> any HDA codecs detected on the bus. If there are two (i.e. iDisp + HDA)
>> HDA codecs found on the bus, load the HDA machine driver.
>
>What if you have a headless device with no codec, that means no HDMI
>support? Why is this restriction necessary?

This machine driver handles HDA and iDisp combination only.

If you want to handle only iDisp or only HDA or more than one HDA
different machine driver needs to be written or this machine
driver needs to be enhanced.

Machine driver has BE DAI link information and so it is specific to those
many devices for a given platform.

>
>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>> ---
>>   sound/soc/intel/skylake/skl.c | 59
>+++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 54 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>> index f948f29..ac64416 100644
>> --- a/sound/soc/intel/skylake/skl.c
>> +++ b/sound/soc/intel/skylake/skl.c
>> @@ -442,6 +442,24 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
>>   						{.name = "ssp5_sclkfs"},
>>   };
>>
>> +static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl,
>> +					struct snd_soc_acpi_mach
>*machines)
>> +{
>> +
>> +	struct snd_soc_acpi_mach *mach;
>> +	struct hdac_bus *bus = skl_to_bus(skl);
>> +
>> +	/* check if we have two HDA codecs */
>> +	if (hweight_long(bus->codec_mask) != 2)
>> +		return NULL;
>> +
>> +	for (mach = machines; mach->id[0]; mach++) {
>> +		if (!strcmp(mach->id, "HDA_GEN"))
>> +			return mach;
>> +	}
>
>that's not really testing if there are actual HDaudio devices, is this
>loop just there to point to a firmware file?

There is a check for number of codecs in the previous line.
By default the iDisp codec would always be present so the check
is for count 2.

>
>> +	return NULL;
>> +}
>> +
>>   static int skl_find_machine(struct skl *skl, void *driver_data)
>>   {
>>   	struct hdac_bus *bus = skl_to_bus(skl);
>> @@ -450,8 +468,12 @@ static int skl_find_machine(struct skl *skl, void
>*driver_data)
>>
>>   	mach = snd_soc_acpi_find_machine(mach);
>>   	if (mach == NULL) {
>> -		dev_err(bus->dev, "No matching machine driver found\n");
>> -		return -ENODEV;
>> +		dev_dbg(bus->dev, "No matching I2S machine driver found\n");
>> +		mach = skl_find_hda_machine(skl, driver_data);
>> +		if (mach == NULL) {
>> +			dev_err(bus->dev, "No matching machine driver
>found\n");
>> +			return -ENODEV;
>> +		}
>>   	}
>>
>>   	skl->mach = mach;
>> @@ -466,8 +488,9 @@ static int skl_find_machine(struct skl *skl, void
>*driver_data)
>>
>>   static int skl_machine_device_register(struct skl *skl)
>>   {
>> -	struct hdac_bus *bus = skl_to_bus(skl);
>>   	struct snd_soc_acpi_mach *mach = skl->mach;
>> +	struct hdac_bus *bus = skl_to_bus(skl);
>> +	struct skl_machine_pdata *pdata;
>>   	struct platform_device *pdev;
>>   	int ret;
>>
>> @@ -484,8 +507,11 @@ static int skl_machine_device_register(struct skl *skl)
>>   		return -EIO;
>>   	}
>>
>> -	if (mach->pdata)
>> +	if (mach->pdata) {
>> +		pdata = (struct skl_machine_pdata *)mach->pdata;
>> +		pdata->platform = dev_name(bus->dev);
>>   		dev_set_drvdata(&pdev->dev, mach->pdata);
>> +	}
>>
>>   	skl->i2s_dev = pdev;
>>
>> @@ -1030,6 +1056,14 @@ static struct snd_soc_acpi_mach sst_skl_devdata[]
>= {
>>   		.quirk_data = &skl_codecs,
>>   		.pdata = &skl_dmic_data
>>   	},
>> +	{
>> +		.id = "HDA_GEN",
>> +		.drv_name = "skl_hda_generic",
>> +		.fw_filename = "intel/dsp_fw_release.bin",
>> +		.machine_quirk = NULL,
>> +		.quirk_data = NULL,
>> +		.pdata = &cnl_pdata,
>
>this is odd, the cnl_pdata says the topology_pcm is used, I thought this
>was not applicable for SKL/KBL. Or put differently, why is this used for
>the hda case only?

This flag is used for using the topology framework for creating the 
FE DAIs and DAI Links. The other I2S machine drivers which are
up-streamed some time back don't have support for creating
DAIs and DAI Links from topology file. Based on your comments
given in the RFC, I have added this support for HDA machine drivers
even for SKL, KBL also.

Regards,
Rakesh

Re: [PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs

[Pierre-Louis Bossart]

On 2/26/18 1:17 AM, Ughreja, Rakesh A wrote:
> 
> 
>> -----Original Message-----
>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>> Sent: Friday, February 23, 2018 10:13 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 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for
>> HDA codecs
>>
>> On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>>> When no I2S based codec entries are found in the BIOS, check if there are
>>> any HDA codecs detected on the bus. If there are two (i.e. iDisp + HDA)
>>> HDA codecs found on the bus, load the HDA machine driver.
>>
>> What if you have a headless device with no codec, that means no HDMI
>> support? Why is this restriction necessary?
> 
> This machine driver handles HDA and iDisp combination only.
> 
> If you want to handle only iDisp or only HDA or more than one HDA
> different machine driver needs to be written or this machine
> driver needs to be enhanced.
> 
> Machine driver has BE DAI link information and so it is specific to those
> many devices for a given platform.

You could pass a flag in the machine driver pdata stating if there is a 
codec in addition to iDisp and add the relevant backends as needed.

> 
>>
>>>
>>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>>> ---
>>>    sound/soc/intel/skylake/skl.c | 59
>> +++++++++++++++++++++++++++++++++++++++----
>>>    1 file changed, 54 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>>> index f948f29..ac64416 100644
>>> --- a/sound/soc/intel/skylake/skl.c
>>> +++ b/sound/soc/intel/skylake/skl.c
>>> @@ -442,6 +442,24 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
>>>    						{.name = "ssp5_sclkfs"},
>>>    };
>>>
>>> +static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl,
>>> +					struct snd_soc_acpi_mach
>> *machines)
>>> +{
>>> +
>>> +	struct snd_soc_acpi_mach *mach;
>>> +	struct hdac_bus *bus = skl_to_bus(skl);
>>> +
>>> +	/* check if we have two HDA codecs */
>>> +	if (hweight_long(bus->codec_mask) != 2)
>>> +		return NULL;
>>> +
>>> +	for (mach = machines; mach->id[0]; mach++) {
>>> +		if (!strcmp(mach->id, "HDA_GEN"))
>>> +			return mach;
>>> +	}
>>
>> that's not really testing if there are actual HDaudio devices, is this
>> loop just there to point to a firmware file?
> 
> There is a check for number of codecs in the previous line.
> By default the iDisp codec would always be present so the check
> is for count 2.

I was referring to the for(mach= loop. I am not sure what the test on 
"HDA_GEN" actually does beyond providing information for the platform 
driver.

> 
>>
>>> +	return NULL;
>>> +}
>>> +
>>>    static int skl_find_machine(struct skl *skl, void *driver_data)
>>>    {
>>>    	struct hdac_bus *bus = skl_to_bus(skl);
>>> @@ -450,8 +468,12 @@ static int skl_find_machine(struct skl *skl, void
>> *driver_data)
>>>
>>>    	mach = snd_soc_acpi_find_machine(mach);
>>>    	if (mach == NULL) {
>>> -		dev_err(bus->dev, "No matching machine driver found\n");
>>> -		return -ENODEV;
>>> +		dev_dbg(bus->dev, "No matching I2S machine driver found\n");
>>> +		mach = skl_find_hda_machine(skl, driver_data);
>>> +		if (mach == NULL) {
>>> +			dev_err(bus->dev, "No matching machine driver
>> found\n");
>>> +			return -ENODEV;
>>> +		}
>>>    	}
>>>
>>>    	skl->mach = mach;
>>> @@ -466,8 +488,9 @@ static int skl_find_machine(struct skl *skl, void
>> *driver_data)
>>>
>>>    static int skl_machine_device_register(struct skl *skl)
>>>    {
>>> -	struct hdac_bus *bus = skl_to_bus(skl);
>>>    	struct snd_soc_acpi_mach *mach = skl->mach;
>>> +	struct hdac_bus *bus = skl_to_bus(skl);
>>> +	struct skl_machine_pdata *pdata;
>>>    	struct platform_device *pdev;
>>>    	int ret;
>>>
>>> @@ -484,8 +507,11 @@ static int skl_machine_device_register(struct skl *skl)
>>>    		return -EIO;
>>>    	}
>>>
>>> -	if (mach->pdata)
>>> +	if (mach->pdata) {
>>> +		pdata = (struct skl_machine_pdata *)mach->pdata;
>>> +		pdata->platform = dev_name(bus->dev);
>>>    		dev_set_drvdata(&pdev->dev, mach->pdata);
>>> +	}
>>>
>>>    	skl->i2s_dev = pdev;
>>>
>>> @@ -1030,6 +1056,14 @@ static struct snd_soc_acpi_mach sst_skl_devdata[]
>> = {
>>>    		.quirk_data = &skl_codecs,
>>>    		.pdata = &skl_dmic_data
>>>    	},
>>> +	{
>>> +		.id = "HDA_GEN",
>>> +		.drv_name = "skl_hda_generic",
>>> +		.fw_filename = "intel/dsp_fw_release.bin",
>>> +		.machine_quirk = NULL,
>>> +		.quirk_data = NULL,
>>> +		.pdata = &cnl_pdata,
>>
>> this is odd, the cnl_pdata says the topology_pcm is used, I thought this
>> was not applicable for SKL/KBL. Or put differently, why is this used for
>> the hda case only?
> 
> This flag is used for using the topology framework for creating the
> FE DAIs and DAI Links. The other I2S machine drivers which are
> up-streamed some time back don't have support for creating
> DAIs and DAI Links from topology file. Based on your comments
> given in the RFC, I have added this support for HDA machine drivers
> even for SKL, KBL also.

What prevents us from updating the machine drivers to use the topology 
in all cases and avoid differences across platforms? I understand that 
some early versions made a choice due to schedule, but for upstream this 
can be realigned.

> 
> Regards,
> Rakesh
> 

Re: [PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Tuesday, February 27, 2018 12:25 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 2/9] ASoC: Intel: Skylake: Add entry in
>sst_acpi_mach for HDA codecs
>
>On 2/26/18 1:17 AM, Ughreja, Rakesh A wrote:
>>
>>
>>> -----Original Message-----
>>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>>> Sent: Friday, February 23, 2018 10:13 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 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach
>for
>>> HDA codecs
>>>
>>> On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>>>> When no I2S based codec entries are found in the BIOS, check if there are
>>>> any HDA codecs detected on the bus. If there are two (i.e. iDisp + HDA)
>>>> HDA codecs found on the bus, load the HDA machine driver.
>>>
>>> What if you have a headless device with no codec, that means no HDMI
>>> support? Why is this restriction necessary?
>>
>> This machine driver handles HDA and iDisp combination only.
>>
>> If you want to handle only iDisp or only HDA or more than one HDA
>> different machine driver needs to be written or this machine
>> driver needs to be enhanced.
>>
>> Machine driver has BE DAI link information and so it is specific to those
>> many devices for a given platform.
>
>You could pass a flag in the machine driver pdata stating if there is a
>codec in addition to iDisp and add the relevant backends as needed.

Yes, I can do that. Will fix it in the next revision.

>
>>
>>>
>>>>
>>>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>>>> ---
>>>>    sound/soc/intel/skylake/skl.c | 59
>>> +++++++++++++++++++++++++++++++++++++++----
>>>>    1 file changed, 54 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>>>> index f948f29..ac64416 100644
>>>> --- a/sound/soc/intel/skylake/skl.c
>>>> +++ b/sound/soc/intel/skylake/skl.c
>>>> @@ -442,6 +442,24 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
>>>>    						{.name = "ssp5_sclkfs"},
>>>>    };
>>>>
>>>> +static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl,
>>>> +					struct snd_soc_acpi_mach
>>> *machines)
>>>> +{
>>>> +
>>>> +	struct snd_soc_acpi_mach *mach;
>>>> +	struct hdac_bus *bus = skl_to_bus(skl);
>>>> +
>>>> +	/* check if we have two HDA codecs */
>>>> +	if (hweight_long(bus->codec_mask) != 2)
>>>> +		return NULL;
>>>> +
>>>> +	for (mach = machines; mach->id[0]; mach++) {
>>>> +		if (!strcmp(mach->id, "HDA_GEN"))
>>>> +			return mach;
>>>> +	}
>>>
>>> that's not really testing if there are actual HDaudio devices, is this
>>> loop just there to point to a firmware file?
>>
>> There is a check for number of codecs in the previous line.
>> By default the iDisp codec would always be present so the check
>> is for count 2.
>
>I was referring to the for(mach= loop. I am not sure what the test on
>"HDA_GEN" actually does beyond providing information for the platform
>driver.

Right, it just finds the entry and provides information to platform driver.

>
>>
>>>
>>>> +	return NULL;
>>>> +}
>>>> +
>>>>    static int skl_find_machine(struct skl *skl, void *driver_data)
>>>>    {
>>>>    	struct hdac_bus *bus = skl_to_bus(skl);
>>>> @@ -450,8 +468,12 @@ static int skl_find_machine(struct skl *skl, void
>>> *driver_data)
>>>>
>>>>    	mach = snd_soc_acpi_find_machine(mach);
>>>>    	if (mach == NULL) {
>>>> -		dev_err(bus->dev, "No matching machine driver found\n");
>>>> -		return -ENODEV;
>>>> +		dev_dbg(bus->dev, "No matching I2S machine driver found\n");
>>>> +		mach = skl_find_hda_machine(skl, driver_data);
>>>> +		if (mach == NULL) {
>>>> +			dev_err(bus->dev, "No matching machine driver
>>> found\n");
>>>> +			return -ENODEV;
>>>> +		}
>>>>    	}
>>>>
>>>>    	skl->mach = mach;
>>>> @@ -466,8 +488,9 @@ static int skl_find_machine(struct skl *skl, void
>>> *driver_data)
>>>>
>>>>    static int skl_machine_device_register(struct skl *skl)
>>>>    {
>>>> -	struct hdac_bus *bus = skl_to_bus(skl);
>>>>    	struct snd_soc_acpi_mach *mach = skl->mach;
>>>> +	struct hdac_bus *bus = skl_to_bus(skl);
>>>> +	struct skl_machine_pdata *pdata;
>>>>    	struct platform_device *pdev;
>>>>    	int ret;
>>>>
>>>> @@ -484,8 +507,11 @@ static int skl_machine_device_register(struct skl
>*skl)
>>>>    		return -EIO;
>>>>    	}
>>>>
>>>> -	if (mach->pdata)
>>>> +	if (mach->pdata) {
>>>> +		pdata = (struct skl_machine_pdata *)mach->pdata;
>>>> +		pdata->platform = dev_name(bus->dev);
>>>>    		dev_set_drvdata(&pdev->dev, mach->pdata);
>>>> +	}
>>>>
>>>>    	skl->i2s_dev = pdev;
>>>>
>>>> @@ -1030,6 +1056,14 @@ static struct snd_soc_acpi_mach
>sst_skl_devdata[]
>>> = {
>>>>    		.quirk_data = &skl_codecs,
>>>>    		.pdata = &skl_dmic_data
>>>>    	},
>>>> +	{
>>>> +		.id = "HDA_GEN",
>>>> +		.drv_name = "skl_hda_generic",
>>>> +		.fw_filename = "intel/dsp_fw_release.bin",
>>>> +		.machine_quirk = NULL,
>>>> +		.quirk_data = NULL,
>>>> +		.pdata = &cnl_pdata,
>>>
>>> this is odd, the cnl_pdata says the topology_pcm is used, I thought this
>>> was not applicable for SKL/KBL. Or put differently, why is this used for
>>> the hda case only?
>>
>> This flag is used for using the topology framework for creating the
>> FE DAIs and DAI Links. The other I2S machine drivers which are
>> up-streamed some time back don't have support for creating
>> DAIs and DAI Links from topology file. Based on your comments
>> given in the RFC, I have added this support for HDA machine drivers
>> even for SKL, KBL also.
>
>What prevents us from updating the machine drivers to use the topology
>in all cases and avoid differences across platforms? I understand that
>some early versions made a choice due to schedule, but for upstream this
>can be realigned.

Certainly that can be done. But that is completely independent work.
That work has no dependency on this patch series.

Regards,
Rakesh

[PATCH v1 3/9] ASoC: Intel: Skylake: add HDA BE DAIs

[Rakesh Ughreja]

Add support for HDA BE DAIs in SKL platform driver.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 50 +++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index f5d5c23..dd109af 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -32,6 +32,7 @@
 #define HDA_MONO 1
 #define HDA_STEREO 2
 #define HDA_QUAD 4
+#define HDA_MAX 8
 
 static const struct snd_pcm_hardware azx_pcm_hw = {
 	.info =			(SNDRV_PCM_INFO_MMAP |
@@ -541,7 +542,10 @@ static int skl_link_hw_params(struct snd_pcm_substream *substream,
 	stream_tag = hdac_stream(link_dev)->stream_tag;
 
 	/* set the stream tag in the codec dai dma params  */
-	snd_soc_dai_set_tdm_slot(codec_dai, stream_tag, 0, 0, 0);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		snd_soc_dai_set_tdm_slot(codec_dai, stream_tag, 0, 0, 0);
+	else
+		snd_soc_dai_set_tdm_slot(codec_dai, 0, stream_tag, 0, 0);
 
 	p_params.s_fmt = snd_pcm_format_width(params_format(params));
 	p_params.ch = params_channels(params);
@@ -956,21 +960,43 @@ static struct snd_soc_dai_driver skl_platform_dai[] = {
 	},
 },
 {
-	.name = "HD-Codec Pin",
+	.name = "Analog CPU DAI",
 	.ops = &skl_link_dai_ops,
 	.playback = {
-		.stream_name = "HD-Codec Tx",
-		.channels_min = HDA_STEREO,
-		.channels_max = HDA_STEREO,
-		.rates = SNDRV_PCM_RATE_48000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.stream_name = "Analog CPU Playback",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_MAX,
+		.rates = SNDRV_PCM_RATE_8000_192000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE |
+			SNDRV_PCM_FMTBIT_S32_LE,
 	},
 	.capture = {
-		.stream_name = "HD-Codec Rx",
-		.channels_min = HDA_STEREO,
-		.channels_max = HDA_STEREO,
-		.rates = SNDRV_PCM_RATE_48000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.stream_name = "Analog CPU Capture",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_MAX,
+		.rates = SNDRV_PCM_RATE_8000_192000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE |
+			SNDRV_PCM_FMTBIT_S32_LE,
+	},
+},
+{
+	.name = "Digital CPU DAI",
+	.ops = &skl_link_dai_ops,
+	.playback = {
+		.stream_name = "Digital CPU Playback",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_MAX,
+		.rates = SNDRV_PCM_RATE_8000_192000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE |
+			SNDRV_PCM_FMTBIT_S32_LE,
+	},
+	.capture = {
+		.stream_name = "Digital CPU Capture",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_MAX,
+		.rates = SNDRV_PCM_RATE_8000_192000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE |
+			SNDRV_PCM_FMTBIT_S32_LE,
 	},
 },
 };
-- 
2.7.4

[PATCH v1 4/9] ASoC: Intel: Skylake: use hda_bus instead of hdac_bus

[Rakesh Ughreja]

Use hda_bus instead of hdac_bus in the SKL ASoC platform driver to enable
reuse of legacy HDA codec drivers.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/intel/skylake/skl.c | 10 +++++++++-
 sound/soc/intel/skylake/skl.h | 10 +++++++---
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index ac64416..b3f830e 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -35,6 +35,7 @@
 #include "skl.h"
 #include "skl-sst-dsp.h"
 #include "skl-sst-ipc.h"
+#include "../../../pci/hda/hda_codec.h"
 
 static struct skl_machine_pdata skl_dmic_data;
 
@@ -637,7 +638,7 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 	mutex_unlock(&bus->cmd_mutex);
 	if (res == -1)
 		return -EIO;
-	dev_dbg(bus->dev, "codec #%d probed OK\n", addr);
+	dev_dbg(bus->dev, "codec #%d probed OK: %x\n", addr, res);
 
 	return snd_hdac_ext_bus_device_init(bus, addr);
 }
@@ -776,6 +777,7 @@ static int skl_create(struct pci_dev *pci,
 {
 	struct skl *skl;
 	struct hdac_bus *bus;
+	struct hda_bus *hbus;
 
 	int err;
 
@@ -791,6 +793,7 @@ static int skl_create(struct pci_dev *pci,
 		return -ENOMEM;
 	}
 
+	hbus = skl_to_hbus(skl);
 	bus = skl_to_bus(skl);
 	snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops);
 	bus->use_posbuf = 1;
@@ -798,6 +801,11 @@ static int skl_create(struct pci_dev *pci,
 	INIT_WORK(&skl->probe_work, skl_probe_work);
 	bus->bdl_pos_adj = 0;
 
+	mutex_init(&hbus->prepare_mutex);
+	hbus->pci = pci;
+	hbus->mixer_assigned = -1;
+	hbus->modelname = "sklbus";
+
 	*rskl = skl;
 
 	return 0;
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index 8a63626..83c4ee3 100644
--- a/sound/soc/intel/skylake/skl.h
+++ b/sound/soc/intel/skylake/skl.h
@@ -26,6 +26,7 @@
 #include <sound/soc.h>
 #include "skl-nhlt.h"
 #include "skl-ssp-clk.h"
+#include "../../../pci/hda/hda_codec.h"
 
 #define SKL_SUSPEND_DELAY 2000
 
@@ -67,7 +68,7 @@ struct skl_fw_config {
 };
 
 struct skl {
-	struct hdac_bus hbus;
+	struct hda_bus hbus;
 	struct pci_dev *pci;
 
 	unsigned int init_done:1; /* delayed init status */
@@ -101,8 +102,11 @@ struct skl {
 	struct snd_soc_acpi_mach *mach;
 };
 
-#define skl_to_bus(s)  (&(s)->hbus)
-#define bus_to_skl(bus) container_of(bus, struct skl, hbus)
+#define skl_to_bus(s)  (&(s)->hbus.core)
+#define bus_to_skl(bus) container_of(bus, struct skl, hbus.core)
+
+#define skl_to_hbus(s) (&(s)->hbus)
+#define hbus_to_skl(hbus) container_of(hbus, struct skl, hbus)
 
 /* to pass dai dma data */
 struct skl_dma_params {
-- 
2.7.4

[PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Rakesh Ughreja]

Split snd_hda_codec_new into two separate functions.
snd_hda_codec_device_init allocates memory and registers with bus.
snd_hda_codec_device_new initialializes the fields and performs
snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC
codec drivers.

In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be
called by ASoC codec drivers.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/pci/hda/hda_codec.c | 67 +++++++++++++++++++++++++++++++++++------------
 sound/pci/hda/hda_codec.h |  2 ++
 2 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 5bc3a74..c487411 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -856,6 +856,38 @@ static void snd_hda_codec_dev_release(struct device *dev)
 	kfree(codec);
 }
 
+static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
+			unsigned int codec_addr, struct hda_codec **codecp)
+{
+	int err;
+	char component[31];
+	struct hda_codec *codec;
+
+	dev_dbg(card->dev, "%s: entry\n", __func__);
+
+	if (snd_BUG_ON(!bus))
+		return -EINVAL;
+	if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
+		return -EINVAL;
+
+	codec = kzalloc(sizeof(*codec), GFP_KERNEL);
+	if (!codec)
+		return -ENOMEM;
+
+	sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
+	err = snd_hdac_device_init(&codec->core, &bus->core, component,
+				   codec_addr);
+	if (err < 0) {
+		kfree(codec);
+		return err;
+	}
+
+	codec->core.type = HDA_DEV_LEGACY;
+	*codecp = codec;
+
+	return err;
+}
+
 /**
  * snd_hda_codec_new - create a HDA codec
  * @bus: the bus to assign
@@ -867,7 +899,19 @@ static void snd_hda_codec_dev_release(struct device *dev)
 int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
 		      unsigned int codec_addr, struct hda_codec **codecp)
 {
-	struct hda_codec *codec;
+	int ret;
+
+	ret = snd_hda_codec_device_init(bus, card, codec_addr, codecp);
+	if (ret < 0)
+		return ret;
+
+	return snd_hda_codec_device_new(bus, card, codec_addr, *codecp);
+}
+EXPORT_SYMBOL_GPL(snd_hda_codec_new);
+
+int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
+			unsigned int codec_addr, struct hda_codec *codec)
+{
 	char component[31];
 	hda_nid_t fg;
 	int err;
@@ -877,25 +921,14 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
 		.dev_free = snd_hda_codec_dev_free,
 	};
 
+	dev_dbg(card->dev, "%s: entry\n", __func__);
+
 	if (snd_BUG_ON(!bus))
 		return -EINVAL;
 	if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
 		return -EINVAL;
 
-	codec = kzalloc(sizeof(*codec), GFP_KERNEL);
-	if (!codec)
-		return -ENOMEM;
-
-	sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
-	err = snd_hdac_device_init(&codec->core, &bus->core, component,
-				   codec_addr);
-	if (err < 0) {
-		kfree(codec);
-		return err;
-	}
-
 	codec->core.dev.release = snd_hda_codec_dev_release;
-	codec->core.type = HDA_DEV_LEGACY;
 	codec->core.exec_verb = codec_exec_verb;
 
 	codec->bus = bus;
@@ -955,15 +988,13 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
 	if (err < 0)
 		goto error;
 
-	if (codecp)
-		*codecp = codec;
 	return 0;
 
  error:
 	put_device(hda_codec_dev(codec));
 	return err;
 }
-EXPORT_SYMBOL_GPL(snd_hda_codec_new);
+EXPORT_SYMBOL_GPL(snd_hda_codec_device_new);
 
 /**
  * snd_hda_codec_update_widgets - Refresh widget caps and pin defaults
@@ -2979,6 +3010,7 @@ int snd_hda_codec_build_controls(struct hda_codec *codec)
 	sync_power_up_states(codec);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(snd_hda_codec_build_controls);
 
 /*
  * PCM stuff
@@ -3184,6 +3216,7 @@ int snd_hda_codec_parse_pcms(struct hda_codec *codec)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(snd_hda_codec_parse_pcms);
 
 /* assign all PCMs of the given codec */
 int snd_hda_codec_build_pcms(struct hda_codec *codec)
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 681c360..8bbedf7 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -307,6 +307,8 @@ struct hda_codec {
  */
 int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
 		      unsigned int codec_addr, struct hda_codec **codecp);
+int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
+		      unsigned int codec_addr, struct hda_codec *codec);
 int snd_hda_codec_configure(struct hda_codec *codec);
 int snd_hda_codec_update_widgets(struct hda_codec *codec);
 
-- 
2.7.4

Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Pierre-Louis Bossart]

On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
> Split snd_hda_codec_new into two separate functions.
> snd_hda_codec_device_init allocates memory and registers with bus.
> snd_hda_codec_device_new initialializes the fields and performs
> snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC
> codec drivers.
> 
> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be
> called by ASoC codec drivers.

I don't get the commit message.
You first say that we can now reuse legacy HDaudio codec drivers in an 
ASoC framework, then say that there will be additional ASoC codec 
drivers? Why would we do this, it seems like a contradicting goal?

> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>   sound/pci/hda/hda_codec.c | 67 +++++++++++++++++++++++++++++++++++------------
>   sound/pci/hda/hda_codec.h |  2 ++
>   2 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 5bc3a74..c487411 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -856,6 +856,38 @@ static void snd_hda_codec_dev_release(struct device *dev)
>   	kfree(codec);
>   }
>   
> +static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
> +			unsigned int codec_addr, struct hda_codec **codecp)
> +{
> +	int err;
> +	char component[31];

looks like magic number?

> +	struct hda_codec *codec;

reverse x-mas tree order?

> +
> +	dev_dbg(card->dev, "%s: entry\n", __func__);
> +
> +	if (snd_BUG_ON(!bus))
> +		return -EINVAL;
> +	if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
> +		return -EINVAL;
> +
> +	codec = kzalloc(sizeof(*codec), GFP_KERNEL);
> +	if (!codec)
> +		return -ENOMEM;
> +
> +	sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
> +	err = snd_hdac_device_init(&codec->core, &bus->core, component,
> +				   codec_addr);
> +	if (err < 0) {
> +		kfree(codec);
> +		return err;
> +	}
> +
> +	codec->core.type = HDA_DEV_LEGACY;
> +	*codecp = codec;
> +
> +	return err;
> +}
> +
>   /**
>    * snd_hda_codec_new - create a HDA codec
>    * @bus: the bus to assign
> @@ -867,7 +899,19 @@ static void snd_hda_codec_dev_release(struct device *dev)
>   int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
>   		      unsigned int codec_addr, struct hda_codec **codecp)
>   {
> -	struct hda_codec *codec;
> +	int ret;
> +
> +	ret = snd_hda_codec_device_init(bus, card, codec_addr, codecp);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snd_hda_codec_device_new(bus, card, codec_addr, *codecp);
> +}
> +EXPORT_SYMBOL_GPL(snd_hda_codec_new);
> +
> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
> +			unsigned int codec_addr, struct hda_codec *codec)
> +{
>   	char component[31];
>   	hda_nid_t fg;
>   	int err;
> @@ -877,25 +921,14 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
>   		.dev_free = snd_hda_codec_dev_free,
>   	};
>   
> +	dev_dbg(card->dev, "%s: entry\n", __func__);
> +
>   	if (snd_BUG_ON(!bus))
>   		return -EINVAL;
>   	if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
>   		return -EINVAL;
>   
> -	codec = kzalloc(sizeof(*codec), GFP_KERNEL);
> -	if (!codec)
> -		return -ENOMEM;
> -
> -	sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
> -	err = snd_hdac_device_init(&codec->core, &bus->core, component,
> -				   codec_addr);
> -	if (err < 0) {
> -		kfree(codec);
> -		return err;
> -	}
> -
>   	codec->core.dev.release = snd_hda_codec_dev_release;
> -	codec->core.type = HDA_DEV_LEGACY;
>   	codec->core.exec_verb = codec_exec_verb;
>   
>   	codec->bus = bus;
> @@ -955,15 +988,13 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
>   	if (err < 0)
>   		goto error;
>   
> -	if (codecp)
> -		*codecp = codec;
>   	return 0;
>   
>    error:
>   	put_device(hda_codec_dev(codec));
>   	return err;
>   }
> -EXPORT_SYMBOL_GPL(snd_hda_codec_new);
> +EXPORT_SYMBOL_GPL(snd_hda_codec_device_new);
>   
>   /**
>    * snd_hda_codec_update_widgets - Refresh widget caps and pin defaults
> @@ -2979,6 +3010,7 @@ int snd_hda_codec_build_controls(struct hda_codec *codec)
>   	sync_power_up_states(codec);
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(snd_hda_codec_build_controls);
>   
>   /*
>    * PCM stuff
> @@ -3184,6 +3216,7 @@ int snd_hda_codec_parse_pcms(struct hda_codec *codec)
>   
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(snd_hda_codec_parse_pcms);
>   
>   /* assign all PCMs of the given codec */
>   int snd_hda_codec_build_pcms(struct hda_codec *codec)
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index 681c360..8bbedf7 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -307,6 +307,8 @@ struct hda_codec {
>    */
>   int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
>   		      unsigned int codec_addr, struct hda_codec **codecp);
> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
> +		      unsigned int codec_addr, struct hda_codec *codec);
>   int snd_hda_codec_configure(struct hda_codec *codec);
>   int snd_hda_codec_update_widgets(struct hda_codec *codec);
>   
> 

Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Friday, February 23, 2018 10:20 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 5/9] ALSA: hda - split snd_hda_codec_new function
>
>On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>> Split snd_hda_codec_new into two separate functions.
>> snd_hda_codec_device_init allocates memory and registers with bus.
>> snd_hda_codec_device_new initialializes the fields and performs
>> snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC
>> codec drivers.
>>
>> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be
>> called by ASoC codec drivers.
>
>I don't get the commit message.
>You first say that we can now reuse legacy HDaudio codec drivers in an
>ASoC framework, then say that there will be additional ASoC codec
>drivers? Why would we do this, it seems like a contradicting goal?
>

Yes, its misleading, so correcting the line as,

In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be
called by ASoC wrapper around the legacy HDA driver (hdac_hda).

>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>> ---
>>   sound/pci/hda/hda_codec.c | 67 +++++++++++++++++++++++++++++++++++--
>----------
>>   sound/pci/hda/hda_codec.h |  2 ++
>>   2 files changed, 52 insertions(+), 17 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>> index 5bc3a74..c487411 100644
>> --- a/sound/pci/hda/hda_codec.c
>> +++ b/sound/pci/hda/hda_codec.c
>> @@ -856,6 +856,38 @@ static void snd_hda_codec_dev_release(struct device
>*dev)
>>   	kfree(codec);
>>   }
>>
>> +static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card
>*card,
>> +			unsigned int codec_addr, struct hda_codec **codecp)
>> +{
>> +	int err;
>> +	char component[31];
>
>looks like magic number?

I kept it as it was in the previous code, but now defined a macro
#define COMPONENT_NAME_SIZE 31

>
>> +	struct hda_codec *codec;
>
>reverse x-mas tree order?

Yes, changed.

Regards,
Rakesh

Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Pierre-Louis Bossart]

On 2/26/18 1:28 AM, Ughreja, Rakesh A wrote:
> 
> 
>> -----Original Message-----
>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>> Sent: Friday, February 23, 2018 10:20 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 5/9] ALSA: hda - split snd_hda_codec_new function
>>
>> On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>>> Split snd_hda_codec_new into two separate functions.
>>> snd_hda_codec_device_init allocates memory and registers with bus.
>>> snd_hda_codec_device_new initialializes the fields and performs
>>> snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC
>>> codec drivers.
>>>
>>> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be
>>> called by ASoC codec drivers.
>>
>> I don't get the commit message.
>> You first say that we can now reuse legacy HDaudio codec drivers in an
>> ASoC framework, then say that there will be additional ASoC codec
>> drivers? Why would we do this, it seems like a contradicting goal?
>>
> 
> Yes, its misleading, so correcting the line as,
> 
> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be
> called by ASoC wrapper around the legacy HDA driver (hdac_hda).

Still confusing. You are both reusing codec drivers and defining a 
wrapper, so are you reusing parts of the codec drivers only?

Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Tuesday, February 27, 2018 12:44 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 5/9] ALSA: hda - split snd_hda_codec_new
>function
>
>On 2/26/18 1:28 AM, Ughreja, Rakesh A wrote:
>>
>>
>>> -----Original Message-----
>>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>>> Sent: Friday, February 23, 2018 10:20 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 5/9] ALSA: hda - split snd_hda_codec_new function
>>>
>>> On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>>>> Split snd_hda_codec_new into two separate functions.
>>>> snd_hda_codec_device_init allocates memory and registers with bus.
>>>> snd_hda_codec_device_new initialializes the fields and performs
>>>> snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC
>>>> codec drivers.
>>>>
>>>> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be
>>>> called by ASoC codec drivers.
>>>
>>> I don't get the commit message.
>>> You first say that we can now reuse legacy HDaudio codec drivers in an
>>> ASoC framework, then say that there will be additional ASoC codec
>>> drivers? Why would we do this, it seems like a contradicting goal?
>>>
>>
>> Yes, its misleading, so correcting the line as,
>>
>> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be
>> called by ASoC wrapper around the legacy HDA driver (hdac_hda).
>
>Still confusing. You are both reusing codec drivers and defining a
>wrapper, so are you reusing parts of the codec drivers only?

There is a kernel module called snd-hda-codec and it reused by all the legacy
codec drivers. I am marking few additional functions with EXPORT_SYMBOL
in the snd-hda-codec and not in the codec driver. 
These new additional functions are used by hdac_hda kernel module.

Existing legacy codec driver is reused completely as it is and not in parts.

Regards,
Rakesh

Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Takashi Iwai]

On Fri, 23 Feb 2018 09:12:26 +0100,
Rakesh Ughreja wrote:
> 
> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
> +			unsigned int codec_addr, struct hda_codec *codec)
> +{
>  	char component[31];

Unused after the patch?


Takashi

Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Friday, February 23, 2018 10:22 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; Koul, Vinod <vinod.koul@intel.com>; Patches
>Audio <patches.audio@intel.com>; pierre-louis.bossart@linux.intel.com
>Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new
>function
>
>On Fri, 23 Feb 2018 09:12:26 +0100,
>Rakesh Ughreja wrote:
>>
>> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
>> +			unsigned int codec_addr, struct hda_codec *codec)
>> +{
>>  	char component[31];
>
>Unused after the patch?

We still use the component variable to construct the name, so I think we still
need it. Following code,

+	sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
+	err = snd_hdac_device_init(&codec->core, &bus->core, component,
+				   codec_addr);


Regards,
Rakesh

Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Takashi Iwai]

On Mon, 26 Feb 2018 09:33:01 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Friday, February 23, 2018 10:22 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; Koul, Vinod <vinod.koul@intel.com>; Patches
> >Audio <patches.audio@intel.com>; pierre-louis.bossart@linux.intel.com
> >Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new
> >function
> >
> >On Fri, 23 Feb 2018 09:12:26 +0100,
> >Rakesh Ughreja wrote:
> >>
> >> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
> >> +			unsigned int codec_addr, struct hda_codec *codec)
> >> +{
> >>  	char component[31];
> >
> >Unused after the patch?
> 
> We still use the component variable to construct the name, so I think we still
> need it. Following code,
> 
> +	sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
> +	err = snd_hdac_device_init(&codec->core, &bus->core, component,
> +				   codec_addr);

Ah yeah, I re-sused the variable for the name string.
It should have been named as "name" or such.


Takashi

Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Monday, February 26, 2018 2:11 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; Koul, Vinod <vinod.koul@intel.com>; Patches
>Audio <patches.audio@intel.com>; pierre-louis.bossart@linux.intel.com
>Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new
>function
>
>On Mon, 26 Feb 2018 09:33:01 +0100,
>Ughreja, Rakesh A wrote:
>>
>>
>>
>> >-----Original Message-----
>> >From: Takashi Iwai [mailto:tiwai@suse.de]
>> >Sent: Friday, February 23, 2018 10:22 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; Koul, Vinod <vinod.koul@intel.com>;
>Patches
>> >Audio <patches.audio@intel.com>; pierre-louis.bossart@linux.intel.com
>> >Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new
>> >function
>> >
>> >On Fri, 23 Feb 2018 09:12:26 +0100,
>> >Rakesh Ughreja wrote:
>> >>
>> >> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card
>*card,
>> >> +			unsigned int codec_addr, struct hda_codec *codec)
>> >> +{
>> >>  	char component[31];
>> >
>> >Unused after the patch?
>>
>> We still use the component variable to construct the name, so I think we still
>> need it. Following code,
>>
>> +	sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
>> +	err = snd_hdac_device_init(&codec->core, &bus->core, component,
>> +				   codec_addr);
>
>Ah yeah, I re-sused the variable for the name string.
>It should have been named as "name" or such.

Let me correct it then. I will rename it to char name[DEVICE_NAME_LEN];

Regards,
Rakesh

[PATCH v1 6/9] ALSA: hdac: remove memory allocation from snd_hdac_ext_bus_device_init

[Rakesh Ughreja]

Remove memory allocation within snd_hdac_ext_bus_device_init, to make
its behaviour identical to snd_hdac_bus_device_init. So that caller
can allocate the parent data structure containing hdac_device.
This API change helps in reusing the legacy HDA codec drivers with
ASoC platform drivers.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 include/sound/hdaudio_ext.h   | 3 ++-
 sound/hda/ext/hdac_ext_bus.c  | 8 ++------
 sound/soc/intel/skylake/skl.c | 8 +++++++-
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index 3c30247..c188b80 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -9,7 +9,8 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 		      const struct hdac_io_ops *io_ops);
 
 void snd_hdac_ext_bus_exit(struct hdac_bus *bus);
-int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr);
+int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
+						struct hdac_device *hdev);
 void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev);
 void snd_hdac_ext_bus_device_remove(struct hdac_bus *bus);
 
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index 52f0776..1eb5824 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -135,16 +135,12 @@ static void default_release(struct device *dev)
  *
  * Returns zero for success or a negative error code.
  */
-int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr)
+int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
+					struct hdac_device *hdev)
 {
-	struct hdac_device *hdev = NULL;
 	char name[15];
 	int ret;
 
-	hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);
-	if (!hdev)
-		return -ENOMEM;
-
 	hdev->bus = bus;
 
 	snprintf(name, sizeof(name), "ehdaudio%dD%d", bus->idx, addr);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index b3f830e..f273b9f 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -631,6 +631,8 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 	unsigned int cmd = (addr << 28) | (AC_NODE_ROOT << 20) |
 		(AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
 	unsigned int res = -1;
+	struct skl *skl = bus_to_skl(bus);
+	struct hdac_device *hdev;
 
 	mutex_lock(&bus->cmd_mutex);
 	snd_hdac_bus_send_cmd(bus, cmd);
@@ -640,7 +642,11 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 		return -EIO;
 	dev_dbg(bus->dev, "codec #%d probed OK: %x\n", addr, res);
 
-	return snd_hdac_ext_bus_device_init(bus, addr);
+	hdev = devm_kzalloc(&skl->pci->dev, sizeof(*hdev), GFP_KERNEL);
+	if (!hdev)
+		return -ENOMEM;
+
+	return snd_hdac_ext_bus_device_init(bus, addr, hdev);
 }
 
 /* Codec initialization */
-- 
2.7.4

[PATCH v1 7/9] ALSA: hdac: add extended ops in the hdac_bus

[Rakesh Ughreja]

Add extended ops in the hdac_bus to allow calling the ASoC HDAC library
ops to reuse the legacy HDA codec drivers with ASoC framework.
Extended ops are used by the legacy codec drivers to call into
hdac_hda library, in the subsequent patches..

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 include/sound/hdaudio.h       | 9 +++++++++
 include/sound/hdaudio_ext.h   | 3 ++-
 sound/hda/ext/hdac_ext_bus.c  | 4 +++-
 sound/soc/intel/skylake/skl.c | 2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 01d3103..f30ecb0 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -214,6 +214,14 @@ struct hdac_bus_ops {
 };
 
 /*
+ * ops used for ASoC HDA codec drivers
+ */
+struct hdac_ext_bus_ops {
+	int (*probe)(struct hdac_device *hdev);
+	int (*remove)(struct hdac_device *hdev);
+};
+
+/*
  * Lowlevel I/O operators
  */
 struct hdac_io_ops {
@@ -268,6 +276,7 @@ struct hdac_bus {
 	struct device *dev;
 	const struct hdac_bus_ops *ops;
 	const struct hdac_io_ops *io_ops;
+	const struct hdac_ext_bus_ops *ext_ops;
 
 	/* h/w resources */
 	unsigned long addr;
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index c188b80..f34aced 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -6,7 +6,8 @@
 
 int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 		      const struct hdac_bus_ops *ops,
-		      const struct hdac_io_ops *io_ops);
+		      const struct hdac_io_ops *io_ops,
+		      const struct hdac_ext_bus_ops *ext_ops);
 
 void snd_hdac_ext_bus_exit(struct hdac_bus *bus);
 int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index 1eb5824..9c37d9a 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -89,7 +89,8 @@ static const struct hdac_io_ops hdac_ext_default_io = {
  */
 int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 			const struct hdac_bus_ops *ops,
-			const struct hdac_io_ops *io_ops)
+			const struct hdac_io_ops *io_ops,
+			const struct hdac_ext_bus_ops *ext_ops)
 {
 	int ret;
 	static int idx;
@@ -102,6 +103,7 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	bus->ext_ops = ext_ops;
 	INIT_LIST_HEAD(&bus->hlink_list);
 	bus->idx = idx++;
 
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index f273b9f..1a5ac1b 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -801,7 +801,7 @@ 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);
+	snd_hdac_ext_bus_init(bus, &pci->dev, &bus_core_ops, io_ops, NULL);
 	bus->use_posbuf = 1;
 	skl->pci = pci;
 	INIT_WORK(&skl->probe_work, skl_probe_work);
-- 
2.7.4

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

[Rakesh Ughreja]

This patch adds a kernel module which is used by the legacy HDA
codec drivers as library. This implements hdac_ext_bus_ops to enable
the reuse of legacy HDA codec drivers with ASoC platform drivers.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/pci/hda/hda_bind.c    |   6 +
 sound/soc/codecs/Kconfig    |   5 +
 sound/soc/codecs/Makefile   |   2 +
 sound/soc/codecs/hdac_hda.c | 448 ++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/hdac_hda.h |  23 +++
 5 files changed, 484 insertions(+)
 create mode 100644 sound/soc/codecs/hdac_hda.c
 create mode 100644 sound/soc/codecs/hdac_hda.h

diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index d361bb7..ec8f846 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -81,6 +81,9 @@ static int hda_codec_driver_probe(struct device *dev)
 	hda_codec_patch_t patch;
 	int err;
 
+	if ((codec->bus->core.ext_ops) && (codec->bus->core.ext_ops->probe))
+		return codec->bus->core.ext_ops->probe(&codec->core);
+
 	if (WARN_ON(!codec->preset))
 		return -EINVAL;
 
@@ -134,6 +137,9 @@ static int hda_codec_driver_remove(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
+	if ((codec->bus->core.ext_ops) && (codec->bus->core.ext_ops->remove))
+		return codec->bus->core.ext_ops->remove(&codec->core);
+
 	if (codec->patch_ops.free)
 		codec->patch_ops.free(codec);
 	snd_hda_codec_cleanup_for_unbind(codec);
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index f72a901..87a166f 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -81,6 +81,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_ES7134
 	select SND_SOC_GTM601
 	select SND_SOC_HDAC_HDMI
+	select SND_SOC_HDAC_HDA
 	select SND_SOC_ICS43432
 	select SND_SOC_INNO_RK3036
 	select SND_SOC_ISABELLE if I2C
@@ -595,6 +596,10 @@ config SND_SOC_HDAC_HDMI
 	select SND_PCM_ELD
 	select HDMI
 
+config SND_SOC_HDAC_HDA
+	tristate
+	select SND_HDA
+
 config SND_SOC_ICS43432
 	tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 56c3252..8fec3a7 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -75,6 +75,7 @@ snd-soc-es8328-i2c-objs := es8328-i2c.o
 snd-soc-es8328-spi-objs := es8328-spi.o
 snd-soc-gtm601-objs := gtm601.o
 snd-soc-hdac-hdmi-objs := hdac_hdmi.o
+snd-soc-hdac-hda-objs := hdac_hda.o
 snd-soc-ics43432-objs := ics43432.o
 snd-soc-inno-rk3036-objs := inno_rk3036.o
 snd-soc-isabelle-objs := isabelle.o
@@ -323,6 +324,7 @@ obj-$(CONFIG_SND_SOC_ES8328_I2C)+= snd-soc-es8328-i2c.o
 obj-$(CONFIG_SND_SOC_ES8328_SPI)+= snd-soc-es8328-spi.o
 obj-$(CONFIG_SND_SOC_GTM601)    += snd-soc-gtm601.o
 obj-$(CONFIG_SND_SOC_HDAC_HDMI) += snd-soc-hdac-hdmi.o
+obj-$(CONFIG_SND_SOC_HDAC_HDA) += snd-soc-hdac-hda.o
 obj-$(CONFIG_SND_SOC_ICS43432)	+= snd-soc-ics43432.o
 obj-$(CONFIG_SND_SOC_INNO_RK3036)	+= snd-soc-inno-rk3036.o
 obj-$(CONFIG_SND_SOC_ISABELLE)	+= snd-soc-isabelle.o
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
new file mode 100644
index 0000000..33b6667
--- /dev/null
+++ b/sound/soc/codecs/hdac_hda.c
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright(c) 2015-17 Intel Corporation.
+
+/*
+ * hdac_hda.c - ASoC exntensions to reuse the legacy HDA codec drivers
+ * with ASoC platform drivers. These APIs are called by the legacy HDA
+ * codec drivers using hdac_ext_bus_ops ops.
+ */
+
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/hdaudio_ext.h>
+#include <sound/hda_register.h>
+#include "../../hda/local.h"
+#include "../../pci/hda/hda_codec.h"
+#include "hdac_hda.h"
+
+#define HDAC_ANALOG_DAI_ID     0
+#define HDAC_DIGITAL_DAI_ID    1
+
+#define STUB_FORMATS	(SNDRV_PCM_FMTBIT_S8 | \
+			SNDRV_PCM_FMTBIT_U8 | \
+			SNDRV_PCM_FMTBIT_S16_LE | \
+			SNDRV_PCM_FMTBIT_U16_LE | \
+			SNDRV_PCM_FMTBIT_S24_LE | \
+			SNDRV_PCM_FMTBIT_U24_LE | \
+			SNDRV_PCM_FMTBIT_S32_LE | \
+			SNDRV_PCM_FMTBIT_U32_LE | \
+			SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE)
+
+static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
+			struct snd_soc_dai *dai);
+static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai);
+static int hdac_hda_dai_prepare(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai);
+static int hdac_hda_dai_hw_free(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai);
+static int hdac_hda_dai_set_tdm_slot(struct snd_soc_dai *dai,
+		unsigned int tx_mask, unsigned int rx_mask,
+		int slots, int slot_width);
+static struct hda_pcm *snd_soc_find_pcm_from_dai(struct hdac_hda_priv *hda_pvt,
+						struct snd_soc_dai *dai);
+
+static struct snd_soc_dai_ops hdac_hda_dai_ops = {
+	.startup = hdac_hda_dai_open,
+	.shutdown = hdac_hda_dai_close,
+	.prepare = hdac_hda_dai_prepare,
+	.hw_free = hdac_hda_dai_hw_free,
+	.set_tdm_slot = hdac_hda_dai_set_tdm_slot,
+};
+
+static struct snd_soc_dai_driver hdac_hda_dais[] = {
+{
+	.id = HDAC_ANALOG_DAI_ID,
+	.name = "Analog Codec DAI",
+	.ops = &hdac_hda_dai_ops,
+	.playback = {
+		.stream_name	= "Analog Codec Playback",
+		.channels_min	= 1,
+		.channels_max	= 16,
+		.rates		= SNDRV_PCM_RATE_8000_192000,
+		.formats	= STUB_FORMATS,
+		.sig_bits	= 24,
+	},
+	.capture = {
+		.stream_name    = "Analog Codec Capture",
+		.channels_min   = 1,
+		.channels_max   = 16,
+		.rates = SNDRV_PCM_RATE_8000_192000,
+		.formats = STUB_FORMATS,
+		.sig_bits = 24,
+	},
+},
+{
+	.id = HDAC_DIGITAL_DAI_ID,
+	.name = "Digital Codec DAI",
+	.ops = &hdac_hda_dai_ops,
+	.playback = {
+		.stream_name    = "Digital Codec Playback",
+		.channels_min   = 1,
+		.channels_max   = 16,
+		.rates          = SNDRV_PCM_RATE_8000_192000,
+		.formats        = STUB_FORMATS,
+		.sig_bits = 24,
+	},
+	.capture = {
+		.stream_name    = "Digital Codec Capture",
+		.channels_min   = 1,
+		.channels_max   = 16,
+		.rates = SNDRV_PCM_RATE_8000_192000,
+		.formats = STUB_FORMATS,
+		.sig_bits = 24,
+	},
+}
+};
+
+static int hdac_hda_dai_set_tdm_slot(struct snd_soc_dai *dai,
+		unsigned int tx_mask, unsigned int rx_mask,
+		int slots, int slot_width)
+{
+	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
+	struct hdac_hda_pcm *pcm;
+
+	pcm = &hda_pvt->pcm[dai->id];
+	if (tx_mask)
+		pcm[dai->id].stream_tag[SNDRV_PCM_STREAM_PLAYBACK] = tx_mask;
+	else
+		pcm[dai->id].stream_tag[SNDRV_PCM_STREAM_CAPTURE] = rx_mask;
+
+	return 0;
+}
+
+static int hdac_hda_dai_hw_free(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai)
+{
+	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
+	struct hda_pcm_stream *hda_stream;
+	struct hda_pcm *pcm;
+
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (pcm == NULL)
+		return -EINVAL;
+
+	hda_stream = &pcm->stream[substream->stream];
+	snd_hda_codec_cleanup(&hda_pvt->codec, hda_stream, substream);
+
+	return 0;
+}
+
+static int hdac_hda_dai_prepare(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai)
+{
+	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct hdac_device *hdev = &hda_pvt->codec.core;
+	struct hda_pcm_stream *hda_stream;
+	unsigned int format_val;
+	struct hda_pcm *pcm;
+	int ret = 0;
+
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (pcm == NULL)
+		return -EINVAL;
+
+	hda_stream = &pcm->stream[substream->stream];
+
+	format_val = snd_hdac_calc_stream_format(runtime->rate,
+						runtime->channels,
+						runtime->format,
+						hda_stream->maxbps,
+						0);
+	if (!format_val) {
+		dev_err(&hdev->dev,
+			"invalid format_val, rate=%d, ch=%d, format=%d\n",
+			runtime->rate, runtime->channels, runtime->format);
+		return -EINVAL;
+	}
+
+	ret = snd_hda_codec_prepare(&hda_pvt->codec, hda_stream,
+			hda_pvt->pcm[dai->id].stream_tag[substream->stream],
+			format_val, substream);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "codec prepare failed %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
+			struct snd_soc_dai *dai)
+{
+	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
+	struct hda_pcm_stream *hda_stream;
+	struct hda_pcm *pcm;
+	int ret = -ENODEV;
+
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (pcm == NULL)
+		return -EINVAL;
+
+	snd_hda_codec_pcm_get(pcm);
+
+	hda_stream = &pcm->stream[substream->stream];
+	if (hda_stream->ops.open)
+		ret = hda_stream->ops.open(hda_stream, &hda_pvt->codec,
+								substream);
+	return ret;
+}
+
+static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
+	struct hda_pcm_stream *hda_stream;
+	struct hda_pcm *pcm;
+
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (pcm == NULL)
+		return;
+
+	hda_stream = &pcm->stream[substream->stream];
+	if (hda_stream->ops.close)
+		hda_stream->ops.close(hda_stream, &hda_pvt->codec, substream);
+
+	snd_hda_codec_pcm_put(pcm);
+}
+
+static struct hda_pcm *snd_soc_find_pcm_from_dai(struct hdac_hda_priv *hda_pvt,
+						struct snd_soc_dai *dai)
+{
+
+	struct hda_codec *hcodec = &hda_pvt->codec;
+	struct hda_pcm *cpcm;
+	const char *pcm_name;
+
+	if (dai->id == HDAC_ANALOG_DAI_ID)
+		pcm_name = "Analog";
+	else
+		pcm_name = "Digital";
+
+	list_for_each_entry(cpcm, &hcodec->pcm_list_head, list) {
+		if (strpbrk(cpcm->name, pcm_name))
+			return cpcm;
+	}
+
+	dev_err(&hcodec->core.dev, "didn't find PCM for DAI %s\n", dai->name);
+	return NULL;
+}
+
+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);
+
+	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);
+
+	ret = snd_hda_codec_device_new(hcodec->bus,
+			codec->component.card->snd_card, hdev->addr, hcodec);
+	if (ret < 0) {
+		dev_err(codec->dev, "failed to create hda codec %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * snd_hda_codec_new1 decrements the usage count and so get the pm
+	 * count else the device will be powered off
+	 */
+	pm_runtime_get_noresume(&hdev->dev);
+
+	hcodec->bus->card = dapm->card->snd_card;
+
+	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
+	if (patch) {
+		ret = patch(hcodec);
+		if (ret < 0) {
+			dev_err(codec->dev, "patch failed %d\n", ret);
+			return ret;
+		}
+	} else {
+		dev_dbg(&hdev->dev, "no patch file found\n");
+	}
+
+	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
+	if (ret < 0) {
+		dev_err(codec->dev, "name failed %s\n", hcodec->preset->name);
+		return ret;
+	}
+
+	ret = snd_hdac_regmap_init(&hcodec->core);
+	if (ret < 0) {
+		dev_err(codec->dev, "regmap init failed\n");
+		return ret;
+	}
+
+	ret = snd_hda_codec_parse_pcms(hcodec);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_hda_codec_build_controls(hcodec);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "unable to create controls %d\n", ret);
+		return ret;
+	}
+
+	hcodec->core.lazy_cache = true;
+
+	/*
+	 * hdac_device core already sets the state to active and calls
+	 * get_noresume. So enable runtime and set the device to suspend.
+	 * pm_runtime_enable is also called during codec registeration
+	 */
+	pm_runtime_put(&hdev->dev);
+	pm_runtime_suspend(&hdev->dev);
+
+	return 0;
+}
+
+static int hdac_hda_codec_remove(struct snd_soc_codec *codec)
+{
+	struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
+	struct hdac_device *hdev = &hda_pvt->codec.core;
+	struct hdac_ext_link *hlink = NULL;
+
+	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;
+	}
+
+	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
+	pm_runtime_disable(&hdev->dev);
+
+	return 0;
+}
+
+
+static const struct snd_soc_dapm_route hdac_hda_dapm_routes[] = {
+	{"AIF1TX", NULL, "Codec Input Pin1"},
+	{"AIF2TX", NULL, "Codec Input Pin2"},
+
+	{"Codec Output Pin1", NULL, "AIF1RX"},
+	{"Codec Output Pin2", NULL, "AIF2RX"},
+};
+
+static const struct snd_soc_dapm_widget hdac_hda_dapm_widgets[] = {
+
+	/* Audio Interface */
+	SND_SOC_DAPM_AIF_IN("AIF1RX", "Analog Codec Playback", 0,
+							SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_AIF_IN("AIF2RX", "Digital Codec Playback", 0,
+							SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_AIF_OUT("AIF1TX", "Analog Codec Capture", 0,
+							SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_AIF_OUT("AIF2TX", "Digital Codec Capture", 0,
+							SND_SOC_NOPM, 0, 0),
+
+	/* Input Pins */
+	SND_SOC_DAPM_INPUT("Codec Input Pin1"),
+	SND_SOC_DAPM_INPUT("Codec Input Pin2"),
+
+	/* Output Pins */
+	SND_SOC_DAPM_OUTPUT("Codec Output Pin1"),
+	SND_SOC_DAPM_OUTPUT("Codec Output Pin2"),
+};
+
+
+static struct snd_soc_codec_driver hdac_hda_codec = {
+	.probe		= hdac_hda_codec_probe,
+	.remove		= hdac_hda_codec_remove,
+	.idle_bias_off = true,
+	.component_driver = {
+		.dapm_widgets           = hdac_hda_dapm_widgets,
+		.num_dapm_widgets       = ARRAY_SIZE(hdac_hda_dapm_widgets),
+		.dapm_routes            = hdac_hda_dapm_routes,
+		.num_dapm_routes        = ARRAY_SIZE(hdac_hda_dapm_routes),
+	},
+};
+
+static int hdac_hda_dev_probe(struct hdac_device *hdev)
+{
+	struct hdac_ext_link *hlink = NULL;
+	struct hdac_hda_priv *hda_pvt;
+	int ret;
+
+	/* hold the ref while we probe */
+	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;
+	}
+	snd_hdac_ext_bus_link_get(hdev->bus, hlink);
+
+	hda_pvt = hdac_to_hda_priv(hdev);
+	if (hda_pvt == NULL)
+		return -ENOMEM;
+
+	/* ASoC specific initialization */
+	ret = snd_soc_register_codec(&hdev->dev, &hdac_hda_codec, hdac_hda_dais,
+					 ARRAY_SIZE(hdac_hda_dais));
+	if (ret < 0) {
+		dev_err(&hdev->dev, "failed to register HDA codec %d\n", ret);
+		return ret;
+	}
+
+	dev_set_drvdata(&hdev->dev, hda_pvt);
+	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
+
+	return ret;
+}
+
+static int hdac_hda_dev_remove(struct hdac_device *hdev)
+{
+	snd_soc_unregister_codec(&hdev->dev);
+	return 0;
+}
+
+static struct hdac_ext_bus_ops hdac_ops = {
+	.probe          = hdac_hda_dev_probe,
+	.remove         = hdac_hda_dev_remove,
+};
+
+struct hdac_ext_bus_ops *snd_soc_hdac_hda_get_ops(void)
+{
+	return &hdac_ops;
+}
+EXPORT_SYMBOL_GPL(snd_soc_hdac_hda_get_ops);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ASoC Extensions for legacy HDA Drivers");
+MODULE_AUTHOR("Rakesh Ughreja<rakesh.a.ughreja@intel.com>");
diff --git a/sound/soc/codecs/hdac_hda.h b/sound/soc/codecs/hdac_hda.h
new file mode 100644
index 0000000..61ce64a
--- /dev/null
+++ b/sound/soc/codecs/hdac_hda.h
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright(c) 2015-17 Intel Corporation.
+
+#ifndef __HDAC_HDA_H__
+#define __HDAC_HDA_H__
+
+struct hdac_hda_pcm {
+	int stream_tag[2];
+};
+
+struct hdac_hda_priv {
+	struct hda_codec codec;
+	struct snd_soc_codec *scodec;
+	struct hdac_hda_pcm pcm[2];
+};
+
+#define hdac_to_hda_priv(_hdac) \
+			container_of(_hdac, struct hdac_hda_priv, codec.core)
+#define hdac_to_hda_codec(_hdac) container_of(_hdac, struct hda_codec, core)
+
+struct hdac_ext_bus_ops *snd_soc_hdac_hda_get_ops(void);
+
+#endif /* __HDAC_HDA_H__ */
-- 
2.7.4

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

[Takashi Iwai]

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.


> +	ret = snd_hda_codec_device_new(hcodec->bus,
> +			codec->component.card->snd_card, hdev->addr, hcodec);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "failed to create hda codec %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * snd_hda_codec_new1 decrements the usage count and so get the pm
> +	 * count else the device will be powered off
> +	 */
> +	pm_runtime_get_noresume(&hdev->dev);
> +
> +	hcodec->bus->card = dapm->card->snd_card;
> +
> +	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
> +	if (patch) {
> +		ret = patch(hcodec);
> +		if (ret < 0) {
> +			dev_err(codec->dev, "patch failed %d\n", ret);
> +			return ret;
> +		}
> +	} else {
> +		dev_dbg(&hdev->dev, "no patch file found\n");
> +	}
> +
> +	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "name failed %s\n", hcodec->preset->name);
> +		return ret;
> +	}
> +
> +	ret = snd_hdac_regmap_init(&hcodec->core);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "regmap init failed\n");
> +		return ret;
> +	}

The regmap and the codec name must be initialized before calling the
patch (i.e. the real probe stuff).

> +
> +	ret = snd_hda_codec_parse_pcms(hcodec);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = snd_hda_codec_build_controls(hcodec);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "unable to create controls %d\n", ret);
> +		return ret;
> +	}
> +
> +	hcodec->core.lazy_cache = true;
> +
> +	/*
> +	 * hdac_device core already sets the state to active and calls
> +	 * get_noresume. So enable runtime and set the device to suspend.
> +	 * pm_runtime_enable is also called during codec registeration
> +	 */
> +	pm_runtime_put(&hdev->dev);
> +	pm_runtime_suspend(&hdev->dev);
> +
> +	return 0;
> +}
> +
> +static int hdac_hda_codec_remove(struct snd_soc_codec *codec)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
> +	struct hdac_device *hdev = &hda_pvt->codec.core;
> +	struct hdac_ext_link *hlink = NULL;
> +
> +	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;
> +	}
> +
> +	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
> +	pm_runtime_disable(&hdev->dev);
> +
> +	return 0;
> +}

.... and I see no proper error paths there, and some cleanups seem
missing, too.

Now I think what you need is rather not to open-code again the mostly
same procedure from hda_codec_driver_probe() / _remove(), but to let
hda_codec_driver_probe() and remove() skip some unnecessary steps for
the ext codec (e.g. registration), in addition to the hlink setup.

That is, hda_codec_driver_probe() would be like:

static int hda_codec_driver_probe(struct device *dev)
{
	....

 	if (WARN_ON(!codec->preset))
 		return -EINVAL;

	if (bus->core.ext_ops) {
		if (!WARN_ON(bus->core.ext_ops->probe))
			return -EINVAL;
		/* register hlink */
		err = bus->core.ext_ops->probe(&codec->core);
		if (err < 0)
			return err;
	}

	err = snd_hda_codec_set_name(codec, codec->preset->name);
	if (err < 0)
		goto error;
	err = snd_hdac_regmap_init(&codec->core);
	if (err < 0)
		goto error;
	.....

	if (!bus->core.ext_ops &&
	    codec->card->registered) {
		err = snd_card_register(codec->card);
		if (err < 0)
		....
	}

	codec->core.lazy_cache = true;
	return 0;

 error_module:
	....
}

static int hda_codec_driver_remove(struct device *dev)
{
	struct hda_codec *codec = dev_to_hda_codec(dev);
	int err;

	if (codec->patch_ops.free)
		codec->patch_ops.free(codec);
	snd_hda_codec_cleanup_for_unbind(codec);
	if (codec->bus->core.ext_ops && codec->bus->core.ext_ops->remove)
		codec->bus->core.ext_ops->remove(&codec->core);
	module_put(dev->driver->owner);
	return 0;
}

... and looking at this, we may rather add the hlink add/remove to
hdac_bus_ops, instead of defining a new ops struct, too.

struct hdac_bus_ops {
	....
	/* reference/unreference ext-bus codec link */
	int (*link_ref)(struct hdac_bus *bus, struct hdac_device *dev);
	int (*link_unref)(struct hdac_bus *bus, struct hdac_device *dev);
};


thanks,

Takashi

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

[Ughreja, Rakesh A]

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

>
>
>> +	ret = snd_hda_codec_device_new(hcodec->bus,
>> +			codec->component.card->snd_card, hdev->addr,
>hcodec);
>> +	if (ret < 0) {
>> +		dev_err(codec->dev, "failed to create hda codec %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * snd_hda_codec_new1 decrements the usage count and so get the pm
>> +	 * count else the device will be powered off
>> +	 */
>> +	pm_runtime_get_noresume(&hdev->dev);
>> +
>> +	hcodec->bus->card = dapm->card->snd_card;
>> +
>> +	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
>> +	if (patch) {
>> +		ret = patch(hcodec);
>> +		if (ret < 0) {
>> +			dev_err(codec->dev, "patch failed %d\n", ret);
>> +			return ret;
>> +		}
>> +	} else {
>> +		dev_dbg(&hdev->dev, "no patch file found\n");
>> +	}
>> +
>> +	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
>> +	if (ret < 0) {
>> +		dev_err(codec->dev, "name failed %s\n", hcodec->preset-
>>name);
>> +		return ret;
>> +	}
>> +
>> +	ret = snd_hdac_regmap_init(&hcodec->core);
>> +	if (ret < 0) {
>> +		dev_err(codec->dev, "regmap init failed\n");
>> +		return ret;
>> +	}
>
>The regmap and the codec name must be initialized before calling the
>patch (i.e. the real probe stuff).

Okay.

>
>> +
>> +	ret = snd_hda_codec_parse_pcms(hcodec);
>> +	if (ret < 0) {
>> +		dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = snd_hda_codec_build_controls(hcodec);
>> +	if (ret < 0) {
>> +		dev_err(&hdev->dev, "unable to create controls %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	hcodec->core.lazy_cache = true;
>> +
>> +	/*
>> +	 * hdac_device core already sets the state to active and calls
>> +	 * get_noresume. So enable runtime and set the device to suspend.
>> +	 * pm_runtime_enable is also called during codec registeration
>> +	 */
>> +	pm_runtime_put(&hdev->dev);
>> +	pm_runtime_suspend(&hdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hdac_hda_codec_remove(struct snd_soc_codec *codec)
>> +{
>> +	struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
>> +	struct hdac_device *hdev = &hda_pvt->codec.core;
>> +	struct hdac_ext_link *hlink = NULL;
>> +
>> +	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;
>> +	}
>> +
>> +	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
>> +	pm_runtime_disable(&hdev->dev);
>> +
>> +	return 0;
>> +}
>
>.... and I see no proper error paths there, and some cleanups seem
>missing, too.

Surely, I can correct it. Do you mind giving some more hints.

>
>Now I think what you need is rather not to open-code again the mostly
>same procedure from hda_codec_driver_probe() / _remove(), but to let
>hda_codec_driver_probe() and remove() skip some unnecessary steps for
>the ext codec (e.g. registration), in addition to the hlink setup.

I think you gave this suggestion in the last series and I tried that [1].
But I think we didn't conclude on that. So let's do it now.

All the functions exception regmap_init which are called in the 
hda_codec_driver_probe requies snd_card and I don't have that until
machine driver creates it. So we will end up in skipping/not calling all the
functions except the regmap_init(). 

If that looks okay to you, I can do that.

>
>That is, hda_codec_driver_probe() would be like:
>
>static int hda_codec_driver_probe(struct device *dev)
>{
>	....
>
> 	if (WARN_ON(!codec->preset))
> 		return -EINVAL;
>
>	if (bus->core.ext_ops) {
>		if (!WARN_ON(bus->core.ext_ops->probe))
>			return -EINVAL;
>		/* register hlink */
>		err = bus->core.ext_ops->probe(&codec->core);
>		if (err < 0)
>			return err;
>	}
>
>	err = snd_hda_codec_set_name(codec, codec->preset->name);
>	if (err < 0)
>		goto error;
>	err = snd_hdac_regmap_init(&codec->core);
>	if (err < 0)
>		goto error;
>	.....
>
>	if (!bus->core.ext_ops &&
>	    codec->card->registered) {
>		err = snd_card_register(codec->card);
>		if (err < 0)
>		....
>	}
>
>	codec->core.lazy_cache = true;
>	return 0;
>
> error_module:
>	....
>}
>
>static int hda_codec_driver_remove(struct device *dev)
>{
>	struct hda_codec *codec = dev_to_hda_codec(dev);
>	int err;
>
>	if (codec->patch_ops.free)
>		codec->patch_ops.free(codec);
>	snd_hda_codec_cleanup_for_unbind(codec);
>	if (codec->bus->core.ext_ops && codec->bus->core.ext_ops->remove)
>		codec->bus->core.ext_ops->remove(&codec->core);
>	module_put(dev->driver->owner);
>	return 0;
>}
>
>... and looking at this, we may rather add the hlink add/remove to
>hdac_bus_ops, instead of defining a new ops struct, too.

Are you talking about these two (snd_hdac_ext_bus_link_put and 
snd_hdac_ext_bus_link_get) functions to put as call backs into
hdac_bus_ops ?

Regards,
Rakesh

[1] https://www.spinics.net/lists/alsa-devel/msg72062.html

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

[Takashi Iwai]

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.


> >
> >> +	ret = snd_hda_codec_device_new(hcodec->bus,
> >> +			codec->component.card->snd_card, hdev->addr,
> >hcodec);
> >> +	if (ret < 0) {
> >> +		dev_err(codec->dev, "failed to create hda codec %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	/*
> >> +	 * snd_hda_codec_new1 decrements the usage count and so get the pm
> >> +	 * count else the device will be powered off
> >> +	 */
> >> +	pm_runtime_get_noresume(&hdev->dev);
> >> +
> >> +	hcodec->bus->card = dapm->card->snd_card;
> >> +
> >> +	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
> >> +	if (patch) {
> >> +		ret = patch(hcodec);
> >> +		if (ret < 0) {
> >> +			dev_err(codec->dev, "patch failed %d\n", ret);
> >> +			return ret;
> >> +		}
> >> +	} else {
> >> +		dev_dbg(&hdev->dev, "no patch file found\n");
> >> +	}
> >> +
> >> +	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
> >> +	if (ret < 0) {
> >> +		dev_err(codec->dev, "name failed %s\n", hcodec->preset-
> >>name);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = snd_hdac_regmap_init(&hcodec->core);
> >> +	if (ret < 0) {
> >> +		dev_err(codec->dev, "regmap init failed\n");
> >> +		return ret;
> >> +	}
> >
> >The regmap and the codec name must be initialized before calling the
> >patch (i.e. the real probe stuff).
> 
> Okay.
> 
> >
> >> +
> >> +	ret = snd_hda_codec_parse_pcms(hcodec);
> >> +	if (ret < 0) {
> >> +		dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = snd_hda_codec_build_controls(hcodec);
> >> +	if (ret < 0) {
> >> +		dev_err(&hdev->dev, "unable to create controls %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	hcodec->core.lazy_cache = true;
> >> +
> >> +	/*
> >> +	 * hdac_device core already sets the state to active and calls
> >> +	 * get_noresume. So enable runtime and set the device to suspend.
> >> +	 * pm_runtime_enable is also called during codec registeration
> >> +	 */
> >> +	pm_runtime_put(&hdev->dev);
> >> +	pm_runtime_suspend(&hdev->dev);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int hdac_hda_codec_remove(struct snd_soc_codec *codec)
> >> +{
> >> +	struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
> >> +	struct hdac_device *hdev = &hda_pvt->codec.core;
> >> +	struct hdac_ext_link *hlink = NULL;
> >> +
> >> +	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;
> >> +	}
> >> +
> >> +	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
> >> +	pm_runtime_disable(&hdev->dev);
> >> +
> >> +	return 0;
> >> +}
> >
> >.... and I see no proper error paths there, and some cleanups seem
> >missing, too.
> 
> Surely, I can correct it. Do you mind giving some more hints.
> 
> >
> >Now I think what you need is rather not to open-code again the mostly
> >same procedure from hda_codec_driver_probe() / _remove(), but to let
> >hda_codec_driver_probe() and remove() skip some unnecessary steps for
> >the ext codec (e.g. registration), in addition to the hlink setup.
> 
> I think you gave this suggestion in the last series and I tried that [1].
> But I think we didn't conclude on that. So let's do it now.
> 
> All the functions exception regmap_init which are called in the 
> hda_codec_driver_probe requies snd_card and I don't have that until
> machine driver creates it. So we will end up in skipping/not calling all the
> functions except the regmap_init(). 
> 
> If that looks okay to you, I can do that.

Ah crap, now I see the point.  The confusion comes from that you have
two probe and two remove functions.  How about renaming the ext_ops
ones?


thanks,

Takashi

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

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Monday, February 26, 2018 3:55 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 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.

Yes, I can move this into the API implementation. So when we call
snd_hdac_ext_bus_link_get it will return only after making sure that the
codecs have responded with the status in the STS register.

>
>
>> >
>> >> +	ret = snd_hda_codec_device_new(hcodec->bus,
>> >> +			codec->component.card->snd_card, hdev->addr,
>> >hcodec);
>> >> +	if (ret < 0) {
>> >> +		dev_err(codec->dev, "failed to create hda codec %d\n", ret);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	/*
>> >> +	 * snd_hda_codec_new1 decrements the usage count and so get the pm
>> >> +	 * count else the device will be powered off
>> >> +	 */
>> >> +	pm_runtime_get_noresume(&hdev->dev);
>> >> +
>> >> +	hcodec->bus->card = dapm->card->snd_card;
>> >> +
>> >> +	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
>> >> +	if (patch) {
>> >> +		ret = patch(hcodec);
>> >> +		if (ret < 0) {
>> >> +			dev_err(codec->dev, "patch failed %d\n", ret);
>> >> +			return ret;
>> >> +		}
>> >> +	} else {
>> >> +		dev_dbg(&hdev->dev, "no patch file found\n");
>> >> +	}
>> >> +
>> >> +	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
>> >> +	if (ret < 0) {
>> >> +		dev_err(codec->dev, "name failed %s\n", hcodec->preset-
>> >>name);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	ret = snd_hdac_regmap_init(&hcodec->core);
>> >> +	if (ret < 0) {
>> >> +		dev_err(codec->dev, "regmap init failed\n");
>> >> +		return ret;
>> >> +	}
>> >
>> >The regmap and the codec name must be initialized before calling the
>> >patch (i.e. the real probe stuff).
>>
>> Okay.
>>
>> >
>> >> +
>> >> +	ret = snd_hda_codec_parse_pcms(hcodec);
>> >> +	if (ret < 0) {
>> >> +		dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	ret = snd_hda_codec_build_controls(hcodec);
>> >> +	if (ret < 0) {
>> >> +		dev_err(&hdev->dev, "unable to create controls %d\n", ret);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	hcodec->core.lazy_cache = true;
>> >> +
>> >> +	/*
>> >> +	 * hdac_device core already sets the state to active and calls
>> >> +	 * get_noresume. So enable runtime and set the device to suspend.
>> >> +	 * pm_runtime_enable is also called during codec registeration
>> >> +	 */
>> >> +	pm_runtime_put(&hdev->dev);
>> >> +	pm_runtime_suspend(&hdev->dev);
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static int hdac_hda_codec_remove(struct snd_soc_codec *codec)
>> >> +{
>> >> +	struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
>> >> +	struct hdac_device *hdev = &hda_pvt->codec.core;
>> >> +	struct hdac_ext_link *hlink = NULL;
>> >> +
>> >> +	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;
>> >> +	}
>> >> +
>> >> +	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
>> >> +	pm_runtime_disable(&hdev->dev);
>> >> +
>> >> +	return 0;
>> >> +}
>> >
>> >.... and I see no proper error paths there, and some cleanups seem
>> >missing, too.
>>
>> Surely, I can correct it. Do you mind giving some more hints.
>>
>> >
>> >Now I think what you need is rather not to open-code again the mostly
>> >same procedure from hda_codec_driver_probe() / _remove(), but to let
>> >hda_codec_driver_probe() and remove() skip some unnecessary steps for
>> >the ext codec (e.g. registration), in addition to the hlink setup.
>>
>> I think you gave this suggestion in the last series and I tried that [1].
>> But I think we didn't conclude on that. So let's do it now.
>>
>> All the functions exception regmap_init which are called in the
>> hda_codec_driver_probe requies snd_card and I don't have that until
>> machine driver creates it. So we will end up in skipping/not calling all the
>> functions except the regmap_init().
>>
>> If that looks okay to you, I can do that.
>
>Ah crap, now I see the point.  The confusion comes from that you have
>two probe and two remove functions.  How about renaming the ext_ops
>ones?

Oh okay, instead of "probe" and "remove", shall I call them "hdev_probe"
and "hdev_remove" to indicate that its related to HDA Device probe at the
bus level and not the ASoC porbe.

Regards,
Rakesh

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

[Takashi Iwai]

On Mon, 26 Feb 2018 16:57:45 +0100,
Ughreja, Rakesh A wrote:
> 
> >Ah crap, now I see the point.  The confusion comes from that you have
> >two probe and two remove functions.  How about renaming the ext_ops
> >ones?
> 
> Oh okay, instead of "probe" and "remove", shall I call them "hdev_probe"
> and "hdev_remove" to indicate that its related to HDA Device probe at the
> bus level and not the ASoC porbe.

I'd call e.g. hdev_attach and hdev_detach or such so that the terms
don't conflict.


thanks,

Takashi

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

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Monday, February 26, 2018 9:30 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 Mon, 26 Feb 2018 16:57:45 +0100,
>Ughreja, Rakesh A wrote:
>>
>> >Ah crap, now I see the point.  The confusion comes from that you have
>> >two probe and two remove functions.  How about renaming the ext_ops
>> >ones?
>>
>> Oh okay, instead of "probe" and "remove", shall I call them "hdev_probe"
>> and "hdev_remove" to indicate that its related to HDA Device probe at the
>> bus level and not the ASoC porbe.
>
>I'd call e.g. hdev_attach and hdev_detach or such so that the terms
>don't conflict.

Sure, I will use the hdev_attach and hdev_detach.

Regards,
Rakesh

Re: [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@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?

Re: [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@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

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

[Pierre-Louis Bossart]

On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
> This patch adds a kernel module which is used by the legacy HDA
> codec drivers as library. This implements hdac_ext_bus_ops to enable
> the reuse of legacy HDA codec drivers with ASoC platform drivers.
> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>   sound/pci/hda/hda_bind.c    |   6 +
>   sound/soc/codecs/Kconfig    |   5 +
>   sound/soc/codecs/Makefile   |   2 +
>   sound/soc/codecs/hdac_hda.c | 448 ++++++++++++++++++++++++++++++++++++++++++++
>   sound/soc/codecs/hdac_hda.h |  23 +++
>   5 files changed, 484 insertions(+)
>   create mode 100644 sound/soc/codecs/hdac_hda.c
>   create mode 100644 sound/soc/codecs/hdac_hda.h

so now we have both hdac_hdmi and hdac_hda?
Not sure I get it.

> 
> diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
> index d361bb7..ec8f846 100644
> --- a/sound/pci/hda/hda_bind.c
> +++ b/sound/pci/hda/hda_bind.c
> @@ -81,6 +81,9 @@ static int hda_codec_driver_probe(struct device *dev)
>   	hda_codec_patch_t patch;
>   	int err;
>   
> +	if ((codec->bus->core.ext_ops) && (codec->bus->core.ext_ops->probe))
> +		return codec->bus->core.ext_ops->probe(&codec->core);
> +
>   	if (WARN_ON(!codec->preset))
>   		return -EINVAL;
>   
> @@ -134,6 +137,9 @@ static int hda_codec_driver_remove(struct device *dev)
>   {
>   	struct hda_codec *codec = dev_to_hda_codec(dev);
>   
> +	if ((codec->bus->core.ext_ops) && (codec->bus->core.ext_ops->remove))
> +		return codec->bus->core.ext_ops->remove(&codec->core);
> +
>   	if (codec->patch_ops.free)
>   		codec->patch_ops.free(codec);
>   	snd_hda_codec_cleanup_for_unbind(codec);
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index f72a901..87a166f 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -81,6 +81,7 @@ config SND_SOC_ALL_CODECS
>   	select SND_SOC_ES7134
>   	select SND_SOC_GTM601
>   	select SND_SOC_HDAC_HDMI
> +	select SND_SOC_HDAC_HDA
>   	select SND_SOC_ICS43432
>   	select SND_SOC_INNO_RK3036
>   	select SND_SOC_ISABELLE if I2C
> @@ -595,6 +596,10 @@ config SND_SOC_HDAC_HDMI
>   	select SND_PCM_ELD
>   	select HDMI
>   
> +config SND_SOC_HDAC_HDA
> +	tristate
> +	select SND_HDA
> +
>   config SND_SOC_ICS43432
>   	tristate
>   
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index 56c3252..8fec3a7 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -75,6 +75,7 @@ snd-soc-es8328-i2c-objs := es8328-i2c.o
>   snd-soc-es8328-spi-objs := es8328-spi.o
>   snd-soc-gtm601-objs := gtm601.o
>   snd-soc-hdac-hdmi-objs := hdac_hdmi.o
> +snd-soc-hdac-hda-objs := hdac_hda.o
>   snd-soc-ics43432-objs := ics43432.o
>   snd-soc-inno-rk3036-objs := inno_rk3036.o
>   snd-soc-isabelle-objs := isabelle.o
> @@ -323,6 +324,7 @@ obj-$(CONFIG_SND_SOC_ES8328_I2C)+= snd-soc-es8328-i2c.o
>   obj-$(CONFIG_SND_SOC_ES8328_SPI)+= snd-soc-es8328-spi.o
>   obj-$(CONFIG_SND_SOC_GTM601)    += snd-soc-gtm601.o
>   obj-$(CONFIG_SND_SOC_HDAC_HDMI) += snd-soc-hdac-hdmi.o
> +obj-$(CONFIG_SND_SOC_HDAC_HDA) += snd-soc-hdac-hda.o
>   obj-$(CONFIG_SND_SOC_ICS43432)	+= snd-soc-ics43432.o
>   obj-$(CONFIG_SND_SOC_INNO_RK3036)	+= snd-soc-inno-rk3036.o
>   obj-$(CONFIG_SND_SOC_ISABELLE)	+= snd-soc-isabelle.o
> diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
> new file mode 100644
> index 0000000..33b6667
> --- /dev/null
> +++ b/sound/soc/codecs/hdac_hda.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2015-17 Intel Corporation.
> +
> +/*
> + * hdac_hda.c - ASoC exntensions to reuse the legacy HDA codec drivers
> + * with ASoC platform drivers. These APIs are called by the legacy HDA
> + * codec drivers using hdac_ext_bus_ops ops.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <sound/hdaudio_ext.h>
> +#include <sound/hda_register.h>
> +#include "../../hda/local.h"
> +#include "../../pci/hda/hda_codec.h"
> +#include "hdac_hda.h"
> +
> +#define HDAC_ANALOG_DAI_ID     0
> +#define HDAC_DIGITAL_DAI_ID    1
> +
> +#define STUB_FORMATS	(SNDRV_PCM_FMTBIT_S8 | \
> +			SNDRV_PCM_FMTBIT_U8 | \
> +			SNDRV_PCM_FMTBIT_S16_LE | \
> +			SNDRV_PCM_FMTBIT_U16_LE | \
> +			SNDRV_PCM_FMTBIT_S24_LE | \
> +			SNDRV_PCM_FMTBIT_U24_LE | \
> +			SNDRV_PCM_FMTBIT_S32_LE | \
> +			SNDRV_PCM_FMTBIT_U32_LE | \
> +			SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE)
> +
> +static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
> +			struct snd_soc_dai *dai);
> +static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
> +		struct snd_soc_dai *dai);
> +static int hdac_hda_dai_prepare(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai);
> +static int hdac_hda_dai_hw_free(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai);
> +static int hdac_hda_dai_set_tdm_slot(struct snd_soc_dai *dai,
> +		unsigned int tx_mask, unsigned int rx_mask,
> +		int slots, int slot_width);
> +static struct hda_pcm *snd_soc_find_pcm_from_dai(struct hdac_hda_priv *hda_pvt,
> +						struct snd_soc_dai *dai);
> +
> +static struct snd_soc_dai_ops hdac_hda_dai_ops = {
> +	.startup = hdac_hda_dai_open,
> +	.shutdown = hdac_hda_dai_close,
> +	.prepare = hdac_hda_dai_prepare,
> +	.hw_free = hdac_hda_dai_hw_free,
> +	.set_tdm_slot = hdac_hda_dai_set_tdm_slot,
> +};
> +
> +static struct snd_soc_dai_driver hdac_hda_dais[] = {
> +{
> +	.id = HDAC_ANALOG_DAI_ID,
> +	.name = "Analog Codec DAI",
> +	.ops = &hdac_hda_dai_ops,
> +	.playback = {
> +		.stream_name	= "Analog Codec Playback",
> +		.channels_min	= 1,
> +		.channels_max	= 16,
> +		.rates		= SNDRV_PCM_RATE_8000_192000,
> +		.formats	= STUB_FORMATS,
> +		.sig_bits	= 24,
> +	},
> +	.capture = {
> +		.stream_name    = "Analog Codec Capture",
> +		.channels_min   = 1,
> +		.channels_max   = 16,
> +		.rates = SNDRV_PCM_RATE_8000_192000,
> +		.formats = STUB_FORMATS,
> +		.sig_bits = 24,
> +	},
> +},
> +{
> +	.id = HDAC_DIGITAL_DAI_ID,
> +	.name = "Digital Codec DAI",
> +	.ops = &hdac_hda_dai_ops,
> +	.playback = {
> +		.stream_name    = "Digital Codec Playback",
> +		.channels_min   = 1,
> +		.channels_max   = 16,
> +		.rates          = SNDRV_PCM_RATE_8000_192000,
> +		.formats        = STUB_FORMATS,
> +		.sig_bits = 24,
> +	},
> +	.capture = {
> +		.stream_name    = "Digital Codec Capture",
> +		.channels_min   = 1,
> +		.channels_max   = 16,
> +		.rates = SNDRV_PCM_RATE_8000_192000,
> +		.formats = STUB_FORMATS,
> +		.sig_bits = 24,
> +	},
> +}
> +};
> +
> +static int hdac_hda_dai_set_tdm_slot(struct snd_soc_dai *dai,
> +		unsigned int tx_mask, unsigned int rx_mask,
> +		int slots, int slot_width)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
> +	struct hdac_hda_pcm *pcm;
> +
> +	pcm = &hda_pvt->pcm[dai->id];
> +	if (tx_mask)
> +		pcm[dai->id].stream_tag[SNDRV_PCM_STREAM_PLAYBACK] = tx_mask;
> +	else
> +		pcm[dai->id].stream_tag[SNDRV_PCM_STREAM_CAPTURE] = rx_mask;
> +
> +	return 0;
> +}
> +
> +static int hdac_hda_dai_hw_free(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
> +	struct hda_pcm_stream *hda_stream;
> +	struct hda_pcm *pcm;
> +
> +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
> +	if (pcm == NULL)
> +		return -EINVAL;
> +
> +	hda_stream = &pcm->stream[substream->stream];
> +	snd_hda_codec_cleanup(&hda_pvt->codec, hda_stream, substream);
> +
> +	return 0;
> +}
> +
> +static int hdac_hda_dai_prepare(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct hdac_device *hdev = &hda_pvt->codec.core;
> +	struct hda_pcm_stream *hda_stream;
> +	unsigned int format_val;
> +	struct hda_pcm *pcm;
> +	int ret = 0;
> +
> +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
> +	if (pcm == NULL)
> +		return -EINVAL;
> +
> +	hda_stream = &pcm->stream[substream->stream];
> +
> +	format_val = snd_hdac_calc_stream_format(runtime->rate,
> +						runtime->channels,
> +						runtime->format,
> +						hda_stream->maxbps,
> +						0);
> +	if (!format_val) {
> +		dev_err(&hdev->dev,
> +			"invalid format_val, rate=%d, ch=%d, format=%d\n",
> +			runtime->rate, runtime->channels, runtime->format);
> +		return -EINVAL;
> +	}
> +
> +	ret = snd_hda_codec_prepare(&hda_pvt->codec, hda_stream,
> +			hda_pvt->pcm[dai->id].stream_tag[substream->stream],
> +			format_val, substream);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "codec prepare failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
> +			struct snd_soc_dai *dai)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
> +	struct hda_pcm_stream *hda_stream;
> +	struct hda_pcm *pcm;
> +	int ret = -ENODEV;
> +
> +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
> +	if (pcm == NULL)
> +		return -EINVAL;
> +
> +	snd_hda_codec_pcm_get(pcm);
> +
> +	hda_stream = &pcm->stream[substream->stream];
> +	if (hda_stream->ops.open)
> +		ret = hda_stream->ops.open(hda_stream, &hda_pvt->codec,
> +								substream);
> +	return ret;
> +}
> +
> +static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
> +		struct snd_soc_dai *dai)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
> +	struct hda_pcm_stream *hda_stream;
> +	struct hda_pcm *pcm;
> +
> +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
> +	if (pcm == NULL)
> +		return;
> +
> +	hda_stream = &pcm->stream[substream->stream];
> +	if (hda_stream->ops.close)
> +		hda_stream->ops.close(hda_stream, &hda_pvt->codec, substream);
> +
> +	snd_hda_codec_pcm_put(pcm);
> +}
> +
> +static struct hda_pcm *snd_soc_find_pcm_from_dai(struct hdac_hda_priv *hda_pvt,
> +						struct snd_soc_dai *dai)
> +{
> +
> +	struct hda_codec *hcodec = &hda_pvt->codec;
> +	struct hda_pcm *cpcm;
> +	const char *pcm_name;
> +
> +	if (dai->id == HDAC_ANALOG_DAI_ID)
> +		pcm_name = "Analog";
> +	else
> +		pcm_name = "Digital";
> +
> +	list_for_each_entry(cpcm, &hcodec->pcm_list_head, list) {
> +		if (strpbrk(cpcm->name, pcm_name))
> +			return cpcm;
> +	}
> +
> +	dev_err(&hcodec->core.dev, "didn't find PCM for DAI %s\n", dai->name);
> +	return NULL;
> +}
> +
> +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);
> +
> +	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);
> +
> +	ret = snd_hda_codec_device_new(hcodec->bus,
> +			codec->component.card->snd_card, hdev->addr, hcodec);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "failed to create hda codec %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * snd_hda_codec_new1 decrements the usage count and so get the pm
> +	 * count else the device will be powered off
> +	 */
> +	pm_runtime_get_noresume(&hdev->dev);
> +
> +	hcodec->bus->card = dapm->card->snd_card;
> +
> +	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
> +	if (patch) {
> +		ret = patch(hcodec);
> +		if (ret < 0) {
> +			dev_err(codec->dev, "patch failed %d\n", ret);
> +			return ret;
> +		}
> +	} else {
> +		dev_dbg(&hdev->dev, "no patch file found\n");
> +	}
> +
> +	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "name failed %s\n", hcodec->preset->name);
> +		return ret;
> +	}
> +
> +	ret = snd_hdac_regmap_init(&hcodec->core);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "regmap init failed\n");
> +		return ret;
> +	}
> +
> +	ret = snd_hda_codec_parse_pcms(hcodec);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = snd_hda_codec_build_controls(hcodec);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "unable to create controls %d\n", ret);
> +		return ret;
> +	}
> +
> +	hcodec->core.lazy_cache = true;
> +
> +	/*
> +	 * hdac_device core already sets the state to active and calls
> +	 * get_noresume. So enable runtime and set the device to suspend.
> +	 * pm_runtime_enable is also called during codec registeration
> +	 */
> +	pm_runtime_put(&hdev->dev);
> +	pm_runtime_suspend(&hdev->dev);
> +
> +	return 0;
> +}
> +
> +static int hdac_hda_codec_remove(struct snd_soc_codec *codec)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
> +	struct hdac_device *hdev = &hda_pvt->codec.core;
> +	struct hdac_ext_link *hlink = NULL;
> +
> +	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;
> +	}
> +
> +	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
> +	pm_runtime_disable(&hdev->dev);
> +
> +	return 0;
> +}
> +
> +
> +static const struct snd_soc_dapm_route hdac_hda_dapm_routes[] = {
> +	{"AIF1TX", NULL, "Codec Input Pin1"},
> +	{"AIF2TX", NULL, "Codec Input Pin2"},
> +
> +	{"Codec Output Pin1", NULL, "AIF1RX"},
> +	{"Codec Output Pin2", NULL, "AIF2RX"},
> +};
> +
> +static const struct snd_soc_dapm_widget hdac_hda_dapm_widgets[] = {
> +
> +	/* Audio Interface */
> +	SND_SOC_DAPM_AIF_IN("AIF1RX", "Analog Codec Playback", 0,
> +							SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_IN("AIF2RX", "Digital Codec Playback", 0,
> +							SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_OUT("AIF1TX", "Analog Codec Capture", 0,
> +							SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_OUT("AIF2TX", "Digital Codec Capture", 0,
> +							SND_SOC_NOPM, 0, 0),
> +
> +	/* Input Pins */
> +	SND_SOC_DAPM_INPUT("Codec Input Pin1"),
> +	SND_SOC_DAPM_INPUT("Codec Input Pin2"),
> +
> +	/* Output Pins */
> +	SND_SOC_DAPM_OUTPUT("Codec Output Pin1"),
> +	SND_SOC_DAPM_OUTPUT("Codec Output Pin2"),
> +};
> +
> +
> +static struct snd_soc_codec_driver hdac_hda_codec = {
> +	.probe		= hdac_hda_codec_probe,
> +	.remove		= hdac_hda_codec_remove,
> +	.idle_bias_off = true,
> +	.component_driver = {
> +		.dapm_widgets           = hdac_hda_dapm_widgets,
> +		.num_dapm_widgets       = ARRAY_SIZE(hdac_hda_dapm_widgets),
> +		.dapm_routes            = hdac_hda_dapm_routes,
> +		.num_dapm_routes        = ARRAY_SIZE(hdac_hda_dapm_routes),
> +	},
> +};
> +
> +static int hdac_hda_dev_probe(struct hdac_device *hdev)
> +{
> +	struct hdac_ext_link *hlink = NULL;
> +	struct hdac_hda_priv *hda_pvt;
> +	int ret;
> +
> +	/* hold the ref while we probe */
> +	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;
> +	}
> +	snd_hdac_ext_bus_link_get(hdev->bus, hlink);
> +
> +	hda_pvt = hdac_to_hda_priv(hdev);
> +	if (hda_pvt == NULL)
> +		return -ENOMEM;
> +
> +	/* ASoC specific initialization */
> +	ret = snd_soc_register_codec(&hdev->dev, &hdac_hda_codec, hdac_hda_dais,
> +					 ARRAY_SIZE(hdac_hda_dais));
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "failed to register HDA codec %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_set_drvdata(&hdev->dev, hda_pvt);
> +	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
> +
> +	return ret;
> +}
> +
> +static int hdac_hda_dev_remove(struct hdac_device *hdev)
> +{
> +	snd_soc_unregister_codec(&hdev->dev);
> +	return 0;
> +}
> +
> +static struct hdac_ext_bus_ops hdac_ops = {
> +	.probe          = hdac_hda_dev_probe,
> +	.remove         = hdac_hda_dev_remove,
> +};
> +
> +struct hdac_ext_bus_ops *snd_soc_hdac_hda_get_ops(void)
> +{
> +	return &hdac_ops;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_hdac_hda_get_ops);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("ASoC Extensions for legacy HDA Drivers");
> +MODULE_AUTHOR("Rakesh Ughreja<rakesh.a.ughreja@intel.com>");
> diff --git a/sound/soc/codecs/hdac_hda.h b/sound/soc/codecs/hdac_hda.h
> new file mode 100644
> index 0000000..61ce64a
> --- /dev/null
> +++ b/sound/soc/codecs/hdac_hda.h
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2015-17 Intel Corporation.
> +
> +#ifndef __HDAC_HDA_H__
> +#define __HDAC_HDA_H__
> +
> +struct hdac_hda_pcm {
> +	int stream_tag[2];
> +};
> +
> +struct hdac_hda_priv {
> +	struct hda_codec codec;
> +	struct snd_soc_codec *scodec;
> +	struct hdac_hda_pcm pcm[2];
> +};
> +
> +#define hdac_to_hda_priv(_hdac) \
> +			container_of(_hdac, struct hdac_hda_priv, codec.core)
> +#define hdac_to_hda_codec(_hdac) container_of(_hdac, struct hda_codec, core)
> +
> +struct hdac_ext_bus_ops *snd_soc_hdac_hda_get_ops(void);
> +
> +#endif /* __HDAC_HDA_H__ */
> 

Re: [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@linux.intel.com]
>Sent: Friday, February 23, 2018 10:25 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 8/9] ASoC: hdac_hda: add asoc extension for legacy HDA
>codec drivers
>
>On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>> This patch adds a kernel module which is used by the legacy HDA
>> codec drivers as library. This implements hdac_ext_bus_ops to enable
>> the reuse of legacy HDA codec drivers with ASoC platform drivers.
>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>> ---
>>   sound/pci/hda/hda_bind.c    |   6 +
>>   sound/soc/codecs/Kconfig    |   5 +
>>   sound/soc/codecs/Makefile   |   2 +
>>   sound/soc/codecs/hdac_hda.c | 448
>++++++++++++++++++++++++++++++++++++++++++++
>>   sound/soc/codecs/hdac_hda.h |  23 +++
>>   5 files changed, 484 insertions(+)
>>   create mode 100644 sound/soc/codecs/hdac_hda.c
>>   create mode 100644 sound/soc/codecs/hdac_hda.h
>
>so now we have both hdac_hdmi and hdac_hda?
>Not sure I get it.

hdac_hdmi is the ASoC HDMI driver which exists today. All the
intel ASoC driver which are primarily used for I2S codecs uses it.
I am not deleting or removing the support for that.

hdac_hda is the ASoC wrapper around the legacy HDA drivers.

Now with this patch series, we have two choices for HDMI/iDisp
codec driver. Either to use the legacy HDMI codec driver by using
the ASoC wrapper or use the existing ASoC hdac_hdmi driver.

Since Intel ASoC platform driver is already proven and tested 
with ASoC hdac_hdmi driver, I am using that in this patch series.

Regards,
Rakesh

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

[Pierre-Louis Bossart]

On 2/26/18 1:44 AM, Ughreja, Rakesh A wrote:
> 
> 
>> -----Original Message-----
>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>> Sent: Friday, February 23, 2018 10:25 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 8/9] ASoC: hdac_hda: add asoc extension for legacy HDA
>> codec drivers
>>
>> On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>>> This patch adds a kernel module which is used by the legacy HDA
>>> codec drivers as library. This implements hdac_ext_bus_ops to enable
>>> the reuse of legacy HDA codec drivers with ASoC platform drivers.
>>>
>>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>>> ---
>>>    sound/pci/hda/hda_bind.c    |   6 +
>>>    sound/soc/codecs/Kconfig    |   5 +
>>>    sound/soc/codecs/Makefile   |   2 +
>>>    sound/soc/codecs/hdac_hda.c | 448
>> ++++++++++++++++++++++++++++++++++++++++++++
>>>    sound/soc/codecs/hdac_hda.h |  23 +++
>>>    5 files changed, 484 insertions(+)
>>>    create mode 100644 sound/soc/codecs/hdac_hda.c
>>>    create mode 100644 sound/soc/codecs/hdac_hda.h
>>
>> so now we have both hdac_hdmi and hdac_hda?
>> Not sure I get it.
> 
> hdac_hdmi is the ASoC HDMI driver which exists today. All the
> intel ASoC driver which are primarily used for I2S codecs uses it.
> I am not deleting or removing the support for that.
> 
> hdac_hda is the ASoC wrapper around the legacy HDA drivers.
> 
> Now with this patch series, we have two choices for HDMI/iDisp
> codec driver. Either to use the legacy HDMI codec driver by using
> the ASoC wrapper or use the existing ASoC hdac_hdmi driver.
> 
> Since Intel ASoC platform driver is already proven and tested
> with ASoC hdac_hdmi driver, I am using that in this patch series.

I get your point, but I will assert that the legacy HDMI codec has been 
tested a lot more than the ASoC one (only for Chromebooks) so I wonder 
if we shouldn't deprecate hdac_hdmi moving forward. Having two codec 
implementations which both talk to the i915 driver makes no sense for 
long term support. We don't need to do this now but it should be on the 
TODO list along with topology support in machine drivers.

Re: [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@linux.intel.com]
>Sent: Tuesday, February 27, 2018 12:56 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 8/9] ASoC: hdac_hda: add asoc extension for
>legacy HDA codec drivers
>
>On 2/26/18 1:44 AM, Ughreja, Rakesh A wrote:
>>
>>
>>> -----Original Message-----
>>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>>> Sent: Friday, February 23, 2018 10:25 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 8/9] ASoC: hdac_hda: add asoc extension for legacy
>HDA
>>> codec drivers
>>>
>>> On 2/23/18 2:12 AM, Rakesh Ughreja wrote:
>>>> This patch adds a kernel module which is used by the legacy HDA
>>>> codec drivers as library. This implements hdac_ext_bus_ops to enable
>>>> the reuse of legacy HDA codec drivers with ASoC platform drivers.
>>>>
>>>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>>>> ---
>>>>    sound/pci/hda/hda_bind.c    |   6 +
>>>>    sound/soc/codecs/Kconfig    |   5 +
>>>>    sound/soc/codecs/Makefile   |   2 +
>>>>    sound/soc/codecs/hdac_hda.c | 448
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>    sound/soc/codecs/hdac_hda.h |  23 +++
>>>>    5 files changed, 484 insertions(+)
>>>>    create mode 100644 sound/soc/codecs/hdac_hda.c
>>>>    create mode 100644 sound/soc/codecs/hdac_hda.h
>>>
>>> so now we have both hdac_hdmi and hdac_hda?
>>> Not sure I get it.
>>
>> hdac_hdmi is the ASoC HDMI driver which exists today. All the
>> intel ASoC driver which are primarily used for I2S codecs uses it.
>> I am not deleting or removing the support for that.
>>
>> hdac_hda is the ASoC wrapper around the legacy HDA drivers.
>>
>> Now with this patch series, we have two choices for HDMI/iDisp
>> codec driver. Either to use the legacy HDMI codec driver by using
>> the ASoC wrapper or use the existing ASoC hdac_hdmi driver.
>>
>> Since Intel ASoC platform driver is already proven and tested
>> with ASoC hdac_hdmi driver, I am using that in this patch series.
>
>I get your point, but I will assert that the legacy HDMI codec has been
>tested a lot more than the ASoC one (only for Chromebooks) so I wonder
>if we shouldn't deprecate hdac_hdmi moving forward. Having two codec
>implementations which both talk to the i915 driver makes no sense for
>long term support. We don't need to do this now but it should be on the
>TODO list along with topology support in machine drivers.

Sure, I can add that in the TODO.

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

[Rakesh Ughreja]

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
 	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);
-- 
2.7.4

Re: [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA codecs

[Pierre-Louis Bossart]

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);
> 

Re: [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@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

Re: [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA codecs

[Pierre-Louis Bossart]

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

Re: [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@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

Re: [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA codecs

[Takashi Iwai]

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

Re: [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA codecs

[Ughreja, Rakesh A]

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

Re: [PATCH v1 9/9] ASoC: Intel: Boards: add support for HDA codecs

[Takashi Iwai]

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