| Message ID | 1507691364-3899-4-git-send-email-cw00.choi@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
> The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq > are available or not. Those sysfs nodes show just the stored value > in the struct devfreq. > > The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}() > might change the state of the device's supported frequency. This patch > shows the available minimum and maximum frequency through sysfs node. > > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > --- > drivers/devfreq/devfreq.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 2ce1fd0a1324..799a0cf75d39 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, > 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); > + struct devfreq *df = to_devfreq(dev); > + unsigned long min_freq = to_devfreq(dev)->min_freq; > + unsigned long available_min_freq = find_available_min_freq(df); > + > + if (available_min_freq != 0 && min_freq < available_min_freq) nitpick: If available_min_freq == 0, it can't be min_freq < available_min_freq anyway; it's unsigned. > + min_freq = available_min_freq; > + > + return sprintf(buf, "%lu\n", min_freq); > } > > static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > 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); > + struct devfreq *df = to_devfreq(dev); > + unsigned long max_freq = to_devfreq(dev)->max_freq; > + unsigned long available_max_freq = find_available_max_freq(df); > + > + if (available_max_freq != 0 && max_freq > available_max_freq) > + max_freq = available_max_freq; similar here. > + > + return sprintf(buf, "%lu\n", max_freq); > } > static DEVICE_ATTR_RW(max_freq);
On Wed, Oct 11, 2017 at 8:15 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote: >> The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq >> are available or not. Those sysfs nodes show just the stored value >> in the struct devfreq. >> >> The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}() >> might change the state of the device's supported frequency. This patch >> shows the available minimum and maximum frequency through sysfs node. >> >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >> --- >> drivers/devfreq/devfreq.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 2ce1fd0a1324..799a0cf75d39 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, >> 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); >> + struct devfreq *df = to_devfreq(dev); >> + unsigned long min_freq = to_devfreq(dev)->min_freq; >> + unsigned long available_min_freq = find_available_min_freq(df); >> + >> + if (available_min_freq != 0 && min_freq < available_min_freq) > > nitpick: > > If available_min_freq == 0, > it can't be min_freq < available_min_freq anyway; > it's unsigned. If the dev_pm_opp_find_*() return the error in the find_available_min_freq(), avaiable_min_freq is zero. So, if available_min_freq is zero, min_freq_show doesn't need to compare 'min_freq < available_min_freq'. In result, if 'available_min_freq' is zero, min_freq_show() only considers the 'min_freq' variable. > >> + min_freq = available_min_freq; >> + >> + return sprintf(buf, "%lu\n", min_freq); >> } >> >> static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >> @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >> 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); >> + struct devfreq *df = to_devfreq(dev); >> + unsigned long max_freq = to_devfreq(dev)->max_freq; >> + unsigned long available_max_freq = find_available_max_freq(df); >> + >> + if (available_max_freq != 0 && max_freq > available_max_freq) >> + max_freq = available_max_freq; > > similar here. ditto. > >> + >> + return sprintf(buf, "%lu\n", max_freq); >> } >> static DEVICE_ATTR_RW(max_freq);
On 2017년 10월 11일 21:57, Chanwoo Choi wrote: > On Wed, Oct 11, 2017 at 8:15 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote: >>> The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq >>> are available or not. Those sysfs nodes show just the stored value >>> in the struct devfreq. >>> >>> The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}() >>> might change the state of the device's supported frequency. This patch >>> shows the available minimum and maximum frequency through sysfs node. >>> >>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>> --- >>> drivers/devfreq/devfreq.c | 18 ++++++++++++++++-- >>> 1 file changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 2ce1fd0a1324..799a0cf75d39 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, >>> 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); >>> + struct devfreq *df = to_devfreq(dev); >>> + unsigned long min_freq = to_devfreq(dev)->min_freq; >>> + unsigned long available_min_freq = find_available_min_freq(df); >>> + >>> + if (available_min_freq != 0 && min_freq < available_min_freq) >> >> nitpick: >> >> If available_min_freq == 0, >> it can't be min_freq < available_min_freq anyway; >> it's unsigned. > > If the dev_pm_opp_find_*() return the error in the find_available_min_freq(), > avaiable_min_freq is zero. So, if available_min_freq is zero, > min_freq_show doesn't need to compare 'min_freq < available_min_freq'. > > In result, if 'available_min_freq' is zero, min_freq_show() only considers > the 'min_freq' variable. I'll modify this patch as following: How about that? + if (available_min_freq == 0) + goto out; + else if (min_freq < available_min_freq) + min_freq = available_min_freq; + +out: + return sprintf(buf, "%lu\n", min_freq); >> >>> + min_freq = available_min_freq; >>> + >>> + return sprintf(buf, "%lu\n", min_freq); >>> } >>> >>> static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >>> @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >>> 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); >>> + struct devfreq *df = to_devfreq(dev); >>> + unsigned long max_freq = to_devfreq(dev)->max_freq; >>> + unsigned long available_max_freq = find_available_max_freq(df); >>> + >>> + if (available_max_freq != 0 && max_freq > available_max_freq) >>> + max_freq = available_max_freq; >> >> similar here. > > ditto. > >> >>> + >>> + return sprintf(buf, "%lu\n", max_freq); >>> } >>> static DEVICE_ATTR_RW(max_freq); > >
Hi, On 2017년 10월 12일 13:08, Chanwoo Choi wrote: > On 2017년 10월 11일 21:57, Chanwoo Choi wrote: >> On Wed, Oct 11, 2017 at 8:15 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote: >>>> The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq >>>> are available or not. Those sysfs nodes show just the stored value >>>> in the struct devfreq. >>>> >>>> The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}() >>>> might change the state of the device's supported frequency. This patch >>>> shows the available minimum and maximum frequency through sysfs node. >>>> >>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>> --- >>>> drivers/devfreq/devfreq.c | 18 ++++++++++++++++-- >>>> 1 file changed, 16 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index 2ce1fd0a1324..799a0cf75d39 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, >>>> 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); >>>> + struct devfreq *df = to_devfreq(dev); >>>> + unsigned long min_freq = to_devfreq(dev)->min_freq; >>>> + unsigned long available_min_freq = find_available_min_freq(df); >>>> + >>>> + if (available_min_freq != 0 && min_freq < available_min_freq) >>> >>> nitpick: >>> >>> If available_min_freq == 0, >>> it can't be min_freq < available_min_freq anyway; >>> it's unsigned. >> >> If the dev_pm_opp_find_*() return the error in the find_available_min_freq(), >> avaiable_min_freq is zero. So, if available_min_freq is zero, >> min_freq_show doesn't need to compare 'min_freq < available_min_freq'. >> >> In result, if 'available_min_freq' is zero, min_freq_show() only considers >> the 'min_freq' variable. > > I'll modify this patch as following: How about that? > > + if (available_min_freq == 0) > + goto out; > + else if (min_freq < available_min_freq) > + min_freq = available_min_freq; > + > +out: > + return sprintf(buf, "%lu\n", min_freq); > Please ignore this approach because I'll use the new 'scaling_min/max_freq' variables on v4. > >>> >>>> + min_freq = available_min_freq; >>>> + >>>> + return sprintf(buf, "%lu\n", min_freq); >>>> } >>>> >>>> static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >>>> @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >>>> 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); >>>> + struct devfreq *df = to_devfreq(dev); >>>> + unsigned long max_freq = to_devfreq(dev)->max_freq; >>>> + unsigned long available_max_freq = find_available_max_freq(df); >>>> + >>>> + if (available_max_freq != 0 && max_freq > available_max_freq) >>>> + max_freq = available_max_freq; >>> >>> similar here. >> >> ditto. >> >>> >>>> + >>>> + return sprintf(buf, "%lu\n", max_freq); >>>> } >>>> static DEVICE_ATTR_RW(max_freq); >> >> > >
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 2ce1fd0a1324..799a0cf75d39 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, 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); + struct devfreq *df = to_devfreq(dev); + unsigned long min_freq = to_devfreq(dev)->min_freq; + unsigned long available_min_freq = find_available_min_freq(df); + + if (available_min_freq != 0 && min_freq < available_min_freq) + min_freq = available_min_freq; + + return sprintf(buf, "%lu\n", min_freq); } static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, 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); + struct devfreq *df = to_devfreq(dev); + unsigned long max_freq = to_devfreq(dev)->max_freq; + unsigned long available_max_freq = find_available_max_freq(df); + + if (available_max_freq != 0 && max_freq > available_max_freq) + max_freq = available_max_freq; + + return sprintf(buf, "%lu\n", max_freq); } static DEVICE_ATTR_RW(max_freq);
The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq are available or not. Those sysfs nodes show just the stored value in the struct devfreq. The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}() might change the state of the device's supported frequency. This patch shows the available minimum and maximum frequency through sysfs node. Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/devfreq/devfreq.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)