[PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.

[PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.

[Lars-Peter Clausen]

Commit 52ba67bf(ASoC: Force all DAPM contexts into the same bias state) changed
dapm_power_widgets to power up all DAPM contexts if at least one context is
powered. As a side-effect of this change it is possible, that a card is not
powered down again although non of it's DAPM contexts should be powered.

This problem can arise if the card contains at least one widget-less DAPM
context.
As opposed to other DAPM contexts the power state for widget-less contexts is
not reset at the beginning of dapm_power_widgets, so once such a context is
powered it stays powered and keeps the whole card in a powered state with it.
For widget-less codec DAPM contexts it is possible to recover from this
situation by sending a dapm_stream_event. Since non-codec DAPM contexts (for
example card contexts) do not receive stream events, cards containing such
contexts will get stuck in powered state indefinitely.
This is currently the case for most cards where the card's DAPM context itself
is widget-less.
If there is more then one widget-less context the card will stay powered
indefinitely as well, since it is not possible to send stream events to two DAPM
contexts at once.

This patch modifies dapm_power_widgets to iterate over all widget-less DAPM
contexts, instead of just looking at the current DAPM context, and if that
context has a codec assigned it looks at the codec state.
If the codec is active and not suspended it is assumed that the context should
be powered.
Otherwise it is assumed that the context does not have to be powered.

As before, if at least one DAPM context should be powered all other contexts are
power as well, to keep all context in the same bias state.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/soc-dapm.c |   51 +++++++++++++------------------------------------
 1 files changed, 14 insertions(+), 37 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 378f08a..3f2e360 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1025,13 +1025,10 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event)
 	LIST_HEAD(down_list);
 	LIST_HEAD(async_domain);
 	int power;
+	int power_card = 0;
 
 	trace_snd_soc_dapm_start(card);
 
-	list_for_each_entry(d, &card->dapm_list, list)
-		if (d->n_widgets)
-			d->dev_power = 0;
-
 	/* Check which widgets we need to power and store them in
 	 * lists indicating if they should be powered up or down.
 	 */
@@ -1053,7 +1050,7 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event)
 			else
 				power = 1;
 			if (power)
-				w->dapm->dev_power = 1;
+				power_card = 1;
 
 			if (w->power == power)
 				continue;
@@ -1070,45 +1067,25 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event)
 		}
 	}
 
-	/* If there are no DAPM widgets then try to figure out power from the
-	 * event type.
+	/* If we have not found a DAPM context yet, which should be powered,
+	 * iterate over all widget-less DAPM codec contexts and try to figure
+	 * out whether it should be powered from the codec state.
 	 */
-	if (!dapm->n_widgets) {
-		switch (event) {
-		case SND_SOC_DAPM_STREAM_START:
-		case SND_SOC_DAPM_STREAM_RESUME:
-			dapm->dev_power = 1;
-			break;
-		case SND_SOC_DAPM_STREAM_STOP:
-			dapm->dev_power = !!dapm->codec->active;
-			break;
-		case SND_SOC_DAPM_STREAM_SUSPEND:
-			dapm->dev_power = 0;
-			break;
-		case SND_SOC_DAPM_STREAM_NOP:
-			switch (dapm->bias_level) {
-				case SND_SOC_BIAS_STANDBY:
-				case SND_SOC_BIAS_OFF:
-					dapm->dev_power = 0;
-					break;
-				default:
-					dapm->dev_power = 1;
-					break;
+	if (!power_card) {
+		list_for_each_entry(d, &card->dapm_list, list) {
+			if (d->n_widgets || !d->codec)
+				continue;
+
+			if (d->codec->active && !d->codec->suspended) {
+				power_card = 1;
+				break;
 			}
-			break;
-		default:
-			break;
 		}
 	}
 
 	/* Force all contexts in the card to the same bias state */
-	power = 0;
 	list_for_each_entry(d, &card->dapm_list, list)
-		if (d->dev_power)
-			power = 1;
-	list_for_each_entry(d, &card->dapm_list, list)
-		d->dev_power = power;
-
+		d->dev_power = power_card;
 
 	/* Run all the bias changes in parallel */
 	list_for_each_entry(d, &dapm->card->dapm_list, list)
-- 
1.7.2.5

[PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Lars-Peter Clausen]

Pass the snd_dapm_soc_update struct as parameter to dapm_widget_update instead
of passing it indirectly as a field of the snd_soc_dapm_context struct.
This should make it more obvious were the struct is coming from and make the
code easier to comprehend.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 include/sound/soc-dapm.h |    2 -
 sound/soc/soc-dapm.c     |   47 ++++++++++++++++++++-------------------------
 2 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index d5f1b9a..cd52ad0 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -498,8 +498,6 @@ struct snd_soc_dapm_context {
 	struct delayed_work delayed_work;
 	unsigned int idle_bias_off:1; /* Use BIAS_OFF instead of STANDBY */
 
-	struct snd_soc_dapm_update *update;
-
 	void (*seq_notifier)(struct snd_soc_dapm_context *,
 			     enum snd_soc_dapm_type, int);
 
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 3f2e360..b51882e 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -917,9 +917,8 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm,
 	}
 }
 
-static void dapm_widget_update(struct snd_soc_dapm_context *dapm)
+static void dapm_widget_update(struct snd_soc_dapm_update *update)
 {
-	struct snd_soc_dapm_update *update = dapm->update;
 	struct snd_soc_dapm_widget *w;
 	int ret;
 
@@ -1016,7 +1015,8 @@ static void dapm_post_sequence_async(void *data, async_cookie_t cookie)
  *  o Input pin to Output pin (bypass, sidetone)
  *  o DAC to ADC (loopback).
  */
-static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event)
+static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event,
+	struct snd_soc_dapm_update *update)
 {
 	struct snd_soc_card *card = dapm->card;
 	struct snd_soc_dapm_widget *w;
@@ -1096,7 +1096,7 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event)
 	/* Power down widgets first; try to avoid amplifying pops. */
 	dapm_seq_run(dapm, &down_list, event, false);
 
-	dapm_widget_update(dapm);
+	dapm_widget_update(update);
 
 	/* Now power up. */
 	dapm_seq_run(dapm, &up_list, event, true);
@@ -1268,8 +1268,10 @@ void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm)
 /* test and update the power status of a mux widget */
 static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
 				 struct snd_kcontrol *kcontrol, int change,
-				 int mux, struct soc_enum *e)
+				 int mux, struct soc_enum *e,
+				 struct snd_soc_dapm_update *update)
 {
+	struct snd_soc_dapm_context *dapm = widget->dapm;
 	struct snd_soc_dapm_path *path;
 	int found = 0;
 
@@ -1282,7 +1284,7 @@ static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
 		return 0;
 
 	/* find dapm widget path assoc with kcontrol */
-	list_for_each_entry(path, &widget->dapm->card->paths, list) {
+	list_for_each_entry(path, &dapm->card->paths, list) {
 		if (path->kcontrol != kcontrol)
 			continue;
 
@@ -1298,15 +1300,17 @@ static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
 	}
 
 	if (found)
-		dapm_power_widgets(widget->dapm, SND_SOC_DAPM_STREAM_NOP);
+		dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, update);
 
 	return 0;
 }
 
 /* test and update the power status of a mixer or switch widget */
 static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
