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

[Karl Asseily]

On Thu, Aug 20, 2026 at 07:52:48PM +0100, Mark Brown wrote:

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

Done.

> _DECODE_MASK doesn't cover S/PDIF.

Fixed, and it was a real bug rather than a naming one - the mask is what
hw_params() uses to enable one decoder and clear the rest, so the S/PDIF
decoder was being left enabled alongside whichever one was selected.

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

Done.

> 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?

It now returns -EBUSY while a stream is open. Reconfiguring live would mean
redoing the decoder and input selection underneath a running stream, and I
have no way to test that the part tolerates it; refusing is honest and the
control is a setup-time thing in practice.

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

Fair - the TLV reasoning was wrong and I have replaced it. I do not think
these are volume controls by intended effect either, though: they are signed
correction coefficients for the second and third harmonic, they cancel
distortion contributed by the analogue stage, and turning one up does not
make anything louder. I have kept the names and fixed the comment to say
that instead. Happy to rename if you still read them as level controls.

> 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.

Agreed, and using the define turned up a bug: the control read
ES9039_IRQ_SOURCES + 1 with BIT(7), which is bit 15 of a 16-bit word and
reserved, so it could never have reported a fault. It now reads
ES9039_SRC_BCK_WS_FAIL from the correct byte. The other status controls use
their defines already.

> 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.

> 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.

> 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.

Taking these three together, because they were one mistake: I had written a
board policy into the driver. v2:

 - accepts SND_SOC_DAIFMT_CBP_CFP and programs PCM_MASTER_MODE, with
   MASTER_BCK_DIV computed from MCLK and the frame size. The part drives
   both clocks or neither, so CBP_CFC and CBC_CFP are still rejected.

 - calls clk_set_rate() for 256 * FS in hw_params() when the clock can be
   retuned, and uses the rate the clock actually settles on.

 - derives synchronous rather than being told it: the part is synchronous
   when it drives the clocks, or when MCLK could be retuned to suit the
   rate. Only a fixed oscillator feeding a consumer-mode part is genuinely
   asynchronous. That is detected at probe by asking clk_round_rate() for a
   different rate and seeing whether the clock offers one.

 - applies the datasheet's two floors rather than one: MCLK >= 128 * FS
   synchronous, >= 130 * FS asynchronous (Table 7, note 1). That difference
   is not academic - with a 24.576 MHz clock, 128 * FS makes 192 kHz exactly
   legal while 130 * FS caps at 189 kHz, so v1 was refusing a rate that
   works.

The startup() constraint now only applies when the clock is genuinely fixed;
with a retunable one hw_params() raises MCLK and fails cleanly if it cannot.

For context rather than argument: the board I wrote this on feeds the part a
fixed 24.576 MHz oscillator and runs it as a clock consumer, which is why I
had the async case in front of me and the synchronous one not. That is a
board choice and it stays in the board's DT, not in the driver.

> 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.

Fixed. Both now record what they want in the driver and the register is
written from the union, so a user unmute cannot unmute a stopped stream and
a stream start cannot override a user mute. The control reports the user's
own setting rather than the register, since the register also carries the
stream mute.

v2 is on the list, with all of the above on hardware:

  https://lore.kernel.org/r/20260821043859.171871-1-karl@xxxxxxxxxxx

Thanks for the review.

Karl