Re: [PATCH v1 1/4] acpi/x86: s2idle: add support for screen off and screen on callbacks

[Mario Limonciello <mario.limonciello@amd.com>]

On 9/19/2024 12:19, Antheas Kapenekakis wrote:
> The screen off and screen on firmware functions are meant to signify
> the system entering a state where the user is not actively interacting
> with it (i.e., in Windows this state is called "Screen Off" and the
> system enters it once it turns the screen off e.g., due to inactivity).
> 
> In this state, the kernel and userspace are fully active, and the user
> might still be interacting with the system somehow (such as with
> listening to music or having a hotspot). Userspace is supposed to
> minimize non-essential activities, but this is not required.
> In addition, there is no requirement of suspending post the screen off
> call. If the user interacts with the system, the kernel should call
> screen on and resume normal operation.
> 
> This patch adds a set of callbacks to allow calling the screen on/off
> callbacks outside of the suspend/resume path. It is based on
> Mario Limonciello's patch on the superm1/dsm-screen-on-off branch.

Based on?  It's nearly an identical patch [1].  The screen_off/screen_on 
lines in struct platform_s2idle_ops are just placed in a different location.

IMO there should be more attribution here, either a Co-developed-by tag 
or sending my patch directly and adding your S-o-b to it.

Link: 
https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/commit/?h=superm1/dsm-screen-on-off&id=7b80581428315f973410dccf0402a86266fb0d9a 
[1]

> However, the intent here is completely different.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   include/linux/suspend.h |  5 +++++
>   kernel/power/suspend.c  | 12 ++++++++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index da6ebca3ff77..96ceaad07839 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -132,6 +132,7 @@ struct platform_suspend_ops {
>   };
>   
>   struct platform_s2idle_ops {
> +	int (*screen_off)(void);
>   	int (*begin)(void);
>   	int (*prepare)(void);
>   	int (*prepare_late)(void);
> @@ -140,6 +141,7 @@ struct platform_s2idle_ops {
>   	void (*restore_early)(void);
>   	void (*restore)(void);
>   	void (*end)(void);
> +	int (*screen_on)(void);
>   };
>   
>   #ifdef CONFIG_SUSPEND
> @@ -160,6 +162,9 @@ extern unsigned int pm_suspend_global_flags;
>   #define PM_SUSPEND_FLAG_FW_RESUME	BIT(1)
>   #define PM_SUSPEND_FLAG_NO_PLATFORM	BIT(2)
>   
> +int platform_suspend_screen_off(void);
> +int platform_suspend_screen_on(void);
> +
>   static inline void pm_suspend_clear_flags(void)
>   {
>   	pm_suspend_global_flags = 0;
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 09f8397bae15..19734b297527 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -254,6 +254,18 @@ static bool sleep_state_supported(suspend_state_t state)
>   	       (valid_state(state) && !cxl_mem_active());
>   }
>   
> +int platform_suspend_screen_off(void)
> +{
> +	return s2idle_ops && s2idle_ops->screen_off ? s2idle_ops->screen_off() : 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_suspend_screen_off);
> +
> +int platform_suspend_screen_on(void)
> +{
> +	return s2idle_ops && s2idle_ops->screen_on ? s2idle_ops->screen_on() : 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_suspend_screen_on);
> +
>   static int platform_suspend_prepare(suspend_state_t state)
>   {
>   	return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare ?