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

[YLCHANG2 <neo.chang70@gmail.com>]

On 5/18/26 19:35, Mark Brown wrote:
> 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.
Thanks for catching this. I will update the Kconfig and Makefile in v3.
>
>> --- /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.
Got it. I will change it in v3.
>
>> +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.
Got it. I will initialize data_count to 0 here in v3.
>
>> +	/* 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?
For NAU8360_R02_I2C_ADDR, it requires a write operation to latch the I2C 
device address.
For NAU8360_R46_I2C_DEVICE_ID, although it is read-only, its value is 
determined by the hardware configuration
of the GPIO1 and GPIO2 pins at boot (e.g., 0x1a, 0x1b, 0x4a, or 0x4b).
>
>> +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.
Since our TDM and standard I2S share the same hardware enable/disable 
registers,
we cannot completely disable the interface here.
I will change this to bypass the TDM slot configuration and simply 
return 0 when rx_mask is 0.
>
>> +	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.
Got it. I will fix it in v3.
>
>> +	/* defalut disable Sense signal after booting */
> Typo of default.
I will fix it in v3.
>
>> +	/* 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.
The user volume is adjusted via the DSP firmware.
However, HW2 is a separate hardware gain stage that
we intentionally do not expose to user space as an ALSA control.
We have to write 0xbf66 here to set its required default value.
>
>> +	/* 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.
This was only used for internal debugging of the codec chip. I will 
remove it in v3.