[PATCH v4 0/8] Enable HDA Codec support on Intel Platforms

[PATCH v4 0/8] Enable HDA Codec support on Intel Platforms

[Pierre-Louis Bossart]

Many Intel platforms (SKL, KBL) etc. in the market supports enhanced
audio capabilities which also includes DSP processing. The default
HDaudio legacy driver does not allow for the use of the DSP, this
patch set makes it possible while reusing existing code for HDAudio
codecs and without significant changes to the legacy driver.

This v4 is based on Takashi's topic/drm_audio_component merged on top
on Marks' for-next branch - the merge is not included here.

Tests were run successfully on multiple platforms (Dell XPS13, KBL
NUC, APL NUC and LeafHill reference board). Both the HDaudio and HDMI
outputs were tested.

Credits: all the initial code was written by Rakesh Ughreja, the
rebase to broonie/for-next, cleanups and additional tests were done by
Pierre Bossart.

TODO in future update: fix the HDMI jack detection which only works
after the mixer values are set, which isn't practical for headless
devices always connected (this is a problem in the hdac_hdmi codec
that was present before this series was submitted)

Changes v4:
- rebase/update on Takashi's topic/drm_audio_component branch
- changes in the HDaudio detection to avoid adding a fake ACPI ID
- new Kconfigs to control HDaudio detection

Changes v3:
- port to component model
- additional tests on ApolloLake and KabyLake NUC devices
- cleanups (alignment, typos, etc)

Changes v2:
- Resolved review comments and rebased to latest kernel.
- added module load support for codec drivers.

Pierre-Louis Bossart (2):
  ASoC: Intel: common: add table for HDA-based platforms
  ASoC: Intel: Skylake: add option to control HDAudio + DSP usage

Rakesh Ughreja (6):
  ASoC: Intel: Skylake: fix widget handling
  ASoC: Intel: Boards: Machine driver for SKL+ w/ HDAudio codecs
  ASoC: Intel: Skylake: use HDAudio if ACPI enumeration fails
  ASoC: Intel: Skylake: add HDA BE DAIs
  ASoC: Intel: Skylake: use hda_bus instead of hdac_bus
  ASoC: hdac_hda: add asoc extension for legacy HDA codec drivers

 include/sound/soc-acpi-intel-match.h              |   6 +
 sound/pci/hda/hda_bind.c                          |  12 +
 sound/soc/codecs/Kconfig                          |   5 +
 sound/soc/codecs/Makefile                         |   2 +
 sound/soc/codecs/hdac_hda.c                       | 469 ++++++++++++++++++++++
 sound/soc/codecs/hdac_hda.h                       |  24 ++
 sound/soc/intel/Kconfig                           |  18 +
 sound/soc/intel/boards/Kconfig                    |   9 +
 sound/soc/intel/boards/Makefile                   |   2 +
 sound/soc/intel/boards/skl_hda_dsp_common.c       | 133 ++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h       |  37 ++
 sound/soc/intel/boards/skl_hda_dsp_generic.c      | 174 ++++++++
 sound/soc/intel/common/Makefile                   |   3 +-
 sound/soc/intel/common/soc-acpi-intel-hda-match.c |  40 ++
 sound/soc/intel/skylake/skl-pcm.c                 |  70 +++-
 sound/soc/intel/skylake/skl-topology.c            |   3 +
 sound/soc/intel/skylake/skl.c                     | 100 ++++-
 sound/soc/intel/skylake/skl.h                     |  12 +-
 18 files changed, 1093 insertions(+), 26 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
 create mode 100644 sound/soc/intel/common/soc-acpi-intel-hda-match.c

-- 
2.14.1

[PATCH v4 1/8] ASoC: Intel: Skylake: fix widget handling

[Pierre-Louis Bossart]

From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>

