Conversation
retro98boy
requested review from
amazingfate and
igorpecovnik
as code owners
github-actions
bot
added
02
📝 WalkthroughWalkthroughThis pull request introduces extensive kernel configuration and patch updates for SM8250 architecture, including kernel config refinements, a new KERNELBRANCH variable assignment, and 70 kernel patches spanning DRM notifiers, device-tree support for multiple devices, battery/charger drivers, input devices, sound subsystem enhancements, and various driver implementations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The 6.19 kernel patches added in this PR are ported entirely from 6.18, where they have been verified to build and function correctly. However, there are modifications to the TDM sound support patch. Since the sm8250 device I am using does not currently utilize TDM audio. @CodeChenL, would you mind testing this PR on your xiaomi-elish when you have a moment? I’d particularly appreciate it if you could verify the sound support. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (12)
patch/kernel/archive/sm8250-6.19/0016-drm-panel-Add-panel-driver-for-samsung-amsa26zp01.patch-273-280 (1)
273-280:⚠️ Potential issue | 🟠 MajorMissing error return after
regulator_bulk_enablefailure in probe.Line 275 logs the error but does not
return ret, so the probe continues with regulators not enabled — the subsequent GPIO request and panel initialization will proceed in an undefined power state.Additionally, this
regulator_bulk_enablecall in probe creates a refcount imbalance:prepare()callsregulator_bulk_enableagain (refcount → 2), butunprepare()only callsregulator_bulk_disableonce (refcount → 1), so the regulators are never truly disabled during panel power-off.If the intent is to power the panel during probe for GPIO acquisition, add a matching
regulator_bulk_disableafter the GPIO is obtained.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(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"); + + regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0016-drm-panel-Add-panel-driver-for-samsung-amsa26zp01.patch` around lines 273 - 280, The probe currently logs but does not return on regulator_bulk_enable failure and creates a refcount imbalance because probe enables regulators again that prepare/unprepare expect; change the probe so that if regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies) returns non-zero you return that error (use dev_err_probe or return ret) instead of only logging, and if you must enable regulators in probe to acquire the reset GPIO (devm_gpiod_get), call regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies) immediately after successfully obtaining ctx->reset_gpio to restore the refcount (so prepare/unprepare remain balanced); reference regulator_bulk_enable, regulator_bulk_disable, ctx->supplies, devm_gpiod_get, dev_err_probe, probe, prepare and unprepare in your change.patch/kernel/archive/sm8250-6.19/0067-Input-driver-for-AYN-Odin2-Gamepad.patch-244-277 (1)
244-277:⚠️ Potential issue | 🟠 MajorGPIO error pointers stored and later dereferenced.
devm_gpiod_get_optionalreturnsNULLwhen the property is absent, or anERR_PTRon a real error. WhenIS_ERRis true, the code logs a warning but leaves theERR_PTRvalue in the struct field. The subsequentif (gamepad_dev->boot_gpio)/reset_gpio/enable_gpiochecks (Lines 265-277) pass forERR_PTRvalues (they're non-NULL), causinggpiod_set_value_cansleepto dereference an invalid pointer → kernel oops.Either propagate the error or clear the field to
NULLafter logging.🐛 Proposed fix (option: propagate the error)
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); - } + if (IS_ERR(gamepad_dev->boot_gpio)) + return dev_err_probe(dev, PTR_ERR(gamepad_dev->boot_gpio), + "Unable to get boot gpio\n"); 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); - } + if (IS_ERR(gamepad_dev->enable_gpio)) + return dev_err_probe(dev, PTR_ERR(gamepad_dev->enable_gpio), + "Unable to get enable gpio\n"); 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 (IS_ERR(gamepad_dev->reset_gpio)) + return dev_err_probe(dev, PTR_ERR(gamepad_dev->reset_gpio), + "Unable to get reset gpio\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0067-Input-driver-for-AYN-Odin2-Gamepad.patch` around lines 244 - 277, The GPIO getters (devm_gpiod_get_optional) can return ERR_PTR which are currently logged but left in gamepad_dev->boot_gpio, ->enable_gpio and ->reset_gpio, and later treated as non-NULL causing invalid derefs in gpiod_set_value_cansleep; update the IS_ERR handling in the probe/init block (the checks immediately after devm_gpiod_get_optional calls) to either propagate the error by returning ret or clear the field to NULL after logging (e.g., set gamepad_dev->boot_gpio = NULL, etc.) so that later if checks only run for real GPIOs and never dereference ERR_PTR values.patch/kernel/archive/sm8250-6.19/0020-arm64-dts-qcom-add-device-tree-for-Xiaoxin-Pad-Pro-1.patch-1798-1803 (1)
1798-1803:⚠️ Potential issue | 🟠 MajorMissing
input-enableoncs35l45_2_irq_defaultbreaks speaker 2 interruptThe other three speaker IRQ pin states (
cs35l45_1_irq_default,cs35l45_3_irq_default,cs35l45_4_irq_default) all haveinput-enable. Without it on gpio51, the pin is not configured as an input, so the IRQ signal fromcs35l45_spk2will not be sensed by the SoC and the interrupt handler for speaker 2 will never fire.🐛 Proposed fix
cs35l45_2_irq_default: cs35l45-2-irq-default { pins = "gpio51"; function = "gpio"; drive-strength = <0x02>; bias-pull-up; + input-enable; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0020-arm64-dts-qcom-add-device-tree-for-Xiaoxin-Pad-Pro-1.patch` around lines 1798 - 1803, The cs35l45_2_irq_default pin-state is missing the input-enable property so gpio51 isn't configured as an input and the cs35l45_spk2 IRQ won't be seen; update the cs35l45_2_irq_default (cs35l45-2-irq-default) node to include input-enable (matching cs35l45_1_irq_default, cs35l45_3_irq_default, cs35l45_4_irq_default) so the SoC senses the speaker 2 interrupt.patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch-60-68 (1)
60-68:⚠️ Potential issue | 🟠 MajorMissing
<linux/regulator/consumer.h>include
struct regulator *vddinhtr3212_priv, and calls todevm_regulator_get,regulator_enable, andregulator_disableall require this header. The driver may compile today only through accidental transitive inclusion; an explicit include is needed for robustness.🐛 Proposed fix
`#include` <linux/of.h> +#include <linux/regulator/consumer.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch` around lines 60 - 68, The driver uses struct regulator *vdd and calls devm_regulator_get, regulator_enable, and regulator_disable (see htr3212_priv and its init/remove paths) but lacks the required header; add an explicit `#include` <linux/regulator/consumer.h> to the top-of-file includes to ensure these symbols and types are declared rather than relying on transitive includes.patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch-21-29 (1)
21-29:⚠️ Potential issue | 🟠 MajorSpurious
select REGMAP_I2Cin Kconfig — driver uses no regmap APIsThe driver communicates exclusively via
i2c_smbus_write_byte_data; there are noregmap_*calls anywhere inleds-htr3212.c. Thisselectadds an unnecessary dependency and should be removed.🐛 Proposed fix
config LEDS_HTR3212 tristate "HEROIC HTR3212 LED driver" depends on LEDS_CLASS && I2C - select REGMAP_I2C help🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch` around lines 21 - 29, Remove the spurious dependency by deleting the "select REGMAP_I2C" line from the LEDS_HTR3212 Kconfig block; this driver (LEDS_HTR3212) does not call any regmap_* APIs (see leds-htr3212.c which only uses i2c_smbus_write_byte_data), so simply remove that select to avoid pulling in REGMAP_I2C unnecessarily and leave the remaining lines (tristate, depends on LEDS_CLASS && I2C, help text) intact.patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch-291-308 (1)
291-308:⚠️ Potential issue | 🟠 MajorRegulator left enabled on probe failure paths
regulator_enableis called at line 291, but ifhtr3212_init_regs(line 302) orhtr3212_parse_dt(line 306) fails, probe returns an error andhtr3212_removeis never invoked — soregulator_disableis never called. The regulator leaks in the enabled state.Either add a devm cleanup action immediately after enabling, or use
devm_regulator_get_enable()which handles the lifecycle automatically.🐛 Proposed fix using devm action
ret = regulator_enable(priv->vdd); - if (ret < 0) { + if (ret < 0) return ret; - } + + ret = devm_add_action_or_reset(dev, (void (*)(void *))regulator_disable, priv->vdd); + if (ret) + return ret;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch` around lines 291 - 308, The probe enables the regulator via regulator_enable(priv->vdd) but on later failures (htr3212_init_regs or htr3212_parse_dt) the probe returns without disabling it; fix by making the enable managed: either replace regulator_enable(priv->vdd) with devm_regulator_get_enable(dev, "vdd") (or equivalent devm_regulator_get_enable call) so the regulator is automatically disabled on driver removal/failure, or immediately after a successful regulator_enable call add a devm cleanup action (e.g. devm_add_action_or_reset) that will call regulator_disable on priv->vdd on probe failure; ensure the cleanup action is registered before calling htr3212_init_regs or htr3212_parse_dt so failures will trigger the disable.patch/kernel/archive/sm8250-6.19/0070-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch-141-167 (1)
141-167:⚠️ Potential issue | 🟠 Major
vbatandvdc_5vdeclare output voltages above theirvin-supply— impossible for fixed regulators.
vreg_l11c_3p3is an LDO at 3.296 V. Using it as thevin-supplyfor a 4.2 V (vbat) and a 5.0 V (vdc_5v) fixed regulator is an incorrect topology description: a linear/fixed regulator cannot boost its input voltage. This also misrepresents the hardware for power-sequencing purposes.
vbatmost likely models battery/main rail voltage and should usevph_pwr(or novin-supplyif it is the root supply).vdc_5vtypically originates from a boost converter or USB VBUS, not from an LDO — itsvin-supplyshould reflect that or be omitted if the source is not modeled.🐛 Suggested correction (verify against schematic)
vbat: vbat-regulator { compatible = "regulator-fixed"; regulator-name = "VBAT"; - vin-supply = <&vreg_l11c_3p3>; + vin-supply = <&vph_pwr>; regulator-min-microvolt = <4200000>; regulator-max-microvolt = <4200000>; regulator-always-on; }; vdc_5v: vdc-5v-regulator { compatible = "regulator-fixed"; regulator-name = "VDC_5V"; regulator-min-microvolt = <5000000>; regulator-max-microvolt = <5000000>; regulator-always-on; - vin-supply = <&vreg_l11c_3p3>; + vin-supply = <&vph_pwr>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0070-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch` around lines 141 - 167, The DT nodes vbat and vdc_5v incorrectly list vreg_l11c_3p3 as their vin-supply while declaring output voltages higher than that input (vbat=4200000, vdc_5v=5000000), which is impossible for fixed/linear regulators; update the topology: remove or change the vin-supply for vbat to the real battery/main rail (e.g., reference vph_pwr or omit vin-supply if it is a root supply) and for vdc_5v set its vin-supply to the actual boost/USB VBUS source (or omit if not modeled) instead of &vreg_l11c_3p3, keeping vdc_3v3 as-is since it matches the 3.3V LDO; adjust regulator-name nodes vbat, vdc_5v and vin-supply references accordingly to reflect real hardware.patch/kernel/archive/sm8250-6.19/0070-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch-24-24 (1)
24-24:⚠️ Potential issue | 🟠 MajorRemove
/dts-v1/;from the.dtsifile — it belongs only in top-level.dtsfiles.
sm8250-retroidpocket-common.dtsicarries a/dts-v1/;directive at line 24. Bothsm8250-retroidpocket-rp5.dtsandsm8250-retroidpocket-rpmini.dtsalso have their own/dts-v1/;(patch lines 1465 and 1571). After the C-preprocessor expands the#include, dtc receives two/dts-v1/;directives in a single translation unit, which will produce a compiler error or warning depending on the dtc version.Upstream Linux kernel DTSI files never contain this directive; it is the responsibility of the top-level
.dtsfile.🐛 Proposed fix
// SPDX-License-Identifier: BSD-3-Clause /* * Copyright (c) 2024, Retroid Pocket. */ -/dts-v1/; - `#include` <dt-bindings/leds/common.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0070-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch` at line 24, Remove the stray /dts-v1/; directive from the common include file (sm8250-retroidpocket-common.dtsi) because only top-level .dts files should declare /dts-v1/;; locate the token "/dts-v1/;" in sm8250-retroidpocket-common.dtsi (the line added around the top of the file) and delete that single directive so the two top-level files sm8250-retroidpocket-rp5.dts and sm8250-retroidpocket-rpmini.dts retain the sole /dts-v1/; declaration after C-preprocessor include expansion.patch/kernel/archive/sm8250-6.19/0042-power-supply-qcom_fg-fix-compile-error.patch-28-33 (1)
28-33:⚠️ Potential issue | 🟠 Major
power_supply_get_by_name("power-supplies")is the wrong function for this API change — the DT property name cannot be used as a PSY lookup name.The original code used
power_supply_get_by_phandle()which correctly resolves the"power-supplies"phandle reference in the device tree to locate the actual charger power supply (e.g.,&bq25970_masteror&bq25970_slave).Switching to
power_supply_get_by_name("power-supplies")attempts to find a power supply registered with the literal name"power-supplies", but no power supply registers under this name. Chargers register with their actual names (e.g.,"bq2597x-master","bq2597x-slave"). This call will always fail, andchip->chg_psywill remain in an error state — breaking charger status integration silently.The API change needs to preserve the dynamic phandle resolution from the device tree, not hardcode a literal name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0042-power-supply-qcom_fg-fix-compile-error.patch` around lines 28 - 33, The change replaced power_supply_get_by_phandle(...) with power_supply_get_by_name("power-supplies"), which is incorrect because "power-supplies" is a DT phandle property not a registered PSY name; restore dynamic phandle resolution by calling power_supply_get_by_phandle(chip->dev->of_node, "power-supplies") (or parse the phandle via of_parse_phandle and resolve the referenced node) to populate chip->chg_psy correctly instead of looking up a literal "power-supplies" name.patch/kernel/archive/sm8250-6.19/0063-usb-gadget-Refactor-NCM-net_device-lifecycle.patch-277-295 (1)
277-295:⚠️ Potential issue | 🟠 MajorPotential lockdep violation:
opts->lock→rtnl_lock()orderingThe
ifname_showhandler acquiresopts->lock(viaguard(mutex)) and then callsrtnl_lock()inside that scope, establishing lock orderopts->lock → RTNL. RTNL is the global network lock held during many operations (rename, netdev notifiers). If any code path ever holds RTNL while attempting to acquireopts->lock, lockdep will flag an AB-BA deadlock.Since
opts->net_opts.name(the pre-bind cached name) is already guarded byopts->lock, RTNL is only needed for thenetdev_name(opts->net)branch. Consider reorganising to readopts->netunderopts->lock, then release the mutex before callingrtnl_lock()— or snapshot the pointer and perform the RTNL-protected name read outside the mutex scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0063-usb-gadget-Refactor-NCM-net_device-lifecycle.patch` around lines 277 - 295, The show handler _f_##_opts_ifname_show currently takes opts->lock and then calls rtnl_lock(), causing a potential lockdep AB-BA; fix by reading the minimal state under opts->lock (snapshot opts->net pointer and copy opts->net_opts.name and ifname_set), then drop opts->lock; if the snapshot net pointer is non-NULL, take rtnl_lock(), call netdev_name(snapshot_net) to get the name, release rtnl_lock(), and use that name; otherwise use the cached opts->net_opts.name that you copied under the mutex—this ensures you never hold opts->lock while taking rtnl_lock.patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch-375-388 (1)
375-388:⚠️ Potential issue | 🟠 Major
POWER_SUPPLY_PROP_ONLINEreturns raw bitmask, not 0/1
USBIN_PLUGIN_RT_STS_BITisBIT(4)(= 16), so*val = (stat & USBIN_PLUGIN_RT_STS_BIT)sets the online property to 16 when plugged in. The power supply core and userspace (sysfs, udev) expect 0 or 1.🐛 Proposed fix
- *val = (stat & USBIN_PLUGIN_RT_STS_BIT); + *val = !!(stat & USBIN_PLUGIN_RT_STS_BIT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch` around lines 375 - 388, In smb5_get_prop_usb_online, the code assigns the raw bitmask (stat & USBIN_PLUGIN_RT_STS_BIT) to *val, which yields 16 when plugged; change it to return a boolean 0/1 by testing the bit and converting to int (e.g., (*val = !!(stat & USBIN_PLUGIN_RT_STS_BIT))) after the successful regmap_read; keep the existing error handling and return paths intact.patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch-939-943 (1)
939-943:⚠️ Potential issue | 🟠 MajorFloat voltage register value written without bounds check
rcis computed as(voltage_max_design_uv - 3487500) / 7500 + 1. If DT provides avoltage-max-design-microvoltbelow 3487500 µV, the result is negative. That negativeintis silently cast tounsigned intwhen passed toregmap_update_bits, writing a wrapped-around large value toFLOAT_VOLTAGE_CFG— potentially programming a dangerously high float voltage. The intermediate value should use a dedicated local variable and be validated before the write.🐛 Proposed fix
- 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); - if (rc < 0) - return dev_err_probe(chip->dev, rc, "Couldn't set vbat max\n"); + { + int vbat_reg = (chip->batt_info->voltage_max_design_uv - 3487500) / 7500 + 1; + + if (vbat_reg < 0 || vbat_reg > 0xFF) + return dev_err_probe(chip->dev, -EINVAL, + "voltage_max_design_uv out of range\n"); + rc = regmap_update_bits(chip->regmap, chip->base + FLOAT_VOLTAGE_CFG, + FLOAT_VOLTAGE_SETTING_MASK, (unsigned int)vbat_reg); + if (rc < 0) + return dev_err_probe(chip->dev, rc, "Couldn't set vbat max\n"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch` around lines 939 - 943, The calculation for rc can produce a negative value which gets converted to unsigned and causes an invalid register write; introduce a dedicated signed local variable (e.g., int val) to compute val = (chip->batt_info->voltage_max_design_uv - 3487500) / 7500 + 1, validate that val is within the allowed range for FLOAT_VOLTAGE_SETTING_MASK (clamp or return an error via dev_err_probe if out of bounds), then pass the validated/clamped unsigned value to regmap_update_bits(chip->regmap, chip->base + FLOAT_VOLTAGE_CFG, FLOAT_VOLTAGE_SETTING_MASK, val) and keep the existing rc as the regmap return code for error handling.
🟡 Minor comments (24)
patch/kernel/archive/sm8250-6.19/0016-drm-panel-Add-panel-driver-for-samsung-amsa26zp01.patch-294-299 (1)
294-299:⚠️ Potential issue | 🟡 MinorMissing
drm_panel_removeon backlight error path.
drm_panel_addis called at line 294, but ifsamsung_amsa26zp01_create_backlightfails at line 296–299, the function returns an error without removing the panel. This leaves a stale panel registration. The same issue applies to the regulator imbalance from probe (if the earlier enable isn't cleaned up).Proposed fix
ctx->panel.backlight = samsung_amsa26zp01_create_backlight(dsi); - if (IS_ERR(ctx->panel.backlight)) + if (IS_ERR(ctx->panel.backlight)) { + drm_panel_remove(&ctx->panel); return dev_err_probe(dev, PTR_ERR(ctx->panel.backlight), "Failed to create backlight\n"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0016-drm-panel-Add-panel-driver-for-samsung-amsa26zp01.patch` around lines 294 - 299, The probe calls drm_panel_add(&ctx->panel) then attempts to create a backlight with samsung_amsa26zp01_create_backlight(dsi); if backlight creation fails the function returns via dev_err_probe but does not call drm_panel_remove, leaving the panel registered (and similarly any previously enabled regulators are not undone). Fix by cleaning up on error: if samsung_amsa26zp01_create_backlight returns an error, call drm_panel_remove(&ctx->panel) before returning, and also ensure any regulator enable/prepare done earlier (e.g. regulator_enable/regulator_bulk_enable or the specific regulator APIs used in this probe) are disabled/unprepared on all error paths; place these cleanup calls immediately before the dev_err_probe return so resources are not leaked.patch/kernel/archive/sm8250-6.19/0016-drm-panel-Add-panel-driver-for-samsung-amsa26zp01.patch-59-63 (1)
59-63:⚠️ Potential issue | 🟡 MinorMissing
#include <linux/regulator/consumer.h>.The driver uses
regulator_bulk_data,devm_regulator_bulk_get,regulator_bulk_enable, andregulator_bulk_disable, but does not include<linux/regulator/consumer.h>. While this may compile today due to transitive includes, it's fragile and violates the kernel convention of explicitly including headers for the APIs you use.Proposed fix
`#include` <linux/gpio/consumer.h> `#include` <linux/module.h> `#include` <linux/of.h> +#include <linux/regulator/consumer.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0016-drm-panel-Add-panel-driver-for-samsung-amsa26zp01.patch` around lines 59 - 63, The patch uses regulator APIs (regulator_bulk_data, devm_regulator_bulk_get, regulator_bulk_enable, regulator_bulk_disable) but does not include the header that defines them; add an explicit `#include` <linux/regulator/consumer.h> to the top-of-file includes so the regulator types and functions are properly declared (place it alongside the existing includes such as linux/backlight.h and linux/of.h).patch/kernel/archive/sm8250-6.19/0038-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-adds-us.patch-57-57 (1)
57-57:⚠️ Potential issue | 🟡 MinorTypo: "bellow" → "below".
-// downstream uses otg-use_external_boost which do not enable bellow +// downstream uses otg-use_external_boost which does not enable belowAlso fixed subject-verb agreement ("do not" → "does not") since "which" refers to the singular
otg-use_external_boost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0038-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-adds-us.patch` at line 57, Update the comment that references the property otg-use_external_boost: correct the typo "bellow" to "below" and fix subject-verb agreement by changing "do not" to "does not" so the line reads something like "// downstream uses otg-use_external_boost which does not enable below". Ensure the change is made where the comment containing "otg-use_external_boost" appears.patch/kernel/archive/sm8250-6.19/0065-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch-26-27 (1)
26-27:⚠️ Potential issue | 🟡 Minor
select DRM_DISPLAY_DP_HELPERappears unnecessary for a pure MIPI-DSI panel driver.The driver source (
panel-ddic-ch13726a.c) contains no calls to any DisplayPort helper APIs.DRM_DISPLAY_DP_HELPERis for DP link training, DPCD access, etc., none of which this DSI panel uses. This looks like a copy-paste from a DP-capable panel's Kconfig entry.🛠️ Proposed fix
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🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0065-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch` around lines 26 - 27, Remove the unnecessary DRM_DISPLAY_DP_HELPER selection from the Kconfig fragment that adds "select DRM_DISPLAY_DP_HELPER" (leave select DRM_DISPLAY_HELPER intact); specifically edit the Kconfig patch so the line "select DRM_DISPLAY_DP_HELPER" is deleted because the panel driver (panel-ddic-ch13726a.c) does not use any DisplayPort helper APIs and therefore should not select DRM_DISPLAY_DP_HELPER.patch/kernel/archive/sm8250-6.19/0020-arm64-dts-qcom-add-device-tree-for-Xiaoxin-Pad-Pro-1.patch-361-361 (1)
361-361:⚠️ Potential issue | 🟡 MinorTypo:
regualtor-allow-set-load→regulator-allow-set-loadThe misspelling causes the property to be silently ignored;
vreg_s2f_0p5(smps2 on pm8009) will not have load management enabled, potentially preventing correct LPM/HPM mode switching.🐛 Proposed fix
- regualtor-allow-set-load; + regulator-allow-set-load;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0020-arm64-dts-qcom-add-device-tree-for-Xiaoxin-Pad-Pro-1.patch` at line 361, There's a typo in the regulator node: the property was written as "regualtor-allow-set-load" which is ignored; change it to the correct "regulator-allow-set-load" on the vreg_s2f_0p5 (smps2 on pm8009) device-tree node so load management is enabled and LPM/HPM switching works correctly.patch/kernel/archive/sm8250-6.19/0020-arm64-dts-qcom-add-device-tree-for-Xiaoxin-Pad-Pro-1.patch-1642-1662 (1)
1642-1662:⚠️ Potential issue | 🟡 MinorUnresolved TODO: touchscreen
compatiblestring may be incorrectThe inline comment
// change to gt6975pindicates thegoodix,gt9916compatible string may be wrong for this hardware. If the actual IC is gt6975p and the kernel does not provide a matching compatible alias, the wrong driver will bind (or no driver will bind), breaking touchscreen support.Please confirm the actual IC variant and update the compatible string before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0020-arm64-dts-qcom-add-device-tree-for-Xiaoxin-Pad-Pro-1.patch` around lines 1642 - 1662, The touchscreen device node (touchscreen@0) has an incorrect placeholder/commented compatible; confirm the actual Goodix IC (gt6975p vs gt9916) and update the compatible property in the touchscreen@0 node to the correct string (e.g., "goodix,gt6975p" if confirmed), remove the inline comment, and if necessary add a secondary compatible entry or both compatible strings to ensure driver binding; verify the kernel tree provides a matching driver for the chosen compatible.patch/kernel/archive/sm8250-6.19/0020-arm64-dts-qcom-add-device-tree-for-Xiaoxin-Pad-Pro-1.patch-69-69 (1)
69-69:⚠️ Potential issue | 🟡 MinorTypo:
classis-typeshould bechassis-typeThe property name is misspelled. The kernel silently ignores unknown properties, so device chassis classification (used by udev/systemd) will not work.
🐛 Proposed fix
- classis-type = "tablet"; + chassis-type = "tablet";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0020-arm64-dts-qcom-add-device-tree-for-Xiaoxin-Pad-Pro-1.patch` at line 69, The device tree property is misspelled: replace the incorrect property name "classis-type" with the correct "chassis-type" in the patch (the line that currently reads classis-type = "tablet";) so the kernel/udev/systemd can detect the device chassis classification correctly.patch/kernel/archive/sm8250-6.19/0052-arm64-dts-qcom-sm8250-Add-device-tree-for-Xiaomi-Mi-.patch-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorMalformed
Signed-off-bytrailer: typo and non-ASCII email domain.Two issues on this line:
Singed-off-by:is misspelled —git interpret-trailersand checkpatch.pl won't recognize it as a valid SOB tag.- The email domain
dawidwróbel.comcontains a non-ASCII character (ó). TheFrom:header (line 2) correctly uses the ASCII formdawidwrobel.com.🐛 Proposed fix
-Singed-off-by: Dawid Wróbel <me@dawidwróbel.com> +Signed-off-by: Dawid Wróbel <me@dawidwrobel.com>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0052-arm64-dts-qcom-sm8250-Add-device-tree-for-Xiaomi-Mi-.patch` at line 15, The trailer line "Singed-off-by: Dawid Wróbel <me@dawidwróbel.com>" is malformed: fix the typo and non-ASCII characters by replacing the label with "Signed-off-by:" and normalizing the name and email to ASCII to match the From header (e.g., use "Dawid Wrobel" and "me@dawidwrobel.com"); update that trailer line in the patch so git interpret-trailers and checkpatch.pl recognize it.patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch-347-347 (1)
347-347:⚠️ Potential issue | 🟡 MinorMalformed email in
MODULE_AUTHOR— missing closing>MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); // ^ missing >🐛 Proposed fix
-MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); +MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch` at line 347, The MODULE_AUTHOR string is malformed because the email lacks a closing '>' causing a syntax/format issue; update the MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); entry to include the missing '>' so it reads MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>"); ensuring the author macro contains a properly formed name and email.patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch-196-197 (1)
196-197:⚠️ Potential issue | 🟡 Minor
always-onis read from the parent device instead of the child LED node
device_property_read_bool(dev, "always-on")checks the parent I2C device node, butalways-onis conventionally a per-LED property living in the child node. This means all LEDs share the same always-on state regardless of their individual DT configuration.🐛 Proposed fix
- if (!device_property_read_bool(dev, "always-on")) + if (!of_property_read_bool(child, "always-on")) cdev->flags |= LED_CORE_SUSPENDRESUME;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch` around lines 196 - 197, The code currently calls device_property_read_bool(dev, "always-on") which reads the parent I2C device node; change the call to read the per-LED child node by using the LED classdev's device (call device_property_read_bool(&cdev->dev, "always-on") so the property is fetched from the LED node) and then set cdev->flags |= LED_CORE_SUSPENDRESUME based on that result.patch/kernel/archive/sm8250-6.19/0064-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patch-4-5 (1)
4-5:⚠️ Potential issue | 🟡 MinorMalformed patch header:
Signed-off-byembedded inSubjectlineThe
Signed-off-by:trailer is concatenated into theSubject:header instead of appearing in the commit body after a blank line. This corrupts the commit message ingit logand is also why the patch filename itself containsSigned-off-by-Teguh. It should be structured as:-Subject: arm64: dts: qcom: sm8250: add uart16 Signed-off-by: Teguh Sobirin - <teguh@sobir.in> +Subject: arm64: dts: qcom: sm8250: add uart16 + +Signed-off-by: Teguh Sobirin <teguh@sobir.in>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0064-arm64-dts-qcom-sm8250-add-uart16-Signed-off-by-Teguh.patch` around lines 4 - 5, The Subject line contains the Signed-off-by trailer merged into it; edit the commit/patch so the Subject is just the short title "arm64: dts: qcom: sm8250: add uart16" and move "Signed-off-by: Teguh Sobirin <teguh@sobir.in>" into the commit body after a blank line as a proper trailer (and rename the patch file to remove "Signed-off-by-..." so the filename matches the cleaned Subject). Ensure the header, blank line, body, and trailer follow normal git commit message format.patch/kernel/archive/sm8250-6.19/0070-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch-59-67 (1)
59-67:⚠️ Potential issue | 🟡 MinorFan PWM device is never engaged by the thermal framework —
cooling-mapsare missing from all thermal zones.The
fannode declares#cooling-cells = <2>, registering itself as a cooling device. However, none of the five thermal zones inthermal-zones(lines 79–139) contain acooling-mapssection that references<&fan>. As a result, the thermal framework will never activate the fan in response to temperature trips, making the PWM fan control non-functional unless controlled from userspace.Add
cooling-mapsentries to the relevant thermal zones (e.g.skin-msm-thermal,pm8150l-pcb-thermal) to wire the fan into the thermal framework. Example skeleton:skin-msm-thermal { thermal-sensors = <&pm8150l_adc_tm 0>; trips { active-config0 { temperature = <50000>; hysteresis = <4000>; type = "passive"; }; + + fan_trip: fan-trip { + temperature = <45000>; + hysteresis = <5000>; + type = "active"; + }; }; + + cooling-maps { + map0 { + trip = <&fan_trip>; + cooling-device = <&fan THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; };Also applies to: 79-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0070-arm64-dts-qcom-add-SM8250-Retroid-Pocket-variant-Sig.patch` around lines 59 - 67, The fan PWM node declares itself as a cooling device but isn't referenced by any thermal zones, so add cooling-maps entries in the thermal-zones to bind the fan to temperature trips: update the thermal zones (e.g. skin-msm-thermal, pm8150l-pcb-thermal and any other zones within the thermal-zones list) to include a cooling-maps block that references <&fan> and maps trip points to cooling levels consistent with the fan's `#cooling-cells` (ensure the map keys match trip indices and values refer to the fan cooling levels), so the thermal framework can drive the pwm-fan node; keep the fan node’s `#cooling-cells` = <2> active and ensure cooling-maps use the correct trip indices for functions like skin-msm-thermal and pm8150l-pcb-thermal.patch/kernel/archive/sm8250-6.19/0050-arm64-dts-qcom-sm8250-xiaomi-elish-add-BT-swctrl-gpi.patch-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorMalformed email in
Signed-off-by
me@dawidwrobelcomis missing the.— it should beme@dawidwrobel.com, consistent with the same author's correct email in patches 0049 and 0054.📝 Proposed fix
-Signed-off-by: Dawid Wróbel <me@dawidwrobelcom> +Signed-off-by: Dawid Wróbel <me@dawidwrobel.com>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0050-arm64-dts-qcom-sm8250-xiaomi-elish-add-BT-swctrl-gpi.patch` at line 12, The Signed-off-by line in the patch header of 0050-arm64-dts-qcom-sm8250-xiaomi-elish-add-BT-swctrl-gpi.patch contains a malformed email ("me@dawidwrobelcom"); update that Signed-off-by entry to the correct email "me@dawidwrobel.com" so it matches the author's correct address used in patches 0049 and 0054.patch/kernel/archive/sm8250-6.19/0036-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-swtich-.patch-4-5 (1)
4-5:⚠️ Potential issue | 🟡 MinorTypo in Subject: "swtich" → "switch"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0036-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-swtich-.patch` around lines 4 - 5, The commit subject contains a typo: update the commit title string "Subject: [PATCH 36/62] arm64: dts: qcom: sm8250-oneplus-instantnoodlep: swtich" to use the correct spelling "switch" (i.e., change "swtich" → "switch") so the subject becomes "Subject: [PATCH 36/62] arm64: dts: qcom: sm8250-oneplus-instantnoodlep: switch fuel-gauge to bq27411".patch/kernel/archive/sm8250-6.19/0040-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-add-mis.patch-4-5 (1)
4-5:⚠️ Potential issue | 🟡 MinorTypo in Subject: "misssing" → "missing"
Triple-s in
add misssing pd.h. Minor but will propagate to any upstream submission.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0040-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-add-mis.patch` around lines 4 - 5, Fix the typo in the patch Subject: change "add misssing pd.h" to "add missing pd.h" in the commit/patch header (look for the Subject line in the patch named 0040-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-add-mis.patch) so the subject reads "arm64: dts: qcom: sm8250-oneplus-instantnoodlep: add missing pd.h".patch/kernel/archive/sm8250-6.19/0015-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch-208-213 (1)
208-213:⚠️ Potential issue | 🟡 Minor
status = "disabled"on pinctrl state sub-nodes has no effect
&qup_i2c7_defaultand&qup_spi7_data_clkare pinctrl state nodes managed by the QCOM TLMM pinctrl driver. According to the QCOM TLMM device tree schema, state nodes useunevaluatedProperties: false, meaning only documented pinctrl properties are valid—thestatusproperty is not recognized and is ignored by the pinctrl driver. These lines are no-ops.To prevent I2C7/SPI7 from probing and reconfiguring those pins, disable the device nodes instead:
Proposed fix
-&qup_i2c7_default { - status = "disabled"; -}; - -&qup_spi7_data_clk { - status = "disabled"; -}; +&i2c7 { + status = "disabled"; +}; + +&spi7 { + status = "disabled"; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0015-arm64-dts-qcom-sm8250-oneplus-instantnoodlep-Restore.patch` around lines 208 - 213, The pinctrl state sub-nodes &qup_i2c7_default and &qup_spi7_data_clk should not set status = "disabled" because QCOM TLMM ignores that property; remove those status lines from those state nodes and instead disable the actual device nodes for I2C7 and SPI7 (the kernel device nodes that instantiate those controllers) by setting their status = "disabled" (e.g., the nodes referenced as qup_i2c7 / qup_spi7 or their compatible controller nodes), so the controllers won't probe and reconfigure the pins.patch/kernel/archive/sm8250-6.19/0060-arm64-dts-qcom-sm8250-xiaomi-elish-add-hall-key.patch-20-36 (1)
20-36:⚠️ Potential issue | 🟡 MinorTypo in label properties: "Swtich" → "Switch".
Both
switch-lid(line 21) andswitch-tablet-mode(line 30) havelabel = "Swtich ..."— this should be"Switch ...". The label is user-visible (e.g., in/proc/bus/input/devices).Also, both nodes use the deprecated
gpio-key,wakeupproperty while the adjacentkey-vol-upnode uses the modernwakeup-source. Consider aligning for consistency.Proposed fix
switch-lid { - label = "Swtich Lid"; + label = "Switch Lid"; gpios = <&tlmm 110 GPIO_ACTIVE_LOW>; linux,input-type = <EV_SW>; debounce-interval = <15>; linux,code = <SW_LID>; - gpio-key,wakeup; + wakeup-source; }; switch-tablet-mode { - label = "Swtich Tablet Mode"; + label = "Switch Tablet Mode"; gpios = <&tlmm 121 GPIO_ACTIVE_LOW>; linux,input-type = <EV_SW>; debounce-interval = <15>; linux,code = <SW_TABLET_MODE>; - gpio-key,wakeup; + wakeup-source; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0060-arm64-dts-qcom-sm8250-xiaomi-elish-add-hall-key.patch` around lines 20 - 36, Fix the typo in the label properties for the nodes switch-lid and switch-tablet-mode (change "Swtich" to "Switch"), and replace the deprecated gpio-key,wakeup property in both nodes with the modern wakeup-source property to match the adjacent key-vol-up node; update only the label and wakeup property names in the switch-lid and switch-tablet-mode nodes (retain other properties like gpios, linux,input-type, debounce-interval, and linux,code).patch/kernel/archive/sm8250-6.19/0068-input-add-Qualcomm-SPMI-haptics-driver.patch-594-602 (1)
594-602:⚠️ Potential issue | 🟡 MinorUnchecked return value from auto-resonance configuration write.
The return value from
spmi_haptics_write_maskedfor the auto-resonance register (line 600–601) is assigned toretbut not checked before being overwritten at line 604.🐛 Proposed fix
ret = spmi_haptics_write_masked(haptics, haptics->base + HAP_LRA_AUTO_RES_REG, mask, val); + if (ret < 0) + return ret; /* Configure the PLAY MODE register */ ret = spmi_haptics_write_play_mode(haptics);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0068-input-add-Qualcomm-SPMI-haptics-driver.patch` around lines 594 - 602, The call to spmi_haptics_write_masked setting auto-resonance (targeting haptics->base + HAP_LRA_AUTO_RES_REG) stores its return in ret but is never checked before ret is reused; update the code to check ret immediately after that call and handle/report the error (e.g., return ret or log and goto out) instead of allowing it to be overwritten by subsequent operations—specifically validate the return from spmi_haptics_write_masked and bail out on failure before proceeding.patch/kernel/archive/sm8250-6.19/0019-ASoC-qcom-sm8250-Add-primary-tdm-support.patch-19-33 (1)
19-33:⚠️ Potential issue | 🟡 MinorUnchecked return values from
snd_soc_dai_set_sysclk.Both the CPU DAI sysclk (line 21) and codec DAI sysclk (line 27) calls have their return values discarded. If clock configuration fails, the startup proceeds silently, which could lead to hard-to-diagnose audio issues.
Additionally, inside the loop, the error check on line 28 only covers
snd_soc_dai_set_fmt— ifset_fmtsucceeds butset_sysclkfails on line 27, the failure is silently ignored.Proposed fix
case PRIMARY_TDM_RX_0: codec_dai_fmt |= SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_DSP_A; - snd_soc_dai_set_sysclk(cpu_dai, - Q6AFE_LPASS_CLK_ID_PRI_TDM_IBIT , + ret = snd_soc_dai_set_sysclk(cpu_dai, + Q6AFE_LPASS_CLK_ID_PRI_TDM_IBIT, TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK); + if (ret < 0) { + dev_err(rtd->dev, "TDM cpu_dai sysclk 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); - 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 codec_dai sysclk err:%d\n", ret); + return ret; + } } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0019-ASoC-qcom-sm8250-Add-primary-tdm-support.patch` around lines 19 - 33, The cpu and codec sysclk calls in the PRIMARY_TDM_RX_0 case ignore return values; capture the return of snd_soc_dai_set_sysclk for the cpu_dai and for each codec_dai (inside the for_each_rtd_codec_dais loop), check if ret < 0 after each call, log an error via dev_err(rtd->dev, "TDM sysclk err:%d\n", ret) (or similar) and return the error immediately; also ensure the existing check for snd_soc_dai_set_fmt still runs and does not get bypassed by reusing the same ret variable without validating the sysclk call results.patch/kernel/archive/sm8250-6.19/0037-power-supply-qcom_pm8150b_charger-adds-charging-cont.patch-113-124 (1)
113-124:⚠️ Potential issue | 🟡 MinorKernel power_supply ABI extension: inserting a new enum value
Adding
POWER_SUPPLY_PROP_CHARGING_ENABLEDin the middle of thepower_supply_propertyenum shifts integer values of all subsequent properties. This is fine for this out-of-tree context since userspace accesses these via sysfs string names, but be aware this would conflict with any other out-of-tree patches that also extend this enum. The sysfs attr array insertion (lines 35–42) correctly mirrors the enum position.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0037-power-supply-qcom_pm8150b_charger-adds-charging-cont.patch` around lines 113 - 124, Inserting POWER_SUPPLY_PROP_CHARGING_ENABLED into the middle of the power_supply_property enum shifts all subsequent values; either move this new enum to the end of power_supply_property to avoid ABI shifts or, if you must insert it in-place, update the corresponding sysfs attribute array (the power_supply_attrs entries that map properties to sysfs names) so the order stays consistent and add a clear comment above power_supply_property and the sysfs attr array documenting the ABI change and risk of conflicts with other out-of-tree patches; ensure references to POWER_SUPPLY_PROP_CHARGING_ENABLED are used consistently (e.g., any switch/case or array index lookups) and run a build to verify no index mismatches.patch/kernel/archive/sm8250-6.19/0063-usb-gadget-Refactor-NCM-net_device-lifecycle.patch-577-580 (1)
577-580:⚠️ Potential issue | 🟡 Minor
PTR_ERRdiscarded — error code masked as-ENOMEMnetdev = gether_setup_default(); if (IS_ERR(netdev)) return -ENOMEM; // masks the actual error
gether_setup_default()can fail with error codes other than-ENOMEM(e.g., viagether_setup_name), and callers will always see-ENOMEMregardless.🐛 Proposed fix
- netdev = gether_setup_default(); - if (IS_ERR(netdev)) - return -ENOMEM; + netdev = gether_setup_default(); + if (IS_ERR(netdev)) + return PTR_ERR(netdev);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0063-usb-gadget-Refactor-NCM-net_device-lifecycle.patch` around lines 577 - 580, The code masks the real error from gether_setup_default() by returning the constant -ENOMEM; instead, check IS_ERR(netdev) and return PTR_ERR(netdev) so the caller receives the original error code (replace the current "return -ENOMEM;" with "return PTR_ERR(netdev);"). Ensure you include the PTR_ERR usage alongside the existing IS_ERR(netdev) check in the same block where netdev is assigned from gether_setup_default().patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch-974-977 (1)
974-977:⚠️ Potential issue | 🟡 MinorTypo: "sentinal" → "sentinel"
🐛 Proposed fix
- { /* sentinal */ } + { /* sentinel */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch` around lines 974 - 977, The struct initializer for smb5_match_id_table contains a misspelled comment "sentinal"; update the comment to "sentinel" in the smb5_match_id_table definition to correct the typo (keep the sentinel empty initializer entry intact: { /* sentinel */ }).patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch-305-309 (1)
305-309:⚠️ Potential issue | 🟡 Minor
CDP_CURRENT_UAexceeds USB BC 1.2 specification
CDP_CURRENT_UAis defined as 3000000 µA (3 A). USB BC 1.2 specifies a maximum of 1500000 µA (1.5 A) for Charging Downstream Ports. Setting 3 A may overload non-PD CDP ports.🐛 Proposed fix
-#define CDP_CURRENT_UA 3000000 +#define CDP_CURRENT_UA 1500000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch` around lines 305 - 309, CDP_CURRENT_UA is incorrectly set to 3000000 µA which exceeds USB BC 1.2 limits; change CDP_CURRENT_UA to at most 1500000 (µA) and ensure CURRENT_MAX_UA is updated to reflect the new maximum (e.g., max of SDP_CURRENT_UA, DCP_CURRENT_UA, CDP_CURRENT_UA), verify all units are µA and adjust any logic that depends on CURRENT_MAX_UA accordingly (references: CDP_CURRENT_UA, SDP_CURRENT_UA, DCP_CURRENT_UA, CURRENT_MAX_UA).patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch-980-986 (1)
980-986:⚠️ Potential issue | 🟡 MinorNo
.removecallback —dev_pm_set_wake_irqleak on driver unbind
dev_pm_set_wake_irq(line 962) is not adevm_function and has no correspondingdev_pm_clear_wake_irqwhen the driver is removed. The wake-IRQ configuration persists after unbind, which can prevent the device from entering low-power states correctly.🐛 Proposed fix — add a remove callback
+static void smb5_remove(struct platform_device *pdev) +{ + struct smb5_chip *chip = platform_get_drvdata(pdev); + + dev_pm_clear_wake_irq(chip->dev); +} + static struct platform_driver qcom_spmi_smb5 = { .probe = smb5_probe, + .remove = smb5_remove, .driver = { .name = "qcom-pm8150b-charger", .of_match_table = smb5_match_id_table, }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch` around lines 980 - 986, The driver registers dev_pm_set_wake_irq in smb5_probe but the platform_driver qcom_spmi_smb5 has no .remove to clear it, leaking wake-IRQ state; add a remove callback (e.g., smb5_remove) and set qcom_spmi_smb5.remove = smb5_remove that calls dev_pm_clear_wake_irq(dev) (matching the dev used in smb5_probe) and performs any cleanup mirror of smb5_probe so the wake IRQ is cleared on unbind.
| + supply_config.of_node = pdev->dev.of_node; | ||
| + | ||
| + chip->batt_psy = devm_power_supply_register(chip->dev, | ||
| + &batt_psy_desc, &supply_config); | ||
| + if (IS_ERR(chip->batt_psy)) { | ||
| + if (PTR_ERR(chip->batt_psy) != -EPROBE_DEFER) | ||
| + dev_err(&pdev->dev, "Failed to register battery\n"); | ||
| + return PTR_ERR(chip->batt_psy); | ||
| + } | ||
| + | ||
| + platform_set_drvdata(pdev, chip); | ||
| + | ||
| + ret = power_supply_get_battery_info(chip->batt_psy, &chip->batt_info); | ||
| + if (ret) { | ||
| + dev_err(&pdev->dev, "Failed to get battery info: %d\n", ret); | ||
| + return ret; | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -100 patch/kernel/archive/sm8250-6.19/0024-power-supply-Add-driver-for-Qualcomm-PMIC-fuel-gauge.patch | cat -nRepository: armbian/build
Length of output: 4343
🏁 Script executed:
wc -l patch/kernel/archive/sm8250-6.19/0024-power-supply-Add-driver-for-Qualcomm-PMIC-fuel-gauge.patchRepository: armbian/build
Length of output: 158
🏁 Script executed:
sed -n '1230,1260p' patch/kernel/archive/sm8250-6.19/0024-power-supply-Add-driver-for-Qualcomm-PMIC-fuel-gauge.patch | cat -nRepository: armbian/build
Length of output: 1191
🏁 Script executed:
sed -n '1360,1375p' patch/kernel/archive/sm8250-6.19/0024-power-supply-Add-driver-for-Qualcomm-PMIC-fuel-gauge.patch | cat -nRepository: armbian/build
Length of output: 550
🌐 Web query:
Linux kernel 6.10 power_supply_get_battery_info API change
💡 Result:
In Linux v6.10, power_supply_get_battery_info() uses the “returns an allocated struct” API (not the old “caller-provides-a-struct” API).
What changed (old → v6.10)
Old API (older kernels / some vendor trees):
int power_supply_get_battery_info(struct power_supply *psy,
struct power_supply_battery_info *info);v6.10 API:
int power_supply_get_battery_info(struct power_supply *psy,
struct power_supply_battery_info **info_out);
void power_supply_put_battery_info(struct power_supply *psy,
struct power_supply_battery_info *info);The core allocates the top-level struct power_supply_battery_info (and any internal tables it hangs off), returns it via *info_out, and you must release it with power_supply_put_battery_info() when done. This change was introduced so the core can hand out shared/static battery-info (and manage mixed static + dynamically allocated sub-objects) more cleanly. [1][2]
Typical v6.10 call pattern
struct power_supply_battery_info *bi;
int ret;
ret = power_supply_get_battery_info(psy, &bi);
if (ret)
return ret;
/* use bi->... */
power_supply_put_battery_info(psy, bi);Porting hint: if your driver had struct power_supply_battery_info bi; on the stack (or embedded in a device struct), you generally need to change it to a pointer and store the returned bi. [2]
Sources:
[1] LKML thread showing the v6.10-style prototype (**info_out) in-tree usage/discussion: https://lkml.indiana.edu/hypermail/linux/kernel/2504.1/01794.html
[2] Commit “power: supply_core: Pass pointer to battery info” explaining the rationale and converting users: https://git.zx2c4.com/linux-rng/commit/drivers/power?id=25fd330370ac40653671f323acc7fb6db27ef6fe
🌐 Web query:
power_supply_put_battery_info kernel 6.10 signature change
💡 Result:
In Linux v6.10, the battery-info helpers are used in the “allocated object” style:
int power_supply_get_battery_info(struct power_supply *psy,
struct power_supply_battery_info **info);
void power_supply_put_battery_info(struct power_supply *psy,
struct power_supply_battery_info *info);So the key practical change (what usually breaks builds) is that you now pass a struct power_supply_battery_info * to power_supply_put_battery_info(), not the address of a caller-owned struct. New in-tree driver code looks like: get into a pointer, then put(psy, bat_info) to release it. [1]
What to change in your driver
Old pattern (caller-owned struct):
struct power_supply_battery_info info;
ret = power_supply_get_battery_info(psy, &info);
...
power_supply_put_battery_info(psy, &info);(Example of this style in older patches: put(..., &bat_info)). [2]
New pattern (framework-owned allocation):
struct power_supply_battery_info *info;
ret = power_supply_get_battery_info(psy, &info);
...
power_supply_put_battery_info(psy, info);(Example of this style in newer patches: put(..., bat_info)). [1]
Sources: [1] [2]
Fix power_supply battery-info API calls for kernel 6.10+ compatibility.
Lines 1247 and 1366 use incompatible API signatures. In kernel 6.10+, power_supply_get_battery_info and power_supply_put_battery_info changed to use pointer-based allocation:
power_supply_get_battery_infonow expects a double pointer (struct power_supply_battery_info **) and still returnsintpower_supply_put_battery_infosignature remains(struct power_supply *psy, struct power_supply_battery_info *info)but info must now be a pointer
Required changes:
- Change
chip->batt_infofrom embedded struct to pointer in the chip structure - Update line 1247 call (signature stays the same, but
batt_infois now a pointer) - Update line 1366 call (signature stays the same)
Example fix pattern
- ret = power_supply_get_battery_info(chip->batt_psy, &chip->batt_info);
- if (ret) {
+ ret = power_supply_get_battery_info(chip->batt_psy, &chip->batt_info);
+ if (ret) {
dev_err(&pdev->dev, "Failed to get battery info: %d\n", ret);
return ret;(Note: The call syntax remains identical; the critical change is declaring batt_info as a pointer in the chip struct, not an embedded struct.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0024-power-supply-Add-driver-for-Qualcomm-PMIC-fuel-gauge.patch`
around lines 1235 - 1251, Change chip->batt_info from an embedded struct to a
pointer in the driver chip struct, then update uses of
power_supply_get_battery_info and power_supply_put_battery_info to pass/receive
a pointer: call power_supply_get_battery_info(chip->batt_psy, &chip->batt_info)
where batt_info is declared as struct power_supply_battery_info *batt_info; and
later call power_supply_put_battery_info(chip->batt_psy, chip->batt_info) (or
free/reset as appropriate). Ensure platform_set_drvdata and error handling
around chip->batt_psy remain unchanged and adjust any dereferences of
chip->batt_info elsewhere to use pointer syntax.
| +static int qcom_fg_gen4_get_temperature(struct qcom_fg_chip *chip, int *val) | ||
| +{ | ||
| + int temp; | ||
| + u8 readval[2]; | ||
| + int ret; | ||
| + | ||
| + ret = qcom_fg_read(chip, readval, ADC_RR_BATT_TEMP_LSB, 2); | ||
| + if (ret) { | ||
| + dev_err(chip->dev, "Failed to read temperature: %d", ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + temp = readval[1] << 8 | readval[0]; | ||
| + *val = temp * 10; | ||
| + return 0; | ||
| +} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PM8150B ADC_RR_BATT_TEMP_LSB register temperature format documentation
💡 Result:
For Qualcomm PM8150B RRADC battery-thermistor temperature result:
-
Registers
ADC_RR_BATT_TEMP_LSB=rradc_base + 0x88ADC_RR_BATT_TEMP_MSB=rradc_base + 0x89- Only MSB[1:0] are used (
GEN4_BATT_TEMP_MSB_MASK = GENMASK(1,0)), giving a 10-bit value total. [1]
-
Data format / units
- The temperature code is a signed 10-bit value (sign bit = bit 9), range -128 to +127.
- 1 LSB = 0.25 °C
- So: Temp(°C) = sign_extend(code10, 9) * 0.25. [2]
Linux driver reference implementation
- The kernel code does
sign_extend32(buf, 9)and converts to deci-°C via* 100 / 40(equivalent to* 0.25 * 10). [2]
Sources: [1] [2]
🏁 Script executed:
# Search for qcom_fg driver references to understand register formats and temperature handling
rg "ADC_RR_BATT_TEMP" --type c
rg "qcom_fg.*temperature" --type c -A 5Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Check if there are other instances of reading temperature with sign extension in the codebase
rg "readval.*<<.*8|.*readval" --type c -B 2 -A 2
rg "\(s16\).*<<\s*8" --type cRepository: armbian/build
Length of output: 39
🏁 Script executed:
# Check if there are other temperature read functions in the patch or driver for comparison
rg "temperature|get_temp" patch/kernel/archive/sm8250-6.19/0025-power-qcom_fg-Add-initial-pm8150b-support.patch -iRepository: armbian/build
Length of output: 635
Temperature reading is critically broken for both negative and positive temperatures.
The register is 10-bit signed (MSB[1:0] + LSB[7:0]), not 16-bit. Each LSB represents 0.25 °C, per the Qualcomm PM8150B RRADC documentation. The patch code has three errors:
- No sign extension — negative temperatures become large positive values (e.g., -20°C reads as ~65000).
- Bit-width mismatch — only lower 10 bits are valid; bits [15:10] must be masked or ignored.
- Wrong unit conversion — multiplying by 10 produces temperature 4× too large. Correct factor is 2.5 (or 100/40 in integer math to convert 0.25°C per LSB to deci-°C).
The kernel reference implementation uses:
temp = sign_extend32(buf, 9); // 10-bit signed
*val = temp * 100 / 40; // converts to deci-°CApply the following fix:
🐛 Required fix
- temp = readval[1] << 8 | readval[0];
- *val = temp * 10;
+ temp = sign_extend32(readval[1] << 8 | readval[0], 9);
+ *val = temp * 100 / 40;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0025-power-qcom_fg-Add-initial-pm8150b-support.patch`
around lines 158 - 173, qcom_fg_gen4_get_temperature assembles a full 16-bit
value, fails to mask to the 10 valid bits, does not sign-extend the 10-bit
signed temperature, and converts units incorrectly; fix by building a 10-bit
value from readval[1] (use only bits [1:0] as MSB) and readval[0] (LSB), mask to
10 bits, sign-extend that 10-bit signed value (e.g. using sign_extend32(value,
9)), then convert LSB units of 0.25°C to deci-°C with *val = temp * 100 / 40
(integer math) and return 0 on success; keep error handling on qcom_fg_read and
use the existing ADC_RR_BATT_TEMP_LSB/readval/temp symbols to locate the code.
| +static irqreturn_t smb5_handle_batt_overvoltage(int irq, void *data) | ||
| +{ | ||
| + struct smb5_chip *chip = data; | ||
| + unsigned int status; | ||
| + | ||
| + regmap_read(chip->regmap, chip->base + BATTERY_CHARGER_STATUS_2, | ||
| + &status); | ||
| + | ||
| + if (status & CHARGER_ERROR_STATUS_BAT_OV_BIT) { | ||
| + /* The hardware stops charging automatically */ | ||
| + dev_err(chip->dev, "battery overvoltage detected\n"); | ||
| + power_supply_changed(chip->chg_psy); | ||
| + } | ||
| + | ||
| + return IRQ_HANDLED; | ||
| +} |
There was a problem hiding this comment.
Uninitialized status variable used if regmap_read fails
regmap_read's return value is silently discarded. If the read fails, status remains uninitialized, and the subsequent if (status & CHARGER_ERROR_STATUS_BAT_OV_BIT) check reads garbage — potentially triggering a spurious "battery overvoltage detected" error log and calling power_supply_changed unnecessarily.
🐛 Proposed fix
static irqreturn_t smb5_handle_batt_overvoltage(int irq, void *data)
{
struct smb5_chip *chip = data;
- unsigned int status;
+ unsigned int status = 0;
+ int rc;
- regmap_read(chip->regmap, chip->base + BATTERY_CHARGER_STATUS_2,
- &status);
+ rc = regmap_read(chip->regmap, chip->base + BATTERY_CHARGER_STATUS_2,
+ &status);
+ if (rc < 0) {
+ dev_err(chip->dev, "Couldn't read charger status 2, rc=%d\n", rc);
+ return IRQ_HANDLED;
+ }📝 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 irqreturn_t smb5_handle_batt_overvoltage(int irq, void *data) | |
| +{ | |
| + struct smb5_chip *chip = data; | |
| + unsigned int status; | |
| + | |
| + regmap_read(chip->regmap, chip->base + BATTERY_CHARGER_STATUS_2, | |
| + &status); | |
| + | |
| + if (status & CHARGER_ERROR_STATUS_BAT_OV_BIT) { | |
| + /* The hardware stops charging automatically */ | |
| + dev_err(chip->dev, "battery overvoltage detected\n"); | |
| + power_supply_changed(chip->chg_psy); | |
| + } | |
| + | |
| + return IRQ_HANDLED; | |
| +} | |
| static irqreturn_t smb5_handle_batt_overvoltage(int irq, void *data) | |
| { | |
| struct smb5_chip *chip = data; | |
| unsigned int status = 0; | |
| int rc; | |
| rc = regmap_read(chip->regmap, chip->base + BATTERY_CHARGER_STATUS_2, | |
| &status); | |
| if (rc < 0) { | |
| dev_err(chip->dev, "Couldn't read charger status 2, rc=%d\n", rc); | |
| return IRQ_HANDLED; | |
| } | |
| if (status & CHARGER_ERROR_STATUS_BAT_OV_BIT) { | |
| /* The hardware stops charging automatically */ | |
| dev_err(chip->dev, "battery overvoltage detected\n"); | |
| power_supply_changed(chip->chg_psy); | |
| } | |
| return IRQ_HANDLED; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0028-From-Teguh-Sobirin-teguh-sobir.in-Date-Sun-24-Nov-20.patch`
around lines 687 - 702, The IRQ handler smb5_handle_batt_overvoltage uses an
uninitialized local variable status because regmap_read's return value is
ignored; change the handler to check the return of regmap_read, initialize
status to 0, and on regmap_read failure log an error (using dev_err with the
returned error) and return IRQ_NONE to avoid evaluating garbage and spuriously
calling power_supply_changed; only evaluate (status &
CHARGER_ERROR_STATUS_BAT_OV_BIT) and call power_supply_changed when regmap_read
succeeds.
| + enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; | ||
| + swctrl-gpios = <&tlmm 124 GPIO_ACTIVE_HIGH>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the hci_qca.c file around the QCA_QCA6390 case
# to confirm the control flow
rg -n -A 30 -B 5 "case QCA_QCA6390" drivers/bluetooth/hci_qca.cRepository: armbian/build
Length of output: 124
🏁 Script executed:
# Check what happens in the default case to confirm enable-gpios handling
rg -n -A 20 "devm_gpiod_get_optional.*enable\|bt_en.*devm_gpiod" drivers/bluetooth/hci_qca.cRepository: armbian/build
Length of output: 124
🏁 Script executed:
# Search for any separate qca6390-bt driver or references
find . -type f -name "*qca6390*" -o -name "*qca6390-bt*" 2>/dev/null | head -20Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Check if there are other files that might consume these GPIOs for QCA6390
rg -rn "qca6390.*gpio\|qca6390.*enable\|qca6390.*swctrl" --type=c --type=rsRepository: armbian/build
Length of output: 83
🏁 Script executed:
# Also search for pwrseq references to understand the power sequencing path
rg -n "pwrseq_get\|qca6390-pmu" drivers/bluetooth/hci_qca.cRepository: armbian/build
Length of output: 124
🏁 Script executed:
# Find the patch files mentioned
find . -name "0049*" -o -name "0050*" | grep -E "(sm8250|elish)"Repository: armbian/build
Length of output: 452
🏁 Script executed:
# List the directory to understand the patch structure
ls -la patch/kernel/archive/sm8250-6.19/ 2>/dev/null | head -30Repository: armbian/build
Length of output: 3062
🏁 Script executed:
# Read the patch 0049 to see what's actually being added
cat patch/kernel/archive/sm8250-6.19/0049-arm64-dts-qcom-sm8250-xiaomi-elish-add-enable-and-sw.patch 2>/dev/null | head -80Repository: armbian/build
Length of output: 1337
🏁 Script executed:
# Also check patch 0050 if it exists
ls -la patch/kernel/archive/sm8250-6.19/0050* 2>/dev/null
cat patch/kernel/archive/sm8250-6.19/0050* 2>/dev/null | head -100Repository: armbian/build
Length of output: 1361
🏁 Script executed:
# Check if there's any kernel source in the repository we can reference
find . -path "*/drivers/bluetooth/hci_qca.c" 2>/dev/null
# Check if kernel source is available elsewhere in the tree
ls -la kernel/ 2>/dev/null | head -20Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Try to understand the context better - search for any documentation or comments
# about the qca6390-pmu and how it relates to the BT UART node
rg -n "qca6390-pmu\|qca6390-bt" patch/kernel/archive/sm8250-6.19/ -A 5 -B 5Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Get the full context of both patches to see the complete picture
cat patch/kernel/archive/sm8250-6.19/0050-arm64-dts-qcom-sm8250-xiaomi-elish-add-BT-swctrl-gpi.patchRepository: armbian/build
Length of output: 1215
🌐 Web query:
QCA6390 Bluetooth driver device tree power sequencing qca6390-pmu enable-gpios
💡 Result:
In mainline Linux DT, enable-gpios is a property of the Bluetooth UART node (qcom,*-bt) per qualcomm-bluetooth.yaml, but QCA6390 Bluetooth does not require enable-gpios in that binding (it instead requires a set of supply regulators). [1]
For QCA6390 power sequencing, the GPIOs that actually assert the chip’s internal enables live on the PMU node, using these properties from qcom,qca6390-pmu.yaml:
wlan-enable-gpios(WLAN_EN)bt-enable-gpios(BT_EN)- plus the PMU input supplies (
vddaon-supply,vddpmu-supply,vddrfa*,vddpcie*,vddio-supply, etc.) and the PMU’sregulators { ldoX { ... } }outputs. [2]
The kernel’s QCA6390 support was reworked to use the power sequencing (pwrseq) subsystem via a Qualcomm WCN PMU pwrseq provider, specifically to ensure the required ordering/delays between enabling rails and asserting BT_EN/WLAN_EN. [3][4]
Minimal sketch (showing where the “enable gpios” go for QCA6390):
pmu {
compatible = "qcom,qca6390-pmu";
/* PMU inputs from host */
vddaon-supply = <&...>;
vddpmu-supply = <&...>;
vddrfa0p95-supply = <&...>;
vddrfa1p3-supply = <&...>;
vddrfa1p9-supply = <&...>;
vddpcie1p3-supply = <&...>;
vddpcie1p9-supply = <&...>;
vddio-supply = <&...>;
/* These are the “enable gpios” for the package */
wlan-enable-gpios = <&tlmm X GPIO_ACTIVE_HIGH>;
bt-enable-gpios = <&tlmm Y GPIO_ACTIVE_HIGH>;
regulators { /* ldo0..ldo9 ... */ };
};
bluetooth {
compatible = "qcom,qca6390-bt";
/* consumes PMU outputs via *-supply phandles; no enable-gpios here */
vddaon-supply = <&...>;
vddrfacmn-supply = <&...>;
vddbtcmx-supply = <&...>;
vddrfa0p8-supply = <&...>;
vddrfa1p2-supply = <&...>;
vddrfa1p7-supply = <&...>;
};
If you’re seeing older discussions about “missing enable-gpio disables power management”, that was a driver-side behavior/bug around the transition to pwrseq usage for QCA6390. [4]
Sources: [1] qualcomm-bluetooth.yaml (kernel.org DT binding), [2] qcom,qca6390-pmu.yaml (kernel.org DT binding), [3] LWN summary of the power sequencing + QCA6390/WCN work, [4] upstream discussion/patch about QCA6390 + power sequencer vs enable-gpio.
Remove enable-gpios and swctrl-gpios from the BT UART node; these properties belong only on the qca6390-pmu node
According to the Qualcomm Bluetooth and QCA6390-PMU device tree bindings, QCA6390 Bluetooth does not use enable-gpios in the UART/BT node. Instead, the actual enable GPIOs for QCA6390 (bt-enable-gpios and swctrl-gpios) are managed by the qca6390-pmu device, which coordinates power sequencing via the kernel's power sequencing subsystem. The BT UART node consumes PMU outputs via *-supply phandles only, not GPIO properties.
Patch 0049 adds these lines:
+ enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
+ swctrl-gpios = <&tlmm 124 GPIO_ACTIVE_HIGH>;
These duplicate GPIO 21 (already defined as bt-enable-gpios in the PMU from patch 0050) and GPIO 124 (added to the PMU in patch 0050). Placing them here creates dead device tree data that will not be consumed by the driver and incorrectly suggests the UART driver independently manages these pins. Remove them from the BT UART node.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0049-arm64-dts-qcom-sm8250-xiaomi-elish-add-enable-and-sw.patch`
around lines 27 - 28, Remove the two GPIO properties from the Bluetooth UART
node: delete the enable-gpios and swctrl-gpios entries currently added to the BT
UART node (they duplicate GPIO 21 and 124 and are not consumed by the UART
driver); instead keep those GPIOs only on the qca6390-pmu node where
bt-enable-gpios and swctrl-gpios belong and are managed by the
PMU/power-sequencing subsystem so the UART node should rely on the PMU via
*-supply phandles only.
| + int rotation; | ||
| + int ret; | ||
| + | ||
| + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); | ||
| + if (!ctx) | ||
| + return -ENOMEM; | ||
| + | ||
| + ctx->display_mode = of_device_get_match_data(dev); | ||
| + | ||
| + ctx->supplies[0].supply = "vdd1v2"; | ||
| + ctx->supplies[1].supply = "vddio"; | ||
| + ctx->supplies[2].supply = "vdd"; | ||
| + ctx->supplies[3].supply = "avdd"; | ||
| + | ||
| + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies), | ||
| + ctx->supplies); | ||
| + if (ret < 0) | ||
| + return dev_err_probe(dev, ret, "Failed to get regulators\n"); | ||
| + | ||
| + ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); | ||
| + if (IS_ERR(ctx->reset_gpio)) | ||
| + return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio), | ||
| + "Failed to get reset-gpios\n"); | ||
| + | ||
| + 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 rotation is read after the -EINVAL guard — orientation will be silently corrupted.
When the DT node has no "rotation" property, of_property_read_u32 returns -EINVAL without writing to rotation. The if (ret == -EINVAL) block correctly sets DRM_MODE_PANEL_ORIENTATION_UNKNOWN, but there is no else or early-exit, so execution immediately falls through into the if (rotation == 0) chain, which reads the uninitialised stack variable. A zero-initialised stack slot (common) will silently overwrite UNKNOWN with NORMAL.
Secondary issues on the same code:
rotationis declared asintbut passed asu32 *toof_property_read_u32— strict aliasing / type violation.- No fallback for unrecognised rotation values (e.g. 45); orientation stays at the kzalloc-zero value which equals
DRM_MODE_PANEL_ORIENTATION_NORMAL, notUNKNOWN.
🐛 Proposed fix
- int rotation;
+ u32 rotation;
int ret;
...
ret = of_property_read_u32(dev->of_node, "rotation", &rotation);
- if (ret == -EINVAL) {
+ if (ret < 0) {
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;
+ } 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
+ 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.
| + int rotation; | |
| + int ret; | |
| + | |
| + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); | |
| + if (!ctx) | |
| + return -ENOMEM; | |
| + | |
| + ctx->display_mode = of_device_get_match_data(dev); | |
| + | |
| + ctx->supplies[0].supply = "vdd1v2"; | |
| + ctx->supplies[1].supply = "vddio"; | |
| + ctx->supplies[2].supply = "vdd"; | |
| + ctx->supplies[3].supply = "avdd"; | |
| + | |
| + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies), | |
| + ctx->supplies); | |
| + if (ret < 0) | |
| + return dev_err_probe(dev, ret, "Failed to get regulators\n"); | |
| + | |
| + ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); | |
| + if (IS_ERR(ctx->reset_gpio)) | |
| + return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio), | |
| + "Failed to get reset-gpios\n"); | |
| + | |
| + 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; | |
| u32 rotation; | |
| int ret; | |
| ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); | |
| if (!ctx) | |
| return -ENOMEM; | |
| ctx->display_mode = of_device_get_match_data(dev); | |
| ctx->supplies[0].supply = "vdd1v2"; | |
| ctx->supplies[1].supply = "vddio"; | |
| ctx->supplies[2].supply = "vdd"; | |
| ctx->supplies[3].supply = "avdd"; | |
| ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies), | |
| ctx->supplies); | |
| if (ret < 0) | |
| return dev_err_probe(dev, ret, "Failed to get regulators\n"); | |
| ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); | |
| if (IS_ERR(ctx->reset_gpio)) | |
| return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio), | |
| "Failed to get reset-gpios\n"); | |
| ret = of_property_read_u32(dev->of_node, "rotation", &rotation); | |
| if (ret < 0) { | |
| 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 | |
| ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0065-drm-panel-Add-DDIC-CH13726A-panel-Signed-off-by-Tegu.patch`
around lines 296 - 332, of_property_read_u32 currently writes into an
uninitialized int and fallsthroughs on -EINVAL; change the logic so you only
read/use the rotation value when of_property_read_u32 returns 0 and set
ctx->orientation to DRM_MODE_PANEL_ORIENTATION_UNKNOWN on any error or
unrecognized value. Concretely: make rotation the same u32 type expected by
of_property_read_u32 (or initialize it to an impossible sentinel), call
of_property_read_u32(dev->of_node, "rotation", &rotation) and if ret == 0 map
0/90/180/270 to the corresponding DRM_MODE_PANEL_ORIENTATION_* constants;
otherwise (ret != 0) set ctx->orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
also ensure any non-0/90/180/270 value falls back to UNKNOWN instead of leaving
the kzalloc-zeroed value.
| + 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.
Critical: GPIO error handling skips IS_ERR() — non-deferred errors are silently swallowed
PTR_ERR() must only be called after IS_ERR() confirms the pointer is an error. As written, if devm_gpiod_get returns any error other than -EPROBE_DEFER (e.g. -ENOENT when the GPIO is absent from DT), the condition is false, priv->sdb holds an error pointer, and gpiod_set_value_cansleep(priv->sdb, 1) at line 296 will crash.
🐛 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
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0066-leds-Add-driver-for-HEROIC-HTR3212-Signed-off-by-Teg.patch`
around lines 281 - 284, The devm_gpiod_get return is being inspected with
PTR_ERR without first checking IS_ERR, so non-EPROBE errors become hidden and
later gpiod_set_value_cansleep(priv->sdb, 1) can crash; change the block around
devm_gpiod_get to first check IS_ERR(priv->sdb), then if IS_ERR return
PTR_ERR(priv->sdb) except if PTR_ERR(priv->sdb) == -EPROBE_DEFER return
-EPROBE_DEFER (or otherwise propagate the original error), ensuring priv->sdb is
only used when it is a valid GPIO descriptor.
| +static size_t gamepad_mcu_uart_rx_bytes(struct serdev_device *serdev, | ||
| + const u8 *data, size_t count) | ||
| +{ | ||
| + struct device *dev = &serdev->dev; | ||
| + struct gamepad_data recv_data_buffer; | ||
| + u8 checksum; | ||
| + | ||
| + if (!data || count < 7) { | ||
| + dev_warn_ratelimited(dev, "invalid packet"); | ||
| + return count; | ||
| + } | ||
| + | ||
| + checksum = gamepad_data_checksum(data, count); | ||
| + if (checksum != *(data + count - 1)) { | ||
| + dev_warn_ratelimited(dev, "packet checksum failed"); | ||
| + return count; | ||
| + } | ||
| + | ||
| + 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); | ||
| + } | ||
| + | ||
| + gamepad_data_handler(serdev, &recv_data_buffer); | ||
| + | ||
| + return count; | ||
| +} |
There was a problem hiding this comment.
Buffer over-read and overflow in gamepad_mcu_uart_rx_bytes.
Two issues in the receive path:
-
Minimum size check is too small (Line 198):
count < 7allowscount == 7, but the parser accesses*(data + 7)at Line 212, which is a one-byte over-read. The minimum valid packet is 8 bytes of header + 1 checksum byte = 9. -
Unbounded
memcpyfrom attacker-controlleddata_len(Lines 217-218):recv_data_buffer.data_lenis parsed directly from the packet with no validation. If the received packet claimsdata_len > MAX_DATA_LEN(64), thememcpyoverflows the stack-allocatedrecv_data_buffer.data[MAX_DATA_LEN]. Similarly, ifdata_len > count - 9, it reads past the input buffer. This is a stack buffer overflow exploitable from the UART bus.
🐛 Proposed fix
static size_t gamepad_mcu_uart_rx_bytes(struct serdev_device *serdev,
const u8 *data, size_t count)
{
struct device *dev = &serdev->dev;
struct gamepad_data recv_data_buffer;
u8 checksum;
- if (!data || count < 7) {
+ if (!data || count < 9) {
dev_warn_ratelimited(dev, "invalid packet");
return count;
}
checksum = gamepad_data_checksum(data, count);
if (checksum != *(data + count - 1)) {
dev_warn_ratelimited(dev, "packet checksum failed");
return count;
}
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 > MAX_DATA_LEN ||
+ recv_data_buffer.data_len > count - 9) {
+ dev_warn_ratelimited(dev, "invalid data length: %u",
+ recv_data_buffer.data_len);
+ return count;
+ }
+
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);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0067-Input-driver-for-AYN-Odin2-Gamepad.patch`
around lines 191 - 224, The function gamepad_mcu_uart_rx_bytes currently
underchecks packet length and trusts recv_data_buffer.data_len from the input;
update it to require at least 9 bytes before parsing (enough for 8-byte header +
1 checksum), verify checksum over the correct range using gamepad_data_checksum,
then parse data_len and validate it against both MAX_DATA_LEN and the remaining
input (e.g., data_len <= MAX_DATA_LEN && data_len <= count - 9) before copying;
if validation fails, log and return without memcpy, and only then call
gamepad_data_handler with the bounded recv_data_buffer.
| +struct spmi_haptics { | ||
| + struct device *dev; | ||
| + struct regmap *regmap; | ||
| + struct input_dev *haptics_input_dev; | ||
| + struct gpio_desc *boost; | ||
| + struct work_struct work; | ||
| + u32 base; | ||
| + | ||
| + atomic_t active; | ||
| + | ||
| + int play_irq; | ||
| + int sc_irq; | ||
| + ktime_t last_sc_time; | ||
| + u8 sc_count; | ||
| + | ||
| + u8 actuator_type; | ||
| + u8 wave_shape; | ||
| + u8 play_mode; | ||
| + int magnitude; | ||
| + u32 vmax; | ||
| + u32 current_limit; | ||
| + u32 play_wave_rate; | ||
| + | ||
| + u32 wave_samp[HAP_WAVE_SAMP_SET_LEN]; | ||
| + u8 brake_pat[HAP_BRAKE_PAT_LEN]; | ||
| + | ||
| + struct mutex play_lock; | ||
| +}; |
There was a problem hiding this comment.
Missing mutex_init(&haptics->play_lock) — the mutex is never initialized.
play_lock is used in spmi_haptics_enable and spmi_haptics_disable but mutex_init() is never called. devm_kzalloc zero-initializes memory, but Linux mutexes require explicit initialization via mutex_init(). This will cause undefined behavior on first lock acquisition.
🐛 Proposed fix: add mutex_init in probe before first use
Add mutex_init(&haptics->play_lock); in spmi_haptics_probe, before the call to spmi_haptics_init:
+ mutex_init(&haptics->play_lock);
+
ret = spmi_haptics_init(haptics);
if (ret < 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0068-input-add-Qualcomm-SPMI-haptics-driver.patch`
around lines 229 - 256, The spmi_haptics struct's play_lock mutex is never
initialized; add a call to mutex_init(&haptics->play_lock) in spmi_haptics_probe
(immediately after allocating/setting up the struct instance) before any code
that may call spmi_haptics_init or spmi_haptics_enable/spmi_haptics_disable, so
the mutex is properly initialized prior to first use.
| + 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; | ||
| + } |
There was a problem hiding this comment.
register_fail cleanup operates on potentially uninitialized work and play_lock.
Early error paths (e.g., platform_get_irq_byname failures at lines 866–878) jump to register_fail, which calls cancel_work_sync(&haptics->work) and mutex_destroy(&haptics->play_lock). At that point, neither INIT_WORK (line 961) nor mutex_init (missing entirely) have been called.
🐛 Proposed fix: restructure error handling
Move INIT_WORK and mutex_init earlier in probe (before any goto register_fail), or use separate error labels:
haptics->dev = &pdev->dev;
+ mutex_init(&haptics->play_lock);
+ INIT_WORK(&haptics->work, spmi_haptics_work);
+
ret = of_property_read_u32(node, "reg", &haptics->base);And remove the duplicate INIT_WORK on line 961.
Also applies to: 987-991
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/sm8250-6.19/0068-input-add-Qualcomm-SPMI-haptics-driver.patch`
around lines 866 - 878, The error path jumps to register_fail after
platform_get_irq_byname failures while cancel_work_sync(&haptics->work) and
mutex_destroy(&haptics->play_lock) are called there even though
INIT_WORK(&haptics->work, ...) and initialization of haptics->play_lock have not
run yet; fix by moving INIT_WORK(&haptics->work, ...) and initializing the mutex
for haptics->play_lock (mutex_init or mutex_init(&haptics->play_lock)) earlier
in probe before any platform_get_irq_byname calls, remove the duplicate
INIT_WORK later (the one currently at the later init site), or alternatively
split the error path into two labels (e.g., irq_fail and register_fail) so only
initialized resources are cleaned up in register_fail and avoid calling
cancel_work_sync or mutex_destroy on uninitialized work/mutex.
OK |
2 participants
Description
The sm8250 edge kernel version is 6.19. Unlike the current kernel (6.18), it lacks optimization and extra feature patches for devices like the xiaomi-elish. This PR addresses these gaps by performing the following four tasks:
Copy kernel patches and configurations from the current kernel
Remove two kernel patches that have already been upstreamed
Fix the patch for sm8250 TDM support
Rwite edge kernel configuration
How Has This Been Tested?
./compile.sh kernel BOARD=xiaomi-elish BRANCH=edge RELEASE=nobleto ensure that the edge kernel compiles successfullySummary by CodeRabbit