rockchip: cleanup and rewrite patches for current
#8912
Conversation
github-actions
bot
added
11
WalkthroughThis comprehensive kernel patch set adds Armbian customizations and enhancements for Rockchip SoCs (RK3228, RK3288, RK3328, RK3399) including new drivers (RK3228 audio codec, RK3288 gpiomem, ESP8089/SSV6051 WiFi), display improvements (hardware cursor, 10-bit YUV modes), video codec support (HEVC decoding), DDR frequency scaling, device-tree configuration tuning, and numerous hardware-specific fixes across USB, MMC, audio, and networking subsystems. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Application
participant V4L2 as V4L2 Core
participant rkvdec as rkvdec Driver
participant Variant as Variant Select
participant HW as Hardware (H.264/HEVC/VP9)
Note over User,HW: Multi-Codec Format Selection Flow (New)
User->>V4L2: enum_output_fmt()
V4L2->>rkvdec: rkvdec_enum_output_fmt()
rkvdec->>Variant: Check device->capabilities
alt HEVC Capable
Variant->>rkvdec: Include HEVC descriptors
else H.264 Only
Variant->>rkvdec: Include H.264 descriptors only
end
rkvdec->>V4L2: Return capability-filtered formats
V4L2->>User: Format list
Note over User,HW: Decoding with Reset Management (New)
User->>V4L2: Start streaming
V4L2->>rkvdec: rkvdec_device_run()
rkvdec->>HW: Check reset_mask
alt Reset Required
rkvdec->>rkvdec: rockchip_pmu_idle_request(true)
rkvdec->>HW: Toggle reset line
rkvdec->>rkvdec: rockchip_pmu_idle_request(false)
end
rkvdec->>HW: Start decoding
HW->>rkvdec: IRQ (job complete)
rkvdec->>V4L2: Job finished callback
V4L2->>User: Frame ready
sequenceDiagram
participant DRM as DRM Core
participant VOP as VOP Driver
participant Cursor as Cursor Plane
participant Primary as Primary Plane
participant HW as Hardware
Note over DRM,HW: Hardware Cursor Plane Support (New)
DRM->>VOP: vop_create_crtc()
alt VOP_FEATURE_SPECIAL_CURSOR_PLANE
VOP->>Cursor: Create cursor_plane with cursor_plane_helper_funcs
VOP->>Primary: Create primary_plane with plane_helper_funcs
else Legacy
VOP->>Primary: Create primary plane (old path)
end
Note over DRM,HW: Cursor Update Path
DRM->>Cursor: drm_plane_helper_update()
Cursor->>Cursor: vop_cursor_atomic_check()
Cursor->>Cursor: vop_cursor_atomic_update()
Cursor->>HW: Write hwc_size, position registers
HW->>HW: Hardware cursor rendered
Note over DRM,HW: Primary/Overlay Unaffected
DRM->>Primary: drm_plane_helper_update()
Primary->>Primary: plane_helper_funcs (unchanged)
sequenceDiagram
participant Boot as Bootloader/Kernel
participant ClkMgr as Clock Manager
participant DMC as DMC DEVFREQ
participant SIP as Rockchip SIP/ATF
participant DDR as DDR Memory
Note over Boot,DDR: RK3228 DDR Frequency Scaling (New)
Boot->>ClkMgr: Initialize clocks (SIP v2 path)
ClkMgr->>DMC: Register dmc devfreq device
DMC->>DMC: Set OPP table from DTS
Note over Boot,DDR: Load Monitoring
DMC->>DDR: Read DFI load statistics
DMC->>DMC: Compare load vs thresholds
alt High Load
DMC->>SIP: ROCKCHIP_SIP_CONFIG_DRAM_POST_SET_RATE (higher freq)
SIP->>DDR: Update DDR freq/voltage
else Low Load
DMC->>SIP: ROCKCHIP_SIP_CONFIG_DRAM_POST_SET_RATE (lower freq)
SIP->>DDR: Reduce DDR freq/voltage
end
DDR->>ClkMgr: Frequency changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Key areas requiring careful attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (27)
patch/kernel/archive/rockchip-6.12/patches.armbian/general-fix-reboot-from-kwiboo.patch (1)
60-66: Fix indentation to use tabs instead of spaces.The added lines use space indentation instead of tabs, violating kernel coding style requirements. The Linux kernel requires tabs for indentation (8-character width).
Apply this fix to use proper tab indentation:
- mmc_set_initial_signal_voltage(host); - - /* - * This delay should be sufficient to allow the power supply - * to reach the minimum voltage. - */ - mmc_delay(host->ios.power_delay_ms); + mmc_set_initial_signal_voltage(host); + + /* + * This delay should be sufficient to allow the power supply + * to reach the minimum voltage. + */ + mmc_delay(host->ios.power_delay_ms);patch/kernel/archive/rockchip-6.12/patches.armbian/general-add-restart-handler-for-act8846.patch (1)
59-65: Unregister the restart handler on driver teardown.You register
act8846_restart_handler, but the driver never callsunregister_restart_handler()in its remove/shutdown paths. If the device is unbound or the module is unloaded, the notifier will keep firing with a danglingact8865_i2c_client, leading to crashes on the next reboot attempt. The kernel reboot API expects register/unregister to be paired.(kernel.googlesource.com)Please mirror the registration with an unregister in
act8865_pmic_remove()(and clearact8865_i2c_client), e.g.:static int act8865_pmic_remove(struct i2c_client *client) { + if (type == ACT8846) + unregister_restart_handler(&act8846_restart_handler); + act8865_i2c_client = NULL; ... }patch/kernel/archive/rockchip-6.12/patches.armbian/general-rk322x-gpio-ir-driver.patch (1)
232-304: Fix PWM channel base handlingFor any device tree that sets
pwm-idto something other than 0, we still poke channel 0’s registers.rockchip_pwm_hw_init()/rockchip_pwm_hw_stop()never add the per-channel stride, so the capture logic never arms the PWM that Trust OS will look at. On top of that,rockchip_pwm_int_ctrl()subtracts(0x10 * pwm_id)from the ioremapped base; forpwm-id = 1this points before the mapped resource, so we end up reading/writing unmapped memory. In both cases the virtual power-off wake path simply can’t fire on boards wired to PWM1/2/3 (e.g. Tinkerboard).Please compute the per-channel base via the stride and use the original controller base for the common register, instead of subtracting the offset. A minimal fix would look like this:
@@ -static void rockchip_pwm_int_ctrl(struct rockchip_rc_dev *gpio_dev, bool enable) -{ - void __iomem *pwm_base = gpio_dev->pwm_base; - struct device *dev = &gpio_dev->rcdev->dev; - int pwm_id = gpio_dev->pwm_id; - void __iomem *reg_int_ctrl; +static void rockchip_pwm_int_ctrl(struct rockchip_rc_dev *gpio_dev, bool enable) +{ + void __iomem *pwm_base = gpio_dev->pwm_base; + struct device *dev = &gpio_dev->rcdev->dev; + void __iomem *reg_int_ctrl; int val; - reg_int_ctrl= pwm_base - (0x10 * pwm_id) + PWM_REG_INT_EN; + reg_int_ctrl = pwm_base + PWM_REG_INT_EN; @@ -static int rockchip_pwm_hw_init(struct rockchip_rc_dev *gpio_dev) -{ - void __iomem *pwm_base = gpio_dev->pwm_base; +static int rockchip_pwm_hw_init(struct rockchip_rc_dev *gpio_dev) +{ + void __iomem *chan_base = gpio_dev->pwm_base + gpio_dev->pwm_id * 0x10; @@ - val = readl_relaxed(pwm_base + PWM_REG_CTRL); + val = readl_relaxed(chan_base + PWM_REG_CTRL); @@ - rockchip_pwm_int_ctrl(gpio_dev, true); + rockchip_pwm_int_ctrl(gpio_dev, true); @@ - val = readl_relaxed(pwm_base + PWM_REG_CTRL); + val = readl_relaxed(chan_base + PWM_REG_CTRL);and apply the same
chan_baseadjustment insiderockchip_pwm_hw_stop(). This ensures we touch the correct PWM channel and stops us from accessing MMIO outside the mapped region.patch/kernel/archive/rockchip-6.12/patches.libreelec/linux-0011-v4l2-from-list.patch (1)
338-389: Fix valid_fmt when non-SPS controls arrive
rkvdec_valid_fmt()now delegates every control toops->valid_fmt(), butrkvdec_h264_valid_fmt()unconditionally readsctrl->p_new.p_h264_sps. For any other control (PPS, scaling matrix, etc.) the union holds different data, so we either dereference garbage or return 0. The subsequent comparison inrkvdec_try_ctrl()(ctx->valid_fmt && ctx->valid_fmt != rkvdec_valid_fmt(...)) will then reject those controls, breaking the decode pipeline, and in practice we can even trip UB by touching the wrong pointer. Mainline patches guard this helper withif (ctrl->id != V4L2_CID_STATELESS_H264_SPS) return 0;before accessing SPS-specific fields. Please add the same check (or similar logic inrkvdec_valid_fmt()) so that only SPS updates influence the negotiated format. (patchew.org)Also applies to: 500-548
patch/kernel/archive/rockchip-6.12/patches.libreelec/linux-1000-drm-rockchip.patch (1)
1207-1275: Restorevop_crtc_mode_validsymbol nameLine [1207]: this hunk renames the CRTC mode validator to
vop_crtc_mode_valid5, butvop_crtc_helper_funcslater in this file still binds.mode_valid = vop_crtc_mode_valid. We now reference a function that no longer exists and the driver fails to build. Please keep the original symbol name (or update every reference) so the object links again.patch/kernel/archive/rockchip-6.12/patches.armbian/general-rockchip-various-fixes.patch (4)
32-42: Fix typo in crypto controller node name.The node name contains a typo:
cypto-controllershould becrypto-controller.Apply this diff to fix the typo:
- crypto: cypto-controller@100a0000 { + crypto: crypto-controller@100a0000 { compatible = "rockchip,rk3288-crypto";
112-137: GPU node status implicitly changed from disabled to enabled.The original GPU node had
status = "disabled";which has been removed. By removing this line, the GPU is now implicitly enabled by default. This is a significant behavioral change that should be explicitly documented or reconsidered.If the GPU should remain disabled by default (common for device tree SoC dtsi files), restore the status property:
operating-points-v2 = <&gpu_opp_table>; #cooling-cells = <2>; /* min followed by max */ + status = "disabled"; };If enabling the GPU by default is intentional, please document this change in the commit message.
453-499: Add error handling for initial page read.The function reads the current page register at line 458 but doesn't check for read errors. If this read fails, the function will proceed with an invalid page value and fail to restore the correct page on exit.
Apply this diff to add error checking:
static int arc_emac_rtl8201f_phy_fixup(struct phy_device *phydev) { unsigned int reg, curr_pg; int err = 0; - curr_pg = phy_read(phydev, RTL_8201F_PG_SELECT_REG); + curr_pg = phy_read(phydev, RTL_8201F_PG_SELECT_REG); + if (curr_pg < 0) + return curr_pg; + err = phy_write(phydev, RTL_8201F_PG_SELECT_REG, 4);
70-78: Fix uart2 pinctrl reference from uart21_xfer back to uart2_xfer.The change from
uart2_xfertouart21_xferintroduces a broken reference.uart21_xferis not defined anywhere in the rockchip-6.12 patches. The originaluart2_xferexists in the base device tree and should be retained, or if an alternative pinmux is needed, verify that the target configuration is actually defined.@@ -350,7 +365,7 @@ uart2: serial@11030000 { clocks = <&cru SCLK_UART2>, <&cru PCLK_UART2>; clock-names = "baudclk", "apb_pclk"; pinctrl-names = "default"; - pinctrl-0 = <&uart2_xfer>; + pinctrl-0 = <&uart2_xfer>; reg-shift = <2>; reg-io-width = <4>; status = "disabled";patch/kernel/archive/rockchip-6.12/patches.armbian/wifi-driver-esp8089.patch (1)
1700-1707: Avoid unregistering inside the module init path
esp_sdio_init()registers the driver, sleeps for one second, unregisters it again, sleeps, and finally re-registers it. This “double probe” dance forcefully detaches every SDIO function on the bus (not just esp8089), introduces a 1 s boot delay, and opens races with any other SDIO driver that might already be bound. On systems with unrelated SDIO peripherals this will manifest as spurious disconnects or outright probe failures. Please restructure the firmware bootstrap so it happens within a single probe cycle—e.g. handle the two-phase load from withinesp_sdio_probe()or usemmc_sw_reset()where appropriate—rather than unregistering the entire driver from the init routine.patch/kernel/archive/rockchip-6.12/patches.libreelec/linux-2001-v4l2-wip-iep-driver.patch (3)
438-440: Fix the always-true field-select macro
IEP_DEIN_OUT_MODE_1FRM_TOP_FIELD()lacks a comparison for the second condition, so the expression is always true (non-zero constant). That forces the hardware into the “top field” path even when the mode should select a bottom field, breaking single-frame deinterlace output. Please comparemagainstIEP_DEIN_MODE_I2O1Texplicitly.-#define IEP_DEIN_OUT_MODE_1FRM_TOP_FIELD(m) (m == IEP_DEIN_MODE_I4O1T || IEP_DEIN_MODE_I2O1T \ +#define IEP_DEIN_OUT_MODE_1FRM_TOP_FIELD(m) ((m == IEP_DEIN_MODE_I4O1T) || \ + (m == IEP_DEIN_MODE_I2O1T) ? 1 : 0)
1368-1387: Abort probe when clock parsing failsWhen
iep_parse_dt()returns an error (e.g., faileddevm_clk_get()), the probe just logs it and keeps going. The driver then storesERR_PTR()s iniep->axi_clk/ahb_clk, and later runtime-pm callbacks will dereference those handles, oopsing the kernel. Please bail out on the failure path (returnretor jump to the cleanup label) before touching clocks, IRQs, or registering the device.ret = iep_parse_dt(iep); - if (ret) - dev_err(&pdev->dev, "Unable to parse OF data\n"); + if (ret) { + dev_err(&pdev->dev, "Unable to parse OF data (%d)\n", ret); + goto err_parse; + } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -err_put_clk: +err_put_clk: pm_runtime_dont_use_autosuspend(iep->dev); pm_runtime_disable(iep->dev); return ret; + +err_parse: + return ret;
1783-1796: Fix rk3288 IEP unit address typoThe rk3288 IEP node is spelled
iep@ff90000, but theregbase is0xff900000. DTC will flag this as a unit-address mismatch, and some tooling rejects it outright. Please rename the node toiep@ff900000(and likewise for itsiep_mmuchild if needed) so the unit address and register base align.patch/kernel/archive/rockchip-6.12/patches.armbian/mmc-tinkerboard-sdmmc-reboot-fix.patch (1)
95-101: Prevent UAF by using a devm-managed restart handler
register_restart_handler()is deprecated and, as used here, leaves a live notifier pointing atdataafter the platform device unbinds. Becausedataisdevm_kzalloc()’d, the devres core frees it on remove, so the next reboot path dereferences freed memory and can BUG — the exact pattern the kernel community has been phasing out in favour of devm-managed sys-off helpers. Please switch to a devm-backed registration (e.g.devm_register_restart_handler()ordevm_add_action_or_reset()aroundunregister_restart_handler()) and handle the error path so the handler isn’t left registered on failure. (lore-kernel.gnuweeb.org)+static void dw_mci_rockchip_unregister_restart(void *nb) +{ + unregister_restart_handler(nb); +} + ... - register_restart_handler(&data->reset_nb); + ret = register_restart_handler(&data->reset_nb); + if (ret) { + dev_err(&pdev->dev, "failed to register restart handler: %d\n", + ret); + return; + } + + if (devm_add_action_or_reset(&pdev->dev, + dw_mci_rockchip_unregister_restart, + &data->reset_nb)) + return;patch/kernel/archive/rockchip-6.12/patches.armbian/rk322x-dmc-driver-04-driver.patch (1)
906-946: Balance the devfreq-event enable/disable lifecycle.
devfreq_event_enable_edev()bumps the DFI driver’s usecount. Ifrk3228_dmc_init()orrk3228_devfreq_init()bails out, or when the driver is later removed, we never call the matchingdevfreq_event_disable_edev(). That leaves the event device permanently enabled, leaking the usecount and preventing clean reprobes/unloads. Please unwind the enable on every failure path and in.remove(). One way is to funnel the error exits through a shared label and only disable when the enable actually ran.@@ static int rk3228_dmc_probe(struct platform_device *pdev) - ret = rk3228_dmc_init(pdev, data); - if (ret) - return ret; + ret = rk3228_dmc_init(pdev, data); + if (ret) + goto err_disable_event; ret = rk3228_devfreq_init(data); if (ret) - return ret; + goto err_disable_event; @@ return 0; + +err_disable_event: + devfreq_event_disable_edev(data->edev); + return ret; } @@ static void rk3228_dmc_remove(struct platform_device *pdev) struct rk3228_dmc *rdev = dev_get_drvdata(&pdev->dev); + if (rdev->devfreq.devfreq) + devfreq_event_disable_edev(rdev->edev); + rk3228_dmc_sysfs_remove(&pdev->dev);patch/kernel/archive/rockchip-6.12/patches.armbian/driver-rk3288-gpiomem.patch (2)
430-446: Compile fix: pass module owner toclass_create()On 6.12 the helper macro still expects the owning module as the first parameter. Calling
class_create(DEVICE_NAME)won't compile. Please switch to the two-argument form so this driver builds.- rk3288_gpiomem_class = class_create(DEVICE_NAME); + rk3288_gpiomem_class = class_create(THIS_MODULE, DEVICE_NAME);
458-470: Avoid dereferencinginstafter it’s freed or never allocatedIf
kzalloc()fails,instis NULL and we crash in the error log. For later failures we freeinstthen still touch it, which is a UAF. Use the existingdevvariable for logging and clear the global pointer before returning.failed_get_resource: kfree(inst); inst = NULL; failed_inst_alloc: - dev_err(inst->dev, "could not load rk3288_gpiomem"); + dev_err(dev, "could not load rk3288_gpiomem");patch/kernel/archive/rockchip-6.12/patches.armbian/driver-tinkerboard-alc4040-codec.patch (2)
77-79: Fix code style: missing space afterifkeyword.The conditional statement is missing a space after the
ifkeyword, which violates standard C coding style.Apply this diff:
- if(USB_ID_VENDOR(chip->usb_id) == 0x0bda && + if (USB_ID_VENDOR(chip->usb_id) == 0x0bda && USB_ID_PRODUCT(chip->usb_id) == 0x481a) { strlcat(card->shortname, " OnBoard", sizeof(card->shortname)); }
81-81: Remove trailing whitespace.Line 81 contains trailing whitespace that should be removed to maintain code cleanliness.
patch/kernel/archive/rockchip-6.12/patches.armbian/drm-rk322x-plane-overlay.patch (1)
95-108: Window 1 is still wired to WIN0 registers
rk3228_win1_datais supposed to drive the second window (base 0x40), yet every register entry still dereferences the WIN0 register block. That means programming the overlay plane will clobber WIN0 instead of WIN1, so the "overlay" plane never comes up correctly. Please retarget these VOP_REG definitions to the WIN1 register set.static const struct vop_win_phy rk3228_win1_data = { - .enable = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 0), - .format = VOP_REG(RK3288_WIN0_CTRL0, 0x7, 1), - .rb_swap = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 12), - .act_info = VOP_REG(RK3288_WIN0_ACT_INFO, 0x1fff1fff, 0), - .dsp_info = VOP_REG(RK3288_WIN0_DSP_INFO, 0x0fff0fff, 0), - .dsp_st = VOP_REG(RK3288_WIN0_DSP_ST, 0x1fff1fff, 0), - .yrgb_mst = VOP_REG(RK3288_WIN0_YRGB_MST, 0xffffffff, 0), - .uv_mst = VOP_REG(RK3288_WIN0_CBR_MST, 0xffffffff, 0), - .yrgb_vir = VOP_REG(RK3288_WIN0_VIR, 0x3fff, 0), - .uv_vir = VOP_REG(RK3288_WIN0_VIR, 0x3fff, 16), - .src_alpha_ctl = VOP_REG(RK3288_WIN0_SRC_ALPHA_CTRL, 0xff, 0), - .dst_alpha_ctl = VOP_REG(RK3288_WIN0_DST_ALPHA_CTRL, 0xff, 0), - .channel = VOP_REG(RK3288_WIN0_CTRL2, 0xff, 0), + .enable = VOP_REG(RK3288_WIN1_CTRL0, 0x1, 0), + .format = VOP_REG(RK3288_WIN1_CTRL0, 0x7, 1), + .rb_swap = VOP_REG(RK3288_WIN1_CTRL0, 0x1, 12), + .act_info = VOP_REG(RK3288_WIN1_ACT_INFO, 0x1fff1fff, 0), + .dsp_info = VOP_REG(RK3288_WIN1_DSP_INFO, 0x0fff0fff, 0), + .dsp_st = VOP_REG(RK3288_WIN1_DSP_ST, 0x1fff1fff, 0), + .yrgb_mst = VOP_REG(RK3288_WIN1_YRGB_MST, 0xffffffff, 0), + .uv_mst = VOP_REG(RK3288_WIN1_CBR_MST, 0xffffffff, 0), + .yrgb_vir = VOP_REG(RK3288_WIN1_VIR, 0x3fff, 0), + .uv_vir = VOP_REG(RK3288_WIN1_VIR, 0x3fff, 16), + .src_alpha_ctl = VOP_REG(RK3288_WIN1_SRC_ALPHA_CTRL, 0xff, 0), + .dst_alpha_ctl = VOP_REG(RK3288_WIN1_DST_ALPHA_CTRL, 0xff, 0), + .channel = VOP_REG(RK3288_WIN1_CTRL2, 0xff, 0), };patch/kernel/archive/rockchip-6.12/patches.armbian/wifi-ath9k-no-bulk-EP3-EP4.patch (1)
124-131: Keep the URB context pointing atstruct hif_usb_rx_buf
ath9k_hif_usb_reg_in_cb()dereferences the completion context as astruct hif_usb_rx_buf. After this change we now submit the URB withskbinstead, so the callback will treat an sk_buff pointer as an rx_buf, corrupt memory, and eventually blow up. Please keep passingrx_bufas before.- ath9k_hif_usb_reg_in_cb, skb); + ath9k_hif_usb_reg_in_cb, rx_buf);patch/kernel/archive/rockchip-6.12/patches.armbian/driver-rk322x-audio-codec.patch (2)
146-155: Add the gpio consumer headerThe driver uses
struct gpio_descanddevm_gpiod_get_optional(), but<linux/gpio/consumer.h>isn’t included. Without it the build fails becausestruct gpio_descand the function prototypes are undefined.#include <linux/device.h> #include <linux/delay.h> #include <linux/of.h> #include <linux/of_gpio.h> +#include <linux/gpio/consumer.h>
584-592: Handle alldevm_clk_get()failuresOnly treating
-EPROBE_DEFERas an error leaves every other failure path returning anERR_PTR. We then feed that intoclk_prepare_enable(), which will explode the first time a board lacks “mclk” or it’s renamed. Please do a fullIS_ERR()check and propagate the error code.- rk3228->mclk = devm_clk_get(&pdev->dev, "mclk"); - if (PTR_ERR(rk3228->mclk) == -EPROBE_DEFER) - return -EPROBE_DEFER; + rk3228->mclk = devm_clk_get(&pdev->dev, "mclk"); + if (IS_ERR(rk3228->mclk)) + return PTR_ERR(rk3228->mclk);patch/kernel/archive/rockchip-6.12/patches.armbian/drm-rockchip-hardware-cursor.patch (2)
200-205: Fix the cursor size validation logic.The condition allows
crtc_w == 0to pass validation, but line 109 invop_cursor_atomic_updateperforms(new_state->crtc_w >> 5) - 1, which would underflow whencrtc_w == 0, resulting inhwc_size = 0xFFFFFFFF. Either reject zero-sized cursors here or handle the zero case explicitly in the update function.Apply this diff to reject zero-sized cursors:
- if (new_plane_state->crtc_w != 0 && - new_plane_state->crtc_w != 32 && + if (new_plane_state->crtc_w != 32 && new_plane_state->crtc_w != 64 && new_plane_state->crtc_w != 96 && new_plane_state->crtc_w != 128)
90-106: Consider unconditionally enabling alpha for cursor planes.The alpha blending configuration is only enabled when
win_index > 0, but dedicated hardware cursor planes typically require alpha compositing regardless of their index. Since cursors inherently need transparency support, consider enabling alpha unconditionally for cursor planes or verifying that cursor planes will never havewin_index == 0.Apply this diff to unconditionally enable alpha for cursor planes:
- if (fb->format->has_alpha && win_index > 0) { + if (fb->format->has_alpha) { VOP_WIN_SET(vop, win, dst_alpha_ctl, DST_FACTOR_M0(ALPHA_SRC_INVERSE));patch/kernel/archive/rockchip-6.12/patches.armbian/general-pl330-02-add-support-for-interleaved-transfers.patch (1)
137-214: Guardnumpbefore using it
xt->numpis a brand-new field instruct dma_interleaved_template; every existing caller will leave it at the default zero unless we update all sites. As written, we multiply and divide bynumpimmediately afterward (period_bytes = size * nump;andfor (i = 0; i < numf / nump; i++)), so any zero value triggers a divide-by-zero fault in the kernel and creates zero-length descriptors. Please establish a non-zero default (e.g., treat zero as 1) before the arithmetic so legacy users keep working.- nump = xt->nump; - numf = xt->numf; + nump = xt->nump ? xt->nump : 1; + numf = xt->numf; - if (flags & DMA_PREP_REPEAT && (!nump || (numf % nump))) + if (flags & DMA_PREP_REPEAT && (numf % nump)) return NULL; @@ - for (i = 0; i < numf / nump; i++) { + for (i = 0; i < numf / nump; i++) {patch/kernel/archive/rockchip-6.12/patches.libreelec/linux-1001-v4l2-rockchip.patch (1)
295-304: Make the reset array mandatory here.
devm_reset_control_array_get(&pdev->dev, false, true)treats the reset array as optional, so a DT missingresetswill hand backNULLand all subsequentreset_control_assert/deassertcalls quietly no-op. That contradicts the new binding whereresets/reset-namesare required, and it means the error-path hard reset never runs—exactly the situation this series is trying to fix. Please drop theoptional=true(or use the non-optional helper) so probe fails when the resets are absent. (android.googlesource.com)
🧹 Nitpick comments (21)
patch/kernel/archive/rockchip-6.12/patches.armbian/ir-keymap-rk322x-box.patch (1)
40-77: Optional: Consider removing empty comment and documenting F-key mappings.The empty comment block (lines 40-42) could be removed or filled with a brief description of the keymap. Additionally, the F1-F6 key mappings might benefit from a comment explaining their intended function on RK322x TV box remotes, as these are less self-explanatory than standard media keys.
patch/kernel/archive/rockchip-6.12/patches.armbian/wifi-driver-ssv6051.patch (1)
1-10: Consider the maintenance burden of large monolithic driver patches.This 49k-line patch adds the entire SSV6051 driver as a single change, making it difficult to review, bisect, or selectively apply. While this may be acceptable for archived/legacy kernels, consider whether:
- Splitting into logical modules (HCI, SDIO, MAC layer) would improve reviewability
- The driver could leverage upstream wireless drivers (e.g., cfg80211/mac80211) more directly
- This is the only way to support SSV6051 on Rockchip, or if alternatives exist
For an archive kernel (6.12), this approach may be justified if no upstream driver exists, but document the rationale.
patch/kernel/archive/rockchip-6.12/patches.armbian/ir-keymap-xt-q8l-v10.patch (2)
38-40: Remove the empty comment block.This multi-line comment contains no content and serves no purpose. Either add meaningful documentation about the keymap or remove the empty comment entirely.
Apply this diff to remove the empty comment:
-/* - -*/ - static struct rc_map_table xt_q8l_v10[] = {
42-78: Remove whitespace-only lines.Lines 43 and 78 appear to contain only whitespace characters (likely tabs). These serve no purpose and should be removed for cleaner code.
Apply this diff to clean up the whitespace:
static struct rc_map_table xt_q8l_v10[] = { - { 0xcc1d11, KEY_ENTER }, { 0xcc1d00, KEY_POWER }, ... { 0xcc1d0d, KEY_F4 }, - };patch/kernel/archive/rockchip-6.12/patches.armbian/general-dwc2-fix-wait-peripheral.patch (1)
18-19: Consider polling instead of fixed delay.The fixed 10ms sleep may not be sufficient across all platforms or conditions. Patch
general-dwc2-fix-wait-time.patchin this same PR uses a polling approach with timeout for a similar timing issue in host mode. For consistency and robustness, consider implementing a polling loop that checks a relevant status register rather than relying on a fixed delay.patch/kernel/archive/rockchip-6.12/patches.armbian/general-dwc2-fix-rk3288-reset-on-wake-quirk.patch (1)
14-19: Verify removal doesn't break RK3288 functionality.The patch removes
no_clock_gating = truefrom RK3288 parameters but provides no explanation for why this quirk is no longer needed. Thedwc2_set_rk_paramsfunction applies to all Rockchip platforms, so this change could have broader impact.Please verify:
- Has this been tested on RK3288 hardware?
- Why was this quirk originally needed and why is it safe to remove now?
- Does removing this break any existing functionality or is it fixing a specific issue?
Consider adding a commit message description explaining the rationale for this change.
patch/kernel/archive/rockchip-6.12/patches.armbian/general-dwc2-fix-wait-time.patch (1)
34-53: Polling logic is correct and well-implemented.The polling approach is appropriate: checking GOTGCTL_CONID_B every 20ms for up to 200ms total. The logic correctly waits for the bit to clear (transition from device mode to host mode).
Minor enhancement: consider including the register value in the error message for easier debugging:
if (count > 10) - dev_err(hsotg->dev, - "Waiting for Host Mode timed out"); + dev_err(hsotg->dev, + "Waiting for Host Mode timed out (GOTGCTL=0x%08x)", + dwc2_readl(hsotg, GOTGCTL));patch/kernel/archive/rockchip-6.12/patches.armbian/general-fix-reboot-from-kwiboo.patch (1)
1-48: Subject line doesn't match the actual change.The subject "Add simpler reboot code" is misleading. The patch modifies MMC power-off timing by adding voltage initialization and delays, not reboot-specific code. Consider updating the subject to reflect the actual changes, such as "mmc: Add voltage initialization and delay during power-off" or "mmc: Fix power-off timing for Rockchip platforms."
This would improve clarity for future maintenance and help others understand the patch's purpose without examining the diff.
patch/kernel/archive/rockchip-6.12/patches.armbian/clk-rk322x-composite-mmc-clk.patch (1)
1-8: Consider a more descriptive commit message.The subject line "better handle mmc/sdio clocks" is somewhat vague. The actual change consolidates clock definitions from a
COMPOSITE_NODIV + DIVpattern to a singleCOMPOSITE_DIV_OFFSETmacro. A more specific subject like "rk322x: consolidate MMC/SDIO clock definitions" would better describe the refactoring.patch/kernel/archive/rockchip-6.12/patches.armbian/general-rockchip-various-fixes.patch (8)
462-497: Consider using usleep_range instead of mdelay.The function uses
mdelay(10)multiple times, which is a busy-wait delay that blocks the CPU. Since this code runs during PHY initialization (not in atomic context),usleep_range()would be more appropriate and CPU-friendly.Replace all
mdelay(10)calls with:- mdelay(10); + usleep_range(10000, 11000);This allows the CPU to sleep during the delay rather than busy-waiting.
465-469: Simplify bitwise operation for clarity.The EEE disable operation uses multiple
~operators with&which is harder to read. This can be simplified using a single mask.Apply this diff to improve readability:
/* disable EEE */ reg = phy_read(phydev, RTL_8201F_PG4_EEE_REG); - reg &= ~RTL_8201F_PG4_EEE_RX_QUIET_EN & - ~RTL_8201F_PG4_EEE_TX_QUIET_EN & - ~RTL_8201F_PG4_EEE_NWAY_EN & - ~RTL_8201F_PG4_EEE_10M_CAP; + reg &= ~(RTL_8201F_PG4_EEE_RX_QUIET_EN | + RTL_8201F_PG4_EEE_TX_QUIET_EN | + RTL_8201F_PG4_EEE_NWAY_EN | + RTL_8201F_PG4_EEE_10M_CAP); err = phy_write(phydev, RTL_8201F_PG4_EEE_REG, reg);
486-491: Remove redundant 16-bit mask.Line 489 applies
& 0xffffto a PHY register read result, butphy_read()already returns a 16-bit value. This mask is redundant.Apply this diff:
if (!(reg & RTL_8201F_PG7_RMSR_CLK_DIR_IN)) { /* disable powersave if phy's clock output is used */ reg = phy_read(phydev, RTL_8201F_PG0_PSMR_REG); - reg &= ~RTL_8201F_PG0_PSMR_PWRSVE_EN & 0xffff; + reg &= ~RTL_8201F_PG0_PSMR_PWRSVE_EN; err = phy_write(phydev, RTL_8201F_PG0_PSMR_REG, reg); }
541-549: Simplify complex conditional logic for readability.The nested conditional at lines 542-548 is difficult to parse. The logic combines phy_flag checks, tmdsclock thresholds, and version checks in a single compound condition.
Consider refactoring for clarity:
for (; cfg->tmdsclock != 0; cfg++) { - if (((!inno->phy_flag || tmdsclock > 33750000) - && tmdsclock <= cfg->tmdsclock - && cfg->version & inno->chip_version) || - (inno->phy_flag && tmdsclock <= 33750000 - && cfg->version & 4)) + bool low_freq_special = (inno->phy_flag && + tmdsclock <= 33750000 && + cfg->version & 4); + bool normal_match = ((!inno->phy_flag || tmdsclock > 33750000) && + tmdsclock <= cfg->tmdsclock && + cfg->version & inno->chip_version); + + if (low_freq_special || normal_match) break; + }
542-548: Replace magic number with named constant.Line 547 uses the magic number
4for a version check. This should be defined as a named constant for clarity.Define a constant near the chip version definitions and use it:
#define INNO_HDMI_PHY_VERSION_RK3228_LOW_FREQ 4Then use it in the conditional:
- (inno->phy_flag && tmdsclock <= 33750000 - && cfg->version & 4)) + (inno->phy_flag && tmdsclock <= 33750000 && + cfg->version & INNO_HDMI_PHY_VERSION_RK3228_LOW_FREQ))
588-588: Consider reducing log level for phy_flag information.The phy_flag value is logged at
dev_infolevel, which will always appear in kernel logs. Unless this information is critical for debugging production issues, consider usingdev_dbginstead to reduce log noise.- dev_info(inno->dev, "phy_flag is: %d\n", inno->phy_flag); + dev_dbg(inno->dev, "phy_flag is: %d\n", inno->phy_flag);
665-666: Remove extra blank line.There's an unnecessary extra blank line at line 666.
Apply this diff:
if (!parent_pd->is_ignore_pwr) child_pd->is_ignore_pwr = false; - rockchip_pm_add_subdomain(pmu, np);
1-672: Consider splitting this monolithic patch for better maintainability.This patch combines changes across 5 distinct subsystems:
- Device tree updates (crypto, MMC, pinctrl, GPU OPP)
- Clock tree restructuring
- Ethernet PHY fixup for RTL8201F
- HDMI PHY efuse flag handling
- PM domain ignore-power mechanism
While I understand from the learnings that Armbian prioritizes patch consistency, combining unrelated changes makes it difficult to:
- Review each change on its technical merits
- Debug hardware-specific issues (e.g., MMC timing vs PHY problems)
- Selectively revert if one change causes issues
- Understand the git history
Consider splitting into logical patches:
rockchip-rk3228-clock-tree-rework.patch- Clock driver changesrockchip-arc-emac-rtl8201f-phy-fixup.patch- Ethernet PHY supportrockchip-inno-hdmi-phy-efuse-flag.patch- HDMI PHY enhancementsrockchip-pm-domain-ignore-power.patch- PM domain improvementsrockchip-rk322x-dt-updates.patch- Device tree updatesThis would align with standard kernel development practices while maintaining Armbian's consistency goals within each logical subsystem.
patch/kernel/archive/rockchip-6.12/patches.armbian/bt-broadcom-serdev-workaround.patch (1)
19-20: Consider C-style comment for kernel code consistency.While C++ style comments (
//) are supported in modern kernel code, C-style comments (/* */) are more traditional for kernel patches. Consider changing line 20 to use C-style commenting:- //set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); + /* set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); */This is a minor style consistency suggestion and not a functional issue.
patch/kernel/archive/rockchip-6.12/patches.armbian/mmc-tinkerboard-sdmmc-reboot-fix.patch (1)
65-78: Drop stray printk noise
printk(KERN_ERR "Meow.\n");/printk(KERN_ERR "woeM.\n");look like debug leftovers and will spam logs on every reboot. Please replace them with meaningful messages or remove them entirely.patch/kernel/archive/rockchip-6.12/patches.armbian/drm-rockchip-hardware-cursor.patch (2)
20-59: Remove unnecessary blank lines.Lines 23 and 59 contain unnecessary blank lines that don't improve readability.
Apply this diff:
static void vop_cursor_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) { - struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);And:
obj = fb->obj[0]; rk_obj = to_rockchip_obj(obj); - - dsp_stx = dest->x1 + crtc->mode.htotal - crtc->mode.hsync_start;
260-261: Remove unnecessary blank line.The blank line before the
hwc_sizefield is unnecessary.Apply this diff:
struct vop_reg channel; - struct vop_reg hwc_size;
| ddc-i2c-bus = <&i2c5>; | ||
| status = "okay"; | ||
| pinctrl-names = "default"; | ||
| -- |
There was a problem hiding this comment.
Do not drop the HDMI status override.
Removing status = "okay"; reverts &hdmi to its SoC-default (disabled in rk3288.dtsi), which will kill HDMI output on MiQi. Keep the status property alongside the new pinctrl addition.
&hdmi {
ddc-i2c-bus = <&i2c5>;
+ status = "okay";
pinctrl-names = "default";
};📝 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.
| ddc-i2c-bus = <&i2c5>; | |
| status = "okay"; | |
| pinctrl-names = "default"; | |
| -- | |
| &hdmi { | |
| ddc-i2c-bus = <&i2c5>; | |
| status = "okay"; | |
| pinctrl-names = "default"; | |
| }; |
🤖 Prompt for AI Agents
In patch/kernel/archive/rockchip-6.12/patches.armbian/dts-miqi-mali-gpu.patch
around lines 31 to 33, the change removed the HDMI node's explicit status
property which causes the node to fall back to the SoC default (disabled) and
disable HDMI on MiQi; restore a explicit status = "okay"; line alongside the new
ddc-i2c-bus / pinctrl-names additions so the &hdmi node remains enabled while
keeping the new pinctrl configuration.
| diff --git a/drivers/net/wireless/ssv6051/Makefile.bak b/drivers/net/wireless/ssv6051/Makefile.bak | ||
| new file mode 100644 | ||
| index 000000000000..2733fa4dd3b7 | ||
| index 000000000000..111111111111 | ||
| --- /dev/null | ||
| +++ b/drivers/net/wireless/ssv6051/Makefile.bak | ||
| @@ -0,0 +1,107 @@ |
There was a problem hiding this comment.
Remove Makefile.bak—backup files should not be committed to version control.
The patch includes Makefile.bak (hunk 5), which is a backup artifact. Remove this file before merging.
-diff --git a/drivers/net/wireless/ssv6051/Makefile.bak b/drivers/net/wireless/ssv6051/Makefile.bak
-new file mode 100644
-index 000000000000..111111111111
---- /dev/null
-+++ b/drivers/net/wireless/ssv6051/Makefile.bak📝 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.
| diff --git a/drivers/net/wireless/ssv6051/Makefile.bak b/drivers/net/wireless/ssv6051/Makefile.bak | |
| new file mode 100644 | |
| index 000000000000..2733fa4dd3b7 | |
| index 000000000000..111111111111 | |
| --- /dev/null | |
| +++ b/drivers/net/wireless/ssv6051/Makefile.bak | |
| @@ -0,0 +1,107 @@ |
🤖 Prompt for AI Agents
In patch/kernel/archive/rockchip-6.12/patches.armbian/wifi-driver-ssv6051.patch
around lines 142-147, the patch adds a backup file
drivers/net/wireless/ssv6051/Makefile.bak which should not be committed; remove
the Makefile.bak entry from the patch (or delete the file and regenerate the
patch) so the backup is excluded and re-create the patch without that file
included.
1 participant
Description
Same story as #8911
How Has This Been Tested?
currentChecklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements