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
