Re: [PATCH 02/14] ASoC: Intel: avs: Generic soc component driver

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> +struct avs_dma_data {
> +	struct avs_tplg_path_template *template;
> +	struct avs_path *path;
> +	/*
> +	 * link stream is stored within substream's runtime
> +	 * private_data to fulfill the needs of codec BE path
> +	 *
> +	 * host stream assigned

not able to parse that comment, what are you trying to say?

> +	 */
> +	struct hdac_ext_stream *host_stream;
> +};
> +
> +static ssize_t topology_name_read(struct file *file, char __user *user_buf, size_t count,
> +				  loff_t *ppos)
> +{
> +	struct snd_soc_component *component = file->private_data;
> +	struct snd_soc_card *card = component->card;
> +	struct snd_soc_acpi_mach *mach = dev_get_platdata(card->dev);
> +	char buf[64];
> +	size_t len;
> +
> +	len = snprintf(buf, sizeof(buf), "%s/%s\n", component->driver->topology_name_prefix,
> +		       mach->tplg_filename);
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static const struct file_operations topology_name_fops = {
> +	.open = simple_open,
> +	.read = topology_name_read,
> +	.llseek = default_llseek,
> +};

can you clarify why this is needed?

> +
> +static int avs_component_load_libraries(struct avs_soc_component *acomp)
> +{
> +	struct avs_tplg *tplg = acomp->tplg;
> +	struct avs_dev *adev = to_avs_dev(acomp->base.dev);
> +	int ret;
> +
> +	if (!tplg->num_libs)
> +		return 0;
> +
> +	/* Parent device may be asleep and library loading involves IPCs. */
> +	ret = pm_runtime_get_sync(adev->dev);
> +	if (ret < 0 && ret != -EACCES) {
> +		pm_runtime_put_noidle(adev->dev);
> +		return ret;
> +	}

Mark recommends the use of pm_runtime_resume_and_get(), see patches from today.

> +
> +	avs_hda_clock_gating_enable(adev, false);
> +	avs_hda_l1sen_enable(adev, false);
> +
> +	ret = avs_dsp_load_libraries(adev, tplg->libs, tplg->num_libs);
> +
> +	avs_hda_l1sen_enable(adev, true);
> +	avs_hda_clock_gating_enable(adev, true);
> +
> +	if (!ret)
> +		ret = avs_module_info_init(adev, false);
> +
> +	pm_runtime_mark_last_busy(adev->dev);
> +	pm_runtime_put_autosuspend(adev->dev);
> +
> +	return ret;
> +}
> +
> +static int avs_component_probe(struct snd_soc_component *component)
> +{
> +	struct snd_soc_card *card = component->card;
> +	struct snd_soc_acpi_mach *mach;
> +	struct avs_soc_component *acomp;
> +	struct avs_dev *adev;
> +	char *filename;
> +	int ret;
> +
> +	dev_dbg(card->dev, "probing %s card %s\n", component->name, card->name);
> +	mach = dev_get_platdata(card->dev);
> +	acomp = to_avs_soc_component(component);
> +	adev = to_avs_dev(component->dev);
> +
> +	acomp->tplg = avs_tplg_new(component);
> +	if (!acomp->tplg)
> +		return -ENOMEM;
> +
> +	if (!mach->tplg_filename)
> +		goto finalize;
> +
> +	/* Load specified topology and create sysfs for it. */
> +	filename = kasprintf(GFP_KERNEL, "%s/%s", component->driver->topology_name_prefix,
> +			     mach->tplg_filename);

what is the link between topology and sysfs?

> +	if (!filename)
> +		return -ENOMEM;
> +
> +	ret = avs_load_topology(component, filename);
> +	kfree(filename);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = avs_component_load_libraries(acomp);
> +	if (ret < 0) {
> +		dev_err(card->dev, "libraries loading failed: %d\n", ret);
> +		goto err_load_libs;
> +	}
> +
> +finalize:
> +	debugfs_create_file("topology_name", 0444, component->debugfs_root, component,
> +			    &topology_name_fops);

that's debugfs here, is this to make it possible to select an alternate topology file? If yes, a comment earlier wouldn't hurt.

> +
> +	mutex_lock(&adev->comp_list_mutex);
> +	list_add_tail(&acomp->node, &adev->comp_list);
> +	mutex_unlock(&adev->comp_list_mutex);
> +
> +	return 0;
> +
> +err_load_libs:
> +	avs_remove_topology(component);
> +	return ret;
> +}
> +


> +static const struct snd_soc_component_driver avs_component_driver = {
> +	.name			= "avs-pcm",
> +	.probe			= avs_component_probe,
> +	.remove			= avs_component_remove,
> +	.open			= avs_component_open,
> +	.pointer		= avs_component_pointer,
> +	.mmap			= avs_component_mmap,
> +	.pcm_construct		= avs_component_construct,
> +	.module_get_upon_open	= 1, /* increment refcount when a pcm is opened */
> +	.topology_name_prefix	= "intel/avs",

is this intentional that the firmware binaries and topologies will be stored in the same intel/avs directory?

> +	.non_legacy_dai_naming	= true,

is this needed? we've never used this for Intel?

> +};
> +