Re: [PATCH v2 2/2] ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

On Mon, May 18, 2026 at 10:47:04AM +0800, Neo Chang wrote:

> Add support for the Nuvoton NAU83G60 audio codec. The NAU83G60 is a
> stereo 30W+30W smart amplifier with an integrated low-latency
> Advanced Audio DSP.

> ---
>  sound/soc/codecs/nau8360-dsp.c |  704 ++++++++++
>  sound/soc/codecs/nau8360-dsp.h |  117 ++
>  sound/soc/codecs/nau8360.c     | 2334 ++++++++++++++++++++++++++++++++
>  sound/soc/codecs/nau8360.h     |  904 +++++++++++++
>  4 files changed, 4059 insertions(+)

There's no update to the build system, this can't have been tested.

> --- /dev/null
> +++ b/sound/soc/codecs/nau8360-dsp.c
> @@ -0,0 +1,704 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * The NAU83G60 Stereo Class-D Amplifier with DSP and I/V-sense driver.

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

> +static int nau8360_reply_from_dsp(struct snd_soc_component *component,
> +	const struct nau8360_cmd_info *cmd_info, int data_size,
> +	void *data, unsigned short dsp_addr)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	struct device *dev = component->dev;
> +	unsigned int payload, *data_buf;
> +	int i, j, ret, frag_len, frag_payload_len, data_count, len_pos, pad_len,
> +		pad_len_exp;

> +	frag_payload_len = frag_len - 1;
> +	if (cmd_info->msg_param)
> +		data_count = data_size;

This is the only place where we initialise data_count.

> +	/* check the reply length same as request */
> +	if (data_count && (cmd_info->cmd_id == NAU8360_DSP_CMD_GET_KCS_RSLTS ||
> +			cmd_info->cmd_id == NAU8360_DSP_CMD_GET_KCS_SETUP)) {
> +		dev_warn(dev, "payload_len %d, expected %d",
> +			data_size - data_count, data_size);
> +	}

but we use it unconditionally here.

> +static bool nau8360_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case NAU8360_R00_SOFTWARE_RST ... NAU8360_R02_I2C_ADDR:
> +	case NAU8360_R06_INT_CLR_STATUS:
> +	case NAU8360_R21_VBAT_READOUT ... NAU8360_R22_TEMP_READOUT:
> +	case NAU8360_R41_CLK_CTL2:
> +	case NAU8360_R46_I2C_DEVICE_ID:

Are the I2C addresss and device ID volatile or do they just not have
defaults?

> +static int nau8360_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
> +	unsigned int rx_mask, int slots, int slot_width)
> +{
> +	struct snd_soc_component *cp = dai->component;
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
> +	unsigned int tx_slot_used = 0, rx_slot_used = 0;
> +	int i, func_slot;
> +
> +	if (!rx_mask)
> +		return -EINVAL;

Specifying no slots should really be accepted as disabling TDM.

> +	if (source == NAU8360_CLK_SRC_MCLK)
> +		value = NAU8360_MCLK_SEL_MCLK;
> +	else if (source == NAU8360_CLK_SRC_PLL)
> +		value = NAU8360_MCLK_SEL_PLL;
> +	else
> +		goto err;

This should be a switch statement rather than a cascade of if
statements.  Thre's a number of other examples of this.

> +	/* defalut disable Sense signal after booting */

Typo of default.

> +	/* Set HW2 default volume */
> +	regmap_write(regmap, NAU8360_R97_HW2_CTL7, 0xbf66);
> +	regmap_write(regmap, NAU8360_R98_HW2_CTL8, 0xbf66);

Most user visible controls like volumes should be left at the hardware
defaults, that avoids encoding some specific use case into the driver.

> +	/* set GPIO3 MUX as Clock output */
> +	regmap_write(regmap, NAU8360_R09_GP_CTRL1, 0x1e9e);

This feels very platform specific and should be configured via DT or
quirks.

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmoK+ZIACgkQJNaLcl1U
h9DWlQf7BkoZVBAdZ7BLO/zhVK6vMFDAM0NMdLv6MmieTpo89Qng4dGDTNSSdyUd
td98ug4vWi4lkk7fSdokTxF8aFjBlnL+W9lKNNGZshlR8i6+7vOg1vdOdNZa09M4
LEj6Uw6OulIsuBnGAxqlBFmiyZ8MvRPO9g4CMNWzDZYgqtGHxD9Rlxmbx2ZTu+IB
41V5fCn6c1i5ANdVNXDyjC9wII583mqlfiP23lb9eZiszv36wWuZb12s1LBbBGqn
sVn+Q+YKUHMO6z+K3QMxpV2vIl+9qEMt3vpTZzexCm89QF9MIIvNCNcQ1/Nka/5A
bVI7UQC5IInFBrJiTBIN9GP2l7282g==
=PkKK
-----END PGP SIGNATURE-----