wm8904 and output volume control

[Emanuele Ghidoli]

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 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.
>>
> 
> Hmm... I think my only concern here is this feels a bit counter
> intuitive, the default value is described as "controlled by
> volume register settings" and we are saying in that situation the
> volume registers don't seem to update properly. That is far from
> impossible but I think we should perhaps poke a little more to
> make sure we understand the bounds here.
I did some more test and I'm not 100% sure of what is going on anymore.


> 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.


> 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?

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?

Emanuele