-
Notifications
You must be signed in to change notification settings - Fork 140
ASoC: Intel: add I2S function topology support #5657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: topic/sof-dev
Are you sure you want to change the base?
Conversation
ujfalusi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only checked the first patch
| } | ||
|
|
||
| dev_err(dev, "%s: No match for SSP%d in NHLT table\n", __func__, | ||
| dev_dbg(dev, "%s: No match for SSP%d in NHLT table\n", __func__, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have this as separate patch?
we are also going to look for DMIC blobs in several nhlt, so change those prints as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the DMIC blobs. Maybe we can change the DMIC prints when we look for DMIC blobs in several nhlt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we do after the second patch...
There could be NHLT blobs w/o DMIC (SSP fragment) and we would print error/warn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intel_nhlt_get_endpoint_blob() will be called for each NHLT blobs, but it uses dev_dbg already.
sound/soc/sof/ipc4-priv.h
Outdated
| struct snd_ipc4_nhlt { | ||
| struct list_head list; | ||
| void *nhlt; | ||
| size_t nhlt_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nhlt_size is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
sound/soc/sof/ipc4-priv.h
Outdated
| size_t nhlt_size; | ||
| }; | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
sound/soc/sof/ipc4-topology.c
Outdated
| channel_count, sample_rate, | ||
| dir, dev_type); | ||
| if (cfg) | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goto out; and remove the cfg check after the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
sound/soc/sof/ipc4-topology.c
Outdated
| struct snd_ipc4_nhlt *tplg_nhlt; | ||
|
|
||
| /* Get the nhlt from topology manifest*/ | ||
| tplg_nhlt = kzalloc(sizeof(*tplg_nhlt), GFP_KERNEL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what is missing in hda-dai.c!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but you need devm_kzalloc() to leak memory..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to devm_kzalloc()
a0fd522 to
8d4c0c3
Compare
ujfalusi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bardliao, looks really nice, I have few nitpicks and one bigger question only
| } | ||
|
|
||
| dev_err(dev, "%s: No match for SSP%d in NHLT table\n", __func__, | ||
| dev_dbg(dev, "%s: No match for SSP%d in NHLT table\n", __func__, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we do after the second patch...
There could be NHLT blobs w/o DMIC (SSP fragment) and we would print error/warn?
sound/soc/sof/ipc4-topology.c
Outdated
| dai_index); | ||
| if (dev_type < 0) | ||
| list_for_each_entry(nhlt, &ipc4_data->nhlt_list, list) { | ||
| dev_type = intel_nhlt_ssp_device_type(sdev->dev, nhlt->nhlt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: nhlt->nhlt, can we do entry->nhlt instead? the outer nhlt is not nhlt as such.
| cfg = intel_nhlt_get_endpoint_blob(sdev->dev, nhlt->nhlt, dai_index, | ||
| nhlt_type, bit_depth, bit_depth, | ||
| channel_count, sample_rate, dir, | ||
| dev_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, this will run for DMIC also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and it uses dev_dbg() already.
| EXPORT_SYMBOL_GPL(sof_sdw_get_tplg_files); | ||
|
|
||
| int sof_i2s_get_tplg_files(struct snd_soc_card *card, const struct snd_soc_acpi_mach *mach, | ||
| const char *prefix, const char ***tplg_files, bool best_effort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you plan to support the best_effort with SSP machines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure
| tplg_dev = TPLG_DEVICE_HDMI; | ||
| tplg_dev_name = "hdmi-pcm5"; | ||
|
|
||
| } else if (strstr(dai_link->name, "SSP")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove the extra line on front
| if (!(*tplg_files)[tplg_num]) | ||
| return -ENOMEM; | ||
| tplg_num++; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my untrained eyes this looks like identical to what sof_sdw_get_tplg_files() would do, just that this lacks the SDW part, no?
Why not rename that function ad use it for all types as universal tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about it. And yes, they look quite similar, but the existing topologies may use different PCM IDs for different machines. I think it would be better to use separated function to keep the flexibility to have different function topology names for different machines.
Each function topology has its own NHLT blob sections. And we need to look for the matching blob from all the NHLT blobs. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
intel_nhlt_ssp_device_type() is called multiple times with different nhlt. We will find the SSP device type from one of the nhlt. It is not an error if one of the nhlt doesn't contain the SSP endpoint. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
The existing code supports get_function_tplg_files callback for SoundWire machine driver only. Some common sections can be used to extend the support to other machines. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
…et_tplg_files The Intel SOF SDW machine drive also supports I2S interface. Add related supports for the sof_sdw_get_tplg_files() callback. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
… I2S machines Add sof_i2s_get_tplg_files() callback for Intel SOF I2S machines. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Use sof_i2s_get_tplg_files() for SOF es83x6 machines. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
8d4c0c3 to
1af3d92
Compare
| ipc4_data->nhlt = devm_kmemdup(sdev->dev, manifest_tlv->data, | ||
| le32_to_cpu(manifest_tlv->size), GFP_KERNEL); | ||
| if (!ipc4_data->nhlt) | ||
| struct snd_ipc4_nhlt *tplg_nhlt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bardliao this is a slight departure from what we do currently right? If there's a BIOS NHLT, we never bother with the topology NHLT even if it exists. But now we parse it add it to the list but after the BIOS NHLT? Will this change the expected behaviour on ac device with both NHLTs present?
Currently, function topology is only used by the SoundWire machines. Extend the support to the I2S machines.