Re: [PATCH 3/3] ASoC: wm8940: Mute also the speaker output

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2841 bytes --]

On Fri, Jun 10, 2022 at 11:23:31AM +0200, Lukasz Majewski wrote:
> > On Mon, Jun 06, 2022 at 05:44:41PM +0200, Lukasz Majewski wrote:

> > > Without this change the BTL speaker produces some
> > > "distortion" noise when test program
> > > (speaker-test -t waw) is ended with ctrl+c.  

> > > As our design uses speaker outputs to drive BTL speaker,
> > > it was necessary to also mute the speaker via the codec
> > > internal WM8940_SPKVOL register with setting
> > > WM8940_SPKMUTE bit.  

> > This will not interact well with both the user visible control of the
> > speaker volume via the Speaker Playback Volume control and the analog
> > bypass paths that the device has - it'll change the state of the
> > control without generating any events, and cut off any bypassed audio
> > that's mixed in.

> I'm wondering why it is safe to call DAI's .digital_mute()
> callback, which explicitly changes state of the "DAC soft mute enable"
> bit (DACMU) ?

If there's a user visible control for the same register bit that's a
bug.  If there's no user visible control for it then there's nothing to
conflict with.

> And on the other hand it is not correct to just mute the speakers?

No, that's not what we're muting playback for - the digital mute is
there specifically to deal with issues with host controllers outputing
noise during startup/teardown.  If there are issues with the speaker
output then they need to be addressed at that point, especially given
that the device has bypass paths.

> > You can probably achieve a similar effect by making the control an
> > _AUTODISABLE one which will allow the core to mute the control when
> > it's not being used in a way that's not visible to userspace.

> The exact definition for the event, which I'm forcing above:

> SOC_SINGLE("Speaker Playback Switch", WM8940_SPKVOL,  6, 1, 1),

> And there is no SOC_SINGLE_AUTODISABLE() macro available.

That seems solvable?  Though if the issue isn't triggered in connection
with a DAPM event (which sounds like the case) then it's probably not
going to help.

> The issue I'm trying to fix:

> - The mclk clock is stopped (after some time) by imx SOC when I end
>   'speaker-test' program with ctrl+c.

> - When the clock is not provided (after ~1sec) I do hear a single short
>   noise from speakers.

> - The other solution (which also works) would be to enable clock once
>   (during probe) and then do not disable it till system is powered
>   off (yes it is a hack :-) ).

If the issue is triggered by the MCLK being disabled prematurely then
the simplest fix is probably to wire up the CODEC MCLK to the clock API
and manage it during set_bias_level() (probably on transition out of and
into _STANDBY) - that should have a similar effect to leaving it enabled
all the time.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]