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

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 6/9/2026 4:41 AM, 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.

Hi Neo,

I've provided a number of possible improvements and nitpicks which scale 
for the entire file - not just the occurrences I've highlighted.  In 
general, it's hard to follow the code.  A lot of arithmetics bundled 
together with limited spacing/comments/self-explanatory names.  Sheer 
number of if-statements and brackets does not help either.

There seem to be quite a bit of defensive programming here too - checks 
just to print a message.  I'd suggest to enlist event tracing and log 
the TX/RX communication unconditionally.  Would eliminate the need for 
many of prints found here.

> +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, len_pos, pad_len, pad_len_exp;
> +	int data_count = 0;

13 local variables.  Does not look good.  I'd try to refactor this one. 
Ask yourself - what's the (one) goal this function shall achieve.  It's 
clear that currently it does more than one thing e.g.: performs 
validation of the payload.  The validation could be moved into a 
separate function.

> +
> +	if (!cmd_info->reply_data) {
> +		dsp_dbg(dev, "The cmd without reply data!!");
> +		ret = nau8360_dsp_replied(nau8360, &frag_len, dsp_addr);
> +		if (ret)
> +			goto err;
> +		else if (!frag_len)
> +			goto done;

No need for 'else if', 'if' suffices.

> +	}
> +
> +	if (!data) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +	data_buf = (unsigned int *)data;
> +
> +	ret = nau8360_dsp_replied(nau8360, &frag_len, dsp_addr);
> +	if (ret)
> +		goto err;
> +	else if (!frag_len)
> +		goto done;

Ditto.

