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

[Charles Keepax]

On Fri, Sep 18, 2026 at 02:19:13PM +0100, Srinivas Kandagatla wrote:
> 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.
> 
> The codec exposes a single SimpleJack SDCA Function providing:
>   - Headphone playback via FU 6 (mute + Q7.8 volume) and OT 43/45.
>   - Headset mic capture via IT 33 with MICB2 bias derived from DT
>     (qcom,micbias2-microvolt).
>   - MBHC-based headset jack detection.
> 
> Implements:
>   - sdw_slave_ops.read_prop: SoundWire slave properties and dpn caps
>     for the compute-mode dataports.
>   - sdca_class_hw_ops.populate_function: fills the SDCA Function data
>     (entities, clusters, init_table) from static tables and patches
>     the IT 33 MIC_BIAS default with the DT-derived per-slave value.
> 
> Binds SoundWire slave id 0x0217:0x0110 when qcom,compute-mode is set
> on the DT node.
> 
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxxxxxxxx>
> ---

Both of these are pretty minor comments, more suggestions really
so feel free to ignore if you prefer way it is.

> +int wcd9378_sdca_read_prop(struct sdw_slave *slave)
> +{
> +	struct sdw_slave_prop *prop = &slave->prop;
> +	struct device *dev = &slave->dev;
> +	struct sdw_dpn_prop *sink, *src;
> +	int ret;
> +
> +	ret = sdw_slave_read_prop(slave);
> +	if (ret)
> +		return ret;

Do you want to call this? It is just reading a bunch of DT
properties you don't have, and if you are using some of them
they should probably go in the binding docs.

> +
> +	prop->use_domain_irq = true;
> +	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY |
> +			      SDW_SCP_INT1_IMPL_DEF;
> +
> +	/* Compute-mode fixed SoundWire slave properties (not described in DT) */
> +	prop->simple_clk_stop_capable = true;
> +	prop->paging_support = true;
> +	prop->clock_reg_supported = true;
> +	prop->lane_control_support = true;
> +
> +	/* Source ports: DP2 (headset mic), DP5 (optimisation TX). */
> +	prop->source_ports = BIT(2) | BIT(5);
> +	/* Sink ports: DP6 (HPH audio), DP7 (HPH envelope), DP8 (optimisation RX). */
> +	prop->sink_ports = BIT(6) | BIT(7) | BIT(8);
> +
> +	src = devm_kcalloc(dev, 2, sizeof(*src), GFP_KERNEL);
> +	if (!src)
> +		return -ENOMEM;
> +
> +	src[0].num = 2;
> +	src[0].type = SDW_DPN_SIMPLE;
> +	src[0].simple_ch_prep_sm = true;
> +	src[0].ch_prep_timeout = 10;
> +	src[0].max_ch = 1;
> +	src[0].min_ch = 1;
> +
> +	src[1].num = 5;
> +	src[1].type = SDW_DPN_SIMPLE;
> +	src[1].simple_ch_prep_sm = true;
> +	src[1].ch_prep_timeout = 10;
> +	src[1].max_ch = 1;
> +	src[1].min_ch = 1;
> +
> +	prop->src_dpn_prop = src;
> +
> +	sink = devm_kcalloc(dev, 3, sizeof(*sink), GFP_KERNEL);
> +	if (!sink)
> +		return -ENOMEM;
> +
> +	sink[0].num = 6;
> +	sink[0].type = SDW_DPN_SIMPLE;
> +	sink[0].simple_ch_prep_sm = true;
> +	sink[0].ch_prep_timeout = 10;
> +	sink[0].max_ch = 2;
> +	sink[0].min_ch = 1;
> +
> +	sink[1].num = 7;
> +	sink[1].type = SDW_DPN_FULL;
> +	sink[1].simple_ch_prep_sm = true;
> +	sink[1].ch_prep_timeout = 10;
> +	sink[1].max_ch = 1;
> +	sink[1].min_ch = 1;
> +
> +	sink[2].num = 8;
> +	sink[2].type = SDW_DPN_REDUCED;
> +	sink[2].simple_ch_prep_sm = true;
> +	sink[2].ch_prep_timeout = 10;
> +	sink[2].max_ch = 2;
> +	sink[2].min_ch = 1;

I would still be tempted to kmemdup static arrays for these
rather than kcallocing and filling in manually.

Thanks,
Charles