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

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

On 8/14/26 00:20, Mark Brown wrote:
> On Thu, Aug 13, 2026 at 02:43:27PM +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.
>> +int nau8360_dsp_init(struct snd_soc_component *cp)
>> +{
>> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
>> +	int i, ret;
>> +
>> +	for (i = 0; i < NAU8360_DSP_FW_NUM; i++) {
>> +		ret = nau8360_dsp_chan_kcs_setup(cp, nau8360->dsp_firmware[i], nau8360_dsp_addr[i]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> Should this or the binding parsing code do something about PBTL mode,
> the binding says that the left DSP firmware is ignored in PBTL mode but
> this looks like it will still try to load the firmware.
In PBTL mode, both the left and right channels must load firmware.
However, the right channel will be the primary channel
(meaning the left channel also needs firmware, but its actual data 
content should be identical to the right channel).
I will update the yaml binding documentation in v9 to clarify that both 
firmware files must be provided and loaded in PBTL mode,
removing the statement that the left firmware is ignored.
>
>> +static int nau8360_peq_coeff_get(struct snd_kcontrol *kcontrol,
>> +	struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_to_dapm(kcontrol);
>> +	struct snd_soc_component *cp = snd_kcontrol_chip(kcontrol);
> This is registered as a regular kcontrol so should get the DAPM context
> with snd_soc_component_to_dapm().  Same for _put().
Thank you for pointing this out.
I will update both the _get() and _put() functions in v9 to properly 
retrieve the DAPM context using
snd_soc_component_get_dapm().
>
>> +	if (snd_soc_dapm_get_bias_level(dapm) > SND_SOC_BIAS_STANDBY) {
>> +		dev_dbg(nau8360->dev, "PEQ access is not allowed during playback");
>> +		return 0;
>> +	}
> What happens if something starts playback simultaneously with this?
> Nothing stops playback starting before we start accessing the registers.
> The check will also fail if we've got an active capture stream, IIUC
> we shouldn't have one without playback since it's an amplifier and
> that's presumably for DSP feedback but at least the docs should be
> clearer.
Due to a hardware limitation, the PEQ registers cannot be updated while 
audio is active.
To prevent playback from starting simultaneously, should we add a lock 
mechanism across the PEQ _put(), _get(), and PCM _startup()?
Also, I will add inline comments regarding the capture stream and update 
the debug message to 'while audio is active'.
>
>> +static int nau8360_hv_pre_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_R9C_HW1_CTL2,
>> +			NAU8360_HW1_CH_MUTE, NAU8360_HW1_CH_MUTE);
>> +		snd_soc_component_update_bits(component, NAU8360_R99_HW2_CTL9,
>> +			NAU8360_HW2_CH_MUTE, NAU8360_HW2_CH_MUTE);
>> +	}
>> +
>> +	return 0;
>> +}
> This is registered as a SND_SOC_DAPM_PRE() so it'll run for capture
> streams too.  That's probably not the end of the world since IIUC that's
> only used for speaker protection data but it's still fun?
You are right. To avoid this unintended behavior during capture-only 
streams,
I will remove the global SND_SOC_DAPM_PRE and SND_SOC_DAPM_POST widgets 
in v9.
I will move the event_on/event_off logic directly into the 
nau8360_hv_event (Class D) widget
to ensure it is only executed during the playback path.