Add Ariaboard Photonicat 2 support for rockchip64-6.18#9535
Open
HackingGate wants to merge 1 commit intoarmbian:mainfrom
Open
Add Ariaboard Photonicat 2 support for rockchip64-6.18#9535HackingGate wants to merge 1 commit intoarmbian:mainfrom
HackingGate wants to merge 1 commit intoarmbian:mainfrom
Conversation
HackingGate
requested review from
ColorfulRhino,
HeyMeco,
JohnTheCoolingFan,
NicoD-SBC,
SuperKali,
TheSnowfield,
Tonymac32,
ahoneybun,
amazingfate,
brentr,
catalinii,
chainsx,
efectn,
igorpecovnik,
joekhoobyar,
krachlatte,
lanefu,
paolosabatino,
prahal,
pyavitz,
rpardini and
schwar3kat
as code owners
Contributor
📝 WalkthroughWalkthroughIntroduces comprehensive support for the RK3576-based Ariaboard Photonicat2 board across bootloader, kernel, device tree, and system extensions. Includes Mainline U-Boot configuration, kernel drivers for GPIO/MMC tuning, PWM infrastructure (MFPWM core and PWM v4), power domain management, device tree definitions, and PMU DKMS extension. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
HackingGate
added
Hardware
Contributor
🚫 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
Framework
HackingGate
removed
Needs review
17 tasks
Member
|
I see a few things were changed from module to built-in. Is this necessary? |
HackingGate
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch (1)
31-31: Rename the helper before this spreads.
alwasyis a typo, andadd_*_flagunderstates that this helper may also power the domain on. A clearer name will make later grep/debug work easier.♻️ Suggested rename
-static int rockchip_pd_add_alwasy_on_flag(struct rockchip_pm_domain *pd) +static int rockchip_pd_enable_always_on(struct rockchip_pm_domain *pd) ... - error = rockchip_pd_add_alwasy_on_flag(pd); + error = rockchip_pd_enable_always_on(pd);Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch` at line 31, Rename the misspelled helper rockchip_pd_add_alwasy_on_flag to a clearer name that reflects it can also power the domain (e.g., rockchip_pd_enable_always_on); update the function definition and all callers/usages (the other occurrence at the second instance) to the new identifier and fix the typo "alwasy" -> "always" in comments/signatures so grep/debugging finds the correct symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/photonicat-pm.sh`:
- Around line 18-23: After fetching changelog_url and setting debian_version,
add validation: check if debian_version is empty or curl failed (e.g. test -z
"$debian_version"), and if so attempt a safe fallback such as deriving a usable
package version from latest_version (for example strip a leading "v" with
debian_version="${latest_version#v}") or log an error and exit non‑zero; then
only construct photonicat_pm_url when debian_version is non‑empty. Update the
code around variables latest_version, changelog_url, debian_version and
photonicat_pm_url to perform this check and conditional fallback before building
the download URL.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patch`:
- Around line 55-74: The loop currently only breaks when the fallback phase
succeeds, so a successful primary probe gets ignored and the code can still
reach the "V2 All phases bad!" path; change the logic in the loop that calls
clk_set_phase(priv->sample_clk, degree) and mmc_send_tuning(mmc, opcode, NULL)
so that if mmc_send_tuning returns success (== 0) for the initial phase you
immediately accept it and break out of the loop, otherwise proceed to the
existing fallback branch (flip by testing success first or invert the current if
condition), keeping references to degrees, priv->last_degree, clk_set_phase,
mmc_send_tuning, dev_info and dev_warn and preserving the fallback behavior and
the -EIO return when no phase succeeds.
- Around line 18-24: The firmware-phase inheritance flag is currently a
function-local static and must be made per-host: add a boolean member to struct
dw_mci_rockchip_priv_data (e.g., use_v2_tuning or bootloader_inherit) and
replace all uses of the local static inherit variable with the per-host field;
update the code paths that initialize and check inheritance (the blocks near the
additions to struct and the logic around lines referenced 38-45 and 101-112 in
the diff) to read/write the new struct member via the host private data instead
of the static, ensuring each DW-MMC host preserves its own firmware-phase
inheritance state.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch`:
- Around line 35-39: The current check that sets GENPD_FLAG_ALWAYS_ON only tests
rockchip_pmu_domain_is_on(pd) and misses domains where mem_status_mask indicates
memory is down; update the condition to treat the domain as off if either the
power bit is clear or (mem_status_mask is set and
rockchip_pmu_domain_is_mem_on(pd) is false) so that rockchip_pd_power(pd, true)
is invoked when needed; apply the same corrected logic to the other occurrence
that sets GENPD_FLAG_ALWAYS_ON and checks rockchip_pmu_domain_is_on (the later
check around the other GENPD_FLAG_ALWAYS_ON set) to keep behavior consistent.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch`:
- Around line 168-185: The helper mfpwm_check_pwmf currently dereferences
pwmf->parent to get dev before validating pwmf/parent; move the validation
before any dereference: check IS_ERR_OR_NULL(pwmf) and
IS_ERR_OR_NULL(pwmf->parent) first inside mfpwm_check_pwmf (returning -EINVAL
and logging via dev_warn), and only after those checks obtain struct device *dev
= &pwmf->parent->pdev->dev to use in the dev_warn messages; update the two
dev_warn calls to use the now-safe dev pointer.
- Around line 189-211: Rollback the ownership state if clk_enable fails in
mfpwm_do_acquire: after calling clk_enable(mfpwm->pclk) check its return value
and on error decrement/restore mfpwm->acquire_cnt and clear mfpwm->active_func
if the counter becomes zero (undo the earlier assignment to mfpwm->active_func
and the increment of mfpwm->acquire_cnt), and keep the dev_warn/dev_dbg as
appropriate; use the existing symbols mfpwm_do_acquire, mfpwm->active_func,
mfpwm->acquire_cnt and clk_enable to locate where to add the rollback so the
parent isn't left logically owned when clk enabling fails.
- Around line 214-234: mfpwm_acquire currently turns transient contention into
-EBUSY by using spin_trylock_irqsave; change it to block on the lock instead so
same-function re-entrant acquires don't fail spuriously: after validating with
mfpwm_check_pwmf, use spin_lock_irqsave(&mfpwm->state_lock, flags) (and keep the
matching spin_unlock_irqrestore) around the mfpwm_do_acquire call so callers
retry/wait rather than receiving -EBUSY; keep references to pwmf->parent/mfpwm,
dev_dbg, mfpwm_do_acquire and the state_lock in the same function.
- Around line 295-297: The allocation result checks use IS_ERR()/PTR_ERR() but
devm_kzalloc() returns NULL on failure; update the checks for both allocations
(the one assigning to func and the one assigning to mfpwm) to use NULL checks
(if (!func) and if (!mfpwm)) and return -ENOMEM on failure instead of PTR_ERR().
Ensure you adjust both occurrences (the devm_kzalloc that creates func and the
separate devm_kzalloc that creates mfpwm around lines shown in the diff).
- Around line 381-400: Check the return of clk_register_mux() for IS_ERR()
before calling clk_prepare(); if it returns an error, log and return the
appropriate PTR_ERR. Also add an error-unwind path when
mfpwm_register_subdevs(mfpwm) fails: call clk_unprepare(mfpwm->chosen_clk) and
clk_unregister_mux(mfpwm->chosen_clk) (or the matching unregister function)
before returning, and ensure platform_set_drvdata(pdev, mfpwm) is only called
after successful resource setup to avoid leaving partial state; use
clk_prepare/clk_unprepare and clk_unregister_mux paired with clk_register_mux
and handle PTR_ERR from clk_register_mux accordingly.
- Around line 411-416: The spinlock in rockchip_mfpwm_remove currently surrounds
sleepable CCF calls clk_unprepare() and clk_unregister_mux(), which must be
moved out of the atomic section: inside the
spin_lock_irqsave(&mfpwm->state_lock, flags) section just capture and clear the
pointer (e.g. copy mfpwm->chosen_clk to a local variable and set
mfpwm->chosen_clk = NULL) and then release the lock with
spin_unlock_irqrestore(&mfpwm->state_lock, flags); after unlocking, if the local
chosen_clk is non-NULL call clk_unprepare(local_clk) and
clk_unregister_mux(local_clk). This preserves the state_lock protection while
avoiding calling sleepable functions in atomic context.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch`:
- Around line 264-335: The commit writes PWMV4_REG_ENABLE before ensuring clocks
are enabled, so if clk_enable or clk_rate_exclusive_get fail the hardware
remains latched-on; update rockchip_pwm_v4_write_wf to either (a) acquire/enable
the core clock and call clk_rate_exclusive_get before writing PWMV4_REG_ENABLE,
or (b) on any failure path after the ENABLE write explicitly clear
PWMV4_REG_ENABLE (using mfpwm_reg_write with PWMV4_REG_ENABLE = 0) before
releasing the mfpwm and returning; ensure the extra mfpwm_acquire/mfpwm_release
pairing around the successful enable path remains balanced and reference the
existing calls to mfpwm_acquire, mfpwm_release, clk_enable, clk_disable,
clk_rate_exclusive_get and PWMV4_REG_ENABLE when making the change.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch`:
- Around line 308-315: The interrupt handler currently returns IRQ_NONE when
unrelated bits remain set even after it clears its own bits; modify the logic in
the handler (the block using clr, intsts, mfpwm_reg_write(pc->pwmf->base,
PWMV4_REG_INTSTS, clr)) so that if clr != 0 (i.e., this driver cleared any
interrupt bits) the function returns IRQ_HANDLED instead of IRQ_NONE; otherwise
keep returning IRQ_NONE when no bits were cleared, and still return IRQ_HANDLED
when intsts == clr as existing code does.
- Around line 162-224: In rkpwmc_enable_write: don't hold
guard(spinlock)(&pc->enable_lock) across sleepable calls
(clk_rate_exclusive_get/put) — take the spinlock only to check/flip
pc->is_enabled and to protect fast register writes and mfpwm_acquire/release
references, then release it before calling clk_rate_exclusive_get/put; also
ensure error paths always reset the hardware registers (use mfpwm_reg_write to
clear PWMV4_REG_INT_EN and PWMV4_REG_ENABLE) before returning and only set
pc->is_enabled = true after all sleepable operations (clk_enable +
clk_rate_exclusive_get) succeed; adjust the error labels (err_unprotect_pwm_clk,
err_disable_pwm_clk, err_release) to clear registers and release mfpwm as needed
and to avoid holding the spinlock during sleepable calls.
---
Nitpick comments:
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch`:
- Line 31: Rename the misspelled helper rockchip_pd_add_alwasy_on_flag to a
clearer name that reflects it can also power the domain (e.g.,
rockchip_pd_enable_always_on); update the function definition and all
callers/usages (the other occurrence at the second instance) to the new
identifier and fix the typo "alwasy" -> "always" in comments/signatures so
grep/debugging finds the correct symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3448c15b-4a6f-40ec-9557-cfcacfea368b
📒 Files selected for processing (16)
config/boards/photonicat2.cscconfig/kernel/linux-rockchip64-current.configextensions/photonicat-pm.shpatch/kernel/archive/rockchip64-6.18/board-photonicat2.patchpatch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dtspatch/kernel/archive/rockchip64-6.18/rk3576-0001-gpio-rockchip-set-input-direction-when-request-irq.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0004-dt-bindings-pwm-rockchip-rk3576-pwm.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0008-arm64-dts-rk3576-add-pwm-nodes.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0009-arm64-dts-rk3576-add-dma-coherent-pcie-gmac.patchpatch/u-boot/v2026.01/board_photonicat2/add-photonicat2-dts.patchpatch/u-boot/v2026.01/defconfig/photonicat2-rk3576_defconfig
Comment on lines
+18
to
+23
| api_url="https://api.github.com/repos/HackingGate/photonicat-pm/releases/latest" | ||
| latest_version=$(curl -s "${api_url}" | jq -r '.tag_name') | ||
| # Get the Debian version from changelog | ||
| changelog_url="https://raw.githubusercontent.com/HackingGate/photonicat-pm/refs/tags/${latest_version}/debian/changelog" | ||
| debian_version=$(curl -s "${changelog_url}" | head -1 | grep -oP 'photonicat-pm \(\K[^)]+') | ||
| photonicat_pm_url="https://github.com/HackingGate/photonicat-pm/releases/download/${latest_version}/photonicat-pm-dkms_${debian_version}_all.deb" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "photonicat-pm.sh" -o -name "*photonicat*"Repository: armbian/build
Length of output: 227
🏁 Script executed:
find . -path "*/extensions/*.sh" -type f | head -20Repository: armbian/build
Length of output: 738
🏁 Script executed:
cat -n extensions/photonicat-pm.shRepository: armbian/build
Length of output: 2311
🏁 Script executed:
rg -A5 -B5 "releases/latest" --type shRepository: armbian/build
Length of output: 6194
🏁 Script executed:
rg "PHOTONICAT_PM_VERSION" --type shRepository: armbian/build
Length of output: 39
🏁 Script executed:
git log --oneline --all -- extensions/photonicat-pm.sh | head -5Repository: armbian/build
Length of output: 100
🏁 Script executed:
git log --oneline --all -- extensions/yt6801.sh | head -5Repository: armbian/build
Length of output: 135
🏁 Script executed:
rg "VERSION|_VERSION" extensions/photonicat-pm.sh extensions/yt6801.sh extensions/bcmdhd.sh -A2 -B2Repository: armbian/build
Length of output: 39
🏁 Script executed:
find config/boards -type f -exec grep -l "photonicat\|yt6801\|aic8800" {} \;Repository: armbian/build
Length of output: 647
🏁 Script executed:
cat -n lib/functions/common/main-helpers.sh | grep -A10 -B5 "set -e"Repository: armbian/build
Length of output: 125
🏁 Script executed:
find lib/functions -name "*.sh" | head -20Repository: armbian/build
Length of output: 984
🏁 Script executed:
grep -r "set -e" lib/functions --include="*.sh" | head -5Repository: armbian/build
Length of output: 414
🏁 Script executed:
cat config/boards/photonicat2.csc | grep -A5 -B5 "photonicat\|EXTENSIONS"Repository: armbian/build
Length of output: 868
🏁 Script executed:
git log --oneline -20 --all -- extensions/photonicat-pm.sh extensions/yt6801.sh extensions/bcmdhd.sh extensions/radxa-aic8800.shRepository: armbian/build
Length of output: 621
🏁 Script executed:
grep -A30 "post_install_kernel_debs__install_yt6801_dkms_package" extensions/yt6801.shRepository: armbian/build
Length of output: 1120
🏁 Script executed:
git log --oneline -50 --all -- extensions/uefi-edk2-rk3588.sh extensions/syterkit-allwinner.shRepository: armbian/build
Length of output: 171
🏁 Script executed:
rg "releases/latest" extensions/ --files-with-matchesRepository: armbian/build
Length of output: 218
🏁 Script executed:
grep -n "curl -s" extensions/photonicat-pm.sh extensions/yt6801.sh extensions/radxa-aic8800.sh extensions/bcmdhd.sh | head -20Repository: armbian/build
Length of output: 530
Add error handling for changelog metadata fetch.
Lines 21-22 introduce a second network round-trip to extract the Debian version from upstream changelog. If this fetch fails or returns empty, debian_version becomes null and line 23 builds a broken download URL, which then fails during chroot installation. Add validation with graceful fallback:
changelog_url="https://raw.githubusercontent.com/HackingGate/photonicat-pm/refs/tags/${latest_version}/debian/changelog"
- debian_version=$(curl -s "${changelog_url}" | head -1 | grep -oP 'photonicat-pm \(\K[^)]+')
+ debian_version=$(curl -fsSL "${changelog_url}" 2>/dev/null | head -1 | grep -oP 'photonicat-pm \(\K[^)]+' || true)
+ if [[ -z "${debian_version}" ]]; then
+ display_alert "Unable to resolve photonicat-pm Debian version" "skipping installation" "warn"
+ return 0
+ fiThis pattern is specific to photonicat-pm's extra metadata layer (unlike sibling DKMS extensions that derive version directly from tag). Other extensions share the releases/latest pattern and accept its non-determinism as acceptable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| api_url="https://api.github.com/repos/HackingGate/photonicat-pm/releases/latest" | |
| latest_version=$(curl -s "${api_url}" | jq -r '.tag_name') | |
| # Get the Debian version from changelog | |
| changelog_url="https://raw.githubusercontent.com/HackingGate/photonicat-pm/refs/tags/${latest_version}/debian/changelog" | |
| debian_version=$(curl -s "${changelog_url}" | head -1 | grep -oP 'photonicat-pm \(\K[^)]+') | |
| photonicat_pm_url="https://github.com/HackingGate/photonicat-pm/releases/download/${latest_version}/photonicat-pm-dkms_${debian_version}_all.deb" | |
| api_url="https://api.github.com/repos/HackingGate/photonicat-pm/releases/latest" | |
| latest_version=$(curl -s "${api_url}" | jq -r '.tag_name') | |
| # Get the Debian version from changelog | |
| changelog_url="https://raw.githubusercontent.com/HackingGate/photonicat-pm/refs/tags/${latest_version}/debian/changelog" | |
| debian_version=$(curl -fsSL "${changelog_url}" 2>/dev/null | head -1 | grep -oP 'photonicat-pm \(\K[^)]+' || true) | |
| if [[ -z "${debian_version}" ]]; then | |
| display_alert "Unable to resolve photonicat-pm Debian version" "skipping installation" "warn" | |
| return 0 | |
| fi | |
| photonicat_pm_url="https://github.com/HackingGate/photonicat-pm/releases/download/${latest_version}/photonicat-pm-dkms_${debian_version}_all.deb" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/photonicat-pm.sh` around lines 18 - 23, After fetching
changelog_url and setting debian_version, add validation: check if
debian_version is empty or curl failed (e.g. test -z "$debian_version"), and if
so attempt a safe fallback such as deriving a usable package version from
latest_version (for example strip a leading "v" with
debian_version="${latest_version#v}") or log an error and exit non‑zero; then
only construct photonicat_pm_url when debian_version is non‑empty. Update the
code around variables latest_version, changelog_url, debian_version and
photonicat_pm_url to perform this check and conditional fallback before building
the download URL.
Comment on lines
+18
to
+24
| @@ -35,6 +35,8 @@ struct dw_mci_rockchip_priv_data { | ||
| int default_sample_phase; | ||
| int num_phases; | ||
| bool internal_phase; | ||
| + int last_degree; | ||
| + bool use_v2_tuning; | ||
| }; |
Contributor
There was a problem hiding this comment.
Make the firmware-phase inheritance state per host.
inherit is a function-local static, so the first DW-MMC instance that executes this path disables inheritance for every later controller. On systems with more than one Rockchip MMC host, later hosts will skip their own bootloader-provided phase and start from a stale default instead. This state should live in struct dw_mci_rockchip_priv_data.
Suggested fix
struct dw_mci_rockchip_priv_data {
int default_sample_phase;
int num_phases;
bool internal_phase;
int last_degree;
+ bool inherit_phase;
bool use_v2_tuning;
};
@@
- static bool inherit = true;
-
- if (inherit) {
- inherit = false;
+ if (priv->inherit_phase) {
+ priv->inherit_phase = false;
i = clk_get_phase(priv->sample_clk) / 90;
degree = degrees[i];
goto done;
}
@@
priv = host->priv;
+ priv->inherit_phase = true;
if (of_property_read_bool(np, "rockchip,use-v2-tuning"))
priv->use_v2_tuning = true;Also applies to: 38-45, 101-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patch`
around lines 18 - 24, The firmware-phase inheritance flag is currently a
function-local static and must be made per-host: add a boolean member to struct
dw_mci_rockchip_priv_data (e.g., use_v2_tuning or bootloader_inherit) and
replace all uses of the local static inherit variable with the per-host field;
update the code paths that initialize and check inheritance (the blocks near the
additions to struct and the logic around lines referenced 38-45 and 101-112 in
the diff) to read/write the new struct member via the host private data instead
of the static, ensuring each DW-MMC host preserves its own firmware-phase
inheritance state.
Comment on lines
+55
to
+74
| + for (i = 0; i < ARRAY_SIZE(degrees); i++) { | ||
| + degree = degrees[i] + priv->last_degree + 90; | ||
| + degree = degree % 360; | ||
| + clk_set_phase(priv->sample_clk, degree); | ||
| + if (mmc_send_tuning(mmc, opcode, NULL)) { | ||
| + /* | ||
| + * Tuning error, the phase is a bad phase, | ||
| + * then try using the calculated best phase. | ||
| + */ | ||
| + dev_info(host->dev, "V2 tuned phase to %d error, try the best phase\n", degree); | ||
| + degree = (degree + 180) % 360; | ||
| + clk_set_phase(priv->sample_clk, degree); | ||
| + if (!mmc_send_tuning(mmc, opcode, NULL)) | ||
| + break; | ||
| + } | ||
| + } | ||
| + | ||
| + if (i == ARRAY_SIZE(degrees)) { | ||
| + dev_warn(host->dev, "V2 All phases bad!"); | ||
| + return -EIO; |
Contributor
There was a problem hiding this comment.
Accept a successful primary phase before falling through to -EIO.
Inside this loop, the only break is in the fallback branch after a reported tuning error. If the current phase is already acceptable, the code keeps iterating and can still hit Line 72 and report "V2 All phases bad!" after one or more successful probes.
Suggested fix
for (i = 0; i < ARRAY_SIZE(degrees); i++) {
degree = degrees[i] + priv->last_degree + 90;
degree = degree % 360;
clk_set_phase(priv->sample_clk, degree);
- if (mmc_send_tuning(mmc, opcode, NULL)) {
- /*
- * Tuning error, the phase is a bad phase,
- * then try using the calculated best phase.
- */
- dev_info(host->dev, "V2 tuned phase to %d error, try the best phase\n", degree);
- degree = (degree + 180) % 360;
- clk_set_phase(priv->sample_clk, degree);
- if (!mmc_send_tuning(mmc, opcode, NULL))
- break;
- }
+ if (!mmc_send_tuning(mmc, opcode, NULL))
+ break;
+
+ /*
+ * Tuning error, the phase is a bad phase,
+ * then try using the calculated best phase.
+ */
+ dev_info(host->dev, "V2 tuned phase to %d error, try the best phase\n", degree);
+ degree = (degree + 180) % 360;
+ clk_set_phase(priv->sample_clk, degree);
+ if (!mmc_send_tuning(mmc, opcode, NULL))
+ break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| + for (i = 0; i < ARRAY_SIZE(degrees); i++) { | |
| + degree = degrees[i] + priv->last_degree + 90; | |
| + degree = degree % 360; | |
| + clk_set_phase(priv->sample_clk, degree); | |
| + if (mmc_send_tuning(mmc, opcode, NULL)) { | |
| + /* | |
| + * Tuning error, the phase is a bad phase, | |
| + * then try using the calculated best phase. | |
| + */ | |
| + dev_info(host->dev, "V2 tuned phase to %d error, try the best phase\n", degree); | |
| + degree = (degree + 180) % 360; | |
| + clk_set_phase(priv->sample_clk, degree); | |
| + if (!mmc_send_tuning(mmc, opcode, NULL)) | |
| + break; | |
| + } | |
| + } | |
| + | |
| + if (i == ARRAY_SIZE(degrees)) { | |
| + dev_warn(host->dev, "V2 All phases bad!"); | |
| + return -EIO; | |
| for (i = 0; i < ARRAY_SIZE(degrees); i++) { | |
| degree = degrees[i] + priv->last_degree + 90; | |
| degree = degree % 360; | |
| clk_set_phase(priv->sample_clk, degree); | |
| if (!mmc_send_tuning(mmc, opcode, NULL)) | |
| break; | |
| /* | |
| * Tuning error, the phase is a bad phase, | |
| * then try using the calculated best phase. | |
| */ | |
| dev_info(host->dev, "V2 tuned phase to %d error, try the best phase\n", degree); | |
| degree = (degree + 180) % 360; | |
| clk_set_phase(priv->sample_clk, degree); | |
| if (!mmc_send_tuning(mmc, opcode, NULL)) | |
| break; | |
| } | |
| if (i == ARRAY_SIZE(degrees)) { | |
| dev_warn(host->dev, "V2 All phases bad!"); | |
| return -EIO; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patch`
around lines 55 - 74, The loop currently only breaks when the fallback phase
succeeds, so a successful primary probe gets ignored and the code can still
reach the "V2 All phases bad!" path; change the logic in the loop that calls
clk_set_phase(priv->sample_clk, degree) and mmc_send_tuning(mmc, opcode, NULL)
so that if mmc_send_tuning returns success (== 0) for the initial phase you
immediately accept it and break out of the loop, otherwise proceed to the
existing fallback branch (flip by testing success first or invert the current if
condition), keeping references to degrees, priv->last_degree, clk_set_phase,
mmc_send_tuning, dev_info and dev_warn and preserving the fallback behavior and
the -EIO return when no phase succeeds.
patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch
Show resolved
Hide resolved
Comment on lines
+168
to
+185
| +static int mfpwm_check_pwmf(const struct rockchip_mfpwm_func *pwmf, | ||
| + const char *fname) | ||
| +{ | ||
| + struct device *dev = &pwmf->parent->pdev->dev; | ||
| + | ||
| + if (IS_ERR_OR_NULL(pwmf)) { | ||
| + dev_warn(dev, "called %s with an erroneous handle, no effect\n", | ||
| + fname); | ||
| + return -EINVAL; | ||
| + } | ||
| + | ||
| + if (IS_ERR_OR_NULL(pwmf->parent)) { | ||
| + dev_warn(dev, "called %s with an erroneous mfpwm_func parent, no effect\n", | ||
| + fname); | ||
| + return -EINVAL; | ||
| + } | ||
| + | ||
| + return 0; |
Contributor
There was a problem hiding this comment.
Validate pwmf before deriving dev from it.
Line 171 takes &pwmf->parent->pdev->dev before either pwmf or pwmf->parent has been checked, so the "erroneous handle" guard itself can oops on NULL or ERR callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch`
around lines 168 - 185, The helper mfpwm_check_pwmf currently dereferences
pwmf->parent to get dev before validating pwmf/parent; move the validation
before any dereference: check IS_ERR_OR_NULL(pwmf) and
IS_ERR_OR_NULL(pwmf->parent) first inside mfpwm_check_pwmf (returning -EINVAL
and logging via dev_warn), and only after those checks obtain struct device *dev
= &pwmf->parent->pdev->dev to use in the dev_warn messages; update the two
dev_warn calls to use the now-safe dev pointer.
Comment on lines
+381
to
+400
| + mfpwm->chosen_clk = clk_register_mux(dev, clk_mux_name, mux_p_names, | ||
| + ARRAY_SIZE(mux_p_names), | ||
| + CLK_SET_RATE_PARENT, | ||
| + mfpwm->base + PWMV4_REG_CLK_CTRL, | ||
| + PWMV4_CLK_SRC_SHIFT, PWMV4_CLK_SRC_WIDTH, | ||
| + CLK_MUX_HIWORD_MASK, NULL); | ||
| + ret = clk_prepare(mfpwm->chosen_clk); | ||
| + if (ret) { | ||
| + dev_err(dev, "failed to prepare PWM clock mux: %pe\n", | ||
| + ERR_PTR(ret)); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + platform_set_drvdata(pdev, mfpwm); | ||
| + | ||
| + ret = mfpwm_register_subdevs(mfpwm); | ||
| + if (ret) { | ||
| + dev_err(dev, "failed to register sub-devices: %pe\n", | ||
| + ERR_PTR(ret)); | ||
| + return ret; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat /tmp/path_to_check && find . -name "*rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch" -type fRepository: armbian/build
Length of output: 107
🏁 Script executed:
find . -name "*rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch" -type f 2>/dev/null | head -5Repository: armbian/build
Length of output: 143
🏁 Script executed:
find . -path "*/rockchip64-6.18/*" -name "*mfpwm*" -type f 2>/dev/nullRepository: armbian/build
Length of output: 143
🏁 Script executed:
ls -la patch/kernel/archive/rockchip64-6.18/ 2>/dev/null | grep -i mfpwmRepository: armbian/build
Length of output: 156
🏁 Script executed:
ls -la patch/kernel/archive/ 2>/dev/null | head -20Repository: armbian/build
Length of output: 1216
🏁 Script executed:
git ls-files | grep -i rockchip64 | head -20Repository: armbian/build
Length of output: 1275
🏁 Script executed:
cat -n ./patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch | sed -n '370,410p'Repository: armbian/build
Length of output: 1536
🏁 Script executed:
cat -n ./patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch | sed -n '406,450p'Repository: armbian/build
Length of output: 1650
🏁 Script executed:
cat -n ./patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch | sed -n '300,370p'Repository: armbian/build
Length of output: 2603
🏁 Script executed:
# Let me check if there's any documentation or API info about clk_register_mux behavior
# Also check if devm_* versions are used elsewhere in the patch
cat -n ./patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch | grep -E "devm_|clk_" | head -40Repository: armbian/build
Length of output: 1198
🌐 Web query:
clk_register_mux kernel API return value error handling
💡 Result:
clk_register_mux() does not return an int. It returns a pointer (struct clk *) and uses the kernel “error pointer” convention:
- Success: returns a valid
struct clk * - Failure: returns an
ERR_PTR(-Exxx)(so you must useIS_ERR()/PTR_ERR()to detect/extract the error code) [1][2][3]
Minimal correct error handling:
struct clk *clk;
clk = clk_register_mux(dev, name, parent_names, num_parents, flags,
reg, shift, width, clk_mux_flags, lock);
if (IS_ERR(clk))
return PTR_ERR(clk); /* returns negative errno */Notes:
- The underlying registration helper is
clk_register_mux_table()and theclk_register_mux()macro just wraps it [1]. - The same pattern applies to the
clk_hw_register_mux*()family (struct clk_hw *return, error viaERR_PTR) [1]. - On a non-devm success path, remember to
clk_unregister(clk)during cleanup/remove; on an error-pointer return you must not unregister (because you don’t have a real clock object) [1][3].
Sources: [1] include/linux/clk-provider.h (prototypes/macros for clk_register_mux* and clk_hw_register_mux*), [2] include/linux/err.h (IS_ERR, PTR_ERR, ERR_PTR), [3] kernel doc “Return Conventions” (error-pointer pattern).
Add IS_ERR() check for clk_register_mux() return value and unwind errors in probe.
Line 381 calls clk_register_mux() which returns a struct clk * that can be an error pointer (ERR_PTR()), but line 387 calls clk_prepare() unconditionally without checking for errors. If clk_register_mux() fails, passing the error pointer to clk_prepare() will cause undefined behavior.
Additionally, if mfpwm_register_subdevs() fails at lines 396-400, the function returns without unwinding the prepared clock. Since remove() is only called on successful probe, the clock would leak. The probe path needs an explicit error handler to call clk_unprepare() and clk_unregister_mux() before returning on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch`
around lines 381 - 400, Check the return of clk_register_mux() for IS_ERR()
before calling clk_prepare(); if it returns an error, log and return the
appropriate PTR_ERR. Also add an error-unwind path when
mfpwm_register_subdevs(mfpwm) fails: call clk_unprepare(mfpwm->chosen_clk) and
clk_unregister_mux(mfpwm->chosen_clk) (or the matching unregister function)
before returning, and ensure platform_set_drvdata(pdev, mfpwm) is only called
after successful resource setup to avoid leaving partial state; use
clk_prepare/clk_unprepare and clk_unregister_mux paired with clk_register_mux
and handle PTR_ERR from clk_register_mux accordingly.
Comment on lines
+411
to
+416
| + spin_lock_irqsave(&mfpwm->state_lock, flags); | ||
| + | ||
| + if (mfpwm->chosen_clk) { | ||
| + clk_unprepare(mfpwm->chosen_clk); | ||
| + clk_unregister_mux(mfpwm->chosen_clk); | ||
| + } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch" --type fRepository: armbian/build
Length of output: 141
🏁 Script executed:
wc -l "patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch"Repository: armbian/build
Length of output: 145
🏁 Script executed:
sed -n '380,450p' "patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch"Repository: armbian/build
Length of output: 2019
🏁 Script executed:
web_search
clk_unprepare kernel CCF sleepableRepository: armbian/build
Length of output: 159
🌐 Web query:
clk_unprepare clk_unregister_mux kernel CCF sleepable operations
💡 Result:
In the Linux Common Clock Framework (CCF) the API is intentionally split into sleepable vs atomic operations:
- Sleepable path:
clk_prepare()/clk_unprepare()
These may sleep because CCF holds the prepare lock (a mutex) across “all other operations” (i.e., everything except.enable/.disable). Therefore they must not be called in atomic context (IRQ, spinlock-held, etc.). [1] - Atomic path:
clk_enable()/clk_disable()
These are held under the enable lock (a spinlock), so the underlying.enable/.disableops must not sleep, and the API is allowed in atomic context. [1] - This split exists specifically because many clock providers can’t implement enable/disable atomically (e.g., clocks controlled over I2C/SCMI), so “might sleep” work is expected to live in prepare/unprepare. [2]
clk_unregister_mux() is not the inverse of clk_unprepare()—it’s the provider teardown for a mux clock that was registered with clk_register_mux() (or table variant):
void clk_unregister_mux(struct clk *clk); [3]
Practical implications:
- If your driver is a clock consumer: pair calls like
clk_prepare()↔clk_unprepare()(sleepable context only)clk_enable()↔clk_disable()(can be atomic)
- If your driver registers a mux clock provider: in remove/exit (process context), ensure it’s no longer in use, then call
clk_unregister_mux()(or use devm-managed registration so it auto-unregisters).clk_unprepare()is only appropriate if your code previously prepared that clock as a consumer—it’s not a required step “before unregistering” a mux clock.
Sources:
[1] Linux kernel docs: CCF locking + atomic vs sleepable rules for enable vs prepare paths. (docs.kernel.org)
[2] LKML patch discussion: explicit “clk_prepare/clk_unprepare for stuff that might sleep” motivation. (spinics.net)
[3] clk-provider.h: mux registration/unregistration API (clk_unregister_mux, clk_hw_unregister_mux). (android.googlesource.com)
Citations:
- 1: https://docs.kernel.org/driver-api/clk.html
- 2: https://www.spinics.net/lists/kernel/msg3808343.html
- 3: https://android.googlesource.com/kernel/msm/%2B/15cf51a0f2ebde6529357685543e0b4170fb3b5c/include/linux/clk-provider.h
Move clk_unprepare() and clk_unregister_mux() outside the spinlock.
clk_unprepare() and clk_unregister_mux() are sleepable CCF operations that acquire mutexes and cannot be called in atomic context. Calling them inside the spinlock on lines 411-416 will cause a "sleeping in atomic context" BUG during module unload. Since rockchip_mfpwm_remove() runs in process context, the spinlock-protected section should be narrowed to only the necessary state-lock operations, and the clock teardown should happen outside the critical section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch`
around lines 411 - 416, The spinlock in rockchip_mfpwm_remove currently
surrounds sleepable CCF calls clk_unprepare() and clk_unregister_mux(), which
must be moved out of the atomic section: inside the
spin_lock_irqsave(&mfpwm->state_lock, flags) section just capture and clear the
pointer (e.g. copy mfpwm->chosen_clk to a local variable and set
mfpwm->chosen_clk = NULL) and then release the lock with
spin_unlock_irqrestore(&mfpwm->state_lock, flags); after unlocking, if the local
chosen_clk is non-NULL call clk_unprepare(local_clk) and
clk_unregister_mux(local_clk). This preserves the state_lock protection while
avoiding calling sleepable functions in atomic context.
Comment on lines
+264
to
+335
| +static int rockchip_pwm_v4_write_wf(struct pwm_chip *chip, struct pwm_device *pwm, | ||
| + const void *_wfhw) | ||
| +{ | ||
| + struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip); | ||
| + const struct rockchip_pwm_v4_wf *wfhw = _wfhw; | ||
| + bool was_enabled = false; | ||
| + int ret = 0; | ||
| + | ||
| + ret = mfpwm_acquire(pc->pwmf); | ||
| + if (ret) | ||
| + return ret; | ||
| + | ||
| + was_enabled = rockchip_pwm_v4_is_enabled(mfpwm_reg_read(pc->pwmf->base, | ||
| + PWMV4_REG_ENABLE)); | ||
| + | ||
| + /* | ||
| + * "But Nicolas", you ask with valid concerns, "why would you enable the | ||
| + * PWM before setting all the parameter registers?" | ||
| + * | ||
| + * Excellent question, Mr. Reader M. Strawman! The RK3576 TRM Part 1 | ||
| + * Section 34.6.3 specifies that this is the intended order of writes. | ||
| + * Doing the PWM_EN and PWM_CLK_EN writes after the params but before | ||
| + * the CTRL_UPDATE_EN, or even after the CTRL_UPDATE_EN, results in | ||
| + * erratic behaviour where repeated turning on and off of the PWM may | ||
| + * not turn it off under all circumstances. This is also why we don't | ||
| + * use relaxed writes; it's not worth the footgun. | ||
| + */ | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, | ||
| + FIELD_PREP_WM16(PWMV4_EN_BOTH_MASK, wfhw->enable)); | ||
| + | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_PERIOD, wfhw->period); | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_DUTY, wfhw->duty); | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_OFFSET, wfhw->offset); | ||
| + | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS); | ||
| + | ||
| + /* Commit new configuration to hardware output. */ | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, | ||
| + PWMV4_CTRL_UPDATE_EN); | ||
| + | ||
| + if (rockchip_pwm_v4_is_enabled(wfhw->enable)) { | ||
| + if (!was_enabled) { | ||
| + dev_dbg(&chip->dev, "Enabling PWM output\n"); | ||
| + ret = clk_enable(pc->pwmf->core); | ||
| + if (ret) | ||
| + goto err_mfpwm_release; | ||
| + ret = clk_rate_exclusive_get(pc->pwmf->core); | ||
| + if (ret) { | ||
| + clk_disable(pc->pwmf->core); | ||
| + goto err_mfpwm_release; | ||
| + } | ||
| + | ||
| + /* | ||
| + * Output should be on now, acquire device to guarantee | ||
| + * exclusion with other device functions while it's on. | ||
| + */ | ||
| + ret = mfpwm_acquire(pc->pwmf); | ||
| + if (ret) | ||
| + goto err_mfpwm_release; | ||
| + } | ||
| + } else if (was_enabled) { | ||
| + dev_dbg(&chip->dev, "Disabling PWM output\n"); | ||
| + clk_rate_exclusive_put(pc->pwmf->core); | ||
| + clk_disable(pc->pwmf->core); | ||
| + /* Output is off now, extra release to balance extra acquire */ | ||
| + mfpwm_release(pc->pwmf); | ||
| + } | ||
| + | ||
| +err_mfpwm_release: | ||
| + mfpwm_release(pc->pwmf); | ||
| + | ||
| + return ret; |
Contributor
There was a problem hiding this comment.
Clock setup failures leave a latched enabled waveform behind.
Lines 291-302 commit the new ENABLE state before clk_enable() and clk_rate_exclusive_get() are known to succeed. If either clock call fails, the error path only drops the temporary MFPWM acquire, so the registers still describe an active waveform and the next successful clock enable can start it unexpectedly. Either secure the clock first or explicitly clear PWMV4_REG_ENABLE on every failure path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch`
around lines 264 - 335, The commit writes PWMV4_REG_ENABLE before ensuring
clocks are enabled, so if clk_enable or clk_rate_exclusive_get fail the hardware
remains latched-on; update rockchip_pwm_v4_write_wf to either (a) acquire/enable
the core clock and call clk_rate_exclusive_get before writing PWMV4_REG_ENABLE,
or (b) on any failure path after the ENABLE write explicitly clear
PWMV4_REG_ENABLE (using mfpwm_reg_write with PWMV4_REG_ENABLE = 0) before
releasing the mfpwm and returning; ensure the extra mfpwm_acquire/mfpwm_release
pairing around the successful enable path remains balanced and reference the
existing calls to mfpwm_acquire, mfpwm_release, clk_enable, clk_disable,
clk_rate_exclusive_get and PWMV4_REG_ENABLE when making the change.
Comment on lines
+162
to
+224
| +static int rkpwmc_enable_write(struct counter_device *counter, | ||
| + struct counter_count *count, | ||
| + u8 enable) | ||
| +{ | ||
| + struct rockchip_pwm_capture *pc = counter_priv(counter); | ||
| + int ret; | ||
| + | ||
| + guard(spinlock)(&pc->enable_lock); | ||
| + | ||
| + if (!!enable != pc->is_enabled) { | ||
| + ret = mfpwm_acquire(pc->pwmf); | ||
| + if (ret) | ||
| + return ret; | ||
| + | ||
| + if (enable) { | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, | ||
| + PWMV4_EN(false)); | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, | ||
| + PWMV4_CTRL_CAP_FLAGS); | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INT_EN, | ||
| + PWMV4_INT_LPC_W(true) | | ||
| + PWMV4_INT_HPC_W(true)); | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, | ||
| + PWMV4_EN(true) | PWMV4_CLK_EN(true)); | ||
| + | ||
| + ret = clk_enable(pc->pwmf->core); | ||
| + if (ret) | ||
| + goto err_release; | ||
| + | ||
| + ret = clk_rate_exclusive_get(pc->pwmf->core); | ||
| + if (ret) | ||
| + goto err_disable_pwm_clk; | ||
| + | ||
| + ret = mfpwm_acquire(pc->pwmf); | ||
| + if (ret) | ||
| + goto err_unprotect_pwm_clk; | ||
| + | ||
| + pc->is_enabled = true; | ||
| + } else { | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INT_EN, | ||
| + PWMV4_INT_LPC_W(false) | | ||
| + PWMV4_INT_HPC_W(false)); | ||
| + mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE, | ||
| + PWMV4_EN(false) | PWMV4_CLK_EN(false)); | ||
| + clk_rate_exclusive_put(pc->pwmf->core); | ||
| + clk_disable(pc->pwmf->core); | ||
| + pc->is_enabled = false; | ||
| + mfpwm_release(pc->pwmf); | ||
| + } | ||
| + | ||
| + mfpwm_release(pc->pwmf); | ||
| + } | ||
| + | ||
| + return 0; | ||
| + | ||
| +err_unprotect_pwm_clk: | ||
| + clk_rate_exclusive_put(pc->pwmf->core); | ||
| +err_disable_pwm_clk: | ||
| + clk_disable(pc->pwmf->core); | ||
| +err_release: | ||
| + mfpwm_release(pc->pwmf); | ||
| + | ||
| + return ret; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
clk_rate_exclusive_get clk_rate_exclusive_put Linux kernel sleepable
💡 Result:
Yes—both clk_rate_exclusive_get() and clk_rate_exclusive_put() are sleepable APIs and therefore must not be called from atomic context (e.g., hardirq/softirq, spinlock-held sections, or other in_atomic() paths).
- The kernel-doc for both functions explicitly states: “Calls to this function may sleep.” (and
..._put()also notes it returns no error status). [1] - Implementation detail: both functions take
clk_prepare_lock(), which usesmutex_trylock()/mutex_lock(&prepare_lock)—a mutex can sleep. [2]
Implication: only call them from a context where sleeping is allowed (process context, not IRQ/atomic).
Sources: [1] drivers/clk/clk.c kernel-doc for clk_rate_exclusive_{get,put}; [2] clk_prepare_lock() implementation showing mutex_lock.
🏁 Script executed:
# First, find and examine the full patch file
fd -t f "rk3576-0007-counter-rockchip-pwm-capture-driver.patch" --exec head -n 500 {}Repository: armbian/build
Length of output: 11253
🏁 Script executed:
# Look for the complete context around the rkpwmc_enable_write function
fd -t f "rk3576-0007-counter-rockchip-pwm-capture-driver.patch" --exec sed -n '150,330p' {}Repository: armbian/build
Length of output: 4624
🏁 Script executed:
# Check if there are any related files in the patch that might provide context
fd -t f "rk3576-0007-counter-rockchip-pwm-capture-driver.patch"Repository: armbian/build
Length of output: 147
Release the spinlock before calling clk_rate_exclusive_get() and reset hardware registers on all error paths.
The guard(spinlock)(&pc->enable_lock) at line 169 wraps the entire function body, including calls to clk_rate_exclusive_get() and clk_rate_exclusive_put() which are documented as sleepable and use mutex_lock() internally—this violates the requirement that spinlocks must not be held across sleepable operations and will trigger a kernel BUG at runtime.
Additionally, hardware registers PWMV4_REG_INT_EN and PWMV4_REG_ENABLE are programmed before clk_rate_exclusive_get() succeeds. If the clock operation fails, the hardware remains armed with interrupts and capture enabled while pc->is_enabled stays false, leaving the state inconsistent. The error path must reset these register bits before returning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch`
around lines 162 - 224, In rkpwmc_enable_write: don't hold
guard(spinlock)(&pc->enable_lock) across sleepable calls
(clk_rate_exclusive_get/put) — take the spinlock only to check/flip
pc->is_enabled and to protect fast register writes and mfpwm_acquire/release
references, then release it before calling clk_rate_exclusive_get/put; also
ensure error paths always reset the hardware registers (use mfpwm_reg_write to
clear PWMV4_REG_INT_EN and PWMV4_REG_ENABLE) before returning and only set
pc->is_enabled = true after all sleepable operations (clk_enable +
clk_rate_exclusive_get) succeed; adjust the error labels (err_unprotect_pwm_clk,
err_disable_pwm_clk, err_release) to clear registers and release mfpwm as needed
and to avoid holding the spinlock during sleepable calls.
patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch
Show resolved
Hide resolved
HackingGate
force-pushed
the
photonicat2
branch
from
2d869cc to
ec2c615
Compare
github-actions
bot
added
05
Contributor
🚫 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. |
HackingGate
force-pushed
the
photonicat2
branch
from
ec2c615 to
bac20d0
Compare
Contributor
🚫 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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/photonicat-pm.sh`:
- Around line 18-27: The metadata fetches bypass the mirror: ensure api_url and
changelog_url are also routed through the ghproxy when GITHUB_MIRROR=="ghproxy"
by prepending the same ghproxy_header used for photonicat_pm_url; update the
logic around api_url, changelog_url, latest_version and debian_version so that
when GITHUB_MIRROR is "ghproxy" you set ghproxy_header="https://ghfast.top/" and
prefix both api_url and changelog_url (in addition to photonicat_pm_url) with
that header before calling curl to resolve latest_version and debian_version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83e5cde2-1a04-4f41-b72b-46e11cacf184
📒 Files selected for processing (3)
config/boards/photonicat2.cscconfig/kernel/linux-rockchip64-current.configextensions/photonicat-pm.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- config/boards/photonicat2.csc
Comment on lines
+18
to
+27
| api_url="https://api.github.com/repos/HackingGate/photonicat-pm/releases/latest" | ||
| latest_version=$(curl -s "${api_url}" | jq -r '.tag_name') | ||
| # Get the Debian version from changelog | ||
| changelog_url="https://raw.githubusercontent.com/HackingGate/photonicat-pm/refs/tags/${latest_version}/debian/changelog" | ||
| debian_version=$(curl -s "${changelog_url}" | head -1 | grep -oP 'photonicat-pm \(\K[^)]+') | ||
| photonicat_pm_url="https://github.com/HackingGate/photonicat-pm/releases/download/${latest_version}/photonicat-pm-dkms_${debian_version}_all.deb" | ||
| if [[ "${GITHUB_MIRROR}" == "ghproxy" ]]; then | ||
| ghproxy_header="https://ghfast.top/" | ||
| photonicat_pm_url=${ghproxy_header}${photonicat_pm_url} | ||
| fi |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify the file exists and examine the code
cat -n extensions/photonicat-pm.sh | head -40Repository: armbian/build
Length of output: 2311
🏁 Script executed:
# Search for GITHUB_MIRROR usage patterns in the codebase
rg "GITHUB_MIRROR" -B 3 -A 3 | head -100Repository: armbian/build
Length of output: 6442
🏁 Script executed:
# Check if there are other similar patterns in the codebase where mirror is applied to API/raw URLs
rg "raw\.githubusercontent\.com|api\.github\.com" -B 2 -A 2 | head -80Repository: armbian/build
Length of output: 5862
🏁 Script executed:
# Check git-ref2info.sh to see how raw.githubusercontent.com is mirrored
sed -n '110,140p' lib/functions/general/git-ref2info.shRepository: armbian/build
Length of output: 1287
🏁 Script executed:
# Check if there are any issues or comments about this in the extensions
rg "api\.github\.com.*GITHUB_MIRROR|TODO|FIXME|BUG" extensions/ -B 2 -A 2Repository: armbian/build
Length of output: 9018
Mirror mode is bypassed during metadata resolution.
With GITHUB_MIRROR=ghproxy, only the release asset URL is mirrored. The API and raw.githubusercontent.com requests on lines 19 and 22 bypass the mirror entirely. In mirror-restricted environments (e.g., China with ghproxy), these metadata fetches will fail before the mirrored download even begins. This contradicts the mirroring pattern used elsewhere in the codebase (git-ref2info.sh, apt-utils.sh) where raw.githubusercontent.com URLs are explicitly wrapped when ghproxy is enabled.
🔧 Minimal fix
api_url="https://api.github.com/repos/HackingGate/photonicat-pm/releases/latest"
+ if [[ "${GITHUB_MIRROR}" == "ghproxy" ]]; then
+ ghproxy_header="https://ghfast.top/"
+ api_url="${ghproxy_header}${api_url}"
+ fi
latest_version=$(curl -s "${api_url}" | jq -r '.tag_name')
# Get the Debian version from changelog
changelog_url="https://raw.githubusercontent.com/HackingGate/photonicat-pm/refs/tags/${latest_version}/debian/changelog"
+ if [[ "${GITHUB_MIRROR}" == "ghproxy" ]]; then
+ changelog_url="${ghproxy_header}${changelog_url}"
+ fi
debian_version=$(curl -s "${changelog_url}" | head -1 | grep -oP 'photonicat-pm \(\K[^)]+')
photonicat_pm_url="https://github.com/HackingGate/photonicat-pm/releases/download/${latest_version}/photonicat-pm-dkms_${debian_version}_all.deb"
if [[ "${GITHUB_MIRROR}" == "ghproxy" ]]; then
- ghproxy_header="https://ghfast.top/"
- photonicat_pm_url=${ghproxy_header}${photonicat_pm_url}
+ photonicat_pm_url="${ghproxy_header}${photonicat_pm_url}"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| api_url="https://api.github.com/repos/HackingGate/photonicat-pm/releases/latest" | |
| latest_version=$(curl -s "${api_url}" | jq -r '.tag_name') | |
| # Get the Debian version from changelog | |
| changelog_url="https://raw.githubusercontent.com/HackingGate/photonicat-pm/refs/tags/${latest_version}/debian/changelog" | |
| debian_version=$(curl -s "${changelog_url}" | head -1 | grep -oP 'photonicat-pm \(\K[^)]+') | |
| photonicat_pm_url="https://github.com/HackingGate/photonicat-pm/releases/download/${latest_version}/photonicat-pm-dkms_${debian_version}_all.deb" | |
| if [[ "${GITHUB_MIRROR}" == "ghproxy" ]]; then | |
| ghproxy_header="https://ghfast.top/" | |
| photonicat_pm_url=${ghproxy_header}${photonicat_pm_url} | |
| fi | |
| api_url="https://api.github.com/repos/HackingGate/photonicat-pm/releases/latest" | |
| if [[ "${GITHUB_MIRROR}" == "ghproxy" ]]; then | |
| ghproxy_header="https://ghfast.top/" | |
| api_url="${ghproxy_header}${api_url}" | |
| fi | |
| latest_version=$(curl -s "${api_url}" | jq -r '.tag_name') | |
| # Get the Debian version from changelog | |
| changelog_url="https://raw.githubusercontent.com/HackingGate/photonicat-pm/refs/tags/${latest_version}/debian/changelog" | |
| if [[ "${GITHUB_MIRROR}" == "ghproxy" ]]; then | |
| changelog_url="${ghproxy_header}${changelog_url}" | |
| fi | |
| debian_version=$(curl -s "${changelog_url}" | head -1 | grep -oP 'photonicat-pm \(\K[^)]+') | |
| photonicat_pm_url="https://github.com/HackingGate/photonicat-pm/releases/download/${latest_version}/photonicat-pm-dkms_${debian_version}_all.deb" | |
| if [[ "${GITHUB_MIRROR}" == "ghproxy" ]]; then | |
| photonicat_pm_url="${ghproxy_header}${photonicat_pm_url}" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/photonicat-pm.sh` around lines 18 - 27, The metadata fetches
bypass the mirror: ensure api_url and changelog_url are also routed through the
ghproxy when GITHUB_MIRROR=="ghproxy" by prepending the same ghproxy_header used
for photonicat_pm_url; update the logic around api_url, changelog_url,
latest_version and debian_version so that when GITHUB_MIRROR is "ghproxy" you
set ghproxy_header="https://ghfast.top/" and prefix both api_url and
changelog_url (in addition to photonicat_pm_url) with that header before calling
curl to resolve latest_version and debian_version.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch (1)
38-39:⚠️ Potential issue | 🟠 MajorAlways-on enable path should also gate on memory-on state.
Line 38 only checks the power bit, but Lines 64-65 treat
mem_status_mask && !rockchip_pmu_domain_is_mem_on(pd)as off. This can mark a domain ALWAYS_ON without forcing a full bring-up when memory is still down.🔧 Minimal fix
- if (!rockchip_pmu_domain_is_on(pd)) { + if (!rockchip_pmu_domain_is_on(pd) || + (pd->info->mem_status_mask && + !rockchip_pmu_domain_is_mem_on(pd))) { error = rockchip_pd_power(pd, true); if (error) {Also applies to: 64-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch` around lines 38 - 39, The ALWAYS_ON enable path only checked rockchip_pmu_domain_is_on(pd) but must also ensure memory is up; change the condition that calls rockchip_pd_power(pd, true) to trigger when the domain is not powered OR when mem_status_mask is set and rockchip_pmu_domain_is_mem_on(pd) is false (i.e. if (!rockchip_pmu_domain_is_on(pd) || (mem_status_mask && !rockchip_pmu_domain_is_mem_on(pd))) call rockchip_pd_power(pd, true)); make the same update for the second check around lines 64-65 so both the initial and duplicate paths gate on mem_status_mask/rockchip_pmu_domain_is_mem_on as well.patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch (6)
411-418:⚠️ Potential issue | 🟠 MajorMove mux teardown out of
state_lock.
clk_unprepare()andclk_unregister_mux()are sleepable CCF teardown paths. Calling them while Lines 411-418 hold a spinlock will trip a sleeping-in-atomic bug on remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 411 - 418, The mux teardown (clk_unprepare/clk_unregister_mux) must not run while holding the spinlock; modify the remove/teardown path in the code that uses mfpwm->state_lock so it instead captures the pointer to mfpwm->chosen_clk under the lock (e.g., store it in a local variable), releases the spinlock (spin_unlock_irqrestore(&mfpwm->state_lock, flags)) and only then calls clk_unprepare(local_clk) and clk_unregister_mux(local_clk); ensure the state keyed by mfpwm->chosen_clk is still valid or cleared while holding state_lock before calling the sleepable CCF teardown functions.
214-234:⚠️ Potential issue | 🟠 MajorDon't turn transient lock contention into a functional busy error.
Lines 227-228 return
-EBUSYwheneverstate_lockis momentarily busy. That breaks the re-entrant contract documented in the header and can make the child drivers' same-handle reacquires fail spuriously even when no other function owns the block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 214 - 234, The current mfpwm_acquire uses spin_trylock_irqsave and returns -EBUSY on transient contention, which violates the re-entrant contract; change the locking to acquire the lock unconditionally (use spin_lock_irqsave on mfpwm->state_lock instead of spin_trylock_irqsave) so callers block until the lock is held, then call mfpwm_do_acquire and finally release with spin_unlock_irqrestore, keeping the existing flags and return handling in mfpwm_acquire.
168-185:⚠️ Potential issue | 🔴 CriticalValidate
pwmfbefore derivingdev.Line 171 dereferences
pwmf->parentbefore either pointer has been checked, so the validation path itself can oops on NULL/ERR handles instead of returning-EINVAL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 168 - 185, The helper mfpwm_check_pwmf currently dereferences pwmf->parent to get dev before validating pwmf and pwmf->parent; move the derivation of struct device *dev = &pwmf->parent->pdev->dev so it occurs after the IS_ERR_OR_NULL(pwmf) and IS_ERR_OR_NULL(pwmf->parent) checks (or alternatively check pwmf and pwmf->parent first and only then build dev), and also ensure you validate pwmf->parent->pdev if necessary before using pdev->dev; update the mfpwm_check_pwmf function to perform pointer checks first and only call dev_warn with a valid dev pointer after those checks succeed.
189-211:⚠️ Potential issue | 🟠 MajorRollback ownership if
clk_enable(pclk)fails.Lines 197-211 publish
active_funcand incrementacquire_cntbeforeclk_enable()is known to succeed. A failing acquire then leaves the channel logically owned even though the caller got an error, so later acquires can be blocked by stale state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 189 - 211, mfpwm_do_acquire currently sets mfpwm->active_func and increments mfpwm->acquire_cnt before calling clk_enable(mfpwm->pclk); if clk_enable fails you must roll back that state so the channel isn't left logically owned. Modify mfpwm_do_acquire so that on clk_enable() failure you decrement mfpwm->acquire_cnt back to the previous value (using the local cnt or decrementing) and, if that makes the count zero, clear mfpwm->active_func; then return the clk_enable error. Alternatively, move clk_enable before mutating active_func/acquire_cnt and only set them after clk_enable succeeds—ensure you reference mfpwm_do_acquire, mfpwm->active_func, mfpwm->acquire_cnt, clk_enable(mfpwm->pclk) and preserve the overflow check logic.
381-400:⚠️ Potential issue | 🔴 CriticalCheck
clk_register_mux()beforeclk_prepare()and unwind failed probe state.Line 381 can return an
ERR_PTR(), so Line 387 may hand an error pointer toclk_prepare(). Also, if subdevice registration fails on Lines 396-400, the prepared mux is leaked because this probe returns beforeremove()can unprepare/unregister it.
295-297:⚠️ Potential issue | 🔴 CriticalUse NULL checks for both
devm_kzalloc()results.
devm_kzalloc()returnsNULL, not an error pointer. TheIS_ERR()/PTR_ERR()tests on Lines 295-297 and 337-339 will miss OOM, and the next field writes dereference NULL.Also applies to: 337-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 295 - 297, The devm_kzalloc() call that allocates 'func' incorrectly uses IS_ERR()/PTR_ERR() — devm_kzalloc() returns NULL on OOM, so change the check to "if (!func) return -ENOMEM;" (and similarly update the other devm_kzalloc() call in this patch that currently uses IS_ERR()/PTR_ERR()); locate the allocations using devm_kzalloc(&mfpwm->pdev->dev, ...) and replace the IS_ERR()/PTR_ERR() handling with NULL checks returning -ENOMEM to avoid dereferencing NULL.patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch (2)
308-315:⚠️ Potential issue | 🟠 MajorReturn
IRQ_HANDLEDonce this handler clears LPC/HPC.If
clr != 0, this driver already serviced the shared interrupt. ReturningIRQ_NONEjust because unrelated bits remain set can make the IRQ core treat a handled interrupt as spurious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch` around lines 308 - 315, The handler currently returns IRQ_NONE when other interrupt bits remain set (intsts != clr) even if this driver cleared its bits (clr != 0); change the conditional so that if the driver cleared any bits (clr != 0) it returns IRQ_HANDLED, otherwise return IRQ_NONE for unrelated interrupts. Concretely, update the check around intsts/clr (used with mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INTSTS, clr)) to return clr ? IRQ_HANDLED : IRQ_NONE when intsts != clr, ensuring the handler returns IRQ_HANDLED whenever it serviced LPC/HPC.
169-224:⚠️ Potential issue | 🔴 CriticalDrop
enable_lockbefore the sleepable clock calls and always reset the hardware on failure.Line 169 keeps
enable_lockheld acrossclk_rate_exclusive_get()/put(), which may sleep. The same error path still exits withPWMV4_REG_INT_EN/PWMV4_REG_ENABLEprogrammed, so a failed enable leaves capture armed in hardware whilepc->is_enabledstays false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch` around lines 169 - 224, The enable path currently holds enable_lock across sleeping calls clk_rate_exclusive_get()/put() and leaves hardware interrupts/enable programmed on failure; release enable_lock (drop guard(spinlock)(&pc->enable_lock)) before calling clk_rate_exclusive_get() (and any other potentially sleeping clk_* calls) and reacquire only if necessary, and ensure every error path always resets hardware by writing PWMV4_REG_INT_EN with LPC/HPC false and PWMV4_REG_ENABLE with PWMV4_EN(false)|PWMV4_CLK_EN(false), clear pc->is_enabled on failure, and properly call mfpwm_release()/clk_disable()/clk_rate_exclusive_put() in the err_* paths so the device is never left armed while pc->is_enabled remains false (reference functions/fields: enable_lock, pc->is_enabled, mfpwm_acquire()/mfpwm_release(), clk_enable()/clk_disable(), clk_rate_exclusive_get()/clk_rate_exclusive_put(), PWMV4_REG_INT_EN, PWMV4_REG_ENABLE).patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch (1)
291-335:⚠️ Potential issue | 🟠 MajorClear the latched enable state on every post-programming failure.
After Line 291 the waveform is already armed in hardware. If
clk_enable(),clk_rate_exclusive_get(), or the secondmfpwm_acquire()fail, this path returns withPWMV4_REG_ENABLEstill committed; the last case also leaks the core-clock protection. Keep the TRM-mandated write order if needed, but explicitly clearPWMV4_REG_ENABLEand unwind the clock state before the final release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch` around lines 291 - 335, The code may return after committing PWMV4_REG_ENABLE while failing clk_enable/clk_rate_exclusive_get/mfpwm_acquire, leaving the hardware latched on and core-clock state leaked; fix by, on any error path after the final mfpwm_reg_write(..., PWMV4_REG_ENABLE, PWMV4_CTRL_UPDATE_EN), explicitly clear the enable latch (write 0 to PWMV4_REG_ENABLE via mfpwm_reg_write) and unwind clock state (call clk_rate_exclusive_put and clk_disable if they were successfully taken) before calling mfpwm_release; update the error-handling around clk_enable, clk_rate_exclusive_get and mfpwm_acquire to track which resources were acquired and release them in reverse order (use the same symbols: PWMV4_REG_ENABLE, mfpwm_reg_write, clk_enable, clk_rate_exclusive_get, clk_rate_exclusive_put, clk_disable, mfpwm_acquire, mfpwm_release).
🧹 Nitpick comments (4)
patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch (1)
31-31: Renamerockchip_pd_add_alwasy_on_flagto fix typo and improve readability.The helper name has a typo (
alwasy), which makes grep/discovery harder.✏️ Suggested rename
-static int rockchip_pd_add_alwasy_on_flag(struct rockchip_pm_domain *pd) +static int rockchip_pd_add_always_on_flag(struct rockchip_pm_domain *pd) @@ - error = rockchip_pd_add_alwasy_on_flag(pd); + error = rockchip_pd_add_always_on_flag(pd);Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch` at line 31, The function name rockchip_pd_add_alwasy_on_flag has a typo; rename it to rockchip_pd_add_always_on_flag and update every declaration and call site (including the other occurrence of rockchip_pd_add_alwasy_on_flag) to the corrected identifier so symbol lookups and grep find the helper; ensure any forward declarations, static definitions, and references within this patch are changed consistently to the new name.patch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dts (1)
1021-1025: Minor: Extra blank line.There's a double blank line at line 1022 before
&vop. This is a minor formatting inconsistency.Suggested fix
}; - &vop { status = "okay"; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dts` around lines 1021 - 1025, Remove the stray extra blank line immediately before the &vop node so the fragment `&vop { status = "okay"; };` is not separated by a double blank line; locate the `&vop` node in the rk3576-photonicat2.dts diff and delete the redundant empty line to restore consistent formatting.patch/u-boot/v2026.01/board_photonicat2/add-photonicat2-dts.patch (1)
892-904: Different eMMC max-frequency between U-Boot and kernel.The
sdhcinode usesmax-frequency = <150000000>(150MHz) here, but the kernel DTS uses<100000000>(100MHz). If this is intentional (faster boot in U-Boot, conservative in kernel for stability), this is fine. Otherwise, consider aligning them to avoid potential compatibility issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.01/board_photonicat2/add-photonicat2-dts.patch` around lines 892 - 904, The sdhci node sets max-frequency = <150000000> which differs from the kernel DTS value of <100000000>, causing a mismatch between U-Boot and kernel; update the &sdhci node (the max-frequency property in the sdhci device tree fragment) to match the kernel value (or add a clear comment documenting the intentional discrepancy and reason), ensuring the property in the sdhci node is changed from 150000000 to 100000000 if you want them aligned and verify no other SDHCI properties (bus-width, mmc-hs400-1_8v, etc.) require adjustment.config/kernel/linux-rockchip64-current.config (1)
884-885: Clarify whether Photonicat2 or RK3576 requires early-boot STMMAC networking.Lines 884–885 switch
CONFIG_STMMAC_ETHandCONFIG_STMMAC_PLATFORMfrom module (as inrockchip64-edge.config) to built-in for the entirerockchip64-currentfamily. The Photonicat2 board config contains no early-boot networking requirements, and other Armbian boards successfully load stmmac as a module. If this board does not require early NIC availability, keeping these as modules avoids unnecessary family-wide kernel package changes (per kernel config scope guidelines).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kernel/linux-rockchip64-current.config` around lines 884 - 885, The kernel now builds CONFIG_STMMAC_ETH and CONFIG_STMMAC_PLATFORM into the image for the whole rockchip64-current family; confirm whether Photonicat2 or the RK3576 SoC actually needs STMMAC available in early boot (e.g., for initramfs/early network or bootloader handoff). If not required, change these options back to module (CONFIG_STMMAC_ETH=m and CONFIG_STMMAC_PLATFORM=m) to avoid forcing a family-wide built-in; if early-boot NIC is required, leave them built-in and add a brief comment in the config noting which board/SoC (Photonicat2 / RK3576) necessitates early STMMAC. Ensure you inspect Photonicat2 board config and RK3576 DT/boot docs to decide.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch`:
- Around line 49-54: Update the MAINTAINERS hunk to point to the actual added
files so get_maintainer.pl picks them up: replace the incorrect F: entries
referencing drivers/soc/rockchip/mfpwm.c and include/soc/rockchip/mfpwm.h with
the real paths drivers/mfd/rockchip-mfpwm.c and
include/linux/mfd/rockchip-mfpwm.h (the files added by this series), ensuring
the MAINTAINERS entry that includes linux-rockchip@... and linux-pwm@...
references the correct file paths.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch`:
- Around line 168-179: The helper rockchip_pwm_v4_round_params can underflow
when *out_duty > *out_period; after calling rockchip_pwm_v4_round_single for
period and duty, clamp *out_duty to at most *out_period (e.g., set *out_duty =
min(*out_duty, *out_period)) before computing or comparing (*out_period -
*out_duty), then round/clip offset using the adjusted duty so the offset clamp
cannot underflow; update references in rockchip_pwm_v4_round_params accordingly.
- Around line 382-408: When handling the "PWM already on" path in the probe,
ensure you unwind the manual resources (mfpwm_acquire/mfpwm_release and clock
refs) on any failure: if clk_enable(pwmf->core) fails, call mfpwm_release(pwmf)
before returning; if clk_rate_exclusive_get(...) fails, call
clk_disable(pwmf->core) and mfpwm_release(pwmf) (in addition to the existing
disable); and if devm_pwmchip_add(dev, chip) fails after acquiring and
protecting the clock, call clk_rate_exclusive_put(pc->pwmf->core),
clk_disable(pwmf->core) and mfpwm_release(pwmf) before returning; keep the
existing -EBUSY path untouched so you only release when mfpwm_acquire succeeded.
In `@patch/u-boot/v2026.01/board_photonicat2/add-photonicat2-dts.patch`:
- Around line 133-144: The vcc_3v3_pcie0 regulator node currently sets
vin-supply to &vcc5v0_otg; change that to &vcc5v0_sys_s5 so the PCIe regulator
uses the system 5V supply consistent with the kernel DTS. Edit the vcc_3v3_pcie0
node (regulator-vcc-3v3-pcie0) and replace vin-supply = <&vcc5v0_otg> with
vin-supply = <&vcc5v0_sys_s5>, keeping the rest of the properties (pinctrl-0,
gpios, startup-delay-us, etc.) unchanged.
---
Duplicate comments:
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch`:
- Around line 38-39: The ALWAYS_ON enable path only checked
rockchip_pmu_domain_is_on(pd) but must also ensure memory is up; change the
condition that calls rockchip_pd_power(pd, true) to trigger when the domain is
not powered OR when mem_status_mask is set and rockchip_pmu_domain_is_mem_on(pd)
is false (i.e. if (!rockchip_pmu_domain_is_on(pd) || (mem_status_mask &&
!rockchip_pmu_domain_is_mem_on(pd))) call rockchip_pd_power(pd, true)); make the
same update for the second check around lines 64-65 so both the initial and
duplicate paths gate on mem_status_mask/rockchip_pmu_domain_is_mem_on as well.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch`:
- Around line 411-418: The mux teardown (clk_unprepare/clk_unregister_mux) must
not run while holding the spinlock; modify the remove/teardown path in the code
that uses mfpwm->state_lock so it instead captures the pointer to
mfpwm->chosen_clk under the lock (e.g., store it in a local variable), releases
the spinlock (spin_unlock_irqrestore(&mfpwm->state_lock, flags)) and only then
calls clk_unprepare(local_clk) and clk_unregister_mux(local_clk); ensure the
state keyed by mfpwm->chosen_clk is still valid or cleared while holding
state_lock before calling the sleepable CCF teardown functions.
- Around line 214-234: The current mfpwm_acquire uses spin_trylock_irqsave and
returns -EBUSY on transient contention, which violates the re-entrant contract;
change the locking to acquire the lock unconditionally (use spin_lock_irqsave on
mfpwm->state_lock instead of spin_trylock_irqsave) so callers block until the
lock is held, then call mfpwm_do_acquire and finally release with
spin_unlock_irqrestore, keeping the existing flags and return handling in
mfpwm_acquire.
- Around line 168-185: The helper mfpwm_check_pwmf currently dereferences
pwmf->parent to get dev before validating pwmf and pwmf->parent; move the
derivation of struct device *dev = &pwmf->parent->pdev->dev so it occurs after
the IS_ERR_OR_NULL(pwmf) and IS_ERR_OR_NULL(pwmf->parent) checks (or
alternatively check pwmf and pwmf->parent first and only then build dev), and
also ensure you validate pwmf->parent->pdev if necessary before using pdev->dev;
update the mfpwm_check_pwmf function to perform pointer checks first and only
call dev_warn with a valid dev pointer after those checks succeed.
- Around line 189-211: mfpwm_do_acquire currently sets mfpwm->active_func and
increments mfpwm->acquire_cnt before calling clk_enable(mfpwm->pclk); if
clk_enable fails you must roll back that state so the channel isn't left
logically owned. Modify mfpwm_do_acquire so that on clk_enable() failure you
decrement mfpwm->acquire_cnt back to the previous value (using the local cnt or
decrementing) and, if that makes the count zero, clear mfpwm->active_func; then
return the clk_enable error. Alternatively, move clk_enable before mutating
active_func/acquire_cnt and only set them after clk_enable succeeds—ensure you
reference mfpwm_do_acquire, mfpwm->active_func, mfpwm->acquire_cnt,
clk_enable(mfpwm->pclk) and preserve the overflow check logic.
- Around line 295-297: The devm_kzalloc() call that allocates 'func' incorrectly
uses IS_ERR()/PTR_ERR() — devm_kzalloc() returns NULL on OOM, so change the
check to "if (!func) return -ENOMEM;" (and similarly update the other
devm_kzalloc() call in this patch that currently uses IS_ERR()/PTR_ERR());
locate the allocations using devm_kzalloc(&mfpwm->pdev->dev, ...) and replace
the IS_ERR()/PTR_ERR() handling with NULL checks returning -ENOMEM to avoid
dereferencing NULL.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch`:
- Around line 291-335: The code may return after committing PWMV4_REG_ENABLE
while failing clk_enable/clk_rate_exclusive_get/mfpwm_acquire, leaving the
hardware latched on and core-clock state leaked; fix by, on any error path after
the final mfpwm_reg_write(..., PWMV4_REG_ENABLE, PWMV4_CTRL_UPDATE_EN),
explicitly clear the enable latch (write 0 to PWMV4_REG_ENABLE via
mfpwm_reg_write) and unwind clock state (call clk_rate_exclusive_put and
clk_disable if they were successfully taken) before calling mfpwm_release;
update the error-handling around clk_enable, clk_rate_exclusive_get and
mfpwm_acquire to track which resources were acquired and release them in reverse
order (use the same symbols: PWMV4_REG_ENABLE, mfpwm_reg_write, clk_enable,
clk_rate_exclusive_get, clk_rate_exclusive_put, clk_disable, mfpwm_acquire,
mfpwm_release).
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch`:
- Around line 308-315: The handler currently returns IRQ_NONE when other
interrupt bits remain set (intsts != clr) even if this driver cleared its bits
(clr != 0); change the conditional so that if the driver cleared any bits (clr
!= 0) it returns IRQ_HANDLED, otherwise return IRQ_NONE for unrelated
interrupts. Concretely, update the check around intsts/clr (used with
mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INTSTS, clr)) to return clr ?
IRQ_HANDLED : IRQ_NONE when intsts != clr, ensuring the handler returns
IRQ_HANDLED whenever it serviced LPC/HPC.
- Around line 169-224: The enable path currently holds enable_lock across
sleeping calls clk_rate_exclusive_get()/put() and leaves hardware
interrupts/enable programmed on failure; release enable_lock (drop
guard(spinlock)(&pc->enable_lock)) before calling clk_rate_exclusive_get() (and
any other potentially sleeping clk_* calls) and reacquire only if necessary, and
ensure every error path always resets hardware by writing PWMV4_REG_INT_EN with
LPC/HPC false and PWMV4_REG_ENABLE with PWMV4_EN(false)|PWMV4_CLK_EN(false),
clear pc->is_enabled on failure, and properly call
mfpwm_release()/clk_disable()/clk_rate_exclusive_put() in the err_* paths so the
device is never left armed while pc->is_enabled remains false (reference
functions/fields: enable_lock, pc->is_enabled, mfpwm_acquire()/mfpwm_release(),
clk_enable()/clk_disable(), clk_rate_exclusive_get()/clk_rate_exclusive_put(),
PWMV4_REG_INT_EN, PWMV4_REG_ENABLE).
---
Nitpick comments:
In `@config/kernel/linux-rockchip64-current.config`:
- Around line 884-885: The kernel now builds CONFIG_STMMAC_ETH and
CONFIG_STMMAC_PLATFORM into the image for the whole rockchip64-current family;
confirm whether Photonicat2 or the RK3576 SoC actually needs STMMAC available in
early boot (e.g., for initramfs/early network or bootloader handoff). If not
required, change these options back to module (CONFIG_STMMAC_ETH=m and
CONFIG_STMMAC_PLATFORM=m) to avoid forcing a family-wide built-in; if early-boot
NIC is required, leave them built-in and add a brief comment in the config
noting which board/SoC (Photonicat2 / RK3576) necessitates early STMMAC. Ensure
you inspect Photonicat2 board config and RK3576 DT/boot docs to decide.
In `@patch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dts`:
- Around line 1021-1025: Remove the stray extra blank line immediately before
the &vop node so the fragment `&vop { status = "okay"; };` is not separated by a
double blank line; locate the `&vop` node in the rk3576-photonicat2.dts diff and
delete the redundant empty line to restore consistent formatting.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch`:
- Line 31: The function name rockchip_pd_add_alwasy_on_flag has a typo; rename
it to rockchip_pd_add_always_on_flag and update every declaration and call site
(including the other occurrence of rockchip_pd_add_alwasy_on_flag) to the
corrected identifier so symbol lookups and grep find the helper; ensure any
forward declarations, static definitions, and references within this patch are
changed consistently to the new name.
In `@patch/u-boot/v2026.01/board_photonicat2/add-photonicat2-dts.patch`:
- Around line 892-904: The sdhci node sets max-frequency = <150000000> which
differs from the kernel DTS value of <100000000>, causing a mismatch between
U-Boot and kernel; update the &sdhci node (the max-frequency property in the
sdhci device tree fragment) to match the kernel value (or add a clear comment
documenting the intentional discrepancy and reason), ensuring the property in
the sdhci node is changed from 150000000 to 100000000 if you want them aligned
and verify no other SDHCI properties (bus-width, mmc-hs400-1_8v, etc.) require
adjustment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9da7f8d0-5f97-46b8-818a-476d4bc0883c
📒 Files selected for processing (16)
config/boards/photonicat2.cscconfig/kernel/linux-rockchip64-current.configextensions/photonicat-pm.shpatch/kernel/archive/rockchip64-6.18/board-photonicat2.patchpatch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dtspatch/kernel/archive/rockchip64-6.18/rk3576-0001-gpio-rockchip-set-input-direction-when-request-irq.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0004-dt-bindings-pwm-rockchip-rk3576-pwm.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0008-arm64-dts-rk3576-add-pwm-nodes.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0009-arm64-dts-rk3576-add-dma-coherent-pcie-gmac.patchpatch/u-boot/v2026.01/board_photonicat2/add-photonicat2-dts.patchpatch/u-boot/v2026.01/defconfig/photonicat2-rk3576_defconfig
🚧 Files skipped from review as they are similar to previous changes (5)
- extensions/photonicat-pm.sh
- patch/kernel/archive/rockchip64-6.18/rk3576-0009-arm64-dts-rk3576-add-dma-coherent-pcie-gmac.patch
- patch/kernel/archive/rockchip64-6.18/rk3576-0001-gpio-rockchip-set-input-direction-when-request-irq.patch
- config/boards/photonicat2.csc
- patch/kernel/archive/rockchip64-6.18/rk3576-0004-dt-bindings-pwm-rockchip-rk3576-pwm.patch
Comment on lines
+49
to
+54
| @@ -22278,6 +22278,8 @@ L: linux-rockchip@lists.infradead.org | ||
| L: linux-pwm@vger.kernel.org | ||
| S: Maintained | ||
| F: Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml | ||
| +F: drivers/soc/rockchip/mfpwm.c | ||
| +F: include/soc/rockchip/mfpwm.h |
Contributor
There was a problem hiding this comment.
Fix the MAINTAINERS paths in this series.
These F: entries point at drivers/soc/... and include/soc/..., but this patch actually adds drivers/mfd/rockchip-mfpwm.c and include/linux/mfd/rockchip-mfpwm.h. As written, get_maintainer.pl will miss the new parent driver/header, and the same stale paths are then carried forward by the later PWM/counter MAINTAINERS hunks.
Suggested patch
- F: drivers/soc/rockchip/mfpwm.c
- F: include/soc/rockchip/mfpwm.h
+ F: drivers/mfd/rockchip-mfpwm.c
+ F: include/linux/mfd/rockchip-mfpwm.h🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch`
around lines 49 - 54, Update the MAINTAINERS hunk to point to the actual added
files so get_maintainer.pl picks them up: replace the incorrect F: entries
referencing drivers/soc/rockchip/mfpwm.c and include/soc/rockchip/mfpwm.h with
the real paths drivers/mfd/rockchip-mfpwm.c and
include/linux/mfd/rockchip-mfpwm.h (the files added by this series), ensuring
the MAINTAINERS entry that includes linux-rockchip@... and linux-pwm@...
references the correct file paths.
Comment on lines
+168
to
+179
| +static void rockchip_pwm_v4_round_params(unsigned long rate, u64 duty, | ||
| + u64 period, u64 offset, u32 *out_duty, | ||
| + u32 *out_period, u32 *out_offset) | ||
| +{ | ||
| + *out_period = rockchip_pwm_v4_round_single(rate, period); | ||
| + | ||
| + *out_duty = rockchip_pwm_v4_round_single(rate, duty); | ||
| + | ||
| + /* As per TRM, PWM_OFFSET: "The value ranges from 0 to (period-duty)" */ | ||
| + *out_offset = rockchip_pwm_v4_round_single(rate, offset); | ||
| + if (*out_offset > (*out_period - *out_duty)) | ||
| + *out_offset = *out_period - *out_duty; |
Contributor
There was a problem hiding this comment.
Clamp duty before computing period - duty.
Line 178 assumes the independently rounded values still satisfy duty <= period. For saturated or otherwise out-of-range inputs, *out_period - *out_duty underflows and turns the offset clamp into a huge positive range, so this helper can emit an out-of-spec offset instead of clamping it.
Suggested patch
*out_period = rockchip_pwm_v4_round_single(rate, period);
*out_duty = rockchip_pwm_v4_round_single(rate, duty);
+ if (*out_duty > *out_period)
+ *out_duty = *out_period;
/* As per TRM, PWM_OFFSET: "The value ranges from 0 to (period-duty)" */
*out_offset = rockchip_pwm_v4_round_single(rate, offset);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch`
around lines 168 - 179, The helper rockchip_pwm_v4_round_params can underflow
when *out_duty > *out_period; after calling rockchip_pwm_v4_round_single for
period and duty, clamp *out_duty to at most *out_period (e.g., set *out_duty =
min(*out_duty, *out_period)) before computing or comparing (*out_period -
*out_duty), then round/clip offset using the adjusted duty so the offset clamp
cannot underflow; update references in rockchip_pwm_v4_round_params accordingly.
Comment on lines
+382
to
+408
| + ret = mfpwm_acquire(pwmf); | ||
| + if (ret == -EBUSY) | ||
| + dev_warn(dev, "PWM hardware already in use, can't check initial state\n"); | ||
| + else if (ret < 0) | ||
| + return dev_err_probe(dev, ret, "Couldn't acquire mfpwm in probe\n"); | ||
| + | ||
| + if (!rockchip_pwm_v4_on_and_continuous(pc)) | ||
| + mfpwm_release(pwmf); | ||
| + else { | ||
| + dev_dbg(dev, "PWM was already on at probe time\n"); | ||
| + ret = clk_enable(pwmf->core); | ||
| + if (ret) | ||
| + return dev_err_probe(dev, ret, "Enabling pwm clock failed\n"); | ||
| + ret = clk_rate_exclusive_get(pc->pwmf->core); | ||
| + if (ret) { | ||
| + clk_disable(pwmf->core); | ||
| + return dev_err_probe(dev, ret, "Protecting pwm clock failed\n"); | ||
| + } | ||
| + } | ||
| + | ||
| + platform_set_drvdata(pdev, chip); | ||
| + | ||
| + chip->ops = &rockchip_pwm_v4_ops; | ||
| + | ||
| + ret = devm_pwmchip_add(dev, chip); | ||
| + if (ret) | ||
| + return dev_err_probe(dev, ret, "Failed to add PWM chip\n"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*rk3576-0006-pwm-rockchip-pwmv4-driver.patch" -type fRepository: armbian/build
Length of output: 139
🏁 Script executed:
cat -n ./patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch | head -450 | tail -100Repository: armbian/build
Length of output: 3617
🏁 Script executed:
cat -n ./patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch | sed -n '358,411p'Repository: armbian/build
Length of output: 2000
🏁 Script executed:
cat ./patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch | grep -A 30 "\.remove\|\.unbind\|static.*remove\|static.*unbind"Repository: armbian/build
Length of output: 39
🏁 Script executed:
wc -l ./patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patchRepository: armbian/build
Length of output: 143
🏁 Script executed:
cat ./patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch | grep -i "remove\|unbind\|devm_add_action"Repository: armbian/build
Length of output: 39
🏁 Script executed:
cat ./patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patchRepository: armbian/build
Length of output: 14708
Add error handling to release resources on probe failures when PWM is already running.
When the PWM is detected as already running (rockchip_pwm_v4_on_and_continuous() returns true), the probe manually holds an mfpwm_acquire() along with enabled and protected clock references. If an error occurs during clk_enable(), clk_rate_exclusive_get(), or devm_pwmchip_add(), these resources leak since devm only manages the allocations, not the manual acquire/clock operations. Add a common error path that unwinds all three: clk_rate_exclusive_put(), clk_disable(), and mfpwm_release().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch`
around lines 382 - 408, When handling the "PWM already on" path in the probe,
ensure you unwind the manual resources (mfpwm_acquire/mfpwm_release and clock
refs) on any failure: if clk_enable(pwmf->core) fails, call mfpwm_release(pwmf)
before returning; if clk_rate_exclusive_get(...) fails, call
clk_disable(pwmf->core) and mfpwm_release(pwmf) (in addition to the existing
disable); and if devm_pwmchip_add(dev, chip) fails after acquiring and
protecting the clock, call clk_rate_exclusive_put(pc->pwmf->core),
clk_disable(pwmf->core) and mfpwm_release(pwmf) before returning; keep the
existing -EBUSY path untouched so you only release when mfpwm_acquire succeeded.
EvilOlaf
reviewed
Mar 17, 2026
Board configuration:
- config/boards/photonicat2.csc
Kernel config:
- config/kernel/linux-rockchip64-current.config
Device tree:
- patch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dts
RK3576 SoC patches (rockchip64-6.18):
0001 gpio: rockchip: set input direction when request irq
Author: Ye Zhang <ye.zhang@rock-chips.com>
Source: [PATCH v5 4/4] gpio: rockchip: Update the GPIO driver
https://lore.kernel.org/r/20241112015408.3139996-5-ye.zhang@rock-chips.com
Series: https://lore.kernel.org/r/20241112015408.3139996-1-ye.zhang@rock-chips.com
0002 mmc: dw_mmc-rockchip: add v2 tuning support
Author: Shawn Lin <shawn.lin@rock-chips.com>
Posted-by: Detlev Casanova <detlev.casanova@collabora.com>
Source: [PATCH v3 2/3] Add dw_mmc support for rk3576
https://lore.kernel.org/r/20240814223555.3695-3-detlev.casanova@collabora.com
Series: https://lore.kernel.org/r/20240814223555.3695-1-detlev.casanova@collabora.com
0003 pmdomain: rockchip: Add always on configuration
Author: Finley Xiao <finley.xiao@rock-chips.com>
Source: Rockchip BSP (not posted to lore.kernel.org)
0004 dt-bindings: pwm: Add a new binding for rockchip,rk3576-pwm
0005 mfd: Add Rockchip mfpwm driver
0006 pwm: Add rockchip PWMv4 driver
0007 counter: Add rockchip-pwm-capture driver
0008 arm64: dts: rockchip: add PWM nodes to RK3576 SoC dtsi
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Source: [PATCH v3 0/5] Add Rockchip RK3576 PWM Support Through MFPWM
https://lore.kernel.org/r/20251027-rk3576-pwm-v3-0-654a5cb1e3f8@collabora.com
0009 arm64: dts: rockchip: add dma-coherent for pcie and gmac of RK3576
Author: Shawn Lin <shawn.lin@rock-chips.com>
Source: https://lore.kernel.org/r/1764313762-78063-1-git-send-email-shawn.lin@rock-chips.com
U-Boot:
- patch/u-boot/v2026.01/board_photonicat2/
BSP:
- extensions/photonicat-pm.sh
HackingGate
force-pushed
the
photonicat2
branch
from
bac20d0 to
16a1a54
Compare
Member
Author
No all, I have updated the PR to contain minimum required changes for Photonicat2. |
Contributor
🚫 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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (12)
patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch (7)
295-297:⚠️ Potential issue | 🔴 CriticalUse NULL checks for
devm_kzalloc()results.
devm_kzalloc()returnsNULLon allocation failure, notERR_PTR(). TheseIS_ERR()/PTR_ERR()checks let allocation failures fall through to immediate NULL dereferences.Also applies to: 337-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 295 - 297, The code incorrectly treats devm_kzalloc() as returning ERR_PTR: replace the IS_ERR/PTR_ERR checks with a NULL check. Specifically, in the allocation of func (devm_kzalloc(&mfpwm->pdev->dev, sizeof(*func), GFP_KERNEL)) and the similar allocation at the later location (lines around the second devm_kzalloc), change the logic to test "if (!func) return -ENOMEM;" (or appropriate error code) and avoid using IS_ERR/PTR_ERR; ensure subsequent code does not dereference func when NULL.
168-185:⚠️ Potential issue | 🔴 CriticalValidate the handle before dereferencing
pwmf->parent.The helper computes
devfrompwmf->parentbefore either pointer is checked, so the error path itself can NULL-deref on bad handles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 168 - 185, The function mfpwm_check_pwmf currently dereferences pwmf->parent to create struct device *dev before validating pwmf and pwmf->parent; move the derivation of dev (the line creating struct device *dev = &pwmf->parent->pdev->dev) to after the IS_ERR_OR_NULL(pwmf) and IS_ERR_OR_NULL(pwmf->parent) checks (or compute dev only when both are non-NULL), and update the dev_warn calls to use the safely-obtained dev; keep the function name mfpwm_check_pwmf and the same return values (-EINVAL/0).
214-234:⚠️ Potential issue | 🟠 MajorDon't turn transient lock contention into
-EBUSY.
spin_trylock_irqsave()makes same-function re-entrant acquires fail wheneverstate_lockis momentarily busy, which contradicts the contract documented in the public header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 214 - 234, The current mfpwm_acquire implementation uses spin_trylock_irqsave on mfpwm->state_lock which turns transient contention into -EBUSY and breaks the documented re-entrancy contract; change the locking to take the lock blocking (use spin_lock_irqsave) around the critical section in mfpwm_acquire (matching the existing spin_unlock_irqrestore call) so callers block until the lock is available, keeping the call to mfpwm_do_acquire, the dev_dbg line, and the error handling from mfpwm_check_pwmf unchanged.
411-416:⚠️ Potential issue | 🟠 MajorMove CCF teardown out of the spinlock.
clk_unprepare()andclk_unregister_mux()are sleepable clock-framework calls. Running them underspin_lock_irqsave()will hit a sleeping-in-atomic bug on remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 411 - 416, The spin_lock_irqsave(&mfpwm->state_lock, flags) currently encloses sleepable CCF calls clk_unprepare() and clk_unregister_mux(), causing sleeping-in-atomic; fix by only holding the lock to read and clear mfpwm->chosen_clk into a local pointer (and set mfpwm->chosen_clk = NULL under the lock), then call spin_unlock_irqrestore(&mfpwm->state_lock, flags) and perform clk_unprepare(local_clk) and clk_unregister_mux(local_clk) outside the spinlock; reference mfpwm->chosen_clk, state_lock, clk_unprepare, clk_unregister_mux and spin_lock_irqsave/irqrestore to locate and apply the change.
381-400:⚠️ Potential issue | 🔴 CriticalCheck
clk_register_mux()before preparing it, and unwind probe failures.
clk_register_mux()can return anERR_PTR, but the code passes it straight intoclk_prepare(). Also, if subdevice registration fails later, probe returns without unpreparing/unregistering the mux becauseremove()never runs for failed probes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 381 - 400, The probe must check the return of clk_register_mux() for IS_ERR before calling clk_prepare() and return PTR_ERR on failure, and if mfpwm_register_subdevs() fails the probe must unwind by calling clk_unprepare() (if prepare succeeded) and unregistering the mux (clk_unregister or the appropriate unregister function for clk_register_mux) and clearing driver data set by platform_set_drvdata(); update the block around clk_register_mux / clk_prepare / platform_set_drvdata / mfpwm_register_subdevs to: validate mfpwm->chosen_clk with IS_ERR and use PTR_ERR for logs/return, only call clk_prepare on a valid clk, and on any subsequent error call clk_unprepare and clk_unregister and clear platform_set_drvdata(NULL) before returning so resources are cleaned up on probe failure.
197-211:⚠️ Potential issue | 🟠 MajorRollback ownership state when
clk_enable()fails.
active_funcandacquire_cntare updated before the APB clock is enabled. Ifclk_enable(mfpwm->pclk)errors, the channel stays logically owned and later callers see a stale busy state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 197 - 211, The code sets mfpwm->active_func and increments mfpwm->acquire_cnt before calling clk_enable(mfpwm->pclk), so if clk_enable fails the channel remains marked busy; fix by rolling back those state changes on clk_enable failure: after calling clk_enable(mfpwm->pclk), if it returns an error clear mfpwm->active_func (set to NULL or previous value) and decrement mfpwm->acquire_cnt (undo the successful check_add_overflow increment), and log the failure (dev_warn) including pwmf->id and the error; alternatively move the clk_enable(mfpwm->pclk) call before assigning active_func/acquire_cnt and only update state after clk_enable succeeds — update references to mfpwm->active_func, mfpwm->acquire_cnt, clk_enable(mfpwm->pclk) and pwmf accordingly.
53-54:⚠️ Potential issue | 🟡 MinorPoint MAINTAINERS at the files this patch actually adds.
These paths still reference
drivers/soc/...andinclude/soc/..., so maintainer tooling will miss the new MFD core and header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch` around lines 53 - 54, Update the MAINTAINERS entry to point to the actual files added by this patch so maintainer tooling picks them up: add or replace references with the exact new filenames drivers/soc/rockchip/mfpwm.c and include/soc/rockchip/mfpwm.h (ensure the MAINTAINERS section that previously referenced different paths now lists these two files and the appropriate Rockchip MFD maintainer names).extensions/photonicat-pm.sh (1)
18-27:⚠️ Potential issue | 🟠 MajorMirror and validate the metadata lookups too.
api_urlandchangelog_urlstill bypassghproxy, and neitherlatest_versionnordebian_versionis checked before composing the asset URL. In mirrored or rate-limited environments this builds a broken download path and only fails later in chroot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/photonicat-pm.sh` around lines 18 - 27, The script builds release metadata and an asset URL but doesn't mirror API/raw requests or validate responses: ensure api_url and changelog_url are routed through the ghproxy mirror when GITHUB_MIRROR=="ghproxy" (reuse ghproxy_header before calling curl) and validate that latest_version and debian_version are non-empty after the curl/jq/grep steps (fail fast with a clear error if either is empty); also consider checking HTTP/curl exit status or response code for those requests so photonicat_pm_url is only composed when metadata lookups succeeded.patch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patch (2)
55-70:⚠️ Potential issue | 🟠 MajorAccept the first successful tuning phase immediately.
The loop only breaks after the 180° fallback succeeds. If
mmc_send_tuning()succeeds on the initial phase, the code keeps iterating and can still hit the"V2 All phases bad!"path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patch` around lines 55 - 70, The loop over ARRAY_SIZE(degrees) currently only breaks when the 180° fallback succeeds, so a successful mmc_send_tuning(mmc, opcode, NULL) on the initial phase is not accepted; change the logic inside the loop that calls clk_set_phase(priv->sample_clk, degree) and mmc_send_tuning() so that if mmc_send_tuning() returns 0 immediately after setting the phase, you accept that phase and break out of the loop (i.e., add a direct success path after mmc_send_tuning() before executing the 180° fallback), referencing the variables/functions degree, degrees[], priv->last_degree, clk_set_phase() and mmc_send_tuning().
22-23:⚠️ Potential issue | 🟠 MajorMake the v2 phase-inheritance state per host.
inheritis still function-localstatic, so the first DW-MMC instance that enters this path disables bootloader-phase inheritance for every later controller. This needs to live in the per-host private data instead.Also applies to: 38-45
patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch (2)
169-224:⚠️ Potential issue | 🔴 CriticalDon't hold
enable_lockacross sleepable clock ops, and clear hardware on failure.
rkpwmc_enable_write()keeps the spinlock held while callingclk_rate_exclusive_get()/put(), which may sleep. The error path also leavesINT_ENandENABLEprogrammed if enablement fails after the registers are armed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch` around lines 169 - 224, rkpwmc_enable_write() currently holds enable_lock across sleepable clock ops and can leave INT_EN/ENABLE programmed on failure; release the spinlock before any sleepable calls (clk_enable, clk_rate_exclusive_get/put) and reacquire only to flip pc->is_enabled, and ensure on any failure after programming registers you clear the hardware (write INT_EN false and ENABLE false) and undo any acquired resources (mfpwm_release, clk_rate_exclusive_put, clk_disable) so registers and pc->is_enabled are consistent; look for functions/fields named rkpwmc_enable_write, enable_lock, clk_enable/clk_disable, clk_rate_exclusive_get/put, mfpwm_acquire/mfpwm_release, mfpwm_reg_write (PWMV4_REG_INT_EN, PWMV4_REG_ENABLE) and pc->is_enabled to implement these changes.
308-315:⚠️ Potential issue | 🟠 MajorReport
IRQ_HANDLEDonce this handler clears its own bits.After clearing LPC/HPC status, the handler still returns
IRQ_NONEwhenever unrelated status bits remain set. That makes a shared IRQ look unhandled even though this driver did work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch` around lines 308 - 315, The handler must report IRQ_HANDLED when it cleared any of its own bits: after writing clr to PWMV4_REG_INTSTS (pc->pwmf->base) return IRQ_HANDLED if clr is non-zero, and only return IRQ_NONE when there were no bits cleared by this driver but other bits remain (i.e. intsts set and clr == 0). Concretely, change the current check using "if (intsts != clr) return IRQ_NONE;" to a logic that first checks "if (clr) return IRQ_HANDLED;" and otherwise "if (intsts) return IRQ_NONE;" so the handler reports handled whenever it cleared its own interrupt bits.
🧹 Nitpick comments (1)
config/kernel/linux-rockchip64-current.config (1)
884-885: Please confirm the family-wideSTMMAC_* =yswitch is intentional.
linux-rockchip64-current.configis shared by every rockchip64 current board, so this changes the GMAC stack from modular to built-in for the whole family. If Photonicat2 doesn't need Ethernet before modules load, keeping these as modules avoids a board-specific requirement becoming global.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/kernel/linux-rockchip64-current.config` around lines 884 - 885, The config enables the STMMAC family as built-in (CONFIG_STMMAC_ETH=y and CONFIG_STMMAC_PLATFORM=y) for all rockchip64 boards; confirm whether Photonicat2 requires GMAC before module init—if not, change these switches to module (CONFIG_STMMAC_ETH=m and CONFIG_STMMAC_PLATFORM=m) so the change isn't family-wide, or if Photonicat2 truly needs a built-in driver leave them as =y and document why; also verify no other board configs depend on a built-in STMMAC and, if only Photonicat2 needs built-in, move the =y settings into Photonicat2’s board-specific config instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/photonicat-pm.sh`:
- Line 29: Replace the wget invocation inside the chroot_sdcard call with curl;
specifically, in the line that currently calls chroot_sdcard "wget
${photonicat_pm_url} -P /tmp", use curl with location-following and
fail-on-error and write the output into /tmp using the basename of
${photonicat_pm_url} so the file lands in /tmp as before — update the invocation
inside chroot_sdcard and ensure ${photonicat_pm_url} is properly quoted.
In `@patch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dts`:
- Around line 838-845: The spi1 pinctrl node (spi1m2_1_pins) is missing the MISO
pin so when &spi1 overrides pinctrl-0 reads will fail; update the rockchip,pins
list under spi1m2_1_pins to include the missing MISO entry (the appropriate
RK_PCx entry for spi1_miso_m2) or replace the custom group with the full
upstream SPI pin group so that CS/CLK/MOSI/MISO are all muxed for the spi1
controller; locate the spi1 node and its spi1m2_1_pins binding to add the
correct MISO RK_PC... entry consistent with the other entries or switch to the
canonical upstream pin group.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch`:
- Around line 287-305: Assign pc->counter before registering the IRQ to avoid
the race where rkpwmc_irq_handler dereferences pc->counter while it's still
NULL: move the statement that sets pc->counter = counter to occur before the
devm_request_irq(...) call (the IRQ registration) so that rkpwmc_irq_handler,
which reads pc->counter and calls counter_push_event(...), always sees a valid
pointer; ensure the assignment happens immediately after pc is initialized and
before any calls to devm_request_irq.
---
Duplicate comments:
In `@extensions/photonicat-pm.sh`:
- Around line 18-27: The script builds release metadata and an asset URL but
doesn't mirror API/raw requests or validate responses: ensure api_url and
changelog_url are routed through the ghproxy mirror when
GITHUB_MIRROR=="ghproxy" (reuse ghproxy_header before calling curl) and validate
that latest_version and debian_version are non-empty after the curl/jq/grep
steps (fail fast with a clear error if either is empty); also consider checking
HTTP/curl exit status or response code for those requests so photonicat_pm_url
is only composed when metadata lookups succeeded.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patch`:
- Around line 55-70: The loop over ARRAY_SIZE(degrees) currently only breaks
when the 180° fallback succeeds, so a successful mmc_send_tuning(mmc, opcode,
NULL) on the initial phase is not accepted; change the logic inside the loop
that calls clk_set_phase(priv->sample_clk, degree) and mmc_send_tuning() so that
if mmc_send_tuning() returns 0 immediately after setting the phase, you accept
that phase and break out of the loop (i.e., add a direct success path after
mmc_send_tuning() before executing the 180° fallback), referencing the
variables/functions degree, degrees[], priv->last_degree, clk_set_phase() and
mmc_send_tuning().
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patch`:
- Around line 295-297: The code incorrectly treats devm_kzalloc() as returning
ERR_PTR: replace the IS_ERR/PTR_ERR checks with a NULL check. Specifically, in
the allocation of func (devm_kzalloc(&mfpwm->pdev->dev, sizeof(*func),
GFP_KERNEL)) and the similar allocation at the later location (lines around the
second devm_kzalloc), change the logic to test "if (!func) return -ENOMEM;" (or
appropriate error code) and avoid using IS_ERR/PTR_ERR; ensure subsequent code
does not dereference func when NULL.
- Around line 168-185: The function mfpwm_check_pwmf currently dereferences
pwmf->parent to create struct device *dev before validating pwmf and
pwmf->parent; move the derivation of dev (the line creating struct device *dev =
&pwmf->parent->pdev->dev) to after the IS_ERR_OR_NULL(pwmf) and
IS_ERR_OR_NULL(pwmf->parent) checks (or compute dev only when both are
non-NULL), and update the dev_warn calls to use the safely-obtained dev; keep
the function name mfpwm_check_pwmf and the same return values (-EINVAL/0).
- Around line 214-234: The current mfpwm_acquire implementation uses
spin_trylock_irqsave on mfpwm->state_lock which turns transient contention into
-EBUSY and breaks the documented re-entrancy contract; change the locking to
take the lock blocking (use spin_lock_irqsave) around the critical section in
mfpwm_acquire (matching the existing spin_unlock_irqrestore call) so callers
block until the lock is available, keeping the call to mfpwm_do_acquire, the
dev_dbg line, and the error handling from mfpwm_check_pwmf unchanged.
- Around line 411-416: The spin_lock_irqsave(&mfpwm->state_lock, flags)
currently encloses sleepable CCF calls clk_unprepare() and clk_unregister_mux(),
causing sleeping-in-atomic; fix by only holding the lock to read and clear
mfpwm->chosen_clk into a local pointer (and set mfpwm->chosen_clk = NULL under
the lock), then call spin_unlock_irqrestore(&mfpwm->state_lock, flags) and
perform clk_unprepare(local_clk) and clk_unregister_mux(local_clk) outside the
spinlock; reference mfpwm->chosen_clk, state_lock, clk_unprepare,
clk_unregister_mux and spin_lock_irqsave/irqrestore to locate and apply the
change.
- Around line 381-400: The probe must check the return of clk_register_mux() for
IS_ERR before calling clk_prepare() and return PTR_ERR on failure, and if
mfpwm_register_subdevs() fails the probe must unwind by calling clk_unprepare()
(if prepare succeeded) and unregistering the mux (clk_unregister or the
appropriate unregister function for clk_register_mux) and clearing driver data
set by platform_set_drvdata(); update the block around clk_register_mux /
clk_prepare / platform_set_drvdata / mfpwm_register_subdevs to: validate
mfpwm->chosen_clk with IS_ERR and use PTR_ERR for logs/return, only call
clk_prepare on a valid clk, and on any subsequent error call clk_unprepare and
clk_unregister and clear platform_set_drvdata(NULL) before returning so
resources are cleaned up on probe failure.
- Around line 197-211: The code sets mfpwm->active_func and increments
mfpwm->acquire_cnt before calling clk_enable(mfpwm->pclk), so if clk_enable
fails the channel remains marked busy; fix by rolling back those state changes
on clk_enable failure: after calling clk_enable(mfpwm->pclk), if it returns an
error clear mfpwm->active_func (set to NULL or previous value) and decrement
mfpwm->acquire_cnt (undo the successful check_add_overflow increment), and log
the failure (dev_warn) including pwmf->id and the error; alternatively move the
clk_enable(mfpwm->pclk) call before assigning active_func/acquire_cnt and only
update state after clk_enable succeeds — update references to
mfpwm->active_func, mfpwm->acquire_cnt, clk_enable(mfpwm->pclk) and pwmf
accordingly.
- Around line 53-54: Update the MAINTAINERS entry to point to the actual files
added by this patch so maintainer tooling picks them up: add or replace
references with the exact new filenames drivers/soc/rockchip/mfpwm.c and
include/soc/rockchip/mfpwm.h (ensure the MAINTAINERS section that previously
referenced different paths now lists these two files and the appropriate
Rockchip MFD maintainer names).
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch`:
- Around line 169-224: rkpwmc_enable_write() currently holds enable_lock across
sleepable clock ops and can leave INT_EN/ENABLE programmed on failure; release
the spinlock before any sleepable calls (clk_enable, clk_rate_exclusive_get/put)
and reacquire only to flip pc->is_enabled, and ensure on any failure after
programming registers you clear the hardware (write INT_EN false and ENABLE
false) and undo any acquired resources (mfpwm_release, clk_rate_exclusive_put,
clk_disable) so registers and pc->is_enabled are consistent; look for
functions/fields named rkpwmc_enable_write, enable_lock, clk_enable/clk_disable,
clk_rate_exclusive_get/put, mfpwm_acquire/mfpwm_release, mfpwm_reg_write
(PWMV4_REG_INT_EN, PWMV4_REG_ENABLE) and pc->is_enabled to implement these
changes.
- Around line 308-315: The handler must report IRQ_HANDLED when it cleared any
of its own bits: after writing clr to PWMV4_REG_INTSTS (pc->pwmf->base) return
IRQ_HANDLED if clr is non-zero, and only return IRQ_NONE when there were no bits
cleared by this driver but other bits remain (i.e. intsts set and clr == 0).
Concretely, change the current check using "if (intsts != clr) return IRQ_NONE;"
to a logic that first checks "if (clr) return IRQ_HANDLED;" and otherwise "if
(intsts) return IRQ_NONE;" so the handler reports handled whenever it cleared
its own interrupt bits.
---
Nitpick comments:
In `@config/kernel/linux-rockchip64-current.config`:
- Around line 884-885: The config enables the STMMAC family as built-in
(CONFIG_STMMAC_ETH=y and CONFIG_STMMAC_PLATFORM=y) for all rockchip64 boards;
confirm whether Photonicat2 requires GMAC before module init—if not, change
these switches to module (CONFIG_STMMAC_ETH=m and CONFIG_STMMAC_PLATFORM=m) so
the change isn't family-wide, or if Photonicat2 truly needs a built-in driver
leave them as =y and document why; also verify no other board configs depend on
a built-in STMMAC and, if only Photonicat2 needs built-in, move the =y settings
into Photonicat2’s board-specific config instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54b244d4-b8b5-4308-8340-b7ceb2e7bee5
📒 Files selected for processing (15)
config/boards/photonicat2.cscconfig/kernel/linux-rockchip64-current.configextensions/photonicat-pm.shpatch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dtspatch/kernel/archive/rockchip64-6.18/rk3576-0001-gpio-rockchip-set-input-direction-when-request-irq.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0002-mmc-dw_mmc-rockchip-add-v2-tuning-support.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0004-dt-bindings-pwm-rockchip-rk3576-pwm.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0005-mfd-rockchip-add-mfpwm-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0008-arm64-dts-rk3576-add-pwm-nodes.patchpatch/kernel/archive/rockchip64-6.18/rk3576-0009-arm64-dts-rk3576-add-dma-coherent-pcie-gmac.patchpatch/u-boot/v2026.01/board_photonicat2/add-photonicat2-dts.patchpatch/u-boot/v2026.01/defconfig/photonicat2-rk3576_defconfig
🚧 Files skipped from review as they are similar to previous changes (4)
- patch/kernel/archive/rockchip64-6.18/rk3576-0003-pmdomain-rockchip-add-always-on-support.patch
- patch/kernel/archive/rockchip64-6.18/rk3576-0006-pwm-rockchip-pwmv4-driver.patch
- patch/u-boot/v2026.01/board_photonicat2/add-photonicat2-dts.patch
- patch/kernel/archive/rockchip64-6.18/rk3576-0008-arm64-dts-rk3576-add-pwm-nodes.patch
| photonicat_pm_url=${ghproxy_header}${photonicat_pm_url} | ||
| fi | ||
| photonicat_pm_dkms_file_name=photonicat-pm-dkms_${debian_version}_all.deb | ||
| use_clean_environment="yes" chroot_sdcard "wget ${photonicat_pm_url} -P /tmp" |
Contributor
There was a problem hiding this comment.
Use curl here instead of wget.
Armbian extensions standardize on curl; introducing wget here makes this extension inconsistent with the rest of the framework. Based on learnings, in the Armbian build framework, wget should not be used. Use curl instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/photonicat-pm.sh` at line 29, Replace the wget invocation inside
the chroot_sdcard call with curl; specifically, in the line that currently calls
chroot_sdcard "wget ${photonicat_pm_url} -P /tmp", use curl with
location-following and fail-on-error and write the output into /tmp using the
basename of ${photonicat_pm_url} so the file lands in /tmp as before — update
the invocation inside chroot_sdcard and ensure ${photonicat_pm_url} is properly
quoted.
Comment on lines
+838
to
+845
| spi1 { | ||
| spi1m2_1_pins: spi1m2-1-pins { | ||
| rockchip,pins = | ||
| /* spi1_clk_m2 */ | ||
| <3 RK_PC7 10 &pcfg_pull_none>, | ||
| /* spi1_mosi_m2 */ | ||
| <3 RK_PC6 10 &pcfg_pull_none>; | ||
| }; |
Contributor
There was a problem hiding this comment.
spi1 pinctrl drops the MISO line.
Once &spi1 overrides pinctrl-0, only CS/CLK/MOSI are muxed here. Reads from the Authenta SPI device will fail unless the missing MISO pin is added or the full upstream SPI pin group is used.
Also applies to: 942-950
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patch/kernel/archive/rockchip64-6.18/dt/rk3576-photonicat2.dts` around lines
838 - 845, The spi1 pinctrl node (spi1m2_1_pins) is missing the MISO pin so when
&spi1 overrides pinctrl-0 reads will fail; update the rockchip,pins list under
spi1m2_1_pins to include the missing MISO entry (the appropriate RK_PCx entry
for spi1_miso_m2) or replace the custom group with the full upstream SPI pin
group so that CS/CLK/MOSI/MISO are all muxed for the spi1 controller; locate the
spi1 node and its spi1m2_1_pins binding to add the correct MISO RK_PC... entry
consistent with the other entries or switch to the canonical upstream pin group.
Comment on lines
+287
to
+305
| +static irqreturn_t rkpwmc_irq_handler(int irq, void *data) | ||
| +{ | ||
| + struct rockchip_pwm_capture *pc = data; | ||
| + u32 intsts; | ||
| + u32 clr = 0; | ||
| + | ||
| + intsts = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_INTSTS); | ||
| + | ||
| + if (!(intsts & RKPWMC_INT_MASK)) | ||
| + return IRQ_NONE; | ||
| + | ||
| + if (intsts & PWMV4_INT_LPC) { | ||
| + clr |= PWMV4_INT_LPC; | ||
| + counter_push_event(pc->counter, COUNTER_EVENT_CHANGE_OF_STATE, 0); | ||
| + } | ||
| + | ||
| + if (intsts & PWMV4_INT_HPC) { | ||
| + clr |= PWMV4_INT_HPC; | ||
| + counter_push_event(pc->counter, COUNTER_EVENT_CHANGE_OF_STATE, 1); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "rk3576-0007-counter-rockchip-pwm-capture-driver.patch" --type fRepository: armbian/build
Length of output: 147
🏁 Script executed:
cat patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch | head -400Repository: armbian/build
Length of output: 11253
Move pc->counter assignment before IRQ registration to eliminate race condition.
The IRQ handler dereferences pc->counter unconditionally, but devm_request_irq() runs at line 273 while pc->counter = counter is not set until line 284. A pending interrupt in that window causes NULL-dereference inside counter_push_event().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch`
around lines 287 - 305, Assign pc->counter before registering the IRQ to avoid
the race where rkpwmc_irq_handler dereferences pc->counter while it's still
NULL: move the statement that sets pc->counter = counter to occur before the
devm_request_irq(...) call (the IRQ registration) so that rkpwmc_irq_handler,
which reads pc->counter and calls counter_push_event(...), always sees a valid
pointer; ensure the assignment happens immediately after pc is initialized and
before any calls to devm_request_irq.
EvilOlaf
approved these changes
Mar 17, 2026
Contributor
|
✅ This PR has been reviewed and approved — all set for merge! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add Ariaboard Photonicat 2 support for rockchip64-6.18
Board configuration:
Kernel config:
Device tree:
RK3576 SoC patches (rockchip64-6.18):
0001 gpio: rockchip: set input direction when request irq
Author: Ye Zhang ye.zhang@rock-chips.com
Source: [PATCH v5 4/4] gpio: rockchip: Update the GPIO driver
https://lore.kernel.org/r/20241112015408.3139996-5-ye.zhang@rock-chips.com
Series: https://lore.kernel.org/r/20241112015408.3139996-1-ye.zhang@rock-chips.com
0002 mmc: dw_mmc-rockchip: add v2 tuning support
Author: Shawn Lin shawn.lin@rock-chips.com
Posted-by: Detlev Casanova detlev.casanova@collabora.com
Source: [PATCH v3 2/3] Add dw_mmc support for rk3576
https://lore.kernel.org/r/20240814223555.3695-3-detlev.casanova@collabora.com
Series: https://lore.kernel.org/r/20240814223555.3695-1-detlev.casanova@collabora.com
0003 pmdomain: rockchip: Add always on configuration
Author: Finley Xiao finley.xiao@rock-chips.com
Source: Rockchip BSP (not posted to lore.kernel.org)
0004 dt-bindings: pwm: Add a new binding for rockchip,rk3576-pwm
0005 mfd: Add Rockchip mfpwm driver
0006 pwm: Add rockchip PWMv4 driver
0007 counter: Add rockchip-pwm-capture driver
0008 arm64: dts: rockchip: add PWM nodes to RK3576 SoC dtsi
Author: Nicolas Frattaroli nicolas.frattaroli@collabora.com
Source: [PATCH v3 0/5] Add Rockchip RK3576 PWM Support Through MFPWM
https://lore.kernel.org/r/20251027-rk3576-pwm-v3-0-654a5cb1e3f8@collabora.com
0009 arm64: dts: rockchip: add dma-coherent for pcie and gmac of RK3576
Author: Shawn Lin shawn.lin@rock-chips.com
Source: https://lore.kernel.org/r/1764313762-78063-1-git-send-email-shawn.lin@rock-chips.com
U-Boot:
BSP:
How Has This Been Tested?
Tested on Photonicat 2 with the following functionality:
photonicat-pmpower manager driver (battery status, hwmon sensors, graceful shutdown; fan device present)aic8800driver for onboard Wi-FiBuild log
https://paste.next.armbian.com/budupijeze
Summary by CodeRabbit