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

On 5/27/26 21:43, Mark Brown wrote:
> On Tue, May 26, 2026 at 09:58:26AM +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 few things below, you also have a bunch of what look like legitimate
> issues picked up by sashiko (some of which overlap with what I've got
> below):
>
>     https://sashiko.dev/#/patchset/20260526015826.440769-1-YLCHANG2%40nuvoton.com
Regarding the sashiko report, I have a few questions:
Are we required to address every issue caught by sashiko? We found that 
some of them are actually false positives.
If we need to explain our code regarding sashiko's findings, who should 
we send the reply to?
Additionally, is there a way to run the sashiko checks locally before 
submitting our patches?
>
>> +static int nau8360_peq_coeff_get(struct snd_kcontrol *kcontrol,
>> +	struct snd_ctl_elem_value *ucontrol)
>> +{
> ...
>
>> +	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++) {
>> +		value = snd_soc_component_read(cp, reg + i);
>> +		*(val + i) = cpu_to_be16(value);
>> +	}
>> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST, 0);
>> +
>> +	return 0;
>> +}
>
>> +static int nau8360_peq_coeff_put(struct snd_kcontrol *kcontrol,
>> +	struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST,
>> +		NAU8360_HW1_MEM_TEST);
> ...
>
>> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST, 0);
> We use an rwsem to protect the controls from userspace so we could get
> more than one caller in these, you need some driver local locking.
Got it. I will add local lock in v4.
>
>> +static int nau8360_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
>> +	unsigned int rx_mask, int slots, int slot_width)
>> +{
>> +	struct snd_soc_component *cp = dai->component;
>> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
>> +	unsigned int tx_slot_used = 0, rx_slot_used = 0;
>> +	int ret = 0;
>> +
>> +	if (slot_width != 16 && slot_width != 24 && slot_width != 32) {
>> +		dev_err(cp->dev, "Invalid TDM channel length: %d", slot_width);
>> +		return -EINVAL;
>> +	}
> This stops the support for disabling TDM from working, a slot_width is
> required here.
Got it. I will fix this in v4.