On 19/12/2022 10:58, Charles Keepax wrote:
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
Hello,
I have found that output volume is set to the default value after a
limited time (~4 seconds) after play stop.
I have analyzed what is happening:
- at first play the volume is the expected one
- After stopping playing + ~4 seconds wm8904_set_bias_level (...,
SND_SOC_BIAS_OFF) is called
- Chip is reset and regulator is switched off if "possible" (not in
our case, it is important to note)
- at second play wm8904_set_bias_level (..., SND_SOC_BIAS_STANDBY)
is called. wm8904_hw_params is also called just before.
I see that all I2C transactions are good, registers are written as
expected, especially volume control register (even the silly
HPOUT_VU bit, to do a volume update is correctly written).
Reading out this register, using i2cget (bypassing regcache), report
the "expected" value. But the real output volume correspond to the
default value (hw default, that it is the same value in
wm8904_reg_defaults structure).
I checked that SYSCLK_ENA is 0 during register writing (needed if
MCLK/SYSCLK is not present). I do not know if there is some other
constrain on these registers.
Yes this might be my guess as well, I wonder if the HPOUT_VU bit
needs SYSCLK to be present to take effect.
If I rewrite the volume control register, a second time, the volume
is set (tested with i2cset, from user space. And from kernel space,
bypassing regcache).
When you write the value the second time is that still before
SYSCLK is present?
Also does just writing one of HPOUT volumes cause the other to
get updated? The HPOUT_VU bit should trigger an update to both.
I resolve also by reverting e9149b8c00d2 ("ASoC: wm8904: fix
regcache handling").
I'm not here to say that the commit is wrong. I analyzed it and seem
perfect (in few words it align the hw with the regcache avoiding
potential incoherence).
Yeah I think that commit is fine, I would wager that your system
doesn't turn off the regulators so without that commit the
register retains the old volume across the "power down".
Thanks,
Charles
Hello Charles,
thank you.
With this patch (draft) seem the "bug" is fixed: (bug that is present,
for sure, also with an effective regulator)
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index ca6a01a230af..33452daf1ae8 100644
--- a/sound/soc/codecs/wm8904.c
+++ b/sound/soc/codecs/wm8904.c
@@ -1903,6 +1903,7 @@ static int wm8904_set_bias_level(struct
snd_soc_component *component,
}
regcache_cache_only(wm8904->regmap, false);
+ regcache_sync_region(wm8904->regmap,
WM8904_CLASS_W_0, WM8904_CLASS_W_0);
regcache_sync(wm8904->regmap);
/* Enable bias */
I infer, from datasheet, that volume update is applied in different way
based on charge pump dynamic vs register control (CP_DYN_PWR bit in
CLASS_W register):
"Under Register control, the HPOUTL_VOL, HPOUTR_VOL, LINEOUTL_VOL and
LINEOUTR_VOL register settings are used to control the charge pump mode
of operation.
Under Dynamic control, the audio signal level in the DAC is used to
control the charge pump mode of operation."
The second sentence do not explain that volume register is still
considered by the component but likely in a different way.
It is important to note that I trace I2C transactions and, without the
patch, the CLASS_W register is written JUST after volume update
registers (with the patch is written before and after).
At this point I have no doubt that we have to update that register
before writing volume.
I guess: is there another way to do same/similar thing (in a better way,
like just force write to that register a little bit before of volume
update. I walk around regmap/regcache but I do not find a different
solution)?
We must pay attention that cached value of this register can be changed
by widget "Class G" (my patch should work also if you toggle this widget).
Thank you,
best regards.
Emanuele Ghidoli
