Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Ryan Lee <RyanS.Lee@maximintegrated.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Mark Brown <broonie@kernel.org>
CC:     "guennadi.liakhovetski@linux.intel.com" 
        <guennadi.liakhovetski@linux.intel.com>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "ryan.lee.maxim@gmail.com" <ryan.lee.maxim@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "tiwai@suse.com" <tiwai@suse.com>,
        "lgirdwood@gmail.com" <lgirdwood@gmail.com>,
        "sathya.prakash.m.r@intel.com" <sathya.prakash.m.r@intel.com>,
        "yung-chuan.liao@linux.intel.com" <yung-chuan.liao@linux.intel.com>
Subject: RE: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty before
 entering sleep
Thread-Topic: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty before
 entering sleep
Thread-Index: AQHXsZF45/jZbOMADUKIjx+r35kUIKu3/BEAgAAPiFCAAAR+AIAAC+UAgAAGCoCAAAOCAIABhRvA
Date:   Tue, 28 Sep 2021 16:43:19 +0000
Message-ID: <SJ0PR11MB56618B8B6DE3922613E83C15E7A89@SJ0PR11MB5661.namprd11.prod.outlook.com>
References: <20210924221305.17886-1-ryans.lee@maximintegrated.com>
 <1b21bbf1-12c7-726d-bff8-76ec88ff8635@linux.intel.com>
 <SJ0PR11MB566107A6AB3D18ABDEDCF245E7A79@SJ0PR11MB5661.namprd11.prod.outlook.com>
 <20210927160622.GE4199@sirena.org.uk>
 <7b8c3875-3f12-f3cb-7da8-4e850e59ee2b@linux.intel.com>
 <20210927171033.GF4199@sirena.org.uk>
 <0af258d4-e33c-15ec-5dcc-a1c9961c0740@linux.intel.com>
In-Reply-To: <0af258d4-e33c-15ec-5dcc-a1c9961c0740@linux.intel.com>
Accept-Language: en-US, ko-KR
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SAp5/CgYJyb+MltA4dsBO0DbW28FTaowRRwYMG9L7QNcAKi6Ij0gzJaAqcUA?=
 =?us-ascii?Q?I0EeUJwBTZnudCpHpgaQeyIJqZxKhqGHoR/2CPKBQ/MqVDqXL/OQgX3EySO2?=
 =?us-ascii?Q?u+z0Lz6GsADnmMLMnF7/3fayS9aocYXm3Xd9qGdwe+7n9Uywr8Rpf2RQ69Ca?=
 =?us-ascii?Q?hJOQBQqtptSGEW5rOWmUYlUsQp2DvsJagSqxurYBeDJnLSTaIPl5gwCm9cj8?=
 =?us-ascii?Q?HM+DLt/O6WWBPo/kvM6iYUV1Z6l1pQPVVKUDlsYndlW0OJe+15BylLHb0vA/?=
 =?us-ascii?Q?FXdKeK62q0g8tZOEjoHvhFbK1Gus0x27m5Awl3KZ5ndROhH51sRP3xjPnReP?=
 =?us-ascii?Q?ZIehu+X5Rd+p3PadZN/SnvpWa69dtyf4SKFSSdFbD0MlZxcu439GEu7+6Gtv?=
 =?us-ascii?Q?RiPsZ7AsqeeAb/QSZn7IFa3kX2Np67R0R85p64r980hP0YLGanlWQPXP7fD8?=
 =?us-ascii?Q?gurp+NdOGaZMu96sYrP7xRskxIdi1TgyesAUohlo4udBhgh4FwwKZy/RZFd9?=
 =?us-ascii?Q?xCpYCeU81N3v758cXg328YtJ1BIGSzKer9+RwuwXxXa/wgX/0Ig5/WM6mzOz?=
 =?us-ascii?Q?FjnwjnxReG+TWfbM4/nHfSPVrbh/5OPLUU30J+fG/w2v5jq4B0Zy8WlfxNfp?=
 =?us-ascii?Q?QjMd6IRzBk2ylFZEudzYfTeNN1WNLFVbi7GKoAxU8hwRvBJPxw5hqVPvdVW5?=
 =?us-ascii?Q?060AXFbZlWRA5oys/edyuIHb631eo8zyW1CCJUKeOy6+0z3uHZYRKkyIu1N4?=
 =?us-ascii?Q?tpBw+Us4lT2yVtY1JVa9rhwYSlZyYOs9H6sl2Y95ewqe9ZutFDNjEu6Cs3CC?=
 =?us-ascii?Q?RDx4Im7XeFHr7cN++Ltj8e7bvkHg7WpZe1SqDBVBUnfNqip+ASdZ99Sk4nR6?=
 =?us-ascii?Q?wUVHrfFNotgouY/nQ9S+tN5GOQssoc6mQYSYR1wlL6aRJrT7PjbwExyE/laT?=
 =?us-ascii?Q?Pl0RGEpdUqhJj0brlLnZ/acEvLWj75yPJEmDGt6UNwUqlMRrwsRd43M27EIq?=
 =?us-ascii?Q?04XqbY5YXmVBbfOFjHQImCxgvRo7RTNiLqvypaQA6p2M/rYreS2GqjwOiyI+?=
 =?us-ascii?Q?pEYrUF8dHCSRrDPch8jtAn8oaoCMSQjf5wi5piceJzEDKZrlRgUSlrYfxdCn?=
 =?us-ascii?Q?7jHy6HrNti0MoaQCxfKMn8FxsMM1FSAmE9cp3UtRpB9Xh98PuoJmgcJe7rLT?=
 =?us-ascii?Q?B28txhmAcMFpIGsbanA3BQl3UT2mY2Y6yAth7HLMxFIBavneelflDuwElhJT?=
 =?us-ascii?Q?us+XTOj5Z2usK2rrz14EwBo2MHyCrDkCb6z5x8h8XGUBZF76T7LpG/6q194g?=
 =?us-ascii?Q?q7OX8+2On5Ztv1+pL7ijrTgJ?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: maximintegrated.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR11MB5661.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ec6f30c-2fb1-4ae8-dddd-08d9829f1404
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Sep 2021 16:43:19.4888
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: fbd909df-ea69-4788-a554-f24b7854ad03
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 3DbnM8OeDmGMqwpGKTH/X8a/9gnL/8jj42BA0lIgK8ovAKvPHayZKdhQH7H1wNO9gBMIqpeUDCpolsrcZFwFvLGZfRC/ylgVMFThbDDvI6o=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5600
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Monday, September 27, 2021 10:23 AM
> To: Mark Brown <broonie@kernel.org>
> Cc: guennadi.liakhovetski@linux.intel.com; alsa-devel@alsa-project.org;
> ryan.lee.maxim@gmail.com; Ryan Lee <RyanS.Lee@maximintegrated.com>;
> linux-kernel@vger.kernel.org; tiwai@suse.com; lgirdwood@gmail.com;
> sathya.prakash.m.r@intel.com; yung-chuan.liao@linux.intel.com
> Subject: Re: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty
> before entering sleep
>=20
> EXTERNAL EMAIL
>=20
>=20
>=20
> On 9/27/21 12:10 PM, Mark Brown wrote:
> > On Mon, Sep 27, 2021 at 11:48:56AM -0500, Pierre-Louis Bossart wrote:
> >> On 9/27/21 11:06 AM, Mark Brown wrote:
> >
> >>> More specifically what it does is make the invalidation of the
> >>> register cache unconditional.  It doesn't really matter if the
> >>> invalidation is done on suspend or resume, so long as it happens
> >>> before we attempt to resync - this could also be done by deleting the
> first_hw_init check.
> >
> >> Mark, that's exactly my point: if the amp rejoins the bus, we will
> >> *always* mark the cache as dirty, before the resync is done in the
> >> resume sequence.
> >
> > Ah, yes - I see.
> >
> >> I am really trying to figure out if we have a major flaw in the
> >> resume sequence and why things are different in the case of the Maxim
> amp.
> >
> >> Instead of changing the suspend sequence, can we please try to modify
> >> the max98373_io_init() routine to unconditionally flag the cache as
> >> dirty, maybe this points to a problem with the management of the
> >> max98373->first_hw_init flag.
> >
> > A quick survey of other drivers suggests that this pattern should be
> > factored out into some helpers as it looks like there's several ways
> > of implementing it that look very similar but not quite the same...
>=20
> No disagreement here, we tried really hard to enforce a common pattern fo=
r
> suspend-resume, but i just noticed that the maxim amp driver is different=
 on
