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