[PATCH 0/9] Add DSP topology management for SKL

[PATCH 0/9] Add DSP topology management for SKL

[Subhransu S. Prusty]

[RESEND of the series after removing the extra patches]

The SKL driver does not code DSP topology in driver. With this series
the soc topology framework is used to parse the topology information
to create widgets, controls and route map for the FW topology.

It adds routines for SKL DSP module configuration, compute resources
for modules, initialize and bind the pipe modules. It uses the SKL IPC
library added earlier to send IPC for initialize the module, bind/unbind
modules.

Last patch in this series removes the unused dais.

Jeeja KP (9):
  ASoC: Intel: Skylake: Add pipe and modules handlers
  ASoC: Intel: Skylake: Add module configuration helpers
  ASoC: Intel: Skylake: add DSP platform widget event handlers
  ASoC: Intel: Skylake: Add FE and BE hw_params handling
  ASoC: Intel: Skylake: Add topology core init and handlers
  ASoC: Intel: Skylake: Initialize and load DSP controls
  ASoC: Intel: Skylake: Add DSP support and enable it
  ASoC: Intel: Skylake: Initialize NHLT table
  ASoC: Intel: Skylake: Remove CPU dai that is not used

 sound/soc/intel/skylake/Makefile             |    3 +-
 sound/soc/intel/skylake/skl-pcm.c            |  173 ++--
 sound/soc/intel/skylake/skl-topology.c       | 1264 ++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl-topology.h       |   21 +
 sound/soc/intel/skylake/skl-tplg-interface.h |   78 ++
 sound/soc/intel/skylake/skl.c                |   29 +-
 sound/soc/intel/skylake/skl.h                |   11 +
 7 files changed, 1511 insertions(+), 68 deletions(-)
 create mode 100644 sound/soc/intel/skylake/skl-topology.c

-- 
2.4.3

[PATCH 1/9] ASoC: Intel: Skylake: Add pipe and modules handlers

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

SKL driver needs to instantiate pipelines and modules in the DSP. Here we
start adding building block for handling these. We add resource checks
(memory/mcps) for pipelines, find the modules in a pipeline, init modules
in a pipe and lastly bind/unbind modules in a pipe
These will be used by pipe event handlers in subsequent patches

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/Makefile       |   3 +-
 sound/soc/intel/skylake/skl-topology.c | 219 +++++++++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl-topology.h |  10 ++
 sound/soc/intel/skylake/skl.h          |  11 ++
 4 files changed, 242 insertions(+), 1 deletion(-)
 create mode 100644 sound/soc/intel/skylake/skl-topology.c

diff --git a/sound/soc/intel/skylake/Makefile b/sound/soc/intel/skylake/Makefile
index 27db221..914b6da 100644
--- a/sound/soc/intel/skylake/Makefile
+++ b/sound/soc/intel/skylake/Makefile
@@ -1,4 +1,5 @@
-snd-soc-skl-objs := skl.o skl-pcm.o skl-nhlt.o skl-messages.o
+snd-soc-skl-objs := skl.o skl-pcm.o skl-nhlt.o skl-messages.o \
+skl-topology.o
 
 obj-$(CONFIG_SND_SOC_INTEL_SKYLAKE) += snd-soc-skl.o
 
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
new file mode 100644
index 0000000..abd1c16
--- /dev/null
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -0,0 +1,219 @@
+/*
+ *  skl-topology.c - Implements Platform component ALSA controls/widget
+ *  handlers.
+ *
+ *  Copyright (C) 2014-2015 Intel Corp
+ *  Author: Jeeja KP <jeeja.kp@intel.com>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as 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/slab.h>
+#include <linux/types.h>
+#include <linux/firmware.h>
+#include <sound/soc.h>
+#include <sound/soc-topology.h>
+#include "skl-sst-dsp.h"
+#include "skl-sst-ipc.h"
+#include "skl-topology.h"
+#include "skl.h"
+#include "skl-tplg-interface.h"
+
+/*
+ * SKL DSP driver modelling uses only few DAPM widgets so for rest we will
+ * ignore. This helpers checks if the SKL driver handles this widget type
+ */
+static int is_skl_dsp_widget_type(struct snd_soc_dapm_widget *w)
+{
+	switch (w->id) {
+	case snd_soc_dapm_dai_link:
+	case snd_soc_dapm_dai_in:
+	case snd_soc_dapm_aif_in:
+	case snd_soc_dapm_aif_out:
+	case snd_soc_dapm_dai_out:
+	case snd_soc_dapm_switch:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static int skl_tplg_bind_unbind_pipes(struct skl_module_cfg *src_module,
+	struct skl_module_cfg *sink_module, struct skl_sst *ctx,  bool bind)
+{
+	int ret;
+
+	if (!bind) {
+		ret = skl_stop_pipe(ctx, src_module->pipe);
+		if (ret < 0)
+			return ret;
+
+		ret = skl_unbind_modules(ctx, src_module, sink_module);
+	} else {
+		ret = skl_bind_modules(ctx, src_module, sink_module);
+	}
+
+	return ret;
+}
+
+/*
+ * Each pipelines needs memory to be allocated. Check if we have free memory
+ * from available pool. Then only add this to pool
+ * This is freed when pipe is deleted
+ * Note: DSP does actual memory management we only keep track for complete
+ * pool
+ */
+static bool skl_tplg_is_pipe_mem_available(struct skl *skl,
+				struct skl_module_cfg *mconfig)
+{
+	struct skl_sst *ctx = skl->skl_sst;
+
+	dev_dbg(ctx->dev, "%s: module_id =%d instance=%d\n", __func__,
+		 mconfig->id.module_id, mconfig->id.instance_id);
+
+	if (skl->resource.mem + mconfig->pipe->memory_pages > skl->resource.max_mem) {
+		dev_err(ctx->dev, "exceeds ppl memory available=%d > mem=%d\n",
+				skl->resource.max_mem, skl->resource.mem);
+		return false;
+	}
+
+	skl->resource.mem += mconfig->pipe->memory_pages;
+	return true;
+}
+
+/*
+ * Each pipelines needs mcps to be allocated. Check if we have mcps for this
+ * pipe. This adds the mcps to driver counter
+ * This is removed on pipeline delete
+ */
+static bool skl_tplg_is_pipe_mcps_available(struct skl *skl,
+				struct skl_module_cfg *mconfig)
+{
+	struct skl_sst *ctx = skl->skl_sst;
+
+	dev_dbg(ctx->dev, "%s: module_id = %d instance=%d\n", __func__,
+			mconfig->id.module_id, mconfig->id.instance_id);
+
+	if (skl->resource.mcps + mconfig->mcps > skl->resource.max_mcps) {
+		dev_err(ctx->dev, "exceeds ppl memory available=%d > mem=%d\n",
+				skl->resource.max_mcps, skl->resource.mcps);
+		return false;
+	}
+
+	skl->resource.mcps += mconfig->mcps;
+	return true;
+}
+
+/*
+ * A pipe can have multiple modules each of the will be a DAPM widget as
+ * well. While managing a pipeline we need to get the list of all the
+ * widgets in a pipelines, so this helper - skl_tplg_get_pipe_widget() helps
+ * to get the SKL type widgets in that pipeline
+ */
+static int skl_tplg_get_pipe_widget(struct device *dev,
+	struct snd_soc_dapm_widget *w, struct skl_pipe *pipe)
+{
+	struct skl_module_cfg *src_module = NULL;
+	struct snd_soc_dapm_path *p = NULL;
+	struct skl_pipe_module *p_module = NULL;
+
+	dev_dbg(dev, "In%s widget=%s\n", __func__, w->name);
+
+	p_module = devm_kzalloc(dev, sizeof(*p_module), GFP_KERNEL);
+	if (!p_module)
+		return -ENOMEM;
+
+	p_module->w = w;
+	list_add_tail(&p_module->node, &pipe->w_list);
+
+	list_for_each_entry(p, &w->sinks, list_source) {
+		if ((p->sink->priv == NULL)
+			&& (!is_skl_dsp_widget_type(w)))
+			continue;
+
+		if ((p->sink->priv != NULL) && (p->connect)
+			&& (is_skl_dsp_widget_type(p->sink))) {
+			src_module = p->sink->priv;
+			if (pipe->ppl_id == src_module->pipe->ppl_id) {
+				dev_dbg(dev, "found widget=%s\n", p->sink->name);
+				skl_tplg_get_pipe_widget(dev, p->sink, pipe);
+			}
+		}
+	}
+	return 0;
+}
+
+/*
+ * Inside a pipe instance, we can have various modules. These modules need
+ * to instantiated in DSP by invoking INIT_MODULE IPC, which is achieved by
+ * skl_init_module() routine, so invoke that for all modules in a pipeline
+ */
+static int skl_tplg_init_pipe_modules(struct skl *skl, struct skl_pipe *pipe)
+{
+	struct skl_pipe_module *w_module;
+	struct snd_soc_dapm_widget *w;
+	struct skl_module_cfg *mconfig;
+	struct skl_sst *ctx = skl->skl_sst;
+	int ret = 0;
+
+	dev_dbg(ctx->dev, "%s: pipe=%d\n", __func__, pipe->ppl_id);
+	list_for_each_entry(w_module, &pipe->w_list, node) {
+		w = w_module->w;
+		dev_dbg(ctx->dev, "Pipe Module =%s\n", w->name);
+		mconfig = w->priv;
+
+		/* check resource available */
+		if (!skl_tplg_is_pipe_mcps_available(skl, mconfig))
+			return -ENOMEM;
+
+		ret = skl_init_module(ctx, mconfig, NULL);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Once all the modules in a pipe are instantiated, they need to be
+ * connected.
+ * On removal, before deleting a pipeline the modules need to disconnected.
+ *
+ * This is achieved by binding/unbinding these modules
+ */
+static int skl_tplg_bind_unbind_pipe_modules(struct skl_sst *ctx,
+				struct skl_pipe *pipe, bool bind)
+{
+	struct skl_pipe_module *w_module;
+	struct skl_module_cfg *src_module = NULL;
+	struct skl_module_cfg *dst_module;
+	int ret = 0;
+
+	dev_dbg(ctx->dev, "%s: pipe=%d\n", __func__, pipe->ppl_id);
+	list_for_each_entry(w_module, &pipe->w_list, node) {
+		dst_module  = w_module->w->priv;
+
+		if (src_module == NULL) {
+			src_module = dst_module;
+			continue;
+		}
+
+		if (bind)
+			ret = skl_bind_modules(ctx, src_module, dst_module);
+		else
+			ret = skl_unbind_modules(ctx, src_module, dst_module);
+		if (ret < 0)
+			return ret;
+
+		src_module = dst_module;
+	}
+	return 0;
+}
diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
index 8c7767b..73d7916 100644
--- a/sound/soc/intel/skylake/skl-topology.h
+++ b/sound/soc/intel/skylake/skl-topology.h
@@ -263,6 +263,16 @@ struct skl_module_cfg {
 	struct skl_specific_cfg formats_config;
 };
 
+struct skl_pipeline {
+	struct skl_pipe *pipe;
+	struct list_head node;
+};
+
+struct skl_dapm_path_list {
+	struct snd_soc_dapm_path *dapm_path;
+	struct list_head node;
+};
+
 int skl_create_pipeline(struct skl_sst *ctx, struct skl_pipe *pipe);
 
 int skl_run_pipe(struct skl_sst *ctx, struct skl_pipe *pipe);
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index f7fdbb0..e980d78 100644
--- a/sound/soc/intel/skylake/skl.h
+++ b/sound/soc/intel/skylake/skl.h
@@ -48,6 +48,13 @@
 #define AZX_REG_VS_SDXEFIFOS_XBASE	0x1094
 #define AZX_REG_VS_SDXEFIFOS_XINTERVAL	0x20
 
+struct skl_dsp_resource {
+	u32 max_mcps;
+	u32 max_mem;
+	u32 mcps;
+	u32 mem;
+};
+
 struct skl {
 	struct hdac_ext_bus ebus;
 	struct pci_dev *pci;
@@ -57,6 +64,10 @@ struct skl {
 
 	void __iomem *nhlt; /* nhlt ptr */
 	struct skl_sst *skl_sst; /* sst skl ctx */
+
+	struct skl_dsp_resource resource;
+	struct list_head ppl_list;
+	struct list_head dapm_path_list;
 };
 
 #define skl_to_ebus(s)	(&(s)->ebus)
-- 
2.4.3

Re: [PATCH 1/9] ASoC: Intel: Skylake: Add pipe and modules handlers

[Mark Brown]

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

On Sat, Aug 08, 2015 at 01:06:16AM +0530, Subhransu S. Prusty wrote:

> start adding building block for handling these. We add resource checks
> (memory/mcps) for pipelines, find the modules in a pipeline, init modules

What are "mcps"?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 1/9] ASoC: Intel: Skylake: Add pipe and modules handlers

[Vinod Koul]

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

On Fri, Aug 14, 2015 at 10:30:31PM +0100, Mark Brown wrote:
> On Sat, Aug 08, 2015 at 01:06:16AM +0530, Subhransu S. Prusty wrote:
> 
> > start adding building block for handling these. We add resource checks
> > (memory/mcps) for pipelines, find the modules in a pipeline, init modules
> 
> What are "mcps"?

Million Clocks Per Second. This represents the compute requirement of a
module, pipe and driver needs to track it so that we don't exceed available
MCPS

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 1/9] ASoC: Intel: Skylake: Add pipe and modules handlers

[Mark Brown]

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

On Sat, Aug 15, 2015 at 06:25:13PM +0530, Vinod Koul wrote:
> On Fri, Aug 14, 2015 at 10:30:31PM +0100, Mark Brown wrote:

> > What are "mcps"?

> Million Clocks Per Second. This represents the compute requirement of a
> module, pipe and driver needs to track it so that we don't exceed available
> MCPS

So expand that acronym somewhere people can find  it...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 1/9] ASoC: Intel: Skylake: Add pipe and modules handlers

[Vinod Koul]

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

On Sat, Aug 15, 2015 at 10:03:02AM -0700, Mark Brown wrote:
> On Sat, Aug 15, 2015 at 06:25:13PM +0530, Vinod Koul wrote:
> > On Fri, Aug 14, 2015 at 10:30:31PM +0100, Mark Brown wrote:
> 
> > > What are "mcps"?
> 
> > Million Clocks Per Second. This represents the compute requirement of a
> > module, pipe and driver needs to track it so that we don't exceed available
> > MCPS
> 
> So expand that acronym somewhere people can find  it...

Sure will add in rev2

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[PATCH 2/9] ASoC: Intel: Skylake: Add module configuration helpers

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

A module's parameter needs to be updated if there is a src or updown mixer
or a converter in the pipeline. The helper function here updates the module
parameter based on FE/BE hw_params if fixup/conversion is required.

Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com>
Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 124 +++++++++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index abd1c16..ede2825 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -27,6 +27,10 @@
 #include "skl.h"
 #include "skl-tplg-interface.h"
 
+#define SKL_CH_FIXUP_MASK		(1 << 0)
+#define SKL_RATE_FIXUP_MASK		(1 << 1)
+#define SKL_FMT_FIXUP_MASK		(1 << 2)
+
 /*
  * SKL DSP driver modelling uses only few DAPM widgets so for rest we will
  * ignore. This helpers checks if the SKL driver handles this widget type
@@ -112,6 +116,124 @@ static bool skl_tplg_is_pipe_mcps_available(struct skl *skl,
 	return true;
 }
 
+static void skl_dump_mconfig(struct skl_sst *ctx,
+					struct skl_module_cfg *mcfg)
+{
+	dev_dbg(ctx->dev, "Dumping config\n");
+	dev_dbg(ctx->dev, "Input Format:\n");
+	dev_dbg(ctx->dev, "channels = %d\n", mcfg->in_fmt.channels);
+	dev_dbg(ctx->dev, "s_freq = %d\n", mcfg->in_fmt.s_freq);
+	dev_dbg(ctx->dev, "ch_cfg = %d\n", mcfg->in_fmt.ch_cfg);
+	dev_dbg(ctx->dev, "valid bit depth = %d\n", mcfg->in_fmt.valid_bit_depth);
+	dev_dbg(ctx->dev, "Output Format:\n");
+	dev_dbg(ctx->dev, "channels = %d\n", mcfg->out_fmt.channels);
+	dev_dbg(ctx->dev, "s_freq = %d\n", mcfg->out_fmt.s_freq);
+	dev_dbg(ctx->dev, "valid bit depth = %d\n", mcfg->out_fmt.valid_bit_depth);
+	dev_dbg(ctx->dev, "ch_cfg = %d\n", mcfg->out_fmt.ch_cfg);
+}
+
+static void skl_tplg_update_params(struct skl_module_fmt *fmt,
+			struct skl_pipe_params *params, int fixup)
+{
+	if (fixup & SKL_RATE_FIXUP_MASK)
+		fmt->s_freq = params->s_freq;
+	if (fixup & SKL_CH_FIXUP_MASK)
+		fmt->channels = params->ch;
+	if (fixup & SKL_FMT_FIXUP_MASK)
+		fmt->valid_bit_depth = params->s_fmt;
+}
+
+/*
+ * A pipeline may have modules which impact the pcm parameters, like SRC,
+ * channel converter, format converter.
+ * We need to calculate the output params by applying the 'fixup'
+ * Topology will tell driver which type of fixup is to be applied by
+ * supplying the fixup mask, so based on that we calculate the output
+ *
+ * Now In FE the pcm hw_params is source/target format. Same is applicable
+ * for BE with its hw_params invoked.
+ * here based on FE, BE pipeline and direction we calculate the input and
+ * outfix and then apply that for a module
+ */
+static void skl_tplg_update_params_fixup(struct skl_module_cfg *m_cfg,
+		struct skl_pipe_params *params, bool is_fe) { int in_fixup,
+	out_fixup; struct skl_module_fmt *in_fmt, *out_fmt;
+
+	in_fmt = &m_cfg->in_fmt;
+	out_fmt = &m_cfg->out_fmt;
+
+	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		if (is_fe) {
+			in_fixup = m_cfg->params_fixup;
+			out_fixup = (~m_cfg->converter) & m_cfg->params_fixup;
+		} else {
+			out_fixup = m_cfg->params_fixup;
+			in_fixup = (~m_cfg->converter) & m_cfg->params_fixup;
+		}
+	} else {
+		if (is_fe) {
+			out_fixup = m_cfg->params_fixup;
+			in_fixup = (~m_cfg->converter) & m_cfg->params_fixup;
+		} else {
+			in_fixup = m_cfg->params_fixup;
+			out_fixup = (~m_cfg->converter) & m_cfg->params_fixup;
+		}
+	}
+
+	skl_tplg_update_params(in_fmt, params, in_fixup);
+	skl_tplg_update_params(out_fmt, params, out_fixup);
+}
+
+/*
+ * A module needs input and output buffers, which are dependent upon pcm
+ * params, so once we have calculate params, we need buffer calculation as
+ * well.
+ */
+static void skl_tplg_update_buffer_size(struct skl_sst *ctx,
+				struct skl_module_cfg *mcfg)
+{
+	int multiplier = 1;
+
+	if (mcfg->m_type == SKL_MODULE_TYPE_SRCINT)
+		multiplier = 5;
+
+	mcfg->ibs = (mcfg->in_fmt.s_freq / 1000) *
+				(mcfg->in_fmt.channels) *
+				(mcfg->in_fmt.bit_depth >> 3) *
+				multiplier;
+
+	mcfg->obs = (mcfg->out_fmt.s_freq / 1000) *
+				(mcfg->out_fmt.channels) *
+				(mcfg->out_fmt.bit_depth >> 3) *
+				multiplier;
+}
+
+static void skl_tplg_update_module_params(struct snd_soc_dapm_widget *w,
+							struct skl_sst *ctx)
+{
+	struct skl_module_cfg *m_cfg = w->priv;
+	struct skl_pipe_params *params = m_cfg->pipe->p_params;
+	int p_conn_type = m_cfg->pipe->conn_type;
+	bool is_fe;
+
+	if (!m_cfg->params_fixup)
+		return;
+
+	dev_dbg(ctx->dev, "Mconfig for widget=%s BEFORE updation\n", w->name);
+	skl_dump_mconfig(ctx, m_cfg);
+
+	if (p_conn_type == SKL_PIPE_CONN_TYPE_FE)
+		is_fe = true;
+	else
+		is_fe = false;
+
+	skl_tplg_update_params_fixup(m_cfg, params, is_fe);
+	skl_tplg_update_buffer_size(ctx, m_cfg);
+
+	dev_dbg(ctx->dev, "Mconfig for widget=%s AFTER updation\n", w->name);
+	skl_dump_mconfig(ctx, m_cfg);
+}
+
 /*
  * A pipe can have multiple modules each of the will be a DAPM widget as
  * well. While managing a pipeline we need to get the list of all the
@@ -174,6 +296,8 @@ static int skl_tplg_init_pipe_modules(struct skl *skl, struct skl_pipe *pipe)
 		if (!skl_tplg_is_pipe_mcps_available(skl, mconfig))
 			return -ENOMEM;
 
+		/* apply fix/conversion to module params based on FE/BE params*/
+		skl_tplg_update_module_params(w, ctx);
 		ret = skl_init_module(ctx, mconfig, NULL);
 		if (ret < 0)
 			return ret;
-- 
2.4.3

[PATCH 3/9] ASoC: Intel: Skylake: add DSP platform widget event handlers

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

This adds widget event handlers for PRE/POST PMU and PRE/POST PMD
event for mixer and pga modules. The SKL topology uses PGA and
mixer modules to model a topology and manage that with DSP.

These event handlers invoke pipeline creation, destroy, module creation,
module bind, unbind and pipeline bind unbind

Event handler sequencing is implement to target the DSP FW sequence
expectations to enable path from source to sink pipe for Playback/Capture.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 411 +++++++++++++++++++++++++++++++++
 1 file changed, 411 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index ede2825..deb5c0d 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -341,3 +341,414 @@ static int skl_tplg_bind_unbind_pipe_modules(struct skl_sst *ctx,
 	}
 	return 0;
 }
