Re: [PATCH v2] PM / devfreq: Use freq_table for available_frequencies

Re: [PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[MyungJoo Ham]

> Some devices use freq_table instead of OPP. For those devices, the
> available_frequencies file shows up empty. Fix that by using freq_table to
> generate the available_frequencies data when it's available.
> 
> OPP find frequency APIs also skips frequencies that have been temporarily
> disabled (say, due to thermal, etc). Since available_frequencies is
> supposed to show the entire list of available frequencies without taking
> temporary limits into consideration, preference is given to freq_table when
> available.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
>  drivers/devfreq/devfreq.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2042ec3..527cbe2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -912,20 +912,27 @@ static ssize_t available_frequencies_show(struct device *d,
>  	struct devfreq *df = to_devfreq(d);
>  	struct device *dev = df->dev.parent;
>  	struct dev_pm_opp *opp;
> +	unsigned int i = 0;
>  	ssize_t count = 0;
>  	unsigned long freq = 0;
>  
> -	rcu_read_lock();
> -	do {
> -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> -		if (IS_ERR(opp))
> -			break;
> -
> -		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> -				   "%lu ", freq);
> -		freq++;
> -	} while (1);
> -	rcu_read_unlock();
> +	if (df->profile->freq_table) {
> +		for (i = 0; i < df->profile->max_state; i++)
> +			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> +					   "%u ", df->profile->freq_table[i]);

You are hereby changing the semmantics of the original
available_frequencies node.

When a frequency/voltage pair has been disabled (opp_disable), probably
by opp_disable(), the frequency is no more "available".
However, when the driver author supplied freq_table as well as OPP
in order to see the statistics, the node will behave differently.

Please do not affect the current users as long as it does not give
additional benefit or fix a bug.


Cheers,
MyungJoo.


> +	} else {
> +		rcu_read_lock();
> +		do {
> +			opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> +			if (IS_ERR(opp))
> +				break;
> +
> +			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> +					   "%lu ", freq);
> +			freq++;
> +		} while (1);
> +		rcu_read_unlock();
> +	}
>  
>  	/* Truncate the trailing space */
>  	if (count)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

