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

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

On 4/24/26 23:56, Mark Brown wrote:
> On Fri, Apr 24, 2026 at 11:39:53AM +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.
>> +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 soc_bytes_ext *params = (void *)kcontrol->private_value;
>> +	int i, ret, reg = nau8360_peq_regaddr(kcontrol->id.name);
>> +	__be16 *data;
>> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST,
>> +		NAU8360_HW1_MEM_TEST);
>> +	for (i = 0; i < params->max / sizeof(u16); i++)
>> +		snd_soc_component_write(cp, reg + i, be16_to_cpu(*(data + i)));
>> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST, 0);
>> +
>> +	kfree(data);
>> +
>> +	return 0;
> This should return 1 if the value was changed to generate notifications.
I will modify this and returns 1 when the value actually changes in v2..
>
>> +/**
>> + * nau8360_set_tdm_slot - configure DAI TDM.
>> + * @tx_mask: 4-bits value representing each active TX slots. Range: 0 (skip), 1~8. Ex.
>> + *	bit 0-3 for left AEC output channel selection
>> + *	bit 4-7 for right AEC output channel selection
>> + *	bit 8-11 for left Isense output channel selection
>> + *	bit 12-15 for right Isense output channel selection
>> + *	bit 16-19 for left Vsense output channel selection
>> + *	bit 20-23 for right Vsense output channel selection
>> + *	bit 24-27 for Junction Temperature (Tj) data output channel selection
>> + *	bit 28-31 for VBAT measured data output channel selection
>> + * @rx_mask: Bitmask representing active RX slots. Ex.
>> + *	bit 0-7 for left DAC channel source selection
>> + *	bit 8-15 for right DAC channel source selection
>> + *	bit 16-23 for left ANC channel source selection
>> + *	bit 24-31 for right ANC channel source selection
>> + *
>> + * Configures a DAI for TDM operation. Only support 8 slots TDM.
>> + */
>> +static int nau8360_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
>> +	unsigned int rx_mask, int slots, int slot_width)
> That's not the way the API is supposed to work, the mask should be which
> slots are active as a bitmask.  It looks like what you want here is a
> series of muxes which control the routing to some AIF widgets
> representing the TDM slots.
Got it. I will rework set_tdm_slot() so tx_mask and rx_mask are used 
only as active slot bitmasks.
The default routing will be TX slots for AEC and RX slots for DAC.
Slot-to-function assignment will be handled through ALSA kcontrols instead.
>
>> +static const struct regmap_config nau8360_regmap_config = {
>> +	.reg_bits = NAU8360_REG_ADDR_LEN,
>> +	.val_bits = NAU8360_REG_DATA_LEN,
>> +
>> +	.max_register = NAU8360_REG_MAX,
>> +	.readable_reg = nau8360_readable_reg,
>> +	.writeable_reg = nau8360_writeable_reg,
>> +	.volatile_reg = nau8360_volatile_reg,
>> +	.reg_read = nau8360_reg_read,
>> +	.reg_write = nau8360_reg_write,
>> +
>> +	.cache_type = REGCACHE_RBTREE,
> Use REGCACHE_MAPLE unless you've got a particular reason to use
> something else, it's a more modern data structure than _RBTREE and makes
> choices more suited to current hardware.
I will change this to REGCACHE_MAPLE in v2.
>
>> +	/* DAC gain setting 0dB by changing current cell current. */
>> +	regmap_update_bits(regmap, NAU8360_R6E_DAC_CFG0, NAU8360_DAC_CUR_MASK,
>> +		NAU8360_DAC_CUR_0DB);
> Things like gains should normally be user visible and left at the chip
> defaults, that way we're not making use case specific decisions.
Agreed. I will remove this setting and leave it at the default in v2.
>
>> +static void nau8360_read_device_properties(struct nau8360 *nau8360)
>> +{
>> +	nau8360->pbtl_enable = device_property_read_bool(dev, "nuvoton,pbtl-enable");
> This is missing from the binding documentation.
I double-checked the patch, and it seems this property is already 
documented in the YAML file.
>
>> +static int nau8360_i2c_probe(struct i2c_client *i2c)
>> +{
>> +	return snd_soc_register_component(dev, &soc_comp_dev_nau8360, &nau8360_dai, 1);
>> +}
> Nothing unregisters this, you should use devm_snd_soc_register_component()
Got it. I will switch to devm_snd_soc_register_component() in v2.