+
+/*
+ * in the Pre-PMU event of mixer we need to do following:
+ *   - check the resources
+ *   - Create the pipeline
+ *   - Initialize the modules in pipeline
+ *   - finally bind all modules together
+ */
+static int skl_tplg_mixer_dapm_pre_pmu_event(struct snd_soc_dapm_widget *w,
+								struct skl *skl)
+{
+	int ret;
+	struct skl_module_cfg *mconfig = w->priv;
+	struct skl_pipe *s_pipe = mconfig->pipe;
+	struct skl_sst *ctx = skl->skl_sst;
+
+	dev_dbg(ctx->dev, "%s: widget =%s\n", __func__, w->name);
+
+	/* check resource available */
+	if (!skl_tplg_is_pipe_mcps_available(skl, mconfig))
+		return -EBUSY;
+
+	if (!skl_tplg_is_pipe_mem_available(skl, mconfig))
+		return -ENOMEM;
+
+	/*
+	 * Create list of modules for pipe
+	 * list contains modules from source to sink
+	 */
+	ret = skl_create_pipeline(ctx, mconfig->pipe);
+	if (ret < 0)
+		return ret;
+
+	if (list_empty(&s_pipe->w_list)) {
+		ret = skl_tplg_get_pipe_widget(ctx->dev, w, s_pipe);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Init all pipe modules from source to sink */
+	ret = skl_tplg_init_pipe_modules(skl, s_pipe);
+	if (ret < 0)
+		return ret;
+
+	/* Bind modules from source to sink */
+	return skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, true);
+}
+/*
+ * in the Pre-PMU event of PGA we need to do following:
+ *   - Bind to sink pipeline
+ *   - Start sink pipeline, if not running
+ *   - Then run current pipe
+ */
+static int skl_tplg_pga_dapm_pre_pmu_event(struct snd_soc_dapm_widget *w,
+								struct skl *skl)
+{
+	struct snd_soc_dapm_path *p;
+	struct skl_dapm_path_list *path_list;
+	struct snd_soc_dapm_widget *source, *sink;
+	struct skl_module_cfg *src_mconfig, *sink_mconfig;
+	struct skl_sst *ctx = skl->skl_sst;
+	int ret = 0;
+
+	dev_dbg(ctx->dev, "%s: widget =%s\n", __func__, w->name);
+
+	source = w;
+
+	/* If its not SKL DSP type dont handle */
+	if (source->priv == NULL &&  (!is_skl_dsp_widget_type(w)))
+		return 0;
+
+	src_mconfig = source->priv;
+
+	/*
+	 * find which sink it is connected to, bind with the sink,
+	 * if sink is not started, start sink pipe first, then start
+	 * this pipe
+	 */
+	list_for_each_entry(p, &w->sinks, list_source) {
+		if (!p->connect)
+			continue;
+
+		dev_dbg(ctx->dev, "%s: src widget=%s\n", __func__, w->name);
+		dev_dbg(ctx->dev, "%s: sink widget=%s\n", __func__, p->sink->name);
+
+		if ((p->sink->priv != NULL) && is_skl_dsp_widget_type(p->sink)) {
+
+			sink = p->sink;
+			src_mconfig = source->priv;
+			sink_mconfig = sink->priv;
+
+			/* Bind source to sink, mixin is always source */
+			ret = skl_tplg_bind_unbind_pipes(src_mconfig,
+						sink_mconfig, ctx, true);
+			if (ret)
+				return ret;
+
+			/* Start sinks pipe first */
+			if (sink_mconfig->pipe->state != SKL_PIPE_STARTED) {
+				ret = skl_run_pipe(ctx, sink_mconfig->pipe);
+				if (ret)
+					return ret;
+			}
+
+			path_list = kzalloc(sizeof(struct skl_dapm_path_list),
+								GFP_KERNEL);
+			if (NULL == path_list)
+				return -ENOMEM;
+
+			/* Add connected path to one global list */
+			path_list->dapm_path = p;
+			list_add_tail(&path_list->node, &skl->dapm_path_list);
+			break;
+		}
+	}
+
+	/* Start source pipe last after starting all sinks */
+	ret = skl_run_pipe(ctx, src_mconfig->pipe);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * in the Post-PMU event of mixer we need to do following:
+ *   - Check if this pipe is running
+ *   - if not, then
+ *	- bind this pipeline to its source pipeline
+ *	- start this pipeline
+ */
+static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w,
+								struct skl *skl)
+{
+	int ret = 0;
+	struct snd_soc_dapm_path *p;
+	struct snd_soc_dapm_widget *source, *sink;
+	struct skl_module_cfg *src_mconfig, *sink_mconfig;
+	struct skl_sst *ctx = skl->skl_sst;
+	int src_pipe_started = 0;
+
+	dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name);
+
+	sink = w;
+
+	/* If this is not a SKL DSP widget, nothing to be done */
+	if (sink->priv == NULL &&  (!is_skl_dsp_widget_type(w)))
+		return 0;
+
+	sink_mconfig = sink->priv;
+
+	/*
+	 * If source pipe is already started, that means source is driving
+	 * one more sink before this sink got connected, Since source is
+	 * started, bind this sink to source and start this pipe.
+	 */
+	list_for_each_entry(p, &w->sources, list_sink) {
+		if (!p->connect)
+			continue;
+		dev_dbg(ctx->dev, "sink widget=%s\n", w->name);
+		dev_dbg(ctx->dev, "src widget=%s\n", p->source->name);
+
+		if ((p->source->priv != NULL) && is_skl_dsp_widget_type(p->source)) {
+			source = p->source;
+			src_mconfig = source->priv;
+			sink_mconfig = sink->priv;
+			src_pipe_started = 1;
+
+			/*
+			 * check pipe state, then no need to bind or start the
+			 * pipe
+			 */
+			if (src_mconfig->pipe->state != SKL_PIPE_STARTED)
+				src_pipe_started = 0;
+		}
+	}
+	if (src_pipe_started) {
+		ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig,
+							 ctx, true);
+		if (ret)
+			return ret;
+
+		return skl_run_pipe(ctx, sink_mconfig->pipe);
+	}
+
+	return ret;
+}
+
+/*
+ * in the Pre-PMD event of mixer we need to do following:
+ *   - Stop the pipe
+ *   - find the source connections
+ *   - unbind with source pipelines if still connected
+ */
+static int skl_tplg_mixer_dapm_pre_pmd_event(struct snd_soc_dapm_widget *w,
+								struct skl *skl)
+{
+	struct snd_soc_dapm_widget *source, *sink;
+	struct skl_module_cfg *src_mconfig, *sink_mconfig;
+	int ret = 0, path_found = 0;
+	struct skl_dapm_path_list *path_list, *tmp_list;
+	struct skl_sst *ctx = skl->skl_sst;
+
+	dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name);
+
+	sink = w;
+	if (sink->priv == NULL &&  (!is_skl_dsp_widget_type(w)))
+		return 0;
+
+	sink_mconfig = sink->priv;
+	/* Stop the pipe */
+	ret = skl_stop_pipe(ctx, sink_mconfig->pipe);
+	if (ret)
+		return ret;
+
+	list_for_each_entry_safe(path_list, tmp_list, &skl->dapm_path_list, node) {
+		if (path_list->dapm_path->sink == sink) {
+			dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name);
+			source = path_list->dapm_path->source;
+			src_mconfig = source->priv;
+			path_found = 1;
+
+			list_del(&path_list->node);
+			kfree(path_list);
+			break;
+		}
+	}
+
+	/*
+	 * If path_found == 1, that means pmd for source pipe has
+	 * not occurred, source is connected to some other sink.
+	 * so its responsibility of sink to unbind itself from source.
+	 */
+	if (path_found)
+		ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig,
+							 ctx, false);
+
+	return ret;
+}
+
+/*
+ * in the Post-PMD event of mixer we need to do following:
+ *   - Free the mcps used
+ *   - Free the mem used
+ *   - Unbind the modules within the pipeline
+ *   - Delete the pipeline (modules are not required to be explicitly
+ *     deleted, pipeline delete is enough here
+ */
+static int skl_tplg_mixer_dapm_post_pmd_event(struct snd_soc_dapm_widget *w,
+								struct skl *skl)
+{
+	struct skl_module_cfg *mconfig = w->priv;
+	int ret = 0;
+	struct skl_sst *ctx = skl->skl_sst;
+	struct skl_pipe *s_pipe = mconfig->pipe;
+
+	dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name);
+
+	skl->resource.mcps -= mconfig->mcps;
+
+	ret = skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, false);
+	if (ret < 0)
+		return ret;
+
+	ret = skl_delete_pipe(ctx, mconfig->pipe);
+	skl->resource.mem -= mconfig->pipe->memory_pages;
+
+	return ret;
+}
+
+/*
+ * in the Post-PMD event of PGA we need to do following:
+ *   - Free the mcps used
+ *   - Stop the pipeline
+ *   - In source pipe is connected, unbind with source pipelines
+ */
+static int skl_tplg_pga_dapm_post_pmd_event(struct snd_soc_dapm_widget *w,
+								struct skl *skl)
+{
+	struct snd_soc_dapm_widget *source, *sink;
+	struct skl_module_cfg *src_mconfig, *sink_mconfig;
+	int ret = 0, path_found = 0;
+	struct skl_dapm_path_list *path_list, *tmp_path_list;
+	struct skl_sst *ctx = skl->skl_sst;
+
+	dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name);
+
+	source = w;
+	if (source->priv == NULL &&  (!is_skl_dsp_widget_type(w)))
+		return 0;
+
+	src_mconfig = source->priv;
+
+	skl->resource.mcps -= src_mconfig->mcps;
+	/* Stop the pipe since this is a mixin module */
+	ret = skl_stop_pipe(ctx, src_mconfig->pipe);
+	if (ret)
+		return ret;
+
+	list_for_each_entry_safe(path_list, tmp_path_list, &skl->dapm_path_list, node) {
+		if (path_list->dapm_path->source == source) {
+			dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name);
+			sink = path_list->dapm_path->sink;
+			sink_mconfig = sink->priv;
+			path_found = 1;
+
+			list_del(&path_list->node);
+			kfree(path_list);
+			break;
+		}
+	}
+
+	/*
+	 * This is a connecter and if path is found that means
+	 * unbind between source and sink has not happened yet
+	 */
+	if (path_found)
+		ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig,
+							 ctx, false);
+
+	return ret;
+}
+
+static struct skl *get_skl_ctx(struct device *dev)
+{
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dev);
+
+	return ebus_to_skl(ebus);
+}
+
+/* In modelling, we assume ther will be one mixer in a pipeline.
+ * If mixer is not required then it is treated as static mixer aka vmixer
+ * with a hard path to source module
+ * So we dont need to check if source is started or not as hard path puts
+ * dependency on each other
+ */
+static int skl_tplg_vmixer_event(struct snd_soc_dapm_widget *w,
+				struct snd_kcontrol *k, int event)
+{
+	struct snd_soc_dapm_context *dapm = w->dapm;
+	struct skl *skl = get_skl_ctx(dapm->dev);
+
+	dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event);
+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		return skl_tplg_mixer_dapm_pre_pmu_event(w, skl);
+
+	case SND_SOC_DAPM_POST_PMD:
+		return skl_tplg_mixer_dapm_post_pmd_event(w, skl);
+	}
+
+	return 0;
+}
+
+/*
+ * In modelling, we assume ther will be one mixer in a pipeline. If a second
+ * one is required that is created as another pipe entity.
+ * The mixer is resposible for pipe management and represent a pipeline
+ * instance
+ */
+static int skl_tplg_mixer_event(struct snd_soc_dapm_widget *w,
+				struct snd_kcontrol *k, int event)
+{
+	struct snd_soc_dapm_context *dapm = w->dapm;
+	struct skl *skl = get_skl_ctx(dapm->dev);
+
+	dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event);
+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		return skl_tplg_mixer_dapm_pre_pmu_event(w, skl);
+
+	case SND_SOC_DAPM_POST_PMU:
+		return skl_tplg_mixer_dapm_post_pmu_event(w, skl);
+
+	case SND_SOC_DAPM_PRE_PMD:
+		return skl_tplg_mixer_dapm_pre_pmd_event(w, skl);
+
+	case SND_SOC_DAPM_POST_PMD:
+		return skl_tplg_mixer_dapm_post_pmd_event(w, skl);
+	}
+
+	return 0;
+}
+
+/*
+ * In modelling, we assumed rest of the modules in pipeline are PGA. But we
+ * are interested in last PGA (leaf PGA) in a pipeline to disconnect with
+ * the sink when it is running (two FE to one BE or one FE to two BE)
+ * scenarios
+ */
+static int skl_tplg_pga_event(struct snd_soc_dapm_widget *w,
+		struct snd_kcontrol *k, int event)
+
+{
+	struct snd_soc_dapm_context *dapm = w->dapm;
+	struct skl *skl = get_skl_ctx(dapm->dev);
+
+	dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event);
+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		return skl_tplg_pga_dapm_pre_pmu_event(w, skl);
+
+	case SND_SOC_DAPM_POST_PMD:
+		return skl_tplg_pga_dapm_post_pmd_event(w, skl);
+	}
+
+	return 0;
+}
-- 
2.4.3

