[RFC 0/2] ASoC: core: Option to reorder widget power sequence

[RFC 0/2] ASoC: core: Option to reorder widget power sequence

[Peter Ujfalusi]

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(-)

-- 
1.7.3.2

[RFC 1/2] ASoC: core: Reordered DAPM update power on widgets

[Peter Ujfalusi]

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;
-- 
1.7.3.2

Re: [RFC 1/2] ASoC: core: Reordered DAPM update power on widgets

[Mark Brown]

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.

Re: [RFC 1/2] ASoC: core: Reordered DAPM update power on widgets

[Peter Ujfalusi]

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

[RFC 2/2] ASoC: tlv320dac33: Request for reordered update power sequence

[Peter Ujfalusi]

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 */
-- 
1.7.3.2

Re: [RFC 2/2] ASoC: tlv320dac33: Request for reordered update power sequence

[Mark Brown]

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.

Re: [RFC 2/2] ASoC: tlv320dac33: Request for reordered update power sequence

[Peter Ujfalusi]

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.

-- 
Péter

Re: [RFC 2/2] ASoC: tlv320dac33: Request for reordered update power sequence

[Mark Brown]

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.

Re: [RFC 2/2] ASoC: tlv320dac33: Request for reordered update power sequence

[Peter Ujfalusi]

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?

-- 
Péter

Re: [RFC 0/2] ASoC: core: Option to reorder widget power sequence

[Mark Brown]

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.

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.

Re: [RFC 0/2] ASoC: core: Option to reorder widget power sequence

[Peter Ujfalusi]

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.

-- 
Péter

Re: [RFC 0/2] ASoC: core: Option to reorder widget power sequence

[Mark Brown]

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.