Re: [PATCH v2 2/7] ASoC: codecs: ES8389: Modify volatile_register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The updated `es8389_volatile_register` list misses critical configuration registers, causing their state to be permanently lost upon resume.
- [High] Variables passed to `regmap_read` are used without checking for a potential error return, which can leave them uninitialized and result in non-deterministic hardware behavior.
- [High] The return value of `es8389_set_bias_level()` is ignored in `es8389_resume()`, potentially leading to silent failures.
--

commit d39c101472412561134bb4faff5fe29c9d7f7dfb
Author: Zhang Yi <zhangyi@everest-semi.com>

ASoC: codecs: ES8389: Modify volatile_register

This commit updates the volatile register definitions for the ES8389 codec,
switching to an allowlist approach for cached registers. It also adjusts
the cache bypass logic during the resume sequence.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 3484c87853cb4..10de2143f8d8f 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c

[ ... ]

> @@ -50,10 +50,29 @@ static const char * const es8389_core_supplies[] = {
>  static bool es8389_volatile_register(struct device *dev,
>  			unsigned int reg)
>  {
> -	if ((reg  <= 0xff))
> -		return true;
> -	else
> +	switch (reg) {
> +	case ES8389_ADCL_VOL:

[ ... ]

> +	case ES8389_ADC_MODE:
> +	case ES8389_DMIC_EN:
>  		return false;
> +	default:
> +		return true;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but by defaulting to true, do we
miss critical configuration registers like ES8389_MASTER_MODE and
ES8389_ADC_HPF2?

If these are left as volatile, they are bypassed during regcache_sync().
Since ASoC doesn't automatically re-invoke set_fmt on resume, and
es8389_init() overwrites these registers with reset defaults, does this
permanently lose the configuration across a suspend/resume cycle?

>  }
>  
>  static const DECLARE_TLV_DB_SCALE(dac_vol_tlv, -9550, 50, 0);
> @@ -861,13 +880,13 @@ static int es8389_resume(struct snd_soc_component *component)
>  	regcache_cache_only(es8389->regmap, false);
>  	regcache_cache_bypass(es8389->regmap, true);
>  	regmap_read(es8389->regmap, ES8389_RESET, &regv);

[Severity: High]
This is a pre-existing issue, but should the return value of regmap_read()
be checked here?

If the underlying I2C/SPI read fails, regv remains uninitialized. This
could result in non-deterministic hardware behavior since the resume
sequence would evaluate stack garbage in the if (regv == 0xff) check below.

This also appears in other places, for example in es8389_init():

    regmap_read(es8389->regmap, ES8389_MAX_REGISTER, &reg);
    es8389->version = reg;

> -	regcache_cache_bypass(es8389->regmap, false);
>  
>  	if (regv == 0xff)
>  		es8389_init(component);
>  	else
>  		es8389_set_bias_level(component, SND_SOC_BIAS_ON);

[Severity: High]
This is also a pre-existing issue, but what happens if es8389_set_bias_level()
encounters an error?

If clk_prepare_enable() fails inside es8389_set_bias_level(), it returns
an error code. Since that return value is ignored here, will the function
silently proceed to regcache_sync() and return success? This might leave
the hardware unpowered while the system falsely believes it resumed
correctly.

>  
> +	regcache_cache_bypass(es8389->regmap, false);
>  	regcache_sync(es8389->regmap);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618082842.2778-1-zhangyi@everest-semi.com?part=2