[RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[Peter Ujfalusi]

Hello Mark, Liam,

I'm facing the following problem with the TWL4030 codec:
The order of DAPM register write order different in case, when I start a capture
operation after setting up the routing, and when I change the routing during
active capture.
This is really visible, when I want to record from the digital microphone
interface.
Because of this ordering problem, when during active capture I switch to the
digital mic from analog path, the codec would enable analog bias (2.2 volts) to
the digital mic for a moment, and than later it will switch to the correct
digital mic bias (1.8 volts).
Since the twl4030 codec needs several bits in different registers to be changed,
when I change between analog and digital input it is not that obvious how to get
the correct ordering all the time.

I have two different ways to fix this:
A: by reordering how DAPM would handle control changes (patch 1)
B: by changing TWL4030 internal DAPM widgets (patch 2)

Obviously only one of the two patch is needed, and I'm really not sure if the
change in soc-dapm is a correct one, since it will affect all codec driver, and
could potentially cause unexpected glitches.

Some more thoughts..
A: Reordering in soc-dapm.
Currently this is how it works:
A.1 Route setup, followed by capture
- User selects the digimic0 interface (DAPM_MUX_E)
  ADCMICSEL:	TX1IN_SEL->1	MUX_E(TX1 Capture Route) enum register
				selects digital path
  MICBIAS_CTL:	MICBIAS1_CTL->1 MUX_E(TX1 Capture Route) POST_REG
				select digimic bias (1.8 volts)
- Capture starts
  MICBIAS_CTL:	MICBIAS1_EN->1 	MICBIAS(Mic Bias 1) power
				enables the micbias1/digimic0 bias (1.8 volts)
  ADCMICSEL:	DIGIMIC0_EN->1	PGA_E(Digimic0 Enable) power
				enable the digimic0 interface

A.2 Changing from analog path to digital path during capture
- User selects the digimic0 interface from analog source
  AVADC_CTL:	ADCL_EN->0	PGA(ADC Physical Left) power
				turn off ADC left
  ANAMICL:	MICAMPL_EN->0	MIXER(Analog Left) power
				turn off mic amp on analog path
  MICBIAS_CTL:	MICBIAS1_EN->1	MICBIAS(Mic Bias 1) power
				enables the micbias1/digimic0 bias (2.2 volts)
  ADCMICSEL:	DIGIMIC0_EN->1	PGA(Digimic0 Enable) power
				enable the digimic0 interface
  ADCMICSEL:	TX1IN_SEL->1	MUX_E(TX1 Capture Route) enum register
				selects digital path
  MICBIAS_CTL:	MICBIAS1_CTL->1	MUX_E(TX1 Capture Route) POST_REG
				select digimic bias (1.8 volts)

With the reordering in soc-dapm, only the runtime order is going to change:
A.2
  ADCMICSEL:	TX1IN_SEL->1	MUX_E(TX1 Capture Route) enum register
				selects digital path
  MICBIAS_CTL:	MICBIAS1_CTL->1	MUX_E(TX1 Capture Route) POST_REG
				select digimic bias (1.8 volts)
  AVADC_CTL:	ADCL_EN->0	PGA(ADC Physical Left) power
				turn off ADC left
  ANAMICL:	MICAMPL_EN->0	MIXER(Analog Left) power
				turn off mic amp on analog path
  MICBIAS_CTL:	MICBIAS1_EN->1	MICBIAS(Mic Bias 1) power
				enables the micbias1/digimic0 bias (1.8 volts)
  ADCMICSEL:	DIGIMIC0_EN->1	PGA(Digimic0 Enable) power
				enable the digimic0 interface

So first DAPM would do the change in the register, than goes on and check the
needed things to be done in the DAPM tree. This works with the TWL codec, but
I'm not sure what are the implication on other codecs.
But from the ordering point of view now there is no difference between 'cold
start' or runtime change in DAPM write sequence.

B: Reordering in TWL4030 codec driver
The sequence will look like this:
B.1 Route setup, followed by capture
- User selects the digimic0 interface (DAPM_MUX_E)
  ADCMICSEL:	TX1IN_SEL->1	MUX(TX1 Capture Route) enum register
				selects digital path
- Capture starts
  MICBIAS_CTL:	MICBIAS1_CTL->1 SUPPLY(micbias1 select) power
				select digimic bias (1.8 volts)
  MICBIAS_CTL:	MICBIAS1_EN->1 	MICBIAS(Mic Bias 1) power
				enables the micbias1/digimic0 bias (1.8 volts)
  ADCMICSEL:	DIGIMIC0_EN->1	PGA_E(Digimic0 Enable) power
				enable the digimic0 interface

B.2 Changing from analog path to digital path during capture
- User selects the digimic0 interface from analog source
  AVADC_CTL:	ADCL_EN->0	PGA(ADC Physical Left) power
				turn off ADC left
  ANAMICL:	MICAMPL_EN->0	MIXER(Analog Left) power
				turn off mic amp on analog path
  MICBIAS_CTL:	MICBIAS1_CTL->1	SUPPLY(micbias1 select) power
				select digimic bias (1.8 volts)
  MICBIAS_CTL:	MICBIAS1_EN->1	MICBIAS(Mic Bias 1) power
				enables the micbias1/digimic0 bias (1.8 volts)
  ADCMICSEL:	DIGIMIC0_EN->1	PGA(Digimic0 Enable) power
				enable the digimic0 interface
  ADCMICSEL:	TX1IN_SEL->1	MUX(TX1 Capture Route) enum register
				selects digital path

In this case the order is still not the same, but at least I avoid feeding wrong
bias to the digimic interface.

I need either the soc-dapm.c patch _or_ the patch against twl4030 codec to solve
my problem.
Do you see another way to actually solve this problem?
Which approach is acceptable for you?

The series generated on top of (needs th previous TWL DAPM fix):
git://git.kernel.org/pub/scm/linux/kernel/git/lrg/asoc-2.6.git:for-2.6.36

Thank you,
Peter

---
Peter Ujfalusi (2):
  ASoC: soc-dapm: Reorder DAPM register write sequence
  ASoC: TWL4030: Capture route runtime DAPM ordering fix

 sound/soc/codecs/twl4030.c |   48 +++++++++++---------------------------------
 sound/soc/soc-dapm.c       |   41 ++++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 52 deletions(-)

--
1.7.2

[RFC 1/2] ASoC: soc-dapm: Reorder DAPM register write sequence

[Peter Ujfalusi]

This patch changes the order of DAPM widget register write
and the DAPM power update walking.

Currently this is what happens:
1. User changes a control, which is associated with DAPM.
2. First the DAPM walking happens based on the coming change
3. DAPM changes the widgets in the changed paths
4. The requested change by the control will be written to the codec.

After the patch:
1. User changes a control, which is associated with DAPM.
2. We check, if there is a change (and store the result in 'change')
3. The requested change by the control will be written to the codec.
4. If there were change DAPM walking happens
5. The rest of DAPM changes happen

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

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 03cb7c0..ccdd441 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1573,7 +1573,7 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 	int max = mc->max;
 	unsigned int mask = (1 << fls(max)) - 1;
 	unsigned int invert = mc->invert;
-	unsigned int val, val2, val_mask;
+	unsigned int val, val2, val_mask, change;
 	int connect;
 	int ret;
 
@@ -1593,17 +1593,7 @@ 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;
-
-		dapm_mixer_update_power(widget, kcontrol, connect);
-	}
+	change = snd_soc_test_bits(widget->codec, reg, val_mask, val);
 
 	if (widget->event) {
 		if (widget->event_flags & SND_SOC_DAPM_PRE_REG) {
@@ -1621,6 +1611,17 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol,
 	} else
 		ret = snd_soc_update_bits(widget->codec, reg, val_mask, val);
 
+	if (change) {
+		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);
+	}
+
 out:
 	mutex_unlock(&widget->codec->mutex);
 	return ret;
