[PATCH] ASoC: dapm - Add API calls to query valid DAPM paths.

[PATCH] ASoC: dapm - Add API calls to query valid DAPM paths.

[Liam Girdwood]

In preparation for ASoC Dynamic PCM (AKA DSP) support.

Add DAPM API calls to determine whether a DAPM audio path is valid between
source and sink widgets and return a lits of active widgets in that path.
This also takes into account all kcontrol mux and mixer settings in between
the source and sink widgets to validate the audio path.

This will be used by the DSP core to determine the runtime DAI mappings
between FE and BE DAIs in order to run PCM operations.

Signed-off-by: Liam Girdwood <lrg@ti.com>
---
 include/sound/soc-dapm.h |   14 ++
 sound/soc/soc-dapm.c     |  371 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 376 insertions(+), 9 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index e09505c..5a7dae6 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -310,6 +310,7 @@ struct snd_soc_dapm_path;
 struct snd_soc_dapm_pin;
 struct snd_soc_dapm_route;
 struct snd_soc_dapm_context;
+struct snd_soc_dapm_widget_list;
 
 int dapm_reg_event(struct snd_soc_dapm_widget *w,
 		   struct snd_kcontrol *kcontrol, int event);
@@ -375,6 +376,13 @@ int snd_soc_dapm_force_enable_pin(struct snd_soc_dapm_context *dapm,
 int snd_soc_dapm_ignore_suspend(struct snd_soc_dapm_context *dapm,
 				const char *pin);
 
+/* dapm path query */
+int snd_soc_dapm_get_connected_widgets_type(struct snd_soc_dapm_context *dapm,
+		const char *stream_name, struct snd_soc_dapm_widget_list **list,
+		int stream, enum snd_soc_dapm_type type);
+int snd_soc_dapm_get_connected_widgets_name(struct snd_soc_dapm_context *dapm,
+		const char *name, struct snd_soc_dapm_widget_list **list, int stream);
+
 /* dapm widget types */
 enum snd_soc_dapm_type {
 	snd_soc_dapm_input = 0,		/* input pin */
@@ -432,6 +440,8 @@ struct snd_soc_dapm_path {
 	u32 connect:1;	/* source and sink widgets are connected */
 	u32 walked:1;	/* path has been walked */
 	u32 weak:1;	/* path ignored for power management */
+	u32 length:6;	/* path length - used by route checker */
+
 
 	int (*connected)(struct snd_soc_dapm_widget *source,
 			 struct snd_soc_dapm_widget *sink);
@@ -456,6 +466,8 @@ struct snd_soc_dapm_widget {
 	unsigned char shift;			/* bits to shift */
 	unsigned int saved_value;		/* widget saved value */
 	unsigned int value;				/* widget current value */
+	unsigned int path_idx;
+	unsigned int hops;
 	unsigned int mask;			/* non-shifted mask */
 	unsigned int on_val;			/* on state value */
 	unsigned int off_val;			/* off state value */
@@ -518,6 +530,8 @@ struct snd_soc_dapm_context {
 	enum snd_soc_bias_level target_bias_level;
 	struct list_head list;
 
+	int num_valid_paths; /* valid paths for route checker */
+
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs_dapm;
 #endif
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 0a78482..800af10 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -48,6 +48,8 @@
 
 #include <trace/events/asoc.h>
 
+#define DAPM_MAX_HOPS 16
+
 /* dapm power sequences - make this per codec in the future */
 static int dapm_up_seq[] = {
 	[snd_soc_dapm_pre] = 0,
@@ -169,6 +171,366 @@ static int soc_widget_update_bits(struct snd_soc_dapm_widget *w,
 	return change;
 }
 
+/* get snd_card from DAPM context */
+static inline struct snd_card *dapm_get_snd_card(
+	struct snd_soc_dapm_context *dapm)
+{
+	if (dapm->codec)
+		return dapm->codec->card->snd_card;
+	else if (dapm->platform)
+		return dapm->platform->card->snd_card;
+
+	dev_err(dapm->dev, "no snd_card for dapm context\n");
+	return NULL;
+}
+
+/* get soc_card from DAPM context */
+static inline struct snd_soc_card *dapm_get_soc_card(
+		struct snd_soc_dapm_context *dapm)
+{
+	if (dapm->codec)
+		return dapm->codec->card;
+	else if (dapm->platform)
+		return dapm->platform->card;
+
+	dev_err(dapm->dev, "no soc_card for dapm context\n");
+	return NULL;
+}
+
+/* reset 'walked' bit for each dapm path */
+static inline void dapm_clear_walk(struct snd_soc_dapm_context *dapm)
+{
+	struct snd_soc_dapm_path *p;
+
+	list_for_each_entry(p, &dapm->card->paths, list)
+		p->walked = 0;
+}
+
+/* clear all 'walked' path data for each widget */
+static void dapm_clear_paths(struct snd_soc_dapm_context *dapm)
+{
+	struct snd_soc_dapm_path *p;
+	struct snd_soc_dapm_widget *w;
+	struct list_head *l;
+
+	list_for_each(l, &dapm->card->paths) {
+		p = list_entry(l, struct snd_soc_dapm_path, list);
+		p->length = 0;
+	}
+	list_for_each(l, &dapm->card->widgets) {
+		w = list_entry(l, struct snd_soc_dapm_widget, list);
+		w->hops = 0;
+	}
+	dapm_clear_walk(dapm);
+}
+
+/* add widget to list if it's not already in the list */
+static int dapm_add_unique_widget(struct snd_soc_dapm_context *dapm,
+	struct snd_soc_dapm_widget_list **list,
+	struct snd_soc_dapm_widget *w)
+{
+	struct snd_soc_dapm_widget_list *wlist;
+	int wlistsize, wlistentries, i;
+
+	/* is the list empty ? */
+	if (*list == NULL) {
+
+		wlistsize = sizeof(struct snd_soc_dapm_widget_list) +
+				sizeof(struct snd_soc_dapm_widget *);
+		*list = kzalloc(wlistsize, GFP_KERNEL);
+		if (*list == NULL) {
+			dev_err(dapm->dev, "can't allocate widget list for %s\n",
+				w->name);
+			return -ENOMEM;
+		}
+	} else {
+		wlist = *list;
+
+		/* is this widget already in the list */
+		for (i = 0; i < wlist->num_widgets; i++) {
+			if (wlist->widgets[i] == w)
+				return 0;
+		}
+
+		wlistentries = wlist->num_widgets + 1;
+		wlistsize = sizeof(struct snd_soc_dapm_widget_list) +
+				wlistentries * sizeof(struct snd_soc_dapm_widget *);
+		*list = krealloc(wlist, wlistsize, GFP_KERNEL);
+		if (*list == NULL) {
+			dev_err(dapm->dev, "can't allocate widget list for %s\n",
+				w->name);
+			return -ENOMEM;
+		}
+	}
+	wlist = *list;
+
+	/* insert the widget */
+	dev_dbg(dapm->dev, "added %s in widget list pos %d\n",
+			w->name, wlist->num_widgets);
+	wlist->widgets[wlist->num_widgets] = w;
+	wlist->num_widgets++;
+	return 1;
+}
+
+/* is widget an output endpoint */
+static int is_output_widget_ep(struct snd_soc_dapm_widget *widget)
+{
+	switch (widget->id) {
+	case snd_soc_dapm_adc:
+	case snd_soc_dapm_aif_out:
+		return 1;
+	case snd_soc_dapm_output:
+		if (widget->connected && !widget->ext)
+			return 1;
+		else
+			return 0;
+	case snd_soc_dapm_hp:
+	case snd_soc_dapm_spk:
+	case snd_soc_dapm_line:
+		return !list_empty(&widget->sources);
+	default:
+		return 0;
+	}
+}
+
+/* is widget an input endpoint */
+static int is_input_widget_ep(struct snd_soc_dapm_widget *widget)
+{
+	switch (widget->id) {
+	case snd_soc_dapm_dac:
+	case snd_soc_dapm_aif_in:
+		return 1;
+	case snd_soc_dapm_input:
+		if (widget->connected && !widget->ext)
+			return 1;
+		else
+			return 0;
+	case snd_soc_dapm_mic:
+		return !list_empty(&widget->sources);
+	default:
+		return 0;
+	}
+}
+
+/*
+ * Find all the playback paths between the root source widget and
+ * sink widgets. Store all path widgets in the list.
+ */
+static int dapm_find_playback_paths(struct snd_soc_dapm_context *dapm,
+		struct snd_soc_dapm_widget *root,
+		struct snd_soc_dapm_widget_list **list, int hops)
+{
+	struct list_head *lp;
+	struct snd_soc_dapm_path *path;
+	int dist = 0;
+
+	if (hops > DAPM_MAX_HOPS)
+		return 0;
+
+	/* are we at an endpoint widget */
+	if (is_output_widget_ep(root)) {
+		dev_dbg(dapm->dev," ! %d: valid playback route found\n", hops);
+		dapm->num_valid_paths++;
+		return 1;
+	}
+
+	/* have we visited this widget before */
+	if (root->hops && root->hops <= hops)
+		return 0;
+	root->hops = hops;
+
+	/* check all the output paths on this source widget by walking
+	 * from source to sink */
+	list_for_each(lp, &root->sinks) {
+		path = list_entry(lp, struct snd_soc_dapm_path, list_source);
+
+		dev_dbg(dapm->dev," %c %d: %s -> %s -> %s\n",
+				path->connect ? '*' : ' ', hops,
+				root->name, path->name, path->sink->name);
+
+		/* have we been down this path before ? */
+		if (path->length && path->length <= hops)
+			continue;
+
+		/* check down the next path if connected */
+		if (path->sink && path->connect &&
+				dapm_find_playback_paths(dapm, path->sink, list, hops + 1)) {
+			path->length = hops;
+
+			/* add widget to list */
+			dapm_add_unique_widget(dapm, list, path->sink);
+
+			if (!dist || dist > path->length)
+				dist = path->length;
+		}
+	}
+
+	return dist;
+}
+
+/*
+ * Find all the capture paths between the root sink widget and
+ * source widgets. Store all path widgets in the list.
+ */
+static int dapm_find_capture_paths(struct snd_soc_dapm_context *dapm,
+		struct snd_soc_dapm_widget *root,
+		struct snd_soc_dapm_widget_list **list, int hops)
+{
+	struct list_head *lp;
+	struct snd_soc_dapm_path *path;
+	int dist = 0;
+
+	if (hops > DAPM_MAX_HOPS)
+		return 0;
+
+	/* are we at an endpoint widget */
+	if (is_input_widget_ep(root)) {
+		dev_dbg(dapm->dev," ! %d: valid capture route found\n", hops);
+		dapm->num_valid_paths++;
+		return 1;
+	}
+
+	/* have we visited this widget before */
+	if (root->hops && root->hops <= hops)
+		return 0;
+	root->hops = hops;
+
+	/* check all the output paths on this source widget by walking from
+	 * sink to source */
+	list_for_each(lp, &root->sources) {
+		path = list_entry(lp, struct snd_soc_dapm_path, list_sink);
+
+		dev_dbg(dapm->dev," %c %d: %s <- %s <- %s\n",
+				path->connect ? '*' : ' ', hops,
+				root->name, path->name, path->source->name);
+
+		/* have we been here before ? */
+		if (path->length && path->length <= hops)
+			continue;
+
+		/* check down the next path if connected */
+		if (path->source && path->connect &&
+				dapm_find_capture_paths(dapm, path->source, list, hops + 1)) {
+			path->length = hops;
+
+			/* add widget to list */
+			dapm_add_unique_widget(dapm, list, path->source);
+
+			if (!dist || dist > path->length)
+				dist = path->length;
+		}
+	}
+
+	return dist;
+}
+
+static int dapm_get_playback_paths(struct snd_soc_dapm_context *dapm,
+		struct snd_soc_dapm_widget *root,
+		struct snd_soc_dapm_widget_list **list)
+{
+	dev_dbg(dapm->dev, "Playback: checking paths from %s\n",root->name);
+	dapm_find_playback_paths(dapm, root, list, 0);
+	return dapm->num_valid_paths;
+}
+
+static int dapm_get_capture_paths(struct snd_soc_dapm_context *dapm,
+		struct snd_soc_dapm_widget *root,
+		struct snd_soc_dapm_widget_list **list)
+{
+	dev_dbg(dapm->dev, "Capture: checking paths to %s\n", root->name);
+	dapm_find_capture_paths(dapm, root, list, 0);
+	return dapm->num_valid_paths;
+}
+
+/**
+ * snd_soc_dapm_get_connected_widgets_type - query audio path and it's widgets.
+ * @dapm: the dapm context.
+ * @stream_name: stream name.
+ * @list: list of active widgets for this stream.
+ * @stream: stream direction.
+ * @type: Initial widget type.
+ *
+ * Queries DAPM graph as to whether an valid audio stream path exists for
+ * the DAPM stream and initial widget type specified. This takes into account
+ * current mixer and mux kcontrol settings. Creates list of valid widgets.
+ *
+ * Returns the number of valid paths or negative error.
+ */
+int snd_soc_dapm_get_connected_widgets_type(
+	struct snd_soc_dapm_context *dapm, const char *stream_name,
+	struct snd_soc_dapm_widget_list **list, int stream,
+	enum snd_soc_dapm_type type)
+{
+	struct snd_soc_dapm_widget *w;
+	int paths;
+
+	/* get stream root widget AIF, DAC or ADC from stream string
+	 * and direction */
+	list_for_each_entry(w, &dapm->card->widgets, list) {
+
+		if (!w->sname)
+			continue;
+
+		if (w->id != type)
+			continue;
+
+		if (strstr(w->sname, stream_name))
+			goto found;
+	}
+	dev_err(dapm->dev, "root widget not found\n");
+	return 0;
+
+found:
+	dapm->num_valid_paths = 0;
+	*list = NULL;
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK)
+		paths = dapm_get_playback_paths(dapm, w, list);
+	else
+		paths = dapm_get_capture_paths(dapm, w, list);
+
+	dapm_clear_paths(dapm);
+	return paths;
+}
+
+/**
+ * snd_soc_dapm_get_connected_widgets_name - query audio path and it's widgets.
+ * @dapm: the dapm context.
+ * @name: initial widget name.
+ * @list: list of active widgets for this stream.
+ * @stream: stream direction.
+ *
+ * Queries DAPM graph as to whether an valid audio stream path exists for
+ * the initial widget specified by name. This takes into account
+ * current mixer and mux kcontrol settings. Creates list of valid widgets.
+ *
+ * Returns the number of valid paths or negative error.
+ */
+int snd_soc_dapm_get_connected_widgets_name(struct snd_soc_dapm_context *dapm,
+		const char *name, struct snd_soc_dapm_widget_list **list, int stream)
+{
+	struct snd_soc_dapm_widget *w;
+	int paths;
+
+	/* get stream root widget AIF, DAC or ADC from stream string
+	 * and direction */
+	list_for_each_entry(w, &dapm->card->widgets, list) {
+
+		if (strstr(w->name, name))
+			goto found;
+	}
+	dev_err(dapm->dev, "root widget %s not found\n", name);
+	return 0;
+
+found:
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK)
+		paths = dapm_get_playback_paths(dapm, w, list);
+	else
+		paths = dapm_get_capture_paths(dapm, w, list);
+
+	dapm_clear_paths(dapm);
+	return paths;
+}
+
 /**
  * snd_soc_dapm_set_bias_level - set the bias level for the system
  * @dapm: DAPM context
@@ -576,15 +938,6 @@ static int dapm_new_pga(struct snd_soc_dapm_widget *w)
 	return 0;
 }
 
-/* reset 'walked' bit for each dapm path */
-static inline void dapm_clear_walk(struct snd_soc_dapm_context *dapm)
-{
-	struct snd_soc_dapm_path *p;
-
-	list_for_each_entry(p, &dapm->card->paths, list)
-		p->walked = 0;
-}
-
 /* We implement power down on suspend by checking the power state of
  * the ALSA card - when we are suspending the ALSA state for the card
  * is set to D3.
-- 
1.7.4.1

Re: [PATCH] ASoC: dapm - Add API calls to query valid DAPM paths.

[Mark Brown]

On Wed, Jul 06, 2011 at 08:49:28PM +0100, Liam Girdwood wrote:

> Add DAPM API calls to determine whether a DAPM audio path is valid between
> source and sink widgets and return a lits of active widgets in that path.
> This also takes into account all kcontrol mux and mixer settings in between
> the source and sink widgets to validate the audio path.

Hrm, I'm not entirely sure about this.  My overall feeling is that it
feels complicated and I'm not comfortable that it's doing something
different to the rest of DAPM that doesn't normally get used.  It
doesn't seem like the tree walk we're doing here should be different
enough to the walk we do for the normal power check to require a
completely separate implementation and having the two checks feels like
it's going to cause issues later on.  I may be missing something but
shouldn't we be able to replace the current list walk with a walk which
uses this sort of interface on each relevant widget it finds?

There's already some drift with things like the callbacks for dynamic
routes not being implemented...

It does also occur to me that perhaps what we want to do here is allow
widgets to find out about paths when they're being activated anyway?
That'd also be useful for things like DVFS charge pumps, sometimes they
can easily be controlled with a fake widget but not always if there's
complicated digital routing within the device.

> @@ -432,6 +440,8 @@ struct snd_soc_dapm_path {
>  	u32 connect:1;	/* source and sink widgets are connected */
>  	u32 walked:1;	/* path has been walked */
>  	u32 weak:1;	/* path ignored for power management */
> +	u32 length:6;	/* path length - used by route checker */
> +

The :6 seems very odd here.

> index 0a78482..800af10 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -48,6 +48,8 @@
>  
>  #include <trace/events/asoc.h>
>  
> +#define DAPM_MAX_HOPS 16
> +

This seems too low, if we need a limit I'd expect it to be an order of
magnitude higher.  I'm not sure we should need a limit, though.

> +/* add widget to list if it's not already in the list */
> +static int dapm_add_unique_widget(struct snd_soc_dapm_context *dapm,
> +	struct snd_soc_dapm_widget_list **list,
> +	struct snd_soc_dapm_widget *w)

Just make this plain old add_widget()?  I can't see us ever wanting a
non-unique version, and it's going to have fun with the standard list
implementation if we do.  Perhaps also shove a list in the name.

> +	/* is the list empty ? */
> +	if (*list == NULL) {
> +
> +		wlistsize = sizeof(struct snd_soc_dapm_widget_list) +
> +				sizeof(struct snd_soc_dapm_widget *);
> +		*list = kzalloc(wlistsize, GFP_KERNEL);
> +		if (*list == NULL) {
> +			dev_err(dapm->dev, "can't allocate widget list for %s\n",
> +				w->name);
> +			return -ENOMEM;

It seems really odd to be allocating here rather than in the caller.

> +		*list = krealloc(wlist, wlistsize, GFP_KERNEL);
> +		if (*list == NULL) {
> +			dev_err(dapm->dev, "can't allocate widget list for %s\n",
> +				w->name);
> +			return -ENOMEM;
> +		}

This will leak the pointer to the list when it fails.

> +/* is widget an output endpoint */
> +static int is_output_widget_ep(struct snd_soc_dapm_widget *widget)
> +{
> +	switch (widget->id) {
> +	case snd_soc_dapm_adc:
> +	case snd_soc_dapm_aif_out:
> +		return 1;
> +	case snd_soc_dapm_output:
> +		if (widget->connected && !widget->ext)
> +			return 1;
> +		else
> +			return 0;
> +	case snd_soc_dapm_hp:
> +	case snd_soc_dapm_spk:
> +	case snd_soc_dapm_line:
> +		return !list_empty(&widget->sources);
> +	default:
> +		return 0;

With both this and the input version we're duplicating sort of the logic
that's in is_connected_foo_ep.  I'd feel much happier if we factored the
shared logic out of those functions rather than duplicating it, as it is
I'm worried the two could drift apart over time.

> +	case snd_soc_dapm_mic:
> +		return !list_empty(&widget->sources);

That doesn't seem right?

> +/**
> + * snd_soc_dapm_get_connected_widgets_type - query audio path and it's widgets.
> + * @dapm: the dapm context.
> + * @stream_name: stream name.
> + * @list: list of active widgets for this stream.
> + * @stream: stream direction.
> + * @type: Initial widget type.
> + *
> + * Queries DAPM graph as to whether an valid audio stream path exists for
> + * the DAPM stream and initial widget type specified. This takes into account
> + * current mixer and mux kcontrol settings. Creates list of valid widgets.

Why would someone want to query by type?  That seems surprising.  Name
and/or direction yes but type seems hard to find a use for.

Re: [PATCH] ASoC: dapm - Add API calls to query valid DAPM paths.

[Liam Girdwood]

On 07/07/11 04:32, Mark Brown wrote:
> On Wed, Jul 06, 2011 at 08:49:28PM +0100, Liam Girdwood wrote:
> 
>> Add DAPM API calls to determine whether a DAPM audio path is valid between
>> source and sink widgets and return a lits of active widgets in that path.
>> This also takes into account all kcontrol mux and mixer settings in between
>> the source and sink widgets to validate the audio path.
> 
> Hrm, I'm not entirely sure about this.  My overall feeling is that it
> feels complicated and I'm not comfortable that it's doing something
> different to the rest of DAPM that doesn't normally get used.  It
> doesn't seem like the tree walk we're doing here should be different
> enough to the walk we do for the normal power check to require a
> completely separate implementation and having the two checks feels like
> it's going to cause issues later on.  I may be missing something but
> shouldn't we be able to replace the current list walk with a walk which
> uses this sort of interface on each relevant widget it finds?
> 
> There's already some drift with things like the callbacks for dynamic
> routes not being implemented...
> 

The reason for using a separate walk here was because this code is taken from my old auto-router and it was to avoid the extra logic introduced by the auto-router. The auto-router did need to check for loops and always select the shortest path when > 1 path was viable (interesting as WM9713 has both loops and multiple paths). Let me see how cleanly I can merge it into the current walk.

> It does also occur to me that perhaps what we want to do here is allow
> widgets to find out about paths when they're being activated anyway?

For dynamic PCM, we need to also work out which DAIs are active based on the graph. Hence for playback we can supply the DAC/AIF and find out all the connected output AIF/Speakers/Pins/etc. This is required at the start of open() in order that we can call the other PCM ops for each DAI, codec and platform.

> That'd also be useful for things like DVFS charge pumps, sometimes they
> can easily be controlled with a fake widget but not always if there's
> complicated digital routing within the device.
> 

Yeah, this can also be used to figure out the paths when widgets are activated too. I can export the API calls in order for components to make use of this information when their widgets are activated.

>> @@ -432,6 +440,8 @@ struct snd_soc_dapm_path {
>>  	u32 connect:1;	/* source and sink widgets are connected */
>>  	u32 walked:1;	/* path has been walked */
>>  	u32 weak:1;	/* path ignored for power management */
>> +	u32 length:6;	/* path length - used by route checker */
>> +
> 
> The :6 seems very odd here.

Yeah, this came from auto-router when I think we cared about squeezing this all into a u32.

> 
>> index 0a78482..800af10 100644
>> --- a/sound/soc/soc-dapm.c
>> +++ b/sound/soc/soc-dapm.c
>> @@ -48,6 +48,8 @@
>>  
>>  #include <trace/events/asoc.h>
>>  
>> +#define DAPM_MAX_HOPS 16
>> +
> 
> This seems too low, if we need a limit I'd expect it to be an order of
> magnitude higher.  I'm not sure we should need a limit, though.

Agreed, I will remove it.

> 
>> +/* add widget to list if it's not already in the list */
>> +static int dapm_add_unique_widget(struct snd_soc_dapm_context *dapm,
>> +	struct snd_soc_dapm_widget_list **list,
>> +	struct snd_soc_dapm_widget *w)
> 
> Just make this plain old add_widget()?  I can't see us ever wanting a
> non-unique version, and it's going to have fun with the standard list
> implementation if we do.  Perhaps also shove a list in the name.

Yeah, that does sound better.

> 
>> +	/* is the list empty ? */
>> +	if (*list == NULL) {
>> +
>> +		wlistsize = sizeof(struct snd_soc_dapm_widget_list) +
>> +				sizeof(struct snd_soc_dapm_widget *);
>> +		*list = kzalloc(wlistsize, GFP_KERNEL);
>> +		if (*list == NULL) {
>> +			dev_err(dapm->dev, "can't allocate widget list for %s\n",
>> +				w->name);
>> +			return -ENOMEM;
> 
> It seems really odd to be allocating here rather than in the caller.
> 
>> +		*list = krealloc(wlist, wlistsize, GFP_KERNEL);
>> +		if (*list == NULL) {
>> +			dev_err(dapm->dev, "can't allocate widget list for %s\n",
>> +				w->name);
>> +			return -ENOMEM;
>> +		}
> 
> This will leak the pointer to the list when it fails.
> 
>> +/* is widget an output endpoint */
>> +static int is_output_widget_ep(struct snd_soc_dapm_widget *widget)
>> +{
>> +	switch (widget->id) {
>> +	case snd_soc_dapm_adc:
>> +	case snd_soc_dapm_aif_out:
>> +		return 1;
>> +	case snd_soc_dapm_output:
>> +		if (widget->connected && !widget->ext)
>> +			return 1;
>> +		else
>> +			return 0;
>> +	case snd_soc_dapm_hp:
>> +	case snd_soc_dapm_spk:
>> +	case snd_soc_dapm_line:
>> +		return !list_empty(&widget->sources);
>> +	default:
>> +		return 0;
> 
> With both this and the input version we're duplicating sort of the logic
> that's in is_connected_foo_ep.  I'd feel much happier if we factored the
> shared logic out of those functions rather than duplicating it, as it is
> I'm worried the two could drift apart over time.
> 
>> +	case snd_soc_dapm_mic:
>> +		return !list_empty(&widget->sources);
> 
> That doesn't seem right?
> 
>> +/**
>> + * snd_soc_dapm_get_connected_widgets_type - query audio path and it's widgets.
>> + * @dapm: the dapm context.
>> + * @stream_name: stream name.
>> + * @list: list of active widgets for this stream.
>> + * @stream: stream direction.
>> + * @type: Initial widget type.
>> + *
>> + * Queries DAPM graph as to whether an valid audio stream path exists for
>> + * the DAPM stream and initial widget type specified. This takes into account
>> + * current mixer and mux kcontrol settings. Creates list of valid widgets.
> 
> Why would someone want to query by type?  That seems surprising.  Name
> and/or direction yes but type seems hard to find a use for.

We query by type so that we can qualify the root widget for a shared common stream name. i.e. Dynamic DSP needs to find the AIF rather than a DAC for stream X.

Liam

Re: [PATCH] ASoC: dapm - Add API calls to query valid DAPM paths.

[Mark Brown]

On Thu, Jul 07, 2011 at 05:18:47PM +0100, Liam Girdwood wrote:

[Please fix your mailer to word wrap within paragraphs, I've reflowed
all your text.]

> The reason for using a separate walk here was because this code is
> taken from my old auto-router and it was to avoid the extra logic
> introduced by the auto-router. The auto-router did need to check for
> loops and always select the shortest path when > 1 path was viable
> (interesting as WM9713 has both loops and multiple paths). Let me see
> how cleanly I can merge it into the current walk.

Alternatively could we replace the current walk?  It's O(n^2) or so in
the number of widgets so it's not the greatest treasure ever.  So long
as we only have one walk and it works I'm happy.

> > It does also occur to me that perhaps what we want to do here is allow
> > widgets to find out about paths when they're being activated anyway?

> For dynamic PCM, we need to also work out which DAIs are active based
> on the graph. Hence for playback we can supply the DAC/AIF and find
> out all the connected output AIF/Speakers/Pins/etc. This is required
> at the start of open() in order that we can call the other PCM ops for
> each DAI, codec and platform.

Right, that's not really what I'm saying though - I'm saying that
widgets in general could be interested in this information.  For
example, some charge pumps can save additional power with some input
signal paths (which is currently handled but this would make that code
more general).

> >> + * snd_soc_dapm_get_connected_widgets_type - query audio path and it's widgets.
> >> + * @dapm: the dapm context.
> >> + * @stream_name: stream name.
> >> + * @list: list of active widgets for this stream.
> >> + * @stream: stream direction.
> >> + * @type: Initial widget type.
> >> + *
> >> + * Queries DAPM graph as to whether an valid audio stream path exists for
> >> + * the DAPM stream and initial widget type specified. This takes into account
> >> + * current mixer and mux kcontrol settings. Creates list of valid widgets.
> > 
> > Why would someone want to query by type?  That seems surprising.  Name
> > and/or direction yes but type seems hard to find a use for.

> We query by type so that we can qualify the root widget for a shared
> common stream name. i.e. Dynamic DSP needs to find the AIF rather than
> a DAC for stream X.

It does?  Hrm.  Perhaps it would make life simpler if we just mandated
the use of explicit AIF widgets?

Re: [PATCH] ASoC: dapm - Add API calls to query valid DAPM paths.

[Liam Girdwood]

On 07/07/11 17:38, Mark Brown wrote:
> On Thu, Jul 07, 2011 at 05:18:47PM +0100, Liam Girdwood wrote:
> 
>> The reason for using a separate walk here was because this code is
>> taken from my old auto-router and it was to avoid the extra logic
>> introduced by the auto-router. The auto-router did need to check for
>> loops and always select the shortest path when > 1 path was viable
>> (interesting as WM9713 has both loops and multiple paths). Let me see
>> how cleanly I can merge it into the current walk.
> 
> Alternatively could we replace the current walk?  It's O(n^2) or so in
> the number of widgets so it's not the greatest treasure ever.  So long
> as we only have one walk and it works I'm happy.
> 
>>> It does also occur to me that perhaps what we want to do here is allow
>>> widgets to find out about paths when they're being activated anyway?
> 
>> For dynamic PCM, we need to also work out which DAIs are active based
>> on the graph. Hence for playback we can supply the DAC/AIF and find
>> out all the connected output AIF/Speakers/Pins/etc. This is required
>> at the start of open() in order that we can call the other PCM ops for
>> each DAI, codec and platform.
> 
> Right, that's not really what I'm saying though - I'm saying that
> widgets in general could be interested in this information.  For
> example, some charge pumps can save additional power with some input
> signal paths (which is currently handled but this would make that code
> more general).

Ok, but I'm not stopping this sort of functionality from being added
later. Right now this is not a priority (for me anyway).

> 
>>>> + * snd_soc_dapm_get_connected_widgets_type - query audio path and it's widgets.
>>>> + * @dapm: the dapm context.
>>>> + * @stream_name: stream name.
>>>> + * @list: list of active widgets for this stream.
>>>> + * @stream: stream direction.
>>>> + * @type: Initial widget type.
>>>> + *
>>>> + * Queries DAPM graph as to whether an valid audio stream path exists for
>>>> + * the DAPM stream and initial widget type specified. This takes into account
>>>> + * current mixer and mux kcontrol settings. Creates list of valid widgets.
>>>
>>> Why would someone want to query by type?  That seems surprising.  Name
>>> and/or direction yes but type seems hard to find a use for.
> 
>> We query by type so that we can qualify the root widget for a shared
>> common stream name. i.e. Dynamic DSP needs to find the AIF rather than
>> a DAC for stream X.
> 
> It does?  Hrm.  Perhaps it would make life simpler if we just mandated
> the use of explicit AIF widgets?

But we may also want to know what was after a DAC. I'm thinking back
from the conference discussions where we could use this information to
selectively enable/disable kcontrols. However, this is not going to be
used at the moment either so I'll stick by the AIF widgets.

Liam

Re: [PATCH] ASoC: dapm - Add API calls to query valid DAPM paths.

[Mark Brown]

Funny formatting due to phone, sorry...

Indeed, but I'd have expected that anything that needed to do that would be able to (and want to) ask by name rather than type?

Liam Girdwood <lrg@ti.com> wrote:

>On 07/07/11 17:38, Mark Brown wrote:
>> On Thu, Jul 07, 2011 at 05:18:47PM +0100, Liam Girdwood wrote:
>> 
>>> The reason for using a separate walk here was because this code is
>>> taken from my old auto-router and it was to avoid the extra logic
>>> introduced by the auto-router. The auto-router did need to check for
>>> loops and always select the shortest path when > 1 path was viable
>>> (interesting as WM9713 has both loops and multiple paths). Let me see
>>> how cleanly I can merge it into the current walk.
>> 
>> Alternatively could we replace the current walk?  It's O(n^2) or so in
>> the number of widgets so it's not the greatest treasure ever.  So long
>> as we only have one walk and it works I'm happy.
>> 
>>>> It does also occur to me that perhaps what we want to do here is allow
>>>> widgets to find out about paths when they're being activated anyway?
>> 
>>> For dynamic PCM, we need to also work out which DAIs are active based
>>> on the graph. Hence for playback we can supply the DAC/AIF and find
>>> out all the connected output AIF/Speakers/Pins/etc. This is required
>>> at the start of open() in order that we can call the other PCM ops for
>>> each DAI, codec and platform.
>> 
>> Right, that's not really what I'm saying though - I'm saying that
>> widgets in general could be interested in this information.  For
>> example, some charge pumps can save additional power with some input
>> signal paths (which is currently handled but this would make that code
>> more general).
>
>Ok, but I'm not stopping this sort of functionality from being added
>later. Right now this is not a priority (for me anyway).
>
>> 
>>>>> + * snd_soc_dapm_get_connected_widgets_type - query audio path and it's widgets.
>>>>> + * @dapm: the dapm context.
>>>>> + * @stream_name: stream name.
>>>>> + * @list: list of active widgets for this stream.
>>>>> + * @stream: stream direction.
>>>>> + * @type: Initial widget type.
>>>>> + *
>>>>> + * Queries DAPM graph as to whether an valid audio stream path exists for
>>>>> + * the DAPM stream and initial widget type specified. This takes into account
>>>>> + * current mixer and mux kcontrol settings. Creates list of valid widgets.
>>>>
>>>> Why would someone want to query by type?  That seems surprising.  Name
>>>> and/or direction yes but type seems hard to find a use for.
>> 
>>> We query by type so that we can qualify the root widget for a shared
>>> common stream name. i.e. Dynamic DSP needs to find the AIF rather than
>>> a DAC for stream X.
>> 
>> It does?  Hrm.  Perhaps it would make life simpler if we just mandated
>> the use of explicit AIF widgets?
>
>But we may also want to know what was after a DAC. I'm thinking back
>from the conference discussions where we could use this information to
>selectively enable/disable kcontrols. However, this is not going to be
>used at the moment either so I'll stick by the AIF widgets.
>
>Liam
>
>

Re: [PATCH] ASoC: dapm - Add API calls to query valid DAPM paths.

[Liam Girdwood]

On Thu, 2011-07-07 at 21:50 +0200, Mark Brown wrote:
> Funny formatting due to phone, sorry...

You are forgiven ;)

> 
> Indeed, but I'd have expected that anything that needed to do that would be able to (and want to) ask by name rather than type?
> 

I was generally thinking here. I'll keep it to the AIF atm and if there
is a good use for type it can always be added back later.

Liam

> Liam Girdwood <lrg@ti.com> wrote:
> 
> >On 07/07/11 17:38, Mark Brown wrote:
> >> On Thu, Jul 07, 2011 at 05:18:47PM +0100, Liam Girdwood wrote:
> >> 
> >>> The reason for using a separate walk here was because this code is
> >>> taken from my old auto-router and it was to avoid the extra logic
> >>> introduced by the auto-router. The auto-router did need to check for
> >>> loops and always select the shortest path when > 1 path was viable
> >>> (interesting as WM9713 has both loops and multiple paths). Let me see
> >>> how cleanly I can merge it into the current walk.
> >> 
> >> Alternatively could we replace the current walk?  It's O(n^2) or so in
> >> the number of widgets so it's not the greatest treasure ever.  So long
> >> as we only have one walk and it works I'm happy.
> >> 
> >>>> It does also occur to me that perhaps what we want to do here is allow
> >>>> widgets to find out about paths when they're being activated anyway?
> >> 
> >>> For dynamic PCM, we need to also work out which DAIs are active based
> >>> on the graph. Hence for playback we can supply the DAC/AIF and find
> >>> out all the connected output AIF/Speakers/Pins/etc. This is required
> >>> at the start of open() in order that we can call the other PCM ops for
> >>> each DAI, codec and platform.
> >> 
> >> Right, that's not really what I'm saying though - I'm saying that
> >> widgets in general could be interested in this information.  For
> >> example, some charge pumps can save additional power with some input
> >> signal paths (which is currently handled but this would make that code
> >> more general).
> >
> >Ok, but I'm not stopping this sort of functionality from being added
> >later. Right now this is not a priority (for me anyway).
> >
> >> 
> >>>>> + * snd_soc_dapm_get_connected_widgets_type - query audio path and it's widgets.
> >>>>> + * @dapm: the dapm context.
> >>>>> + * @stream_name: stream name.
> >>>>> + * @list: list of active widgets for this stream.
> >>>>> + * @stream: stream direction.
> >>>>> + * @type: Initial widget type.
> >>>>> + *
> >>>>> + * Queries DAPM graph as to whether an valid audio stream path exists for
> >>>>> + * the DAPM stream and initial widget type specified. This takes into account
> >>>>> + * current mixer and mux kcontrol settings. Creates list of valid widgets.
> >>>>
> >>>> Why would someone want to query by type?  That seems surprising.  Name
> >>>> and/or direction yes but type seems hard to find a use for.
> >> 
> >>> We query by type so that we can qualify the root widget for a shared
> >>> common stream name. i.e. Dynamic DSP needs to find the AIF rather than
> >>> a DAC for stream X.
> >> 
> >> It does?  Hrm.  Perhaps it would make life simpler if we just mandated
> >> the use of explicit AIF widgets?
> >
> >But we may also want to know what was after a DAC. I'm thinking back
> >from the conference discussions where we could use this information to
> >selectively enable/disable kcontrols. However, this is not going to be
> >used at the moment either so I'll stick by the AIF widgets.
> >
> >Liam
> >
> >