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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Modifying device tree property interpretation breaks existing configurations.

Pre-existing issues:
- [High] Incorrect BCLK (sysclk) calculation for mono streams.
--

commit 97d350d02faf890bb7862a3fb657b9cbe4613b33
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.
When the system needs to be configured to use the MCLK from the SCLK pin,
the code still sets the relevant registers to use the MCLK from the MCLK
pin.

> diff --git a/sound/soc/codecs/es8389.h b/sound/soc/codecs/es8389.h
> --- a/sound/soc/codecs/es8389.h
> +++ b/sound/soc/codecs/es8389.h
[ ... ]
> @@ -116,9 +116,11 @@
>  #define ES8389_TDM_SLOT               (0x70 << 0)
>  #define ES8389_TDM_SHIFT              4
>  
> -#define ES8389_MCLK_SOURCE            (1 << 6)
> -#define ES8389_MCLK_PIN               (1 << 6)
> -#define ES8389_SCLK_PIN               (0 << 6)
> +#define ES8389_MCLK_MASK              (3 << 6)
> +#define ES8389_MCLK_FROM_SCLK         (1 << 6)
> +#define ES8389_MCLK_SOURCE            ES8389_MCLK_PIN
> +#define ES8389_MCLK_PIN               0
> +#define ES8389_SCLK_PIN               1

[ ... ]

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> --- 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) {

[Severity: Critical]
Does changing this condition and redefining the pin macros break existing
device tree configurations?

Previously in es8389.h, ES8389_SCLK_PIN was 0. If an existing device tree
explicitly selected SCLK using <0>, it would match the old condition.
Because ES8389_SCLK_PIN is now redefined as 1, an existing legacy DT passing
<0> will evaluate as false here and incorrectly fall back to MCLK.

Similarly, if a legacy configuration used <64> for MCLK, it would now
evaluate as true and incorrectly select SCLK. Is there a need for
compatibility fallback logic to prevent ABI breakage for deployed platforms?

>  		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 result in an
incorrect sysclk for mono streams?

By using params_channels() in es8389_pcm_hw_params(), the physical clock
rate is derived from the logical channel count. For a mono stream, this
returns 1. However, on standard I2S buses, the physical frame always
contains 2 slots (left and right), meaning the actual physical bit clock on
the wire should be 2 * width * rate.

Could this cause the sysclk calculation to be halved, making it fail to
find a matching frequency and breaking mono playback?

>  	}

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