-				   struct snd_kcontrol *kcontrol, int connect)
+				   struct snd_kcontrol *kcontrol, int connect,
+				   struct snd_soc_dapm_update *update)
 {
+	struct snd_soc_dapm_context *dapm = widget->dapm;
 	struct snd_soc_dapm_path *path;
 	int found = 0;
 
@@ -1316,7 +1320,7 @@ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
 		return -ENODEV;
 
 	/* find dapm widget path assoc with kcontrol */
-	list_for_each_entry(path, &widget->dapm->card->paths, list) {
+	list_for_each_entry(path, &dapm->card->paths, list) {
 		if (path->kcontrol != kcontrol)
 			continue;
 
@@ -1327,7 +1331,7 @@ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
 	}
 
 	if (found)
-		dapm_power_widgets(widget->dapm, SND_SOC_DAPM_STREAM_NOP);
+		dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, update);
 
 	return 0;
 }
@@ -1485,7 +1489,7 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
  */
 int snd_soc_dapm_sync(struct snd_soc_dapm_context *dapm)
 {
-	return dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP);
+	return dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, NULL);
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_sync);
 
@@ -1737,7 +1741,7 @@ int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
 		w->new = 1;
 	}
 
-	dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP);
+	dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, NULL);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_new_widgets);
@@ -1829,11 +1833,8 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 		update.reg = reg;
 		update.mask = mask;
 		update.val = val;
-		widget->dapm->update = &update;
 
-		dapm_mixer_update_power(widget, kcontrol, connect);
-
-		widget->dapm->update = NULL;
+		dapm_mixer_update_power(widget, kcontrol, connect, &update);
 	}
 
 	mutex_unlock(&widget->codec->mutex);
@@ -1910,11 +1911,8 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 	update.reg = e->reg;
 	update.mask = mask;
 	update.val = val;
-	widget->dapm->update = &update;
-
-	dapm_mux_update_power(widget, kcontrol, change, mux, e);
 
-	widget->dapm->update = NULL;
+	dapm_mux_update_power(widget, kcontrol, change, mux, e, &update);
 
 	mutex_unlock(&widget->codec->mutex);
 	return change;
@@ -1962,7 +1960,7 @@ int snd_soc_dapm_put_enum_virt(struct snd_kcontrol *kcontrol,
 
 	change = widget->value != ucontrol->value.enumerated.item[0];
 	widget->value = ucontrol->value.enumerated.item[0];
-	dapm_mux_update_power(widget, kcontrol, change, widget->value, e);
+	dapm_mux_update_power(widget, kcontrol, change, widget->value, e, NULL);
 
 	mutex_unlock(&widget->codec->mutex);
 	return ret;
@@ -2052,11 +2050,8 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 	update.reg = e->reg;
 	update.mask = mask;
 	update.val = val;
-	widget->dapm->update = &update;
-
-	dapm_mux_update_power(widget, kcontrol, change, mux, e);
 
-	widget->dapm->update = NULL;
+	dapm_mux_update_power(widget, kcontrol, change, mux, e, &update);
 
 	mutex_unlock(&widget->codec->mutex);
 	return change;
@@ -2237,7 +2232,7 @@ static void soc_dapm_stream_event(struct snd_soc_dapm_context *dapm,
 		}
 	}
 
-	dapm_power_widgets(dapm, event);
+	dapm_power_widgets(dapm, event, NULL);
 }
 
 /**
-- 
1.7.2.5

[PATCH 3/7] ASoC: Drop unused parameter from dapm_seq_run

[Lars-Peter Clausen]

Drop the dapm_context parameter from dapm_seq_run. It is not used anymore since
commit 7be31be8(ASoC: Extend DAPM to handle power changes on cross-device paths)

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/soc-dapm.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index b51882e..2801c6a 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -823,8 +823,7 @@ static void dapm_seq_run_coalesced(struct snd_soc_dapm_context *dapm,
  * Currently anything that requires more than a single write is not
  * handled.
  */
-static void dapm_seq_run(struct snd_soc_dapm_context *dapm,
-			 struct list_head *list, int event, bool power_up)
+static void dapm_seq_run(struct list_head *list, int event, bool power_up)
 {
 	struct snd_soc_dapm_widget *w, *n;
 	LIST_HEAD(pending);
@@ -1094,12 +1093,12 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event,
 	async_synchronize_full_domain(&async_domain);
 
 	/* Power down widgets first; try to avoid amplifying pops. */
-	dapm_seq_run(dapm, &down_list, event, false);
+	dapm_seq_run(&down_list, event, false);
 
 	dapm_widget_update(update);
 
 	/* Now power up. */
-	dapm_seq_run(dapm, &up_list, event, true);
+	dapm_seq_run(&up_list, event, true);
 
 	/* Run all the bias changes in parallel */
 	list_for_each_entry(d, &dapm->card->dapm_list, list)
@@ -2425,7 +2424,7 @@ static void soc_dapm_shutdown_codec(struct snd_soc_dapm_context *dapm)
 	 */
 	if (powerdown) {
 		snd_soc_dapm_set_bias_level(dapm, SND_SOC_BIAS_PREPARE);
-		dapm_seq_run(dapm, &down_list, 0, false);
+		dapm_seq_run(&down_list, 0, false);
 		snd_soc_dapm_set_bias_level(dapm, SND_SOC_BIAS_STANDBY);
 	}
 }
-- 
1.7.2.5

[PATCH 4/7] ASoC: Pass snd_soc_card instead of snd_soc_dapm_context were appropriate

[Lars-Peter Clausen]

Since the DAPM widget list was moved from DAPM context to snd_soc_card a few
functions which take a snd_soc_dapm_context only use it to get a pointer to the
snd_soc_card. Change these functions to take a snd_soc_card directly.
This will allow to implement DAPM functions which operate on the whole card
instead of individual DAPM contexts.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/soc-dapm.c |   40 +++++++++++++++++++---------------------
 1 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 2801c6a..1c571bc 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -721,10 +721,9 @@ static void dapm_seq_insert(struct snd_soc_dapm_widget *new_widget,
 	list_add_tail(&new_widget->power_list, list);
 }
 