include DAPM Mux and output widgets into the list.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 76dde12cc2bb..2620d77729c5 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -108,6 +108,9 @@ static int is_skl_dsp_widget_type(struct snd_soc_dapm_widget *w,
 	case snd_soc_dapm_aif_out:
 	case snd_soc_dapm_dai_out:
 	case snd_soc_dapm_switch:
+	case snd_soc_dapm_output:
+	case snd_soc_dapm_mux:
+
 		return false;
 	default:
 		return true;
-- 
2.14.1

[PATCH v4 2/8] ASoC: Intel: common: add table for HDA-based platforms

[Pierre-Louis Bossart]

Expose a table containing machine driver information for HDAudio-based
platforms handled by ASoC on Intel hardware.

We only set constant values that are valid across multiple
platforms. The firmware name used by the DSP will be set dynamically
for each platform.

The table is made of a single entry for now, if we need more
complicated set-up where HDAudio is mixed with ACPI-enumerated devices
(I2C, SoundWire) then we'd expect the differentiation to be handled
through information provided by the BIOS (as done for KBL
Chromebooks).

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/soc-acpi-intel-match.h              |  6 ++++
 sound/soc/intel/common/Makefile                   |  3 +-
 sound/soc/intel/common/soc-acpi-intel-hda-match.c | 40 +++++++++++++++++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 sound/soc/intel/common/soc-acpi-intel-hda-match.c

diff --git a/include/sound/soc-acpi-intel-match.h b/include/sound/soc-acpi-intel-match.h
index bb1d24b703fb..f48f59e5b7b0 100644
--- a/include/sound/soc-acpi-intel-match.h
+++ b/include/sound/soc-acpi-intel-match.h
@@ -25,4 +25,10 @@ extern struct snd_soc_acpi_mach snd_soc_acpi_intel_bxt_machines[];
 extern struct snd_soc_acpi_mach snd_soc_acpi_intel_glk_machines[];
 extern struct snd_soc_acpi_mach snd_soc_acpi_intel_cnl_machines[];
 
+/*
+ * generic table used for HDA codec-based platforms, possibly with
+ * additional ACPI-enumerated codecs
+ */
+extern struct snd_soc_acpi_mach snd_soc_acpi_intel_hda_machines[];
+
 #endif
diff --git a/sound/soc/intel/common/Makefile b/sound/soc/intel/common/Makefile
index 915a34cdc8ac..c1f50a079d34 100644
--- a/sound/soc/intel/common/Makefile
+++ b/sound/soc/intel/common/Makefile
@@ -7,7 +7,8 @@ snd-soc-acpi-intel-match-objs := soc-acpi-intel-byt-match.o soc-acpi-intel-cht-m
 	soc-acpi-intel-hsw-bdw-match.o \
 	soc-acpi-intel-skl-match.o soc-acpi-intel-kbl-match.o \
 	soc-acpi-intel-bxt-match.o soc-acpi-intel-glk-match.o \
-	soc-acpi-intel-cnl-match.o
+	soc-acpi-intel-cnl-match.o \
+	soc-acpi-intel-hda-match.o
 
 obj-$(CONFIG_SND_SOC_INTEL_SST) += snd-soc-sst-dsp.o snd-soc-sst-ipc.o
 obj-$(CONFIG_SND_SOC_INTEL_SST_ACPI) += snd-soc-sst-acpi.o
diff --git a/sound/soc/intel/common/soc-acpi-intel-hda-match.c b/sound/soc/intel/common/soc-acpi-intel-hda-match.c
new file mode 100644
index 000000000000..40ce8d4e9b22
--- /dev/null
+++ b/sound/soc/intel/common/soc-acpi-intel-hda-match.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * soc-apci-intel-hda-match.c - tables and support for HDA+ACPI enumeration.
+ *
+ * Copyright (c) 2018, Intel Corporation.
+ *
+ */
+
+#include <sound/soc-acpi.h>
+#include <sound/soc-acpi-intel-match.h>
+#include "../skylake/skl.h"
+
+static struct skl_machine_pdata hda_pdata = {
+	.use_tplg_pcm = true,
+};
+
+struct snd_soc_acpi_mach snd_soc_acpi_intel_hda_machines[] = {
+	{
+		/* .id is not used in this file */
+		.drv_name = "skl_hda_dsp_generic",
+
+		/* .fw_filename is dynamically set in skylake driver */
+
+		/* .sof_fw_filename is dynamically set in sof/intel driver */
+
+		.sof_tplg_filename = "intel/sof-hda-generic.tplg",
+
+		/*
+		 * .machine_quirk and .quirk_data are not used here but
+		 * can be used if we need a more complicated machine driver
+		 * combining HDA+other device (e.g. DMIC).
+		 */
+		.pdata = &hda_pdata,
+	},
+	{},
+};
+EXPORT_SYMBOL_GPL(snd_soc_acpi_intel_hda_machines);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel Common ACPI Match module");
-- 
2.14.1

[PATCH v4 3/8] ASoC: Intel: Boards: Machine driver for SKL+ w/ HDAudio codecs

[Pierre-Louis Bossart]

From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>

Add machine driver for Intel platforms (SKL/KBL/BXT/APL) with
HDA and iDisp codecs. This patch adds support for only iDisp (HDMI/DP)
codec. In the following patches support for HDA codecs 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>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/Kconfig               |   8 ++
 sound/soc/intel/boards/Makefile              |   2 +
 sound/soc/intel/boards/skl_hda_dsp_common.c  | 109 +++++++++++++++++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h  |  37 ++++++++
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 136 +++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl.h                |   2 +
 6 files changed, 294 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 cccda87f4b34..0f0d57859555 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -279,6 +279,14 @@ 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/APL with HDA Codecs"
+	select SND_SOC_HDAC_HDMI
+	help
+	  This adds support for ASoC machine driver for Intel platforms
+	  SKL/KBL/BXT/APL with iDisp, HDA audio codecs.
+          Say Y or m if you have such a device. This is a recommended option.
 	  If unsure select "N".
 
 config SND_SOC_INTEL_GLK_RT5682_MAX98357A_MACH
diff --git a/sound/soc/intel/boards/Makefile b/sound/soc/intel/boards/Makefile
index 87ef8b4058e5..6e88373cbe35 100644
--- a/sound/soc/intel/boards/Makefile
+++ b/sound/soc/intel/boards/Makefile
@@ -20,6 +20,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
 
@@ -46,3 +47,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 000000000000..732d2577d64b
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2015-18 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 000000000000..04149b0dcec4
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright(c) 2015-18 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;
+	const char *platform_name;
+};
+
+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 000000000000..afffe82fcbf8
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright(c) 2015-18 Intel Corporation.
+
+/*
+ * Machine Driver for SKL+ platforms with DSP and iDisp, 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 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 = ctx->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,
+	.dapm_routes = skl_hda_map,
+	.add_dai_link = skl_hda_add_dai_link,
+	.fully_routed = true,
+	.late_probe = skl_hda_card_late_probe,
+};
+
+#define IDISP_DAI_COUNT		3
+#define IDISP_ROUTE_COUNT	3
+#define IDISP_CODEC_MASK	0x4
+
+static int skl_hda_fill_card_info(struct skl_machine_pdata *pdata)
+{
+
+	struct snd_soc_card *card = &hda_soc_card;
+	u32 codec_count, codec_mask;
+	int i, num_links, num_route;
+
+	codec_mask = pdata->codec_mask;
+	codec_count = hweight_long(codec_mask);
+
+	if (codec_count == 1 && pdata->codec_mask & IDISP_CODEC_MASK) {
+		num_links = IDISP_DAI_COUNT;
+		num_route = IDISP_ROUTE_COUNT;
+	} else {
+		return -EINVAL;
+	}
+
+	card->num_links = num_links;
+	card->num_dapm_routes = num_route;
+
+	for (i = 0; i < num_links; i++)
+		skl_hda_be_dai_links[i].platform_name = pdata->platform;
+
+	return 0;
+}
+
+static int skl_hda_audio_probe(struct platform_device *pdev)
+{
+	struct skl_machine_pdata *pdata;
+	struct skl_hda_private *ctx;
+	int ret;
+
+	dev_dbg(&pdev->dev, "%s: entry\n", __func__);
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_ATOMIC);
+	if (!ctx)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&ctx->hdmi_pcm_list);
+
+	pdata = dev_get_drvdata(&pdev->dev);
+	if (!pdata)
+		return -EINVAL;
+
+	ret = skl_hda_fill_card_info(pdata);
+	if (ret < 0)
+		return ret;
+
+	ctx->pcm_count = hda_soc_card.num_links;
+	ctx->platform_name = pdata->platform;
+
+	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 struct platform_driver skl_hda_audio = {
+	.probe = skl_hda_audio_probe,
+	.driver = {
+		.name = "skl_hda_dsp_generic",
+		.pm = &snd_soc_pm_ops,
+	},
+};
+
+module_platform_driver(skl_hda_audio)
+
+/* Module information */
+MODULE_DESCRIPTION("SKL/KBL/BXT/APL HDA Generic Machine driver");
+MODULE_AUTHOR("Rakesh Ughreja <rakesh.a.ughreja@intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:skl_hda_dsp_generic");
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index 78aa8bdcb619..4105a9371b64 100644
--- a/sound/soc/intel/skylake/skl.h
+++ b/sound/soc/intel/skylake/skl.h
@@ -117,6 +117,8 @@ 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;
+	u32 codec_mask;
 };
 
 struct skl_dsp_ops {
-- 
2.14.1

[PATCH v4 4/8] ASoC: Intel: Skylake: use HDAudio if ACPI enumeration fails

[Pierre-Louis Bossart]

From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>

When no I2S based codec entries are found in the BIOS, check if there are
any HDA codecs detected on the bus. Based on the number of codecs found
take appropriate action in machine driver. If there are two HDA codecs
i.e. iDisp + HDA found on the bus, register DAIs and DAI links for both.
If only one codec i.e. iDisp is found then load only iDisp machine driver.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/skylake/skl.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index dce649485649..c83194de4aed 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -472,6 +472,25 @@ 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 hdac_bus *bus = skl_to_bus(skl);
+	struct snd_soc_acpi_mach *mach;
+
+	/* check if we have any codecs detected on bus */
+	if (bus->codec_mask == 0)
+		return NULL;
+
+	/* point to common table */
+	mach = snd_soc_acpi_intel_hda_machines;
+
+	/* all entries in the machine table use the same firmware */
+	mach->fw_filename = machines->fw_filename;
+
+	return mach;
+}
+
 static int skl_find_machine(struct skl *skl, void *driver_data)
 {
 	struct hdac_bus *bus = skl_to_bus(skl);
@@ -479,9 +498,13 @@ static int skl_find_machine(struct skl *skl, void *driver_data)
 	struct skl_machine_pdata *pdata;
 
 	mach = snd_soc_acpi_find_machine(mach);
-	if (mach == NULL) {
-		dev_err(bus->dev, "No matching machine driver found\n");
-		return -ENODEV;
+	if (!mach) {
+		dev_dbg(bus->dev, "No matching I2S machine driver found\n");
+		mach = skl_find_hda_machine(skl, driver_data);
+		if (!mach) {
+			dev_err(bus->dev, "No matching machine driver found\n");
+			return -ENODEV;
+		}
 	}
 
 	skl->mach = mach;
@@ -498,8 +521,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;
 
@@ -516,8 +540,12 @@ 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);
+		pdata->codec_mask = bus->codec_mask;
 		dev_set_drvdata(&pdev->dev, mach->pdata);
+	}
 
 	skl->i2s_dev = pdev;
 
