Skip to content

Conversation

@cujomalainey
Copy link
Contributor

There are TGL devices using either PDM0 or PDM1 port for the DMIC16k
DAI for hotword detection. This patch adds the DMIC_DAI_LINK_16k_PDM
parameter and builds both versions.

Signed-off-by: Ben Zhang benzh@chromium.org

@lgirdwood
Copy link
Member

@cujomalainey looks like we have a conflict with a recent intelligo update.

@fuyuntsuo
Copy link
Contributor

@cujomalainey looks like we have a conflict with a recent intelligo update.

May I know what the conflict is about?

@plbossart
Copy link
Member

@bzhg I don't get what this is trying to fix.

For the regular PDM 48K you would still use PDM0 or PDM0+PDM1. But for the 16k version only the PDM1 interface is used? There are dependencies between the 48 and 16k mode. @singalsu are you good with this change?

@lgirdwood
Copy link
Member

@cujomalainey looks like we have a conflict with a recent intelligo update.

May I know what the conflict is about?

No need to worry, it's a git merge conflict - this is common. It's needs to be fixed in this PR and not your code.

@cujomalainey
Copy link
Contributor Author

@bzhg I don't get what this is trying to fix.

For the regular PDM 48K you would still use PDM0 or PDM0+PDM1. But for the 16k version only the PDM1 interface is used? There are dependencies between the 48 and 16k mode. @singalsu are you good with this change?

@plbossart google hotword needs exactly one channel and KPB needs 2. Selector is not functional enough to select 2 channels from 4 channels. Therefore when we have a board that is only using PDM1 we have to select the active PDM in the SSP until we can patch selector to be more functional. Make sense?

There are TGL devices using either PDM0 or PDM1 port for the DMIC16k
DAI for hotword detection. This patch adds the DMIC_DAI_LINK_16k_PDM
parameter and builds both versions.

Signed-off-by: Ben Zhang <benzh@chromium.org>
@cujomalainey
Copy link
Contributor Author

@lgirdwood thanks for the notification, its fixed now. Would be nice if github generated a notification to the PR owner on that state change.

@lgirdwood
Copy link
Member

@plbossart @singalsu good now ?

@plbossart
Copy link
Member

@cujomalainey @lgirdwood I am not really happy about the different approach taken between the 48k and 16k interfaces. The 48k case assumes there is always 4 ch, and there are board-specific controls in UCM to figure out what channels are valid. I don't understand what limitations are part of the selector that lead us to spin yet another topology file. Can we not fix the selector and use the same approach of specifying valid channels through controls?

@cujomalainey
Copy link
Contributor Author

@cujomalainey @lgirdwood I am not really happy about the different approach taken between the 48k and 16k interfaces. The 48k case assumes there is always 4 ch, and there are board-specific controls in UCM to figure out what channels are valid. I don't understand what limitations are part of the selector that lead us to spin yet another topology file. Can we not fix the selector and use the same approach of specifying valid channels through controls?

@plbossart I agree, and that is our long term plan along with reverting this change once the upgrade is complete. The issue is our roadmap for next 1-2Q is quite heavy with little room for upstream work other than bug fixing. We are hoping to implement a generic format similar to what CRAS uses where we pass in an array of values, the index being the output channel and the value being the source. So on devices with 2 DMIC on PDM1 the selector config would be "2 3 -1 -1"

@plbossart
Copy link
Member

@cujomalainey @lgirdwood I am not really happy about the different approach taken between the 48k and 16k interfaces. The 48k case assumes there is always 4 ch, and there are board-specific controls in UCM to figure out what channels are valid. I don't understand what limitations are part of the selector that lead us to spin yet another topology file. Can we not fix the selector and use the same approach of specifying valid channels through controls?

@plbossart I agree, and that is our long term plan along with reverting this change once the upgrade is complete. The issue is our roadmap for next 1-2Q is quite heavy with little room for upstream work other than bug fixing. We are hoping to implement a generic format similar to what CRAS uses where we pass in an array of values, the index being the output channel and the value being the source. So on devices with 2 DMIC on PDM1 the selector config would be "2 3 -1 -1"

ok, if this is a temporary solution I am good with the approach, I just didn't want to see this become the regular way of dealing with mics. thanks for the details @cujomalainey

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

conditional approval if this is only a temporary fix and we see a better support for the selector long-term

@cujomalainey
Copy link
Contributor Author

Filed above mentioned bug for tracking.

@keyonjie keyonjie requested a review from singalsu May 26, 2021 02:19
@keyonjie
Copy link
Contributor

looks good to me, but let's still wait for @singalsu 's confirmation.

@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood
Copy link
Member

Logger appear dead - rerun CI.

@cujomalainey
Copy link
Contributor Author

all checks passed, 5 approvals, no feedback outstanding, merging

@cujomalainey cujomalainey merged commit 9d21f9e into thesofproject:main May 26, 2021
@cujomalainey cujomalainey deleted the pdm1 branch May 26, 2021 15:08
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.

8 participants