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

On 8/21/26 23:24, Mark Brown wrote:
> On Thu, Aug 20, 2026 at 10:29:25AM +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.
> A couple of issues here, mostly this looks fine.
Hi Mark, Thanks for the review.
>
>> +static int nau8360_peq_coeff_put(struct snd_kcontrol *kcontrol,
>> +	struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_component *cp = snd_kcontrol_chip(kcontrol);
>> +	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(cp);
>> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
>> +	struct soc_bytes_ext *params = (void *)kcontrol->private_value;
>> +	int i, reg, ret = 0;
>> +	__be16 *data = NULL;
>> +	bool changed = false;
>> +
>> +	/* Use the DAPM lock to prevent race conditions during DAPM power-up
>> +	 * state transitions, and check component active status to prohibit
>> +	 * PEQ access during active audio streams (playback and capture).
>> +	 */
>> +	snd_soc_dapm_mutex_lock(dapm);
>> +	if (snd_soc_component_active(cp)) {
>> +		dev_dbg(nau8360->dev,
>> +			"PEQ coefficient access is ignored during audio is active");
>> +		goto unlock_dapm;
> This should probably be -EBUSY or something, though that will upset
> mixer-test.  As things stand since the values change when a stream
> starts and stops there should be an event when that happens, and since
> we report success some value ought to be filled in for the read.
I will change the return value to -EBUSY in v10
>
>> +static void nau8360_dsp_fw_load(struct nau8360 *nau8360)
>> +{
>> +	nau8360->load_fw_done = false;
>> +	schedule_work(&nau8360->load_fw_work);
>> +}
>> +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;
>> +	nau8360_dsp_fw_load(nau8360);
> This only kicks off the firmware load, it doesn't wait for it to
> complete.  That means that if a stream restarts quickly after resume we
> will run into issues - I see that the normal playback path just errors
> if we try to start without the firmware.  We should wait, either here or
> in the startup path.
I will add flush_work() in startup() to ensure the firmware loading is 
complete before the stream starts.
>
>> +	if (NAU8360_IS_DSP_REG(reg))
>> +		*value = nau8360->i2c_read_buf[0] |
>> +			(nau8360->i2c_read_buf[1] << 8) |
>> +			(nau8360->i2c_read_buf[2] << 16) |
>> +			(nau8360->i2c_read_buf[3] << 24);
> get_unaligned_le32()?
I will replace the manual bit shifts with the unaligned function in v10.