[alsa-devel] [PATCH 1/7] ASoC: Fix cards getting stuck in a powered state.
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)
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); }
/**
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.
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
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.
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
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.
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
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.
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.
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
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.
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
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.
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); } }
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); }
/**
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.
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.
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,
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);
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);
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.
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.
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.
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.
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
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.
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
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).
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
participants (2)
-
Lars-Peter Clausen -
Mark Brown