-static void dapm_seq_check_event(struct snd_soc_dapm_context *dapm,
+static void dapm_seq_check_event(struct snd_soc_card *card,
 				 struct snd_soc_dapm_widget *w, int event)
 {
-	struct snd_soc_card *card = dapm->card;
 	const char *ev_name;
 	int power, ret;
 
@@ -754,7 +753,7 @@ static void dapm_seq_check_event(struct snd_soc_dapm_context *dapm,
 		return;
 
 	if (w->event && (w->event_flags & event)) {
-		pop_dbg(dapm->dev, card->pop_time, "pop test : %s %s\n",
+		pop_dbg(card->dev, card->pop_time, "pop test : %s %s\n",
 			w->name, ev_name);
 		trace_snd_soc_dapm_widget_event_start(w, event);
 		ret = w->event(w, NULL, event);
@@ -797,8 +796,8 @@ static void dapm_seq_run_coalesced(struct snd_soc_dapm_context *dapm,
 			w->name, reg, value, mask);
 
 		/* Check for events */
-		dapm_seq_check_event(dapm, w, SND_SOC_DAPM_PRE_PMU);
-		dapm_seq_check_event(dapm, w, SND_SOC_DAPM_PRE_PMD);
+		dapm_seq_check_event(card, w, SND_SOC_DAPM_PRE_PMU);
+		dapm_seq_check_event(card, w, SND_SOC_DAPM_PRE_PMD);
 	}
 
 	if (reg >= 0) {
@@ -810,8 +809,8 @@ static void dapm_seq_run_coalesced(struct snd_soc_dapm_context *dapm,
 	}
 
 	list_for_each_entry(w, pending, power_list) {
-		dapm_seq_check_event(dapm, w, SND_SOC_DAPM_POST_PMU);
-		dapm_seq_check_event(dapm, w, SND_SOC_DAPM_POST_PMD);
+		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMU);
+		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMD);
 	}
 }
 
@@ -1014,10 +1013,9 @@ static void dapm_post_sequence_async(void *data, async_cookie_t cookie)
  *  o Input pin to Output pin (bypass, sidetone)
  *  o DAC to ADC (loopback).
  */
-static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event,
+static int dapm_power_widgets(struct snd_soc_card *card, int event,
 	struct snd_soc_dapm_update *update)
 {
-	struct snd_soc_card *card = dapm->card;
 	struct snd_soc_dapm_widget *w;
 	struct snd_soc_dapm_context *d;
 	LIST_HEAD(up_list);
@@ -1087,7 +1085,7 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event,
 		d->dev_power = power_card;
 
 	/* Run all the bias changes in parallel */
-	list_for_each_entry(d, &dapm->card->dapm_list, list)
+	list_for_each_entry(d, &card->dapm_list, list)
 		async_schedule_domain(dapm_pre_sequence_async, d,
 					&async_domain);
 	async_synchronize_full_domain(&async_domain);
@@ -1101,12 +1099,12 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event,
 	dapm_seq_run(&up_list, event, true);
 
 	/* Run all the bias changes in parallel */
-	list_for_each_entry(d, &dapm->card->dapm_list, list)
+	list_for_each_entry(d, &card->dapm_list, list)
 		async_schedule_domain(dapm_post_sequence_async, d,
 					&async_domain);
 	async_synchronize_full_domain(&async_domain);
 
-	pop_dbg(dapm->dev, card->pop_time,
+	pop_dbg(card->dev, card->pop_time,
 		"DAPM sequencing finished, waiting %dms\n", card->pop_time);
 	pop_wait(card->pop_time);
 
@@ -1270,7 +1268,7 @@ static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
 				 int mux, struct soc_enum *e,
 				 struct snd_soc_dapm_update *update)
 {
-	struct snd_soc_dapm_context *dapm = widget->dapm;
+	struct snd_soc_card *card = widget->dapm->card;
 	struct snd_soc_dapm_path *path;
 	int found = 0;
 
@@ -1283,7 +1281,7 @@ static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
 		return 0;
 
 	/* find dapm widget path assoc with kcontrol */
-	list_for_each_entry(path, &dapm->card->paths, list) {
+	list_for_each_entry(path, &card->paths, list) {
 		if (path->kcontrol != kcontrol)
 			continue;
 
@@ -1299,7 +1297,7 @@ static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
 	}
 
 	if (found)
-		dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, update);
+		dapm_power_widgets(card, SND_SOC_DAPM_STREAM_NOP, update);
 
 	return 0;
 }
@@ -1309,7 +1307,7 @@ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
 				   struct snd_kcontrol *kcontrol, int connect,
 				   struct snd_soc_dapm_update *update)
 {
-	struct snd_soc_dapm_context *dapm = widget->dapm;
+	struct snd_soc_card *card = widget->dapm->card;
 	struct snd_soc_dapm_path *path;
 	int found = 0;
 
@@ -1319,7 +1317,7 @@ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
 		return -ENODEV;
 
 	/* find dapm widget path assoc with kcontrol */
-	list_for_each_entry(path, &dapm->card->paths, list) {
+	list_for_each_entry(path, &card->paths, list) {
 		if (path->kcontrol != kcontrol)
 			continue;
 
@@ -1330,7 +1328,7 @@ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget,
 	}
 
 	if (found)
-		dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, update);
+		dapm_power_widgets(card, SND_SOC_DAPM_STREAM_NOP, update);
 
 	return 0;
 }
@@ -1488,7 +1486,7 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
  */
 int snd_soc_dapm_sync(struct snd_soc_dapm_context *dapm)
 {
-	return dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, NULL);
+	return dapm_power_widgets(dapm->card, SND_SOC_DAPM_STREAM_NOP, NULL);
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_sync);
 
@@ -1740,7 +1738,7 @@ int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
 		w->new = 1;
 	}
 
-	dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, NULL);
+	dapm_power_widgets(dapm->card, SND_SOC_DAPM_STREAM_NOP, NULL);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_new_widgets);
@@ -2231,7 +2229,7 @@ static void soc_dapm_stream_event(struct snd_soc_dapm_context *dapm,
 		}
 	}
 
-	dapm_power_widgets(dapm, event, NULL);
+	dapm_power_widgets(dapm->card, event, NULL);
 }
 
 /**
-- 
1.7.2.5

[PATCH 5/7] ASoC: Move DAPM debugfs directory creation to snd_soc_dapm_debugfs_init

[Lars-Peter Clausen]

Move the creation of the DAPM debugfs directory to snd_soc_dapm_debugfs_init
instead of having the same duplicated code in both codec and card DAPM setup.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 include/sound/soc-dapm.h |    3 ++-
 sound/soc/soc-core.c     |   18 ++----------------
 sound/soc/soc-dapm.c     |   10 ++++++++--
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index cd52ad0..9679c18 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -356,7 +356,8 @@ void snd_soc_dapm_shutdown(struct snd_soc_card *card);
 
 /* dapm sys fs - used by the core */
 int snd_soc_dapm_sys_add(struct device *dev);
-void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm);
+void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm,
+				struct dentry *parent);
 
 /* dapm audio pin control and status */
 int snd_soc_dapm_enable_pin(struct snd_soc_dapm_context *dapm,
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index a823654..aa21fbc 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -302,13 +302,7 @@ static void soc_init_codec_debugfs(struct snd_soc_codec *codec)
 		printk(KERN_WARNING
 		       "ASoC: Failed to create codec register debugfs file\n");
 
-	codec->dapm.debugfs_dapm = debugfs_create_dir("dapm",
-						 codec->debugfs_codec_root);
-	if (!codec->dapm.debugfs_dapm)
-		printk(KERN_WARNING
-		       "Failed to create DAPM debugfs directory\n");
-
-	snd_soc_dapm_debugfs_init(&codec->dapm);
+	snd_soc_dapm_debugfs_init(&codec->dapm, codec->debugfs_codec_root);
 }
 
 static void soc_cleanup_codec_debugfs(struct snd_soc_codec *codec)
@@ -1925,15 +1919,7 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card)
 		snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes,
 					card->num_dapm_routes);
 
-#ifdef CONFIG_DEBUG_FS
-	card->dapm.debugfs_dapm = debugfs_create_dir("dapm",
-						     card->debugfs_card_root);
-	if (!card->dapm.debugfs_dapm)
-		printk(KERN_WARNING
-		       "Failed to create card DAPM debugfs directory\n");
-
-	snd_soc_dapm_debugfs_init(&card->dapm);
-#endif
+	snd_soc_dapm_debugfs_init(&card->dapm, card->debugfs_card_root);
 
 	snprintf(card->snd_card->shortname, sizeof(card->snd_card->shortname),
 		 "%s",  card->name);
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 1c571bc..7c6298b 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1228,13 +1228,19 @@ static const struct file_operations dapm_bias_fops = {
 	.llseek = default_llseek,
 };
 
