Re: [PATCH v2 6/7] ASoC: codecs: ES8389: Add private members about HPF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The fallback logic for a missing `everest,hpf-frq` DT property is broken, causing an integer overflow and an impossible `u8` to `U32_MAX` comparison.
- [High] The ADC High-Pass Filter parameters `hpfl` and `hpfr` are used without being initialized during probe, leading to incorrect hardware configuration.
- [High] Playback stream operations silently corrupt the ADC (capture) High-Pass Filter state due to missing stream direction checks.
- [Medium] The newly introduced device tree property `everest,hpf-frq` contains a typo ('frq' instead of 'freq').
--

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

ASoC: codecs: ES8389: Add private members about HPF

This commit adds HPF (High-Pass Filter) related private members to the
ES8389 codec. It includes a helper function to match the set HPF frequency
and accesses the HPF frequency set by the user from the Device Tree.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 09257687bb84c..bcec9d9200dbe 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
[ ... ]
> @@ -504,6 +507,48 @@ static inline int get_coeff(u8 vddd, u8 dmic, int mclk, int rate)
[ ... ]
> +static bool find_best_hpf_freq(u32 target_hz, u8 *hpf1, u8 *hpf2)
> +{
> +	int best_row = -1, best_col = -1;
> +	u32 min_diff = U32_MAX;
> +	u32 f, diff;
> +	int i, j;
> +
> +	if ((*hpf1 == ES8389_HPF_INVALID) | (*hpf2 == ES8389_HPF_INVALID))

[Severity: High]
Does this code correctly evaluate the fallback condition?

hpf1 and hpf2 are pointers to u8. Dereferencing them yields a maximum value of
255, so comparing them against ES8389_HPF_INVALID (which is U32_MAX) will
always evaluate to false. This prevents the fallback path from ever executing.

> +		return false;
> +
> +	for (i = 0; i < 10; i++) {
> +		for (j = i; j < 10; j++) {
> +			f = hpf_table[i][j];
[ ... ]
> @@ -663,6 +710,14 @@ static int es8389_pcm_hw_params(struct snd_pcm_substream *substream,
>  		return -EINVAL;
>  	}
>  
> +	freq = (es8389->target_freq * 48000) / params_rate(params);

[Severity: High]
If the Device Tree property is missing, target_freq is set to U32_MAX.
Could this cause an integer overflow here, resulting in a garbage freq value?

Additionally, since the High-Pass Filter is an ADC (Capture) specific feature,
does this code safely avoid overwriting the ADC configuration during playback
stream operations?

If es8389_pcm_hw_params() is called for a playback stream, it appears it
will unconditionally recalculate the capture stream's filter configuration
using the playback rate. Could this update logic be restricted by checking
the stream direction?

> +	hpf = find_best_hpf_freq(freq, &es8389->hpfl, &es8389->hpfr);
> +	if (!hpf) {
> +		dev_dbg(component->dev, "Can't find best freq. Use the default setting");
> +		es8389->hpfl = ES8389_HPF_DEFAULT;
> +		es8389->hpfr = ES8389_HPF_DEFAULT;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -743,8 +798,8 @@ static int es8389_mute(struct snd_soc_dai *dai, int mute, int direction)
>  			regmap_update_bits(es8389->regmap, ES8389_DAC_FORMAT_MUTE,
>  						0x03, 0x00);
>  		} else {
> -			regmap_update_bits(es8389->regmap, ES8389_ADC_HPF1, 0x0f, 0x0a);
> -			regmap_update_bits(es8389->regmap, ES8389_ADC_HPF2, 0x0f, 0x0a);
> +			regmap_update_bits(es8389->regmap, ES8389_ADC_HPF1, 0x0f, es8389->hpfl);
> +			regmap_update_bits(es8389->regmap, ES8389_ADC_HPF2, 0x0f, es8389->hpfr);

[Severity: High]
Are hpfl and hpfr properly initialized before they are used here?

Since they are dynamically allocated and zero-initialized, if es8389_mute()
is called prior to es8389_pcm_hw_params() (for instance during stream startup
or idle states), this will write uninitialized 0x00 values to the hardware
registers instead of the previously hardcoded 0x0A default. 

Should they be explicitly initialized in es8389_probe()?

>  			regmap_update_bits(es8389->regmap, ES8389_ADC_FORMAT_MUTE,
>  						0x03, 0x00);
>  		}
> @@ -906,6 +961,12 @@ static int es8389_probe(struct snd_soc_component *component)
>  		es8389->mclk_src = ES8389_MCLK_SOURCE;
>  	}
>  
> +	ret = device_property_read_u32(component->dev, "everest,hpf-frq", &es8389->target_freq);

[Severity: Medium]
This isn't a bug, but there's a typo in the newly introduced device tree
property name ("frq" instead of "freq"). Since Device Tree bindings become
permanent ABI, could this be corrected before it becomes baked into the kernel?

> +	if (ret != 0) {
> +		dev_dbg(component->dev, "hpf-freq return %d\n", ret);
> +		es8389->target_freq = ES8389_HPF_INVALID;
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(es8389_core_supplies); i++)
>  		es8389->core_supply[i].supply = es8389_core_supplies[i];

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