[RFC,v10,5/7] PCI: Make pci_platform_pm_ops's callbacks optional

Commit Message

Jeffy Chen Oct. 27, 2017, 7:26 a.m. UTC

Allow platforms not to provide some of the pci_platform_pm_ops's
callbacks.

Also change the return value from -ENOSYS to -ENODEV for:
warning: drivers/pci/pci.c,594: ENOSYS means 'invalid syscall nr' and nothing else

Signed-off-by: Jeffy Chen <[e-mail]>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None

 drivers/pci/pci.c | 38 +++++++++++++++++++++++++-------------
 drivers/pci/pci.h |  5 +----
 2 files changed, 26 insertions(+), 17 deletions(-)

Comments

Rafael J. Wysocki Nov. 8, 2017, 10:27 p.m. UTC | #1

On Friday, October 27, 2017 9:26:10 AM CET Jeffy Chen wrote:
> Allow platforms not to provide some of the pci_platform_pm_ops's
> callbacks.

So?

What exactly is wrong with having empty ops in there?

Is it really better to have everyone do extra checks every time an op is
invoked even when all of the ops are present?

> Also change the return value from -ENOSYS to -ENODEV for:
> warning: drivers/pci/pci.c,594: ENOSYS means 'invalid syscall nr' and nothing else

Moving stuff around and changing it at the same time is a bad idea.

Change it in one patch and move it around in another one and you'll be less
likely to make a mistake.  Moreover, reviewing it will be easier too, IMO.

Thanks,
Rafael

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f0d68066c726..e120b00a9017 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -572,46 +572,58 @@  static void pci_restore_bars(struct pci_dev *dev)
 
 static const struct pci_platform_pm_ops *pci_platform_pm;
 
-int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
+void pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
 {
-	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
-	    !ops->choose_state  || !ops->set_wakeup || !ops->need_resume)
-		return -EINVAL;
 	pci_platform_pm = ops;
-	return 0;
 }
 
 static inline bool platform_pci_power_manageable(struct pci_dev *dev)
 {
-	return pci_platform_pm ? pci_platform_pm->is_manageable(dev) : false;
+	if (pci_platform_pm && pci_platform_pm->is_manageable)
+		return pci_platform_pm->is_manageable(dev);
+
+	return false;
 }
 
 static inline int platform_pci_set_power_state(struct pci_dev *dev,
 					       pci_power_t t)
 {
-	return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS;
+	if (pci_platform_pm && pci_platform_pm->set_state)
+		return pci_platform_pm->set_state(dev, t);
+
+	return -ENODEV;
 }
 
 static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
 {
-	return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN;
+	if (pci_platform_pm && pci_platform_pm->get_state)
+		return pci_platform_pm->get_state(dev);
+
+	return PCI_UNKNOWN;
 }
 
 static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
 {
-	return pci_platform_pm ?
-			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
+	if (pci_platform_pm && pci_platform_pm->choose_state)
+		return pci_platform_pm->choose_state(dev);
+
+	return PCI_POWER_ERROR;
 }
 
 static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
 {
-	return pci_platform_pm ?
-			pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
+	if (pci_platform_pm && pci_platform_pm->set_wakeup)
+		return pci_platform_pm->set_wakeup(dev, enable);
+
+	return -ENODEV;
 }
 
 static inline bool platform_pci_need_resume(struct pci_dev *dev)
 {
-	return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
+	if (pci_platform_pm && pci_platform_pm->need_resume)
+		return pci_platform_pm->need_resume(dev);
+
+	return false;
 }
 
 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e816a13e6259..048668271014 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -52,9 +52,6 @@  int pci_probe_reset_function(struct pci_dev *dev);
  * @need_resume: returns 'true' if the given device (which is currently
  *		suspended) needs to be resumed to be configured for system
  *		wakeup.
- *
- * If given platform is generally capable of power managing PCI devices, all of
- * these callbacks are mandatory.
  */
 struct pci_platform_pm_ops {
 	bool (*is_manageable)(struct pci_dev *dev);
@@ -65,7 +62,7 @@  struct pci_platform_pm_ops {
 	bool (*need_resume)(struct pci_dev *dev);
 };
 
-int pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
+void pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_power_up(struct pci_dev *dev);
 void pci_disable_enabled_device(struct pci_dev *dev);