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

[Charles Keepax]

On Mon, Sep 07, 2026 at 09:37:25AM +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.hw_init: enables supplies, toggles the reset
>     GPIO, and enables the vendor TX PDM clock via SCP.
>   - 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.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxxxxxxxx>
> ---
> +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 = sdca_class_read_prop(slave);
> +	if (ret)
> +		return ret;

I am not sure it is worth calling this. All this does that doesn't
come from the firmware is set use_domain_irq and scp_int1_mask,
I would just do both of those locally, calling this means you
technically process a large number of DT properties that aren't in
your binding doc and also you devm allocate a bunch of things you
won't use.

> +	/* 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;

Minor nit: Probably simpler to have a static array and kmemdup
it.

> +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;
> +
> +	/* Elevate is_volatile / has_reset to match the DisCo/ACPI path. */
> +	sdca_apply_default_control_classifiers(function);

Just factor those into the static tables, if we have a bunch of
static data might as well just fill it all in. Doing it this way
is confusing because the static data is incorrect then fixed up
later.

Thanks,
Charles