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 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect sysclk calculation for SCLK mode due to using params_width() instead of params_physical_width() or snd_soc_params_to_bclk().
- [High] Component probe function ties devm_ allocations to the I2C device, causing memory and resource leaks on ASoC card unbind/rebind.
- [High] Regulators are enabled in component probe but never disabled in component remove, leading to regulator enable count leaks.
- [High] Silent failure when valid clock coefficients are not found.
--

commit 96650b0d78cc8e7757fca690cc1cc18f2d5796b3
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 9c816491648c7..7a9d5d42a05d0 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 isn't a bug introduced by this patch, but does this calculate the
sysclk correctly for SCLK mode?

It looks like params_width() returns the semantic sample width (e.g., 24
for S24_LE), rather than the physical slot width on the I2S bus
(typically 32 bits for 24-bit samples).

Could this result in an incorrectly calculated sysclk, causing get_coeff()
to fail or configure incorrect clock dividers? Should this use
params_physical_width() or snd_soc_params_to_bclk() instead?

Later in this same function:

sound/soc/codecs/es8389.c:es8389_pcm_hw_params() {
    ...
	coeff = get_coeff(es8389->vddd, dmic_enable, es8389->sysclk, params_rate(params));
	if (coeff >= 0) {
        ...
	} else {
		dev_warn(component->dev, "Clock coefficients do not match");
	}

	return 0;
}

[Severity: High]
This is a pre-existing issue, but does this safely handle errors?

If get_coeff() fails to find valid clock coefficients and returns -EINVAL,
the driver logs a warning but unconditionally returns 0 (success) to the
ALSA core.

Will this cause the audio framework to proceed with an unconfigured codec,
resulting in silent failures or garbled audio? Should it propagate the error
code instead?

[ ... ]
> @@ -897,11 +897,7 @@ static int es8389_probe(struct snd_soc_component *component)
>  	int ret, i;
>  	struct es8389_private *es8389 = snd_soc_component_get_drvdata(component);
>  
> -	ret = device_property_read_u8(component->dev, "everest,mclk-src", &es8389->mclk_src);
> -	if (ret != 0) {
> -		dev_dbg(component->dev, "mclk-src return %d", ret);
> -		es8389->mclk_src = ES8389_MCLK_SOURCE;
> -	}
> +	es8389->mclk_src = device_property_read_bool(component->dev, "everest,mclk-from-sclk");
>  
>  	for (i = 0; i < ARRAY_SIZE(es8389_core_supplies); i++)
>  		es8389->core_supply[i].supply = es8389_core_supplies[i];

[Severity: High]
This isn't a bug introduced by this patch, but does es8389_probe() leak
resources during ASoC card unbind/rebind?

This is an ASoC component probe callback (executed when the sound card
binds), not an I2C driver probe callback. However, later in this
function, it allocates devm resources tied to the I2C device:

sound/soc/codecs/es8389.c:es8389_probe() {
    ...
	ret = devm_regulator_bulk_get(component->dev, ARRAY_SIZE(es8389_core_supplies), es8389->core_supply);
    ...
	es8389->mclk = devm_clk_get_optional(component->dev, "mclk");
    ...
}

Since the I2C device's lifecycle spans beyond the sound card's lifecycle,
every time the sound card is unbound and rebound, won't es8389_probe()
execute again, allocating new regulators and clock handles on the I2C
device's devres list without freeing the previous ones?

Furthermore, this probe function enables the regulators:

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

[Severity: High]
This is a pre-existing issue, but does this leak the regulator enable count?

The corresponding es8389_remove() function does not call
regulator_bulk_disable():

sound/soc/codecs/es8389.c:es8389_remove() {
	struct es8389_private *es8389 = snd_soc_component_get_drvdata(component);

	regmap_write(es8389->regmap, ES8389_MASTER_MODE, 0x28);
	regmap_write(es8389->regmap, ES8389_HPSW, 0x00);
    ...
}

Every time the ASoC component is bound and then unbound, won't the
regulator's internal enable count be incremented but never decremented?
Could this prevent the regulator from ever being powered off by the system,
leading to increased power consumption?

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