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

On 8/4/26 23:58, Mark Brown wrote:
> On Tue, Aug 04, 2026 at 11:29:51AM +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.
>> +
>> +#include <linux/delay.h>
>> +#include <linux/firmware.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <sound/soc.h>
>> +
>> +#include "nau8360-dsp.h"
>> +#include "nau8360.h"
> You use bitfield.h so a direct include would be safer.
Got it. I will add #include <linux/bitfield.h>.
>
>> +static int nau8360_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>> +{
>> +		break;
>> +	case SND_SOC_DAIFMT_LEFT_J:
>> +		ctrl_val = NAU8360_FRAME_START_H2L | NAU8360_RX_OFFSET_LEFT;
>> +		ctrl1_val = NAU8360_TX_OFFSET_LEFT;
>> +		break;
>> +	case SND_SOC_DAIFMT_RIGHT_J:
>> +		ctrl_val = NAU8360_FRAME_START_H2L | NAU8360_RX_OFFSET_RIGHT;
>> +		ctrl1_val = NAU8360_TX_OFFSET_RIGHT;
>> +		break;
> NAU8360_RX_OFFSET_LEFT and NAU8360_RX_OFFSET_RIGHT are defined
> identically, presumably at least one of them is wrong and certainly one
> of the above cases is.

Thank you for pointing out the problem. There is indeed a mistake here.

The configuration missed the left/right justify settings. I will correct 
both the register definitions and the case logic in the v8 patch.

>
>> +static int nau8360_set_sysclk(struct snd_soc_component *cp,
>> +	int clk_id, int source, unsigned int freq, int dir)
>> +{
>> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
>> +	struct device *dev = nau8360->dev;
>> +	static const char * const idtab[] = { "DIG", "ANA", "Internal" };
>> +	static const char * const srctab[] = { "MCLK", "PLL", "HIRC48M", "BCLK" };
>> +	int ret;
>> +
>> +	if (dir == SND_SOC_CLOCK_OUT) {
>> +		dev_dbg(dev, "sysclk: freq %d (out)", freq);
>> +		return nau8360_set_sysclk_output(nau8360, freq);
>> +	}
>> +
>> +	switch (clk_id) {
>> +	case NAU8360_CLK_ID_INT:
> Usually we don't have a lot of fine grained control of the internal
> clock dividers of the device, things are a lot easier when the device
> just figures out what it needs based on it's input clocks.
Thanks for the feedback.

To make sure I understand: Should we remove the internal clock IDs from 
set_sysclk and handle clock configurations automatically inside the 
codec driver?

Does this mean we should avoid configuring them via the machine driver 
entirely? If so, what is the preferred way to handle clock fallback when 
playback stops or MCLK is absent

(e.g., via PCM shutdown hooks or DAPM events)?

>
>> +static int __maybe_unused nau8360_resume(struct snd_soc_component *component)
>> +{
>> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
>> +	struct regmap *regmap = nau8360->regmap;
>> +	int ret;
>> +
>> +	/* disable Sense at standby */
>> +	snd_soc_dapm_disable_pin(nau8360->dapm, "Sense");
>> +	snd_soc_dapm_sync(nau8360->dapm);
>> +
>> +	ret = nau8360_dsp_setup(component);
>> +
>> +	regcache_cache_only(regmap, false);
> We start the DSP with the device in cache only mode - that seems odd?
Got it. I will fix this in the v8 patch by disabling cache-only mode 
before DSP setup.
>
>> +static struct snd_soc_dai_driver nau8360_dai = {
>> +	.name = NAU8360_CODEC_DAI,
>> +	.playback = {
>> +		.stream_name = "Playback",
>> +		.channels_min = 1,
>> +		.channels_max = 4,
>> +		.rates = NAU8360_RATES,
>> +		.formats = NAU8360_FORMATS,
>> +	},
>> +	.capture = {
>> +		.stream_name = "Capture",
>> +		.channels_min = 1,
>> +		.channels_max = 8,
>> +		.rates = NAU8360_RATES,
>> +		.formats = NAU8360_FORMATS,
>> +	},
>> +	.ops = &nau8360_dai_ops,
>> +};
> Do you need symmetric_rates, the hw_params looks to program the same
> registers for both direction?
  Yes. Since playback and capture share the same configuration 
registers, I will add symmetric_rates in the v8 version.