-void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm)
+void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm,
+	struct dentry *parent)
 {
 	struct snd_soc_dapm_widget *w;
 	struct dentry *d;
 
-	if (!dapm->debugfs_dapm)
+	dapm->debugfs_dapm = debugfs_create_dir("dapm", parent);
+
+	if (!dapm->debugfs_dapm) {
+		printk(KERN_WARNING
+		       "Failed to create DAPM debugfs directory\n");
 		return;
+	}
 
 	d = debugfs_create_file("bias_level", 0444,
 				dapm->debugfs_dapm, dapm,
-- 
1.7.2.5

[PATCH 6/7] ASoC: Move DAPM widget debugfs entry creation to snd_soc_dapm_new_widgets

[Lars-Peter Clausen]

Currently debugfs entries for a DAPM widgets are only added in
snd_soc_dapm_debugfs_init. If a widget is added later (for example in the
dai_link's probe callback) it will not show up in debugfs.
This patch moves the creation of the widget debugfs entry to
snd_soc_dapm_new_widgets where it will be added after the widget has been
properly instantiated.

As a side-effect this will also reduce the number of times the DAPM widget list
is iterated during a card's instantiation.

Since it is possible that snd_soc_dapm_new_widgets is invoked form the codecs or
cards probe callbacks, the creation of the debugfs dapm directory has to be
moved before these are called.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

---
Note that the card's dapm debugfs entry is not removed in the error path of
snd_soc_instantiate_card, but it hasn't been before either. I'll try to address
this along with some other leaks in snd_soc_instantiate_card in a follow up
patch.
---
 sound/soc/soc-core.c |    9 +++++----
 sound/soc/soc-dapm.c |   35 +++++++++++++++++++++++------------
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index aa21fbc..c30fa9d 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1493,6 +1493,8 @@ static int soc_probe_codec(struct snd_soc_card *card,
 	if (!try_module_get(codec->dev->driver->owner))
 		return -ENODEV;
 
+	soc_init_codec_debugfs(codec);
+
 	if (driver->probe) {
 		ret = driver->probe(codec);
 		if (ret < 0) {
@@ -1513,8 +1515,6 @@ static int soc_probe_codec(struct snd_soc_card *card,
 		snd_soc_dapm_add_routes(&codec->dapm, driver->dapm_routes,
 					driver->num_dapm_routes);
 
-	soc_init_codec_debugfs(codec);
-
 	/* mark codec as probed and add to card codec list */
 	codec->probed = 1;
 	list_add(&codec->card_list, &card->codec_dev_list);
@@ -1523,6 +1523,7 @@ static int soc_probe_codec(struct snd_soc_card *card,
 	return 0;
 
 err_probe:
+	soc_cleanup_codec_debugfs(codec);
 	module_put(codec->dev->driver->owner);
 
 	return ret;
@@ -1873,6 +1874,8 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card)
 	card->dapm.card = card;
 	list_add(&card->dapm.list, &card->dapm_list);
 
+	snd_soc_dapm_debugfs_init(&card->dapm, card->debugfs_card_root);
+
 #ifdef CONFIG_PM_SLEEP
 	/* deferred resume work */
 	INIT_WORK(&card->deferred_resume_work, soc_resume_deferred);
@@ -1919,8 +1922,6 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card)
 		snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes,
 					card->num_dapm_routes);
 
-	snd_soc_dapm_debugfs_init(&card->dapm, card->debugfs_card_root);
-
 	snprintf(card->snd_card->shortname, sizeof(card->snd_card->shortname),
 		 "%s",  card->name);
 	snprintf(card->snd_card->longname, sizeof(card->snd_card->longname),
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 7c6298b..e3c5d36 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1231,7 +1231,6 @@ static const struct file_operations dapm_bias_fops = {
 void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm,
 	struct dentry *parent)
 {
-	struct snd_soc_dapm_widget *w;
 	struct dentry *d;
 
 	dapm->debugfs_dapm = debugfs_create_dir("dapm", parent);
@@ -1248,24 +1247,34 @@ void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm,
 	if (!d)
 		dev_warn(dapm->dev,
 			 "ASoC: Failed to create bias level debugfs file\n");
+}
 
-	list_for_each_entry(w, &dapm->card->widgets, list) {
-		if (!w->name || w->dapm != dapm)
-			continue;
+static void dapm_debugfs_add_widget(struct snd_soc_dapm_widget *w)
+{
+	struct snd_soc_dapm_context *dapm = w->dapm;
+	struct dentry *d;
 
-		d = debugfs_create_file(w->name, 0444,
-					dapm->debugfs_dapm, w,
-					&dapm_widget_power_fops);
-		if (!d)
-			dev_warn(w->dapm->dev,
-				"ASoC: Failed to create %s debugfs file\n",
-				w->name);
-	}
+	if (!dapm->debugfs_dapm || !w->name)
+		return;
+
+	d = debugfs_create_file(w->name, 0444,
+				dapm->debugfs_dapm, w,
+				&dapm_widget_power_fops);
+	if (!d)
+		dev_warn(w->dapm->dev,
+			"ASoC: Failed to create %s debugfs file\n",
+			w->name);
 }
+
 #else
 void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm)
 {
 }
+
+static inline void dapm_debugfs_add_widget(struct snd_soc_dapm_widget *w)
+{
+}
+
 #endif
 
 /* test and update the power status of a mux widget */
@@ -1742,6 +1751,8 @@ int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
 		}
 
 		w->new = 1;
+
+		dapm_debugfs_add_widget(w);
 	}
 
 	dapm_power_widgets(dapm->card, SND_SOC_DAPM_STREAM_NOP, NULL);
-- 
1.7.2.5

[PATCH 7/7] ASoC: Instantiate all DAPM widgets at once

[Lars-Peter Clausen]

Currently after each codec or auxdev has been probed its DAPM widgets are
instantiated.

Since widgets are kept in a per card list and routes can interconnect between
widgets from different DAPM contexts on the same card it is possible that
snd_soc_dapm_new_widgets is run on an incomplete DAPM configuration which might
cause unnecessary register writes and/or cause audible problems.

This patch addresses the issue by instantiating all widgets from all DAPM contexts
after all components of the card have been initialized.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 include/sound/soc-dapm.h |   16 +++++++++++++++-
 sound/soc/soc-core.c     |    5 ++---
 sound/soc/soc-dapm.c     |   29 ++++++++++++++---------------
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 9679c18..5666c3e 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -344,7 +344,7 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm,
 	int num);
 
 /* dapm path setup */
-int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm);
+int snd_soc_dapm_card_new_widgets(struct snd_soc_card *card);
 void snd_soc_dapm_free(struct snd_soc_dapm_context *dapm);
 int snd_soc_dapm_add_routes(struct snd_soc_dapm_context *dapm,
 			    const struct snd_soc_dapm_route *route, int num);
@@ -515,4 +515,18 @@ struct snd_soc_dapm_context {
 #endif
 };
 
+/*
+ * snd_soc_dapm_new_widgets - add new dapm widgets
+ * @dapm: DAPM context
+ *
+ * Checks the DAPM context's card for any new dapm widgets and creates them if
+ * found.
+ *
+ * Returns 0 for success.
+ */
+static inline int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
+{
+	return snd_soc_dapm_card_new_widgets(dapm->card);
+}
+
 #endif
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index c30fa9d..ea38e0a 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1567,9 +1567,6 @@ static int soc_post_component_init(struct snd_soc_card *card,
 	}
 	codec->name_prefix = temp;
 
-	/* Make sure all DAPM widgets are instantiated */
-	snd_soc_dapm_new_widgets(&codec->dapm);
-
 	/* register the rtd device */
 	rtd->codec = codec;
 	rtd->dev.parent = card->dev;
@@ -1935,6 +1932,8 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card)
 			goto probe_aux_dev_err;
 		}
 	}
