Re: [PATCH v4 07/20] PM / devfreq: Show the related information according to governor type
From: MyungJoo Ham <myungjoo.ham@samsung.com>
Date: 2015-12-14 09:49:55
Also in:
linux-pm, linux-samsung-soc, lkml
This patch modifies the following sysfs entry of DEVFREQ framework because the devfreq device using passive governor don't need the same information of the devfreq device using rest governor. - polling_interval : passive gov don't use the sampling rate. - available_governors : passive gov don't be changed on runtime in this version. - trans_stat : passive governor don't support trans_stat in this version. Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> [linux.amoon: Tested on Odroid U3] Tested-by: Anand Moon <redacted>
You have a major update that is not mendtioned in the commit message. (add governor "type")
quoted hunk
--- drivers/devfreq/devfreq.c | 31 +++++++++++++++++++++++++------ drivers/devfreq/governor.h | 7 +++++++ drivers/devfreq/governor_passive.c | 1 + drivers/devfreq/governor_performance.c | 1 + drivers/devfreq/governor_powersave.c | 1 + drivers/devfreq/governor_simpleondemand.c | 1 + drivers/devfreq/governor_userspace.c | 1 + include/linux/devfreq.h | 2 ++ 8 files changed, 39 insertions(+), 6 deletions(-)diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 78ea4cdaa82c..18ad956fec93 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c@@ -597,7 +597,7 @@ struct devfreq *devfreq_add_device(struct device *dev, goto err_init; } - if (!strncmp(devfreq->governor_name, "passive", 7)) { + if (devfreq->governor->type == DEVFREQ_GOV_PASSIVE) { struct devfreq *parent_devfreq = ((struct devfreq_passive_data *)data)->parent;
As mentioned in a previous reply, this code may be removed.
quoted hunk
@@ -963,13 +963,23 @@ static ssize_t available_governors_show(struct device *d, struct device_attribute *attr, char *buf) { - struct devfreq_governor *tmp_governor; + struct devfreq *devfreq = to_devfreq(d); ssize_t count = 0; mutex_lock(&devfreq_list_lock); - list_for_each_entry(tmp_governor, &devfreq_governor_list, node) + if (devfreq->governor->type == DEVFREQ_GOV_PASSIVE) { count += scnprintf(&buf[count], (PAGE_SIZE - count - 2), - "%s ", tmp_governor->name); + "%s ", devfreq->governor->name); + } else { + struct devfreq_governor *tmp_governor; + + list_for_each_entry(tmp_governor, &devfreq_governor_list, node) { + if (tmp_governor->type == DEVFREQ_GOV_PASSIVE) + continue; + count += scnprintf(&buf[count], (PAGE_SIZE - count - 2), + "%s ", tmp_governor->name); + } + }
Uh no. As long as we do not have a list of all possible governors for each device, let's stick with what we've defined in ABI documentation: show all available governors "in the system". You MAY have a device that may run both PASSIVE and USERSPACE.
quoted hunk
mutex_unlock(&devfreq_list_lock); /* Truncate the trailing space */@@ -1006,6 +1016,11 @@ static DEVICE_ATTR_RO(target_freq); static ssize_t polling_interval_show(struct device *dev, struct device_attribute *attr, char *buf) { + struct devfreq *df = to_devfreq(dev); + + if (df->governor->type == DEVFREQ_GOV_PASSIVE) + return sprintf(buf, "Not Supported.\n"); + return sprintf(buf, "%d\n", to_devfreq(dev)->profile->polling_ms); }
When polling interval is irrlevent with the governor, you don't need to print "Not Supported". Instead, printing "0" is enough, (polling_ms=0 == no polling) which is what devfreq is doing now.
quoted hunk
@@ -1020,6 +1035,9 @@ static ssize_t polling_interval_store(struct device *dev, if (!df->governor) return -EINVAL; + if (df->governor->type == DEVFREQ_GOV_PASSIVE) + return -EINVAL; +
Please simply return -EINVAL with governor's event_handler with DEVFREQ_GOV_INTERVAL (I see the return value checking is missing at df->governor->event_handler(). You may want to add return value checking there to properly handle what you want.)
quoted hunk
ret = sscanf(buf, "%u", &value); if (ret != 1) return -EINVAL;@@ -1137,11 +1155,12 @@ static ssize_t trans_stat_show(struct device *dev, int i, j; unsigned int max_state = devfreq->profile->max_state; + if (max_state == 0 || devfreq->governor->type == DEVFREQ_GOV_PASSIVE) + return sprintf(buf, "Not Supported.\n"); + if (!devfreq->stop_polling && devfreq_update_status(devfreq, devfreq->previous_freq)) return 0; - if (max_state == 0) - return sprintf(buf, "Not Supported.\n");
We have no reason to always disable this for PASSIVE.
quoted hunk
len = sprintf(buf, " From : To\n"); len += sprintf(buf + len, " :");diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h index fad7d6321978..43513a58f5bf 100644 --- a/drivers/devfreq/governor.h +++ b/drivers/devfreq/governor.h@@ -18,6 +18,13 @@ #define to_devfreq(DEV) container_of((DEV), struct devfreq, dev) +/* Devfreq governor type */ +#define DEVFREQ_GOV_ONDEMAND 0x1 +#define DEVFREQ_GOV_PERFORMANCE 0x2 +#define DEVFREQ_GOV_POWERSAVE 0x3 +#define DEVFREQ_GOV_USERSPACE 0x4 +#define DEVFREQ_GOV_PASSIVE 0x4
Uh.. both USERSPACE AND PASSIVE are 0x4?