Re: [PATCH v4 07/20] PM / devfreq: Show the related information according to governor type

* Re: [PATCH v4 07/20] PM / devfreq: Show the related information according to governor type
@ 2015-12-14  9:49 ` MyungJoo Ham
  0 siblings, 0 replies; 3+ messages in thread
From: MyungJoo Ham @ 2015-12-14  9:49 UTC (permalink / raw)
  To: 최찬우,
	크쉬시토프,
	kgene
  Cc: 박경민,
	robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, linux,
	tjakobi, linux.amoon, linux-kernel, linux-pm, linux-samsung-soc,
	devicetree

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 5418 bytes --]

>   
>  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 <linux.amoon@gmail.com>

You have a major update that is not mendtioned in the commit message.
(add governor "type")

> ---
>  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.

>  
> @@ -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.

>  	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.

>  
> @@ -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.)

>  	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.

>  
>  	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?

ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éÝ¶\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dÊ‡Ú™ë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 3+ messages in thread