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

[Mark Brown]

On Thu, Aug 20, 2026 at 09:26:25AM +0300, Karl Asseily wrote:
> The ES9039Q2M is a 32-bit two-channel audio DAC with an asynchronous
> sample rate converter. It selects between two control personalities with
> its MODE pin: hardware mode, strapped by HW0/HW1/HW2 with no control bus
> at all, and software mode over I2C or SPI. This driver implements
> software mode over I2C, which MODE = GND selects.



> @@ -0,0 +1,1259 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ESS Technology ES9039Q2M 32-bit 2-channel audio DAC

Please make the entire comment block a C++ one so things look more
intentional.

> +#define ES9039_SYS_MODE			0x01	/* reg 1, reset 0xb1 */
> +#define   ES9039_ENABLE_TDM_DECODE	BIT(0)	/* set at reset */
> +#define   ES9039_ENABLE_DSD_DECODE	BIT(1)
> +#define   ES9039_ENABLE_DOP_DECODE	BIT(2)
> +#define   ES9039_ENABLE_SPDIF_DECODE	BIT(3)
> +#define   ES9039_SYNC_MODE		BIT(6)	/* 0 = ASYNC, which DATUM uses */
> +#define   ES9039_ENABLE_DAC_CLK		BIT(7)	/* set at reset */
> +#define   ES9039_DECODE_MASK		(ES9039_ENABLE_TDM_DECODE | \
> +					 ES9039_ENABLE_DSD_DECODE | \
> +					 ES9039_ENABLE_DOP_DECODE)

_DECODE_MASK doesn't cover S/PDIF.

> +
> +	ret = es9039_write_le(priv->regmap, p->reg + (p->shift / 8), 2,
> +			      (u16)v);
> +	return ret ? ret : 1;

Please write normal conditional expressions rather than ternary ones for
improved legibility.

> +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);
> +	if (ret)
> +		return ret;
> +
> +	return 1;
> +}

If a stream is already running then the handling in hw_params() won't
have a chance to kick in - should this return an error in that case, or
try to do some reconfiguration?

> +	/*
> +	 * Not "... Volume". ALSA reserves that suffix for gain controls
> +	 * carrying a dB scale, and these are signed correction coefficients
> +	 * for the second and third harmonic - they have no dB meaning and no
> +	 * TLV. Useful values come from measuring a given board's distortion on
> +	 * an analyser and solving for them; zero, the reset value, is the only
> +	 * honest default until someone has.
> +	 */

Volume controls don't *need* TLV data or dB scales, it's more about
their intended effect.

> +	ES9039_STAT("Clock Fault", ES9039_IRQ_SOURCES + 1, BIT(7), 1),

There was a symbolic name for WS_FAIL, and there's TDM_VALID as well.
It feels like this should be able to use the defines, especially since
it's a driver local control type.

> +/*
> + * In asynchronous mode - a fixed oscillator with no relationship to the incoming
> + * frame clock, which is how any board using the ASRC properly is wired - the
> + * datasheet requires MCLK >= 130 * FS (hardware mode table, ASYNC rows). Cap the
> + * rate accordingly so a machine driver cannot open a stream the clock cannot
> + * legally carry. With a 24.576 MHz oscillator that ceiling is 189 kHz, which
> + * means 176.4 kHz is available and 192 kHz is not.
> + */
> +#define ES9039_ASYNC_MIN_MCLK_FS	130
> +
> +static int es9039q2m_startup(struct snd_pcm_substream *substream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct es9039q2m_priv *priv = snd_soc_component_get_drvdata(dai->component);
> +	unsigned int max_rate;
> +
> +	if (!priv->mclk_rate)
> +		return 0;
> +
> +	max_rate = priv->mclk_rate / ES9039_ASYNC_MIN_MCLK_FS;
> +
> +	return snd_pcm_hw_constraint_minmax(substream->runtime,
> +					    SNDRV_PCM_HW_PARAM_RATE,
> +					    8000, max_rate);
> +}

This means that if MCLK is specified but syncronous we'll reject things
that could be supported, that doesn't seem ideal.  We should have some
mechanism for specifying if the clock is actually async.

> +	/*
> +	 * The part can generate BCLK/WS (PCM_MASTER_MODE), but on a board fed a
> +	 * fixed oscillator that means deriving them from MCLK and throwing away
> +	 * the ASRC's entire purpose. Consumer only, and say so.
> +	 */
> +	if ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != SND_SOC_DAIFMT_CBC_CFC)
> +		return -EINVAL;

That seems odd - usually an ASRC is a workaround for mismatched clocks,
not a goal.  It is very common for the CODEC to drive clocks on the
audio bus from a high quality MCLK.

> +static int es9039q2m_mute_stream(struct snd_soc_dai *dai, int mute, int dir)
> +{
> +	struct es9039q2m_priv *priv = snd_soc_component_get_drvdata(dai->component);
> +
> +	return regmap_update_bits(priv->regmap, ES9039_DAC_MUTE,
> +				  ES9039_MUTE_BOTH, mute ? ES9039_MUTE_BOTH : 0);
> +}

This is shared with the user visible mute control, they're going to
fight and be buggy.  The register needs to be owned by one of them or
they need to coordinate.

> +	if (priv->mclk) {
> +		priv->mclk_rate = clk_get_rate(priv->mclk);
> +		if (priv->mclk_rate > 50000000)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "mclk %u Hz exceeds the 50 MHz maximum\n",
> +					     priv->mclk_rate);
> +	}

As per the above comments on driving the clocks from the CODEC it'd be
pretty standard to use clk_set_rate() to configure the MCLK to be a
suitable multiple of the sample rate and get everything nice and
synchronous.

Attachment: signature.asc
Description: PGP signature