Re: [PATCH v2 5/6] ASoC: qcom: lpass-va-macro: Add shikra compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] 36 new ADPT configuration registers are missing from the va_is_rw_register() whitelist, causing regmap to silently drop their defaults during initialization.
- [Medium] The FS counter is enabled before it is bypassed, which contradicts the hardware initialization sequence stated in the commit message.
--

commit 7975fbfb906db9ea84cf327e35c78c8a83584393
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: qcom: lpass-va-macro: Add shikra compatible

Add shikra specific VA macro data and register programming needed by
the LPASS codec v4.0 implementation. This includes adding the v4.1 ADPT
blocks and required FS counter bypass initialization.

> diff --git a/sound/soc/codecs/lpass-va-macro.c b/sound/soc/codecs/lpass-va-macro.c
> index a3b3af0d7db3c..8353921f718e3 100644
> --- a/sound/soc/codecs/lpass-va-macro.c
> +++ b/sound/soc/codecs/lpass-va-macro.c
[ ... ]
> @@ -357,6 +405,127 @@ static const struct reg_default va_defaults[] = {
>  	{ CDC_VA_TX3_TX_PATH_SEC6, 0x00},
>  };
>  
> +static const struct reg_default va_4_1_defaults[] = {
[ ... ]
> +	/* ADPT blocks */
> +	{ CDC_VA_CDC_ADPT0_ADPT_CTRL, 0x51},
> +	{ CDC_VA_CDC_ADPT0_ADPT_GAIN_0, 0x11},
> +	{ CDC_VA_CDC_ADPT0_ADPT_GAIN_1, 0x01},
> +	{ CDC_VA_CDC_ADPT0_DH_FSM_CTRL, 0x02},
> +	{ CDC_VA_CDC_ADPT0_CUTOFF_FSM_CTRL_0, 0x77},

[Severity: High]
Are the newly added ADPT configuration registers (such as ADPT_GAIN, DH_FSM,
and CUTOFF_FSM) accessible by regmap?

Because these 36 registers are missing from the va_is_rw_register() whitelist
below, they will be treated as non-writeable by the regmap subsystem. During
probe, when regcache_sync_region() is called from va_macro_mclk_enable(),
regmap will silently discard the default values specified here in
va_4_1_defaults, leaving the v4.1 adaptive filters unconfigured.

[ ... ]
> @@ -430,6 +599,10 @@ static bool va_is_rw_register(struct device *dev, unsigned int reg)
>  	case CDC_VA_TX3_TX_PATH_SEC4:
>  	case CDC_VA_TX3_TX_PATH_SEC5:
>  	case CDC_VA_TX3_TX_PATH_SEC6:
> +	case CDC_VA_CDC_ADPT0_ADPT_CTRL:
> +	case CDC_VA_CDC_ADPT1_ADPT_CTRL:
> +	case CDC_VA_CDC_ADPT2_ADPT_CTRL:
> +	case CDC_VA_CDC_ADPT3_ADPT_CTRL:
>  		return true;
>  	}

[ ... ]
> @@ -471,10 +665,14 @@ static int va_clk_rsc_fs_gen_request(struct va_macro *va, bool enable)
>  		regmap_update_bits(regmap, CDC_VA_CLK_RST_CTRL_MCLK_CONTROL,
>  				   CDC_VA_MCLK_CONTROL_EN,
>  				   CDC_VA_MCLK_CONTROL_EN);
> -		/* clear the fs counter */
> +
>  		regmap_update_bits(regmap, CDC_VA_CLK_RST_CTRL_FS_CNT_CONTROL,
>  				   CDC_VA_FS_CONTROL_EN | CDC_VA_FS_COUNTER_CLR,
>  				   CDC_VA_FS_CONTROL_EN | CDC_VA_FS_COUNTER_CLR);
> +
> +		if (lpass_macro_get_codec_version() >= LPASS_CODEC_VERSION_4_1)
> +			regmap_set_bits(regmap, CDC_VA_CLK_RST_CTRL_FS_CNT_CONTROL,
> +					CDC_VA_FS_CNT_BYPASS);

[Severity: Medium]
Will this sequence enable the block before bypassing it?

The commit message notes that the hardware requires the FS counter to be
bypassed before it is enabled. However, the preceding regmap_update_bits()
call asserts the CDC_VA_FS_CONTROL_EN bit prior to the CDC_VA_FS_CNT_BYPASS
bit being set here. Could this open a window where the control block is
enabled without the bypass, potentially placing the ADPT hardware in an
invalid state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-add_shikra_support-v2-0-fdd2129c696c@oss.qualcomm.com?part=5