Re: [PATCH v2 2/3] ASoC: topology: Add topology core

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2236 bytes --]

On Mon, May 25, 2015 at 06:22:49PM +0100, Liam Girdwood wrote:

This looks really nice at a high level, obviously it's a lot of code so
there might be detail problems but I expect we can fix those up later.

> @@ -564,6 +565,7 @@ struct snd_soc_dapm_widget {
>  	int num_kcontrols;
>  	const struct snd_kcontrol_new *kcontrol_news;
>  	struct snd_kcontrol **kcontrols;
> +	struct snd_soc_dobj dobj;
>  
>  	/* widget input and outputs */
>  	struct list_head sources;

This adds the dynamic object (which isn't enormous but isn't tiny) to
every widget which could add up a bit.  I don't know if it's worth
making it a pointer to a dobj and allocating that separately?  OTOH it's
really routes rather than widgets that are the big cost, and it depends
on allocators being friendly and not putting lots of padding round
dobjs.  Definitely fixable incrementally if it is an issue though.

> +	/* external kcontrol init - used to set ext funcs + private data */
> +	int (*control_load)(struct snd_soc_component *,
> +		struct snd_kcontrol_new *, struct snd_soc_tplg_ctl_hdr *);
> +	int (*control_unload)(struct snd_soc_component *,
> +		struct snd_soc_dobj *);

Do we have examples of all these external users yet?  Just thinking
about what happens if we need a load/unload function but the kernel
doesn't know about this particular firmware type - the file didn't say
code would be needed and perhaps the code is optional anyway (some
optimisation or something).  Is that something that was thought about
yet, what sort of uses are we expecting?

> +/* pass vendor data to component driver for processing */
> +static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
> +	struct snd_soc_tplg_hdr *hdr)
> +{
> +	int ret = 0;
> +
> +	if (tplg->comp && tplg->ops && tplg->ops->vendor_load)
> +		ret = tplg->ops->vendor_load(tplg->comp, hdr);
> +
> +	if (ret < 0)
> +		dev_err(tplg->dev,
> +			"ASoC: vendor load failed at hdr offset %ld/0x%lx for type %d:%d\n",
> +			soc_tplg_get_hdr_offset(tplg),
> +			soc_tplg_get_hdr_offset(tplg),
> +			hdr->type, hdr->vendor_type);
> +	return ret;
> +}

For example I wonder if we should warn if we don't have a handler for
vendor data that we see?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]