Skip to content

Conversation

@igorpecovnik
Copy link
Member

@igorpecovnik igorpecovnik commented Oct 21, 2025

Description

Mathing Allwinner family via LINUXFAMILY is giving different results. sunxi doesn't cover sun5* ...

0e45d79#r168370107

How Has This Been Tested?

  • Manul run

Checklist:

  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Mathing Allwinner family via LINUXFAMILY is giving different results. sunxi doesn't cover sun5* ...
@igorpecovnik igorpecovnik requested a review from a team as a code owner October 21, 2025 07:32
@igorpecovnik igorpecovnik added the Work in progress Unfinished / work in progress label Oct 21, 2025
@github-actions github-actions bot added 11 Milestone: Fourth quarter release size/small PR with less then 50 lines Needs review Seeking for review Framework Framework components labels Oct 21, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

This PR broadens platform pattern matches in lib/functions/compilation/patch/drivers_network.sh by changing multiple conditionals from "sunxi*" to "sun*" and updates related comments. It also modifies config/kernel/linux-sunxi64-edge.config by removing CONFIG_MHI_BUS and adding numerous WLAN/USB wireless driver configuration options (many set to =m or =y). No function signatures or external APIs were changed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Mix of homogeneous script pattern edits (low effort) and a large, heterogeneous kernel config change requiring verification of enabled drivers and build implications.

Possibly related PRs

Suggested labels

Ready to merge, Patches

Suggested reviewers

  • PanderMusubi
  • rpardini
  • janprunk
  • chraac
  • krachlatte
  • EvilOlaf
  • YaoFei509
  • AGM1968
  • teknoid
  • Tonymac32
  • JohnTheCoolingFan
  • pyavitz
  • schwar3kat

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Wireless driver patching: fix inconsistency in matching" directly aligns with the primary change in the changeset. The main modification in drivers_network.sh involves fixing an inconsistency by broadening the platform matching pattern from sunxi* to sun* to properly cover Allwinner family variants (including sun5* devices). The title accurately captures this core intent—fixing a matching inconsistency in wireless driver patching. The title is concise, specific, and clearly communicates the primary objective of the PR.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed The pull request description directly addresses the core issue reflected in the changes: fixing the LINUXFAMILY matching inconsistency by broadening the pattern from "sunxi*" to "sun*" to include sun5* devices. The description references a GitHub commit for context and indicates manual testing has been completed. While the description does not explicitly detail the extensive kernel configuration changes to wireless driver options in the config file, these changes appear related to the broader "wireless driver patching" theme mentioned in the PR title and are plausibly connected to the driver patching improvements. The description is clearly related to at least a significant portion of the changeset and is not off-topic.

📜 Recent review details

Configuration used: CodeRabbit UI

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 56acd50 and ea32921.

📒 Files selected for processing (1)
  • config/kernel/linux-sunxi64-edge.config (4 hunks)

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.

@igorpecovnik igorpecovnik removed the Work in progress Unfinished / work in progress label Oct 21, 2025
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines Hardware Hardware related like kernel, U-Boot, ... and removed size/small PR with less then 50 lines labels Oct 21, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

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 173e0b9 and cd01a2a.

