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;
MIME-Version: 1.0
References: <20230414140203.707729-1-pan@semihalf.com> <20230414140203.707729-6-pan@semihalf.com>
 <cb35f3f2-4dc9-4d56-96bd-bcffb33b7aaf@sirena.org.uk>
In-Reply-To: <cb35f3f2-4dc9-4d56-96bd-bcffb33b7aaf@sirena.org.uk>
From:   =?UTF-8?Q?Pawe=C5=82_Anikiel?= <pan@semihalf.com>
Date:   Tue, 25 Apr 2023 18:02:20 +0200
Message-ID: <CAF9_jYSMieE=GP3T-gsXe+SDjuWKGVy9LjhMHKB9_hW7czuZmg@mail.gmail.com>
Subject: Re: [PATCH 5/9] ASoC: ssm2602: Add workaround for playback with
 external MCLK
To:     Mark Brown <broonie@kernel.org>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Apr 14, 2023 at 7:35=E2=80=AFPM Mark Brown <broonie@kernel.org> wro=
te:
>
> 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 "at the same time" part is done by writing multiple bits at once
to SSM2602_PWR. But before that, SSM2602_ACTIVE has to be set, and
then the chip is reset (SSM2602_RESET) to power everything down again.

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

Yes, I meant 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.

These are shell commands run from userspace (with no ssm2602 driver
present in the kernel). ssmset is a wrapper for the i2cset command:
ssmset() {
        i2cset -y 0 0x1a $(($1*2)) $2
}
I definitely should have made that more clear.

Do you think these logs are worth adding? If so, I'll improve the
explanation what these mean.

>
> > +     /* Workaround for what seems to be a hardware quirk: when using a=
n
> > +      * external MCLK signal, powering on Output and DAC for the first
> > +      * time produces output distortions unless they're powered togeth=
er
> > +      * with whole chip power. We power them here for the first time,
> > +      * as doing it later may be impossible (e.g. when starting playba=
ck
> > +      * 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.

Ok, I'll correct that.

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

I haven't considered that. I will look at regmap_register_patch() and
try to use it.

Regards,
Pawe=C5=82