Re: [PATCH v2] ASoC: codecs: aw88261: complete the start at trigger time

[Val Packett <val@packett.cool>]

On 10/17/26 7:00 AM, Vladyslav Tymoshchuk wrote:
> The amplifier is started from the PRE_PMU event of its AIF widget, that
> is from DAPM in prepare. aw88261_dev_start() waits up to 20 ms there for
> the PLL to lock to the bit clock and powers the amplifier down again if
> it does not.
>
> The Qualcomm AudioReach backends only start their graph, and with it
> the I2S/TDM clocks, in trigger since commit b54a38af7138 ("ASoC: qcom:
> q6apm-lpass-dai: move graph start to trigger"). There every start fails
> with "start failure (-1)" and the speaker stays silent for the whole
> stream, as seen with six AW88261 on a TDM link of SM8750.
>
> Split the start at the first step that needs the bit clock, the PLL
> lock check, whatever the host does:
>
>   - PRE_PMU clears power down (PWDN), configures the I2S interface and
>     the PLL and writes the boost settings of aw88261_reg_force_set().
>     The amplifier stays powered down (AMPPD) and muted.
>   - The unmute, called after the trigger of the CPU DAI with
>     .mute_unmute_on_trigger, does the rest: it checks the PLL lock,
>     clears AMPPD, checks SYSST, enables the feedback and unmutes. If the
>     PLL does not lock, "start failure" is logged, the amplifier is left
>     as PRE_PMU left it and the next trigger start tries again.
>   - The mute in trigger undoes the unmute (mute, feedback off, AMPPD),
>     and POST_PMD sets PWDN as before.
>
> So only the steps that need the clock run in trigger: 14 register
> accesses and 1 ms per amplifier for the unmute when the PLL has locked,
> 12 and 1 ms for the mute. The same steps run there on hosts that have
> the clock running from prepare. aw88261_reg_force_set() only writes
> boost settings and now runs before AMPPD is cleared rather than right
> after.
>
> wsa883x and wsa884x split their start the same way, with the
> configuration in DAPM and GLOBAL_PA_EN in the trigger-time mute. The
> vendor driver of this amplifier family (aw882xx) also starts the
> amplifier from .mute_stream, with the same sequence.
>
> The register accesses go over I2C and sleep, so they need a nonatomic
> trigger, which the Qualcomm DPCM links using this codec have. On an
> atomic PCM, as seen from substream->pcm->nonatomic in .startup, PRE_PMU
> does the whole start as before and the mute callback does nothing.
>
> POST_PMD can also run for an amplifier that was never powered up, as
> seen when the card is registered, so make "already power off" a debug
> message.
>
> Assisted-by: LLM
> Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
> ---
> [..]
> +/*
> + * First part of the start, which does not need the bit clock: power up and
> + * configure the I2S interface and the PLL. The amplifier stays powered down
> + * (AMPPD) and muted.
> + */
> +static int aw88261_dev_power_up(struct aw88261 *aw88261)
> [..]
> +/*
> + * Second part of the start, which needs the bit clock as the PLL locks to
> + * it: check the lock, enable the amplifier and the feedback, and unmute.
> + * On failure the device is left as aw88261_dev_power_up() left it.
> + */
> +static int aw88261_dev_enable(struct aw88261 *aw88261)
> [..]
> +/* Undo aw88261_dev_enable() */
> +static void aw88261_dev_disable(struct aw_device *aw_dev)
> [..]
> +static int aw88261_dev_start(struct aw88261 *aw88261, bool enable)

"power_up", "start", *and* "enable".. this naming is a mess now.

Would be nicer to make the *names* more descriptive of what's actually 
being turned on/off.

> [..]
> --- a/sound/soc/codecs/aw88261.h
> +++ b/sound/soc/codecs/aw88261.h
> @@ -564,6 +564,8 @@ enum {
>   enum {
>   	AW88261_DEV_PW_OFF = 0,
>   	AW88261_DEV_PW_ON,
> +	/* powered up and configured, amplifier off: waits for the bit clock */
> +	AW88261_DEV_PW_READY,

Would make more sense to stick the intermediate state in the, well, 
middle. And rename the states to something that makes sense like { OFF, 
WAITING_FOR_BIT_CLOCK, RUNNING } instead of making everything a comment 
like the slop machine left it.

> [..]
> base-commit: 0f8bc3b93b76d16682a4d4999f9559b1e18016f9


It does work here, anyhow, so

Tested-by: Val Packett <val@packett.cool> # sm7325-motorola-dubai


Thanks,
~val