Initial support for kernel 6.18 on Ayn odin2 #9286
Conversation
github-actions
bot
added
02
📝 WalkthroughWalkthroughThis PR updates the SM8550 kernel configuration from version 6.12 to 6.18, introduces multiple new hardware drivers (RSInput gamepad, Chipone ICNA3512 panel, HEROIC HTR3212 LED, Synaptics TD4328 panel), adds UART15 and comprehensive Ayn Odin2 device-tree support, and includes various driver enhancements for clock management, MMC frequency limits, USB initialization, and WiFi/Bluetooth MAC address derivation from SoC serial numbers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`:
- Around line 281-311: The RX path in rsinput_rx currently treats each serdev
callback as a full frame (copying into rx_buf, checking checksum, and dropping
on failure), which loses fragmented UART packets and causes spurious MCU resets;
change rsinput_rx to maintain a persistent per-device receive buffer (move
rx_buf to persist across callbacks inside struct rsinput_driver if not already),
append incoming data to that buffer instead of overwriting, implement a search
loop that locates frame headers and extracts complete frames (using
MCU_PKT_SIZE_MIN and frame length fields) before calling compute_checksum and
rsinput_process_data, and resynchronize by discarding bytes until a valid header
is found on checksum failures rather than reinitializing MCU immediately; also
remove the static prev_states in handle_cmd_status and make prev_states a member
of rsinput_driver so state is not shared across device instances.
- Around line 306-309: The receive_buf callback must not sleep; replace the
direct call to rsinput_init_commands(drv) in the branch where computed_checksum
!= received_checksum with scheduling a worker that runs rsinput_init_commands in
process context: add a struct work_struct reinit_work to struct rsinput_driver,
implement a static void rsinput_reinit_work(struct work_struct *work) that
container_ofs to struct rsinput_driver and calls rsinput_init_commands(drv),
initialize the work in probe with INIT_WORK(&drv->reinit_work,
rsinput_reinit_work), and in the receive_buf path call
schedule_work(&drv->reinit_work) (then keep the existing goto error flow).
- Around line 333-352: The driver stores potential error pointers in
drv->boot_gpio, drv->enable_gpio and drv->reset_gpio via devm_gpiod_get_optional
but only logs warnings, so later checks like if (drv->boot_gpio) can treat error
pointers as valid and gpiod_set_value_cansleep() will crash; change each of
those three acquisition blocks to return on error using
dev_err_probe(&serdev->dev, error, "Unable to get %s gpio\n", "<name>") (or
equivalent dev_err handling) so that if PTR_ERR(drv->*_gpio) indicates failure
you propagate the error (handling -EPROBE_DEFER properly) instead of continuing
and dereferencing the error pointer; apply the same pattern for boot, enable and
reset GPIO acquisitions referenced by devm_gpiod_get_optional and calls to
gpiod_set_value_cansleep().
In
`@patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`:
- Around line 286-301: The error paths after calling
mipi_dsi_picture_parameter_set(...) and mipi_dsi_compression_mode(...) currently
return without undoing power/reset state; modify those branches to perform the
same cleanup sequence used on successful shutdown/unprepare (disable regulators
and assert reset) before returning: invoke the existing power-off/unprepare
helper used elsewhere in this driver (e.g., the function that disables
regulators and asserts the panel reset — match the symbol used in this driver
such as icna3512_power_off or chipone_icna3512_unprepare) so both failure
branches cleanly disable supplies and assert reset, then return the error code.
In
`@patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch`:
- Around line 282-284: The devm_gpiod_get() return for priv->sdb is only checked
against -EPROBE_DEFER which leaves other ERR_PTR values in priv->sdb and can
cause gpiod_set_value_cansleep() to oops; change the handling in the probe path
that calls devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH) to check
IS_ERR(priv->sdb): if PTR_ERR(priv->sdb) == -EPROBE_DEFER return -EPROBE_DEFER,
otherwise log an error (including PTR_ERR(priv->sdb) and context) and return the
error code (PTR_ERR(priv->sdb)) so no invalid ERR_PTR is used later by
gpiod_set_value_cansleep() or related calls.
- Around line 292-309: After enabling the regulator with
regulator_enable(priv->vdd) you must disable it on subsequent probe failures to
avoid leaking the regulator; change the probe flow so failures in
htr3212_init_regs() or htr3212_parse_dt() jump to a cleanup label (e.g.
err_disable_reg) that calls regulator_disable(priv->vdd) before returning, or
explicitly call regulator_disable(priv->vdd) in those error return paths; update
the probe function around regulator_enable, htr3212_init_regs, and
htr3212_parse_dt to use this cleanup pattern (and remove early returns that
bypass regulator_disable).
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`:
- Around line 43-48: The PRIMARY_MI2S_RX branch currently calls
clk_prepare_enable(pdata->i2s_clk) and two snd_soc_dai_set_fmt() calls without
checking return values; add an int ret; at the top of the function, capture and
check ret = clk_prepare_enable(pdata->i2s_clk) and if negative log/return the
error, then call ret = snd_soc_dai_set_fmt(cpu_dai, fmt) and check/handle error
(undo clk with clk_disable_unprepare(pdata->i2s_clk) if needed) before calling
ret = snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt) and handle that error
similarly so failures are propagated and resources cleaned up.
In
`@patch/kernel/archive/sm8550-6.18/0007-mmc-sdhci-msm-Limit-HS-mode-frequency-to-37.5MHz.patch`:
- Around line 44-56: The OPP table used by sdhci on level‑shifter devices lacks
a 37.5 MHz entry so dev_pm_opp_set_rate(desired_rate) will fail when
msm_host->uses_level_shifter forces desired_rate to
LEVEL_SHIFTER_HIGH_SPEED_FREQ; add a new opp entry for 37.5 MHz to the
sdhc2_opp_table (the table added/modified in the patch that defines
qcom,use-level-shifter) including the correct opp-hz for 37,500,000 Hz and
sensible opp-peak-kBps / opp-avg-kBps values so
dev_pm_opp_set_rate(mmc_dev(host->mmc), LEVEL_SHIFTER_HIGH_SPEED_FREQ) succeeds
for devices that set qcom,use-level-shifter.
In `@patch/kernel/archive/sm8550-6.18/0014-HACK-fix-usb-boot-hang.patch`:
- Around line 19-30: The current change removes the CNR readiness handshake
(handshake(..., XHCI_STS_CNR, ...)) and global dev_warn fallback, which can
break non-Odin2 controllers; restore the handshake and warning for all
controllers except the Odin2/SM8550 quirk: detect the affected HW by checking
the PCI device identity or an existing quirk flag on pdev (e.g., match PCI ID or
set/inspect a quirk in probe), and only skip the handshake and dev_warn when
that quirk/PCI ID is present; keep using op_reg_base, XHCI_STS_OFFSET,
XHCI_STS_CNR, the timeout variable, and dev_warn(&pdev->dev, ...) for the normal
path so other controllers retain the original behavior and diagnostics.
In
`@patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`:
- Around line 108-141: The patch removes the previous NVM/firmware fallback and
hard-fails when generate_bdaddr_from_serial or qca_set_bdaddr fail; restore a
fallback to the original qca_check_bdaddr path instead of returning immediately
so BT can still use NVM BD_ADDR. Change qca_uart_setup to: call
generate_bdaddr_from_serial and, on error, log a warning and call
qca_check_bdaddr(&config) as the fallback; if generate succeeds but
qca_set_bdaddr fails, log the failure and fall back to qca_check_bdaddr(&config)
as well; only return an error if both the serial-derived path and
qca_check_bdaddr both fail. Ensure you still update
hdev->public_addr/hdev->bdaddr only on successful qca_set_bdaddr, and keep the
bt_dev_info log when the BD_ADDR is successfully set.
- Around line 245-268: generate_macaddr_from_serial is being used but its
success path and the per‑pdev uniqueness logic are inverted and missing offset:
if generate_macaddr_from_serial succeeds you must use the generated_macaddr
(call ath12k_reverse_mac into ar->mac_addr, log via ath12k_info) and add
ar->pdev_idx to the appropriate byte of generated_macaddr to avoid duplicate
MACs across pdevs; if generate_macaddr_from_serial fails, fall back to the
existing behavior that prefers ar->ab->pdevs_macaddr_valid and copies
pdev->mac_addr via ether_addr_copy, otherwise copy ar->ab->mac_addr and add
ar->pdev_idx as the current fallback does. Ensure you reference
generate_macaddr_from_serial, generated_macaddr, ath12k_reverse_mac,
ath12k_info, ether_addr_copy, ar->ab->pdevs_macaddr_valid and ar->pdev_idx when
making the change.
- Around line 20-90: Remove the unused static_bdaddr declaration and add the
missing include for <linux/ctype.h> in the source that defines
generate_bdaddr_from_serial (place the include immediately after the existing
`#include` "btqca.h") so isxdigit() resolves, and make the same include addition
in the other source that calls isxdigit (the ath12k/mac.c usage); ensure you
only remove the static const bdaddr_t static_bdaddr = { ... } symbol and do not
change the generate_bdaddr_from_serial logic or its bt_dev_err/bt_dev_info
calls.
In
`@patch/kernel/archive/sm8550-6.18/0017-Disable-sdhci_pltfm_free-calling.patch`:
- Around line 15-21: The sdhci_pltfm_free(pdev) call was commented out, causing
resource leaks; restore cleanup but guard it so it only runs when platform data
was actually initialized to avoid double-free. In sdhci_msm_probe and the
corresponding sdhci_msm_remove, add a boolean or check a pointer/flag stored on
the platform_device (e.g., msm_host or a ->pdata_initialized flag) when
allocation/setup succeeds, set it to true, and only call sdhci_pltfm_free(pdev)
when that flag/pointer indicates initialization; ensure the flag is cleared
before early returns and on remove to keep teardown symmetric and safe.
In
`@patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`:
- Around line 1580-1694: The firmware paths in the odin2 DTS overlay are
incorrectly pointing to "qcom/sm8550/ayn/odin2portal/"; update the six
firmware-name properties to the correct "qcom/sm8550/ayn/odin2/" path (change
entries in the &gpu zap-shader node, &remoteproc_adsp node, &remoteproc_cdsp
node, and both &spk_amp_l and &spk_amp_r nodes), or alternatively refactor to a
shared firmware path/label and reference that from both odin2 and odin2portal so
the Device Tree doesn't mix Portal-specific paths into the base odin2 device.
🟡 Minor comments (7)
patch/kernel/archive/sm8550-6.18/0019-HACK-Revert-back-to-6.12-aw88166-driver.patch-36-39 (1)
36-39: Typo in error message: "32r-bit" should be "32-bit".Proposed fix
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", (u32)dsp_addr, *dsp_data);patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch-218-223 (1)
218-223: Verify DRM panel callback assignment:.disablevs.unprepare.The
td4328_unpreparefunction (which performs power-off with regulator disable) is assigned to.disableinstead of the semantically correct.unpreparecallback. The struct leaves.unprepareas NULL. While the functions themselves are correctly implemented for power sequencing, this pairing is unconventional—the DRM framework typically expects.prepare/.unpreparefor power control. The code is auto-generated and marked WIP, but the assignment should use.unpreparefor semantic correctness and to ensure the framework invokes the power-down sequence as designed.Suggested fix
static const struct drm_panel_funcs td4328_panel_funcs = { .prepare = td4328_prepare, - .disable = td4328_unprepare, + .unprepare = td4328_unprepare, .get_modes = td4328_get_modes, .get_orientation = td4328_get_orientation, };Confirm this pattern works as intended on actual hardware, particularly during suspend/resume cycles.
patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch-17-28 (1)
17-28: Missing Makefile entry for odin2portal DTB.The patch creates
qcs8550-ayn-odin2portal.dts(line 15) but only addsqcs8550-ayn-odin2.dtbto the Makefile. The Portal variant won't be built.Proposed fix
dtb-$(CONFIG_ARCH_QCOM) += qcs8550-aim300-aiot.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs8550-ayn-odin2.dtb +dtb-$(CONFIG_ARCH_QCOM) += qcs8550-ayn-odin2portal.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs9100-ride.dtbpatch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch-348-348 (1)
348-348: Missing closing angle bracket in MODULE_AUTHOR.Typo:
<teguh@sobir.in"should be<teguh@sobir.in>".Proposed fix
-MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); +MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>");patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch-197-198 (1)
197-198: Readalways-onproperty from the child LED node, not parent device
device_property_read_bool(dev, "always-on")reads from the parent device node, but this function parses individual LED child nodes. Thealways-onproperty should be per-LED and read from thechildnode instead. Other per-LED properties likeregare correctly read fromchildon line 137.Proposed fix
- if (!device_property_read_bool(dev, "always-on")) + if (!device_property_read_bool(&of_node_to_device(child), "always-on")) cdev->flags |= LED_CORE_SUSPENDRESUME;Or more idiomatically:
- if (!device_property_read_bool(dev, "always-on")) + if (!of_property_read_bool(child, "always-on")) cdev->flags |= LED_CORE_SUSPENDRESUME;patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch-22-24 (1)
22-24: Tighten the Kconfig prompt wording.The prompt is user‑facing; tweak grammar for clarity.
💬 Suggested wording
-config JOYSTICK_RSINPUT - tristate "UART Based gamepad driver that found in AYN and Retroid Pocket products" +config JOYSTICK_RSINPUT + tristate "UART-based gamepad driver found in AYN and Retroid Pocket products"patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch-210-237 (1)
210-237: Make button state per‑device instead of static.
static unsigned long prev_statesis shared across all devices; multiple RSInput pads will corrupt each other’s state. Store it instruct rsinput_driver.🧩 Per-device state
struct rsinput_driver { struct serdev_device *serdev; struct input_dev *input; struct gpio_desc *boot_gpio; struct gpio_desc *enable_gpio; struct gpio_desc *reset_gpio; uint8_t rx_buf[256]; uint8_t sequence_number; + unsigned long prev_states; }; - static unsigned long prev_states; - unsigned long keys = data[FRAME_POS_DATA_1] | (data[FRAME_POS_DATA_2] << 8); + unsigned long keys = data[FRAME_POS_DATA_1] | (data[FRAME_POS_DATA_2] << 8); unsigned long current_states = keys, changes; @@ - bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap)); + bitmap_xor(&changes, ¤t_states, &drv->prev_states, ARRAY_SIZE(keymap)); @@ - prev_states = keys; + drv->prev_states = keys;
🧹 Nitpick comments (12)
patch/kernel/archive/sm8550-6.18/0019-HACK-Revert-back-to-6.12-aw88166-driver.patch (1)
370-374: Minor style: Missing space before{.Linux kernel coding style requires a space before the opening brace.
Proposed fix
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK){ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { dev_dbg(dai->dev, "STREAM_PLAYBACK stop");patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch (3)
143-176: Kernel coding style violations in init sequence.Several issues that would likely be flagged by checkpatch:
- Line 163:
else {should be on same line as closing brace:} else {- Mixed indentation throughout - kernel style requires tabs, but many lines use spaces (e.g., lines 152, 155-161, 164-175, 178-182, 190-192)
- Line 145: Unnecessary blank line after opening brace
Example fix for else placement
- } - else { + } else {
432-443: Initial backlight brightness set to maximum.The backlight is initialized with
brightness = max_brightness = 4096. This means the display will be at full brightness on boot. If this is intentional for the Odin2 device, it's fine, but typically panels start at a lower default.
489-501: Coding style issues in probe function.
- Line 497: Missing space before brace:
if (ret < 0){should beif (ret < 0) {- Lines 491-494, 496, 500: Use spaces instead of tabs for indentation
Style fix
- ret = mipi_dsi_attach(pinfo->dsi); - if (ret < 0){ + ret = mipi_dsi_attach(pinfo->dsi); + if (ret < 0) { dev_err_probe(dev, ret, "Failed to attach to DSI host\n"); drm_panel_remove(&pinfo->panel); - return ret; - } + return ret; + }patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch (1)
107-109: Consider checking return values for vendor-specific DSI commands.The
mipi_dsi_dcs_write_seqcalls don't check return values. While not critical for auto-generated panel drivers, checking could help diagnose initialization issues.patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (3)
244-358: Commented-out code in thermal zone definitions.Multiple trip points and cooling maps are commented out (lines 247-263, 307-317, 349-356). If these are unused, consider removing them to keep the DT clean. If they're reference values for future tuning, a brief comment explaining their purpose would help maintainability.
1696-1704: Inconsistent indentation in soc node.Lines 1697-1703 use 4 spaces instead of tabs for indentation, which is inconsistent with the rest of the device tree. Similarly, line 1574 (
backlight = <&backlight>;) appears to have space indentation.Proposed fix for soc node
&soc { - qcom_tzlog: qcom_tzlog { - status = "disabled"; - }; + qcom_tzlog: qcom_tzlog { + status = "disabled"; + }; - arch_timer: arch_timer { - status = "disabled"; - }; + arch_timer: arch_timer { + status = "disabled"; + }; };
1862-1876: Potentially confusing LED node naming convention.The node name
ledl_b1suggests "blue" but hascolor = <LED_COLOR_ID_RED>andlabel = "l:r1". This pattern continues withledl_g1having green andledl_r1having blue. If the node names follow a hardware register mapping rather than color semantics, consider adding a brief comment to prevent confusion during maintenance.patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch (2)
22-30: UnnecessaryREGMAP_I2Cselection.The Kconfig selects
REGMAP_I2C, but the driver usesi2c_smbus_write_byte_data()directly instead of the regmap API. Consider removing theselect REGMAP_I2Cline, or refactoring the driver to use regmap for consistency.
132-141: Minor simplification possible.The explicit
return 0after the error check is redundant sinceretwould be 0 on success.Optional 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); }patch/kernel/archive/sm8550-6.18/0006-dt-bindings-mmc-qcom-Document-level-shifter-flag-for.patch (1)
31-33: Makeqcom,use-level-shifterexplicitly boolean in the schema.Without a type, dt-schema will accept non-boolean values. Tightening the schema avoids invalid DTS values slipping through.
♻️ Proposed schema tightening
qcom,use-level-shifter: + type: boolean description: Flag to indicate if platform has level shifter for SD card.patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (1)
153-163: Remove unused static_macaddr
static_macaddrisn’t referenced and may trigger unused‑const warnings under W=1. Drop it or wire it as a fallback default.🧹 Remove unused static_macaddr
-/* Define a static, predefined MAC_ADDR structure */ -static const struct macaddr_t static_macaddr = { - .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } -};
| +static size_t rsinput_rx(struct serdev_device *serdev, const u8 *data, size_t count) { | ||
| + struct rsinput_driver *drv = serdev_device_get_drvdata(serdev); | ||
| + uint8_t received_checksum, computed_checksum; | ||
| + | ||
| + if (!drv || !data || count == 0) { | ||
| + dev_warn_ratelimited(&serdev->dev, "Invalid RX data\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + if (count > sizeof(drv->rx_buf)) { | ||
| + dev_warn_ratelimited(&serdev->dev, "RX buffer overflow\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + memcpy(drv->rx_buf, data, count); | ||
| + | ||
| + if (count < MCU_PKT_SIZE_MIN) { | ||
| + dev_warn_ratelimited(&serdev->dev, "Frame too short for checksum validation\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + received_checksum = drv->rx_buf[count - 1]; | ||
| + | ||
| + computed_checksum = compute_checksum(drv->rx_buf, count); | ||
| + | ||
| + if (computed_checksum != received_checksum) { | ||
| + rsinput_init_commands(drv); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + rsinput_process_data(drv, drv->rx_buf, count); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files "*rsinput*" "*0002*" 2>/dev/null | head -20Repository: armbian/build
Length of output: 170
🏁 Script executed:
fd -type f "0002-input-Add-driver-for-RSInput-Gamepad.patch" 2>/dev/nullRepository: armbian/build
Length of output: 39
🏁 Script executed:
find . -type f -name "*0002*" 2>/dev/nullRepository: armbian/build
Length of output: 9545
🏁 Script executed:
cat ./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 15253
RX path assumes full frames arrive in a single callback, causing data loss on fragmented UART packets.
UART commonly fragments data across multiple receive callbacks. Current logic directly copies each callback into rx_buf, validates it as a complete frame, and drops it on checksum failure—overwriting any partial frame on the next callback. This causes loss of fragmented packets and spurious MCU reinitializations.
Implement persistent frame buffering: accumulate incoming data in a ring or linear buffer, search for frame headers, and assemble complete frames before validation. Handle resynchronization when stream alignment is lost.
Additionally, prev_states in handle_cmd_status() (line 168) is declared static, causing state leakage across multiple device instances. Make it per-device by moving it to the rsinput_driver struct.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 281 - 311, The RX path in rsinput_rx currently treats each serdev
callback as a full frame (copying into rx_buf, checking checksum, and dropping
on failure), which loses fragmented UART packets and causes spurious MCU resets;
change rsinput_rx to maintain a persistent per-device receive buffer (move
rx_buf to persist across callbacks inside struct rsinput_driver if not already),
append incoming data to that buffer instead of overwriting, implement a search
loop that locates frame headers and extracts complete frames (using
MCU_PKT_SIZE_MIN and frame length fields) before calling compute_checksum and
rsinput_process_data, and resynchronize by discarding bytes until a valid header
is found on checksum failures rather than reinitializing MCU immediately; also
remove the static prev_states in handle_cmd_status and make prev_states a member
of rsinput_driver so state is not shared across device instances.
| + if (computed_checksum != received_checksum) { | ||
| + rsinput_init_commands(drv); | ||
| + goto error; | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the patch file and examine its structure
find . -name "*RSInput*" -o -name "*0002-input*" | head -20Repository: armbian/build
Length of output: 450
🏁 Script executed:
# Let's search for the patch file in the archive directory
fd -t f "0002-input-Add-driver-for-RSInput" patch/Repository: armbian/build
Length of output: 137
🏁 Script executed:
# Look for the specific patch file
find . -path "*/patch/kernel/archive/sm8550-6.18/0002-input*" -type fRepository: armbian/build
Length of output: 139
🏁 Script executed:
# List files in that directory
ls -la patch/kernel/archive/sm8550-6.18/ 2>/dev/null | head -20Repository: armbian/build
Length of output: 1967
🏁 Script executed:
cat -n patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | head -350 | tail -80Repository: armbian/build
Length of output: 2911
🏁 Script executed:
# Get the full patch to understand the context better
wc -l patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 141
🏁 Script executed:
# Let's see the area around lines 306-309 with more context
sed -n '290,330p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 1102
🏁 Script executed:
# Search for rsinput_rx function definition
sed -n '1,400p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | grep -n "rsinput_rx\|receive_buf"Repository: armbian/build
Length of output: 246
🏁 Script executed:
# Get broader context of the receive callback function
sed -n '280,350p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 2164
🏁 Script executed:
# Search for rsinput_init_commands implementation in the patch
sed -n '1,467p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | grep -n "rsinput_init_commands"Repository: armbian/build
Length of output: 210
🏁 Script executed:
# Get the full implementation of rsinput_init_commands
sed -n '1,467p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | sed -n '/^+static.*rsinput_init_commands/,/^+^}/p' | head -50Repository: armbian/build
Length of output: 1981
🏁 Script executed:
# Search more broadly for the function
grep -n "rsinput_init_commands" patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 210
🏁 Script executed:
# Get the function definition around those line numbers
sed -n '100,200p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 3303
Don't sleep inside the serdev receive callback.
rsinput_init_commands() contains msleep(100) calls (at lines 160 and 168) and should not run in receive_buf context. Defer re‑init to a workqueue or threaded context.
Suggested fix
- if (computed_checksum != received_checksum) {
- rsinput_init_commands(drv);
- goto error;
- }
+ if (computed_checksum != received_checksum) {
+ schedule_work(&drv->reinit_work);
+ goto error;
+ }Additional changes needed:
/* in struct rsinput_driver */
struct work_struct reinit_work;
/* in probe */
INIT_WORK(&drv->reinit_work, rsinput_reinit_work);
/* worker */
static void rsinput_reinit_work(struct work_struct *work)
{
struct rsinput_driver *drv = container_of(work, struct rsinput_driver, reinit_work);
rsinput_init_commands(drv);
}🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 306 - 309, The receive_buf callback must not sleep; replace the
direct call to rsinput_init_commands(drv) in the branch where computed_checksum
!= received_checksum with scheduling a worker that runs rsinput_init_commands in
process context: add a struct work_struct reinit_work to struct rsinput_driver,
implement a static void rsinput_reinit_work(struct work_struct *work) that
container_ofs to struct rsinput_driver and calls rsinput_init_commands(drv),
initialize the work in probe with INIT_WORK(&drv->reinit_work,
rsinput_reinit_work), and in the receive_buf path call
schedule_work(&drv->reinit_work) (then keep the existing goto error flow).
| + drv->boot_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->boot_gpio)) { | ||
| + error = PTR_ERR(drv->boot_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); | ||
| + } | ||
| + | ||
| + drv->enable_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->enable_gpio)) { | ||
| + error = PTR_ERR(drv->enable_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get enable gpio: %d\n", error); | ||
| + } | ||
| + | ||
| + drv->reset_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->reset_gpio)) { | ||
| + error = PTR_ERR(drv->reset_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get reset gpio: %d\n", error); | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "0002-input-Add-driver-for-RSInput-Gamepad.patch" -type fRepository: armbian/build
Length of output: 139
🏁 Script executed:
cat -n ./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | head -360Repository: armbian/build
Length of output: 14562
🏁 Script executed:
cat -n ./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | sed -n '353,418p'Repository: armbian/build
Length of output: 2706
🏁 Script executed:
# Search for gpiod_get_optional usage patterns in Linux kernel source
cd /tmp && git clone --depth 1 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2>/dev/null || true
if [ -d linux ]; then
cd linux
# Find examples of proper error handling with gpiod_get_optional
grep -r "devm_gpiod_get_optional" --include="*.c" -A 5 | head -100
fiRepository: armbian/build
Length of output: 5495
🏁 Script executed:
cat > /tmp/test_error_ptr.c << 'EOF'
`#include` <stdio.h>
`#include` <stdint.h>
// Simulate Linux kernel error pointer encoding
`#define` IS_ERR(ptr) (((unsigned long)ptr) >= (unsigned long)-4095)
`#define` PTR_ERR(ptr) ((long)ptr)
int main() {
// Simulate an error pointer
void *error_ptr = (void *)-ENOENT; // -2
// Check if error pointer is non-NULL in boolean context
if (error_ptr) {
printf("Error pointer evaluates to TRUE in boolean context\n");
} else {
printf("Error pointer evaluates to FALSE in boolean context\n");
}
// Check if it's an error
if (IS_ERR(error_ptr)) {
printf("IS_ERR() returns true\n");
printf("Error code: %ld\n", PTR_ERR(error_ptr));
}
return 0;
}
EOF
gcc /tmp/test_error_ptr.c -o /tmp/test_error_ptr && /tmp/test_error_ptrRepository: armbian/build
Length of output: 101
🏁 Script executed:
# Check if there are other dereferences of these GPIO pointers besides gpiod_set_value_cansleep
grep -n "boot_gpio\|enable_gpio\|reset_gpio" ./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | tail -20Repository: armbian/build
Length of output: 908
Handle optional GPIO errors by returning instead of continuing.
Error pointers are stored in drv->boot_gpio, drv->enable_gpio, and drv->reset_gpio but only logged with warnings. Later, when these fields are tested with if (drv->*_gpio), error pointers evaluate as true and get dereferenced by gpiod_set_value_cansleep(), causing crashes. Return on error using dev_err_probe() to properly handle -EPROBE_DEFER and other failures, matching the kernel's standard pattern for optional GPIO functions.
🛠️ Safer error handling
- drv->boot_gpio =
- devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH);
- if (IS_ERR(drv->boot_gpio)) {
- error = PTR_ERR(drv->boot_gpio);
- dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error);
- }
+ drv->boot_gpio =
+ devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH);
+ if (IS_ERR(drv->boot_gpio)) {
+ error = PTR_ERR(drv->boot_gpio);
+ return dev_err_probe(&serdev->dev, error, "Unable to get boot gpio\n");
+ }(Apply the same pattern for enable and reset GPIOs.)
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 333 - 352, The driver stores potential error pointers in
drv->boot_gpio, drv->enable_gpio and drv->reset_gpio via devm_gpiod_get_optional
but only logs warnings, so later checks like if (drv->boot_gpio) can treat error
pointers as valid and gpiod_set_value_cansleep() will crash; change each of
those three acquisition blocks to return on error using
dev_err_probe(&serdev->dev, error, "Unable to get %s gpio\n", "<name>") (or
equivalent dev_err handling) so that if PTR_ERR(drv->*_gpio) indicates failure
you propagate the error (handling -EPROBE_DEFER properly) instead of continuing
and dereferencing the error pointer; apply the same pattern for boot, enable and
reset GPIO acquisitions referenced by devm_gpiod_get_optional and calls to
gpiod_set_value_cansleep().
| + | ||
| + ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | ||
| + if (ret < 0) { | ||
| + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + /* Not required, ICNA3512 has DSC always enabled. */ | ||
| + ret = mipi_dsi_compression_mode(pinfo->dsi, true); | ||
| + if (ret < 0) { | ||
| + dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + return 0; | ||
| +} |
There was a problem hiding this comment.
Missing error cleanup after PPS and compression mode failures.
If mipi_dsi_picture_parameter_set() or mipi_dsi_compression_mode() fails, the function returns without disabling regulators or asserting reset. This leaves hardware in an inconsistent state and leaks power.
Proposed fix: add cleanup on error paths
ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps);
if (ret < 0) {
dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
- return ret;
+ goto err_disable;
}
/* Not required, ICNA3512 has DSC always enabled. */
ret = mipi_dsi_compression_mode(pinfo->dsi, true);
if (ret < 0) {
dev_err(panel->dev, "failed to enable compression mode: %d\n", ret);
- return ret;
+ goto err_disable;
}
return 0;
+
+err_disable:
+ gpiod_set_value_cansleep(pinfo->reset_gpio, 1);
+ regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies);
+ 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 = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | |
| + if (ret < 0) { | |
| + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | |
| + return ret; | |
| + } | |
| + | |
| + /* Not required, ICNA3512 has DSC always enabled. */ | |
| + ret = mipi_dsi_compression_mode(pinfo->dsi, true); | |
| + if (ret < 0) { | |
| + dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | |
| + return ret; | |
| + } | |
| + | |
| + return 0; | |
| +} | |
| ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | |
| if (ret < 0) { | |
| dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | |
| goto err_disable; | |
| } | |
| /* Not required, ICNA3512 has DSC always enabled. */ | |
| ret = mipi_dsi_compression_mode(pinfo->dsi, true); | |
| if (ret < 0) { | |
| dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | |
| goto err_disable; | |
| } | |
| return 0; | |
| err_disable: | |
| gpiod_set_value_cansleep(pinfo->reset_gpio, 1); | |
| regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`
around lines 286 - 301, The error paths after calling
mipi_dsi_picture_parameter_set(...) and mipi_dsi_compression_mode(...) currently
return without undoing power/reset state; modify those branches to perform the
same cleanup sequence used on successful shutdown/unprepare (disable regulators
and assert reset) before returning: invoke the existing power-off/unprepare
helper used elsewhere in this driver (e.g., the function that disables
regulators and asserts the panel reset — match the symbol used in this driver
such as icna3512_power_off or chipone_icna3512_unprepare) so both failure
branches cleanly disable supplies and assert reset, then return the error code.
| + 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 may cause kernel oops.
Only -EPROBE_DEFER is checked, but other errors from devm_gpiod_get() would leave an ERR_PTR in priv->sdb. Subsequent gpiod_set_value_cansleep() calls with this invalid pointer can cause undefined behavior.
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/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch`
around lines 282 - 284, The devm_gpiod_get() return for priv->sdb is only
checked against -EPROBE_DEFER which leaves other ERR_PTR values in priv->sdb and
can cause gpiod_set_value_cansleep() to oops; change the handling in the probe
path that calls devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH) to check
IS_ERR(priv->sdb): if PTR_ERR(priv->sdb) == -EPROBE_DEFER return -EPROBE_DEFER,
otherwise log an error (including PTR_ERR(priv->sdb) and context) and return the
error code (PTR_ERR(priv->sdb)) so no invalid ERR_PTR is used later by
gpiod_set_value_cansleep() or related calls.
| +extern const char *qcom_serial_number; | ||
| + | ||
| +/* Define a static, predefined BD_ADDR structure */ | ||
| +static const bdaddr_t static_bdaddr = { | ||
| + .b = { 0x00, 0x03, 0x7F, 0x33, 0x22, 0x11 } | ||
| +}; | ||
| + | ||
| +/** | ||
| + * generate_bdaddr_from_serial - Generates a BD_ADDR using the serial number | ||
| + * @bdaddr: Pointer to bdaddr_t structure to populate | ||
| + * | ||
| + * This function sets the first 3 bytes to 00:03:7F and the last 3 bytes | ||
| + * are derived from the last 6 characters of qcom_serial_number in reversed order. | ||
| + * | ||
| + * Returns 0 on success, negative error code on failure. | ||
| + */ | ||
| +static int generate_bdaddr_from_serial(struct hci_dev *hdev, bdaddr_t *bdaddr) | ||
| +{ | ||
| + size_t serial_len; | ||
| + const char *serial = qcom_serial_number; | ||
| + char last6[7] = {0}; // 6 characters + null terminator | ||
| + int i; | ||
| + int ret; | ||
| + | ||
| + if (!serial) { | ||
| + bt_dev_err(hdev, "qcom_serial_number is NULL"); | ||
| + return -EINVAL; | ||
| + } | ||
| + | ||
| + serial_len = strlen(serial); | ||
| + if (serial_len < 6) { | ||
| + bt_dev_err(hdev, "qcom_serial_number is too short: %zu characters", serial_len); | ||
| + return -EINVAL; | ||
| + } | ||
| + | ||
| + // Extract the last 6 characters | ||
| + strncpy(last6, serial + serial_len - 6, 6); | ||
| + | ||
| + // Initialize the first 3 bytes | ||
| + bdaddr->b[5] = 0x00; | ||
| + bdaddr->b[4] = 0x03; | ||
| + bdaddr->b[3] = 0x7F; | ||
| + | ||
| + // Convert the last 6 characters into 3 bytes in reversed order | ||
| + for (i = 0; i < 3; i++) { | ||
| + char byte_str[3] = {0}; | ||
| + u8 byte_val; | ||
| + | ||
| + byte_str[0] = last6[i * 2]; | ||
| + byte_str[1] = last6[i * 2 + 1]; | ||
| + | ||
| + if (!isxdigit(byte_str[0]) || !isxdigit(byte_str[1])) { | ||
| + bt_dev_err(hdev, "Invalid hex characters in serial number: %c%c", | ||
| + byte_str[0], byte_str[1]); | ||
| + return -EINVAL; | ||
| + } | ||
| + | ||
| + ret = kstrtou8(byte_str, 16, &byte_val); | ||
| + if (ret < 0) { | ||
| + bt_dev_err(hdev, "Failed to convert hex string to u8: %c%c", | ||
| + byte_str[0], byte_str[1]); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + bdaddr->b[i] = byte_val; // Assign to bytes 0,1,2 (reversed order) | ||
| + } | ||
| + | ||
| + bt_dev_info(hdev, "Generated BD_ADDR: %pMR", bdaddr); | ||
| + | ||
| + return 0; | ||
| +} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the patch file exists and its structure
echo "=== Checking patch file existence ==="
ls -lh patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch
echo ""
echo "=== Searching for isxdigit usage and ctype include in patch ==="
# Search in the patch file itself
grep -n "isxdigit" patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch || echo "No isxdigit found in patch"
grep -n "linux/ctype.h" patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch || echo "No linux/ctype.h include found in patch"
echo ""
echo "=== Searching for static_bdaddr usage in patch ==="
grep -n "static_bdaddr" patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch || echo "No static_bdaddr usage found"
echo ""
echo "=== Checking actual driver files ==="
ls -lh drivers/bluetooth/btqca.c drivers/net/wireless/ath/ath12k/mac.c 2>/dev/null || echo "Driver files not found in current state"Repository: armbian/build
Length of output: 637
🏁 Script executed:
#!/bin/bash
echo "=== Checking includes added by patch in btqca.c section ==="
# Look for include directives in the patch
grep -n "^+#include" patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch
echo ""
echo "=== Full static_bdaddr references (all occurrences) ==="
grep -n "static_bdaddr" patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch
echo ""
echo "=== Context around line 23 where static_bdaddr is defined ==="
sed -n '20,30p' patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch
echo ""
echo "=== Extract btqca.c section header to find includes ==="
sed -n '1,100p' patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch | grep -A 20 "btqca.c"Repository: armbian/build
Length of output: 1676
🏁 Script executed:
#!/bin/bash
echo "=== Check full patch to see all added includes ==="
grep -E "^\+" patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch | grep "#include" | head -20
echo ""
echo "=== Extract larger section of patch to see include context ==="
sed -n '1,150p' patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patchRepository: armbian/build
Length of output: 4795
Add missing <linux/ctype.h> include and remove unused static_bdaddr
The patch uses isxdigit() (lines 71, 209 in btqca.c and ath12k/mac.c) but does not include <linux/ctype.h>. This will cause a build failure. Additionally, static_bdaddr is defined but never used—remove it.
Changes needed
Add after #include "btqca.h":
`#include` "btqca.h"
+#include <linux/ctype.h>Remove the unused structure:
-/* Define a static, predefined BD_ADDR structure */
-static const bdaddr_t static_bdaddr = {
- .b = { 0x00, 0x03, 0x7F, 0x33, 0x22, 0x11 }
-};Apply the same ctype.h include to ath12k/mac.c.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 20 - 90, Remove the unused static_bdaddr declaration and add the
missing include for <linux/ctype.h> in the source that defines
generate_bdaddr_from_serial (place the include immediately after the existing
`#include` "btqca.h") so isxdigit() resolves, and make the same include addition
in the other source that calls isxdigit (the ath12k/mac.c usage); ensure you
only remove the static const bdaddr_t static_bdaddr = { ... } symbol and do not
change the generate_bdaddr_from_serial logic or its bt_dev_err/bt_dev_info
calls.
| + bdaddr_t generated_bdaddr; | ||
|
|
||
| bt_dev_dbg(hdev, "QCA setup on UART"); | ||
|
|
||
| @@ -993,9 +1066,30 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, | ||
| break; | ||
| } | ||
|
|
||
| - err = qca_check_bdaddr(hdev, &config); | ||
| - if (err) | ||
| + /* Generate BD_ADDR from qcom_serial_number */ | ||
| + err = generate_bdaddr_from_serial(hdev, &generated_bdaddr); | ||
| + if (err) { | ||
| + bt_dev_err(hdev, "Failed to generate BD_ADDR from serial number"); | ||
| + return err; | ||
| + } | ||
| + | ||
| + /* Set the generated BD_ADDR */ | ||
| + err = qca_set_bdaddr(hdev, &generated_bdaddr); | ||
| + if (err) { | ||
| + bt_dev_err(hdev, "Failed to set the generated BD_ADDR from serial number"); | ||
| return err; | ||
| + } | ||
| + | ||
| + /* Update hdev->public_addr and hdev->bdaddr */ | ||
| + bacpy(&hdev->public_addr, &generated_bdaddr); | ||
| + bacpy(&hdev->bdaddr, &generated_bdaddr); | ||
| + bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr); | ||
| + | ||
| + | ||
| + /* Disable reading BD_ADDR from NVM */ | ||
| + //err = qca_check_bdaddr(hdev, &config); | ||
| + //if (err) | ||
| + // return err; |
There was a problem hiding this comment.
Add fallback when serial‑derived BD_ADDR fails
If qcom_serial_number is NULL/invalid or socinfo hasn’t probed yet, Bluetooth setup now hard‑fails. Keeping the previous NVM/firmware fallback avoids losing BT on those paths.
🐛 Suggested fallback to NVM BD_ADDR
- /* Generate BD_ADDR from qcom_serial_number */
- err = generate_bdaddr_from_serial(hdev, &generated_bdaddr);
- if (err) {
- bt_dev_err(hdev, "Failed to generate BD_ADDR from serial number");
- return err;
- }
-
- /* Set the generated BD_ADDR */
- err = qca_set_bdaddr(hdev, &generated_bdaddr);
- if (err) {
- bt_dev_err(hdev, "Failed to set the generated BD_ADDR from serial number");
- return err;
- }
-
- /* Update hdev->public_addr and hdev->bdaddr */
- bacpy(&hdev->public_addr, &generated_bdaddr);
- bacpy(&hdev->bdaddr, &generated_bdaddr);
- bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr);
-
-
- /* Disable reading BD_ADDR from NVM */
- //err = qca_check_bdaddr(hdev, &config);
- //if (err)
- // return err;
+ /* Generate BD_ADDR from qcom_serial_number */
+ err = generate_bdaddr_from_serial(hdev, &generated_bdaddr);
+ if (err) {
+ bt_dev_warn(hdev, "Serial-derived BD_ADDR unavailable, falling back to NVM");
+ err = qca_check_bdaddr(hdev, &config);
+ if (err)
+ return err;
+ } else {
+ /* Set the generated BD_ADDR */
+ err = qca_set_bdaddr(hdev, &generated_bdaddr);
+ if (err) {
+ bt_dev_err(hdev, "Failed to set the generated BD_ADDR from serial number");
+ return err;
+ }
+
+ /* Update hdev->public_addr and hdev->bdaddr */
+ bacpy(&hdev->public_addr, &generated_bdaddr);
+ bacpy(&hdev->bdaddr, &generated_bdaddr);
+ bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr);
+ }📝 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.
| + bdaddr_t generated_bdaddr; | |
| bt_dev_dbg(hdev, "QCA setup on UART"); | |
| @@ -993,9 +1066,30 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, | |
| break; | |
| } | |
| - err = qca_check_bdaddr(hdev, &config); | |
| - if (err) | |
| + /* Generate BD_ADDR from qcom_serial_number */ | |
| + err = generate_bdaddr_from_serial(hdev, &generated_bdaddr); | |
| + if (err) { | |
| + bt_dev_err(hdev, "Failed to generate BD_ADDR from serial number"); | |
| + return err; | |
| + } | |
| + | |
| + /* Set the generated BD_ADDR */ | |
| + err = qca_set_bdaddr(hdev, &generated_bdaddr); | |
| + if (err) { | |
| + bt_dev_err(hdev, "Failed to set the generated BD_ADDR from serial number"); | |
| return err; | |
| + } | |
| + | |
| + /* Update hdev->public_addr and hdev->bdaddr */ | |
| + bacpy(&hdev->public_addr, &generated_bdaddr); | |
| + bacpy(&hdev->bdaddr, &generated_bdaddr); | |
| + bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr); | |
| + | |
| + | |
| + /* Disable reading BD_ADDR from NVM */ | |
| + //err = qca_check_bdaddr(hdev, &config); | |
| + //if (err) | |
| + // return err; | |
| bdaddr_t generated_bdaddr; | |
| bt_dev_dbg(hdev, "QCA setup on UART"); | |
| @@ -993,9 +1066,30 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, | |
| break; | |
| } | |
| - /* Generate BD_ADDR from qcom_serial_number */ | |
| - err = generate_bdaddr_from_serial(hdev, &generated_bdaddr); | |
| - if (err) { | |
| - bt_dev_err(hdev, "Failed to generate BD_ADDR from serial number"); | |
| - return err; | |
| - } | |
| - | |
| - /* Set the generated BD_ADDR */ | |
| - err = qca_set_bdaddr(hdev, &generated_bdaddr); | |
| - if (err) { | |
| /* Generate BD_ADDR from qcom_serial_number */ | |
| err = generate_bdaddr_from_serial(hdev, &generated_bdaddr); | |
| if (err) { | |
| bt_dev_warn(hdev, "Serial-derived BD_ADDR unavailable, falling back to NVM"); | |
| err = qca_check_bdaddr(hdev, &config); | |
| if (err) | |
| return err; | |
| } else { | |
| /* Set the generated BD_ADDR */ | |
| err = qca_set_bdaddr(hdev, &generated_bdaddr); | |
| if (err) { | |
| bt_dev_err(hdev, "Failed to set the generated BD_ADDR from serial number"); | |
| return err; | |
| } | |
| /* Update hdev->public_addr and hdev->bdaddr */ | |
| bacpy(&hdev->public_addr, &generated_bdaddr); | |
| bacpy(&hdev->bdaddr, &generated_bdaddr); | |
| bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr); | |
| } | |
| return err; | |
| - } | |
| - | |
| - /* Update hdev->public_addr and hdev->bdaddr */ | |
| - bacpy(&hdev->public_addr, &generated_bdaddr); | |
| - bacpy(&hdev->bdaddr, &generated_bdaddr); | |
| - bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr); | |
| - | |
| - | |
| - /* Disable reading BD_ADDR from NVM */ | |
| - //err = qca_check_bdaddr(hdev, &config); | |
| - //if (err) | |
| - // return err; |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 108 - 141, The patch removes the previous NVM/firmware fallback and
hard-fails when generate_bdaddr_from_serial or qca_set_bdaddr fail; restore a
fallback to the original qca_check_bdaddr path instead of returning immediately
so BT can still use NVM BD_ADDR. Change qca_uart_setup to: call
generate_bdaddr_from_serial and, on error, log a warning and call
qca_check_bdaddr(&config) as the fallback; if generate succeeds but
qca_set_bdaddr fails, log the failure and fall back to qca_check_bdaddr(&config)
as well; only return an error if both the serial-derived path and
qca_check_bdaddr both fail. Ensure you still update
hdev->public_addr/hdev->bdaddr only on successful qca_set_bdaddr, and keep the
bt_dev_info log when the BD_ADDR is successfully set.
patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0017-Disable-sdhci_pltfm_free-calling.patch
Outdated
Show resolved
Hide resolved
| + | ||
| + pwm_backlight_default: pwm-backlight-default-state { | ||
| + pins = "gpio5"; | ||
| + function = "func3"; | ||
| + input-disable; | ||
| + output-low; | ||
| + output-enable; | ||
| + bias-disable; | ||
| + power-source = <0>; | ||
| + qcom,drive-strength = <2>; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&pmk8550_pwm { | ||
| + status = "okay"; | ||
| +}; | ||
| + | ||
| +&i2c0 { | ||
| + clock-frequency = <400000>; | ||
| + status = "okay"; | ||
| + | ||
| + pwm_rgb_left: sn3112@54 { | ||
| + compatible = "si-en,sn3112-pwm"; | ||
| + reg = <0x54>; | ||
| + sdb-gpios = <&tlmm 55 GPIO_ACTIVE_LOW>; | ||
| + vdd-supply = <&vdd_mcu_3v3>; | ||
| + #pwm-cells = <1>; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&i2c4 { | ||
| + clock-frequency = <400000>; | ||
| + status = "okay"; | ||
| + | ||
| + touchscreen@20 { | ||
| + compatible = "syna,rmi4-i2c"; | ||
| + reg = <0x20>; | ||
| + #address-cells = <1>; | ||
| + #size-cells = <0>; | ||
| + interrupts-extended = <&tlmm 25 0x2008>; | ||
| + | ||
| + pinctrl-names = "default", "sleep"; | ||
| + pinctrl-0 = <&ts_int_default>; | ||
| + pinctrl-1 = <&ts_int_sleep>; | ||
| + | ||
| + vio-supply = <&vreg_l12b_1p8>; | ||
| + | ||
| + syna,startup-delay-ms = <200>; | ||
| + syna,reset-delay-ms = <200>; | ||
| + | ||
| + rmi4-f01@1 { | ||
| + syna,nosleep-mode = <0x1>; | ||
| + reg = <0x1>; | ||
| + }; | ||
| + | ||
| + rmi4-f12@12 { | ||
| + reg = <0x12>; | ||
| + syna,rezero-wait-ms = <20>; | ||
| + syna,clip-x-low = <0>; | ||
| + syna,clip-y-low = <0>; | ||
| + syna,clip-x-high = <1080>; | ||
| + syna,clip-y-high = <1920>; | ||
| + syna,sensor-type = <1>; | ||
| + touchscreen-inverted-x; | ||
| + }; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&i2c12 { | ||
| + clock-frequency = <400000>; | ||
| + status = "okay"; | ||
| + | ||
| + pwm_rgb_right: sn3112@54 { | ||
| + compatible = "si-en,sn3112-pwm"; | ||
| + reg = <0x54>; | ||
| + sdb-gpios = <&tlmm 56 GPIO_ACTIVE_LOW>; | ||
| + vdd-supply = <&vdd_mcu_3v3>; | ||
| + #pwm-cells = <1>; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&remoteproc_adsp { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/adsp.mbn", | ||
| + "qcom/sm8550/ayn/odin2portal/adsp_dtb.mbn"; | ||
| + status = "okay"; | ||
| +}; | ||
| + | ||
| +&remoteproc_cdsp { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/cdsp.mbn", | ||
| + "qcom/sm8550/ayn/odin2portal/cdsp_dtb.mbn"; | ||
| + status = "okay"; | ||
| +}; | ||
| + | ||
| +&spk_amp_l { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/aw883xx_acf.bin"; | ||
| +}; | ||
| + | ||
| +&spk_amp_r { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/aw883xx_acf.bin"; | ||
| +}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, list available patches in the directory to confirm file exists
echo "=== Available patches in sm8550-6.18 ==="
ls -1 patch/kernel/archive/sm8550-6.18/ | grep -i odin || echo "No odin patches found"
echo ""
echo "=== Checking the specific patch file ==="
# Check if the patch file exists
test -f "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch" && echo "Patch file exists" || echo "Patch file not found"Repository: armbian/build
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Read the specific lines from the patch to verify the firmware paths
if test -f "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch"; then
echo "=== Firmware paths in lines 1580-1694 ==="
sed -n '1580,1694p' "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch" | grep -E "(firmware-name|odin2)"
fiRepository: armbian/build
Length of output: 503
🏁 Script executed:
#!/bin/bash
# Check all odin-related patches to understand the device variants
echo "=== All odin-related patches ==="
fd -t f "odin" patch/kernel/archive/sm8550-6.18/
echo ""
echo "=== Grep for odin2 vs odin2portal in patches ==="
rg -l "odin2" patch/kernel/archive/sm8550-6.18/ || echo "No matches found"Repository: armbian/build
Length of output: 409
🏁 Script executed:
#!/bin/bash
# Check if there's a sm8550-6.12 version with odin patches to see original naming
echo "=== Checking sm8550-6.12 for odin patches ==="
ls -1 patch/kernel/archive/sm8550-6.12/ 2>/dev/null | grep -i odin || echo "No 6.12 odin patches found"Repository: armbian/build
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Check the 6.12 Odin2 patch to see what firmware paths were originally used
echo "=== Checking 6.12 Odin2 DTS patch for firmware paths ==="
if test -f "patch/kernel/archive/sm8550-6.12/0050_arm64--dts--qcom--Add-AYN-Odin2.patch"; then
echo "File found, checking firmware references:"
grep -n "firmware-name" "patch/kernel/archive/sm8550-6.12/0050_arm64--dts--qcom--Add-AYN-Odin2.patch" | head -10
else
echo "File not found"
fiRepository: armbian/build
Length of output: 484
🏁 Script executed:
#!/bin/bash
# Check the device tree file name and base device node to confirm this is for odin2
echo "=== Checking device tree file name and structure in 6.18 patch ==="
if test -f "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch"; then
echo "Device tree file name:"
grep -m1 "^+++ b/arch" "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch"
echo ""
echo "First few lines of device tree content:"
sed -n '1,100p' "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch" | grep -E "^\+[^+]|device|model|compatible" | head -15
fiRepository: armbian/build
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Check if odin2portal firmware files or device tree exist elsewhere
echo "=== Searching for odin2portal references in all patches ==="
rg -n "odin2portal" patch/kernel/archive/sm8550-6.18/ --glob "*.patch" | head -15
echo ""
echo "=== Check 6.12 patches for odin2portal ==="
rg -n "odin2portal" patch/kernel/archive/sm8550-6.12/ --glob "*.patch" | head -5Repository: armbian/build
Length of output: 3210
Firmware paths incorrectly reference odin2portal in the odin2 device tree.
The qcs8550-ayn-odin2.dts file uses firmware paths pointing to qcom/sm8550/ayn/odin2portal/ instead of qcom/sm8550/ayn/odin2/:
- a740_zap.mbn
- adsp.mbn and adsp_dtb.mbn
- cdsp.mbn and cdsp_dtb.mbn
- aw883xx_acf.bin (speaker amps)
While the patch correctly creates a separate qcs8550-ayn-odin2portal.dts file with the same firmware paths, the base odin2 device should not share the Portal variant's firmware paths. If both devices use identical firmware, this should be explicitly documented or refactored to use a shared path. Otherwise, update all six firmware references in the odin2 section to the correct paths.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`
around lines 1580 - 1694, The firmware paths in the odin2 DTS overlay are
incorrectly pointing to "qcom/sm8550/ayn/odin2portal/"; update the six
firmware-name properties to the correct "qcom/sm8550/ayn/odin2/" path (change
entries in the &gpu zap-shader node, &remoteproc_adsp node, &remoteproc_cdsp
node, and both &spk_amp_l and &spk_amp_r nodes), or alternatively refactor to a
shared firmware path/label and reference that from both odin2 and odin2portal so
the Device Tree doesn't mix Portal-specific paths into the base odin2 device.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`:
- Around line 184-199: The code in handle_cmd_commod copies starting at
FRAME_POS_DATA_1 which includes the CMD_COMMOD subcommand byte; change the logic
to skip that byte by starting the version copy at FRAME_POS_DATA_1 + 1, require
payload_length >= 2 for a valid version, compute version_length =
min(payload_length - 1, MCU_VERSION_MAX_LEN - 1), and memcpy from
&data[FRAME_POS_DATA_1 + 1] into mcu_version before null-terminating and logging
with dev_info.
- Around line 370-428: Move the serdev_device_set_client_ops(serdev,
&rsinput_rx_ops) call to before serdev_device_open(serdev) so the serdev ops are
installed prior to opening; ensure drv->serdev is assigned after successful open
as before. Also add serdev_device_close(serdev) on each early return path that
occurs after serdev_device_open(serdev) (the error returns after opening at the
checks that follow: baudrate setup, devm_input_allocate_device/drv->input
allocation, input_register_device, and after device_property reads if they can
fail) so you don't leak the open handle; keep the existing
rsinput_init_commands() error path close behavior. Use the symbols
serdev_device_set_client_ops, serdev_device_open, serdev_device_close,
rsinput_rx_ops, rsinput_init_commands, and drv->serdev to locate and update the
code.
In
`@patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch`:
- Line 348: The MODULE_AUTHOR macro invocation is malformed:
MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); is missing the closing angle
bracket and closing quote; update the MODULE_AUTHOR line (the MODULE_AUTHOR
symbol in the patch) to include the complete author string e.g. add the missing
">" and trailing quote so it becomes MODULE_AUTHOR("Teguh Sobirin
<teguh@sobir.in>");.
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`:
- Around line 34-54: sc8280xp_snd_startup enables pdata->i2s_clk before calling
qcom_snd_sdw_startup(), but never disables it if qcom_snd_sdw_startup() fails;
capture the return value from qcom_snd_sdw_startup(substream) into an int ret,
and if ret is negative and the cpu_dai->id case was PRIMARY_MI2S_RX then call
clk_disable_unprepare(pdata->i2s_clk) (or the existing sc8280xp_snd_shutdown
helper if present) before returning ret; ensure you return ret instead of
directly returning qcom_snd_sdw_startup(substream).
In
`@patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch`:
- Around line 99-149: td4328_on() and td4328_off() set/clear MIPI_DSI_MODE_LPM
but don't restore the original dsi->mode_flags on error paths; modify both
functions (td4328_on and td4328_off) to save the original mode_flags at the
start (e.g., orig = dsi->mode_flags) and on any early return due to failure
restore dsi->mode_flags = orig before returning, ensuring the flag state is
always restored on all error paths.
In
`@patch/kernel/archive/sm8550-6.18/0016-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`:
- Around line 1713-1715: The interrupt specifiers are inconsistent: the
odin2.dts sdhc_2 node uses 4-cell entries (<GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH 0>)
while odin2portal.dts uses the standard 3-cell form (<GIC_SPI 207
IRQ_TYPE_LEVEL_HIGH>); update the sdhc_2 node to use 3-cell GIC_SPI specifiers
by removing the trailing zero from each interrupts entry so both nodes use
<GIC_SPI N IRQ_TYPE_LEVEL_HIGH>, and verify interrupt-names ("hc_irq","pwr_irq")
still align with the two entries.
In
`@patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 31-39: The error log in the AW88166_DSP_32_DATA case has a typo
("32r-bit") and should match the 16-bit message format; update the dev_err call
in the AW88166_DSP_32_DATA branch (where aw_dev_dsp_read_32bit is called) to use
"32-bit dsp_data[0x%x] failed" and keep the same arguments ((u32)dsp_addr,
*dsp_data) so the message reads "read dsp_addr[0x%x] 32-bit dsp_data[0x%x]
failed".
- Around line 72-85: The loop doing 16-bit DSP writes must validate buffer
length and use unaligned-safe access: add a pre-check that len is even (return
-EINVAL or jump to error_operation on odd len), replace cpu_to_be16p((u16
*)(data + i)) with get_unaligned_be16(data + i) and write that value, and
include the kernel unaligned header (e.g. `#include` <linux/unaligned.h>). Keep
existing locking and error paths (aw_dev->dsp_lock, regmap_write,
AW88166_DSPMADD_REG, AW88166_DSPMDAT_REG, error_operation) unchanged except for
the new length check and using get_unaligned_be16 instead of cpu_to_be16p.
♻️ Duplicate comments (5)
patch/kernel/archive/sm8550-6.18/0014-HACK-fix-usb-boot-hang.patch (1)
19-34: Scope the xHCI readiness skip to affected hardware.Removing the CNR handshake and warning globally means we now write to operational registers even when the host controller reports "not ready," which can cause undefined behavior on other xHCI controllers and removes diagnostics. Please gate this bypass to the specific Odin2/SM8550 controller (e.g., PCI ID match or quirk flag) and keep the default handshake for everyone else.
patch/kernel/archive/sm8550-6.18/0016-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (1)
1580-1694: Firmware paths incorrectly referenceodin2portalin theodin2device tree.The
qcs8550-ayn-odin2.dtsfile uses firmware paths pointing toqcom/sm8550/ayn/odin2portal/instead ofqcom/sm8550/ayn/odin2/for:
- GPU zap-shader (a740_zap.mbn)
- ADSP firmware (adsp.mbn, adsp_dtb.mbn)
- CDSP firmware (cdsp.mbn, cdsp_dtb.mbn)
- Speaker amp firmware (aw883xx_acf.bin)
If both devices share identical firmware, please add a comment documenting this. Otherwise, update the paths to reference the correct
odin2subdirectory.patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch (2)
282-284: Incomplete GPIO error handling may cause kernel oops.Only
-EPROBE_DEFERis checked, but other errors fromdevm_gpiod_get()would leave anERR_PTRinpriv->sdb. Subsequentgpiod_set_value_cansleep()calls with this invalid pointer can cause undefined behavior.Proposed fix
priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); - if (PTR_ERR(priv->sdb) == -EPROBE_DEFER) - return -EPROBE_DEFER; + if (IS_ERR(priv->sdb)) + return PTR_ERR(priv->sdb);
292-311: Resource leak: regulator not disabled on probe failure.If
htr3212_init_regs()orhtr3212_parse_dt()fails,regulator_enable()was already called but is never disabled. This leaks the regulator reference.Proposed fix using goto-based cleanup
ret = regulator_enable(priv->vdd); - if (ret < 0) { + 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_reg_disable; ret = htr3212_parse_dt(dev, priv); if (ret) - return ret; + goto err_reg_disable; return 0; + +err_reg_disable: + gpiod_set_value_cansleep(priv->sdb, 0); + regulator_disable(priv->vdd); + return ret; }patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (1)
20-85: Add<linux/ctype.h>forisxdigit()usage.🛠️ Suggested fix
-#include "btqca.h" +#include "btqca.h" +#include <linux/ctype.h>-/* ath12k/mac.c includes */ +#include <linux/ctype.h>
🧹 Nitpick comments (3)
patch/kernel/archive/sm8550-6.18/0006-dt-bindings-mmc-qcom-Document-level-shifter-flag-for.patch (1)
31-33: Consider adding explicit type for the boolean property.The
qcom,use-level-shifterproperty works as a boolean flag, but for clarity and binding validation, consider addingtype: booleanto the property definition.qcom,use-level-shifter: description: Flag to indicate if platform has level shifter for SD card. + type: booleanpatch/kernel/archive/sm8550-6.18/0016-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (1)
1574-1574: Inconsistent indentation.Line 1574 uses spaces for indentation while the rest of the file uses tabs. This should be tabs for consistency with kernel DTS style.
- backlight = <&backlight>; + backlight = <&backlight>;patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch (1)
136-141: Simplify return statement.The
if (ret) return ret; return 0;pattern can be simplified.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); }
| +static void handle_cmd_commod(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) { | ||
| + switch (data[FRAME_POS_DATA_1]) { | ||
| + case DATA_COMMOD_VERSION: | ||
| + if (payload_length >= 1) { | ||
| + char mcu_version[MCU_VERSION_MAX_LEN] = {0}; | ||
| + size_t version_length = payload_length; | ||
| + if (version_length > MCU_VERSION_MAX_LEN - 1) { | ||
| + version_length = MCU_VERSION_MAX_LEN - 1; | ||
| + } | ||
| + memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length); | ||
| + mcu_version[version_length] = '\0'; | ||
| + dev_info(&drv->serdev->dev, "MCU Version: %s\n", mcu_version); | ||
| + } else { | ||
| + dev_err(&drv->serdev->dev, "Invalid MCU version response length\n"); | ||
| + } | ||
| + break; |
There was a problem hiding this comment.
Skip the CMD_COMMOD subcommand byte when building the MCU version string.
mcu_version currently includes the subcommand byte at FRAME_POS_DATA_1, which is not part of the version string.
🛠️ Suggested fix
case DATA_COMMOD_VERSION:
- if (payload_length >= 1) {
+ if (payload_length >= 2) {
char mcu_version[MCU_VERSION_MAX_LEN] = {0};
- size_t version_length = payload_length;
+ size_t version_length = payload_length - 1;
if (version_length > MCU_VERSION_MAX_LEN - 1) {
version_length = MCU_VERSION_MAX_LEN - 1;
}
- memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length);
+ memcpy(mcu_version, &data[FRAME_POS_DATA_2], version_length);
mcu_version[version_length] = '\0';
dev_info(&drv->serdev->dev, "MCU Version: %s\n", mcu_version);🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 184 - 199, The code in handle_cmd_commod copies starting at
FRAME_POS_DATA_1 which includes the CMD_COMMOD subcommand byte; change the logic
to skip that byte by starting the version copy at FRAME_POS_DATA_1 + 1, require
payload_length >= 2 for a valid version, compute version_length =
min(payload_length - 1, MCU_VERSION_MAX_LEN - 1), and memcpy from
&data[FRAME_POS_DATA_1 + 1] into mcu_version before null-terminating and logging
with dev_info.
| + error = serdev_device_open(serdev); | ||
| + if (error) | ||
| + return dev_err_probe(&serdev->dev, error, "Unable to open UART device\n"); | ||
| + | ||
| + drv->serdev = serdev; | ||
| + drv->sequence_number = 0; | ||
| + | ||
| + serdev_device_set_drvdata(serdev, drv); | ||
| + | ||
| + error = serdev_device_set_baudrate(serdev, 115200); | ||
| + if (error < 0) | ||
| + return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n"); | ||
| + | ||
| + serdev_device_set_flow_control(serdev, false); | ||
| + | ||
| + drv->input = devm_input_allocate_device(&serdev->dev); | ||
| + if (!drv->input) | ||
| + return -ENOMEM; | ||
| + | ||
| + drv->input->phys = "rsinput-gamepad/input0"; | ||
| + | ||
| + error = device_property_read_string(&serdev->dev, "gamepad-name", &drv->input->name); | ||
| + if (error) { | ||
| + drv->input->name = "RSInput Gamepad"; | ||
| + } | ||
| + | ||
| + device_property_read_u32(&serdev->dev, "gamepad-bus", &gamepad_bus); | ||
| + device_property_read_u32(&serdev->dev, "gamepad-vid", &gamepad_vid); | ||
| + device_property_read_u32(&serdev->dev, "gamepad-pid", &gamepad_pid); | ||
| + device_property_read_u32(&serdev->dev, "gamepad-rev", &gamepad_rev); | ||
| + | ||
| + drv->input->id.bustype = (u16)gamepad_bus; | ||
| + drv->input->id.vendor = (u16)gamepad_vid; | ||
| + drv->input->id.product = (u16)gamepad_pid; | ||
| + drv->input->id.version = (u16)gamepad_rev; | ||
| + | ||
| + __set_bit(EV_KEY, drv->input->evbit); | ||
| + for (int i = 0; i < ARRAY_SIZE(keymap); i++) | ||
| + input_set_capability(drv->input, EV_KEY, keymap[i]); | ||
| + | ||
| + __set_bit(EV_ABS, drv->input->evbit); | ||
| + for (int i = ABS_X; i <= ABS_RZ; i++) | ||
| + input_set_abs_params(drv->input, i, -0x580, 0x580, | ||
| + 0, 0); | ||
| + | ||
| + input_set_abs_params(drv->input, ABS_HAT2X, 0, 1830, 0, 30); | ||
| + input_set_abs_params(drv->input, ABS_HAT2Y, 0, 1830, 0, 30); | ||
| + | ||
| + error = input_register_device(drv->input); | ||
| + if (error) | ||
| + return error; | ||
| + | ||
| + serdev_device_set_client_ops(serdev, &rsinput_rx_ops); | ||
| + | ||
| + error = rsinput_init_commands(drv); | ||
| + if (error < 0) { | ||
| + serdev_device_close(serdev); | ||
| + return error; | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch" | sed -n '370,428p'Repository: armbian/build
Length of output: 170
🏁 Script executed:
pwd && ls -laRepository: armbian/build
Length of output: 1470
🏁 Script executed:
cat -n "patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch" | sed -n '370,428p'Repository: armbian/build
Length of output: 2517
🏁 Script executed:
# Search for other serdev usage in the codebase and documentation
find . -type f -name "*.patch" -o -name "*.c" -o -name "*.h" | xargs grep -l "serdev_device_set_client_ops\|serdev_device_open" | head -20Repository: armbian/build
Length of output: 740
🏁 Script executed:
# Fix the search command
find . -type f \( -name "*.patch" -o -name "*.c" -o -name "*.h" \) -exec grep -l "serdev_device_set_client_ops" {} \;Repository: armbian/build
Length of output: 707
🏁 Script executed:
grep -A 30 -B 5 "serdev_device_open" "./patch/kernel/archive/sm8250-6.12/0038-Input-driver-for-AYN-Odin2-Gamepad.patch" | head -80Repository: armbian/build
Length of output: 1316
🏁 Script executed:
grep -A 60 "input_register_device" "./patch/kernel/archive/sm8250-6.12/0038-Input-driver-for-AYN-Odin2-Gamepad.patch" | grep -B 5 -A 5 "set_client_ops"Repository: armbian/build
Length of output: 323
🏁 Script executed:
grep -A 50 "serdev_device_open" "./patch/kernel/archive/sunxi-6.18/patches.megous/modem-6.18/0001-misc-modem-power-Power-manager-for-modems.patch" | head -80Repository: armbian/build
Length of output: 2013
🏁 Script executed:
grep -B 20 "serdev_device_set_client_ops" "./patch/kernel/archive/sunxi-6.18/patches.megous/modem-6.18/0001-misc-modem-power-Power-manager-for-modems.patch" | head -50Repository: armbian/build
Length of output: 560
🏁 Script executed:
grep -A 15 "err_shutdown:" "./patch/kernel/archive/sunxi-6.18/patches.megous/modem-6.18/0001-misc-modem-power-Power-manager-for-modems.patch" | head -30Repository: armbian/build
Length of output: 573
🏁 Script executed:
web_search
Linux kernel serdev_device_open serdev_device_set_client_ops ordering requirementsRepository: armbian/build
Length of output: 151
🌐 Web query:
serdev_device_set_client_ops serdev_device_open ordering Linux kernel API
💡 Result:
In a serdev client driver, call serdev_device_set_client_ops() before serdev_device_open() / devm_serdev_device_open().
Reason: the controller’s data/tx callbacks (serdev_controller_receive_buf() / serdev_controller_write_wakeup()) dereference serdev->ops (and then ->receive_buf / ->write_wakeup) and do not check serdev->ops for NULL, so the ops must be installed before the controller is opened/enabled and can start delivering RX/TX events. [1] Also, serdev_device_open() calls into the controller’s ->open() and enables runtime PM, which is the point after which the controller may start I/O. [2]
Typical probe ordering:
serdev_device_set_client_ops(serdev, &my_ops);
ret = devm_serdev_device_open(dev, serdev); /* or serdev_device_open() */
if (ret)
return ret;
serdev_device_set_baudrate(serdev, ...);
serdev_device_set_flow_control(serdev, ...);Sources: [1] (serdev.h: serdev_device_set_client_ops(), serdev_controller_receive_buf(), serdev_controller_write_wakeup()) [2] (serdev core: serdev_device_open()).
🏁 Script executed:
# Check the sm8550-6.12 version of the RSInput driver
grep -n "serdev_device_open\|serdev_device_set_client_ops" "./patch/kernel/archive/sm8550-6.12/0031_input--Add-driver-for-RSInput-Gamepad.patch" | head -20Repository: armbian/build
Length of output: 165
🏁 Script executed:
# Verify which error paths don't close the serdev handle
grep -n "return" "./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch" | grep -A 1 -B 1 "370\|371\|372\|380\|381\|387\|420"Repository: armbian/build
Length of output: 345
Set serdev ops before open and close on all error paths.
serdev_device_open() is called before serdev_device_set_client_ops(), which violates the kernel serdev API requirement. The controller's RX/TX callbacks dereference serdev->ops without null checks, so ops must be set before the device is opened. Additionally, failures after open() at lines 372, 381, 387, and 420 leak the open handle since they return directly without calling serdev_device_close(). Only the rsinput_init_commands error path closes the device.
🛠️ Suggested ordering + cleanup
- error = serdev_device_open(serdev);
+ serdev_device_set_client_ops(serdev, &rsinput_rx_ops);
+ error = serdev_device_open(serdev);
if (error)
return dev_err_probe(&serdev->dev, error, "Unable to open UART device\n");
@@
error = serdev_device_set_baudrate(serdev, 115200);
if (error < 0)
- return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n");
+ goto err_close;
@@
drv->input = devm_input_allocate_device(&serdev->dev);
- if (!drv->input)
- return -ENOMEM;
+ if (!drv->input) {
+ error = -ENOMEM;
+ goto err_close;
+ }
@@
error = input_register_device(drv->input);
if (error)
- return error;
-
- serdev_device_set_client_ops(serdev, &rsinput_rx_ops);
+ goto err_close;
@@
error = rsinput_init_commands(drv);
- if (error < 0) {
- serdev_device_close(serdev);
- return error;
- }
+ if (error < 0)
+ goto err_close;
return 0;
+
+err_close:
+ serdev_device_close(serdev);
+ return error;🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 370 - 428, Move the serdev_device_set_client_ops(serdev,
&rsinput_rx_ops) call to before serdev_device_open(serdev) so the serdev ops are
installed prior to opening; ensure drv->serdev is assigned after successful open
as before. Also add serdev_device_close(serdev) on each early return path that
occurs after serdev_device_open(serdev) (the error returns after opening at the
checks that follow: baudrate setup, devm_input_allocate_device/drv->input
allocation, input_register_device, and after device_property reads if they can
fail) so you don't leak the open handle; keep the existing
rsinput_init_commands() error path close behavior. Use the symbols
serdev_device_set_client_ops, serdev_device_open, serdev_device_close,
rsinput_rx_ops, rsinput_init_commands, and drv->serdev to locate and update the
code.
patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch
Show resolved
Hide resolved
| +static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) | ||
| +{ | ||
| + unsigned int fmt = SND_SOC_DAIFMT_BP_FP; | ||
| + unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; | ||
| + struct snd_soc_pcm_runtime *rtd = substream->private_data; | ||
| + struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); | ||
| + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); | ||
| + struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); | ||
| + | ||
| + switch (cpu_dai->id) { | ||
| + case PRIMARY_MI2S_RX: | ||
| + clk_prepare_enable(pdata->i2s_clk); | ||
| + snd_soc_dai_set_fmt(cpu_dai, fmt); | ||
| + snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | ||
| + break; | ||
| + default: | ||
| + break; | ||
| + } | ||
| + | ||
| + return qcom_snd_sdw_startup(substream); | ||
| +} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat "patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch"Repository: armbian/build
Length of output: 3220
🏁 Script executed:
rg -A 10 "qcom_snd_sdw_startup" --type c | head -40Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Search for similar startup error handling in ASoC drivers
rg -B 5 -A 10 "\.startup.*=" sound/soc/qcom/ --type c | head -60Repository: armbian/build
Length of output: 112
🏁 Script executed:
# Search all patches for qcom_snd_sdw_startup references
rg "qcom_snd_sdw_startup" patch/Repository: armbian/build
Length of output: 578
🏁 Script executed:
# Look at other ASoC startup handlers to understand error handling patterns
rg -B 3 -A 15 "static.*startup.*snd_pcm_substream" patch/ --type patch | head -80Repository: armbian/build
Length of output: 86
🏁 Script executed:
# Search for qcom_snd_sdw_startup definition or declaration
grep -r "qcom_snd_sdw_startup" patch/ | head -20Repository: armbian/build
Length of output: 578
🏁 Script executed:
# Look for the function that defines qcom_snd_sdw_startup
grep -r "int.*qcom_snd_sdw_startup\|qcom_snd_sdw_startup.*int" patch/Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Check the 6.12 version of the same patch for comparison
cat patch/kernel/archive/sm8550-6.12/0036_ASoC--qcom--sc8280xp-Add-support-for-Primary-I2S.patch | grep -A 50 "sc8280xp_snd_startup"Repository: armbian/build
Length of output: 2129
🏁 Script executed:
# Search for qcom_snd_sdw_startup in kernel documentation or other files
find patch/ -name "*.c" -o -name "*.h" | xargs grep -l "qcom_snd_sdw_startup" 2>/dev/null | head -5Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Search in patches for qcom_snd_sdw or common.c patterns
grep -r "qcom_snd_sdw" patch/ | grep -i "static.*int\|return.*error\|fail" | head -10Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Check if there are other qcom drivers with similar error handling
grep -r "clk_prepare_enable" patch/kernel/archive/sm8550-6.18/ | head -5Repository: armbian/build
Length of output: 325
🏁 Script executed:
# Look for examples of error handling in ASoC startup handlers in other patches
grep -B 5 -A 15 "static.*int.*startup" patch/kernel/archive/sm8550-6.18/*.patch | grep -A 15 -B 5 "return.*ret\|if.*ret" | head -40Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Check if qcom_snd_sdw_startup is defined elsewhere in patches
grep -r "qcom_snd_sdw_startup\|def.*qcom_snd" patch/ | grep -v "\.patch:" | head -20Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Look for error paths after resource allocation in ASoC drivers
grep -B 3 -A 8 "clk_prepare_enable" patch/kernel/archive/sm8550-6.18/*.patch | head -50Repository: armbian/build
Length of output: 2788
🏁 Script executed:
# Search for standard ASoC pattern - check if shutdown is paired with startup in framework
grep -r "startup\|shutdown" patch/kernel/archive/sm8550-6.18/*.patch | grep -i "snd_soc_ops\|\.startup\|\.shutdown" | head -20Repository: armbian/build
Length of output: 666
🏁 Script executed:
# Look for error handling patterns after clock enable in kernel drivers
grep -B 2 -A 5 "clk_prepare_enable" patch/ -r | grep -B 5 -A 5 "if\|return\|error" | head -50Repository: armbian/build
Length of output: 4812
🏁 Script executed:
# Check if clk_prepare_enable can return an error value
grep -r "clk_prepare_enable" patch/ | grep "ret\|error\|if\|check" | head -15Repository: armbian/build
Length of output: 1767
🏁 Script executed:
# Look for the kernel source to understand clk_prepare_enable signature
web_search "clk_prepare_enable return value kernel documentation"Repository: armbian/build
Length of output: 107
🌐 Web query:
clk_prepare_enable return value kernel clock framework
💡 Result:
clk_prepare_enable(struct clk *clk) returns an int status:
0on success- Negative
-errnoon failure (it returns the error fromclk_prepare()orclk_enable()). Ifclk_enable()fails, it callsclk_unprepare(clk)to roll back and then returns the enable error. [1]
It is intended for non-atomic (sleepable) context, because clk_prepare() may sleep. [1][2]
Sources: [1] [2]
🌐 Web query:
ASoC snd_soc_ops startup shutdown error handling framework
💡 Result:
In ASoC, the framework-level error handling for struct snd_soc_ops/DAI ops startup()/shutdown() is implemented in sound/soc/soc-pcm.c:
-
Call order on open (
soc_pcm_open):snd_soc_dai_startup(cpu_dai, substream)- open all rtd components (
soc_pcm_components_open) snd_soc_dai_startup(codec_dai, substream)for each codec DAI- optional machine/link
rtd->dai_link->ops->startup(substream)
If any step fails, the function jumps to rollback labels that unwind only what was successfully started/opened. [1][2]
-
Rollback / unwind guarantees on error:
- If machine/link
startup()ran and later checks fail, machine/linkshutdown()is called (config_err:path). [2] - If any codec DAI
startup()fails (or later steps fail), already-started codec DAIs are shut down in reverse via a rollback iterator, then components are closed, then CPU DAI is shut down. [2] - Component open/close is written to support partial success: a later patch explicitly fixed/centralized “close already-opened components on error” behavior so partial opens are cleaned up correctly. [4]
- If machine/link
-
Normal close (
soc_pcm_close):- Deactivates runtime, mutes, then calls
snd_soc_dai_shutdown(cpu_dai, ...),snd_soc_dai_shutdown(codec_dai, ...), optional machine/linkshutdown(), thensoc_pcm_components_close(...). [3]
- Deactivates runtime, mutes, then calls
Practical guidance for driver authors:
startup()must return< 0on failure; the core will unwind and callshutdown()only for parts that were successfully started. [1][2]shutdown()must be safe as a cleanup hook (typically tolerate partial setup, and be effectively idempotent with respect to the resources you acquired instartup()), because it’s used both on normal close and on open-time error rollback. [2][3]
Sources: Linux ASoC core (soc_pcm_open error labels and unwind paths; soc_pcm_close shutdown ordering), plus the patch discussion about fixing component open/close error unwind. [1][2][3][4]
Add error-path cleanup for i2s_clk if qcom_snd_sdw_startup() fails.
The clock is enabled before calling qcom_snd_sdw_startup(), which can fail. If it does, shutdown() is not called, leaving the clock enabled. Capture the return value and disable the clock on error before returning.
Proposed fix
static int sc8280xp_snd_startup(struct snd_pcm_substream *substream)
{
unsigned int fmt = SND_SOC_DAIFMT_BP_FP;
unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S;
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card);
struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);
+ int ret;
switch (cpu_dai->id) {
case PRIMARY_MI2S_RX:
clk_prepare_enable(pdata->i2s_clk);
snd_soc_dai_set_fmt(cpu_dai, fmt);
snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
break;
default:
break;
}
- return qcom_snd_sdw_startup(substream);
+ ret = qcom_snd_sdw_startup(substream);
+ if (ret && cpu_dai->id == PRIMARY_MI2S_RX)
+ clk_disable_unprepare(pdata->i2s_clk);
+ 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.
| +static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) | |
| +{ | |
| + unsigned int fmt = SND_SOC_DAIFMT_BP_FP; | |
| + unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; | |
| + struct snd_soc_pcm_runtime *rtd = substream->private_data; | |
| + struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); | |
| + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); | |
| + struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); | |
| + | |
| + switch (cpu_dai->id) { | |
| + case PRIMARY_MI2S_RX: | |
| + clk_prepare_enable(pdata->i2s_clk); | |
| + snd_soc_dai_set_fmt(cpu_dai, fmt); | |
| + snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | |
| + break; | |
| + default: | |
| + break; | |
| + } | |
| + | |
| + return qcom_snd_sdw_startup(substream); | |
| +} | |
| static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) | |
| { | |
| unsigned int fmt = SND_SOC_DAIFMT_BP_FP; | |
| unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; | |
| struct snd_soc_pcm_runtime *rtd = substream->private_data; | |
| struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); | |
| struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); | |
| struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); | |
| int ret; | |
| switch (cpu_dai->id) { | |
| case PRIMARY_MI2S_RX: | |
| clk_prepare_enable(pdata->i2s_clk); | |
| snd_soc_dai_set_fmt(cpu_dai, fmt); | |
| snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | |
| break; | |
| default: | |
| break; | |
| } | |
| ret = qcom_snd_sdw_startup(substream); | |
| if (ret && cpu_dai->id == PRIMARY_MI2S_RX) | |
| clk_disable_unprepare(pdata->i2s_clk); | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`
around lines 34 - 54, sc8280xp_snd_startup enables pdata->i2s_clk before calling
qcom_snd_sdw_startup(), but never disables it if qcom_snd_sdw_startup() fails;
capture the return value from qcom_snd_sdw_startup(substream) into an int ret,
and if ret is negative and the cpu_dai->id case was PRIMARY_MI2S_RX then call
clk_disable_unprepare(pdata->i2s_clk) (or the existing sc8280xp_snd_shutdown
helper if present) before returning ret; ensure you return ret instead of
directly returning qcom_snd_sdw_startup(substream).
| +static int td4328_on(struct td4328 *ctx) | ||
| +{ | ||
| + struct mipi_dsi_device *dsi = ctx->dsi; | ||
| + struct device *dev = &dsi->dev; | ||
| + int ret; | ||
| + | ||
| + dsi->mode_flags |= MIPI_DSI_MODE_LPM; | ||
| + | ||
| + mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x00); | ||
| + mipi_dsi_dcs_write_seq(dsi, 0xb3, 0x31); | ||
| + mipi_dsi_dcs_write_seq(dsi, 0xd6, 0x00); | ||
| + | ||
| + ret = mipi_dsi_dcs_exit_sleep_mode(dsi); | ||
| + if (ret < 0) { | ||
| + dev_err(dev, "Failed to exit sleep mode: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + msleep(70); | ||
| + | ||
| + ret = mipi_dsi_dcs_set_display_on(dsi); | ||
| + if (ret < 0) { | ||
| + dev_err(dev, "Failed to set display on: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int td4328_off(struct td4328 *ctx) | ||
| +{ | ||
| + struct mipi_dsi_device *dsi = ctx->dsi; | ||
| + struct device *dev = &dsi->dev; | ||
| + int ret; | ||
| + | ||
| + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; | ||
| + | ||
| + ret = mipi_dsi_dcs_set_display_off(dsi); | ||
| + if (ret < 0) { | ||
| + dev_err(dev, "Failed to set display off: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + msleep(50); | ||
| + | ||
| + ret = mipi_dsi_dcs_enter_sleep_mode(dsi); | ||
| + if (ret < 0) { | ||
| + dev_err(dev, "Failed to enter sleep mode: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + msleep(120); | ||
| + | ||
| + return 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch" | sed -n '90,160p'Repository: armbian/build
Length of output: 170
🏁 Script executed:
# Search for similar panel drivers to check their MIPI_DSI_MODE_LPM handling
fd -type f -name "*.c" -path "*/panel/*" | head -10Repository: armbian/build
Length of output: 227
🏁 Script executed:
# Look for MIPI_DSI_MODE_LPM usage patterns in the kernel
rg "MIPI_DSI_MODE_LPM" -A 3 -B 3 | head -100Repository: armbian/build
Length of output: 10293
🏁 Script executed:
sed -n '90,160p' "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch"Repository: armbian/build
Length of output: 1709
🏁 Script executed:
# Also check if there's any cleanup on error or other LPM handling
grep -n "mode_flags\|LPM" "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch"Repository: armbian/build
Length of output: 219
🏁 Script executed:
# Find the context around line 252 to see the initial mode_flags setting
sed -n '245,260p' "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch"Repository: armbian/build
Length of output: 507
🏁 Script executed:
# Check the prepare/unprepare functions to see the calling context
sed -n '160,250p' "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch"Repository: armbian/build
Length of output: 2409
Restore LPM flag on error paths in td4328_on() and td4328_off().
The functions rely on balanced pairs (prepare calls on(), unprepare calls off()), but td4328_on() sets MIPI_DSI_MODE_LPM and fails without restoring it. If initialization fails after that flag change, subsequent DSI operations may use incorrect mode flags. Save and restore mode_flags within each function to ensure cleanup on all paths:
Suggested pattern
static int td4328_on(struct td4328 *ctx)
{
struct mipi_dsi_device *dsi = ctx->dsi;
struct device *dev = &dsi->dev;
+ unsigned long mode_flags = dsi->mode_flags;
int ret;
dsi->mode_flags |= MIPI_DSI_MODE_LPM;
@@
ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
if (ret < 0) {
dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
- return ret;
+ goto restore_flags;
}
@@
ret = mipi_dsi_dcs_set_display_on(dsi);
if (ret < 0) {
dev_err(dev, "Failed to set display on: %d\n", ret);
- return ret;
+ goto restore_flags;
}
+ ret = 0;
+restore_flags:
+ dsi->mode_flags = mode_flags;
return ret;
}Apply the same pattern in td4328_off().
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch`
around lines 99 - 149, td4328_on() and td4328_off() set/clear MIPI_DSI_MODE_LPM
but don't restore the original dsi->mode_flags on error paths; modify both
functions (td4328_on and td4328_off) to save the original mode_flags at the
start (e.g., orig = dsi->mode_flags) and on any early return due to failure
restore dsi->mode_flags = orig before returning, ensuring the flag state is
always restored on all error paths.
| + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH 0>, | ||
| + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH 0>; | ||
| + interrupt-names = "hc_irq", "pwr_irq"; |
There was a problem hiding this comment.
Inconsistent interrupt specifier cell count.
The odin2.dts sdhc_2 node uses 4-cell interrupt specifiers:
interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH 0>,
<GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH 0>;
While odin2portal.dts at lines 2057-2058 uses 3-cell specifiers:
interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
The standard GIC_SPI interrupt specifier is 3 cells. Please verify and make consistent.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0016-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`
around lines 1713 - 1715, The interrupt specifiers are inconsistent: the
odin2.dts sdhc_2 node uses 4-cell entries (<GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH 0>)
while odin2portal.dts uses the standard 3-cell form (<GIC_SPI 207
IRQ_TYPE_LEVEL_HIGH>); update the sdhc_2 node to use 3-cell GIC_SPI specifiers
by removing the trailing zero from each interrupts entry so both nodes use
<GIC_SPI N IRQ_TYPE_LEVEL_HIGH>, and verify interrupt-names ("hc_irq","pwr_irq")
still align with the two entries.
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed", | ||
| + (u32)dsp_addr, *dsp_data); | ||
| break; | ||
| case AW88166_DSP_32_DATA: | ||
| ret = aw_dev_dsp_read_32bit(aw_dev, dsp_addr, dsp_data); | ||
| if (ret) | ||
| - dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit failed", (u32)dsp_addr); | ||
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", | ||
| + (u32)dsp_addr, *dsp_data); |
There was a problem hiding this comment.
Fix typo in error log ("32r-bit").
🛠️ Proposed fix
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed",
+ dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed",
(u32)dsp_addr, *dsp_data);📝 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.
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed", | |
| + (u32)dsp_addr, *dsp_data); | |
| break; | |
| case AW88166_DSP_32_DATA: | |
| ret = aw_dev_dsp_read_32bit(aw_dev, dsp_addr, dsp_data); | |
| if (ret) | |
| - dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit failed", (u32)dsp_addr); | |
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", | |
| + (u32)dsp_addr, *dsp_data); | |
| dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed", | |
| (u32)dsp_addr, *dsp_data); | |
| break; | |
| case AW88166_DSP_32_DATA: | |
| ret = aw_dev_dsp_read_32bit(aw_dev, dsp_addr, dsp_data); | |
| if (ret) | |
| dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", | |
| (u32)dsp_addr, *dsp_data); |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 31 - 39, The error log in the AW88166_DSP_32_DATA case has a typo
("32r-bit") and should match the 16-bit message format; update the dev_err call
in the AW88166_DSP_32_DATA branch (where aw_dev_dsp_read_32bit is called) to use
"32-bit dsp_data[0x%x] failed" and keep the same arguments ((u32)dsp_addr,
*dsp_data) so the message reads "read dsp_addr[0x%x] 32-bit dsp_data[0x%x]
failed".
| + __be16 reg_val; | ||
| + | ||
| + mutex_lock(&aw_dev->dsp_lock); | ||
| + /* i2c write */ | ||
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base); | ||
| + if (ret) | ||
| + goto error_operation; | ||
| + for (i = 0; i < len; i += 2) { | ||
| + reg_val = cpu_to_be16p((u16 *)(data + i)); | ||
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, | ||
| + (u16)reg_val); | ||
| + if (ret) | ||
| + goto error_operation; | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the patch file
find . -name "0017-HACK-Revert-back-to-6.12-aw88166-driver.patch" -type fRepository: armbian/build
Length of output: 142
🏁 Script executed:
# Read the patch file to examine the code at lines 72-85 and surrounding context
wc -l patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patchRepository: armbian/build
Length of output: 144
🏁 Script executed:
# Read the patch file to understand the complete function and context
cat -n patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch | sed -n '50,110p'Repository: armbian/build
Length of output: 2115
🏁 Script executed:
# Search for the function that contains this code - look for the function definition
cat -n patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch | sed -n '1,80p'Repository: armbian/build
Length of output: 3342
🏁 Script executed:
# Look for the function definition - search backwards from line 72
grep -n "^-\|^+\|^@@" patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch | head -20Repository: armbian/build
Length of output: 964
🏁 Script executed:
# Search for callers of aw_dev_dsp_update_container
rg "aw_dev_dsp_update_container" --type cRepository: armbian/build
Length of output: 39
🏁 Script executed:
# Search for information about cpu_to_be16p in the codebase or check kernel behavior
# Also look for get_unaligned_be16 usage as comparison
rg "cpu_to_be16p|get_unaligned_be16" --type cRepository: armbian/build
Length of output: 39
🏁 Script executed:
# Let me search within the patch file for function definitions and callers
grep -n "aw_dev_dsp_update_container" patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patchRepository: armbian/build
Length of output: 125
🏁 Script executed:
# Look at more of the patch to understand the full function and how it's called
cat -n patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch | sed -n '40,95p'Repository: armbian/build
Length of output: 2051
🌐 Web query:
cpu_to_be16p kernel unaligned access safety
💡 Result:
cpu_to_be16p(p) (and the other *_to_*p() “pointer” byteorder helpers) are intended to be usable on unaligned pointers, but may incur a performance penalty on some architectures; for unaligned data, the kernel explicitly recommends using the unaligned-access helpers instead. [1]
So, for portable + clearly-unaligned-safe code, prefer:
get_unaligned_be16(p)(orbe16_to_cpu(get_unaligned((__be16 *)p))in older-style code), andput_unaligned_be16(val, p)for stores,
because the kernel documentation states the standard way to avoid unaligned access problems is get_unaligned() / put_unaligned() (and friends). [2]
Rule of thumb: use cpu_to_be16p() only when you know the pointer is suitably aligned (or you accept possible arch-dependent slow paths); use get_unaligned_be16() when alignment is uncertain. [1][2]
Sources: [1] Linux include/linux/byteorder/generic.h comment on ...p macros and unaligned data, [2] Linux “Unaligned Memory Accesses” documentation.
🌐 Web query:
get_unaligned_be16 linux kernel unaligned memory access
💡 Result:
get_unaligned_be16() is the Linux-kernel helper for safely reading a 16-bit big-endian value from memory that may be unaligned (i.e., the pointer isn’t guaranteed to be 2-byte aligned). It exists to avoid alignment faults (or slow fixups) on architectures that can’t efficiently do unaligned loads. [1]
Typical use
`#include` <linux/unaligned.h>
u16 v = get_unaligned_be16(p); /* p points to 2 bytes in big-endian order */What it does
On architectures where unaligned loads aren’t assumed efficient, the kernel often implements it by byte assembling, equivalent to:
(u16)(p[0] << 8 | p[1])so it never performs a potentially-faulting unaligned *(u16 *)p load. [2]
On architectures that do advertise efficient unaligned access (CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS), the generic unaligned framework may select a more direct implementation, but the API contract stays the same: works even if p is unaligned. [3]
Why you’d use it
Kernel docs explicitly recommend get_unaligned()/put_unaligned() (and the endian-specific variants like get_unaligned_be16) as the portable way to avoid unaligned accesses when parsing “off-the-wire” / packed data. [1]
Sources: Linux kernel unaligned-access documentation [1], example byte-shift implementation of get_unaligned_be16 [2], and how the kernel selects implementations based on endianness / unaligned-access capability [3].
Add length validation and use safe unaligned access for DSP writes.
The 16-bit write loop reads data + i as u16 * without alignment guarantees or odd-length checks. Per kernel documentation, cpu_to_be16p() is not recommended for unaligned data; use get_unaligned_be16() instead. Additionally, if len is odd, the final iteration will read beyond the buffer.
Suggested fix
+ if (len & 0x1)
+ return -EINVAL;
+
for (i = 0; i < len; i += 2) {
- reg_val = cpu_to_be16p((u16 *)(data + i));
+ reg_val = get_unaligned_be16(data + i);
ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG,
(u16)reg_val);Add the required include:
+#include <linux/unaligned.h>🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 72 - 85, The loop doing 16-bit DSP writes must validate buffer
length and use unaligned-safe access: add a pre-check that len is even (return
-EINVAL or jump to error_operation on odd len), replace cpu_to_be16p((u16
*)(data + i)) with get_unaligned_be16(data + i) and write that value, and
include the kernel unaligned header (e.g. `#include` <linux/unaligned.h>). Keep
existing locking and error paths (aw_dev->dsp_lock, regmap_write,
AW88166_DSPMADD_REG, AW88166_DSPMDAT_REG, error_operation) unchanged except for
the new length check and using get_unaligned_be16 instead of cpu_to_be16p.
Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`:
- Around line 45-66: The calls to clk_prepare_enable(pdata->i2s_clk) and
clk_disable_unprepare(pdata->i2s_clk) must be guarded against NULL because
devm_clk_get_optional() may return NULL; update the startup path (where
clk_prepare_enable is called in the PRIMARY_MI2S_RX case) and the shutdown path
(sc8280xp_snd_shutdown where clk_disable_unprepare is called) to check
pdata->i2s_clk for non-NULL before calling clk_prepare_enable or
clk_disable_unprepare so we avoid dereferencing a NULL clock pointer.
♻️ Duplicate comments (17)
patch/kernel/archive/sm8550-6.18/0014-HACK-fix-usb-boot-hang.patch (1)
19-30: Scope the xHCI readiness skip to the affected controller only.
Removing the CNR handshake and warning globally can affect other xHCI controllers; gate the bypass by quirk/PCI ID and keep the default path for everyone else.patch/kernel/archive/sm8550-6.18/0007-mmc-sdhci-msm-Limit-HS-mode-frequency-to-37.5MHz.patch (1)
44-56: Ensure 37.5 MHz exists in the OPP table whenqcom,use-level-shifteris set.
If the OPP table lacks 37.5 MHz,dev_pm_opp_set_rate()will fail and HS mode will be rejected on affected devices.patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch (1)
34-53: Startup path still needs error handling and cleanup.
The return values ofclk_prepare_enable()/snd_soc_dai_set_fmt()aren’t checked, andi2s_clkisn’t disabled ifqcom_snd_sdw_startup()fails.patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch (1)
99-149: Restore DSI mode flags on all exit paths.Line 105 sets
MIPI_DSI_MODE_LPMand Lines 112/118/136/143 can return early without restoring the originalmode_flags, leaving the DSI device in the wrong mode after failures.🐛 Suggested fix (apply same pattern to
td4328_off())static int td4328_on(struct td4328 *ctx) { struct mipi_dsi_device *dsi = ctx->dsi; struct device *dev = &dsi->dev; + unsigned long mode_flags = dsi->mode_flags; int ret; dsi->mode_flags |= MIPI_DSI_MODE_LPM; @@ ret = mipi_dsi_dcs_exit_sleep_mode(dsi); if (ret < 0) { dev_err(dev, "Failed to exit sleep mode: %d\n", ret); - return ret; + goto restore_flags; } @@ ret = mipi_dsi_dcs_set_display_on(dsi); if (ret < 0) { dev_err(dev, "Failed to set display on: %d\n", ret); - return ret; + goto restore_flags; } - return 0; + ret = 0; +restore_flags: + dsi->mode_flags = mode_flags; + return ret; }patch/kernel/archive/sm8550-6.18/0017-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (2)
1591-1705: Odin2 DTS points at Portal firmware paths.Line 1593 and Lines 1688–1704 reference
.../odin2portal/...paths inside the Odin2 DTS. Unless Odin2 and Odin2 Portal intentionally share identical firmware directories, these should be.../odin2/...(or a shared label) to avoid loading the wrong blobs.🐛 Example fix (apply to ADSP/CDSP/amp paths too)
&gpu { zap-shader { - firmware-name = "qcom/sm8550/ayn/odin2portal/a740_zap.mbn"; + firmware-name = "qcom/sm8550/ayn/odin2/a740_zap.mbn"; }; };
1720-1725: Interrupt specifier cell count mismatch.Line 1724–1725 uses 4-cell
GIC_SPIspecifiers, while the Portal DTS at Line 2068–2069 uses the standard 3-cell form. These should be consistent with the GIC binding.🐛 Suggested fix
- interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH 0>, - <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH 0>; + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch (6)
184-195: Skip the subcommand byte when building the MCU version string.Line 185 copies from
FRAME_POS_DATA_1, which includes the CMD_COMMOD subcommand. The version string should start atFRAME_POS_DATA_2.🐛 Suggested fix
- if (payload_length >= 1) { + if (payload_length >= 2) { char mcu_version[MCU_VERSION_MAX_LEN] = {0}; - size_t version_length = payload_length; + size_t version_length = payload_length - 1; if (version_length > MCU_VERSION_MAX_LEN - 1) { version_length = MCU_VERSION_MAX_LEN - 1; } - memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length); + memcpy(mcu_version, &data[FRAME_POS_DATA_2], version_length);
209-237: Makeprev_statesper-device instead of static.Line 211 declares
static unsigned long prev_states, which leaks state across device instances. Move it intostruct rsinput_driverand referencedrv->prev_stateshere.
281-311: RX path assumes full frame per callback.Lines 295–311 copy a single callback into
rx_bufand validate immediately, which drops fragmented UART frames. Buffer and assemble frames across callbacks before checksum validation.
306-308: Avoid sleeping in the serdev receive callback.Line 307 calls
rsinput_init_commands()(which sleeps) insidereceive_buf. Defer to a workqueue or threaded context.🐛 Suggested fix
- if (computed_checksum != received_checksum) { - rsinput_init_commands(drv); - goto error; - } + if (computed_checksum != received_checksum) { + schedule_work(&drv->reinit_work); + goto error; + }Additional changes needed (outside this hunk):
- Add
struct work_struct reinit_work;tostruct rsinput_driver.- Initialize with
INIT_WORK(&drv->reinit_work, rsinput_reinit_work);in probe.- Implement
rsinput_reinit_work()to callrsinput_init_commands().
333-352: Return on optional GPIO errors to avoid deref of error pointers.Lines 333–352 log
IS_ERR()but continue; laterif (drv->*_gpio)treats error pointers as valid and dereferences them. Return on error usingdev_err_probe().🐛 Suggested fix (apply to boot/enable/reset)
drv->boot_gpio = devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); if (IS_ERR(drv->boot_gpio)) { error = PTR_ERR(drv->boot_gpio); - dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); + return dev_err_probe(&serdev->dev, error, "Unable to get boot gpio\n"); }
370-423: Set serdev ops before open and close on all error paths.Line 370 opens the device before
serdev_device_set_client_ops(), and several early returns after open (Lines 381/387/420) leak the open handle.🐛 Suggested fix
- error = serdev_device_open(serdev); + serdev_device_set_client_ops(serdev, &rsinput_rx_ops); + error = serdev_device_open(serdev); if (error) return dev_err_probe(&serdev->dev, error, "Unable to open UART device\n"); @@ error = serdev_device_set_baudrate(serdev, 115200); if (error < 0) - return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n"); + goto err_close; @@ drv->input = devm_input_allocate_device(&serdev->dev); - if (!drv->input) - return -ENOMEM; + if (!drv->input) { + error = -ENOMEM; + goto err_close; + } @@ error = input_register_device(drv->input); if (error) - return error; - - serdev_device_set_client_ops(serdev, &rsinput_rx_ops); + goto err_close; @@ error = rsinput_init_commands(drv); - if (error < 0) { - serdev_device_close(serdev); - return error; - } + if (error < 0) + goto err_close; return 0; + +err_close: + serdev_device_close(serdev); + return error;patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch (2)
35-39: Fix typo in error log ("32r-bit").Line 38 has a typo in the 32-bit DSP read error message.
🛠️ Suggested fix
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", (u32)dsp_addr, *dsp_data);
72-85: Guard odd-length writes and use unaligned-safe access.Lines 79–82 cast
data + itou16 *without alignment guarantees and don’t check oddlen. This can read past the buffer and fault on strict-alignment arches.🐛 Suggested fix
+ if (len & 0x1) + return -EINVAL; + for (i = 0; i < len; i += 2) { - reg_val = cpu_to_be16p((u16 *)(data + i)); + reg_val = get_unaligned_be16(data + i); ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, (u16)reg_val);Additional include needed (outside this hunk):
+#include <linux/unaligned.h>patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (3)
18-21: Add<linux/ctype.h>forisxdigit().Line 71 (btqca.c) and Line 209 (ath12k/mac.c) use
isxdigit()but no ctype include is present, which breaks the build on stricter compilers.🐛 Suggested fix
`#include` "btqca.h" +#include <linux/ctype.h>`#include` "mac.h" +#include <linux/ctype.h>Also applies to: 153-154
116-135: Keep the NVM fallback when serial-derived BD_ADDR fails.Lines 118–129 hard-fail if serial generation or setting fails, removing the previous fallback path and risking BT bring-up failures when socinfo is unavailable.
🐛 Suggested fallback
/* Generate BD_ADDR from qcom_serial_number */ err = generate_bdaddr_from_serial(hdev, &generated_bdaddr); if (err) { - bt_dev_err(hdev, "Failed to generate BD_ADDR from serial number"); - return err; + bt_dev_warn(hdev, "Serial BD_ADDR unavailable, falling back to NVM"); + err = qca_check_bdaddr(hdev, &config); + if (err) + return err; + } else { + /* Set the generated BD_ADDR */ + err = qca_set_bdaddr(hdev, &generated_bdaddr); + if (err) { + bt_dev_err(hdev, "Failed to set serial BD_ADDR"); + err = qca_check_bdaddr(hdev, &config); + if (err) + return err; + } else { + bacpy(&hdev->public_addr, &generated_bdaddr); + bacpy(&hdev->bdaddr, &generated_bdaddr); + bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr); + } }
252-268: Preserve per‑pdev MACs and avoid duplicates.Lines 252–268 override valid per‑pdev MACs and don’t offset by
pdev_idxon the serial‑derived path, so multi‑radio devices can get duplicate MACs.🐛 Suggested fix
- ret = generate_macaddr_from_serial(ar, &generated_macaddr); - if (ret) { - if (ar->ab->pdevs_macaddr_valid) { - ether_addr_copy(ar->mac_addr, pdev->mac_addr); - } else { - ether_addr_copy(ar->mac_addr, ar->ab->mac_addr); - ar->mac_addr[4] += ar->pdev_idx; - } - } else { - ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); - ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b); - } + if (ar->ab->pdevs_macaddr_valid) { + ether_addr_copy(ar->mac_addr, pdev->mac_addr); + } else { + ret = generate_macaddr_from_serial(ar, &generated_macaddr); + if (!ret) { + ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); + ar->mac_addr[4] += ar->pdev_idx; + ath12k_info(ab, "MAC_ADDR set to %pM", ar->mac_addr); + } else { + ether_addr_copy(ar->mac_addr, ar->ab->mac_addr); + ar->mac_addr[4] += ar->pdev_idx; + } + }
| + clk_prepare_enable(pdata->i2s_clk); | ||
| + snd_soc_dai_set_fmt(cpu_dai, fmt); | ||
| + snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | ||
| + break; | ||
| + default: | ||
| + break; | ||
| + } | ||
| + | ||
| + return qcom_snd_sdw_startup(substream); | ||
| +} | ||
| + | ||
| static void sc8280xp_snd_shutdown(struct snd_pcm_substream *substream) | ||
| { | ||
| struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); | ||
| @@ -75,6 +99,14 @@ static void sc8280xp_snd_shutdown(struct snd_pcm_substream *substream) | ||
| struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); | ||
| struct sdw_stream_runtime *sruntime = qcom_snd_sdw_get_stream(substream); | ||
|
|
||
| + switch (cpu_dai->id) { | ||
| + case PRIMARY_MI2S_RX: | ||
| + clk_disable_unprepare(pdata->i2s_clk); | ||
| + break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch | head -100Repository: armbian/build
Length of output: 3892
🏁 Script executed:
# Search for how devm_clk_get_optional is used in similar code
rg "devm_clk_get_optional" -A 5 -B 2 | head -50Repository: armbian/build
Length of output: 4377
🏁 Script executed:
# Search for how clk_prepare_enable is typically used with optional clocks
rg "clk_prepare_enable" -B 5 -A 2 patch/kernel/archive/ | grep -A 7 "devm_clk_get_optional" | head -40Repository: armbian/build
Length of output: 1406
🏁 Script executed:
# Let me check the Linux kernel documentation or actual implementation behavior
# by looking for patterns where clk_prepare_enable is guarded with NULL check
rg "if.*clk.*\)" -A 1 patch/kernel/archive/ | grep -B 1 "clk_prepare_enable" | head -30Repository: armbian/build
Length of output: 1891
🏁 Script executed:
# Check what the Linux kernel clock API documentation says about NULL handling
# Let me search for kernel source behavior with optional clocks
web_search "clk_prepare_enable NULL pointer kernel" 2>&1 | head -50Repository: armbian/build
Length of output: 105
🌐 Web query:
kernel clk_prepare_enable NULL deref devm_clk_get_optional
💡 Result:
devm_clk_get_optional(dev, id) returns NULL when the clock simply doesn’t exist (it turns -ENOENT into NULL). (kernel.org)
clk_prepare_enable(clk) itself does not check for NULL; it blindly calls clk_prepare(clk) then clk_enable(clk). (sbexr.rabexc.org)
In mainline Common Clock Framework (CCF), this is still intended to be safe because clk_prepare() (and similarly clk_enable()) is implemented to treat a NULL clk as a no-op (if (!clk) return 0;). (android.googlesource.com)
So if you’re seeing a NULL dereference in/under clk_prepare_enable() after devm_clk_get_optional():
- Most common cause: you didn’t handle the non-ENOENT error case.
devm_clk_get_optional()can returnERR_PTR(...)for real failures (e.g.,-EPROBE_DEFER), and passing that intoclk_prepare_enable()can crash. (Fix: always doif (IS_ERR(clk)) return PTR_ERR(clk);before using it.) (kernel.org) - Other cause (vendor/old trees): your clock implementation may not follow CCF’s “NULL is ok” behavior; then you must guard it yourself:
if (clk) clk_prepare_enable(clk);.
If your kernel has it, the simplest pattern is to use the managed helper that does the enable step for you and still returns NULL when the clock is absent: devm_clk_get_optional_enabled(). (kernel.org)
Citations:
- 1: https://www.kernel.org/doc/html/v6.13/core-api/kernel-api.html?utm_source=openai
- 2: https://sbexr.rabexc.org/latest/sources/46/b9a8bab5b925ec.html
- 3: https://android.googlesource.com/kernel/common/%2B/ASB-2018-05-05_4.4-n/drivers/clk/clk.c?utm_source=openai
- 4: https://www.kernel.org/doc/html/v6.13/core-api/kernel-api.html?utm_source=openai
- 5: https://www.kernel.org/doc/html/v6.13/core-api/kernel-api.html?utm_source=openai
Guard optional i2s_clk before enable/disable to prevent NULL dereference.
devm_clk_get_optional() returns NULL when the clock is unavailable. While mainline kernels implement clk_prepare_enable() and clk_disable_unprepare() with NULL checks, vendor/proprietary kernel trees (like this Snapdragon 8550 tree) may not follow this pattern. Add defensive NULL guards:
case PRIMARY_MI2S_RX:
- clk_prepare_enable(pdata->i2s_clk);
+ if (pdata->i2s_clk)
+ clk_prepare_enable(pdata->i2s_clk);
snd_soc_dai_set_fmt(cpu_dai, fmt);
snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
break;
---
case PRIMARY_MI2S_RX:
- clk_disable_unprepare(pdata->i2s_clk);
+ if (pdata->i2s_clk)
+ clk_disable_unprepare(pdata->i2s_clk);
break;Also applies to: 87-89
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`
around lines 45 - 66, The calls to clk_prepare_enable(pdata->i2s_clk) and
clk_disable_unprepare(pdata->i2s_clk) must be guarded against NULL because
devm_clk_get_optional() may return NULL; update the startup path (where
clk_prepare_enable is called in the PRIMARY_MI2S_RX case) and the shutdown path
(sc8280xp_snd_shutdown where clk_disable_unprepare is called) to check
pdata->i2s_clk for non-NULL before calling clk_prepare_enable or
clk_disable_unprepare so we avoid dereferencing a NULL clock pointer.
1 participant
Description
This PR includes initial support for running Linux kernel 6.18 on Ayn odin2 device
Documentation summary for feature / change
Basically this is a porting of existing 6.12 patches to 6.18. Dropped some ones which are included in 6.18 already.
Verified following functions are working:
Not working:
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.