[alsa-devel] [RFC 0/2] soc-dapm or TWL4030: Runtime DAPM ordering problem
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
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);
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"},
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.
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...
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.
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
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.
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.
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.
participants (2)
-
Mark Brown -
Peter Ujfalusi