Re: [PATCH v2 7/9] ASoC: wm8978: restore safe state after resume errors

[Charles Keepax]

On Wed, Oct 14, 2026 at 11:24:50AM +0800, Pengpeng Hou wrote:
> wm8978_resume() ignores cache replay, bias restoration and PLL update
> results, allowing subsequent resume steps to run after a failed operation.
> 
> Stop at the first failure and request SND_SOC_BIAS_OFF on the error path.
> Bias OFF intentionally preserves PLL power, so also clear the power
> register as system suspend does. Keep the original failure if rollback
> reports another error. Restore cache-only mode and mark the cache dirty
> so later accesses do not write through a partially restored device.
> Re-enable write-through before a later replay attempt. A positive
> update_bits() result means that
> bits changed, not an error.
> 
> The bias callback currently does not propagate every register write error;
> the rollback is therefore best-effort, not a claim of verified hardware
> power-down. The ASoC wrapper continues its established best-effort resume.
> 
> The issue was found by our static-analysis tool and manually reviewed.
> 
> Fixes: 0d34e91596ef ("ASoC: add a WM8978 codec driver")
> Assisted-by: gpt 5
> Signed-off-by: Pengpeng Hou <hppiscas@xxxxxxx>
> ---
> Changes since v1:
> - restore cache-only mode and dirty state on replay failure
> - restore bias OFF on the error path, as Charles requested

I was more interested in a rational for either approach. But
probably adding it seems more logical to me.

> - also turn off PLL power, matching system suspend rather than bias OFF alone
> - preserve positive update_bits() success
> - send as one correctly threaded series, as Mark requested
> 
> Previous series:
> https://lore.kernel.org/all/20260906034321.86080-1-hppiscas@xxxxxxx/
> 
>  sound/soc/codecs/wm8978.c |   31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c
> index 258bc9f88f520e4de034393d39317e07e731d1ad..f5d3f26cc21790a8dfc4bf909a5fc5d56ba8f577 100644
> --- a/sound/soc/codecs/wm8978.c
> +++ b/sound/soc/codecs/wm8978.c
> @@ -952,17 +952,40 @@ static int wm8978_resume(struct snd_soc_component *component)
>  {
>  	struct wm8978_priv *wm8978 = snd_soc_component_get_drvdata(component);
>  	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(component);
> +	int ret, err;
>  
>  	/* Sync reg_cache with the hardware */
> -	regcache_sync(wm8978->regmap);
> +	regcache_cache_only(wm8978->regmap, false);

Nothing ever set cache_only true? Surely you either want a
separate patch to add cache_only support, or to not do this.
Although given the driver appears to only support system suspend
I would assume there isn't much point in cache only.

Thanks,
Charles