Re: [PATCH v5 3/6] HID: himax: Add DRM panel follower support
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-03 17:56:11
Also in:
linux-devicetree
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] input: unbounded resource leaks and permanent initialization failures on DRM panel prepare retries
- [High] hid: destroying and recreating the HID device on panel suspend/resume breaks userspace input continuity
- [High] input: masking of -EPROBE_DEFER breaks DRM panel follower probe deferral
- [Medium] hid: missing hardware boot delay after reset pin deassertion in himax_chip_resume()
--
commit eb8f38c8024df3c3e0a8931cb1c98f90a31716d2
Author: Allen_Lin [off-list ref]
HID: himax: Add DRM panel follower support
The touch function of the HX83102J is part of a TDDI controller and has
to follow the power sequence of the panel. Register as a DRM panel
follower, and suspend and resume the touch function when the panel is
unprepared and prepared.
Link: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org (local)
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-himax.c b/drivers/hid/hid-himax.c index 1cdb310dec642..653c92829ec7f 100644 --- a/drivers/hid/hid-himax.c +++ b/drivers/hid/hid-himax.c
[ ... ]
quoted hunk ↗ jump to hunk
+static int himax_chip_suspend(struct himax_ts_data *ts) +{ + himax_int_enable(ts, false); + gpiod_set_value(ts->pdata.gpiod_rst, 1); + himax_power_set(ts, false); + himax_hid_remove(ts);
[Severity: High] Does tearing down the HID device here break userspace input continuity? When the panel unblanks and blanks, destroying and recreating the HID device causes userspace compositors (like X11 or Wayland) to permanently lose track of the touch device or drop existing input grabs.
quoted hunk ↗ jump to hunk
+ + return 0; +} + +/** + * himax_chip_resume() - Setup flags, I/O and resume + * @ts: Himax touch screen data + * + * This function is used to resume the touch screen. It will set the resume + * success flag to false, and disable reset pin. Then call the himax_resume_proc() + * to process detailed resume procedure. + * + * Return: 0 on success, negative error code on failure + */ +static int himax_chip_resume(struct himax_ts_data *ts) +{ + if (himax_power_set(ts, true)) + return -EIO; + gpiod_set_value(ts->pdata.gpiod_rst, 0); + himax_resume_proc(ts);
[Severity: Medium] Is there a missing hardware boot delay after deasserting the reset pin? The hardware seems to require a 20ms delay after the reset pin is pulled low (as seen with usleep_range(20000, 20100) in himax_pin_reset()), but here gpiod_set_value(..., 0) is immediately followed by SPI communication in himax_resume_proc(). Could this cause intermittent hardware failures, SPI timeouts, or ignored commands on resume?
+ himax_hid_probe(ts);
[Severity: High] This recreates the HID device on panel resume, which as mentioned above, breaks userspace input continuity.
quoted hunk ↗ jump to hunk
+ himax_int_enable(ts, true); + + return 0; +}
[ ... ]
quoted hunk ↗ jump to hunk
+static int __himax_initial_power_up(struct himax_ts_data *ts) +{ + int ret; + + ret = himax_platform_init(ts); + if (ret) { + dev_err(ts->dev, "%s: platform init failed\n", __func__); + return ret; + } + + ret = himax_chip_detect(ts); + if (ret) { + dev_err(ts->dev, "%s: IC detect failed\n", __func__); + return ret; + }
[Severity: High] Does this error path leak resources? If __himax_initial_power_up() fails midway (e.g., himax_chip_detect() fails due to IC boot time), it returns without cleaning up the resources allocated in himax_platform_init() (like the kzalloc'd ts->xfer_buf). Since the panel follower mechanism will retry asynchronously when the panel is toggled via himax_panel_prepared(), the retry will overwrite the ts->xfer_buf pointer and permanently leak memory. Additionally, since himax_ts_register_interrupt() uses devm_request_threaded_irq() and the probe function already completed, devm allocations are not rolled back on asynchronous errors. A subsequent retry will fail permanently with -EBUSY because the IRQ is already devres-claimed. Also, if the driver unbinds, himax_spi_drv_remove() checks if (ts->probe_finish) and skips himax_platform_deinit(), leaving ts->xfer_buf permanently leaked. [ ... ]
quoted hunk ↗ jump to hunk
@@ -1660,25 +1932,13 @@ static int himax_spi_drv_probe(struct spi_device *spi) ts->initialized = false; ts->ic_boot_done = false; - ret = himax_platform_init(ts); - if (ret) { - dev_err(ts->dev, "%s: platform init failed\n", __func__); - return ret; - } - - ret = himax_chip_detect(ts); + ret = himax_initial_power_up(ts); if (ret) { - dev_err(ts->dev, "%s: IC detect failed\n", __func__); - return ret; + dev_err(ts->dev, "%s: initial power up failed\n", __func__); + return -ENODEV;
[Severity: High] Does returning -ENODEV here break DRM panel follower probe deferral? If the display panel driver is not yet ready, drm_panel_add_follower() (called via himax_initial_power_up()) correctly returns -EPROBE_DEFER. Overwriting this return value with -ENODEV permanently breaks standard probe deferral logic.
}
-- Sashiko AI review · https://sashiko.dev/#/patchset/20261003142741.48634-1-michal@nozomi.space?part=3