Re: [PATCH 3/9] ASoC: Intel: Skylake: add DSP platform widget event handlers

[Mark Brown]

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

On Sat, Aug 08, 2015 at 01:06:18AM +0530, Subhransu S. Prusty wrote:

> +	/* If its not SKL DSP type dont handle */
> +	if (source->priv == NULL &&  (!is_skl_dsp_widget_type(w)))
> +		return 0;

Why would this event be assigned to a widget that is not a suitable
type, and what if it's an unsuitable widget type that happens ton have
private data?  I'm not sure I understand the priv check.

You've also got a typo with "don't" and an extra space after the &&.

> +	list_for_each_entry(p, &w->sinks, list_source) {
> +		if (!p->connect)
> +			continue;

Hrm.  Do we handle routing changes well?

> +			/* Add connected path to one global list */
> +			path_list->dapm_path = p;
> +			list_add_tail(&path_list->node, &skl->dapm_path_list);
> +			break;

We have our own private path list here which includes the DAPM path...
can't we outsource some of this to the topology and DAPM code?  I'm a
little fuzzy on what the list is for, it appears to be used mainly for
deallocation though it's called dapm_path_list (which is a bit worrying).

> +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w,
> +								struct skl *skl)
> +{

This looks a lot like the previous function, there's similar duplication
in the other event functions.

> +	}
> +	if (src_pipe_started) {

Missing blank line here.

> +/*
> + * In modelling, we assume ther will be one mixer in a pipeline. If a second
> + * one is required that is created as another pipe entity.
> + * The mixer is resposible for pipe management and represent a pipeline
> + * instance
> + */

You mean to say you assume there will be *exactly* one mixer?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 3/9] ASoC: Intel: Skylake: add DSP platform widget event handlers

[Vinod Koul]

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

On Fri, Aug 14, 2015 at 10:43:36PM +0100, Mark Brown wrote:
> On Sat, Aug 08, 2015 at 01:06:18AM +0530, Subhransu S. Prusty wrote:
> 
> > +	/* If its not SKL DSP type dont handle */
> > +	if (source->priv == NULL &&  (!is_skl_dsp_widget_type(w)))
> > +		return 0;
> 
> Why would this event be assigned to a widget that is not a suitable
> type, and what if it's an unsuitable widget type that happens ton have
> private data?  I'm not sure I understand the priv check.

Yes rethinking now I don't see why this would be called for events we don't
register as you pointed. I will recheck this bit and get back.

> You've also got a typo with "don't" and an extra space after the &&.
Oops, will fix. Thanks for pointing.

> > +	list_for_each_entry(p, &w->sinks, list_source) {
> > +		if (!p->connect)
> > +			continue;
> 
> Hrm.  Do we handle routing changes well?

Yes we do :)
This is the pmu handler for a mixer widget. Here we need to send Bind event
to firmware for the paths which are connected, so if a path is not connected
we don't need to send bind event so we skip that.

> 
> > +			/* Add connected path to one global list */
> > +			path_list->dapm_path = p;
> > +			list_add_tail(&path_list->node, &skl->dapm_path_list);
> > +			break;
> 
> We have our own private path list here which includes the DAPM path...
> can't we outsource some of this to the topology and DAPM code?  I'm a
> little fuzzy on what the list is for, it appears to be used mainly for
> deallocation though it's called dapm_path_list (which is a bit worrying).

If you look at the usage of the dapm_path_list, it is used in pmd handler
for this mixer. For a mixer there can be many paths and we need to
disconnect the previously connected path. DAPM will disconnect and send
event, so we wont know which paths were disconnected and thats why we keep a
track if this by using this list. At PMU we keep adding and at PMD we check
the path which was disconnected and send the event for that.
This gives driver very quick lookup of which disconnect to send. I am not
sure if we should be changing framework for this...

> > +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w,
> > +								struct skl *skl)
> > +{
> 
> This looks a lot like the previous function, there's similar duplication
> in the other event functions.

Actually logic is quite different. The skl_tplg_mixer_dapm_post_pmu_event is
for mixer and is supposed to bind to source pipe and start
The previous one is for PGA and starts sink pipelines it is connected to and
then its own. It doesn't do bind.

Said that I will check and try to see if common code for checking and
starting can be factored out from these into a common helper

> 
> > +	}
> > +	if (src_pipe_started) {
> 
> Missing blank line here.
Will fix

> 
> > +/*
> > + * In modelling, we assume ther will be one mixer in a pipeline. If a second
> > + * one is required that is created as another pipe entity.
> > + * The mixer is resposible for pipe management and represent a pipeline
> > + * instance
> > + */
> 
> You mean to say you assume there will be *exactly* one mixer?
Yes :) and if we need another mixer then that will become another pipeline

Thanks
-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 3/9] ASoC: Intel: Skylake: add DSP platform widget event handlers

[Mark Brown]

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

On Sat, Aug 15, 2015 at 07:12:08PM +0530, Vinod Koul wrote:
> On Fri, Aug 14, 2015 at 10:43:36PM +0100, Mark Brown wrote:

> > > +	list_for_each_entry(p, &w->sinks, list_source) {
> > > +		if (!p->connect)
> > > +			continue;

> > Hrm.  Do we handle routing changes well?

> Yes we do :)
> This is the pmu handler for a mixer widget. Here we need to send Bind event
> to firmware for the paths which are connected, so if a path is not connected
> we don't need to send bind event so we skip that.

And we couldn't connect in any other way?  This case is fine but I'm
worrying about what happens if that connection gets made later - we
won't get another PMU event since the widget is already powered.

> > > +			/* Add connected path to one global list */
> > > +			path_list->dapm_path = p;
> > > +			list_add_tail(&path_list->node, &skl->dapm_path_list);
> > > +			break;

> > We have our own private path list here which includes the DAPM path...
> > can't we outsource some of this to the topology and DAPM code?  I'm a
> > little fuzzy on what the list is for, it appears to be used mainly for
> > deallocation though it's called dapm_path_list (which is a bit worrying).

