diff mbox series

[v2] firewire: ohci: Convert to generic power management

Message ID 20220607212157.343033-1-helgaas@kernel.org
State New
Headers show
Series [v2] firewire: ohci: Convert to generic power management | expand

Commit Message

Bjorn Helgaas June 7, 2022, 9:21 p.m. UTC 
From: Vaibhav Gupta <vaibhavgupta40@gmail.com>

Convert firewire-ohci from legacy PCI power management to the generic power
management framework.

Previously firewire-ohci used legacy PCI power management, and
pci_suspend() and pci_resume() were responsible for both device-specific
things and generic PCI things like saving and restoring config space and
managing power state:

  pci_suspend
    software_reset                         <-- device-specific
    pci_save_state                         <-- generic PCI
    pci_set_power_state                    <-- generic PCI
    pmac_ohci_off                          <-- device-specific

  pci_resume
    pmac_ohci_on                           <-- device-specific
    pci_set_power_state(PCI_D0)            <-- generic PCI
    pci_restore_state                      <-- generic PCI
    pci_enable_device                      <-- generic PCI
    ohci_enable                            <-- device-specific

Convert to generic power management where the PCI bus PM methods do the
generic PCI things, and the driver needs only the device-specific part,
i.e.,

  suspend_devices_and_enter
    dpm_suspend_start(PMSG_SUSPEND)
      pci_pm_suspend                       # PCI bus .suspend() method
        pci_suspend                        # driver->pm->suspend
          software_reset                   <-- device-specific
          pmac_ohci_off                    <-- device-specific
    suspend_enter
      dpm_suspend_noirq(PMSG_SUSPEND)
        pci_pm_suspend_noirq               # PCI bus .suspend_noirq() method
          pci_save_state                   <-- generic PCI
          pci_prepare_to_sleep             <-- generic PCI
            pci_set_power_state
    ...
    dpm_resume_end(PMSG_RESUME)
      pci_pm_resume                        # PCI bus .resume() method
        pci_restore_standard_config
          pci_set_power_state(PCI_D0)      <-- generic PCI
          pci_restore_state                <-- generic PCI
        pci_resume                         # driver->pm->resume
          pmac_ohci_on                     <-- device-specific
          ohci_enable                      <-- device-specific

[bhelgaas: commit log]
Link: https://lore.kernel.org/r/20200720150715.624520-1-vaibhavgupta40@gmail.com
Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/firewire/ohci.c | 35 ++++++++---------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

Comments

Bjorn Helgaas June 8, 2022, 2:24 p.m. UTC | #1 
On Tue, Jun 07, 2022 at 04:21:57PM -0500, Bjorn Helgaas wrote:
> From: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> 
> Convert firewire-ohci from legacy PCI power management to the generic power
> management framework.
> 
> Previously firewire-ohci used legacy PCI power management, and
> pci_suspend() and pci_resume() were responsible for both device-specific
> things and generic PCI things like saving and restoring config space and
> managing power state:
> 
>   pci_suspend
>     software_reset                         <-- device-specific
>     pci_save_state                         <-- generic PCI
>     pci_set_power_state                    <-- generic PCI
>     pmac_ohci_off                          <-- device-specific
> 
>   pci_resume
>     pmac_ohci_on                           <-- device-specific
>     pci_set_power_state(PCI_D0)            <-- generic PCI
>     pci_restore_state                      <-- generic PCI
>     pci_enable_device                      <-- generic PCI
>     ohci_enable                            <-- device-specific
> 
> Convert to generic power management where the PCI bus PM methods do the
> generic PCI things, and the driver needs only the device-specific part,
> i.e.,
> 
>   suspend_devices_and_enter
>     dpm_suspend_start(PMSG_SUSPEND)
>       pci_pm_suspend                       # PCI bus .suspend() method
>         pci_suspend                        # driver->pm->suspend
>           software_reset                   <-- device-specific
>           pmac_ohci_off                    <-- device-specific
>     suspend_enter
>       dpm_suspend_noirq(PMSG_SUSPEND)
>         pci_pm_suspend_noirq               # PCI bus .suspend_noirq() method
>           pci_save_state                   <-- generic PCI
>           pci_prepare_to_sleep             <-- generic PCI
>             pci_set_power_state
>     ...
>     dpm_resume_end(PMSG_RESUME)
>       pci_pm_resume                        # PCI bus .resume() method
>         pci_restore_standard_config
>           pci_set_power_state(PCI_D0)      <-- generic PCI
>           pci_restore_state                <-- generic PCI
>         pci_resume                         # driver->pm->resume
>           pmac_ohci_on                     <-- device-specific
>           ohci_enable                      <-- device-specific