+	/* Make sure all DAPM widgets are instantiated */
+	snd_soc_dapm_card_new_widgets(card);
 
 	ret = snd_card_register(card->snd_card);
 	if (ret < 0) {
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index e3c5d36..46e396c 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -323,9 +323,9 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm,
 }
 
 /* create new dapm mixer control */
-static int dapm_new_mixer(struct snd_soc_dapm_context *dapm,
-	struct snd_soc_dapm_widget *w)
+static int dapm_new_mixer(struct snd_soc_dapm_widget *w)
 {
+	struct snd_soc_dapm_context *dapm = w->dapm;
 	int i, ret = 0;
 	size_t name_len, prefix_len;
 	struct snd_soc_dapm_path *path;
@@ -404,9 +404,9 @@ static int dapm_new_mixer(struct snd_soc_dapm_context *dapm,
 }
 
 /* create new dapm mux control */
-static int dapm_new_mux(struct snd_soc_dapm_context *dapm,
-	struct snd_soc_dapm_widget *w)
+static int dapm_new_mux(struct snd_soc_dapm_widget *w)
 {
+	struct snd_soc_dapm_context *dapm = w->dapm;
 	struct snd_soc_dapm_path *path = NULL;
 	struct snd_kcontrol *kcontrol;
 	struct snd_card *card = dapm->card->snd_card;
@@ -451,8 +451,7 @@ err:
 }
 
 /* create new dapm volume control */
-static int dapm_new_pga(struct snd_soc_dapm_context *dapm,
-	struct snd_soc_dapm_widget *w)
+static int dapm_new_pga(struct snd_soc_dapm_widget *w)
 {
 	if (w->num_kcontrols)
 		dev_err(w->dapm->dev,
@@ -1679,19 +1678,19 @@ int snd_soc_dapm_add_routes(struct snd_soc_dapm_context *dapm,
 EXPORT_SYMBOL_GPL(snd_soc_dapm_add_routes);
 
 /**
- * snd_soc_dapm_new_widgets - add new dapm widgets
- * @dapm: DAPM context
+ * snd_soc_dapm_card_new_widgets - add new dapm widgets
+ * @card: Card to check for new widgets
  *
- * Checks the codec for any new dapm widgets and creates them if found.
+ * Checks the card for any new dapm widgets and creates them if found.
  *
  * Returns 0 for success.
  */
-int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
+int snd_soc_dapm_card_new_widgets(struct snd_soc_card *card)
 {
 	struct snd_soc_dapm_widget *w;
 	unsigned int val;
 
-	list_for_each_entry(w, &dapm->card->widgets, list)
+	list_for_each_entry(w, &card->widgets, list)
 	{
 		if (w->new)
 			continue;
@@ -1701,13 +1700,13 @@ int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
 		case snd_soc_dapm_mixer:
 		case snd_soc_dapm_mixer_named_ctl:
 			w->power_check = dapm_generic_check_power;
-			dapm_new_mixer(dapm, w);
+			dapm_new_mixer(w);
 			break;
 		case snd_soc_dapm_mux:
 		case snd_soc_dapm_virt_mux:
 		case snd_soc_dapm_value_mux:
 			w->power_check = dapm_generic_check_power;
-			dapm_new_mux(dapm, w);
+			dapm_new_mux(w);
 			break;
 		case snd_soc_dapm_adc:
 		case snd_soc_dapm_aif_out:
@@ -1720,7 +1719,7 @@ int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
 		case snd_soc_dapm_pga:
 		case snd_soc_dapm_out_drv:
 			w->power_check = dapm_generic_check_power;
-			dapm_new_pga(dapm, w);
+			dapm_new_pga(w);
 			break;
 		case snd_soc_dapm_input:
 		case snd_soc_dapm_output:
@@ -1755,7 +1754,7 @@ int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
 		dapm_debugfs_add_widget(w);
 	}
 
-	dapm_power_widgets(dapm->card, SND_SOC_DAPM_STREAM_NOP, NULL);
+	dapm_power_widgets(card, SND_SOC_DAPM_STREAM_NOP, NULL);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_new_widgets);
-- 
1.7.2.5

Re: [PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.

[Mark Brown]

On Thu, Apr 28, 2011 at 06:46:07PM +0200, Lars-Peter Clausen wrote:

> This problem can arise if the card contains at least one widget-less DAPM
> context.

No, this is getting silly.  We need to just make DAPM mandatory for all
contexts - they get essentially no testing from people doing active
development and the special casing they require is just a constant
source of fragility.

For CODECs we can easily add some widgets for them based on the DAI, for
cards we should just shove a random widget in there with the name of the
card, it doesn't need to be wired up to anything.

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Mark Brown]

On Thu, Apr 28, 2011 at 06:46:08PM +0200, Lars-Peter Clausen wrote:
> Pass the snd_dapm_soc_update struct as parameter to dapm_widget_update instead
> of passing it indirectly as a field of the snd_soc_dapm_context struct.
> This should make it more obvious were the struct is coming from and make the
> code easier to comprehend.

If we're going to do something like this it would be better to make the
update a more complex object so we weren't passing the stream update
separately.  The update should be "what we're doing right now" which
includes the stream operation (even though the stream operation is only
used to keep the pre and post widgets limping along) and looking at the
code they mostly don't care about the streams at all, only tlv320dac33
is using them and I think it can live without.

TBH I'm not sure how much of a help I find this, it's nice to not have
to pass the operation we're doing about all the time especially if we
end up using more context information in the middle of the operation
(which I suspect we may end up doing).  You still have to work back
through the call chain to find where the data came from.

Re: [PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.

[Lars-Peter Clausen]

On 04/28/2011 09:15 PM, Mark Brown wrote:
> On Thu, Apr 28, 2011 at 06:46:07PM +0200, Lars-Peter Clausen wrote:
> 
>> This problem can arise if the card contains at least one widget-less DAPM
>> context.
> 
> No, this is getting silly.  We need to just make DAPM mandatory for all
> contexts - they get essentially no testing from people doing active
> development and the special casing they require is just a constant
> source of fragility.

I've thought about that. But I didn't wanted to break existing codec drivers.
As far as I can see there are at least 4 drivers in mainline code which do not
register any DAPM widgets, but provide set_bias_level callbacks:
wm8804.c, uda134x.c, stac9766.c, cq93vc.c

These would have to be updated.

> 
> For CODECs we can easily add some widgets for them based on the DAI, for
> cards we should just shove a random widget in there with the name of the
> card, it doesn't need to be wired up to anything.

For codecs we can use SND_SOC_DAPM_AIF_{IN,OUT} widgets.

I don't understand you comment regarding cards though. It does not make sense
to add a random widget which is not connected anywhere, since it either would
have no effect or keep the card power up.
The problem was not due to widget-less contexts per se, but due to the special
treatment they received regarding their power state.
As written in the commit message the power state was kept across multiple
dapm_power_widget calls while for normal context the power state was cleared at
the beginning of dapm_power_widget. So once a widget-less context was powered
it could not (easily) be powered down again, which as a result kept the whole
card powered.

If we drop that special casing, we can handle widget-less context just fine,
with the assumption that they don't need to be powered.

- Lars

Re: [PATCH 4/7] ASoC: Pass snd_soc_card instead of snd_soc_dapm_context were appropriate

[Mark Brown]

On Thu, Apr 28, 2011 at 06:46:10PM +0200, Lars-Peter Clausen wrote:

>  	if (w->event && (w->event_flags & event)) {
> -		pop_dbg(dapm->dev, card->pop_time, "pop test : %s %s\n",
> +		pop_dbg(card->dev, card->pop_time, "pop test : %s %s\n",

This isn't a good change - logging the widgets with the card device
rather than the device of the widget isn't going to clarify things.

> -	pop_dbg(dapm->dev, card->pop_time,
> +	pop_dbg(card->dev, card->pop_time,
>  		"DAPM sequencing finished, waiting %dms\n", card->pop_time);

It's fine here because this is the system-wide sequencing that's
terminated.

>  int snd_soc_dapm_sync(struct snd_soc_dapm_context *dapm)
>  {
> -	return dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, NULL);
> +	return dapm_power_widgets(dapm->card, SND_SOC_DAPM_STREAM_NOP, NULL);
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_dapm_sync);

This should really just operate on the card, syncing an isolated DAPM
context is meaningless.  I'll just go do that...

Otherwise these look good.

Re: [PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.

[Mark Brown]

On Thu, Apr 28, 2011 at 09:47:12PM +0200, Lars-Peter Clausen wrote:
> On 04/28/2011 09:15 PM, Mark Brown wrote:

> > For CODECs we can easily add some widgets for them based on the DAI, for
> > cards we should just shove a random widget in there with the name of the
> > card, it doesn't need to be wired up to anything.

> For codecs we can use SND_SOC_DAPM_AIF_{IN,OUT} widgets.

Quite, but they do also need to be connected to outputs and inputs so
that they're part of complete paths.

> I don't understand you comment regarding cards though. It does not make sense
> to add a random widget which is not connected anywhere, since it either would
> have no effect or keep the card power up.

It minimises the risk of some other unexpected behaviour being noticed,
hopefully it's not needed but equally well if it's not needed it won't
do any harm and if it is needed it's obviously helpful.

> If we drop that special casing, we can handle widget-less context just fine,
> with the assumption that they don't need to be powered.

That's what's *supposed* to happen, certainly.

Re: [PATCH 7/7] ASoC: Instantiate all DAPM widgets at once

[Mark Brown]

On Thu, Apr 28, 2011 at 06:46:13PM +0200, Lars-Peter Clausen wrote:
> Currently after each codec or auxdev has been probed its DAPM widgets are
> instantiated.
> 
> Since widgets are kept in a per card list and routes can interconnect between
> widgets from different DAPM contexts on the same card it is possible that
> snd_soc_dapm_new_widgets is run on an incomplete DAPM configuration which might
> cause unnecessary register writes and/or cause audible problems.
> 
> This patch addresses the issue by instantiating all widgets from all DAPM contexts
> after all components of the card have been initialized.

Except it doesn't because...

> +static inline int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
> +{
> +	return snd_soc_dapm_card_new_widgets(dapm->card);
> +}

...we can still instantiate widgets at any time, and lots of code will
do just that (and needs to do that because it needs the widgets to be
present in order to allow paths to be added).

There's also the fact that this mixes in the functional change to move
the instantiation of the widgets around with the argument change and a
rename, it'd be better to split the two of those out so we can clearly
see the functional change.

Re: [PATCH 4/7] ASoC: Pass snd_soc_card instead of snd_soc_dapm_context were appropriate

[Mark Brown]

On Thu, Apr 28, 2011 at 08:47:57PM +0100, Mark Brown wrote:
> On Thu, Apr 28, 2011 at 06:46:10PM +0200, Lars-Peter Clausen wrote:

> >  int snd_soc_dapm_sync(struct snd_soc_dapm_context *dapm)
> >  {
> > -	return dapm_power_widgets(dapm, SND_SOC_DAPM_STREAM_NOP, NULL);
> > +	return dapm_power_widgets(dapm->card, SND_SOC_DAPM_STREAM_NOP, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(snd_soc_dapm_sync);

> This should really just operate on the card, syncing an isolated DAPM
> context is meaningless.  I'll just go do that...

On second thoughts while that's obviously the case almost no context
that needs to call this function has a card handy so it's just a bit
annoying for it to take a card as a parameter.  Feh.

Re: [PATCH 7/7] ASoC: Instantiate all DAPM widgets at once

[Lars-Peter Clausen]

On 04/28/2011 10:07 PM, Mark Brown wrote:
> On Thu, Apr 28, 2011 at 06:46:13PM +0200, Lars-Peter Clausen wrote:
>> Currently after each codec or auxdev has been probed its DAPM widgets are
>> instantiated.
>>
>> Since widgets are kept in a per card list and routes can interconnect between
>> widgets from different DAPM contexts on the same card it is possible that
>> snd_soc_dapm_new_widgets is run on an incomplete DAPM configuration which might
>> cause unnecessary register writes and/or cause audible problems.
>>
>> This patch addresses the issue by instantiating all widgets from all DAPM contexts
>> after all components of the card have been initialized.
> 
> Except it doesn't because...

Well, a driver can of course decide that it wants to instantiate the widgets
now, but...
> 
>> +static inline int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm)
>> +{
>> +	return snd_soc_dapm_card_new_widgets(dapm->card);
>> +}
> 
> ...we can still instantiate widgets at any time, and lots of code will
> do just that (and needs to do that because it needs the widgets to be
> present in order to allow paths to be added).

... there are exactly 5 codec drivers which call snd_soc_dapm_new_widgets, all
of them call this at the end of their codec probe callback, which means it can
be dropped, because the core will call it again just right after the codecs
probe callback (without this patch).
snd_soc_dapm_new_widgets was mainly left in to let drivers register widgets
after the initialization phase which could be required for dynamic
reconfiguration of some DSPs.

Routes can be added just fine without the widgets being instantiated. In fact
snd_soc_dapm_new_widgets requires the routes to be setup to initialize some
widgets(mixers,muxs) properly.

> 
> There's also the fact that this mixes in the functional change to move
> the instantiation of the widgets around with the argument change and a
> rename, it'd be better to split the two of those out so we can clearly
> see the functional change.

ok.

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Lars-Peter Clausen]

On 04/28/2011 09:40 PM, Mark Brown wrote:
> On Thu, Apr 28, 2011 at 06:46:08PM +0200, Lars-Peter Clausen wrote:
>> Pass the snd_dapm_soc_update struct as parameter to dapm_widget_update instead
>> of passing it indirectly as a field of the snd_soc_dapm_context struct.
>> This should make it more obvious were the struct is coming from and make the
>> code easier to comprehend.
> 
> If we're going to do something like this it would be better to make the
> update a more complex object so we weren't passing the stream update
> separately.  The update should be "what we're doing right now" which
> includes the stream operation. [...]

Hm... the current update struct only contains information on register updates,
since we don't not always want to update a register when we sent a stream event
and vice versa it does make sense to keep them separate.
If we end up adding more context information later we might want to reconsider,
but currently keeping them separate is fine in my opinion.

> 
> TBH I'm not sure how much of a help I find this, it's nice to not have
> to pass the operation we're doing about all the time especially if we
> end up using more context information in the middle of the operation
> (which I suspect we may end up doing).  You still have to work back
> through the call chain to find where the data came from.

Well, but you know that it is a parameter to the function call. With the
current scheme you don't know this immediately. Right now dapm->update is some
random global variable and it is not quite clear that it is only valid for the
time dapm_power_widgets is called.

- Lars

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Mark Brown]

On Thu, Apr 28, 2011 at 10:39:08PM +0200, Lars-Peter Clausen wrote:
> On 04/28/2011 09:40 PM, Mark Brown wrote:

> > If we're going to do something like this it would be better to make the
> > update a more complex object so we weren't passing the stream update
> > separately.  The update should be "what we're doing right now" which
> > includes the stream operation. [...]

> Hm... the current update struct only contains information on register updates,
> since we don't not always want to update a register when we sent a stream event
> and vice versa it does make sense to keep them separate.

Sure, but we've already got a perfectly good way of representing no
register which we can use.  No big deal there.

> > TBH I'm not sure how much of a help I find this, it's nice to not have
> > to pass the operation we're doing about all the time especially if we
> > end up using more context information in the middle of the operation
> > (which I suspect we may end up doing).  You still have to work back
> > through the call chain to find where the data came from.

> Well, but you know that it is a parameter to the function call. With the
> current scheme you don't know this immediately. Right now dapm->update is some
> random global variable and it is not quite clear that it is only valid for the
> time dapm_power_widgets is called.

It should be valid at any time, it's cleared after use.  It only makes
sense when something is in the middle of doing an update.  Half of the
thing for me is that we can now grep for actual users without seeing
things that are just bouncing the data around.

Re: [PATCH 7/7] ASoC: Instantiate all DAPM widgets at once

[Mark Brown]

On Thu, Apr 28, 2011 at 10:25:05PM +0200, Lars-Peter Clausen wrote:
> On 04/28/2011 10:07 PM, Mark Brown wrote:

> > ...we can still instantiate widgets at any time, and lots of code will
> > do just that (and needs to do that because it needs the widgets to be
> > present in order to allow paths to be added).

> ... there are exactly 5 codec drivers which call snd_soc_dapm_new_widgets, all
> of them call this at the end of their codec probe callback, which means it can
> be dropped, because the core will call it again just right after the codecs
> probe callback (without this patch).

So it'd seem more sensible to do that, especially if that's what the
changelog says that the change does...  Reading the change the thing
that jumps out is that it's mostly a half done function rename rather
than the change it was described as being.

> Routes can be added just fine without the widgets being instantiated. In fact
> snd_soc_dapm_new_widgets requires the routes to be setup to initialize some
> widgets(mixers,muxs) properly.

Meh, right - wrong way round.

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Lars-Peter Clausen]

On 04/28/2011 10:58 PM, Mark Brown wrote:
> On Thu, Apr 28, 2011 at 10:39:08PM +0200, Lars-Peter Clausen wrote:
>> On 04/28/2011 09:40 PM, Mark Brown wrote:
>  [...]
> 
>>> TBH I'm not sure how much of a help I find this, it's nice to not have
>>> to pass the operation we're doing about all the time especially if we
>>> end up using more context information in the middle of the operation
>>> (which I suspect we may end up doing).  You still have to work back
>>> through the call chain to find where the data came from.
> 
>> Well, but you know that it is a parameter to the function call. With the
>> current scheme you don't know this immediately. Right now dapm->update is some
>> random global variable and it is not quite clear that it is only valid for the
>> time dapm_power_widgets is called.
> 
> It should be valid at any time, it's cleared after use. 

But you only know that when looking at all callers, which is my point.
Being part of the dapm struct which is available to all drivers doesn't make
this easier.

> It only makes sense when something is in the middle of doing an update. Half
> of the thing for me is that we can now grep for actual users without seeing
> things that are just bouncing the data around.

Hm. I won't argue to death here. I could only repeat what I've said before.
But if we keep passing it indirectly we should at least make the struct opaque
to the world outside of soc-dapm and move the update pointer from
snd_soc_dapm_context to snd_soc_card.

- Lars

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Mark Brown]

On Thu, Apr 28, 2011 at 11:24:38PM +0200, Lars-Peter Clausen wrote:

> Hm. I won't argue to death here. I could only repeat what I've said before.
> But if we keep passing it indirectly we should at least make the struct opaque
> to the world outside of soc-dapm and move the update pointer from
> snd_soc_dapm_context to snd_soc_card.

Moving to the card doesn't seem helpful, while we're currently doing one
update at once it's not immediately obvious that we want to keep doing
that indefinitely.  Like I've said before with the stream events it's
not clear that we should still be using them any more, IIRC there's only
one user which actually cares (tlv320dac33) and that would probably be
perfectly happy with using events on an AIF widget.

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Lars-Peter Clausen]

On 04/28/2011 11:48 PM, Mark Brown wrote:
> On Thu, Apr 28, 2011 at 11:24:38PM +0200, Lars-Peter Clausen wrote:
> 
>> Hm. I won't argue to death here. I could only repeat what I've said before.
>> But if we keep passing it indirectly we should at least make the struct opaque
>> to the world outside of soc-dapm and move the update pointer from
>> snd_soc_dapm_context to snd_soc_card.
> 
> Moving to the card doesn't seem helpful, while we're currently doing one
> update at once it's not immediately obvious that we want to keep doing
> that indefinitely.

But keeping it in the dapm context would limit us to one update per context
which doesn't seem very sustainable either.
Turning the pointer in the card struct into a list seems like an option though.

dapm_power_widgets works over the whole card, so the updates to be done should
in my opinion be kept on a per card basis as well.

- Lars

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Mark Brown]

On Fri, Apr 29, 2011 at 12:04:04AM +0200, Lars-Peter Clausen wrote:
> On 04/28/2011 11:48 PM, Mark Brown wrote:

> > Moving to the card doesn't seem helpful, while we're currently doing one
> > update at once it's not immediately obvious that we want to keep doing
> > that indefinitely.

> But keeping it in the dapm context would limit us to one update per context
> which doesn't seem very sustainable either.

The contexts are pretty much I/O limited - there's a limit to how much
you can do simultaneously on a context because at some point you have to
interact with the hardware and that's usually the only thing that takes
enough time to really care about.  Each context will usually go along
with a control interface and sometimes those control interfaces will be
able to run separately.

Though for global state changes like this that's pretty much irrelevant
as we need to walk the entire graph at once.

> Turning the pointer in the card struct into a list seems like an option though.

> dapm_power_widgets works over the whole card, so the updates to be done should
> in my opinion be kept on a per card basis as well.

They're going to be tied to a context anyway, simply as a result of
getting the I/O context for a register write - if they get moved to the
card they'll need to have a context added.

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Mark Brown]

On Thu, Apr 28, 2011 at 11:18:00PM +0100, Mark Brown wrote:
> On Fri, Apr 29, 2011 at 12:04:04AM +0200, Lars-Peter Clausen wrote:
> > On 04/28/2011 11:48 PM, Mark Brown wrote:

> > > Moving to the card doesn't seem helpful, while we're currently doing one
> > > update at once it's not immediately obvious that we want to keep doing
> > > that indefinitely.

> > But keeping it in the dapm context would limit us to one update per context
> > which doesn't seem very sustainable either.

> Though for global state changes like this that's pretty much irrelevant
> as we need to walk the entire graph at once.

BTW, this means that my above comment was too hasty; I can't actually
see a situation where we'd ever want to have multiple updates in flight.
Though I also don't immediately see a need to move as the update gets
applied to a specific context anyway and it seems nicer to just stop
using the stream events for anything other than the integration with the
CPU DMA.

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Lars-Peter Clausen]

On 04/29/2011 12:23 AM, Mark Brown wrote:
> On Thu, Apr 28, 2011 at 11:18:00PM +0100, Mark Brown wrote:
>> On Fri, Apr 29, 2011 at 12:04:04AM +0200, Lars-Peter Clausen wrote:
>>> On 04/28/2011 11:48 PM, Mark Brown wrote:
> 
>>>> Moving to the card doesn't seem helpful, while we're currently doing one
>>>> update at once it's not immediately obvious that we want to keep doing
>>>> that indefinitely.
> 
>>> But keeping it in the dapm context would limit us to one update per context
>>> which doesn't seem very sustainable either.
> 
>> Though for global state changes like this that's pretty much irrelevant
>> as we need to walk the entire graph at once.
> 
> BTW, this means that my above comment was too hasty; I can't actually
> see a situation where we'd ever want to have multiple updates in flight.
> Though I also don't immediately see a need to move as the update gets
> applied to a specific context anyway ...

If we'd keep the update in the dapm context we'd have to iterate over all
contexts to find the one context which contains the update.
As you've said we have to walk the whole graph and it does not make a nice
interface if the function takes one specific dapm context while it works on the
whole card.

Currently we get the codec from the widget for which the update is run and not
from the dapm context anyway.

- Lars

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Mark Brown]

On Fri, Apr 29, 2011 at 12:34:11AM +0200, Lars-Peter Clausen wrote:

> If we'd keep the update in the dapm context we'd have to iterate over all
> contexts to find the one context which contains the update.
> As you've said we have to walk the whole graph and it does not make a nice
> interface if the function takes one specific dapm context while it works on the
> whole card.

So that's part of a separate refactoring and only applies if we're not
running DAPM in a particular context and should be something along the
lines of "now we pass the card in it's much more sensible to..."; I'm
writing this while looking at current code.

> Currently we get the codec from the widget for which the update is run and not
> from the dapm context anyway.

That's just a redundancy in the data structure; the codec and context
are just two different ways of getting to the same thing.

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Lars-Peter Clausen]

