[alsa-devel] [PATCH 1/3] ASoC: Explicitly say if we're powering up or down
Rather than passing the sequence to use for DAPM widgets around by reference explicitly say if we're powering up or down until the point where we need the sequence itself. This should make no practical difference in itself but supports future refactoring.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/soc-dapm.c | 37 +++++++++++++++++++++++++------------ 1 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 499730a..57e1c9f 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -726,8 +726,15 @@ static int dapm_supply_check_power(struct snd_soc_dapm_widget *w)
static int dapm_seq_compare(struct snd_soc_dapm_widget *a, struct snd_soc_dapm_widget *b, - int sort[]) + bool power_up) { + int *sort; + + if (power_up) + sort = dapm_up_seq; + else + sort = dapm_down_seq; + if (sort[a->id] != sort[b->id]) return sort[a->id] - sort[b->id]; if (a->reg != b->reg) @@ -741,12 +748,12 @@ static int dapm_seq_compare(struct snd_soc_dapm_widget *a, /* Insert a widget in order into a DAPM power sequence. */ static void dapm_seq_insert(struct snd_soc_dapm_widget *new_widget, struct list_head *list, - int sort[]) + bool power_up) { struct snd_soc_dapm_widget *w;
list_for_each_entry(w, list, power_list) - if (dapm_seq_compare(new_widget, w, sort) < 0) { + if (dapm_seq_compare(new_widget, w, power_up) < 0) { list_add_tail(&new_widget->power_list, &w->power_list); return; } @@ -857,7 +864,7 @@ static void dapm_seq_run_coalesced(struct snd_soc_dapm_context *dapm, * handled. */ static void dapm_seq_run(struct snd_soc_dapm_context *dapm, - struct list_head *list, int event, int sort[]) + struct list_head *list, int event, bool power_up) { struct snd_soc_dapm_widget *w, *n; LIST_HEAD(pending); @@ -865,6 +872,12 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm, int cur_reg = SND_SOC_NOPM; struct snd_soc_dapm_context *cur_dapm = NULL; int ret; + int *sort; + + if (power_up) + sort = dapm_up_seq; + else + sort = dapm_down_seq;
list_for_each_entry_safe(w, n, list, power_list) { ret = 0; @@ -1002,10 +1015,10 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event) list_for_each_entry(w, &card->widgets, list) { switch (w->id) { case snd_soc_dapm_pre: - dapm_seq_insert(w, &down_list, dapm_down_seq); + dapm_seq_insert(w, &down_list, false); break; case snd_soc_dapm_post: - dapm_seq_insert(w, &up_list, dapm_up_seq); + dapm_seq_insert(w, &up_list, true); break;
default: @@ -1025,9 +1038,9 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event) trace_snd_soc_dapm_widget_power(w, power);
if (power) - dapm_seq_insert(w, &up_list, dapm_up_seq); + dapm_seq_insert(w, &up_list, true); else - dapm_seq_insert(w, &down_list, dapm_down_seq); + dapm_seq_insert(w, &down_list, false);
w->power = power; break; @@ -1086,12 +1099,12 @@ 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, dapm_down_seq); + dapm_seq_run(dapm, &down_list, event, false);
dapm_widget_update(dapm);
/* Now power up. */ - dapm_seq_run(dapm, &up_list, event, dapm_up_seq); + dapm_seq_run(dapm, &up_list, event, true);
list_for_each_entry(d, &dapm->card->dapm_list, list) { /* If we just powered the last thing off drop to standby bias */ @@ -2372,7 +2385,7 @@ static void soc_dapm_shutdown_codec(struct snd_soc_dapm_context *dapm) if (w->dapm != dapm) continue; if (w->power) { - dapm_seq_insert(w, &down_list, dapm_down_seq); + dapm_seq_insert(w, &down_list, false); w->power = 0; powerdown = 1; } @@ -2383,7 +2396,7 @@ static void soc_dapm_shutdown_codec(struct snd_soc_dapm_context *dapm) */ if (powerdown) { snd_soc_dapm_set_bias_level(NULL, dapm, SND_SOC_BIAS_PREPARE); - dapm_seq_run(dapm, &down_list, 0, dapm_down_seq); + dapm_seq_run(dapm, &down_list, 0, false); snd_soc_dapm_set_bias_level(NULL, dapm, SND_SOC_BIAS_STANDBY); } }
With larger devices there may be many widgets of the same type in series in an audio path. Allow drivers to specify an additional level of ordering within each widget type by adding a subsequence number to widgets and then splitting operations on widgets so that widgets of the same type but different sequence numbers are processed separately. A typical example would be a supply widget which requires that another widget be enabled to provide power or clocking.
SND_SOC_DAPM_PGA_S() and SND_SOC_DAPM_SUPPLY_S() macros are provided allowing this to be used with PGAs and supplies as these are the most commonly affected widgets.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- include/sound/soc-dapm.h | 13 +++++++++++++ sound/soc/soc-dapm.c | 11 ++++++++++- 2 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 8031769..a3760c9 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -157,6 +157,18 @@ .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = 1, \ .event = wevent, .event_flags = wflags}
+/* additional sequencing control within an event type */ +#define SND_SOC_DAPM_PGA_S(wname, wsubseq, wreg, wshift, winvert, wcontrols, \ + wncontrols, wevent, wflags) \ +{ .id = snd_soc_dapm_pga, .name = wname, .reg = wreg, .shift = wshift, \ + .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols, \ + .event = wevent, .event_flags = wflags, .subseq = wsubseq} +#define SND_SOC_DAPM_SUPPLY_S(wname, wsubseq, wreg, wshift, winvert, wevent, \ + wflags) \ +{ .id = snd_soc_dapm_supply, .name = wname, .reg = wreg, \ + .shift = wshift, .invert = winvert, .event = wevent, \ + .event_flags = wflags, .subseq = wsubseq} + /* Simplified versions of above macros, assuming wncontrols = ARRAY_SIZE(wcontrols) */ #define SOC_PGA_E_ARRAY(wname, wreg, wshift, winvert, wcontrols, \ wevent, wflags) \ @@ -450,6 +462,7 @@ struct snd_soc_dapm_widget { unsigned char ext:1; /* has external widgets */ unsigned char force:1; /* force state */ unsigned char ignore_suspend:1; /* kept enabled over suspend */ + int subseq; /* sort within widget type */
int (*power_check)(struct snd_soc_dapm_widget *w);
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 57e1c9f..eb7436c 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -737,6 +737,12 @@ static int dapm_seq_compare(struct snd_soc_dapm_widget *a,
if (sort[a->id] != sort[b->id]) return sort[a->id] - sort[b->id]; + if (a->subseq != b->subseq) { + if (power_up) + return a->subseq - b->subseq; + else + return b->subseq - a->subseq; + } if (a->reg != b->reg) return a->reg - b->reg; if (a->dapm != b->dapm) @@ -869,6 +875,7 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm, struct snd_soc_dapm_widget *w, *n; LIST_HEAD(pending); int cur_sort = -1; + int cur_subseq = -1; int cur_reg = SND_SOC_NOPM; struct snd_soc_dapm_context *cur_dapm = NULL; int ret; @@ -884,12 +891,13 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm,
/* Do we need to apply any queued changes? */ if (sort[w->id] != cur_sort || w->reg != cur_reg || - w->dapm != cur_dapm) { + w->dapm != cur_dapm || w->subseq != cur_subseq) { if (!list_empty(&pending)) dapm_seq_run_coalesced(cur_dapm, &pending);
INIT_LIST_HEAD(&pending); cur_sort = -1; + cur_subseq = -1; cur_reg = SND_SOC_NOPM; cur_dapm = NULL; } @@ -934,6 +942,7 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm, default: /* Queue it up for application */ cur_sort = sort[w->id]; + cur_subseq = w->subseq; cur_reg = w->reg; cur_dapm = w->dapm; list_move(&w->power_list, &pending);
On 01/18/11 18:20, ext Mark Brown wrote:
With larger devices there may be many widgets of the same type in series in an audio path. Allow drivers to specify an additional level of ordering within each widget type by adding a subsequence number to widgets and then splitting operations on widgets so that widgets of the same type but different sequence numbers are processed separately. A typical example would be a supply widget which requires that another widget be enabled to provide power or clocking.
SND_SOC_DAPM_PGA_S() and SND_SOC_DAPM_SUPPLY_S() macros are provided allowing this to be used with PGAs and supplies as these are the most commonly affected widgets.
Not really for this series... I have been also wondering on the power up/down sequence ordering in a complex setup. I might be totally wrong, but never the less: I have been wondering, if we just go, and follow the sequence of the DAPM widgets in on the path on powerup, and follow the reverse sequence on powerdown.
Take something like this: DAC->PGA1->MIXER1->PGA2->MUX->PGA3->MIXER2->OUTPUT->SPEAKER ^ ^ ^ SUPPLY1 SUPPLY2 SUPPLY3
The sequence now: SUPPLY1->SUPPLY2->SUPPLY3->MUX->DAC->MIXER1->MIXER2->PGA1->PGA2->PGA3->SPEAKER
Now we are going to be able to change the order of PGA, and SUPPLY. We were able to do that earlier by changing the order of the PGA/SUPPLY within the snd_soc_dapm_widget struct (I know... it is not really explicit).
But if we actually follow the route we could have sequence of: SUPPLY1->DAC->SUPPLY2->PGA1->MIXER1->PGA2->MUX->SUPPLY3->PGA3->MIXER2->SPEAKER
When we build up the DAPM tree, we usually follow the spec, basically modeling the HW, so we can take care of the recommended sequences.
IMHO in this way we would have better control over the sequences, and we might be in better position to combat pop noises, since we have more explicit control over the sequence of events.
I have not looked how hard it would be to change the DAPM to do this. Anyways I would not started to do this without asking... I know there are really good reasons to have the current DAPM sequence handling, but I do wonder, if it make sense to do this.
What do you think?
On Wed, Jan 19, 2011 at 10:19:38AM +0200, Peter Ujfalusi wrote:
I have been wondering, if we just go, and follow the sequence of the DAPM widgets in on the path on powerup, and follow the reverse sequence on powerdown.
It's certainly worth considering. It's not unambiguously clear that this is ideal - there's a few cases where you pretty much always want to do things in a different order to the straight path, the primary one being that since PGAs make everything more noisy you generally want to power them on last. There's also the issue that you don't want to oversequence things, more often than not bringing things up in parallel performs at least as well or even better.
IMHO in this way we would have better control over the sequences, and we might be in better position to combat pop noises, since we have more explicit control over the sequence of events.
OOI, do we actually have issues here at the minute? The only issue I'm aware of is that we don't have a facility for keeping PGAs muted while sequences are running.
I have not looked how hard it would be to change the DAPM to do this. Anyways I would not started to do this without asking... I know there are really good reasons to have the current DAPM sequence handling, but I do wonder, if it make sense to do this.
It's potentially useful. There are some considerations as above that mean we don't want a straight graph walk but there's certainly some room for using the infrastructure.
Many modern devices have features such as DC servos which take time to start. Currently these are handled by per-widget events but this makes it difficult to paralleise operations on multiple widgets, meaning delays can end up being needlessly serialised. By providing a callback to drivers when all widgets of a given type have been handled during a DAPM sequence the core allows drivers to start operations separately and wait for them to complete much more simply.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- include/sound/soc-dapm.h | 3 +++ include/sound/soc.h | 3 +++ sound/soc/soc-core.c | 1 + sound/soc/soc-dapm.c | 16 +++++++++++++++- 4 files changed, 22 insertions(+), 1 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index a3760c9..6c9ae23 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -500,6 +500,9 @@ struct snd_soc_dapm_context {
struct snd_soc_dapm_update *update;
+ void (*seq_notifier)(struct snd_soc_dapm_context *, + enum snd_soc_dapm_type); + struct device *dev; /* from parent - for debug */ struct snd_soc_codec *codec; /* parent codec */ struct snd_soc_card *card; /* parent card */ diff --git a/include/sound/soc.h b/include/sound/soc.h index d824e70..c0d2f4b 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -545,6 +545,9 @@ struct snd_soc_codec_driver { /* codec bias level */ int (*set_bias_level)(struct snd_soc_codec *, enum snd_soc_bias_level level); + + void (*seq_notifier)(struct snd_soc_dapm_context *, + enum snd_soc_dapm_type); };
/* SoC platform interface */ diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 8305712..08d7b98 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3496,6 +3496,7 @@ int snd_soc_register_codec(struct device *dev, codec->dapm.bias_level = SND_SOC_BIAS_OFF; codec->dapm.dev = dev; codec->dapm.codec = codec; + codec->dapm.seq_notifier = codec_drv->seq_notifier; codec->dev = dev; codec->driver = codec_drv; codec->num_dai = num_dai; diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index eb7436c..37b376f 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -878,7 +878,7 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm, int cur_subseq = -1; int cur_reg = SND_SOC_NOPM; struct snd_soc_dapm_context *cur_dapm = NULL; - int ret; + int ret, i; int *sort;
if (power_up) @@ -895,6 +895,13 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm, if (!list_empty(&pending)) dapm_seq_run_coalesced(cur_dapm, &pending);
+ if (cur_dapm && cur_dapm->seq_notifier) { + for (i = 0; i < ARRAY_SIZE(dapm_up_seq); i++) + if (sort[i] == cur_sort) + cur_dapm->seq_notifier(cur_dapm, + i); + } + INIT_LIST_HEAD(&pending); cur_sort = -1; cur_subseq = -1; @@ -956,6 +963,13 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm,
if (!list_empty(&pending)) dapm_seq_run_coalesced(dapm, &pending); + + if (cur_dapm && cur_dapm->seq_notifier) { + for (i = 0; i < ARRAY_SIZE(dapm_up_seq); i++) + if (sort[i] == cur_sort) + cur_dapm->seq_notifier(cur_dapm, + i); + } }
static void dapm_widget_update(struct snd_soc_dapm_context *dapm)
participants (2)
-
Mark Brown -
Peter Ujfalusi