Re: [PATCH v2] input: touchscreen: Convert to DEFINE_SIMPLE_DEV_PM_OPS()
From: sashiko-bot@kernel.org
Date: 2026-08-21 04:19:51
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] The logic in `ad7879_toggle()` is completely inverted, causing the sysfs `disable` attribute to malfunction and desynchronize the hardware state from software tracking. --
--- Patch [1]: [PATCH v2] input: touchscreen: Convert to DEFINE_SIMPLE_DEV_PM_OPS() ---commit b2cacedba4aaf53cb97b0b4bbb302d78a3a37daf Author: Triet Hoang [off-list ref] input: touchscreen: Convert to DEFINE_SIMPLE_DEV_PM_OPS() This commit converts the deprecated SIMPLE_DEV_PM_OPS to DEFINE_SIMPLE_DEV_PM_OPS and pm_sleep_ptr() across several touchscreen drivers. This allows dropping the __maybe_unused annotations and reduces kernel size when PM features are disabled.
quoted hunk ↗ jump to hunk
diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c index 31d2a3029d5f6..10f8574f478e2 100644 --- a/drivers/input/touchscreen/ad7879.c +++ b/drivers/input/touchscreen/ad7879.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -329,8 +329,7 @@ static int __maybe_unused ad7879_resume(struct device *dev) return 0; } -SIMPLE_DEV_PM_OPS(ad7879_pm_ops, ad7879_suspend, ad7879_resume); -EXPORT_SYMBOL(ad7879_pm_ops); +EXPORT_SIMPLE_DEV_PM_OPS(ad7879_pm_ops, ad7879_suspend, ad7879_resume); static void ad7879_toggle(struct ad7879 *ts, bool disable) {
[Severity: High]
This is a pre-existing issue, but the logic in this function appears to be
completely inverted.
Looking at the full ad7879_toggle() function in
drivers/input/touchscreen/ad7879.c:
static void ad7879_toggle(struct ad7879 *ts, bool disable)
{
guard(mutex)(&ts->input->mutex);
if (!ts->suspended && input_device_enabled(ts->input)) {
if (disable) {
if (ts->disabled)
__ad7879_enable(ts);
} else {
if (!ts->disabled)
__ad7879_disable(ts);
}
}
ts->disabled = disable;
}
If a user writes 1 to the disable sysfs attribute (disable == true), and the
device is already marked disabled (ts->disabled == true), won't this logic
actually enable the hardware by calling __ad7879_enable(ts)?
Conversely, if the user writes 0 to enable the device (disable == false)
while the device is currently enabled (ts->disabled == false), won't this
unexpectedly disable the hardware?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260821040905.104590-1-triet.hoang.dev@gmail.com?part=1