diff mbox

[1/2] power: add new interface to return pm_transition state

Message ID 1371931874-3220-1-git-send-email-shuah.kh@samsung.com (mailing list archive)
State Rejected, archived
Headers show

Commit Message

Shuah Khan June 22, 2013, 8:11 p.m. UTC
Add a new interface get_pm_transition() to return pm_transition state.
This interface is intended to be used from dev_pm_ops class and type
suspend interfaces that call legacy pm ops driver suspend interfaces.
Legacy suspend pm_ops take pm_message_t as a parameter.

e.g: drm_class_suspend() calls into driver suspend routines
via drm_dev->driver->suspend(drm_dev, state).

Once drm_class_suspend() is converted to dev_pm_ops, it will no longer
have access to pm_transition which it has to pass into driver legacy
suspend calls. get_pm_transition() interface addresses this need.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/base/power/main.c |   17 +++++++++++++++++
 include/linux/pm.h        |    3 +++
 2 files changed, 20 insertions(+)

Comments

Rafael Wysocki June 22, 2013, 9:26 p.m. UTC | #1
On Saturday, June 22, 2013 02:11:14 PM Shuah Khan wrote:
> Add a new interface get_pm_transition() to return pm_transition state.
> This interface is intended to be used from dev_pm_ops class and type
> suspend interfaces that call legacy pm ops driver suspend interfaces.
> Legacy suspend pm_ops take pm_message_t as a parameter.
> 
> e.g: drm_class_suspend() calls into driver suspend routines
> via drm_dev->driver->suspend(drm_dev, state).
> 
> Once drm_class_suspend() is converted to dev_pm_ops, it will no longer
> have access to pm_transition which it has to pass into driver legacy
> suspend calls. get_pm_transition() interface addresses this need.

That shouldn't be necessary because each transition has its own callback
in strict dev_pm_ops.

Thanks,
Rafael


> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> ---
>  drivers/base/power/main.c |   17 +++++++++++++++++
>  include/linux/pm.h        |    3 +++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index c2132b8..3e2e09d 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -57,6 +57,23 @@ static pm_message_t pm_transition;
>  static int async_error;
>  
>  /**
> + * get_pm_transition - return pm_transition state.
> + * This interface is intended to be used from dev_pm_ops class and type
> + * suspend interfaces that call legacy pm ops driver suspend interfaces.
> + * Legacy suspend pm_ops take pm_message_t as a parameter.
> + * e.g: drm_class_suspend() calls into driver suspend routines
> + * via drm_dev->driver->suspend(drm_dev, state).
> + * Once drm_class_suspend() is converted to dev_pm_ops, it will no longer
> + * have access to pm_transition which it has to pass into driver legacy
> + * suspend calls. get_pm_transition() interface addresses this need.
> +*/
> +pm_message_t get_pm_transition()
> +{
> +	return pm_transition;
> +}
> +EXPORT_SYMBOL_GPL(get_pm_transition);
> +
> +/**
>   * device_pm_sleep_init - Initialize system suspend-related device fields.
>   * @dev: Device object being initialized.
>   */
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index a224c7f..46f3252 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -63,6 +63,9 @@ typedef struct pm_message {
>  	int event;
>  } pm_message_t;
>  
> +/* drivers/base/power/main.c */
> +extern pm_message_t get_pm_transition(void);
> +
>  /**
>   * struct dev_pm_ops - device PM callbacks
>   *
>
Shuah Khan June 24, 2013, 4:20 p.m. UTC | #2
On 06/22/2013 03:17 PM, Rafael J. Wysocki wrote:
> On Saturday, June 22, 2013 02:11:14 PM Shuah Khan wrote:
>> Add a new interface get_pm_transition() to return pm_transition state.
>> This interface is intended to be used from dev_pm_ops class and type
>> suspend interfaces that call legacy pm ops driver suspend interfaces.
>> Legacy suspend pm_ops take pm_message_t as a parameter.
>>
>> e.g: drm_class_suspend() calls into driver suspend routines
>> via drm_dev->driver->suspend(drm_dev, state).
>>
>> Once drm_class_suspend() is converted to dev_pm_ops, it will no longer
>> have access to pm_transition which it has to pass into driver legacy
>> suspend calls. get_pm_transition() interface addresses this need.
>
> That shouldn't be necessary because each transition has its own callback
> in strict dev_pm_ops.
>
> Thanks,
> Rafael
>

Yes that is correct that there is no need pass in or know pm_transition 
with dev_pm_ops. The issue I am running into is the legacy pm_ops class 
suspend/resume routines call. In the example, I mentioned in my 
changelog, drm_class_suspend() calls legacy pm_ops and passes in state. 
It passes in the state to the driver legacy suspend routine. I have seen 
code paths in drivers that differentiate between PM_EVENT_FREEZE, 
PM_EVENT_SUSPEND, PM_EVENT_SLEEP etc.

I considered passing PM_EVENT_SUSPEND to 
drm_dev->driver->suspend(drm_dev, state) from drm_class_suspend() which 
would eliminate the need for this new interface. However, I am concerned 
about breaking driver legacy suspend routines that key of off the state 
to execute different code paths for PM_EVENT_FREEZE vs. 
PM_EVENT_SUSPEND. Suspend routines get called when state is is 
PM_EVENT_FREEZE based on my testing.

I would rather not add a new interface. Hoping you will have another 
idea on how to pass in the state to legacy suspend/resume without adding 
this new interface. My thinking is that this new interface is temporary 
measure until all of the legacy suspend routines get converted to 
dev_pm_ops and at the tile legacy interface gets removed, this new 
interface can go away as well.

thanks,
-- Shuah

Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research 
America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658
--
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
Rafael Wysocki June 24, 2013, 7:44 p.m. UTC | #3
On Monday, June 24, 2013 04:20:06 PM Shuah Khan wrote:
> On 06/22/2013 03:17 PM, Rafael J. Wysocki wrote:
> > On Saturday, June 22, 2013 02:11:14 PM Shuah Khan wrote:
> >> Add a new interface get_pm_transition() to return pm_transition state.
> >> This interface is intended to be used from dev_pm_ops class and type
> >> suspend interfaces that call legacy pm ops driver suspend interfaces.
> >> Legacy suspend pm_ops take pm_message_t as a parameter.
> >>
> >> e.g: drm_class_suspend() calls into driver suspend routines
> >> via drm_dev->driver->suspend(drm_dev, state).
> >>
> >> Once drm_class_suspend() is converted to dev_pm_ops, it will no longer
> >> have access to pm_transition which it has to pass into driver legacy
> >> suspend calls. get_pm_transition() interface addresses this need.
> >
> > That shouldn't be necessary because each transition has its own callback
> > in strict dev_pm_ops.
> >
> > Thanks,
> > Rafael
> >
> 
> Yes that is correct that there is no need pass in or know pm_transition 
> with dev_pm_ops. The issue I am running into is the legacy pm_ops class 
> suspend/resume routines call. In the example, I mentioned in my 
> changelog, drm_class_suspend() calls legacy pm_ops and passes in state. 
> It passes in the state to the driver legacy suspend routine. I have seen 
> code paths in drivers that differentiate between PM_EVENT_FREEZE, 
> PM_EVENT_SUSPEND, PM_EVENT_SLEEP etc.
> 
> I considered passing PM_EVENT_SUSPEND to 
> drm_dev->driver->suspend(drm_dev, state) from drm_class_suspend() which 
> would eliminate the need for this new interface. However, I am concerned 
> about breaking driver legacy suspend routines that key of off the state 
> to execute different code paths for PM_EVENT_FREEZE vs. 
> PM_EVENT_SUSPEND. Suspend routines get called when state is is 
> PM_EVENT_FREEZE based on my testing.
> 
> I would rather not add a new interface. Hoping you will have another 
> idea on how to pass in the state to legacy suspend/resume without adding 
> this new interface. My thinking is that this new interface is temporary 
> measure until all of the legacy suspend routines get converted to 
> dev_pm_ops and at the tile legacy interface gets removed, this new 
> interface can go away as well.

There are multiple bus types with legacy callbacks.  PCI and platform for
two examples and USB does something similar.  Please have a look at these.

Thanks,
Rafael
diff mbox

Patch

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index c2132b8..3e2e09d 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -57,6 +57,23 @@  static pm_message_t pm_transition;
 static int async_error;
 
 /**
+ * get_pm_transition - return pm_transition state.
+ * This interface is intended to be used from dev_pm_ops class and type
+ * suspend interfaces that call legacy pm ops driver suspend interfaces.
+ * Legacy suspend pm_ops take pm_message_t as a parameter.
+ * e.g: drm_class_suspend() calls into driver suspend routines
+ * via drm_dev->driver->suspend(drm_dev, state).
+ * Once drm_class_suspend() is converted to dev_pm_ops, it will no longer
+ * have access to pm_transition which it has to pass into driver legacy
+ * suspend calls. get_pm_transition() interface addresses this need.
+*/
+pm_message_t get_pm_transition()
+{
+	return pm_transition;
+}
+EXPORT_SYMBOL_GPL(get_pm_transition);
+
+/**
  * device_pm_sleep_init - Initialize system suspend-related device fields.
  * @dev: Device object being initialized.
  */
diff --git a/include/linux/pm.h b/include/linux/pm.h
index a224c7f..46f3252 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -63,6 +63,9 @@  typedef struct pm_message {
 	int event;
 } pm_message_t;
 
+/* drivers/base/power/main.c */
+extern pm_message_t get_pm_transition(void);
+
 /**
  * struct dev_pm_ops - device PM callbacks
  *