rkr7.2: clk: rk808: fix platform_device_id names for the clkout cells + regulator, rtc, pwrkey cells - #518
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change updates RK8xx platform device ID tables. The clock driver uses Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
arch/arm64/boot/dts/rockchip/rk3568-mixtile-edge2.dts (1)
537-538: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument
rockchip,clk-32k-always-onin the RK809 binding.
drivers/clk/clk-rk808.cconsumes this property to markrk808-clkout2as critical, but the RK809 bindings only defineclock-output-namesand do not define it. Add a boolean property definition before relying on it in this and the existingrk3576s-rk809-tablet-v10node.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arch/arm64/boot/dts/rockchip/rk3568-mixtile-edge2.dts` around lines 537 - 538, Add the boolean rockchip,clk-32k-always-on property definition to the RK809 binding schema, alongside the existing clock-output-names definition, so both rk3568-mixtile-edge2 and rk3576s-rk809-tablet-v10 nodes are formally supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drivers/auxdisplay/lcd-vk2c21.c`:
- Around line 434-441: Update brightness_show to acquire d->lock before reading
d->brightness, copy the protected value to a local variable, then release the
lock before formatting the sysfs response. Use the local value for both the
default comparison and numeric output, preserving the existing output format.
---
Nitpick comments:
In `@arch/arm64/boot/dts/rockchip/rk3568-mixtile-edge2.dts`:
- Around line 537-538: Add the boolean rockchip,clk-32k-always-on property
definition to the RK809 binding schema, alongside the existing
clock-output-names definition, so both rk3568-mixtile-edge2 and
rk3576s-rk809-tablet-v10 nodes are formally supported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1bafa44-8899-4860-b8d2-41eaab7fb82c
📒 Files selected for processing (4)
arch/arm64/boot/dts/rockchip/rk3568-mixtile-edge2.dtsarch/arm64/boot/dts/rockchip/rk3588-recomputer-rk3588-devkit.dtsdrivers/auxdisplay/lcd-vk2c21.cdrivers/clk/clk-rk808.c
Commit ea9b5ac ("mfd: rk8xx: add multi-PMIC coexistence support for rk8xx series") renamed the per-variant MFD cells and gave each child driver a platform_device_id table to match on. The clkout table got the suffix wrong: it lists "rk805-clk", "rk817-clk", ... while rk808.c registers the platform devices as "rk805-clkout", "rk817-clkout", ... No entry matches a real device, and the driver name does not save it either. platform_match() returns as soon as a driver has an id_table: if (pdrv->id_table) return platform_match_id(pdrv->id_table, pdev) != NULL; /* fall-back to driver name match */ return (strcmp(pdev->name, drv->name) == 0); so introducing the table also stopped "rk808-clkout" from matching the driver's own name, which is how it used to bind. rk808_clkout_probe() therefore never runs on any variant, RK808 included, and the PMIC registers no clock provider at all. Every consumer of <&rk8xx 1> then gets -EPROBE_DEFER forever. On boards with an SDIO WiFi module that means mmc-pwrseq-simple never probes, so the SDIO host never gets a pwrseq and never enumerates the card: [WLAN_RFKILL]: wlan_platdata_parse_dt: The ref_wifi_clk not found ! platform sdio-pwrseq: deferred probe pending dwmmc_rockchip fe000000.mmc: <re-probed indefinitely, no mmc host> Before that commit there was no id_table and every variant used the "rk808-clkout" cell name, so all of them bound via the driver name. That is why this only shows up now. Fix the table to use the device names rk808.c actually creates. RK801 has no clkout cell, so it is intentionally absent. Signed-off-by: Ricardo Pardini <ricardo@pardini.net> Assisted-By: Claude Opus 5 <noreply@anthropic.com>
…wrkey cells Same class of bug as the clkout table: the id tables added by ea9b5ac ("mfd: rk8xx: add multi-PMIC coexistence support for rk8xx series") list device names that rk808.c never creates, while omitting names it does. RK809 shares the rk817s[] cell array with RK817 (see the RK809_ID/RK817_ID fallthrough in rk808_probe()), so the platform devices are called "rk817-regulator", "rk817-rtc" and "rk817-pwrkey". There is no such thing as an "rk809-*" device, making those three entries permanently dead. Conversely "rk808-regulator" and "rk808-rtc" are created by rk808s[] but are in no table, and "rk801-pwrkey" is created by rk801s[] but is in no table. This is not cosmetic. platform_match() returns as soon as a driver has an id_table: if (pdrv->id_table) return platform_match_id(pdrv->id_table, pdev) != NULL; /* fall-back to driver name match */ return (strcmp(pdev->name, drv->name) == 0); so once these tables exist the drv->name fallback is unreachable, and a device missing from the table binds to nothing at all. RK808 boards therefore come up with no regulators and no RTC, and RK801 boards with no power key. Replace the impossible rk809-* entries with the names that are actually missing. rk801-regulator needs no entry: it is handled by its own driver in drivers/regulator/rk801-regulator.c, which has no id_table and so still matches by name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ef9f004 to
c433f91
Compare
clk: rk808: fix platform_device_id names for the clkout cells)clk: rk808: fix platform_device_id names for the clkout cells + regulator, rtc, pwrkey cells
|
Added pwrkey/rtc/regulator cell fixes, dropped unrelated board-level patches. @coderabbitai re-review |
|
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
clk: rk808: fix platform_device_id names for the clkout cells
Commit ea9b5ac ("mfd: rk8xx: add multi-PMIC coexistence support for
rk8xx series") renamed the per-variant MFD cells and gave each child driver
a platform_device_id table to match on. The clkout table got the suffix
wrong: it lists "rk805-clk", "rk817-clk", ... while rk808.c registers the
platform devices as "rk805-clkout", "rk817-clkout", ...
No entry matches a real device, and the driver name does not save it either.
platform_match() returns as soon as a driver has an id_table:
if (pdrv->id_table)
return platform_match_id(pdrv->id_table, pdev) != NULL;
/* fall-back to driver name match */
return (strcmp(pdev->name, drv->name) == 0);
so introducing the table also stopped "rk808-clkout" from matching the
driver's own name, which is how it used to bind. rk808_clkout_probe()
therefore never runs on any variant, RK808 included, and the PMIC registers
no clock provider at all.
Every consumer of <&rk8xx 1> then gets -EPROBE_DEFER forever. On boards with
an SDIO WiFi module that means mmc-pwrseq-simple never probes, so the SDIO
host never gets a pwrseq and never enumerates the card:
[WLAN_RFKILL]: wlan_platdata_parse_dt: The ref_wifi_clk not found !
platform sdio-pwrseq: deferred probe pending
dwmmc_rockchip fe000000.mmc: <re-probed indefinitely, no mmc host>
Before that commit there was no id_table and every variant used the
"rk808-clkout" cell name, so all of them bound via the driver name. That is
why this only shows up now.
Fix the table to use the device names rk808.c actually creates. RK801 has no
clkout cell, so it is intentionally absent.
mfd: rk8xx: fix platform_device_id names for the regulator, rtc and pwrkey cells
Same class of bug as the clkout table: the id tables added by ea9b5ac
("mfd: rk8xx: add multi-PMIC coexistence support for rk8xx series") list
device names that rk808.c never creates, while omitting names it does.
RK809 shares the rk817s[] cell array with RK817 (see the RK809_ID/RK817_ID
fallthrough in rk808_probe()), so the platform devices are called
"rk817-regulator", "rk817-rtc" and "rk817-pwrkey". There is no such thing as
an "rk809-*" device, making those three entries permanently dead. Conversely
"rk808-regulator" and "rk808-rtc" are created by rk808s[] but are in no
table, and "rk801-pwrkey" is created by rk801s[] but is in no table.
This is not cosmetic. platform_match() returns as soon as a driver has an
id_table:
if (pdrv->id_table)
return platform_match_id(pdrv->id_table, pdev) != NULL;
/* fall-back to driver name match */
return (strcmp(pdev->name, drv->name) == 0);
so once these tables exist the drv->name fallback is unreachable, and a
device missing from the table binds to nothing at all. RK808 boards
therefore come up with no regulators and no RTC, and RK801 boards with no
power key.
Replace the impossible rk809-* entries with the names that are actually
missing. rk801-regulator needs no entry: it is handled by its own driver in
drivers/regulator/rk801-regulator.c, which has no id_table and so still
matches by name.
Assisted-by: Claude Opus 5