rockchip/uefi-loong64 6.19: rewrite patches against 6.19-rc4 #9172
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request adds comprehensive Rockchip kernel 6.19 architecture support across multiple domains: new RK3228/RK3288 codec and GPIO drivers, RK322x DFI/DMC support, DRM/VOP hardware cursor, color format and HDMI 2.1 FRL capabilities, RKVDEC video decoder refactoring with CABAC and H.264/HEVC common code extraction, clock management updates, PHY enhancements, and extensive RK3588/RK3576 device-tree configuration for audio, video, PCIe, NPU, and power management. Changes
Sequence Diagram(s)sequenceDiagram
participant App as User Application<br/>/dev/gpiomem
participant CharDev as RK3288 GPIO Char<br/>Driver
participant MMU as Memory<br/>Management
participant GPIO as GPIO<br/>Hardware Registers
App->>CharDev: open("/dev/gpiomem")
activate CharDev
CharDev->>CharDev: verify allowed address
CharDev->>MMU: mmap mapping
activate MMU
MMU->>GPIO: map physical address
deactivate MMU
CharDev-->>App: mmap handle
deactivate CharDev
App->>GPIO: read/write via mmap
GPIO-->>App: register values
sequenceDiagram
participant PHY as Rockchip PHY<br/>(rk_hdptx)
participant Bridge as DW HDMI-QP<br/>Bridge
participant Sink as HDMI Sink<br/>(Display)
participant PLL as PLL Config<br/>Module
Bridge->>PHY: set_mode(FRL)
activate PHY
PHY->>PLL: select FRL LCPLL config
activate PLL
PLL-->>PHY: PLL parameters
deactivate PLL
PHY->>Bridge: rate computed from PLL
deactivate PHY
Bridge->>Bridge: verify FRL support
alt FRL Supported
Bridge->>PHY: force_link_rate(FRL_9G_4L)
activate PHY
PHY->>Bridge: link_cfg updated
deactivate PHY
Bridge->>Sink: SCDC FRL config
Sink-->>Bridge: FRL LTS training status
else TMDS Fallback
Bridge->>PHY: set_mode(TMDS)
activate PHY
PHY->>PLL: select ROPLL TMDS config
deactivate PHY
end
sequenceDiagram
participant Encoder as CRTC<br/>Atomic Encoder
participant ConnState as Connector<br/>State
participant DRM as DRM Framework
participant Bridge as HDMI<br/>Bridge
participant PHY as PHY Driver
Encoder->>ConnState: read color_format property
activate ConnState
ConnState-->>Encoder: color_format enum
deactivate ConnState
Encoder->>DRM: convert to hdmi_colorspace
activate DRM
DRM-->>Encoder: HDMI colorspace
deactivate DRM
Encoder->>Bridge: apply color format
activate Bridge
Bridge->>Bridge: filter bus_formats by color_fmt
Bridge->>PHY: set PHY colorimetry
activate PHY
PHY->>PHY: update BCSH/DSP_BG registers
deactivate PHY
Bridge-->>Encoder: format applied
deactivate Bridge
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
02
github-actions
bot
added
Needs review
|
@paolosabatino I've just merged #9067 which included a bump to 6.19-rc4. As courtesy here's a rewrite against that version. If you rather handle it yourself let me know and I'll drop it.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (34)
patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0134-FROMLIST-v1-pmdomain-rockchip-Fix-init-genpd-as-GENP.patch (1)
31-34: Handle error codes fromregulator_is_enabled()explicitly.The
regulator_is_enabled()function returns anint(1 for enabled, 0 for disabled, < 0 for errors), not a boolean. Negative error codes will be implicitly converted totrue, causing the function to incorrectly report that the regulator is enabled when an error occurred.This could prevent proper power-off of the domain when regulator state cannot be determined.
🔎 Proposed fix to handle error codes correctly
static bool rockchip_pd_regulator_is_enabled(struct rockchip_pm_domain *pd) { - return IS_ERR_OR_NULL(pd->supply) ? false : regulator_is_enabled(pd->supply); + int ret; + + if (IS_ERR_OR_NULL(pd->supply)) + return false; + + ret = regulator_is_enabled(pd->supply); + return ret > 0; }patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0142-WIP-YUV420-drm-rockchip-vop2-Add-YUV420-output-forma.patch (1)
36-41: Minor typo in TODO comment.Line 40: "elsewere" should be "elsewhere".
patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0143-WIP-YUV420-drm-rockchip-dw_hdmi_qp-Add-YUV420-output.patch (1)
85-89: Dead code:supported_formatsis assigned twice and first value is immediately overwritten.Lines 85-86 set
plat_data.supported_formatsto include YUV420, but line 89 immediately overwrites it withdrm_to_hdmi_fmts(supported_colorformats). The YUV420 format added on line 86 is discarded.This appears to be a merge/rebase artifact. Either remove lines 85-86 if
supported_colorformatsalready includes YUV420, or remove line 89 if the manual assignment is intended.#!/bin/bash # Check what supported_colorformats contains in the base code rg -n "supported_colorformats" --type=c -C 5patch/kernel/archive/rockchip-6.19/patches.armbian/wifi-driver-esp8089-02.patch (2)
63-365: Critical: Remove the.origbackup file from the patch.The patch includes a complete
.origfile (297 lines), which is a temporary backup created by patch tools during the rewrite process. These files should never be committed to the repository.This suggests the patch rewrite process (
REWRITE_PATCHES=yes) encountered conflicts or wasn't properly cleaned up afterward.🔎 Required fix
Remove the entire section that adds
esp_debug.c.orig:-diff --git a/drivers/net/wireless/esp8089/esp_debug.c.orig b/drivers/net/wireless/esp8089/esp_debug.c.orig -new file mode 100644 -index 000000000000..111111111111 ---- /dev/null -+++ b/drivers/net/wireless/esp8089/esp_debug.c.orig -@@ -0,0 +1,297 @@ -[... entire 297-line file content ...]After removing this section, regenerate the patch using:
git format-patch -1 HEADBased on learnings, the rewrite-patches tool can leave behind artifacts when the git base revision has issues.
366-413: Critical: Remove the.rejfile from the patch.The patch intentionally adds
esp_debug.c.rejto the source tree. A.rejfile is an artifact created when patches fail to apply cleanly and should never be committed to version control. Including it indicates that prior patches failed to apply, which is incorrect state to commit.Remove the entire section adding
esp_debug.c.rej(lines 366-413):Patch section to remove
-diff --git a/drivers/net/wireless/esp8089/esp_debug.c.rej b/drivers/net/wireless/esp8089/esp_debug.c.rej -new file mode 100644 -index 000000000000..111111111111 ---- /dev/null -+++ b/drivers/net/wireless/esp8089/esp_debug.c.rej -@@ -0,0 +1,42 @@ -+--- drivers/net/wireless/esp8089/esp_debug.c -+++ drivers/net/wireless/esp8089/esp_debug.c -+@@ -186,31 +186,27 @@ void esp_debugfs_exit(void) -[...entire reject file content...]This same issue exists in the rockchip-6.18 patch version as well—apply the fix consistently across versions.
patch/kernel/archive/rockchip-6.19/patches.armbian/general-rockchip-various-fixes.patch (4)
32-41: Typo in device tree node name:cypto-controllershould becrypto-controller.The node name has a typo that could cause confusion when searching for or referencing this node.
Suggested fix
- crypto: cypto-controller@100a0000 { + crypto: crypto-controller@100a0000 {
454-461: Missing error check on initialphy_readforcurr_pg.If
phy_readfails and returns a negative error code, that value will later be passed tophy_writewhen restoring the page register. Consider checking the return value.Suggested fix
static int arc_emac_rtl8201f_phy_fixup(struct phy_device *phydev) { - unsigned int reg, curr_pg; + int reg, curr_pg; int err = 0; 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);
463-470: Potential issue:phy_readreturn value not checked before use.The
phy_readat line 465 can return a negative error code if the read fails. Using a negative value in the bitmask operations could produce unexpected results.Suggested fix
/* disable EEE */ reg = phy_read(phydev, RTL_8201F_PG4_EEE_REG); + if (reg < 0) { + err = reg; + goto out_err; + } reg &= ~RTL_8201F_PG4_EEE_RX_QUIET_EN &
480-490: Missing error check onphy_readfor RMSR and PSMR registers.Lines 480 and 487 read PHY registers without checking for errors before using the values.
patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0152-WIP-arm64-dts-rockchip-add-missing-UFS-regulators.patch (1)
21-23: Regulator naming mismatch—the patch references regulators that do not exist.The patch references
vcc_1v2_ufs_vccq_s0andvcc_1v8_ufs_vccq2_s0, but these regulators are not defined anywhere. Similar regulators with different names exist in rk3576 DTS files:vcc_1v2_ufs_s0andvcc_1v8_ufs_s0. Either correct the regulator names to match existing definitions or add the missing regulator definitions to the device tree. Note thatvcc_3v3_s0is properly defined.patch/kernel/archive/uefi-loong64-6.19/0004-drm-xe-use-4KiB-alignment-for-cursor-jumps.patch (1)
115-115: Patch uses undefined macro XE_PAGE_SIZE instead of SZ_4K.The patch description explicitly states to "use
SZ_4K'" for 4KiB alignment, but line 655 of the diff usesXE_PAGE_SIZEinstead. Web search confirms there is no stable upstream macro namedXE_PAGE_SIZEin the Linux kernel xe driver. The patch either needs to defineXE_PAGE_SIZEbefore use or change the implementation to use the correct kernel macroSZ_4K` (4096 bytes) as stated in the commit message. This inconsistency between the documented intent and actual implementation is a defect.patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0069-FROMLIST-v7-media-visl-Add-HEVC-short-and-long-term-.patch (3)
76-122: Verify that prerequisite patches (0067 and 0068) define the V4L2 HEVC extended SPS structures and control IDs.The patch depends on two prerequisite patches in the same series:
rockchip-0067-FROMLIST-v7-media-uapi-HEVC-Add-v4l2_ctrl_hevc_ext_s...rockchip-0068-FROMLIST-v7-media-v4l2-ctrls-Add-hevc_ext_sps_-ls-t_...These should define the required structures and constants. However, the trace code has a critical issue:
TP_fast_assign(__entry->lt = *lt)andTP_fast_assign(__entry->st = *st)dereference pointers without NULL checks. Ifvisl_find_control_data()returns NULL (when the control is not provided), the kernel will crash. Add NULL checks invisl-dec.cbefore calling the trace functions, or guard the dereferences in the trace macro.
49-49: Use angle brackets for system header includes.The include statements use quotes instead of angle brackets. Kernel headers should use angle brackets:
-#include "linux/v4l2-controls.h" +#include <linux/v4l2-controls.h>This applies to both locations: line 49 in visl-dec.h and line 68 in visl-trace-hevc.h. All other linux/* headers throughout the codebase consistently use angle brackets.
24-25: Add NULL pointer checks before tracing RPS controls.The trace functions dereference pointers that come from
visl_find_control_data(), which returns NULL if the control is not found. Since these controls are marked asV4L2_CTRL_FLAG_DYNAMIC_ARRAY(optional), add NULL checks before lines 24-25:+ if (run->hevc.rps_lt) + trace_v4l2_ctrl_hevc_ext_sps_lt_rps(run->hevc.rps_lt); + if (run->hevc.rps_st) + trace_v4l2_ctrl_hevc_ext_sps_st_rps(run->hevc.rps_st);Alternatively, document if these controls are guaranteed to be mandatory at runtime.
patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0111-FROMLIST-v4-drm-Add-new-general-DRM-property-color-f.patch (3)
112-116: Fix documentation parameter name.The doc comment references
@colorspacebut the actual parameter iscolor_fmt.Proposed fix
/** * drm_get_color_format_name - return a string for color format - * @colorspace: color format to compute name of + * @color_fmt: color format to compute name of * */
165-175: Fix typos in error messages.Two issues in the error messages:
- Line 166: "provded" → "provided"
- Line 172: "provded" → "provided" and "colorspace" → "color format"
Proposed fix
if (!supported_color_formats) { - drm_err(dev, "No supported color formats provded on [CONNECTOR:%d:%s]\n", + drm_err(dev, "No supported color formats provided on [CONNECTOR:%d:%s]\n", connector->base.id, connector->name); return -EINVAL; } if ((supported_color_formats & -BIT(DRM_MODE_COLOR_FORMAT_COUNT)) != 0) { - drm_err(dev, "Unknown colorspace provded on [CONNECTOR:%d:%s]\n", + drm_err(dev, "Unknown color format provided on [CONNECTOR:%d:%s]\n", connector->base.id, connector->name); return -EINVAL; }
266-274: Add NULL check before attaching property.If
drm_connector_attach_color_format_propertyis called beforedrm_mode_create_*_color_format_property,propwill be NULL, causing a NULL pointer dereference indrm_object_attach_property.Proposed fix
int drm_connector_attach_color_format_property(struct drm_connector *connector) { struct drm_property *prop = connector->color_format_property; + if (!prop) + return -EINVAL; + drm_object_attach_property(&connector->base, prop, DRM_COLOR_FORMAT_AUTO); return 0; }patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0141-WIP-SCRAMB-drm-bridge-dw-hdmi-qp-Add-high-TMDS-clock.patch (3)
93-111: Blocking sleep in atomic enable path.Line 110 calls
msleep(100)withindw_hdmi_qp_enable_scramb(), which is invoked fromdw_hdmi_qp_bridge_atomic_enable()(line 160). Blocking for 100ms in the atomic commit path can cause frame timing issues, atomic commit timeouts, and poor user experience.Consider using a non-blocking approach, such as scheduling delayed work to verify scrambling status after the required delay, rather than blocking the atomic commit.
239-295: Clean up commented-out code and resolve TODOs before merging.The detect function contains multiple unresolved items:
- Line 258: TODO about alternative EDID reading methods
- Lines 271-273: Commented-out scrambling check condition
- Line 289: FIXME about disabling output before scrambling setup
- Line 292: Commented-out
dw_hdmi_qp_set_scramb()callThese indicate incomplete implementation. Since this is a WIP patch, these need to be resolved, and the commented-out code should either be implemented or removed before the patch is ready for mainline.
Do you want me to help identify which commented-out code blocks should be enabled or removed based on the intended functionality?
307-327: Critical API mismatch:detect_ctxdoes not exist in kernel 6.19.The patch attempts to use
.detect_ctxcallback, but kernel 6.19 only supports the.detectcallback with signature:enum drm_connector_status (*detect)(struct drm_bridge *bridge);The
detect_ctxAPI withstruct drm_modeset_acquire_ctx *ctxparameter does not exist in kernel 6.19 and will cause compilation failure. The function signature change fromenum drm_connector_statusreturn type tointand parameter change fromstruct drm_connector *connectortostruct drm_modeset_acquire_ctx *ctxappears to be from a newer kernel version. Either the patch targets the wrong kernel version, or this API change must be adapted to match kernel 6.19'sdetectcallback interface.patch/kernel/archive/rockchip-6.19/patches.armbian/driver-tinkerboard-alc4040-codec.patch (1)
79-79: Replacestrlcatwith a safer string concatenation approach for kernel 6.19.
strlcpy/strlcatare deprecated in kernel 6.19 and should not be used in new code (they can over-read). For this string concatenation, usescnprintfor manually manage concatenation withstrscpyusing pointer arithmetic to advance the buffer position.patch/kernel/archive/rockchip-6.19/patches.armbian/mmc-tinkerboard-sdmmc-reboot-fix.patch (2)
65-77: Remove debug printk statements and add error handling.The
printk(KERN_ERR "Meow.\n")andprintk(KERN_ERR "woeM.\n")are debug artifacts that should not appear in production kernel code.Additionally,
regulator_enable()andregulator_set_voltage()return values are not checked, which could mask failures during the critical reboot sequence.🔎 Proposed fix
- printk(KERN_ERR "Meow.\n"); - mmc_power_off(mmc); mdelay(20); - if (!IS_ERR(mmc->supply.vmmc)) - regulator_enable(mmc->supply.vmmc); + if (!IS_ERR(mmc->supply.vmmc)) { + int ret = regulator_enable(mmc->supply.vmmc); + if (ret) + dev_warn(&data->pdev->dev, "vmmc enable failed: %d\n", ret); + } - if (!IS_ERR(mmc->supply.vqmmc)) - regulator_set_voltage(mmc->supply.vqmmc, 3000000, 3300000); - - printk(KERN_ERR "woeM.\n"); + if (!IS_ERR(mmc->supply.vqmmc)) { + int ret = regulator_set_voltage(mmc->supply.vqmmc, 3000000, 3300000); + if (ret) + dev_warn(&data->pdev->dev, "vqmmc set voltage failed: %d\n", ret); + } return NOTIFY_DONE;
95-100: Race condition:pdevassigned after handler registration.If a reboot is triggered immediately after
register_restart_handler()returns but beforedata->pdev = pdevexecutes, the notifier callback will access an uninitializeddata->pdev, causing a NULL pointer dereference.🔎 Proposed fix
data->reset_nb.notifier_call = dw_mci_rockchip_broken_boards_reset_nb; data->reset_nb.priority = 255; + data->pdev = pdev; register_restart_handler(&data->reset_nb); - - data->pdev = pdev; }patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0060-FROMLIST-v4-phy-rockchip-samsung-hdptx-Compute-clk-r.patch (1)
86-98: Add validation guards against division by zero in PLL rate calculation.Two division operations lack protection against zero denominators:
Line 90: The denominator
(ropoll_hw.sdc_deno * ropoll_hw.sdc_n - ropoll_hw.sdc_num)could be zero if hardware registers contain values wheresdc_deno * sdc_n == sdc_num. While this may represent an invalid PLL configuration, it should be defensively validated rather than triggering a kernel divide-by-zero fault.Line 119: The division by
hdptx->hdmi_cfg.bpcalso lacks protection. Ifbpcis zero, this would similarly cause a divide-by-zero fault.Consider adding boundary checks before both divisions to return an error or default value when denominators would be zero.
patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0110-FROMLIST-v4-drm-amd-display-Remove-unnecessary-SIGNA.patch (1)
1-46: Critical: AMD display patch misplaced in Rockchip directory.This patch modifies
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, which is AMD-specific code, but it's located in thepatch/kernel/archive/rockchip-6.19/patches.libreelec/directory. This appears to be a file placement error during the patch rewrite process.Please verify:
- Should this patch be in a different patch directory (e.g., uefi-x86 or a generic kernel patches directory)?
- Or should this patch be removed from rockchip-6.19 entirely?
patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0117-FROMLIST-v4-drm-rockchip-Implement-color-format-DRM-.patch (2)
27-28: Critical: Inverted logic prevents property attachment.The condition
if (!drm_mode_create_hdmi_color_format_property(...))is inverted. DRM functions typically return 0 on success, so!0evaluates to false, preventing the property from being attached when it should be.🔎 Proposed fix
- if (!drm_mode_create_hdmi_color_format_property(connector, supported_colorformats)) + if (drm_mode_create_hdmi_color_format_property(connector, supported_colorformats) == 0) drm_connector_attach_color_format_property(connector);Or more idiomatically:
- if (!drm_mode_create_hdmi_color_format_property(connector, supported_colorformats)) - drm_connector_attach_color_format_property(connector); + ret = drm_mode_create_hdmi_color_format_property(connector, supported_colorformats); + if (ret == 0) + drm_connector_attach_color_format_property(connector);
76-78: Critical: Duplicate BT2020 colorspace check.Lines 76-78 duplicate the BT2020 colorspace handling already performed in the switch statement above (lines 66-69). This appears to be a copy-paste error or merge conflict artifact.
🔎 Proposed fix
Remove the duplicate check:
default: break; } - if (colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB || - colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC) - val |= BIT(6); } vop2_vp_write(vp, RK3568_VP_BCSH_CTRL, val);patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0139-WIP-SCRAMB-drm-bridge-Add-detect_ctx-hook.patch (1)
41-46: Complete TODO and fix typo before merging WIP patch.This WIP patch has two issues:
- Line 41: TODO comment indicates incomplete documentation for the
detect_ctxvariant- Line 44: Typo: "detect_cts" should be "detect_ctx"
🔎 Proposed fix for typo
//TODO: document variant used by bridge_connector framework // When ctx == NULL, detect_ctx should not return < 0 and behaves // exactly like ->detect() above. - // When both detect_cts and detect are defined, the latter is ignored. + // When both detect_ctx and detect are defined, the latter is ignored. int (*detect_ctx)(struct drm_bridge *bridge, struct drm_modeset_acquire_ctx *ctx);patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0121-FROMLIST-v1-pmdomain-rockchip-quiet-regulator-error-.patch (1)
26-29: Redundant error code in format string.
dev_err_probe()already appends the error code to the message, so the explicit%dandretargument will cause the error code to appear twice in the output.🔎 Suggested fix
- dev_err_probe(pd->pmu->dev, ret, - "Failed to enable supply: %d\n", ret); + dev_err_probe(pd->pmu->dev, ret, + "Failed to enable supply\n");patch/kernel/archive/rockchip-6.19/patches.armbian/rk322x-dmc-driver-01-sipv2-calls.patch (2)
58-70: Missing NULL check after ioremap.
ioremap()can return NULL on failure, butddr_data.share_memoryis used unconditionally in the rate functions. If ioremap fails, subsequent accesses throughddr_data.share_memorywill cause a NULL pointer dereference.🔎 Suggested fix
static void rockchip_ddrclk_data_init(void) { struct arm_smccc_res res; arm_smccc_smc(ROCKCHIP_SIP_SHARE_MEM, 1, SHARE_PAGE_TYPE_DDR, 0, 0, 0, 0, 0, &res); if (!res.a0) { ddr_data.share_memory = (void __iomem *)ioremap(res.a1, 1<<12); + if (!ddr_data.share_memory) { + pr_err("%s: failed to ioremap share memory\n", __func__); + return; + } ddr_data.inited_flag = 1; } }
72-100: Consider validating share_memory before access in set_rate_v2.If
rockchip_ddrclk_data_init()fails (ioremap returns NULL),ddr_data.inited_flagremains 0, causing repeated init attempts. However, after the first failed init, subsequent calls still proceed to dereferencepwhich would be NULL.🔎 Suggested fix
static int rockchip_ddrclk_sip_set_rate_v2(struct clk_hw *hw, unsigned long drate, unsigned long prate) { struct share_params *p; struct arm_smccc_res res; if (!ddr_data.inited_flag) rockchip_ddrclk_data_init(); + if (!ddr_data.share_memory) + return -ENOMEM; + p = (struct share_params *)ddr_data.share_memory;patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0147-WIP-FRL-drm-bridge-dw-hdmi-qp-Add-HDMI-2.1-FRL-suppo.patch (2)
71-113: Critical: Infinite loop due to missing loop condition comparison.Line 78 has
for (i = 0; ARRAY_SIZE(common_frl_n_table); i++)which is missing the comparison operator.ARRAY_SIZE(common_frl_n_table)evaluates to a non-zero constant (5), making the condition always true. This causes an infinite loop or out-of-bounds array access.🔎 Proposed fix
- for (i = 0; ARRAY_SIZE(common_frl_n_table); i++) { + for (i = 0; i < ARRAY_SIZE(common_frl_n_table); i++) {
127-129: Minor: Double semicolon at end of return statement.Line 129 has an extra semicolon after the function call return statement.
🔎 Proposed fix
- return dw_hdmi_qp_match_frl_n_table(hdmi, - link_cfg->frl_rate_per_lane, - sample_rate);; + return dw_hdmi_qp_match_frl_n_table(hdmi, + link_cfg->frl_rate_per_lane, + sample_rate);patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0074-FROMLIST-v7-media-rkvdec-Move-hevc-functions-to-comm.patch (1)
249-299: Missing include guards in new header file.The header file lacks include guards, which will cause redefinition errors if included multiple times. Kernel headers should use the standard guard pattern.
🔎 Proposed fix to add include guards
/* SPDX-License-Identifier: GPL-2.0 */ /* * Rockchip video decoder hevc common functions * * Copyright (C) 2025 Collabora, Ltd. * Detlev Casanova <detlev.casanova@collabora.com> * * Copyright (C) 2023 Collabora, Ltd. * Sebastian Fricke <sebastian.fricke@collabora.com> * * Copyright (C) 2019 Collabora, Ltd. * Boris Brezillon <boris.brezillon@collabora.com> * * Copyright (C) 2016 Rockchip Electronics Co., Ltd. * Jeffy Chen <jeffy.chen@rock-chips.com> */ +#ifndef _RKVDEC_HEVC_COMMON_H_ +#define _RKVDEC_HEVC_COMMON_H_ + #include <media/v4l2-mem2mem.h> #include "rkvdec.h" #define RKV_HEVC_CABAC_TABLE_SIZE 27456 extern const u8 rkvdec_hevc_cabac_table[RKV_HEVC_CABAC_TABLE_SIZE]; ... (rest of header content) ... void rkvdec_hevc_run_preamble(struct rkvdec_ctx *ctx, struct rkvdec_hevc_run *run); + +#endif /* _RKVDEC_HEVC_COMMON_H_ */
🤖 Fix all issues with AI Agents
In
@patch/kernel/archive/rockchip-6.19/patches.armbian/dts-rk3288-fix-mmc-aliases.patch:
- Around line 1-4: The commit subject is inaccurate: it says "add missing mmc
nodes" but the patch actually adds MMC aliases (mmc0, mmc1, mmc2, mmc3) that
reference existing nodes (&sdmmc, &sdio0, &emmc, &sdio1); update the Subject
line to "add missing mmc aliases" (or equivalent) to reflect that the patch
creates aliases rather than new device nodes.
In
@patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0125-FROMLIST-v1-dt-bindings-iommu-rockchip-Add-support-f.patch:
- Around line 7-8: The schema change incorrectly makes the interface clock
mandatory by setting minItems: 2 for the clocks array; update the schema so the
additional interface clocks remain optional by reverting minItems back to 1 (or
removing the minItems constraint) for the clocks array referenced near the
definitions that enumerate aclk and iface, ensuring the dts binding allows a
single clock by default while still accepting multiple interface clocks when
present.
In
@patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0130-FROMLIST-v2-phy-rockchip-inno-usb2-fix-disconnection.patch:
- Line 54: Update the Reviewed-by tag in the patch header: replace the current
"Reviewed-by: Theo Lebrun <theo.lebrun@bootlin.com>" entry with the correctly
accented form "Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>" so the
FROMLIST metadata preserves the exact upstream reviewer attribution (look for
the Reviewed-by line in the patch header and change the reviewer name only).
🧹 Nitpick comments (26)
patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0150-KWIBOO-media-cec-adap-add-debounce-support-when-sett.patch (1)
86-88: Consider adding input validation for the module parameter.The
cec_debounce_msis declared asint, allowing negative values. While the code handles this gracefully (negative values disable debounce due to the> 0check), usinguintwould make the intent clearer and prevent any confusion.🔎 Optional: Use unsigned type for clarity
-int cec_debounce_ms; -module_param_named(debounce_ms, cec_debounce_ms, int, 0644); +unsigned int cec_debounce_ms; +module_param_named(debounce_ms, cec_debounce_ms, uint, 0644);This would also require updating the extern declaration in
cec-priv.h.patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0134-FROMLIST-v1-pmdomain-rockchip-Fix-init-genpd-as-GENP.patch (1)
44-50: Consider checking the return value ofrockchip_pd_power().The call to
rockchip_pd_power(pd, false)can fail, but the return value is not checked. If the power-off operation fails during initialization, the system continues without any indication of the failure, which could lead to unexpected power management behavior.While the regulator acquisition pattern appears intentional (allowing error pointers to defer power-on until regulator is ready), a failed power-off operation should potentially be logged or handled.
🔎 Suggested improvement
if (pd->info->need_regulator) { if (IS_ERR_OR_NULL(pd->supply)) pd->supply = devm_of_regulator_get(pmu->dev, pd->node, "domain"); - if (!rockchip_pd_regulator_is_enabled(pd)) - rockchip_pd_power(pd, false); + if (!rockchip_pd_regulator_is_enabled(pd)) { + ret = rockchip_pd_power(pd, false); + if (ret < 0) + dev_warn(pmu->dev, "failed to power off %s: %d\n", + pd->genpd.name, ret); + } }Note: This assumes a
retvariable is available in scope. Alternatively, the error can be handled inline without storing it.patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0072-FROMLIST-v7-media-rkvdec-Use-structs-to-represent-th.patch (1)
84-127: Consider noting the implementation approach for potential upstream feedback.The switch statement is verbose with repetitive cases. While it works correctly (and upstream has already reviewed/tested this), alternative approaches like macros or computed offsets could reduce the 44 lines to a more compact form. However, the explicit approach does make the mapping very clear.
Since this is upstream code already accepted, this is purely an observation rather than a blocking concern.
patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0142-WIP-YUV420-drm-rockchip-vop2-Add-YUV420-output-forma.patch (1)
85-92: Variable declaration after statement in if-block.Line 87 declares
u32 csc_modeafter the executable statement on line 86. While modern kernel code allows mixed declarations (C11), placing the declaration at the start of the block would be more conventional and avoid potential issues with older toolchains.Suggested reorder
if (vcstate->output_mode == ROCKCHIP_OUT_MODE_YUV420) { + u32 csc_mode = vop2_convert_csc_mode(vcstate->color_space); val = RK3568_VP_BCSH_CTRL__BCSH_R2Y_EN; - u32 csc_mode = vop2_convert_csc_mode(vcstate->color_space); val |= FIELD_PREP(RK3568_VP_BCSH_CTRL__BCSH_R2Y_CSC_MODE, csc_mode); } else {patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0137-FROMLIST-v2-media-verisilicon-AV1-Fix-tx-mode-bit-se.patch (1)
27-33: Minor: Inconsistent enum naming convention.
ROCKCHIP_AV1_TX_MODE_16x16uses lowercase 'x' while other entries use uppercase 'X' (e.g.,ONLY_4X4). This is a cosmetic issue from upstream and doesn't affect functionality.patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0153-WIP-arm64-dts-rockchip-add-pcie-wifi-support-to-Oran.patch (1)
24-34: Regulator name does not match voltage specification; matches official upstream implementation.The regulator is named
vcc3v3_pcie20(implying 3.3V) but uses 1.8V values (1800000 microvolt min/max). While this naming mismatch is potentially confusing, the voltage specification aligns with the official OrangePi-5B AP6275P support patch in upstream Armbian (rk3588-1072). The AP6275P module's datasheet indicates it can operate across 1.68–3.6 V, though RF performance is optimized at 3.3V with separate 1.8V I/O supply.Since this matches the confirmed working upstream implementation, the voltage values are correct for the OrangePi-5B hardware design. However, consider renaming the regulator to
vcc1v8_pcie20for clarity if other boards require different voltages, or document the naming convention if intentionally kept consistent with existing board definitions.patch/kernel/archive/rockchip-6.19/patches.armbian/general-rockchip-various-fixes.patch (5)
499-499: Unnecessary trailing semicolon after function closing brace.-}; +}
523-526: Redundant comment: "phy_flag flag".Consider clarifying what this flag represents.
- /* phy_flag flag */ + /* HDMI PHY variant flag from efuse */ bool phy_flag;
541-549: Complex conditional logic is hard to follow and potentially fragile.The nested conditions mixing
phy_flag,tmdsclockthresholds, andversionchecks are difficult to reason about. Consider refactoring for clarity.The logic appears to be:
- If
phy_flagis false ORtmdsclock > 33750000: use normal matching (tmdsclock <= cfg->tmdsclock && version match)- If
phy_flagis true ANDtmdsclock <= 33750000: match only entries with version & 4Suggested refactor for readability
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)) + if (tmdsclock <= cfg->tmdsclock) { + if (inno->phy_flag && tmdsclock <= 33750000) { + if (cfg->version & 4) + break; + } else if (cfg->version & inno->chip_version) { + break; + } + } - break;
582-588: Simplify boolean assignment and consider usingdev_dbgfor production.Line 585's ternary is redundant, and the
dev_infolog at line 588 will appear on every boot. Considerdev_dbgfor non-essential diagnostic output.Suggested changes
if (len == 1) - inno->phy_flag = (efuse_buf[0] & BIT(1)) ? true : false; + inno->phy_flag = !!(efuse_buf[0] & BIT(1)); kfree(efuse_buf); - dev_info(inno->dev, "phy_flag is: %d\n", inno->phy_flag); + dev_dbg(inno->dev, "phy_flag is: %d\n", inno->phy_flag);
655-667: Approve parent-child power domain propagation logic; minor style nit.The logic correctly ensures that if a parent domain requires power control, its child domains must also participate. There's a double blank line at lines 665-666.
if (!parent_pd->is_ignore_pwr) child_pd->is_ignore_pwr = false; - rockchip_pm_add_subdomain(pmu, np);patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0146-WIP-FRL-drm-connector-hdmi-Handle-FRL-in-hdmi_clock_.patch (1)
25-25: Consider using kernel-style comment syntax.The C++ style comment
//TODO:should preferably be/* TODO: */to align with traditional kernel coding style, though modern compilers accept both.🔎 Suggested change
- //TODO: add proper FRL rate check + /* TODO: add proper FRL rate check */patch/kernel/archive/rockchip-6.19/patches.armbian/general-add-overlay-compilation-support.patch (1)
20-31: Verify the topmost_dir logic for multi-platform builds.The
topmost_dirvariable extracts the top-level directory from only the first entry in thedtbslist. Ifdtbscontains device trees from multiple top-level directories (e.g., bothrockchip/...andamlogic/...), the filtering logic on lines 26 and 28 would incorrectly use only the first directory for all entries.For the rockchip-6.19 kernel patch, this is likely acceptable since all dtbs entries would share the same
rockchip/prefix. However, this assumption makes the logic fragile if the pattern is reused in multi-platform contexts.🔎 More robust approach
Consider iterating over unique top-level directories or documenting the single-platform assumption explicitly:
# NOTE: This logic assumes all dtbs entries share the same top-level directory # (e.g., all under rockchip/). For multi-platform builds, this would need revision. topmost_dir = $(firstword $(subst /, ,$(dtbs)))patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0125-FROMLIST-v1-dt-bindings-iommu-rockchip-Add-support-f.patch (1)
23-29: Consider adding explicit maxItems for clarity.The schema now supports up to 5 clocks (aclk + iface + iface_1 + iface_2 + iface_3), but
maxItemsis not explicitly defined. AddingmaxItems: 5would make the schema clearer and more maintainable.🔎 Suggested addition
clocks: minItems: 2 + maxItems: 5 items: - description: Core clock - description: Interface clockAnd similarly for clock-names:
clock-names: minItems: 2 + maxItems: 5 items: - const: aclk - const: ifacepatch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0141-WIP-SCRAMB-drm-bridge-dw-hdmi-qp-Add-high-TMDS-clock.patch (1)
84-91: Consider adding NULL check for defensive programming.While the current implementation appears safe (the work is canceled before
connectoris set to NULL in line 179), adding a NULL check forhdmi->connectorin line 89 would make the code more robust against future reordering or refactoring.🔎 Proposed defensive NULL check
static void dw_hdmi_qp_scramb_work(struct work_struct *work) { struct dw_hdmi_qp *hdmi = container_of(to_delayed_work(work), struct dw_hdmi_qp, scramb_work); + if (!hdmi->connector) + return; + if (!drm_scdc_get_scrambling_status(hdmi->connector)) dw_hdmi_qp_set_scramb(hdmi); }patch/kernel/archive/rockchip-6.19/patches.armbian/driver-tinkerboard-alc4040-codec.patch (1)
70-81: Fix coding style issues in the conditional block.The code has a few style issues that should be addressed:
- Missing space after
ifkeyword (line 77)- Trailing whitespace after the closing brace (line 81)
🔎 Proposed style fixes
strim(card->shortname); - /* Tinker Board ALC4040 CODEC */ - - 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)); } - }patch/kernel/archive/rockchip-6.19/patches.armbian/driver-rk3288-gpiomem.patch (3)
221-221: Missing newline at end of Kconfig file.The Kconfig file is missing a trailing newline, which can cause issues with some tools and fails POSIX text file conventions.
🔎 Proposed fix
Provides users with root-free access to the GPIO registers on the 3288. Calling mmap(/dev/gpiomem) will map the GPIO register page to the user's pointer. +
228-229: Missing newline at end of Makefile.Same issue - missing trailing newline in the Makefile.
🔎 Proposed fix
obj-$(CONFIG_RK3288_DEVGPIOMEM)+= rk3288-gpiomem.o +
538-539: Missing newline at end of C source file.The driver source file is missing a trailing newline.
🔎 Proposed fix
MODULE_DESCRIPTION("gpiomem driver for accessing GPIO from userspace"); MODULE_AUTHOR("Luke Wren <luke@raspberrypi.org>"); +patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0031-FROMLIST-v9-dt-bindings-iommu-verisilicon-Add-bindin.patch (1)
85-87: Minor style: missing space in example compatible string.The example
compatibleproperty is missing a space after the comma, which is inconsistent with typical DT style.🔎 Suggested fix
- compatible = "rockchip,rk3588-av1-iommu","verisilicon,iommu-1.2"; + compatible = "rockchip,rk3588-av1-iommu", "verisilicon,iommu-1.2";patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0147-WIP-FRL-drm-bridge-dw-hdmi-qp-Add-HDMI-2.1-FRL-suppo.patch (2)
356-359: Use logical OR instead of bitwise OR in conditionals.Lines 356 and 359 use bitwise OR (
|) instead of logical OR (||) inifconditions. While functionally equivalent here (comparing to 0xf/0xe), logical OR is conventional and enables short-circuit evaluation.🔎 Proposed fix
- } else if ((ln0 == 0xf) | (ln1 == 0xf) | (ln2 == 0xf) | (ln3 == 0xf)) { + } else if ((ln0 == 0xf) || (ln1 == 0xf) || (ln2 == 0xf) || (ln3 == 0xf)) { dev_err(hdmi->dev, "goto lts4\n"); ret = LTS4; - } else if ((ln0 == 0xe) | (ln1 == 0xe) | (ln2 == 0xe) | (ln3 == 0xe)) { + } else if ((ln0 == 0xe) || (ln1 == 0xe) || (ln2 == 0xe) || (ln3 == 0xe)) {
1-5: Note: WIP patch with multiple TODO comments.The subject line indicates this is a Work-In-Progress patch (
[WIP-FRL]). Multiple TODO comments throughout indicate incomplete functionality (set_ffe, force_link_rate error handling, previous_mode). This is acceptable for the current state but should be tracked for completion.Do you want me to open an issue to track the remaining TODOs in this FRL implementation?
patch/kernel/archive/rockchip-6.19/patches.armbian/rk322x-dmc-driver-03-dfi-driver.patch (2)
56-94: Consider using kernel-preferred comment style.The C++ style comments (
//) work but kernel coding style prefers C-style comments (/* */). This is a minor style nit.- dfi->buswidth[0] = 2; // 16 bit bus width + dfi->buswidth[0] = 2; /* 16 bit bus width */ - dfi->ddrmon_stride = 0x0; // single channel controller + dfi->ddrmon_stride = 0x0; /* single channel controller */
146-154: Consider usingdev_dbginstead ofdev_noticefor initialization message.The
dev_noticelog level may be too verbose for production. Unless this information is critical for user troubleshooting,dev_dbgwould be more appropriate.- dev_notice(dfi->dev, "dfi initialized, dram type: 0x%x, channels: %d\n", dfi->ddr_type, dfi->max_channels); + dev_dbg(dfi->dev, "dfi initialized, dram type: 0x%x, channels: %d\n", dfi->ddr_type, dfi->max_channels);patch/kernel/archive/rockchip-6.19/patches.libreelec/rockchip-0032-FROMLIST-v9-iommu-Add-verisilicon-IOMMU-driver.patch (2)
603-621: Redundant NULL check on list_entry result.
list_entryreturns a computed pointer offset from the list node, never NULL (unless the list head itself is NULL, which can't happen with a properly initialized list). Theif (!iommu)check on line 609 will never be true.list_for_each(pos, &vsi_domain->iommus) { struct vsi_iommu *iommu; iommu = list_entry(pos, struct vsi_iommu, node); - if (!iommu) - continue; spin_lock(&iommu->lock);
778-778: Consider checking return value ofdma_set_mask_and_coherent.While a 32-bit DMA mask should always succeed on supported platforms, checking the return value is a defensive practice.
- dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); + if (err) + goto err_unprepare_clocks;
patch/kernel/archive/rockchip-6.19/patches.armbian/dts-rk3288-fix-mmc-aliases.patch
Show resolved
Hide resolved
...9/patches.libreelec/rockchip-0125-FROMLIST-v1-dt-bindings-iommu-rockchip-Add-support-f.patch
Show resolved
Hide resolved
...9/patches.libreelec/rockchip-0130-FROMLIST-v2-phy-rockchip-inno-usb2-fix-disconnection.patch
Show resolved
Hide resolved
rpardini
force-pushed
the
pr/rockchipuefi-loong64-619-rewrite-patches-against-619-rc4
branch
from
af4c923 to
1a83204
Compare
rpardini
added
Ready to merge
2 participants
rockchip/uefi-loong64 6.19: rewrite patches against 6.19-rc4
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.