Re: [PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass

[Cezary Rojewski <cezary.rojewski@intel.com>]

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