-- 
2.14.1

[PATCH v4 5/8] ASoC: Intel: Skylake: add HDA BE DAIs

[Pierre-Louis Bossart]

From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>

Add support for HDA BE DAIs in SKL platform driver.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 70 ++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 12 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 823e39103edd..00b7a91b18c9 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 |
@@ -569,7 +570,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);
@@ -995,21 +999,63 @@ 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 = "Alt Analog CPU DAI",
+	.ops = &skl_link_dai_ops,
+	.playback = {
+		.stream_name = "Alt 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 = "Alt 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.14.1

[PATCH v4 6/8] ASoC: Intel: Skylake: use hda_bus instead of hdac_bus

[Pierre-Louis Bossart]

From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>

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>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.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 c83194de4aed..57af55ca785e 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -36,6 +36,7 @@
 #include "skl.h"
 #include "skl-sst-dsp.h"
 #include "skl-sst-ipc.h"
+#include "../../../pci/hda/hda_codec.h"
 
 /*
  * initialize the PCI registers
@@ -673,7 +674,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);
 
 	hdev = devm_kzalloc(&skl->pci->dev, sizeof(*hdev), GFP_KERNEL);
 	if (!hdev)
@@ -816,6 +817,7 @@ static int skl_create(struct pci_dev *pci,
 {
 	struct skl *skl;
 	struct hdac_bus *bus;
+	struct hda_bus *hbus;
 
 	int err;
 
@@ -831,6 +833,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, NULL);
 	bus->use_posbuf = 1;
@@ -838,6 +841,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 4105a9371b64..803cce47ffb2 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
 
@@ -71,7 +72,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 */
@@ -105,8 +106,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.14.1

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

[Pierre-Louis Bossart]

From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>

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>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/pci/hda/hda_bind.c                     |  12 +
 sound/soc/codecs/Kconfig                     |   5 +
 sound/soc/codecs/Makefile                    |   2 +
 sound/soc/codecs/hdac_hda.c                  | 469 +++++++++++++++++++++++++++
 sound/soc/codecs/hdac_hda.h                  |  24 ++
 sound/soc/intel/boards/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 |  38 +++
 sound/soc/intel/skylake/skl.c                |  47 ++-
 10 files changed, 619 insertions(+), 5 deletions(-)
 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 d361bb77ca00..b1440a6c515f 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -81,6 +81,12 @@ static int hda_codec_driver_probe(struct device *dev)
 	hda_codec_patch_t patch;
 	int err;
 
+	if (codec->bus->core.ext_ops) {
+		if (WARN_ON(!codec->bus->core.ext_ops->hdev_attach))
+			return -EINVAL;
+		return codec->bus->core.ext_ops->hdev_attach(&codec->core);
+	}
+
 	if (WARN_ON(!codec->preset))
 		return -EINVAL;
 
@@ -134,6 +140,12 @@ static int hda_codec_driver_remove(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
+	if (codec->bus->core.ext_ops) {
+		if (WARN_ON(!codec->bus->core.ext_ops->hdev_detach))
+			return -EINVAL;
+		return codec->bus->core.ext_ops->hdev_detach(&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 efb095dbcd71..bf0b949eb7e8 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -82,6 +82,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_ES7241
 	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
@@ -615,6 +616,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 7ae7c85e8219..3046b33ca9d3 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -78,6 +78,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
@@ -338,6 +339,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 000000000000..672af726a2d9
--- /dev/null
+++ b/sound/soc/codecs/hdac_hda.c
@@ -0,0 +1,469 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright(c) 2015-17 Intel Corporation.
+
+/*
+ * hdac_hda.c - ASoC extensions 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 HDAC_ALT_ANALOG_DAI_ID		2
+
+#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,
+	},
+},
+{
+	.id = HDAC_ALT_ANALOG_DAI_ID,
+	.name = "Alt Analog Codec DAI",
+	.ops = &hdac_hda_dai_ops,
+	.playback = {
+		.stream_name	= "Alt Analog Codec Playback",
+		.channels_min	= 1,
+		.channels_max	= 16,
+		.rates		= SNDRV_PCM_RATE_8000_192000,
+		.formats	= STUB_FORMATS,
+		.sig_bits	= 24,
+	},
+	.capture = {
+		.stream_name    = "Alt Analog 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 snd_soc_component *component = dai->component;
+	struct hdac_hda_priv *hda_pvt;
+	struct hdac_hda_pcm *pcm;
+
+	hda_pvt = snd_soc_component_get_drvdata(component);
+	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 snd_soc_component *component = dai->component;
+	struct hdac_hda_priv *hda_pvt;
+	struct hda_pcm_stream *hda_stream;
+	struct hda_pcm *pcm;
+
+	hda_pvt = snd_soc_component_get_drvdata(component);
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (!pcm)
+		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 snd_soc_component *component = dai->component;
+	struct hdac_hda_priv *hda_pvt;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct hdac_device *hdev;
+	struct hda_pcm_stream *hda_stream;
+	unsigned int format_val;
+	struct hda_pcm *pcm;
+	int ret = 0;
+
+	hda_pvt = snd_soc_component_get_drvdata(component);
+	hdev = &hda_pvt->codec.core;
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (!pcm)
+		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 snd_soc_component *component = dai->component;
+	struct hdac_hda_priv *hda_pvt;
+	struct hda_pcm_stream *hda_stream;
+	struct hda_pcm *pcm;
+	int ret = -ENODEV;
+
+	hda_pvt = snd_soc_component_get_drvdata(component);
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (!pcm)
+		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 snd_soc_component *component = dai->component;
+	struct hdac_hda_priv *hda_pvt;
+	struct hda_pcm_stream *hda_stream;
+	struct hda_pcm *pcm;
+
+	hda_pvt = snd_soc_component_get_drvdata(component);
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (!pcm)
+		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 if (dai->id == HDAC_DIGITAL_DAI_ID)
+		pcm_name = "Digital";
+	else
+		pcm_name = "Alt Analog";
+
+	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_component *component)
+{
+	struct hdac_hda_priv *hda_pvt =
+			snd_soc_component_get_drvdata(component);
+	struct snd_soc_dapm_context *dapm =
+			snd_soc_component_get_dapm(component);
+	struct hdac_device *hdev = &hda_pvt->codec.core;
+	struct hda_codec *hcodec = &hda_pvt->codec;
+	struct hdac_ext_link *hlink;
+	hda_codec_patch_t patch;
+	int ret;
+
+	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);
+
+	ret = snd_hda_codec_device_new(hcodec->bus,
+			component->card->snd_card, hdev->addr, hcodec);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "failed to create hda codec %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * snd_hda_codec_device_new decrements the usage count so call get pm
+	 * else the device will be powered off
+	 */
+	pm_runtime_get_noresume(&hdev->dev);
+
+	hcodec->bus->card = dapm->card->snd_card;
+
+	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "name failed %s\n", hcodec->preset->name);
+		return ret;
+	}
+
+	ret = snd_hdac_regmap_init(&hcodec->core);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "regmap init failed\n");
+		return ret;
+	}
+
+	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
+	if (patch) {
+		ret = patch(hcodec);
+		if (ret < 0) {
+			dev_err(&hdev->dev, "patch failed %d\n", ret);
+			return ret;
+		}
+	} else {
+		dev_dbg(&hdev->dev, "no patch file found\n");
+	}
+
+	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 void hdac_hda_codec_remove(struct snd_soc_component *component)
+{
+	struct hdac_hda_priv *hda_pvt =
+		      snd_soc_component_get_drvdata(component);
+	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;
+	}
+
+	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
+	pm_runtime_disable(&hdev->dev);
+}
+
+static const struct snd_soc_dapm_route hdac_hda_dapm_routes[] = {
+	{"AIF1TX", NULL, "Codec Input Pin1"},
+	{"AIF2TX", NULL, "Codec Input Pin2"},
+	{"AIF3TX", NULL, "Codec Input Pin3"},
+
+	{"Codec Output Pin1", NULL, "AIF1RX"},
+	{"Codec Output Pin2", NULL, "AIF2RX"},
+	{"Codec Output Pin3", NULL, "AIF3RX"},
+};
+
+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_IN("AIF3RX", "Alt Analog 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),
+	SND_SOC_DAPM_AIF_OUT("AIF3TX", "Alt Analog Codec Capture", 0,
+			     SND_SOC_NOPM, 0, 0),
+
+	/* Input Pins */
+	SND_SOC_DAPM_INPUT("Codec Input Pin1"),
+	SND_SOC_DAPM_INPUT("Codec Input Pin2"),
+	SND_SOC_DAPM_INPUT("Codec Input Pin3"),
+
+	/* Output Pins */
+	SND_SOC_DAPM_OUTPUT("Codec Output Pin1"),
+	SND_SOC_DAPM_OUTPUT("Codec Output Pin2"),
+	SND_SOC_DAPM_OUTPUT("Codec Output Pin3"),
+};
+
+static const struct snd_soc_component_driver hdac_hda_codec = {
+	.probe		= hdac_hda_codec_probe,
+	.remove		= hdac_hda_codec_remove,
+	.idle_bias_on	= false,
+	.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;
+	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)
+		return -ENOMEM;
+
+	/* ASoC specific initialization */
+	ret = snd_soc_register_component(&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_component(&hdev->dev);
+	return 0;
+}
+
+static struct hdac_ext_bus_ops hdac_ops = {
+	.hdev_attach = hdac_hda_dev_probe,
+	.hdev_detach = 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 000000000000..d0e440f546b9
--- /dev/null
+++ b/sound/soc/codecs/hdac_hda.h
@@ -0,0 +1,24 @@
+/* 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 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__ */
diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index 0f0d57859555..88e4b4284738 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -283,6 +283,7 @@ config SND_SOC_INTEL_KBL_DA7219_MAX98357A_MACH
 config SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
 	tristate "SKL/KBL/BXT/APL with HDA Codecs"
 	select SND_SOC_HDAC_HDMI
+	select SND_SOC_HDAC_HDA
 	help
 	  This adds support for ASoC machine driver for Intel platforms
 	  SKL/KBL/BXT/APL with iDisp, HDA audio codecs.
diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
index 732d2577d64b..145b1bd2fa28 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
@@ -75,6 +75,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 04149b0dcec4..99bf47571c6f 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.h
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
@@ -15,7 +15,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 afffe82fcbf8..0d06b4703b87 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -16,6 +16,15 @@
 #include "../skylake/skl.h"
 #include "skl_hda_dsp_common.h"
 
+static const struct snd_soc_dapm_widget skl_hda_widgets[] = {
+	SND_SOC_DAPM_HP("Analog Out", NULL),
+	SND_SOC_DAPM_MIC("Analog In", NULL),
+	SND_SOC_DAPM_HP("Alt Analog Out", NULL),
+	SND_SOC_DAPM_MIC("Alt Analog In", NULL),
+	SND_SOC_DAPM_SPK("Digital Out", NULL),
+	SND_SOC_DAPM_MIC("Digital In", NULL),
+};
+
 static const struct snd_soc_dapm_route skl_hda_map[] = {
 
 	{ "hifi3", NULL, "iDisp3 Tx"},
@@ -24,6 +33,29 @@ static const struct snd_soc_dapm_route skl_hda_map[] = {
 	{ "iDisp2 Tx", NULL, "iDisp2_out"},
 	{ "hifi1", NULL, "iDisp1 Tx"},
 	{ "iDisp1 Tx", NULL, "iDisp1_out"},
+
+	{ "Analog Out", NULL, "Codec Output Pin1" },
+	{ "Digital Out", NULL, "Codec Output Pin2" },
+	{ "Alt Analog Out", NULL, "Codec Output Pin3" },
+
+	{ "Codec Input Pin1", NULL, "Analog In" },
+	{ "Codec Input Pin2", NULL, "Digital In" },
+	{ "Codec Input Pin3", NULL, "Alt Analog In" },
+
+	/* CODEC BE connections */
+	{ "Analog Codec Playback", NULL, "Analog CPU Playback" },
+	{ "Analog CPU Playback", NULL, "codec0_out" },
+	{ "Digital Codec Playback", NULL, "Digital CPU Playback" },
+	{ "Digital CPU Playback", NULL, "codec1_out" },
+	{ "Alt Analog Codec Playback", NULL, "Alt Analog CPU Playback" },
+	{ "Alt Analog CPU Playback", NULL, "codec2_out" },
+
+	{ "codec0_in", NULL, "Analog CPU Capture" },
+	{ "Analog CPU Capture", NULL, "Analog Codec Capture" },
+	{ "codec1_in", NULL, "Digital CPU Capture" },
+	{ "Digital CPU Capture", NULL, "Digital Codec Capture" },
+	{ "codec2_in", NULL, "Alt Analog CPU Capture" },
+	{ "Alt Analog CPU Capture", NULL, "Alt Analog Codec Capture" },
 };
 
 static int skl_hda_card_late_probe(struct snd_soc_card *card)
@@ -52,6 +84,7 @@ static struct snd_soc_card hda_soc_card = {
 	.name = "skl_hda_card",
 	.owner = THIS_MODULE,
 	.dai_link = skl_hda_be_dai_links,
+	.dapm_widgets = skl_hda_widgets,
 	.dapm_routes = skl_hda_map,
 	.add_dai_link = skl_hda_add_dai_link,
 	.fully_routed = true,
@@ -75,6 +108,11 @@ static int skl_hda_fill_card_info(struct skl_machine_pdata *pdata)
 	if (codec_count == 1 && pdata->codec_mask & IDISP_CODEC_MASK) {
 		num_links = IDISP_DAI_COUNT;
 		num_route = IDISP_ROUTE_COUNT;
+	} else if (codec_count == 2 && codec_mask & IDISP_CODEC_MASK) {
+		num_links = ARRAY_SIZE(skl_hda_be_dai_links);
+		num_route = ARRAY_SIZE(skl_hda_map),
+		card->dapm_widgets = skl_hda_widgets;
+		card->num_dapm_widgets = ARRAY_SIZE(skl_hda_widgets);
 	} else {
 		return -EINVAL;
 	}
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 57af55ca785e..e8bf46cfea45 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -37,6 +37,7 @@
 #include "skl-sst-dsp.h"
 #include "skl-sst-ipc.h"
 #include "../../../pci/hda/hda_codec.h"
+#include "../../../soc/codecs/hdac_hda.h"
 
 /*
  * initialize the PCI registers
@@ -657,6 +658,24 @@ static void skl_clock_device_unregister(struct skl *skl)
 		platform_device_unregister(skl->clk_dev);
 }
 
+#define IDISP_INTEL_VENDOR_ID	0x80860000
+
+/*
+ * load the legacy codec driver
+ */
+static void load_codec_module(struct hda_codec *codec)
+{
+#ifdef MODULE
+	char modalias[MODULE_NAME_LEN];
+	const char *mod = NULL;
+
+	snd_hdac_codec_modalias(&codec->core, modalias, sizeof(modalias));
+	mod = modalias;
+	dev_dbg(&codec->core.dev, "loading %s codec module\n", mod);
+	request_module(mod);
+#endif
+}
+
 /*
  * Probe the given codec address
  */
@@ -666,7 +685,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);
@@ -676,11 +697,24 @@ 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;
+
+	/* use legacy bus only for HDA codecs, idisp uses ext bus */
+	if ((res & 0xFFFF0000) != IDISP_INTEL_VENDOR_ID) {
+		hdev->type = HDA_DEV_LEGACY;
+		load_codec_module(&hda_codec->codec);
+	}
+	return 0;
 }
 
 /* Codec initialization */
@@ -815,6 +849,7 @@ static int skl_create(struct pci_dev *pci,
 		      const struct hdac_io_ops *io_ops,
 		      struct skl **rskl)
 {
+	struct hdac_ext_bus_ops *ext_ops;
 	struct skl *skl;
 	struct hdac_bus *bus;
 	struct hda_bus *hbus;
@@ -835,7 +870,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.14.1

[PATCH v4 8/8] ASoC: Intel: Skylake: add option to control HDAudio + DSP usage

[Pierre-Louis Bossart]

Add two options to explicitly enable HDAudio + DSP usage and force its
use

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/Kconfig       | 18 ++++++++++++++++++
 sound/soc/intel/skylake/skl.c |  7 ++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 0caa1f4eb94d..494cc1015207 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -117,6 +117,24 @@ config SND_SOC_INTEL_SKYLAKE
 	  GeminiLake or CannonLake platform with the DSP enabled in the BIOS
 	  then enable this option by saying Y or m.
 
+config SND_SOC_INTEL_SKYLAKE_HDA_DSP
+	tristate "Enable HDA+DSP support"
+	depends on SND_SOC_INTEL_SKYLAKE
+	help
+	  If you have a Intel Skylake+ platform with the DSP enabled in the BIOS,
+	  and an HDAudio codec, the enable this option by saying Y or m.
+	  This option will only have an effect if there are no ACPI-enumerated audio
+	  devices (I2C, SoundWire).
+
+config SND_SOC_INTEL_SKYLAKE_FORCE_HDA_DSP
+	tristate "Force HDA+DSP support"
+	depends on SND_SOC_INTEL_SKYLAKE_HDA_DSP
+	help
+	  If you have a Intel Skylake+ platform with the DSP enabled in the BIOS,
+	  and an HDAudio codec, the enable this option by saying Y or m.
+	  This option will ignore information from the BIOS and force the use of the
+	  HDaudio codec, if present. This is a debug option not recommended for distros.
+
 config SND_SOC_ACPI_INTEL_MATCH
 	tristate
 	select SND_SOC_ACPI if ACPI
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index e8bf46cfea45..b14a47e765de 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -474,6 +474,7 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
 						{.name = "ssp5_sclkfs"},
 };
 
+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDA_DSP)
 static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl,
 					struct snd_soc_acpi_mach *machines)
 {
@@ -492,6 +493,7 @@ static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl,
 
 	return mach;
 }
