Re: [PATCH 3/3] ASoC: cs42l43: Improve pop performance of the input power up

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Mon, Nov 02, 2026 at 04:15:47PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] ASoC: cs42l43: swapped filter registers for ADC2 and PDM1L in cs42l43_decim_info
> - [Medium] ASoC: cs42l43: missing DAPM lock on Decimator HPF controls racing with cs42l43_mic_ev()
> - [Low] ASoC: cs42l43: missing static const on cs42l43_decim_info tables
> --
> [Severity: High]
> Are the .freg assignments for entries 1 and 2 in cs42l43_decim_info[]
> swapped?
> 
> cs42l43_mic_ev() indexes cs42l43_decim_info[] by w->shift from
> CS42L43_BLOCK_EN3:
> 
>   0: CS42L43_ADC1_EN_SHIFT       -> Decimator 1
>   1: CS42L43_ADC2_EN_SHIFT       -> Decimator 2 (.vmask uses DECIM2)
>   2: CS42L43_PDM1_DIN_L_EN_SHIFT -> Decimator 1 (.vmask uses DECIM1)
>   3: CS42L43_PDM1_DIN_R_EN_SHIFT -> Decimator 2 (.vmask uses DECIM2)
> 
> Entry 1 (ADC2, Decimator 2) sets .freg = CS42L43_DECIM_HPF_WNF_CTRL1, and
> entry 2 (PDM1L, Decimator 1) sets .freg = CS42L43_DECIM_HPF_WNF_CTRL2.
> 
> When ADC2 or PDM1L powers up individually on CS42L43, cs42l43_mic_ev()
> adjusts the HPF register of the wrong decimator channel.
> 
> Furthermore, when stereo ADC1 + ADC2 (shifts 0 and 1) or PDM1L + PDM1R
> (shifts 2 and 3) power up together in a coalesced DAPM sequence via
> dapm_seq_run_coalesced(), SND_SOC_DAPM_PRE_PMU runs for both widgets
> before SND_SOC_DAPM_POST_PMU runs for either. Because both widgets in the
> pair point info->freg to the same register, the first PRE_PMU caches the
> user setting and writes the temporary fval, and the second PRE_PMU then
> reads that temporary fval into priv->decim_cache[w->shift][1]. When
> SND_SOC_DAPM_POST_PMU runs, the second widget writes fval back to
> info->freg, permanently overwriting the user's HPF configuration while the
> other decimator channel's HPF was never adjusted.

No, on cs42l43 the ADC and PDM are muxed so both can't be powered
up together and share the same decimator control bits.

> [Severity: Low]
> This isn't a bug, but should cs42l43_decim_info[] and
> cs42l43b_decim_info[] (and the info pointer in cs42l43_mic_ev()) be
> marked static const since they are only used inside sound/soc/codecs/cs42l43.c
> and have no external declarations?

Yeah these should be static const will fixup in a v2.

> [Severity: Medium]
> Should the Decimator 1..6 HPF Switch and Decimator 1..6 HPF Corner
> Frequency controls in cs42l43_controls[] and cs42l43_b_controls[] also use
> cs42l43_dapm_get_volsw() / cs42l43_dapm_put_volsw() and
> cs42l43_dapm_get_enum() / cs42l43_dapm_put_enum() to hold the DAPM mutex?
> 
> The other controls whose register bits are cached in SND_SOC_DAPM_PRE_PMU
> and restored in SND_SOC_DAPM_POST_PMU (Decimator 1..6 Switch, Decimator
> 1..6 Ramp Up/Down, and EQ Switch) serialize kcontrol access under
> snd_soc_dapm_mutex_lock(dapm).
> 
> Because the HPF controls in cs42l43_controls[] and cs42l43_b_controls[]
> use plain SOC_SINGLE and SOC_ENUM:
> 
> 	SOC_SINGLE("Decimator 1 HPF Switch", CS42L43_DECIM_HPF_WNF_CTRL1,
> 		   CS42L43_DECIM_HPF_EN_SHIFT, 1, 0),
> 	...
> 	SOC_ENUM("Decimator 1 HPF Corner Frequency", cs42l43_dec1_hpf_corner),
> 
> can a userspace read or write during the 150ms msleep() window in
> cs42l43_mic_ev() observe the temporary fval override or have a newly
> written HPF setting clobbered when SND_SOC_DAPM_POST_PMU restores
> priv->decim_cache[w->shift][1]?

Yeah this is also a good shout I will move these over to the dapm
version of the helper.

Thanks,
Charles