All of lore.kernel.org
 help / color / mirror / Atom feed
From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Keyon Jie <yang.jie@linux.intel.com>, alsa-devel@alsa-project.org
Cc: tiwai@suse.de, broonie@kernel.org,
	pierre-louis.bossart@linux.intel.com,
	ranjani.sridharan@linux.intel.com
Subject: Re: [PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass
Date: Tue, 26 May 2020 14:38:32 +0200	[thread overview]
Message-ID: <4c05fcc4-2f94-1ca0-2148-af70ef739d00@intel.com> (raw)
In-Reply-To: <20200521074847.71406-2-yang.jie@linux.intel.com>

On 2020-05-21 9:48 AM, Keyon Jie wrote:
> The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is
> comparing two different enums, refine the element loading functions, and
> log the information when the header is being parsed in the corresponding
> parsing pass.
> 
> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
> ---
>   sound/soc/soc-topology.c | 53 +++++++++++++++++++++++++++++++---------
>   1 file changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 49875978a1ce..70c0ff7ce13f 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -2636,6 +2636,17 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
>   	return ret;
>   }
>   
> +static int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr *) = {
> +	[SOC_TPLG_PASS_MANIFEST]	= soc_tplg_manifest_load,
> +	[SOC_TPLG_PASS_VENDOR]		= soc_tplg_vendor_load,
> +	[SOC_TPLG_PASS_MIXER]		= soc_tplg_kcontrol_elems_load,
> +	[SOC_TPLG_PASS_WIDGET]		= soc_tplg_dapm_widget_elems_load,
> +	[SOC_TPLG_PASS_PCM_DAI]		= soc_tplg_pcm_elems_load,
> +	[SOC_TPLG_PASS_GRAPH]		= soc_tplg_dapm_graph_elems_load,
> +	[SOC_TPLG_PASS_BE_DAI]		= soc_tplg_dai_elems_load,
> +	[SOC_TPLG_PASS_LINK]		= soc_tplg_link_elems_load,
> +};
> +

Do we really need private array for this? There is a separate 
soc_tplg_load_header function already. By separate, I mean an isolate 
scope for single task only - validating tplg headers. While I agree with 
the idea behind second patch of the series - removal of individual 
checks in favor of generic one - I believe said checks could be 
"inlined". See below.

>   /* validate header magic, size and type */
>   static int soc_valid_header(struct soc_tplg *tplg,
>   	struct snd_soc_tplg_hdr *hdr)
> @@ -2685,19 +2696,31 @@ static int soc_valid_header(struct soc_tplg *tplg,
>   		return -EINVAL;
>   	}
>   
> -	if (tplg->pass == le32_to_cpu(hdr->type))
> +	return 1;
> +}
> +
> +/* check and load the element for the current pass */
> +static int soc_pass_load(struct soc_tplg *tplg,
> +			 struct snd_soc_tplg_hdr *hdr,
> +			 unsigned int hdr_pass)
> +{
> +	if (tplg->pass == hdr_pass) {
>   		dev_dbg(tplg->dev,
>   			"ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n",
>   			hdr->payload_size, hdr->type, hdr->version,
>   			hdr->vendor_type, tplg->pass);
> +		return elem_load[hdr_pass](tplg, hdr);
> +	}
>   
> -	return 1;
> +	return 0;
>   }

Remove the function and merge its body into soc_tplg_load_header.

>   
>   /* check header type and call appropriate handler */
>   static int soc_tplg_load_header(struct soc_tplg *tplg,
>   	struct snd_soc_tplg_hdr *hdr)
>   {
> +	unsigned int hdr_pass;
> +
>   	tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
>   
>   	/* check for matching ID */
> @@ -2711,27 +2734,35 @@ static int soc_tplg_load_header(struct soc_tplg *tplg,
>   	case SND_SOC_TPLG_TYPE_MIXER:
>   	case SND_SOC_TPLG_TYPE_ENUM:
>   	case SND_SOC_TPLG_TYPE_BYTES:
> -		return soc_tplg_kcontrol_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_MIXER;
> +		break;
>   	case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
> -		return soc_tplg_dapm_graph_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_GRAPH;
> +		break;
>   	case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
> -		return soc_tplg_dapm_widget_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_WIDGET;
> +		break;
>   	case SND_SOC_TPLG_TYPE_PCM:
> -		return soc_tplg_pcm_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_PCM_DAI;
> +		break;
>   	case SND_SOC_TPLG_TYPE_DAI:
> -		return soc_tplg_dai_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_BE_DAI;
> +		break;
>   	case SND_SOC_TPLG_TYPE_DAI_LINK:
>   	case SND_SOC_TPLG_TYPE_BACKEND_LINK:
>   		/* physical link configurations */
> -		return soc_tplg_link_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_LINK;
> +		break;
>   	case SND_SOC_TPLG_TYPE_MANIFEST:
> -		return soc_tplg_manifest_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_MANIFEST;
> +		break;
>   	default:
>   		/* bespoke vendor data object */
> -		return soc_tplg_vendor_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_VENDOR;
> +		break;
>   	}
>   
> -	return 0;
> +	return soc_pass_load(tplg, hdr, hdr_pass);

By having "log" code here we have one place for hdr validation, rather 
than two (the second being just an "if" to be fair..) and private array 
is no longer necessary. Local func ptr variable would take care of 
storing adequate function to call.

>   }
>   
>   /* process the topology file headers */
> 

  reply	other threads:[~2020-05-26 12:39 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21  7:48 [PATCH v2 0/2] ASoC topology header parsing refinement Keyon Jie
2020-05-21  7:48 ` [PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass Keyon Jie
2020-05-26 12:38   ` Cezary Rojewski [this message]
2020-05-26 14:45     ` Keyon Jie
2020-05-26 15:30       ` Cezary Rojewski
2020-05-27  1:17         ` Keyon Jie
2020-05-21  7:48 ` [PATCH v2 2/2] ASoC: topology: remove the redundant pass checks Keyon Jie
2020-05-26 12:40 ` [PATCH v2 0/2] ASoC topology header parsing refinement Cezary Rojewski
2020-05-26 13:29   ` Pierre-Louis Bossart
2020-05-27  1:05     ` Keyon Jie
2020-05-26 14:34   ` Keyon Jie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4c05fcc4-2f94-1ca0-2148-af70ef739d00@intel.com \
    --to=cezary.rojewski@intel.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=ranjani.sridharan@linux.intel.com \
    --cc=tiwai@suse.de \
    --cc=yang.jie@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.