| Message ID | 1393237368-15500-1-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 February 2014 00:00, Kevin Hilman <khilman@linaro.org> wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> While fetching the proper runtime PM callback, we walk the hierarchy of >> device's power domains, subsystems and drivers. >> >> This is common for rpm_suspend(), rpm_idle() and rpm_resume(). Let's >> clean up the code by using a macro that handles this. >> >> Cc: Kevin Hilman <khilman@linaro.org> >> Cc: Alan Stern <stern@rowland.harvard.edu> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Mark Brown <broonie@kernel.org> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Cc: Alessandro Rubini <rubini@unipv.it> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> >> Changes in v2: >> Updated the macro to return a callback instead. >> Suggested by Josh Cartwright. >> >> --- >> drivers/base/power/runtime.c | 63 ++++++++++++++++-------------------------- >> 1 file changed, 24 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c >> index 72e00e6..cc7d1ed 100644 >> --- a/drivers/base/power/runtime.c >> +++ b/drivers/base/power/runtime.c >> @@ -13,6 +13,27 @@ >> #include <trace/events/rpm.h> >> #include "power.h" >> >> +#define RPM_GET_CALLBACK(dev, cb) \ >> +({ \ >> + int (*__rpm_cb)(struct device *__d); \ >> + \ >> + if (dev->pm_domain) \ >> + __rpm_cb = dev->pm_domain->ops.cb; \ >> + else if (dev->type && dev->type->pm) \ >> + __rpm_cb = dev->type->pm->cb; \ >> + else if (dev->class && dev->class->pm) \ >> + __rpm_cb = dev->class->pm->cb; \ >> + else if (dev->bus && dev->bus->pm) \ >> + __rpm_cb = dev->bus->pm->cb; \ >> + else \ >> + __rpm_cb = NULL; \ >> + \ >> + if (!__rpm_cb && dev->driver && dev->driver->pm) \ >> + __rpm_cb = dev->driver->pm->cb; \ >> + \ >> + __rpm_cb; \ >> +}) > > So the main question from v1 remains: why use a macro, and not a function? > I am no big fan of macros, but in this case I thought it make sense. Using _a_ function would not be enough, since we would need three, one for each runtime PM callback (suspend, idle, resume), right? I am happy to change to whatever you guys thinks best, I have no strong opinion. Kind regards Uffe > Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 February 2014 16:04, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 27 Feb 2014, Ulf Hansson wrote: > >> > So the main question from v1 remains: why use a macro, and not a function? >> > >> >> I am no big fan of macros, but in this case I thought it make sense. >> Using _a_ function would not be enough, since we would need three, one >> for each runtime PM callback (suspend, idle, resume), right? >> >> I am happy to change to whatever you guys thinks best, I have no strong opinion. > > A reasonable compromise would be to define the macro, and then use it > in those three new functions (and nowhere else). Okay, let me send a v3 that tries this approach then. > > The existing rpm_idle, rpm_suspend, and rpm_resume routines should then > be changed to use the new functions. And of course, the new functions > could be called directly by subsystems or PM domains. Not sure we should export these functions as a part of this patchset. Would it not be preferred, to first see if there are any that needs it? Kind regards Ulf Hansson > > Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 February 2014 17:22, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 27 Feb 2014, Ulf Hansson wrote: > >> > A reasonable compromise would be to define the macro, and then use it >> > in those three new functions (and nowhere else). >> >> Okay, let me send a v3 that tries this approach then. >> >> > >> > The existing rpm_idle, rpm_suspend, and rpm_resume routines should then >> > be changed to use the new functions. And of course, the new functions >> > could be called directly by subsystems or PM domains. >> >> Not sure we should export these functions as a part of this patchset. >> Would it not be preferred, to first see if there are any that needs >> it? > > I don't understand. V2 of the patchset exports > pm_runtime_force_suspend and pm_runtime_force_resume. Why wouldn't you > want to export them in V3? There are some confusion here. :-) pm_runtime_force_suspend|resume() surely need to be exported, but that's "patch v2 2/8". I think we were debating whether this patch "patch v2 1/8" should use a macro to walk the ladder to fetch the runtime PM callback - or if we should implement a three c-functions to handle it. I prefer to keep this patch as is, thus using the macro. Kind regards Ulf Hansson > > Alan Stern >
On 27 February 2014 22:25, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 27 Feb 2014, Ulf Hansson wrote: > >> On 27 February 2014 17:22, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Thu, 27 Feb 2014, Ulf Hansson wrote: >> > >> >> > A reasonable compromise would be to define the macro, and then use it >> >> > in those three new functions (and nowhere else). >> >> >> >> Okay, let me send a v3 that tries this approach then. >> >> >> >> > >> >> > The existing rpm_idle, rpm_suspend, and rpm_resume routines should then >> >> > be changed to use the new functions. And of course, the new functions >> >> > could be called directly by subsystems or PM domains. >> >> >> >> Not sure we should export these functions as a part of this patchset. >> >> Would it not be preferred, to first see if there are any that needs >> >> it? >> > >> > I don't understand. V2 of the patchset exports >> > pm_runtime_force_suspend and pm_runtime_force_resume. Why wouldn't you >> > want to export them in V3? >> >> There are some confusion here. :-) pm_runtime_force_suspend|resume() >> surely need to be exported, but that's "patch v2 2/8". >> >> I think we were debating whether this patch "patch v2 1/8" should use >> a macro to walk the ladder to fetch the runtime PM callback - or if we >> should implement a three c-functions to handle it. I prefer to keep >> this patch as is, thus using the macro. > > But you're going to expand the macro (say, the version that handles > runtime_resume callbacks) in two places: rpm_resume and > pm_runtime_force_resume. That's inefficient. The macro generates > fairly complicated code, and so it should be expanded in only one > place: a single new function. The new function can be called by both > rpm_resume and pm_runtime_force_resume. That's a valid point. Maybe you were trying to tell me this before as well, not sure. Anyway, l will send a v3 to address your comments. Kind regards Ulf Hansson > > In fact, from comparing those two routines, it looks like the new > function could include a little more than just the macro expansion. > > Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 72e00e6..cc7d1ed 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -13,6 +13,27 @@ #include <trace/events/rpm.h> #include "power.h" +#define RPM_GET_CALLBACK(dev, cb) \ +({ \ + int (*__rpm_cb)(struct device *__d); \ + \ + if (dev->pm_domain) \ + __rpm_cb = dev->pm_domain->ops.cb; \ + else if (dev->type && dev->type->pm) \ + __rpm_cb = dev->type->pm->cb; \ + else if (dev->class && dev->class->pm) \ + __rpm_cb = dev->class->pm->cb; \ + else if (dev->bus && dev->bus->pm) \ + __rpm_cb = dev->bus->pm->cb; \ + else \ + __rpm_cb = NULL; \ + \ + if (!__rpm_cb && dev->driver && dev->driver->pm) \ + __rpm_cb = dev->driver->pm->cb; \ + \ + __rpm_cb; \ +}) + static int rpm_resume(struct device *dev, int rpmflags); static int rpm_suspend(struct device *dev, int rpmflags); @@ -310,19 +331,7 @@ static int rpm_idle(struct device *dev, int rpmflags) dev->power.idle_notification = true; - if (dev->pm_domain) - callback = dev->pm_domain->ops.runtime_idle; - else if (dev->type && dev->type->pm) - callback = dev->type->pm->runtime_idle; - else if (dev->class && dev->class->pm) - callback = dev->class->pm->runtime_idle; - else if (dev->bus && dev->bus->pm) - callback = dev->bus->pm->runtime_idle; - else - callback = NULL; - - if (!callback && dev->driver && dev->driver->pm) - callback = dev->driver->pm->runtime_idle; + callback = RPM_GET_CALLBACK(dev, runtime_idle); if (callback) retval = __rpm_callback(callback, dev); @@ -492,19 +501,7 @@ static int rpm_suspend(struct device *dev, int rpmflags) __update_runtime_status(dev, RPM_SUSPENDING); - if (dev->pm_domain) - callback = dev->pm_domain->ops.runtime_suspend; - else if (dev->type && dev->type->pm) - callback = dev->type->pm->runtime_suspend; - else if (dev->class && dev->class->pm) - callback = dev->class->pm->runtime_suspend; - else if (dev->bus && dev->bus->pm) - callback = dev->bus->pm->runtime_suspend; - else - callback = NULL; - - if (!callback && dev->driver && dev->driver->pm) - callback = dev->driver->pm->runtime_suspend; + callback = RPM_GET_CALLBACK(dev, runtime_suspend); retval = rpm_callback(callback, dev); if (retval) @@ -724,19 +721,7 @@ static int rpm_resume(struct device *dev, int rpmflags) __update_runtime_status(dev, RPM_RESUMING); - if (dev->pm_domain) - callback = dev->pm_domain->ops.runtime_resume; - else if (dev->type && dev->type->pm) - callback = dev->type->pm->runtime_resume; - else if (dev->class && dev->class->pm) - callback = dev->class->pm->runtime_resume; - else if (dev->bus && dev->bus->pm) - callback = dev->bus->pm->runtime_resume; - else - callback = NULL; - - if (!callback && dev->driver && dev->driver->pm) - callback = dev->driver->pm->runtime_resume; + callback = RPM_GET_CALLBACK(dev, runtime_resume); retval = rpm_callback(callback, dev); if (retval) {
While fetching the proper runtime PM callback, we walk the hierarchy of device's power domains, subsystems and drivers. This is common for rpm_suspend(), rpm_idle() and rpm_resume(). Let's clean up the code by using a macro that handles this. Cc: Kevin Hilman <khilman@linaro.org> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Mark Brown <broonie@kernel.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: Alessandro Rubini <rubini@unipv.it> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Changes in v2: Updated the macro to return a callback instead. Suggested by Josh Cartwright. --- drivers/base/power/runtime.c | 63 ++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 39 deletions(-)