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 (SCLK) frequency calculation when using SCLK as MCLK.
- [High] es8389_set_dai_fmt fails to clear the master mode bit when transitioning to slave mode.
- [High] es8389_pcm_hw_params ignores the error from get_coeff() and returns 0 (success).
--

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

ASoC: codecs: ES8389: Fix the issue about mclk_src

This commit fixes an issue with incorrect modifications to the mclk_src
variable. It changes mclk_src to a boolean and reads it using
device_property_read_bool, simplifying the MCLK configuration logic.

> 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 is a pre-existing issue, but does this calculation yield the correct
physical BCLK rate?

Using params_width() returns the logical sample width (e.g., 24 for S24_LE)
instead of the physical wire width, and this ignores TDM slot configurations.

This could result in calculating a lower SCLK frequency than what is
physically driven on the bus, leading to an incorrect sysclk and the wrong
internal clock dividers.

Should this use snd_soc_params_to_bclk(params) instead?


Also in sound/soc/codecs/es8389.c:es8389_set_dai_fmt():

	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
	case SND_SOC_DAIFMT_CBC_CFP:
		regmap_update_bits(es8389->regmap, ES8389_MASTER_MODE,
				ES8389_MASTER_MODE_EN, ES8389_MASTER_MODE_EN);
		es8389->mastermode = 1;
		break;
	case SND_SOC_DAIFMT_CBC_CFC:
		es8389->mastermode = 0;
		break;

[Severity: High]
This is a pre-existing issue, but does the transition to slave mode properly
clear the hardware state?

When SND_SOC_DAIFMT_CBC_CFC is requested, the internal mastermode variable is
updated, but it appears ES8389_MASTER_MODE_EN is not cleared in the
ES8389_MASTER_MODE register. If the codec was previously set to master mode,
could this leave it incorrectly operating as a clock provider and cause bus
contention?


Later in sound/soc/codecs/es8389.c:es8389_pcm_hw_params():

	coeff = get_coeff(es8389->vddd, dmic_enable, es8389->sysclk, params_rate(params));
	if (coeff >= 0) {
		...
		regmap_write(es8389->regmap, ES8389_SYSTEM19, coeff_div[coeff].Reg0x19);
	} else {
		dev_warn(component->dev, "Clock coefficients do not match");
	}

	return 0;

[Severity: High]
This is a pre-existing issue, but should this function return an error code
when get_coeff() fails?

Currently, if get_coeff() fails to find a matching configuration, it prints a
warning but unconditionally returns 0. Could this cause the ALSA core to
assume the hardware is correctly configured and proceed to start the stream,
potentially resulting in silent or distorted audio?

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