[RFC v3 00/11] Enable HDA Codec support on Intel Platforms (Series2)

[RFC v3 00/11] 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 works 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.

I would like to receive feedback before proceeding further on this
direction.

INFO:
- This series is tested on KBL based product (Dell XPS 13).
- Basic playback is working with headset and speakers.
- HDMI playback is working.
- Capture operation is not tested.
- Playback using legacy drivers is tested.
- More platforms and use cases coverage can be added once we have basic
  agreement in terms of the overall approach.

Changes in v3:
- snd_hda_codec_new API is split into two, so that it can be used by
  legacy as well as ASoC codec drivers. API is split into two to avoid bus
  registration when called by ASoC drivers.
- Moved the __hda_codec_driver_register and hda_codec_driver_unregister APIs
  into generic driver and it performs registration for ASoC codec drivers
  also. So now there is only one kernel image for ASoC as well as legacy codec
  drivers. Registration functions had to moved to generic driver to resolve the
  circular dependency between hdac_hda and hda_codec drivers.
- There is a Kconfig entry added to compile the ASoC version of HDA codec
  driver. For now only the Realtek codec entry is added. Same thing needs to be
  done for rest of the codec drivers also.

Changes in v2:
- Using Topology framework to create DAIs and DAI Links
- Moved most commonly used functions into a separate file for machine driver
- Implemented separate Realtek ASoC HDA Driver driver for Realtek HDA codecs.
  It is just a driver registration wrapper which reuses the legacy HDA codec
  driver. This allows the removes the limitation that was present in v1.
- Implemented the review feedback.

Rakesh Ughreja (11):
  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 - make some of the functions externally visible
  ASoC: hdac_hda: add ASoC based HDA codec driver
  ALSA: hda: split API snd_hda_codec_new for using it from ASoC codec
    drivers
  ASoC: hdac_hda: add DAI, widgets and related ops
  ASoC: hdac_hda: add runtime PM support
  ASoC: codec: Support for ASoC Realtek HDA codec Driver
  ASoC: Intel: Boards: add support for HDA codecs

 include/sound/hdaudio_ext.h                  |   3 +-
 sound/hda/ext/hdac_ext_bus.c                 |   9 +-
 sound/pci/hda/hda_bind.c                     |  10 +-
 sound/pci/hda/hda_codec.c                    |  88 +++-
 sound/pci/hda/hda_codec.h                    |  11 +-
 sound/pci/hda/hda_generic.c                  |  38 ++
 sound/soc/codecs/Kconfig                     |  12 +
 sound/soc/codecs/Makefile                    |   2 +
 sound/soc/codecs/hdac_hda.c                  | 629 +++++++++++++++++++++++++++
 sound/soc/codecs/hdac_hda.h                  |  22 +
 sound/soc/intel/boards/Kconfig               |  11 +
 sound/soc/intel/boards/Makefile              |   2 +
 sound/soc/intel/boards/skl_hda_dsp_common.c  | 173 ++++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h  |  42 ++
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 141 ++++++
 sound/soc/intel/skylake/skl-pcm.c            |  32 +-
 sound/soc/intel/skylake/skl.c                |  79 +++-
 sound/soc/intel/skylake/skl.h                |  10 +-
 18 files changed, 1268 insertions(+), 46 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

[RFC v3 01/11] ASoC: Intel: Boards: Machine driver for Intel platforms

[Rakesh Ughreja]

Add machine driver for Intel platforms(SKL/KBL) 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. BXT,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  | 130 +++++++++++++++++++++++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h  |  42 +++++++++
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 112 +++++++++++++++++++++++
 5 files changed, 296 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 6f75470..e01137d 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -262,4 +262,14 @@ config SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH
           Say Y if you have such a device.
           If unsure select "N".
 
+config SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
+        tristate "ASoC Audio driver for SKL/KBL 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 if you have such a device.
+          If unsure select "N".
 endif
diff --git a/sound/soc/intel/boards/Makefile b/sound/soc/intel/boards/Makefile
index 69d2dfa..7f6ab8e 100644
--- a/sound/soc/intel/boards/Makefile
+++ b/sound/soc/intel/boards/Makefile
@@ -17,6 +17,7 @@ snd-soc-sst-byt-cht-nocodec-objs := bytcht_nocodec.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
 
@@ -40,3 +41,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..5558334
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
@@ -0,0 +1,130 @@
+/*
+ * Intel Machine Driver for SKL/KBL platforms with HDA Codecs
+ *
+ * Copyright (C) 2017, Intel Corporation. All rights reserved.
+ *
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#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
+
+static struct snd_soc_dai *skl_hda_get_codec_dai(struct snd_soc_card *card,
+						     const char *dai_name)
+{
+	struct snd_soc_pcm_runtime *rtd;
+
+	list_for_each_entry(rtd, &card->rtd_list, list) {
+		if (!strcmp(rtd->codec_dai->name, dai_name))
+			return rtd->codec_dai;
+	}
+
+	return NULL;
+}
+
+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 = skl_hda_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_codec *codec = NULL;
+	int err;
+	char jack_name[NAME_SIZE];
+
+	list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) {
+		codec = pcm->codec_dai->codec;
+		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 (!codec)
+		return -EINVAL;
+
+	return hdac_hdmi_jack_port_init(codec, &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..c9cb66b
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
@@ -0,0 +1,42 @@
+/*
+ * Intel Machine Driver for platform with HDA Codecs definitions
+ *
+ * Copyright (C) 2017, Intel Corporation. All rights reserved.
+ *
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __SOUND_SOC_HDA_DSP_COMMON_H
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+#define __SOUND_SOC_HDA_DSP_COMMON_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..2d2b901
--- /dev/null
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -0,0 +1,112 @@
+/*
+ * Intel Machine Driver for SKL/KBL platforms with HDA Codecs
+ *
+ * Copyright (C) 2017, Intel Corporation. All rights reserved.
+ *
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#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 = "0000:00:1f.3";
+	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_hda_private *ctx;
+
+	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);
+
+	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");
-- 
2.7.4

[RFC v3 02/11] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs

[Rakesh Ughreja]

When no I2S based codecs are detected in the BIOS, check if there are
any HDA codecs present. If yes, load the corresponding machine driver.

TODO:
support for detecting the presence of HDA codec is not implemented.
it will be implemented in the next revision.

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

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 6fc2173..b99c1d8 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -442,6 +442,26 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
 						{.name = "ssp5_sclkfs"},
 };
 
+static struct snd_soc_acpi_mach *skl_probe_hda_machine(
+					struct snd_soc_acpi_mach *machines)
+{
+
+	struct snd_soc_acpi_mach *mach;
+
+	/*
+	 * FIXME:
+	 * First check if there are any HDA codecs present on the system
+	 * then search the match table.
+	 * For now this function is not detecting the presence of any
+	 * HDA codecs.
+	 */
+	for (mach = machines; mach->id[0]; mach++) {
+		if (!strcmp(mach->id, "HDA_GEN"))
+			return mach;
+	}
+	return NULL;
+}
+
 static int skl_machine_device_register(struct skl *skl, void *driver_data)
 {
 	struct hdac_bus *bus = skl_to_bus(skl);
@@ -451,9 +471,14 @@ static int skl_machine_device_register(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_probe_hda_machine(driver_data);
+		if (mach == NULL) {
+			dev_err(bus->dev, "No matching machine driver found\n");
+			return -ENODEV;
+		}
 	}
+
 	skl->fw_name = mach->fw_filename;
 
 	pdev = platform_device_alloc(mach->drv_name, -1);
@@ -1005,6 +1030,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,
+	},
 	{}
 };
 
@@ -1067,7 +1100,14 @@ static struct snd_soc_acpi_mach sst_kbl_devdata[] = {
 		.drv_name = "kbl_rt5663",
 		.fw_filename = "intel/dsp_fw_kbl.bin",
 	},
-
+	{
+		.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

[RFC v3 03/11] 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 | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 02cdb1c..6cd6640 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -956,21 +956,39 @@ 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,
+		.stream_name = "Analog CPU Playback",
+		.channels_min = HDA_MONO,
 		.channels_max = HDA_STEREO,
 		.rates = SNDRV_PCM_RATE_48000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
 	},
 	.capture = {
-		.stream_name = "HD-Codec Rx",
-		.channels_min = HDA_STEREO,
+		.stream_name = "Analog CPU Capture",
+		.channels_min = HDA_MONO,
 		.channels_max = HDA_STEREO,
 		.rates = SNDRV_PCM_RATE_48000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+},
+{
+	.name = "Digital CPU DAI",
+	.ops = &skl_link_dai_ops,
+	.playback = {
+		.stream_name = "Digital CPU Playback",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+	.capture = {
+		.stream_name = "Digital CPU Capture",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
 	},
 },
 };
-- 
2.7.4

[RFC v3 04/11] ASoC: Intel: Skylake: use hda_bus instead of hdac_bus

[Rakesh Ughreja]

This patch prepares SKL platform driver to make reuse of legacy HDA
codec drivers. It does following things.

use hda_bus instead of hdac_bus in the SKL platform driver to align
with the legacy controller driver.

modify snd_hdac_ext_bus_device_init definition to align with
snd_hdac_bus_device_init, used by legacy drivers.
Memory for hdac_device is allocated by the caller

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 include/sound/hdaudio_ext.h   |  3 ++-
 sound/hda/ext/hdac_ext_bus.c  |  9 ++-------
 sound/soc/intel/skylake/skl.c | 27 ++++++++++++++++++++++++++-
 sound/soc/intel/skylake/skl.h | 10 +++++++---
 4 files changed, 37 insertions(+), 12 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..e4bcb76 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);
@@ -175,7 +171,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init);
 void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev)
 {
 	snd_hdac_device_exit(hdev);
-	kfree(hdev);
 }
 EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_exit);
 
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index b99c1d8..c2e9c19 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;
 
@@ -617,6 +618,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);
@@ -626,7 +629,19 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 		return -EIO;
 	dev_dbg(bus->dev, "codec #%d probed OK\n", addr);
 
-	return snd_hdac_ext_bus_device_init(bus, addr);
+	/*
+	 * FIXME:
+	 * The legacy HDA controller driver allocates data structure
+	 * for hda_codec. Ideally it should allocate only hdac_device
+	 * so that it has no dependency on the data structure of the
+	 * codec driver. If legacy controller driver can be changed this
+	 * code change can be avoided.
+	 */
+	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 */
@@ -751,6 +766,7 @@ static int skl_create(struct pci_dev *pci,
 {
 	struct skl *skl;
 	struct hdac_bus *bus;
+	struct hda_bus *hbus;
 
 	int err;
 
@@ -766,6 +782,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;
@@ -773,6 +790,14 @@ static int skl_create(struct pci_dev *pci,
 	INIT_WORK(&skl->probe_work, skl_probe_work);
 	bus->bdl_pos_adj = 0;
 
+	/*
+	 * TODO: other parameters can be taken the way it is taken by
+	 * legacy HDA driver
+	 */
+	mutex_init(&hbus->prepare_mutex);
+	hbus->pci = pci;
+	hbus->mixer_assigned = -1;
+
 	*rskl = skl;
 
 	return 0;
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index f3a48a4..777508f 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
 
@@ -63,7 +64,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 */
@@ -96,8 +97,11 @@ struct skl {
 	struct skl_fw_config cfg;
 };
 
-#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

[RFC v3 05/11] ALSA: hda - make some of the functions externally visible

[Rakesh Ughreja]

Mark some functions with EXPORT_SYMBOL_GPL so that it can be called by
other kernel modules. Move the probe function into generic driver.
These APIs would be called by ASoC based HDA codec driver which will be
added in the later patches.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/pci/hda/hda_bind.c    | 10 +++++-----
 sound/pci/hda/hda_codec.c   |  9 +++++++--
 sound/pci/hda/hda_codec.h   |  6 ++++++
 sound/pci/hda/hda_generic.c | 13 +++++++++++++
 4 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index d361bb7..d8715a1 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -149,8 +149,8 @@ static void hda_codec_driver_shutdown(struct device *dev)
 		codec->patch_ops.reboot_notify(codec);
 }
 
-int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
-			       struct module *owner)
+int __hda_legacy_codec_driver_register(struct hda_codec_driver *drv,
+					const char *name, struct module *owner)
 {
 	drv->core.driver.name = name;
 	drv->core.driver.owner = owner;
@@ -164,13 +164,13 @@ int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
 	drv->core.unsol_event = hda_codec_unsol_event;
 	return driver_register(&drv->core.driver);
 }
-EXPORT_SYMBOL_GPL(__hda_codec_driver_register);
+EXPORT_SYMBOL_GPL(__hda_legacy_codec_driver_register);
 
-void hda_codec_driver_unregister(struct hda_codec_driver *drv)
+void hda_legacy_codec_driver_unregister(struct hda_codec_driver *drv)
 {
 	driver_unregister(&drv->core.driver);
 }
-EXPORT_SYMBOL_GPL(hda_codec_driver_unregister);
+EXPORT_SYMBOL_GPL(hda_legacy_codec_driver_unregister);
 
 static inline bool codec_probed(struct hda_codec *codec)
 {
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index e018ecb..085fd9e 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2903,7 +2903,7 @@ static void hda_call_codec_resume(struct hda_codec *codec)
 	atomic_dec(&codec->core.in_pm);
 }
 
-static int hda_codec_runtime_suspend(struct device *dev)
+int hda_codec_runtime_suspend(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 	struct hda_pcm *pcm;
@@ -2919,8 +2919,9 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	snd_hdac_link_power(&codec->core, false);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(hda_codec_runtime_suspend);
 
-static int hda_codec_runtime_resume(struct device *dev)
+int hda_codec_runtime_resume(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
@@ -2930,6 +2931,7 @@ static int hda_codec_runtime_resume(struct device *dev)
 	pm_runtime_mark_last_busy(dev);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(hda_codec_runtime_resume);
 #endif /* CONFIG_PM */
 
 /* referred in hda_bind.c */
@@ -3005,6 +3007,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
@@ -3210,6 +3213,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)
@@ -3246,6 +3250,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(snd_hda_codec_build_pcms);
 
 /**
  * snd_hda_add_new_ctls - create controls from the array
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 681c360..ef626cf 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -99,6 +99,10 @@ struct hda_codec_driver {
 	const struct hda_device_id *id;
 };
 
+int __hda_legacy_codec_driver_register(struct hda_codec_driver *drv,
+				const char *name, struct module *owner);
+void hda_legacy_codec_driver_unregister(struct hda_codec_driver *drv);
+
 int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
 			       struct module *owner);
 #define hda_codec_driver_register(drv) \
@@ -497,6 +501,8 @@ int hda_call_check_power_status(struct hda_codec *codec, hda_nid_t nid)
 #ifdef CONFIG_PM
 void snd_hda_set_power_save(struct hda_bus *bus, int delay);
 void snd_hda_update_power_acct(struct hda_codec *codec);
+int hda_codec_runtime_suspend(struct device *dev);
+int hda_codec_runtime_resume(struct device *dev);
 #else
 static inline void snd_hda_set_power_save(struct hda_bus *bus, int delay) {}
 #endif
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 5cc6509..09ab02e 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -5961,6 +5961,19 @@ static int snd_hda_parse_generic_codec(struct hda_codec *codec)
 	return err;
 }
 
+int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
+			       struct module *owner)
+{
+	return __hda_legacy_codec_driver_register(drv, name, owner);
+}
+EXPORT_SYMBOL_GPL(__hda_codec_driver_register);
+
+void hda_codec_driver_unregister(struct hda_codec_driver *drv)
+{
+	hda_legacy_codec_driver_unregister(drv);
+}
+EXPORT_SYMBOL_GPL(hda_codec_driver_unregister);
+
 static const struct hda_device_id snd_hda_id_generic[] = {
 	HDA_CODEC_ENTRY(HDA_CODEC_ID_GENERIC, "Generic", snd_hda_parse_generic_codec),
 	{} /* terminator */
-- 
2.7.4

[RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Rakesh Ughreja]

This patch adds ASoC based HDA codec driver that can be used with
all Intel platforms.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/pci/hda/hda_codec.h     |  1 +
 sound/pci/hda/hda_generic.c   | 25 ++++++++++++
 sound/soc/codecs/Kconfig      |  6 +++
 sound/soc/codecs/Makefile     |  2 +
 sound/soc/codecs/hdac_hda.c   | 94 +++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/hdac_hda.h   | 22 ++++++++++
 sound/soc/intel/skylake/skl.c | 10 ++++-
 7 files changed, 158 insertions(+), 2 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_codec.h b/sound/pci/hda/hda_codec.h
index ef626cf..1525c5a 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -96,6 +96,7 @@ typedef int (*hda_codec_patch_t)(struct hda_codec *);
 
 struct hda_codec_driver {
 	struct hdac_driver core;
+	struct hdac_driver *asocdrv;
 	const struct hda_device_id *id;
 };
 
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 09ab02e..e0b46a4 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -38,6 +38,7 @@
 #include "hda_jack.h"
 #include "hda_beep.h"
 #include "hda_generic.h"
+#include "../../sound/soc/codecs/hdac_hda.h"
 
 
 /**
@@ -5964,12 +5965,36 @@ static int snd_hda_parse_generic_codec(struct hda_codec *codec)
 int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
 			       struct module *owner)
 {
+	int ret;
+
+	/*
+	 * Register ASoC HDA driver as well
+	 */
+	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)) {
+
+		drv->core.id_table = drv->id;
+		drv->asocdrv = kmalloc(sizeof(*drv->asocdrv), GFP_KERNEL);
+		if (!drv->asocdrv)
+			return -ENOMEM;
+
+		ret = __hdac_hda_codec_driver_register(&drv->core, name, owner);
+		if (ret < 0)
+			return ret;
+	}
 	return __hda_legacy_codec_driver_register(drv, name, owner);
 }
 EXPORT_SYMBOL_GPL(__hda_codec_driver_register);
 
 void hda_codec_driver_unregister(struct hda_codec_driver *drv)
 {
+	/*
+	 * Unregister ASoC HDA driver as well
+	 */
+	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)) {
+		hdac_hda_codec_driver_unregister(&drv->core);
+		kfree(drv->asocdrv);
+	}
+
 	hda_legacy_codec_driver_unregister(drv);
 }
 EXPORT_SYMBOL_GPL(hda_codec_driver_unregister);
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index ba5de07..1d41d11 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -79,6 +79,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
@@ -581,6 +582,11 @@ config SND_SOC_HDAC_HDMI
 	select SND_PCM_ELD
 	select HDMI
 
