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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] es9039_dop_put() corrupts hardware state when toggled mid-stream and races with hw_params().
- [Medium] 'Clock Fault' status control queries the wrong register offset.
- [Medium] Probe initialization disables VOLUME_HOLD contrary to its documented intent.
--

--- Patch [3]: [PATCH 3/4] ASoC: es9039q2m: add ESS Technology ES9039Q2M codec driver ---
commit bc64110211ab9eddd7d7d8ac88b78bebe86562d6
Author: Karl Asseily <karl@asseily.com>

ASoC: es9039q2m: add ESS Technology ES9039Q2M codec driver

This commit introduces the ASoC driver for the ESS Technology ES9039Q2M codec. 
It implements the software control mode over I2C, supporting PCM, DSD, DoP, 
and S/PDIF inputs along with an asynchronous sample rate converter.

> diff --git a/sound/soc/codecs/es9039q2m.c b/sound/soc/codecs/es9039q2m.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/codecs/es9039q2m.c
[ ... ]
> +static int es9039_dop_put(struct snd_kcontrol *kcontrol,
> +			  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *c = snd_kcontrol_chip(kcontrol);
> +	struct es9039q2m_priv *priv = snd_soc_component_get_drvdata(c);
> +	bool on = !!ucontrol->value.integer.value[0];
> +	int ret;
> +
> +	if (on == priv->dop_auto)
> +		return 0;
> +
> +	priv->dop_auto = on;
> +
> +	ret = regmap_update_bits(priv->regmap, ES9039_INPUT_SEL,
> +				 ES9039_AUTO_INPUT_SEL,
> +				 on ? ES9039_AUTO_INPUT_SEL : 0);

[Severity: Medium]
Will toggling this control mid-stream corrupt the hardware state?

If toggled on, it sets ES9039_AUTO_INPUT_SEL here but doesn't update
ES9039_SYS_MODE to set ES9039_ENABLE_DOP_DECODE, since that was only evaluated
in es9039q2m_hw_params() when the stream started.

The commit message notes that selecting DoP without enabling the marker
decoder leaves the part hunting and causes it to mute. Is there a need to
synchronize the decoder state in ES9039_SYS_MODE when this control is toggled?

[ ... ]
> +	/* --- status, read-only --- */
> +	ES9039_STAT("Automute Active CH1", ES9039_AUTOMUTE_READ, BIT(0), 1),
> +	ES9039_STAT("Automute Active CH2", ES9039_AUTOMUTE_READ, BIT(1), 1),
> +	ES9039_STAT("DoP Valid", ES9039_INPUT_STREAM_READ,
> +		    ES9039_RD_DOP_VALID, 1),
> +	ES9039_STAT("TDM Data Valid", ES9039_INPUT_STREAM_READ,
> +		    ES9039_RD_TDM_VALID, 1),
> +	ES9039_STAT("SPDIF Valid", ES9039_INPUT_STREAM_READ,
> +		    ES9039_RD_SPDIF_VALID, 1),
> +	ES9039_STAT("Clock Fault", ES9039_IRQ_SOURCES + 1, BIT(7), 1),

[Severity: Medium]
Is this querying the wrong register offset for the clock fault bit?

ES9039_SRC_BCK_WS_FAIL is defined as BIT(7) of the 16-bit word at
ES9039_IRQ_SOURCES. By adding 1 to the register address and still using
BIT(7) as the mask, won't this check bit 15 of the 16-bit word instead of
bit 7?

[ ... ]
> +static int es9039q2m_component_probe(struct snd_soc_component *component)
> +{
> +	struct es9039q2m_priv *priv = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	/*
> +	 * Come up muted. The volume registers default to 0 dB, and an unmuted
> +	 * DAC arriving into an already-powered analogue stage is how you get a
> +	 * thump. ASoC unmutes via mute_stream() when a stream starts.
> +	 */
> +	ret = regmap_update_bits(priv->regmap, ES9039_DAC_MUTE,
> +				 ES9039_MUTE_BOTH, ES9039_MUTE_BOTH);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * VOLUME_HOLD makes both channel volumes latch together, so a stereo
> +	 * change can never momentarily skew the image.
> +	 */
> +	ret = regmap_update_bits(priv->regmap, ES9039_IIR_SPDIF,
> +				 ES9039_VOLUME_HOLD, 0);

[Severity: Medium]
Is this intended to disable the volume hold feature?

The comment explicitly mentions that VOLUME_HOLD makes the channel volumes
latch together, but passing 0 disables it. Should this pass
ES9039_VOLUME_HOLD instead of 0 to enable the feature?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820062626.39218-1-karl@asseily.com?part=3