Skip to content
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

Ptl i2srt5682 sdwrt722 #5166

Merged

Conversation

RDharageswari
Copy link

@RDharageswari RDharageswari commented Sep 4, 2024

This PR adds the following:
RT722 SDCA card for PTL platform
Add machine and topology entries to the soc-acpi-intel-ptl-match for the I2S rt5682

Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a nitpick of alignment. Can you check the CI build error, please?

DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_ptlrvp"),
},
.driver_data = (void *)(SOF_SDW_TGL_HDMI |
SOF_SDW_PCH_DMIC),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please align with the parentheses. I.e.

(void *) (SOF_SDW_TGL_HDMI |
          SOF_SDW_PCH_DMIC),

@bardliao
Copy link
Collaborator

bardliao commented Sep 6, 2024

SOFCI TEST

Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

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

@RDharageswari SOF_SDW_PCH_DMIC has been changed to SOC_SDW_PCH_DMIC. That's why your PR can't build

bardliao
bardliao previously approved these changes Sep 9, 2024
.matches = {
DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_ptlrvp"),
},
.driver_data = (void *)(SOF_SDW_TGL_HDMI |
Copy link
Member

Choose a reason for hiding this comment

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

no, all devices after TGL only use 3 HDMI ports, this will add an HDMI port for no reason.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, i will remove.

DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_ptlrvp"),
},
.driver_data = (void *)(SOF_SDW_TGL_HDMI |
SOC_SDW_PCH_DMIC),
Copy link
Member

Choose a reason for hiding this comment

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

Also can you clarify why you need the PCH_DMIC? Is this a supported configuration? At the very least there should be an explanation of which microphone path is supported.

Copy link
Author

Choose a reason for hiding this comment

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

hi Pierre,
This is the POR configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Humm, now I am completely lost. You list rt5682 in this PR, along with RT711. Neither are by default on the RVP, so this quirk is problematic indeed.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Pierre,
RT722 is the onboard codec and the patch adds RT722 to the ACPI table.
sorry for the wrong branch name.

bardliao
bardliao previously approved these changes Sep 10, 2024
@RDharageswari RDharageswari changed the title Ptl i2srt5682 sdwrt711 Ptl i2srt5682 sdwrt722 Sep 11, 2024
plbossart
plbossart previously approved these changes Sep 16, 2024
.link_mask = BIT(1),
.links = ptl_rt722_l1,
.drv_name = "sof_sdw",
.sof_tplg_filename = "sof-ptl-rt722-4ch.tplg",
Copy link
Collaborator

Choose a reason for hiding this comment

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

From where this topology is coming and what is the -4ch postfix means? Is it PCH-DMIC? Should not that be dynamically added to the base topology name based on the number of DMICs detected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the "-4ch" should be added automatically. See https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/intel/hda.c#L1321. @RDharageswari Can you test if "-4ch" is added automatically on your device?

Copy link
Author

@RDharageswari RDharageswari Sep 19, 2024

Choose a reason for hiding this comment

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

as discussed on chat with Bard, -4ch .DMIC string can be appended through module params. Will post the topology changes for 4ch dmic soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The topology PR is thesofproject/sof#9493

Copy link
Member

Choose a reason for hiding this comment

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

the topology is wrong @bardliao, it relies on 2ch.

Enable on-board rt722 based sound card for PTL RVP.

Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
This patch adds the driver data for rt5682 codec on SSP0 and
max98360a speaker amplifiers on SSP1 for PTL platform.

Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
@bardliao
Copy link
Collaborator

SOFCI TEST

@bardliao bardliao merged commit 49e31db into thesofproject:topic/sof-dev Sep 24, 2024
12 of 14 checks passed
@bardliao
Copy link
Collaborator

The pause resume failures on https://sof-ci.01.org/linuxpr/PR5166/build4708/devicetest/index.html should not be caused by this PR. And we got approvals. Merging it.

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.

6 participants