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

Mario Limonciello
2025-05-01T21:55:55Z

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
> },
>
>
>