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

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

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