[PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass
Keyon Jie
yang.jie at linux.intel.com
Tue May 26 16:45:33 CEST 2020
On 5/26/20 8:38 PM, Cezary Rojewski wrote:
> 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 at 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 */we
>> 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.
Hi Cezary, so what you suggested above is changing the
soc_tplg_load_header() to be something like this, right?
static int soc_tplg_load_header(struct soc_tplg *tplg,
struct snd_soc_tplg_hdr *hdr)
{
unsigned int hdr_pass;
int (*elem_load)(struct soc_tplg *, struct snd_soc_tplg_hdr *);
tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
/* check for matching ID */
if (le32_to_cpu(hdr->index) != tplg->req_index &&
tplg->req_index != SND_SOC_TPLG_INDEX_ALL)
return 0;
tplg->index = le32_to_cpu(hdr->index);
switch (le32_to_cpu(hdr->type)) {
case SND_SOC_TPLG_TYPE_MIXER:
case SND_SOC_TPLG_TYPE_ENUM:
case SND_SOC_TPLG_TYPE_BYTES:
hdr_pass = SOC_TPLG_PASS_MIXER;
elem_load = soc_tplg_kcontrol_elems_load;
break;
case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
hdr_pass = SOC_TPLG_PASS_GRAPH;
elem_load = soc_tplg_dapm_graph_elems_load;
break;
case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
hdr_pass = SOC_TPLG_PASS_WIDGET;
elem_load = soc_tplg_dapm_widget_elems_load;
break;
case SND_SOC_TPLG_TYPE_PCM:
hdr_pass = SOC_TPLG_PASS_PCM_DAI;
elem_load = soc_tplg_pcm_elems_load;
break;
case SND_SOC_TPLG_TYPE_DAI:
hdr_pass = SOC_TPLG_PASS_BE_DAI;
elem_load = soc_tplg_dai_elems_load;
break;
case SND_SOC_TPLG_TYPE_DAI_LINK:
case SND_SOC_TPLG_TYPE_BACKEND_LINK:
/* physical link configurations */
hdr_pass = SOC_TPLG_PASS_LINK;
elem_load = soc_tplg_link_elems_load;
break;
case SND_SOC_TPLG_TYPE_MANIFEST:
hdr_pass = SOC_TPLG_PASS_MANIFEST;
elem_load = soc_tplg_manifest_load;
break;
default:
/* bespoke vendor data object */
hdr_pass = SOC_TPLG_PASS_VENDOR;
elem_load = soc_tplg_vendor_load;
break;
}
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(tplg, hdr);
}
return 0;
}
I am also fine with this, though I thought my previous version looks
more organized and not so error-prone as we need 8 more assignation here.
Mark, Pierre, preference about this?
Thanks,
~Keyon
>
>> }
>> /* process the topology file headers */
>>
More information about the Alsa-devel
mailing list