+#endif
 
 static int skl_find_machine(struct skl *skl, void *driver_data)
 {
@@ -500,13 +502,16 @@ static int skl_find_machine(struct skl *skl, void *driver_data)
 	struct skl_machine_pdata *pdata;
 
 	mach = snd_soc_acpi_find_machine(mach);
-	if (!mach) {
+	if (!mach || IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_FORCE_HDA_DSP)) {
 		dev_dbg(bus->dev, "No matching I2S machine driver found\n");
+#if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDA_DSP)
 		mach = skl_find_hda_machine(skl, driver_data);
+#endif
 		if (!mach) {
 			dev_err(bus->dev, "No matching machine driver found\n");
 			return -ENODEV;
 		}
+
 	}
 
 	skl->mach = mach;
-- 
2.14.1

Re: [PATCH v4 1/8] ASoC: Intel: Skylake: fix widget handling

[Vinod]

On 24-07-18, 19:50, Pierre-Louis Bossart wrote:
> From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> 
> include DAPM Mux and output widgets into the list.

Pierre,

Curious how is this a fix?

> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/intel/skylake/skl-topology.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> index 76dde12cc2bb..2620d77729c5 100644
> --- a/sound/soc/intel/skylake/skl-topology.c
> +++ b/sound/soc/intel/skylake/skl-topology.c
> @@ -108,6 +108,9 @@ static int is_skl_dsp_widget_type(struct snd_soc_dapm_widget *w,
>  	case snd_soc_dapm_aif_out:
>  	case snd_soc_dapm_dai_out:
>  	case snd_soc_dapm_switch:
> +	case snd_soc_dapm_output:
> +	case snd_soc_dapm_mux:
> +
>  		return false;
>  	default:
>  		return true;
> -- 
> 2.14.1

-- 
~Vinod

Re: [PATCH v4 3/8] ASoC: Intel: Boards: Machine driver for SKL+ w/ HDAudio codecs

[Vinod]

On 24-07-18, 19:50, Pierre-Louis Bossart wrote:

> +/* 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",

you are setting this one in skl_hda_fill_card_info() so this is
superfluous

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

shouldn't this be queried. not all will have 3 links

> +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];

many people prefer inverted christmas tree for these..

> +static struct platform_driver skl_hda_audio = {
> +	.probe = skl_hda_audio_probe,
> +	.driver = {
> +		.name = "skl_hda_dsp_generic",

who creates this pdev, is it the board details (mach name?)

-- 
~Vinod

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

[Vinod]

On 24-07-18, 19:50, Pierre-Louis Bossart wrote:

> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2015-17 Intel Corporation.

17..?

this style is fine, so is the one on other patches but then these two
are different, so please stick to one style which you find better. FWIW
I would prefer it this way.

> +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 snd_soc_component *component = dai->component;
> +	struct hdac_hda_priv *hda_pvt;
> +	struct hdac_hda_pcm *pcm;
> +
> +	hda_pvt = snd_soc_component_get_drvdata(component);

cant this lookup fail, would make sense to error check here

> +static int hdac_hda_dai_hw_free(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct hdac_hda_priv *hda_pvt;
> +	struct hda_pcm_stream *hda_stream;
> +	struct hda_pcm *pcm;
> +
> +	hda_pvt = snd_soc_component_get_drvdata(component);

here as well..

> +static int hdac_hda_dai_prepare(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct hdac_hda_priv *hda_pvt;
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct hdac_device *hdev;
> +	struct hda_pcm_stream *hda_stream;
> +	unsigned int format_val;
> +	struct hda_pcm *pcm;
> +	int ret = 0;
> +
> +	hda_pvt = snd_soc_component_get_drvdata(component);
> +	hdev = &hda_pvt->codec.core;
> +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
> +	if (!pcm)
> +		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;

this can be:

        if (ret < 0)
                dev_err()

        return ret;

> +static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct hdac_hda_priv *hda_pvt;
> +	struct hda_pcm_stream *hda_stream;
> +	struct hda_pcm *pcm;
> +	int ret = -ENODEV;
> +
> +	hda_pvt = snd_soc_component_get_drvdata(component);
> +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
> +	if (!pcm)
> +		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);

are the ops not mandatory? Do you expect them to not be present?

> +static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *dai)

aligning second line the opening brace of former will help readability

> +static struct hda_pcm *snd_soc_find_pcm_from_dai(struct hdac_hda_priv *hda_pvt,
> +						 struct snd_soc_dai *dai)

align here too please

> +{
> +	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 if (dai->id == HDAC_DIGITAL_DAI_ID)
> +		pcm_name = "Digital";
> +	else
> +		pcm_name = "Alt Analog";

switch?

> +static int hdac_hda_codec_probe(struct snd_soc_component *component)
> +{
> +	struct hdac_hda_priv *hda_pvt =
> +			snd_soc_component_get_drvdata(component);
> +	struct snd_soc_dapm_context *dapm =
> +			snd_soc_component_get_dapm(component);
> +	struct hdac_device *hdev = &hda_pvt->codec.core;
> +	struct hda_codec *hcodec = &hda_pvt->codec;
> +	struct hdac_ext_link *hlink;
> +	hda_codec_patch_t patch;
> +	int ret;
> +
> +	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);

snd_hdac_ext_bus_get_link and snd_hdac_ext_bus_link_get, yeah naming is
hard ;-)

> +
> +	ret = snd_hda_codec_device_new(hcodec->bus,
> +			component->card->snd_card, hdev->addr, hcodec);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "failed to create hda codec %d\n", ret);

you should drop the link reference grabbed in previous calls in error
here and rest of error returns?

> +		return ret;
> +	}
> +
> +	/*
> +	 * snd_hda_codec_device_new decrements the usage count so call get pm
> +	 * else the device will be powered off
> +	 */
> +	pm_runtime_get_noresume(&hdev->dev);

no error check for this?

> +static const struct snd_soc_component_driver hdac_hda_codec = {
> +	.probe		= hdac_hda_codec_probe,
> +	.remove		= hdac_hda_codec_remove,
> +	.idle_bias_on	= false,
> +	.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),

is it diff or code shows table misaligned..?

> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright(c) 2015-17 Intel Corporation.
> + */

same patch different style ;/

> +static void load_codec_module(struct hda_codec *codec)
> +{
> +#ifdef MODULE
> +	char modalias[MODULE_NAME_LEN];
> +	const char *mod = NULL;
> +
> +	snd_hdac_codec_modalias(&codec->core, modalias, sizeof(modalias));
> +	mod = modalias;
> +	dev_dbg(&codec->core.dev, "loading %s codec module\n", mod);
> +	request_module(mod);

any reason why we dont want to use standard loading mechanisms for
these?

-- 
~Vinod

Re: [PATCH v4 1/8] ASoC: Intel: Skylake: fix widget handling

[Pierre-Louis Bossart]

On 7/25/18 5:59 AM, Vinod wrote:
> On 24-07-18, 19:50, Pierre-Louis Bossart wrote:
>> From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>>
>> include DAPM Mux and output widgets into the list.
> 
> Pierre,
> 
> Curious how is this a fix?

This is Skylake-specific topology black magic that you are probably 
better equiped to understand that me... This just takes out the outputs 
and mux from a number of topology handling cases.

> 
>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   sound/soc/intel/skylake/skl-topology.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
>> index 76dde12cc2bb..2620d77729c5 100644
>> --- a/sound/soc/intel/skylake/skl-topology.c
>> +++ b/sound/soc/intel/skylake/skl-topology.c
>> @@ -108,6 +108,9 @@ static int is_skl_dsp_widget_type(struct snd_soc_dapm_widget *w,
>>   	case snd_soc_dapm_aif_out:
>>   	case snd_soc_dapm_dai_out:
>>   	case snd_soc_dapm_switch:
>> +	case snd_soc_dapm_output:
>> +	case snd_soc_dapm_mux:
>> +
>>   		return false;
>>   	default:
>>   		return true;
>> -- 
>> 2.14.1
> 

Re: [PATCH v4 3/8] ASoC: Intel: Boards: Machine driver for SKL+ w/ HDAudio codecs

[Pierre-Louis Bossart]

On 7/25/18 6:11 AM, Vinod wrote:
> On 24-07-18, 19:50, Pierre-Louis Bossart wrote:
> 
>> +/* 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",
> 
> you are setting this one in skl_hda_fill_card_info() so this is
> superfluous

agree, will remove.

> 
>> +		.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,
> 
> shouldn't this be queried. not all will have 3 links

Not that I know of. I've always seen SKL+ with 3 audio streams to iDisp.

>> +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];
> 
> many people prefer inverted christmas tree for these..

yes, will fix.

> 
>> +static struct platform_driver skl_hda_audio = {
>> +	.probe = skl_hda_audio_probe,
>> +	.driver = {
>> +		.name = "skl_hda_dsp_generic",
> 
> who creates this pdev, is it the board details (mach name?)

Not sure I understand your question, this part is similar to all other 
Intel machine drivers and the way by with the probe happens is also 
similar. There is nothing new here, the skylake platform driver finds an 
entry in a table, gets the driver name and creates the relevant device.

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

[Pierre-Louis Bossart]

On 7/25/18 6:47 AM, Vinod wrote:
> On 24-07-18, 19:50, Pierre-Louis Bossart wrote:
> 
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright(c) 2015-17 Intel Corporation.
> 
> 17..?
> 
> this style is fine, so is the one on other patches but then these two
> are different, so please stick to one style which you find better. FWIW
> I would prefer it this way.

ok.

> 
>> +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 snd_soc_component *component = dai->component;
>> +	struct hdac_hda_priv *hda_pvt;
>> +	struct hdac_hda_pcm *pcm;
>> +
>> +	hda_pvt = snd_soc_component_get_drvdata(component);
> 
> cant this lookup fail, would make sense to error check here

There are multiple cases in e.g. hdac_hdmi where we don't to a check 
when getting the drvdata. Not sure if it's necessary given that this is 
used in a callback so the odds of having a bad parameters are quite low.

> 
>> +static int hdac_hda_dai_hw_free(struct snd_pcm_substream *substream,
>> +				struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_component *component = dai->component;
>> +	struct hdac_hda_priv *hda_pvt;
>> +	struct hda_pcm_stream *hda_stream;
>> +	struct hda_pcm *pcm;
>> +
>> +	hda_pvt = snd_soc_component_get_drvdata(component);
> 
> here as well..
> 
>> +static int hdac_hda_dai_prepare(struct snd_pcm_substream *substream,
>> +				struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_component *component = dai->component;
>> +	struct hdac_hda_priv *hda_pvt;
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +	struct hdac_device *hdev;
>> +	struct hda_pcm_stream *hda_stream;
>> +	unsigned int format_val;
>> +	struct hda_pcm *pcm;
>> +	int ret = 0;
>> +
>> +	hda_pvt = snd_soc_component_get_drvdata(component);
>> +	hdev = &hda_pvt->codec.core;
>> +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
>> +	if (!pcm)
>> +		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;
> 
> this can be:
> 
>          if (ret < 0)
>                  dev_err()
> 
>          return ret;

Yes will fix.

> 
>> +static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
>> +			     struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_component *component = dai->component;
>> +	struct hdac_hda_priv *hda_pvt;
>> +	struct hda_pcm_stream *hda_stream;
>> +	struct hda_pcm *pcm;
>> +	int ret = -ENODEV;
>> +
>> +	hda_pvt = snd_soc_component_get_drvdata(component);
>> +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
>> +	if (!pcm)
>> +		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);
> 
> are the ops not mandatory? Do you expect them to not be present?

I believe they are required and there should an error flags otherwise.
> 
>> +static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
>> +			       struct snd_soc_dai *dai)
> 
> aligning second line the opening brace of former will help readability
> 
>> +static struct hda_pcm *snd_soc_find_pcm_from_dai(struct hdac_hda_priv *hda_pvt,
>> +						 struct snd_soc_dai *dai)
> 
> align here too please

Yes. it feels like payback for my SoundWire comments :-)

> 
>> +{
>> +	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 if (dai->id == HDAC_DIGITAL_DAI_ID)
>> +		pcm_name = "Digital";
>> +	else
>> +		pcm_name = "Alt Analog";
> 
> switch?

Yes, this can be done in a nicer way.

> 
>> +static int hdac_hda_codec_probe(struct snd_soc_component *component)
>> +{
>> +	struct hdac_hda_priv *hda_pvt =
>> +			snd_soc_component_get_drvdata(component);
>> +	struct snd_soc_dapm_context *dapm =
>> +			snd_soc_component_get_dapm(component);
>> +	struct hdac_device *hdev = &hda_pvt->codec.core;
>> +	struct hda_codec *hcodec = &hda_pvt->codec;
>> +	struct hdac_ext_link *hlink;
>> +	hda_codec_patch_t patch;
>> +	int ret;
>> +
>> +	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);
> 
> snd_hdac_ext_bus_get_link and snd_hdac_ext_bus_link_get, yeah naming is
> hard ;-)

Yeah. I am not happy about this one, but this is a separate problem.

> 
>> +
>> +	ret = snd_hda_codec_device_new(hcodec->bus,
>> +			component->card->snd_card, hdev->addr, hcodec);
>> +	if (ret < 0) {
>> +		dev_err(&hdev->dev, "failed to create hda codec %d\n", ret);
> 
> you should drop the link reference grabbed in previous calls in error
> here and rest of error returns?

did you mean add a link_put() ? I didn't pay attention to this part and 
indeed this needs more work.

> 
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * snd_hda_codec_device_new decrements the usage count so call get pm
>> +	 * else the device will be powered off
>> +	 */
>> +	pm_runtime_get_noresume(&hdev->dev);
> 
> no error check for this?

I don't see any error checks for this function in the entire kernel 
tree? It returns a void btw so not sure what you were asking for here.

> 
>> +static const struct snd_soc_component_driver hdac_hda_codec = {
>> +	.probe		= hdac_hda_codec_probe,
>> +	.remove		= hdac_hda_codec_remove,
>> +	.idle_bias_on	= false,
>> +	.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),
> 
> is it diff or code shows table misaligned..?

I'll fix this.

> 
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright(c) 2015-17 Intel Corporation.
>> + */
> 
> same patch different style ;/

ok. Checkpatch reports different things for SPDX but I'll align on the 
C++ version.

> 
>> +static void load_codec_module(struct hda_codec *codec)
>> +{
>> +#ifdef MODULE
>> +	char modalias[MODULE_NAME_LEN];
>> +	const char *mod = NULL;
>> +
>> +	snd_hdac_codec_modalias(&codec->core, modalias, sizeof(modalias));
>> +	mod = modalias;
>> +	dev_dbg(&codec->core.dev, "loading %s codec module\n", mod);
>> +	request_module(mod);
> 
> any reason why we dont want to use standard loading mechanisms for
> these?

what standard loading mechanisms are you referring to here? There is no 
ACPI doing the work for us.

Thanks for the reviews!

Re: [PATCH v4 1/8] ASoC: Intel: Skylake: fix widget handling

[Vinod]

On 25-07-18, 09:04, Pierre-Louis Bossart wrote:
> On 7/25/18 5:59 AM, Vinod wrote:
> > On 24-07-18, 19:50, Pierre-Louis Bossart wrote:
> > > From: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> > > 
> > > include DAPM Mux and output widgets into the list.
> > 
> > Pierre,
> > 
> > Curious how is this a fix?
> 
> This is Skylake-specific topology black magic that you are probably better
> equiped to understand that me... This just takes out the outputs and mux
> from a number of topology handling cases.

:D

I agree with the change. But it is not a fix.. So "Add additional widget
handling" would be a better title.

Fix is used for bugs and should be applied to next -rc.

-- 
~Vinod

Re: [PATCH v4 3/8] ASoC: Intel: Boards: Machine driver for SKL+ w/ HDAudio codecs

[Vinod]

On 25-07-18, 09:10, Pierre-Louis Bossart wrote:
> On 7/25/18 6:11 AM, Vinod wrote:

> > > +		.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,
> > 
> > shouldn't this be queried. not all will have 3 links
> 
> Not that I know of. I've always seen SKL+ with 3 audio streams to iDisp.

IIRC later ones (after KBL) have 5 CVTs and 4 pins (or vice-versa)

> > > +static struct platform_driver skl_hda_audio = {
> > > +	.probe = skl_hda_audio_probe,
> > > +	.driver = {
> > > +		.name = "skl_hda_dsp_generic",
> > 
> > who creates this pdev, is it the board details (mach name?)
> 
> Not sure I understand your question, this part is similar to all other Intel
> machine drivers and the way by with the probe happens is also similar. There
> is nothing new here, the skylake platform driver finds an entry in a table,
> gets the driver name and creates the relevant device.

Okay sounds good. I was curious if the generic machine device is created
generically or use the tables..

-- 
~Vinod

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

[Vinod]

On 25-07-18, 09:27, Pierre-Louis Bossart wrote:
> On 7/25/18 6:47 AM, Vinod wrote:

> > > +static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
> > > +			     struct snd_soc_dai *dai)
> > > +{
> > > +	struct snd_soc_component *component = dai->component;
> > > +	struct hdac_hda_priv *hda_pvt;
> > > +	struct hda_pcm_stream *hda_stream;
> > > +	struct hda_pcm *pcm;
> > > +	int ret = -ENODEV;
> > > +
> > > +	hda_pvt = snd_soc_component_get_drvdata(component);
> > > +	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
> > > +	if (!pcm)
> > > +		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);
> > 
> > are the ops not mandatory? Do you expect them to not be present?
> 
> I believe they are required and there should an error flags otherwise.

if they are required then check can be skipped

> > > +static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
> > > +			       struct snd_soc_dai *dai)
> > 
> > aligning second line the opening brace of former will help readability
> > 
> > > +static struct hda_pcm *snd_soc_find_pcm_from_dai(struct hdac_hda_priv *hda_pvt,
> > > +						 struct snd_soc_dai *dai)
> > 
> > align here too please
> 
> Yes. it feels like payback for my SoundWire comments :-)

Well it's karma :D

> > > +	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);
> > 
> > snd_hdac_ext_bus_get_link and snd_hdac_ext_bus_link_get, yeah naming is
> > hard ;-)
> 
> Yeah. I am not happy about this one, but this is a separate problem.

Agreed and maybe that should be fixed too ;-)

> > > +	ret = snd_hda_codec_device_new(hcodec->bus,
> > > +			component->card->snd_card, hdev->addr, hcodec);
> > > +	if (ret < 0) {
> > > +		dev_err(&hdev->dev, "failed to create hda codec %d\n", ret);
> > 
> > you should drop the link reference grabbed in previous calls in error
> > here and rest of error returns?
> 
> did you mean add a link_put() ? I didn't pay attention to this part and
> indeed this needs more work.

Yes on err link_put() should be invoked. The link refcount will go for a
toss otherwise

> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright(c) 2015-17 Intel Corporation.
> > > + */
> > 
> > same patch different style ;/
> 
> ok. Checkpatch reports different things for SPDX but I'll align on the C++
> version.

Yeah we still have bunch of versions floating out. Only consensus seems
to be for SPDX line in C99 style. So pick one you like and be consistent
with it.

> > > +static void load_codec_module(struct hda_codec *codec)
> > > +{
> > > +#ifdef MODULE
> > > +	char modalias[MODULE_NAME_LEN];
> > > +	const char *mod = NULL;
> > > +
> > > +	snd_hdac_codec_modalias(&codec->core, modalias, sizeof(modalias));
> > > +	mod = modalias;
> > > +	dev_dbg(&codec->core.dev, "loading %s codec module\n", mod);
> > > +	request_module(mod);
> > 
> > any reason why we dont want to use standard loading mechanisms for
> > these?
> 
> what standard loading mechanisms are you referring to here? There is no ACPI
> doing the work for us.

Standard linux driver loading. udev loads the driver when the device is
created and we match. Like the one we do for soundwire, hdmi (ext hda)
etc.

-- 
~Vinod

Re: [PATCH v4 1/8] ASoC: Intel: Skylake: fix widget handling

[Pierre-Louis Bossart]

>>>> include DAPM Mux and output widgets into the list.
>>>
>>> Pierre,
>>>
>>> Curious how is this a fix?
>>
>> This is Skylake-specific topology black magic that you are probably better
>> equiped to understand that me... This just takes out the outputs and mux
>> from a number of topology handling cases.
> 
> :D
> 
> I agree with the change. But it is not a fix.. So "Add additional widget
> handling" would be a better title.
> 
> Fix is used for bugs and should be applied to next -rc.

ok, will change.

Re: [PATCH v4 3/8] ASoC: Intel: Boards: Machine driver for SKL+ w/ HDAudio codecs

[Pierre-Louis Bossart]

On 7/25/18 11:22 AM, Vinod wrote:
> On 25-07-18, 09:10, Pierre-Louis Bossart wrote:
>> On 7/25/18 6:11 AM, Vinod wrote:
> 
>>>> +		.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,
>>>
>>> shouldn't this be queried. not all will have 3 links
>>
>> Not that I know of. I've always seen SKL+ with 3 audio streams to iDisp.
> 
> IIRC later ones (after KBL) have 5 CVTs and 4 pins (or vice-versa)

Let me check on this.

> 
>>>> +static struct platform_driver skl_hda_audio = {
>>>> +	.probe = skl_hda_audio_probe,
>>>> +	.driver = {
>>>> +		.name = "skl_hda_dsp_generic",
>>>
>>> who creates this pdev, is it the board details (mach name?)
>>
>> Not sure I understand your question, this part is similar to all other Intel
>> machine drivers and the way by with the probe happens is also similar. There
>> is nothing new here, the skylake platform driver finds an entry in a table,
>> gets the driver name and creates the relevant device.
> 
> Okay sounds good. I was curious if the generic machine device is created
> generically or use the tables..

we anticipate that there will be quirks, e.g. a machine with an HDaudio 
codec but DMICs, and the idea is that those quirks would point a 
different table entry (or set a pdata field). For now we haven't done 
any work on those quirks, we'll deal with them as we see new variants.

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 589 bytes --]

On Wed, Jul 25, 2018 at 09:59:14PM +0530, Vinod wrote:
> On 25-07-18, 09:27, Pierre-Louis Bossart wrote:

> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * Copyright(c) 2015-17 Intel Corporation.
> > > > + */

> > > same patch different style ;/

> > ok. Checkpatch reports different things for SPDX but I'll align on the C++
> > version.

> Yeah we still have bunch of versions floating out. Only consensus seems
> to be for SPDX line in C99 style. So pick one you like and be consistent
> with it.

Headers are a C comment as they might be included by non-C stuff.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Pierre-Louis Bossart]

On 07/26/2018 09:58 AM, Mark Brown wrote:
> On Wed, Jul 25, 2018 at 09:59:14PM +0530, Vinod wrote:
>> On 25-07-18, 09:27, Pierre-Louis Bossart wrote:
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +/*
>>>>> + * Copyright(c) 2015-17 Intel Corporation.
>>>>> + */
>>>> same patch different style ;/
>>> ok. Checkpatch reports different things for SPDX but I'll align on the C++
>>> version.
>> Yeah we still have bunch of versions floating out. Only consensus seems
>> to be for SPDX line in C99 style. So pick one you like and be consistent
>> with it.
> Headers are a C comment as they might be included by non-C stuff.
Ah, makes sense. if that's alright I'll use the following style then.

.h files

/* SPDX-License-Identifier: GPL-2.0 */
/*
  * Copyright(c) 2015-17 Intel Corporation.
  */

.c files
// SPDX-License-Identifier: GPL-2.0
/*
  * Copyright(c) 2015-17 Intel Corporation.
  */

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 540 bytes --]

On Thu, Jul 26, 2018 at 10:36:08AM -0500, Pierre-Louis Bossart wrote:

> Ah, makes sense. if that's alright I'll use the following style then.

> .h files
> 
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
>  * Copyright(c) 2015-17 Intel Corporation.
>  */

Yup.

> .c files
> // SPDX-License-Identifier: GPL-2.0
> /*
>  * Copyright(c) 2015-17 Intel Corporation.
>  */

Lots of people are doing that, I do prefer that the entire comment ends
up C++ so it looks a bit more consistent but most people are happy with
that.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]