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:

New issues:
- [Medium] Newly added coefficient row for `mclk=12288000`, `rate=192000` is artificially restricted to a single voltage.

Pre-existing issues:
- [High] Modulo arithmetic bug in `get_coeff()` causes `0` to act as a wildcard, rendering specific configuration rows unreachable.
- [High] Silent failure in `es8389_pcm_hw_params()` when clock coefficients are not found.
--

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

    ASoC: codecs: ES8389: Modify the clock table
    
    Updated the configuration for certain frequencies

> 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: Medium]
Is it intentional to restrict this new row to dvdd_vol=1 and dmic_sel=0, rather
than using the wildcard values (2, 2) like the other 192000Hz row?

According to the logic in get_coeff(), dvdd_vol=1 strictly requires vddd=1
(ES8389_3V3). If the device is running on 1.8V (vddd=0), get_coeff() will
reject this row and return -EINVAL. Could this break 192kHz playback on 1.8V
systems?

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

[Severity: High]
This is a pre-existing issue, but does the bitwise and modulo arithmetic here
unintentionally cause 0 to act as a wildcard?

In get_coeff():

	vddd_det |= ~(coeff_div[i].dvdd_vol % 2) & 0x01;
	dmic_det |= ~(coeff_div[i].dmic_sel % 2) & 0x01;

Because 0 % 2 == 0, the expression ~0 & 1 evaluates to 1. This means any row
with dmic_sel == 0 or dvdd_vol == 0 acts as a wildcard and matches regardless
of the actual requested dmic or vddd values.

For configurations where a 0 row precedes a 1 row for the same frequency, like:

	{50, 400000, 8000, ..., 2, 0},
	{50, 400000, 8000, ..., 2, 1},

get_coeff() will always return the first row (the 0 configuration), making the
second row completely unreachable and preventing correct hardware setup when
dmic=1 is requested.

[Severity: High]
This is also a pre-existing issue, but when get_coeff() fails to find a
matching row and returns -EINVAL, es8389_pcm_hw_params() does not propagate the
error:

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

	return 0;

Returning 0 causes ALSA to continue preparing and triggering the PCM stream
without configuring the codec hardware properly. Could this lead to undefined
behavior or silent audio failures?

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