Date:   Wed, 1 Dec 2021 16:32:02 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Richard Fitzgerald <rf@opensource.cirrus.com>
Cc:     alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        patches@opensource.cirrus.com
Subject: Re: [PATCH] ASoC: cs42l42: Implement system suspend
Message-ID: <YaejghraYE6lD7FD@sirena.org.uk>
References: <20211201153648.17259-1-rf@opensource.cirrus.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="RkDed11whgknlEPT"
Content-Disposition: inline
In-Reply-To: <20211201153648.17259-1-rf@opensource.cirrus.com>
Precedence: bulk


--RkDed11whgknlEPT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
> Add system suspend functions to handle clean power-down on suspend and
> restoring state on resume.
>=20
> The jack state could change during suspend. Plug->unplug and unplug->plug
> are straightforward because this looks no different from any other plug
> state change. However, the jack could be unplugged and a different type

This fiddling about with the jack detect feels like it should be at
least one separate change, possibly multiple changes - the reporting of
button states feels like it should be a good cleanup/fix separately for
example.

> of jack plugged, and on resume the plug state would not have changed.
> Some code changes are needed to the jack handling so that on resume a
> plugged state will be re-evaluated instead of filtered out. In this case

It would be helpful to elaborate on what these code changes might be.

> +	/*
> +	 * PWR_CTL2 must be volatile so it can be used as a canary bit to
> +	 * detect a reset during system suspend.
> +	 */
> +	case CS42L42_PWR_CTL2:

This feels a bit hackish - is the cost of doing the cache sync really so
expensive that it's worth the effort of trying to skip it?

> +	if (cs42l42->suspended) {
> +		mutex_unlock(&cs42l42->irq_lock);
> +		return IRQ_NONE;
> +	}

Given that you're using disable_irq() to force the interrupt off (which
is a bit rude but often the best one can do) how might we be getting an
interrupt while suspended?  This seems to indicate an error condition.

>  			case CS42L42_PLUG_OMTP:
>  				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADSET,
> -						    SND_JACK_HEADSET);
> +						    SND_JACK_HEADSET |
> +						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> +						    SND_JACK_BTN_2 | SND_JACK_BTN_3);
>  				break;
>  			case CS42L42_PLUG_HEADPHONE:
>  				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADPHONE,
> -						    SND_JACK_HEADPHONE);
> +						    SND_JACK_HEADSET |
> +						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> +						    SND_JACK_BTN_2 | SND_JACK_BTN_3);

This unconditionally clears the button status - will something notice if
the buttons are pressed?

> +	} else {
> +		/*
> +		 * Only call regcache_mark_dirty() if cs42l42 reset, so
> +		 * regcache_sync() writes all non-default cached values.
> +		 * If it didn't reset we don't want to filter out syncing
> +		 * dirty cache entries that have default value.
> +		 * DISCHARGE_FILT=3D=3D1 after suspend. If the cs42l42 reset
> +		 * it will now be 0.
> +		 */

Something needs to tell regmap that the cache is dirty otherwise it
won't sync anything, including the non-default register values?  There's
currently an assumption coded in there that the cache is dirty because
the device was reset and everything has default values.

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

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmGno4IACgkQJNaLcl1U
h9DJ6Qf/YafozpTqOUoPhMpfJCM+TFh3s5pwYjW/9Owy6eXZR6F9a2b0z9CRQrG2
YoAmhZw/rXZ/2IvlvhVM8rl+Fy/uvQAbZECjWPmOjuiDcPPdbMHflKjs0frsTKz6
97yD2Q7nkyofOIsQtc0V+1XA4ndeUH1Z1Ky4J7bgaODtrgSVLQ6lyMHNIqq7AbX+
hMquuSUSTqJW6lhvIit2NHFEdY1AKlagS7wMw+Re9qGkzAGtdQc3AOXvwZybgrz/
p4B2sHnJ+sCI1jmgm5p/mDPmShrwx5W0Fl623KDMqLvRtPc725Kg97IFJ9skjjBf
mwxwhYHq8RvnKZpkmuqm3wXkhzjO2g==
=I0x7
-----END PGP SIGNATURE-----

--RkDed11whgknlEPT--