> If you look at the usage of the dapm_path_list, it is used in pmd handler
> for this mixer. For a mixer there can be many paths and we need to

This is the sort of thing where the whole lack of documentation thing
that I keep going on about becomes really important - there's a limited
amount of time I can spend on any individual patch series and so things
that need to be reverse engineered are just going to get queried a lot
of the time (and even if they get reverse engineered the feedback is
often going to be that the code needs to be clearer).

> disconnect the previously connected path. DAPM will disconnect and send
> event, so we wont know which paths were disconnected and thats why we keep a
> track if this by using this list. At PMU we keep adding and at PMD we check
> the path which was disconnected and send the event for that.
> This gives driver very quick lookup of which disconnect to send. I am not
> sure if we should be changing framework for this...

So this is about generating a notification to say which routes are being
disconnected?  That seems like a fairly general thing, I can imagine
that being quite common for DSPs - indeed it seems essential given that
we won't always trigger any power changes on disconnection.

> > > +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w,
> > > +								struct skl *skl)
> > > +{

> > This looks a lot like the previous function, there's similar duplication
> > in the other event functions.

> Actually logic is quite different. The skl_tplg_mixer_dapm_post_pmu_event is
> for mixer and is supposed to bind to source pipe and start
> The previous one is for PGA and starts sink pipelines it is connected to and
> then its own. It doesn't do bind.

There's certainly a lot of visual similarity there.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 3/9] ASoC: Intel: Skylake: add DSP platform widget event handlers

[Vinod Koul]

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

On Sat, Aug 15, 2015 at 07:36:38AM -0700, Mark Brown wrote:
> On Sat, Aug 15, 2015 at 07:12:08PM +0530, Vinod Koul wrote:
> > On Fri, Aug 14, 2015 at 10:43:36PM +0100, Mark Brown wrote:
> 
> > > > +	list_for_each_entry(p, &w->sinks, list_source) {
> > > > +		if (!p->connect)
> > > > +			continue;
> 
> > > Hrm.  Do we handle routing changes well?
> 
> > Yes we do :)
> > This is the pmu handler for a mixer widget. Here we need to send Bind event
> > to firmware for the paths which are connected, so if a path is not connected
> > we don't need to send bind event so we skip that.
> 
> And we couldn't connect in any other way?  This case is fine but I'm
> worrying about what happens if that connection gets made later - we
> won't get another PMU event since the widget is already powered.

Yes you are right this won't be handled here, but will be handled in source
pipe it is connecting to. The DAPM event will be called for that pipe, and
there we check if source or sink pipe is already running which is clue to
driver that we should do bind there rather than here

I will add that part as well in event handler comments

> > > > +			/* Add connected path to one global list */
> > > > +			path_list->dapm_path = p;
> > > > +			list_add_tail(&path_list->node, &skl->dapm_path_list);
> > > > +			break;
> 
> > > We have our own private path list here which includes the DAPM path...
> > > can't we outsource some of this to the topology and DAPM code?  I'm a
> > > little fuzzy on what the list is for, it appears to be used mainly for
> > > deallocation though it's called dapm_path_list (which is a bit worrying).
> 
> > If you look at the usage of the dapm_path_list, it is used in pmd handler
> > for this mixer. For a mixer there can be many paths and we need to
> 
> This is the sort of thing where the whole lack of documentation thing
> that I keep going on about becomes really important - there's a limited
> amount of time I can spend on any individual patch series and so things
> that need to be reverse engineered are just going to get queried a lot
> of the time (and even if they get reverse engineered the feedback is
> often going to be that the code needs to be clearer).

Okay, I did try to add comments to help understand but looks like I still
have more work to do. Will try to add these details as well

> > disconnect the previously connected path. DAPM will disconnect and send
> > event, so we wont know which paths were disconnected and thats why we keep a
> > track if this by using this list. At PMU we keep adding and at PMD we check
> > the path which was disconnected and send the event for that.
> > This gives driver very quick lookup of which disconnect to send. I am not
> > sure if we should be changing framework for this...
> 
> So this is about generating a notification to say which routes are being
> disconnected?  That seems like a fairly general thing, I can imagine
> that being quite common for DSPs - indeed it seems essential given that
> we won't always trigger any power changes on disconnection.

Okay fair point. Would it be okay to carry this approach here for now and
with later DAPM updates for next kernel cycle?

> 
> > > > +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w,
> > > > +								struct skl *skl)
> > > > +{
> 
> > > This looks a lot like the previous function, there's similar duplication
> > > in the other event functions.
> 
> > Actually logic is quite different. The skl_tplg_mixer_dapm_post_pmu_event is
> > for mixer and is supposed to bind to source pipe and start
> > The previous one is for PGA and starts sink pipelines it is connected to and
> > then its own. It doesn't do bind.
> 
> There's certainly a lot of visual similarity there.

Oh yes, they do look a lot similar...


-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 3/9] ASoC: Intel: Skylake: add DSP platform widget event handlers

[Mark Brown]

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

On Sat, Aug 15, 2015 at 08:42:04PM +0530, Vinod Koul wrote:
> On Sat, Aug 15, 2015 at 07:36:38AM -0700, Mark Brown wrote:

> > This is the sort of thing where the whole lack of documentation thing
> > that I keep going on about becomes really important - there's a limited
> > amount of time I can spend on any individual patch series and so things
> > that need to be reverse engineered are just going to get queried a lot
> > of the time (and even if they get reverse engineered the feedback is
> > often going to be that the code needs to be clearer).

> Okay, I did try to add comments to help understand but looks like I still
> have more work to do. Will try to add these details as well

If you're adding any of this stuff it's really not obvious, all I'm
seeing is very tactical stuff down in the details of the code which
isn't always useful without any big picture - bear in mind that there's
a lot of Intel internal abstractions talking to Intel internal
abstractions none of which are terribly obvious, and of course the
tendency to throw in things like the NHLT table or acronyms like mcps.

Bigger changelogs would help a lot here, as would building the
functionality up gradually rather than dumping large sections of code.
Right now the changelogs just tend to be some fairly brief comments on
the purpose of the code and don't really go into the structure or the
design decisions that went into it.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

For FE and BE, the PCM parameters come from FE and BE hw_params values
passed. For a FE we convert the FE params to DSP expected module format
and pass to DSP. For a BE we need to find the gateway settings (i2s/PDM)
to be applied. These are queried from NHLT table and applied.

Further for BE based on direction the settings are applied as either source
or destination parameters.

These helpers here allow the format to be calculated and queried as per
firmware format.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 274 +++++++++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl-topology.h |  11 ++
 2 files changed, 285 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index deb5c0d..7f3c54a 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -752,3 +752,277 @@ static int skl_tplg_pga_event(struct snd_soc_dapm_widget *w,
 
 	return 0;
 }
