Re: [PATCH update] PM: Introduce core framework for run-time PM of I/O devices (rev. 15)
From: Magnus Damm <magnus.damm@gmail.com>
Date: 2009-08-12 10:37:15
Hi Rafael, On Mon, Aug 10, 2009 at 6:13 AM, Rafael J. Wysocki[off-list ref] wrote:
From: Rafael J. Wysocki <redacted> Subject: PM: Introduce core framework for run-time PM of I/O devices (rev. 15) Introduce a core framework for run-time power management of I/O devices. Add device run-time PM fields to 'struct dev_pm_info' and device run-time PM callbacks to 'struct dev_pm_ops'. Introduce a run-time PM workqueue and define some device run-time PM helper functions at the core level. Document all these things. Special thanks to Alan Stern for his help with the design and multiple detailed reviews of the pereceding versions of this patch and to Magnus Damm for testing feedback. Signed-off-by: Rafael J. Wysocki <redacted>
Looking good! I have a few nitpicks below, but from a functional perspective it's all good. I've tested v15 with platform device drivers for I2C, UIO and framebuffer. Before adding my "Acked-by" I also want to test the V4L capture driver, but I need to wait a few days until I can get my hands on such a hardware platform. Thanks for folding in and fixing up the debug patch. I was able to drop most remaining patches thanks to feedback from Alan. So the only needed patch apart from this one (and the ones in your linux-next branch) is the one in this micro-series: "PM: Runtime PM v15 for Platform Devices 20090812".
quoted hunk
--- linux-2.6.orig/include/linux/pm.h +++ linux-2.6/include/linux/pm.h
[..]
struct dev_pm_info {
pm_message_t power_state;
- unsigned can_wakeup:1;
- unsigned should_wakeup:1;
+ unsigned int can_wakeup:1;
+ unsigned int should_wakeup:1;
enum dpm_state status; /* Owned by the PM core */
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM_SLEEP
struct list_head entry;
#endif
+#ifdef CONFIG_PM_RUNTIME
+ struct timer_list suspend_timer;
+ unsigned long timer_expires;
+ struct work_struct work;
+ wait_queue_head_t wait_queue;
+ spinlock_t lock;
+ atomic_t usage_count;
+ atomic_t child_count;I suppose child_count has to be atomic?
quoted hunk
--- /dev/null +++ linux-2.6/drivers/base/power/runtime.c
[...]
+int __pm_runtime_suspend(struct device *dev, bool from_wq) + __releases(&dev->power.lock) __acquires(&dev->power.lock)
[...]
quoted hunk
+ if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) { + spin_unlock_irq(&dev->power.lock); + + retval = dev->bus->pm->runtime_suspend(dev); + + spin_lock_irq(&dev->power.lock); + dev->power.runtime_error = retval; + } else { + retval = -ENOSYS; + }
Nit: { and } above do not follow the regular coding style.
+int __pm_runtime_resume(struct device *dev, bool from_wq) + __releases(&dev->power.lock) __acquires(&dev->power.lock)
[...]
quoted hunk
+ if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) { + spin_unlock_irq(&dev->power.lock); + + retval = dev->bus->pm->runtime_resume(dev); + + spin_lock_irq(&dev->power.lock); + dev->power.runtime_error = retval; + } else { + retval = -ENOSYS; + }
Same minor issue here. Apart from that all is fine. Thank you. / magnus