On 21/12/2022 17:56, Charles Keepax wrote:
On Tue, Dec 20, 2022 at 08:12:23PM +0100, Emanuele Ghidoli wrote:
On 20/12/2022 11:00, Charles Keepax wrote:
On Mon, Dec 19, 2022 at 04:20:10PM +0100, Emanuele Ghidoli wrote:
On 19/12/2022 10:58, Charles Keepax wrote:
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
I see that that CP_DYN_PWR bit is disabled when audio is going
through one of the bypass paths. Would you be able to enable one
of the bypass paths and see if you can manually adjust the volume
on the headphone output, with a bypass path active?
With the previous change, I tested all the possible combination with
one channel from the DAC and the other toggling from DAC to Bypass
changing the volume and it's always correct.
Would also perhaps be interesting as a test to completely remove
the write to CP_DYN_PWR from probe and leave things set to manual
and see how the volume behaves then?
When I tried to remove any write to this register my modification
stopped working.
Apologies just to be totally clear here, you are saying that
whilst a bypass path is active (ie. the class G widget has
cleared CP_DYN_PWR), you can still control the volume? But if you
remove the set of CP_DYN_PWR from probe, the volume doesn't
update at all, audio playing or not?
Yes, exactly. But I have also commented:
SND_SOC_DAPM_SUPPLY("Class G", WM8904_CLASS_W_0, 0, 1, NULL, 0)
In every case the volume updates, while playing, when you write the relevant register
(raw i2cset or changing volume using amixer).
To be clear:
The volume is not updated, after BIAS off, if we are in CLASS G WITH
bypass DISABLED (that, without these modification, it is a condition we
cannot trigger. Normally: Bypass ON->class G, bypass OFF->class W).
Effectively, maybe, the test with bypass enabled is affected by the fact
codec is not switched off (bias is kept on... otherwise as soon I stop
playing something from my linux device bypass will stop working due to
codec reset/power off).
In other words the Dynamic Audio Power Managent (DAPM),
which I "understand" only now, is doing its work.
I guess the interests here are to find out if the SYSCLK is
involved at all.
I tested keep the clock always enabled, removing
clk_disable_unprepare when going into SND_SOC_BIAS_OFF and it has
zero effects.
Or did you mean something else?
Yeah that is not quite what I was getting at. I am wondering if
volume updates work whilst CP_DYN_PWR==0 and CLK_SYS_ENA==1.
Why are you wondering? It should be a standard working case (obviously
with MCLK running). I know, from datasheet, that:
"CLK_SYS_ENA = 1 and MCLK is not present, then register access will be
unsuccessful". But it is not our case.
Said all of that, I did one last test, forcing a volume update on
the charge pump enable callback, cp_event(), with this and only this
modification in everything is working fine.
Could it just be as easy as that the volume is applied only when the
charge pump is already active?
From the datasheet this seems a good explanation:
The Charge Pump is enabled by setting the CP_ENA bit. When enabled, the
charge pump adjusts the output voltages (CPVOUTP and CPVOUTN).
What do you think?
I think we are getting pretty close, but we need to try and
narrow down what the requirement is here, is it the charge pump,
or the sysclk? That needs to be on for the volume update to work.
Watching another codec driver (wm8964: see out_pga_event comment) and
the Startup-sequence (of WM8904) in datasheet we figure out that volume
update must be done after PGA enable.
I tested another patch, I'm pretty convinced that it is the right way to
do it. Now it is working in all conditions (even Class G with disabled bypass).
Maybe some hw guy in Cirrus Logic can dig around?
Anyway, this is the tested patch, that, to me, sound good:
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index ca6a01a230af..791d8738d1c0 100644
--- a/sound/soc/codecs/wm8904.c
+++ b/sound/soc/codecs/wm8904.c
@@ -697,6 +697,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w,
int dcs_mask;
int dcs_l, dcs_r;
int dcs_l_reg, dcs_r_reg;
+ int an_out_reg;
int timeout;
int pwr_reg;
@@ -712,6 +713,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w,
dcs_mask = WM8904_DCS_ENA_CHAN_0 | WM8904_DCS_ENA_CHAN_1;
dcs_r_reg = WM8904_DC_SERVO_8;
dcs_l_reg = WM8904_DC_SERVO_9;
+ an_out_reg = WM8904_ANALOGUE_OUT1_LEFT;
dcs_l = 0;
dcs_r = 1;
break;
@@ -720,6 +722,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w,
dcs_mask = WM8904_DCS_ENA_CHAN_2 | WM8904_DCS_ENA_CHAN_3;
dcs_r_reg = WM8904_DC_SERVO_6;
dcs_l_reg = WM8904_DC_SERVO_7;
+ an_out_reg = WM8904_ANALOGUE_OUT2_LEFT;
dcs_l = 2;
dcs_r = 3;
break;
@@ -792,6 +795,10 @@ static int out_pga_event(struct snd_soc_dapm_widget *w,
snd_soc_component_update_bits(component, reg,
WM8904_HPL_ENA_OUTP | WM8904_HPR_ENA_OUTP,
WM8904_HPL_ENA_OUTP | WM8904_HPR_ENA_OUTP);
+
+ /* Update volume, requires PGA to be powered */
+ val = snd_soc_component_read(component, an_out_reg);
+ snd_soc_component_write(component, an_out_reg, val);
break;
case SND_SOC_DAPM_POST_PMU:
Thanks,
Charles
Thank you.
Best regards,
Emanuele
