Re: [PATCH v2] input: touchscreen: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

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 <[email protected]>

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.

> 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

[ ... ]

> @@ -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/[email protected]?part=1