Skip to content

Revert "Audio: Copier: Add missing DMIC DAI index mapping to create_dai()" - #6877

Merged
lgirdwood merged 1 commit into
thesofproject:mainfrom
serhiy-katsyuba-intel:dmic_revert
Jan 4, 2023
Merged

Revert "Audio: Copier: Add missing DMIC DAI index mapping to create_dai()"#6877
lgirdwood merged 1 commit into
thesofproject:mainfrom
serhiy-katsyuba-intel:dmic_revert

Conversation

@serhiy-katsyuba-intel

@serhiy-katsyuba-intelserhiy-katsyuba-intel commented Dec 21, 2022

Copy link
Copy Markdown
Contributor

This reverts commit 8d166a4.

This restores working DMIC functionality on Windows platform broken by #6668.

ATTENTION: This will break DMIC1 (DMIC with DAI index 1) functionality on Linux platform, DMIC0 should still work fine! Non working DMIC1 should be fixed on Linux driver side, not this firmware.

Signed-off-by: Serhiy Katsyuba serhiy.katsyuba@intel.com

@serhiy-katsyuba-intelserhiy-katsyuba-intel changed the title Revert "Audio: Copier: Add missing DMIC DAI index mapping to create_dai()Revert "Audio: Copier: Add missing DMIC DAI index mapping to create_dai()"Dec 21, 2022
@juimonen

juimonen commented Dec 21, 2022

Copy link
Copy Markdown

I think the "fix" would be to extract the least significant 8 bits and use that as dai_index. The node_id can have other bits set in it from kernel side.

@serhiy-katsyuba-intel

serhiy-katsyuba-intel commented Dec 21, 2022

Copy link
Copy Markdown
ContributorAuthor

I think the "fix" would be to extract the least significant 8 bits and use that as dai_index. The node_id can have other bits set in it from kernel side.

dai_index comes from node_id.f.v_index, it's 8 bit:
dai_index[dai_count - 1] = node_id.f.v_index;

@juimonen

juimonen commented Dec 21, 2022

Copy link
Copy Markdown

yes correct. I'll make the corresponding kernel PR.

@serhiy-katsyuba-intel
Revert Audio: Copier: Add missing DMIC DAI index mapping to create_dai()
2792f0f
This reverts commit 8d166a4.
This restores working DMIC functionality on Windows platform broken
by thesofproject#6668.
ATTENTION: This will break DMIC1 (DMIC with DAI index 1) functionality
on Linux platform, DMIC0 should still work fine!
Non working DMIC1 should be fixed on Linux driver side, not this firmware.
Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
@juimonenjuimonen mentioned this pull request Dec 21, 2022
Merged
@lgirdwood

lgirdwood commented Dec 21, 2022

Copy link
Copy Markdown
Member

@ujfalusi@plbossart fyi - how long has kernel had the wrong ID mapping ?
@serhiy-katsyuba-intel@juimonen the FW PR which introduced this was 2 days ago, so I dont think we have any legacy issues to block, this revert on FW side.

abonislawski
abonislawski approved these changes Dec 21, 2022
View reviewed changes

@abonislawskiabonislawski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For me we can go directly with this PR, abi should be used if something needs to be synced
Unfortunately there was no DMIC tests in mtl CI when 6668 merged

@serhiy-katsyuba-intel
serhiy-katsyuba-intel marked this pull request as ready for review December 21, 2022 16:38
@serhiy-katsyuba-intel

serhiy-katsyuba-intel commented Dec 21, 2022

Copy link
Copy Markdown
ContributorAuthor

@ujfalusi@plbossart fyi - how long has kernel had the wrong ID mapping ?

Since March 2022. However, the problem affects only DMIC1. DMIC0 works fine with and without #6668. DMIC1 seems was not tested on Linux until recently.

@juimonen

juimonen commented Dec 21, 2022

Copy link
Copy Markdown

@lgirdwood no I don't think so. I made thesofproject/linux#4114, that should work with this revert. Should be tested however.

@serhiy-katsyuba-intelserhiy-katsyuba-intel mentioned this pull request Dec 21, 2022
Closed
@ranj063

ranj063 commented Dec 21, 2022

Copy link
Copy Markdown
Collaborator

@lgirdwood no I don't think so. I made thesofproject/linux#4114, that should work with this revert. Should be tested however.

@libinyang Could you please test with this revert and the linux PR from @juimonen. Thanks!

@lgirdwood

lgirdwood commented Dec 22, 2022

Copy link
Copy Markdown
Member

ok, @libinyang pls let us know if this PR and the kernel PR work for you.

@lgirdwood

lgirdwood commented Dec 23, 2022

Copy link
Copy Markdown
Member

@libinyang ping - good to merge ? @mengdonglin fyi.

@wszypelt

wszypelt commented Jan 3, 2023

Copy link
Copy Markdown

@libinyang, @lgirdwood good to merge?

@lgirdwood
lgirdwood merged commit 3b65313 into thesofproject:mainJan 4, 2023
@lgirdwood

lgirdwood commented Jan 4, 2023

Copy link
Copy Markdown
Member

@libinyang pls shout if there are any issues and we can address

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

Reviewers

@ranj063ranj063ranj063 approved these changes
@singalsusingalsusingalsu approved these changes
@abonislawskiabonislawskiabonislawski approved these changes
@tmlemantmlemantmleman approved these changes
@lgirdwoodlgirdwoodAwaiting requested review from lgirdwood
@plbossartplbossartAwaiting requested review from plbossart
@mmaka1mmaka1Awaiting requested review from mmaka1
@lbetlejlbetlejAwaiting requested review from lbetlej
@dbalutadbalutaAwaiting requested review from dbaluta
+1 more reviewer
@wszypeltwszypeltwszypelt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@serhiy-katsyuba-intel@juimonen@lgirdwood@ranj063@wszypelt@singalsu@abonislawski@tmleman