+config SND_SOC_HDAC_HDA
+	tristate
+	select SND_HDA_EXT_CORE
+	select SND_PCM_ELD
+
 config SND_SOC_ICS43432
 	tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 0977349..dd5f90f 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -73,6 +73,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
@@ -317,6 +318,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..387b00f
--- /dev/null
+++ b/sound/soc/codecs/hdac_hda.c
@@ -0,0 +1,94 @@
+/*
+ *  hdac_hda.c - ASoc HDA-HDA codec driver for Intel platforms
+ *
+ *  Copyright (C) 2015-2017 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#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"
+
+static int hdac_hda_dev_probe(struct hdac_device *hdev)
+{
+	struct hdac_ext_link *hlink = NULL;
+	struct hdac_hda_priv *hda_pvt;
+	int ret;
+
+	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+
+	/* 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;
+
+	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)
+{
+	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+	return 0;
+}
+
+#define hdac_hda_runtime_suspend NULL
+#define hdac_hda_runtime_resume NULL
+
+static const struct dev_pm_ops hdac_hda_pm = {
+	SET_RUNTIME_PM_OPS(hdac_hda_runtime_suspend,
+				hdac_hda_runtime_resume, NULL)
+};
+
+int __hdac_hda_codec_driver_register(struct hdac_driver *drv,
+	const char *name, struct module *owner)
+{
+	char new_name[strlen(name) + strlen("-asoc")];
+
+	sprintf(new_name, "%s-asoc", name);
+	drv->driver.name = new_name;
+	drv->driver.pm = &hdac_hda_pm;
+	drv->probe = hdac_hda_dev_probe;
+	drv->remove = hdac_hda_dev_remove;
+
+	return snd_hda_ext_driver_register(drv);
+}
+EXPORT_SYMBOL_GPL(__hdac_hda_codec_driver_register);
+
+void hdac_hda_codec_driver_unregister(struct hdac_driver *drv)
+{
+	snd_hda_ext_driver_unregister(drv);
+}
+EXPORT_SYMBOL_GPL(hdac_hda_codec_driver_unregister);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ASoC HDA codec driver");
+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..a089ec5
--- /dev/null
+++ b/sound/soc/codecs/hdac_hda.h
@@ -0,0 +1,22 @@
+#ifndef __HDAC_HDA_H__
+#define __HDAC_HDA_H__
+
+struct hdac_hda_priv {
+	struct hda_codec codec;
+	struct hda_bus *hbus;
+	int stream_tag;
+
+	struct snd_soc_codec *scodec;
+};
+
+#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)
+
+int __hdac_hda_codec_driver_register(struct hdac_driver *drv,
+				const char *name, struct module *owner);
+#define hdac_hda_codec_driver_register(drv) \
+	__hdac_hda_codec_driver_register(drv, KBUILD_MODNAME, THIS_MODULE)
+void hdac_hda_codec_driver_unregister(struct hdac_driver *drv);
+
+#endif /* __HDAC_HDA_H__ */
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index c2e9c19..a56a916 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;
 
@@ -619,6 +620,7 @@ 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;
 
 	mutex_lock(&bus->cmd_mutex);
@@ -637,10 +639,14 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 	 * codec driver. If legacy controller driver can be changed this
 	 * code change can be avoided.
 	 */
-	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;
 
+	hda_codec->hbus = skl_to_hbus(skl);
+	hdev = &hda_codec->codec.core;
+
 	return snd_hdac_ext_bus_device_init(bus, addr, hdev);
 }
 
-- 
2.7.4

[RFC v3 07/11] ALSA: hda: split API snd_hda_codec_new for using it from ASoC codec drivers

[Rakesh Ughreja]

Split snd_hda_codec_new API 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 split would help in reusing the functions from
ASoC HDA codec drivers.

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

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 085fd9e..b77dcf2 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -857,25 +857,21 @@ static void snd_hda_codec_dev_release(struct device *dev)
 }
 
 /**
- * snd_hda_codec_new - create a HDA codec
+ * snd_hda_codec_device_init - allocate and register hdac device with bus
  * @bus: the bus to assign
  * @codec_addr: the codec address
  * @codecp: the pointer to store the generated codec
  *
  * Returns 0 if successful, or a negative error code.
  */
