Re: [PATCH v2 05/13] topology: Add PCM parser.

From: Takashi Iwai <tiwai@suse.de>
To: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>,
	alsa-devel@alsa-project.org, Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH v2 05/13] topology: Add PCM parser.
Date: Wed, 01 Jul 2015 18:14:47 +0200	[thread overview]
Message-ID: <s5hbnfvlurs.wl-tiwai@suse.de> (raw)
In-Reply-To: <1435758275-4047-5-git-send-email-liam.r.girdwood@linux.intel.com>

At Wed,  1 Jul 2015 14:44:27 +0100,
Liam Girdwood wrote:
> 
> +/* copy referenced caps to the pcm */
> +static void copy_pcm_caps(const char *id, struct snd_soc_tplg_stream_caps *caps,
> +	struct tplg_elem *ref_elem)
> +{
> +	struct snd_soc_tplg_stream_caps *ref_caps = ref_elem->stream_caps;
> +
> +	tplg_dbg("Copy pcm caps (%ld bytes) from '%s' to '%s' \n",
> +		sizeof(*caps), ref_elem->id, id);
> +
> +	memcpy((void*)caps, ref_caps, sizeof(*caps));

This can be simply
	*caps = *ref_caps;

> +/* copy referenced config to the pcm */
> +static void copy_pcm_config(const char *id,
> +	struct snd_soc_tplg_stream_config *cfg, struct tplg_elem *ref_elem)
> +{
> +	struct snd_soc_tplg_stream_config *ref_cfg = ref_elem->stream_cfg;
> +
> +	tplg_dbg("Copy pcm config (%ld bytes) from '%s' to '%s' \n",
> +		sizeof(*cfg), ref_elem->id, id);
> +
> +	memcpy((void*)cfg, ref_cfg, sizeof(*cfg));

Ditto.

> +int tplg_build_pcm_dai(snd_tplg_t *tplg, unsigned int type)
> +{
> +	struct list_head *base, *pos, *npos;
> +	struct tplg_elem *elem;
> +	int err = 0;
> +
> +	switch (type) {
> +	case PARSER_TYPE_PCM:
> +		base = &tplg->pcm_list;
> +		break;
> +	case PARSER_TYPE_BE:
> +		base = &tplg->be_list;
> +		break;
> +	case PARSER_TYPE_CC:
> +		base = &tplg->cc_list;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	list_for_each_safe(pos, npos, base) {

Does it need to be the safe version?

> +		elem = list_entry(pos, struct tplg_elem, list);
> +		if (elem->type != type) {
> +			fprintf(stderr, "error: invalid elem '%s'\n", elem->id);
> +			return -EINVAL;
> +		}
> +
> +		err = tplg_build_pcm_cfg_caps(tplg, elem);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +/* PCM stream configuration
> + *
> + * Describes the PCM configuration for playback and capture streams.
> + *
> + *	config."name" {
> + *		format "S24_LE"
> + *		rate "48000"
> + *		channels "2"
> + *		tdm_slot "0xf"
> + *      }

Such a description should go to doxygen comment, no?
Also, it'd be better to mention more clearly that "name" here
corresponds to the stream direction.

> +int tplg_parse_pcm(snd_tplg_t *tplg,
> +	snd_config_t *cfg, void *private ATTRIBUTE_UNUSED)
> +{
> +	struct snd_soc_tplg_pcm_dai *pcm_dai;
> +	struct tplg_elem *elem;
> +	snd_config_iterator_t i, next;
> +	snd_config_t *n;
> +	const char *id, *val = NULL;
> +	int err;
> +
> +	elem = tplg_elem_new_common(tplg, cfg, PARSER_TYPE_PCM);
> +	if (!elem)
> +		return -ENOMEM;
> +
> +	pcm_dai = elem->pcm;
> +	pcm_dai->size = elem->size;
> +	strncpy(pcm_dai->name, elem->id, SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
> +	pcm_dai->name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN - 1] = 0;
> +
> +	tplg_dbg(" PCM: %s\n", elem->id);
> +
> +	snd_config_for_each(i, next, cfg) {
> +
> +		n = snd_config_iterator_entry(i);
> +		if (snd_config_get_id(n, &id) < 0)
> +			continue;
> +
> +		/* skip comments */
> +		if (strcmp(id, "comment") == 0)
> +			continue;
> +		if (id[0] == '#')
> +			continue;
> +
> +		if (strcmp(id, "index") == 0) {
> +			if (snd_config_get_string(n, &val) < 0)
> +				return -EINVAL;
> +
> +			elem->index = atoi(val);
> +			tplg_dbg("\t%s: %d\n", id, elem->index);
> +			continue;
> +		}
> +
> +		if (strcmp(id, "ID") == 0) {

Only this word is capital letters while others are not?
I don't mind too much, but just wondered, because currently the parser
is case-sensitive.

Takashi