[PATCH 0/6] PM / devfreq: Clean code and add set the freq_table array

[PATCH 0/6] PM / devfreq: Clean code and add set the freq_table array

[Chanwoo Choi]

This patch-set clean the code for both devfreq and devfreq-event framework
and add the 'freq_table' for devfreq device. After initializing the
'freq_table', the 'trans_stat' sysfs provide the appropriate information.

Chanwoo Choi (6):
  PM / devfreq: event: Remove the error log of devfreq_event_get_edev_by_phandle()
  PM / devfreq: event: Fix the error and warning from script/checkpatch.pl
  PM / devfreq: Add show_one macro to delete the duplicate code
  PM / devfreq: Set the freq_table of devfreq device
  PM / devfreq: Modify the indentation of trans_stat sysfs for readability
  PM / devfreq: Set the min_freq and max_freq of devfreq device

 drivers/devfreq/devfreq-event.c | 16 +++-----
 drivers/devfreq/devfreq.c       | 81 ++++++++++++++++++++++++++++++++---------
 include/linux/devfreq.h         |  4 +-
 3 files changed, 71 insertions(+), 30 deletions(-)

-- 
1.9.1

[PATCH 1/6] PM / devfreq: event: Remove the error log of devfreq_event_get_edev_by_phandle()

[Chanwoo Choi]