> suspend (resume is consistent with the rest).

OK. I believe it was similar before. But it looks like 'regcache_mark_dirty=
' is being
disappeared on suspend function.

static int __maybe_unused rt5682_dev_suspend(struct device *dev)
{
	struct rt5682_priv *rt5682 =3D dev_get_drvdata(dev);

	if (!rt5682->hw_init)
		return 0;

	cancel_delayed_work_sync(&rt5682->jack_detect_work);

	regcache_cache_only(rt5682->regmap, true);
	regcache_mark_dirty(rt5682->regmap);

	return 0;
}

>=20
>=20
> static int __maybe_unused rt711_dev_suspend(struct device *dev) {
>         struct rt711_priv *rt711 =3D dev_get_drvdata(dev);
>=20
>         if (!rt711->hw_init)
>                 return 0;
>=20
>         cancel_delayed_work_sync(&rt711->jack_detect_work);
>         cancel_delayed_work_sync(&rt711->jack_btn_check_work);
>         cancel_work_sync(&rt711->calibration_work);
>=20
>         regcache_cache_only(rt711->regmap, true);
>=20
>         return 0;
> }
>=20
> static int __maybe_unused rt1308_dev_suspend(struct device *dev) {
>         struct rt1308_sdw_priv *rt1308 =3D dev_get_drvdata(dev);
>=20
>         if (!rt1308->hw_init)
>                 return 0;
>=20
>         regcache_cache_only(rt1308->regmap, true);
>=20
>         return 0;
> }
>=20
> static __maybe_unused int max98373_suspend(struct device *dev) {
>         struct max98373_priv *max98373 =3D dev_get_drvdata(dev);
>         int i;
>=20
> <<<< missing test
>=20
>         /* cache feedback register values before suspend */
>         for (i =3D 0; i < max98373->cache_num; i++)
>                 regmap_read(max98373->regmap, max98373->cache[i].reg,
> &max98373->cache[i].val);
>=20
> <<<< why is this needed???
[]=20
It looks like this was added to get a last ADC values when ADC value read i=
s not available during suspension.
https://www.spinics.net/lists/alsa-devel/msg119808.html

>=20
>         regcache_cache_only(max98373->regmap, true);
>=20
>         return 0;
> }
>=20
>=20