> +
> +	frag_payload_len = frag_len - 1;
> +	if (cmd_info->msg_param)
> +		data_count = data_size;
> +	for (i = 0; i < frag_payload_len; i++) {
> +		ret = regmap_read(nau8360->regmap, dsp_addr, &payload);
> +		if (ret) {
> +			dev_err(dev, "failed to read payload of dsp");
> +			goto err;
> +		}
> +		if (cmd_info->msg_param) {
> +			if (data_count >= NAU8360_DSP_DATA_BYTE) {
> +				*data_buf++ = payload;
> +				data_count -= NAU8360_DSP_DATA_BYTE;
> +				payload_read(dev, payload);
> +			} else {
> +				for (j = 0; j < NAU8360_DSP_DATA_BYTE; j++) {
> +					if (data_count <= 0)
> +						break;
> +
> +					((u8 *)data_buf)[j] = (payload >> (j * 8)) & 0xff;
> +					data_count--;
> +				}
> +				payload_read(dev, payload);
> +				break;
> +			}
> +		} else {
> +			*data_buf = payload;
> +			payload_read(dev, payload);
> +		}
> +	}
> +
> +	/* 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);
> +	}
> +	dsp_dbg(dev, "reading trailing fragment");
> +	ret = regmap_read(nau8360->regmap, dsp_addr, &payload);
> +	if (ret) {
> +		dev_err(dev, "failed to read trailing fragment");
> +		goto err;
> +	}
> +
> +	len_pos = payload & 0xff;
> +	len_pos |= ((payload >> 8) & 0xc0) << 2;
> +	if (len_pos != frag_len) {
> +		dev_err(dev, "LEN_POST %02X, expect %02X", len_pos, frag_len);
> +		ret = -EPROTO;
> +		goto err;
> +	}
> +
> +	pad_len = ((payload >> 8) & 0x30) >> 4;
> +	if (cmd_info->msg_param)
> +		pad_len_exp = frag_payload_len * NAU8360_DSP_DATA_BYTE -
> +			(data_size - data_count);
> +	else
> +		pad_len_exp = 0;
> +	if (pad_len != pad_len_exp) {
> +		dev_err(dev, "PAD_LEN %02X, expect %02X", pad_len, pad_len_exp);
> +		ret = -EPROTO;
> +		goto err;
> +	}
> +	dsp_dbg(dev, "LEN_POST 0x%x, PAD_LEN 0x%x", len_pos, pad_len);
> +	payload_read(dev, payload);
> +done:
> +	return 0;
> +err:
> +	dev_err(dev, "DSP reply error %d !!!", ret);
> +	return ret;
> +}

...

> +int nau8360_dsp_init(struct snd_soc_component *component)
> +{
> +	dev_info(component->dev, "DSP initializing...");
> +	return nau8360_dsp_set_kcs_setup(component);
> +}

Do we need a function that just inlines to nau8360_dsp_set_kcs_setup() 
except for a single print?  Doubt so.

...

> +static int nau8360_adacl_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +
> +	if (SND_SOC_DAPM_EVENT_ON(event))
> +		snd_soc_component_update_bits(component, NAU8360_R6E_DAC_CFG0,
> +			NAU8360_PD_DACL_DIS, 0);
> +
> +	return 0;
> +}
> +
> +static int nau8360_adacr_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +
> +	if (SND_SOC_DAPM_EVENT_ON(event))
> +		snd_soc_component_update_bits(component, NAU8360_R6E_DAC_CFG0,
> +			NAU8360_PD_DACR_DIS, 0);
> +
> +	return 0;
> +}
> +
> +static int nau8360_dacl_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +
> +	if (SND_SOC_DAPM_EVENT_OFF(event))
> +		snd_soc_component_update_bits(component, NAU8360_R6E_DAC_CFG0,
> +			NAU8360_PD_DACL_DIS, NAU8360_PD_DACL_DIS);
> +
> +	return 0;
> +}
> +
> +static int nau8360_dacr_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +
> +	if (SND_SOC_DAPM_EVENT_OFF(event))
> +		snd_soc_component_update_bits(component, NAU8360_R6E_DAC_CFG0,
> +			NAU8360_PD_DACR_DIS, NAU8360_PD_DACR_DIS);
> +
> +	return 0;
> +}

Refactor the above four functions - you could create a wrapper to lower 
LOCs without huring the readability.  If you prefer, instead of a 
one-for-four, you could have one "positive" wrapper (handles EVENT_ON 
cases) and a "negative" one (handles EVENT_OFF cases).

...

> +static int nau8360_codec_probe(struct snd_soc_component *component)
> +{
> +	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(component);
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	struct regmap *regmap = nau8360->regmap;
> +	struct device *dev = nau8360->dev;
> +	int ret, vbat, vsaw_level, vsaw_slope;
> +
> +	nau8360->dapm = dapm;
> +	nau8360_coeff_set_def(nau8360);
> +	ret = nau8360_dsp_setup(component);
> +	if (ret)
> +		goto err;
> +
> +	ret = nau8360_dsp_setup_controls(component);
> +	if (ret) {
> +		nau8360_dsp_enable(regmap, false);
> +		dev_err(dev, "DSP setup controls failed(%d)", ret);
> +		goto err;
> +	}
> +
> +	/* default disable Sense signal after booting */
> +	snd_soc_dapm_disable_pin(nau8360->dapm, "Sense");
> +	snd_soc_dapm_sync(nau8360->dapm);
> +
> +	/* VBAT is sensed by chip. */
> +	ret = nau8360_vbat_level(regmap, &vbat);
> +	if (ret) {
> +		dev_err(dev, "Failed to get valid VBAT level: %d\n", ret);
> +		goto err;
> +	}
> +	dev_dbg(dev, "VBAT %dV for nau8360", vbat);
> +
> +	/* Config sawtooth clock according to VBAT. Class D modulator input short setting
> +	 * for mute and de-pop purpose. Restore normal after initiation.
> +	 */
> +	ret = nau8360_sawtooth_params(vbat, &vsaw_level, &vsaw_slope);
> +	if (ret) {
> +		dev_err(dev, "can't get sawtooth clock parameters (%d)", ret);
> +		goto err;
> +	}
> +	regmap_update_bits(regmap, NAU8360_RA5_ANA_REG_1, NAU8360_VSAW_LV_MASK |
> +		NAU8360_KVCO_SAW_MASK, (vsaw_level << NAU8360_VSAW_LV_SFT) |
> +		(vsaw_slope << NAU8360_KVCO_SAW_SFT));
> +
> +	return 0;
> +err:
> +	return ret;

Why are we using the goto here if there is no unwinding found within the 
error path?  Either you've forgotten to add the unwinding or the goto is 
redundant.

Repeat for all the functions with similar behavior found in this file.

