Re: [PATCH v4 8/8] ASoC: codecs: add Qualcomm Tambora (WCD9378) SDCA codec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: wcd9378: Missing DT-derived micbias patching
- [Medium] ASoC: wcd9378: Meaningful code and commit message mismatch regarding DT property
- [Medium] ASoC: wcd9378: Unprotected modification of static global topology array
--

commit b641a8d55f5574997647d703d7136b63d9de6c0b
Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

ASoC: codecs: add Qualcomm Tambora (WCD9378) SDCA codec

Add support for the Qualcomm Tambora (WCD9378) headset codec in SDCA
mode over SoundWire. On ARM/DT platforms without ACPI/DisCo firmware
the SDCA topology and SoundWire port properties are supplied as static
data through the codec driver.

>     Binds SoundWire slave id 0x0217:0x0110 when qcom,compute-mode is set
>     on the DT node.

[Severity: Medium]
The commit message mentions that the driver binds when qcom,compute-mode is
set on the DT node, but the code actually checks for qcom,sdca-compliant. Is
this a typo in the commit message or should the driver be checking for a
different property?

> diff --git a/sound/soc/codecs/wcd9378-sdca.c b/sound/soc/codecs/wcd9378-sdca.c
> index 0000000000000..f1f94c13cba52
> --- /dev/null
> +++ b/sound/soc/codecs/wcd9378-sdca.c
[ ... ]
> +static int wcd9378_sdca_populate_function(struct sdw_slave *slave,
> +					  struct sdca_function_data *function)
> +{
> +	/* @function->desc is already set by the framework; fill payload only. */
> +	if (function->desc->type != wcd9378_sdca_desc.type)
> +		return -EINVAL;
> +
> +	function->num_entities    = wcd9378_sdca_data.num_entities;
> +	function->entities        = wcd9378_sdca_data.entities;
> +	function->num_clusters    = wcd9378_sdca_data.num_clusters;
> +	function->clusters        = wcd9378_sdca_data.clusters;
> +	function->num_init_table  = wcd9378_sdca_data.num_init_table;
> +	function->init_table      = wcd9378_sdca_data.init_table;
> +	function->reset_max_delay = wcd9378_sdca_data.reset_max_delay;
> +

[Severity: High]
The commit message mentions that this function patches the IT 33 MIC_BIAS
default with the DT-derived per-slave value (qcom,micbias2-microvolt). Does this
function miss the logic to parse the DT property and patch the micbias voltage?

Currently, the micbias voltage remains hardcoded at 2.75V via
range_it33_micbias_data[]. Hardware requiring a different voltage might receive
2.75V instead.

> +	/* Elevate is_volatile / has_reset to match the DisCo/ACPI path. */
> +	sdca_apply_default_control_classifiers(function);

[Severity: Medium]
Does wcd9378_sdca_populate_function() improperly mutate the statically
declared global array wcd9378_sdca_entities[]?

The driver maps the global wcd9378_sdca_data.entities to function->entities
and then passes it to sdca_apply_default_control_classifiers(), which modifies
the is_volatile and has_reset booleans in-place. If multiple instances probe
concurrently, could this cause a data race or violate device isolation?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918131914.681588-1-srinivas.kandagatla@oss.qualcomm.com?part=8