[alsa-devel] [PATCH v2 3/3] topology: A API calls to directly build topology data from templates

[Takashi Iwai]

On Mon, 10 Aug 2015 20:13:49 +0200,
Liam Girdwood wrote:
> +int tplg_add_bytes(snd_tplg_t *tplg, struct snd_tplg_bytes_template *bytes_ctl,
> +	struct tplg_elem **e)
> +{
> +	struct snd_soc_tplg_bytes_control *be;
> +	struct tplg_elem *elem;
> +	int ret;
> +
> +	tplg_dbg(" Control Bytes: %s\n", bytes_ctl->hdr.name);
> +
> +	if (bytes_ctl->hdr.type != SND_SOC_TPLG_TYPE_BYTES) {
> +		SNDERR("error: invalid bytes type %d\n", bytes_ctl->hdr.type);
> +		return -EINVAL;
> +	}
> +
> +	elem = tplg_elem_new_common(tplg, NULL, bytes_ctl->hdr.name,
> +		SND_TPLG_TYPE_BYTES);
> +	if (!elem)
> +		return -ENOMEM;
> +
> +	be = elem->bytes_ext;
> +	be->size = elem->size;
> +	ret = init_ctl_hdr(&be->hdr, &bytes_ctl->hdr);
> +	if (ret < 0) {
> +		tplg_elem_free(elem);
> +		return ret;
> +	}
> +
> +	be->max = bytes_ctl->max;
> +	be->mask = bytes_ctl->mask;
> +	be->base = bytes_ctl->base;
> +	be->num_regs = bytes_ctl->num_regs;
> +	be->ext_ops.put = bytes_ctl->ext_ops.put;
> +	be->ext_ops.get = bytes_ctl->ext_ops.get;
> +
> +	if (bytes_ctl->priv != NULL) {
> +		be = realloc(be,
> +			elem->size + bytes_ctl->priv->size);
> +		if (!be) {
> +			tplg_elem_free(elem);
> +			return -ENOMEM;
> +		}
> +		elem->bytes_ext = be;
> +		elem->size += bytes_ctl->priv->size;
> +
> +		memcpy(be->priv.data, bytes_ctl->priv->data,
> +			bytes_ctl->priv->size);
> +
> +		be->priv.size = bytes_ctl->priv->size;
> +	}
> +
> +	/* check on TLV bytes control */
> +	if (be->hdr.access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
> +		if (be->hdr.access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
> +			!= SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) {
> +			SNDERR("error: Invalid TLV bytes control access 0x%x\n",
> +				be->hdr.access);
> +			return -EINVAL;

Missing tplg_elem_free()?

> +struct tplg_elem* tplg_elem_new_route(snd_tplg_t *tplg)
> +{
> +	struct tplg_elem *elem;
> +	struct snd_soc_tplg_dapm_graph_elem *line;
> +
> +	elem = tplg_elem_new();
> +	if (!elem)
> +		return NULL;
> +
> +	list_add_tail(&elem->list, &tplg->route_list);
> +	strcpy(elem->id, "line");
> +	elem->type = SND_TPLG_TYPE_DAPM_GRAPH;
> +	elem->size = sizeof(*line);
> +
> +	line = calloc(1, sizeof(*line));
> +	if (!line)
> +		return NULL;

Missing free of elem in the error path.
Also, tplg->route_list still contains the stray elem.

I guess this was a bug in the original code, so you can address it
before this patch.


> +int tplg_add_widget_object(snd_tplg_t *tplg, snd_tplg_obj_template_t *t)
> +{
> +	struct snd_tplg_widget_template *wt = t->widget;
> +	struct snd_soc_tplg_dapm_widget *w;
> +	struct tplg_elem *elem;
> +	int i, ret = 0;
> +
> +	tplg_dbg("Widget: %s\n", wt->name);
> +
> +	elem = tplg_elem_new_common(tplg, NULL, wt->name,
> +		SND_TPLG_TYPE_DAPM_WIDGET);
> +	if (!elem)
> +		return -ENOMEM;
> +
> +	/* init new widget */
> +	w = elem->widget;
> +	w->size = elem->size;
> +
> +	w->id = wt->id;
> +	elem_copy_text(w->name, wt->name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
> +	if (wt->sname)
> +		elem_copy_text(w->sname, wt->sname,
> +		SNDRV_CTL_ELEM_ID_NAME_MAXLEN);

Indentation problem.

> +	/* add controls to the widget's reference list */
> +	for (i = 0 ; i < wt->num_ctls; i++) {
> +		struct snd_tplg_ctl_template *ct = wt->ctl[i];
> +		struct tplg_elem *elem_ctl;
> +		struct snd_tplg_mixer_template *mt;
> +		struct snd_tplg_bytes_template *bt;
> +		struct snd_tplg_enum_template *et;
> +
> +		if (!ct) {
> +			tplg_elem_free(elem);
> +			return -EINVAL;
> +		}
> +
> +		switch (ct->type) {
> +		case SND_SOC_TPLG_TYPE_MIXER:
> +			mt = container_of(ct, struct snd_tplg_mixer_template, hdr);
> +			ret = tplg_add_mixer(tplg, mt, &elem_ctl);
> +			break;
> +
> +		case SND_SOC_TPLG_TYPE_BYTES:
> +			bt = container_of(ct, struct snd_tplg_bytes_template, hdr);
> +			ret = tplg_add_bytes(tplg, bt, &elem_ctl);
> +			break;
> +
> +		case SND_SOC_TPLG_TYPE_ENUM:
> +			et = container_of(ct, struct snd_tplg_enum_template, hdr);
> +			ret = tplg_add_enum(tplg, bt, &elem_ctl);

et instead of bt?

> +int snd_tplg_build(snd_tplg_t *tplg, const char *outfile)
> +{
> +	int err;
> +
> +	/* delete any old output files */
> +	unlink(outfile);
> +
> +	tplg->out_fd =
> +		open(outfile, O_RDWR | O_CREAT, S_IRWXU | S_IRWXG | S_IRWXO);
> +	if (tplg->out_fd < 0) {
> +		SNDERR("error: failed to open %s err %d\n",
> +			outfile, -errno);
> +		return -errno;
> +	}
> +
> +	err = tplg_build_integ(tplg);
> +	if (err < 0) {
> +		SNDERR("error: failed to check topology integrity\n");
> +		goto out;
> +	}
> +
> +	err = tplg_write_data(tplg);
> +	if (err < 0) {
> +		SNDERR("error: failed to write data %d\n", err);
> +		goto out;
> +	}
> +
> +out:
> +	close(tplg->out_fd);

It's a question whether we should unlink automatically at error or
not.  If we leave the file as is, it should be documented.

> diff --git a/src/topology/tplg_local.h b/src/topology/tplg_local.h
> index 953d822..97ee33e 100644
> --- a/src/topology/tplg_local.h
> +++ b/src/topology/tplg_local.h
> @@ -34,6 +34,10 @@
>  #define ALSA_TPLG_DIR	ALSA_CONFIG_DIR "/topology"
>  #define ARRAY_SIZE(x) (sizeof(x) / sizeof(x[0]))
>  
> +#define container_of(ptr, type, member) ({                      \
> +	 const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
> +	(type *)( (char *)__mptr - offsetof(type,member) );})

This is a pretty common macro, so I prefer having it in
include/local.h.


thanks,

Takashi