-int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
-		      unsigned int codec_addr, struct hda_codec **codecp)
+static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
+			unsigned int codec_addr, struct hda_codec **codecp)
 {
-	struct hda_codec *codec;
-	char component[31];
-	hda_nid_t fg;
 	int err;
-	static struct snd_device_ops dev_ops = {
-		.dev_register = snd_hda_codec_dev_register,
-		.dev_disconnect = snd_hda_codec_dev_disconnect,
-		.dev_free = snd_hda_codec_dev_free,
-	};
+	char component[31];
+	struct hda_codec *codec;
+
+	dev_dbg(card->dev, "%s: entry\n", __func__);
 
 	if (snd_BUG_ON(!bus))
 		return -EINVAL;
@@ -888,14 +884,67 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
 
 	sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
 	err = snd_hdac_device_init(&codec->core, &bus->core, component,
-				   codec_addr);
+					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
+ * @codec_addr: the codec address
+ * @codecp: the pointer to store the generated codec
+ *
+ * Returns 0 if successful, or a negative error code.
+ */
+int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
+			unsigned int codec_addr, struct hda_codec **codecp)
+{
+	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);
+
+/**
+ * snd_hda_codec_device_new - add HDA codec into device list
+ * @bus: the bus to assign
+ * @codec_addr: the codec address
+ * @codec: the pointer to store the codec
+ *
+ * Returns 0 if successful, or a negative error code.
+ */
+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;
+	static struct snd_device_ops dev_ops = {
+		.dev_register = snd_hda_codec_dev_register,
+		.dev_disconnect = snd_hda_codec_dev_disconnect,
+		.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->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 +1004,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
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 1525c5a..33395b1 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -311,7 +311,9 @@ struct hda_codec {
  * constructors
  */
 int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
-		      unsigned int codec_addr, struct hda_codec **codecp);
+			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

[RFC v3 08/11] ASoC: hdac_hda: add DAI, widgets and related ops

[Rakesh Ughreja]

Add support for widgets, routes, DAI and related handlers.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/codecs/hdac_hda.c | 486 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 486 insertions(+)

diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index 387b00f..4b4d80c 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -28,6 +28,483 @@
 #include "../../pci/hda/hda_codec.h"
 #include "hdac_hda.h"
 
+#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_params(struct snd_pcm_substream *substream,
+	struct snd_pcm_hw_params *hparams, 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_params = hdac_hda_dai_hw_params,
+	.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[] = {
+{
+	.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,
+	},
+},
+{
+	.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_codec_match(struct hdac_device *hdev, struct hdac_driver *drv)
+{
+	struct hda_codec *codec = hdac_to_hda_codec(hdev);
+	const struct hda_device_id *list;
+	u32 id, rev_id;
+
+	if (!codec) {
+		dev_err(&hdev->dev, "%p\n", codec);
+		return 0;
+	}
+
+	/* check probe_id instead of vendor_id if set */
+	id = codec->probe_id ? codec->probe_id : codec->core.vendor_id;
+	rev_id = codec->core.revision_id;
+
+	dev_dbg(&hdev->dev, "%s: probe_id=0x%x and vendor_id=0x%x\n",
+			__func__, codec->probe_id, codec->core.vendor_id);
+
+	for (list = drv->id_table; list->vendor_id; list++) {
+		if (list->vendor_id == id &&
+			(!list->rev_id || list->rev_id == rev_id)) {
+			codec->preset = list;
+			dev_dbg(&hdev->dev, "found patch file %x\n", id);
+			return 1;
+		}
+	}
+	return 0;
+}
+
+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_device *hdev = &hda_pvt->codec.core;
+
+	hda_pvt->stream_tag = tx_mask;
+	dev_dbg(&hdev->dev, "%s: strm_tag: %d\n", __func__,
+						hda_pvt->stream_tag);
+	return 0;
+}
+
+static int hdac_hda_dai_hw_params(struct snd_pcm_substream *substream,
+	struct snd_pcm_hw_params *hparams, struct snd_soc_dai *dai)
+{
+	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
+	struct hdac_device *hdev = &hda_pvt->codec.core;
+
+	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+
+	/*
+	 * TODO:
+	 * verify if the parameters are supported by the codec
+	 */
+	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 hdac_device *hdev = &hda_pvt->codec.core;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct hda_pcm_stream *hda_stream;
+	struct hda_pcm *pcm;
+
+	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (pcm == NULL)
+		return -EINVAL;
+
+	dev_dbg(&hdev->dev, "pcm = %s: rate=%d ch=%d fmt=%d\n", pcm->name,
+		runtime->rate, runtime->channels, runtime->format);
+
+	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)
+{
+	int ret = 0;
+	struct hdac_hda_priv *hda_pvt = snd_soc_dai_get_drvdata(dai);
+	struct hdac_device *hdev = &hda_pvt->codec.core;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct hda_pcm_stream *hda_stream;
+	unsigned int format_val;
+	struct hda_pcm *pcm;
+
+	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (pcm == NULL)
+		return -EINVAL;
+
+	dev_dbg(&hdev->dev, "pcm = %s: rate=%d ch=%d fmt=%d\n", pcm->name,
+		runtime->rate, runtime->channels, runtime->format);
+
+	hda_stream = &pcm->stream[substream->stream];
+
+	dev_dbg(&hdev->dev, "fmt=0x%llx rate=0x%x maxbps=%d\n",
+		hda_stream->formats, hda_stream->rates, hda_stream->maxbps);
+
+	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->stream_tag, format_val, substream);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "codec prepare failed %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(&hdev->dev, "%s: exit\n", __func__);
+	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 hdac_device *hdev = &hda_pvt->codec.core;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct hda_pcm *pcm;
+	struct hda_pcm_stream *hda_stream;
+	int ret = 0;
+
+	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (pcm == NULL)
+		return -EINVAL;
+
+	dev_dbg(&hdev->dev, "pcm = %s: rate=%d ch=%d fmt=%d\n", pcm->name,
+		runtime->rate, runtime->channels, runtime->format);
+
+	hda_stream = &pcm->stream[substream->stream];
+
+	snd_hda_codec_pcm_get(pcm);
+
+	if (hda_stream->ops.open)
+		ret = hda_stream->ops.open(hda_stream, &hda_pvt->codec,
+								substream);
+	else
+		ret = -ENODEV;
+
+	dev_dbg(&hdev->dev, "%s: exit=%d\n", __func__, ret);
+
+	return 0;
+}
+
+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 hdac_device *hdev = &hda_pvt->codec.core;
+	struct hda_pcm *pcm;
+	struct hda_pcm_stream *hda_stream;
+
+	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+
+	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
+	if (pcm == NULL)
+		return;
+
+	hda_stream = &pcm->stream[substream->stream];
+
+	if (hda_stream->ops.open)
+		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_pcm *cpcm;
+	struct hda_codec *hcodec = &hda_pvt->codec;
+	const char *pcm_name;
+
+	if (strpbrk(dai->name, "Analog"))
+		pcm_name = "Analog";
+	else
+		pcm_name = "Digital";
+
+	dev_dbg(&hcodec->core.dev, "DAI %s\n", pcm_name);
+
+	list_for_each_entry(cpcm, &hcodec->pcm_list_head, list) {
+		dev_dbg(&hcodec->core.dev, "PCM %s\n", cpcm->name);
+		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 hdac_device *hdev = &hda_pvt->codec.core;
+	struct hdac_ext_link *hlink = NULL;
+	struct hda_codec *hcodec = &hda_pvt->codec;
+	struct snd_soc_dapm_context *dapm =
+			snd_soc_component_get_dapm(&codec->component);
+	int ret, i = 0;
+	u16 codec_mask;
+	hda_codec_patch_t patch;
+	struct hdac_driver *hdrv = drv_to_hdac_driver(hdev->dev.driver);
+
+	dev_dbg(&hdev->dev, "%s: Entry in_pm=%d pm_usage=%d\n", __func__,
+			hdev->in_pm.counter,
+			hdev->dev.power.usage_count.counter);
+
+	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);
+
+	dev_dbg(&hdev->dev, "before card created in_pm=%d pm_usage=%d\n",
+			hdev->in_pm.counter,
+			hdev->dev.power.usage_count.counter);
+
+	ret = snd_hda_codec_device_new(hda_pvt->hbus,
+			codec->component.card->snd_card, hdev->addr, hcodec);
+	if (ret < 0) {
+		dev_err(codec->dev, "%s: failed to create hda codec %d\n",
+					__func__, 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);
+
+	hda_pvt->hbus->card = dapm->card->snd_card;
+
+	dev_dbg(&hdev->dev, "after card created in_pm=%d pm_usage=%d\n",
+			hdev->in_pm.counter,
+			hdev->dev.power.usage_count.counter);
+
+	ret = hdac_codec_match(hdev, hdrv);
+	if (ret < 1) {
+		dev_err(codec->dev, "no match found for the codec\n");
+		return ret;
+	}
+
+	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
+	if (patch) {
+		ret = patch(hcodec);
+		if (ret < 0) {
+			dev_err(codec->dev, "no match found for the codec\n");
+			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, "unable to set name %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);
+
+	dev_dbg(&hdev->dev, "%s: exit\n", __func__);
+
+	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;
+
+	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+
+	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;
@@ -48,6 +525,14 @@ static int hdac_hda_dev_probe(struct hdac_device *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);
 
@@ -57,6 +542,7 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
 static int hdac_hda_dev_remove(struct hdac_device *hdev)
 {
 	dev_dbg(&hdev->dev, "%s: entry\n", __func__);
+	snd_soc_unregister_codec(&hdev->dev);
 	return 0;
 }
 
-- 
2.7.4

[RFC v3 09/11] ASoC: hdac_hda: add runtime PM support

[Rakesh Ughreja]

Add power management support.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/codecs/hdac_hda.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index 4b4d80c..22cabd4 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -546,8 +546,57 @@ static int hdac_hda_dev_remove(struct hdac_device *hdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int hdac_hda_runtime_suspend(struct device *dev)
+{
+	struct hdac_device *hdac = dev_to_hdac_dev(dev);
+	struct hdac_bus *bus = hdac->bus;
+	struct hdac_ext_link *hlink = NULL;
+
+	dev_dbg(dev, "%s: entry\n", __func__);
+
+	dev_dbg(dev, "calling legacy codec suspend\n");
+	hda_codec_runtime_suspend(dev);
+	dev_dbg(dev, "legacy codec suspended\n");
+
+	hlink = snd_hdac_ext_bus_get_link(bus, dev_name(dev));
+	if (!hlink) {
+		dev_err(dev, "hdac link not found\n");
+		return -EIO;
+	}
+	snd_hdac_ext_bus_link_put(bus, hlink);
+	return 0;
+}
+
+static int hdac_hda_runtime_resume(struct device *dev)
+{
+	struct hdac_device *hdac = dev_to_hdac_dev(dev);
+	struct hdac_bus *bus = hdac->bus;
+	struct hdac_ext_link *hlink = NULL;
+
+	dev_dbg(dev, "%s: entry\n", __func__);
+
+	/* controller may not have been initialized for the first time */
+	if (!bus)
+		return 0;
+
+	hlink = snd_hdac_ext_bus_get_link(bus, dev_name(dev));
+	if (!hlink) {
+		dev_err(dev, "hdac link not found\n");
+		return -EIO;
+	}
+	snd_hdac_ext_bus_link_get(bus, hlink);
+
+	dev_dbg(dev, "calling legacy codec resume\n");
+	hda_codec_runtime_resume(dev);
+	dev_dbg(dev, "%s: exit\n", __func__);
+
+	return 0;
+}
+#else
 #define hdac_hda_runtime_suspend NULL
 #define hdac_hda_runtime_resume NULL
+#endif
 
 static const struct dev_pm_ops hdac_hda_pm = {
 	SET_RUNTIME_PM_OPS(hdac_hda_runtime_suspend,
-- 
2.7.4

[RFC v3 10/11] ASoC: codec: Support for ASoC Realtek HDA codec Driver

[Rakesh Ughreja]

This patch adds a Kconfig entry to compile existing legacy Realtek
HDA codec drivers as ASoC codec driver.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/codecs/Kconfig | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 1d41d11..97fd997 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -80,6 +80,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_GTM601
 	select SND_SOC_HDAC_HDMI
 	select SND_SOC_HDAC_HDA
+	select SND_SOC_HDA_REALTEK
 	select SND_SOC_ICS43432
 	select SND_SOC_INNO_RK3036
 	select SND_SOC_ISABELLE if I2C
@@ -587,6 +588,11 @@ config SND_SOC_HDAC_HDA
 	select SND_HDA_EXT_CORE
 	select SND_PCM_ELD
 
+config SND_SOC_HDA_REALTEK
+	select SND_HDA_CODEC_REALTEK
+	select SND_SOC_HDAC_HDA
+	tristate "Realtek ASoC HDA Codec Driver patch"
+
 config SND_SOC_ICS43432
 	tristate
 
-- 
2.7.4

[RFC v3 11/11] 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/boards/Kconfig               |  1 +
 sound/soc/intel/boards/skl_hda_dsp_common.c  | 43 ++++++++++++++++++++++++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h  |  2 +-
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 29 +++++++++++++++++++
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index e01137d..4108205 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -267,6 +267,7 @@ config SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH
         select SND_SOC_INTEL_SST
         depends on SND_SOC_INTEL_SKYLAKE
         select SND_SOC_HDAC_HDMI
+        depends on SND_HDA_CODEC_REALTEK
         help
           This adds support for ASoC Onboard Codec HDA machine driver. This will
           create an alsa sound card for HDA Codecs.
diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
index 5558334..c575d8e 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
@@ -62,6 +62,24 @@ int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
 	return 0;
 }
 
+static int skl_hda_link_fixup(struct snd_soc_pcm_runtime *rtd,
+					struct snd_pcm_hw_params *params)
+{
+	struct snd_interval *rate = hw_param_interval(params,
+						SNDRV_PCM_HW_PARAM_RATE);
+	struct snd_interval *channels = hw_param_interval(params,
+						SNDRV_PCM_HW_PARAM_CHANNELS);
+	struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
+
+	/* The output is 48KHz, stereo, 16bits */
+	rate->min = rate->max = 48000;
+	channels->min = channels->max = 2;
+
+	snd_mask_none(fmt);
+	snd_mask_set(fmt, SNDRV_PCM_FORMAT_S24_LE);
+	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] = {
 
@@ -96,6 +114,31 @@ 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,
+		.be_hw_params_fixup = skl_hda_link_fixup,
+	},
+	{
+		.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 c9cb66b..463938a 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.h
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
@@ -21,7 +21,7 @@
 #include <sound/jack.h>
 #define __SOUND_SOC_HDA_DSP_COMMON_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 2d2b901..e0d7e12 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -25,8 +25,33 @@
 #include "../skylake/skl.h"
 #include "skl_hda_dsp_common.h"
 
+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"},
@@ -63,6 +88,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,
-- 
2.7.4

Re: [RFC v3 05/11] ALSA: hda - make some of the functions externally visible

[Takashi Iwai]

On Fri, 15 Dec 2017 12:30:42 +0100,
Rakesh Ughreja wrote:
> 
> Mark some functions with EXPORT_SYMBOL_GPL so that it can be called by
> other kernel modules. Move the probe function into generic driver.
> These APIs would be called by ASoC based HDA codec driver which will be
> added in the later patches.
> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>  sound/pci/hda/hda_bind.c    | 10 +++++-----
>  sound/pci/hda/hda_codec.c   |  9 +++++++--
>  sound/pci/hda/hda_codec.h   |  6 ++++++
>  sound/pci/hda/hda_generic.c | 13 +++++++++++++
>  4 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
> index d361bb7..d8715a1 100644
> --- a/sound/pci/hda/hda_bind.c
> +++ b/sound/pci/hda/hda_bind.c
> @@ -149,8 +149,8 @@ static void hda_codec_driver_shutdown(struct device *dev)
>  		codec->patch_ops.reboot_notify(codec);
>  }
>  
> -int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
> -			       struct module *owner)
> +int __hda_legacy_codec_driver_register(struct hda_codec_driver *drv,
> +					const char *name, struct module *owner)
>  {
>  	drv->core.driver.name = name;
>  	drv->core.driver.owner = owner;
> @@ -164,13 +164,13 @@ int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
>  	drv->core.unsol_event = hda_codec_unsol_event;
>  	return driver_register(&drv->core.driver);
>  }
> -EXPORT_SYMBOL_GPL(__hda_codec_driver_register);
> +EXPORT_SYMBOL_GPL(__hda_legacy_codec_driver_register);
>  
> -void hda_codec_driver_unregister(struct hda_codec_driver *drv)
> +void hda_legacy_codec_driver_unregister(struct hda_codec_driver *drv)
>  {
>  	driver_unregister(&drv->core.driver);
>  }
> -EXPORT_SYMBOL_GPL(hda_codec_driver_unregister);
> +EXPORT_SYMBOL_GPL(hda_legacy_codec_driver_unregister);
>  
>  static inline bool codec_probed(struct hda_codec *codec)
>  {
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index e018ecb..085fd9e 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2903,7 +2903,7 @@ static void hda_call_codec_resume(struct hda_codec *codec)
>  	atomic_dec(&codec->core.in_pm);
>  }
>  
> -static int hda_codec_runtime_suspend(struct device *dev)
> +int hda_codec_runtime_suspend(struct device *dev)
>  {
>  	struct hda_codec *codec = dev_to_hda_codec(dev);
>  	struct hda_pcm *pcm;
> @@ -2919,8 +2919,9 @@ static int hda_codec_runtime_suspend(struct device *dev)
>  	snd_hdac_link_power(&codec->core, false);
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(hda_codec_runtime_suspend);
>  
> -static int hda_codec_runtime_resume(struct device *dev)
> +int hda_codec_runtime_resume(struct device *dev)
>  {
>  	struct hda_codec *codec = dev_to_hda_codec(dev);
>  
> @@ -2930,6 +2931,7 @@ static int hda_codec_runtime_resume(struct device *dev)
>  	pm_runtime_mark_last_busy(dev);
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(hda_codec_runtime_resume);
>  #endif /* CONFIG_PM */
>  
>  /* referred in hda_bind.c */
> @@ -3005,6 +3007,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
> @@ -3210,6 +3213,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)
> @@ -3246,6 +3250,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(snd_hda_codec_build_pcms);
>  
>  /**
>   * snd_hda_add_new_ctls - create controls from the array
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index 681c360..ef626cf 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -99,6 +99,10 @@ struct hda_codec_driver {
>  	const struct hda_device_id *id;
>  };
>  
> +int __hda_legacy_codec_driver_register(struct hda_codec_driver *drv,
> +				const char *name, struct module *owner);
> +void hda_legacy_codec_driver_unregister(struct hda_codec_driver *drv);
> +
>  int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
>  			       struct module *owner);
>  #define hda_codec_driver_register(drv) \
> @@ -497,6 +501,8 @@ int hda_call_check_power_status(struct hda_codec *codec, hda_nid_t nid)
>  #ifdef CONFIG_PM
>  void snd_hda_set_power_save(struct hda_bus *bus, int delay);
>  void snd_hda_update_power_acct(struct hda_codec *codec);
> +int hda_codec_runtime_suspend(struct device *dev);
> +int hda_codec_runtime_resume(struct device *dev);
>  #else
>  static inline void snd_hda_set_power_save(struct hda_bus *bus, int delay) {}
>  #endif
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 5cc6509..09ab02e 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -5961,6 +5961,19 @@ static int snd_hda_parse_generic_codec(struct hda_codec *codec)
>  	return err;
>  }
>  
> +int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
> +			       struct module *owner)
> +{
> +	return __hda_legacy_codec_driver_register(drv, name, owner);
> +}
> +EXPORT_SYMBOL_GPL(__hda_codec_driver_register);
> +
> +void hda_codec_driver_unregister(struct hda_codec_driver *drv)
> +{
> +	hda_legacy_codec_driver_unregister(drv);
> +}
> +EXPORT_SYMBOL_GPL(hda_codec_driver_unregister);

I'm afraid that this will break things when no generic driver is
selected in kconfig.  What's the reason to move there?


thanks,

Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Fri, 15 Dec 2017 12:30:43 +0100,
Rakesh Ughreja wrote:
> 
> This patch adds ASoC based HDA codec driver that can be used with
> all Intel platforms.
> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>  sound/pci/hda/hda_codec.h     |  1 +
>  sound/pci/hda/hda_generic.c   | 25 ++++++++++++
>  sound/soc/codecs/Kconfig      |  6 +++
>  sound/soc/codecs/Makefile     |  2 +
>  sound/soc/codecs/hdac_hda.c   | 94 +++++++++++++++++++++++++++++++++++++++++++
>  sound/soc/codecs/hdac_hda.h   | 22 ++++++++++
>  sound/soc/intel/skylake/skl.c | 10 ++++-
>  7 files changed, 158 insertions(+), 2 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_codec.h b/sound/pci/hda/hda_codec.h
> index ef626cf..1525c5a 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -96,6 +96,7 @@ typedef int (*hda_codec_patch_t)(struct hda_codec *);
>  
>  struct hda_codec_driver {
>  	struct hdac_driver core;
> +	struct hdac_driver *asocdrv;
>  	const struct hda_device_id *id;
>  };
>  
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 09ab02e..e0b46a4 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -38,6 +38,7 @@
>  #include "hda_jack.h"
>  #include "hda_beep.h"
>  #include "hda_generic.h"
> +#include "../../sound/soc/codecs/hdac_hda.h"
>  
>  
>  /**
> @@ -5964,12 +5965,36 @@ static int snd_hda_parse_generic_codec(struct hda_codec *codec)
>  int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
>  			       struct module *owner)
>  {
> +	int ret;
> +
> +	/*
> +	 * Register ASoC HDA driver as well
> +	 */
> +	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)) {
> +
> +		drv->core.id_table = drv->id;
> +		drv->asocdrv = kmalloc(sizeof(*drv->asocdrv), GFP_KERNEL);
> +		if (!drv->asocdrv)
> +			return -ENOMEM;
> +
> +		ret = __hdac_hda_codec_driver_register(&drv->core, name, owner);
> +		if (ret < 0)
> +			return ret;
> +	}

Hrm, now I see why you moved the function.
But this change essentially means that the code-path is always enabled
when the ASoC HD-audio driver is *built*.  Distros may build both but
blacklist, and it would break the legacy driver.

Can we check differently?  For example, we may put some difference in
the driver and check it here instead of the static IS_ENABLED().
The code can be put with #if IS_ENABLED() for optimization, but the
actual behavior should be runtime-dependent.


thanks,

Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Friday, December 15, 2017 5:08 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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver
>
>On Fri, 15 Dec 2017 12:30:43 +0100,
>Rakesh Ughreja wrote:
>>
>> This patch adds ASoC based HDA codec driver that can be used with
>> all Intel platforms.
>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>> ---
>>  sound/pci/hda/hda_codec.h     |  1 +
>>  sound/pci/hda/hda_generic.c   | 25 ++++++++++++
>>  sound/soc/codecs/Kconfig      |  6 +++
>>  sound/soc/codecs/Makefile     |  2 +
>>  sound/soc/codecs/hdac_hda.c   | 94
>+++++++++++++++++++++++++++++++++++++++++++
>>  sound/soc/codecs/hdac_hda.h   | 22 ++++++++++
>>  sound/soc/intel/skylake/skl.c | 10 ++++-
>>  7 files changed, 158 insertions(+), 2 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_codec.h b/sound/pci/hda/hda_codec.h
>> index ef626cf..1525c5a 100644
>> --- a/sound/pci/hda/hda_codec.h
>> +++ b/sound/pci/hda/hda_codec.h
>> @@ -96,6 +96,7 @@ typedef int (*hda_codec_patch_t)(struct hda_codec *);
>>
>>  struct hda_codec_driver {
>>  	struct hdac_driver core;
>> +	struct hdac_driver *asocdrv;
>>  	const struct hda_device_id *id;
>>  };
>>
>> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
>> index 09ab02e..e0b46a4 100644
>> --- a/sound/pci/hda/hda_generic.c
>> +++ b/sound/pci/hda/hda_generic.c
>> @@ -38,6 +38,7 @@
>>  #include "hda_jack.h"
>>  #include "hda_beep.h"
>>  #include "hda_generic.h"
>> +#include "../../sound/soc/codecs/hdac_hda.h"
>>
>>
>>  /**
>> @@ -5964,12 +5965,36 @@ static int snd_hda_parse_generic_codec(struct
>hda_codec *codec)
>>  int __hda_codec_driver_register(struct hda_codec_driver *drv, const char
>*name,
>>  			       struct module *owner)
>>  {
>> +	int ret;
>> +
>> +	/*
>> +	 * Register ASoC HDA driver as well
>> +	 */
>> +	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)) {
>> +
>> +		drv->core.id_table = drv->id;
>> +		drv->asocdrv = kmalloc(sizeof(*drv->asocdrv), GFP_KERNEL);
>> +		if (!drv->asocdrv)
>> +			return -ENOMEM;
>> +
>> +		ret = __hdac_hda_codec_driver_register(&drv->core, name,
>owner);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>
>Hrm, now I see why you moved the function.
>But this change essentially means that the code-path is always enabled
>when the ASoC HD-audio driver is *built*.  Distros may build both but
>blacklist, and it would break the legacy driver.

In this series, I am registering the driver two times. First using generic
ASoC driver and then using Generic legacy driver. I am appending "-asoc"
string to the driver name while registering for ASoC HDA, so do second
time registration.

Are you suggesting that I should do only once ? Now I understand that
there is no point in doing two times registration.

>
>Can we check differently?  For example, we may put some difference in
>the driver and check it here instead of the static IS_ENABLED().

Do you think a module parameter is a good idea ?

>The code can be put with #if IS_ENABLED() for optimization, but the
>actual behavior should be runtime-dependent.

Sure, I will do it.

>
>
>thanks,
>
>Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Fri, 15 Dec 2017 13:20:30 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Friday, December 15, 2017 5:08 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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver
> >
> >On Fri, 15 Dec 2017 12:30:43 +0100,
> >Rakesh Ughreja wrote:
> >>
> >> This patch adds ASoC based HDA codec driver that can be used with
> >> all Intel platforms.
> >>
> >> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> >> ---
> >>  sound/pci/hda/hda_codec.h     |  1 +
> >>  sound/pci/hda/hda_generic.c   | 25 ++++++++++++
> >>  sound/soc/codecs/Kconfig      |  6 +++
> >>  sound/soc/codecs/Makefile     |  2 +
> >>  sound/soc/codecs/hdac_hda.c   | 94
> >+++++++++++++++++++++++++++++++++++++++++++
> >>  sound/soc/codecs/hdac_hda.h   | 22 ++++++++++
> >>  sound/soc/intel/skylake/skl.c | 10 ++++-
> >>  7 files changed, 158 insertions(+), 2 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_codec.h b/sound/pci/hda/hda_codec.h
> >> index ef626cf..1525c5a 100644
> >> --- a/sound/pci/hda/hda_codec.h
> >> +++ b/sound/pci/hda/hda_codec.h
> >> @@ -96,6 +96,7 @@ typedef int (*hda_codec_patch_t)(struct hda_codec *);
> >>
> >>  struct hda_codec_driver {
> >>  	struct hdac_driver core;
> >> +	struct hdac_driver *asocdrv;
> >>  	const struct hda_device_id *id;
> >>  };
> >>
> >> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> >> index 09ab02e..e0b46a4 100644
> >> --- a/sound/pci/hda/hda_generic.c
> >> +++ b/sound/pci/hda/hda_generic.c
> >> @@ -38,6 +38,7 @@
> >>  #include "hda_jack.h"
> >>  #include "hda_beep.h"
> >>  #include "hda_generic.h"
> >> +#include "../../sound/soc/codecs/hdac_hda.h"
> >>
> >>
> >>  /**
> >> @@ -5964,12 +5965,36 @@ static int snd_hda_parse_generic_codec(struct
> >hda_codec *codec)
> >>  int __hda_codec_driver_register(struct hda_codec_driver *drv, const char
> >*name,
> >>  			       struct module *owner)
> >>  {
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * Register ASoC HDA driver as well
> >> +	 */
> >> +	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)) {
> >> +
> >> +		drv->core.id_table = drv->id;
> >> +		drv->asocdrv = kmalloc(sizeof(*drv->asocdrv), GFP_KERNEL);
> >> +		if (!drv->asocdrv)
> >> +			return -ENOMEM;
> >> +
> >> +		ret = __hdac_hda_codec_driver_register(&drv->core, name,
> >owner);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	}
> >
> >Hrm, now I see why you moved the function.
> >But this change essentially means that the code-path is always enabled
> >when the ASoC HD-audio driver is *built*.  Distros may build both but
> >blacklist, and it would break the legacy driver.
> 
> In this series, I am registering the driver two times. First using generic
> ASoC driver and then using Generic legacy driver. I am appending "-asoc"
> string to the driver name while registering for ASoC HDA, so do second
> time registration.
> 
> Are you suggesting that I should do only once ? Now I understand that
> there is no point in doing two times registration.

