[RFC v2 0/3] ASoC: Option to reorder widget power sequence

[RFC v2 0/3] ASoC: Option to reorder widget power sequence

[Peter Ujfalusi]

Hello,

Chasnges sicne v1:
- Flag moved to per widget
- Flag name changed to: route_change_before_powerup
- Comments from Mark has been addressed in the soc-dapm.c file
- New helper function to set the reorder power up sequence.
- tlv320dac33 to be the first codec to use the reversed power up order

Note: power down sequence remains the same for the reordered widgets as well.

Intro from the v1:

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 (3):
  ASoC: core: DAPM widget flag for reversed power up sequence
  ASoC: core: Helper function to request reversed widget power up
  ASoC: tlv320dac33: Reverse the power up sequence for bypass switches

 include/sound/soc-dapm.h       |    3 ++
 sound/soc/codecs/tlv320dac33.c |    4 +++
 sound/soc/soc-dapm.c           |   54 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 59 insertions(+), 2 deletions(-)

-- 
1.7.3.2

[RFC v2 1/3] ASoC: core: DAPM widget flag for reversed power up sequence

[Peter Ujfalusi]

New flag indicating, that the DAPM widget needs different
power up sequence.
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 widget (route_change_before_powerup),
reordered power up sequence can be requesetd, 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.

With the flag only the power up sequence can be changed, the power down
sequence remains the same.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 include/sound/soc-dapm.h |    1 +
 sound/soc/soc-dapm.c     |   17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 041e98b..e5a3c74 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -447,6 +447,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 */
+	unsigned char route_change_before_powerup:1;	/* reveresed power up */
 
 	int (*power_check)(struct snd_soc_dapm_widget *w);
 
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 6a29d59..7fae969 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1626,8 +1626,9 @@ 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 update_power_first = 1;
 	int connect;
-	int ret;
+	int ret = 0;
 
 	val = (ucontrol->value.integer.value[0] & mask);
 
@@ -1653,9 +1654,16 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 		else
 			/* old connection must be powered down */
 			connect = invert ? 1:0;
+	} else {
+		goto out;
+	}
+
+	if (widget->route_change_before_powerup && connect)
+		/* Reordering the power up sequence  */
+		update_power_first = 0;
 
+	if (update_power_first)
 		dapm_mixer_update_power(widget, kcontrol, connect);