+
+/*
+ * The FE params are passed by hw_params of the DAI.
+ * On hw_params, the params are stored in Gateway module of the FE and we
+ * need to calculate the format in DSP module configuration, that conversion
+ * is done here
+ */
+static void skl_tplg_update_pipe_params(struct device *dev,
+		struct skl_module_cfg *mconfig,
+		struct skl_pipe_params *params)
+{
+	struct skl_pipe *pipe = mconfig->pipe;
+	struct skl_module_fmt *format = NULL;
+
+	memcpy(pipe->p_params, params, sizeof(*params));
+
+	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		format = &mconfig->in_fmt;
+	else
+		format = &mconfig->out_fmt;
+
+	/* set the hw_params */
+	format->s_freq = params->s_freq;
+	format->channels = params->ch;
+	format->valid_bit_depth = skl_get_bit_depth(params->s_fmt);
+
+	/*
+	 * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so
+	 * update bit depth accordingly
+	 */
+	if (format->valid_bit_depth == SKL_DEPTH_16BIT)
+		format->bit_depth = format->valid_bit_depth;
+	else if (format->valid_bit_depth == SKL_DEPTH_24BIT)
+		format->bit_depth = SKL_DEPTH_32BIT;
+
+	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		mconfig->ibs = (format->s_freq / 1000) *
+				(format->channels) *
+				(format->bit_depth >> 3);
+	} else {
+		mconfig->obs = (format->s_freq / 1000) *
+				(format->channels) *
+				(format->bit_depth >> 3);
+	}
+}
+
+/*
+ * Query the module config for the FE DAI
+ * This is used to find the hw_params set for that DAI and apply to FE
+ * pipeline
+ */
+static struct skl_module_cfg *
+skl_tplg_fe_get_cpr_module(struct snd_soc_dai *dai, int stream)
+{
+	struct snd_soc_dapm_widget *w;
+	struct snd_soc_dapm_path *p = NULL;
+
+	dev_dbg(dai->dev, "%s: enter, dai-name=%s\n", __func__, dai->name);
+
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		w = dai->playback_widget;
+		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
+		list_for_each_entry(p, &w->sinks, list_source) {
+			if (p->connect && p->sink->power &&
+					is_skl_dsp_widget_type(p->sink))
+				continue;
+
+			if (p->sink->priv) {
+				dev_dbg(dai->dev, "set params for %s\n", p->sink->name);
+				return p->sink->priv;
+			}
+		}
+	} else {
+		w = dai->capture_widget;
+		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
+		list_for_each_entry(p, &w->sources, list_sink) {
+			if (p->connect && p->source->power &&
+					is_skl_dsp_widget_type(p->source))
+				continue;
+
+			if (p->source->priv) {
+				dev_dbg(dai->dev, "set params for %s\n", p->source->name);
+				return p->source->priv;
+			}
+		}
+	}
+	return NULL;
+}
+
+int skl_tplg_fe_update_params(struct snd_soc_dai *dai,
+			struct skl_pipe_params *params)
+{
+	struct skl_module_cfg *m_cfg;
+
+	m_cfg = skl_tplg_fe_get_cpr_module(dai, params->stream);
+
+	if (m_cfg)
+		skl_tplg_update_pipe_params(dai->dev, m_cfg, params);
+
+	return 0;
+}
+
+static u8 skl_tplg_be_link_type(int dev_type)
+{
+	int ret;
+
+	switch (dev_type) {
+	case SKL_DEVICE_BT:
+		ret = NHLT_LINK_SSP;
+		break;
+
+	case SKL_DEVICE_DMIC:
+		ret = NHLT_LINK_DMIC;
+		break;
+
+	case SKL_DEVICE_I2S:
+		ret = NHLT_LINK_SSP;
+		break;
+
+	case SKL_DEVICE_HDALINK:
+		ret = NHLT_LINK_HDA;
+		break;
+
+	default:
+		ret = NHLT_LINK_INVALID;
+		break;
+	}
+
+	return ret;
+}
+
+/*
+ * Fill the BE gateway parameters
+ * The BE gateway expects a blob of parameters which are kept in the ACPI
+ * NHLT blob, so query the blob for interface type (i2s/pdm) and instance.
+ * The port cna have multiple settings so pick based on the PCM
+ * parameters
+ */
+static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
+		struct skl_module_cfg *mconfig, struct skl_pipe_params *params)
+{
+	struct skl_pipe *pipe = mconfig->pipe;
+	struct nhlt_specific_cfg *cfg;
+	struct skl *skl = get_skl_ctx(dai->dev);
+	int link_type = skl_tplg_be_link_type(mconfig->dev_type);
+
+	memcpy(pipe->p_params, params, sizeof(*params));
+
+	/* update the blob based on virtual bus_id*/
+	cfg = (struct nhlt_specific_cfg *) skl_get_ep_blob(skl,
+			mconfig->vbus_id, link_type, params->s_fmt,
+			params->ch, params->s_freq, params->stream);
+
+	if (cfg) {
+		mconfig->formats_config.caps_size = cfg->size;
+		memcpy(mconfig->formats_config.caps, &cfg->caps, cfg->size);
+	} else {
+		dev_dbg(dai->dev, "Blob is NULL");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int skl_tplg_be_set_params(struct snd_soc_dai *dai,
+					struct snd_soc_dapm_widget *w,
+					struct skl_pipe_params *params)
+{
+	if (!w->power) {
+		dev_dbg(dai->dev, "set params for widget=%s\n", w->name);
+		return skl_tplg_be_fill_pipe_params(dai, w->priv, params);
+	}
+
+	return -EINVAL;
+}
+
+static int skl_tplg_be_set_src_pipe_params(struct snd_soc_dai *dai,
+						struct snd_soc_dapm_widget *w,
+						struct skl_pipe_params *params)
+{
+	struct snd_soc_dapm_path *p;
+	int ret = 0;
+
+	dev_dbg(dai->dev, "In %swidget name=%s\n", __func__, w->name);
+
+	list_for_each_entry(p, &w->sources, list_sink) {
+		if (p->connect && is_skl_dsp_widget_type(p->source) &&
+						 p->source->priv) {
+			ret = skl_tplg_be_set_params(dai, p->source, params);
+
+			if (ret < 0)
+				return ret;
+		} else {
+			ret = skl_tplg_be_set_src_pipe_params(dai, p->source,
+									 params);
+		}
+	}
+
+	return ret;
+}
+
+static int skl_tplg_be_set_sink_pipe_params(struct snd_soc_dai *dai,
+	struct snd_soc_dapm_widget *w, struct skl_pipe_params *params)
+{
+	struct snd_soc_dapm_path *p = NULL;
+	int ret = 0;
+
+	dev_dbg(dai->dev, "widget name=%s\n", w->name);
+
+	list_for_each_entry(p, &w->sinks, list_source) {
+		if (p->connect && is_skl_dsp_widget_type(p->sink) &&
+						p->sink->priv) {
+			ret = skl_tplg_be_set_params(dai, p->sink, params);
+
+			if (ret < 0)
+				return ret;
+		} else {
+			ret = skl_tplg_be_set_sink_pipe_params(dai, p->sink,
+									 params);
+		}
+	}
+
+	return ret;
+}
+
+/*
+ * BE hw_params can be a source parameters (capture) or sink parameters
+ * (playback). Based on sink and source we need to either find the source
+ * list or the sink list and set the pipeline parameters
+ */
+int skl_tplg_be_update_params(struct snd_soc_dai *dai,
+				struct skl_pipe_params *params)
+{
+	struct snd_soc_dapm_widget *w;
+
+	dev_dbg(dai->dev, "In %s dai-name=%s\n", __func__, dai->name);
+
+	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		w = dai->playback_widget;
+
+		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
+		return skl_tplg_be_set_src_pipe_params(dai, w, params);
+	}
+
+	if (params->stream == SNDRV_PCM_STREAM_CAPTURE) {
+		w = dai->capture_widget;
+
+		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
+		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
+	}
+
+	return 0;
+}
+
+/* Run/Stop the FE pipeline */
+int skl_tplg_set_fe_pipeline_state(struct snd_soc_dai *dai, bool start,
+								int stream)
+{
+	struct skl *skl = get_skl_ctx(dai->dev);
+	struct skl_sst *ctx = skl->skl_sst;
+	struct skl_module_cfg *mconfig = NULL;
+
+	dev_dbg(dai->dev, "%s: enter, dai-name=%s dir=%d\n", __func__, dai->name, stream);
+
+	mconfig = skl_tplg_fe_get_cpr_module(dai, stream);
+	if (mconfig != NULL) {
+		if (start)
+			return skl_run_pipe(ctx, mconfig->pipe);
+		else
+			return skl_stop_pipe(ctx, mconfig->pipe);
+	}
+
+	return 0;
+}
diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
index 73d7916..94c721e 100644
--- a/sound/soc/intel/skylake/skl-topology.h
+++ b/sound/soc/intel/skylake/skl-topology.h
@@ -273,6 +273,17 @@ struct skl_dapm_path_list {
 	struct list_head node;
 };
 
+int skl_tplg_fe_update_params(struct snd_soc_dai *dai,
+	struct skl_pipe_params *params);
+int skl_tplg_be_update_params(struct snd_soc_dai *dai,
+	struct skl_pipe_params *params);
+void skl_tplg_set_be_dmic_config(struct snd_soc_dai *dai,
+	struct skl_pipe_params *params, int stream);
+int skl_tplg_init(struct snd_soc_platform *platform,
+				struct hdac_ext_bus *ebus);
+int skl_tplg_set_fe_pipeline_state(struct snd_soc_dai *dai,
+			bool start, int stream);
+
 int skl_create_pipeline(struct skl_sst *ctx, struct skl_pipe *pipe);
 
 int skl_run_pipe(struct skl_sst *ctx, struct skl_pipe *pipe);
-- 
2.4.3

Re: [PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling

[Mark Brown]

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

On Sat, Aug 08, 2015 at 01:06:19AM +0530, Subhransu S. Prusty wrote:

> +	/*
> +	 * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so
> +	 * update bit depth accordingly
> +	 */
> +	if (format->valid_bit_depth == SKL_DEPTH_16BIT)
> +		format->bit_depth = format->valid_bit_depth;
> +	else if (format->valid_bit_depth == SKL_DEPTH_24BIT)
> +		format->bit_depth = SKL_DEPTH_32BIT;
> +

What if the depth is neither 16 bit nor 24 bit?  Shouldn't this just be
a simple override for the 24 bit case?

> +/*
> + * Fill the BE gateway parameters
> + * The BE gateway expects a blob of parameters which are kept in the ACPI
> + * NHLT blob, so query the blob for interface type (i2s/pdm) and instance.
> + * The port cna have multiple settings so pick based on the PCM
> + * parameters
> + */
> +static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
> +		struct skl_module_cfg *mconfig, struct skl_pipe_params *params)
> +{
> +	struct skl_pipe *pipe = mconfig->pipe;
> +	struct nhlt_specific_cfg *cfg;
> +	struct skl *skl = get_skl_ctx(dai->dev);
> +	int link_type = skl_tplg_be_link_type(mconfig->dev_type);
> +
> +	memcpy(pipe->p_params, params, sizeof(*params));
> +
> +	/* update the blob based on virtual bus_id*/
> +	cfg = (struct nhlt_specific_cfg *) skl_get_ep_blob(skl,
> +			mconfig->vbus_id, link_type, params->s_fmt,
> +			params->ch, params->s_freq, params->stream);

I don't seem to have slk_get_ep_blob() but hopefully it's returning a
void * in which case why do we need to cast it?  In any case it needs to
be defined in mainline before this can be applied :(

> +	if (cfg) {
> +		mconfig->formats_config.caps_size = cfg->size;
> +		memcpy(mconfig->formats_config.caps, &cfg->caps, cfg->size);
> +	} else {
> +		dev_dbg(dai->dev, "Blob is NULL");
> +		return -EINVAL;
> +	}

This seems like a more serious problem than something we just dev_dbg()
about?

> +static int skl_tplg_be_set_params(struct snd_soc_dai *dai,
> +					struct snd_soc_dapm_widget *w,
> +					struct skl_pipe_params *params)
> +{
> +	if (!w->power) {
> +		dev_dbg(dai->dev, "set params for widget=%s\n", w->name);
> +		return skl_tplg_be_fill_pipe_params(dai, w->priv, params);
> +	}
> +
> +	return -EINVAL;

Shouldn't that be -EBUSY?  The normal idiom would be to check to see if
we were busy and error out rather than writing it as only configuing if
not busy (which looks like an error case now).

> +	list_for_each_entry(p, &w->sources, list_sink) {
> +		if (p->connect && is_skl_dsp_widget_type(p->source) &&
> +						 p->source->priv) {
> +			ret = skl_tplg_be_set_params(dai, p->source, params);
> +
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			ret = skl_tplg_be_set_src_pipe_params(dai, p->source,
> +									 params);
> +		}

These two cases appear to be identical apart from ignoring the error in
the else case...

> +	}
> +
> +	return ret;

...unless it happens to be the last entry in the list in which case we
pay attention.

> +static int skl_tplg_be_set_sink_pipe_params(struct snd_soc_dai *dai,
> +	struct snd_soc_dapm_widget *w, struct skl_pipe_params *params)
> +{
> +	struct snd_soc_dapm_path *p = NULL;
> +	int ret = 0;
> +
> +	dev_dbg(dai->dev, "widget name=%s\n", w->name);
> +
> +	list_for_each_entry(p, &w->sinks, list_source) {
> +		if (p->connect && is_skl_dsp_widget_type(p->sink) &&
> +						p->sink->priv) {
> +			ret = skl_tplg_be_set_params(dai, p->sink, params);
> +
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			ret = skl_tplg_be_set_sink_pipe_params(dai, p->sink,
> +									 params);
> +		}
> +	}

There's some more common code here and the same patterns as above...  :/

> +	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		w = dai->playback_widget;
> +
> +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> +		return skl_tplg_be_set_src_pipe_params(dai, w, params);
> +	}
> +
> +	if (params->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		w = dai->capture_widget;
> +
> +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> +		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
> +	}

Normally that'd be written as an if/else, and the only difference
between the two cases here is which widget we pick...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling

[Vinod Koul]

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

On Fri, Aug 14, 2015 at 10:53:10PM +0100, Mark Brown wrote:
> On Sat, Aug 08, 2015 at 01:06:19AM +0530, Subhransu S. Prusty wrote:
> 
> > +	/*
> > +	 * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so
> > +	 * update bit depth accordingly
> > +	 */
> > +	if (format->valid_bit_depth == SKL_DEPTH_16BIT)
> > +		format->bit_depth = format->valid_bit_depth;
> > +	else if (format->valid_bit_depth == SKL_DEPTH_24BIT)
> > +		format->bit_depth = SKL_DEPTH_32BIT;
> > +
> 
> What if the depth is neither 16 bit nor 24 bit?  Shouldn't this just be
> a simple override for the 24 bit case?

These are the only two DSP supports, so am not sure that overriding will
work with DSP. I think we should add else and error out.

> 
> > +/*
> > + * Fill the BE gateway parameters
> > + * The BE gateway expects a blob of parameters which are kept in the ACPI
> > + * NHLT blob, so query the blob for interface type (i2s/pdm) and instance.
> > + * The port cna have multiple settings so pick based on the PCM
> > + * parameters
> > + */
> > +static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
> > +		struct skl_module_cfg *mconfig, struct skl_pipe_params *params)
> > +{
> > +	struct skl_pipe *pipe = mconfig->pipe;
> > +	struct nhlt_specific_cfg *cfg;
> > +	struct skl *skl = get_skl_ctx(dai->dev);
> > +	int link_type = skl_tplg_be_link_type(mconfig->dev_type);
> > +
> > +	memcpy(pipe->p_params, params, sizeof(*params));
> > +
> > +	/* update the blob based on virtual bus_id*/
> > +	cfg = (struct nhlt_specific_cfg *) skl_get_ep_blob(skl,
> > +			mconfig->vbus_id, link_type, params->s_fmt,
> > +			params->ch, params->s_freq, params->stream);
> 
> I don't seem to have slk_get_ep_blob() but hopefully it's returning a
> void * in which case why do we need to cast it?  In any case it needs to
> be defined in mainline before this can be applied :(

It is already merged in your tree and part of NHLT patch. And my bad, it
returns struct nhlt_specific_cfg so the cast is not required and will fix

> 
> > +	if (cfg) {
> > +		mconfig->formats_config.caps_size = cfg->size;
> > +		memcpy(mconfig->formats_config.caps, &cfg->caps, cfg->size);
> > +	} else {
> > +		dev_dbg(dai->dev, "Blob is NULL");
> > +		return -EINVAL;
> > +	}
> 
> This seems like a more serious problem than something we just dev_dbg()
> about?

Yes this should be an error as it will fail and we can't start a stream
without this, will fix. Thanks for pointing.

> 
> > +static int skl_tplg_be_set_params(struct snd_soc_dai *dai,
> > +					struct snd_soc_dapm_widget *w,
> > +					struct skl_pipe_params *params)
> > +{
> > +	if (!w->power) {
> > +		dev_dbg(dai->dev, "set params for widget=%s\n", w->name);
> > +		return skl_tplg_be_fill_pipe_params(dai, w->priv, params);
> > +	}
> > +
> > +	return -EINVAL;
> 
> Shouldn't that be -EBUSY?  The normal idiom would be to check to see if
> we were busy and error out rather than writing it as only configuing if
> not busy (which looks like an error case now).

Not EBUSY. So here we are trying to set params for the pipe. So we would
expect that pipe (widget) should be powered up by DAPM, but if that is not
the case then we are in bad state. I am thinking EIO might suit this one better

> 
> > +	list_for_each_entry(p, &w->sources, list_sink) {
> > +		if (p->connect && is_skl_dsp_widget_type(p->source) &&
> > +						 p->source->priv) {
> > +			ret = skl_tplg_be_set_params(dai, p->source, params);
> > +
> > +			if (ret < 0)
> > +				return ret;
> > +		} else {
> > +			ret = skl_tplg_be_set_src_pipe_params(dai, p->source,
> > +									 params);
> > +		}
> 
> These two cases appear to be identical apart from ignoring the error in
> the else case...

Former is skl_tplg_be_set_params whereas latter is
skl_tplg_be_set_src_pipe_params(). In former case we set the params but in
former we set for source pipe

Agree we should check return here, will add that
> 
> > +	}
> > +
> > +	return ret;
> 
> ...unless it happens to be the last entry in the list in which case we
> pay attention.
> 
> > +static int skl_tplg_be_set_sink_pipe_params(struct snd_soc_dai *dai,
> > +	struct snd_soc_dapm_widget *w, struct skl_pipe_params *params)
> > +{
> > +	struct snd_soc_dapm_path *p = NULL;
> > +	int ret = 0;
> > +
> > +	dev_dbg(dai->dev, "widget name=%s\n", w->name);
> > +
> > +	list_for_each_entry(p, &w->sinks, list_source) {
> > +		if (p->connect && is_skl_dsp_widget_type(p->sink) &&
> > +						p->sink->priv) {
> > +			ret = skl_tplg_be_set_params(dai, p->sink, params);
> > +
> > +			if (ret < 0)
> > +				return ret;
> > +		} else {
> > +			ret = skl_tplg_be_set_sink_pipe_params(dai, p->sink,
> > +									 params);
> > +		}
> > +	}
> 
> There's some more common code here and the same patterns as above...  :/

well the problem here is sink, source. Former was for source pipe using
p->source, here we have sink pipe and using p->sink.
Same here too that we either set params or for sink pipe

Jeeja did try to make this common but it becomes quite hard, if you have
a trick up your sleeve for this, am all ears :)

> 
> > +	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +		w = dai->playback_widget;
> > +
> > +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> > +		return skl_tplg_be_set_src_pipe_params(dai, w, params);
> > +	}
> > +
> > +	if (params->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > +		w = dai->capture_widget;
> > +
> > +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> > +		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
> > +	}
> 
> Normally that'd be written as an if/else, and the only difference
> between the two cases here is which widget we pick...

Yes we can change to else. Btw should we rather do if and if else, and else
for error check, in case the value became bad. I am thinking latter.

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling

[Mark Brown]

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

On Sat, Aug 15, 2015 at 07:30:14PM +0530, Vinod Koul wrote:
> On Fri, Aug 14, 2015 at 10:53:10PM +0100, Mark Brown wrote:
> > On Sat, Aug 08, 2015 at 01:06:19AM +0530, Subhransu S. Prusty wrote:

> > > +	/*
> > > +	 * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so
> > > +	 * update bit depth accordingly
> > > +	 */
> > > +	if (format->valid_bit_depth == SKL_DEPTH_16BIT)
> > > +		format->bit_depth = format->valid_bit_depth;
> > > +	else if (format->valid_bit_depth == SKL_DEPTH_24BIT)
> > > +		format->bit_depth = SKL_DEPTH_32BIT;
> > > +

> > What if the depth is neither 16 bit nor 24 bit?  Shouldn't this just be
> > a simple override for the 24 bit case?

> These are the only two DSP supports, so am not sure that overriding will
> work with DSP. I think we should add else and error out.

Then write a switch statement.  :/

> > > +static int skl_tplg_be_set_params(struct snd_soc_dai *dai,
> > > +					struct snd_soc_dapm_widget *w,
> > > +					struct skl_pipe_params *params)
> > > +{
> > > +	if (!w->power) {
> > > +		dev_dbg(dai->dev, "set params for widget=%s\n", w->name);
> > > +		return skl_tplg_be_fill_pipe_params(dai, w->priv, params);
> > > +	}
> > > +
> > > +	return -EINVAL;
> > 
> > Shouldn't that be -EBUSY?  The normal idiom would be to check to see if
> > we were busy and error out rather than writing it as only configuing if
> > not busy (which looks like an error case now).
> 
> Not EBUSY. So here we are trying to set params for the pipe. So we would
> expect that pipe (widget) should be powered up by DAPM, but if that is not
> the case then we are in bad state. I am thinking EIO might suit this one better

Right, that's why I'm saying -EBUSY - we can't configure the pipe
because it is current in use.

> > There's some more common code here and the same patterns as above...  :/

> well the problem here is sink, source. Former was for source pipe using
> p->source, here we have sink pipe and using p->sink.
> Same here too that we either set params or for sink pipe

> Jeeja did try to make this common but it becomes quite hard, if you have
> a trick up your sleeve for this, am all ears :)

Well, I don't have the code any more.

> > > +	if (params->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > > +		w = dai->capture_widget;
> > > +
> > > +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> > > +		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
> > > +	}

> > Normally that'd be written as an if/else, and the only difference
> > between the two cases here is which widget we pick...

> Yes we can change to else. Btw should we rather do if and if else, and else
> for error check, in case the value became bad. I am thinking latter.

Either works.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling

[Vinod Koul]

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

On Sat, Aug 15, 2015 at 07:46:46AM -0700, Mark Brown wrote:
> On Sat, Aug 15, 2015 at 07:30:14PM +0530, Vinod Koul wrote:
> > On Fri, Aug 14, 2015 at 10:53:10PM +0100, Mark Brown wrote:
> > > On Sat, Aug 08, 2015 at 01:06:19AM +0530, Subhransu S. Prusty wrote:
> 
> > > > +	/*
> > > > +	 * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so
> > > > +	 * update bit depth accordingly
> > > > +	 */
> > > > +	if (format->valid_bit_depth == SKL_DEPTH_16BIT)
> > > > +		format->bit_depth = format->valid_bit_depth;
> > > > +	else if (format->valid_bit_depth == SKL_DEPTH_24BIT)
> > > > +		format->bit_depth = SKL_DEPTH_32BIT;
> > > > +
> 
> > > What if the depth is neither 16 bit nor 24 bit?  Shouldn't this just be
> > > a simple override for the 24 bit case?
> 
> > These are the only two DSP supports, so am not sure that overriding will
> > work with DSP. I think we should add else and error out.
> 
> Then write a switch statement.  :/

Ok

> 
> > > > +static int skl_tplg_be_set_params(struct snd_soc_dai *dai,
> > > > +					struct snd_soc_dapm_widget *w,
> > > > +					struct skl_pipe_params *params)
> > > > +{
> > > > +	if (!w->power) {
> > > > +		dev_dbg(dai->dev, "set params for widget=%s\n", w->name);
> > > > +		return skl_tplg_be_fill_pipe_params(dai, w->priv, params);
> > > > +	}
> > > > +
> > > > +	return -EINVAL;
> > > 
> > > Shouldn't that be -EBUSY?  The normal idiom would be to check to see if
> > > we were busy and error out rather than writing it as only configuing if
> > > not busy (which looks like an error case now).
> > 
> > Not EBUSY. So here we are trying to set params for the pipe. So we would
> > expect that pipe (widget) should be powered up by DAPM, but if that is not
> > the case then we are in bad state. I am thinking EIO might suit this one better
> 
> Right, that's why I'm saying -EBUSY - we can't configure the pipe
> because it is current in use.

Ok will update

> > > There's some more common code here and the same patterns as above...  :/
> 
> > well the problem here is sink, source. Former was for source pipe using
> > p->source, here we have sink pipe and using p->sink.
> > Same here too that we either set params or for sink pipe
> 
> > Jeeja did try to make this common but it becomes quite hard, if you have
> > a trick up your sleeve for this, am all ears :)
> 
> Well, I don't have the code any more.
> 
> > > > +	if (params->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > > > +		w = dai->capture_widget;
> > > > +
> > > > +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> > > > +		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
> > > > +	}
> 
> > > Normally that'd be written as an if/else, and the only difference
> > > between the two cases here is which widget we pick...
> 
> > Yes we can change to else. Btw should we rather do if and if else, and else
> > for error check, in case the value became bad. I am thinking latter.
> 
> Either works.
Ok will add latter

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[PATCH 5/9] ASoC: Intel: Skylake: Add topology core init and handlers

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

The SKL driver does not code DSP topology in driver. It uses the newly
added ASoC topology core to parse the topology information (controls,
widgets and map) from topology binary. We add init routine to invoke
topology load and callback for topology creation.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/skl-topology.c       | 236 +++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl-tplg-interface.h |  78 +++++++++
 2 files changed, 314 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 7f3c54a..511f912 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -1026,3 +1026,239 @@ int skl_tplg_set_fe_pipeline_state(struct snd_soc_dai *dai, bool start,
 
 	return 0;
 }
+
+const struct snd_soc_tplg_widget_events skl_tplg_widget_ops[] = {
+	{SKL_MIXER_EVENT, skl_tplg_mixer_event},
+	{SKL_VMIXER_EVENT, skl_tplg_vmixer_event},
+	{SKL_PGA_EVENT, skl_tplg_pga_event},
+};
+
+/*
+ * The topology binary passes the pin info for a module so initialize the pin
+ * info passed into module instance
+ */
+static void skl_fill_module_pin_info(struct device *dev,
+			struct skl_module_pin *m_pin,
+			int max_pin)
+{
+	int i;
+
+	for (i = 0; i < max_pin; i++) {
+		m_pin[i].id.module_id = 0;
+		m_pin[i].id.instance_id = 0;
+		m_pin[i].in_use = false;
+		m_pin[i].is_dynamic = true;
+		m_pin[i].pin_index = i;
+	}
+}
+
+/*
+ * Add pipeline from topology binary into driver pipeline list
+ *
+ * If already added we return that instance
+ * Otherwise we create a new instance and add into driver list
+ */
+static struct skl_pipe *skl_tplg_add_pipe(struct device *dev,
+						struct skl *skl,
+						struct skl_dfw_pipe *dfw_pipe)
+{
+	struct skl_pipeline  *ppl;
+	struct skl_pipe *pipe;
+	struct skl_pipe_params *params;
+
+	list_for_each_entry(ppl, &skl->ppl_list, node) {
+		if (ppl->pipe->ppl_id == dfw_pipe->pipe_id)
+			return ppl->pipe;
+	}
+
+	ppl = devm_kzalloc(dev, sizeof(*ppl), GFP_KERNEL);
+	if (!ppl)
+		return NULL;
+
+	pipe  = devm_kzalloc(dev, sizeof(*pipe), GFP_KERNEL);
+	if (!pipe)
+		return NULL;
+
+	params = devm_kzalloc(dev, sizeof(*params), GFP_KERNEL);
+	if (!params)
+		return NULL;
+
+	pipe->ppl_id = dfw_pipe->pipe_id;
+	pipe->memory_pages = dfw_pipe->memory_pages;
+	pipe->pipe_priority = dfw_pipe->pipe_priority;
+	pipe->conn_type = dfw_pipe->conn_type;
+	pipe->state = SKL_PIPE_INVALID;
+	pipe->p_params = params;
+	INIT_LIST_HEAD(&pipe->w_list);
+
+	ppl->pipe = pipe;
+	list_add(&ppl->node, &skl->ppl_list);
+
+	return ppl->pipe;
+}
+
+static void skl_tplg_dump_widget_info(struct device *dev,
+					struct skl_dfw_module *dfw_cfg,
+					struct snd_soc_tplg_dapm_widget *w)
+{
+	dev_dbg(dev, "widget  Info for widget=%s\n", w->name);
+	dev_dbg(dev, "In%s pvt_data_len=%d\n", __func__, w->priv.size);
+	dev_dbg(dev, "copying widget private data\n");
+
+	dev_dbg(dev, "module id = %d\n", dfw_cfg->module_id);
+	dev_dbg(dev, "module instance = %d\n", dfw_cfg->instance_id);
+	dev_dbg(dev, "mcps = %d\n", dfw_cfg->max_mcps);
+	dev_dbg(dev, "ibs = %d\n", dfw_cfg->ibs);
+	dev_dbg(dev, "obs = %d\n", dfw_cfg->obs);
+	dev_dbg(dev, "core_id=%d\n", dfw_cfg->core_id);
+}
+
+/*
+ * Topology core widget load callback
+ *
+ * This is used to save the private data for each widget which gives
+ * information to the driver about module and pipeline parameters which DSP
+ * FW expects like ids, resource values, formats etc
+ */
+static int skl_tplg_widget_load(struct snd_soc_component *cmpnt,
+					struct snd_soc_dapm_widget *w,
+					struct snd_soc_tplg_dapm_widget *tplg_w)
+{
+	int ret;
+	struct hdac_ext_bus *ebus  = snd_soc_component_get_drvdata(cmpnt);
+	struct skl *skl = ebus_to_skl(ebus);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct skl_module_cfg *mconfig;
+	struct skl_pipe *pipe;
+	struct skl_dfw_module *dfw_config = (struct skl_dfw_module *) tplg_w->priv.data;
+
+	skl_tplg_dump_widget_info(bus->dev, dfw_config, tplg_w);
+
+	if (!tplg_w->priv.size)
+		goto bind_event;
+
+	mconfig = devm_kzalloc(bus->dev, sizeof(*mconfig), GFP_KERNEL);
+
+	if (!mconfig)
+		return -ENOMEM;
+
+	w->priv = mconfig;
+	mconfig->id.module_id = dfw_config->module_id;
+	mconfig->id.instance_id = dfw_config->instance_id;
+	mconfig->mcps = dfw_config->max_mcps;
+	mconfig->ibs = dfw_config->ibs;
+	mconfig->obs = dfw_config->obs;
+	mconfig->core_id = dfw_config->core_id;
+	mconfig->max_in_queue = dfw_config->max_in_queue;
+	mconfig->max_out_queue = dfw_config->max_out_queue;
+	mconfig->is_loadable = dfw_config->is_loadable;
+	mconfig->in_fmt.channels  = dfw_config->in_fmt.channels;
+	mconfig->in_fmt.s_freq = dfw_config->in_fmt.freq;
+	mconfig->in_fmt.bit_depth = dfw_config->in_fmt.bit_depth;
+	mconfig->in_fmt.valid_bit_depth = dfw_config->in_fmt.valid_bit_depth;
+	mconfig->in_fmt.ch_cfg = dfw_config->in_fmt.ch_cfg;
+	mconfig->out_fmt.channels  = dfw_config->out_fmt.channels;
+	mconfig->out_fmt.s_freq = dfw_config->out_fmt.freq;
+	mconfig->out_fmt.bit_depth = dfw_config->out_fmt.bit_depth;
+	mconfig->out_fmt.valid_bit_depth = dfw_config->out_fmt.valid_bit_depth;
+	mconfig->out_fmt.ch_cfg = dfw_config->out_fmt.ch_cfg;
+	mconfig->params_fixup = dfw_config->params_fixup;
+	mconfig->converter = dfw_config->converter;
+	mconfig->m_type = dfw_config->module_type;
+	mconfig->vbus_id = dfw_config->vbus_id;
+
+	pipe =	skl_tplg_add_pipe(bus->dev, skl, &dfw_config->pipe);
+	if (pipe)
+		mconfig->pipe = pipe;
+
+	mconfig->dev_type =  dfw_config->dev_type;
+	mconfig->hw_conn_type = dfw_config->hw_conn_type;
+	mconfig->time_slot =  dfw_config->time_slot;
+	mconfig->formats_config.caps_size = dfw_config->caps.caps_size;
+
+	mconfig->m_in_pin = devm_kzalloc(bus->dev, (mconfig->max_in_queue) *
+						sizeof(*mconfig->m_in_pin),
+						GFP_KERNEL);
+	if (!mconfig->m_in_pin)
+		return -ENOMEM;
+
+	mconfig->m_out_pin = devm_kzalloc(bus->dev, (mconfig->max_in_queue) *
+						sizeof(*mconfig->m_out_pin),
+						GFP_KERNEL);
+	if (!mconfig->m_out_pin)
+		return -ENOMEM;
+
+	skl_fill_module_pin_info(bus->dev, mconfig->m_in_pin,
+				mconfig->max_in_queue);
+	skl_fill_module_pin_info(bus->dev, mconfig->m_out_pin,
+				mconfig->max_out_queue);
+
+	if (mconfig->formats_config.caps_size == 0)
+		goto bind_event;
+
+	mconfig->formats_config.caps = (u32 *)devm_kzalloc(bus->dev,
+				mconfig->formats_config.caps_size, GFP_KERNEL);
+
+	if (mconfig->formats_config.caps == NULL)
+		return -ENOMEM;
+
+	memcpy(mconfig->formats_config.caps, dfw_config->caps.caps,
+						 dfw_config->caps.caps_size);
+
+bind_event:
+	if (tplg_w->event_type == 0) {
+		dev_info(bus->dev, "ASoC: No event handler required\n");
+		return 0;
+	}
+
+	ret = snd_soc_tplg_widget_bind_event(w, skl_tplg_widget_ops,
+			ARRAY_SIZE(skl_tplg_widget_ops), tplg_w->event_type);
+
+	if (ret) {
+		dev_err(bus->dev, "%s: No matching event handlers found for %d\n",
+					__func__, tplg_w->event_type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct snd_soc_tplg_ops skl_tplg_ops  = {
+	.widget_load = skl_tplg_widget_load,
+};
+
+/* This will be read from topology manifest, currently defined here */
+#define SKL_MAX_MCPS 30000000
+#define SKL_FW_MAX_MEM 1000000
+
+/*
+ * SKL topology init routine
+ */
+int skl_tplg_init(struct snd_soc_platform *platform, struct hdac_ext_bus *ebus)
+{
+	int ret;
+	const struct firmware *fw;
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct skl *skl = ebus_to_skl(ebus);
+
+	dev_dbg(bus->dev, "In%s req firmware topology bin\n",  __func__);
+	ret = request_firmware(&fw, "dfw_sst.bin", bus->dev);
+	if (fw == NULL) {
+		dev_err(bus->dev, "config firmware request failed with %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(bus->dev, "In%s soc fw load_platform\n", __func__);
+	/* Index is for each config load */
+	ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0);
+
+	if (ret < 0) {
+		dev_err(bus->dev, "tplg component load failed%d\n", ret);
+		return -EINVAL;
+	}
+
+	skl->resource.max_mcps = SKL_MAX_MCPS;
+	skl->resource.max_mem = SKL_FW_MAX_MEM;
+
+	return 0;
+}
diff --git a/sound/soc/intel/skylake/skl-tplg-interface.h b/sound/soc/intel/skylake/skl-tplg-interface.h
index a506898..b258c90 100644
--- a/sound/soc/intel/skylake/skl-tplg-interface.h
+++ b/sound/soc/intel/skylake/skl-tplg-interface.h
@@ -19,6 +19,27 @@
 #ifndef __HDA_TPLG_INTERFACE_H__
 #define __HDA_TPLG_INTERFACE_H__
 
+/* Default types range from 0~12. type can range from 0 to 0xff
+ * SST types start at higher to avoid any overlapping in future */
+#define SOC_CONTROL_TYPE_HDA_SST_ALGO_PARAMS	200
+#define SOC_CONTROL_TYPE_HDA_SST_MUX		201
+#define SOC_CONTROL_TYPE_HDA_SST_MIX		201
+#define SOC_CONTROL_TYPE_HDA_SST_BYTE		203
+
+#define HDA_SST_CFG_MAX	900 /* size of copier cfg*/
+#define MAX_IN_QUEUE 8
+#define MAX_OUT_QUEUE 8
+
+/* Event types goes here */
+/* Reserve event type 0 for no event handlers */
+enum skl_event_types {
+	SKL_EVENT_NONE = 0,
+	SKL_MIXER_EVENT,
+	SKL_MUX_EVENT,
+	SKL_VMIXER_EVENT,
+	SKL_PGA_EVENT
+};
+
 /**
  * enum skl_ch_cfg - channel configuration
  *
@@ -85,4 +106,61 @@ enum skl_dev_type {
 	SKL_DEVICE_HDALINK = 0x4,
 	SKL_DEVICE_NONE
 };
+
+struct skl_dfw_module_pin {
+	u16 module_id;
+	u16 instance_id;
+	u8 pin_id;
+	bool is_dynamic;
+} __packed;
+
+struct skl_dfw_module_fmt {
+	u32 channels;
+	u32 freq;
+	u32 bit_depth;
+	u32 valid_bit_depth;
+	u32 ch_cfg;
+} __packed;
+
+struct skl_dfw_module_caps {
+	u32 caps_size;
+	u32 caps[HDA_SST_CFG_MAX];
+};
+
+struct skl_dfw_pipe {
+	u8 pipe_id;
+	u8 pipe_priority;
+	u16 conn_type;
+	u32 memory_pages;
+} __packed;
+
+struct skl_dfw_module {
+	u16 module_id;
+	u16 instance_id;
+	u32 max_mcps;
+	u8 core_id;
+	u8 max_in_queue;
+	u8 max_out_queue;
+	u8 is_loadable;
+	u8 conn_type;
+	u8 dev_type;
+	u8 hw_conn_type;
+	u8 time_slot;
+	u32 obs;
+	u32 ibs;
+	u32 params_fixup;
+	u32 converter;
+	u32 module_type;
+	u32 vbus_id;
+	struct skl_dfw_pipe pipe;
+	struct skl_dfw_module_fmt in_fmt;
+	struct skl_dfw_module_fmt out_fmt;
+	struct skl_dfw_module_caps caps;
+} __packed;
+
+struct skl_dfw_algo_data {
+	u32 max;
+	char *params;
+} __packed;
+
 #endif
-- 
2.4.3

Re: [PATCH 5/9] ASoC: Intel: Skylake: Add topology core init and handlers

[Mark Brown]

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

On Sat, Aug 08, 2015 at 01:06:20AM +0530, Subhransu S. Prusty wrote:

> +	struct skl_pipeline  *ppl;

> +	pipe  = devm_kzalloc(dev, sizeof(*pipe), GFP_KERNEL);

There's lots of random spaces in this code so far and some further down
too.

> +	struct skl_dfw_module *dfw_config = (struct skl_dfw_module *) tplg_w->priv.data;
> +
> +	skl_tplg_dump_widget_info(bus->dev, dfw_config, tplg_w);

Consider implementing debugfs for this...

> +/* This will be read from topology manifest, currently defined here */
> +#define SKL_MAX_MCPS 30000000
> +#define SKL_FW_MAX_MEM 1000000

Oh dear, this sounds like we need another ABI update to add these
manifests?

> +	dev_dbg(bus->dev, "In%s req firmware topology bin\n",  __func__);

Those "In%s" are going to come out as the function name prefixed with
In.  What's that for?  It's just going to make the logs harder to read
as far as I can tell :(

> +	const struct firmware *fw;

> +	ret = request_firmware(&fw, "dfw_sst.bin", bus->dev);
> +	if (fw == NULL) {
> +		dev_err(bus->dev, "config firmware request failed with %d\n", ret);
> +		return ret;
> +	}

We're ignoring the return value (which is what we should be paying
attention to here) and checking to see if fw is NULL but fw wasn't
initialized :(

> +	/* Index is for each config load */
> +	ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0);

Which index?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 5/9] ASoC: Intel: Skylake: Add topology core init and handlers

[Vinod Koul]

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

On Fri, Aug 14, 2015 at 11:03:13PM +0100, Mark Brown wrote:
> On Sat, Aug 08, 2015 at 01:06:20AM +0530, Subhransu S. Prusty wrote:
> 
> > +	struct skl_pipeline  *ppl;
> 
> > +	pipe  = devm_kzalloc(dev, sizeof(*pipe), GFP_KERNEL);
> 
> There's lots of random spaces in this code so far and some further down
> too.
sorry about that, will fix

> 
> > +	struct skl_dfw_module *dfw_config = (struct skl_dfw_module *) tplg_w->priv.data;
> > +
> > +	skl_tplg_dump_widget_info(bus->dev, dfw_config, tplg_w);
> 
> Consider implementing debugfs for this...

yes that a good idea, will drop this here.

> 
> > +/* This will be read from topology manifest, currently defined here */
> > +#define SKL_MAX_MCPS 30000000
> > +#define SKL_FW_MAX_MEM 1000000
> 
> Oh dear, this sounds like we need another ABI update to add these
> manifests?

Not yet. So we will add this and few other stuff in topology manifest vendor
data for Intel. So Topology Kernel ABI will not get modified with this.
Yes we need to start adding these data sets to Kernel ABI, but I am planning
to add that, one shot at the end of SKL cycle so that we don't have to modify
ABI defines.

> 
> > +	dev_dbg(bus->dev, "In%s req firmware topology bin\n",  __func__);
> 
> Those "In%s" are going to come out as the function name prefixed with
> In.  What's that for?  It's just going to make the logs harder to read
> as far as I can tell :(

Will fix this

> 
> > +	const struct firmware *fw;
> 
> > +	ret = request_firmware(&fw, "dfw_sst.bin", bus->dev);
> > +	if (fw == NULL) {
> > +		dev_err(bus->dev, "config firmware request failed with %d\n", ret);
> > +		return ret;
> > +	}
> 
> We're ignoring the return value (which is what we should be paying
> attention to here) and checking to see if fw is NULL but fw wasn't
> initialized :(

Yes, will fix

> 
> > +	/* Index is for each config load */
> > +	ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0);
> 
> Which index?
The last arg of snd_soc_tplg_component_load() is id which is set as
tplg.req_index = id;

So the comment tries to explain how last 0 index is added. We have only one
load so we will be always 0 index


Thanks
-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH 5/9] ASoC: Intel: Skylake: Add topology core init and handlers

[Mark Brown]

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

On Sat, Aug 15, 2015 at 07:46:58PM +0530, Vinod Koul wrote:
> On Fri, Aug 14, 2015 at 11:03:13PM +0100, Mark Brown wrote:

> > > +	/* Index is for each config load */
> > > +	ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0);

> > Which index?

> The last arg of snd_soc_tplg_component_load() is id which is set as
> tplg.req_index = id;

> So the comment tries to explain how last 0 index is added. We have only one
> load so we will be always 0 index

Your comment isn't explaining that at all, sorry - it's making things
less clear.  It's peering into the implementation to translate ID to
index and even with s/id/index/ it's begging the question "ID/index is
what for each component load?".  If you're adding a comment here I'd
expect to see something like "Use the same ID each time because..."
which explains something that isn't in the code.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 5/9] ASoC: Intel: Skylake: Add topology core init and handlers

[Vinod Koul]

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

On Sat, Aug 15, 2015 at 10:00:58AM -0700, Mark Brown wrote:
> On Sat, Aug 15, 2015 at 07:46:58PM +0530, Vinod Koul wrote:
> > On Fri, Aug 14, 2015 at 11:03:13PM +0100, Mark Brown wrote:
> 
> > > > +	/* Index is for each config load */
> > > > +	ret = snd_soc_tplg_component_load(&platform->component, &skl_tplg_ops, fw, 0);
> 
> > > Which index?
> 
> > The last arg of snd_soc_tplg_component_load() is id which is set as
> > tplg.req_index = id;
> 
> > So the comment tries to explain how last 0 index is added. We have only one
> > load so we will be always 0 index
> 
> Your comment isn't explaining that at all, sorry - it's making things
> less clear.  It's peering into the implementation to translate ID to
> index and even with s/id/index/ it's begging the question "ID/index is
> what for each component load?".  If you're adding a comment here I'd
> expect to see something like "Use the same ID each time because..."
> which explains something that isn't in the code.

Fair point, will add this detail now

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[PATCH 6/9] ASoC: Intel: Skylake: Initialize and load DSP controls

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

Initialize and creates DSP controls if processing pipe capability
is supported by HW. Updates the dma_id, hw_params to module param to
be used when DSP module has to be configured.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 140 ++++++++++++++++++++++++++++++--------
 1 file changed, 113 insertions(+), 27 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 7d617bf..045c7d4 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -24,6 +24,7 @@
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include "skl.h"
+#include "skl-topology.h"
 
 #define HDA_MONO 1
 #define HDA_STEREO 2
@@ -214,6 +215,7 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream,
 	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
 	struct hdac_ext_stream *stream = get_hdac_ext_stream(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct skl_pipe_params p_params = {0};
 	int ret, dma_id;
 
 	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
@@ -228,6 +230,13 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream,
 	dma_id = hdac_stream(stream)->stream_tag - 1;
 	dev_dbg(dai->dev, "dma_id=%d\n", dma_id);
 
+	p_params.s_fmt = snd_pcm_format_width(params_format(params));
+	p_params.ch = params_channels(params);
+	p_params.s_freq = params_rate(params);
+	p_params.host_dma_id = dma_id;
+	p_params.stream = substream->stream;
+	skl_tplg_fe_update_params(dai, &p_params);
+
 	return 0;
 }
 
@@ -268,6 +277,46 @@ static int skl_pcm_hw_free(struct snd_pcm_substream *substream,
 	return skl_substream_free_pages(ebus_to_hbus(ebus), substream);
 }
 
+static int skl_be_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct skl_pipe_params p_params = {0};
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+
+	p_params.s_fmt = snd_pcm_format_width(params_format(params));
+	p_params.ch = params_channels(params);
+	p_params.s_freq = params_rate(params);
+	p_params.stream = substream->stream;
+	skl_tplg_be_update_params(dai, &p_params);
+
+	return 0;
+}
+
+static int skl_pcm_trigger(struct snd_pcm_substream *substream, int cmd,
+		struct snd_soc_dai *dai)
+{
+	dev_dbg(dai->dev, "In %s cmd=%d\n", __func__, cmd);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		return skl_tplg_set_fe_pipeline_state(dai, true,
+							substream->stream);
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+		return skl_tplg_set_fe_pipeline_state(dai, false,
+							substream->stream);
+
+	default:
+		return 0;
+	}
+
+	return 0;
+}
+
 static int skl_link_hw_params(struct snd_pcm_substream *substream,
 				struct snd_pcm_hw_params *params,
 				struct snd_soc_dai *dai)
@@ -277,9 +326,10 @@ static int skl_link_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
 	struct skl_dma_params *dma_params;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
-	int dma_id;
+	struct skl_pipe_params p_params = {0};
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
 
-	pr_debug("%s\n", __func__);
 	link_dev = snd_hdac_ext_stream_assign(ebus, substream,
 					HDAC_EXT_STREAM_TYPE_LINK);
 	if (!link_dev)
@@ -293,7 +343,14 @@ static int skl_link_hw_params(struct snd_pcm_substream *substream,
 	if (dma_params)
 		dma_params->stream_tag =  hdac_stream(link_dev)->stream_tag;
 	snd_soc_dai_set_dma_data(codec_dai, substream, (void *)dma_params);
-	dma_id = hdac_stream(link_dev)->stream_tag - 1;
+
+	p_params.s_fmt = snd_pcm_format_width(params_format(params));
+	p_params.ch = params_channels(params);
+	p_params.s_freq = params_rate(params);
+	p_params.stream = substream->stream;
+	p_params.link_dma_id = hdac_stream(link_dev)->stream_tag - 1;
+
+	skl_tplg_be_update_params(dai, &p_params);
 
 	return 0;
 }
@@ -308,8 +365,6 @@ static int skl_link_pcm_prepare(struct snd_pcm_substream *substream,
 	unsigned int format_val = 0;
 	struct skl_dma_params *dma_params;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
-	struct snd_pcm_hw_params *params;
-	struct snd_interval *channels, *rate;
 	struct hdac_ext_link *link;
 
 	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
@@ -317,18 +372,6 @@ static int skl_link_pcm_prepare(struct snd_pcm_substream *substream,
 		dev_dbg(dai->dev, "already stream is prepared - returning\n");
 		return 0;
 	}
-	params  = devm_kzalloc(dai->dev, sizeof(*params), GFP_KERNEL);
-	if (params == NULL)
-		return -ENOMEM;
-
-	channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
-	channels->min = channels->max = substream->runtime->channels;
-	rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
-	rate->min = rate->max = substream->runtime->rate;
-	snd_mask_set(&params->masks[SNDRV_PCM_HW_PARAM_FORMAT -
-					SNDRV_PCM_HW_PARAM_FIRST_MASK],
-					substream->runtime->format);
-
 
 	dma_params  = (struct skl_dma_params *)
 			snd_soc_dai_get_dma_data(codec_dai, substream);
@@ -399,13 +442,13 @@ static int skl_link_hw_free(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static int skl_hda_be_startup(struct snd_pcm_substream *substream,
+static int skl_be_startup(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
 	return pm_runtime_get_sync(dai->dev);
 }
 
-static void skl_hda_be_shutdown(struct snd_pcm_substream *substream,
+static void skl_be_shutdown(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
 	pm_runtime_mark_last_busy(dai->dev);
@@ -418,20 +461,28 @@ static struct snd_soc_dai_ops skl_pcm_dai_ops = {
 	.prepare = skl_pcm_prepare,
 	.hw_params = skl_pcm_hw_params,
 	.hw_free = skl_pcm_hw_free,
+	.trigger = skl_pcm_trigger,
 };
 
 static struct snd_soc_dai_ops skl_dmic_dai_ops = {
-	.startup = skl_hda_be_startup,
-	.shutdown = skl_hda_be_shutdown,
+	.startup = skl_be_startup,
+	.hw_params = skl_be_hw_params,
+	.shutdown = skl_be_shutdown,
+};
+
+static struct snd_soc_dai_ops skl_be_ssp_dai_ops = {
+	.startup = skl_be_startup,
+	.hw_params = skl_be_hw_params,
+	.shutdown = skl_be_shutdown,
 };
 
 static struct snd_soc_dai_ops skl_link_dai_ops = {
-	.startup = skl_hda_be_startup,
+	.startup = skl_be_startup,
 	.prepare = skl_link_pcm_prepare,
 	.hw_params = skl_link_hw_params,
 	.hw_free = skl_link_hw_free,
 	.trigger = skl_link_pcm_trigger,
-	.shutdown = skl_hda_be_shutdown,
+	.shutdown = skl_be_shutdown,
 };
 
 static struct snd_soc_dai_driver skl_platform_dai[] = {
@@ -488,6 +539,24 @@ static struct snd_soc_dai_driver skl_platform_dai[] = {
 },
 /* BE CPU  Dais */
 {
+	.name = "SSP0 Pin",
+	.ops = &skl_be_ssp_dai_ops,
+	.playback = {
+		.stream_name = "ssp0 Tx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+	.capture = {
+		.stream_name = "ssp0 Rx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+},
+{
 	.name = "iDisp Pin",
 	.ops = &skl_link_dai_ops,
 	.playback = {
@@ -577,7 +646,7 @@ static int skl_platform_open(struct snd_pcm_substream *substream)
 	return 0;
 }
 
-static int skl_pcm_trigger(struct snd_pcm_substream *substream,
+static int skl_coupled_trigger(struct snd_pcm_substream *substream,
 					int cmd)
 {
 	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
@@ -651,7 +720,7 @@ static int skl_pcm_trigger(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static int skl_dsp_trigger(struct snd_pcm_substream *substream,
+static int skl_decoupled_trigger(struct snd_pcm_substream *substream,
 		int cmd)
 {
 	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
@@ -708,9 +777,9 @@ static int skl_platform_pcm_trigger(struct snd_pcm_substream *substream,
 	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
 
 	if (ebus->ppcap)
-		return skl_dsp_trigger(substream, cmd);
+		return skl_decoupled_trigger(substream, cmd);
 	else
-		return skl_pcm_trigger(substream, cmd);
+		return skl_coupled_trigger(substream, cmd);
 }
 
 /* calculate runtime delay from LPIB */
@@ -877,7 +946,19 @@ static int skl_pcm_new(struct snd_soc_pcm_runtime *rtd)
 	return retval;
 }
 
+static int skl_platform_soc_probe(struct snd_soc_platform *platform)
+{
+	struct hdac_ext_bus *ebus = dev_get_drvdata(platform->dev);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+
+	dev_dbg(bus->dev, "Enter:%s\n", __func__);
+
+	if (ebus->ppcap)
+		return skl_tplg_init(platform, ebus);
+	return 0;
+}
 static struct snd_soc_platform_driver skl_platform_drv  = {
+	.probe		= skl_platform_soc_probe,
 	.ops		= &skl_platform_ops,
 	.pcm_new	= skl_pcm_new,
 	.pcm_free	= skl_pcm_free,
@@ -890,6 +971,11 @@ static const struct snd_soc_component_driver skl_component = {
 int skl_platform_register(struct device *dev)
 {
 	int ret;
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dev);
+	struct skl *skl = ebus_to_skl(ebus);
+
+	INIT_LIST_HEAD(&skl->ppl_list);
+	INIT_LIST_HEAD(&skl->dapm_path_list);
 
 	ret = snd_soc_register_platform(dev, &skl_platform_drv);
 	if (ret) {
-- 
2.4.3

[PATCH 7/9] ASoC: Intel: Skylake: Add DSP support and enable it

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

If processing pipe capability is supported, add DSP support.
Adds initialization/free/suspend/resume DSP functionality.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/skl.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 348d094..2f1890e 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -166,11 +166,16 @@ static int skl_runtime_suspend(struct device *dev)
 	struct pci_dev *pci = to_pci_dev(dev);
 	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
 	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct skl *skl = ebus_to_skl(ebus);
+	int ret;
 
 	dev_dbg(bus->dev, "in %s\n", __func__);
 
 	/* enable controller wake up event */
 	snd_hdac_chip_updatew(bus, WAKEEN, 0, STATESTS_INT_MASK);
+	ret = skl_suspend_dsp(skl);
+	if (ret < 0)
+		return ret;
 
 	snd_hdac_bus_stop_chip(bus);
 	snd_hdac_bus_enter_link_reset(bus);
@@ -183,7 +188,7 @@ static int skl_runtime_resume(struct device *dev)
 	struct pci_dev *pci = to_pci_dev(dev);
 	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
 	struct hdac_bus *bus = ebus_to_hbus(ebus);
-	struct skl *hda = ebus_to_skl(ebus);
+	struct skl *skl = ebus_to_skl(ebus);
 	int status;
 
 	dev_dbg(bus->dev, "in %s\n", __func__);
@@ -191,12 +196,12 @@ static int skl_runtime_resume(struct device *dev)
 	/* Read STATESTS before controller reset */
 	status = snd_hdac_chip_readw(bus, STATESTS);
 
-	skl_init_pci(hda);
+	skl_init_pci(skl);
 	snd_hdac_bus_init_chip(bus, true);
 	/* disable controller Wake Up event */
 	snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
 
-	return 0;
+	return skl_resume_dsp(skl);
 }
 #endif /* CONFIG_PM */
 
@@ -457,17 +462,19 @@ static int skl_probe(struct pci_dev *pci,
 
 	/* check if dsp is there */
 	if (ebus->ppcap) {
-		/* TODO register with dsp IPC */
-		dev_dbg(bus->dev, "Register dsp\n");
+		err = skl_init_dsp(skl);
+		if (err < 0) {
+			dev_dbg(bus->dev, "error failed to register dsp\n");
+			goto out_free;
+		}
 	}
-
 	if (ebus->mlcap)
 		snd_hdac_ext_bus_get_ml_capabilities(ebus);
 
 	/* create device for soc dmic */
 	err = skl_dmic_device_register(skl);
 	if (err < 0)
-		goto out_free;
+		goto out_dsp_free;
 
 	/* register platform dai and controls */
 	err = skl_platform_register(bus->dev);
@@ -491,6 +498,8 @@ out_unregister:
 	skl_platform_unregister(bus->dev);
 out_dmic_free:
 	skl_dmic_device_unregister(skl);
+out_dsp_free:
+	skl_free_dsp(skl);
 out_free:
 	skl->init_failed = 1;
 	skl_free(ebus);
@@ -507,6 +516,7 @@ static void skl_remove(struct pci_dev *pci)
 		pm_runtime_get_noresume(&pci->dev);
 	pci_dev_put(pci);
 	skl_platform_unregister(&pci->dev);
+	skl_free_dsp(skl);
 	skl_dmic_device_unregister(skl);
 	skl_free(ebus);
 	dev_set_drvdata(&pci->dev, NULL);
-- 
2.4.3

[PATCH 8/9] ASoC: Intel: Skylake: Initialize NHLT table

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

Load and Initialize Non HDA Link Table in Skylake driver
to get platform configuration.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/skl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 2f1890e..ca135b8 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -458,6 +458,11 @@ static int skl_probe(struct pci_dev *pci,
 	if (err < 0)
 		goto out_free;
 
+	skl->nhlt = skl_nhlt_init(bus->dev);
+
+	if (skl->nhlt == NULL)
+		goto out_free;
+
 	pci_set_drvdata(skl->pci, ebus);
 
 	/* check if dsp is there */
-- 
2.4.3

[PATCH 9/9] ASoC: Intel: Skylake: Remove CPU dai that is not used

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

For each endpoint instance, need to create CPU dai,
Removed unused CPU dai DMIC23/HDA-SPK/HDA-AMIC dais.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 045c7d4..22ab7ac 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -579,17 +579,6 @@ static struct snd_soc_dai_driver skl_platform_dai[] = {
 	},
 },
 {
-	.name = "DMIC23 Pin",
-	.ops = &skl_dmic_dai_ops,
-	.capture = {
-		.stream_name = "DMIC23 Rx",
-		.channels_min = HDA_STEREO,
-		.channels_max = HDA_STEREO,
-		.rates = SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_16000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
-	},
-},
-{
 	.name = "HD-Codec Pin",
 	.ops = &skl_link_dai_ops,
 	.playback = {
@@ -607,28 +596,6 @@ static struct snd_soc_dai_driver skl_platform_dai[] = {
 		.formats = SNDRV_PCM_FMTBIT_S16_LE,
 	},
 },
-{
-	.name = "HD-Codec-SPK Pin",
-	.ops = &skl_link_dai_ops,
-	.playback = {
-		.stream_name = "HD-Codec-SPK Tx",
-		.channels_min = HDA_STEREO,
-		.channels_max = HDA_STEREO,
-		.rates = SNDRV_PCM_RATE_48000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
-	},
-},
-{
-	.name = "HD-Codec-AMIC Pin",
-	.ops = &skl_link_dai_ops,
-	.capture = {
-		.stream_name = "HD-Codec-AMIC Rx",
-		.channels_min = HDA_STEREO,
-		.channels_max = HDA_STEREO,
-		.rates = SNDRV_PCM_RATE_48000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
-	},
-},
 };
 
 static int skl_platform_open(struct snd_pcm_substream *substream)
-- 
2.4.3

Re: [PATCH 9/9] ASoC: Intel: Skylake: Remove CPU dai that is not used

[Mark Brown]

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

On Sat, Aug 08, 2015 at 01:06:24AM +0530, Subhransu S. Prusty wrote:
> From: Jeeja KP <jeeja.kp@intel.com>
> 
> For each endpoint instance, need to create CPU dai,
> Removed unused CPU dai DMIC23/HDA-SPK/HDA-AMIC dais.

I'm not sure I understand the first line here?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 9/9] ASoC: Intel: Skylake: Remove CPU dai that is not used

[Vinod Koul]

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

On Fri, Aug 14, 2015 at 11:06:33PM +0100, Mark Brown wrote:
> On Sat, Aug 08, 2015 at 01:06:24AM +0530, Subhransu S. Prusty wrote:
> > From: Jeeja KP <jeeja.kp@intel.com>
> > 
> > For each endpoint instance, need to create CPU dai,
> > Removed unused CPU dai DMIC23/HDA-SPK/HDA-AMIC dais.
> 
> I'm not sure I understand the first line here?

We need to create CPU DAI for each endpoint instance. For this we should
have one DMIC DAI, one HDA DAI and SSP DAI. DMIC23, HDA-SPK/AMIC was not
required so this patch removes

Will fix the description

Thanks
-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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