This patch just removes the error log when devfreq_event_get_edev_by_phandle()
fail to get the instance of devfreq-event device. It is related to sequence of
the probe() of each driver. So, this error log might show the always during
kernel booting. Instead, each driver using this function can show the
appropriate error log.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq-event.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
index f304a0289eda..f51de879b2be 100644
--- a/drivers/devfreq/devfreq-event.c
+++ b/drivers/devfreq/devfreq-event.c
@@ -226,17 +226,12 @@ struct devfreq_event_dev *devfreq_event_get_edev_by_phandle(struct device *dev,
 	struct device_node *node;
 	struct devfreq_event_dev *edev;
 
-	if (!dev->of_node) {
-		dev_err(dev, "device does not have a device node entry\n");
+	if (!dev->of_node)
 		return ERR_PTR(-EINVAL);
-	}
 
 	node = of_parse_phandle(dev->of_node, "devfreq-events", index);
-	if (!node) {
-		dev_err(dev, "failed to get phandle in %s node\n",
-			dev->of_node->full_name);
+	if (!node)
 		return ERR_PTR(-ENODEV);
-	}
 
 	mutex_lock(&devfreq_event_list_lock);
 	list_for_each_entry(edev, &devfreq_event_list, node) {
@@ -248,8 +243,6 @@ out:
 	mutex_unlock(&devfreq_event_list_lock);
 
 	if (!edev) {
-		dev_err(dev, "unable to get devfreq-event device : %s\n",
-			node->name);
 		of_node_put(node);
 		return ERR_PTR(-ENODEV);
 	}
-- 
1.9.1

[PATCH 2/6] PM / devfreq: event: Fix the error and warning from script/checkpatch.pl

[Chanwoo Choi]

This patch just fixes following error and warning by using
scripts/checkpatch.pl.

- Follwoing issue from checkpatch.pl:
ERROR: space prohibited before that close parenthesis ')'
+	if (count < 0 ) {

WARNING: line over 80 characters
+	ptr = devres_alloc(devm_devfreq_event_release, sizeof(*ptr), GFP_KERNEL);

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq-event.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
index f51de879b2be..38bf144ca147 100644
--- a/drivers/devfreq/devfreq-event.c
+++ b/drivers/devfreq/devfreq-event.c
@@ -270,7 +270,7 @@ int devfreq_event_get_edev_count(struct device *dev)
 
 	count = of_property_count_elems_of_size(dev->of_node, "devfreq-events",
 						sizeof(u32));
-	if (count < 0 ) {
+	if (count < 0) {
 		dev_err(dev,
 			"failed to get the count of devfreq-event in %s node\n",
 			dev->of_node->full_name);
@@ -395,7 +395,8 @@ struct devfreq_event_dev *devm_devfreq_event_add_edev(struct device *dev,
 {
 	struct devfreq_event_dev **ptr, *edev;
 
-	ptr = devres_alloc(devm_devfreq_event_release, sizeof(*ptr), GFP_KERNEL);
+	ptr = devres_alloc(devm_devfreq_event_release, sizeof(*ptr),
+				GFP_KERNEL);
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-- 
1.9.1

[PATCH 3/6] PM / devfreq: Add show_one macro to delete the duplicate code

[Chanwoo Choi]

This patch adds the 'show_one' macro to simplify the duplicate code
of both max_freq_show() and min_freq_show().

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index ca848cc6a8fd..49fc7dcf664c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -921,12 +921,6 @@ unlock:
 	return ret;
 }
 
-static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
-			     char *buf)
-{
-	return sprintf(buf, "%lu\n", to_devfreq(dev)->min_freq);
-}
-
 static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -953,13 +947,17 @@ unlock:
 	mutex_unlock(&df->lock);
 	return ret;
 }
-static DEVICE_ATTR_RW(min_freq);
 
-static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
-			     char *buf)
-{
-	return sprintf(buf, "%lu\n", to_devfreq(dev)->max_freq);
+#define show_one(name)						\
+static ssize_t name##_show					\
+(struct device *dev, struct device_attribute *attr, char *buf)	\
+{								\
+	return sprintf(buf, "%lu\n", to_devfreq(dev)->name);	\
 }
+show_one(min_freq);
+show_one(max_freq);
+
+static DEVICE_ATTR_RW(min_freq);
 static DEVICE_ATTR_RW(max_freq);
 
 static ssize_t available_frequencies_show(struct device *d,
-- 
1.9.1

[PATCH 4/6] PM / devfreq: Set the freq_table of devfreq device

[Chanwoo Choi]

This patch initialize the freq_table array of each devfreq device by using
the devfreq_set_freq_table(). If freq_table is NULL, the devfreq framework
is not able to support the frequency transtion information through sysfs.

The OPP core uses the integer type for the number of opps in the opp list
and uses the 'unsigned long' type for each frequency. So, this patch modifies
the type of some variable as following:
- the type of freq_table : unsigned int -> unsigned long
- the type of max_state  : unsigned int -> int

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 50 ++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/devfreq.h   |  4 ++--
 2 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 49fc7dcf664c..761e46a95c4b 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -85,6 +85,45 @@ static int devfreq_get_freq_level(struct devfreq *devfreq, unsigned long freq)
 }
 
 /**
+ * devfreq_set_freq_table() - Initialize freq_table for the frequency
+ * @devfreq:	the devfreq instance
+ */
+static void devfreq_set_freq_table(struct devfreq *devfreq)
+{
+	struct devfreq_dev_profile *profile = devfreq->profile;
+	struct dev_pm_opp *opp;
+	unsigned long freq;
+	int i;
+
+	/* Initialize the freq_table from OPP table */
+	profile->max_state = dev_pm_opp_get_opp_count(devfreq->dev.parent);
+	if (profile->max_state <= 0)
+		return;
+
+	profile->freq_table = devm_kcalloc(devfreq->dev.parent,
+					profile->max_state,
+					sizeof(*profile->freq_table),
+					GFP_KERNEL);
+	if (!profile->freq_table) {
+		profile->max_state = 0;
+		return;
+	}
+
+	rcu_read_lock();
+	for (i = 0, freq = 0; i < profile->max_state; i++, freq++) {
+		opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &freq);
+		if (IS_ERR(opp)) {
+			devm_kfree(devfreq->dev.parent, profile->freq_table);
+			profile->max_state = 0;
+			rcu_read_unlock();
+			return;
+		}
+		profile->freq_table[i] = freq;
+	}
+	rcu_read_unlock();
+}
+
+/**
  * devfreq_update_status() - Update statistics of devfreq behavior
  * @devfreq:	the devfreq instance
  * @freq:	the update target frequency
@@ -477,7 +516,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->previous_freq = profile->initial_freq;
 	devfreq->data = data;
 	devfreq->nb.notifier_call = devfreq_notifier_call;
+	mutex_unlock(&devfreq->lock);
 
+	if (devfreq->profile->max_state <= 0 && !devfreq->profile->freq_table)
+		devfreq_set_freq_table(devfreq);
+
+	mutex_lock(&devfreq->lock);
 	devfreq->trans_table =	devm_kzalloc(dev, sizeof(unsigned int) *
 						devfreq->profile->max_state *
 						devfreq->profile->max_state,
@@ -998,7 +1042,7 @@ static ssize_t trans_stat_show(struct device *dev,
 	struct devfreq *devfreq = to_devfreq(dev);
 	ssize_t len;
 	int i, j;
-	unsigned int max_state = devfreq->profile->max_state;
+	int max_state = devfreq->profile->max_state;
 
 	if (!devfreq->stop_polling &&
 			devfreq_update_status(devfreq, devfreq->previous_freq))
@@ -1007,7 +1051,7 @@ static ssize_t trans_stat_show(struct device *dev,
 	len = sprintf(buf, "   From  :   To\n");
 	len += sprintf(buf + len, "         :");
 	for (i = 0; i < max_state; i++)
-		len += sprintf(buf + len, "%8u",
+		len += sprintf(buf + len, "%8ld",
 				devfreq->profile->freq_table[i]);
 
 	len += sprintf(buf + len, "   time(ms)\n");
@@ -1019,7 +1063,7 @@ static ssize_t trans_stat_show(struct device *dev,
 		} else {
 			len += sprintf(buf + len, " ");
 		}
-		len += sprintf(buf + len, "%8u:",
+		len += sprintf(buf + len, "%8ld:",
 				devfreq->profile->freq_table[i]);
 		for (j = 0; j < max_state; j++)
 			len += sprintf(buf + len, "%8u",
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 68030e22af35..c6117ceb0c5d 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -89,8 +89,8 @@ struct devfreq_dev_profile {
 	int (*get_cur_freq)(struct device *dev, unsigned long *freq);
 	void (*exit)(struct device *dev);
 
-	unsigned int *freq_table;
-	unsigned int max_state;
+	unsigned long *freq_table;
+	int max_state;
 };
 
 /**
-- 
1.9.1

[PATCH 5/6] PM / devfreq: Modify the indentation of trans_stat sysfs for readability

[Chanwoo Choi]

This patch modifies the indentation of 'trans_stat' sysfs to improve readability.
The 1GHz is 1000,000,000. So it needs the least 10 position to show the GHz unit.

- Before apply this patch,
-sh-3.2# cat trans_stat
   From  :   To
         :50000000100000000133000000200000000400000000   time(ms)
*50000000:       0       0       0       0       7   1817635
 100000000:       4       0       0       0       4      1590
 133000000:       1       4       0       0       7       975
 200000000:       2       2       7       0       1      2655
 400000000:       0       2       5      12       0      1860
Total transition : 58

- After apply this patch,
-sh-3.2# cat trans_stat
     From  :   To
           :  50000000 100000000 133000000 200000000 400000000   time(ms)
*  50000000:         0         0         0         0         7     14405
  100000000:         4         0         0         0         3      2015
  133000000:         2         3         0         0         7      1020
  200000000:         1         2         7         0         0      2970
  400000000:         0         2         5        10         0      1575
Total transition : 53

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 761e46a95c4b..c292ceb7ff19 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1048,10 +1048,10 @@ static ssize_t trans_stat_show(struct device *dev,
 			devfreq_update_status(devfreq, devfreq->previous_freq))
 		return 0;
 
-	len = sprintf(buf, "   From  :   To\n");
-	len += sprintf(buf + len, "         :");
+	len = sprintf(buf, "     From  :   To\n");
+	len += sprintf(buf + len, "           :");
 	for (i = 0; i < max_state; i++)
-		len += sprintf(buf + len, "%8ld",
+		len += sprintf(buf + len, "%10ld",
 				devfreq->profile->freq_table[i]);
 
 	len += sprintf(buf + len, "   time(ms)\n");
@@ -1063,10 +1063,10 @@ static ssize_t trans_stat_show(struct device *dev,
 		} else {
 			len += sprintf(buf + len, " ");
 		}
-		len += sprintf(buf + len, "%8ld:",
+		len += sprintf(buf + len, "%10ld:",
 				devfreq->profile->freq_table[i]);
 		for (j = 0; j < max_state; j++)
-			len += sprintf(buf + len, "%8u",
+			len += sprintf(buf + len, "%10u",
 				devfreq->trans_table[(i * max_state) + j]);
 		len += sprintf(buf + len, "%10u\n",
 			jiffies_to_msecs(devfreq->time_in_state[i]));
-- 
1.9.1

[PATCH 6/6] PM / devfreq: Set the min_freq and max_freq of devfreq device

[Chanwoo Choi]

After probing the devfreq device driver, the value of both min_freq and
max_freq are zero(0). So, this patch initializes the 'min_freq' and 'max_freq'
field of devfreq device by using the freq_table array.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index c292ceb7ff19..0b24ae7b7a48 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -121,6 +121,11 @@ static void devfreq_set_freq_table(struct devfreq *devfreq)
 		profile->freq_table[i] = freq;
 	}
 	rcu_read_unlock();
+
+	mutex_lock(&devfreq->lock);
+	devfreq->min_freq = profile->freq_table[0];
+	devfreq->max_freq = profile->freq_table[profile->max_state - 1];
+	mutex_unlock(&devfreq->lock);
 }
 
 /**
-- 
1.9.1

Re: [PATCH 4/6] PM / devfreq: Set the freq_table of devfreq device

[MyungJoo Ham]

>  This patch initialize the freq_table array of each devfreq device by using
> the devfreq_set_freq_table(). If freq_table is NULL, the devfreq framework
> is not able to support the frequency transtion information through sysfs.
> 
> The OPP core uses the integer type for the number of opps in the opp list
> and uses the 'unsigned long' type for each frequency. So, this patch modifies
> the type of some variable as following:
> - the type of freq_table : unsigned int -> unsigned long
> - the type of max_state  : unsigned int -> int

I have some comments on this patch described below.

I've created an 'updated' patch based on this at:
https://git.kernel.org/cgit/linux/kernel/git/mzx/devfreq.git/commit/?h=for-rafael&id=9e35a6caf143cd77f14d5e62269ed00ea1775854

It will be applied as suggested on the link above unless you
have strong reason to make max_state signed.

[]
> +static void devfreq_set_freq_table(struct devfreq *devfreq)
> +{
> +	struct devfreq_dev_profile *profile = devfreq->profile;
> +	struct dev_pm_opp *opp;
> +	unsigned long freq;
# -	int i;

+	int i, count;

> +
> +	/* Initialize the freq_table from OPP table */
# -	profile->max_state = dev_pm_opp_get_opp_count(devfreq->dev.parent);
# -	if (profile->max_state <= 0)
# -		return;

+	count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
+	if (count < 0)
+		return;

If dev_pm_opp_get_opp_count() gives us an error (probably, this
device does not support OPP and does not give freq_table for the
statistics support), we do not need to store that error in
profile->max_state. We just need to return. (no need to be
signed.)

[]
> -		len += sprintf(buf + len, "%8u",
> +		len += sprintf(buf + len, "%8ld",
>  				devfreq->profile->freq_table[i]);
[]
> -		len += sprintf(buf + len, "%8u:",
> +		len += sprintf(buf + len, "%8ld:",
>  				devfreq->profile->freq_table[i]);

freq_table is unsigned.

Re: [PATCH 4/6] PM / devfreq: Set the freq_table of devfreq device

[MyungJoo Ham]

>   
>  This patch initialize the freq_table array of each devfreq device by using
> the devfreq_set_freq_table(). If freq_table is NULL, the devfreq framework
> is not able to support the frequency transtion information through sysfs.
> 
> The OPP core uses the integer type for the number of opps in the opp list
> and uses the 'unsigned long' type for each frequency. So, this patch modifies
> the type of some variable as following:
> - the type of freq_table : unsigned int -> unsigned long
> - the type of max_state  : unsigned int -> int
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
[]
> +/**
>   * devfreq_update_status() - Update statistics of devfreq behavior
>   * @devfreq:	the devfreq instance
>   * @freq:	the update target frequency
> @@ -477,7 +516,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->previous_freq = profile->initial_freq;
>  	devfreq->data = data;
>  	devfreq->nb.notifier_call = devfreq_notifier_call;
> +	mutex_unlock(&devfreq->lock);
>  
> +	if (devfreq->profile->max_state <= 0 && !devfreq->profile->freq_table)
> +		devfreq_set_freq_table(devfreq);
> +
> +	mutex_lock(&devfreq->lock);

Anyway, what about modifying the block above as:

+	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
+		mutex_unlock(&devfreq->lock);
 		devfreq_set_freq_table(devfreq);
+		mutex_lock(&devfreq->lock);
+	}

>  	devfreq->trans_table =	devm_kzalloc(dev, sizeof(unsigned int) *
>  						devfreq->profile->max_state *
>  						devfreq->profile->max_state,
[]

If that's not a problem, I'll squash
https://git.kernel.org/cgit/linux/kernel/git/mzx/devfreq.git/commit/?h=for-rafael&id=55df2b43cdb5ac82d26b64d739f3d758c9b7486c
with the given patch.

Re: [PATCH 4/6] PM / devfreq: Set the freq_table of devfreq device

[Chanwoo Choi]

On Mon, Nov 23, 2015 at 4:29 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>>
>>  This patch initialize the freq_table array of each devfreq device by using
>> the devfreq_set_freq_table(). If freq_table is NULL, the devfreq framework
>> is not able to support the frequency transtion information through sysfs.
>>
>> The OPP core uses the integer type for the number of opps in the opp list
>> and uses the 'unsigned long' type for each frequency. So, this patch modifies
>> the type of some variable as following:
>> - the type of freq_table : unsigned int -> unsigned long
>> - the type of max_state  : unsigned int -> int
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> []
>> +/**
>>   * devfreq_update_status() - Update statistics of devfreq behavior
>>   * @devfreq: the devfreq instance
>>   * @freq:    the update target frequency
>> @@ -477,7 +516,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>       devfreq->previous_freq = profile->initial_freq;
>>       devfreq->data = data;
>>       devfreq->nb.notifier_call = devfreq_notifier_call;
>> +     mutex_unlock(&devfreq->lock);
>>
>> +     if (devfreq->profile->max_state <= 0 && !devfreq->profile->freq_table)
>> +             devfreq_set_freq_table(devfreq);
>> +
>> +     mutex_lock(&devfreq->lock);
>
> Anyway, what about modifying the block above as:
>
> +       if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
> +               mutex_unlock(&devfreq->lock);
>                 devfreq_set_freq_table(devfreq);
> +               mutex_lock(&devfreq->lock);
> +       }
>
>>       devfreq->trans_table =  devm_kzalloc(dev, sizeof(unsigned int) *
>>                                               devfreq->profile->max_state *
>>                                               devfreq->profile->max_state,
> []
>
> If that's not a problem, I'll squash
> https://git.kernel.org/cgit/linux/kernel/git/mzx/devfreq.git/commit/?h=for-rafael&id=55df2b43cdb5ac82d26b64d739f3d758c9b7486c
> with the given patch.

I agree. Thanks for fixing this.

Regards,
Chanwoo Choi