Re: [PATCH v2 3/4] ASoC: es9039q2m: add ESS Technology ES9039Q2M codec driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Clock setup unconditionally requests 256x MCLK and checks a hardcoded 128x minimum, violating hardware limits and breaking 64FS mode for high sample rates (e.g., 768kHz).
- [Medium] Internal software states (`priv->mute_user`, `priv->dop_auto`) are mutated before the hardware write succeeds, causing permanent desynchronization on I2C errors.
--

--- Patch [3]: [PATCH v2 3/4] ASoC: es9039q2m: add ESS Technology ES9039Q2M codec driver ---
commit d6078d6a073963832dc02c18cb1b239c05eaaf57
Author: Karl Asseily <[email protected]>

ASoC: es9039q2m: add ESS Technology ES9039Q2M codec driver

This commit adds a new ALSA SoC driver for the ESS Technology ES9039Q2M
32-bit 2-channel audio DAC. It provides support for PCM, DSD, DoP, and
S/PDIF input formats, handling software mode over I2C.

> diff --git a/sound/soc/codecs/es9039q2m.c b/sound/soc/codecs/es9039q2m.c
> new file mode 100644
> index 0000000000000..bd09e230f9a3e
> --- /dev/null
> +++ b/sound/soc/codecs/es9039q2m.c

[ ... ]

> @@ -821,11 +821,11 @@ static int es9039_mute_put(struct snd_kcontrol *kcontrol,
>  	if (val == priv->mute_user)
>  		return 0;
>  
>  	priv->mute_user = val;
>  
>  	ret = es9039_apply_mute(priv);
>  	if (ret)
>  		return ret;

[Severity: Medium]
Does this leave the cached state out of sync with the hardware?

If es9039_apply_mute() fails (e.g., due to an I2C error), priv->mute_user
has already been updated. Since the function returns early without reverting
the state, subsequent attempts by the user to set the intended value will be
short-circuited by the `if (val == priv->mute_user)` equality check. This
could leave the hardware permanently out of sync until a different value
is toggled.

[ ... ]

> @@ -857,11 +857,11 @@ static int es9039_dop_put(struct snd_kcontrol *kcontrol,
>  	if (priv->stream_rate)
>  		return -EBUSY;
>  
>  	priv->dop_auto = on;
>  
>  	ret = es9039_apply_dop(priv, on);
>  	if (ret)
>  		return ret;

[Severity: Medium]
Similar to the mute control above, if es9039_apply_dop() fails, does this
leave priv->dop_auto permanently out of sync with the hardware?

[ ... ]

> @@ -1108,18 +1108,18 @@ static int es9039_setup_clocking(struct es9039q2m_priv *priv,
>  	ratio = priv->bclk_ratio ? priv->bclk_ratio : 64;
>  
>  	if (!priv->mclk_fixed) {
>  		/*
>  		 * 256 * FS is the usual choice and clears both the 128 and 130
>  		 * floors with room to spare. Take whatever the clock actually
>  		 * settles on rather than assuming the request was honoured.
>  		 */
>  		ret = clk_set_rate(priv->mclk, 256 * rate);
>  		if (ret)
>  			return ret;

[Severity: High]
For high sample rates like 768kHz (which the driver advertises support for),
will this request a clock rate of 196.6 MHz?

The ES9039Q2M maximum MCLK is documented as 50 MHz. Should this limit the
requested clock multiplier based on the sample rate so it doesn't violate
the physical limits of the chip?

>  
>  		priv->mclk_rate = clk_get_rate(priv->mclk);
>  	}
>  
>  	if (es9039_is_sync(priv))
>  		min_mult = ES9039_SYNC_MIN_MCLK_FS;
>  	else
>  		min_mult = ES9039_ASYNC_MIN_MCLK_FS;
>  
>  	if (priv->mclk_rate < min_mult * rate) {
>  		dev_err(dai->dev,

[Severity: High]
If the clock provider safely clamps the requested clock to the 50 MHz limit
for a 768kHz stream, will this check unconditionally fail?

The 768kHz rate requires 64FS mode (which is handled later in
es9039q2m_hw_params()), but min_mult here is hardcoded to 128 or 130, meaning
50M < 128 * 768k. Could this prevent 768kHz streams from being played at all?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3