> +}
> +
> +static int __maybe_unused nau8360_suspend(struct snd_soc_component *component)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +
> +	regmap_update_bits(nau8360->regmap, NAU8360_R90_HW2_CTL0, NAU8360_HW2_STALL,
> +		NAU8360_HW2_STALL);
> +	nau8360_dsp_enable(nau8360->regmap, false);
> +	regcache_cache_only(nau8360->regmap, true);
> +	regcache_mark_dirty(nau8360->regmap);
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	regcache_cache_only(regmap, false);
> +	nau8360_peq_mem_enable(regmap, true);
> +	regcache_sync(regmap);
> +	nau8360_peq_mem_enable(regmap, false);
> +
> +	/* disable Sense at standby */
> +	snd_soc_dapm_disable_pin(nau8360->dapm, "Sense");
> +	snd_soc_dapm_sync(nau8360->dapm);
> +
> +	ret = nau8360_dsp_setup(component);
> +	if (ret)
> +		goto err;

Drop the goto, no error handling here.

> +
> +	return 0;
> +err:
> +	return ret;
> +}

...

> +static int nau8360_reg_write(void *context, unsigned int reg, unsigned int value)
> +{
> +	struct i2c_client *client = context;
> +	int ret, count = 0;
> +	u8 buf[6];
> +
> +	buf[count++] = reg >> 8;
> +	buf[count++] = reg & 0xff;
> +	if (reg != NAU8360_RF000_DSP_COMM && reg != NAU8360_RF002_DSP_COMM) {

Refactor to positive a check.  Positive checks are easier to follow and 
as long as there is no major impact on complexity of the code that 
follows, there is no reason to hurt readability.

> +		/* format for Codec, 2 bytes value and endian big */
> +		buf[count++] = value >> 8;
> +		buf[count++] = value & 0xff;
> +		dev_dbg(&client->dev, " %x <= %x", reg, value);
> +	} else {
> +		/* format for DSP, 4 bytes value and native */
> +		buf[count++] = value & 0xff;
> +		buf[count++] = (value >> 8) & 0xff;
> +		buf[count++] = (value >> 16) & 0xff;
> +		buf[count++] = (value >> 24) & 0xff;
> +	}
> +
> +	ret = i2c_master_send(client, buf, count);
> +	if (ret == count)
> +		return 0;
> +	else if (ret < 0)
> +		return ret;
> +	else
> +		return -EIO;

No need for 'else if', 'if' suffices.  And the 'else' can be dropped.

> +}

...

