Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing DT patching for IT 33 MIC_BIAS and unsafe mutation of global static topology structures.
- [High] Disconnected playback topology graph (OT 43/45 bypass processing units).
--
commit 87e473cde38b9ce58f289bd43fe4fcd2cf63830e
Author: Srinivas Kandagatla [off-list ref]
ASoC: codecs: add Qualcomm Tambora (WCD9378) SDCA codec
This commit adds support for the Qualcomm Tambora (WCD9378) headset codec
in SDCA mode over SoundWire. It supplies static SDCA topology and SoundWire
port properties for ARM/DT platforms without ACPI/DisCo firmware.
quoted hunk ↗ jump to hunk
diff --git a/sound/soc/codecs/wcd9378-sdca.c b/sound/soc/codecs/wcd9378-sdca.c
index 0000000000000..6a77fac017005
--- /dev/null
+++ b/sound/soc/codecs/wcd9378-sdca.c
[ ... ]
quoted hunk ↗ jump to hunk
+/* OT 43 (Headphone), OT 45 (Headset): no controls. */
+static struct sdca_entity *entity_ot43_sources[] = { &wcd9378_sdca_entities[QSJ_IT41] };
+static struct sdca_entity *entity_ot45_sources[] = { &wcd9378_sdca_entities[QSJ_IT41] };
[Severity: High]
Does this bypass the processing chain for headphone and headset playback?
The output terminals are hardcoded to source directly from the input terminal
QSJ_IT41. Because the SDCA/DAPM graph traverses backward from sinks to sources,
this appears to orphan the intermediate units (MFPU 21, XU 42, SU 43, SU 45)
and the volume control unit (FU 6).
The commit message mentions that playback uses FU 6 for mute and volume, but
the volume controls won't be functional if the unit is bypassed in the graph.
[ ... ]
quoted hunk ↗ jump to hunk
+ /* E006: FU 6 (FU42) - vendor Feature Unit; HPH mute + Q7.8 volume. */
+ { .id = 0x6, .label = "FU 6", .type = SDCA_ENTITY_TYPE_FU,
+ .num_controls = ARRAY_SIZE(entity_fu6_controls), .controls = entity_fu6_controls },
[Severity: High]
Is it intentional that FU 6 is defined without any sources here?
Combined with the OT 43 and OT 45 source assignments above, FU 6 currently
operates as a disconnected dead end in the topology graph.
[ ... ]
quoted hunk ↗ jump to hunk
+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;
[ ... ]
quoted hunk ↗ jump to hunk
+ /* Elevate is_volatile / has_reset to match the DisCo/ACPI path. */
+ sdca_apply_default_control_classifiers(function);
+
+ return 0;
+}
[Severity: High]
Can modifying these entities cause a data race if multiple device instances are
probed?
Here, function->entities is assigned directly to the globally shared static
array wcd9378_sdca_data.entities. Calling sdca_apply_default_control_classifiers()
subsequently writes to the structure's fields. This mutates global state without
making a deep copy for the specific device instance.
Additionally, the commit message mentions that wcd9378_sdca_populate_function()
"patches the IT 33 MIC_BIAS default with the DT-derived per-slave value" via
the qcom,micbias2-microvolt property. The logic to parse and apply this DT
property appears to be missing from this function, which leaves the mic bias
voltage stuck at the 2.75V default.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907083727.733705-1-srinivas.kandagatla@oss.qualcomm.com?part=11