-	}
 
 	if (widget->event) {
 		if (widget->event_flags & SND_SOC_DAPM_PRE_REG) {
@@ -1673,6 +1681,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 (!update_power_first)
+		dapm_mixer_update_power(widget, kcontrol, connect);
 out:
 	mutex_unlock(&widget->codec->mutex);
 	return ret;
-- 
1.7.3.2

[RFC v2 2/3] ASoC: core: Helper function to request reversed widget power up

[Peter Ujfalusi]

Helper function for drivers to request reversed power up sequence
on a given DAPM widget.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 include/sound/soc-dapm.h |    2 ++
 sound/soc/soc-dapm.c     |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index e5a3c74..d5e11df 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -358,6 +358,8 @@ int snd_soc_dapm_force_enable_pin(struct snd_soc_dapm_context *dapm,
 				  const char *pin);
 int snd_soc_dapm_ignore_suspend(struct snd_soc_dapm_context *dapm,
 				const char *pin);
+int snd_soc_dapm_reverse_powerup_sequence(struct snd_soc_dapm_context *dapm,
+					  const char *name);
 
 /* dapm widget types */
 enum snd_soc_dapm_type {
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 7fae969..a3a3e94 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -119,6 +119,19 @@ static inline struct snd_soc_dapm_widget *dapm_cnew_widget(
 	return kmemdup(_widget, sizeof(*_widget), GFP_KERNEL);
 }
 
+/* Find a widget by name */
+static inline struct snd_soc_dapm_widget *snd_soc_dapm_get_widget(
+			struct snd_soc_dapm_context *dapm, const char *name)
+{
+	struct snd_soc_dapm_widget *w;
+
+	list_for_each_entry(w, &dapm->widgets, list) {
+		if (!strcmp(w->name, name))
+			return w;
+	}
+	return NULL;
+}
+
 /**
  * snd_soc_dapm_set_bias_level - set the bias level for the system
  * @card: audio device
@@ -2260,6 +2273,30 @@ int snd_soc_dapm_ignore_suspend(struct snd_soc_dapm_context *dapm,
 EXPORT_SYMBOL_GPL(snd_soc_dapm_ignore_suspend);
 
 /**
+ * snd_soc_dapm_reverse_powerup_sequence - reverse power up sequence on a widget
+ * @codec: audio codec
+ * @pin: Name of the DAPM widget
+ *
+ * Change the power up sequence on a given DAPM widget.
+ *
+ * Returns 0 on success, otherwise -EINVAL.
+ */
+int snd_soc_dapm_reverse_powerup_sequence(struct snd_soc_dapm_context *dapm,
+					  const char *name)
+{
+	int ret = -EINVAL;
+	struct snd_soc_dapm_widget *widget;
+
+	widget = snd_soc_dapm_get_widget(dapm, name);
+	if (widget) {
+		widget->route_change_before_powerup = 1;
+		ret = 0;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_reverse_powerup_sequence);
+
+/**
  * snd_soc_dapm_free - free dapm resources
  * @card: SoC device
  *
-- 
1.7.3.2

[RFC v2 3/3] ASoC: tlv320dac33: Reverse the power up sequence for bypass switches

[Peter Ujfalusi]

Switching of the bypass on generates a pop noise.
Reverse the order of the power up sequence.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/codecs/tlv320dac33.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c
index ccb267f..2aec9e2 100644
--- a/sound/soc/codecs/tlv320dac33.c
+++ b/sound/soc/codecs/tlv320dac33.c
@@ -634,6 +634,10 @@ static int dac33_add_widgets(struct snd_soc_codec *codec)
 	/* set up audio path interconnects */
 	snd_soc_dapm_add_routes(dapm, audio_map, ARRAY_SIZE(audio_map));
 
+	/* Reverse the power up sequence for bypass switches */
+	snd_soc_dapm_reverse_powerup_sequence(dapm, "Analog Left Bypass");
+	snd_soc_dapm_reverse_powerup_sequence(dapm, "Analog Right Bypass");
+
 	return 0;
 }
 
-- 
1.7.3.2

Re: [RFC v2 0/3] ASoC: Option to reorder widget power sequence

[Mark Brown]

On Fri, Dec 03, 2010 at 03:31:10PM +0200, Peter Ujfalusi wrote:

> 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.

Sorry I've not got back to you yet - I want to implement the thing I was
talking about with inserting the register update into the DAPM sequence.
I think that is going to be easier and more generally useful, and it
doesn't rely on drivers knowing what's going on with the DAPM sequncing
code which is much cleaner from an abstraction point of view.

Re: [RFC v2 0/3] ASoC: Option to reorder widget power sequence

[Peter Ujfalusi]

On Tuesday 07 December 2010 00:40:08 ext Mark Brown wrote:
> On Fri, Dec 03, 2010 at 03:31:10PM +0200, Peter Ujfalusi wrote:
> > 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.
> 
> Sorry I've not got back to you yet - I want to implement the thing I was
> talking about with inserting the register update into the DAPM sequence.
> I think that is going to be easier and more generally useful, and it
> doesn't rely on drivers knowing what's going on with the DAPM sequncing
> code which is much cleaner from an abstraction point of view.

Fair enough.
I did taken a look at the possibility to do the register update between DAPM 
power down and up, but I thought it is a bit big change (the place, where the 
DAPM power down and up happens is quite deep, and I was not sure how to inject 
things between those).

I'm no longer sure, when the register update should happen.
It might be worth doing the update at around the power enable for the widget.
IN that way at least we are going to filter the power updates before the 
switch:
DAC -> PGA1 -> widget -> switch -> PGA2 -> out

If we update the register for the switch before the DAPM power up, than we can 
expose the switching from DAC, PGA1, widget.
If we do the update at the same time (or around), when the power for the 
switch would have been enabled, than we are filtering the switching for the 
DAPM widgets before the switch.

-- 
Péter

Re: [RFC v2 0/3] ASoC: Option to reorder widget power sequence

[Mark Brown]

On Tue, Dec 07, 2010 at 11:29:15AM +0200, Peter Ujfalusi wrote:

> Fair enough.
> I did taken a look at the possibility to do the register update between DAPM 
> power down and up, but I thought it is a bit big change (the place, where the 
> DAPM power down and up happens is quite deep, and I was not sure how to inject 
> things between those).

Stash the callback in the DAPM context - we have to lock the context to
do the path walks anyway so no issue with reentrancy.

> If we do the update at the same time (or around), when the power for the 
> switch would have been enabled, than we are filtering the switching for the 
> DAPM widgets before the switch.

On the other hand it's possible that this will result in a sharp switch
in of a DC offset rather than a slow ramp, and anything doing DC offset
correction is going to want its input to be up before correcting.