📒 Files selected for processing (1)
  • config/kernel/linux-sunxi64-edge.config (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-27T15:54:35.119Z
Learnt from: pyavitz
PR: armbian/build#8421
File: config/kernel/linux-sunxi64-edge.config:2168-2173
Timestamp: 2025-07-27T15:54:35.119Z
Learning: In the Armbian build system, staging 802.11 drivers like CONFIG_RTLLIB, CONFIG_RTL8192E, CONFIG_R8712U are kept enabled alongside upstream rtw88 drivers because rtw88 is still under development and suffers from reliability issues. The staging drivers serve as necessary fallbacks when upstream drivers are unstable, prioritizing working hardware over avoiding technical conflicts.

Applied to files:

  • config/kernel/linux-sunxi64-edge.config
🔇 Additional comments (1)
config/kernel/linux-sunxi64-edge.config (1)

599-599: No issues found with CONFIG_MHI_BUS removal.

Verification confirms CONFIG_MHI_BUS is modem/WWAN-specific (Qualcomm PCIe modems) and unrelated to the new wireless drivers being added. Removing the host-side controller while retaining CONFIG_MHI_BUS_EP=m (endpoint) is intentional and appropriate for this sunxi64 edge configuration. No dependencies are broken.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
config/kernel/linux-sunxi64-edge.config (1)

839-874: Verify Realtek/MTK/RTW88 staging+upstream coexistence is stable and well-tested.

Per learnings, coexistence of staging drivers (e.g., CONFIG_RTL8723DS=m at line 926) alongside upstream rtw88 drivers is intentional due to rtw88 developmental issues. However, ensure:

  1. Module load order is explicitly controlled to avoid conflicts (staging drivers should not auto-load if upstream driver is present).
  2. All added wireless drivers are tested on the target boards to confirm reliability.
  3. Any known issues or mutual-exclusivity constraints are documented.
📜 Review details

Configuration used: CodeRabbit UI

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 cd01a2a and 56acd50.

📒 Files selected for processing (1)
  • config/kernel/linux-sunxi64-edge.config (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-27T15:54:35.119Z
Learnt from: pyavitz
PR: armbian/build#8421
File: config/kernel/linux-sunxi64-edge.config:2168-2173
Timestamp: 2025-07-27T15:54:35.119Z
Learning: In the Armbian build system, staging 802.11 drivers like CONFIG_RTLLIB, CONFIG_RTL8192E, CONFIG_R8712U are kept enabled alongside upstream rtw88 drivers because rtw88 is still under development and suffers from reliability issues. The staging drivers serve as necessary fallbacks when upstream drivers are unstable, prioritizing working hardware over avoiding technical conflicts.

Applied to files:

  • config/kernel/linux-sunxi64-edge.config
🔇 Additional comments (1)
config/kernel/linux-sunxi64-edge.config (1)

842-849: ****

The review comment contains multiple factual errors and is based on a misunderstanding of the platform appropriateness of these drivers.

Key findings:

  1. Platform appropriateness is correct: UWE5622 and Spreadtrum drivers (CONFIG_SPARD_WLAN_SUPPORT, CONFIG_SC23XX, etc.) are legitimately used on Allwinner H616 boards (part of sunxi64 platform), not "non-Allwinner" or "platform-mismatched" as claimed.

  2. Driver classification error: CONFIG_WLAN_UWE5621/5622 are Unisoc drivers, not "Rockchip UWE driver modules." The developer correctly did not include CONFIG_RK_WIFI* (actual Rockchip-specific drivers), which are absent from the sunxi64 config as expected.

  3. Incorrect builtin/module status: The review claims SC2342_INTEG, SC2355, SC2366 are =y (builtin), but they are actually =m (modules):

    • Line 938: CONFIG_SC2342_INTEG=m
    • Line 939: CONFIG_SC2355=m
    • Line 940: CONFIG_SC2366=m

The drivers in question are appropriate for this configuration and should remain. The review's concerns about platform misalignment do not apply.

Likely an incorrect or invalid review comment.

@igorpecovnik igorpecovnik added Ready to merge Reviewed, tested and ready for merge and removed Needs review Seeking for review labels Oct 23, 2025
@github-actions github-actions bot added Needs review Seeking for review and removed Ready to merge Reviewed, tested and ready for merge labels Oct 23, 2025
@igorpecovnik igorpecovnik merged commit 8b0f9ad into main Oct 23, 2025
9 of 10 checks passed
@igorpecovnik igorpecovnik deleted the fixmatching branch October 23, 2025 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

11 Milestone: Fourth quarter release Framework Framework components Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review size/medium PR with more then 50 and less then 250 lines

Development

Successfully merging this pull request may close these issues.

2 participants