Re: [PATCH v6 v6 8/9] ASoC: qcom: lpass-tx-macro: Add support for Bolero v4.0

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] ASoC: qcom: lpass-tx-macro: no-op ADPT disable in
  tx_macro_enable_dec()
- [Medium] ASoC: qcom: lpass-tx-macro: missing DAPM routes for
  RX_SWR_MIC in v15 map
- [Low] ASoC: qcom: lpass-tx-macro: mixing __free() with goto in
  tx_macro_probe()
- [Low] ASoC: qcom: lpass-tx-macro: stale REGCACHE_MAPLE claim in commit
  message
--

commit 359688a973454b992ccd5b2a57890a5f9b9973cf
Author: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>

ASoC: qcom: lpass-tx-macro: Add support for Bolero v4.0

This commit adds Bolero v4.0 TX macro support for Hawi, including register
defaults, regmap extensions, decimator and port-select DAPM widgets and
routes, and ADPT control disabling.

> Add the v4.0 register defaults and extend the regmap accessors and
> maximum register offset only for the v4.0 match data. Use REGCACHE_MAPLE
> for the TX macro regmap since the register defaults are sparse and the
> v4.0 layout extends the register range.

[Severity: Low]
This isn't a bug, but tx_regmap_config already switched to REGCACHE_MAPLE in
commit 8ed9306885eb ("ASoC: codecs: lpass-tx-macro: switch cache to
REGCACHE_MAPLE") and this patch does not change .cache_type.

> diff --git a/sound/soc/codecs/lpass-tx-macro.c b/sound/soc/codecs/lpass-tx-macro.c
> index 6deaaabe43afa..6480a6c9032e3 100644
> --- a/sound/soc/codecs/lpass-tx-macro.c
> +++ b/sound/soc/codecs/lpass-tx-macro.c

[ ... ]

> @@ -434,6 +458,26 @@ static struct reg_default tx_defaults[] = {
>  	{ CDC_TX7_TX_PATH_SEC6, 0x00},
>  };
>  
> +static const struct reg_default tx_4_0_defaults[] = {
> +	{ CDC_TX_INP_MUX_ADC_MUX0_CFG2, 0x00},
> +	{ CDC_TX_INP_MUX_ADC_MUX1_CFG2, 0x00},
> +	{ CDC_TX_INP_MUX_ADC_MUX2_CFG2, 0x00},
> +	{ CDC_TX_INP_MUX_ADC_MUX3_CFG2, 0x00},
> +	{ CDC_TX_INP_MUX_ADC_MUX4_CFG2, 0x00},
> +	{ CDC_TX_INP_MUX_ADC_MUX5_CFG2, 0x00},
> +	{ CDC_TX_INP_MUX_ADC_MUX6_CFG2, 0x00},
> +	{ CDC_TX_INP_MUX_ADC_MUX7_CFG2, 0x00},
> +	{ CDC_TX0_TX_PATH_CFG2, 0x03},
> +	{ CDC_TX1_TX_PATH_CFG2, 0x03},
> +	{ CDC_TX2_TX_PATH_CFG2, 0x03},
> +	{ CDC_TX3_TX_PATH_CFG2, 0x03},
> +	{ CDC_TX4_TX_PATH_CFG2, 0x03},
> +	{ CDC_TX5_TX_PATH_CFG2, 0x03},
> +	{ CDC_TX6_TX_PATH_CFG2, 0x03},
> +	{ CDC_TX7_TX_PATH_CFG2, 0x03},
> +	{ CDC_ADPT0_ADPT_CTRL, 0x00},
> +};

[ ... ]

> @@ -929,6 +998,10 @@ static int tx_macro_enable_dec(struct snd_soc_dapm_widget *w,
>  
>  	switch (event) {
>  	case SND_SOC_DAPM_PRE_PMU:
> +		/* Disable the ADPT block before enabling the decimator */
> +		if (tx->data->has_adpt)
> +			snd_soc_component_write_field(component, CDC_ADPT0_ADPT_CTRL,
> +						      CDC_TX_ADPT_CTRL_EN_MASK, 0x0);

[Severity: Medium]
Does snd_soc_component_write_field() ever write to CDC_ADPT0_ADPT_CTRL here?

tx_4_0_defaults initializes the non-volatile cache entry for
CDC_ADPT0_ADPT_CTRL to 0x00, and nothing in the driver ever writes a
non-zero value to it.  Both snd_soc_component_write_field() and
regcache_sync() see that the cached value already matches 0x00 and skip the
register write, leaving the ADPT block in its hardware reset state during
capture.  Should tx_4_0_defaults record the actual non-zero hardware reset
value of CDC_ADPT0_ADPT_CTRL (as lpass-va-macro.c does in va_4_1_defaults)?

[ ... ]

> @@ -2027,6 +2103,418 @@ static const struct snd_soc_dapm_route tx_audio_map_v9_2[] = {
>  	{"TX SMIC MUX7", "SWR_MIC11", "TX SWR_INPUT11"},
>  };
>  
> +static const char * const adc_mux_text_v9_3[] = {
> +	"MSM_DMIC", "SWR_MIC", "ANC_FB_TUNE1", "RX_SWR_MIC"
> +};

[ ... ]

> +static const struct snd_soc_dapm_route tx_audio_map_v15[] = {
> +	/* DEC0 */
> +	{"TX DEC0 MUX", "SWR_MIC", "TX SMIC MUX0"},

[Severity: Medium]
Are DAPM routes missing for the "RX_SWR_MIC" option in tx_audio_map_v15?

adc_mux_text_v9_3 adds "RX_SWR_MIC" to TX DEC0 MUX through TX DEC7 MUX, but
neither tx_audio_map nor tx_audio_map_v15 defines a route with the
"RX_SWR_MIC" control for any decimator.  When userspace selects "RX_SWR_MIC"
on a decimator mux, DAPM finds no connected input path, so the decimator
widget never powers up and tx_macro_enable_dec() is not called.

[ ... ]

> @@ -2250,6 +2751,9 @@ static int tx_macro_probe(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  	struct tx_macro *tx;
>  	void __iomem *base;
> +	struct reg_default *reg_defaults __free(kfree) = NULL;
> +	struct regmap_config *reg_config __free(kfree) = NULL;
> +	size_t def_count;
>  	int ret, reg;
>  
>  	tx = devm_kzalloc(dev, sizeof(*tx), GFP_KERNEL);
> @@ -2304,7 +2808,40 @@ static int tx_macro_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	tx->regmap = devm_regmap_init_mmio(dev, base, &tx_regmap_config);
> +	reg_config = kmemdup(&tx_regmap_config, sizeof(*reg_config), GFP_KERNEL);
> +	if (!reg_config) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}

[Severity: Low]
This isn't a bug, but tx_macro_probe() mixes __free(kfree) variables with
goto-based error handling and declares reg_config as NULL at the top of the
function instead of defining and initializing it at kmemdup().  Registering
lpass_macro_pds_exit_action() via devm_add_action_or_reset() (as
rx_macro_probe() does in sound/soc/codecs/lpass-rx-macro.c) would avoid
mixing __free() with goto err.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-add_shikra_support-v6-0-2cda302237cc@oss.qualcomm.com?part=8