Bump sm8250 #9180
Bump sm8250 #9180
Conversation
Copies and modifications to some patches from SM8250-6.12 Signed-off-by: CodeChenL <[email protected]>
from sm8250-6.12, to complete the device functionality. Signed-off-by: CodeChenL <[email protected]>
📝 WalkthroughWalkthroughAdds SM8250 kernel bumps (current→6.18, edge→6.19), new kernel config symbols, and a large set of kernel patches: DRM notifier API, touchscreen drivers/DT and pen support, ASoC TDM/audio, haptics, charger and fuel‑gauge drivers, Synaptics TCM and gamepad drivers, multiple board device‑trees, panel/LED drivers, USB gadget refactor, and assorted Kconfig/Makefile updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Probe as Touch Driver Probe
participant HW as Device (SPI/I2C, GPIO, Regulators)
participant Kernel as Linux Kernel
participant Input as Input Subsystem
participant FW as Firmware loader
Probe->>HW: acquire regulators, setup GPIO, deassert reset
Probe->>Kernel: request_irq(irq, handler)
Probe->>HW: disable_pen_input_device(true)
Probe->>FW: schedule firmware download (delayed work)
FW-->>Probe: firmware loaded
Probe->>HW: disable_pen_input_device(false)
HW->>Kernel: IRQ (touch)
Kernel->>Probe: irq_handler
Probe->>HW: read touch report
Probe->>Input: report events + input_sync()
sequenceDiagram
participant Userspace as App
participant ALSA as ASoC core
participant DSP as Q6AFE/Q6ASM
participant Codec as CS35L41
Userspace->>ALSA: open PCM + hw_params
ALSA->>ALSA: sm8250_tdm_snd_hw_params() configures slots/format
ALSA->>DSP: configure fmt / sysclk for TDM
ALSA->>Codec: set DAI formats, slots & channel map
Userspace->>ALSA: start stream
DSP->>Codec: feed TDM multi‑DAC streams
Codec->>Speakers: audio out
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/kernel/linux-sm8250-edge.config (1)
1-1: Update the version comment to reflect kernel 6.18.The comment still references kernel 6.14, but this config is now for the 6.18 edge branch.
📝 Proposed fix
-# Armbian defconfig generated with 6.14 +# Armbian defconfig generated with 6.18
🤖 Fix all issues with AI agents
In @patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml:
- Around line 4-8: Update the YAML metadata to reflect the 6.18 kernel bump:
change the `name` field from sm8250-6.12 to sm8250-6.18, update the `branch`
field from linux-6.12.y to linux-6.18.y, and replace the `last-known-good-tag`
(currently v6.12.5) with an appropriate 6.18 tag (e.g., v6.18.1 or the correct
6.18.x tag you intend to use).
In
@patch/kernel/archive/sm8250-6.18/0019-input-nt36xxx-Enable-pen-support.patch:
- Around line 78-81: The suspend path disables the pen only when
ts->pen_input_dev_enable is true but resume always calls
disable_pen_input_device(false), causing asymmetric state; fix by saving the
current pen enabled flag before suspend (e.g., store ts->pen_input_dev_enable
into a new field like ts->pen_was_enabled or a local/state variable in the
suspend handler) and on resume only call disable_pen_input_device(false) if that
saved flag indicates the pen was enabled before suspend, ensuring
disable_pen_input_device(true) is called in suspend only when
ts->pen_input_dev_enable was true and the resume restores state conditionally
based on the saved flag.
In
@patch/kernel/archive/sm8250-6.18/0020-nt36xxx-add-pen-input-resolution.patch:
- Around line 35-36: The hardcoded
PANEL_DEFAULT_WIDTH_MM/PANEL_DEFAULT_HEIGHT_MM values used in the nt36xxx code
when calling input_abs_set_res(ts->pen_input_dev, ABS_X/ABS_Y, ...) should be
made device-specific: read panel physical width/height (e.g.,
"pen-panel-width-mm" and "pen-panel-height-mm" or reuse existing panel size DT
properties) from the device tree in the driver probe (where struct ts is
initialized) and fall back to the current defaults if missing, then use those
retrieved u32 values instead of the compile-time constants when computing the
resolution for input_abs_set_res; update the probe/struct initialization code to
store these values (e.g., ts->panel_width_mm / ts->panel_height_mm) and
reference them where ABS_X/ABS_Y resolutions are set so different devices can
provide correct dimensions.
In
@patch/kernel/archive/sm8250-6.18/0027-Input-touchscreen-add-Synaptics-TCM-oncell-S3908.patch:
- Line 553: devm_gpiod_get() result for tcm->reset_gpio isn't checked for
ERR_PTR; if it fails the error pointer may later be passed to
gpiod_set_value_cansleep() causing a crash. After calling
devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW) in the probe/init path
(where tcm->reset_gpio is assigned), check IS_ERR(tcm->reset_gpio) and handle
the error: log the failure with dev_err(&client->dev, ...), return the
appropriate PTR_ERR(tcm->reset_gpio) (or fall back if preferred) so
gpiod_set_value_cansleep() is never called with an error pointer. Ensure
references to tcm->reset_gpio and devm_gpiod_get/dev_err/PTR_ERR are updated
accordingly.
- Line 438: The code calls le32_to_cpu(header->length) but
tcm_message_header::length is defined as __le16, so replace the 32-bit
conversion with le16_to_cpu(header->length) and ensure the receiving variable
(len) is an appropriate 16-bit type (e.g., u16 or unsigned int) or explicitly
cast to size_t when used for buffer sizes/limits; update any subsequent uses
that assume a 32-bit value to avoid sign/overflow issues.
- Around line 564-602: The IRQ is requested via
devm_request_threaded_irq(&client->dev, client->irq, NULL, tcm_report_irq, ...)
before the input device is allocated/registered, which can race with
tcm_handle_touch_report reading tcm->input; move the devm_request_threaded_irq
call so it runs after tcm_hw_init, after tcm->input is allocated and configured
(input_set_abs_params, touchscreen_parse_properties, input_mt_init_slots,
input_set_drvdata) and after input_register_device succeeds, so the IRQ handler
always sees a valid tcm->input; ensure error paths unregister or rely on devm
semantics as appropriate.
In
@patch/kernel/archive/sm8250-6.18/0028-arm64-dts-qcom-add-OnePlus-8T-kebab.patch:
- Around line 686-691: channel@4e is incorrectly using the same ADC mux constant
as channel@4d (ADC5_AMUX_THM2_100K_PU) causing a duplicate ADC register; update
the reg property of channel@4e (the node with label "wifi_therm") to
ADC5_AMUX_THM3_100K_PU to match the expected node/address pattern and ensure the
wifi_therm channel references the correct ADC mux.
- Around line 169-179: The panel_avdd_5p5 regulator and its pinctrl disagree on
the GPIO number: panel_avdd_5p5 declares gpio = <&tlmm 61 GPIO_ACTIVE_HIGH>
while panel_avdd_pins uses pins = "gpio65"; fix by making the GPIO reference
consistent (either change the regulator gpio to use 65 or change panel_avdd_pins
to "gpio61"), ensure the &tlmm controller and GPIO_ACTIVE_HIGH flag remain
correct, and update any related labels/comments so panel_avdd_5p5,
panel_avdd_pins, gpio and pins all match.
In
@patch/kernel/archive/sm8250-6.18/0038-Input-driver-for-AYN-Odin2-Gamepad.patch:
- Around line 244-277: The GPIO pointers returned by devm_gpiod_get_optional
(boot_gpio, enable_gpio, reset_gpio) are left as ERR_PTR values after IS_ERR
logging and later treated as truthy, causing invalid gpiod_set_value_cansleep
calls; update the error handling so that after detecting
IS_ERR(gamepad_dev->boot_gpio/enable_gpio/reset_gpio) you set that field to NULL
(or otherwise clear the ERR_PTR) OR change the later conditional checks to use
!IS_ERR(...) && gamepad_dev->*_gpio before calling gpiod_set_value_cansleep,
ensuring only valid non-error gpio pointers are used.
- Around line 141-175: Move the function-static prev_states into struct
gamepad_device (add an unsigned long prev_states field) and update
gamepad_input_handler to use dev->prev_states instead of the static variable;
also add a guard at the top of gamepad_input_handler that returns early if
data->data_len < 14 to prevent out-of-bounds reads, and keep the existing
bitmap_xor and for_each_set_bit logic but operate on &dev->prev_states and write
back dev->prev_states = keys after input_sync; reference symbols: struct
gamepad_device, prev_states, gamepad_input_handler, data->data_len, data->data,
keymap, ARRAY_SIZE(keymap).
In
@patch/kernel/archive/sm8250-6.18/0039-input-add-Qualcomm-SPMI-haptics-driver.patch:
- Around line 838-992: The probe allocates the spmi_haptics struct but never
initializes the play_lock mutex, yet mutex_destroy(&haptics->play_lock) is
called on error and in remove; fix by calling mutex_init(&haptics->play_lock)
early in spmi_haptics_probe (e.g., immediately after devm_kzalloc and before any
code that can jump to register_fail or before INIT_WORK/spmi_haptics_init), so
the mutex is always initialized before any cancel_work_sync/ mutex_destroy paths
run.
- Line 1004: Replace the deprecated SIMPLE_DEV_PM_OPS usage with
DEFINE_SIMPLE_DEV_PM_OPS: change the line creating spmi_haptics_pm_ops to use
DEFINE_SIMPLE_DEV_PM_OPS(spmi_haptics_pm_ops, spmi_haptics_suspend, NULL); so
the compiler sees spmi_haptics_suspend/resume correctly and you can remove any
__maybe_unused annotations that were compensating for the old macro.
🟡 Minor comments (7)
patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml-1-1 (1)
1-1: Update the file path reference in the comment.The comment references
sm8250-6.7but this file is forsm8250-6.18.📝 Proposed fix
-config: # This is file 'patch/kernel/archive/sm8250-6.7/0000.patching_config.yaml' +config: # This is file 'patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml'patch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patch-582-590 (1)
582-590: Usestatus = "okay"instead ofstatus = "ok".The devicetree specification defines the canonical value as
"okay", not"ok". While some parsers may accept both, using the standard value ensures consistency.Suggested fix
charger@66 { compatible = "ti,bq25980"; - status = "ok"; + status = "okay"; reg = <0x66>;patch/kernel/archive/sm8250-6.18/0001-drm-Add-drm-notifier-support.patch-79-89 (1)
79-89: Fix typo and event constant names in documentation.Two issues in the function documentation:
- Line 82: "inculde" should be "include"
- Line 81: References
MSM_DRM_EARLY_EVENT_BLANKandMSM_DRM_EVENT_BLANK, but the actual constants areMI_DRM_EVENT_BLANKandMI_DRM_EARLY_EVENT_BLANK(defined in the header)📝 Proposed fix
* mi_drm_notifier_call_chain - notify clients of drm_events - * @val: event MSM_DRM_EARLY_EVENT_BLANK or MSM_DRM_EVENT_BLANK - * @v: notifier data, inculde display id and display blank + * @val: event MI_DRM_EARLY_EVENT_BLANK or MI_DRM_EVENT_BLANK + * @v: notifier data, include display id and display blank * event(unblank or power down).patch/kernel/archive/sm8250-6.18/0001-drm-Add-drm-notifier-support.patch-52-77 (1)
52-77: Fix variable name mismatch in documentation.The function documentation references
msm_drm_notifier_list(lines 57, 71), but the actual variable is namedmi_drm_notifier_list(line 50). This inconsistency can confuse developers reading the code.📝 Proposed fix
* This function registers a notifier callback function - * to msm_drm_notifier_list, which would be called when + * to mi_drm_notifier_list, which would be called when * received unblank/power down event.* This function unregisters the callback function from - * msm_drm_notifier_list. + * mi_drm_notifier_list.patch/kernel/archive/sm8250-6.18/0028-arm64-dts-qcom-add-OnePlus-8T-kebab.patch-90-94 (1)
90-94: Typo in node name:key-vol-dowmshould bekey-vol-down.The node name has a typo ("dowm" instead of "down"), which is inconsistent with the label "Volume down".
Proposed fix
- key-vol-dowm { + key-vol-down { label = "Volume down"; linux,code = <KEY_VOLUMEDOWN>; gpios = <&pm8150_gpios 7 GPIO_ACTIVE_LOW>; };patch/kernel/archive/sm8250-6.18/0027-Input-touchscreen-add-Synaptics-TCM-oncell-S3908.patch-36-46 (1)
36-46: Kconfig help text is incomplete.The help text for
TOUCHSCREEN_SYNAPTICS_TCM_ONCELLat line 40 is incomplete: "Say Y if you have the Synaptics S3908 TCM Oncell" (missing "touchscreen" or similar).📝 Proposed fix
help - Say Y if you have the Synaptics S3908 TCM Oncell + Say Y if you have the Synaptics S3908 TCM Oncell touchscreen.patch/kernel/archive/sm8250-6.18/0027-Input-touchscreen-add-Synaptics-TCM-oncell-S3908.patch-76-92 (1)
76-92: Fix duplicate include and use modern GPIO API.Issues found:
- Line 78 and 85:
linux/input/touchscreen.his included twice- Line 88:
linux/of_gpio.his deprecated; uselinux/gpio/consumer.hinstead (the driver already usesdevm_gpiod_get)- Line 90:
linux/pm_runtime.his included but PM runtime functions are not used🔧 Proposed fix
#include <linux/i2c.h> #include <linux/input.h> #include <linux/input/touchscreen.h> #include <linux/mod_devicetable.h> #include <linux/module.h> #include <linux/property.h> #include <linux/unaligned.h> #include <linux/delay.h> #include <linux/input/mt.h> -#include <linux/input/touchscreen.h> #include <linux/interrupt.h> #include <linux/irq.h> -#include <linux/of_gpio.h> +#include <linux/gpio/consumer.h> #include <linux/module.h> -#include <linux/pm_runtime.h> #include <linux/regulator/consumer.h>
🧹 Nitpick comments (14)
patch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch (1)
80-80: Trailing whitespace detected.Line 80 contains trailing whitespace (spaces after the closing brace). This may cause issues with strict patch validation or linting tools.
Suggested fix
- }; + }; reserved-memory {Remove the trailing spaces after
};on line 80.patch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patch (3)
79-96: Consider usingwakeup-sourceinstead of deprecatedgpio-key,wakeup.The
gpio-key,wakeupproperty is deprecated in favor ofwakeup-sourcein the gpio-keys binding.Suggested fix
vol-up { label = "Volume Up"; gpios = <&pm8150_gpios 6 GPIO_ACTIVE_LOW>; linux,code = <KEY_VOLUMEUP>; debounce-interval = <15>; linux,can-disable; - gpio-key,wakeup; + wakeup-source; }; vol-down { label = "Volume Down"; linux,code = <KEY_VOLUMEDOWN>; gpios = <&pm8150_gpios 7 GPIO_ACTIVE_LOW>; debounce-interval = <15>; linux,can-disable; - gpio-key,wakeup; + wakeup-source; };
606-622: Consider using IRQ_TYPE_ macros for interrupt flags.*The interrupt flags value
0x2008is less readable than using named macros. If this isIRQ_TYPE_LEVEL_LOW(0x8) with additional flags, consider documenting or using the appropriate macro. The NFC node at line 572 usesIRQ_TYPE_LEVEL_HIGHwhich is more readable.
564-577: Inconsistent indentation: spaces used instead of tabs.The I2C device nodes (
&i2c1,&i2c5,&i2c16) use 4-space indentation while the rest of the file uses tabs. Kernel DTS files conventionally use tabs for indentation.patch/kernel/archive/sm8250-6.18/0001-drm-Add-drm-notifier-support.patch (1)
32-45: Consider modernizing to SPDX license identifiers.Both source files use the old GPL-2.0 boilerplate license text. Modern kernel code typically uses SPDX license identifiers for clarity and brevity. Consider replacing the boilerplate with:
// SPDX-License-Identifier: GPL-2.0-onlyThis keeps the license intent identical while following current kernel standards.
Also applies to: 96-109
patch/kernel/archive/sm8250-6.18/0038-Input-driver-for-AYN-Odin2-Gamepad.patch (2)
22-25: Kconfig entry missing help text and GPIO dependency.The Kconfig entry is minimal. Consider adding:
- A
helpsection describing the driver for users configuring the kernel.- A dependency on
GPIOLIBsince the driver usesdevm_gpiod_get_optional.Suggested Kconfig improvement
config JOYSTICK_ODIN2 tristate "AYN Odin2 gamepad" depends on SERIAL_DEV_BUS + depends on GPIOLIB || COMPILE_TEST + help + Say Y here if you have an AYN Odin2 handheld gaming device + and want to use its built-in gamepad controller. + + To compile this driver as a module, choose M here: the + module will be called odin2.
359-359: MODULE_ALIAS uses wrong prefix for serdev driver.The driver uses
MODULE_ALIAS("platform:" DRIVER_NAME)but this is a serdev driver, not a platform driver. The alias won't match anything useful. For serdev drivers, module autoloading is typically handled via the OF match table'sMODULE_DEVICE_TABLE(of, ...)which is already present.Consider removing the misleading alias or using the correct format.
Suggested fix
-MODULE_ALIAS("platform:" DRIVER_NAME); +MODULE_ALIAS("of:N*T*Cayn,odin2-gamepad");Or simply remove the line since
MODULE_DEVICE_TABLE(of, ...)already handles autoloading.patch/kernel/archive/sm8250-6.18/0020-nt36xxx-add-pen-input-resolution.patch (2)
19-19: Consider moving variable declarations to the beginning of the block.Line 19 declares variables in the middle of a block after other statements. While valid in C99+, the Linux kernel coding style typically prefers variable declarations at the beginning of blocks for consistency.
34-34: Fix formatting: extra space before comma.Line 34 has an extra space before the comma:
y_max ,should bey_max,.✨ Formatting fix
- input_set_abs_params(ts->pen_input_dev, ABS_Y, 0, y_max , 0, 0); + input_set_abs_params(ts->pen_input_dev, ABS_Y, 0, y_max, 0, 0);patch/kernel/archive/sm8250-6.18/0019-input-nt36xxx-Enable-pen-support.patch (1)
64-72: Use standard C comment style.The patch uses C++ style
//comments instead of the kernel standard/* */style. While this works, kernel code conventionally uses C89-style comments for consistency.♻️ Preferred style
-//#ifdef CONFIG_FACTORY_BUILD +/* +#ifdef CONFIG_FACTORY_BUILD +*/ ts->pen_input_dev_enable = 1; -//#else -// ts->pen_input_dev_enable = 0; -//#endif +/* +#else + ts->pen_input_dev_enable = 0; +#endif +*/patch/kernel/archive/sm8250-6.18/0027-Input-touchscreen-add-Synaptics-TCM-oncell-S3908.patch (2)
594-594: Consider deriving max touch slots from device capabilities.The maximum number of touch slots is hardcoded as
10. The device reportsmax_objectsin thetcm_app_infostructure (line 184), which could be used here for a more accurate configuration. Alternatively, define this as a named constant for clarity.💡 Possible improvement
Option 1: Use device-reported maximum:
+ u8 max_objects[2]; + // ... in tcm_hw_init, also populate max_objects ... + // ... in tcm_probe ... - ret = input_mt_init_slots(tcm->input, 10, INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED); + ret = input_mt_init_slots(tcm->input, max_objects, INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);Option 2: Define as a constant:
+#define TCM_MAX_TOUCH_SLOTS 10 + // ... in tcm_probe ... - ret = input_mt_init_slots(tcm->input, 10, INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED); + ret = input_mt_init_slots(tcm->input, TCM_MAX_TOUCH_SLOTS, INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
364-365: Touch report length validation logs but continues processing.If the touch report length is invalid (not a multiple of the expected point size), the code logs an error but continues processing. This could lead to parsing garbage data. Consider returning early or clamping the number of processed points.
🛡️ More defensive approach
if ((len - 3) % sizeof(*point)) - dev_err(&tcm->client->dev, "invalid touch report length\n"); + dev_warn(&tcm->client->dev, "invalid touch report length %zu (expected multiple of %zu)\n", + len, sizeof(*point)); + /* Clamp to avoid processing incomplete point */ + len = ((len - 1) / sizeof(*point)) * sizeof(*point) + 1; + }patch/kernel/archive/sm8250-6.18/0006-ASoC-qcom-sm8250-Add-tdm-support.patch (2)
100-106: Unchecked return value ofsnd_soc_dai_set_sysclk.The return value of
snd_soc_dai_set_sysclk()is not checked. If this call fails, the error goes unnoticed and could cause hard-to-debug audio issues at runtime.♻️ Suggested fix
for_each_rtd_codec_dais(rtd, j, codec_dai) { ret = snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); - snd_soc_dai_set_sysclk(codec_dai, 0, TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK); if (ret < 0) { dev_err(rtd->dev, "TDM fmt err:%d\n", ret); return ret; } + ret = snd_soc_dai_set_sysclk(codec_dai, 0, TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK); + if (ret < 0) { + dev_err(rtd->dev, "TDM sysclk err:%d\n", ret); + return ret; + } }
41-73: Inconsistent indentation in TDM hw_params function.The playback and capture branches use inconsistent indentation (mixture of 2-space and 3-space indents, and different alignments). While this is an existing patch being ported, consider normalizing to kernel coding style (tabs) if/when updating this patch.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
config/kernel/linux-sm8250-edge.configconfig/sources/families/sm8250.confpatch/kernel/archive/sm8250-6.18/0000.patching_config.yamlpatch/kernel/archive/sm8250-6.18/0001-drm-Add-drm-notifier-support.patchpatch/kernel/archive/sm8250-6.18/0003-Input-Add-nt36523-touchscreen-driver.patchpatch/kernel/archive/sm8250-6.18/0005-arm64-dts-qcom-sm8250-xiaomi-elish-enable-touchscree.patchpatch/kernel/archive/sm8250-6.18/0006-ASoC-qcom-sm8250-Add-tdm-support.patchpatch/kernel/archive/sm8250-6.18/0007-arm64-dts-qcom-sm8250-xiaomi-elish-Add-sound-support.patchpatch/kernel/archive/sm8250-6.18/0008-Asoc-wm_adsp-Add-prefix-support.patchpatch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patchpatch/kernel/archive/sm8250-6.18/0016-arm64-dts-qcom-sm8250-xiaomi-elish-add-keyboard-supp.patchpatch/kernel/archive/sm8250-6.18/0017-arm64-dts-qcom-sm8250-xiaomi-elish-remove-framebuffe.patchpatch/kernel/archive/sm8250-6.18/0019-input-nt36xxx-Enable-pen-support.patchpatch/kernel/archive/sm8250-6.18/0020-nt36xxx-add-pen-input-resolution.patchpatch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patchpatch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patchpatch/kernel/archive/sm8250-6.18/0024-input-nt36523-short-the-firmware-download-delay-from.patchpatch/kernel/archive/sm8250-6.18/0027-Input-touchscreen-add-Synaptics-TCM-oncell-S3908.patchpatch/kernel/archive/sm8250-6.18/0028-arm64-dts-qcom-add-OnePlus-8T-kebab.patchpatch/kernel/archive/sm8250-6.18/0038-Input-driver-for-AYN-Odin2-Gamepad.patchpatch/kernel/archive/sm8250-6.18/0039-input-add-Qualcomm-SPMI-haptics-driver.patchpatch/kernel/archive/sm8250-6.18/0040-arm64-dts-qcom-pm8150b-introduce-spmi-haptics.patchpatch/kernel/archive/sm8250-6.18/dt/.placeholder
🧰 Additional context used
🧠 Learnings (41)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Learnt from: rpardini
Repo: armbian/build PR: 9159
File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161
Timestamp: 2026-01-03T20:46:29.189Z
Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches.
<!-- </add_learning>
Learnt from: tabrisnet
Repo: armbian/build PR: 8746
File: config/sources/families/filogic.conf:61-68
Timestamp: 2025-10-13T02:26:18.249Z
Learning: In the Armbian build system, the "edge" kernel branch is intended to be bleeding-edge and experimental. It does not require hardware testing or stability verification before merging, as it's expected to contain potentially unstable or untested code for early adopters and developers.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-07-21T04:12:02.439Z
Learning: In the Armbian build system, for recurring maintenance tasks like kernel version bumping, TODO comments should use generic version formats (e.g., "MAJOR.MINOR-rc1") rather than specific version numbers (e.g., "6.17-rc1") to avoid the need for frequent comment updates that would create unnecessary maintenance overhead.
Learnt from: rpardini
Repo: armbian/build PR: 8879
File: config/sources/families/uefi-x86.conf:0-0
Timestamp: 2025-11-06T15:36:04.682Z
Learning: As of PR #8879, the uefi-x86 family in the Armbian build system now includes kernel patches for the first time. The current and edge branches for uefi-x86 are specifically configured for Apple T2-based x86 machines, including T2-specific patches from the linux-t2 project and custom kernel configuration options for Apple hardware drivers.
📚 Learning: 2025-06-25T03:42:09.086Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-06-25T03:40:52.109Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-11-02T20:49:56.719Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.
Applied to files:
config/sources/families/sm8250.confpatch/kernel/archive/sm8250-6.18/0000.patching_config.yamlconfig/kernel/linux-sm8250-edge.config
📚 Learning: 2025-11-10T22:05:40.490Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8913
File: config/sources/families/k3-beagle.conf:16-16
Timestamp: 2025-11-10T22:05:40.490Z
Learning: In the Armbian build system, kernel branches using non-mainline/vendor forks (like BeagleBoard's linux repository) should be named "vendor" or "vendor-rt" rather than "current" or "edge". The "current" and "edge" naming is reserved for mainline kernel branches. This affects both the case statement in family config files (e.g., `vendor | vendor-rt)` instead of `current | current-rt)`) and the corresponding KERNEL_TARGET declarations in board config files.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-04-30T16:16:47.150Z
Learnt from: The-going
Repo: armbian/build PR: 8147
File: config/sources/families/include/sunxi64_common.inc:38-39
Timestamp: 2025-04-30T16:16:47.150Z
Learning: The Armbian build system references Linux kernel versions in the form "tag:v6.14.4" in the KERNELBRANCH variable, even when point release tags might not be directly visible in the upstream repository in the same form.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-09-14T06:29:18.958Z
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:64-70
Timestamp: 2025-09-14T06:29:18.958Z
Learning: In the Armbian build system, vendor branch configurations in family files are designed to be shared across multiple SoCs within the same family that use the same vendor kernel tree. For example, rk35xx and rockchip-rk3588 families both use identical vendor branch settings (same KERNELSOURCE, KERNELBRANCH, and KERNELPATCHDIR), demonstrating that vendor branches are intentionally generic rather than SoC-specific.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-10-26T10:41:35.118Z
Learnt from: HackingGate
Repo: armbian/build PR: 8665
File: config/boards/photonicat2.csc:4-4
Timestamp: 2025-10-26T10:41:35.118Z
Learning: In the Armbian build system, rk3576 boards consistently use BOARDFAMILY="rk35xx" for both vendor and edge kernel targets. The rk35xx family configuration sources rockchip64_common.inc, which provides edge and current kernel branch definitions, making these branches available even though they're not defined directly in rk35xx.conf.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-12-12T23:09:56.813Z
Learnt from: tabrisnet
Repo: armbian/build PR: 9058
File: config/sources/families/spacemit.conf:39-45
Timestamp: 2025-12-12T23:09:56.813Z
Learning: In Armbian build configs for vendor kernel sources, prefer the following branch naming conventions: use 'vendor' or 'vendor-rt' for stable vendor releases, and 'vendor-edge' for bleeding-edge/pre-release vendor versions. The 'edge' naming without the 'vendor-' prefix is reserved for mainline kernel branches. Apply this pattern to family config files under config/sources/families (e.g., spacemit.conf) to ensure consistent vendor kernel sourcing naming across the repository.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-07-21T04:12:02.439Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-07-21T04:12:02.439Z
Learning: In the Armbian build system, for recurring maintenance tasks like kernel version bumping, TODO comments should use generic version formats (e.g., "MAJOR.MINOR-rc1") rather than specific version numbers (e.g., "6.17-rc1") to avoid the need for frequent comment updates that would create unnecessary maintenance overhead.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-12-19T13:56:45.124Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Applied to files:
config/sources/families/sm8250.confpatch/kernel/archive/sm8250-6.18/0028-arm64-dts-qcom-add-OnePlus-8T-kebab.patchpatch/kernel/archive/sm8250-6.18/0000.patching_config.yamlpatch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patchpatch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patch
📚 Learning: 2025-10-13T02:26:18.249Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8746
File: config/sources/families/filogic.conf:61-68
Timestamp: 2025-10-13T02:26:18.249Z
Learning: In the Armbian build system, the "edge" kernel branch is intended to be bleeding-edge and experimental. It does not require hardware testing or stability verification before merging, as it's expected to contain potentially unstable or untested code for early adopters and developers.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-09-29T06:32:32.361Z
Learnt from: amazingfate
Repo: armbian/build PR: 8687
File: config/sources/mainline-kernel.conf.sh:11-11
Timestamp: 2025-09-29T06:32:32.361Z
Learning: There can be timing discrepancies between different kernel.org sources (web pages vs git repositories) when new kernel releases are published. Direct verification via git.kernel.org repository URLs is more reliable than general web searches for confirming tag availability.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2026-01-06T21:17:42.268Z
Learnt from: paolosabatino
Repo: armbian/build PR: 9171
File: patch/misc/bluetooth-rtl8723cs/bluetooth-btrtl-add-rtl8703bs.patch:34-34
Timestamp: 2026-01-06T21:17:42.268Z
Learning: In Linux kernel btrtl.c driver patches, recent kernel practice removes the .bin extension from fw_name entries in ic_id_table[] to support multiple firmware file formats/versions. The driver's firmware-parsing code dynamically selects the correct file variant. When reviewing bluetooth-rtl patches, fw_name without .bin extension (e.g., "rtl_bt/rtl8723cs_xx_fw") is the modern correct pattern, not an error.
Applied to files:
patch/kernel/archive/sm8250-6.18/0008-Asoc-wm_adsp-Add-prefix-support.patchpatch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patchpatch/kernel/archive/sm8250-6.18/0024-input-nt36523-short-the-firmware-download-delay-from.patch
📚 Learning: 2025-12-17T15:46:22.709Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 9096
File: patch/kernel/archive/sunxi-6.18/patches.armbian/arm64-dts-sun50i-h616-add-overlays.patch:4-5
Timestamp: 2025-12-17T15:46:22.709Z
Learning: In Armbian kernel patches, when patches are renamed or reorganized, the old Subject line may be intentionally preserved in the patch header (as a second Subject line) for archival/historical purposes to document the previous patch name. This is an Armbian-specific practice for tracking patch evolution and should not be flagged as an error when the maintainer has intentionally included it for documentation.
Applied to files:
patch/kernel/archive/sm8250-6.18/0008-Asoc-wm_adsp-Add-prefix-support.patchpatch/kernel/archive/sm8250-6.18/0028-arm64-dts-qcom-add-OnePlus-8T-kebab.patchpatch/kernel/archive/sm8250-6.18/0000.patching_config.yamlpatch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patchpatch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patchpatch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
📚 Learning: 2025-12-17T05:09:02.306Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Applied to files:
patch/kernel/archive/sm8250-6.18/0008-Asoc-wm_adsp-Add-prefix-support.patchpatch/kernel/archive/sm8250-6.18/0028-arm64-dts-qcom-add-OnePlus-8T-kebab.patchpatch/kernel/archive/sm8250-6.18/0017-arm64-dts-qcom-sm8250-xiaomi-elish-remove-framebuffe.patchpatch/kernel/archive/sm8250-6.18/0000.patching_config.yamlpatch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patchpatch/kernel/archive/sm8250-6.18/0024-input-nt36523-short-the-firmware-download-delay-from.patchpatch/kernel/archive/sm8250-6.18/0007-arm64-dts-qcom-sm8250-xiaomi-elish-Add-sound-support.patchpatch/kernel/archive/sm8250-6.18/0016-arm64-dts-qcom-sm8250-xiaomi-elish-add-keyboard-supp.patchpatch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patchpatch/kernel/archive/sm8250-6.18/0005-arm64-dts-qcom-sm8250-xiaomi-elish-enable-touchscree.patchpatch/kernel/archive/sm8250-6.18/0027-Input-touchscreen-add-Synaptics-TCM-oncell-S3908.patchpatch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
📚 Learning: 2025-09-11T06:06:52.328Z
Learnt from: SuperKali
Repo: armbian/build PR: 8608
File: lib/functions/compilation/patch/drivers_network.sh:361-363
Timestamp: 2025-09-11T06:06:52.328Z
Learning: For the RTW88 SDIO RF path detection fix patch (004-rtw88-sdio-rf-path-detection-fix.patch), SuperKali prefers using exact version matching (eq 6.1 || eq 6.16) rather than range checks, as the patch has only been tested on those specific kernel versions and may be mainstreamed soon.
Applied to files:
patch/kernel/archive/sm8250-6.18/0008-Asoc-wm_adsp-Add-prefix-support.patchpatch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patchpatch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
📚 Learning: 2025-09-14T11:37:35.089Z
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:65-72
Timestamp: 2025-09-14T11:37:35.089Z
Learning: In the Armbian build system, patch directories referenced in BOOTPATCHDIR and KERNELPATCHDIR configurations can be non-existent without causing build failures. Empty patch directories are also ignored by git, so missing patch directories should not be flagged as errors during code review.
Applied to files:
patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml
📚 Learning: 2025-11-20T18:20:11.985Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8968
File: patch/u-boot/u-boot-sunxi/arm64-dts-sun50i-h6-orangepi.dtsi-Rollback-r_rsb-to-r_i2c.patch:36-36
Timestamp: 2025-11-20T18:20:11.985Z
Learning: The rewrite-patches tool (REWRITE_PATCHES=yes) in the Armbian build system can inadvertently introduce semantic changes when the u-boot/kernel git base revision differs from expected state. The tool applies patches, commits them, and re-exports them using git format-patch, which can cause the re-exported patch to reflect the base revision's state rather than preserving the original patch intent. This is particularly problematic for device tree changes like interrupt specifications. The tool currently lacks validation mechanisms to detect such semantic drift, and affected patches must be manually corrected after rewriting.
Applied to files:
patch/kernel/archive/sm8250-6.18/0000.patching_config.yamlpatch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patchpatch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
📚 Learning: 2025-12-21T10:28:48.455Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 9086
File: patch/kernel/archive/uefi-arm64-6.19/net-stmicro-stmmac-Phytium-onboard-ethernet-drivers-and-ACPI-glue-for-6.x.patch.disabled:625-633
Timestamp: 2025-12-21T10:28:48.455Z
Learning: The patch "net-stmicro-stmmac-Phytium-onboard-ethernet-drivers-and-ACPI-glue-for-6.x.patch" in patch/kernel/archive/uefi-arm64-6.19/ is known to break compilation on kernel 6.19 and is therefore disabled (has .disabled extension).
Applied to files:
patch/kernel/archive/sm8250-6.18/0000.patching_config.yamlpatch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
📚 Learning: 2025-07-23T07:30:52.265Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8417
File: config/boards/orangepi5pro.csc:57-58
Timestamp: 2025-07-23T07:30:52.265Z
Learning: In the Armbian build system, BOOTPATCHDIR can contain board-specific subdirectories (e.g., board_orangepi5pro) for applying patches to specific boards only. The framework automatically checks if such board-specific subdirectories exist for the board being built and applies those patches accordingly.
Applied to files:
patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml
📚 Learning: 2025-09-27T21:47:58.020Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-bcm2711-edge.config:859-861
Timestamp: 2025-09-27T21:47:58.020Z
Learning: In the Armbian build system, kernel configuration files in config/kernel/ are generated through an automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
patch/kernel/archive/sm8250-6.18/0000.patching_config.yamlconfig/kernel/linux-sm8250-edge.config
📚 Learning: 2025-09-27T21:50:04.845Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sm8250-edge.config:80-82
Timestamp: 2025-09-27T21:50:04.845Z
Learning: In the Armbian build system, kernel configuration files are generated through this automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
patch/kernel/archive/sm8250-6.18/0000.patching_config.yamlconfig/kernel/linux-sm8250-edge.config
📚 Learning: 2026-01-03T20:46:29.189Z
Learnt from: rpardini
Repo: armbian/build PR: 9159
File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161
Timestamp: 2026-01-03T20:46:29.189Z
Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches.
<!-- </add_learning>
Applied to files:
patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml
📚 Learning: 2025-09-27T21:49:55.796Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sm8250-current.config:78-80
Timestamp: 2025-09-27T21:49:55.796Z
Learning: In the Armbian build system, kernel configuration files are generated through an automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
patch/kernel/archive/sm8250-6.18/0000.patching_config.yamlconfig/kernel/linux-sm8250-edge.config
📚 Learning: 2025-08-03T15:21:20.148Z
Learnt from: pyavitz
Repo: armbian/build PR: 8455
File: config/sources/families/sun50iw1.conf:19-24
Timestamp: 2025-08-03T15:21:20.148Z
Learning: In the Armbian build system, when copying firmware files during family_tweaks_s(), use /lib/firmware/updates/ instead of /lib/firmware/ to avoid conflicts with the Armbian firmware package. The /lib/firmware/updates directory takes precedence in Linux firmware loading hierarchy and is the proper location for user-installed firmware files.
Applied to files:
patch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patch
📚 Learning: 2025-03-31T22:20:41.849Z
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
Applied to files:
patch/kernel/archive/sm8250-6.18/0024-input-nt36523-short-the-firmware-download-delay-from.patch
📚 Learning: 2025-11-08T06:39:24.527Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8886
File: patch/kernel/rk35xx-vendor-6.1/dt/rk3528-nanopi-rev01.dts:9-9
Timestamp: 2025-11-08T06:39:24.527Z
Learning: In Armbian's Rockchip vendor kernel branches (rk35xx-vendor), device tree files may not require the same headers as mainline Linux. For example, input key definitions like KEY_BACK and KEY_VOLUMEUP may be handled differently in BSP kernels and don't necessarily need explicit includes like <dt-bindings/input/input.h> that would be required in mainline kernel device trees.
Applied to files:
patch/kernel/archive/sm8250-6.18/0016-arm64-dts-qcom-sm8250-xiaomi-elish-add-keyboard-supp.patchpatch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patchpatch/kernel/archive/sm8250-6.18/0039-input-add-Qualcomm-SPMI-haptics-driver.patch
📚 Learning: 2025-08-30T04:13:16.457Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-30T04:13:16.457Z
Learning: Armbian kernel configuration files like linux-filogic-current.config are autogenerated overlays on top of arch defconfig. Comments added manually will be lost during future updates by maintainers, and explicit "CONFIG_OPTION is not set" statements aren't needed for mutually exclusive options since these are overlay configs that only specify changes from the base configuration.
Applied to files:
config/kernel/linux-sm8250-edge.config
📚 Learning: 2025-03-31T12:57:13.880Z
Learnt from: libiunc
Repo: armbian/build PR: 8033
File: config/kernel/linux-starfive2-vendor.config:43-43
Timestamp: 2025-03-31T12:57:13.880Z
Learning: For StarFive2 platform kernel configurations, maintain alignment with vendor-provided configurations rather than modifying security settings like SECCOMP. This ensures hardware compatibility as intended by the manufacturer.
Applied to files:
config/kernel/linux-sm8250-edge.config
📚 Learning: 2025-10-23T19:48:42.980Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8812
File: config/kernel/linux-sm8250-edge.config:498-501
Timestamp: 2025-10-23T19:48:42.980Z
Learning: For Armbian EDGE kernel configs, CONFIG_ATH12K=m alone is sufficient for PCI-based Wi-Fi 7 devices (e.g., WCN785x/QCN9274). A separate CONFIG_ATH12K_PCI option is not required, as confirmed by maintainer testing.
Applied to files:
config/kernel/linux-sm8250-edge.config
📚 Learning: 2025-09-27T21:50:15.915Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sunxi64-current.config:94-94
Timestamp: 2025-09-27T21:50:15.915Z
Learning: When kernel config files are generated through Armbian's automated process (previous config → armbian-kernel.sh changes → make oldconfig → Armbian machinery processing), manual config file edits are not appropriate since they would be overwritten. Deprecated option handling should be implemented in the automated tooling instead.
Applied to files:
config/kernel/linux-sm8250-edge.config
📚 Learning: 2025-12-23T04:44:34.317Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 9067
File: patch/kernel/archive/rockchip64-6.19/dt/rk3399-orangepi-4-lts.dts:869-875
Timestamp: 2025-12-23T04:44:34.317Z
Learning: When reviewing device tree changes, validate node properties against the official kernel device tree binding documentation (*.yaml files) rather than relying solely on historical patterns in existing DTS files. Copy-paste errors can persist across versions, and bindings define the authoritative set of valid properties for each device node type.
Applied to files:
patch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patch
📚 Learning: 2025-09-18T03:36:17.862Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8537
File: config/boards/qcom-robotics-rb5.conf:1-1
Timestamp: 2025-09-18T03:36:17.862Z
Learning: In Armbian board configuration files (regardless of file extension), the standard pattern is to have only one line as a comment describing the board hardware specifications. This single-line description typically includes the SoC model, core count, RAM options, and key features like connectivity options, storage interfaces, and special features. The file extensions indicate board support status: .conf (standard support), .csc (community supported), .eos (end of support), .tvb (tvbox), .wip (work in progress).
Applied to files:
patch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patch
📚 Learning: 2025-07-27T15:53:30.629Z
Learnt from: pyavitz
Repo: armbian/build PR: 8421
File: config/kernel/linux-sunxi64-edge.config:805-825
Timestamp: 2025-07-27T15:53:30.629Z
Learning: In the Armbian build system, kernel configurations prioritize broad hardware compatibility over optimization concerns. Even when including numerous legacy/obsolete drivers causes technical issues like increased initramfs size and slower enumeration, the project philosophy is to "appease the masses" by ensuring maximum device compatibility rather than optimizing for specific use cases.
Applied to files:
patch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patch
📚 Learning: 2025-07-17T04:12:33.125Z
Learnt from: pyavitz
Repo: armbian/build PR: 8388
File: config/boards/kickpik2b.conf:1-14
Timestamp: 2025-07-17T04:12:33.125Z
Learning: In the Armbian build system, board configuration files (*.conf) follow a project standard of not including shebang lines, even though they contain bash code and are sourced by the build system. This is an established Armbian convention that individual contributors are expected to follow, and changes to this standard would require broader project maintainer approval.
Applied to files:
patch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patch
📚 Learning: 2025-09-18T03:36:17.862Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8537
File: config/boards/qcom-robotics-rb5.conf:1-1
Timestamp: 2025-09-18T03:36:17.862Z
Learning: In Armbian board configuration files, .wip and .conf extensions require BOARD_MAINTAINER to be present and contain at least one maintainer. If no maintainer is present, the board support rules state it does not qualify for standard support and must be moved to community support (.csc extension). This is documented at https://docs.armbian.com/User-Guide_Board-Support-Rules/
Applied to files:
patch/kernel/archive/sm8250-6.18/0009-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Add-dev.patch
📚 Learning: 2025-09-09T07:30:22.419Z
Learnt from: amazingfate
Repo: armbian/build PR: 8595
File: lib/functions/compilation/patch/drivers_network.sh:0-0
Timestamp: 2025-09-09T07:30:22.419Z
Learning: RTW_WARN_LMT sed workaround in driver_rtl8852bs() function in lib/functions/compilation/patch/drivers_network.sh is necessary for kernel 6.1 compatibility, even when the upstream wifi-rtl8852bs driver has the macro defined for newer kernels. The workaround ensures cross-kernel compatibility from 6.1 to 6.16+.
Applied to files:
patch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
📚 Learning: 2025-08-02T14:59:28.145Z
Learnt from: amazingfate
Repo: armbian/build PR: 8453
File: lib/functions/compilation/patch/drivers_network.sh:213-216
Timestamp: 2025-08-02T14:59:28.145Z
Learning: The wireless driver patches wireless-rtl8812au-Fix-6.16.patch, wireless-rtl8811cu-Fix-6.16.patch, and wireless-rtl88x2bu-Fix-6.16.patch in the Armbian build system are backward compatible and can be applied unconditionally without version checks, even though they're named for 6.16+ fixes.
Applied to files:
patch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
📚 Learning: 2025-07-17T04:16:29.551Z
Learnt from: pyavitz
Repo: armbian/build PR: 8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
Applied to files:
patch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
📚 Learning: 2025-10-22T07:51:53.015Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8789
File: config/kernel/linux-sunxi64-edge.config:839-839
Timestamp: 2025-10-22T07:51:53.015Z
Learning: In Armbian's wireless driver configuration, CONFIG_RK_WIFI_DEVICE_* options are Rockchip-specific device registration settings, while CONFIG_WLAN_UWE* are the actual cross-platform driver modules for UWE5622 wireless chips. The UWE5622 chip (manufactured by Unisoc/Spreadtrum) is used on multiple Allwinner boards including Orange Pi Zero 2/2W/3 and Orange Pi 3 LTS, so CONFIG_WLAN_UWE5621/5622 along with Unisoc infrastructure options (CONFIG_SPARD_WLAN_SUPPORT, CONFIG_SC23XX, CONFIG_UNISOC_WIFI_PS, CONFIG_WCN_BSP_DRIVER_BUILDIN) are correct in sunxi64 kernel configs.
Applied to files:
patch/kernel/archive/sm8250-6.18/0021-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch
| +int32_t disable_pen_input_device(bool disable) { | ||
| + uint8_t buf[8] = {0}; | ||
| + int32_t ret = 0; | ||
| + | ||
| + NVT_LOG("++\n"); | ||
| + if (!bTouchIsAwake || !ts) { | ||
| + NVT_LOG("touch suspend, stop set pen state %s", disable ? "DISABLE" : "ENABLE"); | ||
| + goto nvt_set_pen_enable_out; | ||
| + } | ||
| + | ||
| + msleep(35); | ||
| + disable = (!(ts->pen_input_dev_enable) || ts->pen_is_charge) ? true : disable; | ||
| + | ||
| + //---set xdata index to EVENT BUF ADDR--- | ||
| + ret = nvt_set_page(ts->mmap->EVENT_BUF_ADDR | EVENT_MAP_HOST_CMD); | ||
| + if (ret < 0) { | ||
| + NVT_ERR("Set event buffer index fail!\n"); | ||
| + goto nvt_set_pen_enable_out; | ||
| + } | ||
| + | ||
| + buf[0] = EVENT_MAP_HOST_CMD; | ||
| + buf[1] = 0x7B; | ||
| + buf[2] = !!disable; | ||
| + ret = CTP_SPI_WRITE(ts->client, buf, 3); | ||
| + if (ret < 0) { | ||
| + NVT_ERR("set pen %s failed!\n", disable ? "DISABLE" : "ENABLE"); | ||
| + goto nvt_set_pen_enable_out; | ||
| + } | ||
| + NVT_LOG("pen charge state is %s, %s pen input device\n", | ||
| + ts->pen_is_charge ? "ENABLE" : "DISABLE", | ||
| + disable ? "DISABLE" : "ENABLE"); | ||
| + | ||
| +nvt_set_pen_enable_out: | ||
| + NVT_LOG("--\n"); | ||
| + return ret; | ||
| +} |
There was a problem hiding this comment.
Add synchronization to prevent race conditions.
The function accesses shared state (bTouchIsAwake, ts->pen_input_dev_enable, ts->pen_is_charge) without holding any locks, but it's called from contexts with different locking states:
- Line 80 (suspend): lock state unclear
- Line 88 (resume): called after
mutex_unlock - Line 112 (firmware update): inside
mutex_lock
This creates potential race conditions where these fields could be modified concurrently.
Additionally, the msleep(35) at line 30 is a magic number with no explanation for the specific delay duration.
🔒 Recommended approach
Consider either:
- Document the locking requirements in a comment and ensure all call sites hold
ts->lock - Acquire
ts->lockinside this function if it's safe to do so from all call contexts - Add lock assertions to verify correct locking at runtime (e.g.,
lockdep_assert_held(&ts->lock))
For the magic number, add a comment explaining why 35ms is required:
+ /* Wait 35ms for controller to be ready for pen state change */
msleep(35);📝 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.
| +int32_t disable_pen_input_device(bool disable) { | |
| + uint8_t buf[8] = {0}; | |
| + int32_t ret = 0; | |
| + | |
| + NVT_LOG("++\n"); | |
| + if (!bTouchIsAwake || !ts) { | |
| + NVT_LOG("touch suspend, stop set pen state %s", disable ? "DISABLE" : "ENABLE"); | |
| + goto nvt_set_pen_enable_out; | |
| + } | |
| + | |
| + msleep(35); | |
| + disable = (!(ts->pen_input_dev_enable) || ts->pen_is_charge) ? true : disable; | |
| + | |
| + //---set xdata index to EVENT BUF ADDR--- | |
| + ret = nvt_set_page(ts->mmap->EVENT_BUF_ADDR | EVENT_MAP_HOST_CMD); | |
| + if (ret < 0) { | |
| + NVT_ERR("Set event buffer index fail!\n"); | |
| + goto nvt_set_pen_enable_out; | |
| + } | |
| + | |
| + buf[0] = EVENT_MAP_HOST_CMD; | |
| + buf[1] = 0x7B; | |
| + buf[2] = !!disable; | |
| + ret = CTP_SPI_WRITE(ts->client, buf, 3); | |
| + if (ret < 0) { | |
| + NVT_ERR("set pen %s failed!\n", disable ? "DISABLE" : "ENABLE"); | |
| + goto nvt_set_pen_enable_out; | |
| + } | |
| + NVT_LOG("pen charge state is %s, %s pen input device\n", | |
| + ts->pen_is_charge ? "ENABLE" : "DISABLE", | |
| + disable ? "DISABLE" : "ENABLE"); | |
| + | |
| +nvt_set_pen_enable_out: | |
| + NVT_LOG("--\n"); | |
| + return ret; | |
| +} | |
| int32_t disable_pen_input_device(bool disable) { | |
| uint8_t buf[8] = {0}; | |
| int32_t ret = 0; | |
| NVT_LOG("++\n"); | |
| if (!bTouchIsAwake || !ts) { | |
| NVT_LOG("touch suspend, stop set pen state %s", disable ? "DISABLE" : "ENABLE"); | |
| goto nvt_set_pen_enable_out; | |
| } | |
| /* Wait 35ms for controller to be ready for pen state change */ | |
| msleep(35); | |
| disable = (!(ts->pen_input_dev_enable) || ts->pen_is_charge) ? true : disable; | |
| //---set xdata index to EVENT BUF ADDR--- | |
| ret = nvt_set_page(ts->mmap->EVENT_BUF_ADDR | EVENT_MAP_HOST_CMD); | |
| if (ret < 0) { | |
| NVT_ERR("Set event buffer index fail!\n"); | |
| goto nvt_set_pen_enable_out; | |
| } | |
| buf[0] = EVENT_MAP_HOST_CMD; | |
| buf[1] = 0x7B; | |
| buf[2] = !!disable; | |
| ret = CTP_SPI_WRITE(ts->client, buf, 3); | |
| if (ret < 0) { | |
| NVT_ERR("set pen %s failed!\n", disable ? "DISABLE" : "ENABLE"); | |
| goto nvt_set_pen_enable_out; | |
| } | |
| NVT_LOG("pen charge state is %s, %s pen input device\n", | |
| ts->pen_is_charge ? "ENABLE" : "DISABLE", | |
| disable ? "DISABLE" : "ENABLE"); | |
| nvt_set_pen_enable_out: | |
| NVT_LOG("--\n"); | |
| return ret; | |
| } |
| if (ts->pen_input_dev_enable) { | ||
| NVT_LOG("if enable pen,will close it"); | ||
| + disable_pen_input_device(true); | ||
| } |
There was a problem hiding this comment.
Fix asymmetric suspend/resume logic.
In suspend, the pen is only disabled when ts->pen_input_dev_enable is true (lines 78-81), but in resume, disable_pen_input_device(false) is called unconditionally (line 88). This asymmetry means the pen could be re-enabled on resume even if it wasn't enabled before suspend.
🔧 Suggested fix
Store the pen state before suspend and restore it conditionally:
In suspend:
if (ts->pen_input_dev_enable) {
NVT_LOG("if enable pen,will close it");
disable_pen_input_device(true);
}In resume:
- disable_pen_input_device(false);
+ if (ts->pen_input_dev_enable) {
+ disable_pen_input_device(false);
+ }Or track the previous state explicitly if the behavior needs to differ.
Also applies to: 88-88
🤖 Prompt for AI Agents
In @patch/kernel/archive/sm8250-6.18/0019-input-nt36xxx-Enable-pen-support.patch
around lines 78 - 81, The suspend path disables the pen only when
ts->pen_input_dev_enable is true but resume always calls
disable_pen_input_device(false), causing asymmetric state; fix by saving the
current pen enabled flag before suspend (e.g., store ts->pen_input_dev_enable
into a new field like ts->pen_was_enabled or a local/state variable in the
suspend handler) and on resume only call disable_pen_input_device(false) if that
saved flag indicates the pen was enabled before suspend, ensuring
disable_pen_input_device(true) is called in suspend only when
ts->pen_input_dev_enable was true and the resume restores state conditionally
based on the saved flag.
| + input_abs_set_res(ts->pen_input_dev, ABS_X, x_max / PANEL_DEFAULT_WIDTH_MM); | ||
| + input_abs_set_res(ts->pen_input_dev, ABS_Y, y_max / PANEL_DEFAULT_HEIGHT_MM); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the patch file and examine its full context
find . -name "*nt36xxx*" -type fRepository: armbian/build
Length of output: 372
🏁 Script executed:
# Check if the patch file exists and examine its content
cat -n patch/kernel/archive/sm8250-6.18/0020-nt36xxx-add-pen-input-resolution.patch 2>/dev/null | head -100Repository: armbian/build
Length of output: 3026
🏁 Script executed:
# Search for SM8250 device tree files
fd -e dts -e dtsi . | grep -i sm8250Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Search for panel dimension definitions in the patch
rg -n "PANEL_DEFAULT.*MM|148|237" patch/kernel/archive/sm8250-6.18/ 2>/dev/nullRepository: armbian/build
Length of output: 900
🏁 Script executed:
# Search for nt36xxx/nt36523 driver files to understand what devices it supports
fd -e c -e h . | xargs rg -l "nt36523|nt36xxx" 2>/dev/null | head -20Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Search for Xiaomi Elish references to understand device support
rg -n "Xiaomi.*Elish|Elish|xiaomi.*elish" . --type-list | head -20Repository: armbian/build
Length of output: 446
🏁 Script executed:
# Search for all SM8250 device patch files that might define panel dimensions
fd -name "*.patch" | xargs rg -l "sm8250|SM8250" 2>/dev/null | head -20Repository: armbian/build
Length of output: 2142
🏁 Script executed:
# Look for display panel dimension specifications in all SM8250 related patches
fd -name "*sm8250*.patch" | xargs rg -n "width|height|mm|panel" 2>/dev/null | grep -i "display\|panel\|mm\|screen"Repository: armbian/build
Length of output: 50619
🏁 Script executed:
# Check which touchscreen drivers are used by SM8250 devices
fd -name "*sm8250*.patch" | xargs rg -n "nt36523|nt36xxx|touchscreen" 2>/dev/nullRepository: armbian/build
Length of output: 50618
🏁 Script executed:
# Search for Xiaomi Elish display properties/dimensions in patches
rg -i "elish" patch/kernel/archive/sm8250-6.18/ | head -30Repository: armbian/build
Length of output: 5250
🏁 Script executed:
# Look for display/panel specifications in Xiaomi Elish device tree patches
cat patch/kernel/archive/sm8250-6.18/0005-arm64-dts-qcom-sm8250-xiaomi-elish-enable-touchscree.patch | grep -A 20 -B 5 "display\|panel\|size"Repository: armbian/build
Length of output: 1470
🏁 Script executed:
# Find all nt36xxx/nt36523 references to see which devices actually use it
rg "nt36523|nt36xxx" patch/kernel/archive/sm8250-6.18/ | head -50Repository: armbian/build
Length of output: 7286
🏁 Script executed:
# Check if there are other touchscreen drivers (non-nt36xxx) for SM8250 devices
rg "touchscreen|compatible" patch/kernel/archive/sm8250-6.18/*.patch | grep -v "nt36" | head -20Repository: armbian/build
Length of output: 2445
Consider making panel dimensions device-specific or configurable.
The resolution calculation uses hardcoded panel dimensions (148mm × 237mm) that apply universally to all devices using the nt36xxx driver. While the SM8250 currently has limited nt36xxx adoption (Xiaomi Elish, OnePlus instantnoodlep), these devices may have different screen sizes. Hardcoding values could result in incorrect pen input resolution scaling if devices with different physical panel dimensions are added in the future.
Device-specific configuration through device tree properties or per-device constants would provide better maintainability and prevent issues as support expands.
🤖 Prompt for AI Agents
In @patch/kernel/archive/sm8250-6.18/0020-nt36xxx-add-pen-input-resolution.patch
around lines 35 - 36, The hardcoded
PANEL_DEFAULT_WIDTH_MM/PANEL_DEFAULT_HEIGHT_MM values used in the nt36xxx code
when calling input_abs_set_res(ts->pen_input_dev, ABS_X/ABS_Y, ...) should be
made device-specific: read panel physical width/height (e.g.,
"pen-panel-width-mm" and "pen-panel-height-mm" or reuse existing panel size DT
properties) from the device tree in the driver probe (where struct ts is
initialized) and fall back to the current defaults if missing, then use those
retrieved u32 values instead of the compile-time constants when computing the
resolution for input_abs_set_res; update the probe/struct initialization code to
store these values (e.g., ts->panel_width_mm / ts->panel_height_mm) and
reference them where ABS_X/ABS_Y resolutions are set so different devices can
provide correct dimensions.
| + return IRQ_HANDLED; | ||
| + } | ||
| + | ||
| + len = le32_to_cpu(header->length); |
There was a problem hiding this comment.
Critical: Type mismatch in endianness conversion.
Line 438 uses le32_to_cpu(header->length) but header->length is defined as __le16 (2 bytes) in the tcm_message_header structure (line 153). This reads 4 bytes instead of 2, potentially reading garbage data and causing len to have an incorrect value, which could lead to buffer overflows.
🐛 Proposed fix
- len = le32_to_cpu(header->length);
+ len = le16_to_cpu(header->length);📝 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.
| + len = le32_to_cpu(header->length); | |
| len = le16_to_cpu(header->length); |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0027-Input-touchscreen-add-Synaptics-TCM-oncell-S3908.patch
at line 438, The code calls le32_to_cpu(header->length) but
tcm_message_header::length is defined as __le16, so replace the 32-bit
conversion with le16_to_cpu(header->length) and ensure the receiving variable
(len) is an appropriate 16-bit type (e.g., u16 or unsigned int) or explicitly
cast to size_t when used for buffer sizes/limits; update any subsequent uses
that assume a 32-bit value to avoid sign/overflow issues.
| +static void gamepad_input_handler(struct gamepad_device *dev, | ||
| + struct gamepad_data *data) | ||
| +{ | ||
| + static unsigned long prev_states; | ||
| + unsigned long keys = data->data[0] | (data->data[1] << 8); | ||
| + unsigned long current_states = keys, changes; | ||
| + int i; | ||
| + struct input_dev *indev; | ||
| + | ||
| + indev = dev->dev_input; | ||
| + if (!indev) | ||
| + return; | ||
| + | ||
| + bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap)); | ||
| + | ||
| + for_each_set_bit(i, &changes, ARRAY_SIZE(keymap)) { | ||
| + input_report_key(indev, keymap[i], (current_states & BIT(i))); | ||
| + } | ||
| + | ||
| + input_report_abs(indev, ABS_HAT2X, | ||
| + 0x755 - (data->data[2] | (data->data[3] << 8))); | ||
| + input_report_abs(indev, ABS_HAT2Y, | ||
| + 0x755 - (data->data[4] | (data->data[5] << 8))); | ||
| + input_report_abs(indev, ABS_X, | ||
| + -(int16_t)(data->data[6] | (data->data[7] << 8))); | ||
| + input_report_abs(indev, ABS_Y, | ||
| + -(int16_t)(data->data[8] | (data->data[9] << 8))); | ||
| + input_report_abs(indev, ABS_RX, | ||
| + -(int16_t)(data->data[10] | (data->data[11] << 8))); | ||
| + input_report_abs(indev, ABS_RY, | ||
| + -(int16_t)(data->data[12] | (data->data[13] << 8))); | ||
| + | ||
| + input_sync(indev); | ||
| + prev_states = keys; | ||
| +} |
There was a problem hiding this comment.
Static prev_states breaks multi-device support; missing data length validation.
Two issues in this handler:
-
Static variable sharing:
prev_statesis function-static, meaning all device instances share the same state. If multiple Odin2 gamepads are connected, button state tracking will be corrupted. Move this tostruct gamepad_device. -
Missing bounds check: The handler accesses
data->data[0..13]but doesn't verifydata->data_len >= 14. If a malformed packet arrives with shorter data, this reads uninitialized memory.
Proposed fix
Add prev_states to the device struct:
struct gamepad_device {
struct device *dev;
struct gpio_desc *boot_gpio;
struct gpio_desc *enable_gpio;
struct gpio_desc *reset_gpio;
struct input_dev *dev_input;
+ unsigned long prev_states;
};Update the handler:
static void gamepad_input_handler(struct gamepad_device *dev,
struct gamepad_data *data)
{
- static unsigned long prev_states;
unsigned long keys = data->data[0] | (data->data[1] << 8);
- unsigned long current_states = keys, changes;
+ unsigned long current_states = keys, changes;
int i;
struct input_dev *indev;
indev = dev->dev_input;
if (!indev)
return;
- bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap));
+ if (data->data_len < 14)
+ return;
+
+ bitmap_xor(&changes, ¤t_states, &dev->prev_states, ARRAY_SIZE(keymap));
...
- prev_states = keys;
+ dev->prev_states = keys;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0038-Input-driver-for-AYN-Odin2-Gamepad.patch
around lines 141 - 175, Move the function-static prev_states into struct
gamepad_device (add an unsigned long prev_states field) and update
gamepad_input_handler to use dev->prev_states instead of the static variable;
also add a guard at the top of gamepad_input_handler that returns early if
data->data_len < 14 to prevent out-of-bounds reads, and keep the existing
bitmap_xor and for_each_set_bit logic but operate on &dev->prev_states and write
back dev->prev_states = keys after input_sync; reference symbols: struct
gamepad_device, prev_states, gamepad_input_handler, data->data_len, data->data,
keymap, ARRAY_SIZE(keymap).
| + memcpy(recv_data_buffer.header, data, 4); | ||
| + recv_data_buffer.frame_number = *(data + 4); | ||
| + recv_data_buffer.command = *(data + 5); | ||
| + recv_data_buffer.data_len = *(data + 6) | (*(data + 7) << 8); | ||
| + | ||
| + if (recv_data_buffer.data_len) { | ||
| + memset(&recv_data_buffer.data[0], 0, | ||
| + sizeof(recv_data_buffer.data)); | ||
| + memcpy(&recv_data_buffer.data[0], data + 8, | ||
| + recv_data_buffer.data_len); |
There was a problem hiding this comment.
Buffer overflow: data_len not validated before memcpy.
The data_len field is read directly from the packet (lines 212) but is never validated against MAX_DATA_LEN (64) before copying into recv_data_buffer.data. A malformed or malicious packet with data_len > 64 will cause a stack buffer overflow.
Additionally, the code should verify that count >= 8 + data_len + 1 (header + data + checksum) to ensure the packet actually contains the claimed data.
Proposed fix
recv_data_buffer.data_len = *(data + 6) | (*(data + 7) << 8);
+ if (recv_data_buffer.data_len > MAX_DATA_LEN) {
+ dev_warn_ratelimited(dev, "data_len %u exceeds max %u",
+ recv_data_buffer.data_len, MAX_DATA_LEN);
+ return count;
+ }
+
+ if (count < 8 + recv_data_buffer.data_len + 1) {
+ dev_warn_ratelimited(dev, "packet too short for claimed data_len");
+ return count;
+ }
+
if (recv_data_buffer.data_len) {
memset(&recv_data_buffer.data[0], 0,
sizeof(recv_data_buffer.data));| + gamepad_dev->boot_gpio = | ||
| + devm_gpiod_get_optional(dev, "boot", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(gamepad_dev->boot_gpio)) { | ||
| + ret = PTR_ERR(gamepad_dev->boot_gpio); | ||
| + dev_warn(dev, "Unable to get boot gpio: %d\n", ret); | ||
| + } | ||
| + | ||
| + gamepad_dev->enable_gpio = | ||
| + devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(gamepad_dev->enable_gpio)) { | ||
| + ret = PTR_ERR(gamepad_dev->enable_gpio); | ||
| + dev_warn(dev, "Unable to get enable gpio: %d\n", ret); | ||
| + } | ||
| + | ||
| + gamepad_dev->reset_gpio = | ||
| + devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(gamepad_dev->reset_gpio)) { | ||
| + ret = PTR_ERR(gamepad_dev->reset_gpio); | ||
| + dev_warn(dev, "Unable to get reset gpio: %d\n", ret); | ||
| + } | ||
| + | ||
| + if (gamepad_dev->boot_gpio) | ||
| + gpiod_set_value_cansleep(gamepad_dev->boot_gpio, 0); | ||
| + | ||
| + if (gamepad_dev->reset_gpio) | ||
| + gpiod_set_value_cansleep(gamepad_dev->reset_gpio, 0); | ||
| + | ||
| + msleep(100); | ||
| + | ||
| + if (gamepad_dev->enable_gpio) | ||
| + gpiod_set_value_cansleep(gamepad_dev->enable_gpio, 1); | ||
| + | ||
| + if (gamepad_dev->reset_gpio) | ||
| + gpiod_set_value_cansleep(gamepad_dev->reset_gpio, 1); |
There was a problem hiding this comment.
GPIO error pointers not cleared, leading to potential crash.
When devm_gpiod_get_optional returns an error, the code logs a warning but leaves the ERR_PTR value in the struct. The subsequent checks like if (gamepad_dev->boot_gpio) evaluate to true for error pointers, causing gpiod_set_value_cansleep to be called on an invalid pointer.
Proposed fix
gamepad_dev->boot_gpio =
devm_gpiod_get_optional(dev, "boot", GPIOD_OUT_HIGH);
if (IS_ERR(gamepad_dev->boot_gpio)) {
ret = PTR_ERR(gamepad_dev->boot_gpio);
dev_warn(dev, "Unable to get boot gpio: %d\n", ret);
+ gamepad_dev->boot_gpio = NULL;
}
gamepad_dev->enable_gpio =
devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
if (IS_ERR(gamepad_dev->enable_gpio)) {
ret = PTR_ERR(gamepad_dev->enable_gpio);
dev_warn(dev, "Unable to get enable gpio: %d\n", ret);
+ gamepad_dev->enable_gpio = NULL;
}
gamepad_dev->reset_gpio =
devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(gamepad_dev->reset_gpio)) {
ret = PTR_ERR(gamepad_dev->reset_gpio);
dev_warn(dev, "Unable to get reset gpio: %d\n", ret);
+ gamepad_dev->reset_gpio = NULL;
}🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0038-Input-driver-for-AYN-Odin2-Gamepad.patch
around lines 244 - 277, The GPIO pointers returned by devm_gpiod_get_optional
(boot_gpio, enable_gpio, reset_gpio) are left as ERR_PTR values after IS_ERR
logging and later treated as truthy, causing invalid gpiod_set_value_cansleep
calls; update the error handling so that after detecting
IS_ERR(gamepad_dev->boot_gpio/enable_gpio/reset_gpio) you set that field to NULL
(or otherwise clear the ERR_PTR) OR change the later conditional checks to use
!IS_ERR(...) && gamepad_dev->*_gpio before calling gpiod_set_value_cansleep,
ensuring only valid non-error gpio pointers are used.
| +static int spmi_haptics_probe(struct platform_device *pdev) | ||
| +{ | ||
| + struct spmi_haptics *haptics; | ||
| + struct device_node *node; | ||
| + struct input_dev *input_dev; | ||
| + int ret; | ||
| + u32 val; | ||
| + int i; | ||
| + | ||
| + haptics = devm_kzalloc(&pdev->dev, sizeof(*haptics), GFP_KERNEL); | ||
| + if (!haptics) | ||
| + return -ENOMEM; | ||
| + | ||
| + haptics->regmap = dev_get_regmap(pdev->dev.parent, NULL); | ||
| + if (!haptics->regmap) | ||
| + return -ENODEV; | ||
| + | ||
| + node = pdev->dev.of_node; | ||
| + | ||
| + haptics->dev = &pdev->dev; | ||
| + | ||
| + ret = of_property_read_u32(node, "reg", &haptics->base); | ||
| + if (ret < 0) { | ||
| + dev_err(haptics->dev, "Couldn't find reg in node = %s ret = %d\n", | ||
| + node->full_name, ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + haptics->play_irq = platform_get_irq_byname(pdev, "play"); | ||
| + if (haptics->play_irq < 0) { | ||
| + dev_err(&pdev->dev, "Unable to get play irq\n"); | ||
| + ret = haptics->play_irq; | ||
| + goto register_fail; | ||
| + } | ||
| + | ||
| + haptics->sc_irq = platform_get_irq_byname(pdev, "short"); | ||
| + if (haptics->sc_irq < 0) { | ||
| + dev_err(&pdev->dev, "Unable to get sc irq\n"); | ||
| + ret = haptics->sc_irq; | ||
| + goto register_fail; | ||
| + } | ||
| + | ||
| + // We only support LRAs for now | ||
| + haptics->actuator_type = HAP_TYPE_LRA; | ||
| + ret = of_property_read_u32(node, "qcom,actuator-type", &val); | ||
| + if (!ret) { | ||
| + haptics->actuator_type = val; | ||
| + } | ||
| + | ||
| + // Only buffer mode is currently supported | ||
| + haptics->play_mode = HAP_PLAY_BUFFER; | ||
| + ret = of_property_read_u32(node, "qcom,play-mode", &val); | ||
| + if (!ret) { | ||
| + if (val != HAP_PLAY_BUFFER) { | ||
| + dev_err(&pdev->dev, "qcom,play-mode (%d) isn't supported\n", val); | ||
| + ret = -EINVAL; | ||
| + goto register_fail; | ||
| + } | ||
| + haptics->play_mode = val; | ||
| + } | ||
| + | ||
| + ret = of_property_read_u32(node, "qcom,wave-play-rate-us", &val); | ||
| + if (!ret) { | ||
| + haptics->play_wave_rate = val; | ||
| + } else if (ret != -EINVAL) { | ||
| + dev_err(haptics->dev, "Unable to read play rate ret=%d\n", ret); | ||
| + goto register_fail; | ||
| + } | ||
| + | ||
| + haptics->play_wave_rate = | ||
| + clamp_t(u32, haptics->play_wave_rate, | ||
| + HAP_WAVE_PLAY_RATE_MIN_US, HAP_WAVE_PLAY_RATE_MAX_US); | ||
| + | ||
| + haptics->wave_shape = HAP_WAVE_SINE; | ||
| + ret = of_property_read_u32(node, "qcom,wave-shape", &val); | ||
| + if (!ret) { | ||
| + if (val != HAP_WAVE_SINE && val != HAP_WAVE_SQUARE) { | ||
| + dev_err(&pdev->dev, "qcom,wave-shape is invalid: %d\n", val); | ||
| + ret = -EINVAL; | ||
| + goto register_fail; | ||
| + } | ||
| + haptics->wave_shape = val; | ||
| + } | ||
| + | ||
| + haptics->brake_pat[0] = 0x3; | ||
| + haptics->brake_pat[1] = 0x3; | ||
| + haptics->brake_pat[2] = 0x2; | ||
| + haptics->brake_pat[3] = 0x1; | ||
| + | ||
| + ret = of_property_read_u8_array(node, "qcom,brake-pattern", haptics->brake_pat, 4); | ||
| + if (ret < 0 && ret != -EINVAL) { | ||
| + dev_err(&pdev->dev, "qcom,brake-pattern is invalid, ret = %d\n", ret); | ||
| + goto register_fail; | ||
| + } | ||
| + | ||
| + haptics->boost = | ||
| + devm_gpiod_get_optional(haptics->dev, "qcom,boost", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(haptics->boost)) { | ||
| + ret = PTR_ERR(haptics->boost); | ||
| + dev_warn(haptics->dev, "Unable to get boost gpio: %d\n", ret); | ||
| + } | ||
| + | ||
| + if (haptics->boost) | ||
| + gpiod_set_value_cansleep(haptics->boost, 1); | ||
| + | ||
| + haptics->current_limit = HAP_ILIM_400_MA; | ||
| + | ||
| + for (i = 0; i < HAP_WAVE_SAMP_LEN; i++) | ||
| + haptics->wave_samp[i] = HAP_WF_SAMP_MAX; | ||
| + | ||
| + ret = spmi_haptics_init(haptics); | ||
| + if (ret < 0) { | ||
| + dev_err(&pdev->dev, "Error initialising haptics, ret=%d\n", | ||
| + ret); | ||
| + goto register_fail; | ||
| + } | ||
| + | ||
| + platform_set_drvdata(pdev, haptics); | ||
| + | ||
| + input_dev = devm_input_allocate_device(&pdev->dev); | ||
| + if (!input_dev) | ||
| + return -ENOMEM; | ||
| + | ||
| + INIT_WORK(&haptics->work, spmi_haptics_work); | ||
| + haptics->haptics_input_dev = input_dev; | ||
| + | ||
| + input_dev->name = "spmi_haptics"; | ||
| + input_dev->id.version = 1; | ||
| + input_dev->close = spmi_haptics_close; | ||
| + input_set_drvdata(input_dev, haptics); | ||
| + // Figure out how to make this FF_PERIODIC | ||
| + input_set_capability(haptics->haptics_input_dev, EV_FF, FF_RUMBLE); | ||
| + | ||
| + ret = input_ff_create_memless(input_dev, NULL, | ||
| + spmi_haptics_play_effect); | ||
| + if (ret) { | ||
| + dev_err(&pdev->dev, | ||
| + "couldn't register vibrator as FF device\n"); | ||
| + goto register_fail; | ||
| + } | ||
| + | ||
| + ret = input_register_device(input_dev); | ||
| + if (ret) { | ||
| + dev_err(&pdev->dev, "couldn't register input device\n"); | ||
| + goto register_fail; | ||
| + } | ||
| + | ||
| + return 0; | ||
| + | ||
| +register_fail: | ||
| + cancel_work_sync(&haptics->work); | ||
| + mutex_destroy(&haptics->play_lock); | ||
| + | ||
| + return ret; | ||
| +} |
There was a problem hiding this comment.
Critical: Mutex never initialized before being destroyed.
The play_lock mutex is destroyed in the error path (line 989) and in the remove function (line 1011), but mutex_init(&haptics->play_lock) is never called in the probe function. This will cause undefined behavior.
🔧 Proposed fix
Add mutex initialization in the probe function after allocating the haptics structure:
haptics->dev = &pdev->dev;
+ mutex_init(&haptics->play_lock);
ret = of_property_read_u32(node, "reg", &haptics->base);📝 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.
| +static int spmi_haptics_probe(struct platform_device *pdev) | |
| +{ | |
| + struct spmi_haptics *haptics; | |
| + struct device_node *node; | |
| + struct input_dev *input_dev; | |
| + int ret; | |
| + u32 val; | |
| + int i; | |
| + | |
| + haptics = devm_kzalloc(&pdev->dev, sizeof(*haptics), GFP_KERNEL); | |
| + if (!haptics) | |
| + return -ENOMEM; | |
| + | |
| + haptics->regmap = dev_get_regmap(pdev->dev.parent, NULL); | |
| + if (!haptics->regmap) | |
| + return -ENODEV; | |
| + | |
| + node = pdev->dev.of_node; | |
| + | |
| + haptics->dev = &pdev->dev; | |
| + | |
| + ret = of_property_read_u32(node, "reg", &haptics->base); | |
| + if (ret < 0) { | |
| + dev_err(haptics->dev, "Couldn't find reg in node = %s ret = %d\n", | |
| + node->full_name, ret); | |
| + return ret; | |
| + } | |
| + | |
| + haptics->play_irq = platform_get_irq_byname(pdev, "play"); | |
| + if (haptics->play_irq < 0) { | |
| + dev_err(&pdev->dev, "Unable to get play irq\n"); | |
| + ret = haptics->play_irq; | |
| + goto register_fail; | |
| + } | |
| + | |
| + haptics->sc_irq = platform_get_irq_byname(pdev, "short"); | |
| + if (haptics->sc_irq < 0) { | |
| + dev_err(&pdev->dev, "Unable to get sc irq\n"); | |
| + ret = haptics->sc_irq; | |
| + goto register_fail; | |
| + } | |
| + | |
| + // We only support LRAs for now | |
| + haptics->actuator_type = HAP_TYPE_LRA; | |
| + ret = of_property_read_u32(node, "qcom,actuator-type", &val); | |
| + if (!ret) { | |
| + haptics->actuator_type = val; | |
| + } | |
| + | |
| + // Only buffer mode is currently supported | |
| + haptics->play_mode = HAP_PLAY_BUFFER; | |
| + ret = of_property_read_u32(node, "qcom,play-mode", &val); | |
| + if (!ret) { | |
| + if (val != HAP_PLAY_BUFFER) { | |
| + dev_err(&pdev->dev, "qcom,play-mode (%d) isn't supported\n", val); | |
| + ret = -EINVAL; | |
| + goto register_fail; | |
| + } | |
| + haptics->play_mode = val; | |
| + } | |
| + | |
| + ret = of_property_read_u32(node, "qcom,wave-play-rate-us", &val); | |
| + if (!ret) { | |
| + haptics->play_wave_rate = val; | |
| + } else if (ret != -EINVAL) { | |
| + dev_err(haptics->dev, "Unable to read play rate ret=%d\n", ret); | |
| + goto register_fail; | |
| + } | |
| + | |
| + haptics->play_wave_rate = | |
| + clamp_t(u32, haptics->play_wave_rate, | |
| + HAP_WAVE_PLAY_RATE_MIN_US, HAP_WAVE_PLAY_RATE_MAX_US); | |
| + | |
| + haptics->wave_shape = HAP_WAVE_SINE; | |
| + ret = of_property_read_u32(node, "qcom,wave-shape", &val); | |
| + if (!ret) { | |
| + if (val != HAP_WAVE_SINE && val != HAP_WAVE_SQUARE) { | |
| + dev_err(&pdev->dev, "qcom,wave-shape is invalid: %d\n", val); | |
| + ret = -EINVAL; | |
| + goto register_fail; | |
| + } | |
| + haptics->wave_shape = val; | |
| + } | |
| + | |
| + haptics->brake_pat[0] = 0x3; | |
| + haptics->brake_pat[1] = 0x3; | |
| + haptics->brake_pat[2] = 0x2; | |
| + haptics->brake_pat[3] = 0x1; | |
| + | |
| + ret = of_property_read_u8_array(node, "qcom,brake-pattern", haptics->brake_pat, 4); | |
| + if (ret < 0 && ret != -EINVAL) { | |
| + dev_err(&pdev->dev, "qcom,brake-pattern is invalid, ret = %d\n", ret); | |
| + goto register_fail; | |
| + } | |
| + | |
| + haptics->boost = | |
| + devm_gpiod_get_optional(haptics->dev, "qcom,boost", GPIOD_OUT_HIGH); | |
| + if (IS_ERR(haptics->boost)) { | |
| + ret = PTR_ERR(haptics->boost); | |
| + dev_warn(haptics->dev, "Unable to get boost gpio: %d\n", ret); | |
| + } | |
| + | |
| + if (haptics->boost) | |
| + gpiod_set_value_cansleep(haptics->boost, 1); | |
| + | |
| + haptics->current_limit = HAP_ILIM_400_MA; | |
| + | |
| + for (i = 0; i < HAP_WAVE_SAMP_LEN; i++) | |
| + haptics->wave_samp[i] = HAP_WF_SAMP_MAX; | |
| + | |
| + ret = spmi_haptics_init(haptics); | |
| + if (ret < 0) { | |
| + dev_err(&pdev->dev, "Error initialising haptics, ret=%d\n", | |
| + ret); | |
| + goto register_fail; | |
| + } | |
| + | |
| + platform_set_drvdata(pdev, haptics); | |
| + | |
| + input_dev = devm_input_allocate_device(&pdev->dev); | |
| + if (!input_dev) | |
| + return -ENOMEM; | |
| + | |
| + INIT_WORK(&haptics->work, spmi_haptics_work); | |
| + haptics->haptics_input_dev = input_dev; | |
| + | |
| + input_dev->name = "spmi_haptics"; | |
| + input_dev->id.version = 1; | |
| + input_dev->close = spmi_haptics_close; | |
| + input_set_drvdata(input_dev, haptics); | |
| + // Figure out how to make this FF_PERIODIC | |
| + input_set_capability(haptics->haptics_input_dev, EV_FF, FF_RUMBLE); | |
| + | |
| + ret = input_ff_create_memless(input_dev, NULL, | |
| + spmi_haptics_play_effect); | |
| + if (ret) { | |
| + dev_err(&pdev->dev, | |
| + "couldn't register vibrator as FF device\n"); | |
| + goto register_fail; | |
| + } | |
| + | |
| + ret = input_register_device(input_dev); | |
| + if (ret) { | |
| + dev_err(&pdev->dev, "couldn't register input device\n"); | |
| + goto register_fail; | |
| + } | |
| + | |
| + return 0; | |
| + | |
| +register_fail: | |
| + cancel_work_sync(&haptics->work); | |
| + mutex_destroy(&haptics->play_lock); | |
| + | |
| + return ret; | |
| +} | |
| static int spmi_haptics_probe(struct platform_device *pdev) | |
| { | |
| struct spmi_haptics *haptics; | |
| struct device_node *node; | |
| struct input_dev *input_dev; | |
| int ret; | |
| u32 val; | |
| int i; | |
| haptics = devm_kzalloc(&pdev->dev, sizeof(*haptics), GFP_KERNEL); | |
| if (!haptics) | |
| return -ENOMEM; | |
| haptics->regmap = dev_get_regmap(pdev->dev.parent, NULL); | |
| if (!haptics->regmap) | |
| return -ENODEV; | |
| node = pdev->dev.of_node; | |
| haptics->dev = &pdev->dev; | |
| mutex_init(&haptics->play_lock); | |
| ret = of_property_read_u32(node, "reg", &haptics->base); | |
| if (ret < 0) { | |
| dev_err(haptics->dev, "Couldn't find reg in node = %s ret = %d\n", | |
| node->full_name, ret); | |
| return ret; | |
| } | |
| haptics->play_irq = platform_get_irq_byname(pdev, "play"); | |
| if (haptics->play_irq < 0) { | |
| dev_err(&pdev->dev, "Unable to get play irq\n"); | |
| ret = haptics->play_irq; | |
| goto register_fail; | |
| } | |
| haptics->sc_irq = platform_get_irq_byname(pdev, "short"); | |
| if (haptics->sc_irq < 0) { | |
| dev_err(&pdev->dev, "Unable to get sc irq\n"); | |
| ret = haptics->sc_irq; | |
| goto register_fail; | |
| } | |
| // We only support LRAs for now | |
| haptics->actuator_type = HAP_TYPE_LRA; | |
| ret = of_property_read_u32(node, "qcom,actuator-type", &val); | |
| if (!ret) { | |
| haptics->actuator_type = val; | |
| } | |
| // Only buffer mode is currently supported | |
| haptics->play_mode = HAP_PLAY_BUFFER; | |
| ret = of_property_read_u32(node, "qcom,play-mode", &val); | |
| if (!ret) { | |
| if (val != HAP_PLAY_BUFFER) { | |
| dev_err(&pdev->dev, "qcom,play-mode (%d) isn't supported\n", val); | |
| ret = -EINVAL; | |
| goto register_fail; | |
| } | |
| haptics->play_mode = val; | |
| } | |
| ret = of_property_read_u32(node, "qcom,wave-play-rate-us", &val); | |
| if (!ret) { | |
| haptics->play_wave_rate = val; | |
| } else if (ret != -EINVAL) { | |
| dev_err(haptics->dev, "Unable to read play rate ret=%d\n", ret); | |
| goto register_fail; | |
| } | |
| haptics->play_wave_rate = | |
| clamp_t(u32, haptics->play_wave_rate, | |
| HAP_WAVE_PLAY_RATE_MIN_US, HAP_WAVE_PLAY_RATE_MAX_US); | |
| haptics->wave_shape = HAP_WAVE_SINE; | |
| ret = of_property_read_u32(node, "qcom,wave-shape", &val); | |
| if (!ret) { | |
| if (val != HAP_WAVE_SINE && val != HAP_WAVE_SQUARE) { | |
| dev_err(&pdev->dev, "qcom,wave-shape is invalid: %d\n", val); | |
| ret = -EINVAL; | |
| goto register_fail; | |
| } | |
| haptics->wave_shape = val; | |
| } | |
| haptics->brake_pat[0] = 0x3; | |
| haptics->brake_pat[1] = 0x3; | |
| haptics->brake_pat[2] = 0x2; | |
| haptics->brake_pat[3] = 0x1; | |
| ret = of_property_read_u8_array(node, "qcom,brake-pattern", haptics->brake_pat, 4); | |
| if (ret < 0 && ret != -EINVAL) { | |
| dev_err(&pdev->dev, "qcom,brake-pattern is invalid, ret = %d\n", ret); | |
| goto register_fail; | |
| } | |
| haptics->boost = | |
| devm_gpiod_get_optional(haptics->dev, "qcom,boost", GPIOD_OUT_HIGH); | |
| if (IS_ERR(haptics->boost)) { | |
| ret = PTR_ERR(haptics->boost); | |
| dev_warn(haptics->dev, "Unable to get boost gpio: %d\n", ret); | |
| } | |
| if (haptics->boost) | |
| gpiod_set_value_cansleep(haptics->boost, 1); | |
| haptics->current_limit = HAP_ILIM_400_MA; | |
| for (i = 0; i < HAP_WAVE_SAMP_LEN; i++) | |
| haptics->wave_samp[i] = HAP_WF_SAMP_MAX; | |
| ret = spmi_haptics_init(haptics); | |
| if (ret < 0) { | |
| dev_err(&pdev->dev, "Error initialising haptics, ret=%d\n", | |
| ret); | |
| goto register_fail; | |
| } | |
| platform_set_drvdata(pdev, haptics); | |
| input_dev = devm_input_allocate_device(&pdev->dev); | |
| if (!input_dev) | |
| return -ENOMEM; | |
| INIT_WORK(&haptics->work, spmi_haptics_work); | |
| haptics->haptics_input_dev = input_dev; | |
| input_dev->name = "spmi_haptics"; | |
| input_dev->id.version = 1; | |
| input_dev->close = spmi_haptics_close; | |
| input_set_drvdata(input_dev, haptics); | |
| // Figure out how to make this FF_PERIODIC | |
| input_set_capability(haptics->haptics_input_dev, EV_FF, FF_RUMBLE); | |
| ret = input_ff_create_memless(input_dev, NULL, | |
| spmi_haptics_play_effect); | |
| if (ret) { | |
| dev_err(&pdev->dev, | |
| "couldn't register vibrator as FF device\n"); | |
| goto register_fail; | |
| } | |
| ret = input_register_device(input_dev); | |
| if (ret) { | |
| dev_err(&pdev->dev, "couldn't register input device\n"); | |
| goto register_fail; | |
| } | |
| return 0; | |
| register_fail: | |
| cancel_work_sync(&haptics->work); | |
| mutex_destroy(&haptics->play_lock); | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0039-input-add-Qualcomm-SPMI-haptics-driver.patch
around lines 838 - 992, The probe allocates the spmi_haptics struct but never
initializes the play_lock mutex, yet mutex_destroy(&haptics->play_lock) is
called on error and in remove; fix by calling mutex_init(&haptics->play_lock)
early in spmi_haptics_probe (e.g., immediately after devm_kzalloc and before any
code that can jump to register_fail or before INIT_WORK/spmi_haptics_init), so
the mutex is always initialized before any cancel_work_sync/ mutex_destroy paths
run.
| + return 0; | ||
| +} | ||
| + | ||
| +static SIMPLE_DEV_PM_OPS(spmi_haptics_pm_ops, spmi_haptics_suspend, NULL); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SIMPLE_DEV_PM_OPS deprecated Linux kernel 6.18 DEFINE_SIMPLE_DEV_PM_OPS
💡 Result:
Yes. In recent kernel trees the old SIMPLE_DEV_PM_OPS macro was marked deprecated — use DEFINE_SIMPLE_DEV_PM_OPS (together with pm_sleep_ptr when appropriate) instead. The change was made to let the compiler see the functions (avoiding __maybe_unused workarounds) while still allowing unused code to be dropped. See the kernel pm.h and the commit discussion. [1][2][3]
References
- pm.h showing DEFINE_SIMPLE_DEV_PM_OPS and the deprecation note for SIMPLE_DEV_PM_OPS. [1]
- LKML/patch discussion showing the new macros (pm_sleep_ptr / DEFINE_SIMPLE_DEV_PM_OPS) and deprecation. [2]
- Commit message explaining rationale (avoid unused-function warnings; use pm_sleep_ptr + DEFINE_SIMPLE_DEV_PM_OPS). [3]
🌐 Web query:
Linux kernel 6.18 pm.h SIMPLE_DEV_PM_OPS DEFINE_SIMPLE_DEV_PM_OPS
💡 Result:
Short summary:
-
DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) is the current helper macro that creates a struct dev_pm_ops named "name" with system-suspend callbacks set to suspend_fn/resume_fn (and NULL for runtime callbacks). Use this in drivers to supply both suspend/resume hooks in one line. [1][2]
-
SIMPLE_DEV_PM_OPS(...) is the older/deprecated form; it similarly defines a const struct dev_pm_ops but is marked deprecated in recent pm.h and may add __maybe_unused in some builds. Prefer DEFINE_SIMPLE_DEV_PM_OPS. [2][3]
-
There are EXPORT_* variants (EXPORT_SIMPLE_DEV_PM_OPS / EXPORT_GPL_SIMPLE_DEV_PM_OPS and CONFIG_PM-aware EXPORT[_GPL]_SIMPLE_DEV_PM_OPS) that handle symbol exporting (or produce a static dummy when CONFIG_PM is off) so callers can use pm_ptr()/pm_sleep_ptr() safely across CONFIG_PM builds. These were added in the DEV_PM_OPS macros rework. [3][0search0]
References:
- Kernel docs (Device PM operations). [1]
- include/linux/pm.h (macros DEFINE_SIMPLE_DEV_PM_OPS / SIMPLE_DEV_PM_OPS). [2]
- DEV_PM_OPS macros rework / patch discussion (EXPORT and CONFIG_PM behavior, deprecation note). [3]
Use DEFINE_SIMPLE_DEV_PM_OPS instead of deprecated SIMPLE_DEV_PM_OPS.
The SIMPLE_DEV_PM_OPS macro is deprecated in kernel 6.18. Replace it with DEFINE_SIMPLE_DEV_PM_OPS, which allows the compiler to see the functions while still eliminating unused code, removing the need for the __maybe_unused attribute.
♻️ Proposed refactor
-static int __maybe_unused spmi_haptics_suspend(struct device *dev)
+static int spmi_haptics_suspend(struct device *dev)
{
struct spmi_haptics *haptics = dev_get_drvdata(dev);
cancel_work_sync(&haptics->work);
spmi_haptics_disable(haptics);
return 0;
}
-static SIMPLE_DEV_PM_OPS(spmi_haptics_pm_ops, spmi_haptics_suspend, NULL);
+static DEFINE_SIMPLE_DEV_PM_OPS(spmi_haptics_pm_ops, spmi_haptics_suspend, NULL);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0039-input-add-Qualcomm-SPMI-haptics-driver.patch
at line 1004, Replace the deprecated SIMPLE_DEV_PM_OPS usage with
DEFINE_SIMPLE_DEV_PM_OPS: change the line creating spmi_haptics_pm_ops to use
DEFINE_SIMPLE_DEV_PM_OPS(spmi_haptics_pm_ops, spmi_haptics_suspend, NULL); so
the compiler sees spmi_haptics_suspend/resume correctly and you can remove any
__maybe_unused annotations that were compensating for the old macro.
From sm8250-6.12, to fixed touchscreen malfunction after waking from sleep. Signed-off-by: CodeChenL <[email protected]>
|
If 6.18 works we can bump current to 6.18 since it is aleady new lts. |
I have tested most of the features on xiaomi-elish and used it for a week. I think 6.18 is quite stable on sm8250. |
From sm8250-6.12, and made some modifications to be compatible with the latest kernel, to support retroidpocket-rp5 and retroidpocket-rpmini. Signed-off-by: CodeChenL <[email protected]>
|
Currently, all Armbian-supported SM8250 devices have available device trees. |
|
@tjstyle Can you help me test how it works on Retroid Pocket? |
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In
@patch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch:
- Around line 445-449: The code uses usb_online returned by
smb5_get_prop_usb_online without checking the function return code; update the
smb5_get_prop_usb_online call in the same function (like
smb5_apsd_get_charger_type) to check rc immediately and return rc on error
before testing usb_online, then only set *val = POWER_SUPPLY_STATUS_DISCHARGING
when rc == 0 and usb_online is false.
- Around line 21-28: The Kconfig entry for CHARGER_QCOM_SMB5 has a misleading
short description ("Qualcomm PMI8998 PMIC charger driver") copied from another
config; update the wording in the CHARGER_QCOM_SMB5 tristate/descriptive string
to correctly reference SMB5/PM8150B (e.g., "Qualcomm PM8150B/SMB5 PMIC charger
driver" or similar) so the short description matches the help text and driver
target.
- Around line 396-400: smb5_get_prop_usb_online's return value (rc) isn't
checked before using usb_online; verify rc after calling
smb5_get_prop_usb_online and handle errors before reading usb_online — e.g., if
rc < 0 return rc (or set *val to an appropriate default and return rc) instead
of using usb_online when the call failed; update the block around
smb5_get_prop_usb_online, usb_online, *val and POWER_SUPPLY_USB_TYPE_UNKNOWN
accordingly.
- Around line 728-729: The regmap write is missing the driver's base offset:
change the regmap_write call that currently passes BARK_BITE_WDOG_PET to use the
full register address by adding chip->base (i.e., pass chip->base +
BARK_BITE_WDOG_PET) so regmap_write(chip->regmap, chip->base +
BARK_BITE_WDOG_PET, BARK_BITE_WDOG_PET_BIT); keep the same regmap, bit constant
and error handling.
- Around line 597-599: The handler logs battery overvoltage when
CHARGER_ERROR_STATUS_BAT_OV_BIT is set but never updates the health output;
modify the function that checks stat (the block containing
CHARGER_ERROR_STATUS_BAT_OV_BIT and dev_err) to set *val =
POWER_SUPPLY_HEALTH_OVERVOLTAGE when that bit is present and ensure the function
returns that health (or short-circuits further checks) so callers don't
incorrectly report POWER_SUPPLY_HEALTH_GOOD.
In
@patch/kernel/archive/sm8250-6.18/0033-power-supply-Driver-for-Qualcomm-FG.patch:
- Around line 487-489: The debug message passed to dev_vdbg (the call using
chip->dev with the format string "Wrtiting address 0x%x with offset %d of length
%d to SRAM") contains a typo; update the format string to "Writing address 0x%x
with offset %d of length %d to SRAM" (or similar grammar like "with offset %d
and length %d") in that dev_vdbg invocation to correct the spelling.
- Around line 1157-1165: The code unconditionally assigns chip->status =
propval.intval even when power_supply_get_property() (called for psy ==
chip->chg_psy) fails, so guard the use of propval by checking the return value:
after calling power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS, &propval)
set chip->status = POWER_SUPPLY_STATUS_UNKNOWN on error (ret != 0) and only
assign chip->status = propval.intval when ret == 0; leave the
power_supply_changed(chip->batt_psy) call as-is.
- Around line 1223-1229: Typo in the dev_err message: change "sourete" to
"source" in the error string emitted after qcom_fg_masked_write; locate the
block that calls qcom_fg_masked_write with MEM_INTF_IMA_CFG(chip) and update the
dev_err call (using chip->dev) to log "Failed to configure interrupt source:
%d\n" so the message is correct and still includes the return code.
- Around line 497-517: The loop writes 'len' bytes via qcom_fg_write to
MEM_INTF_WR_DATA0 but the data interface is 4 bytes wide, so it must only write
up to (4 - offset) bytes per iteration; change the loop in the function using
qcom_fg_write/MEM_INTF_WR_DATA0/MEM_INTF_ADDR_LSB to compute write_size =
min(len, 4 - offset), call qcom_fg_write with write_size (to MEM_INTF_WR_DATA0 +
offset), check ret and goto out on error, then advance addr by 4 (per-word
address increment), advance wr_data by write_size, subtract write_size from len,
and set offset = 0 before the next iteration. Ensure the initial address write
(qcom_fg_write to MEM_INTF_ADDR_LSB) remains unchanged and error-checked.
- Around line 1045-1049: The error message in the qcom_fg_read call is wrong:
change the dev_err invocation in the qcom_fg_read error path to indicate a read
failure (e.g., "Failed to read RD_DATA3: %d\n") instead of "Failed to write
RD_DATA3", so update the dev_err in the block after ret = qcom_fg_read(...) to
correctly reflect the operation (use the same dev_err(chip->dev, ...) call but
with "read" in the message).
- Around line 436-456: The loop can request more than the 4-byte SRAM register
window and corrupt data; compute a chunk = min(len, 4 - offset), call
qcom_fg_read(chip, rd_data, MEM_INTF_RD_DATA0(chip) + offset, chunk), then
update rd_data += chunk, len -= chunk, set offset = 0, and only advance addr by
4 when chunk == (4 - offset) (i.e., when you consumed the rest of the current
4-byte word); retain error checks around qcom_fg_write/qcom_fg_read and use the
same symbols (qcom_fg_write, qcom_fg_read, MEM_INTF_RD_DATA0, addr, rd_data,
len, offset) to locate and apply this fix.
In
@patch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch:
- Line 630: Fix inconsistent indentation by replacing mixed spaces with tabs for
the device tree properties: ensure the `color = <LED_COLOR_ID_RED>;` line uses a
leading tab (not spaces), the `enable-gpios = <...>;` property on the
`enable-gpios` line uses a leading tab, normalize the gpio configuration block
(the lines configuring GPIOs around the gpio entries at the block containing the
gpio setup on lines ~1066-1070) to use tabs for all indentation, and remove
trailing whitespace after the `"HPHR_OUT",` entry; keep tabs as the consistent
indentation style to match kernel DTS coding standards.
- Line 24: Remove the stray `/dts-v1/;` directive from the .dtsi include (it
should only be in top-level .dts files); locate the line containing `/dts-v1/;`
(as shown in the diff) and delete it so the include file no longer contains the
directive, then re-run device-tree compilation to verify the error is resolved.
- Around line 4-5: The patch Subject is malformed: the "Subject:" header is
split and includes the Signed-off-by line; fix the commit header so the Subject
is a single clean line like "arm64: dts: qcom: add SM8250 Retroid Pocket
variant" and move the "Signed-off-by: Teguh Sobirin <[email protected]>" into the
patch body/footer (commit message body) beneath the subject, ensuring no other
header lines are broken or wrapped across multiple lines.
🧹 Nitpick comments (11)
patch/kernel/archive/sm8250-6.18/0033-power-supply-Driver-for-Qualcomm-FG.patch (2)
743-743: Missing space in temperature calculation.Missing space between minus operator and constant for consistency with coding style.
✨ Proposed fix
- *val = temp -2730; + *val = temp - 2730; return 0;
1386-1387: Code style: Add space afterifkeyword.The Linux kernel coding style prefers a space after control flow keywords (
if,while,for). This pattern appears in multiple locations throughout the file (lines 303, 332, 337, 371, 1386).✨ Example fix for this location
- if(chip->sram_wq) + if (chip->sram_wq) destroy_workqueue(chip->sram_wq);patch/kernel/archive/sm8250-6.18/0035-arm64-dts-qcom-pm8150b-Add-a-FG-Signed-off-by-Teguh-.patch (1)
4-5: Malformed patch subject line.The
Signed-off-bytrailer is concatenated into the Subject line instead of being in the commit message body. This should be:Subject: arm64: dts: qcom: pm8150b: Add a FG Signed-off-by: Teguh Sobirin <[email protected]>While this doesn't affect patch application, it results in a confusing git history entry.
patch/kernel/archive/sm8250-6.18/0034-arm64-dts-qcom-pm8150b-Add-a-charger-Signed-off-by-T.patch (1)
4-5: Same malformed subject line issue.The
Signed-off-bytrailer is concatenated into the Subject line. Same fix as the fuel gauge patch applies here.patch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch (7)
4-5: Same malformed subject line.All three patches in this series have the
Signed-off-byincorrectly concatenated into the Subject line.
307-308: Minor typo in comment.The comment says "pmi8150b" but should be "pm8150b" to match the driver name.
493-493: Trailing whitespace.Line 493 has trailing whitespace after the multiplication operator.
521-523: Consistent pattern: unchecked return value.Same pattern -
smb5_get_prop_usb_onlinereturn value isn't checked. Consider a consistent error handling approach across all call sites.
736-742: Static descriptor name is misleading but overwritten.The static
.name = "pmi8998_charger"doesn't match the actual device, but the probe function correctly generates the name from match data. Consider using a generic placeholder like"smb5_charger"for clarity.Also, Line 741 has inconsistent indentation (extra leading space).
935-937: Variable reuse reduces clarity.The
rcvariable is used both for the calculated voltage value and the return code fromregmap_update_bits. While functional, this is confusing. Consider using a separate variable for the calculation.Suggested improvement
- rc = (chip->batt_info->voltage_max_design_uv - 3487500) / 7500 + 1; - rc = regmap_update_bits(chip->regmap, chip->base + FLOAT_VOLTAGE_CFG, - FLOAT_VOLTAGE_SETTING_MASK, rc); + int vfloat = (chip->batt_info->voltage_max_design_uv - 3487500) / 7500 + 1; + rc = regmap_update_bits(chip->regmap, chip->base + FLOAT_VOLTAGE_CFG, + FLOAT_VOLTAGE_SETTING_MASK, vfloat);
972-972: Typo: "sentinal" should be "sentinel".Common typo in kernel driver boilerplate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
config/sources/families/sm8250.confpatch/kernel/archive/sm8250-6.18/0000.patching_config.yamlpatch/kernel/archive/sm8250-6.18/0029-arm64-dts-qcom-Add-venus-firmware-for-OnePlus-8T.patchpatch/kernel/archive/sm8250-6.18/0031-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patchpatch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patchpatch/kernel/archive/sm8250-6.18/0033-power-supply-Driver-for-Qualcomm-FG.patchpatch/kernel/archive/sm8250-6.18/0034-arm64-dts-qcom-pm8150b-Add-a-charger-Signed-off-by-T.patchpatch/kernel/archive/sm8250-6.18/0035-arm64-dts-qcom-pm8150b-Add-a-FG-Signed-off-by-Teguh-.patchpatch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch
🚧 Files skipped from review as they are similar to previous changes (1)
- patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Learnt from: rpardini
Repo: armbian/build PR: 9159
File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161
Timestamp: 2026-01-03T20:46:29.189Z
Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches.
<!-- </add_learning>
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: EvilOlaf
Repo: armbian/build PR: 9086
File: patch/kernel/archive/uefi-arm64-6.19/net-stmicro-stmmac-Phytium-onboard-ethernet-drivers-and-ACPI-glue-for-6.x.patch.disabled:625-633
Timestamp: 2025-12-21T10:28:48.455Z
Learning: The patch "net-stmicro-stmmac-Phytium-onboard-ethernet-drivers-and-ACPI-glue-for-6.x.patch" in patch/kernel/archive/uefi-arm64-6.19/ is known to break compilation on kernel 6.19 and is therefore disabled (has .disabled extension).
Learnt from: tabrisnet
Repo: armbian/build PR: 8913
File: config/sources/families/k3-beagle.conf:16-16
Timestamp: 2025-11-10T22:05:40.490Z
Learning: In the Armbian build system, kernel branches using non-mainline/vendor forks (like BeagleBoard's linux repository) should be named "vendor" or "vendor-rt" rather than "current" or "edge". The "current" and "edge" naming is reserved for mainline kernel branches. This affects both the case statement in family config files (e.g., `vendor | vendor-rt)` instead of `current | current-rt)`) and the corresponding KERNEL_TARGET declarations in board config files.
Learnt from: rpardini
Repo: armbian/build PR: 8879
File: config/sources/families/uefi-x86.conf:0-0
Timestamp: 2025-11-06T15:36:04.682Z
Learning: As of PR #8879, the uefi-x86 family in the Armbian build system now includes kernel patches for the first time. The current and edge branches for uefi-x86 are specifically configured for Apple T2-based x86 machines, including T2-specific patches from the linux-t2 project and custom kernel configuration options for Apple hardware drivers.
📚 Learning: 2025-12-17T05:09:02.306Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Applied to files:
patch/kernel/archive/sm8250-6.18/0031-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patchpatch/kernel/archive/sm8250-6.18/0035-arm64-dts-qcom-pm8150b-Add-a-FG-Signed-off-by-Teguh-.patchpatch/kernel/archive/sm8250-6.18/0034-arm64-dts-qcom-pm8150b-Add-a-charger-Signed-off-by-T.patchpatch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patchpatch/kernel/archive/sm8250-6.18/0029-arm64-dts-qcom-Add-venus-firmware-for-OnePlus-8T.patchpatch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch
📚 Learning: 2025-12-19T13:56:45.124Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Applied to files:
patch/kernel/archive/sm8250-6.18/0031-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patchpatch/kernel/archive/sm8250-6.18/0034-arm64-dts-qcom-pm8150b-Add-a-charger-Signed-off-by-T.patchpatch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patchpatch/kernel/archive/sm8250-6.18/0033-power-supply-Driver-for-Qualcomm-FG.patchconfig/sources/families/sm8250.confpatch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch
📚 Learning: 2025-11-20T18:20:11.985Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8968
File: patch/u-boot/u-boot-sunxi/arm64-dts-sun50i-h6-orangepi.dtsi-Rollback-r_rsb-to-r_i2c.patch:36-36
Timestamp: 2025-11-20T18:20:11.985Z
Learning: The rewrite-patches tool (REWRITE_PATCHES=yes) in the Armbian build system can inadvertently introduce semantic changes when the u-boot/kernel git base revision differs from expected state. The tool applies patches, commits them, and re-exports them using git format-patch, which can cause the re-exported patch to reflect the base revision's state rather than preserving the original patch intent. This is particularly problematic for device tree changes like interrupt specifications. The tool currently lacks validation mechanisms to detect such semantic drift, and affected patches must be manually corrected after rewriting.
Applied to files:
patch/kernel/archive/sm8250-6.18/0031-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patchpatch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch
📚 Learning: 2025-03-31T22:20:41.849Z
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
Applied to files:
patch/kernel/archive/sm8250-6.18/0031-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patch
📚 Learning: 2026-01-10T04:28:58.758Z
Learnt from: kasimling
Repo: armbian/build PR: 9177
File: patch/kernel/archive/rockchip64-6.18/rk3588-1212-arm64-dts-Automatic-fan-speed-and-USB-3.0-Type-A-por.patch:37-41
Timestamp: 2026-01-10T04:28:58.758Z
Learning: When reviewing Linux kernel device-tree bindings involving thermal cooling devices, THERMAL_NO_LIMIT can be used for both min_state (lower) and max_state (upper). If used as lower/min_state, the framework converts it to 0; if used as upper/max_state, it converts to the cooling device's max_state. Verify patch entries like: cooling-device = <&fan THERMAL_NO_LIMIT 1> are valid and correctly reflect the intended bounds. Ensure consistent interpretation of NO_LIMIT in both min and max contexts and that the resulting values align with the device's actual limits.
Applied to files:
patch/kernel/archive/sm8250-6.18/0031-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patchpatch/kernel/archive/sm8250-6.18/0035-arm64-dts-qcom-pm8150b-Add-a-FG-Signed-off-by-Teguh-.patchpatch/kernel/archive/sm8250-6.18/0034-arm64-dts-qcom-pm8150b-Add-a-charger-Signed-off-by-T.patchpatch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patchpatch/kernel/archive/sm8250-6.18/0029-arm64-dts-qcom-Add-venus-firmware-for-OnePlus-8T.patchpatch/kernel/archive/sm8250-6.18/0033-power-supply-Driver-for-Qualcomm-FG.patchpatch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch
📚 Learning: 2025-12-17T15:46:22.709Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 9096
File: patch/kernel/archive/sunxi-6.18/patches.armbian/arm64-dts-sun50i-h616-add-overlays.patch:4-5
Timestamp: 2025-12-17T15:46:22.709Z
Learning: In Armbian kernel patches, when patches are renamed or reorganized, the old Subject line may be intentionally preserved in the patch header (as a second Subject line) for archival/historical purposes to document the previous patch name. This is an Armbian-specific practice for tracking patch evolution and should not be flagged as an error when the maintainer has intentionally included it for documentation.
Applied to files:
patch/kernel/archive/sm8250-6.18/0035-arm64-dts-qcom-pm8150b-Add-a-FG-Signed-off-by-Teguh-.patchpatch/kernel/archive/sm8250-6.18/0034-arm64-dts-qcom-pm8150b-Add-a-charger-Signed-off-by-T.patchpatch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patchpatch/kernel/archive/sm8250-6.18/0029-arm64-dts-qcom-Add-venus-firmware-for-OnePlus-8T.patch
📚 Learning: 2026-01-07T18:21:41.085Z
Learnt from: rpardini
Repo: armbian/build PR: 9180
File: patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml:4-8
Timestamp: 2026-01-07T18:21:41.085Z
Learning: In Armbian patch/kernel/archive/**/0000.patching_config.yaml files, the version metadata fields (name, branch, last-known-good-tag, kind, type) under the "info stuff" comment are not used by the patching scripts. When reviewing these files, do not suggest updating or fixing these metadata fields. Instead, suggest removing them entirely as they serve no functional purpose.
Applied to files:
patch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch
📚 Learning: 2026-01-03T20:46:29.189Z
Learnt from: rpardini
Repo: armbian/build PR: 9159
File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161
Timestamp: 2026-01-03T20:46:29.189Z
Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches.
<!-- </add_learning>
Applied to files:
patch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patchconfig/sources/families/sm8250.conf
📚 Learning: 2025-11-08T06:39:24.527Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8886
File: patch/kernel/rk35xx-vendor-6.1/dt/rk3528-nanopi-rev01.dts:9-9
Timestamp: 2025-11-08T06:39:24.527Z
Learning: In Armbian's Rockchip vendor kernel branches (rk35xx-vendor), device tree files may not require the same headers as mainline Linux. For example, input key definitions like KEY_BACK and KEY_VOLUMEUP may be handled differently in BSP kernels and don't necessarily need explicit includes like <dt-bindings/input/input.h> that would be required in mainline kernel device trees.
Applied to files:
patch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch
📚 Learning: 2025-06-25T03:42:09.086Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-11-10T22:05:40.490Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8913
File: config/sources/families/k3-beagle.conf:16-16
Timestamp: 2025-11-10T22:05:40.490Z
Learning: In the Armbian build system, kernel branches using non-mainline/vendor forks (like BeagleBoard's linux repository) should be named "vendor" or "vendor-rt" rather than "current" or "edge". The "current" and "edge" naming is reserved for mainline kernel branches. This affects both the case statement in family config files (e.g., `vendor | vendor-rt)` instead of `current | current-rt)`) and the corresponding KERNEL_TARGET declarations in board config files.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-11-02T20:49:56.719Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-10-26T10:41:35.118Z
Learnt from: HackingGate
Repo: armbian/build PR: 8665
File: config/boards/photonicat2.csc:4-4
Timestamp: 2025-10-26T10:41:35.118Z
Learning: In the Armbian build system, rk3576 boards consistently use BOARDFAMILY="rk35xx" for both vendor and edge kernel targets. The rk35xx family configuration sources rockchip64_common.inc, which provides edge and current kernel branch definitions, making these branches available even though they're not defined directly in rk35xx.conf.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-06-25T03:40:52.109Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-12-12T23:09:56.813Z
Learnt from: tabrisnet
Repo: armbian/build PR: 9058
File: config/sources/families/spacemit.conf:39-45
Timestamp: 2025-12-12T23:09:56.813Z
Learning: In Armbian build configs for vendor kernel sources, prefer the following branch naming conventions: use 'vendor' or 'vendor-rt' for stable vendor releases, and 'vendor-edge' for bleeding-edge/pre-release vendor versions. The 'edge' naming without the 'vendor-' prefix is reserved for mainline kernel branches. Apply this pattern to family config files under config/sources/families (e.g., spacemit.conf) to ensure consistent vendor kernel sourcing naming across the repository.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-09-14T06:29:18.958Z
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:64-70
Timestamp: 2025-09-14T06:29:18.958Z
Learning: In the Armbian build system, vendor branch configurations in family files are designed to be shared across multiple SoCs within the same family that use the same vendor kernel tree. For example, rk35xx and rockchip-rk3588 families both use identical vendor branch settings (same KERNELSOURCE, KERNELBRANCH, and KERNELPATCHDIR), demonstrating that vendor branches are intentionally generic rather than SoC-specific.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-07-21T04:12:02.439Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-07-21T04:12:02.439Z
Learning: In the Armbian build system, for recurring maintenance tasks like kernel version bumping, TODO comments should use generic version formats (e.g., "MAJOR.MINOR-rc1") rather than specific version numbers (e.g., "6.17-rc1") to avoid the need for frequent comment updates that would create unnecessary maintenance overhead.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-10-13T02:26:18.249Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8746
File: config/sources/families/filogic.conf:61-68
Timestamp: 2025-10-13T02:26:18.249Z
Learning: In the Armbian build system, the "edge" kernel branch is intended to be bleeding-edge and experimental. It does not require hardware testing or stability verification before merging, as it's expected to contain potentially unstable or untested code for early adopters and developers.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-04-30T16:16:47.150Z
Learnt from: The-going
Repo: armbian/build PR: 8147
File: config/sources/families/include/sunxi64_common.inc:38-39
Timestamp: 2025-04-30T16:16:47.150Z
Learning: The Armbian build system references Linux kernel versions in the form "tag:v6.14.4" in the KERNELBRANCH variable, even when point release tags might not be directly visible in the upstream repository in the same form.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-10-14T05:08:11.785Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8754
File: config/boards/bestv-r3300-l.csc:14-16
Timestamp: 2025-10-14T05:08:11.785Z
Learning: In the Armbian build system, BOOTBRANCH_BOARD is a valid framework variable used as a fallback when BOOTBRANCH is unset. The framework checks BOOTBRANCH_BOARD before applying the default bootloader branch value (see config/sources/common.conf). Board configuration files can use BOOTBRANCH_BOARD to specify the bootloader branch.
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-08-21T08:10:59.502Z
Learnt from: leggewie
Repo: armbian/build PR: 8524
File: config/boards/orangepi2.csc:6-6
Timestamp: 2025-08-21T08:10:59.502Z
Learning: Not all Armbian boards support all kernel versions (legacy, current, edge). Some boards may only support specific kernel versions due to hardware limitations or lack of mainline support, which is why their KERNEL_TARGET contains only the supported options (e.g., just "legacy").
Applied to files:
config/sources/families/sm8250.conf
📚 Learning: 2025-05-05T12:35:07.143Z
Learnt from: Grippy98
Repo: armbian/build PR: 8152
File: lib/functions/configuration/interactive.sh:209-266
Timestamp: 2025-05-05T12:35:07.143Z
Learning: For the interactive kernel selection in Armbian, KERNEL_MAJOR_MINOR and KERNEL_DESCRIPTION are parsed from family.conf but deliberately not set as environment variables to avoid potential interference with other parts of the build system.
Applied to files:
config/sources/families/sm8250.conf
🔇 Additional comments (13)
config/sources/families/sm8250.conf (2)
18-22: Verifylinux-6.18.yis the intended ref forcurrent(and exists in the expected kernel source).Looks fine structurally, but since this relies on external branch naming, please confirm the source you’re pulling from actually exposes
linux-6.18.yand Armbian’s kernel fetch logic resolvesbranch:linux-6.18.yas expected. (As per guidelines: external dependency/API validity should be verified.)
24-27: The missingKERNELBRANCHis intentional and not a bug.For mainline kernel branches (like "edge"), Armbian's build system automatically sets
KERNELBRANCHvia themainline_kernel_decide_version__900_defaults()hook inconfig/sources/mainline-kernel.conf.sh. WhenKERNEL_MAJOR_MINOR="6.19"is declared without an explicitKERNELBRANCH, the hook deterministically appliesKERNELBRANCH="branch:linux-6.19.y".This pattern is intentional and used consistently across many family configs (k3_common.inc, mvebu.conf, sm8550.conf, imx6.conf, etc.). The k3_common.inc file explicitly documents this:
# KERNELBRANCH defined in mainline-kernel.conf.sh.The build system is not falling back to an unintended default—it's executing a well-designed, explicit mechanism. The proposed fix would be redundant.
Likely an incorrect or invalid review comment.
patch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch (2)
1455-1560: RP5 variant structure looks good.The RP5 device tree properly includes the
/dts-v1/;directive (appropriate for .dts files), includes the common dtsi, and correctly overrides device-specific properties like framebuffer dimensions, LED groupings, and touchscreen configuration.
1561-1664: RPmini variant structure looks good.The RPmini device tree follows the same structure as RP5, with appropriate hardware-specific differences (960×1280 display vs 1080×1920, 90° rotation vs 270°). The variant properly reuses the common dtsi and overrides only device-specific properties.
patch/kernel/archive/sm8250-6.18/0031-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patch (3)
43-46: Pinctrl configuration follows established pattern.The qup_uart16_default state correctly defines the standard 2-pin UART configuration (TX/RX on gpio50, gpio51) and follows the same pattern as other UART pinctrl entries in this file (e.g., qup_uart12_default, qup_uart17_default).
1-10: Fix the commit message format.The commit subject line incorrectly includes the "Signed-off-by" text. The signature should be in the commit body, not the subject line.
Expected format:
Subject: arm64: dts: qcom: sm8250: add uart16 Signed-off-by: Teguh Sobirin <[email protected]>⛔ Skipped due to learnings
Learnt from: EvilOlaf Repo: armbian/build PR: 9096 File: patch/kernel/archive/sunxi-6.18/patches.armbian/arm64-dts-sun50i-h616-add-overlays.patch:4-5 Timestamp: 2025-12-17T15:46:22.709Z Learning: In Armbian kernel patches, when patches are renamed or reorganized, the old Subject line may be intentionally preserved in the patch header (as a second Subject line) for archival/historical purposes to document the previous patch name. This is an Armbian-specific practice for tracking patch evolution and should not be flagged as an error when the maintainer has intentionally included it for documentation.Learnt from: EvilOlaf Repo: armbian/build PR: 0 File: :0-0 Timestamp: 2025-12-17T05:09:02.306Z Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.Learnt from: EvilOlaf Repo: armbian/build PR: 8968 File: patch/u-boot/u-boot-sunxi/arm64-dts-sun50i-h6-orangepi.dtsi-Rollback-r_rsb-to-r_i2c.patch:36-36 Timestamp: 2025-11-20T18:20:11.985Z Learning: The rewrite-patches tool (REWRITE_PATCHES=yes) in the Armbian build system can inadvertently introduce semantic changes when the u-boot/kernel git base revision differs from expected state. The tool applies patches, commits them, and re-exports them using git format-patch, which can cause the re-exported patch to reflect the base revision's state rather than preserving the original patch intent. This is particularly problematic for device tree changes like interrupt specifications. The tool currently lacks validation mechanisms to detect such semantic drift, and affected patches must be manually corrected after rewriting.Learnt from: EvilOlaf Repo: armbian/build PR: 0 File: :0-0 Timestamp: 2025-12-19T13:56:45.124Z Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.Learnt from: rpardini Repo: armbian/build PR: 9180 File: patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml:4-8 Timestamp: 2026-01-07T18:21:41.085Z Learning: In Armbian patch/kernel/archive/**/0000.patching_config.yaml files, the version metadata fields (name, branch, last-known-good-tag, kind, type) under the "info stuff" comment are not used by the patching scripts. When reviewing these files, do not suggest updating or fixing these metadata fields. Instead, suggest removing them entirely as they serve no functional purpose.Learnt from: rpardini Repo: armbian/build PR: 9159 File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161 Timestamp: 2026-01-03T20:46:29.189Z Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches. <!-- </add_learning>
19-34: UART16 node definition is correct and properly ported.This patch is carried forward identically from sm8250-6.12, maintaining compatibility across the kernel version bump. The uart16 node shares address 0x888000 with the spi16 node (expected and correct for Qualcomm QUP blocks—these hardware units can be configured as SPI, I2C, or UART). Board-specific device trees should enable only one mode per QUP instance.
The node includes all required properties (compatible, clocks, interrupts, power domains, interconnects) and correctly sets status to "disabled" for the SoC-level device tree. No changes needed.
patch/kernel/archive/sm8250-6.18/0029-arm64-dts-qcom-Add-venus-firmware-for-OnePlus-8T.patch (1)
19-21: Use correct Venus firmware path from linux-firmware.The firmware path
qcom/sm8250/OnePlus/kebab/venus.mbndoes not match the linux-firmware directory structure. Venus firmware in linux-firmware is stored under versioned directories (qcom/venus-5.2/,qcom/venus-5.4/, etc.), not under device-specific subdirectories. For SM8250, usefirmware-name = "qcom/venus-5.4/venus.mbn"(orqcom/venus-5.2/venus.mbnfor older firmware). Alternatively, other OnePlus SM8250 devices in this patchset use"postmarketos/venus.mbn"— confirm which version is appropriate for OnePlus 8T and update accordingly.⛔ Skipped due to learnings
Learnt from: EvilOlaf Repo: armbian/build PR: 0 File: :0-0 Timestamp: 2025-12-17T05:09:02.306Z Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.Learnt from: paolosabatino Repo: armbian/build PR: 9171 File: patch/misc/bluetooth-rtl8723cs/bluetooth-btrtl-add-rtl8703bs.patch:34-34 Timestamp: 2026-01-06T21:17:42.268Z Learning: In Linux kernel btrtl.c driver patches, recent kernel practice removes the .bin extension from fw_name entries in ic_id_table[] to support multiple firmware file formats/versions. The driver's firmware-parsing code dynamically selects the correct file variant. When reviewing bluetooth-rtl patches, fw_name without .bin extension (e.g., "rtl_bt/rtl8723cs_xx_fw") is the modern correct pattern, not an error.Learnt from: EvilOlaf Repo: armbian/build PR: 9067 File: patch/kernel/archive/rockchip64-6.19/dt/rk3399-orangepi-4-lts.dts:869-875 Timestamp: 2025-12-23T04:44:34.317Z Learning: When reviewing device tree changes, validate node properties against the official kernel device tree binding documentation (*.yaml files) rather than relying solely on historical patterns in existing DTS files. Copy-paste errors can persist across versions, and bindings define the authoritative set of valid properties for each device node type.Learnt from: EvilOlaf Repo: armbian/build PR: 0 File: :0-0 Timestamp: 2025-12-19T13:56:45.124Z Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.Learnt from: rpardini Repo: armbian/build PR: 9180 File: patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml:4-8 Timestamp: 2026-01-07T18:21:41.085Z Learning: In Armbian patch/kernel/archive/**/0000.patching_config.yaml files, the version metadata fields (name, branch, last-known-good-tag, kind, type) under the "info stuff" comment are not used by the patching scripts. When reviewing these files, do not suggest updating or fixing these metadata fields. Instead, suggest removing them entirely as they serve no functional purpose.patch/kernel/archive/sm8250-6.18/0034-arm64-dts-qcom-pm8150b-Add-a-charger-Signed-off-by-T.patch (2)
19-37: Charger DT node looks correct and matches the driver.The node structure aligns with the SMB5 charger driver in patch 0032. The
io-channelsproperty references ADC channels 7 and 8 which are added in this same patch.
46-56: ADC channel node names should be validated against the official QCOM ADC5 device tree binding documentation.The patch uses
channel@7andchannel@8withregvalues ofADC5_USB_IN_IandADC5_USB_IN_V_16respectively. According to upstream QCOM ADC5 patterns, generic channel node names likeadc-chan@<N>are preferred, and the unit-address should match the actual register value. Additionally, the IIO mappings suggestADC5_USB_IN_Icorresponds to index 0, not 7, indicating a potential mismatch between node names and their register definitions.Verify that the chosen node names and their unit-addresses align with the official QCOM ADC5 binding documentation (qcom,spmi-adc5-gen3.yaml in upstream kernel) and confirm the actual values of the ADC5_* macros to ensure DT compatibility.
patch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch (2)
41-41: Makefile entry is correct.The object file name matches the source file and is properly gated by the config option.
564-583: IIO channel reading logic is correct.The function appropriately returns 0 when not charging (avoiding unnecessary IIO reads) and properly validates the channel pointer.
patch/kernel/archive/sm8250-6.18/0035-arm64-dts-qcom-pm8150b-Add-a-FG-Signed-off-by-Teguh-.patch (1)
19-25: The fuel gauge driver already exists in patch 0033 (power-supply-Driver-for-Qualcomm-FG.patch), which precedes this DT node patch in the series. The driver includes explicit support forqcom,pm8150b-fgwith the registered device match entry andops_fg_gen4operations structure. The DT node is correctly structured and the driver dependency is satisfied.
| +config CHARGER_QCOM_SMB5 | ||
| + tristate "Qualcomm PMI8998 PMIC charger driver" | ||
| + depends on MFD_SPMI_PMIC | ||
| + depends on IIO | ||
| + help | ||
| + Say Y here to enable the Qualcomm PMIC Charger driver. This | ||
| + adds support for the SMB5 switch mode battery charger found | ||
| + in PM8150B and related PMICs. |
There was a problem hiding this comment.
Kconfig description is misleading.
The config description says "Qualcomm PMI8998 PMIC charger driver" but the driver and help text indicate it's for PM8150B. This appears to be copy-paste from CHARGER_QCOM_SMB2 above.
Suggested fix
config CHARGER_QCOM_SMB5
- tristate "Qualcomm PMI8998 PMIC charger driver"
+ tristate "Qualcomm PM8150B PMIC charger driver"
depends on MFD_SPMI_PMIC
depends on IIO
help🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch
around lines 21 - 28, The Kconfig entry for CHARGER_QCOM_SMB5 has a misleading
short description ("Qualcomm PMI8998 PMIC charger driver") copied from another
config; update the wording in the CHARGER_QCOM_SMB5 tristate/descriptive string
to correctly reference SMB5/PM8150B (e.g., "Qualcomm PM8150B/SMB5 PMIC charger
driver" or similar) so the short description matches the help text and driver
target.
| + rc = smb5_get_prop_usb_online(chip, &usb_online); | ||
| + if (!usb_online) { | ||
| + *val = POWER_SUPPLY_USB_TYPE_UNKNOWN; | ||
| + return rc; | ||
| + } |
There was a problem hiding this comment.
Missing error check for smb5_get_prop_usb_online.
If smb5_get_prop_usb_online fails, usb_online may be uninitialized or stale. The error should be checked before using the value.
Suggested fix
rc = smb5_get_prop_usb_online(chip, &usb_online);
+ if (rc < 0)
+ return rc;
if (!usb_online) {
*val = POWER_SUPPLY_USB_TYPE_UNKNOWN;
- return rc;
+ return 0;
}📝 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.
| + rc = smb5_get_prop_usb_online(chip, &usb_online); | |
| + if (!usb_online) { | |
| + *val = POWER_SUPPLY_USB_TYPE_UNKNOWN; | |
| + return rc; | |
| + } | |
| rc = smb5_get_prop_usb_online(chip, &usb_online); | |
| if (rc < 0) | |
| return rc; | |
| if (!usb_online) { | |
| *val = POWER_SUPPLY_USB_TYPE_UNKNOWN; | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch
around lines 396 - 400, smb5_get_prop_usb_online's return value (rc) isn't
checked before using usb_online; verify rc after calling
smb5_get_prop_usb_online and handle errors before reading usb_online — e.g., if
rc < 0 return rc (or set *val to an appropriate default and return rc) instead
of using usb_online when the call failed; update the block around
smb5_get_prop_usb_online, usb_online, *val and POWER_SUPPLY_USB_TYPE_UNKNOWN
accordingly.
| + rc = smb5_get_prop_usb_online(chip, &usb_online); | ||
| + if (!usb_online) { | ||
| + *val = POWER_SUPPLY_STATUS_DISCHARGING; | ||
| + return rc; | ||
| + } |
There was a problem hiding this comment.
Same missing error check pattern.
Same issue as smb5_apsd_get_charger_type - the return value of smb5_get_prop_usb_online should be checked before using usb_online.
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch
around lines 445 - 449, The code uses usb_online returned by
smb5_get_prop_usb_online without checking the function return code; update the
smb5_get_prop_usb_online call in the same function (like
smb5_apsd_get_charger_type) to check rc immediately and return rc on error
before testing usb_online, then only set *val = POWER_SUPPLY_STATUS_DISCHARGING
when rc == 0 and usb_online is false.
| + if (stat & CHARGER_ERROR_STATUS_BAT_OV_BIT) { | ||
| + dev_err(chip->dev, "battery over-voltage"); | ||
| + } |
There was a problem hiding this comment.
Battery overvoltage detected but health value not set.
When CHARGER_ERROR_STATUS_BAT_OV_BIT is set, the function logs an error but doesn't set *val to POWER_SUPPLY_HEALTH_OVERVOLTAGE. The function may then return POWER_SUPPLY_HEALTH_GOOD if temperature checks pass.
Suggested fix
if (stat & CHARGER_ERROR_STATUS_BAT_OV_BIT) {
- dev_err(chip->dev, "battery over-voltage");
+ dev_err(chip->dev, "battery over-voltage\n");
+ *val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ return 0;
}📝 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.
| + if (stat & CHARGER_ERROR_STATUS_BAT_OV_BIT) { | |
| + dev_err(chip->dev, "battery over-voltage"); | |
| + } | |
| if (stat & CHARGER_ERROR_STATUS_BAT_OV_BIT) { | |
| dev_err(chip->dev, "battery over-voltage\n"); | |
| *val = POWER_SUPPLY_HEALTH_OVERVOLTAGE; | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch
around lines 597 - 599, The handler logs battery overvoltage when
CHARGER_ERROR_STATUS_BAT_OV_BIT is set but never updates the health output;
modify the function that checks stat (the block containing
CHARGER_ERROR_STATUS_BAT_OV_BIT and dev_err) to set *val =
POWER_SUPPLY_HEALTH_OVERVOLTAGE when that bit is present and ensure the function
returns that health (or short-circuits further checks) so callers don't
incorrectly report POWER_SUPPLY_HEALTH_GOOD.
| + rc = regmap_write(chip->regmap, BARK_BITE_WDOG_PET, | ||
| + BARK_BITE_WDOG_PET_BIT); |
There was a problem hiding this comment.
Missing base address offset for watchdog register.
The register write uses BARK_BITE_WDOG_PET directly, but all other register accesses in this driver use chip->base + offset. This will write to the wrong address.
Suggested fix
- rc = regmap_write(chip->regmap, BARK_BITE_WDOG_PET,
+ rc = regmap_write(chip->regmap, chip->base + BARK_BITE_WDOG_PET,
BARK_BITE_WDOG_PET_BIT);📝 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.
| + rc = regmap_write(chip->regmap, BARK_BITE_WDOG_PET, | |
| + BARK_BITE_WDOG_PET_BIT); | |
| rc = regmap_write(chip->regmap, chip->base + BARK_BITE_WDOG_PET, | |
| BARK_BITE_WDOG_PET_BIT); |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0032-power-supply-Driver-for-Qualcomm-SMB5-Signed-off-by-.patch
around lines 728 - 729, The regmap write is missing the driver's base offset:
change the regmap_write call that currently passes BARK_BITE_WDOG_PET to use the
full register address by adding chip->base (i.e., pass chip->base +
BARK_BITE_WDOG_PET) so regmap_write(chip->regmap, chip->base +
BARK_BITE_WDOG_PET, BARK_BITE_WDOG_PET_BIT); keep the same regmap, bit constant
and error handling.
| + if (psy == chip->chg_psy) { | ||
| + ret = power_supply_get_property(psy, | ||
| + POWER_SUPPLY_PROP_STATUS, &propval); | ||
| + if (ret) | ||
| + chip->status = POWER_SUPPLY_STATUS_UNKNOWN; | ||
| + | ||
| + chip->status = propval.intval; | ||
| + | ||
| + power_supply_changed(chip->batt_psy); |
There was a problem hiding this comment.
Use of uninitialized variable after error.
When power_supply_get_property() fails at line 1158, propval.intval is uninitialized. Line 1161 correctly sets chip->status to UNKNOWN, but line 1163 unconditionally overwrites it with the uninitialized propval.intval, potentially setting chip->status to garbage.
🔧 Proposed fix
if (psy == chip->chg_psy) {
ret = power_supply_get_property(psy,
POWER_SUPPLY_PROP_STATUS, &propval);
- if (ret)
+ if (ret) {
chip->status = POWER_SUPPLY_STATUS_UNKNOWN;
-
- chip->status = propval.intval;
+ } else {
+ chip->status = propval.intval;
+ }
power_supply_changed(chip->batt_psy);📝 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.
| + if (psy == chip->chg_psy) { | |
| + ret = power_supply_get_property(psy, | |
| + POWER_SUPPLY_PROP_STATUS, &propval); | |
| + if (ret) | |
| + chip->status = POWER_SUPPLY_STATUS_UNKNOWN; | |
| + | |
| + chip->status = propval.intval; | |
| + | |
| + power_supply_changed(chip->batt_psy); | |
| if (psy == chip->chg_psy) { | |
| ret = power_supply_get_property(psy, | |
| POWER_SUPPLY_PROP_STATUS, &propval); | |
| if (ret) { | |
| chip->status = POWER_SUPPLY_STATUS_UNKNOWN; | |
| } else { | |
| chip->status = propval.intval; | |
| } | |
| power_supply_changed(chip->batt_psy); |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0033-power-supply-Driver-for-Qualcomm-FG.patch
around lines 1157 - 1165, The code unconditionally assigns chip->status =
propval.intval even when power_supply_get_property() (called for psy ==
chip->chg_psy) fails, so guard the use of propval by checking the return value:
after calling power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS, &propval)
set chip->status = POWER_SUPPLY_STATUS_UNKNOWN on error (ret != 0) and only
assign chip->status = propval.intval when ret == 0; leave the
power_supply_changed(chip->batt_psy) call as-is.
| + ret = qcom_fg_masked_write(chip, | ||
| + MEM_INTF_IMA_CFG(chip), BIT(3), BIT(3)); | ||
| + if (ret) { | ||
| + dev_err(chip->dev, | ||
| + "Failed to configure interrupt sourete: %d\n", ret); | ||
| + return ret; | ||
| + } |
There was a problem hiding this comment.
Typo in error message.
"sourete" should be "source".
📝 Proposed fix
if (ret) {
dev_err(chip->dev,
- "Failed to configure interrupt sourete: %d\n", ret);
+ "Failed to configure interrupt source: %d\n", ret);
return ret;
}📝 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.
| + ret = qcom_fg_masked_write(chip, | |
| + MEM_INTF_IMA_CFG(chip), BIT(3), BIT(3)); | |
| + if (ret) { | |
| + dev_err(chip->dev, | |
| + "Failed to configure interrupt sourete: %d\n", ret); | |
| + return ret; | |
| + } | |
| ret = qcom_fg_masked_write(chip, | |
| MEM_INTF_IMA_CFG(chip), BIT(3), BIT(3)); | |
| if (ret) { | |
| dev_err(chip->dev, | |
| "Failed to configure interrupt source: %d\n", ret); | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0033-power-supply-Driver-for-Qualcomm-FG.patch
around lines 1223 - 1229, Typo in the dev_err message: change "sourete" to
"source" in the error string emitted after qcom_fg_masked_write; locate the
block that calls qcom_fg_masked_write with MEM_INTF_IMA_CFG(chip) and update the
dev_err call (using chip->dev) to log "Failed to configure interrupt source:
%d\n" so the message is correct and still includes the return code.
| Subject: arm64: dts: qcom: add SM8250 Retroid Pocket variant Signed-off-by: | ||
| Teguh Sobirin <[email protected]> |
There was a problem hiding this comment.
Fix malformed patch Subject line.
The Subject line is improperly formatted and split across multiple lines. It should be a single line with proper formatting, and the Signed-off-by should be in the patch body, not the subject.
📝 Proposed fix
-Subject: arm64: dts: qcom: add SM8250 Retroid Pocket variant Signed-off-by:
- Teguh Sobirin <[email protected]>
+Subject: arm64: dts: qcom: add SM8250 Retroid Pocket variant
+
+Add device tree support for SM8250-based Retroid Pocket devices.
+
+Signed-off-by: Teguh Sobirin <[email protected]>📝 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.
| Subject: arm64: dts: qcom: add SM8250 Retroid Pocket variant Signed-off-by: | |
| Teguh Sobirin <[email protected]> | |
| Subject: arm64: dts: qcom: add SM8250 Retroid Pocket variant | |
| Add device tree support for SM8250-based Retroid Pocket devices. | |
| Signed-off-by: Teguh Sobirin <[email protected]> |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch
around lines 4 - 5, The patch Subject is malformed: the "Subject:" header is
split and includes the Signed-off-by line; fix the commit header so the Subject
is a single clean line like "arm64: dts: qcom: add SM8250 Retroid Pocket
variant" and move the "Signed-off-by: Teguh Sobirin <[email protected]>" into the
patch body/footer (commit message body) beneath the subject, ensuring no other
header lines are broken or wrapped across multiple lines.
| + * Copyright (c) 2024, Retroid Pocket. | ||
| + */ | ||
| + | ||
| +/dts-v1/; |
There was a problem hiding this comment.
Remove /dts-v1/; directive from .dtsi file.
The /dts-v1/; directive should only appear in top-level .dts files, not in .dtsi include files. Having it in an include file can cause device tree compilation issues.
🔧 Proposed fix
-/dts-v1/;
-
#include <dt-bindings/leds/common.h>📝 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.
| +/dts-v1/; | |
| #include <dt-bindings/leds/common.h> |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch
at line 24, Remove the stray `/dts-v1/;` directive from the .dtsi include (it
should only be in top-level .dts files); locate the line containing `/dts-v1/;`
(as shown in the diff) and delete it so the include file no longer contains the
directive, then re-run device-tree compilation to verify the error is resolved.
| + ledr_r2: led@6 { | ||
| + reg = <6>; | ||
| + label = "r:r2"; | ||
| + color = <LED_COLOR_ID_RED>; |
There was a problem hiding this comment.
Fix inconsistent whitespace and indentation.
Several lines have inconsistent whitespace:
- Line 630: Mixed tab/space before
colorproperty - Line 771: Mixed tab/space before
enable-gpiosproperty - Lines 1066-1070: Inconsistent indentation in gpio configuration
- Line 1142: Trailing whitespace after
"HPHR_OUT",
Device tree files should use consistent indentation (tabs) to comply with kernel coding standards.
Also applies to: 771-771, 1066-1070, 1142-1142
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0041-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch
at line 630, Fix inconsistent indentation by replacing mixed spaces with tabs
for the device tree properties: ensure the `color = <LED_COLOR_ID_RED>;` line
uses a leading tab (not spaces), the `enable-gpios = <...>;` property on the
`enable-gpios` line uses a leading tab, normalize the gpio configuration block
(the lines configuring GPIOs around the gpio entries at the block containing the
gpio setup on lines ~1066-1070) to use tabs for all indentation, and remove
trailing whitespace after the `"HPHR_OUT",` entry; keep tabs as the consistent
indentation style to match kernel DTS coding standards.
|
@CodeChenL Hello, thank you for your work! I was originally planning to bump the SM8250 current kernel to 6.18, but I’ve closed my task after seeing your PR. Would it be possible for you to bump the current kernel to 6.18 instead of the edge kernel? If that’s not convenient for you, I can try to bump current to 6.18 and edge to 6.19 once this PR is merged. However, I don't have a Xiaomi device; I only have an SM8250 device that I maintain downstream. Would you be able to help with testing when the time comes? :) |
From sm8250-6.12
This is exactly what I did last night, advancing current to 6.18. I would be very happy to test with you! |
|
I am gradually porting the 6.12 patch to 6.18, and after the review, I will organize the commits neatly. |
Ah, I just checked the commit messages and you're right. My apologies for the oversight! Anyway, thank you for the clarification and for your work. |
|
But the 0010-0015 section in https://github.com/armbian/build/tree/main/patch/kernel/archive/sm8250-6.12 confuses me, what problem is it used to fix? A large screen? which device depends on these patches? |
These patches are introduced from kernel of postmarket os. If xiaomi elish can work without them, we can consider dropping them since they're not merged mainline for a long time. |
Currently, the DSI screen of Xiaomi-Elish is functioning perfectly. It's really great to receive this news, because they deviate too far from the main line, making it difficult to port |
I'm not very familiar with DRM and Qualcomm DPU, but I suspect these patches are optimizing for dual-DSI screen devices or single screens using dual DSI controllers? I've checked other projects like Batocera, REG-Linux, and ROCKNIX—which mainly support Retroid handhelds—and it seems they don't have these specific patches in their 6.18 kernels. |
From sm8250-6.12, and made some modifications to be compatible with the latest kernel
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In
@patch/kernel/archive/sm8250-6.18/0006-ASoC-qcom-sm8250-Add-tdm-support.patch:
- Around line 100-107: The loop using for_each_rtd_codec_dais sets codec formats
and sysclk but ignores snd_soc_dai_set_sysclk’s return value; modify the loop so
you capture and check the return of snd_soc_dai_set_sysclk (e.g., assign its
return to ret), and if ret < 0 log the error with dev_err(rtd->dev, "TDM sysclk
err:%d\n", ret) and return ret, just like the existing check for
snd_soc_dai_set_fmt; keep snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt) and
TDM_BCLK_RATE/SNDRV_PCM_STREAM_PLAYBACK usage intact.
In
@patch/kernel/archive/sm8250-6.18/0025-drm-mipi-add-mipi_dsi_generic_write_multi_type.patch:
- Around line 34-36: The doc comment for mipi_dsi_generic_write_multi_type is
incorrect: it claims the function "will automatically choose the right data type
depending on the payload length" but the function takes an explicit type
parameter named type and uses it directly; update the comment to state that the
caller-provided type is used (do not claim auto-selection), remove or replace
the misleading sentence, and briefly document the type parameter behavior and
expected payload handling so the comment matches the implementation.
In
@patch/kernel/archive/sm8250-6.18/0026-drm-panel-add-driver-for-samsung-amb655x.patch:
- Around line 274-278: The error log in the regulator_bulk_disable path is
incorrect: when calling regulator_bulk_disable(ARRAY_SIZE(amb655x->supplies),
amb655x->supplies) the dev_err currently says "Failed to enable regulators";
change the message to reflect the disable action (e.g., "Failed to disable
regulators: %d\n") so the dev_err(dev, ...) correctly reports failures from
regulator_bulk_disable and continue returning ret. Include the same ret variable
and dev as currently used.
- Around line 372-379: The probe currently logs regulator_bulk_enable failures
but continues execution and never disables regulators on later failures; change
the regulator_bulk_enable call handling to return on error (use ret from
regulator_bulk_enable), and add an err_disable_regulators label that calls
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies) and then
returns the appropriate dev_err_probe(...) result; update all subsequent error
paths (failure of devm_gpiod_get_optional for ctx->reset_gpio, backlight init,
dsi_attach, etc.) to jump to err_disable_regulators so regulators are always
disabled on error.
In
@patch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch:
- Around line 320-332: of_property_read_u32 may leave rotation uninitialized and
the current flow falls through even on error; initialize rotation to a safe
sentinel (e.g., -1), check the return value from of_property_read_u32 and if it
fails set ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN and skip further
parsing, and when parsing valid values use explicit checks for 0/90/180/270
mapping to DRM_MODE_PANEL_ORIENTATION_NORMAL/RIGHT_UP/BOTTOM_UP/LEFT_UP and
otherwise set orientation to UNKNOWN (do not silently leave previous value).
Reference symbols: of_property_read_u32, rotation, ctx->orientation,
DRM_MODE_PANEL_ORIENTATION_UNKNOWN, DRM_MODE_PANEL_ORIENTATION_NORMAL,
DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,
DRM_MODE_PANEL_ORIENTATION_LEFT_UP.
In
@patch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch:
- Around line 281-284: The code only checks for PTR_ERR(priv->sdb) ==
-EPROBE_DEFER after calling devm_gpiod_get, leaving other error pointers in
priv->sdb that will be dereferenced later (priv->sdb used at line ~296); change
the check to use IS_ERR(priv->sdb) and return PTR_ERR(priv->sdb) on error (or
handle ENOENT specially if intended), e.g. replace the PTR_ERR(...) ==
-EPROBE_DEFER branch with a full IS_ERR check and propagate the actual error
code from PTR_ERR(priv->sdb) so no error pointer is stored in priv->sdb.
- Line 347: Fix the typo in the MODULE_AUTHOR macro by adding the missing
closing angle bracket to the email address in the string passed to MODULE_AUTHOR
(the symbol MODULE_AUTHOR in the patch); change the author string so it ends
with ">" before the closing quote and parentheses so the macro becomes
syntactically correct.
- Around line 291-310: The probe enables the regulator
(regulator_enable(priv->vdd)) and sets the SDB GPIO high
(gpiod_set_value_cansleep(priv->sdb, 1)) before calling htr3212_init_regs and
htr3212_parse_dt, but on failures those resources are not cleaned up; add proper
error handling by introducing goto-based cleanup labels (e.g., err_disable_reg)
and on any failure after the regulator/GPIO are acquired call
gpiod_set_value_cansleep(priv->sdb, 0) and regulator_disable(priv->vdd) (and
undo i2c_set_clientdata if necessary) before returning, updating the failure
paths from htr3212_init_regs and htr3212_parse_dt to jump to the cleanup
label(s).
🧹 Nitpick comments (6)
patch/kernel/archive/sm8250-6.18/0006-ASoC-qcom-sm8250-Add-tdm-support.patch (3)
20-20: Consider markingtdm_slot_offsetasconst.The array is read-only and never modified. Adding
constimproves code clarity and allows compiler optimizations.Suggested change
-static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28}; +static const unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28};
41-73: Inconsistent indentation violates kernel coding style.The function body uses spaces with varying indentation levels (2-space, then misaligned blocks). Linux kernel coding style requires tabs for indentation. This will likely trigger checkpatch.pl warnings.
Suggested fix (showing corrected indentation with tabs)
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0x03, - slots, slot_width); - if (ret < 0) { - dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n", - __func__, ret); - goto end; - } - - ret = snd_soc_dai_set_channel_map(cpu_dai, 0, NULL, - channels, tdm_slot_offset); - if (ret < 0) { - dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n", - __func__, ret); - goto end; - } + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0x03, + slots, slot_width); + if (ret < 0) { + dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n", + __func__, ret); + goto end; + } + + ret = snd_soc_dai_set_channel_map(cpu_dai, 0, NULL, + channels, tdm_slot_offset); + if (ret < 0) { + dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n", + __func__, ret); + goto end; + } } else { - ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0xf, 0, - slots, slot_width); - if (ret < 0) { - dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n", - __func__, ret); - goto end; - } - - ret = snd_soc_dai_set_channel_map(cpu_dai, channels, - tdm_slot_offset, 0, NULL); - if (ret < 0) { - dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n", - __func__, ret); - goto end; - } + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0xf, 0, + slots, slot_width); + if (ret < 0) { + dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n", + __func__, ret); + goto end; + } + + ret = snd_soc_dai_set_channel_map(cpu_dai, channels, + tdm_slot_offset, 0, NULL); + if (ret < 0) { + dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n", + __func__, ret); + goto end; + } }
86-86: Minor style: missing space after comma.- int ret,j; + int ret, j;patch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch (2)
21-31: Consider removing unused DRM_DISPLAY_DP_HELPER select.The driver doesn't appear to use DisplayPort helpers. The
select DRM_DISPLAY_DP_HELPERmay have been copied from another panel driver but isn't needed here. This adds unnecessary build dependencies.Suggested Kconfig cleanup
config DRM_PANEL_DDIC_CH13726A tristate "DDIC CH13726A-based DSI panel" depends on OF depends on DRM_MIPI_DSI depends on BACKLIGHT_CLASS_DEVICE - select DRM_DISPLAY_DP_HELPER - select DRM_DISPLAY_HELPER help Say Y here if you want to enable support for DDIC CH13726A-based display panels.
339-340: Minor: Double space and inconsistent indentation.Style fix
- dsi->mode_flags = MIPI_DSI_MODE_VIDEO_BURST | - MIPI_DSI_CLOCK_NON_CONTINUOUS; + dsi->mode_flags = MIPI_DSI_MODE_VIDEO_BURST | + MIPI_DSI_CLOCK_NON_CONTINUOUS;patch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch (1)
131-140: Simplify redundant return logic.The
if (ret) return ret; return 0;pattern can be simplified.Proposed simplification
static int htr3212_reset_regs(struct htr3212_priv *priv) { - int ret; - - ret = htr3212_write(priv, HTR3212_RESET, 0x00); - if (ret) - return ret; - - return 0; + return htr3212_write(priv, HTR3212_RESET, 0x00); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
patch/kernel/archive/sm8250-6.18/0006-ASoC-qcom-sm8250-Add-tdm-support.patchpatch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patchpatch/kernel/archive/sm8250-6.18/0025-drm-mipi-add-mipi_dsi_generic_write_multi_type.patchpatch/kernel/archive/sm8250-6.18/0026-drm-panel-add-driver-for-samsung-amb655x.patchpatch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patchpatch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patchpatch/kernel/archive/sm8250-6.18/0042-arm64-dts-qcom-sm8250-oneplus-Add-pm8150b-type-c-nod.patch
🚧 Files skipped from review as they are similar to previous changes (1)
- patch/kernel/archive/sm8250-6.18/0023-Asoc-wm_adsp-Use-xiaomi-elish-firmware-name.patch
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: EvilOlaf
Repo: armbian/build PR: 9096
File: patch/kernel/archive/sunxi-6.18/patches.armbian/arm64-dts-sun50i-h616-add-overlays.patch:4-5
Timestamp: 2025-12-17T15:46:22.709Z
Learning: In Armbian kernel patches, when patches are renamed or reorganized, the old Subject line may be intentionally preserved in the patch header (as a second Subject line) for archival/historical purposes to document the previous patch name. This is an Armbian-specific practice for tracking patch evolution and should not be flagged as an error when the maintainer has intentionally included it for documentation.
Learnt from: rpardini
Repo: armbian/build PR: 9159
File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161
Timestamp: 2026-01-03T20:46:29.189Z
Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches.
<!-- </add_learning>
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: EvilOlaf
Repo: armbian/build PR: 9086
File: patch/kernel/archive/uefi-arm64-6.19/net-stmicro-stmmac-Phytium-onboard-ethernet-drivers-and-ACPI-glue-for-6.x.patch.disabled:625-633
Timestamp: 2025-12-21T10:28:48.455Z
Learning: The patch "net-stmicro-stmmac-Phytium-onboard-ethernet-drivers-and-ACPI-glue-for-6.x.patch" in patch/kernel/archive/uefi-arm64-6.19/ is known to break compilation on kernel 6.19 and is therefore disabled (has .disabled extension).
📚 Learning: 2026-01-10T04:28:58.758Z
Learnt from: kasimling
Repo: armbian/build PR: 9177
File: patch/kernel/archive/rockchip64-6.18/rk3588-1212-arm64-dts-Automatic-fan-speed-and-USB-3.0-Type-A-por.patch:37-41
Timestamp: 2026-01-10T04:28:58.758Z
Learning: When reviewing Linux kernel device-tree bindings involving thermal cooling devices, THERMAL_NO_LIMIT can be used for both min_state (lower) and max_state (upper). If used as lower/min_state, the framework converts it to 0; if used as upper/max_state, it converts to the cooling device's max_state. Verify patch entries like: cooling-device = <&fan THERMAL_NO_LIMIT 1> are valid and correctly reflect the intended bounds. Ensure consistent interpretation of NO_LIMIT in both min and max contexts and that the resulting values align with the device's actual limits.
Applied to files:
patch/kernel/archive/sm8250-6.18/0025-drm-mipi-add-mipi_dsi_generic_write_multi_type.patchpatch/kernel/archive/sm8250-6.18/0026-drm-panel-add-driver-for-samsung-amb655x.patchpatch/kernel/archive/sm8250-6.18/0006-ASoC-qcom-sm8250-Add-tdm-support.patchpatch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patchpatch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patchpatch/kernel/archive/sm8250-6.18/0042-arm64-dts-qcom-sm8250-oneplus-Add-pm8150b-type-c-nod.patch
📚 Learning: 2025-12-13T11:00:33.143Z
Learnt from: pyavitz
Repo: armbian/build PR: 9058
File: patch/kernel/archive/spacemit-6.6/dt/lcd_tc358762xbg_dpi_800x480.dtsi:3-61
Timestamp: 2025-12-13T11:00:33.143Z
Learning: On the SpacemiT MusePi Pro board with spacemit-6.6 kernel, LCD/DSI support is disabled in k1-musepi-pro.dts because enabling it causes the unit to lock up during boot. The lcd_tc358762xbg_dpi_800x480.dtsi file exists but DSI remains disabled at the controller level.
Applied to files:
patch/kernel/archive/sm8250-6.18/0026-drm-panel-add-driver-for-samsung-amb655x.patch
📚 Learning: 2025-12-17T05:09:02.306Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Applied to files:
patch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patchpatch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patchpatch/kernel/archive/sm8250-6.18/0042-arm64-dts-qcom-sm8250-oneplus-Add-pm8150b-type-c-nod.patch
📚 Learning: 2025-12-19T13:56:45.124Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Applied to files:
patch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch
📚 Learning: 2025-12-17T15:46:22.709Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 9096
File: patch/kernel/archive/sunxi-6.18/patches.armbian/arm64-dts-sun50i-h616-add-overlays.patch:4-5
Timestamp: 2025-12-17T15:46:22.709Z
Learning: In Armbian kernel patches, when patches are renamed or reorganized, the old Subject line may be intentionally preserved in the patch header (as a second Subject line) for archival/historical purposes to document the previous patch name. This is an Armbian-specific practice for tracking patch evolution and should not be flagged as an error when the maintainer has intentionally included it for documentation.
Applied to files:
patch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch
📚 Learning: 2026-01-03T07:27:45.955Z
Learnt from: iav
Repo: armbian/build PR: 9158
File: patch/kernel/archive/rockchip64-6.18/add-board-helios64.patch:598-600
Timestamp: 2026-01-03T07:27:45.955Z
Learning: In Rockchip vendor kernel branches (e.g., rockchip64-6.18), the FUSB302 device tree binding accepts usb-role-switch as a phandle reference to a USB role switch provider (e.g., typec_extcon_bridge), not just as a boolean flag as in mainline kernel bindings. The vendor implementation allows usb-role-switch = <&typec_extcon_bridge>; to work correctly.
Applied to files:
patch/kernel/archive/sm8250-6.18/0042-arm64-dts-qcom-sm8250-oneplus-Add-pm8150b-type-c-nod.patch
🔇 Additional comments (17)
patch/kernel/archive/sm8250-6.18/0042-arm64-dts-qcom-sm8250-oneplus-Add-pm8150b-type-c-nod.patch (4)
1-21: Patch metadata and header look good.The commit message accurately describes the change: adding Type-C node for OTG functionality and PD negotiation support. The diff stat aligns with the changes below.
22-58: Type-C connector configuration is well-structured.The USB-C connector node properly defines:
- Dual role for both power and data (required for OTG)
- Reasonable PD negotiation values (15W source, up to 60W sink with variable PDO)
- Correct endpoint linking to the DWC3 controller
The
self-poweredproperty andop-sink-microwattof 10W are appropriate for a smartphone device.
60-64: VBUS regulator configuration is consistent with PD capabilities.The current limits (500mA min, 3A max) align correctly with the source-pdos definition of 5V @ 3A.
69-80: USB DWC3 OTG configuration is correct.The changes properly enable OTG functionality:
dr_mode = "otg"enables dual-role operationusb-role-switchenables dynamic role switching based on Type-C CC line state- The bidirectional endpoint linking between
pm8150b_hs_inandusb_1_dwc3_hs_outfollows the correct DTS graph binding pattern for USB Type-Cpatch/kernel/archive/sm8250-6.18/0025-drm-mipi-add-mipi_dsi_generic_write_multi_type.patch (2)
41-65: Implementation looks correct.The function correctly:
- Initializes the DSI message with the explicit type parameter
- Respects the accumulated error pattern used by other
_multihelpers- Logs failures with appropriate context
- Returns bytes written or negative error
87-99: Macro implementation is correct.The
mipi_dsi_dcs_write_long_multimacro follows the established pattern and correctly usesMIPI_DSI_DCS_LONG_WRITEto force long write commands for panels like Samsung AMB655X that require this behavior.patch/kernel/archive/sm8250-6.18/0006-ASoC-qcom-sm8250-Add-tdm-support.patch (1)
116-119: LGTM!The switch with range expression correctly routes TDM DAI IDs to the dedicated TDM handler while allowing non-TDM DAIs to fall through to the SDW hw_params path.
patch/kernel/archive/sm8250-6.18/0026-drm-panel-add-driver-for-samsung-amb655x.patch (4)
213-219: Verify:set_display_oncalled in the off sequence.Line 215 calls
mipi_dsi_dcs_set_display_on_multiwithin the panel off function, which is counterintuitive. If this is an intentional panel-specific quirk (some panels require unusual sequences), consider adding a comment explaining why. Otherwise, this may be a copy-paste error and should beset_display_off_multi.
59-77: LGTM!The includes and header structure are appropriate for a MIPI DSI panel driver with DSC support.
283-314: LGTM!The display mode definition and
get_modesimplementation are correct. The clock calculation follows standard DRM practices.
427-457: LGTM!The remove function, OF match table, and module registration follow standard kernel driver patterns.
patch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch (5)
43-43: LGTM!The Makefile entry follows the established pattern for panel drivers.
292-361: LGTM!The probe and remove lifecycle is correctly implemented with proper cleanup on error paths and use of devm_* managed resources.
148-192: LGTM!The prepare/unprepare functions have proper guard checks, error handling, and cleanup paths.
194-220: LGTM!Display mode definitions follow kernel conventions with correct clock calculations for 60Hz refresh rate.
375-380: LGTM!The OF match table correctly maps compatibles to their respective display modes.
patch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch (1)
196-197: Verifyalways-onproperty is read from correct DT node.
device_property_read_bool(dev, "always-on")reads from the parent I2C device node, not the LED child node. If per-LED suspend control is intended, this should read fromchildinstead.
| + for_each_rtd_codec_dais(rtd, j, codec_dai) { | ||
| + ret = snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | ||
| + snd_soc_dai_set_sysclk(codec_dai, 0, TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK); | ||
| + if (ret < 0) { | ||
| + dev_err(rtd->dev, "TDM fmt err:%d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + } |
There was a problem hiding this comment.
Unchecked return value from snd_soc_dai_set_sysclk.
Line 102 ignores the return value of snd_soc_dai_set_sysclk, but the subsequent check on line 103 only validates the snd_soc_dai_set_fmt result. If the sysclk configuration fails, the error will be silently ignored.
Suggested fix
for_each_rtd_codec_dais(rtd, j, codec_dai) {
ret = snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
- snd_soc_dai_set_sysclk(codec_dai, 0, TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
if (ret < 0) {
dev_err(rtd->dev, "TDM fmt err:%d\n", ret);
return ret;
}
+ ret = snd_soc_dai_set_sysclk(codec_dai, 0, TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
+ if (ret < 0) {
+ dev_err(rtd->dev, "TDM sysclk err:%d\n", ret);
+ return ret;
+ }
}📝 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.
| + for_each_rtd_codec_dais(rtd, j, codec_dai) { | |
| + ret = snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | |
| + snd_soc_dai_set_sysclk(codec_dai, 0, TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK); | |
| + if (ret < 0) { | |
| + dev_err(rtd->dev, "TDM fmt err:%d\n", ret); | |
| + return ret; | |
| + } | |
| + } | |
| for_each_rtd_codec_dais(rtd, j, codec_dai) { | |
| ret = snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | |
| if (ret < 0) { | |
| dev_err(rtd->dev, "TDM fmt err:%d\n", ret); | |
| return ret; | |
| } | |
| ret = snd_soc_dai_set_sysclk(codec_dai, 0, TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK); | |
| if (ret < 0) { | |
| dev_err(rtd->dev, "TDM sysclk err:%d\n", ret); | |
| return ret; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @patch/kernel/archive/sm8250-6.18/0006-ASoC-qcom-sm8250-Add-tdm-support.patch
around lines 100 - 107, The loop using for_each_rtd_codec_dais sets codec
formats and sysclk but ignores snd_soc_dai_set_sysclk’s return value; modify the
loop so you capture and check the return of snd_soc_dai_set_sysclk (e.g., assign
its return to ret), and if ret < 0 log the error with dev_err(rtd->dev, "TDM
sysclk err:%d\n", ret) and return ret, just like the existing check for
snd_soc_dai_set_fmt; keep snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt) and
TDM_BCLK_RATE/SNDRV_PCM_STREAM_PLAYBACK usage intact.
| + * | ||
| + * This function will automatically choose the right data type depending on | ||
| + * the payload length. |
There was a problem hiding this comment.
Incorrect documentation comment.
The description states "This function will automatically choose the right data type depending on the payload length" but the function actually takes an explicit type parameter (line 42) and uses it directly (line 49) without any automatic selection. This appears to be copy-pasted from mipi_dsi_generic_write_multi().
📝 Suggested documentation fix
-+ * This function will automatically choose the right data type depending on
-+ * the payload length.
++ * This function will use the provided data type, overriding the normal rules
++ * for automatic type selection based on payload length.📝 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.
| + * | |
| + * This function will automatically choose the right data type depending on | |
| + * the payload length. | |
| * | |
| * This function will use the provided data type, overriding the normal rules | |
| * for automatic type selection based on payload length. |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0025-drm-mipi-add-mipi_dsi_generic_write_multi_type.patch
around lines 34 - 36, The doc comment for mipi_dsi_generic_write_multi_type is
incorrect: it claims the function "will automatically choose the right data type
depending on the payload length" but the function takes an explicit type
parameter named type and uses it directly; update the comment to state that the
caller-provided type is used (do not claim auto-selection), remove or replace
the misleading sentence, and briefly document the type parameter behavior and
expected payload handling so the comment matches the implementation.
| + ret = regulator_bulk_disable(ARRAY_SIZE(amb655x->supplies), amb655x->supplies); | ||
| + if (ret) { | ||
| + dev_err(dev, "Failed to enable regulators: %d\n", ret); | ||
| + return ret; | ||
| + } |
There was a problem hiding this comment.
Fix incorrect error message in regulator disable path.
The error message says "Failed to enable regulators" but this code is disabling regulators.
Proposed fix
ret = regulator_bulk_disable(ARRAY_SIZE(amb655x->supplies), amb655x->supplies);
if (ret) {
- dev_err(dev, "Failed to enable regulators: %d\n", ret);
+ dev_err(dev, "Failed to disable regulators: %d\n", ret);
return ret;
}🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0026-drm-panel-add-driver-for-samsung-amb655x.patch
around lines 274 - 278, The error log in the regulator_bulk_disable path is
incorrect: when calling regulator_bulk_disable(ARRAY_SIZE(amb655x->supplies),
amb655x->supplies) the dev_err currently says "Failed to enable regulators";
change the message to reflect the disable action (e.g., "Failed to disable
regulators: %d\n") so the dev_err(dev, ...) correctly reports failures from
regulator_bulk_disable and continue returning ret. Include the same ret variable
and dev as currently used.
| + ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies); | ||
| + if (ret) | ||
| + dev_err(dev, "Failed to enable regulators: %d\n", ret); | ||
| + | ||
| + ctx->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(ctx->reset_gpio)) | ||
| + return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio), | ||
| + "Failed to get reset-gpios\n"); |
There was a problem hiding this comment.
Missing return on regulator enable failure and cleanup on error paths.
Two issues in the probe error handling:
- Lines 372-374: If
regulator_bulk_enablefails, the error is logged but the function continues instead of returning. - If subsequent operations fail (reset_gpio, backlight, dsi_attach), the regulators remain enabled since there's no cleanup.
Proposed fix
ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
- if (ret)
- dev_err(dev, "Failed to enable regulators: %d\n", ret);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable regulators\n");
ctx->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(ctx->reset_gpio))
- return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
+ if (IS_ERR(ctx->reset_gpio)) {
+ ret = PTR_ERR(ctx->reset_gpio);
+ goto err_disable_regulators;
+ }
- "Failed to get reset-gpios\n");And add cleanup label before the final return 0;:
err_disable_regulators:
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
return dev_err_probe(dev, ret, "...");🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0026-drm-panel-add-driver-for-samsung-amb655x.patch
around lines 372 - 379, The probe currently logs regulator_bulk_enable failures
but continues execution and never disables regulators on later failures; change
the regulator_bulk_enable call handling to return on error (use ret from
regulator_bulk_enable), and add an err_disable_regulators label that calls
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies) and then
returns the appropriate dev_err_probe(...) result; update all subsequent error
paths (failure of devm_gpiod_get_optional for ctx->reset_gpio, backlight init,
dsi_attach, etc.) to jump to err_disable_regulators so regulators are always
disabled on error.
| + ret = of_property_read_u32(dev->of_node, "rotation", &rotation); | ||
| + if (ret == -EINVAL) { | ||
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN; | ||
| + } | ||
| + | ||
| + if (rotation == 0) | ||
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL; | ||
| + else if (rotation == 90) | ||
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP; | ||
| + else if (rotation == 180) | ||
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP; | ||
| + else if (rotation == 270) | ||
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP; |
There was a problem hiding this comment.
Uninitialized variable read and broken control flow in rotation parsing.
Several issues with this logic:
- If
of_property_read_u32fails with any error (not just-EINVAL),rotationremains uninitialized - Even when setting
ORIENTATION_UNKNOWN, the code falls through and evaluates the uninitializedrotationvariable - Invalid rotation values (e.g., 45) silently leave orientation at whatever was previously set
Proposed fix
ret = of_property_read_u32(dev->of_node, "rotation", &rotation);
- if (ret == -EINVAL) {
+ if (ret) {
ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
- }
-
- if (rotation == 0)
+ } else if (rotation == 0) {
ctx->orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
- else if (rotation == 90)
+ } else if (rotation == 90) {
ctx->orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
- else if (rotation == 180)
+ } else if (rotation == 180) {
ctx->orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
- else if (rotation == 270)
+ } else if (rotation == 270) {
ctx->orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
+ } else {
+ dev_warn(dev, "Invalid rotation %u, using unknown orientation\n", rotation);
+ ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+ }📝 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.
| + ret = of_property_read_u32(dev->of_node, "rotation", &rotation); | |
| + if (ret == -EINVAL) { | |
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN; | |
| + } | |
| + | |
| + if (rotation == 0) | |
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL; | |
| + else if (rotation == 90) | |
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP; | |
| + else if (rotation == 180) | |
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP; | |
| + else if (rotation == 270) | |
| + ctx->orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP; | |
| ret = of_property_read_u32(dev->of_node, "rotation", &rotation); | |
| if (ret) { | |
| ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN; | |
| } else if (rotation == 0) { | |
| ctx->orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL; | |
| } else if (rotation == 90) { | |
| ctx->orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP; | |
| } else if (rotation == 180) { | |
| ctx->orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP; | |
| } else if (rotation == 270) { | |
| ctx->orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP; | |
| } else { | |
| dev_warn(dev, "Invalid rotation %u, using unknown orientation\n", rotation); | |
| ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN; | |
| } |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0036-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch
around lines 320 - 332, of_property_read_u32 may leave rotation uninitialized
and the current flow falls through even on error; initialize rotation to a safe
sentinel (e.g., -1), check the return value from of_property_read_u32 and if it
fails set ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN and skip further
parsing, and when parsing valid values use explicit checks for 0/90/180/270
mapping to DRM_MODE_PANEL_ORIENTATION_NORMAL/RIGHT_UP/BOTTOM_UP/LEFT_UP and
otherwise set orientation to UNKNOWN (do not silently leave previous value).
Reference symbols: of_property_read_u32, rotation, ctx->orientation,
DRM_MODE_PANEL_ORIENTATION_UNKNOWN, DRM_MODE_PANEL_ORIENTATION_NORMAL,
DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,
DRM_MODE_PANEL_ORIENTATION_LEFT_UP.
| + priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); | ||
| + if (PTR_ERR(priv->sdb) == -EPROBE_DEFER) | ||
| + return -EPROBE_DEFER; | ||
| + |
There was a problem hiding this comment.
Incomplete GPIO error handling allows use of error pointer.
devm_gpiod_get can fail with errors other than -EPROBE_DEFER. The current check only handles deferred probing; other failures (e.g., -ENOENT, -EBUSY) leave an error pointer in priv->sdb, which is later dereferenced at line 296.
Proposed fix
- priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH);
- if (PTR_ERR(priv->sdb) == -EPROBE_DEFER)
- return -EPROBE_DEFER;
+ priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH);
+ if (IS_ERR(priv->sdb))
+ return PTR_ERR(priv->sdb);📝 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.
| + priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); | |
| + if (PTR_ERR(priv->sdb) == -EPROBE_DEFER) | |
| + return -EPROBE_DEFER; | |
| + | |
| priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); | |
| if (IS_ERR(priv->sdb)) | |
| return PTR_ERR(priv->sdb); | |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch
around lines 281 - 284, The code only checks for PTR_ERR(priv->sdb) ==
-EPROBE_DEFER after calling devm_gpiod_get, leaving other error pointers in
priv->sdb that will be dereferenced later (priv->sdb used at line ~296); change
the check to use IS_ERR(priv->sdb) and return PTR_ERR(priv->sdb) on error (or
handle ENOENT specially if intended), e.g. replace the PTR_ERR(...) ==
-EPROBE_DEFER branch with a full IS_ERR check and propagate the actual error
code from PTR_ERR(priv->sdb) so no error pointer is stored in priv->sdb.
| + ret = regulator_enable(priv->vdd); | ||
| + if (ret < 0) { | ||
| + return ret; | ||
| + } | ||
| + | ||
| + gpiod_set_value_cansleep(priv->sdb, 1); | ||
| + usleep_range(10000, 11000); | ||
| + | ||
| + priv->client = client; | ||
| + i2c_set_clientdata(client, priv); | ||
| + | ||
| + ret = htr3212_init_regs(priv); | ||
| + if (ret) | ||
| + return ret; | ||
| + | ||
| + ret = htr3212_parse_dt(dev, priv); | ||
| + if (ret) | ||
| + return ret; | ||
| + | ||
| + return 0; |
There was a problem hiding this comment.
Resource leak on probe failure paths.
If htr3212_init_regs() or htr3212_parse_dt() fails, the regulator remains enabled and GPIO stays high. These resources need cleanup on error.
Proposed fix using goto-based error handling
ret = regulator_enable(priv->vdd);
- if (ret < 0) {
- return ret;
- }
+ if (ret < 0)
+ return ret;
gpiod_set_value_cansleep(priv->sdb, 1);
usleep_range(10000, 11000);
priv->client = client;
i2c_set_clientdata(client, priv);
ret = htr3212_init_regs(priv);
if (ret)
- return ret;
+ goto err_disable;
ret = htr3212_parse_dt(dev, priv);
if (ret)
- return ret;
+ goto err_disable;
return 0;
+
+err_disable:
+ gpiod_set_value_cansleep(priv->sdb, 0);
+ regulator_disable(priv->vdd);
+ return ret;
}📝 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.
| + ret = regulator_enable(priv->vdd); | |
| + if (ret < 0) { | |
| + return ret; | |
| + } | |
| + | |
| + gpiod_set_value_cansleep(priv->sdb, 1); | |
| + usleep_range(10000, 11000); | |
| + | |
| + priv->client = client; | |
| + i2c_set_clientdata(client, priv); | |
| + | |
| + ret = htr3212_init_regs(priv); | |
| + if (ret) | |
| + return ret; | |
| + | |
| + ret = htr3212_parse_dt(dev, priv); | |
| + if (ret) | |
| + return ret; | |
| + | |
| + return 0; | |
| ret = regulator_enable(priv->vdd); | |
| if (ret < 0) | |
| return ret; | |
| gpiod_set_value_cansleep(priv->sdb, 1); | |
| usleep_range(10000, 11000); | |
| priv->client = client; | |
| i2c_set_clientdata(client, priv); | |
| ret = htr3212_init_regs(priv); | |
| if (ret) | |
| goto err_disable; | |
| ret = htr3212_parse_dt(dev, priv); | |
| if (ret) | |
| goto err_disable; | |
| return 0; | |
| err_disable: | |
| gpiod_set_value_cansleep(priv->sdb, 0); | |
| regulator_disable(priv->vdd); | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch
around lines 291 - 310, The probe enables the regulator
(regulator_enable(priv->vdd)) and sets the SDB GPIO high
(gpiod_set_value_cansleep(priv->sdb, 1)) before calling htr3212_init_regs and
htr3212_parse_dt, but on failures those resources are not cleaned up; add proper
error handling by introducing goto-based cleanup labels (e.g., err_disable_reg)
and on any failure after the regulator/GPIO are acquired call
gpiod_set_value_cansleep(priv->sdb, 0) and regulator_disable(priv->vdd) (and
undo i2c_set_clientdata if necessary) before returning, updating the failure
paths from htr3212_init_regs and htr3212_parse_dt to jump to the cleanup
label(s).
| + | ||
| +module_i2c_driver(htr3212_driver); | ||
| + | ||
| +MODULE_AUTHOR("Teguh Sobirin <[email protected]"); |
There was a problem hiding this comment.
Typo: Missing closing bracket in MODULE_AUTHOR.
The email address is missing its closing >.
Proposed fix
-MODULE_AUTHOR("Teguh Sobirin <[email protected]");
+MODULE_AUTHOR("Teguh Sobirin <[email protected]>");📝 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.
| +MODULE_AUTHOR("Teguh Sobirin <[email protected]"); | |
| MODULE_AUTHOR("Teguh Sobirin <[email protected]>"); |
🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0037-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch
at line 347, Fix the typo in the MODULE_AUTHOR macro by adding the missing
closing angle bracket to the email address in the string passed to MODULE_AUTHOR
(the symbol MODULE_AUTHOR in the patch); change the author string so it ends
with ">" before the closing quote and parentheses so the macro becomes
syntactically correct.
|
I'm not very familiar with the build framework. |
|
I have ported all patches to 6.18 (excluding those already merged into the mainline and the aforementioned 0010-0015). |
Tricky, as armbian/build can run on many different host OS & under Docker (CI is on |
We indeed don't need to guarantee dependency control before the host compilation, but we should at least ensure that cli with docker and CI are ready to use out of the box. I think we need to include a new version of mkbootimg in https://github.com/armbian/docker-armbian-build/pkgs/container/docker-armbian-build/638345440?tag=armbian-ubuntu-noble-latest . The new version of mkbootimg already exists in the armbian noble repository. |
We don't use the Armbian repo in the armbian/build prebuilt Docker images, as that would cause an infinite dependency loop. I still think building it from source, or external binaries, and only when needed might be easier. |
|
Here is kernel tree from pmos: https://gitlab.com/sm8250-mainline/linux/-/commits/sm8250/v6.13?ref_type=heads, which has not get updated since v6.13. |
It seems that a bq25970 charger driver has been added, and I am very interested in it, as the current charging speed is too slow |
Alright, then I'll maintain the status quo |
To be honest, I had already started looking for the bq25970 fast charging driver in the Android kernel last night |
At the moment quick charge only work with special charger of xiaomi, and general pd charger won't work. |
To be honest, the speed of the current pd charger is incredibly slow and it has compatibility issues. When used with my new Xiaomi charger with 120W, it sometimes works and sometimes doesn't. What you said, "a general pd charger won't work," does it mean the speed is slow or it's completely unusable? |
Although the Xiaomi 65W charger can negotiate and charge stably at present, the power is only 5W |
From https://gitlab.com/sm8250-mainline/linux/-/commits/sm8250/v6.13 To fix compatibility and slow charging issues with the charger Signed-off-by: CodeChenL <[email protected]>
Form https://lore.kernel.org/all/[email protected]/ To fix slow startup, and the issue where the system fails to release ncm devices when shutting down after plugging or unplugging a usb hub, which references a null pointer This is important for devices without serial ports, such as the Elixir, because they enable NCM by default for headless debugging, making it easy to trigger related bugs. Signed-off-by: CodeChenL <[email protected]>
To equip the current kernel version. Signed-off-by: CodeChenL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@patch/kernel/archive/sm8250-6.18/0046-usb-gadget-Refactor-NCM-net_device-lifecycle.patch:
- Around line 192-199: The parsing loop using hex_to_bin() (in the macro that
writes into new_addr and similarly in USB_ETHER_OPTS_ATTR_HOST_ADDR) doesn't
check for hex_to_bin() returning -1, which yields corrupted bytes; either
replace this manual parse with the safe mac_pton() helper or validate each
hex_to_bin() result (check for -1) and abort/propagate an error if seen before
assembling new_addr and calling is_valid_ether_addr(); update both the loop that
fills new_addr and the same logic in USB_ETHER_OPTS_ATTR_HOST_ADDR to perform
this validation.
🧹 Nitpick comments (3)
patch/kernel/archive/sm8250-6.18/0044-arm64-dts-qcom-sm8250-xiaomi-elish-add-bq25970-charger -node.patch (2)
19-21: Use"okay"for DT status property.While
"ok"is technically accepted,"okay"is the preferred spelling per modern device tree conventions and will passdtbs_checkwithout warnings.Suggested fix
&i2c15 { clock-frequency = <400000>; - status = "ok"; + status = "okay";
26-29: Consider using IRQ type macros for readability.The raw interrupt flag
0x2002obscures the intended trigger type. Using macros from<dt-bindings/interrupt-controller/irq.h>(e.g.,IRQ_TYPE_EDGE_FALLING) would improve maintainability.patch/kernel/archive/sm8250-6.18/0046-usb-gadget-Refactor-NCM-net_device-lifecycle.patch (1)
577-580: Consider preserving the original error code fromgether_setup_default().If
gether_setup_default()could return error codes other than-ENOMEM, they would be masked. UsingPTR_ERR(netdev)would preserve the actual error.netdev = gether_setup_default(); if (IS_ERR(netdev)) - return -ENOMEM; + return PTR_ERR(netdev);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
config/kernel/linux-sm8250-current.configpatch/kernel/archive/sm8250-6.18/0043-power-supply-add-bq25970-charger-driver.patchpatch/kernel/archive/sm8250-6.18/0044-arm64-dts-qcom-sm8250-xiaomi-elish-add-bq25970-charger -node.patchpatch/kernel/archive/sm8250-6.18/0045-arm64-dts-qcom-sm8250-xiaomi-elish-add-flash-led-node.patchpatch/kernel/archive/sm8250-6.18/0046-usb-gadget-Refactor-NCM-net_device-lifecycle.patch
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Learnt from: EvilOlaf
Repo: armbian/build PR: 9096
File: patch/kernel/archive/sunxi-6.18/patches.armbian/arm64-dts-sun50i-h616-add-overlays.patch:4-5
Timestamp: 2025-12-17T15:46:22.709Z
Learning: In Armbian kernel patches, when patches are renamed or reorganized, the old Subject line may be intentionally preserved in the patch header (as a second Subject line) for archival/historical purposes to document the previous patch name. This is an Armbian-specific practice for tracking patch evolution and should not be flagged as an error when the maintainer has intentionally included it for documentation.
Learnt from: rpardini
Repo: armbian/build PR: 9159
File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161
Timestamp: 2026-01-03T20:46:29.189Z
Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches.
<!-- </add_learning>
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: EvilOlaf
Repo: armbian/build PR: 9086
File: patch/kernel/archive/uefi-arm64-6.19/net-stmicro-stmmac-Phytium-onboard-ethernet-drivers-and-ACPI-glue-for-6.x.patch.disabled:625-633
Timestamp: 2025-12-21T10:28:48.455Z
Learning: The patch "net-stmicro-stmmac-Phytium-onboard-ethernet-drivers-and-ACPI-glue-for-6.x.patch" in patch/kernel/archive/uefi-arm64-6.19/ is known to break compilation on kernel 6.19 and is therefore disabled (has .disabled extension).
📚 Learning: 2025-12-17T05:09:02.306Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Applied to files:
patch/kernel/archive/sm8250-6.18/0045-arm64-dts-qcom-sm8250-xiaomi-elish-add-flash-led-node.patchpatch/kernel/archive/sm8250-6.18/0044-arm64-dts-qcom-sm8250-xiaomi-elish-add-bq25970-charger -node.patch
📚 Learning: 2026-01-10T04:28:58.758Z
Learnt from: kasimling
Repo: armbian/build PR: 9177
File: patch/kernel/archive/rockchip64-6.18/rk3588-1212-arm64-dts-Automatic-fan-speed-and-USB-3.0-Type-A-por.patch:37-41
Timestamp: 2026-01-10T04:28:58.758Z
Learning: When reviewing Linux kernel device-tree bindings involving thermal cooling devices, THERMAL_NO_LIMIT can be used for both min_state (lower) and max_state (upper). If used as lower/min_state, the framework converts it to 0; if used as upper/max_state, it converts to the cooling device's max_state. Verify patch entries like: cooling-device = <&fan THERMAL_NO_LIMIT 1> are valid and correctly reflect the intended bounds. Ensure consistent interpretation of NO_LIMIT in both min and max contexts and that the resulting values align with the device's actual limits.
Applied to files:
patch/kernel/archive/sm8250-6.18/0045-arm64-dts-qcom-sm8250-xiaomi-elish-add-flash-led-node.patchpatch/kernel/archive/sm8250-6.18/0044-arm64-dts-qcom-sm8250-xiaomi-elish-add-bq25970-charger -node.patchpatch/kernel/archive/sm8250-6.18/0046-usb-gadget-Refactor-NCM-net_device-lifecycle.patch
📚 Learning: 2025-12-19T13:56:45.124Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Applied to files:
patch/kernel/archive/sm8250-6.18/0044-arm64-dts-qcom-sm8250-xiaomi-elish-add-bq25970-charger -node.patch
📚 Learning: 2026-01-12T17:21:32.630Z
Learnt from: rpardini
Repo: armbian/build PR: 9233
File: patch/u-boot/v2026.01/1000-rockchip-rk3588-stable-MAC-addresses-system-fixup-on-fdt-gmacX.patch:86-94
Timestamp: 2026-01-12T17:21:32.630Z
Learning: For U-Boot patches that implement custom MAC address injection into device tree nodes (such as for Rockchip RK3588 gmac interfaces), consider using U-Boot's standard fdt_fixup_ethernet() function instead. This function automatically walks /aliases looking for ethernet, ethernet0, ethernet1, etc., resolves each to a device node, and writes the corresponding environment MAC (ethaddr, eth1addr) into the mac-address or local-mac-address property. The standard approach requires: 1) adding ethernet aliases to board device trees (e.g., ethernet0 = &gmac0), 2) calling fdt_fixup_ethernet(blob) in ft_board_setup() or ft_system_setup(), and 3) ensuring ethaddr/eth1addr are set in the environment. This is simpler, well-tested, and easier to upstream than custom implementations.
Applied to files:
patch/kernel/archive/sm8250-6.18/0046-usb-gadget-Refactor-NCM-net_device-lifecycle.patch
📚 Learning: 2025-10-22T07:56:19.424Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8789
File: config/kernel/linux-sunxi64-edge.config:839-839
Timestamp: 2025-10-22T07:56:19.424Z
Learning: In Linux kernel configuration, some `=y` (builtin) options are infrastructure or feature flags that enable subsystems or features for modular drivers, rather than directly compiling code into the kernel. For example, in Armbian wireless configs, options like CONFIG_SPARD_WLAN_SUPPORT=y, CONFIG_SC23XX=y, CONFIG_WCN_BSP_DRIVER_BUILDIN=y, CONFIG_UNISOC_WIFI_PS=y are module infrastructure/feature enablers, while the actual drivers (CONFIG_WLAN_UWE5621=m, CONFIG_WLAN_UWE5622=m) remain as loadable modules. These infrastructure options don't cause kernel bloat.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-08-30T04:13:16.457Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-30T04:13:16.457Z
Learning: Armbian kernel configuration files like linux-filogic-current.config are autogenerated overlays on top of arch defconfig. Comments added manually will be lost during future updates by maintainers, and explicit "CONFIG_OPTION is not set" statements aren't needed for mutually exclusive options since these are overlay configs that only specify changes from the base configuration.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-07-19T00:03:42.995Z
Learnt from: lanefu
Repo: armbian/build PR: 8377
File: config/kernel/linux-uefi-arm64-cloud.config:165-170
Timestamp: 2025-07-19T00:03:42.995Z
Learning: CONFIG_NETKIT is a valid kernel configuration option in Linux kernel 6.12 and later versions, despite not being present in earlier versions like 6.9/6.10-rc.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-11-02T20:49:56.719Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-09-22T21:52:01.225Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8661
File: lib/functions/compilation/armbian-kernel.sh:194-201
Timestamp: 2025-09-22T21:52:01.225Z
Learning: The kernel_config_set_m function in lib/functions/compilation/armbian-kernel.sh preserves existing 'y' (built-in) settings and only sets options to 'm' (module) when they are not already built-in, achieving "prefer modules but allow built-ins" behavior.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-10-23T19:48:42.980Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8812
File: config/kernel/linux-sm8250-edge.config:498-501
Timestamp: 2025-10-23T19:48:42.980Z
Learning: For Armbian EDGE kernel configs, CONFIG_ATH12K=m alone is sufficient for PCI-based Wi-Fi 7 devices (e.g., WCN785x/QCN9274). A separate CONFIG_ATH12K_PCI option is not required, as confirmed by maintainer testing.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-09-27T21:50:15.915Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sunxi64-current.config:94-94
Timestamp: 2025-09-27T21:50:15.915Z
Learning: When kernel config files are generated through Armbian's automated process (previous config → armbian-kernel.sh changes → make oldconfig → Armbian machinery processing), manual config file edits are not appropriate since they would be overwritten. Deprecated option handling should be implemented in the automated tooling instead.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-09-27T21:47:58.020Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-bcm2711-edge.config:859-861
Timestamp: 2025-09-27T21:47:58.020Z
Learning: In the Armbian build system, kernel configuration files in config/kernel/ are generated through an automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-07-27T15:53:30.629Z
Learnt from: pyavitz
Repo: armbian/build PR: 8421
File: config/kernel/linux-sunxi64-edge.config:805-825
Timestamp: 2025-07-27T15:53:30.629Z
Learning: In the Armbian build system, kernel configurations prioritize broad hardware compatibility over optimization concerns. Even when including numerous legacy/obsolete drivers causes technical issues like increased initramfs size and slower enumeration, the project philosophy is to "appease the masses" by ensuring maximum device compatibility rather than optimizing for specific use cases.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-09-10T01:24:50.833Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-09-10T01:24:50.833Z
Learning: In Armbian's kernel configuration, both kernel_config_set_string CONFIG_LOCALVERSION '""' and kernel_config_set_string CONFIG_LOCALVERSION "" produce identical results in the final .config file (CONFIG_LOCALVERSION=""). The scripts/config tool handles quoting appropriately regardless of input format.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-04-30T16:16:47.150Z
Learnt from: The-going
Repo: armbian/build PR: 8147
File: config/sources/families/include/sunxi64_common.inc:38-39
Timestamp: 2025-04-30T16:16:47.150Z
Learning: The Armbian build system references Linux kernel versions in the form "tag:v6.14.4" in the KERNELBRANCH variable, even when point release tags might not be directly visible in the upstream repository in the same form.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-09-27T21:49:55.796Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sm8250-current.config:78-80
Timestamp: 2025-09-27T21:49:55.796Z
Learning: In the Armbian build system, kernel configuration files are generated through an automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-09-27T21:50:04.845Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8678
File: config/kernel/linux-sm8250-edge.config:80-82
Timestamp: 2025-09-27T21:50:04.845Z
Learning: In the Armbian build system, kernel configuration files are generated through this automated process: taking previous config → applying scripted changes from armbian-kernel.sh → running kernel's `make oldconfig` → processing by Armbian machinery back into config files. This automated process properly handles kernel configuration dependencies and reduces the likelihood of manual configuration errors.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-07-21T04:12:02.439Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-07-21T04:12:02.439Z
Learning: In the Armbian build system, for recurring maintenance tasks like kernel version bumping, TODO comments should use generic version formats (e.g., "MAJOR.MINOR-rc1") rather than specific version numbers (e.g., "6.17-rc1") to avoid the need for frequent comment updates that would create unnecessary maintenance overhead.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-08-11T22:00:13.411Z
Learnt from: rafayahmed317
Repo: armbian/build PR: 8484
File: config/kernel/linux-rk35xx-vendor.config:904-906
Timestamp: 2025-08-11T22:00:13.411Z
Learning: In the linux-rk35xx-vendor kernel, the LTE driver options (CONFIG_LTE, CONFIG_LTE_RM310, CONFIG_LTE_EM05) are defined as boolean in drivers/net/lte/Kconfig and cannot be built as modules - they must be either built-in (=y) or disabled.
Applied to files:
config/kernel/linux-sm8250-current.config
📚 Learning: 2025-10-23T19:50:25.841Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8812
File: config/kernel/linux-rockchip-edge.config:730-733
Timestamp: 2025-10-23T19:50:25.841Z
Learning: For ATH12K wireless driver configuration: only CONFIG_ATH12K=m needs to be explicitly set in kernel config files. The kernel build system automatically selects CONFIG_ATH12K_PCI when both CONFIG_ATH12K and CONFIG_PCI are enabled. This pattern is consistent across all Armbian edge kernel configs. CONFIG_ATH12K_PCI does not need to be explicitly added to config files.
Applied to files:
config/kernel/linux-sm8250-current.config
🔇 Additional comments (21)
patch/kernel/archive/sm8250-6.18/0045-arm64-dts-qcom-sm8250-xiaomi-elish-add-flash-led-node.patch (2)
14-21: LGTM!The
<dt-bindings/leds/common.h>include is correctly placed in alphabetical order and provides the necessaryLED_FUNCTION_FLASHandLED_COLOR_ID_WHITEmacros used in the flash LED node.
26-37: Flash LED node configuration looks correct.The flash LED binding properties are well-formed and use appropriate values:
led-sources = <1>— uses LED channel 1 (hardware-specific)led-max-microamp = <200000>— 200mA torch/indicator current limitflash-max-microamp = <1500000>— 1.5A flash current limit (standard for mobile flash)flash-max-timeout-us = <1280000>— 1.28s timeout (within safe hardware limits)The node correctly references
&pm8150l_flashwhich is defined in the upstream PM8150L PMIC dtsi. Based on learnings, this is part of porting existing SM8250 support to kernel 6.18.patch/kernel/archive/sm8250-6.18/0044-arm64-dts-qcom-sm8250-xiaomi-elish-add-bq25970-charger -node.patch (2)
110-116: LGTM!The pinctrl configuration for the master charger interrupt is correct: pull-up bias and input-enable are appropriate for an active-low interrupt line.
23-99: Charger node configuration looks reasonable for dual-charger topology.The threshold differences between master and slave nodes (e.g.,
die-therm-threshold145°C vs 125°C,bus-ocp-threshold3800mA vs 3500mA) appear intentional for the master/slave charger arrangement. Thesc8551,ac-ovp-thresholdproperties suggest driver compatibility with the SC8551 variant.config/kernel/linux-sm8250-current.config (11)
1-1: Version header updated to 6.18.Consistent with the PR objective of bumping SM8250 current kernel to 6.18.
73-74: KVM virtualization support enabled.Enabling
CONFIG_VIRTUALIZATIONandCONFIG_KVMis appropriate for the SM8250 (Snapdragon 865) platform which supports ARM virtualization extensions.
289-291: Wireless extensions and mesh networking enabled.
CFG80211_WEXTprovides compatibility with legacy wireless tools, andMAC80211_MESHenables 802.11s mesh networking capabilities. Both are sensible additions for broader wireless functionality.
358-358: NTSYNC module added for Wine/Proton compatibility.
CONFIG_NTSYNC=mprovides NT synchronization primitives useful for Wine/Proton gaming workloads. Appropriate as a module for desktop use cases on SM8250 tablets.
491-491: PPP module added.Standard Point-to-Point Protocol support for VPN and dial-up networking scenarios.
526-526: ATH12K Wi-Fi 7 support added.
CONFIG_ATH12K=menables support for Qualcomm Wi-Fi 7 chipsets (WCN785x/QCN9274). Based on learnings, this alone is sufficient asCONFIG_ATH12K_PCIis auto-selected when PCI is enabled.
612-612: BQ25970 charger driver enabled.Built-in support for the BQ25970 flash-charging controller aligns with the commit messages referencing charger compatibility improvements from downstream trees.
734-734: Logitech HID driver moved to module.Converting from built-in to module is fine; reduces kernel size while maintaining functionality when loaded.
742-753: USB HID device and ACM support added.
CONFIG_USB_HIDDEV=yenables userspace access to HID devices via/dev/usb/hiddevN, andCONFIG_USB_ACM=mprovides USB modem/serial support. Both are useful additions.
799-807: MMC subsystem converted from built-in to modules.The MMC stack and SDHCI drivers are now built as modules rather than built-in. This should be safe for SM8250 devices since they use UFS (
CONFIG_SCSI_UFSHCD=y) as the primary boot storage, with MMC/SD serving as optional secondary storage.Please confirm that no SM8250 devices in the Armbian build rely on MMC/SD as the root filesystem boot device. If any device boots from eMMC/SD, ensure the initramfs includes these modules or revert to built-in.
822-834: LED drivers and triggers converted to modules.Converting various LED subsystem components (
LEDS_USER,LEDS_GROUP_MULTICOLOR,LEDS_PWM_MULTICOLOR,LEDS_QCOM_FLASH,LEDS_QCOM_LPG, and triggers) from built-in to modules is appropriate. These are not critical for boot and can be loaded on demand.patch/kernel/archive/sm8250-6.18/0046-usb-gadget-Refactor-NCM-net_device-lifecycle.patch (6)
370-383: LGTM!The cleanup helper properly handles the NULL case, checks registration state before unregistering, flushes the work queue to prevent use-after-free, and then frees the net_device. The
DEFINE_FREEmacro integration at line 397 enables clean auto-cleanup semantics.
717-719: LGTM!The unbind sequence correctly nullifies
ioportbefore cleanup and usesnetdev_priv()directly from the storedncm->netdevpointer to obtain theeth_devfor cleanup.
765-773: LGTM!The
u_ncm.hchanges correctly add thegether_optsstructure for configuration caching and remove the now-unnecessaryboundflag, aligning with the new lifecycle model.
1-50: Patch addresses documented NCM lifecycle issues.This upstream patch from Google correctly addresses the NULL pointer dereference and dangling sysfs symlink issues described in the commit message. The refactoring to defer
net_deviceallocation toncm_bind()and usegether_optsfor pre-bind configuration caching is a sound approach. Based on learnings, this is an existing fix being ported to the 6.18 kernel rather than new functionality.
659-665: Theretain_and_null_ptrmacro is unavailable in kernel 6.18 and will cause build failures.The
retain_and_null_ptrmacro was introduced on March 19, 2025, which is after Linux 6.18 was released in September 2024. This means the code at lines 659–665 and 690–692 that usesretain_and_null_ptrwill fail to compile on the target kernel version. Either the patch must be updated to use an alternative approach compatible with 6.18, or the kernel version requirement must be raised.⛔ Skipped due to learnings
Learnt from: tabrisnet Repo: armbian/build PR: 8661 File: lib/functions/compilation/armbian-kernel.sh:194-199 Timestamp: 2025-09-25T18:37:00.330Z Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.Learnt from: tabrisnet Repo: armbian/build PR: 8661 File: lib/functions/compilation/armbian-kernel.sh:194-199 Timestamp: 2025-09-25T18:37:00.330Z Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.Learnt from: lanefu Repo: armbian/build PR: 8377 File: config/kernel/linux-uefi-arm64-cloud.config:165-170 Timestamp: 2025-07-19T00:03:42.995Z Learning: CONFIG_NETKIT was introduced in Linux kernel 4.14.330 (November 2023) and enables BPF-programmable network devices that can operate in Layer 3 or Layer 2 mode. It's a valid configuration option in modern kernels including 6.12.
506-512: The mutex protection here is correct and presents no deadlock risk. Neithergether_disconnect()norncm_reset_values()attempt to acquireopts->lock—they do not even have access to it. Thescoped_guardonopts->locksafely protects theopts->netassignment and disconnection logic without creating any reentrant locking scenario.Likely an incorrect or invalid review comment.
| + for (int i = 0; i < ETH_ALEN; i++) { \ | ||
| + unsigned char num; \ | ||
| + if ((*p == '.') || (*p == ':')) \ | ||
| + p++; \ | ||
| + num = hex_to_bin(*p++) << 4; \ | ||
| + num |= hex_to_bin(*p++); \ | ||
| + new_addr[i] = num; \ | ||
| + } \ |
There was a problem hiding this comment.
MAC address parsing lacks proper error handling for invalid hex characters.
The hex_to_bin() function returns -1 for invalid hex characters, but this error is not checked. When an invalid character is encountered, the code will silently use 0xff (from -1 << 4 | -1) or other corrupted values, potentially passing the is_valid_ether_addr() check with garbage data.
Suggested improvement
The same issue exists in the USB_ETHER_OPTS_ATTR_HOST_ADDR macro (lines 230-237). Consider validating hex_to_bin() return values or using mac_pton() helper which handles validation internally.
for (int i = 0; i < ETH_ALEN; i++) { \
- unsigned char num; \
+ int hi, lo; \
if ((*p == '.') || (*p == ':')) \
p++; \
- num = hex_to_bin(*p++) << 4; \
- num |= hex_to_bin(*p++); \
- new_addr[i] = num; \
+ hi = hex_to_bin(*p++); \
+ lo = hex_to_bin(*p++); \
+ if (hi < 0 || lo < 0) \
+ return -EINVAL; \
+ new_addr[i] = (hi << 4) | lo; \
} \🤖 Prompt for AI Agents
In
@patch/kernel/archive/sm8250-6.18/0046-usb-gadget-Refactor-NCM-net_device-lifecycle.patch
around lines 192 - 199, The parsing loop using hex_to_bin() (in the macro that
writes into new_addr and similarly in USB_ETHER_OPTS_ATTR_HOST_ADDR) doesn't
check for hex_to_bin() returning -1, which yields corrupted bytes; either
replace this manual parse with the safe mac_pton() helper or validate each
hex_to_bin() result (check for -1) and abort/propagate an error if seen before
assembling new_addr and calling is_valid_ether_addr(); update both the loop that
fills new_addr and the same logic in USB_ETHER_OPTS_ATTR_HOST_ADDR to perform
this validation.
After testing, I found that adding the bq25970 driver did not affect the use of a regular PD charger, and it also made my new version of the Xiaomi 120W charger work stably. |
|
Wait everyone, I found that postmarketos is still actively developing sm8250: https://gitlab.postmarketos.org/soc/qualcomm-sm8250/linux/-/commits/6.17.0?ref_type=HEADS |
This time the porting was much easier, I finished it up in just half an evening, I'll force push later, but before that I need to sleep first |
Description
Bump sm8250 edge to linux 6.18
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
./compile.sh kernel BOARD=xiaomi-elish BRANCH=edgeTest with elish：
Checklist:
Please delete options that are not relevant.
Summary by CodeRabbit
New Features
Improvements
Fixes
✏️ Tip: You can customize this high-level summary in your review settings.