OK, that's the implementation I didn't expect.
And, yes, registering twice doesn't make sense -- especially you melt
many components into the pot, and now we get dependencies like hell.

> >Can we check differently?  For example, we may put some difference in
> >the driver and check it here instead of the static IS_ENABLED().
> 
> Do you think a module parameter is a good idea ?

I don't think so.  We do need to consider a better way.

Maybe an alternative is to give the additional indirect calls.
That is, put some new ops or hook to the bus for calling some extra
probing task in addition to the standard codec probe.


thanks,

Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Friday, December 15, 2017 9:18 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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver
>

>> >Can we check differently?  For example, we may put some difference in
>> >the driver and check it here instead of the static IS_ENABLED().
>>
>> Do you think a module parameter is a good idea ?
>
>I don't think so.  We do need to consider a better way.
>
>Maybe an alternative is to give the additional indirect calls.
>That is, put some new ops or hook to the bus for calling some extra
>probing task in addition to the standard codec probe.
>

I am not sure if I understand you fully, so asking some follow up
Questions.

I am assuming you are asking me to implement something like following.
Where I have to implement snd_hda_get_mode() function which would
return "true" if we need to register the driver as "asoc" driver.

Is that right understanding ?

int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
                               struct module *owner)
{
        /*
         * check if we need to register ASoC HDA driver
         */
#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
        int asoc_mode = snd_hda_get_mode();
        if (asoc_mode) {
                drv->core.id_table = drv->id;
                return __hdac_hda_codec_driver_register(&drv->core, name, owner);
        }
#endif
        return __hda_legacy_codec_driver_register(drv, name, owner);
}

If above is true then the follow up question is, what are the criteria to determine
the mode. Since I cannot assume that the bus instance is already created at the
time of driver registration, I am not sure how to determine what kind of platform
driver would be loaded in future.

Regards,
Rakesh

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Sat, 16 Dec 2017 08:48:35 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Friday, December 15, 2017 9:18 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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver
> >
> 
> >> >Can we check differently?  For example, we may put some difference in
> >> >the driver and check it here instead of the static IS_ENABLED().
> >>
> >> Do you think a module parameter is a good idea ?
> >
> >I don't think so.  We do need to consider a better way.
> >
> >Maybe an alternative is to give the additional indirect calls.
> >That is, put some new ops or hook to the bus for calling some extra
> >probing task in addition to the standard codec probe.
> >
> 
> I am not sure if I understand you fully, so asking some follow up
> Questions.
> 
> I am assuming you are asking me to implement something like following.
> Where I have to implement snd_hda_get_mode() function which would
> return "true" if we need to register the driver as "asoc" driver.
> 
> Is that right understanding ?
> 
> int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
>                                struct module *owner)
> {
>         /*
>          * check if we need to register ASoC HDA driver
>          */
> #if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
>         int asoc_mode = snd_hda_get_mode();
>         if (asoc_mode) {
>                 drv->core.id_table = drv->id;
>                 return __hdac_hda_codec_driver_register(&drv->core, name, owner);
>         }
> #endif
>         return __hda_legacy_codec_driver_register(drv, name, owner);
> }
> 
> If above is true then the follow up question is, what are the criteria to determine
> the mode. Since I cannot assume that the bus instance is already created at the
> time of driver registration, I am not sure how to determine what kind of platform
> driver would be loaded in future.

My assumption is that there is only one hda_codec_driver_register().
The legacy code needs to be rewritten to implement the standard
probe/remove as preliminary.  Any the rest differentiation is done via
additional callbacks at probe/remove time.


Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Saturday, December 16, 2017 2:44 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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver
>

>> I am not sure if I understand you fully, so asking some follow up
>> Questions.
>>
>> I am assuming you are asking me to implement something like following.
>> Where I have to implement snd_hda_get_mode() function which would
>> return "true" if we need to register the driver as "asoc" driver.
>>
>> Is that right understanding ?
>>
>> int __hda_codec_driver_register(struct hda_codec_driver *drv, const char
>*name,
>>                                struct module *owner)
>> {
>>         /*
>>          * check if we need to register ASoC HDA driver
>>          */
>> #if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
>>         int asoc_mode = snd_hda_get_mode();
>>         if (asoc_mode) {
>>                 drv->core.id_table = drv->id;
>>                 return __hdac_hda_codec_driver_register(&drv->core, name, owner);
>>         }
>> #endif
>>         return __hda_legacy_codec_driver_register(drv, name, owner);
>> }
>>
>> If above is true then the follow up question is, what are the criteria to determine
>> the mode. Since I cannot assume that the bus instance is already created at the
>> time of driver registration, I am not sure how to determine what kind of platform
>> driver would be loaded in future.
>
>My assumption is that there is only one hda_codec_driver_register().
>The legacy code needs to be rewritten to implement the standard
>probe/remove as preliminary.  Any the rest differentiation is done via
>additional callbacks at probe/remove time.
>

Oh I see. Let me work on this and may ask you some additional clarifications.

Regards,
Rakesh

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Ughreja, Rakesh A
>Sent: Monday, December 18, 2017 9:36 AM
>To: Takashi Iwai <tiwai@suse.de>
>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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec
>driver
>
>
>
>>-----Original Message-----
>>From: Takashi Iwai [mailto:tiwai@suse.de]
>>Sent: Saturday, December 16, 2017 2:44 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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec
>driver
>>
>
>>> I am not sure if I understand you fully, so asking some follow up
>>> Questions.
>>>
>>> I am assuming you are asking me to implement something like following.
>>> Where I have to implement snd_hda_get_mode() function which would
>>> return "true" if we need to register the driver as "asoc" driver.
>>>
>>> Is that right understanding ?
>>>
>>> int __hda_codec_driver_register(struct hda_codec_driver *drv, const char
>>*name,
>>>                                struct module *owner)
>>> {
>>>         /*
>>>          * check if we need to register ASoC HDA driver
>>>          */
>>> #if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
>>>         int asoc_mode = snd_hda_get_mode();
>>>         if (asoc_mode) {
>>>                 drv->core.id_table = drv->id;
>>>                 return __hdac_hda_codec_driver_register(&drv->core, name,
>owner);
>>>         }
>>> #endif
>>>         return __hda_legacy_codec_driver_register(drv, name, owner);
>>> }
>>>
>>> If above is true then the follow up question is, what are the criteria to
>determine
>>> the mode. Since I cannot assume that the bus instance is already created at
>the
>>> time of driver registration, I am not sure how to determine what kind of
>platform
>>> driver would be loaded in future.
>>
>>My assumption is that there is only one hda_codec_driver_register().
>>The legacy code needs to be rewritten to implement the standard
>>probe/remove as preliminary.  Any the rest differentiation is done via
>>additional callbacks at probe/remove time.
>>
>
>Oh I see. Let me work on this and may ask you some additional clarifications.
>
>Regards,
>Rakesh

Hi Takashi,

I worked on the concept that you proposed and here is the patch with main 
code change. Basically I wrote common driver handlers for HDA Driver
snd_hdac_drv_probe, snd_hdac_drv_remove, snd_hdac_drv_shutdown, 
snd_hdac_drv_match, snd_hdac_drv_unsol_event etc.

Once the common driver is probed it checks what kind of bus it is enumerated
on by calling the bus API. If it is ASOC bus type it calls the ASOC driver
registered callbacks and if it is LEGACY bus type it calls the Legacy driver
registered callbacks.

ASoC based platform drivers would create ASOC bus type while the legacy
controller drivers would create LEGACY bus type. I added the bus_type as
a part of hdac_bus, which gets set during snd_hdac_bus_init or 
snd_hdac_ext_bus_init. hdac_device->type and hdac_driver->type are
set the HDA_DEV_CORE during registrations and enumeration. 

Also I have kept these routines as part of codec library, so that all the other
drivers can use it without duplicating the code.

Let me know if you are okay, I can include these changes as part of my
next series.

Regards,
Rakesh

--- patch--
This patch adds common enumeration functions for HDA codec drivers.
Once the codec is enumerated on the hdac_bus, the bus_type is checked
before the corresponding callback functions are called.

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

diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index d8715a1..7d8d54c 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -11,6 +11,7 @@
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <sound/core.h>
+#include <sound/hdaudio_ext.h>
 #include "hda_codec.h"
 #include "hda_local.h"
 
@@ -74,10 +75,10 @@ int snd_hda_codec_set_name(struct hda_codec *codec, const char *name)
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_set_name);
 
-static int hda_codec_driver_probe(struct device *dev)
+static int hda_codec_driver_probe(struct hdac_device *hdev)
 {
-	struct hda_codec *codec = dev_to_hda_codec(dev);
-	struct module *owner = dev->driver->owner;
+	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
+	struct module *owner = hdev->dev.driver->owner;
 	hda_codec_patch_t patch;
 	int err;
 
@@ -130,48 +131,25 @@ static int hda_codec_driver_probe(struct device *dev)
 	return err;
 }
 
-static int hda_codec_driver_remove(struct device *dev)
+static int hda_codec_driver_remove(struct hdac_device *hdev)
 {
-	struct hda_codec *codec = dev_to_hda_codec(dev);
+	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
 
 	if (codec->patch_ops.free)
 		codec->patch_ops.free(codec);
 	snd_hda_codec_cleanup_for_unbind(codec);
-	module_put(dev->driver->owner);
+	module_put(hdev->dev.driver->owner);
 	return 0;
 }
 
