Conversation
igorpecovnik
requested review from
ColorfulRhino,
HeyMeco,
JohnTheCoolingFan,
SuperKali,
TheSnowfield,
Tonymac32,
ahoneybun,
amazingfate,
brentr,
catalinii,
chainsx,
efectn,
joekhoobyar,
krachlatte,
lanefu,
paolosabatino,
prahal,
pyavitz,
rpardini and
schwar3kat
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds NanoPC T6 LTS Plus support: new Armbian board config plus kernel Device Tree files (6.18 and 7.0) that replace rt5616 with an ES8389 simple-audio-card, add a GPIO-controlled 5V USB regulator tied to USB PHYs, and install udev audio naming rules. Changes
Sequence Diagram(s)mermaid Bootloader->>Kernel: Load kernel with rk3588-nanopc-t6-lts-plus DT Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
size/large
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
config/boards/nanopct6-lts-plus.conf (1)
16-21: Quote$SDCARDto handle paths with spaces.While unlikely in practice, unquoted variable expansion can cause issues if
$SDCARDcontains spaces.🔧 Proposed fix
- mkdir -p $SDCARD/etc/udev/rules.d/ - echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmi0-sound", ENV{SOUND_DESCRIPTION}="HDMI0 Audio"' > $SDCARD/etc/udev/rules.d/90-naming-audios.rules - echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmi1-sound", ENV{SOUND_DESCRIPTION}="HDMI1 Audio"' >> $SDCARD/etc/udev/rules.d/90-naming-audios.rules - echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmiin-sound", ENV{SOUND_DESCRIPTION}="HDMI-In Audio"' >> $SDCARD/etc/udev/rules.d/90-naming-audios.rules - echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-dp0-sound", ENV{SOUND_DESCRIPTION}="DP0 Audio"' >> $SDCARD/etc/udev/rules.d/90-naming-audios.rules - echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-es8389-sound", ENV{SOUND_DESCRIPTION}="ES8389 Audio"' >> $SDCARD/etc/udev/rules.d/90-naming-audios.rules + mkdir -p "$SDCARD/etc/udev/rules.d/" + echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmi0-sound", ENV{SOUND_DESCRIPTION}="HDMI0 Audio"' > "$SDCARD/etc/udev/rules.d/90-naming-audios.rules" + echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmi1-sound", ENV{SOUND_DESCRIPTION}="HDMI1 Audio"' >> "$SDCARD/etc/udev/rules.d/90-naming-audios.rules" + echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmiin-sound", ENV{SOUND_DESCRIPTION}="HDMI-In Audio"' >> "$SDCARD/etc/udev/rules.d/90-naming-audios.rules" + echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-dp0-sound", ENV{SOUND_DESCRIPTION}="DP0 Audio"' >> "$SDCARD/etc/udev/rules.d/90-naming-audios.rules" + echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-es8389-sound", ENV{SOUND_DESCRIPTION}="ES8389 Audio"' >> "$SDCARD/etc/udev/rules.d/90-naming-audios.rules"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/boards/nanopct6-lts-plus.conf` around lines 16 - 21, The shell commands use unquoted $SDCARD in mkdir and the echo redirections (e.g., mkdir -p $SDCARD/etc/udev/rules.d/ and the echo ... > $SDCARD/etc/udev/rules.d/90-naming-audios.rules), so update all occurrences to quote the variable (preferably as "${SDCARD}") when composing paths to /etc/udev/rules.d/ and the file 90-naming-audios.rules to safely handle paths with spaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/boards/nanopct6-lts-plus.conf`:
- Line 7: The BOARD_MAINTAINER field in the .conf is empty so the file cannot
use the .conf extension; either populate BOARD_MAINTAINER with at least one
maintainer (e.g., "Full Name <email@example.com>") or rename the file to use the
.csc or .wip extension to mark community-supported / work-in-progress status so
it complies with Armbian board support rules.
- Around line 13-24: The child function
post_family_tweaks__nanopct6_lts_plus_naming_audios is being executed before the
parent post_family_tweaks__nanopct6_naming_audios which truncates the rules
file; rename the child to
post_family_tweaks__zzz_nanopct6_lts_plus_naming_audios so it runs last (or
alternatively add a no-op override for
post_family_tweaks__nanopct6_naming_audios in this file) to prevent the parent
from overwriting the child's appended rules; update the function name used in
this config and ensure no other references rely on the old name.
In `@patch/kernel/archive/rockchip64-6.18/dt/rk3588-nanopc-t6-lts-plus.dts`:
- Around line 67-77: The device node es8389:audio-codec@10 uses the
everest,es8389 compatible which is not present in kernel 6.18; either backport
the ES8389 driver and binding into this 6.18 tree (apply the
sound/soc/codecs/es8389.c driver and the
Documentation/devicetree/bindings/sound/everest,es8389.yaml binding) so the
compatible string and properties in the es8389 node are recognized, or retarget
this DTS change to kernel 6.19+ where everest,es8389 is upstream; ensure the
es8389:audio-codec@10 node, compatible = "everest,es8389", and the clock
properties remain unchanged once the driver/binding are available.
---
Nitpick comments:
In `@config/boards/nanopct6-lts-plus.conf`:
- Around line 16-21: The shell commands use unquoted $SDCARD in mkdir and the
echo redirections (e.g., mkdir -p $SDCARD/etc/udev/rules.d/ and the echo ... >
$SDCARD/etc/udev/rules.d/90-naming-audios.rules), so update all occurrences to
quote the variable (preferably as "${SDCARD}") when composing paths to
/etc/udev/rules.d/ and the file 90-naming-audios.rules to safely handle paths
with spaces.
🪄 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: ddf1b2dc-2a6e-4d40-ab37-1ce3b3ed8192
📒 Files selected for processing (3)
config/boards/nanopct6-lts-plus.confpatch/kernel/archive/rockchip64-6.18/dt/rk3588-nanopc-t6-lts-plus.dtspatch/kernel/archive/rockchip64-7.0/dt/rk3588-nanopc-t6-lts-plus.dts
| function post_family_tweaks__nanopct6_lts_plus_naming_audios() { | ||
| display_alert "$BOARD" "Renaming nanopct6-lts-plus audio" "info" | ||
|
|
||
| mkdir -p $SDCARD/etc/udev/rules.d/ | ||
| echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmi0-sound", ENV{SOUND_DESCRIPTION}="HDMI0 Audio"' > $SDCARD/etc/udev/rules.d/90-naming-audios.rules | ||
| echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmi1-sound", ENV{SOUND_DESCRIPTION}="HDMI1 Audio"' >> $SDCARD/etc/udev/rules.d/90-naming-audios.rules | ||
| echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-hdmiin-sound", ENV{SOUND_DESCRIPTION}="HDMI-In Audio"' >> $SDCARD/etc/udev/rules.d/90-naming-audios.rules | ||
| echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-dp0-sound", ENV{SOUND_DESCRIPTION}="DP0 Audio"' >> $SDCARD/etc/udev/rules.d/90-naming-audios.rules | ||
| echo 'SUBSYSTEM=="sound", ENV{ID_PATH}=="platform-es8389-sound", ENV{SOUND_DESCRIPTION}="ES8389 Audio"' >> $SDCARD/etc/udev/rules.d/90-naming-audios.rules | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if both functions will be invoked and in what order
# Check how Armbian's extension system orders hook functions
rg -n 'call_extension_method.*post_family_tweaks' --type sh -A 5 -B 2 lib/Repository: armbian/build
Length of output: 1698
🏁 Script executed:
#!/bin/bash
# Check the parent nanopct6.conf for its audio naming function
cat config/boards/nanopct6.conf | grep -A 15 'post_family_tweaks__nanopct6_naming_audios'Repository: armbian/build
Length of output: 1183
🏁 Script executed:
#!/bin/bash
# Search for call_extension_method implementation to understand execution order
rg -n 'call_extension_method' lib/ --type sh -A 20 | head -100Repository: armbian/build
Length of output: 9766
🏁 Script executed:
#!/bin/bash
# Check if there's any mechanism to handle duplicate hook functions or skip parent functions
rg -n 'post_family_tweaks' config/boards/ --type sh | grep -E '(nanopct6|lts)'Repository: armbian/build
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for call_extension_method implementation
rg -n 'call_extension_method\s*\(\)' lib/ --type sh -A 50 | head -150Repository: armbian/build
Length of output: 13960
🏁 Script executed:
#!/bin/bash
# Check both functions exist in their respective files and understand parent-child relationship
echo "=== Parent function in nanopct6.conf ==="
rg -n 'post_family_tweaks__nanopct6_naming_audios' config/boards/nanopct6.conf -A 10
echo -e "\n=== Child function in nanopct6-lts-plus.conf ==="
rg -n 'post_family_tweaks__nanopct6_lts_plus_naming_audios' config/boards/nanopct6-lts-plus.conf -A 10Repository: armbian/build
Length of output: 2093
Functions with the same hook point execute in alphanumerical order, causing parent post_family_tweaks__nanopct6_naming_audios to overwrite child rules.
The parent post_family_tweaks__nanopct6_naming_audios (from nanopct6.conf) executes after post_family_tweaks__nanopct6_lts_plus_naming_audios because "nanopct6_naming_audios" sorts after "nanopct6_lts_plus_naming_audios". Since the parent's first echo uses > (truncate), it will overwrite the entire file, losing the ES8389 Audio rule that the child function defined.
Rename this function to post_family_tweaks__zzz_nanopct6_lts_plus_naming_audios to ensure it executes last, or redefine the parent function as a no-op in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/boards/nanopct6-lts-plus.conf` around lines 13 - 24, The child
function post_family_tweaks__nanopct6_lts_plus_naming_audios is being executed
before the parent post_family_tweaks__nanopct6_naming_audios which truncates the
rules file; rename the child to
post_family_tweaks__zzz_nanopct6_lts_plus_naming_audios so it runs last (or
alternatively add a no-op override for
post_family_tweaks__nanopct6_naming_audios in this file) to prevent the parent
from overwriting the child's appended rules; update the function name used in
this config and ensure no other references rely on the old name.
|
✅ This PR has been reviewed and approved — all set for merge! |
igorpecovnik
force-pushed
the
nanopct6ltsplus
branch
from
704dae5 to
75e7125
Compare
github-actions
bot
added
Ready to merge
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/boards/nanopct6-lts-plus.conf (1)
13-24:⚠️ Potential issue | 🟠 MajorParent hook will overwrite child's ES8389 audio rule due to execution order.
The parent
post_family_tweaks__nanopct6_naming_audios(fromnanopct6.conf) executes after this function because"nanopct6_naming_audios"sorts after"nanopct6_lts_plus_naming_audios"alphanumerically. Since the parent's firstechouses>(truncate), the ES8389 Audio rule on line 21 will be lost.Rename to
post_family_tweaks__zzz_nanopct6_lts_plus_naming_audiosso it executes last, or override the parent function as a no-op in this file.,
🐛 Suggested fix: rename function to execute last
-function post_family_tweaks__nanopct6_lts_plus_naming_audios() { +function post_family_tweaks__zzz_nanopct6_lts_plus_naming_audios() { display_alert "$BOARD" "Renaming nanopct6-lts-plus audio" "info"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/boards/nanopct6-lts-plus.conf` around lines 13 - 24, The child's function post_family_tweaks__nanopct6_lts_plus_naming_audios is executed before the parent post_family_tweaks__nanopct6_naming_audios and gets its ES8389 rule truncated by the parent's leading echo (>)—rename the function to post_family_tweaks__zzz_nanopct6_lts_plus_naming_audios so it sorts last and runs after the parent (or alternatively add a no-op override for post_family_tweaks__nanopct6_naming_audios in this file) so the ES8389 echo line (ENV{ID_PATH}=="platform-es8389-sound") is not lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/boards/nanopct6-lts-plus.conf`:
- Around line 13-24: The child's function
post_family_tweaks__nanopct6_lts_plus_naming_audios is executed before the
parent post_family_tweaks__nanopct6_naming_audios and gets its ES8389 rule
truncated by the parent's leading echo (>)—rename the function to
post_family_tweaks__zzz_nanopct6_lts_plus_naming_audios so it sorts last and
runs after the parent (or alternatively add a no-op override for
post_family_tweaks__nanopct6_naming_audios in this file) so the ES8389 echo line
(ENV{ID_PATH}=="platform-es8389-sound") is not lost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3625a1c2-995c-419f-aed8-954457ae2042
📒 Files selected for processing (3)
config/boards/nanopct6-lts-plus.confpatch/kernel/archive/rockchip64-6.18/dt/rk3588-nanopc-t6-lts-plus.dtspatch/kernel/archive/rockchip64-7.0/dt/rk3588-nanopc-t6-lts-plus.dts
🚧 Files skipped from review as they are similar to previous changes (2)
- patch/kernel/archive/rockchip64-6.18/dt/rk3588-nanopc-t6-lts-plus.dts
- patch/kernel/archive/rockchip64-7.0/dt/rk3588-nanopc-t6-lts-plus.dts
igorpecovnik
added
Work in progress
igorpecovnik
force-pushed
the
nanopct6ltsplus
branch
from
75e7125 to
47544d1
Compare
github-actions
bot
added
Needs review
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/boards/nanopct6-lts-plus.conf (1)
13-24:⚠️ Potential issue | 🟠 MajorHook order still allows parent rule file overwrite.
Line 13 defines
post_family_tweaks__nanopct6_lts_plus_naming_audios, which sorts before the inheritedpost_family_tweaks__nanopct6_naming_audios; the parent then truncates the same rules file (>), so the ES8389 rule from Line 21 is lost.Suggested fix
-function post_family_tweaks__nanopct6_lts_plus_naming_audios() { +function post_family_tweaks__999_nanopct6_lts_plus_naming_audios() { display_alert "$BOARD" "Renaming nanopct6-lts-plus audio" "info"Alternative: redefine
post_family_tweaks__nanopct6_naming_audios()as a no-op in this file.Based on learnings: In the Armbian extension framework, functions with the same hook point run in alphanumerical order by the suffix after
__.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/boards/nanopct6-lts-plus.conf` around lines 13 - 24, The hook runs before the parent hook and gets overwritten because post_family_tweaks__nanopct6_naming_audios in the parent truncates the same rules file; rename this function so it sorts after the parent (e.g., post_family_tweaks__nanopct6_lts_plus_z_naming_audios or similar alphanumeric suffix that comes later) so its echo lines append after the parent, or alternatively define post_family_tweaks__nanopct6_naming_audios() in this file as a no-op to prevent the parent from truncating the file; update the function name or noop override accordingly and keep the same body/behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/boards/nanopct6-lts-plus.conf`:
- Around line 13-24: The hook runs before the parent hook and gets overwritten
because post_family_tweaks__nanopct6_naming_audios in the parent truncates the
same rules file; rename this function so it sorts after the parent (e.g.,
post_family_tweaks__nanopct6_lts_plus_z_naming_audios or similar alphanumeric
suffix that comes later) so its echo lines append after the parent, or
alternatively define post_family_tweaks__nanopct6_naming_audios() in this file
as a no-op to prevent the parent from truncating the file; update the function
name or noop override accordingly and keep the same body/behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 045559a4-9c21-426c-8b0d-73f56ef1c0f0
📒 Files selected for processing (3)
config/boards/nanopct6-lts-plus.confpatch/kernel/archive/rockchip64-6.18/dt/rk3588-nanopc-t6-lts-plus.dtspatch/kernel/archive/rockchip64-7.0/dt/rk3588-nanopc-t6-lts-plus.dts
🚧 Files skipped from review as they are similar to previous changes (2)
- patch/kernel/archive/rockchip64-6.18/dt/rk3588-nanopc-t6-lts-plus.dts
- patch/kernel/archive/rockchip64-7.0/dt/rk3588-nanopc-t6-lts-plus.dts
igorpecovnik
force-pushed
the
nanopct6ltsplus
branch
2 times, most recently
from
1949b83 to
c7799b3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/boards/nanopct6-lts-plus.conf (1)
18-29:⚠️ Potential issue | 🟠 MajorAudio udev hook may still be overwritten by parent hook order.
Because this file sources
nanopct6.confand uses the samepost_family_tweaks__*hook point, alphanumerical execution can run this function before the parent audio hook; if the parent still writes with>, the ES8389 rule gets replaced.Suggested fix
-function post_family_tweaks__nanopct6_lts_plus_naming_audios() { +function post_family_tweaks__zzz_nanopct6_lts_plus_naming_audios() {#!/bin/bash # Verify whether parent + child hooks collide and whether parent truncates rules file. echo "=== Hook definitions ===" rg -n -C3 'post_family_tweaks__nanopct6(_lts_plus)?_naming_audios' config/boards --type sh echo echo "=== Parent rule writing operators in nanopct6.conf ===" rg -n -C2 '90-naming-audios.rules|post_family_tweaks__nanopct6_naming_audios|>\s*\$SDCARD|>>\s*\$SDCARD' config/boards/nanopct6.conf --type sh echo echo "=== Hook execution ordering implementation ===" rg -n -C4 'call_extension_method.*post_family_tweaks|compgen -A function|sort' lib --type shBased on learnings: in Armbian extensions, functions sharing a hook point execute in alphanumerical order (
hook_point__unique_identifier()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/boards/nanopct6-lts-plus.conf` around lines 18 - 29, The child hook post_family_tweaks__nanopct6_lts_plus_naming_audios can run before the parent post_family_tweaks__nanopct6_naming_audios and get its 90-naming-audios.rules truncated because the first echo uses >; change the first echo that currently writes with > to use >> (i.e., append like the other lines) or otherwise always append (e.g., tee -a) so the ES8389 and other rules are never overwritten when both hooks run, keeping the function name and the 90-naming-audios.rules target the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/boards/nanopct6-lts-plus.conf`:
- Around line 18-29: The child hook
post_family_tweaks__nanopct6_lts_plus_naming_audios can run before the parent
post_family_tweaks__nanopct6_naming_audios and get its 90-naming-audios.rules
truncated because the first echo uses >; change the first echo that currently
writes with > to use >> (i.e., append like the other lines) or otherwise always
append (e.g., tee -a) so the ES8389 and other rules are never overwritten when
both hooks run, keeping the function name and the 90-naming-audios.rules target
the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42f0e305-6eb3-4a2b-abc3-6f8f0d5291e8
📒 Files selected for processing (3)
config/boards/nanopct6-lts-plus.confpatch/kernel/archive/rockchip64-6.18/dt/rk3588-nanopc-t6-lts-plus.dtspatch/kernel/archive/rockchip64-7.0/dt/rk3588-nanopc-t6-lts-plus.dts
🚧 Files skipped from review as they are similar to previous changes (1)
- patch/kernel/archive/rockchip64-6.18/dt/rk3588-nanopc-t6-lts-plus.dts
igorpecovnik
force-pushed
the
nanopct6ltsplus
branch
from
c7799b3 to
523f462
Compare
rpardini
force-pushed
the
nanopct6ltsplus
branch
from
523f462 to
6de4e8b
Compare
| KERNEL_TEST_TARGET="vendor,current" | ||
| declare -g GRUB_FDT_FILE="${BOOT_FDT_FILE}" # u-boot autodetects, but grub needs to know; store it here so we can unset later | ||
|
|
||
| # Override: u-boot can't auto-detect the LTS Plus variant, so keep BOOT_FDT_FILE set |
There was a problem hiding this comment.
Indeed; could you poke at adc@fec10000 to see what it needs? That way we could send patch to upstream at https://github.com/u-boot/u-boot/blob/master/board/friendlyelec/nanopc-t6-rk3588/nanopc-t6-rk3588.c
4 participants
Summary
Add board support for the FriendlyElec NanoPC-T6 LTS Plus, a variant of the NanoPC-T6 LTS that replaces the Realtek RT5616 audio codec with an Everest ES8389.
What's included
config/boards/nanopct6-lts-plus.conf) — sources the base T6 config, overrides DTB and audio udev rules for ES8389patch/kernel/archive/rockchip64-{6.18,7.0}/dt/rk3588-nanopc-t6-lts-plus.dts) — based on the mainline T6 dtsi with:u2phy3_hostsupply wired to the USB 2.0 host regulatorNeeds: armbian/linux-rockchip#462
Ref: #9446
Hardware differences from T6 LTS
Reference: friendlyarm/kernel-rockchip@524e3e0
Test plan
Summary by CodeRabbit