Re: [PATCH 2/7] ASoC: codecs: ES8389: Fix the issue about mclk_src

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect BCLK calculation for MCLK derivation
- [High] Missing check on the return value of regmap_read() during resume
- [High] Resource Leak: Regulators enabled during component probe are leaked on removal
--

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

ASoC: codecs: ES8389: Fix the issue about mclk_src

Fix the issue with incorrect modifications to mclk_src.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index be7a36f17083f..886069be1535f 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
[ ... ]
> @@ -607,9 +607,9 @@ static int es8389_pcm_hw_params(struct snd_pcm_substream *substream,
>  	regmap_update_bits(es8389->regmap, ES8389_ADC_FORMAT_MUTE, ES8389_DATA_LEN_MASK, state);
>  	regmap_update_bits(es8389->regmap, ES8389_DAC_FORMAT_MUTE, ES8389_DATA_LEN_MASK, state);
>  
> -	if (es8389->mclk_src == ES8389_SCLK_PIN) {
> +	if (es8389->mclk_src) {
>  		regmap_update_bits(es8389->regmap, ES8389_MASTER_CLK,
> -					ES8389_MCLK_SOURCE, es8389->mclk_src);
> +					ES8389_MCLK_MASK, ES8389_MCLK_FROM_SCLK);
>  		es8389->sysclk = params_channels(params) * params_width(params) * params_rate(params);
                                                           ^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does this incorrectly calculate the
physical BCLK frequency?

Using params_width() returns the sample data width instead of the physical
slot width on the wire. When mclk_src is set, es8389->sysclk acts as the MCLK
derived from SCLK.

By under-calculating the physical SCLK, the get_coeff() function is provided
with an incorrect MCLK frequency assumption, which can lead to the selection
of wrong internal clock dividers and distorted or missing audio.

Could the physical BCLK be calculated using snd_soc_params_to_bclk(params)
instead?

>  	}

Additionally, I noticed a few other pre-existing issues in this driver:

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing check on the
return value of regmap_read() during resume?

In es8389_resume():

static int es8389_resume(struct snd_soc_component *component)
{
	...
	unsigned int regv;

	regcache_cache_only(es8389->regmap, false);
	regcache_cache_bypass(es8389->regmap, true);
	regmap_read(es8389->regmap, ES8389_RESET, &regv);

	if (regv == 0xff)
	...
}

If the I2C read fails during resume, the stack variable regv is left
uninitialized and evaluated against 0xff. Could this introduce undefined
behavior and lead to incorrect skipping or triggering of the hardware
re-initialization?


[Severity: High]
This isn't a bug introduced by this patch, but are the regulators enabled
during probe leaked on removal?

es8389_probe() successfully enables core regulators:

static int es8389_probe(struct snd_soc_component *component)
{
	...
	ret = regulator_bulk_enable(ARRAY_SIZE(es8389_core_supplies),
				    es8389->core_supply);
	...
}

However, es8389_remove() does not call the corresponding
regulator_bulk_disable().

Since devm_regulator_bulk_get() only automatically frees the handles but does
not implicitly disable the regulators, this leaks the regulator enable
reference count. Could this cause the power domains to remain permanently on
even when the module is unbound or removed?

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