@@ -1690,7 +1691,6 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 	mutex_lock(&widget->codec->mutex);
 	widget->value = val;
 	change = snd_soc_test_bits(widget->codec, e->reg, mask, val);
-	dapm_mux_update_power(widget, kcontrol, change, mux, e);
 
 	if (widget->event_flags & SND_SOC_DAPM_PRE_REG) {
 		ret = widget->event(widget,
@@ -1701,9 +1701,14 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 
 	ret = snd_soc_update_bits(widget->codec, e->reg, mask, val);
 
-	if (widget->event_flags & SND_SOC_DAPM_POST_REG)
+	if (widget->event_flags & SND_SOC_DAPM_POST_REG) {
 		ret = widget->event(widget,
 				    kcontrol, SND_SOC_DAPM_POST_REG);
+		if (ret < 0)
+			goto out;
+	}
+
+	dapm_mux_update_power(widget, kcontrol, change, mux, e);
 
 out:
 	mutex_unlock(&widget->codec->mutex);
@@ -1836,7 +1841,6 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 	mutex_lock(&widget->codec->mutex);
 	widget->value = val;
 	change = snd_soc_test_bits(widget->codec, e->reg, mask, val);
-	dapm_mux_update_power(widget, kcontrol, change, mux, e);
 
 	if (widget->event_flags & SND_SOC_DAPM_PRE_REG) {
 		ret = widget->event(widget,
@@ -1847,9 +1851,14 @@ int snd_soc_dapm_put_value_enum_double(struct snd_kcontrol *kcontrol,
 
 	ret = snd_soc_update_bits(widget->codec, e->reg, mask, val);
 
-	if (widget->event_flags & SND_SOC_DAPM_POST_REG)
+	if (widget->event_flags & SND_SOC_DAPM_POST_REG) {
 		ret = widget->event(widget,
 				    kcontrol, SND_SOC_DAPM_POST_REG);
+		if (ret < 0)
+			goto out;
+	}
+
+	dapm_mux_update_power(widget, kcontrol, change, mux, e);
 
 out:
 	mutex_unlock(&widget->codec->mutex);
-- 
1.7.2

[RFC 2/2] ASoC: TWL4030: Capture route runtime DAPM ordering fix

[Peter Ujfalusi]

Fix the ordering problem in DAPM domain, when the user
changes between digital and analog sources during active
capture (or loopback) scenario.
Before this patch, when the user changed from analog source
to digital there were a short time, when the codec enabled
analog mic bias (2.2 volts) instead of the correct digital
mic bias (1.8 volts) to the digital microphones.

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

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index d401c59..7b618bb 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -577,36 +577,6 @@ static const struct snd_kcontrol_new twl4030_dapm_dbypassv_control =
 			TWL4030_REG_VSTPGA, 0, 0x29, 0,
 			twl4030_dapm_dbypassv_tlv);
 
-static int micpath_event(struct snd_soc_dapm_widget *w,
-	struct snd_kcontrol *kcontrol, int event)
-{
-	struct soc_enum *e = (struct soc_enum *)w->kcontrols->private_value;
-	unsigned char adcmicsel, micbias_ctl;
-
-	adcmicsel = twl4030_read_reg_cache(w->codec, TWL4030_REG_ADCMICSEL);
-	micbias_ctl = twl4030_read_reg_cache(w->codec, TWL4030_REG_MICBIAS_CTL);
-	/* Prepare the bits for the given TX path:
-	 * shift_l == 0: TX1 microphone path
-	 * shift_l == 2: TX2 microphone path */
-	if (e->shift_l) {
-		/* TX2 microphone path */
-		if (adcmicsel & TWL4030_TX2IN_SEL)
-			micbias_ctl |= TWL4030_MICBIAS2_CTL; /* digimic */
-		else
-			micbias_ctl &= ~TWL4030_MICBIAS2_CTL;
-	} else {
-		/* TX1 microphone path */
-		if (adcmicsel & TWL4030_TX1IN_SEL)
-			micbias_ctl |= TWL4030_MICBIAS1_CTL; /* digimic */
-		else
-			micbias_ctl &= ~TWL4030_MICBIAS1_CTL;
-	}
-
-	twl4030_write(w->codec, TWL4030_REG_MICBIAS_CTL, micbias_ctl);
-
-	return 0;
-}
-
 /*
  * Output PGA builder:
  * Handle the muting and unmuting of the given output (turning off the
@@ -1430,12 +1400,10 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 	/* Analog/Digital mic path selection.
 	   TX1 Left/Right: either analog Left/Right or Digimic0
 	   TX2 Left/Right: either analog Left/Right or Digimic1 */
-	SND_SOC_DAPM_MUX_E("TX1 Capture Route", SND_SOC_NOPM, 0, 0,
-		&twl4030_dapm_micpathtx1_control, micpath_event,
-		SND_SOC_DAPM_POST_REG),
-	SND_SOC_DAPM_MUX_E("TX2 Capture Route", SND_SOC_NOPM, 0, 0,
-		&twl4030_dapm_micpathtx2_control, micpath_event,
-		SND_SOC_DAPM_POST_REG),
+	SND_SOC_DAPM_MUX("TX1 Capture Route", SND_SOC_NOPM, 0, 0,
+		&twl4030_dapm_micpathtx1_control),
+	SND_SOC_DAPM_MUX("TX2 Capture Route", SND_SOC_NOPM, 0, 0,
+		&twl4030_dapm_micpathtx2_control),
 
 	/* Analog input mixers for the capture amplifiers */
 	SND_SOC_DAPM_MIXER("Analog Left",
@@ -1459,6 +1427,11 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 		TWL4030_REG_ADCMICSEL, 3, 0, NULL, 0,
 		digimic_event, SND_SOC_DAPM_POST_PMU),
 
+	SND_SOC_DAPM_SUPPLY("micbias1 select", TWL4030_REG_MICBIAS_CTL, 5, 0,
+			    NULL, 0),
+	SND_SOC_DAPM_SUPPLY("micbias2 select", TWL4030_REG_MICBIAS_CTL, 6, 0,
+			    NULL, 0),
+
 	SND_SOC_DAPM_MICBIAS("Mic Bias 1", TWL4030_REG_MICBIAS_CTL, 0, 0),
 	SND_SOC_DAPM_MICBIAS("Mic Bias 2", TWL4030_REG_MICBIAS_CTL, 1, 0),
 	SND_SOC_DAPM_MICBIAS("Headset Mic Bias", TWL4030_REG_MICBIAS_CTL, 2, 0),
@@ -1590,6 +1563,9 @@ static const struct snd_soc_dapm_route intercon[] = {
 	{"Digimic0 Enable", NULL, "DIGIMIC0"},
 	{"Digimic1 Enable", NULL, "DIGIMIC1"},
 
+	{"DIGIMIC0", NULL, "micbias1 select"},
+	{"DIGIMIC1", NULL, "micbias2 select"},
+
 	/* TX1 Left capture path */
 	{"TX1 Capture Route", "Analog", "ADC Physical Left"},
 	{"TX1 Capture Route", "Digimic0", "Digimic0 Enable"},
-- 
1.7.2

Re: [RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[Mark Brown]

On 2 Aug 2010, at 08:08, Peter Ujfalusi wrote:

> I'm facing the following problem with the TWL4030 codec:
> The order of DAPM register write order different in case, when I start a capture
> operation after setting up the routing, and when I change the routing during
> active capture.
> This is really visible, when I want to record from the digital microphone
> interface.

> Because of this ordering problem, when during active capture I switch to the
> digital mic from analog path, the codec would enable analog bias (2.2 volts) to
> the digital mic for a moment, and than later it will switch to the correct
> digital mic bias (1.8 volts).

It'd be really helpful in this section if you could've said what the actual problem you see is...

> Since the twl4030 codec needs several bits in different registers to be changed,
> when I change between analog and digital input it is not that obvious how to get
> the correct ordering all the time.
> 
> I have two different ways to fix this:
> A: by reordering how DAPM would handle control changes (patch 1)
> B: by changing TWL4030 internal DAPM widgets (patch 2)

...you even start proposing solutions before describing what they're fixing :)

Just summarising briefly since I can't see a clear problem statement I think what you're saying is that DAPM applies control changes for routing before checking for power up changes but for some TWL4030 routes it would be more convenient to only write the changes for route setup after changing the power state?

I think this is best handled in TWL4030, or possibly with some sort of optional flag in DAPM which TWL4030 and anything else could then use. 

With most things it's safer to implement the route change first - especially in the analogue domain route changes tend to introduce DC offsets so it's safer to set the route prior to powering up so the offset changes don't get amplified. This is also very important when doing DC offset correction, obviously there it's important to measure the offset of the path you're actually trying to run rather than using whatever path was there before.

Re: [RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[Peter Ujfalusi]

On Monday 02 August 2010 13:27:21 ext Mark Brown wrote:
> On 2 Aug 2010, at 08:08, Peter Ujfalusi wrote:
> > I'm facing the following problem with the TWL4030 codec:
> > The order of DAPM register write order different in case, when I start a
> > capture operation after setting up the routing, and when I change the
> > routing during active capture.
> > This is really visible, when I want to record from the digital microphone
> > interface.
> > 
> > Because of this ordering problem, when during active capture I switch to
> > the digital mic from analog path, the codec would enable analog bias
> > (2.2 volts) to the digital mic for a moment, and than later it will
> > switch to the correct digital mic bias (1.8 volts).
> 
> It'd be really helpful in this section if you could've said what the actual
> problem you see is...

The problem in the TWL4030 codec during active capture: change from analog 
source to digital microphone will momentary apply 2.2V mic bias to the digital 
microphone instead of the correct 1.8V.

This only happens during active capture. If the change from analog to digital 
mic happens before the capture has been started, than it is fine.

> > Since the twl4030 codec needs several bits in different registers to be
> > changed, when I change between analog and digital input it is not that
> > obvious how to get the correct ordering all the time.
> > 
> > I have two different ways to fix this:
> > A: by reordering how DAPM would handle control changes (patch 1)
> > B: by changing TWL4030 internal DAPM widgets (patch 2)
> 
> ...you even start proposing solutions before describing what they're fixing
> :)
> 
> Just summarising briefly since I can't see a clear problem statement I
> think what you're saying is that DAPM applies control changes for routing
> before checking for power up changes but for some TWL4030 routes it would
> be more convenient to only write the changes for route setup after
> changing the power state?
> 
> I think this is best handled in TWL4030, or possibly with some sort of
> optional flag in DAPM which TWL4030 and anything else could then use.
> 
> With most things it's safer to implement the route change first -
> especially in the analogue domain route changes tend to introduce DC
> offsets so it's safer to set the route prior to powering up so the offset
> changes don't get amplified. This is also very important when doing DC
> offset correction, obviously there it's important to measure the offset of
> the path you're actually trying to run rather than using whatever path was
> there before.

Sure, but what I'm telling is that we/I have basically two different scenarios 
to fix for pop noise.

A. When the user configures the capture routing, and starts the capture
B. When the user changes the capture routing during active capture (analog to 
digital, or back).

The sequences in these case:
A.1 The changes in register(s) are applied based on the DAPM_MUX's enum
      control
A.2 The DAPM power up changes happen for the selected route

B.1 The DAPM power up changes happen for the selected new route
B.2 The changes in register(s) are applied based on the DAPM_MUX's enum
      control

Since the DAPM_MUX does not have any sensible power bit to toggle, just 
selects between the digital or analog path.
But if it would have power bit, than that power would have bee applied in A.2 
or B.1 phase, when the DAPM power up changes are happening.

So we have different sequences, and somehow I need to make sure, that at least 
I'm not doing any harm to components.

This might be not a problem for others, but since the TWL4030 has quite 
complicated internal routings, and the controls for some of the routings are 
scattered all around the register table with bits here and there, and 
dependency to other bits, at least I'm not that happy that I need to handle 
things differently...

-- 
Péter

Re: [RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[Mark Brown]

On 2 Aug 2010, at 11:59, Peter Ujfalusi wrote:

> On Monday 02 August 2010 13:27:21 ext Mark Brown wrote:

>> With most things it's safer to implement the route change first -
>> especially in the analogue domain route changes tend to introduce DC
>> offsets so it's safer to set the route prior to powering up so the offset
>> changes don't get amplified. This is also very important when doing DC
>> offset correction, obviously there it's important to measure the offset of
>> the path you're actually trying to run rather than using whatever path was
>> there before.

> Sure, but what I'm telling is that we/I have basically two different scenarios 
> to fix for pop noise.

> A. When the user configures the capture routing, and starts the capture
> B. When the user changes the capture routing during active capture (analog to 
> digital, or back).

It feels like you're focusing in on one specific use case here - there's way more active->active transitions that are possible. For example, you could switch in bypass or start a second (unrelated) capture stream on devices that can support that. I think any change in DAPM needs to be motivated in more general terms than you're using here. For driver-specific changes this is fine but for changes in the generic code we need to think things through more generically.

> This might be not a problem for others, but since the TWL4030 has quite 
> complicated internal routings, and the controls for some of the routings are 
> scattered all around the register table with bits here and there, and 
> dependency to other bits, at least I'm not that happy that I need to handle 
> things differently...

I don't think the feature set is particularly unusual here, really - the main problem with TWL4030 appears to be the large number of magic "make the chip behave totally differently" bits that it has like with the selection between DSP and I2S modes.

Re: [RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[Peter Ujfalusi]

On Tuesday 03 August 2010 05:56:40 ext Mark Brown wrote:
> > A. When the user configures the capture routing, and starts the capture
> > B. When the user changes the capture routing during active capture
> > (analog to digital, or back).
> 
> It feels like you're focusing in on one specific use case here - there's
> way more active->active transitions that are possible. For example, you
> could switch in bypass or start a second (unrelated) capture stream on
> devices that can support that. I think any change in DAPM needs to be
> motivated in more general terms than you're using here. For
> driver-specific changes this is fine but for changes in the generic code
> we need to think things through more generically.

Sure, I'm pointing these two cases, since I have noticed the problem in these. 
But the same thing happens, when one enables the bypass vs during enabled bypass 
the routing is changed.
We do have different register write sequences, which means that we could have 
different type of pop depending on the current sequence.
In startup (playback/capture/bypass) we most likely have pop from components 
powering up, while during active state (and changing the routings) we might need 
to deal with the pop coming from components turning off.
As I said, this might be not an issue at all, but the twl4030 codec used to 
switch two bits with DAPM_MUX (within the enum, and than with POST_REG event). 
Because of the ordering change, in runtime this causes problems, since the 
DAPM_MUX's POST_REG comes as the last thing in the sequence.

The patch against soc-dapm.c is my way of asking about this different sequences 
on startup vs runtime routing change.

I can fix this problem within the twl4030 codec as well (patch 2), which gives 
me satisfying results, so I'm happy if you ack that.

I only need one of the two patch, and my bet goes for the fix within the twl4030 
codec driver, but I do wanted to bring up the root cause of this problem 
(surfaced with my codec driver). 


Thanks,
Péter

Re: [RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[Mark Brown]

On Tue, Aug 03, 2010 at 09:15:12AM +0300, Peter Ujfalusi wrote:

> We do have different register write sequences, which means that we could have 
> different type of pop depending on the current sequence.
> In startup (playback/capture/bypass) we most likely have pop from components 
> powering up, while during active state (and changing the routings) we might need 
> to deal with the pop coming from components turning off.

Good, this is the sort of analysis we need to make a change to DAPM
itself.  I don't think it's resonable to say that the active->active
changes are primarily concerned with powerdown pops, though - if you're
making a change where you're adding in a new source then obviously the
powerup of the path is going to be relevant.

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.

> Because of the ordering change, in runtime this causes problems, since the 
> DAPM_MUX's POST_REG comes as the last thing in the sequence.

You keep saying "ordering change" but I'm still not clear what you mean
by this?  Are you just talking about the fact that things end up
happening in different orders for the active->active transitions or do
you see some active reordering of things in the code?

> I only need one of the two patch, and my bet goes for the fix within the twl4030 
> codec driver, but I do wanted to bring up the root cause of this problem 
> (surfaced with my codec driver). 

Honestly, looking at the TWL4030 specific patch it looks like a better
idea anyway regardless of any changes to the core since it's just moving
from the use of an event to pure DAPM widgets which is generally a more
robust approach.  Events generally cause hassle if they're doing much
more than implementing multi-write sequences for turning the widget on
and off.

If you could provide a version of the patch with a standalone changelog
I think I'd ack it.

Re: [RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[Peter Ujfalusi]

On Tuesday 03 August 2010 11:39:44 ext Mark Brown wrote:
> Good, this is the sort of analysis we need to make a change to DAPM
> itself.  I don't think it's resonable to say that the active->active
> changes are primarily concerned with powerdown pops, though - if you're
> making a change where you're adding in a new source then obviously the
> powerup of the path is going to be relevant.
> 
> 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.

I tend to agree. The patch for the core is quite dramatic change, and it can 
introduce other types of problems.
But... It would make the actual register write sequence consistent in all cases.
 
> > Because of the ordering change, in runtime this causes problems, since
> > the DAPM_MUX's POST_REG comes as the last thing in the sequence.
> 
> You keep saying "ordering change" but I'm still not clear what you mean
> by this?  Are you just talking about the fact that things end up
> happening in different orders for the active->active transitions or do
> you see some active reordering of things in the code?

Yes, I mean that things end up happening in different order. The DAPM power 
sequence is always the same, but combined that with the writes not coming from 
the DAPM powering, than the sequence is different.

> > I only need one of the two patch, and my bet goes for the fix within the
> > twl4030 codec driver, but I do wanted to bring up the root cause of this
> > problem (surfaced with my codec driver).
> 
> Honestly, looking at the TWL4030 specific patch it looks like a better
> idea anyway regardless of any changes to the core since it's just moving
> from the use of an event to pure DAPM widgets which is generally a more
> robust approach.  Events generally cause hassle if they're doing much
> more than implementing multi-write sequences for turning the widget on
> and off.

Agreed.

> If you could provide a version of the patch with a standalone changelog
> I think I'd ack it.

I have sent the patch separately against the twl4030 codec.

-- 
Péter

Re: [RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem

[Mark Brown]

On Tue, Aug 03, 2010 at 12:09:10PM +0300, Peter Ujfalusi wrote:
> On Tuesday 03 August 2010 11:39:44 ext Mark Brown wrote:

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

> I tend to agree. The patch for the core is quite dramatic change, and it can 
> introduce other types of problems.
> But... It would make the actual register write sequence consistent in all cases.

The power and route configuration settings aren't really part of the
same sequence, though - other widgets can also trigger power changes,
and obviously a route change need not imply a power change.