wm8904 and output volume control

[Emanuele Ghidoli]

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