[alsa-devel] [RFC 0/2] ASoC: core: Option to reorder widget power sequence
Hello,
Quite some time ago I have faced similar issue with the sequence of the DAPM register writes [1] For that issue, I used a workaround within the twl4030 codec driver. Now I have again issue with the register write sequence.
The dac33 codec emmits a pop noise, when user enable the analog bypass path. The route cause in this codec are the DAPM update power and register write sequence: 1. User enable the bypass 2. DAPM power up sequence takes place 2.1 codec DAPM widgets got powered on, codec is enabled 2.2 External speaker enabled 3. The bit enabling the bypass got changed.
Step 3 cause a pop, which propagate to the speaker, since it is already enabled.
The first patch implements a new flag for the core to reorder the sequence to: 1. User enable the bypass 2. The bit enabling the bypass got changed. 3. DAPM power up sequence takes place 3.1 codec DAPM widgets got powered on, codec is enabled 3.2 External speaker enabled
The default sequence is kept, codec driver must ask to have the reordered sequence to avoid any side effect on other codecs.
[1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-August/030024.html
Peter
--- Peter Ujfalusi (2): ASoC: core: Reordered DAPM update power on widgets ASoC: tlv320dac33: Request for reordered update power sequence
include/sound/soc-dapm.h | 1 + sound/soc/codecs/tlv320dac33.c | 1 + sound/soc/soc-dapm.c | 27 +++++++++++++++++++-------- 3 files changed, 21 insertions(+), 8 deletions(-)
Add option to request reorder update power on codecs. Certain codecs can generate pop noise, when for example enabling the bypass path. The route cause in these codecs are the DAPM update power and register write sequence: 1. User enable the bypass 2. DAPM power up sequence takes place 2.1 codec DAPM widgets got powered on, codec is enabled 2.2 External speaker enabled 3. The bit enabling the bypass got changed.
Step 3 can cause audible pop on the speaker.
By introducing new flag for the DAPM (dapm_reorder_pupdate), codecs can request for reordered power up sequence, which will look like this:
1. User enable the bypass 2. The bit enabling the bypass got changed. 3. DAPM power up sequence takes place 3.1 codec DAPM widgets got powered on, codec is enabled 3.2 External speaker enabled
The pop noise going to be filtered out, since the speaker going to be enabled after the bit change taken place.
The default (current) sequence is not changed, since codec drivers must request for reordered sequence.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- include/sound/soc-dapm.h | 1 + sound/soc/soc-dapm.c | 27 +++++++++++++++++++-------- 2 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 041e98b..38f8aad 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -474,6 +474,7 @@ struct snd_soc_dapm_context { enum snd_soc_bias_level suspend_bias_level; struct delayed_work delayed_work; unsigned int idle_bias_off:1; /* Use BIAS_OFF instead of STANDBY */ + unsigned int dapm_reorder_pupdate:1; /* Reordered pupdate in widgets */
struct device *dev; /* from parent - for debug */ struct snd_soc_codec *codec; /* parent codec */ diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 6a29d59..432ecf5 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1626,6 +1626,7 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, unsigned int mask = (1 << fls(max)) - 1; unsigned int invert = mc->invert; unsigned int val, val2, val_mask; + unsigned int change, dapm_pupdate_first = 1; int connect; int ret;
@@ -1646,16 +1647,21 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, mutex_lock(&widget->codec->mutex); widget->value = val;
- if (snd_soc_test_bits(widget->codec, reg, val_mask, val)) { - if (val) - /* new connection */ - connect = invert ? 0:1; - else - /* old connection must be powered down */ - connect = invert ? 1:0; + change = snd_soc_test_bits(widget->codec, reg, val_mask, val); + if (val) + /* new connection */ + connect = invert ? 0 : 1; + else + /* old connection must be powered down */ + connect = invert ? 1 : 0;
- dapm_mixer_update_power(widget, kcontrol, connect); + if (widget->dapm->dapm_reorder_pupdate) { + /* reodered DAPM change requested */ + if (connect) + dapm_pupdate_first = 0; } + if (change && dapm_pupdate_first) + dapm_mixer_update_power(widget, kcontrol, connect);
if (widget->event) { if (widget->event_flags & SND_SOC_DAPM_PRE_REG) { @@ -1673,6 +1679,11 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, } else ret = snd_soc_update_bits(widget->codec, reg, val_mask, val);
+ if (ret < 0) + goto out; + + if (change && !dapm_pupdate_first) + dapm_mixer_update_power(widget, kcontrol, connect); out: mutex_unlock(&widget->codec->mutex); return ret;
On Thu, Dec 02, 2010 at 02:03:09PM +0200, Peter Ujfalusi wrote:
- unsigned int dapm_reorder_pupdate:1; /* Reordered pupdate in widgets */
Please use a more meaningful name than pupdate, I can't parse what that stands for easily (I'm guessing power update?) and even with that it's not clear what the option actually does.
- unsigned int change, dapm_pupdate_first = 1;
Please split the new variable onto a separate line - mixed initialised and uninitialised variables are confusing to read.
- if (snd_soc_test_bits(widget->codec, reg, val_mask, val)) {
if (val)/* new connection */connect = invert ? 0:1;else/* old connection must be powered down */connect = invert ? 1:0;
- change = snd_soc_test_bits(widget->codec, reg, val_mask, val);
- if (val)
/* new connection */connect = invert ? 0 : 1;- else
/* old connection must be powered down */
This is really confusing - if there's no change why are we not just exiting the function immediately? It makes the rest of the code much harder to follow as the conditionals all get more complex.
On Thursday 02 December 2010 14:21:28 ext Mark Brown wrote:
On Thu, Dec 02, 2010 at 02:03:09PM +0200, Peter Ujfalusi wrote:
- unsigned int dapm_reorder_pupdate:1; /* Reordered pupdate in widgets */
Please use a more meaningful name than pupdate, I can't parse what that stands for easily (I'm guessing power update?) and even with that it's not clear what the option actually does.
I'll try to figure out better name.
- unsigned int change, dapm_pupdate_first = 1;
Please split the new variable onto a separate line - mixed initialised and uninitialised variables are confusing to read.
True.
- if (snd_soc_test_bits(widget->codec, reg, val_mask, val)) {
if (val)/* new connection */connect = invert ? 0:1;else/* old connection must be powered down */connect = invert ? 1:0;
- change = snd_soc_test_bits(widget->codec, reg, val_mask, val);
- if (val)
/* new connection */connect = invert ? 0 : 1;- else
/* old connection must be powered down */This is really confusing - if there's no change why are we not just exiting the function immediately? It makes the rest of the code much harder to follow as the conditionals all get more complex.
Yes, in that case we do not even need the change variable, since if there is no change I just: goto out; Makes it cleaner.
Thanks, Péter
DAC33 codec emmits pop noise, when the bypass path enable bit get changed. By using the reordered sequence this pop noise can be filtered out.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tlv320dac33.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c index ccb267f..31f7a22 100644 --- a/sound/soc/codecs/tlv320dac33.c +++ b/sound/soc/codecs/tlv320dac33.c @@ -1416,6 +1416,7 @@ static int dac33_soc_probe(struct snd_soc_codec *codec) codec->control_data = dac33->control_data; codec->hw_write = (hw_write_t) i2c_master_send; codec->dapm.idle_bias_off = 1; + codec->dapm.dapm_reorder_pupdate = 1; dac33->codec = codec;
/* Read the tlv320dac33 ID registers */
On Thu, Dec 02, 2010 at 02:03:10PM +0200, Peter Ujfalusi wrote:
- codec->dapm.dapm_reorder_pupdate = 1;
Sorry, meant to say on the previous patch: it's also not ideal that this affects all widgets on the CODEC, it means that this is an all or nothing change which isn't ideal.
On Thursday 02 December 2010 14:24:23 ext Mark Brown wrote:
On Thu, Dec 02, 2010 at 02:03:10PM +0200, Peter Ujfalusi wrote:
- codec->dapm.dapm_reorder_pupdate = 1;
I'm really bad at naming for sure.
Sorry, meant to say on the previous patch: it's also not ideal that this affects all widgets on the CODEC, it means that this is an all or nothing change which isn't ideal.
That was my intention to have the ability set this reordered mode for the codec.
Back to the naming.. What about reorder_dapm_update_power, and move the flag to the snd_soc_dapm_widget struct?
I could think two ways of moving this per widget config: Either add new widgets: SND_SOC_DAPM_REORDERED_SWITCH, and SND_SOC_DAPM_REORDERED_SWITCH_E, which will have the same list of parameters, but sets the reorder_dapm_update_power flag for the widget. But the widget names looks terible.
Or have a helper function, which can set this flag for the given widget, something like: snd_soc_dapm_set_update_power_reordering(struct snd_soc_codec *codec, const char *name); It will find the widget, and sets the flag for that widget only. The function name again looks bad.
On Thu, Dec 02, 2010 at 02:52:04PM +0200, Peter Ujfalusi wrote:
Back to the naming.. What about reorder_dapm_update_power, and move the flag to the snd_soc_dapm_widget struct?
If we're going down this route I'd rather have a name that reflected the new ordering rather than the fact that there's an ordering change. Not that any ideas occur to me right now.
On Thursday 02 December 2010 15:15:06 ext Mark Brown wrote:
On Thu, Dec 02, 2010 at 02:52:04PM +0200, Peter Ujfalusi wrote:
Back to the naming.. What about reorder_dapm_update_power, and move the flag to the snd_soc_dapm_widget struct?
If we're going down this route I'd rather have a name that reflected the new ordering rather than the fact that there's an ordering change. Not that any ideas occur to me right now.
How about: route_change_before_powerup
A bit long, but this describes well the meaning.
I think it is kind of silly to have a function, which just sets this flag, and I think having yet another DAPM widgets for this type is again a bad idea. Do we have helper function, which can find a widget by name, and return the pointer to the snd_soc_dapm_widget? I think if we have that, drivers can use that to get the widget they want to change the power up order, and set this flag. If we ever need other new flags for widgets, this can be used for that purpose. We could have an inline function, which uses the dapm_find_widget(), and sets the flag, to minimize duplicated code all over the place.
So I can move the flag per widget, and we are going to have pretty interface too.
What do you think?
On Thu, Dec 02, 2010 at 02:03:08PM +0200, Peter Ujfalusi wrote:
The first patch implements a new flag for the core to reorder the sequence to:
- User enable the bypass
- The bit enabling the bypass got changed.
- DAPM power up sequence takes place
3.1 codec DAPM widgets got powered on, codec is enabled 3.2 External speaker enabled
Is this OK on power down? On power down we'll adjust the input path to the widget then power down the output path so noise introduced by switching away from the current source will get amplified. Keeping the current sequence for power down seems more consistent with what you're trying to do.
Looking at the mailing list archive the issues previously were partly things being obscured by this being in terms of the then very non-standard TWL4030 code and also this:
| I think there's something useful we can do with DAPM here (independantly | of anything in TWL4030), probably involving splitting up the routing | change walk to run power down first and then power up but the current | patch feels like it's going to cause at least as many problems as it | solves.
We've now got the split power down/power up sequences - ideally what we'd be able to do here is get the route update to happen in between the power down and power up sequences.
On Thursday 02 December 2010 15:05:16 ext Mark Brown wrote:
On Thu, Dec 02, 2010 at 02:03:08PM +0200, Peter Ujfalusi wrote:
The first patch implements a new flag for the core to reorder the sequence to: 1. User enable the bypass 2. The bit enabling the bypass got changed. 3. DAPM power up sequence takes place 3.1 codec DAPM widgets got powered on, codec is enabled 3.2 External speaker enabled
Is this OK on power down? On power down we'll adjust the input path to the widget then power down the output path so noise introduced by switching away from the current source will get amplified. Keeping the current sequence for power down seems more consistent with what you're trying to do.
The power down sequence is kept as it was, only the power up sequence has been changed: 1. User disable the bypass 2. DAPM power down sequence takes place 2.1 External speaker disabled 2.1 codec DAPM widgets got powered down, codec is powered down 3. The bit for the bypass got changed.
Looking at the mailing list archive the issues previously were partly things being obscured by this being in terms of the then very
Yes, I know that the old patch changed it for up, and down. I did learnet my lesson, so I only changed the power up sequence.
non-standard TWL4030 code and also this: | I think there's something useful we can do with DAPM here (independantly | of anything in TWL4030), probably involving splitting up the routing | change walk to run power down first and then power up but the current | patch feels like it's going to cause at least as many problems as it | solves.
We've now got the split power down/power up sequences - ideally what we'd be able to do here is get the route update to happen in between the power down and power up sequences.
What do you mean? IMHO using differnet sequence of register write/dapm update power on path enable and disable shall be enough.
Also: this patch only solving the pop noise in my case, when the codec is powered on, because the bypass got enabled/disabled. If the bypass is enabled during active audio (aplay /dev/zero), than the pop from the bypass switch is there, but honestly I can not do anything about it. It is a hardware bug, and user space has to deal with it in some way.
On Fri, Dec 03, 2010 at 09:33:28AM +0200, Peter Ujfalusi wrote:
On Thursday 02 December 2010 15:05:16 ext Mark Brown wrote:
We've now got the split power down/power up sequences - ideally what we'd be able to do here is get the route update to happen in between the power down and power up sequences.
What do you mean?
When we do DAPM we run one sequence of register updates to implement power downs and then a second sequence to implement the power ups. What you're really asking for here is to have the path update happen between these two. I'm not sure if that is any clearer? This seems like a totally sensible thing for us to be doing in general.
IMHO using differnet sequence of register write/dapm update power on path enable and disable shall be enough.
You're working at the widget level but really this sounds like a global issue in the sequencing we do - it's fixing one specific use case but the level it's doing it at makes the change much less generally useful.
participants (2)
-
Mark Brown -
Peter Ujfalusi