-static void hda_codec_driver_shutdown(struct device *dev)
+static void hda_codec_driver_shutdown(struct hdac_device *hdev)
 {
-	struct hda_codec *codec = dev_to_hda_codec(dev);
+	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
 
-	if (!pm_runtime_suspended(dev) && codec->patch_ops.reboot_notify)
+	if (!pm_runtime_suspended(&hdev->dev) && codec->patch_ops.reboot_notify)
 		codec->patch_ops.reboot_notify(codec);
 }
 
-int __hda_legacy_codec_driver_register(struct hda_codec_driver *drv,
-					const char *name, struct module *owner)
-{
-	drv->core.driver.name = name;
-	drv->core.driver.owner = owner;
-	drv->core.driver.bus = &snd_hda_bus_type;
-	drv->core.driver.probe = hda_codec_driver_probe;
-	drv->core.driver.remove = hda_codec_driver_remove;
-	drv->core.driver.shutdown = hda_codec_driver_shutdown;
-	drv->core.driver.pm = &hda_codec_driver_pm;
-	drv->core.type = HDA_DEV_LEGACY;
-	drv->core.match = hda_codec_match;
-	drv->core.unsol_event = hda_codec_unsol_event;
-	return driver_register(&drv->core.driver);
-}
-EXPORT_SYMBOL_GPL(__hda_legacy_codec_driver_register);
-
-void hda_legacy_codec_driver_unregister(struct hda_codec_driver *drv)
-{
-	driver_unregister(&drv->core.driver);
-}
-EXPORT_SYMBOL_GPL(hda_legacy_codec_driver_unregister);
-
 static inline bool codec_probed(struct hda_codec *codec)
 {
 	return device_attach(hda_codec_dev(codec)) > 0 && codec->preset;
@@ -305,3 +283,213 @@ int snd_hda_codec_configure(struct hda_codec *codec)
 	return err;
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_configure);
+
+
+/*
+ * Newly implemented functions for common routines
+ */
+static struct hdac_driver *snd_hdac_drvs[2];
+
+int snd_hdac_drv_probe(struct hdac_device *hdev)
+{
+	int bus_type;
+	struct hdac_driver *drv;
+
+	bus_type = snd_hdac_get_bus_type(hdev->bus);
+	bus_type --;
+
+	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
+
+	drv = snd_hdac_drvs[bus_type];
+	if (drv->probe)
+		return drv->probe(hdev);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_drv_probe);
+
+int snd_hdac_drv_remove(struct hdac_device *hdev)
+{
+	int bus_type;
+	struct hdac_driver *drv;
+
+	bus_type = snd_hdac_get_bus_type(hdev->bus);
+	bus_type --;
+
+	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
+
+	drv = snd_hdac_drvs[bus_type];
+	if (drv->remove)
+		return drv->remove(hdev);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_drv_remove);
+
+void snd_hdac_drv_shutdown(struct hdac_device *hdev)
+{
+	int bus_type;
+	struct hdac_driver *drv;
+
+	bus_type = snd_hdac_get_bus_type(hdev->bus);
+	bus_type --;
+
+	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
+
+	drv = snd_hdac_drvs[bus_type];
+	if (drv->shutdown)
+		return drv->shutdown(hdev);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_drv_shutdown);
+
+int snd_hdac_drv_match(struct hdac_device *hdev, struct hdac_driver *drv)
+{
+	int bus_type;
+	struct hdac_driver *cdrv;
+
+	bus_type = snd_hdac_get_bus_type(hdev->bus);
+	bus_type --;
+
+	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
+
+	cdrv = snd_hdac_drvs[bus_type];
+	if (cdrv->match)
+		return cdrv->match(hdev, drv);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_drv_match);
+
+void snd_hdac_drv_unsol_event(struct hdac_device *hdev, unsigned int event)
+{
+	int bus_type;
+	struct hdac_driver *drv;
+
+	bus_type = snd_hdac_get_bus_type(hdev->bus);
+	bus_type --;
+
+	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
+
+	drv = snd_hdac_drvs[bus_type];
+	if (drv->unsol_event)
+		return drv->unsol_event(hdev, event);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_drv_unsol_event);
+
+int snd_hdac_runtime_suspend(struct device *dev)
+{
+
+	int bus_type;
+	struct hdac_driver *drv;
+	struct hdac_device *hdev = dev_to_hdac_dev(dev);
+
+	bus_type = snd_hdac_get_bus_type(hdev->bus);
+	bus_type --;
+
+	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
+
+	drv = snd_hdac_drvs[bus_type];
+	if (drv->driver.pm->runtime_suspend)
+		return drv->driver.pm->runtime_suspend(dev);
+
+	return 0;
+}
+
+int snd_hdac_runtime_resume(struct device *dev)
+{
+
+	int bus_type;
+	struct hdac_driver *drv;
+	struct hdac_device *hdev = dev_to_hdac_dev(dev);
+
+	bus_type = snd_hdac_get_bus_type(hdev->bus);
+	bus_type --;
+
+	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
+
+	drv = snd_hdac_drvs[bus_type];
+	if (drv->driver.pm->runtime_resume)
+		return drv->driver.pm->runtime_resume(dev);
+
+	return 0;
+}
+
+const struct dev_pm_ops snd_hdac_drv_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(snd_hdac_runtime_suspend, snd_hdac_runtime_resume,
+			   NULL)
+};
+
+int snd_hdac_callback_register(struct hdac_driver *drv)
+{
+	int idx;
+
+	if(drv->type != HDA_DEV_LEGACY && drv->type != HDA_DEV_ASOC)
+		return -EINVAL;
+
+	printk("%s: registered callback %d\n", drv->driver.name, drv->type);
+	idx = drv->type - 1;
+	if (!snd_hdac_drvs[idx])
+		snd_hdac_drvs[idx] = drv;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_callback_register);
+
+int snd_hdac_callback_unregister(struct hdac_driver *drv)
+{
+	int idx;
+
+	if(drv->type != HDA_DEV_LEGACY && drv->type != HDA_DEV_ASOC)
+		return -EINVAL;
+
+	idx = drv->type - 1;
+	snd_hdac_drvs[idx] = NULL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_callback_unregister);
+
+static struct hdac_driver hdac_legacy_driver = {
+	.type = HDA_DEV_LEGACY,
+	.match = hda_codec_match,
+	.unsol_event = hda_codec_unsol_event,
+	.probe = hda_codec_driver_probe,
+	.remove = hda_codec_driver_remove,
+	.shutdown = hda_codec_driver_shutdown,
+	.driver.pm = &hda_codec_driver_pm,
+};
+
+int __snd_hdac_driver_register(struct hda_codec_driver *drv, const char *name,
+			       struct module *owner)
+{
+	int ret;
+	struct hdac_driver *core = &drv->core;
+
+	printk("%s: entry ..%s\n", __func__, name);
+
+	core->id_table = drv->id;
+	core->driver.name = name;
+	core->driver.owner = owner;
+	core->driver.bus = &snd_hda_bus_type;
+	core->driver.pm = &snd_hdac_drv_pm;
+	core->type = HDA_DEV_CORE;
+	core->probe = snd_hdac_drv_probe;
+	core->remove = snd_hdac_drv_remove;
+	core->shutdown = snd_hdac_drv_shutdown;
+	core->match = snd_hdac_drv_match;
+	core->unsol_event = snd_hdac_drv_unsol_event;
+
+	ret = snd_hdac_callback_register(&hdac_legacy_driver);
+	if (ret < 0)
+		return ret;
+
+	return snd_hda_ext_driver_register(core);
+
+
+}
+EXPORT_SYMBOL_GPL(__snd_hdac_driver_register);
+
+void snd_hdac_driver_unregister(struct hda_codec_driver *drv)
+{
+	snd_hda_ext_driver_unregister(&drv->core);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_driver_unregister);
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index dda8401..0a3b56e 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -99,18 +99,18 @@ struct hda_codec_driver {
 	const struct hda_device_id *id;
 };
 
-int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
-			       struct module *owner);
-#define hda_codec_driver_register(drv) \
-	__hda_codec_driver_register(drv, KBUILD_MODNAME, THIS_MODULE)
-void hda_codec_driver_unregister(struct hda_codec_driver *drv);
+int __snd_hdac_driver_register(struct hda_codec_driver *drv, const char *name,
+				struct module *owner);
+#define snd_hdac_driver_register(drv) \
+	__snd_hdac_driver_register(drv, KBUILD_MODNAME, THIS_MODULE)
+void snd_hdac_driver_unregister(struct hda_codec_driver *drv);
+
 #define module_hda_codec_driver(drv) \
