[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Ulf Hansson]

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;						\
+})
+
 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) {
-- 
1.7.9.5

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Kevin Hilman]

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

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Ulf Hansson]

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

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Alan Stern]

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

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Ulf Hansson]

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
>

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[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

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Ulf Hansson]

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
>

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[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

[PATCH V2 1/8] PM / Runtime: Fetch runtime PM callbacks using a macro

[Ulf Hansson]

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
>