Re: [PATCH v2 05/11] ASoC: SDCA: add populate_function hw_op for DT function data

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 9/7/26 10:37, Srinivas Kandagatla wrote:
> sdca_parse_function() walks a firmware node, but on DT there is no
> DisCo node -- sdca_lookup_functions() is a no-op and the SDCA
> function descriptors come with a NULL fwnode.

It'd be good to have a link to an example set of DT properties for SDCA.
The way the functions were organized in ACPI is based on a DT-inspired
_DSD mechanism to have nested blocks.

edit: After reaching the last patch I realized there are no such
properties. The main problem is how to deal with board-specific
initialization data, the suggestion to encode all the tables in C seems
limited to me.

> Add a populate_function hw_op that the class function driver falls
> back to when @function->desc->node is NULL: it fills the caller-owned
> sdca_function_data (entities, clusters, init_table, delays) from the
> codec's static tables, matching by function type.  Leave
> @function->desc alone -- the framework owns the per-instance
> descriptor, so devices with more than one function of the same type
> keep their per-instance SoundWire address.

I wasn't able to understand that last sentence - and I don't remember
seeing a case with different functions of the same type. It's permitted
by the spec but it'd be fun to manage...

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
>  include/sound/sdca_class.h           | 6 ++++++
>  sound/soc/sdca/sdca_class_function.c | 9 ++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/sdca_class.h b/include/sound/sdca_class.h
> index 3342937d09fd..3a1b6d65048f 100644
> --- a/include/sound/sdca_class.h
> +++ b/include/sound/sdca_class.h
> @@ -26,9 +26,15 @@ struct sdca_function_data;
>   *           before the class regmap is created and before the slave is
>   *           ATTACHED; callers needing bus I/O must sdw_slave_wait_for_init()
>   *           first.
> + * @populate_function: fill @function (entities, clusters, init_table, ...)
> + *           from static tables in place of sdca_parse_function() on
> + *           DT/non-DisCo platforms.  Must leave @function->desc alone.
> + *           Return 0 on success or a negative errno.  May be NULL.
>   */
>  struct sdca_class_hw_ops {
>  	int (*hw_init)(struct sdw_slave *slave);
> +	int (*populate_function)(struct sdw_slave *slave,
> +				 struct sdca_function_data *function);
>  };

Erm, this populate_function() callback would not generate any hardware
access, would it? It's really different to hw_init which is supposed to
configure the codec and must run after enumeration.

I am struggling a bit with the 'hw_ops', it's really only a matter of
reading tables from platform firmware, and this could be done without
any dependency on hardware, couldn't it?
>  
>  struct sdca_class_drv {
> diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
> index 10a2b031b572..8bca88865a4d 100644
> --- a/sound/soc/sdca/sdca_class_function.c
> +++ b/sound/soc/sdca/sdca_class_function.c
> @@ -329,7 +329,14 @@ static int class_function_probe(struct auxiliary_device *auxdev,
>  	drv->core = core;
>  	drv->function = &sdev->function;
>  
> -	ret = sdca_parse_function(dev, drv->function);
> +	if (drv->function->desc->node) {
> +		ret = sdca_parse_function(dev, drv->function);
> +	} else if (core->hw_ops && core->hw_ops->populate_function) {
> +		ret = core->hw_ops->populate_function(core->sdw, drv->function);
> +	} else {
> +		dev_err(dev, "no firmware node and no populate_function hook\n");
> +		return -ENOENT;
> +	}
>  	if (ret)
>  		return ret;
>