On 04/29/2011 12:48 AM, Mark Brown wrote:
> On Fri, Apr 29, 2011 at 12:34:11AM +0200, Lars-Peter Clausen wrote:
>  ...
> 
>> Currently we get the codec from the widget for which the update is run and not
>> from the dapm context anyway.
> 
> That's just a redundancy in the data structure; the codec and context
> are just two different ways of getting to the same thing.

Yes, but the widget has a pointer to the dapm context as well. So we can get
the context from the widget and don't have to pass it separately. Or will there
be non-widget related updates, which ought to be done in the middle of the
powering sequence, in the foreseeable future?

- Lars

Re: [PATCH 2/7] ASoC: DAPM: Pass snd_dapm_soc_update as parameter

[Mark Brown]

On Fri, Apr 29, 2011 at 12:58:30AM +0200, Lars-Peter Clausen wrote:

> Yes, but the widget has a pointer to the dapm context as well. So we can get
> the context from the widget and don't have to pass it separately. Or will there
> be non-widget related updates, which ought to be done in the middle of the
> powering sequence, in the foreseeable future?

No.

Re: [PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.

[Lars-Peter Clausen]

On 04/28/2011 09:52 PM, Mark Brown wrote:
> On Thu, Apr 28, 2011 at 09:47:12PM +0200, Lars-Peter Clausen wrote:
>> On 04/28/2011 09:15 PM, Mark Brown wrote:
> 
>>> For CODECs we can easily add some widgets for them based on the DAI, for
>>> cards we should just shove a random widget in there with the name of the
>>> card, it doesn't need to be wired up to anything.
> 
>> For codecs we can use SND_SOC_DAPM_AIF_{IN,OUT} widgets.
> 
> Quite, but they do also need to be connected to outputs and inputs so
> that they're part of complete paths.

Hm... right.
I know this isn't optimal, but would it be accepable to have a
SND_SOC_DAPM_STREAM(sname) widget, where the power_check function would just
consider stream's state, so we can get rid of the special casing without having
to mess with the codec drivers to much?

- Lars

Re: [PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.

[Mark Brown]

On Fri, Apr 29, 2011 at 01:14:00AM +0200, Lars-Peter Clausen wrote:

> I know this isn't optimal, but would it be accepable to have a
> SND_SOC_DAPM_STREAM(sname) widget, where the power_check function would just
> consider stream's state, so we can get rid of the special casing without having
> to mess with the codec drivers to much?

I was expecting that the core would just autogenerate some widgets for
the streams if it saw a CODEC appearing with no widgets in its context
after probe.  No code change to the CODEC drivers at all.

In principle a stream widget is fine, but you'd need ones for capture
and playback paths and they'd need to also function as pins so they can
be joined up with the outside world (which now we have multi-component
is a big reason to push back on things that don't do DAPM; it's not
purely about the annoyance of having to bodge around them).

Re: [PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.

[Lars-Peter Clausen]

On 04/29/2011 01:17 AM, Mark Brown wrote:
> On Fri, Apr 29, 2011 at 01:14:00AM +0200, Lars-Peter Clausen wrote:
> 
>> I know this isn't optimal, but would it be accepable to have a
>> SND_SOC_DAPM_STREAM(sname) widget, where the power_check function would just
>> consider stream's state, so we can get rid of the special casing without having
>> to mess with the codec drivers to much?
> 
> I was expecting that the core would just autogenerate some widgets for
> the streams if it saw a CODEC appearing with no widgets in its context
> after probe.  No code change to the CODEC drivers at all.

Hm, I guess that would work as well. Though it would require that a codec
driver does not add DAPM widgets after it has been probed.

> 
> In principle a stream widget is fine, but you'd need ones for capture
> and playback paths and they'd need to also function as pins so they can
> be joined up with the outside world (which now we have multi-component
> is a big reason to push back on things that don't do DAPM; it's not
> purely about the annoyance of having to bodge around them).

My idea about the stream widget was, that it takes a stream name and is not
connected anywhere. If the stream is active, that widget ensures, that the
context gets powered, so it would basically mimic the current behaviour for
widget-less contexts.
If we wanted to use it to connect to the outside world, we'd probably be better
off using the already existing widgets. For example ADC, DAC widgets with
DAPM_NOPM.

- Lars