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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] es9039q2m: unprotected read-modify-write in es9039_wide_put
- [Medium] es9039q2m: unprotected VOLUME_HOLD sequence allows torn volume updates
- [Medium] es9039q2m: FIR coefficient RAM is lost on system suspend
- [Low] i2c: missing named initializer for struct i2c_device_id
--

commit b181df27142d5449ea7b02fc0c4c0aa3fd24e5a8
Author: Karl Asseily <karl@asseily.com>

ASoC: es9039q2m: add ESS Technology ES9039Q2M codec driver

This patch adds I2C support for the ESS Technology ES9039Q2M 32-bit
2-channel audio DAC. It includes handling for software mode over I2C,
an asynchronous sample rate converter, and automatic DoP detection.

> diff --git a/sound/soc/codecs/es9039q2m.c b/sound/soc/codecs/es9039q2m.c
> index 0000000000000..4edaab87e3f6f
> --- /dev/null
> +++ b/sound/soc/codecs/es9039q2m.c

[ ... ]

> +static int es9039_wide_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);
> +	struct es9039_wide_ctl *p = (void *)kcontrol->private_value;
> +	long v = ucontrol->value.integer.value[0];
> +	u32 old;
> +	int ret;
> +
> +	if (v < 0 || v > p->max)
> +		return -EINVAL;
> +
> +	ret = es9039_read_le(priv->regmap, p->reg, p->bytes, &old);
> +	if (ret)
> +		return ret;
> +
> +	if ((old & p->mask) == (u32)v)
> +		return 0;
> +
> +	/* Preserve the bits outside the field - reg 124 carries MUTE_RAMP. */
> +	ret = es9039_write_le(priv->regmap, p->reg, p->bytes,
> +			      (old & ~p->mask) | (u32)v);

[Severity: Medium]
Could this manual read-modify-write sequence race with other ALSA control updates?

Because snd_ctl_elem_write() takes the ALSA controls_rwsem lock in read mode,
concurrent control writes can interleave here. If another thread updates the
"Mute Ramp To Ground Switch" (which shares register 125, mapped here) right
after es9039_read_le() completes, es9039_write_le() will overwrite and lose
that concurrent change.

Should this sequence be protected by a lock?

[ ... ]

> +static int es9039_vol_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);
> +	int ret;
> +
> +	ret = regmap_update_bits(priv->regmap, ES9039_IIR_SPDIF,
> +				 ES9039_VOLUME_HOLD, ES9039_VOLUME_HOLD);
> +	if (ret)
> +		return ret;
> +
> +	ret = snd_soc_put_volsw(kcontrol, ucontrol);
> +
> +	regmap_update_bits(priv->regmap, ES9039_IIR_SPDIF,
> +			   ES9039_VOLUME_HOLD, 0);

[Severity: Medium]
Does this sequence need protection against concurrent volume updates?

If two threads call es9039_vol_put() concurrently, the sequences could
interleave. For example, Thread 1 could clear ES9039_VOLUME_HOLD while
Thread 2 is halfway through its snd_soc_put_volsw() writes.

Thread 2's remaining register writes would then apply immediately instead of
being latched together, which defeats the hardware pop-suppression mechanism.

[ ... ]

> +static int es9039q2m_resume(struct snd_soc_component *component)
> +{
> +	struct es9039q2m_priv *priv = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	guard(mutex)(&priv->lock);
> +
> +	regcache_cache_only(priv->regmap, false);
> +
> +	/*
> +	 * Mute the part before anything else, and write it straight to the
> +	 * hardware rather than through the cache.
> +	 *
> +	 * Where the board kept the supplies up this is a no-op - the part still
> +	 * holds the mute state it went to sleep with. Where the board removed
> +	 * them it is not: the part comes back at reset defaults, which are
> +	 * DAC_MODE clear, both channels UNMUTED, and both volume registers at
> +	 * 0x00, their 0 dB setting. regcache_sync() walks registers in ascending
> +	 * address order, so it would set DAC_MODE in register 0 - lighting the
> +	 * datapath up unmuted at full scale - and only restore the real mute in
> +	 * register 86, long after. That is the thump the probe path mutes to
> +	 * avoid, arriving by the other door.
> +	 *
> +	 * The cache is bypassed so the cached mute state is untouched; the sync
> +	 * below puts it back, after the volume registers at 74 and 75.
> +	 */
> +	regcache_cache_bypass(priv->regmap, true);
> +	ret = regmap_update_bits(priv->regmap, ES9039_DAC_MUTE,
> +				 ES9039_MUTE_BOTH, ES9039_MUTE_BOTH);
> +	regcache_cache_bypass(priv->regmap, false);
> +
> +	if (ret)
> +		dev_warn(component->dev,
> +			 "could not mute before resync: %d\n", ret);
> +
> +	ret = regcache_sync(priv->regmap);

[Severity: Medium]
Will the programmable FIR coefficients survive a system suspend?

If the device loses power during suspend, the FIR memory is cleared.
When resume calls regcache_sync() here, it will restore ES9039_PROG_RAM_CTRL
(which turns the custom FIR switch back on). However, the FIR taps
themselves are not restored since the data port is write-only and not
tracked by regmap.

Can this result in silent or distorted audio on resume?

[ ... ]

> +static const struct i2c_device_id es9039q2m_i2c_id[] = {
> +	{ "es9039q2m" },
> +	{ }
> +};

[Severity: Low]
Can we use named initializers for this array?

The I2C subsystem requires explicitly named initializers for struct
i2c_device_id arrays to ensure proper device matching.

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