Re: [PATCH 3/7] ASoC: codecs: ES8389: Modify the clock table

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The matching logic in `get_coeff()` incorrectly treats `dmic_sel == 0` and `dvdd_vol == 0` as wildcards, causing subsequent specific entries to be shadowed and become unreachable dead code.
- [High] Failure to find valid clock coefficients in the hardware parameters callback only logs a warning but incorrectly returns success, leading to invalid hardware states.
- [High] Changing the DAI format dynamically to Slave Mode (`SND_SOC_DAIFMT_CBC_CFC`) fails to clear the hardware Master Mode enable bit.
--

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

ASoC: codecs: ES8389: Modify the clock table

This patch modifies the ES8389 clock configuration table to update several
frequency settings and adds a new entry for the 192000Hz rate.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 7a9d5d42a05d0..6ad0b580068cf 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
> @@ -434,52 +434,54 @@ static const struct _coeff_div  coeff_div[] = {
[ ... ]
>  	{128, 24576000, 192000, 0x00, 0x50, 0x00, 0xC0, 0x18, 0xC1, 0x81, 0xC0, 0x00, 0x8F, 0x7F, 0xBF, 0xC0, 0x3F, 0x7F, 0x80, 0x12, 0xC0, 0x3F, 0xF9, 0x3F, 2, 2},
> +	{64, 12288000, 192000, 0x00, 0x41, 0x00, 0xC0, 0x18, 0xC1, 0x80, 0x00, 0x00, 0x8F, 0x7F, 0xEF, 0xC0, 0x7F, 0x7F, 0x80, 0x12, 0xC0, 0x3F, 0xF9, 0x3F, 1, 0},

[Severity: High]
This is a pre-existing issue, but is it possible this newly added entry
unintentionally acts as a wildcard due to dmic_sel=0?

In get_coeff(), wildcard matching is implemented using ~(val % 2) & 0x01. Since
0 % 2 == 0, dmic_sel=0 will evaluate to 1, causing this entry to match both 0
and 1 at runtime.

Will this cause subsequent exact-match entries with the same parameters to be
shadowed and unreachable?

>  };
>  
>  static inline int get_coeff(u8 vddd, u8 dmic, int mclk, int rate)

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

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;
}

Returning success to the ALSA framework despite failing to configure the
hardware clocks allows the stream to start in an unconfigured state. Can this
result in broken or silent audio?

[Severity: High]
This is a pre-existing issue, but could dynamically switching to Slave Mode in
es8389_set_dai_fmt() leave the hardware stuck in Master Mode?

sound/soc/codecs/es8389.c:es8389_set_dai_fmt() {
    ...
	case SND_SOC_DAIFMT_CBC_CFC:
		es8389->mastermode = 0;
		break;
    ...
}

While the driver updates its internal state to mastermode = 0, it appears to
forget to call regmap_update_bits() to clear the ES8389_MASTER_MODE_EN flag.

If a dynamic switch occurs, will the hardware continue to drive the bus clocks
and cause the audio interface to fail?

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