Re: [PATCH v0 4/7] ASoC: codecs: ES8389: Modify the initial configuration

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Resource leaks due to missing cleanup in component remove callback.
- [High] Uninitialized stack variables are used due to missing `regmap_read` error handling.
- [High] Hardware sequencing violation: clock is enabled before power supplies.
- [High] Hardware sequencing violation and mclk enablement bypass during resume.
--

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

ASoC: codecs: ES8389: Modify the initial configuration

This commit modifies the initial configuration for the ES8389 codec.
It changes the values written to the ES8389_ISO_CTL and ES8389_OSC_CLK
registers during the initialization sequence.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index cd8284da8c62c..ad016c255c773 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
> @@ -792,7 +792,7 @@ static void es8389_init(struct snd_soc_component *component)
>  
>  	regmap_read(es8389->regmap, ES8389_MAX_REGISTER, &reg);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that regmap_read()
is called without checking the return value. If the I2C read fails, the
stack variable reg remains uninitialized.

Does this code unintentionally use an uninitialized value for es8389->version
and in other functions like es8389_pcm_hw_params() or es8389_mute()?

For example, in es8389_resume():

sound/soc/codecs/es8389.c:es8389_resume() {
...
	regmap_read(es8389->regmap, ES8389_RESET, &regv);
	if (regv == 0xff)
...
}

If the read fails, regv could be uninitialized stack garbage, potentially
causing it to take the 0xff branch incorrectly.

>  	es8389->version = reg;
> -	regmap_write(es8389->regmap, ES8389_ISO_CTL, 0x00);
> +	regmap_write(es8389->regmap, ES8389_ISO_CTL, 0x56);
>  	regmap_write(es8389->regmap, ES8389_RESET, 0x7E);

[ ... ]

> @@ -844,7 +844,7 @@ static void es8389_init(struct snd_soc_component *component)
>  	regmap_write(es8389->regmap, ES8389_SCLK_DIV, 0x04);
>  	regmap_write(es8389->regmap, ES8389_LRCK_DIV1, 0x01);
>  	regmap_write(es8389->regmap, ES8389_LRCK_DIV2, 0x00);
> -	regmap_write(es8389->regmap, ES8389_OSC_CLK, 0x00);
> +	regmap_write(es8389->regmap, ES8389_OSC_CLK, 0x10);
>  	regmap_write(es8389->regmap, ES8389_ADC_OSR, 0x1F);

[Severity: High]
This isn't a bug introduced by this patch, but looking at the component
remove callback, is it possible that we are leaking the clock and regulator
resources?

In es8389_probe(), the clock and regulators are enabled:

sound/soc/codecs/es8389.c:es8389_probe() {
...
	ret = clk_prepare_enable(es8389->mclk);
...
	ret = regulator_bulk_enable(ARRAY_SIZE(es8389_core_supplies),
				    es8389->core_supply);
...
}

However, es8389_remove() does not seem to call clk_disable_unprepare() or
regulator_bulk_disable(). Could this permanently leak these resources every
time the sound card is bound and unbound?


[Severity: High]
This is a pre-existing issue, but does the power sequencing in es8389_probe()
follow standard hardware requirements?

sound/soc/codecs/es8389.c:es8389_probe() {
...
	ret = clk_prepare_enable(es8389->mclk);
...
	ret = regulator_bulk_enable(ARRAY_SIZE(es8389_core_supplies),
				    es8389->core_supply);
...
}

The clock is enabled before the core power supplies are enabled. Can driving a
clock signal into an unpowered IC draw power through the clock pin's ESD
protection diodes, potentially causing unpredictable logic states?


[Severity: High]
This is another pre-existing issue, but could we bypass the master clock
enablement during resume?

sound/soc/codecs/es8389.c:es8389_resume() {
...
	regmap_read(es8389->regmap, ES8389_RESET, &regv);
	if (regv == 0xff)
		es8389_init(component);
	else
		es8389_set_bias_level(component, SND_SOC_BIAS_ON);
...
}

During es8389_suspend(), the bias level drops to STANDBY, disabling the
master clock. In es8389_resume(), if the register read returns 0xff,
es8389_init() is called instead of es8389_set_bias_level(..., SND_SOC_BIAS_ON).

Does this result in writing to the codec registers without the master clock
enabled, and leaving the master clock permanently disabled after the resume
completes?

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