Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Mar 2023 17:07:49 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc:     perex@perex.cz, tiwai@suse.com, lgirdwood@gmail.com,
        linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
        johan+linaro@kernel.org, steev@kali.org,
        dmitry.baryshkov@linaro.org
Subject: Re: [PATCH 3/4] ASoC: codecs: wsa883x: mute/unmute PA in correct
 sequence
Message-ID: <ff3eb88a-6941-4303-a4ba-17cad3842b88@sirena.org.uk>
References: <20230323164403.6654-1-srinivas.kandagatla@linaro.org>
 <20230323164403.6654-4-srinivas.kandagatla@linaro.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="J/cCEY6usy6hBul2"
Content-Disposition: inline
In-Reply-To: <20230323164403.6654-4-srinivas.kandagatla@linaro.org>
Precedence: bulk


--J/cCEY6usy6hBul2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Mar 23, 2023 at 04:44:02PM +0000, Srinivas Kandagatla wrote:
> In the current setup the PA is left unmuted even when the
> Soundwire ports are not started streaming. This can lead to click
> and pop sounds during start.
> There is a same issue in the reverse order where in the PA is
> left unmute even after the data stream is stopped, the time
> between data stream stopping and port closing is long enough
> to accumulate DC on the line resulting in Click/Pop noise
> during end of stream.

Wow, that hardware sounds *super* fragile.

> Moving the mute/unmute to trigger stop/start respectively seems to
> help a lot with this Click/Pop issues reported on this Codec.

> +static int wsa883x_trigger(struct snd_pcm_substream *s, int cmd,
> +			   struct snd_soc_dai *dai)
> +{
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		wsa883x_digital_mute(dai, false, 0);
> +		break;

>  static const struct snd_soc_dai_ops wsa883x_dai_ops = {
> +	.startup = wsa883x_startup,
>  	.hw_params = wsa883x_hw_params,
>  	.hw_free = wsa883x_hw_free,
> -	.mute_stream = wsa883x_digital_mute,
> +	.trigger = wsa883x_trigger,

The trigger is run in atomic context, can you really write safely to a
SoundWire device there?

This feels like we should be doing it at the framework level, either
tightening up where the mute happens in general or having some option
that devices can select if they really need it.

--J/cCEY6usy6hBul2
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmQch2QACgkQJNaLcl1U
h9CdLAf/V22IjvGkc6ep2k+xNGFGZdI59ZoRgE4Sqa7d9kEiMBjiMx9SSCz/LJnI
knGwmxnaSzXxhHXelAWPr9dis09vqeciiuxerX4dkPjOB5UF5TwFPTWOpDRICuKs
XaETlp285PZ/v4ew94q8wAEGEafARsTo52P2qjzLDKvAZVW9uOfbxwL36vqa/ZZW
WHrvSJItQdagVjYdHKVyrLqvU7PrHCkqg58OmQmv6JIpXRlSg2+yTHCGmZJRKKYE
SNZOMPB0wZfJvcRrhEI5R4ia8X5SBjRg98IgYd/kG19bsSye13usKxZuIGgtZgXW
2eDN0wDoFyF296QftPJpLxB1jFyLhw==
=yGjx
-----END PGP SIGNATURE-----

--J/cCEY6usy6hBul2--