-	module_driver(drv, hda_codec_driver_register, \
-		      hda_codec_driver_unregister)
+	module_driver(drv, snd_hdac_driver_register, \
+		      snd_hdac_driver_unregister)
+
+int snd_hdac_callback_register(struct hdac_driver *drv);
+int snd_hdac_callback_unregister(struct hdac_driver *drv);
 
 /* ops set by the preset patch */
 struct hda_codec_ops {
-- 
2.7.4

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Tue, 19 Dec 2017 10:19:17 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Ughreja, Rakesh A
> >Sent: Monday, December 18, 2017 9:36 AM
> >To: Takashi Iwai <tiwai@suse.de>
> >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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec
> >driver
> >
> >
> >
> >>-----Original Message-----
> >>From: Takashi Iwai [mailto:tiwai@suse.de]
> >>Sent: Saturday, December 16, 2017 2:44 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: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec
> >driver
> >>
> >
> >>> I am not sure if I understand you fully, so asking some follow up
> >>> Questions.
> >>>
> >>> I am assuming you are asking me to implement something like following.
> >>> Where I have to implement snd_hda_get_mode() function which would
> >>> return "true" if we need to register the driver as "asoc" driver.
> >>>
> >>> Is that right understanding ?
> >>>
> >>> int __hda_codec_driver_register(struct hda_codec_driver *drv, const char
> >>*name,
> >>>                                struct module *owner)
> >>> {
> >>>         /*
> >>>          * check if we need to register ASoC HDA driver
> >>>          */
> >>> #if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
> >>>         int asoc_mode = snd_hda_get_mode();
> >>>         if (asoc_mode) {
> >>>                 drv->core.id_table = drv->id;
> >>>                 return __hdac_hda_codec_driver_register(&drv->core, name,
> >owner);
> >>>         }
> >>> #endif
> >>>         return __hda_legacy_codec_driver_register(drv, name, owner);
> >>> }
> >>>
> >>> If above is true then the follow up question is, what are the criteria to
> >determine
> >>> the mode. Since I cannot assume that the bus instance is already created at
> >the
> >>> time of driver registration, I am not sure how to determine what kind of
> >platform
> >>> driver would be loaded in future.
> >>
> >>My assumption is that there is only one hda_codec_driver_register().
> >>The legacy code needs to be rewritten to implement the standard
> >>probe/remove as preliminary.  Any the rest differentiation is done via
> >>additional callbacks at probe/remove time.
> >>
> >
> >Oh I see. Let me work on this and may ask you some additional clarifications.
> >
> >Regards,
> >Rakesh
> 
> Hi Takashi,
> 
> I worked on the concept that you proposed and here is the patch with main 
> code change. Basically I wrote common driver handlers for HDA Driver
> snd_hdac_drv_probe, snd_hdac_drv_remove, snd_hdac_drv_shutdown, 
> snd_hdac_drv_match, snd_hdac_drv_unsol_event etc.
> 
> Once the common driver is probed it checks what kind of bus it is enumerated
> on by calling the bus API. If it is ASOC bus type it calls the ASOC driver
> registered callbacks and if it is LEGACY bus type it calls the Legacy driver
> registered callbacks.
> 
> ASoC based platform drivers would create ASOC bus type while the legacy
> controller drivers would create LEGACY bus type. I added the bus_type as
> a part of hdac_bus, which gets set during snd_hdac_bus_init or 
> snd_hdac_ext_bus_init. hdac_device->type and hdac_driver->type are
> set the HDA_DEV_CORE during registrations and enumeration. 
> 
> Also I have kept these routines as part of codec library, so that all the other
> drivers can use it without duplicating the code.
> 
> Let me know if you are okay, I can include these changes as part of my
> next series.

I need to think more deeply, but after a quick look, I find it too
overhead.

May we start from a "big picture" to describe the whole driver
bindings?


Takashi


> 
> Regards,
> Rakesh
> 
> --- patch--
> This patch adds common enumeration functions for HDA codec drivers.
> Once the codec is enumerated on the hdac_bus, the bus_type is checked
> before the corresponding callback functions are called.
> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>  sound/pci/hda/hda_bind.c  | 252 ++++++++++++++++++++++++++++++++++++++++------
>  sound/pci/hda/hda_codec.h |  22 ++--
>  2 files changed, 231 insertions(+), 43 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
> index d8715a1..7d8d54c 100644
> --- a/sound/pci/hda/hda_bind.c
> +++ b/sound/pci/hda/hda_bind.c
> @@ -11,6 +11,7 @@
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <sound/core.h>
> +#include <sound/hdaudio_ext.h>
>  #include "hda_codec.h"
>  #include "hda_local.h"
>  
> @@ -74,10 +75,10 @@ int snd_hda_codec_set_name(struct hda_codec *codec, const char *name)
>  }
>  EXPORT_SYMBOL_GPL(snd_hda_codec_set_name);
>  
> -static int hda_codec_driver_probe(struct device *dev)
> +static int hda_codec_driver_probe(struct hdac_device *hdev)
>  {
> -	struct hda_codec *codec = dev_to_hda_codec(dev);
> -	struct module *owner = dev->driver->owner;
> +	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
> +	struct module *owner = hdev->dev.driver->owner;
>  	hda_codec_patch_t patch;
>  	int err;
>  
> @@ -130,48 +131,25 @@ static int hda_codec_driver_probe(struct device *dev)
>  	return err;
>  }
>  
> -static int hda_codec_driver_remove(struct device *dev)
> +static int hda_codec_driver_remove(struct hdac_device *hdev)
>  {
> -	struct hda_codec *codec = dev_to_hda_codec(dev);
> +	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
>  
>  	if (codec->patch_ops.free)
>  		codec->patch_ops.free(codec);
>  	snd_hda_codec_cleanup_for_unbind(codec);
> -	module_put(dev->driver->owner);
> +	module_put(hdev->dev.driver->owner);
>  	return 0;
>  }
>  
> -static void hda_codec_driver_shutdown(struct device *dev)
> +static void hda_codec_driver_shutdown(struct hdac_device *hdev)
>  {
> -	struct hda_codec *codec = dev_to_hda_codec(dev);
> +	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
>  
> -	if (!pm_runtime_suspended(dev) && codec->patch_ops.reboot_notify)
> +	if (!pm_runtime_suspended(&hdev->dev) && codec->patch_ops.reboot_notify)
>  		codec->patch_ops.reboot_notify(codec);
>  }
>  
> -int __hda_legacy_codec_driver_register(struct hda_codec_driver *drv,
> -					const char *name, struct module *owner)
> -{
> -	drv->core.driver.name = name;
> -	drv->core.driver.owner = owner;
> -	drv->core.driver.bus = &snd_hda_bus_type;
> -	drv->core.driver.probe = hda_codec_driver_probe;
> -	drv->core.driver.remove = hda_codec_driver_remove;
> -	drv->core.driver.shutdown = hda_codec_driver_shutdown;
> -	drv->core.driver.pm = &hda_codec_driver_pm;
> -	drv->core.type = HDA_DEV_LEGACY;
> -	drv->core.match = hda_codec_match;
> -	drv->core.unsol_event = hda_codec_unsol_event;
> -	return driver_register(&drv->core.driver);
> -}
> -EXPORT_SYMBOL_GPL(__hda_legacy_codec_driver_register);
> -
> -void hda_legacy_codec_driver_unregister(struct hda_codec_driver *drv)
> -{
> -	driver_unregister(&drv->core.driver);
> -}
> -EXPORT_SYMBOL_GPL(hda_legacy_codec_driver_unregister);
> -
>  static inline bool codec_probed(struct hda_codec *codec)
>  {
>  	return device_attach(hda_codec_dev(codec)) > 0 && codec->preset;
> @@ -305,3 +283,213 @@ int snd_hda_codec_configure(struct hda_codec *codec)
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(snd_hda_codec_configure);
> +
> +
> +/*
> + * Newly implemented functions for common routines
> + */
> +static struct hdac_driver *snd_hdac_drvs[2];
> +
> +int snd_hdac_drv_probe(struct hdac_device *hdev)
> +{
> +	int bus_type;
> +	struct hdac_driver *drv;
> +
> +	bus_type = snd_hdac_get_bus_type(hdev->bus);
> +	bus_type --;
> +
> +	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
> +
> +	drv = snd_hdac_drvs[bus_type];
> +	if (drv->probe)
> +		return drv->probe(hdev);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_drv_probe);
> +
> +int snd_hdac_drv_remove(struct hdac_device *hdev)
> +{
> +	int bus_type;
> +	struct hdac_driver *drv;
> +
> +	bus_type = snd_hdac_get_bus_type(hdev->bus);
> +	bus_type --;
> +
> +	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
> +
> +	drv = snd_hdac_drvs[bus_type];
> +	if (drv->remove)
> +		return drv->remove(hdev);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_drv_remove);
> +
> +void snd_hdac_drv_shutdown(struct hdac_device *hdev)
> +{
> +	int bus_type;
> +	struct hdac_driver *drv;
> +
> +	bus_type = snd_hdac_get_bus_type(hdev->bus);
> +	bus_type --;
> +
> +	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
> +
> +	drv = snd_hdac_drvs[bus_type];
> +	if (drv->shutdown)
> +		return drv->shutdown(hdev);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_drv_shutdown);
> +
> +int snd_hdac_drv_match(struct hdac_device *hdev, struct hdac_driver *drv)
> +{
> +	int bus_type;
> +	struct hdac_driver *cdrv;
> +
> +	bus_type = snd_hdac_get_bus_type(hdev->bus);
> +	bus_type --;
> +
> +	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
> +
> +	cdrv = snd_hdac_drvs[bus_type];
> +	if (cdrv->match)
> +		return cdrv->match(hdev, drv);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_drv_match);
> +
> +void snd_hdac_drv_unsol_event(struct hdac_device *hdev, unsigned int event)
> +{
> +	int bus_type;
> +	struct hdac_driver *drv;
> +
> +	bus_type = snd_hdac_get_bus_type(hdev->bus);
> +	bus_type --;
> +
> +	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
> +
> +	drv = snd_hdac_drvs[bus_type];
> +	if (drv->unsol_event)
> +		return drv->unsol_event(hdev, event);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_drv_unsol_event);
> +
> +int snd_hdac_runtime_suspend(struct device *dev)
> +{
> +
> +	int bus_type;
> +	struct hdac_driver *drv;
> +	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> +
> +	bus_type = snd_hdac_get_bus_type(hdev->bus);
> +	bus_type --;
> +
> +	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
> +
> +	drv = snd_hdac_drvs[bus_type];
> +	if (drv->driver.pm->runtime_suspend)
> +		return drv->driver.pm->runtime_suspend(dev);
> +
> +	return 0;
> +}
> +
> +int snd_hdac_runtime_resume(struct device *dev)
> +{
> +
> +	int bus_type;
> +	struct hdac_driver *drv;
> +	struct hdac_device *hdev = dev_to_hdac_dev(dev);
> +
> +	bus_type = snd_hdac_get_bus_type(hdev->bus);
> +	bus_type --;
> +
> +	dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
> +
> +	drv = snd_hdac_drvs[bus_type];
> +	if (drv->driver.pm->runtime_resume)
> +		return drv->driver.pm->runtime_resume(dev);
> +
> +	return 0;
> +}
> +
> +const struct dev_pm_ops snd_hdac_drv_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(snd_hdac_runtime_suspend, snd_hdac_runtime_resume,
> +			   NULL)
> +};
> +
> +int snd_hdac_callback_register(struct hdac_driver *drv)
> +{
> +	int idx;
> +
> +	if(drv->type != HDA_DEV_LEGACY && drv->type != HDA_DEV_ASOC)
> +		return -EINVAL;
> +
> +	printk("%s: registered callback %d\n", drv->driver.name, drv->type);
> +	idx = drv->type - 1;
> +	if (!snd_hdac_drvs[idx])
> +		snd_hdac_drvs[idx] = drv;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_callback_register);
> +
> +int snd_hdac_callback_unregister(struct hdac_driver *drv)
> +{
> +	int idx;
> +
> +	if(drv->type != HDA_DEV_LEGACY && drv->type != HDA_DEV_ASOC)
> +		return -EINVAL;
> +
> +	idx = drv->type - 1;
> +	snd_hdac_drvs[idx] = NULL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_callback_unregister);
> +
> +static struct hdac_driver hdac_legacy_driver = {
> +	.type = HDA_DEV_LEGACY,
> +	.match = hda_codec_match,
> +	.unsol_event = hda_codec_unsol_event,
> +	.probe = hda_codec_driver_probe,
> +	.remove = hda_codec_driver_remove,
> +	.shutdown = hda_codec_driver_shutdown,
> +	.driver.pm = &hda_codec_driver_pm,
> +};
> +
> +int __snd_hdac_driver_register(struct hda_codec_driver *drv, const char *name,
> +			       struct module *owner)
> +{
> +	int ret;
> +	struct hdac_driver *core = &drv->core;
> +
> +	printk("%s: entry ..%s\n", __func__, name);
> +
> +	core->id_table = drv->id;
> +	core->driver.name = name;
> +	core->driver.owner = owner;
> +	core->driver.bus = &snd_hda_bus_type;
> +	core->driver.pm = &snd_hdac_drv_pm;
> +	core->type = HDA_DEV_CORE;
> +	core->probe = snd_hdac_drv_probe;
> +	core->remove = snd_hdac_drv_remove;
> +	core->shutdown = snd_hdac_drv_shutdown;
> +	core->match = snd_hdac_drv_match;
> +	core->unsol_event = snd_hdac_drv_unsol_event;
> +
> +	ret = snd_hdac_callback_register(&hdac_legacy_driver);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snd_hda_ext_driver_register(core);
> +
> +
> +}
> +EXPORT_SYMBOL_GPL(__snd_hdac_driver_register);
> +
> +void snd_hdac_driver_unregister(struct hda_codec_driver *drv)
> +{
> +	snd_hda_ext_driver_unregister(&drv->core);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_driver_unregister);
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index dda8401..0a3b56e 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -99,18 +99,18 @@ struct hda_codec_driver {
>  	const struct hda_device_id *id;
>  };
>  
> -int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
> -			       struct module *owner);
> -#define hda_codec_driver_register(drv) \
> -	__hda_codec_driver_register(drv, KBUILD_MODNAME, THIS_MODULE)
> -void hda_codec_driver_unregister(struct hda_codec_driver *drv);
> +int __snd_hdac_driver_register(struct hda_codec_driver *drv, const char *name,
> +				struct module *owner);
> +#define snd_hdac_driver_register(drv) \
> +	__snd_hdac_driver_register(drv, KBUILD_MODNAME, THIS_MODULE)
> +void snd_hdac_driver_unregister(struct hda_codec_driver *drv);
> +
>  #define module_hda_codec_driver(drv) \
> -	module_driver(drv, hda_codec_driver_register, \
> -		      hda_codec_driver_unregister)
> +	module_driver(drv, snd_hdac_driver_register, \
> +		      snd_hdac_driver_unregister)
> +
> +int snd_hdac_callback_register(struct hdac_driver *drv);
> +int snd_hdac_callback_unregister(struct hdac_driver *drv);
>  
>  /* ops set by the preset patch */
>  struct hda_codec_ops {
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Tuesday, December 19, 2017 4:58 PM
>To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
>Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
>louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
><patches.audio@intel.com>; broonie@kernel.org
>Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
>codec driver
>
>>
>> Hi Takashi,
>>
>> I worked on the concept that you proposed and here is the patch with main
>> code change. Basically I wrote common driver handlers for HDA Driver
>> snd_hdac_drv_probe, snd_hdac_drv_remove, snd_hdac_drv_shutdown,
>> snd_hdac_drv_match, snd_hdac_drv_unsol_event etc.
>>
>> Once the common driver is probed it checks what kind of bus it is enumerated
>> on by calling the bus API. If it is ASOC bus type it calls the ASOC driver
>> registered callbacks and if it is LEGACY bus type it calls the Legacy driver
>> registered callbacks.
>>
>> ASoC based platform drivers would create ASOC bus type while the legacy
>> controller drivers would create LEGACY bus type. I added the bus_type as
>> a part of hdac_bus, which gets set during snd_hdac_bus_init or
>> snd_hdac_ext_bus_init. hdac_device->type and hdac_driver->type are
>> set the HDA_DEV_CORE during registrations and enumeration.
>>
>> Also I have kept these routines as part of codec library, so that all the other
>> drivers can use it without duplicating the code.
>>
>> Let me know if you are okay, I can include these changes as part of my
>> next series.
>
>I need to think more deeply, but after a quick look, I find it too
>overhead.

Are you referring to the calling overhead because of the wrapper involved ?

The way I see is we have two options.

1. Single driver option. - There is going to be a common wrapper here,
which needs to come into picture before it re-directs it to the appropriate
driver. This is what is done in the following patch.

2. Separate driver for ASoC and Legacy. - This requires ID tables, Can we move
id tables into separate header file ? then it can solve the problem involved in
option 1. This also solves the problem related to wrappers as well as the
problem related to accessing the ID tables via extern symbols, that you
mentioned in the previous series.

>
>May we start from a "big picture" to describe the whole driver
>bindings?
>

This patch registers the hdac_driver callback at the root level agnostic to
asoc and legacy and then selects the appropriate legacy or asoc callbacks,
based on the bus which enumerated the driver.

I cannot think of any other approach if we want to go with a single driver
approach. You will have to give some more hints :-)

Thank you for taking time and giving feedback.

Regards,
Rakesh

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Tue, 19 Dec 2017 16:26:04 +0100,
Ughreja, Rakesh A wrote:
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Tuesday, December 19, 2017 4:58 PM
> >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
> >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
> ><patches.audio@intel.com>; broonie@kernel.org
> >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
> >codec driver
> >
> >>
> >> Hi Takashi,
> >>
> >> I worked on the concept that you proposed and here is the patch with main
> >> code change. Basically I wrote common driver handlers for HDA Driver
> >> snd_hdac_drv_probe, snd_hdac_drv_remove, snd_hdac_drv_shutdown,
> >> snd_hdac_drv_match, snd_hdac_drv_unsol_event etc.
> >>
> >> Once the common driver is probed it checks what kind of bus it is enumerated
> >> on by calling the bus API. If it is ASOC bus type it calls the ASOC driver
> >> registered callbacks and if it is LEGACY bus type it calls the Legacy driver
> >> registered callbacks.
> >>
> >> ASoC based platform drivers would create ASOC bus type while the legacy
> >> controller drivers would create LEGACY bus type. I added the bus_type as
> >> a part of hdac_bus, which gets set during snd_hdac_bus_init or
> >> snd_hdac_ext_bus_init. hdac_device->type and hdac_driver->type are
> >> set the HDA_DEV_CORE during registrations and enumeration.
> >>
> >> Also I have kept these routines as part of codec library, so that all the other
> >> drivers can use it without duplicating the code.
> >>
> >> Let me know if you are okay, I can include these changes as part of my
> >> next series.
> >
> >I need to think more deeply, but after a quick look, I find it too
> >overhead.
> 
> Are you referring to the calling overhead because of the wrapper involved ?
> 
> The way I see is we have two options.
> 
> 1. Single driver option. - There is going to be a common wrapper here,
> which needs to come into picture before it re-directs it to the appropriate
> driver. This is what is done in the following patch.
> 
> 2. Separate driver for ASoC and Legacy. - This requires ID tables, Can we move
> id tables into separate header file ? then it can solve the problem involved in
> option 1. This also solves the problem related to wrappers as well as the
> problem related to accessing the ID tables via extern symbols, that you
> mentioned in the previous series.

I believe (2) is no-go, it's a straight maintenance hell.

> >May we start from a "big picture" to describe the whole driver
> >bindings?
> >
> 
> This patch registers the hdac_driver callback at the root level agnostic to
> asoc and legacy and then selects the appropriate legacy or asoc callbacks,
> based on the bus which enumerated the driver.
> 
> I cannot think of any other approach if we want to go with a single driver
> approach. You will have to give some more hints :-)

Well, a big unclear question to me is why do we need to bind the stuff
so differently.  Can't we simply provide the same binding to the
legacy codec from asoc driver?  In the legacy-support mode, asoc
driver creates the legacy-compatible codec objects with the
legacy-compatible hda_bus.


Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Tuesday, December 19, 2017 9:10 PM
>To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
>Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
>louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
><patches.audio@intel.com>; broonie@kernel.org
>Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
>codec driver
>


>>
>> Are you referring to the calling overhead because of the wrapper involved ?
>>
>> The way I see is we have two options.
>>
>> 1. Single driver option. - There is going to be a common wrapper here,
>> which needs to come into picture before it re-directs it to the appropriate
>> driver. This is what is done in the following patch.
>>
>> 2. Separate driver for ASoC and Legacy. - This requires ID tables, Can we move
>> id tables into separate header file ? then it can solve the problem involved in
>> option 1. This also solves the problem related to wrappers as well as the
>> problem related to accessing the ID tables via extern symbols, that you
>> mentioned in the previous series.
>
>I believe (2) is no-go, it's a straight maintenance hell.

Got it.

>
>> >May we start from a "big picture" to describe the whole driver
>> >bindings?
>> >
>>
>> This patch registers the hdac_driver callback at the root level agnostic to
>> asoc and legacy and then selects the appropriate legacy or asoc callbacks,
>> based on the bus which enumerated the driver.
>>
>> I cannot think of any other approach if we want to go with a single driver
>> approach. You will have to give some more hints :-)
>
>Well, a big unclear question to me is why do we need to bind the stuff
>so differently.  Can't we simply provide the same binding to the
>legacy codec from asoc driver?  In the legacy-support mode, asoc
>driver creates the legacy-compatible codec objects with the
>legacy-compatible hda_bus.
>