Re: [PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[MyungJoo Ham]

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

> Some devices use freq_table instead of OPP. For those devices, the
> available_frequencies file shows up empty. Fix that by using freq_table to
> generate the available_frequencies data when it's available.
> 
> OPP find frequency APIs also skips frequencies that have been temporarily
> disabled (say, due to thermal, etc). Since available_frequencies is
> supposed to show the entire list of available frequencies without taking
> temporary limits into consideration, preference is given to freq_table when
> available.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
>  drivers/devfreq/devfreq.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2042ec3..527cbe2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -912,20 +912,27 @@ static ssize_t available_frequencies_show(struct device *d,
>  	struct devfreq *df = to_devfreq(d);
>  	struct device *dev = df->dev.parent;
>  	struct dev_pm_opp *opp;
> +	unsigned int i = 0;
>  	ssize_t count = 0;
>  	unsigned long freq = 0;
>  
> -	rcu_read_lock();
> -	do {
> -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> -		if (IS_ERR(opp))
> -			break;
> -
> -		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> -				   "%lu ", freq);
> -		freq++;
> -	} while (1);
> -	rcu_read_unlock();
> +	if (df->profile->freq_table) {
> +		for (i = 0; i < df->profile->max_state; i++)
> +			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> +					   "%u ", df->profile->freq_table[i]);

You are hereby changing the semmantics of the original
available_frequencies node.

When a frequency/voltage pair has been disabled (opp_disable), probably
by opp_disable(), the frequency is no more "available".
However, when the driver author supplied freq_table as well as OPP
in order to see the statistics, the node will behave differently.

Please do not affect the current users as long as it does not give
additional benefit or fix a bug.


Cheers,
MyungJoo.


> +	} else {
> +		rcu_read_lock();
> +		do {
> +			opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> +			if (IS_ERR(opp))
> +				break;
> +
> +			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> +					   "%lu ", freq);
> +			freq++;
> +		} while (1);
> +		rcu_read_unlock();
> +	}
>  
>  	/* Truncate the trailing space */
>  	if (count)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
ÿôèº{.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¥

Re: [PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 04/27/2014 06:41 PM, MyungJoo Ham wrote:
>> Some devices use freq_table instead of OPP. For those devices, the
>> available_frequencies file shows up empty. Fix that by using freq_table to
>> generate the available_frequencies data when it's available.
>>
>> OPP find frequency APIs also skips frequencies that have been temporarily
>> disabled (say, due to thermal, etc). Since available_frequencies is
>> supposed to show the entire list of available frequencies without taking
>> temporary limits into consideration, preference is given to freq_table when
>> available.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>>   drivers/devfreq/devfreq.c | 29 ++++++++++++++++++-----------
>>   1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 2042ec3..527cbe2 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -912,20 +912,27 @@ static ssize_t available_frequencies_show(struct device *d,
>>   	struct devfreq *df = to_devfreq(d);
>>   	struct device *dev = df->dev.parent;
>>   	struct dev_pm_opp *opp;
>> +	unsigned int i = 0;
>>   	ssize_t count = 0;
>>   	unsigned long freq = 0;
>>   
>> -	rcu_read_lock();
>> -	do {
>> -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>> -		if (IS_ERR(opp))
>> -			break;
>> -
>> -		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
>> -				   "%lu ", freq);
>> -		freq++;
>> -	} while (1);
>> -	rcu_read_unlock();
>> +	if (df->profile->freq_table) {
>> +		for (i = 0; i < df->profile->max_state; i++)
>> +			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
>> +					   "%u ", df->profile->freq_table[i]);
> 
> You are hereby changing the semmantics of the original
> available_frequencies node.
> 
> When a frequency/voltage pair has been disabled (opp_disable), probably
> by opp_disable(), the frequency is no more "available".
> However, when the driver author supplied freq_table as well as OPP
> in order to see the statistics, the node will behave differently.
> 
> Please do not affect the current users as long as it does not give
> additional benefit or fix a bug.

I was actually trying to stick with the semantics as it was documented.
The documentation for this file says it'll show frequencies that are not
allowed by the current min/max settings either. To me, an OPP disable
seems similar to some frequencies "disabled" by min/max settings.

Giving preference to OPP is not a hard change to do, but it seems to go
againsts the documented semantics.

Thoughts?

-Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 04/27/2014 06:41 PM, MyungJoo Ham wrote:
>> Some devices use freq_table instead of OPP. For those devices, the
>> available_frequencies file shows up empty. Fix that by using freq_table to
>> generate the available_frequencies data when it's available.
>>
>> OPP find frequency APIs also skips frequencies that have been temporarily
>> disabled (say, due to thermal, etc). Since available_frequencies is
>> supposed to show the entire list of available frequencies without taking
>> temporary limits into consideration, preference is given to freq_table when
>> available.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>>   drivers/devfreq/devfreq.c | 29 ++++++++++++++++++-----------
>>   1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 2042ec3..527cbe2 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -912,20 +912,27 @@ static ssize_t available_frequencies_show(struct device *d,
>>   	struct devfreq *df = to_devfreq(d);
>>   	struct device *dev = df->dev.parent;
>>   	struct dev_pm_opp *opp;
>> +	unsigned int i = 0;
>>   	ssize_t count = 0;
>>   	unsigned long freq = 0;
>>   
>> -	rcu_read_lock();
>> -	do {
>> -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>> -		if (IS_ERR(opp))
>> -			break;
>> -
>> -		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
>> -				   "%lu ", freq);
>> -		freq++;
>> -	} while (1);
>> -	rcu_read_unlock();
>> +	if (df->profile->freq_table) {
>> +		for (i = 0; i < df->profile->max_state; i++)
>> +			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
>> +					   "%u ", df->profile->freq_table[i]);
> 
> You are hereby changing the semmantics of the original
> available_frequencies node.
> 
> When a frequency/voltage pair has been disabled (opp_disable), probably
> by opp_disable(), the frequency is no more "available".
> However, when the driver author supplied freq_table as well as OPP
> in order to see the statistics, the node will behave differently.
> 
> Please do not affect the current users as long as it does not give
> additional benefit or fix a bug.

I was actually trying to stick with the semantics as it was documented.
The documentation for this file says it'll show frequencies that are not
allowed by the current min/max settings either. To me, an OPP disable
seems similar to some frequencies "disabled" by min/max settings.

Giving preference to OPP is not a hard change to do, but it seems to go
againsts the documented semantics.

Thoughts?

-Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 04/29/2014 01:00 PM, Saravana Kannan wrote:
> On 04/27/2014 06:41 PM, MyungJoo Ham wrote:
>> You are hereby changing the semmantics of the original
>> available_frequencies node.
>>
>> When a frequency/voltage pair has been disabled (opp_disable), probably
>> by opp_disable(), the frequency is no more "available".
>> However, when the driver author supplied freq_table as well as OPP
>> in order to see the statistics, the node will behave differently.
>>
>> Please do not affect the current users as long as it does not give
>> additional benefit or fix a bug.
> 
> I was actually trying to stick with the semantics as it was documented.
> The documentation for this file says it'll show frequencies that are not
> allowed by the current min/max settings either. To me, an OPP disable
> seems similar to some frequencies "disabled" by min/max settings.
> 
> Giving preference to OPP is not a hard change to do, but it seems to go
> againsts the documented semantics.
> 
> Thoughts?

I'll send out another patch like you wanted -- with OPP being given
preference over freq_table when listing frequencies.

But I would still like to hear your thoughts. As of today, there's no
clean way to get the complete list of available frequencies that would
give a consistent output irrespective of the temporary limits/conditions
imposed by thermal, current limiting, etc. The round about way is to cat
trans_stat and parse the frequencies from that.

That's why I was trying to give preference to freq_table.

Thanks,
Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 04/29/2014 01:00 PM, Saravana Kannan wrote:
> On 04/27/2014 06:41 PM, MyungJoo Ham wrote:
>> You are hereby changing the semmantics of the original
>> available_frequencies node.
>>
>> When a frequency/voltage pair has been disabled (opp_disable), probably
>> by opp_disable(), the frequency is no more "available".
>> However, when the driver author supplied freq_table as well as OPP
>> in order to see the statistics, the node will behave differently.
>>
>> Please do not affect the current users as long as it does not give
>> additional benefit or fix a bug.
> 
> I was actually trying to stick with the semantics as it was documented.
> The documentation for this file says it'll show frequencies that are not
> allowed by the current min/max settings either. To me, an OPP disable
> seems similar to some frequencies "disabled" by min/max settings.
> 
> Giving preference to OPP is not a hard change to do, but it seems to go
> againsts the documented semantics.
> 
> Thoughts?

I'll send out another patch like you wanted -- with OPP being given
preference over freq_table when listing frequencies.

But I would still like to hear your thoughts. As of today, there's no
clean way to get the complete list of available frequencies that would
give a consistent output irrespective of the temporary limits/conditions
imposed by thermal, current limiting, etc. The round about way is to cat
trans_stat and parse the frequencies from that.

That's why I was trying to give preference to freq_table.

Thanks,
Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: Re: [PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[MyungJoo Ham]

> On 04/29/2014 01:00 PM, Saravana Kannan wrote:
> > On 04/27/2014 06:41 PM, MyungJoo Ham wrote:
> >> You are hereby changing the semmantics of the original
> >> available_frequencies node.
> >>
> >> When a frequency/voltage pair has been disabled (opp_disable), probably
> >> by opp_disable(), the frequency is no more "available".
> >> However, when the driver author supplied freq_table as well as OPP
> >> in order to see the statistics, the node will behave differently.
> >>
> >> Please do not affect the current users as long as it does not give
> >> additional benefit or fix a bug.
> > 
> > I was actually trying to stick with the semantics as it was documented.
> > The documentation for this file says it'll show frequencies that are not
> > allowed by the current min/max settings either. To me, an OPP disable
> > seems similar to some frequencies "disabled" by min/max settings.
> > 
> > Giving preference to OPP is not a hard change to do, but it seems to go
> > againsts the documented semantics.
> > 
> > Thoughts?
> 
> I'll send out another patch like you wanted -- with OPP being given
> preference over freq_table when listing frequencies.
> 
> But I would still like to hear your thoughts. As of today, there's no
> clean way to get the complete list of available frequencies that would
> give a consistent output irrespective of the temporary limits/conditions
> imposed by thermal, current limiting, etc. The round about way is to cat
> trans_stat and parse the frequencies from that.
> 
> That's why I was trying to give preference to freq_table.
> 
> Thanks,
> Saravana

The node, available_frequencies, was suggested before freq_table concept.
At that time, available_frequencies was supposed to show the list of
available OPP lists for those who use OPP for devfreq device, excluding
those disabled by OPP. (OPP lists are external to devfreq and devfreq's
min/max are internal to devfreq)

Locally, this node has been used to debug the behavior of a devfreq device.
With min/max nodes, we know the range while we cannot (easily at shell)
see which OPP points are available at the moment, where we have been able
to use available_frequencies.

We do not want to lose such capavility as long as we do not have OPP sysfs
automatically assigned to any OPP lists. If I remember correctly, we don't
have it, yet.


A. I want to minimize semantics changes in sysfs. Adding another without
interfering with previous usage is ok.
B. (more importantly) I don't want to lose the debugging capabilities.


Cheers,
MyungJoo.

> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
> 
>        
>   
>          
> 

Re: [PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 05/08/2014 03:02 AM, MyungJoo Ham wrote:
>> On 04/29/2014 01:00 PM, Saravana Kannan wrote:
>>> On 04/27/2014 06:41 PM, MyungJoo Ham wrote:
>>>> You are hereby changing the semmantics of the original
>>>> available_frequencies node.
>>>>
>>>> When a frequency/voltage pair has been disabled (opp_disable), probably
>>>> by opp_disable(), the frequency is no more "available".
>>>> However, when the driver author supplied freq_table as well as OPP
>>>> in order to see the statistics, the node will behave differently.
>>>>
>>>> Please do not affect the current users as long as it does not give
>>>> additional benefit or fix a bug.
>>>
>>> I was actually trying to stick with the semantics as it was documented.
>>> The documentation for this file says it'll show frequencies that are not
>>> allowed by the current min/max settings either. To me, an OPP disable
>>> seems similar to some frequencies "disabled" by min/max settings.
>>>
>>> Giving preference to OPP is not a hard change to do, but it seems to go
>>> againsts the documented semantics.
>>>
>>> Thoughts?
>>
>> I'll send out another patch like you wanted -- with OPP being given
>> preference over freq_table when listing frequencies.
>>
>> But I would still like to hear your thoughts. As of today, there's no
>> clean way to get the complete list of available frequencies that would
>> give a consistent output irrespective of the temporary limits/conditions
>> imposed by thermal, current limiting, etc. The round about way is to cat
>> trans_stat and parse the frequencies from that.
>>
>> That's why I was trying to give preference to freq_table.
>>
>> Thanks,
>> Saravana
> 
> The node, available_frequencies, was suggested before freq_table concept.
> At that time, available_frequencies was supposed to show the list of
> available OPP lists for those who use OPP for devfreq device, excluding
> those disabled by OPP. (OPP lists are external to devfreq and devfreq's
> min/max are internal to devfreq)
> 
> Locally, this node has been used to debug the behavior of a devfreq device.
> With min/max nodes, we know the range while we cannot (easily at shell)
> see which OPP points are available at the moment, where we have been able
> to use available_frequencies.
> 
> We do not want to lose such capavility as long as we do not have OPP sysfs
> automatically assigned to any OPP lists. If I remember correctly, we don't
> have it, yet.
> 
> 
> A. I want to minimize semantics changes in sysfs. Adding another without
> interfering with previous usage is ok.
> B. (more importantly) I don't want to lose the debugging capabilities.
> 

Thanks for the response MyungJoo. Makes sense. I was also discussing
this internally and was considering a "possible_frequencies" similar to
available vs possible CPUs.

I'll make such a patch for that and send it out. In that case, I'll
probably leave "available_frequencies" alone.

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 05/08/2014 03:02 AM, MyungJoo Ham wrote:
>> On 04/29/2014 01:00 PM, Saravana Kannan wrote:
>>> On 04/27/2014 06:41 PM, MyungJoo Ham wrote:
>>>> You are hereby changing the semmantics of the original
>>>> available_frequencies node.
>>>>
>>>> When a frequency/voltage pair has been disabled (opp_disable), probably
>>>> by opp_disable(), the frequency is no more "available".
>>>> However, when the driver author supplied freq_table as well as OPP
>>>> in order to see the statistics, the node will behave differently.
>>>>
>>>> Please do not affect the current users as long as it does not give
>>>> additional benefit or fix a bug.
>>>
>>> I was actually trying to stick with the semantics as it was documented.
>>> The documentation for this file says it'll show frequencies that are not
>>> allowed by the current min/max settings either. To me, an OPP disable
>>> seems similar to some frequencies "disabled" by min/max settings.
>>>
>>> Giving preference to OPP is not a hard change to do, but it seems to go
>>> againsts the documented semantics.
>>>
>>> Thoughts?
>>
>> I'll send out another patch like you wanted -- with OPP being given
>> preference over freq_table when listing frequencies.
>>
>> But I would still like to hear your thoughts. As of today, there's no
>> clean way to get the complete list of available frequencies that would
>> give a consistent output irrespective of the temporary limits/conditions
>> imposed by thermal, current limiting, etc. The round about way is to cat
>> trans_stat and parse the frequencies from that.
>>
>> That's why I was trying to give preference to freq_table.
>>
>> Thanks,
>> Saravana
> 
> The node, available_frequencies, was suggested before freq_table concept.
> At that time, available_frequencies was supposed to show the list of
> available OPP lists for those who use OPP for devfreq device, excluding
> those disabled by OPP. (OPP lists are external to devfreq and devfreq's
> min/max are internal to devfreq)
> 
> Locally, this node has been used to debug the behavior of a devfreq device.
> With min/max nodes, we know the range while we cannot (easily at shell)
> see which OPP points are available at the moment, where we have been able
> to use available_frequencies.
> 
> We do not want to lose such capavility as long as we do not have OPP sysfs
> automatically assigned to any OPP lists. If I remember correctly, we don't
> have it, yet.
> 
> 
> A. I want to minimize semantics changes in sysfs. Adding another without
> interfering with previous usage is ok.
> B. (more importantly) I don't want to lose the debugging capabilities.
> 

Thanks for the response MyungJoo. Makes sense. I was also discussing
this internally and was considering a "possible_frequencies" similar to
available vs possible CPUs.

I'll make such a patch for that and send it out. In that case, I'll
probably leave "available_frequencies" alone.

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: Re: [PATCH] PM / devfreq: Use freq_table for available_frequencies

[함명주]

> On 04/14/2014 06:36 PM, 함명주 wrote:
> >> MyungJoo/Kyungmin,
> >>
> >> Bump. Can we accept this patch please?
> >>
> >> -Saravana
> > 
> > Nack.
> > 
> > Please note that freq_table is also an optional value, which may
> > be null.
> 
> Ah, I saw that the max_freq would be zero if freq_table was NULL and I
> assumed that it can't be NULL. But I see that the max_freq limit is not
> applied if it's zero. Thanks for catching it.
> 
> > Besides, please be aware that your code is under rcu_read_lock().
> 
> Valid point. I was just trying to keep the diff simple. No one's really
> going to be catting this file often when performance matters.
> 
> > 
> > 
> > Cheers,
> > MyungJoo.
> > 
> > ps. I'll send a related patch (avoid accessing null but not-an-error
> > pointer at other sysfs nodes). Thank you for letting me catch such bugs anyway.
> 
> I can go ahead and do this myself if you don't mind.

No, we don't need it. It was a false alarm.
Reading again, I've found that we've already made other sysfs nodes
check if either freq_table is null or its size is 0.

So, we only need to look at this available_frequencies node now.

I'll add some notes on the ABI doc for available_frequencies soon.

Cheers,
MyungJoo.

> 
> -Saravana
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
> 
>        
>   
>          
> 

[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

Some devices use freq_table instead of OPP. For those devices, the
available_frequencies file shows up empty. Fix that by using freq_table to
generate the available_frequencies data when it's available.

OPP find frequency APIs also skips frequencies that have been temporarily
disabled (say, due to thermal, etc). Since available_frequencies is
supposed to show the entire list of available frequencies without taking
temporary limits into consideration, preference is given to freq_table when
available.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 drivers/devfreq/devfreq.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2042ec3..527cbe2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -912,20 +912,27 @@ static ssize_t available_frequencies_show(struct device *d,
 	struct devfreq *df = to_devfreq(d);
 	struct device *dev = df->dev.parent;
 	struct dev_pm_opp *opp;
+	unsigned int i = 0;
 	ssize_t count = 0;
 	unsigned long freq = 0;
 
-	rcu_read_lock();
-	do {
-		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
-		if (IS_ERR(opp))
-			break;
-
-		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
-				   "%lu ", freq);
-		freq++;
-	} while (1);
-	rcu_read_unlock();
+	if (df->profile->freq_table) {
+		for (i = 0; i < df->profile->max_state; i++)
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					   "%u ", df->profile->freq_table[i]);
+	} else {
+		rcu_read_lock();
+		do {
+			opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+			if (IS_ERR(opp))
+				break;
+
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					   "%lu ", freq);
+			freq++;
+		} while (1);
+		rcu_read_unlock();
+	}
 
 	/* Truncate the trailing space */
 	if (count)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

Some devices use freq_table instead of OPP. For those devices, the
available_frequencies file shows up empty. Fix that by using freq_table to
generate the available_frequencies data when it's available.

OPP find frequency APIs also skips frequencies that have been temporarily
disabled (say, due to thermal, etc). Since available_frequencies is
supposed to show the entire list of available frequencies without taking
temporary limits into consideration, preference is given to freq_table when
available.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 drivers/devfreq/devfreq.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2042ec3..527cbe2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -912,20 +912,27 @@ static ssize_t available_frequencies_show(struct device *d,
 	struct devfreq *df = to_devfreq(d);
 	struct device *dev = df->dev.parent;
 	struct dev_pm_opp *opp;
+	unsigned int i = 0;
 	ssize_t count = 0;
 	unsigned long freq = 0;
 
-	rcu_read_lock();
-	do {
-		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
-		if (IS_ERR(opp))
-			break;
-
-		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
-				   "%lu ", freq);
-		freq++;
-	} while (1);
-	rcu_read_unlock();
+	if (df->profile->freq_table) {
+		for (i = 0; i < df->profile->max_state; i++)
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					   "%u ", df->profile->freq_table[i]);
+	} else {
+		rcu_read_lock();
+		do {
+			opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+			if (IS_ERR(opp))
+				break;
+
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					   "%lu ", freq);
+			freq++;
+		} while (1);
+		rcu_read_unlock();
+	}
 
 	/* Truncate the trailing space */
 	if (count)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation