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

[Val Packett <val@packett.cool>]

On 10/18/26 4:20 PM, Val Packett wrote:
>
> 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 


Though after this last reboot now, it's weirdly stuck when on the 
speaker profile, changing the volume did not play the "boop" sounds.. 
(nothing appearing in dmesg).. until I switched to the earpiece profile, 
when a big bunch of those played in a row!

Not guaranteed that this is related to these driver changes, but 
suspicious..


> Thanks,
> ~val
>