[PATCH v6 18/22] ASoC: qdsp6: audioreach: add topology support

[Srinivas Kandagatla]

Thanks Pierre for such quick review,

On 15/09/2021 17:22, Pierre-Louis Bossart wrote:
> 
>> +static int audioreach_widget_load_buffer(struct snd_soc_component *component,
>> +					 int index, struct snd_soc_dapm_widget *w,
>> +					 struct snd_soc_tplg_dapm_widget *tplg_w)
>> +{
>> +	struct snd_soc_tplg_vendor_array *mod_array;
>> +	struct audioreach_module *mod;
>> +	struct snd_soc_dobj *dobj;
>> +	int ret;
>> +
>> +	ret = audioreach_widget_load_module_common(component, index, w, tplg_w);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dobj = &w->dobj;
>> +	mod = dobj->private;
>> +
>> +	mod_array = audioreach_get_module_array(&tplg_w->priv);
>> +
>> +	switch (mod->module_id) {
>> +	case MODULE_ID_CODEC_DMA_SINK:
>> +	case MODULE_ID_CODEC_DMA_SOURCE:
>> +		audioreach_widget_dma_module_load(mod, mod_array);
>> +		break;
>> +	case MODULE_ID_DATA_LOGGING:
>> +		audioreach_widget_log_module_load(mod, mod_array);
>> +		break;
>> +	case MODULE_ID_I2S_SINK:
>> +	case MODULE_ID_I2S_SOURCE:
>> +		audioreach_widget_i2s_module_load(mod, mod_array);
>> +		break;
> 
> no default case?

Thanks for spotting this, its fixed now in next version.
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 
>> +int audioreach_tplg_init(struct snd_soc_component *component)
>> +{
>> +	struct device *dev = component->dev;
>> +	const struct firmware *fw;
>> +	int ret;
>> +
>> +	ret = request_firmware(&fw, "audioreach.bin", dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "tplg fw audioreach.bin load failed with %d\n", ret);
>> +		return ret;
>> +	}
> 
> How does this work if you want to change the topology, which will happen
> rather frequently if you have a framework precisely to change the DSP
> graph? You need to override a file in userspace?
> 
> Shouldn't you have a means to identify what topology file you want on a
> platform-basis?
> 
> Or at the very least a means to change the file name with a kernel
> parameter or something.

I totally agree, I was planning to do that as a next step. But now that 
you pointed it out, I can take a look at SOF for some ideas and add it 
in next version.

--srini
> 
>> +
>> +	ret = snd_soc_tplg_component_load(component, &audioreach_tplg_ops, fw);
>> +	if (ret < 0) {
>> +		dev_err(dev, "tplg component load failed%d\n", ret);
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	release_firmware(fw);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(audioreach_tplg_init);
>>