-
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.
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 need to call out that in preparation for supporting multiple topology fragments per function which could end up calling this function multiple types with NHLT blobs from different fragments, downgrade the error message to debug level.
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.
Commit message updated. But do you mean to move the commit to before the ASoC: SOF: get nhlt from all topologies commit?
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
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.
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 patch needs a bit more detailed description of what you're trying to do in the commit message. You need to say that you're adding a list of NHLTs and that for each device type, you want to search for the blob in all NHLTs in the list. Otherwise, its unclear why we need to search multiple NHLTs for DMIC? Or alternatively, if this was just a side effect, you need to move the iterative check for the SSP blob into the SSP case in the switch above and skip it here.
| 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
| 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.
8d4c0c3 to
1af3d92
Compare
|
Tested HDMI in capture and I2S functional. observed the expected behavior. |
sound/soc/sof/ipc4-topology.c
Outdated
| { | ||
| struct sof_ipc4_fw_data *ipc4_data = sdev->private; | ||
| struct nhlt_specific_cfg *cfg; | ||
| struct snd_ipc4_nhlt *entry; |
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.
dont you need to init this to NULL?
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, done
| 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.
@bardliao this patch needs a bit more detailed description of what you're trying to do in the commit message. You need to say that you're adding a list of NHLTs and that for each device type, you want to search for the blob in all NHLTs in the list. Otherwise, its unclear why we need to search multiple NHLTs for DMIC? Or alternatively, if this was just a side effect, you need to move the iterative check for the SSP blob into the SSP case in the switch above and skip it here.
| } | ||
|
|
||
| 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.
you need to call out that in preparation for supporting multiple topology fragments per function which could end up calling this function multiple types with NHLT blobs from different fragments, downgrade the error message to debug level.
| int dai_link_id, int tplg_dev) | ||
| { | ||
| char *filename = NULL; | ||
| /* |
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.
missing line?
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
| if (strstr(dai_link->name, "Codec")) { | ||
| /* | ||
| * Assume DAI link 0 is jack which is true in all existing | ||
| * machine driver |
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.
typo: machine drivers
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
| } else if (strstr(dai_link->name, "iDisp")) { | ||
| 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.
can you split this into seaprate function based on DAI 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.
Not sure how to do it. It seems the only way we can tell the DAI type is from the DAI link name. And the SSP DAI link name could be SSP%d-Codec, SSP%d-BT, SSP%d-HDMI for different types.
| case TPLG_DEVICE_SSP_JACK: | ||
| case TPLG_DEVICE_SSP_AMP: | ||
| case TPLG_DEVICE_SSP_BT: | ||
| case TPLG_DEVICE_SSP_HDMI_IN: |
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.
why is this in this patch and not the previous one?
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, it should be in the previous patch.
The existing code assumes there is only one NHLT blob either from BIOS or topology. As function topologies are used and each function topology contains a NHLT blob section, we need to search the matching NHLT blob from all the NHLT blobs. The commit suggests adding a list of NHLTs and search the NHLTs from the list. Signed-off-by: Bard Liao <[email protected]>
To support multiple topology fragments per function, this function could be called multiple types with different NHLT blobs from different fragments. Downgrade the error message to debug level since the SSP device type could be found from one of the nhlt. Signed-off-by: Bard Liao <[email protected]>
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 <[email protected]>
…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 <[email protected]>
… I2S machines Add sof_i2s_get_tplg_files() callback for Intel SOF I2S machines. Signed-off-by: Bard Liao <[email protected]>
Use sof_i2s_get_tplg_files() for SOF es83x6 machines. Signed-off-by: Bard Liao <[email protected]>
1af3d92 to
9fdc58f
Compare
Currently, function topology is only used by the SoundWire machines. Extend the support to the I2S machines.