wm8904 and output volume control

[Charles Keepax]

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:
> 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 */
> 

That is some good detective work.

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

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?

I guess the interests here are to find out if the SYSCLK is
involved at all. And if the issue is changing CP_DYN_PWR after a
volume update is what causes the problem. I could perhaps see a
situation where the volume update data is sent to a different
place depending on the value of CP_DYN_PWR, meaning that changing
that bit after doing a volume update causes the volume data to be
lost, but that implies we might need a fix for the Class G widget
as well.

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

I think assuming we get a point that the requirement is
simply that CLASS_W needs written before the volume, the change
as you propose it looks good. But I suspect there is a little
more going on here.

Thanks,
Charles