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

[Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>]

Thanks Pierre for review,

On 9/7/26 12:32 PM, Pierre-Louis Bossart wrote:
> On 9/7/26 10:37, 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.
> 
> wow, I realize now I completely misunderstood what this whole endeavor
> was about. I *thought* the point was to read the information about all
> the functions from Device Tree tables. I now understand there are no
> such tables, all the information is encoded in C as part of the
> higher-level codec driver.
> 
> Is this really intended?
Unfortunately Yes.

> I mean, the whole ACPI set of definitions relied on the _DSD mechanism
> that mimics what DT provides. Do we really want all this information in
> C? Why not have a set of DT properties for each function?
> 

We discussed this topic at LPC 2025, Devicetree MC:
    "DeviceTrees - MIPI SoundWire Device Class for Audio (SDCA) and
     classic ACPI-DT problem"
    https://lpc.events/event/19/contributions/2024/

Among other options presented, representing them in a intermediate
format was something which was doable.

I have also proposed another follow up of this topic in this years LPC
Devicetree MC too

RFC of this patchset got some comments from DT maitainers.

DT maintainers are not happy with the idea of keeping this info in DT
while it can be derived from compatible string.

https://lkml.org/lkml/2026/7/29/1166

> I guess my main objection is for opaque initialization data aka blind
> writes or SWF table, this should really come from platform firmware, no?
This table is directly generated from ACPI tables both from Lenovo T14
and Reference platform.

on ARM platforms DT is is the only firmware entry for such things and
its not 1:1 with ACPI example, Somethings that can be derived can not be
in Device tree description so its bit of mix.

> With this approach you'd have an endless set of kernel quirks for each
> board variant using the same codec.

@Krzysztof Kozlowski <krzk+dt@kernel.org> that is a valid point.

Idea is to gate them using platform specific compatibles, so far we have
few laptops that are pretty much identical w.r.t the description, may be
we got lucky in this early stages.

In future if it turns out to need a quirks per platform or changes
needed in this table then we should be able to handle it with platform
specific device compatibles.



>> +static int wcd9378_sdca_hw_init(struct sdw_slave *slave)
>> +{
>> +	struct device *dev = &slave->dev;
>> +	struct gpio_desc *reset;
>> +	int ret;
>> +
>> +	/* No SPMI parent: supplies and reset live on the SoundWire DT node. */
>> +	ret = devm_regulator_bulk_get_enable(dev,
>> +					     ARRAY_SIZE(wcd9378_sdca_supplies),
>> +					     wcd9378_sdca_supplies);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to enable supplies\n");
>> +
>> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(reset))
>> +		return dev_err_probe(dev, PTR_ERR(reset),
>> +				     "failed to get reset GPIO\n");
>> +
>> +	if (reset) {
>> +		gpiod_set_value(reset, 1);
>> +		usleep_range(20, 30);
>> +		gpiod_set_value(reset, 0);
>> +		usleep_range(20, 30);
>> +	}
> 
> Could this be part of wcd9378_sdca_probe()

Good point, I have now moved this to probe and eliminates need of
hw_init callback all together, I can fold that in next version.


--srini
>> +
>> +	/* SCP writes below need the slave attached. */
>> +	ret = sdw_slave_wait_for_init(slave, 5000);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "slave attach timeout: %d\n", ret);
> 
> That wait doesn't seem required, if you are using the existing class
> probe there's already a wait?
>> +
>> +	/*
>> +	 * TX PDM clock: bank-1 shadow + SCP_COMMIT. SCP survives PDE
>> +	 * cycles; one-shot at hw_init before any port is enabled.
>> +	 */
>> +	ret = sdw_write_no_pm(slave, WCD9378_SCP_HOST_CLK_DIV2_CTL_B1, 0x01);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "HOST_CLK_DIV2_CTL_B1: %d\n", ret);
>> +
>> +	ret = sdw_write_no_pm(slave, SDW_SCP_COMMIT, 0x02);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "SCP_COMMIT: %d\n", ret);
>> +
>> +	return 0;
> 
> and this could also be done in the existing .status callback upon
> enumeration.
> 
> In other words the need for this hw_init() isn't very clear to me...
> 
>> +}
>> +
>> +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);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct sdca_class_hw_ops wcd9378_sdca_hw_ops = {
>> +	.hw_init           = wcd9378_sdca_hw_init,
>> +	.populate_function = wcd9378_sdca_populate_function,
>> +};
>> +
>> +int wcd9378_sdca_probe(struct sdw_slave *slave,
>> +		       const struct sdw_device_id *id)
>> +{
>> +	struct device *dev = &slave->dev;
>> +	struct sdca_device_data *data = &slave->sdca_data;
>> +	struct wcd9378_priv *priv;
>> +
>> +	/*
>> +	 * 0x0217:0x0110 covers both mobile and compute modes; the
>> +	 * qcom,wcd9378c variant compatible identifies compute-mode
>> +	 * nodes only.  Mobile-mode nodes carry the plain class-ID
>> +	 * compatible and are picked up by the mobile driver.
>> +	 */
>> +	if (!device_is_compatible(dev, "qcom,wcd9378c"))
>> +		return -ENODEV;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	dev_set_drvdata(dev, priv);
>> +
>> +	/* DT has no DisCo enumeration; seed the descriptor here. */
>> +	if (!data->num_functions) {
>> +		data->function[0].type = wcd9378_sdca_desc.type;
>> +		data->function[0].adr  = wcd9378_sdca_desc.adr;
>> +		data->function[0].name = wcd9378_sdca_desc.name;
>> +		data->num_functions    = 1;
>> +	}
>> +
>> +	return sdca_class_probe(slave, &priv->class, &wcd9378_sdca_hw_ops);
>> +}