[PATCH v1] pinctrl: amd: Fix hibernation support with CONFIG_SUSPEND unset

[PATCH v1] pinctrl: amd: Fix hibernation support with CONFIG_SUSPEND unset

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After recent changes, pinctrl-amd will not support hibernation when
CONFIG_HIBERNATION is set and CONFIG_SUSPEND isn't because it will not
register amd_gpio_pm_ops then.

Address this by restoring dependencies on CONFIG_PM_SLEEP where
necessary for hibernation support.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Mario, this is on top of:

https://patchwork.kernel.org/project/linux-acpi/patch/20250414203551.779320-1-superm1@kernel.org/

which is currently in my bleeding-edge branch only.

---
 drivers/pinctrl/pinctrl-amd.c |   63 ++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 27 deletions(-)

--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -894,26 +894,7 @@
 	}
 }
 
-#ifdef CONFIG_SUSPEND
-static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
-{
-	const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
-
-	if (!pd)
-		return false;
-
-	/*
-	 * Only restore the pin if it is actually in use by the kernel (or
-	 * by userspace).
-	 */
-	if (pd->mux_owner || pd->gpio_owner ||
-	    gpiochip_line_is_irq(&gpio_dev->gc, pin))
-		return true;
-
-	return false;
-}
-
-#ifdef CONFIG_ACPI
+#if defined(CONFIG_SUSPEND) && defined(CONFIG_ACPI)
 static void amd_gpio_check_pending(void)
 {
 	struct amd_gpio *gpio_dev = pinctrl_dev;
@@ -936,8 +917,40 @@
 static struct acpi_s2idle_dev_ops pinctrl_amd_s2idle_dev_ops = {
 	.check = amd_gpio_check_pending,
 };
+
+static void amd_gpio_register_s2idle_ops(void)
+{
+	acpi_register_lps0_dev(&pinctrl_amd_s2idle_dev_ops);
+}
+
+static void amd_gpio_unregister_s2idle_ops(void)
+{
+	acpi_unregister_lps0_dev(&pinctrl_amd_s2idle_dev_ops);
+}
+#else
+static inline void amd_gpio_register_s2idle_ops(void) {}
+static inline void amd_gpio_unregister_s2idle_ops(void) {}
 #endif
 
+#ifdef CONFIG_PM_SLEEP
+static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
+{
+	const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
+
+	if (!pd)
+		return false;
+
+	/*
+	 * Only restore the pin if it is actually in use by the kernel (or
+	 * by userspace).
+	 */
+	if (pd->mux_owner || pd->gpio_owner ||
+	    gpiochip_line_is_irq(&gpio_dev->gc, pin))
+		return true;
+
+	return false;
+}
+
 static int amd_gpio_suspend_hibernate_common(struct device *dev, bool is_suspend)
 {
 	struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
@@ -1211,9 +1224,7 @@
 
 	platform_set_drvdata(pdev, gpio_dev);
 	acpi_register_wakeup_handler(gpio_dev->irq, amd_gpio_check_wake, gpio_dev);
-#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
-	acpi_register_lps0_dev(&pinctrl_amd_s2idle_dev_ops);
-#endif
+	amd_gpio_register_s2idle_ops();
 
 	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
 	return ret;
@@ -1232,9 +1243,7 @@
 
 	gpiochip_remove(&gpio_dev->gc);
 	acpi_unregister_wakeup_handler(amd_gpio_check_wake, gpio_dev);
-#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
-	acpi_unregister_lps0_dev(&pinctrl_amd_s2idle_dev_ops);
-#endif
+	amd_gpio_unregister_s2idle_ops();
 }
 
 #ifdef CONFIG_ACPI
@@ -1251,7 +1260,7 @@
 	.driver		= {
 		.name	= "amd_gpio",
 		.acpi_match_table = ACPI_PTR(amd_gpio_acpi_match),
-#ifdef CONFIG_SUSPEND
+#ifdef CONFIG_PM_SLEEP
 		.pm	= &amd_gpio_pm_ops,
 #endif
 	},

Re: [PATCH v1] pinctrl: amd: Fix hibernation support with CONFIG_SUSPEND unset

[Mario Limonciello]

On 4/30/2025 1:27 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After recent changes, pinctrl-amd will not support hibernation when
> CONFIG_HIBERNATION is set and CONFIG_SUSPEND isn't because it will not
> register amd_gpio_pm_ops then.
> 
> Address this by restoring dependencies on CONFIG_PM_SLEEP where
> necessary for hibernation support.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Mario, this is on top of:
> 
> https://patchwork.kernel.org/project/linux-acpi/patch/20250414203551.779320-1-superm1@kernel.org/
> 
> which is currently in my bleeding-edge branch only.

Thanks!  This makes sense now why there was CONFIG_PM_SLEEP in the first 
place. 🤦

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> ---
>   drivers/pinctrl/pinctrl-amd.c |   63 ++++++++++++++++++++++++------------------
>   1 file changed, 36 insertions(+), 27 deletions(-)
> 
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -894,26 +894,7 @@
>   	}
>   }
>   
> -#ifdef CONFIG_SUSPEND
> -static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
> -{
> -	const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
> -
> -	if (!pd)
> -		return false;
> -
> -	/*
> -	 * Only restore the pin if it is actually in use by the kernel (or
> -	 * by userspace).
> -	 */
> -	if (pd->mux_owner || pd->gpio_owner ||
> -	    gpiochip_line_is_irq(&gpio_dev->gc, pin))
> -		return true;
> -
> -	return false;
> -}
> -
> -#ifdef CONFIG_ACPI
> +#if defined(CONFIG_SUSPEND) && defined(CONFIG_ACPI)
>   static void amd_gpio_check_pending(void)
>   {
>   	struct amd_gpio *gpio_dev = pinctrl_dev;
> @@ -936,8 +917,40 @@
>   static struct acpi_s2idle_dev_ops pinctrl_amd_s2idle_dev_ops = {
>   	.check = amd_gpio_check_pending,
>   };
> +
> +static void amd_gpio_register_s2idle_ops(void)
> +{
> +	acpi_register_lps0_dev(&pinctrl_amd_s2idle_dev_ops);
> +}
> +
> +static void amd_gpio_unregister_s2idle_ops(void)
> +{
> +	acpi_unregister_lps0_dev(&pinctrl_amd_s2idle_dev_ops);
> +}
> +#else
> +static inline void amd_gpio_register_s2idle_ops(void) {}
> +static inline void amd_gpio_unregister_s2idle_ops(void) {}
>   #endif
>   
> +#ifdef CONFIG_PM_SLEEP
> +static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
> +{
> +	const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
> +
> +	if (!pd)
> +		return false;
> +
> +	/*
> +	 * Only restore the pin if it is actually in use by the kernel (or
> +	 * by userspace).
> +	 */
> +	if (pd->mux_owner || pd->gpio_owner ||
> +	    gpiochip_line_is_irq(&gpio_dev->gc, pin))
> +		return true;
> +
> +	return false;
> +}
> +
>   static int amd_gpio_suspend_hibernate_common(struct device *dev, bool is_suspend)
>   {
>   	struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
> @@ -1211,9 +1224,7 @@
>   
>   	platform_set_drvdata(pdev, gpio_dev);
>   	acpi_register_wakeup_handler(gpio_dev->irq, amd_gpio_check_wake, gpio_dev);
> -#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
> -	acpi_register_lps0_dev(&pinctrl_amd_s2idle_dev_ops);
> -#endif
> +	amd_gpio_register_s2idle_ops();
>   
>   	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
>   	return ret;
> @@ -1232,9 +1243,7 @@
>   
>   	gpiochip_remove(&gpio_dev->gc);
>   	acpi_unregister_wakeup_handler(amd_gpio_check_wake, gpio_dev);
> -#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
> -	acpi_unregister_lps0_dev(&pinctrl_amd_s2idle_dev_ops);
> -#endif
> +	amd_gpio_unregister_s2idle_ops();
>   }
>   
>   #ifdef CONFIG_ACPI
> @@ -1251,7 +1260,7 @@
>   	.driver		= {
>   		.name	= "amd_gpio",
>   		.acpi_match_table = ACPI_PTR(amd_gpio_acpi_match),
> -#ifdef CONFIG_SUSPEND
> +#ifdef CONFIG_PM_SLEEP
>   		.pm	= &amd_gpio_pm_ops,
>   #endif
>   	},
> 
> 
> 

Re: [PATCH v1] pinctrl: amd: Fix hibernation support with CONFIG_SUSPEND unset

[Linus Walleij]

On Wed, Apr 30, 2025 at 8:27 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> After recent changes, pinctrl-amd will not support hibernation when
> CONFIG_HIBERNATION is set and CONFIG_SUSPEND isn't because it will not
> register amd_gpio_pm_ops then.
>
> Address this by restoring dependencies on CONFIG_PM_SLEEP where
> necessary for hibernation support.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> Mario, this is on top of:
>
> https://patchwork.kernel.org/project/linux-acpi/patch/20250414203551.779320-1-superm1@kernel.org/
>
> which is currently in my bleeding-edge branch only.

It seems the prerequisites are in your tree so take it there!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij