Re: [PATCH 5/7] ASoC: codecs: ES8389: Add private members related to HPF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ADC High Pass Filter (HPF) configurations are exposed as devicetree properties rather than ALSA kcontrols, violating ASoC subsystem conventions.
- [Medium] Using `device_property_read_u8()` for a scalar devicetree property creates a silent fallback trap if standard 32-bit devicetree cells are used.
--

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

ASoC: codecs: ES8389: Add private members related to HPF

This commit adds private members to manage the left and right High Pass
Filter (HPF) values on the ES8389 codec. It reads these configurations from
devicetree properties during probe and applies them in the un-mute sequence.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 284eb707f9f7f..2460cf9775c75 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c

[ ... ]

> @@ -902,6 +904,18 @@ static int es8389_probe(struct snd_soc_component *component)
>  
>  	es8389->mclk_src = device_property_read_bool(component->dev, "everest,mclk-from-sclk");
>  
> +	ret = device_property_read_u8(component->dev, "everest,hpfl", &es8389->hpfl);

[Severity: Medium]
Devicetree is typically reserved for describing hardware topology, while
software-configurable tuning parameters like an ADC High Pass Filter are
usually exposed as ALSA kcontrols. Should these HPF values be registered as
mixer controls instead of devicetree properties so userspace can dynamically
adjust them at runtime?

[Severity: Medium]
Could using device_property_read_u8() here lead to a silent misconfiguration
if standard 32-bit devicetree cells are used?

If a devicetree specifies the property as a standard cell, for example:
    everest,hpfl = <10>;

The read_u8 function will read the first zero-padded byte (0x00) and return
success instead of an error. This means the fallback to ES8389_HPF_DEFAULT
will be bypassed, and the hardware will be configured with 0 instead. Would
it be safer to read this as a u32 using device_property_read_u32() and then
cast or bounds-check the result?

> +	if (ret != 0) {
> +		dev_dbg(component->dev, "hpfl return %d\n", ret);
> +		es8389->hpfl = ES8389_HPF_DEFAULT;
> +	}
> +
> +	ret = device_property_read_u8(component->dev, "everest,hpfr", &es8389->hpfr);
> +	if (ret != 0) {
> +		dev_dbg(component->dev, "hpfr return %d\n", ret);
> +		es8389->hpfr = ES8389_HPF_DEFAULT;
> +	}
> +

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