Skip to content

Conversation

@rpardini
Copy link
Member

@rpardini rpardini commented Dec 27, 2025

Summary by CodeRabbit

  • New Features
    • Enabled HDMI1 video output port with audio support, allowing connection of an additional HDMI display with audio on RK3588-based devices.
    • Enhanced hardware configuration to support dual HDMI display connectivity.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 27, 2025

📝 Walkthrough

Walkthrough

This patch enables HDMI1 support for the RK3588 CM3588 NAS by activating the HDMI1 node, wiring endpoint connections between VP1 output and HDMI1 input, enabling audio nodes, and enabling the HDMI receiver and PHY components.

Changes

Cohort / File(s) Summary
RK3588 CM3588 NAS Device Tree Configuration
patch/kernel/archive/rockchip64-6.18/rk3588-1210-arm64-dts-rockchip-Enable-HDMI1-and-audio-for-HDMI0and1.patch
Adds HDMI1 connector node, enables HDMI0/HDMI1 audio and PHY components (hdmi0_sound, hdmi1, hdmi1_sound, hdptxphy1), wires HDMI1 endpoint connections (hdmi1_in_vp1, hdmi1_out_con), connects VP1 output to HDMI1 input, and enables hdmi_receiver_cma.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • ColorfulRhino
  • JohnTheCoolingFan
  • TheSnowfield
  • paolosabatino
  • igorpecovnik
  • joekhoobyar
  • krachlatte

Poem

🐰 Whisker-twitch with delight!
HDMI1 shines so bright,
VP1 wired just right,
Audio flows through the night,
One more port for pure sight!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: enabling HDMI1 and audio support for the CM3588-NAS device, which matches the patch content perfectly.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1e7ef9d and 3fe2af0.

📒 Files selected for processing (1)
  • patch/kernel/archive/rockchip64-6.18/rk3588-1210-arm64-dts-rockchip-Enable-HDMI1-and-audio-for-HDMI0and1.patch
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
🔇 Additional comments (6)
patch/kernel/archive/rockchip64-6.18/rk3588-1210-arm64-dts-rockchip-Enable-HDMI1-and-audio-for-HDMI0and1.patch (6)

19-28: LGTM: HDMI1 connector definition follows standard DT conventions.

The HDMI connector node is correctly defined with the standard "hdmi-connector" compatible string, Type A designation, and proper endpoint wiring to the HDMI1 output.


37-39: LGTM: Audio enablement for both HDMI ports.

Enabling both hdmi0_sound and hdmi1_sound correctly provides audio support for both HDMI outputs as stated in the PR objectives.

Also applies to: 57-59


41-43: LGTM: HDMI1 controller and PHY enablement.

Enabling both the HDMI1 controller and its associated HDPTXPHY1 is necessary for HDMI1 functionality.

Also applies to: 68-70


61-63: Clarification: Is HDMI input capture intended?

The hdmi_receiver_cma node typically provides HDMI input/capture functionality (not output). Is this board intended to support HDMI input capture, or is this being enabled as part of enabling all HDMI-related functionality?

If HDMI input capture is not needed, this can remain enabled without harm (it just reserves some CMA memory), but it's worth documenting the intent.


1-10: No issues found. This patch introduces new functionality (HDMI1 support and audio for HDMI0/1) that complements the existing HDMI0 support from earlier kernel versions. It is not a port of previous code but rather an expansion of HDMI capabilities for the CM3588-NAS board.


45-49: Display pipeline wiring is correct.

The endpoint connections properly wire the display path: VP1 → HDMI1 → Connector. The bidirectional endpoint references (hdmi1_in_vp1vp1_out_hdmi1 and hdmi1_out_conhdmi1_con_in) follow standard device tree display pipeline conventions and mirror the established HDMI0 pattern already in the patch. The use of ROCKCHIP_VOP2_EP_HDMI1 is consistent with existing device tree configurations in this repository (e.g., h88k.dts) and will be available from the kernel's dt-bindings when the patch is applied.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines 02 Milestone: First quarter release Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... labels Dec 27, 2025
@rpardini rpardini marked this pull request as ready for review December 27, 2025 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

02 Milestone: First quarter release Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review Patches Patches related to kernel, U-Boot, ... size/medium PR with more then 50 and less then 250 lines

Development

Successfully merging this pull request may close these issues.

1 participant