Re: [PATCH 2/2] ASoC: codecs: Add NXP TFA9865 amplifier driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] ASoC: tfa9865: sleeping in atomic trigger context in
  tfa9865_mute_stream()
- [Medium] ASoC: tfa9865: AMPE state desync on Amp Switch toggle in
  tfa9865_mute_stream()
- [Medium] ASoC: tfa9865: missing software reset or MANSCONF clear in
  tfa9865_init()
--

commit 9362aea049536ac41ce721fb8b374074a607c998
Author: Venkat Yadlapati <venkat.yadlapati@yahoo.com>

ASoC: codecs: Add NXP TFA9865 amplifier driver

This commit adds an ASoC codec driver for the NXP/Goodix TFA9865 mono
class-D speaker amplifier, supporting the B0 revision with 32-bit I2S
slots.

> diff --git a/sound/soc/codecs/tfa9865.c b/sound/soc/codecs/tfa9865.c
> new file mode 100644
> index 0000000000000..7467b8b21eda7
> --- /dev/null
> +++ b/sound/soc/codecs/tfa9865.c

[ ... ]

> +static const struct snd_soc_dapm_widget tfa9865_dapm_widgets[] = {
> +	SND_SOC_DAPM_OUTPUT("OUT"),
> +	SND_SOC_DAPM_SUPPLY("POWER", TFA9865_SYS_CTRL0, TFA9865_SYS_CTRL0_PWDN, 1, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY("BOOST", TFA9865_SYS_CTRL0, TFA9865_SYS_CTRL0_DCA, 0, NULL, 0),
> +	SND_SOC_DAPM_OUT_DRV("AMPE", SND_SOC_NOPM, 0, 0, NULL, 0),
> +	SND_SOC_DAPM_SWITCH("Amp", SND_SOC_NOPM, 0, 0, &amp_switch),

[ ... ]

> +static int tfa9865_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	unsigned int val;
> +
> +	/*
> +	 * Enable the amplifier only once the I2S clocks run, enabling it
> +	 * earlier makes it pop when the clocks start. Leave it off if the
> +	 * output is not powered.
> +	 */
> +	if (!mute) {
> +		val = snd_soc_component_read(component, TFA9865_SYS_CTRL0);
> +		if (val & BIT(TFA9865_SYS_CTRL0_PWDN))
> +			return 0;
> +	}
> +
> +	return snd_soc_component_update_bits(component, TFA9865_SYS_CTRL0,
> +					     BIT(TFA9865_SYS_CTRL0_AMPE),
> +					     mute ? 0 : BIT(TFA9865_SYS_CTRL0_AMPE));
> +}

[Severity: Medium]
Does toggling the virtual "Amp Switch" control during playback leave the
amplifier enable bit out of sync with the dapm power state?

The "AMPE" widget is configured with no register control and no dapm
event callback, so the amplifier enable bit is only updated in
tfa9865_mute_stream().  If playback starts while "Amp Switch" is off (its
initial state), the power supply widget is off, so tfa9865_mute_stream()
sees the power-down bit set and returns 0 without enabling the amplifier.
When userspace later turns "Amp Switch" on while the stream is active,
dapm powers up the supply and boost widgets, but tfa9865_mute_stream() is
not called again, leaving the speaker silent for the rest of the stream.
Conversely, turning "Amp Switch" off mid-stream powers down the boost and
supply widgets while leaving the amplifier enable bit set.

Also, snd_soc_component_read() returns a negative error code cast to
unsigned int on failure, so testing val without checking for errors first
misinterprets failed reads.

> +
> +static const struct snd_soc_dai_ops tfa9865_dai_ops = {
> +	.hw_params = tfa9865_hw_params,
> +	.mute_stream = tfa9865_mute_stream,
> +	.no_capture_mute = 1,
> +	.mute_unmute_on_trigger = 1,
> +};

[Severity: High]
Does setting .mute_unmute_on_trigger sleep in atomic context when a stream
starts or stops?

With .mute_unmute_on_trigger set, snd_soc_pcm_dai_trigger() calls
snd_soc_dai_digital_mute() -> tfa9865_mute_stream() directly from the pcm
trigger callback while holding the pcm substream spinlock with interrupts
disabled.  tfa9865_mute_stream() then calls snd_soc_component_read() and
snd_soc_component_update_bits(), which lock component->io_mutex and
regmap->mutex and perform sleeping i2c transfers.

[ ... ]

> +static int tfa9865_init(struct tfa9865 *tfa9865)
> +{
> +	struct regmap *regmap = tfa9865->regmap;
> +	unsigned int seed;
> +	int ret;
> +
> +	/* Leave power down to make the protected registers accessible */
> +	regmap_clear_bits(regmap, TFA9865_SYS_CTRL0, BIT(TFA9865_SYS_CTRL0_PWDN));
> +	regmap_clear_bits(regmap, TFA9865_CLK_GATING_CTRL, TFA9865_CLK_GATING_CTRL_MANAOOSC);
> +	ret = tfa9865_wait_manstate(tfa9865, TFA9865_MANSTATE_WAIT_SRC);
> +	if (ret)
> +		return ret;

[ ... ]

> +	/* Configuration done, let the device manager start on power up */
> +	return regmap_set_bits(regmap, TFA9865_SYS_CTRL1, TFA9865_SYS_CTRL1_MANSCONF);
> +}

[Severity: Medium]
Can tfa9865_wait_manstate() time out on driver rebind or warm reboot when
no reset gpio is wired?

tfa9865_init() sets the configuration-done bit at the end of probe so the
device manager advances past the wait-for-source state on later power-ups.
Because reset_gpiod is optional and tfa9865_init() does not issue a
software reset or clear the configuration-done bit before clearing the
power-down bit, re-probing the driver while vddd stays powered lets the
device manager advance past the wait-for-source state and causes
tfa9865_wait_manstate() to fail with -ETIMEDOUT.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261116-vantage-tfa9865-v1-0-e88f0dccc427@yahoo.com?part=2