I want to point out that this changes the ordering of pmac_ohci_off()
with respect to pci_save_state() and pci_set_power_state().
Previously we did those as:

  pci_save_state
  pci_set_power_state
  pmac_ohci_off

and after this patch it would be:

  pmac_ohci_off
  pci_save_state
  pci_set_power_state

The ordering of pmac_ohci_on() in the resume path is similarly
changed.

I don't know what pmac_ohci_off() and pmac_ohci_on() do, so I don't
know whether this is an issue.  In general, drivers don't need to do
anything after their device is put in D3hot, but maybe firewire-ohci
is unique in this way.

> [bhelgaas: commit log]
> Link: https://lore.kernel.org/r/20200720150715.624520-1-vaibhavgupta40@gmail.com
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/firewire/ohci.c | 35 ++++++++---------------------------
>  1 file changed, 8 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 17c9d825188b..aee705132330 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -3165,8 +3165,7 @@ static int ohci_set_iso_channels(struct fw_iso_context *base, u64 *channels)
>  	return ret;
>  }
>  
> -#ifdef CONFIG_PM
> -static void ohci_resume_iso_dma(struct fw_ohci *ohci)
> +static void __maybe_unused ohci_resume_iso_dma(struct fw_ohci *ohci)
>  {
>  	int i;
>  	struct iso_context *ctx;
> @@ -3183,7 +3182,6 @@ static void ohci_resume_iso_dma(struct fw_ohci *ohci)
>  			ohci_start_iso(&ctx->base, 0, ctx->sync, ctx->tags);
>  	}
>  }
> -#endif
>  
>  static int queue_iso_transmit(struct iso_context *ctx,
>  			      struct fw_iso_packet *packet,
> @@ -3789,39 +3787,24 @@ static void pci_remove(struct pci_dev *dev)
>  	dev_notice(&dev->dev, "removed fw-ohci device\n");
>  }
>  
> -#ifdef CONFIG_PM
> -static int pci_suspend(struct pci_dev *dev, pm_message_t state)
> +static int __maybe_unused pci_suspend(struct device *devp)
>  {
> +	struct pci_dev *dev = to_pci_dev(devp);
>  	struct fw_ohci *ohci = pci_get_drvdata(dev);
> -	int err;
>  
>  	software_reset(ohci);
> -	err = pci_save_state(dev);
> -	if (err) {
> -		ohci_err(ohci, "pci_save_state failed\n");
> -		return err;
> -	}
> -	err = pci_set_power_state(dev, pci_choose_state(dev, state));
> -	if (err)
> -		ohci_err(ohci, "pci_set_power_state failed with %d\n", err);
>  	pmac_ohci_off(dev);
>  
>  	return 0;
>  }
>  
> -static int pci_resume(struct pci_dev *dev)
> +static int __maybe_unused pci_resume(struct device *devp)
>  {
> +	struct pci_dev *dev = to_pci_dev(devp);
>  	struct fw_ohci *ohci = pci_get_drvdata(dev);
>  	int err;
>  
>  	pmac_ohci_on(dev);
> -	pci_set_power_state(dev, PCI_D0);
> -	pci_restore_state(dev);
> -	err = pci_enable_device(dev);
> -	if (err) {
> -		ohci_err(ohci, "pci_enable_device failed\n");
> -		return err;
> -	}
>  
>  	/* Some systems don't setup GUID register on resume from ram  */
>  	if (!reg_read(ohci, OHCI1394_GUIDLo) &&
> @@ -3838,7 +3821,6 @@ static int pci_resume(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static const struct pci_device_id pci_table[] = {
>  	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_FIREWIRE_OHCI, ~0) },
> @@ -3847,15 +3829,14 @@ static const struct pci_device_id pci_table[] = {
>  
>  MODULE_DEVICE_TABLE(pci, pci_table);
>  
> +static SIMPLE_DEV_PM_OPS(pci_pm_ops, pci_suspend, pci_resume);
> +
>  static struct pci_driver fw_ohci_pci_driver = {
>  	.name		= ohci_driver_name,
>  	.id_table	= pci_table,
>  	.probe		= pci_probe,
>  	.remove		= pci_remove,
> -#ifdef CONFIG_PM
> -	.resume		= pci_resume,
> -	.suspend	= pci_suspend,
> -#endif
> +	.driver.pm	= &pci_pm_ops,
>  };
>  
>  static int __init fw_ohci_init(void)
> -- 
> 2.25.1
>
diff mbox series

Patch

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 17c9d825188b..aee705132330 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -3165,8 +3165,7 @@  static int ohci_set_iso_channels(struct fw_iso_context *base, u64 *channels)
 	return ret;
 }
 
-#ifdef CONFIG_PM
-static void ohci_resume_iso_dma(struct fw_ohci *ohci)
+static void __maybe_unused ohci_resume_iso_dma(struct fw_ohci *ohci)
 {
 	int i;
 	struct iso_context *ctx;
@@ -3183,7 +3182,6 @@  static void ohci_resume_iso_dma(struct fw_ohci *ohci)
 			ohci_start_iso(&ctx->base, 0, ctx->sync, ctx->tags);
 	}
 }
-#endif
 
 static int queue_iso_transmit(struct iso_context *ctx,
 			      struct fw_iso_packet *packet,
@@ -3789,39 +3787,24 @@  static void pci_remove(struct pci_dev *dev)
 	dev_notice(&dev->dev, "removed fw-ohci device\n");
 }
 
-#ifdef CONFIG_PM
-static int pci_suspend(struct pci_dev *dev, pm_message_t state)
+static int __maybe_unused pci_suspend(struct device *devp)
 {
+	struct pci_dev *dev = to_pci_dev(devp);
 	struct fw_ohci *ohci = pci_get_drvdata(dev);
-	int err;
 
 	software_reset(ohci);
-	err = pci_save_state(dev);
-	if (err) {
-		ohci_err(ohci, "pci_save_state failed\n");
-		return err;
-	}
-	err = pci_set_power_state(dev, pci_choose_state(dev, state));
-	if (err)
-		ohci_err(ohci, "pci_set_power_state failed with %d\n", err);
 	pmac_ohci_off(dev);
 
 	return 0;
 }
 
-static int pci_resume(struct pci_dev *dev)
+static int __maybe_unused pci_resume(struct device *devp)
 {
+	struct pci_dev *dev = to_pci_dev(devp);
 	struct fw_ohci *ohci = pci_get_drvdata(dev);
 	int err;
 
 	pmac_ohci_on(dev);
-	pci_set_power_state(dev, PCI_D0);
-	pci_restore_state(dev);
-	err = pci_enable_device(dev);
-	if (err) {
-		ohci_err(ohci, "pci_enable_device failed\n");
-		return err;
-	}
 
 	/* Some systems don't setup GUID register on resume from ram  */
 	if (!reg_read(ohci, OHCI1394_GUIDLo) &&
@@ -3838,7 +3821,6 @@  static int pci_resume(struct pci_dev *dev)
 
 	return 0;
 }
-#endif
 
 static const struct pci_device_id pci_table[] = {
 	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_FIREWIRE_OHCI, ~0) },
@@ -3847,15 +3829,14 @@  static const struct pci_device_id pci_table[] = {
 
 MODULE_DEVICE_TABLE(pci, pci_table);
 
+static SIMPLE_DEV_PM_OPS(pci_pm_ops, pci_suspend, pci_resume);
+
 static struct pci_driver fw_ohci_pci_driver = {
 	.name		= ohci_driver_name,
 	.id_table	= pci_table,
 	.probe		= pci_probe,
 	.remove		= pci_remove,
-#ifdef CONFIG_PM
-	.resume		= pci_resume,
-	.suspend	= pci_suspend,
-#endif
+	.driver.pm	= &pci_pm_ops,
 };
 
 static int __init fw_ohci_init(void)