Hi Chanwoo,

On 12/4/18 7:19 AM, Chanwoo Choi wrote:

Hi Lukasz,

On 2018년 12월 03일 23:31, Lukasz Luba wrote:

quoted

This patch adds implementation for global suspend/resume for
devfreq framework. System suspend will next use these functions.

The patch is based on earlier work by Tobias Jakobi.

Please remove it from each patch description.

OK

quoted

Suggested-by: Tobias Jakobi <redacted>
Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Lukasz Luba <redacted>
---
  drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++++++++++++++++
  include/linux/devfreq.h   |  6 ++++++
  2 files changed, 48 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 36bed24..7d60423 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c

@@ -935,6 +935,48 @@ int devfreq_resume_device(struct devfreq *devfreq)
  EXPORT_SYMBOL(devfreq_resume_device);
  
  /**
+ * devfreq_suspend() - Suspend devfreq governors and devices
+ *
+ * Called during system wide Suspend/Hibernate cycles for suspending governors
+ * and devices preserving the state for resume. On some platforms the devfreq
+ * device must have precise state (frequency) after resume in order to provide
+ * fully operating setup.
+ */
+void devfreq_suspend(void)
+{
+	struct devfreq *devfreq;
+	int ret;
+
+	mutex_lock(&devfreq_list_lock);
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		ret = devfreq_suspend_device(devfreq);
+		if (ret)
+			dev_warn(&devfreq->dev, "device suspend failed\n");

When I checked the cpufreq_suspend(), cpufreq_suspend() prints message as 'err' level.
I think that dev_err is more proper than dev_warn.

I'm not sure what is more correct log.
But, 'devfreq->dev' device has the separate suspend/resume function.
So, I think that devfreq_suspend() should print error log containing
that it is error by devfreq framework.

"device suspend failed"
-> "failed to suspend devfreq device"

OK, changed in next v3 patch set.

quoted

+	}
+	mutex_unlock(&devfreq_list_lock);
+}
+
+/**
+ * devfreq_resume() - Resume devfreq governors and devices
+ *
+ * Called during system wide Suspend/Hibernate cycle for resuming governors and
+ * devices that are suspended with devfreq_suspend().
+ */
+void devfreq_resume(void)
+{
+	struct devfreq *devfreq;
+	int ret;
+
+	mutex_lock(&devfreq_list_lock);
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		ret = devfreq_resume_device(devfreq);
+		if (ret)
+			dev_warn(&devfreq->dev, "device resume failed\n");

ditto.

"device resume failed"
-> "failed to resume devfreq device"

ACK

Regards,
Lukasz

quoted

+	}
+	mutex_unlock(&devfreq_list_lock);
+}
+
+/**
   * devfreq_add_governor() - Add devfreq governor
   * @governor:	the devfreq governor to be added
   */

diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index d985199..fbffa74 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h

@@ -205,6 +205,9 @@ extern void devm_devfreq_remove_device(struct device *dev,
  extern int devfreq_suspend_device(struct devfreq *devfreq);
  extern int devfreq_resume_device(struct devfreq *devfreq);
  
+extern void devfreq_suspend(void);
+extern void devfreq_resume(void);
+
  /**
   * update_devfreq() - Reevaluate the device and configure frequency
   * @devfreq:	the devfreq device

@@ -331,6 +334,9 @@ static inline int devfreq_resume_device(struct devfreq *devfreq)
  	return 0;
  }
  
+static inline void devfreq_suspend(void) {}
+static inline void devfreq_resume(void) {}
+
  static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
  					   unsigned long *freq, u32 flags)
  {