Re: [alsa-devel] [RFC/RFT PATCH] ASoC: topology: Improve backwards compatibility with v4 topology files

[Pierre-Louis Bossart]

> > > +static int skl_tplg_get_pvt_data_v4(struct snd_soc_tplg_dapm_widget
> *tplg_w,
> > > +                                 struct skl *skl, struct device *dev,
> > > +                                 struct skl_module_cfg *mconfig)
> > > +{
> > > +     struct skl_dfw_v4_module *dfw =
> > > +                             (struct skl_dfw_v4_module
> *)tplg_w->priv.data;
> > > +     int ret;
> > > +
> > > +     dev_dbg(dev, "Parsing Skylake v4 widget topology data\n");
> > > +
> > > +     ret = guid_parse(dfw->uuid, (guid_t *)mconfig->guid);
> > > +     if (ret)
> > > +             return ret;
> > > +     mconfig->id.module_id = -1;
> > > +     mconfig->id.instance_id = dfw->instance_id;
> > > +     mconfig->module->resources[0].cps = dfw->max_mcps;
> > > +     mconfig->module->resources[0].ibs = dfw->ibs;
> > > +     mconfig->module->resources[0].obs = dfw->obs;
> > > +     mconfig->core_id = dfw->core_id;
> > > +     mconfig->module->max_input_pins = dfw->max_in_queue;
> > > +     mconfig->module->max_output_pins = dfw->max_out_queue;
> > > +     mconfig->module->loadable = dfw->is_loadable;
> > > +     skl_tplg_fill_fmt_v4(mconfig->module->formats[0].inputs,
> dfw->in_fmt,
> > > +                          MAX_IN_QUEUE);
> > > +     skl_tplg_fill_fmt_v4(mconfig->module->formats[0].outputs,
> dfw->out_fmt,
> > > +                          MAX_OUT_QUEUE);
> > Not clear to me if there is a confusion between MAX_IN_QUEUE and
> > MODULE_MAX_IN_PINS. The two values happen to be identical.
>
>
> The target (mconfig->module->formats[]) size is MAX_IN_QUEUE. Upstream
> v4.4/v4.5
> use both defines interchangeably as far as I can see.
>
> sound/soc/intel/skylake/skl-topology.h: struct skl_module_fmt
> in_fmt[MODULE_MAX_IN_PINS];
> sound/soc/intel/skylake/skl-tplg-interface.h:   struct skl_dfw_module_fmt
> in_fmt[MAX_IN_QUEUE];
>
> I could make it
>                    min(MAX_IN_QUEUE, dfw->max_in_queue)
> Would that be better ?

Looks like your code was fine in the first place.
> > > +
> > > +     mconfig->params_fixup = dfw->params_fixup;
> > > +     mconfig->converter = dfw->converter;
> > > +     mconfig->m_type = dfw->module_type;
> > > +     mconfig->vbus_id = dfw->vbus_id;
> > > +     mconfig->module->resources[0].is_pages = dfw->mem_pages;
> > > +
> > > +     ret = skl_tplg_add_pipe_v4(dev, mconfig, skl, &dfw->pipe);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     mconfig->dev_type = dfw->dev_type;
> > > +     mconfig->hw_conn_type = dfw->hw_conn_type;
> > > +     mconfig->time_slot = dfw->time_slot;
> > > +     mconfig->formats_config.caps_size = dfw->caps.caps_size;
>
> > chromeos-3.18 has this:
> >        if (dfw_config->is_loadable)
> >            memcpy(mconfig->guid, dfw_config->uuid,
> >                        ARRAY_SIZE(dfw_config->uuid));
>
> > Is this needed here?
>
>
> Direct memcpy doesn't work anymore since the uuid format is different. The
> above is replaced
> with (unconditional)
>
>           ret = guid_parse(dfw->uuid, (guid_t *)mconfig->guid);
>           if (ret)
>                   return ret;
>
> at the beginning of skl_tplg_get_pvt_data_v4(). The new code, as far as I
> can see, loads
> the uuid unconditionally if it finds SND_SOC_TPLG_TUPLE_TYPE_UUID. I wanted
> to
> be on the safe side and decided to do the same.

ok.