Skip to content

Conversation

@bardliao
Copy link
Collaborator

Currently, function topology is only used by the SoundWire machines. Extend the support to the I2S machines.

Copy link
Collaborator

@ujfalusi ujfalusi left a 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__,
Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

Copy link
Collaborator

@ujfalusi ujfalusi Jan 29, 2026

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?

Copy link
Collaborator Author

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.

struct snd_ipc4_nhlt {
struct list_head list;
void *nhlt;
size_t nhlt_size;
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

size_t nhlt_size;
};


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line break

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

channel_count, sample_rate,
dir, dev_type);
if (cfg)
break;
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

struct snd_ipc4_nhlt *tplg_nhlt;

/* Get the nhlt from topology manifest*/
tplg_nhlt = kzalloc(sizeof(*tplg_nhlt), GFP_KERNEL);
Copy link
Collaborator

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!

Copy link
Collaborator

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..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to devm_kzalloc()

@bardliao bardliao force-pushed the get-i2s-tplg branch 2 times, most recently from a0fd522 to 8d4c0c3 Compare January 29, 2026 03:30
Copy link
Collaborator

@ujfalusi ujfalusi left a 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__,
Copy link
Collaborator

@ujfalusi ujfalusi Jan 29, 2026

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?

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,
Copy link
Collaborator

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);
Copy link
Collaborator

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?

Copy link
Collaborator Author

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)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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")) {
Copy link
Collaborator

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++;
}
Copy link
Collaborator

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?

Copy link
Collaborator Author

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>
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;
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants