On 2020-05-26 4:45 PM, Keyon Jie wrote:
On 5/26/20 8:38 PM, Cezary Rojewski wrote:
On 2020-05-21 9:48 AM, Keyon Jie wrote:
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
Another option, if you want to reduce assignment count, is to keep soc_pass_load while still removing the private map. Said soc_pass_load would require declaration update to accept function ptr on top of what's already there.
In consequence, soc_tplg_load_header becomes a switch-case with a bunch of case X: return soc_pass_load(tplg, hdr, pass=_MY_PASS (e.g. SOC_TPLG_PASS_VENDOR), elem_load=_MY_LOAD_FUNC (e.g. soc_tplg_vendor_load))
Czarek