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:   Fri, 14 Apr 2023 18:35:18 +0100
From:   Mark Brown <broonie@kernel.org>
To:     =?utf-8?B?UGF3ZcWC?= Anikiel <pan@semihalf.com>
Cc:     alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, lgirdwood@gmail.com, perex@perex.cz,
        tiwai@suse.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, dinguyen@kernel.org,
        lars@metafoo.de, nuno.sa@analog.com, upstream@semihalf.com
Subject: Re: [PATCH 5/9] ASoC: ssm2602: Add workaround for playback with
 external MCLK
Message-ID: <cb35f3f2-4dc9-4d56-96bd-bcffb33b7aaf@sirena.org.uk>
References: <20230414140203.707729-1-pan@semihalf.com>
 <20230414140203.707729-6-pan@semihalf.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="EH6yRfJ9i6hhIpAh"
Content-Disposition: inline
In-Reply-To: <20230414140203.707729-6-pan@semihalf.com>
Precedence: bulk


--EH6yRfJ9i6hhIpAh
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Apr 14, 2023 at 04:01:59PM +0200, Pawe=C5=82 Anikiel wrote:

> Apply a workaround for what seems to be a hardware quirk: when using
> an external MCLK signal, powering on Output and DAC for the first time
> produces output distortions unless they're powered together with whole
> chip power.

This doesn't seem coherent, these are multiple register writes so
clearly can't be done at the same moment as initial power on.  Clearly
there's some other constraint here.

> The workaround powers them on in probe for the first time, as doing it
> later may be impossible (e.g. when starting playback while recording,
> whole chip power will already be on).

It doesn't do that, it powers them on at component probe.

> Here are some sequences run at the very start before a sw reset (and
> later using one of the NOT OK sequences from above):
>=20
>   ssmset 0x09 0x01 # core
>   ssmset 0x06 0x07 # chip, out, dac
>   OK

I can't tell what any of this is trying to say, especially given all the
magic numbers, and obviously no actual use of the driver should be
writing directly to the register map.

> +	/* Workaround for what seems to be a hardware quirk: when using an
> +	 * external MCLK signal, powering on Output and DAC for the first
> +	 * time produces output distortions unless they're powered together
> +	 * with whole chip power. We power them here for the first time,
> +	 * as doing it later may be impossible (e.g. when starting playback
> +	 * while recording, whole chip power will already be on)
> +	 */
> +	regmap_write(ssm2602->regmap, SSM2602_ACTIVE, 0x01);
> +	regmap_write(ssm2602->regmap, SSM2602_PWR,    0x07);
> +	regmap_write(ssm2602->regmap, SSM2602_RESET,  0x00);
> +

The rest of the driver uses symbolic names for register values, this
code should too. =20

This also seems buggy in that it writes non-default values to the
hardware then does a reset, meaning that the cache and hardware values
will be out of sync, and since it only happens on probe there will be an
issue after suspend if power is removed.  It looks like this would be
most comfortably implemented as a register patch applied as soon as the
regmap is instantiated.  See regmap_register_patch().

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

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmQ5jtUACgkQJNaLcl1U
h9Ajkwf+ODUCBkDtTHlEW7Kmhees6SWhoER3fk+u//I4yyeodw2AopWmhSQaUs0m
k5/cPXokWbQaCDic+hod7YejaAtDGHj3lDH9s4CAsE9SRtuYV1SCL8N94LN6ZETG
/3MlDr+ScZ+ga+8OsSQVvGMfYZlSuRlTiUJmocRWO5dJ/thyDzh/89QdgGfQt0eo
g6asIK+pLjG4N7Pl20E/bAG5sG1AHGkYiAxOLVD6vsXzcBZT+GoI5xYxD2rleRNi
cAN5Og3SghfLXhZNQeDBZ4/7mjCF/AwymoAsPuYwQRkWeWdZh29gOhPqSUD/Mzss
EtMguFBBN3y1NPRsMRBqzvmFV4TkcQ==
=zZC1
-----END PGP SIGNATURE-----

--EH6yRfJ9i6hhIpAh--