Both the drivers i.e. ASoC and Legacy are registering the driver in
exact same way by filling the hdac_driver fields. There is no difference
in terms of HDA bus interactions. First series unifies even the data
structures hdac_device, hdac_driver and hdac_bus.

Once the device is enumerated and the hdac_driver->probe
is called the difference starts, primarily because ASoC vs ALSA
codec driver differences. Here also if you notice, after taking care of 
the ASoC related components, ASoC codec driver calls exact same APIs of the 
legacy HDA codec driver which are called by the legacy controller driver.

The hda_bus and hda_codec data structures are also used by the ASoC
driver as it is to call legacy codec driver APIs.

So I am not sure if we are doing binding in two different ways, or I
misunderstood you completely ?

Regards,
Rakesh

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Tue, 19 Dec 2017 17:14:38 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Tuesday, December 19, 2017 9:10 PM
> >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
> >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
> ><patches.audio@intel.com>; broonie@kernel.org
> >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
> >codec driver
> >
> 
> 
> >>
> >> Are you referring to the calling overhead because of the wrapper involved ?
> >>
> >> The way I see is we have two options.
> >>
> >> 1. Single driver option. - There is going to be a common wrapper here,
> >> which needs to come into picture before it re-directs it to the appropriate
> >> driver. This is what is done in the following patch.
> >>
> >> 2. Separate driver for ASoC and Legacy. - This requires ID tables, Can we move
> >> id tables into separate header file ? then it can solve the problem involved in
> >> option 1. This also solves the problem related to wrappers as well as the
> >> problem related to accessing the ID tables via extern symbols, that you
> >> mentioned in the previous series.
> >
> >I believe (2) is no-go, it's a straight maintenance hell.
> 
> Got it.
> 
> >
> >> >May we start from a "big picture" to describe the whole driver
> >> >bindings?
> >> >
> >>
> >> This patch registers the hdac_driver callback at the root level agnostic to
> >> asoc and legacy and then selects the appropriate legacy or asoc callbacks,
> >> based on the bus which enumerated the driver.
> >>
> >> I cannot think of any other approach if we want to go with a single driver
> >> approach. You will have to give some more hints :-)
> >
> >Well, a big unclear question to me is why do we need to bind the stuff
> >so differently.  Can't we simply provide the same binding to the
> >legacy codec from asoc driver?  In the legacy-support mode, asoc
> >driver creates the legacy-compatible codec objects with the
> >legacy-compatible hda_bus.
> >
> 
> Both the drivers i.e. ASoC and Legacy are registering the driver in
> exact same way by filling the hdac_driver fields. There is no difference
> in terms of HDA bus interactions. First series unifies even the data
> structures hdac_device, hdac_driver and hdac_bus.

Yes, that's a good part.

> Once the device is enumerated and the hdac_driver->probe
> is called the difference starts, primarily because ASoC vs ALSA
> codec driver differences.

Why so different?  Is it hard to integrate them somehow?

What exactly do we need to do in addition to the existing legacy
HD-audio codec probe/remove/whatever?

> Here also if you notice, after taking care of 
> the ASoC related components, ASoC codec driver calls exact same APIs of the 
> legacy HDA codec driver which are called by the legacy controller driver.
> 
> The hda_bus and hda_codec data structures are also used by the ASoC
> driver as it is to call legacy codec driver APIs.
> 
> So I am not sure if we are doing binding in two different ways, or I
> misunderstood you completely ?

Well, I still am not sure why do we need two ways, completely
switching the whole binding.  If it's a matter of some additional
calls on top of the legacy probe, we can add a new optional bus ops,
and call it in the probe function, too.  At the time of probe
callback, the bus is already present.


thanks,

Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Tuesday, December 19, 2017 9:54 PM
>To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
>Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
>louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
><patches.audio@intel.com>; broonie@kernel.org
>Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
>codec driver
>
>> >
>> >Well, a big unclear question to me is why do we need to bind the stuff
>> >so differently.  Can't we simply provide the same binding to the
>> >legacy codec from asoc driver?  In the legacy-support mode, asoc
>> >driver creates the legacy-compatible codec objects with the
>> >legacy-compatible hda_bus.
>> >
>>
>> Both the drivers i.e. ASoC and Legacy are registering the driver in
>> exact same way by filling the hdac_driver fields. There is no difference
>> in terms of HDA bus interactions. First series unifies even the data
>> structures hdac_device, hdac_driver and hdac_bus.
>
>Yes, that's a good part.
>
>> Once the device is enumerated and the hdac_driver->probe
>> is called the difference starts, primarily because ASoC vs ALSA
>> codec driver differences.
>
>Why so different?  Is it hard to integrate them somehow?
>
>What exactly do we need to do in addition to the existing legacy
>HD-audio codec probe/remove/whatever?

probe/remove of the ASoC driver takes care of registering/unregistering 
the snd_soc_codec_driver with ASoC framework. Which would register
dai, widgets and controls associated with the codec.

But in this driver probe the card is not created and so nothing else is done.
It waits until the ASoC framework calls snd_soc_codec probe.

So the way I see is ASoC driver probe is just a bus level probe and its
one level before the legacy codec driver probe.

Legacy codec driver probe is kind of similar to snd_soc_codec probe.
Again if you think from unification, we are calling the exact same APIs
which are called in the legacy driver probe. Some legacy APIs are not 
used e.g. build_pcms.

snd_soc_codec_driver callbacks are handled by ASoC framework
and so we can not call it from anywhere else. But within this we call 
most of the APIs of the legacy codec driver.

You can look at the ~/sound/soc/codecs/hdac_hda.c file, its just one file
for the ASoC driver.

>
>> Here also if you notice, after taking care of
>> the ASoC related components, ASoC codec driver calls exact same APIs of the
>> legacy HDA codec driver which are called by the legacy controller driver.
>>
>> The hda_bus and hda_codec data structures are also used by the ASoC
>> driver as it is to call legacy codec driver APIs.
>>
>> So I am not sure if we are doing binding in two different ways, or I
>> misunderstood you completely ?
>
>Well, I still am not sure why do we need two ways, completely
>switching the whole binding.  If it's a matter of some additional
>calls on top of the legacy probe, we can add a new optional bus ops,
>and call it in the probe function, too.  At the time of probe
>callback, the bus is already present.

>From the bus point of view, there are no differences in the driver.
The Series 1 has already done that.

The difference is only due to ASoC vs ALSA framework. 

In the last patch, instead of checking the bus_type everywhere in the code,
it uses it to call the individual driver function. That way the hdac_driver 
callback implementation remains separate and much cleaner for ASoC
and legacy drivers. So the legacy codec driver APIs are still used but
without too many if else conditions.

Regards,
Rakesh

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Tue, 19 Dec 2017 18:12:03 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Tuesday, December 19, 2017 9:54 PM
> >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
> >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
> ><patches.audio@intel.com>; broonie@kernel.org
> >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
> >codec driver
> >
> >> >
> >> >Well, a big unclear question to me is why do we need to bind the stuff
> >> >so differently.  Can't we simply provide the same binding to the
> >> >legacy codec from asoc driver?  In the legacy-support mode, asoc
> >> >driver creates the legacy-compatible codec objects with the
> >> >legacy-compatible hda_bus.
> >> >
> >>
> >> Both the drivers i.e. ASoC and Legacy are registering the driver in
> >> exact same way by filling the hdac_driver fields. There is no difference
> >> in terms of HDA bus interactions. First series unifies even the data
> >> structures hdac_device, hdac_driver and hdac_bus.
> >
> >Yes, that's a good part.
> >
> >> Once the device is enumerated and the hdac_driver->probe
> >> is called the difference starts, primarily because ASoC vs ALSA
> >> codec driver differences.
> >
> >Why so different?  Is it hard to integrate them somehow?
> >
> >What exactly do we need to do in addition to the existing legacy
> >HD-audio codec probe/remove/whatever?
> 
> probe/remove of the ASoC driver takes care of registering/unregistering 
> the snd_soc_codec_driver with ASoC framework. Which would register
> dai, widgets and controls associated with the codec.
> 
> But in this driver probe the card is not created and so nothing else is done.
> It waits until the ASoC framework calls snd_soc_codec probe.
> 
> So the way I see is ASoC driver probe is just a bus level probe and its
> one level before the legacy codec driver probe.
> 
> Legacy codec driver probe is kind of similar to snd_soc_codec probe.
> Again if you think from unification, we are calling the exact same APIs
> which are called in the legacy driver probe. Some legacy APIs are not 
> used e.g. build_pcms.
> 
> snd_soc_codec_driver callbacks are handled by ASoC framework
> and so we can not call it from anywhere else. But within this we call 
> most of the APIs of the legacy codec driver.
> 
> You can look at the ~/sound/soc/codecs/hdac_hda.c file, its just one file
> for the ASoC driver.
> 
> >
> >> Here also if you notice, after taking care of
> >> the ASoC related components, ASoC codec driver calls exact same APIs of the
> >> legacy HDA codec driver which are called by the legacy controller driver.
> >>
> >> The hda_bus and hda_codec data structures are also used by the ASoC
> >> driver as it is to call legacy codec driver APIs.
> >>
> >> So I am not sure if we are doing binding in two different ways, or I
> >> misunderstood you completely ?
> >
> >Well, I still am not sure why do we need two ways, completely
> >switching the whole binding.  If it's a matter of some additional
> >calls on top of the legacy probe, we can add a new optional bus ops,
> >and call it in the probe function, too.  At the time of probe
> >callback, the bus is already present.
> 
> From the bus point of view, there are no differences in the driver.
> The Series 1 has already done that.
> 
> The difference is only due to ASoC vs ALSA framework. 
> 
> In the last patch, instead of checking the bus_type everywhere in the code,
> it uses it to call the individual driver function. That way the hdac_driver 
> callback implementation remains separate and much cleaner for ASoC
> and legacy drivers. So the legacy codec driver APIs are still used but
> without too many if else conditions.

Well, honestly speaking, that complete morphing scares me.  If it's a
simple additional call, it's easier to track the code flow.  But your
patch essentially fakes the legacy codec and bus structs and twists
the control by that switch...  which looks too fragile to me.


thanks,

Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Mark Brown]

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

On Tue, Dec 19, 2017 at 08:17:37PM +0100, Takashi Iwai wrote:

> Well, honestly speaking, that complete morphing scares me.  If it's a
> simple additional call, it's easier to track the code flow.  But your
> patch essentially fakes the legacy codec and bus structs and twists
> the control by that switch...  which looks too fragile to me.

Especially when it's layered on top of all the other x86 code which is
also complicated and fragile - feels like there's lots of places things
can go wrong and lots of potential unexpected interactions.

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

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

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Mark Brown [mailto:broonie@kernel.org]
>Sent: Wednesday, December 20, 2017 3:56 PM
>To: Takashi Iwai <tiwai@suse.de>
>Cc: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa-
>project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
>louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
><patches.audio@intel.com>
>Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based
>HDA codec driver
>
>On Tue, Dec 19, 2017 at 08:17:37PM +0100, Takashi Iwai wrote:
>
>> Well, honestly speaking, that complete morphing scares me.  If it's a
>> simple additional call, it's easier to track the code flow.  But your
>> patch essentially fakes the legacy codec and bus structs and twists
>> the control by that switch...  which looks too fragile to me.
>
>Especially when it's layered on top of all the other x86 code which is
>also complicated and fragile - feels like there's lots of places things
>can go wrong and lots of potential unexpected interactions.

Thanks, for the feedback Mark and Takashi. I will send the test patch 
based on the suggestions from Takashi within couple of days.

Regards,
Rakesh

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Ughreja, Rakesh A
>Sent: Wednesday, December 20, 2017 4:23 PM
>To: Mark Brown <broonie@kernel.org>; Takashi Iwai <tiwai@suse.de>
>Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
>louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
><patches.audio@intel.com>
>Subject: RE: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
>codec driver
>

>>> Well, honestly speaking, that complete morphing scares me.  If it's a
>>> simple additional call, it's easier to track the code flow.  But your
>>> patch essentially fakes the legacy codec and bus structs and twists
>>> the control by that switch...  which looks too fragile to me.
>>
>>Especially when it's layered on top of all the other x86 code which is
>>also complicated and fragile - feels like there's lots of places things
>>can go wrong and lots of potential unexpected interactions.
>
>Thanks, for the feedback Mark and Takashi. I will send the test patch
>based on the suggestions from Takashi within couple of days.
>

Hi Takashi/Mark,

I tried working on the concept and it turned out to be much simpler
than what I thought. I added a type field for the bus which can be 
queried at runtime by the legacy codec driver and based on the type
it can call the callbacks of the hdac_hda library. With this change
the hdac_hda.c file is just a passive library and not a driver, which
provides APIs which are called by the legacy codec driver to work
with ASoC platform drivers.

There are only two callbacks required, i.e. probe and remove.
This is to register and unregister the asoc codec.

Let me know if this is the right way to go. I can send you the full series
once you are okay with the direction.

--- test patch below --
This patch adds bus type field in hdac_bus structure to distinguish the
bus instance. Bus allocated using snd_hdac_bus_init API sets the bus
type to HDA_BUS_LEGACY and snd_hdac_ext_bus_init API sets it to
HDA_BUS_EXT. codec drivers can identify the bus type by calling
snd_hdac_get_bus_type API.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 include/sound/hdaudio.h      | 15 +++++++++++++++
 sound/hda/ext/hdac_ext_bus.c |  1 +
 sound/hda/hdac_bus.c         |  1 +
 sound/pci/hda/hda_bind.c     | 14 ++++++++++++++
 sound/pci/hda/hda_codec.h    | 12 ++++++++++++
 5 files changed, 43 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 30ec1b0..9db6a11 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -97,6 +97,12 @@ enum {
 	HDA_DEV_ASOC,
 };
 
