Re: [PATCH v5] gpio: bt8xx: use generic PCI PM

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Oct 13, 2025 at 03:52:59PM +0000, Vaibhav Gupta wrote:
> Switch to the new generic PCI power management framework and remove legacy
> callbacks like .suspend() and .resume(). With the generic framework, the
> standard PCI related work like:
> 	- pci_save/restore_state()
> 	- pci_enable/disable_device()
> 	- pci_set_power_state()
> is handled by the PCI core and this driver should implement only gpio-bt8xx
> specific operations in its respective callback functions.

Tiny nits:

s/use generic PCI PM/use generic power management/  # subject; not PCI
s/new generic PCI power/generic power management/   # no longer "new" :)

> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> ---
>  drivers/gpio/gpio-bt8xx.c | 29 +++++++----------------------
>  1 file changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-bt8xx.c b/drivers/gpio/gpio-bt8xx.c
> index 05401da03ca3..70b49c385b43 100644
> --- a/drivers/gpio/gpio-bt8xx.c
> +++ b/drivers/gpio/gpio-bt8xx.c
> @@ -52,10 +52,8 @@ struct bt8xxgpio {
>  	struct pci_dev *pdev;
>  	struct gpio_chip gpio;
>  
> -#ifdef CONFIG_PM
>  	u32 saved_outen;
>  	u32 saved_data;
> -#endif
>  };
>  
>  #define bgwrite(dat, adr)	writel((dat), bg->mmio+(adr))
> @@ -224,9 +222,9 @@ static void bt8xxgpio_remove(struct pci_dev *pdev)
>  	pci_disable_device(pdev);
>  }
>  
> -#ifdef CONFIG_PM
> -static int bt8xxgpio_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused bt8xxgpio_suspend(struct device *dev)

I think __maybe_unused is unnecessary because of this path:

  DEFINE_SIMPLE_DEV_PM_OPS
    _DEFINE_DEV_PM_OPS
      SYSTEM_SLEEP_PM_OPS
        pm_sleep_ptr
	  PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))

Detailed explanation here:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/util_macros.h?id=v6.17#n86

>  {
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct bt8xxgpio *bg = pci_get_drvdata(pdev);
>  
>  	scoped_guard(spinlock_irqsave, &bg->lock) {

Unrelated to this patch, but it's not clear to me why bg->lock is
needed here (or maybe anywhere).

  $ git grep -l "static int.*_suspend" drivers/gpio | wc -l
  26
  $ git grep -W "static int.*_suspend" drivers/gpio | grep lock | grep -v unlock | wc -l

It looks like only about 8 of the 26 gpio drivers that implement
.suspend() protect it with a lock.  I would expect a lock to be needed
in all of them or none of them.

> @@ -238,23 +236,13 @@ static int bt8xxgpio_suspend(struct pci_dev *pdev, pm_message_t state)
>  		bgwrite(0x0, BT848_GPIO_OUT_EN);
>  	}
>  
> -	pci_save_state(pdev);
> -	pci_disable_device(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> -
>  	return 0;
>  }
>  
> -static int bt8xxgpio_resume(struct pci_dev *pdev)
> +static int __maybe_unused bt8xxgpio_resume(struct device *dev)
>  {
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct bt8xxgpio *bg = pci_get_drvdata(pdev);
> -	int err;
> -
> -	pci_set_power_state(pdev, PCI_D0);
> -	err = pci_enable_device(pdev);
> -	if (err)
> -		return err;
> -	pci_restore_state(pdev);
>  
>  	guard(spinlock_irqsave)(&bg->lock);
>  
> @@ -267,10 +255,8 @@ static int bt8xxgpio_resume(struct pci_dev *pdev)
>  
>  	return 0;
>  }
> -#else
> -#define bt8xxgpio_suspend NULL
> -#define bt8xxgpio_resume NULL
> -#endif /* CONFIG_PM */
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(bt8xxgpio_pm_ops, bt8xxgpio_suspend, bt8xxgpio_resume);
>  
>  static const struct pci_device_id bt8xxgpio_pci_tbl[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_BROOKTREE, PCI_DEVICE_ID_BT848) },
> @@ -286,8 +272,7 @@ static struct pci_driver bt8xxgpio_pci_driver = {
>  	.id_table	= bt8xxgpio_pci_tbl,
>  	.probe		= bt8xxgpio_probe,
>  	.remove		= bt8xxgpio_remove,
> -	.suspend	= bt8xxgpio_suspend,
> -	.resume		= bt8xxgpio_resume,
> +	.driver.pm	= &bt8xxgpio_pm_ops,
>  };
>  
>  module_pci_driver(bt8xxgpio_pci_driver);
> -- 
> 2.51.0
>