On 2022-04-26 11:33 PM, Pierre-Louis Bossart wrote:
+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 assignednot able to parse that comment, what are you trying to say?
Sure. This actually stems from the legacy driver architecture. PCM operations for legacy HD-Audio are found in sound/pci/hda/hda_controller.c with respective types declared in sound/pci/hda/hda_controller.h. Operations found there make use of struct azx_dev and get_azx_dev(). So, to be able to re-use sound/pci/hda as much as possible plus declare virtually no custom HD-Audio related logic, we satisfy the needs of lower level code by assigning BE stream to substream->runtime->private_data.
As HD-Audio legacy operates in coupled mode only, there is no need for it to differentiate between HOST and LINK side. It's not true for DSP configurations though. So, we declare separate pointer for HOST stream allowing PCM operations found here to have easy access to both, LINK and HOST streams whenever necessary.
*/- 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?
The usage of default_llseek() or the topology_name_fops as a whole? The latter is here to obtain name of the firmware file requested by given machine board easily from the debugfs. The former is probably just a copy-paste from other declarations of file_operation entries for READ-only files.
+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.
Will definitely check this out, thanks for pointing this out.
- 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?
This comment is misleading as debugfs entry is being created as seen in the code below.
To answer the question assuming s/sysfs/debugfs/:
Ability to allow for reading name of the file requested by given machine board from user space. AVS driver spawns multiple machine boards, each requesting a topology file. To make it easier for the validation and normal users to understand what has requested what, we provide a simple, read-only debugfs entry for each of the boards within the avs tree.
- 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.
Nah, its purpose is very basic: get the exact name of the topology file loaded. Example:
intel/avs/hda-8086280b-tplg.bin
- 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?
I'll recheck this but back in time when we drawn PCM ops for the first time there was a reason, certainly. Out of my head - impacts behavior of snd_soc_register_dais().
+};