--- Begin Message ---
- To: Mark Brown <broonie@xxxxxxxxxx>
- Subject: Re: [PATCH 5/9] ASoC: ssm2602: Add workaround for playback with external MCLK
- From: Paweł Anikiel <pan@xxxxxxxxxxxx>
- Date: Tue, 25 Apr 2023 18:02:20 +0200
- Cc: alsa-devel@xxxxxxxxxxxxxxxx, devicetree@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, lgirdwood@xxxxxxxxx, tiwai@xxxxxxxx, robh+dt@xxxxxxxxxx, krzysztof.kozlowski+dt@xxxxxxxxxx, dinguyen@xxxxxxxxxx, lars@xxxxxxxxxx, nuno.sa@xxxxxxxxxx, upstream@xxxxxxxxxxxx
- In-reply-to: <cb35f3f2-4dc9-4d56-96bd-bcffb33b7aaf@sirena.org.uk>
- References: <20230414140203.707729-1-pan@semihalf.com> <20230414140203.707729-6-pan@semihalf.com> <cb35f3f2-4dc9-4d56-96bd-bcffb33b7aaf@sirena.org.uk>
On Fri, Apr 14, 2023 at 7:35 PM Mark Brown <broonie@xxxxxxxxxx> wrote: > > 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 "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 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. 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ł
--- End Message ---
