Add kernel level support for KICKPI K3B#511
Conversation
Keep the default panfrost clock initialization path intact, but optionally acquire and enable additional Rockchip-specific GPU clocks (clk_gpu, clk_gpu_brg and aclk_gpu) when present. This keeps existing "gpu"/"bus" based platforms such as rk3568 working unchanged, while allowing rk3562 to bring up panfrost without hanging during early GPU register access.
WalkthroughThis PR adds device tree support for a new rk3562-based KickPi K3B board with a DSI display/touchscreen overlay, extends the Panfrost driver with optional extra clock management, introduces a complete new Jadard (JD9366/JD-series) touchscreen chipset driver with Kconfig/build wiring, common/module/platform/debug/sorting code and IC-specific headers, adds a delayed initial-ON feature to the GPIO LED driver, and adds a new Maxio Ethernet PHY driver with integration points in stmmac_main and phy_device. Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (16)
drivers/net/phy/maxio.c-120-130 (1)
120-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnchecked
PHY_READinsidePHY_WRITEandret |=with negative error codes.In
maxio_mae0621a_resume:
PHY_READ(phydev, MII_BMCR)is embedded inside thePHY_WRITEcall. If the read fails (returns negative), the OR withBMCR_RESETproduces a garbage value that gets written toMII_BMCR, potentially putting the PHY in an undefined state.
ret |= PHY_WRITE(...)uses bitwise OR to accumulate errors. Ifgenphy_resumereturns a negative error code,ret |=produces a bitwise combination that is not a meaningful error value.🔒 Proposed fix
static int maxio_mae0621a_resume(struct phy_device *phydev) { int ret; + int bmcr; ret = genphy_resume(phydev); + if (ret < 0) + return ret; - ret |= PHY_WRITE(phydev, MII_BMCR, BMCR_RESET | PHY_READ(phydev, MII_BMCR)); + bmcr = PHY_READ(phydev, MII_BMCR); + if (bmcr < 0) + return bmcr; + + ret = PHY_WRITE(phydev, MII_BMCR, BMCR_RESET | bmcr); + if (ret < 0) + return ret; + msleep(20); return ret;🤖 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 `@drivers/net/phy/maxio.c` around lines 120 - 130, The `maxio_mae0621a_resume` flow is mixing unchecked register reads with error accumulation, so fix it by handling `PHY_READ(phydev, MII_BMCR)` separately before calling `PHY_WRITE`, and bail out immediately if the read fails. Then replace the `ret |=` pattern with normal sequential error checking so `genphy_resume` and the BMCR write each return a real negative error code from `maxio_mae0621a_resume` instead of a bitwise-combined value.drivers/net/phy/maxio.c-156-216 (1)
156-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNo error checking on
maxio_read_pagedin status and link-compatibility paths.
phy_resolve_aneg_linkmode_maxioandphy_resolve_link_compatibility_maxiocallmaxio_read_pagedand use the return value directly in bitwise operations without checking for negative error codes. A negative return (e.g.,-EIO=-5=0xFFFFFFFB) has high bits set, sophysr_p_a43 & MAXIO_PHY_SPEEDoriner & AUTONEG_COMPLETED_INT_ENwill produce incorrect results, setting wrong speed/duplex/link state.Additionally,
phy_resolve_link_compatibility_maxiodereferencesphydev->privwithout a null check — if probe failed to allocate orremovealready freed it, this is a null-pointer dereference.🔒 Proposed fix for `phy_resolve_aneg_linkmode_maxio`
static void phy_resolve_aneg_linkmode_maxio(struct phy_device *phydev){ int physr_p_a43 = maxio_read_paged(phydev,0xa43,MAXIO_PHYSR_P_A43); + if (physr_p_a43 < 0) + return; + if ((physr_p_a43&MAXIO_PHY_SPEED) == MAXIO_PHY_1000M) {🤖 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 `@drivers/net/phy/maxio.c` around lines 156 - 216, Both phy_resolve_aneg_linkmode_maxio and phy_resolve_link_compatibility_maxio are using maxio_read_paged results directly in bit tests, so add explicit negative-return handling before reading speed/duplex/link bits and bail out early on error. In phy_resolve_aneg_linkmode_maxio, guard physr_p_a43 before updating phydev->speed, duplex, pause, and asym_pause; in phy_resolve_link_compatibility_maxio, guard iner/physr/insr before using AUTONEG_COMPLETED_INT_EN, LINKOK, or AUTONEG_COMPLETED. Also add a phydev->priv null check at the start of phy_resolve_link_compatibility_maxio before dereferencing paras.drivers/leds/leds-gpio.c-157-165 (1)
157-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not apply the delay to
default-state = "keep".Line 164 delays any currently-high GPIO, including
LEDS_GPIO_DEFSTATE_KEEP. That breaks the existing “keep current hardware state” contract by forcing a kept-on LED off until the timer expires. Gate the delay onLEDS_GPIO_DEFSTATE_ONinstead.Proposed fix
- if (led_dat->initial_on_delay_ms && state) { + if (led_dat->initial_on_delay_ms && + template->default_state == LEDS_GPIO_DEFSTATE_ON) { led_dat->pending_initial_on = 1; led_dat->cdev.brightness = LED_OFF; state = 0;🤖 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 `@drivers/leds/leds-gpio.c` around lines 157 - 165, The initial-on delay handling in the LED GPIO setup should not affect LEDs using the KEEP default state. In the logic around `template->default_state` and `led_dat->initial_on_delay_ms`, change the delay gate so it only applies when the requested state comes from `LEDS_GPIO_DEFSTATE_ON`, not when `LEDS_GPIO_DEFSTATE_KEEP` reads an already-high GPIO. Keep the existing KEEP path in `leds_gpio`/`led_dat` intact so hardware state is preserved immediately.drivers/input/touchscreen/jdchipset/jadard_common.h-503-506 (1)
503-506: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
CONFIG_TOUCHSCREEN_JADARD_DEBUGhas no corresponding Kconfig option.This preprocessor guard references
CONFIG_TOUCHSCREEN_JADARD_DEBUG, but no such Kconfig symbol exists — it's only force-defined viaccflags-yin the Makefile. If the Makefile ccflags are later fixed to use Kconfig, this guard will silently disable debug code. Add a proper Kconfig option for it.🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_common.h` around lines 503 - 506, The debug field block in jadard_common.h is guarded by CONFIG_TOUCHSCREEN_JADARD_DEBUG, but that symbol is not defined by Kconfig. Add a matching Kconfig option for the Jadard debug feature and then keep the same guard in the struct definition so the debug members in jadard_common.h stay controlled by a real config symbol rather than a Makefile-only define.drivers/input/touchscreen/Kconfig-514-525 (1)
514-525: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
depends on I2Cto the Kconfig entry.The Jadard driver communicates over I2C (the platform layer implements I2C bus read/write and registers an I2C driver), but this Kconfig entry has no
depends on I2C. Enabling it without I2C support will cause build or probe failures. Most I2C touchscreen entries in this file (e.g., TOUCHSCREEN_GOODIX, TOUCHSCREEN_ELAN, TOUCHSCREEN_SIS_I2C) declare this dependency.🔧 Proposed fix
config TOUCHSCREEN_JADARD_CHIPSET tristate "JADARD touch panel driver support" + depends on I2C help🤖 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 `@drivers/input/touchscreen/Kconfig` around lines 514 - 525, The TOUCHSCREEN_JADARD_CHIPSET Kconfig entry is missing the I2C dependency required by the driver’s bus communication. Update the Kconfig stanza for TOUCHSCREEN_JADARD_CHIPSET to add a depends on I2C constraint, matching the pattern used by other I2C touchscreen entries in this file, and keep the existing help text and sourced jdchipset Kconfig unchanged.drivers/input/touchscreen/jdchipset/Makefile-1-2 (1)
1-2: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDon't hardcode debug and sorting flags via
ccflags-y.Lines 1–2 unconditionally define
CONFIG_TOUCHSCREEN_JADARD_DEBUG=yandCONFIG_TOUCHSCREEN_JADARD_SORTING=y, bypassing the Kconfig system. This forces debug code and sorting test code into all builds, including production. These should be proper Kconfig options (they're already referenced via#if defined(CONFIG_TOUCHSCREEN_JADARD_DEBUG)injadard_common.h) or controlled via existing Kconfig symbols.🔧 Proposed fix
-ccflags-y += -DCONFIG_TOUCHSCREEN_JADARD_DEBUG=y -ccflags-y += -DCONFIG_TOUCHSCREEN_JADARD_SORTING=y +# These should be controlled via Kconfig options, e.g.: +# config TOUCHSCREEN_JADARD_DEBUG +# bool "Jadard debug support" +# depends on TOUCHSCREEN_JADARD_CHIPSET +ccflags-$(CONFIG_TOUCHSCREEN_JADARD_DEBUG) += -DCONFIG_TOUCHSCREEN_JADARD_DEBUG=y +ccflags-$(CONFIG_TOUCHSCREEN_JADARD_SORTING) += -DCONFIG_TOUCHSCREEN_JADARD_SORTING=y🤖 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 `@drivers/input/touchscreen/jdchipset/Makefile` around lines 1 - 2, Remove the hardcoded `ccflags-y` definitions in the JADARD Makefile and wire these features through Kconfig instead, so `CONFIG_TOUCHSCREEN_JADARD_DEBUG` and `CONFIG_TOUCHSCREEN_JADARD_SORTING` are only enabled when the corresponding config symbols are selected. Update the build setup around the JADARD driver files and keep the existing `#if defined(CONFIG_TOUCHSCREEN_JADARD_DEBUG)` checks in `jadard_common.h` working by sourcing those symbols from Kconfig rather than forcing them in every build.drivers/input/touchscreen/jdchipset/jadard_common.h-115-120 (1)
115-120: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
JD_PRODUCT_TYPEis set to 0 (Cell phone) but should be 1 (JD9366T Tablet) for this board.The comment on lines 117–119 defines
0as "Cell phone" and1as "JD9366T Tablet". The KICKPI K3B uses a JD9366T controller on a tablet form factor, butJD_PRODUCT_TYPEis hardcoded to0. This may affect touch data sizes, report formats, or behavior specific to the product type.🔧 Proposed fix
-#define JD_PRODUCT_TYPE (0) +#define JD_PRODUCT_TYPE (1)🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_common.h` around lines 115 - 120, Update the JD product selection in jadard_common.h so the board uses the tablet variant instead of the cell phone variant. Change the JD_PRODUCT_TYPE definition to match the JD9366T Tablet setting described in the nearby comment, and keep the identifier JD_PRODUCT_TYPE aligned with the KICKPI K3B’s JD9366T controller/form factor.drivers/input/touchscreen/jdchipset/Makefile-4-6 (1)
4-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
TARGET_BUILD_VARIANTis Android-specific and won't work in standard kernel builds.
TARGET_BUILD_VARIANTis set by the Android build system (e.g.,uservsuserdebug), not by the standard kernel Kbuild. In non-Android builds contexts, this variable is undefined, so__JADARD_GKI__will never be defined. If this flag is needed, consider using a proper Kconfig symbol instead.🔧 Proposed fix
-ifeq ($(TARGET_BUILD_VARIANT), user) - ccflags-y += -D__JADARD_GKI__ -endif +# Use a Kconfig symbol instead: +# ccflags-$(CONFIG_TOUCHSCREEN_JADARD_GKI) += -D__JADARD_GKI__🤖 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 `@drivers/input/touchscreen/jdchipset/Makefile` around lines 4 - 6, The Makefile currently gates __JADARD_GKI__ on TARGET_BUILD_VARIANT, which is Android-specific and undefined in normal Kbuild flows. Replace this conditional in the jdchipset Makefile with a proper Kconfig-controlled option or another kernel-supported build symbol, and use that symbol to add the ccflags-y define so the flag is set consistently across all builds.drivers/input/touchscreen/jdchipset/jadard_module.c-49-50 (1)
49-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the DBIC page parameter before reading
par[0].When
cmd == 0xDEandpage_en > 0, this path dereferencespar[0]without checkingparorpar_len. A bad internal caller can Oops the kernel.🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_module.c` around lines 49 - 50, The DBIC page handling in `g_module_fp.fp_SetDbicPage` can dereference `par[0]` without verifying the buffer is valid. Update the `cmd == 0xDE` and `page_en > 0` branch in `jadard_module.c` to check that `par` is non-NULL and that `par_len` is large enough before reading `par[0]`, and only call `fp_SetDbicPage` after those guards pass.drivers/input/touchscreen/jdchipset/jadard_platform.c-793-808 (1)
793-808: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up
tswhen common init fails.
jadard_chip_common_probe()allocatests, stores it in client/global state, then returnsjadard_chip_common_init()directly. If init fails, the allocation and clientdata are left behind.🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_platform.c` around lines 793 - 808, The probe path in jadard_chip_common_probe() leaves the allocated jadard_ts_data and client/global state behind when jadard_chip_common_init() fails. Capture the return value from jadard_chip_common_init(), and if it is an error, undo the setup done on ts by clearing the client data and pjadard_ts_data and freeing the allocated ts before returning the failure code. Keep the cleanup localized to the jadard_chip_common_probe() flow so the successful path remains unchanged.drivers/input/touchscreen/jdchipset/jadard_platform.c-821-847 (1)
821-847: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWire
jadard_common_pm_opsinto the driver..pmis commented out here, so these suspend/resume hooks never run during system sleep.🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_platform.c` around lines 821 - 847, The jadard_common driver defines jadard_common_pm_ops but never registers it because the .pm hook in jadard_common_driver is commented out, so suspend/resume callbacks won’t be used. Update the jadard_common_driver setup to wire jadard_common_pm_ops into the driver’s .pm field inside the .driver block, and keep the CONFIG_PM-related handling consistent with the existing jadard_common_suspend and jadard_common_resume symbols.drivers/input/touchscreen/jdchipset/jadard_platform.c-117-122 (1)
117-122: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a single
i2c_msg.flagsinitializer.The second
.flagsentry overwrites the first, so the message ends up inheriting all ofclient->flagsinstead of just ten-bit addressing plus read mode.Proposed fix
- .flags = client->flags & I2C_M_TEN, - .flags = client->flags | I2C_M_RD, + .flags = (client->flags & I2C_M_TEN) | I2C_M_RD,🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_platform.c` around lines 117 - 122, The i2c_msg setup in jadard_platform should use only one .flags initializer, since the duplicate entry overwrites the intended ten-bit-addressing flag. Update the msg construction in the I2C transfer helper to keep just the correct combination of I2C_M_TEN and I2C_M_RD, and remove the extra client->flags inheritance so the read message is built with the intended flags only.drivers/input/touchscreen/jdchipset/jadard_platform.c-273-280 (1)
273-280: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFix the
avdderror unwind.regulator_put()must not be called onpdata->vcc_anahere because it still holds anERR_PTR; release any previously acquired regulator before returning.🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_platform.c` around lines 273 - 280, The avdd error unwind in jadard platform regulator setup is releasing the wrong handle after a failed regulator_get. In the error path for pdata->vcc_ana, do not call regulator_put() on the ERR_PTR stored in pdata->vcc_ana; instead, just return the PTR_ERR value and only unwind any earlier successfully acquired regulators in the same init path. Use the regulator acquisition/error-handling block around pdata->vcc_ana in jadard_platform.c to keep the cleanup order correct.drivers/input/touchscreen/jdchipset/jadard_debug.c-936-942 (1)
936-942: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
breakcauses fallthrough and leaked file handle.
JD_DATA_TYPE_LABELfalls through intoJD_DATA_TYPE_LAPLACE: the firstfilp_open()result is immediately overwritten, leaking that file handle and always openingJD_LAPLACE_DUMP_FILEinstead ofJD_LABEL_DUMP_FILEwhenDataTypeis Label.🐛 Proposed fix
case JD_DATA_TYPE_LABEL: jd_diag_mutual_fn = filp_open(JD_LABEL_DUMP_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0); jd_diag_mutual_fn->f_pos = 0; + break; case JD_DATA_TYPE_LAPLACE: jd_diag_mutual_fn = filp_open(JD_LAPLACE_DUMP_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0); jd_diag_mutual_fn->f_pos = 0; break;🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_debug.c` around lines 936 - 942, Add the missing control flow break in the JD_DATA_TYPE_LABEL branch inside the dump-file selection logic so it does not fall through into JD_DATA_TYPE_LAPLACE. Update the switch handling around jd_diag_mutual_fn and the filp_open calls so Label opens JD_LABEL_DUMP_FILE and returns that handle without being overwritten, preserving the correct file target and avoiding the leaked handle.drivers/input/touchscreen/jdchipset/jadard_debug.c-2294-2394 (1)
2294-2394: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winWorld-writable (
S_IWUGO) debug/register/firmware proc nodes expose hardware control to any local user.
register,display,debug,reset,fw_dump,diag,int_en,buf_rd, etc. are all created withS_IWUGO, granting write access to any local user, not just root. Viadebug(buf_tmp[0]=='t'→request_firmware(fileName,...)), an unprivileged process can trigger firmware "upgrade" with an attacker-chosen filename, erase flash (erase), or write arbitrary raw touch/display registers (register,display). This is a meaningful local privilege exposure for what should be a debug-only interface.🔒 Proposed direction
- jadard_proc_register_file = proc_create(JADARD_PROC_REGISTER_FILE, (S_IWUGO | S_IRUGO), + jadard_proc_register_file = proc_create(JADARD_PROC_REGISTER_FILE, (S_IWUSR | S_IRUSR), pjadard_touch_proc_dir, &jadard_proc_register_ops);Apply similarly to
reset,debug,fw_dump,display,display_stdat minimum, or add acapable(CAP_SYS_ADMIN)check in each write handler.🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_debug.c` around lines 2294 - 2394, The proc nodes created in jadard_touch_proc_init are too permissive because S_IWUGO grants write access to all local users. Tighten the permissions for the debug/control entries created via proc_create for jadard_proc_register_file, jadard_proc_display_file, jadard_proc_debug_file, jadard_proc_reset_file, jadard_proc_fw_dump_file, jadard_proc_diag_file, jadard_proc_int_en_file, jadard_proc_buf_rd_file, and jadard_proc_display_std_file so they are root-only or otherwise restricted, and if any of these must remain writable, add a capability check such as CAP_SYS_ADMIN in the corresponding write handlers (for example the ops behind jadard_proc_debug_ops and related *_ops).drivers/input/touchscreen/jdchipset/jadard_debug.c-918-948 (1)
918-948: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck
filp_open()return before using->f_pos
filp_open()can returnERR_PTR; dereferencingjd_diag_mutual_fn->f_poshere will crash the kernel on open failure. Use the existingIS_ERR()guard pattern fromjadard_ts_fw_dump_work_funcbefore touching the file pointer.🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_debug.c` around lines 918 - 948, The mutual data log file setup in jadard_debug.c dereferences jd_diag_mutual_fn->f_pos immediately after filp_open() without checking for errors. Update the open path in this switch to follow the same IS_ERR()/error-handling pattern used in jadard_ts_fw_dump_work_func before touching jd_diag_mutual_fn, and only set f_pos when filp_open() succeeds; otherwise handle the failure and skip using the file pointer.
🟡 Minor comments (2)
drivers/net/phy/maxio.c-294-316 (1)
294-316: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the 100ms busy wait in probe.
mdelay(100)spins the CPU in a sleepable path;msleep(100)is the better fit.🤖 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 `@drivers/net/phy/maxio.c` around lines 294 - 316, The probe path in maxio_mae0621a_probe uses a 100ms busy wait, which is inappropriate here and should be replaced with a sleepable delay. Update the post-setup wait after the MAXIO_PAGE_SELECT write to use the kernel sleep API instead of mdelay(100), keeping the same 100ms timing but avoiding CPU spinning in this probe routine.drivers/input/touchscreen/jdchipset/jadard_debug.c-2149-2159 (1)
2149-2159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPotential
size_tunderflow inlen - 2beforescnprintf.If a caller writes just
"t"(len=1) todebug,len - 2underflows (size_t) to a huge value passed as thescnprintfsize bound. Currently bounded in practice sincebuf_tmpis fixed-size and NUL-padded, but the size argument is logically wrong and fragile — combine with the world-writable permission above and this deserves a proper length check.💡 Proposed fix
} else if (buf_tmp[0] == 't') { + if (len < 3) { + JD_E("%s: missing firmware file name\n", __func__); + return -EINVAL; + } if (pjadard_ts_data->diag_thread_active) {🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_debug.c` around lines 2149 - 2159, The `scnprintf` call in the `t` command path uses `len - 2`, which can underflow when very short input like `"t"` is written to `debug`. In `jadard_debug.c`, update the parsing logic around the `buf_tmp[0] == 't'` branch to validate that `len` is large enough before subtracting, and only copy the filename portion when the input actually contains the expected prefix and payload. Use the existing `buf_tmp`, `len`, and `fileName` handling in `jadard_debug`/the upgrade flow to keep the size argument safe.
🧹 Nitpick comments (4)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c (1)
53-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPHY IDs are duplicated between stmmac_main.c and maxio.c.
The same PHY ID constants (0x7b744411, 0x7b744412) are defined here and in
drivers/net/phy/maxio.c(lines 398, 411, 427-428). If one side changes, the other can silently drift. Extract these to a shared header (e.g.,include/linux/maxio_phy.hor extend an existing PHY ID header).🤖 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 `@drivers/net/ethernet/stmicro/stmmac/stmmac_main.c` around lines 53 - 55, The MAXIO PHY ID constants are duplicated between stmmac_main.c and maxio.c, so move them into a shared header and have both files include it instead of defining their own copies. Use the existing MAXIO_PHY_MAE0621A_Q2C_ID and MAXIO_PHY_MAE0621A_Q3C_ID symbols as the single source of truth, either in a new shared header like include/linux/maxio_phy.h or an existing PHY ID header, and update both stmmac_main.c and maxio.c to reference those shared definitions.drivers/input/touchscreen/jdchipset/jadard_common.h (1)
1-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClean up includes: remove dead code and verify deprecated/unusual headers.
- Line 4:
/*#include<asm/segment.h> */is dead code and should be removed.- Line 17:
<linux/gpio.h>is deprecated in modern kernels; the preferred header is<linux/gpio/consumer.h>or<linux/of_gpio.h>(already included conditionally on line 29).- Line 22:
<linux/buffer_head.h>is a filesystem/block-layer header unusual for a touchscreen driver — verify it's actually needed.✏️ Proposed cleanup
-/* `#include` <asm/segment.h> */ `#include` <linux/uaccess.h> `#include` <linux/atomic.h> `#include` <linux/vmalloc.h> `#include` <linux/version.h> `#include` <linux/delay.h> `#include` <linux/i2c.h> `#include` <linux/input.h> `#include` <linux/interrupt.h> `#include` <linux/module.h> `#include` <linux/async.h> `#include` <linux/platform_device.h> `#include` <linux/slab.h> -#include <linux/gpio.h> `#include` <linux/input/mt.h> `#include` <linux/firmware.h> `#include` <linux/types.h> `#include` <linux/fs.h> -#include <linux/buffer_head.h> `#include` <linux/pm_wakeup.h> `#include` <linux/seq_file.h> `#include` <linux/proc_fs.h>🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_common.h` around lines 1 - 26, Remove the dead commented-out asm include from jadard_common.h, and in the header include block for jadard_common.h verify whether the touchscreen driver actually needs linux/gpio.h and linux/buffer_head.h. Replace gpio usage with the appropriate modern GPIO header path if needed, and drop buffer_head if it is not referenced anywhere else in the jdchipset code.drivers/input/touchscreen/jdchipset/Kconfig (1)
95-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse consistent
TOUCHSCREEN_JADARD_prefix for all config symbols.
JD_DB,JD_FB, andJD_HIDare too short and risk collisions with other Kconfig symbols in the global namespace. All other symbols in this file use theTOUCHSCREEN_JADARD_prefix. This also affects preprocessor usage injadard_common.h(e.g.,CONFIG_JD_HID,CONFIG_JD_DB,CONFIG_JD_FB).🔧 Proposed fix
-config JD_DB +config TOUCHSCREEN_JADARD_DB ... -config JD_FB +config TOUCHSCREEN_JADARD_FB ... -config JD_HID +config TOUCHSCREEN_JADARD_HIDThe corresponding
#if defined(CONFIG_JD_HID)/CONFIG_JD_DB/CONFIG_JD_FBreferences injadard_common.hand the Makefile (obj-$(CONFIG_JD_HID)) would need to be updated to match.🤖 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 `@drivers/input/touchscreen/jdchipset/Kconfig` around lines 95 - 111, Rename the short Kconfig symbols `JD_DB`, `JD_FB`, and `JD_HID` to use the same `TOUCHSCREEN_JADARD_` prefix as the rest of the Jadard options, and update every consumer to match. In particular, adjust the `config` entries in the Kconfig file and the symbol checks in `jadard_common.h` that currently reference `CONFIG_JD_HID`, `CONFIG_JD_DB`, and `CONFIG_JD_FB`, as well as the Makefile `obj-$(CONFIG_JD_HID)` usage. Keep the feature names and help text unchanged, only align the symbol identifiers consistently across the driver.drivers/input/touchscreen/jdchipset/jadard_platform.h (1)
20-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep error and warning logs enabled outside debug builds.
With
CONFIG_TOUCHSCREEN_JADARD_DEBUG=n,JD_E()andJD_W()become no-ops, so probe, I2C, GPIO, and regulator failures are silent. Gate only info/debug logs; map errors/warnings topr_err/pr_warn.🤖 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 `@drivers/input/touchscreen/jdchipset/jadard_platform.h` around lines 20 - 30, `JD_E()` and `JD_W()` are currently disabled in non-debug builds because they expand to no-ops in the `#else` branch of `jadard_platform.h`, which hides important probe/I2C/GPIO/regulator failures. Update the logging macros so only `JD_I()` and `JD_D()` are gated by the debug/esd flags, and keep `JD_W()` and `JD_E()` active by mapping them to warning/error logging regardless of `CONFIG_TOUCHSCREEN_JADARD_DEBUG` in the `JD_I`, `JD_W`, `JD_E`, `JD_D` macro definitions.
🤖 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/input/touchscreen/jdchipset/jadard_module.c`:
- Around line 355-379: The zero-flash upgrade flow in jadard_module.c is not
safely serialized and has cleanup gaps in the upgrade path around the firmware
write logic. Update the jd_g_f_0f_update guard in the upgrade routine to use a
mutex or atomic-style protection so only one caller can enter the firmware-write
path, and refactor the firmware handling in the same upgrade function into a
single cleanup exit that always restores IRQ state, clears jd_g_f_0f_update,
releases fw when request_firmware() was used, and updates
pjadard_ts_data->fw_ready consistently on failure. Use the upgrade flow symbols
jd_g_f_0f_update, fp_ram_write, release_firmware, and pjadard_ts_data->fw_ready
to locate and fix the affected paths.
In `@drivers/leds/leds-gpio.c`:
- Around line 27-29: Separate the delayed-work lifecycle from the pending LED
state in leds-gpio.c: pending_initial_on is being cleared before the work item
has fully finished, which can cause shutdown paths in gpio_led_set() and the
devm teardown to skip cancel_delayed_work_sync() while work is still in flight.
Add a distinct flag for whether initial_on_work has been initialized/scheduled,
update the setup and worker paths that touch pending_initial_on, and make the
shutdown/cleanup logic always synchronize any scheduled delayed work based on
that new flag.
In `@drivers/net/ethernet/stmicro/stmmac/stmmac_main.c`:
- Around line 7738-7747: The PHY reinitialization block in stmmac_main should
first guard all dereferences of ndev->phydev so fixed-link or unattached PHY
cases do not crash, then only call the config_init method when ndev->phydev and
ndev->phydev->drv are valid and the PHY ID matches the intended MAXIO devices.
Also capture and check the int return from config_init in this resume path, and
propagate or log the failure instead of ignoring it so resume can stop or report
a misconfigured PHY.
In `@drivers/net/phy/maxio.c`:
- Around line 251-292: The maxio_mae0621a_config_init path accumulates failures
in ret but always returns 0, so stmmac_resume cannot detect PHY init errors;
change it to return the accumulated error result and preserve failure
propagation from the maxio_write_paged calls and maxio_self_check. Apply the
same return-value fix to maxio_mae0621aq3ci_config_init as well, using the same
ret handling pattern so both config_init implementations report initialization
failures correctly.
- Around line 40-67: The maxio_read_paged and maxio_write_paged helpers do not
propagate page-select failures and can restore an invalid page value, so update
both functions to return the PHY_READ(phydev, MAXIO_PAGE_SELECT) error
immediately when oldpage is negative, and only restore the page when the saved
page is valid. Also check the page-switch PHY_WRITE before attempting the paged
read/write, and make both helpers static so their scope is limited to maxio.c.
---
Major comments:
In `@drivers/input/touchscreen/jdchipset/jadard_common.h`:
- Around line 503-506: The debug field block in jadard_common.h is guarded by
CONFIG_TOUCHSCREEN_JADARD_DEBUG, but that symbol is not defined by Kconfig. Add
a matching Kconfig option for the Jadard debug feature and then keep the same
guard in the struct definition so the debug members in jadard_common.h stay
controlled by a real config symbol rather than a Makefile-only define.
- Around line 115-120: Update the JD product selection in jadard_common.h so the
board uses the tablet variant instead of the cell phone variant. Change the
JD_PRODUCT_TYPE definition to match the JD9366T Tablet setting described in the
nearby comment, and keep the identifier JD_PRODUCT_TYPE aligned with the KICKPI
K3B’s JD9366T controller/form factor.
In `@drivers/input/touchscreen/jdchipset/jadard_debug.c`:
- Around line 936-942: Add the missing control flow break in the
JD_DATA_TYPE_LABEL branch inside the dump-file selection logic so it does not
fall through into JD_DATA_TYPE_LAPLACE. Update the switch handling around
jd_diag_mutual_fn and the filp_open calls so Label opens JD_LABEL_DUMP_FILE and
returns that handle without being overwritten, preserving the correct file
target and avoiding the leaked handle.
- Around line 2294-2394: The proc nodes created in jadard_touch_proc_init are
too permissive because S_IWUGO grants write access to all local users. Tighten
the permissions for the debug/control entries created via proc_create for
jadard_proc_register_file, jadard_proc_display_file, jadard_proc_debug_file,
jadard_proc_reset_file, jadard_proc_fw_dump_file, jadard_proc_diag_file,
jadard_proc_int_en_file, jadard_proc_buf_rd_file, and
jadard_proc_display_std_file so they are root-only or otherwise restricted, and
if any of these must remain writable, add a capability check such as
CAP_SYS_ADMIN in the corresponding write handlers (for example the ops behind
jadard_proc_debug_ops and related *_ops).
- Around line 918-948: The mutual data log file setup in jadard_debug.c
dereferences jd_diag_mutual_fn->f_pos immediately after filp_open() without
checking for errors. Update the open path in this switch to follow the same
IS_ERR()/error-handling pattern used in jadard_ts_fw_dump_work_func before
touching jd_diag_mutual_fn, and only set f_pos when filp_open() succeeds;
otherwise handle the failure and skip using the file pointer.
In `@drivers/input/touchscreen/jdchipset/jadard_module.c`:
- Around line 49-50: The DBIC page handling in `g_module_fp.fp_SetDbicPage` can
dereference `par[0]` without verifying the buffer is valid. Update the `cmd ==
0xDE` and `page_en > 0` branch in `jadard_module.c` to check that `par` is
non-NULL and that `par_len` is large enough before reading `par[0]`, and only
call `fp_SetDbicPage` after those guards pass.
In `@drivers/input/touchscreen/jdchipset/jadard_platform.c`:
- Around line 793-808: The probe path in jadard_chip_common_probe() leaves the
allocated jadard_ts_data and client/global state behind when
jadard_chip_common_init() fails. Capture the return value from
jadard_chip_common_init(), and if it is an error, undo the setup done on ts by
clearing the client data and pjadard_ts_data and freeing the allocated ts before
returning the failure code. Keep the cleanup localized to the
jadard_chip_common_probe() flow so the successful path remains unchanged.
- Around line 821-847: The jadard_common driver defines jadard_common_pm_ops but
never registers it because the .pm hook in jadard_common_driver is commented
out, so suspend/resume callbacks won’t be used. Update the jadard_common_driver
setup to wire jadard_common_pm_ops into the driver’s .pm field inside the
.driver block, and keep the CONFIG_PM-related handling consistent with the
existing jadard_common_suspend and jadard_common_resume symbols.
- Around line 117-122: The i2c_msg setup in jadard_platform should use only one
.flags initializer, since the duplicate entry overwrites the intended
ten-bit-addressing flag. Update the msg construction in the I2C transfer helper
to keep just the correct combination of I2C_M_TEN and I2C_M_RD, and remove the
extra client->flags inheritance so the read message is built with the intended
flags only.
- Around line 273-280: The avdd error unwind in jadard platform regulator setup
is releasing the wrong handle after a failed regulator_get. In the error path
for pdata->vcc_ana, do not call regulator_put() on the ERR_PTR stored in
pdata->vcc_ana; instead, just return the PTR_ERR value and only unwind any
earlier successfully acquired regulators in the same init path. Use the
regulator acquisition/error-handling block around pdata->vcc_ana in
jadard_platform.c to keep the cleanup order correct.
In `@drivers/input/touchscreen/jdchipset/Makefile`:
- Around line 1-2: Remove the hardcoded `ccflags-y` definitions in the JADARD
Makefile and wire these features through Kconfig instead, so
`CONFIG_TOUCHSCREEN_JADARD_DEBUG` and `CONFIG_TOUCHSCREEN_JADARD_SORTING` are
only enabled when the corresponding config symbols are selected. Update the
build setup around the JADARD driver files and keep the existing `#if
defined(CONFIG_TOUCHSCREEN_JADARD_DEBUG)` checks in `jadard_common.h` working by
sourcing those symbols from Kconfig rather than forcing them in every build.
- Around line 4-6: The Makefile currently gates __JADARD_GKI__ on
TARGET_BUILD_VARIANT, which is Android-specific and undefined in normal Kbuild
flows. Replace this conditional in the jdchipset Makefile with a proper
Kconfig-controlled option or another kernel-supported build symbol, and use that
symbol to add the ccflags-y define so the flag is set consistently across all
builds.
In `@drivers/input/touchscreen/Kconfig`:
- Around line 514-525: The TOUCHSCREEN_JADARD_CHIPSET Kconfig entry is missing
the I2C dependency required by the driver’s bus communication. Update the
Kconfig stanza for TOUCHSCREEN_JADARD_CHIPSET to add a depends on I2C
constraint, matching the pattern used by other I2C touchscreen entries in this
file, and keep the existing help text and sourced jdchipset Kconfig unchanged.
In `@drivers/leds/leds-gpio.c`:
- Around line 157-165: The initial-on delay handling in the LED GPIO setup
should not affect LEDs using the KEEP default state. In the logic around
`template->default_state` and `led_dat->initial_on_delay_ms`, change the delay
gate so it only applies when the requested state comes from
`LEDS_GPIO_DEFSTATE_ON`, not when `LEDS_GPIO_DEFSTATE_KEEP` reads an
already-high GPIO. Keep the existing KEEP path in `leds_gpio`/`led_dat` intact
so hardware state is preserved immediately.
In `@drivers/net/phy/maxio.c`:
- Around line 120-130: The `maxio_mae0621a_resume` flow is mixing unchecked
register reads with error accumulation, so fix it by handling `PHY_READ(phydev,
MII_BMCR)` separately before calling `PHY_WRITE`, and bail out immediately if
the read fails. Then replace the `ret |=` pattern with normal sequential error
checking so `genphy_resume` and the BMCR write each return a real negative error
code from `maxio_mae0621a_resume` instead of a bitwise-combined value.
- Around line 156-216: Both phy_resolve_aneg_linkmode_maxio and
phy_resolve_link_compatibility_maxio are using maxio_read_paged results directly
in bit tests, so add explicit negative-return handling before reading
speed/duplex/link bits and bail out early on error. In
phy_resolve_aneg_linkmode_maxio, guard physr_p_a43 before updating
phydev->speed, duplex, pause, and asym_pause; in
phy_resolve_link_compatibility_maxio, guard iner/physr/insr before using
AUTONEG_COMPLETED_INT_EN, LINKOK, or AUTONEG_COMPLETED. Also add a phydev->priv
null check at the start of phy_resolve_link_compatibility_maxio before
dereferencing paras.
---
Minor comments:
In `@drivers/input/touchscreen/jdchipset/jadard_debug.c`:
- Around line 2149-2159: The `scnprintf` call in the `t` command path uses `len
- 2`, which can underflow when very short input like `"t"` is written to
`debug`. In `jadard_debug.c`, update the parsing logic around the `buf_tmp[0] ==
't'` branch to validate that `len` is large enough before subtracting, and only
copy the filename portion when the input actually contains the expected prefix
and payload. Use the existing `buf_tmp`, `len`, and `fileName` handling in
`jadard_debug`/the upgrade flow to keep the size argument safe.
In `@drivers/net/phy/maxio.c`:
- Around line 294-316: The probe path in maxio_mae0621a_probe uses a 100ms busy
wait, which is inappropriate here and should be replaced with a sleepable delay.
Update the post-setup wait after the MAXIO_PAGE_SELECT write to use the kernel
sleep API instead of mdelay(100), keeping the same 100ms timing but avoiding CPU
spinning in this probe routine.
---
Nitpick comments:
In `@drivers/input/touchscreen/jdchipset/jadard_common.h`:
- Around line 1-26: Remove the dead commented-out asm include from
jadard_common.h, and in the header include block for jadard_common.h verify
whether the touchscreen driver actually needs linux/gpio.h and
linux/buffer_head.h. Replace gpio usage with the appropriate modern GPIO header
path if needed, and drop buffer_head if it is not referenced anywhere else in
the jdchipset code.
In `@drivers/input/touchscreen/jdchipset/jadard_platform.h`:
- Around line 20-30: `JD_E()` and `JD_W()` are currently disabled in non-debug
builds because they expand to no-ops in the `#else` branch of
`jadard_platform.h`, which hides important probe/I2C/GPIO/regulator failures.
Update the logging macros so only `JD_I()` and `JD_D()` are gated by the
debug/esd flags, and keep `JD_W()` and `JD_E()` active by mapping them to
warning/error logging regardless of `CONFIG_TOUCHSCREEN_JADARD_DEBUG` in the
`JD_I`, `JD_W`, `JD_E`, `JD_D` macro definitions.
In `@drivers/input/touchscreen/jdchipset/Kconfig`:
- Around line 95-111: Rename the short Kconfig symbols `JD_DB`, `JD_FB`, and
`JD_HID` to use the same `TOUCHSCREEN_JADARD_` prefix as the rest of the Jadard
options, and update every consumer to match. In particular, adjust the `config`
entries in the Kconfig file and the symbol checks in `jadard_common.h` that
currently reference `CONFIG_JD_HID`, `CONFIG_JD_DB`, and `CONFIG_JD_FB`, as well
as the Makefile `obj-$(CONFIG_JD_HID)` usage. Keep the feature names and help
text unchanged, only align the symbol identifiers consistently across the
driver.
In `@drivers/net/ethernet/stmicro/stmmac/stmmac_main.c`:
- Around line 53-55: The MAXIO PHY ID constants are duplicated between
stmmac_main.c and maxio.c, so move them into a shared header and have both files
include it instead of defining their own copies. Use the existing
MAXIO_PHY_MAE0621A_Q2C_ID and MAXIO_PHY_MAE0621A_Q3C_ID symbols as the single
source of truth, either in a new shared header like include/linux/maxio_phy.h or
an existing PHY ID header, and update both stmmac_main.c and maxio.c to
reference those shared definitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e292ef78-08b0-4c32-b0db-b50488603000
📒 Files selected for processing (28)
arch/arm64/boot/dts/rockchip/Makefilearch/arm64/boot/dts/rockchip/overlay/Makefilearch/arm64/boot/dts/rockchip/overlay/rk3562-kickpi-k3b-sq101p-x4ei451-84h501.dtsarch/arm64/boot/dts/rockchip/rk3562-kickpi-k3b.dtsdrivers/gpu/drm/panfrost/panfrost_device.cdrivers/gpu/drm/panfrost/panfrost_device.hdrivers/input/touchscreen/Kconfigdrivers/input/touchscreen/Makefiledrivers/input/touchscreen/jdchipset/Kconfigdrivers/input/touchscreen/jdchipset/Makefiledrivers/input/touchscreen/jdchipset/jadard_common.cdrivers/input/touchscreen/jdchipset/jadard_common.hdrivers/input/touchscreen/jdchipset/jadard_debug.cdrivers/input/touchscreen/jdchipset/jadard_debug.hdrivers/input/touchscreen/jdchipset/jadard_ic_JD9366T.cdrivers/input/touchscreen/jdchipset/jadard_ic_JD9366T.hdrivers/input/touchscreen/jdchipset/jadard_module.cdrivers/input/touchscreen/jdchipset/jadard_module.hdrivers/input/touchscreen/jdchipset/jadard_platform.cdrivers/input/touchscreen/jdchipset/jadard_platform.hdrivers/input/touchscreen/jdchipset/jadard_sorting.cdrivers/input/touchscreen/jdchipset/jadard_sorting.hdrivers/leds/leds-gpio.cdrivers/net/ethernet/stmicro/stmmac/stmmac_main.cdrivers/net/phy/Kconfigdrivers/net/phy/Makefiledrivers/net/phy/maxio.cdrivers/net/phy/phy_device.c
|
In general this looks good but I'm not quite able to follow why the panfrost changes were needed as that GPU has been supported with Rockchip SoC for a while? |
The Mali GPU requires specific clocks to function properly. For the Panfrost driver, its clock request logic is handled as follows: // https://github.com/armbian/linux-rockchip/blob/rk-6.1-rkr5.1/drivers/gpu/drm/panfrost/panfrost_device.c
static int panfrost_clk_init(struct panfrost_device *pfdev)
{
int err;
unsigned long rate;
pfdev->clock = devm_clk_get(pfdev->dev, NULL);
if (IS_ERR(pfdev->clock)) {
dev_err(pfdev->dev, "get clock failed %ld\n", PTR_ERR(pfdev->clock));
return PTR_ERR(pfdev->clock);
}
rate = clk_get_rate(pfdev->clock);
dev_info(pfdev->dev, "clock rate = %lu\n", rate);
err = clk_prepare_enable(pfdev->clock);
if (err)
return err;
pfdev->bus_clock = devm_clk_get_optional(pfdev->dev, "bus");
if (IS_ERR(pfdev->bus_clock)) {
dev_err(pfdev->dev, "get bus_clock failed %ld\n",
PTR_ERR(pfdev->bus_clock));
err = PTR_ERR(pfdev->bus_clock);
goto disable_clock;
}
if (pfdev->bus_clock) {
rate = clk_get_rate(pfdev->bus_clock);
dev_info(pfdev->dev, "bus_clock rate = %lu\n", rate);
err = clk_prepare_enable(pfdev->bus_clock);
if (err)
goto disable_clock;
}
return 0;
disable_clock:
clk_disable_unprepare(pfdev->clock);
return err;
}As shown in the driver source, Panfrost requests the first clock defined in the DTS (regardless of its name), and then requests a second clock explicitly named "bus". For RK3568/RK3566 (rk356x.dtsi), the clocks in the GPU node are defined like this: This works perfectly out-of-the-box for RK3568/RK3566. All that's required to make Panfrost work there is converting the interrupt-names to lowercase ("gpu", "mmu", "job"), which is a Panfrost requirement. However, for RK3576, the GPU clocks are defined differently: Obviously, Panfrost cannot obtain the clk_gpu clock using this configuration. Proceeding with subsequent operations—such as accessing GPU registers without this clock enabled—leads to severe consequences, such as an SoC hang/freeze. Fortunately, @amazingfate used a clever commit to enable clk_gpu without modifying the Panfrost driver itself: by handing over clk_gpu to the power-domain to handle its request and enablement. This leaves Panfrost responsible only for requesting the first clock (which is likely the core clock that scales frequency up/down for DVFS). I initially attempted to mimic this approach for RK3562, but the Panfrost probe timed out and exited. For reference, here are the required GPU clocks on the RK3562: I plan to experiment further with modifying the DTS structure to properly map these clock requests. If that doesn't yield a solution, I will report back here. Realistically, adding a specific patch to Panfrost shouldn't impact other SoCs, though it might look a bit less elegant. @amazingfate, do you have any suggestions or insights on this? Any guidance would be highly appreciated! |
|
@HeyMeco I spent a long time today trying to achieve a successful Panfrost load on the RK3562 solely by modifying the DTS, but none of my attempts succeeded. Every probe ended up causing an SoC hang. Therefore, I believe modifying the Panfrost source code to enable these clocks is the only way forward. |
2 participants
This PR adds kernel-level support for the RK3562 SBC KICKPI K3B. It mainly accomplishes the following:
1.Adds the base DTS for KICKPI K3B.
2.Adds a JD9366-based DSI panel DTB overlay for KICKPI K3B. Since the KICKPI K3B only features a DSI display interface, using a DTBO to support custom panels is a practical approach for users. The screen DTSO provided here is configured for my own panel and can be used as a reference.
3.Adds the I2C touch driver for JD9366. This is the TDDI touch driver for the panel I used, with the source code originating from JADARD.
4.Adds Panfrost support for the RK3562 Mali-G52 GPU. If the clk_gpu, clk_gpu_brg, and aclk_gpu clocks (declared in rk3562.dtsi) are not enabled, the kernel will hang during Panfrost initialization:
5.Adds the driver for the MAXIO PHY used by KICKPI K3B.
6.Introduces the
initial-on-delay-msproperty for gpio-leds. This property forces a specified delay before turning on LEDs that are configured to be "on" during the boot process. This is introduced to resolve a specific hardware quirk on the KICKPI K3B:If the battery functionality is enabled in software but no physical battery is connected, powering the device via Type-C will cause it to hang during the boot process. To workaround this issue, KICKPI introduces a 10-second delay before enabling the USB Type-A power supply.