+enum {
+	HDA_BUS_INVALID,
+	HDA_BUS_LEGACY,
+	HDA_BUS_EXT,
+};
+
 /* direction */
 enum {
 	HDA_INPUT, HDA_OUTPUT
@@ -265,6 +271,7 @@ struct hdac_rb {
  */
 struct hdac_bus {
 	struct device *dev;
+	int type;
 	const struct hdac_bus_ops *ops;
 	const struct hdac_io_ops *io_ops;
 
@@ -341,6 +348,14 @@ struct hdac_bus {
 
 };
 
+static inline int snd_hdac_get_bus_type(struct hdac_bus *bus)
+{
+	if (bus)
+		return bus->type;
+	else
+		return HDA_BUS_INVALID;
+}
+
 int snd_hdac_bus_init(struct hdac_bus *bus, struct device *dev,
 		      const struct hdac_bus_ops *ops,
 		      const struct hdac_io_ops *io_ops);
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index e4bcb76..9d002da 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -102,6 +102,7 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	bus->type = HDA_BUS_EXT;
 	INIT_LIST_HEAD(&bus->hlink_list);
 	bus->idx = idx++;
 
diff --git a/sound/hda/hdac_bus.c b/sound/hda/hdac_bus.c
index 714a517..bb7567d 100644
--- a/sound/hda/hdac_bus.c
+++ b/sound/hda/hdac_bus.c
@@ -35,6 +35,7 @@ int snd_hdac_bus_init(struct hdac_bus *bus, struct device *dev,
 	else
 		bus->ops = &default_ops;
 	bus->io_ops = io_ops;
+	bus->type = HDA_BUS_LEGACY;
 	INIT_LIST_HEAD(&bus->stream_list);
 	INIT_LIST_HEAD(&bus->codec_list);
 	INIT_WORK(&bus->unsol_work, process_unsol_events);
diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index d361bb7..1998376 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -81,6 +81,15 @@ static int hda_codec_driver_probe(struct device *dev)
 	hda_codec_patch_t patch;
 	int err;
 
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
+	dev_err(dev, "BUS Type = %d\n", snd_hdac_get_bus_type(&codec->bus->core));
+	if (HDA_BUS_EXT == snd_hdac_get_bus_type(&codec->bus->core)) {
+		if (!codec->ext_ops)
+			return -EINVAL;
+		return codec->ext_ops->probe(&codec->core);
+	}
+#endif
+
 	if (WARN_ON(!codec->preset))
 		return -EINVAL;
 
@@ -134,6 +143,11 @@ static int hda_codec_driver_remove(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
+	if (HDA_BUS_EXT == snd_hdac_get_bus_type(&codec->bus->core)) {
+		return codec->ext_ops->remove(&codec->core);
+	}
+#endif
 	if (codec->patch_ops.free)
 		codec->patch_ops.free(codec);
 	snd_hda_codec_cleanup_for_unbind(codec);
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 8bbedf7..2b7b7db 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -108,6 +108,13 @@ void hda_codec_driver_unregister(struct hda_codec_driver *drv);
 	module_driver(drv, hda_codec_driver_register, \
 		      hda_codec_driver_unregister)
 
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
+struct snd_hda_ext_ops {
+	int (*probe)(struct hdac_device *hdev);
+	int (*remove)(struct hdac_device *hdev);
+};
+#endif
+
 /* ops set by the preset patch */
 struct hda_codec_ops {
 	int (*build_controls)(struct hda_codec *codec);
@@ -289,6 +296,11 @@ struct hda_codec {
 
 	/* additional init verbs */
 	struct snd_array verbs;
+
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
+        struct snd_hda_ext_ops *ext_ops;
+#endif
+
 };
 
 #define dev_to_hda_codec(_dev)	container_of(_dev, struct hda_codec, core.dev)
-- 
2.7.4

Regards,
Rakesh

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Thu, 21 Dec 2017 16:36:47 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Ughreja, Rakesh A
> >Sent: Wednesday, December 20, 2017 4:23 PM
> >To: Mark Brown <broonie@kernel.org>; Takashi Iwai <tiwai@suse.de>
> >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre-
> >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio
> ><patches.audio@intel.com>
> >Subject: RE: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
> >codec driver
> >
> 
> >>> Well, honestly speaking, that complete morphing scares me.  If it's a
> >>> simple additional call, it's easier to track the code flow.  But your
> >>> patch essentially fakes the legacy codec and bus structs and twists
> >>> the control by that switch...  which looks too fragile to me.
> >>
> >>Especially when it's layered on top of all the other x86 code which is
> >>also complicated and fragile - feels like there's lots of places things
> >>can go wrong and lots of potential unexpected interactions.
> >
> >Thanks, for the feedback Mark and Takashi. I will send the test patch
> >based on the suggestions from Takashi within couple of days.
> >
> 
> Hi Takashi/Mark,
> 
> I tried working on the concept and it turned out to be much simpler
> than what I thought. I added a type field for the bus which can be 
> queried at runtime by the legacy codec driver and based on the type
> it can call the callbacks of the hdac_hda library. With this change
> the hdac_hda.c file is just a passive library and not a driver, which
> provides APIs which are called by the legacy codec driver to work
> with ASoC platform drivers.
> 
> There are only two callbacks required, i.e. probe and remove.
> This is to register and unregister the asoc codec.
> 
> Let me know if this is the right way to go. I can send you the full series
> once you are okay with the direction.

Yes, this is a sort of idea I had.
My original thought was to have an extra ops in hdac_bus, and refer
directly there instead of setting in each hda_codec object.  Not coded
yet, so I can't judge which one is better.  Maybe you can actually
quick-try and test differences.

But still an open question is where to hook.  You've put the branch at
the very beginning of each probe/remove, that is, completely replacing
the whole probe/remove callbacks.  Meanwhile, the legacy codec driver
still expects the legacy hda_codec object handling, so keeping more
common stuff would make sense.  That is, if we do switching at the
beginning, the rest should just compose the same helper functions in
slightly different ways.  Or we share most parts of probe/remove in
both legacy and asoc but branch off after some later point in the
probe/remove functions.

In anyway, a whole patchset would be helpful so that I can give it a
try, too.  But maybe I'll have little time tomorrow and a few days
thereafter due to vacation.


thanks,

Takashi

> 
> --- test patch below --
> This patch adds bus type field in hdac_bus structure to distinguish the
> bus instance. Bus allocated using snd_hdac_bus_init API sets the bus
> type to HDA_BUS_LEGACY and snd_hdac_ext_bus_init API sets it to
> HDA_BUS_EXT. codec drivers can identify the bus type by calling
> snd_hdac_get_bus_type API.
> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>  include/sound/hdaudio.h      | 15 +++++++++++++++
>  sound/hda/ext/hdac_ext_bus.c |  1 +
>  sound/hda/hdac_bus.c         |  1 +
>  sound/pci/hda/hda_bind.c     | 14 ++++++++++++++
>  sound/pci/hda/hda_codec.h    | 12 ++++++++++++
>  5 files changed, 43 insertions(+)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 30ec1b0..9db6a11 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -97,6 +97,12 @@ enum {
>  	HDA_DEV_ASOC,
>  };
>  
> +enum {
> +	HDA_BUS_INVALID,
> +	HDA_BUS_LEGACY,
> +	HDA_BUS_EXT,
> +};
> +
>  /* direction */
>  enum {
>  	HDA_INPUT, HDA_OUTPUT
> @@ -265,6 +271,7 @@ struct hdac_rb {
>   */
>  struct hdac_bus {
>  	struct device *dev;
> +	int type;
>  	const struct hdac_bus_ops *ops;
>  	const struct hdac_io_ops *io_ops;
>  
> @@ -341,6 +348,14 @@ struct hdac_bus {
>  
>  };
>  
> +static inline int snd_hdac_get_bus_type(struct hdac_bus *bus)
> +{
> +	if (bus)
> +		return bus->type;
> +	else
> +		return HDA_BUS_INVALID;
> +}
> +
>  int snd_hdac_bus_init(struct hdac_bus *bus, struct device *dev,
>  		      const struct hdac_bus_ops *ops,
>  		      const struct hdac_io_ops *io_ops);
> diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
> index e4bcb76..9d002da 100644
> --- a/sound/hda/ext/hdac_ext_bus.c
> +++ b/sound/hda/ext/hdac_ext_bus.c
> @@ -102,6 +102,7 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
>  	if (ret < 0)
>  		return ret;
>  
> +	bus->type = HDA_BUS_EXT;
>  	INIT_LIST_HEAD(&bus->hlink_list);
>  	bus->idx = idx++;
>  
> diff --git a/sound/hda/hdac_bus.c b/sound/hda/hdac_bus.c
> index 714a517..bb7567d 100644
> --- a/sound/hda/hdac_bus.c
> +++ b/sound/hda/hdac_bus.c
> @@ -35,6 +35,7 @@ int snd_hdac_bus_init(struct hdac_bus *bus, struct device *dev,
>  	else
>  		bus->ops = &default_ops;
>  	bus->io_ops = io_ops;
> +	bus->type = HDA_BUS_LEGACY;
>  	INIT_LIST_HEAD(&bus->stream_list);
>  	INIT_LIST_HEAD(&bus->codec_list);
>  	INIT_WORK(&bus->unsol_work, process_unsol_events);
> diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
> index d361bb7..1998376 100644
> --- a/sound/pci/hda/hda_bind.c
> +++ b/sound/pci/hda/hda_bind.c
> @@ -81,6 +81,15 @@ static int hda_codec_driver_probe(struct device *dev)
>  	hda_codec_patch_t patch;
>  	int err;
>  
> +#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
> +	dev_err(dev, "BUS Type = %d\n", snd_hdac_get_bus_type(&codec->bus->core));
> +	if (HDA_BUS_EXT == snd_hdac_get_bus_type(&codec->bus->core)) {
> +		if (!codec->ext_ops)
> +			return -EINVAL;
> +		return codec->ext_ops->probe(&codec->core);
> +	}
> +#endif
> +
>  	if (WARN_ON(!codec->preset))
>  		return -EINVAL;
>  
> @@ -134,6 +143,11 @@ static int hda_codec_driver_remove(struct device *dev)
>  {
>  	struct hda_codec *codec = dev_to_hda_codec(dev);
>  
> +#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
> +	if (HDA_BUS_EXT == snd_hdac_get_bus_type(&codec->bus->core)) {
> +		return codec->ext_ops->remove(&codec->core);
> +	}
> +#endif
>  	if (codec->patch_ops.free)
>  		codec->patch_ops.free(codec);
>  	snd_hda_codec_cleanup_for_unbind(codec);
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index 8bbedf7..2b7b7db 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -108,6 +108,13 @@ void hda_codec_driver_unregister(struct hda_codec_driver *drv);
>  	module_driver(drv, hda_codec_driver_register, \
>  		      hda_codec_driver_unregister)
>  
> +#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
> +struct snd_hda_ext_ops {
> +	int (*probe)(struct hdac_device *hdev);
> +	int (*remove)(struct hdac_device *hdev);
> +};
> +#endif
> +
>  /* ops set by the preset patch */
>  struct hda_codec_ops {
>  	int (*build_controls)(struct hda_codec *codec);
> @@ -289,6 +296,11 @@ struct hda_codec {
>  
>  	/* additional init verbs */
>  	struct snd_array verbs;
> +
> +#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA)
> +        struct snd_hda_ext_ops *ext_ops;
> +#endif
> +
>  };
>  
>  #define dev_to_hda_codec(_dev)	container_of(_dev, struct hda_codec, core.dev)
> -- 
> 2.7.4
> 
> Regards,
> Rakesh
> 

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Thursday, December 21, 2017 9:18 PM
>To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
>Cc: Mark Brown <broonie@kernel.org>; alsa-devel@alsa-project.org; Koul, Vinod
><vinod.koul@intel.com>; pierre-louis.bossart@linux.intel.com;
>liam.r.girdwood@linux.intel.com; Patches Audio <patches.audio@intel.com>
>Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
>codec driver

>>
>> Let me know if this is the right way to go. I can send you the full series
>> once you are okay with the direction.
>
>Yes, this is a sort of idea I had.
>My original thought was to have an extra ops in hdac_bus, and refer
>directly there instead of setting in each hda_codec object.  Not coded
>yet, so I can't judge which one is better.  Maybe you can actually
>quick-try and test differences.

Let me try that out and will move to hdac_bus if I don't run into any
practical issues.

>
>But still an open question is where to hook.  You've put the branch at
>the very beginning of each probe/remove, that is, completely replacing
>the whole probe/remove callbacks.  Meanwhile, the legacy codec driver
>still expects the legacy hda_codec object handling, so keeping more
>common stuff would make sense.  That is, if we do switching at the
>beginning, the rest should just compose the same helper functions in
>slightly different ways.  Or we share most parts of probe/remove in
>both legacy and asoc but branch off after some later point in the
>probe/remove functions.

I think I am wrong in doing the above. I was thinking that I cannot do
all those steps done in the probe before the snd_soc_codec probe gets
called.

After looking at the code again after your comments, It looks like 
I can call all these before the card is registered by the machine driver.

Is that right understanding ? same thing is happening in legacy
driver also. The card is registered at the end. So I will just skip that step.

>
>In anyway, a whole patchset would be helpful so that I can give it a
>try, too.  But maybe I'll have little time tomorrow and a few days
>thereafter due to vacation.

Sure, will send you the series tomorrow by correcting few more things.

Regards,
Rakesh

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Thu, 21 Dec 2017 17:39:10 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Thursday, December 21, 2017 9:18 PM
> >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> >Cc: Mark Brown <broonie@kernel.org>; alsa-devel@alsa-project.org; Koul, Vinod
> ><vinod.koul@intel.com>; pierre-louis.bossart@linux.intel.com;
> >liam.r.girdwood@linux.intel.com; Patches Audio <patches.audio@intel.com>
> >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
> >codec driver
> 
> >>
> >> Let me know if this is the right way to go. I can send you the full series
> >> once you are okay with the direction.
> >
> >Yes, this is a sort of idea I had.
> >My original thought was to have an extra ops in hdac_bus, and refer
> >directly there instead of setting in each hda_codec object.  Not coded
> >yet, so I can't judge which one is better.  Maybe you can actually
> >quick-try and test differences.
> 
> Let me try that out and will move to hdac_bus if I don't run into any
> practical issues.
> 
> >
> >But still an open question is where to hook.  You've put the branch at
> >the very beginning of each probe/remove, that is, completely replacing
> >the whole probe/remove callbacks.  Meanwhile, the legacy codec driver
> >still expects the legacy hda_codec object handling, so keeping more
> >common stuff would make sense.  That is, if we do switching at the
> >beginning, the rest should just compose the same helper functions in
> >slightly different ways.  Or we share most parts of probe/remove in
> >both legacy and asoc but branch off after some later point in the
> >probe/remove functions.
> 
> I think I am wrong in doing the above. I was thinking that I cannot do
> all those steps done in the probe before the snd_soc_codec probe gets
> called.
> 
> After looking at the code again after your comments, It looks like 
> I can call all these before the card is registered by the machine driver.
> 
> Is that right understanding ? same thing is happening in legacy
> driver also. The card is registered at the end. So I will just skip that step.

Well, I'm not 100% sure about the ordering, so we just need trying.


> >In anyway, a whole patchset would be helpful so that I can give it a
> >try, too.  But maybe I'll have little time tomorrow and a few days
> >thereafter due to vacation.
> 
> Sure, will send you the series tomorrow by correcting few more things.

OK, thanks.


Takashi

Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Thursday, December 21, 2017 10:15 PM
>To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
>Cc: Mark Brown <broonie@kernel.org>; alsa-devel@alsa-project.org; Koul,
>Vinod <vinod.koul@intel.com>; pierre-louis.bossart@linux.intel.com;
>liam.r.girdwood@linux.intel.com; Patches Audio <patches.audio@intel.com>
>Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based
>HDA codec driver
>

>>
>> I think I am wrong in doing the above. I was thinking that I cannot do
>> all those steps done in the probe before the snd_soc_codec probe gets
>> called.
>>
>> After looking at the code again after your comments, It looks like
>> I can call all these before the card is registered by the machine driver.
>>
>> Is that right understanding ? same thing is happening in legacy
>> driver also. The card is registered at the end. So I will just skip that step.
>
>Well, I'm not 100% sure about the ordering, so we just need trying.
>

Hi Takashi,

I tried various things, but I could not unify the probe of the legacy hda codec
driver and the asoc library probe, mainly because all the APIs in the legacy
hda codec drivers require "snd_card" pointer, which is getting allocated by
the machine driver when the machine driver is loaded. So except the regmap
init I could not do anything in the legacy probe. So for now I have kept them
completely separate but I can do regmap init when the bus type is EXT.

Regards,
Rakesh