Skip to content

Ptl i2srt5682 sdwrt722 - #5166

Merged
bardliao merged 2 commits into
thesofproject:topic/sof-devfrom
RDharageswari:ptl_i2srt5682_sdwrt711
Sep 24, 2024
Merged

Ptl i2srt5682 sdwrt722#5166
bardliao merged 2 commits into
thesofproject:topic/sof-devfrom
RDharageswari:ptl_i2srt5682_sdwrt711

Conversation

@RDharageswari

@RDharageswariRDharageswari commented Sep 4, 2024

Copy link
Copy Markdown

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

Comment threadsound/soc/intel/boards/sof_sdw.c Outdated
Comment threadsound/soc/intel/common/soc-acpi-intel-ptl-match.c

@bardliaobardliao left a comment

Copy link
Copy Markdown
Collaborator

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?

Comment threadsound/soc/intel/boards/sof_sdw.c Outdated
DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_ptlrvp"),
},
.driver_data = (void *)(SOF_SDW_TGL_HDMI |
SOF_SDW_PCH_DMIC),

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

SOFCI TEST

@bardliaobardliao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RDharageswariSOF_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
Comment threadsound/soc/intel/boards/sof_sdw.c Outdated
.matches = {
DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_ptlrvp"),
},
.driver_data = (void *)(SOF_SDW_TGL_HDMI |

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.

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

Copy link
Copy Markdown
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.

Comment threadsound/soc/intel/boards/sof_sdw.c Outdated
DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_ptlrvp"),
},
.driver_data = (void *)(SOF_SDW_TGL_HDMI |
SOC_SDW_PCH_DMIC),

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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@RDharageswariRDharageswari changed the title Ptl i2srt5682 sdwrt711Ptl i2srt5682 sdwrt722Sep 11, 2024
Comment threadsound/soc/intel/common/soc-acpi-intel-ptl-match.c
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
Copy Markdown
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
Copy Markdown
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?

@RDharageswariRDharageswariSep 19, 2024

Copy link
Copy Markdown
Author

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

Dharageswari R added 2 commits September 18, 2024 20:51
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
Copy Markdown
Collaborator

SOFCI TEST

@bardliao
bardliao merged commit 49e31db into thesofproject:topic/sof-devSep 24, 2024
@bardliao

Copy link
Copy Markdown
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 freeto 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

@RDharageswari@bardliao@plbossart@ranj063@ujfalusi@shumingfan