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

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.

> +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.

> +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.

> +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?

> +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?

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmpyDC4ACgkQJNaLcl1U
h9AahQf9F5kK6n9yrZew7//ocl7ez/1yPjOehgMQ2zHFn1Bqjb3f7ACqmhu5fxXL
z1EtIsw8T6KHAsRgh0z3JWFPqqFsHyKE5ahsEaNH3l4rssxIblKASLjmLud5PC2R
6B95uhA5uZOO7tzwQ5uHynnte1R8i5YoeW8PXToTYcZSYkg09WTtGlCxWvUsoVMZ
gjEj1IjtbEDTg8iNa0npAf/lDMssfePe69UOpB1A3Ls63W1CIvf0F5On97tJGk95
v/QAOh3u9nM0FbQ2G7HfXnuKQWMNUL30g+1+hRix3HG1QnO+ICZd9uOzwmExvYF7
gzjoNvlZIe2get3PoOZGi93KrNQGEA==
=kmsg
-----END PGP SIGNATURE-----