| Message ID | 20091012155041.GA1071@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | d35ef90bf9e7cab9aa85e9c0724bd1ac6f784601 |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
On Mon, Oct 12, 2009 at 8:50 AM, Anton Vorontsov <avorontsov@ru.mvista.com> wrote: > Linux power management subsystem supports vast amount of new PM > callbacks that are crucial for proper suspend and hibernation support > in drivers. > > This patch implements support for dev_pm_ops, preserving support > for legacy callbacks. > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> Hmmm... I'm not very familiar with the PM callbacks, but change doesn't look right to me. In particular, a lot of these new hooks don't do anything remotely of_platform bus specific. For example, of_platform_pm_prepare() checks if there is drv, drv->pm, and drv->pm->prepare. If all are true, then it calls drv->pm->prepare(). I see that the platform bus platform_pm_prepare() function is absolutely identical. I haven't looked, but I wouldn't be surprised if other busses do the same. I think these simple pm ops should be made library functions that platform, of_platform and other simple busses can just populate their pm ops structure with. g. > --- > drivers/of/platform.c | 305 ++++++++++++++++++++++++++++++++++++++++++++++--- > 1 files changed, 290 insertions(+), 15 deletions(-) > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 298de0f..d58ade1 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -65,47 +65,322 @@ static int of_platform_device_remove(struct device *dev) > return 0; > } > > -static int of_platform_device_suspend(struct device *dev, pm_message_t state) > +static void of_platform_device_shutdown(struct device *dev) > { > struct of_device *of_dev = to_of_device(dev); > struct of_platform_driver *drv = to_of_platform_driver(dev->driver); > - int error = 0; > > - if (dev->driver && drv->suspend) > - error = drv->suspend(of_dev, state); > - return error; > + if (dev->driver && drv->shutdown) > + drv->shutdown(of_dev); > } > > -static int of_platform_device_resume(struct device * dev) > +#ifdef CONFIG_PM_SLEEP > + > +static int of_platform_legacy_suspend(struct device *dev, pm_message_t mesg) > { > struct of_device *of_dev = to_of_device(dev); > struct of_platform_driver *drv = to_of_platform_driver(dev->driver); > - int error = 0; > + int ret = 0; > > - if (dev->driver && drv->resume) > - error = drv->resume(of_dev); > - return error; > + if (dev->driver && drv->suspend) > + ret = drv->suspend(of_dev, mesg); > + return ret; > } > > -static void of_platform_device_shutdown(struct device *dev) > +static int of_platform_legacy_resume(struct device *dev) > { > struct of_device *of_dev = to_of_device(dev); > struct of_platform_driver *drv = to_of_platform_driver(dev->driver); > + int ret = 0; > > - if (dev->driver && drv->shutdown) > - drv->shutdown(of_dev); > + if (dev->driver && drv->resume) > + ret = drv->resume(of_dev); > + return ret; > +} > + > +static int of_platform_pm_prepare(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (drv && drv->pm && drv->pm->prepare) > + ret = drv->pm->prepare(dev); > + > + return ret; > +} > + > +static void of_platform_pm_complete(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + > + if (drv && drv->pm && drv->pm->complete) > + drv->pm->complete(dev); > +} > + > +#ifdef CONFIG_SUSPEND > + > +static int of_platform_pm_suspend(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->suspend) > + ret = drv->pm->suspend(dev); > + } else { > + ret = of_platform_legacy_suspend(dev, PMSG_SUSPEND); > + } > + > + return ret; > } > > +static int of_platform_pm_suspend_noirq(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->suspend_noirq) > + ret = drv->pm->suspend_noirq(dev); > + } > + > + return ret; > +} > + > +static int of_platform_pm_resume(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->resume) > + ret = drv->pm->resume(dev); > + } else { > + ret = of_platform_legacy_resume(dev); > + } > + > + return ret; > +} > + > +static int of_platform_pm_resume_noirq(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->resume_noirq) > + ret = drv->pm->resume_noirq(dev); > + } > + > + return ret; > +} > + > +#else /* !CONFIG_SUSPEND */ > + > +#define of_platform_pm_suspend NULL > +#define of_platform_pm_resume NULL > +#define of_platform_pm_suspend_noirq NULL > +#define of_platform_pm_resume_noirq NULL > + > +#endif /* !CONFIG_SUSPEND */ > + > +#ifdef CONFIG_HIBERNATION > + > +static int of_platform_pm_freeze(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->freeze) > + ret = drv->pm->freeze(dev); > + } else { > + ret = of_platform_legacy_suspend(dev, PMSG_FREEZE); > + } > + > + return ret; > +} > + > +static int of_platform_pm_freeze_noirq(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->freeze_noirq) > + ret = drv->pm->freeze_noirq(dev); > + } > + > + return ret; > +} > + > +static int of_platform_pm_thaw(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->thaw) > + ret = drv->pm->thaw(dev); > + } else { > + ret = of_platform_legacy_resume(dev); > + } > + > + return ret; > +} > + > +static int of_platform_pm_thaw_noirq(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->thaw_noirq) > + ret = drv->pm->thaw_noirq(dev); > + } > + > + return ret; > +} > + > +static int of_platform_pm_poweroff(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->poweroff) > + ret = drv->pm->poweroff(dev); > + } else { > + ret = of_platform_legacy_suspend(dev, PMSG_HIBERNATE); > + } > + > + return ret; > +} > + > +static int of_platform_pm_poweroff_noirq(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->poweroff_noirq) > + ret = drv->pm->poweroff_noirq(dev); > + } > + > + return ret; > +} > + > +static int of_platform_pm_restore(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->restore) > + ret = drv->pm->restore(dev); > + } else { > + ret = of_platform_legacy_resume(dev); > + } > + > + return ret; > +} > + > +static int of_platform_pm_restore_noirq(struct device *dev) > +{ > + struct device_driver *drv = dev->driver; > + int ret = 0; > + > + if (!drv) > + return 0; > + > + if (drv->pm) { > + if (drv->pm->restore_noirq) > + ret = drv->pm->restore_noirq(dev); > + } > + > + return ret; > +} > + > +#else /* !CONFIG_HIBERNATION */ > + > +#define of_platform_pm_freeze NULL > +#define of_platform_pm_thaw NULL > +#define of_platform_pm_poweroff NULL > +#define of_platform_pm_restore NULL > +#define of_platform_pm_freeze_noirq NULL > +#define of_platform_pm_thaw_noirq NULL > +#define of_platform_pm_poweroff_noirq NULL > +#define of_platform_pm_restore_noirq NULL > + > +#endif /* !CONFIG_HIBERNATION */ > + > +static struct dev_pm_ops of_platform_dev_pm_ops = { > + .prepare = of_platform_pm_prepare, > + .complete = of_platform_pm_complete, > + .suspend = of_platform_pm_suspend, > + .resume = of_platform_pm_resume, > + .freeze = of_platform_pm_freeze, > + .thaw = of_platform_pm_thaw, > + .poweroff = of_platform_pm_poweroff, > + .restore = of_platform_pm_restore, > + .suspend_noirq = of_platform_pm_suspend_noirq, > + .resume_noirq = of_platform_pm_resume_noirq, > + .freeze_noirq = of_platform_pm_freeze_noirq, > + .thaw_noirq = of_platform_pm_thaw_noirq, > + .poweroff_noirq = of_platform_pm_poweroff_noirq, > + .restore_noirq = of_platform_pm_restore_noirq, > +}; > + > +#define OF_PLATFORM_PM_OPS_PTR (&of_platform_dev_pm_ops) > + > +#else /* !CONFIG_PM_SLEEP */ > + > +#define OF_PLATFORM_PM_OPS_PTR NULL > + > +#endif /* !CONFIG_PM_SLEEP */ > + > int of_bus_type_init(struct bus_type *bus, const char *name) > { > bus->name = name; > bus->match = of_platform_bus_match; > bus->probe = of_platform_device_probe; > bus->remove = of_platform_device_remove; > - bus->suspend = of_platform_device_suspend; > - bus->resume = of_platform_device_resume; > bus->shutdown = of_platform_device_shutdown; > bus->dev_attrs = of_platform_device_attrs; > + bus->pm = OF_PLATFORM_PM_OPS_PTR; > return bus_register(bus); > } > > -- > 1.6.3.3 >
On Mon, Oct 12, 2009 at 03:09:53PM -0700, Grant Likely wrote: > On Mon, Oct 12, 2009 at 8:50 AM, Anton Vorontsov > <avorontsov@ru.mvista.com> wrote: > > Linux power management subsystem supports vast amount of new PM > > callbacks that are crucial for proper suspend and hibernation support > > in drivers. > > > > This patch implements support for dev_pm_ops, preserving support > > for legacy callbacks. > > > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> > > Hmmm... I'm not very familiar with the PM callbacks, but change > doesn't look right to me. In particular, a lot of these new hooks > don't do anything remotely of_platform bus specific. For example, > of_platform_pm_prepare() checks if there is drv, drv->pm, and > drv->pm->prepare. If all are true, then it calls drv->pm->prepare(). > I see that the platform bus platform_pm_prepare() function is > absolutely identical. I haven't looked, but I wouldn't be surprised > if other busses do the same. > > I think these simple pm ops should be made library functions that > platform, of_platform and other simple busses can just populate their > pm ops structure with. Some hooks can be made as library functions, but some can't (for example that do of_plaform_driver->suspend(), as opposite to of_platform_driver->driver.suspend(), i.e. the legacy hooks). Also, if you look into PCI bus hooks, you'll see that these hooks aren't pure proxies for drivers, they do real work, so they won't like to reuse or share anything. For OF platfrom bus, we can share these functions with platform: of_platform_pm_suspend_noirq of_platform_pm_resume_noirq of_platform_pm_freeze_noirq of_platform_pm_thaw_noirq of_platform_pm_poweroff_noirq of_platform_pm_restore_noirq These we cannot share: of_platform_pm_suspend of_platform_pm_resume of_platform_pm_freeze of_platform_pm_thaw of_platform_pm_poweroff of_platform_pm_restore I agree that there is some room for improvements in general (e.g. merging platform and of_platform devices/drivers), but it's not as easy as you would like to think. Let's make it in a separate step that don't stop real features from being implemented (e.g. hibernate). For the six functions that we can reuse I can prepare a cleanup patch that we can merge via -mm, or it can just sit and collect needed acks and can be merged via any tree. But please, no cross-tree dependencies for the cruicial features. Thanks,
On Tue, 2009-10-13 at 02:44 +0400, Anton Vorontsov wrote: > I agree that there is some room for improvements in general (e.g. > merging platform and of_platform devices/drivers), but it's not as > easy as you would like to think. Let's make it in a separate step > that don't stop real features from being implemented (e.g. > hibernate). > > For the six functions that we can reuse I can prepare a cleanup > patch that we can merge via -mm, or it can just sit and collect > needed acks and can be merged via any tree. But please, no > cross-tree dependencies for the cruicial features. I agree. I'll take the patch for now. In the long run, I'm all for killing of_platform if we can find a "proper" way to replace it with platform. IE. With dev_archdata, any device carries the of device node, so of_platform doesn't really buy us much anymore. We could even "default" by populating platform device resources with standard-parsing of "reg" properties etc... So for devices who don't actually need anything more, we may get away re-using platform devices as-is, all we would need is some kind of conversion table or such to map OF match to platform dev names, or maybe a secondary match table in the drivers themselves. Anyway, that's an old discussion, something we still need to sort out... Ben.
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 298de0f..d58ade1 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -65,47 +65,322 @@ static int of_platform_device_remove(struct device *dev) return 0; } -static int of_platform_device_suspend(struct device *dev, pm_message_t state) +static void of_platform_device_shutdown(struct device *dev) { struct of_device *of_dev = to_of_device(dev); struct of_platform_driver *drv = to_of_platform_driver(dev->driver); - int error = 0; - if (dev->driver && drv->suspend) - error = drv->suspend(of_dev, state); - return error; + if (dev->driver && drv->shutdown) + drv->shutdown(of_dev); } -static int of_platform_device_resume(struct device * dev) +#ifdef CONFIG_PM_SLEEP + +static int of_platform_legacy_suspend(struct device *dev, pm_message_t mesg) { struct of_device *of_dev = to_of_device(dev); struct of_platform_driver *drv = to_of_platform_driver(dev->driver); - int error = 0; + int ret = 0; - if (dev->driver && drv->resume) - error = drv->resume(of_dev); - return error; + if (dev->driver && drv->suspend) + ret = drv->suspend(of_dev, mesg); + return ret; } -static void of_platform_device_shutdown(struct device *dev) +static int of_platform_legacy_resume(struct device *dev) { struct of_device *of_dev = to_of_device(dev); struct of_platform_driver *drv = to_of_platform_driver(dev->driver); + int ret = 0; - if (dev->driver && drv->shutdown) - drv->shutdown(of_dev); + if (dev->driver && drv->resume) + ret = drv->resume(of_dev); + return ret; +} + +static int of_platform_pm_prepare(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (drv && drv->pm && drv->pm->prepare) + ret = drv->pm->prepare(dev); + + return ret; +} + +static void of_platform_pm_complete(struct device *dev) +{ + struct device_driver *drv = dev->driver; + + if (drv && drv->pm && drv->pm->complete) + drv->pm->complete(dev); +} + +#ifdef CONFIG_SUSPEND + +static int of_platform_pm_suspend(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->suspend) + ret = drv->pm->suspend(dev); + } else { + ret = of_platform_legacy_suspend(dev, PMSG_SUSPEND); + } + + return ret; } +static int of_platform_pm_suspend_noirq(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->suspend_noirq) + ret = drv->pm->suspend_noirq(dev); + } + + return ret; +} + +static int of_platform_pm_resume(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->resume) + ret = drv->pm->resume(dev); + } else { + ret = of_platform_legacy_resume(dev); + } + + return ret; +} + +static int of_platform_pm_resume_noirq(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->resume_noirq) + ret = drv->pm->resume_noirq(dev); + } + + return ret; +} + +#else /* !CONFIG_SUSPEND */ + +#define of_platform_pm_suspend NULL +#define of_platform_pm_resume NULL +#define of_platform_pm_suspend_noirq NULL +#define of_platform_pm_resume_noirq NULL + +#endif /* !CONFIG_SUSPEND */ + +#ifdef CONFIG_HIBERNATION + +static int of_platform_pm_freeze(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->freeze) + ret = drv->pm->freeze(dev); + } else { + ret = of_platform_legacy_suspend(dev, PMSG_FREEZE); + } + + return ret; +} + +static int of_platform_pm_freeze_noirq(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->freeze_noirq) + ret = drv->pm->freeze_noirq(dev); + } + + return ret; +} + +static int of_platform_pm_thaw(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->thaw) + ret = drv->pm->thaw(dev); + } else { + ret = of_platform_legacy_resume(dev); + } + + return ret; +} + +static int of_platform_pm_thaw_noirq(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->thaw_noirq) + ret = drv->pm->thaw_noirq(dev); + } + + return ret; +} + +static int of_platform_pm_poweroff(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->poweroff) + ret = drv->pm->poweroff(dev); + } else { + ret = of_platform_legacy_suspend(dev, PMSG_HIBERNATE); + } + + return ret; +} + +static int of_platform_pm_poweroff_noirq(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->poweroff_noirq) + ret = drv->pm->poweroff_noirq(dev); + } + + return ret; +} + +static int of_platform_pm_restore(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->restore) + ret = drv->pm->restore(dev); + } else { + ret = of_platform_legacy_resume(dev); + } + + return ret; +} + +static int of_platform_pm_restore_noirq(struct device *dev) +{ + struct device_driver *drv = dev->driver; + int ret = 0; + + if (!drv) + return 0; + + if (drv->pm) { + if (drv->pm->restore_noirq) + ret = drv->pm->restore_noirq(dev); + } + + return ret; +} + +#else /* !CONFIG_HIBERNATION */ + +#define of_platform_pm_freeze NULL +#define of_platform_pm_thaw NULL +#define of_platform_pm_poweroff NULL +#define of_platform_pm_restore NULL +#define of_platform_pm_freeze_noirq NULL +#define of_platform_pm_thaw_noirq NULL +#define of_platform_pm_poweroff_noirq NULL +#define of_platform_pm_restore_noirq NULL + +#endif /* !CONFIG_HIBERNATION */ + +static struct dev_pm_ops of_platform_dev_pm_ops = { + .prepare = of_platform_pm_prepare, + .complete = of_platform_pm_complete, + .suspend = of_platform_pm_suspend, + .resume = of_platform_pm_resume, + .freeze = of_platform_pm_freeze, + .thaw = of_platform_pm_thaw, + .poweroff = of_platform_pm_poweroff, + .restore = of_platform_pm_restore, + .suspend_noirq = of_platform_pm_suspend_noirq, + .resume_noirq = of_platform_pm_resume_noirq, + .freeze_noirq = of_platform_pm_freeze_noirq, + .thaw_noirq = of_platform_pm_thaw_noirq, + .poweroff_noirq = of_platform_pm_poweroff_noirq, + .restore_noirq = of_platform_pm_restore_noirq, +}; + +#define OF_PLATFORM_PM_OPS_PTR (&of_platform_dev_pm_ops) + +#else /* !CONFIG_PM_SLEEP */ + +#define OF_PLATFORM_PM_OPS_PTR NULL + +#endif /* !CONFIG_PM_SLEEP */ + int of_bus_type_init(struct bus_type *bus, const char *name) { bus->name = name; bus->match = of_platform_bus_match; bus->probe = of_platform_device_probe; bus->remove = of_platform_device_remove; - bus->suspend = of_platform_device_suspend; - bus->resume = of_platform_device_resume; bus->shutdown = of_platform_device_shutdown; bus->dev_attrs = of_platform_device_attrs; + bus->pm = OF_PLATFORM_PM_OPS_PTR; return bus_register(bus); }
Linux power management subsystem supports vast amount of new PM callbacks that are crucial for proper suspend and hibernation support in drivers. This patch implements support for dev_pm_ops, preserving support for legacy callbacks. Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> --- drivers/of/platform.c | 305 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 290 insertions(+), 15 deletions(-)