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