Re: [PATCH 5/9] ASoC: ssm2602: Add workaround for playback with external MCLK

[Mark Brown <broonie@kernel.org>]

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

On Fri, Apr 14, 2023 at 04:01:59PM +0200, Paweł 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):
> 
>   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.  

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

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