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

[Pierre-Louis Bossart]

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?
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?

I guess my main objection is for opaque initialization data aka blind
writes or SWF table, this should really come from platform firmware, no?
With this approach you'd have an endless set of kernel quirks for each
board variant using the same codec.
> +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()
> +
> +	/* 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);
> +}