| Message ID | 1393237368-15500-1-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? Kevin
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
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). 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. Alan Stern
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 >
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? Alan Stern
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 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. 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
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 >
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(-)