SC8280XP: Initial refactor from board to family config#9818
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR consolidates SC8280XP Snapdragon board support by extracting shared firmware, service, and userspace logic into a new family configuration. ThinkPad X13s is refactored to delegate duplicated concerns to the family level. Radxa Dragon Q8B board is added using the new family foundation with board-specific firmware variants and Bluetooth customization. ChangesSC8280XP Family Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
github-actions
Bot
added
05
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
github-actions
Bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
config/sources/families/sc8280xp.conf (2)
28-28: 💤 Low valueFactor out duplicate GRUB command line.
The same
GRUB_CMDLINE_LINUX_DEFAULTvalue is defined for both branches. Consider declaring it once before or after the case statement to reduce duplication.♻️ Proposed refactor
+declare -g GRUB_CMDLINE_LINUX_DEFAULT="clk_ignore_unused pd_ignore_unused arm64.nopauth efi=noruntime" + case $BRANCH in sc8280xp) declare -g KERNEL_MAJOR_MINOR="7.0" declare -g KERNELSOURCE='https://github.com/steev/linux.git' declare -g KERNELBRANCH='branch:lenovo-x13s-linux-7.0.y' declare -g LINUXCONFIG="linux-${ARCH}-${BRANCH}" - declare -g GRUB_CMDLINE_LINUX_DEFAULT="clk_ignore_unused pd_ignore_unused arm64.nopauth efi=noruntime" ;; vendor) declare -g KERNEL_MAJOR_MINOR="7.0" declare -g KERNELSOURCE='https://github.com/radxa/kernel.git' declare -g KERNELBRANCH='branch:linux-7.0.2' - declare -g GRUB_CMDLINE_LINUX_DEFAULT="clk_ignore_unused pd_ignore_unused arm64.nopauth efi=noruntime" ;; esacAlso applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/sources/families/sc8280xp.conf` at line 28, The GRUB_CMDLINE_LINUX_DEFAULT variable is duplicated in both case branches; to fix, remove the duplicate assignments inside the case and declare a single GRUB_CMDLINE_LINUX_DEFAULT="clk_ignore_unused pd_ignore_unused arm64.nopauth efi=noruntime" once (either before or after the case statement) so both branches inherit the same value; update any branch-specific logic only if it needs to augment that base value rather than reassign it.
51-54: 💤 Low valueHardcoded release list requires manual maintenance.
The function checks for specific Debian/Ubuntu releases. This will need updates for each new release (e.g., when Debian 14 or Ubuntu 26.04 are released). Consider documenting this maintenance requirement or adding a comment explaining why specific releases are gated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/sources/families/sc8280xp.conf` around lines 51 - 54, The release check in sc8280xp_is_userspace_supported() currently hardcodes specific releases and requires manual updates for new OS versions; add an explanatory comment above the function referencing why only these releases are supported, include a TODO explaining that RELEASE must be updated when new Debian/Ubuntu releases are added (e.g., Debian 14/Ubuntu 26.04), and optionally note where RELEASE is defined so future maintainers know how to change it; keep the function body unchanged but document the maintenance requirement and expected update procedure.config/boards/radxa-dragon-q8b.conf (1)
11-22: ⚡ Quick winMove Bluetooth MAC initialization to family config to eliminate duplication.
This function is duplicated identically in
thinkpad-x13s.conf. Both boards share the same logic for Bluetooth MAC address initialization and belong to thesc8280xpfamily. Move the function toconfig/sources/families/sc8280xp.confaspost_family_tweaks_bsp__sc8280xp_bluetooth_addr()and remove the board-specific versions. This follows the existing pattern used bypost_family_tweaks_bsp__sc8280xp_always_start_pdmapper()in the same file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/boards/radxa-dragon-q8b.conf` around lines 11 - 22, The bluetooth MAC init function is duplicated across boards; move the implementation into the sc8280xp family file and remove the board-specific copies: create post_family_tweaks_bsp__sc8280xp_bluetooth_addr() in the families sc8280xp config with the same body (random_mac_address generation, display_alert and add_file_from_stdin_to_bsp_destination override), delete post_family_tweaks_bsp__radxa_dragon_q8b_bluetooth_addr() and the identical thinkpad-x13s version, and ensure no board-specific callers need renaming (the family hook name should match existing family-hook usage alongside post_family_tweaks_bsp__sc8280xp_always_start_pdmapper()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/boards/radxa-dragon-q8b.conf`:
- Line 1: The board config is missing a CI hint for which kernel branches to
test; add a global declaration for KERNEL_TEST_TARGET matching the board's
kernel choice (KERNEL_TARGET value "vendor") so CI will run kernel tests for the
vendor kernel—update the radxa-dragon-q8b.conf to declare the KERNEL_TEST_TARGET
variable as a global and set it to "vendor" (refer to KERNEL_TEST_TARGET and
existing KERNEL_TARGET="vendor" in the file).
---
Nitpick comments:
In `@config/boards/radxa-dragon-q8b.conf`:
- Around line 11-22: The bluetooth MAC init function is duplicated across
boards; move the implementation into the sc8280xp family file and remove the
board-specific copies: create post_family_tweaks_bsp__sc8280xp_bluetooth_addr()
in the families sc8280xp config with the same body (random_mac_address
generation, display_alert and add_file_from_stdin_to_bsp_destination override),
delete post_family_tweaks_bsp__radxa_dragon_q8b_bluetooth_addr() and the
identical thinkpad-x13s version, and ensure no board-specific callers need
renaming (the family hook name should match existing family-hook usage alongside
post_family_tweaks_bsp__sc8280xp_always_start_pdmapper()).
In `@config/sources/families/sc8280xp.conf`:
- Line 28: The GRUB_CMDLINE_LINUX_DEFAULT variable is duplicated in both case
branches; to fix, remove the duplicate assignments inside the case and declare a
single GRUB_CMDLINE_LINUX_DEFAULT="clk_ignore_unused pd_ignore_unused
arm64.nopauth efi=noruntime" once (either before or after the case statement) so
both branches inherit the same value; update any branch-specific logic only if
it needs to augment that base value rather than reassign it.
- Around line 51-54: The release check in sc8280xp_is_userspace_supported()
currently hardcodes specific releases and requires manual updates for new OS
versions; add an explanatory comment above the function referencing why only
these releases are supported, include a TODO explaining that RELEASE must be
updated when new Debian/Ubuntu releases are added (e.g., Debian 14/Ubuntu
26.04), and optionally note where RELEASE is defined so future maintainers know
how to change it; keep the function body unchanged but document the maintenance
requirement and expected update procedure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ea32700-b275-4c71-bc4d-f2b3b8475a7c
📒 Files selected for processing (5)
config/boards/radxa-dragon-q8b.confconfig/boards/thinkpad-x13s.confconfig/kernel/linux-sc8280xp-sc8280xp.configconfig/kernel/linux-sc8280xp-vendor.configconfig/sources/families/sc8280xp.conf
| @@ -0,0 +1,58 @@ | |||
| # Qualcomm Snapdragon 8cx Gen 3 Adreno 690 Radxa Dragon Q8B | |||
There was a problem hiding this comment.
Add KERNEL_TEST_TARGET for CI coverage.
Pipeline validation recommends adding KERNEL_TEST_TARGET to specify which kernel branches should be tested for this board. Since this board uses KERNEL_TARGET="vendor", consider adding:
declare -g KERNEL_TEST_TARGET="vendor"📋 Proposed addition
declare -g BOARDFAMILY="sc8280xp"
declare -g KERNEL_TARGET="vendor"
+declare -g KERNEL_TEST_TARGET="vendor"🧰 Tools
🪛 GitHub Actions: Validate board configs / 0_Validate changed board configs.txt
[warning] 1-1: validate-board-config.py: KERNEL_TEST_TARGET: recommended, comma-separated list of branches to test (e.g. current,edge)
🪛 GitHub Actions: Validate board configs / Validate changed board configs
[warning] 1-1: KERNEL_TEST_TARGET: recommended, comma-separated list of branches to test (e.g. current,edge)
🪛 GitHub Check: Validate changed board configs
[warning] 1-1:
KERNEL_TEST_TARGET: recommended, comma-separated list of branches to test (e.g. current,edge)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/boards/radxa-dragon-q8b.conf` at line 1, The board config is missing a
CI hint for which kernel branches to test; add a global declaration for
KERNEL_TEST_TARGET matching the board's kernel choice (KERNEL_TARGET value
"vendor") so CI will run kernel tests for the vendor kernel—update the
radxa-dragon-q8b.conf to declare the KERNEL_TEST_TARGET variable as a global and
set it to "vendor" (refer to KERNEL_TEST_TARGET and existing
KERNEL_TARGET="vendor" in the file).
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
|
This probably works, but goes in the opposite direction I intended: sc8280xp was just a different branch from the default UEFI with the hope that enough will be mainlined so that it could just use the standard UEFI kernel (eg just drop the custom BRANCH and use current/edge, plus grub-with-dtb (or recently the With this move that gets further away, not closer, and I can't really tell what it buys us? In practice the dragon thing gets its own kernel, and so does the x13s. Also, has "the other sc8280xp" ("Windows Dev Kit 2023", |
2 participants
Description
In preparation for future boards using the sc8280xp / Snapdragon 8cx Gen 3 SoC I refactored the Thinkpad X13s to a family config similar to how we handle other boards and their families. What we need to do now is to test for regressions. @rpardini seems to be the active maintainer around this board / SoC so it would be nice if you could take a look over this too.
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Improvements