> +static struct reg_default *nau8360_alloc_defaults(struct device *dev, int *total_regs)
> +{
> +	struct reg_default *dyn_defaults;
> +	int reg_num = ARRAY_SIZE(nau8360_reg_defaults);
> +	int total = reg_num + (2 * NAU8360_TOT_BAND_PER_CH * NAU8360_TOT_BAND_COE);
> +	int i, j, idx;
> +
> +	dyn_defaults = devm_kzalloc(dev, total * sizeof(*dyn_defaults), GFP_KERNEL);
> +	if (!dyn_defaults)
> +		return NULL;
> +
> +	memcpy(dyn_defaults, nau8360_reg_defaults, sizeof(*dyn_defaults) * reg_num);
> +	idx = reg_num;
> +
> +	for (i = 0; i < NAU8360_TOT_BAND_PER_CH; i++) {
> +		unsigned int l_base = NAU8360_R100_LEFT_BIQ0_COE +
> +					(i * NAU8360_TOT_BAND_COE_RANGE);
> +		unsigned int r_base = NAU8360_R200_RIGHT_BIQ0_COE +
> +					(i * NAU8360_TOT_BAND_COE_RANGE);

Just add a local:
	unsigned int range = i * NAU8360_TOT_BAND_COE_RANGE;
to simplify the code.

> +
> +		for (j = 0; j < NAU8360_TOT_BAND_COE; j++) {
> +			dyn_defaults[idx++].reg = l_base + j;
> +			dyn_defaults[idx++].reg = r_base + j;
> +		}
> +	}
> +
> +	*total_regs = total;
> +
> +	return dyn_defaults;
> +}
> +
> +static int nau8360_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct nau8360 *nau8360 = dev_get_platdata(dev);
> +	struct regmap_config regmap_cfg = nau8360_regmap_config;
> +	struct reg_default *dyn_defaults;
> +	int num_total_regs;
> +	int i, ret, value;
> +
> +	if (!nau8360) {

What?  This seems redundant.

> +		nau8360 = devm_kzalloc(dev, sizeof(*nau8360), GFP_KERNEL);
> +		if (!nau8360)
> +			return -ENOMEM;
> +	}
> +	i2c_set_clientdata(i2c, nau8360);
> +	mutex_init(&nau8360->lock);
> +
> +	dyn_defaults = nau8360_alloc_defaults(dev, &num_total_regs);
> +	if (!dyn_defaults)
> +		return -ENOMEM;
> +
> +	regmap_cfg.reg_defaults = dyn_defaults;
> +	regmap_cfg.num_reg_defaults = num_total_regs;
> +
> +	nau8360->regmap = devm_regmap_init(dev, NULL, i2c, &regmap_cfg);
> +	if (IS_ERR(nau8360->regmap))
> +		return PTR_ERR(nau8360->regmap);
> +	nau8360->dev = dev;
> +
> +	nau8360_reset_chip(nau8360->regmap);
> +	ret = regmap_read(nau8360->regmap, NAU8360_R46_I2C_DEVICE_ID, &value);
> +	if (ret) {
> +		dev_err(dev, "Failed to read NAU83G60 device id %d",
> +			ret);

No need for a new line here, you won't exceed the recommended char limit.

> +		return ret;
> +	}
> +
> +	for (i = 0; i < NAU8360_TDM_TXN; i++)
> +		nau8360->tdm_tx_func_slot[i] = TDM_SLOT_NONE;
> +	for (i = 0; i < NAU8360_TDM_RXN; i++)
> +		nau8360->tdm_rx_func_slot[i] = TDM_SLOT_NONE;
> +	nau8360->tdm_chan_len = 32;

Either use constant or comment any use of hardcodes.

> +	ret = nau8360_read_device_properties(nau8360);
> +	if (ret)
> +		return ret;
> +
> +	nau8360_print_device_properties(nau8360);
> +	nau8360_init_regs(nau8360);
> +
> +	return devm_snd_soc_register_component(dev, &soc_comp_dev_nau8360, &nau8360_dai, 1);
> +}
> +
> +static const struct i2c_device_id nau8360_i2c_ids[] = {
> +	{ "nau8360", 0 },
> +	{ }

Please match the style of existing entires.  Checkout Mark's for-next to 
be up-to-date with recommended style or see commit 910714d4e79b ("ASoC: 
Use named initializers for arrays of i2c_device_data").

[1]: 
https://patch.msgid.link/ae2ff4898eb340bd8bcafb7b75443eb4a0ce3e76.1778692164.git.u.kleine-koenig@b
aylibre.com

> +};
> +MODULE_DEVICE_TABLE(i2c, nau8360_i2c_ids);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id nau8360_of_ids[] = {
> +	{ .compatible = "nuvoton,nau8360", },
> +	{}

Ditto.  Sidenote: you've used a different style then for the i2c table.

> +};
> +MODULE_DEVICE_TABLE(of, nau8360_of_ids);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id nau8360_acpi_match[] = {

struct acpi_device_id is available even if CONFIG_ACPI is disabled.  Not 
sure if we even need preproc #ifdef here.  Remove it and do some 
compilation checks.

> +	{"NVTN2002", 0,},
> +	{},

Ditto.  Sidenote: you've used a different style then for the i2c and the 
of table.

> +};
> +MODULE_DEVICE_TABLE(acpi, nau8360_acpi_match);
> +#endif
> +
> +static struct i2c_driver nau8360_i2c_driver = {
> +	.driver = {
> +		.name = "nau8360",
> +		.of_match_table = of_match_ptr(nau8360_of_ids),
> +		.acpi_match_table = ACPI_PTR(nau8360_acpi_match),

Does this compile if I disable e.g.: CONFIG_ACPI ?

> +	},
> +	.probe = nau8360_i2c_probe,
> +	.id_table = nau8360_i2c_ids,
> +};
> +module_i2c_driver(nau8360_i2c_driver);