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

[Keyon Jie <yang.jie@linux.intel.com>]

On 5/26/20 11:30 PM, Cezary Rojewski wrote:
> 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))

I would prefer to use the assignment one, let me update the series now.

Thanks,
~Keyon

> 
> Czarek