* [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa @ 2019-09-25 18:43 ` Matthias Kaehlcke 2019-09-25 18:43 ` [PATCH 2/2] devfreq: Rename df->previous_freq to df->cur_freq Matthias Kaehlcke ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-25 18:43 UTC (permalink / raw) To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar Cc: linux-pm, linux-kernel, linux-tegra, Douglas Anderson, Matthias Kaehlcke devfreq has two functions with very similar names, devfreq_update_status() and devfreq_update_stats(). _update_status() currently updates frequency transitions statistics, while _update_stats() retrieves the device 'status'. The function names are inversed with respect to what the functions are actually doing, rename devfreq_update_status() to devfreq_update_stats() and viceversa. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- We could also rename the current devfreq_update_stats() to devfreq_refresh_status() to make it easier to distinguish it from devfreq_update_stats(). --- drivers/devfreq/devfreq.c | 12 ++++++------ drivers/devfreq/governor.h | 4 ++-- drivers/devfreq/governor_passive.c | 2 +- drivers/devfreq/governor_simpleondemand.c | 2 +- drivers/devfreq/tegra30-devfreq.c | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 446490c9d635..fb4318d59aa9 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -151,11 +151,11 @@ static int set_freq_table(struct devfreq *devfreq) } /** - * devfreq_update_status() - Update statistics of devfreq behavior + * devfreq_update_stats() - Update statistics of devfreq behavior * @devfreq: the devfreq instance * @freq: the update target frequency */ -int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) +int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) { int lev, prev_lev, ret = 0; unsigned long cur_time; @@ -191,7 +191,7 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) devfreq->last_stat_updated = cur_time; return ret; } -EXPORT_SYMBOL(devfreq_update_status); +EXPORT_SYMBOL(devfreq_update_stats); /** * find_devfreq_governor() - find devfreq governor from name @@ -311,7 +311,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, freqs.new = new_freq; devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); - if (devfreq_update_status(devfreq, new_freq)) + if (devfreq_update_stats(devfreq, new_freq)) dev_err(&devfreq->dev, "Couldn't update frequency transition information.\n"); @@ -450,7 +450,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) return; } - devfreq_update_status(devfreq, devfreq->previous_freq); + devfreq_update_stats(devfreq, devfreq->previous_freq); devfreq->stop_polling = true; mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work); @@ -1398,7 +1398,7 @@ static ssize_t trans_stat_show(struct device *dev, unsigned int max_state = devfreq->profile->max_state; if (!devfreq->stop_polling && - devfreq_update_status(devfreq, devfreq->previous_freq)) + devfreq_update_stats(devfreq, devfreq->previous_freq)) return 0; if (max_state == 0) return sprintf(buf, "Not Supported.\n"); diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h index bbe5ff9fcecf..e11f447be2b5 100644 --- a/drivers/devfreq/governor.h +++ b/drivers/devfreq/governor.h @@ -64,9 +64,9 @@ extern void devfreq_interval_update(struct devfreq *devfreq, extern int devfreq_add_governor(struct devfreq_governor *governor); extern int devfreq_remove_governor(struct devfreq_governor *governor); -extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq); +extern int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq); -static inline int devfreq_update_stats(struct devfreq *df) +static inline int devfreq_update_status(struct devfreq *df) { return df->profile->get_dev_status(df->dev.parent, &df->last_status); } diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c index be6eeab9c814..1c746b96d3db 100644 --- a/drivers/devfreq/governor_passive.c +++ b/drivers/devfreq/governor_passive.c @@ -110,7 +110,7 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq) goto out; if (devfreq->profile->freq_table - && (devfreq_update_status(devfreq, freq))) + && (devfreq_update_stats(devfreq, freq))) dev_err(&devfreq->dev, "Couldn't update frequency transition information.\n"); diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c index 3d809f228619..2cbf26bdcfd6 100644 --- a/drivers/devfreq/governor_simpleondemand.c +++ b/drivers/devfreq/governor_simpleondemand.c @@ -25,7 +25,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; struct devfreq_simple_ondemand_data *data = df->data; - err = devfreq_update_stats(df); + err = devfreq_update_status(df); if (err) return err; diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c index a6ba75f4106d..536273a811fe 100644 --- a/drivers/devfreq/tegra30-devfreq.c +++ b/drivers/devfreq/tegra30-devfreq.c @@ -526,7 +526,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq, unsigned int i; int err; - err = devfreq_update_stats(devfreq); + err = devfreq_update_status(devfreq); if (err) return err; -- 2.23.0.444.g18eeb5a265-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] devfreq: Rename df->previous_freq to df->cur_freq 2019-09-25 18:43 ` [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Matthias Kaehlcke @ 2019-09-25 18:43 ` Matthias Kaehlcke 2019-09-26 1:41 ` Chanwoo Choi 2019-09-26 1:36 ` [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Chanwoo Choi 2019-09-26 7:21 ` Ben Dooks 2 siblings, 1 reply; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-25 18:43 UTC (permalink / raw) To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar Cc: linux-pm, linux-kernel, linux-tegra, Douglas Anderson, Matthias Kaehlcke The vast majority of code uses df->previous_freq to get the current frequency of the devfreq device, not the previous one. Rename the struct field to reflect this. Add a comment to devfreq_update_stats() to make clear that df->cur_freq must only be updated after calling this function in the context of a frequency transition. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/devfreq/devfreq.c | 28 ++++++++++++++++------------ drivers/devfreq/governor_passive.c | 6 +++--- drivers/devfreq/governor_userspace.c | 2 +- drivers/devfreq/tegra20-devfreq.c | 2 +- drivers/devfreq/tegra30-devfreq.c | 2 +- include/linux/devfreq.h | 4 ++-- include/trace/events/devfreq.h | 2 +- 7 files changed, 25 insertions(+), 21 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index fb4318d59aa9..bf42130bb4ec 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -154,6 +154,10 @@ static int set_freq_table(struct devfreq *devfreq) * devfreq_update_stats() - Update statistics of devfreq behavior * @devfreq: the devfreq instance * @freq: the update target frequency + * + * If the function is called in the context of a frequency transition + * it expects df->cur_freq to contain the value *before* the transition. + * The field must only be updated after calling devfreq_update_stats(). */ int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) { @@ -162,11 +166,11 @@ int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) cur_time = jiffies; - /* Immediately exit if previous_freq is not initialized yet. */ - if (!devfreq->previous_freq) + /* Immediately exit if cur_freq is not initialized yet. */ + if (!devfreq->cur_freq) goto out; - prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq); + prev_lev = devfreq_get_freq_level(devfreq, devfreq->cur_freq); if (prev_lev < 0) { ret = prev_lev; goto out; @@ -295,7 +299,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, if (devfreq->profile->get_cur_freq) devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); else - cur_freq = devfreq->previous_freq; + cur_freq = devfreq->cur_freq; freqs.old = cur_freq; freqs.new = new_freq; @@ -315,7 +319,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, dev_err(&devfreq->dev, "Couldn't update frequency transition information.\n"); - devfreq->previous_freq = new_freq; + devfreq->cur_freq = new_freq; if (devfreq->suspend_freq) devfreq->resume_freq = cur_freq; @@ -450,7 +454,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) return; } - devfreq_update_stats(devfreq, devfreq->previous_freq); + devfreq_update_stats(devfreq, devfreq->cur_freq); devfreq->stop_polling = true; mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work); @@ -483,7 +487,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) if (devfreq->profile->get_cur_freq && !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) - devfreq->previous_freq = freq; + devfreq->cur_freq = freq; out: mutex_unlock(&devfreq->lock); @@ -644,7 +648,7 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->dev.release = devfreq_dev_release; devfreq->profile = profile; strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); - devfreq->previous_freq = profile->initial_freq; + devfreq->cur_freq = profile->initial_freq; devfreq->last_status.current_frequency = profile->initial_freq; devfreq->data = data; devfreq->nb.notifier_call = devfreq_notifier_call; @@ -1235,14 +1239,14 @@ static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr, !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) return sprintf(buf, "%lu\n", freq); - return sprintf(buf, "%lu\n", devfreq->previous_freq); + return sprintf(buf, "%lu\n", devfreq->cur_freq); } static DEVICE_ATTR_RO(cur_freq); static ssize_t target_freq_show(struct device *dev, struct device_attribute *attr, char *buf) { - return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq); + return sprintf(buf, "%lu\n", to_devfreq(dev)->cur_freq); } static DEVICE_ATTR_RO(target_freq); @@ -1398,7 +1402,7 @@ static ssize_t trans_stat_show(struct device *dev, unsigned int max_state = devfreq->profile->max_state; if (!devfreq->stop_polling && - devfreq_update_stats(devfreq, devfreq->previous_freq)) + devfreq_update_stats(devfreq, devfreq->cur_freq)) return 0; if (max_state == 0) return sprintf(buf, "Not Supported.\n"); @@ -1413,7 +1417,7 @@ static ssize_t trans_stat_show(struct device *dev, for (i = 0; i < max_state; i++) { if (devfreq->profile->freq_table[i] - == devfreq->previous_freq) { + == devfreq->cur_freq) { len += sprintf(buf + len, "*"); } else { len += sprintf(buf + len, " "); diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c index 1c746b96d3db..2d818eaceb39 100644 --- a/drivers/devfreq/governor_passive.c +++ b/drivers/devfreq/governor_passive.c @@ -114,7 +114,7 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq) dev_err(&devfreq->dev, "Couldn't update frequency transition information.\n"); - devfreq->previous_freq = freq; + devfreq->cur_freq = freq; out: mutex_unlock(&devfreq->lock); @@ -134,11 +134,11 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb, switch (event) { case DEVFREQ_PRECHANGE: - if (parent->previous_freq > freq) + if (parent->cur_freq > freq) update_devfreq_passive(devfreq, freq); break; case DEVFREQ_POSTCHANGE: - if (parent->previous_freq < freq) + if (parent->cur_freq < freq) update_devfreq_passive(devfreq, freq); break; } diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c index af94942fcf95..566b8d1f0c17 100644 --- a/drivers/devfreq/governor_userspace.c +++ b/drivers/devfreq/governor_userspace.c @@ -26,7 +26,7 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq) if (data->valid) *freq = data->user_frequency; else - *freq = df->previous_freq; /* No user freq specified yet */ + *freq = df->cur_freq; /* No user freq specified yet */ return 0; } diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c index ff82bac9ee4e..f99bd6557df5 100644 --- a/drivers/devfreq/tegra20-devfreq.c +++ b/drivers/devfreq/tegra20-devfreq.c @@ -61,7 +61,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, return 0; restore_min_rate: - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq); + clk_set_min_rate(tegra->emc_clock, devfreq->cur_freq); return err; } diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c index 536273a811fe..7b3bf7a0b15f 100644 --- a/drivers/devfreq/tegra30-devfreq.c +++ b/drivers/devfreq/tegra30-devfreq.c @@ -474,7 +474,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, return 0; restore_min_rate: - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq); + clk_set_min_rate(tegra->emc_clock, devfreq->cur_freq); return err; } diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index 2bae9ed3c783..21d0108df4c5 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -120,7 +120,7 @@ struct devfreq_dev_profile { * reevaluate operable frequencies. Devfreq users may use * devfreq.nb to the corresponding register notifier call chain. * @work: delayed work for load monitoring. - * @previous_freq: previously configured frequency value. + * @cur_freq: the current frequency. * @data: Private data of the governor. The devfreq framework does not * touch this. * @min_freq: Limit minimum frequency requested by user (0: none) @@ -156,7 +156,7 @@ struct devfreq { struct notifier_block nb; struct delayed_work work; - unsigned long previous_freq; + unsigned long cur_freq; struct devfreq_dev_status last_status; void *data; /* private data for governors */ diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h index cf5b8772175d..916cfaed5489 100644 --- a/include/trace/events/devfreq.h +++ b/include/trace/events/devfreq.h @@ -22,7 +22,7 @@ TRACE_EVENT(devfreq_monitor, ), TP_fast_assign( - __entry->freq = devfreq->previous_freq; + __entry->freq = devfreq->cur_freq; __entry->busy_time = devfreq->last_status.busy_time; __entry->total_time = devfreq->last_status.total_time; __entry->polling_ms = devfreq->profile->polling_ms; -- 2.23.0.444.g18eeb5a265-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] devfreq: Rename df->previous_freq to df->cur_freq 2019-09-25 18:43 ` [PATCH 2/2] devfreq: Rename df->previous_freq to df->cur_freq Matthias Kaehlcke @ 2019-09-26 1:41 ` Chanwoo Choi 2019-09-26 17:35 ` Matthias Kaehlcke 0 siblings, 1 reply; 9+ messages in thread From: Chanwoo Choi @ 2019-09-26 1:41 UTC (permalink / raw) To: Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar Cc: linux-pm, linux-kernel, linux-tegra, Douglas Anderson Hi, I'm not objecting this patch. But, as I commented on previous patch, Actually, according to reference time of the 'df->previous_freq', 'previous_freq' is proper or 'cur_freq is proper. But, In the comment of 'struct devfreq', it means the configured time as following: It was the intention of author (Myungjoo). * @previous_freq: previously configured frequency value. I think that it's not problem ans better to keep the name if possible. I leave the final decision of this patch to Myungjoo. If he like this patch, I have no any objection. On 19. 9. 26. 오전 3:43, Matthias Kaehlcke wrote: > The vast majority of code uses df->previous_freq to get the current > frequency of the devfreq device, not the previous one. Rename the > struct field to reflect this. > > Add a comment to devfreq_update_stats() to make clear that df->cur_freq > must only be updated after calling this function in the context of a > frequency transition. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/devfreq/devfreq.c | 28 ++++++++++++++++------------ > drivers/devfreq/governor_passive.c | 6 +++--- > drivers/devfreq/governor_userspace.c | 2 +- > drivers/devfreq/tegra20-devfreq.c | 2 +- > drivers/devfreq/tegra30-devfreq.c | 2 +- > include/linux/devfreq.h | 4 ++-- > include/trace/events/devfreq.h | 2 +- > 7 files changed, 25 insertions(+), 21 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index fb4318d59aa9..bf42130bb4ec 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -154,6 +154,10 @@ static int set_freq_table(struct devfreq *devfreq) > * devfreq_update_stats() - Update statistics of devfreq behavior > * @devfreq: the devfreq instance > * @freq: the update target frequency > + * > + * If the function is called in the context of a frequency transition > + * it expects df->cur_freq to contain the value *before* the transition. > + * The field must only be updated after calling devfreq_update_stats(). > */ > int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > { > @@ -162,11 +166,11 @@ int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > > cur_time = jiffies; > > - /* Immediately exit if previous_freq is not initialized yet. */ > - if (!devfreq->previous_freq) > + /* Immediately exit if cur_freq is not initialized yet. */ > + if (!devfreq->cur_freq) > goto out; > > - prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq); > + prev_lev = devfreq_get_freq_level(devfreq, devfreq->cur_freq); > if (prev_lev < 0) { > ret = prev_lev; > goto out; > @@ -295,7 +299,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, > if (devfreq->profile->get_cur_freq) > devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); > else > - cur_freq = devfreq->previous_freq; > + cur_freq = devfreq->cur_freq; > > freqs.old = cur_freq; > freqs.new = new_freq; > @@ -315,7 +319,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, > dev_err(&devfreq->dev, > "Couldn't update frequency transition information.\n"); > > - devfreq->previous_freq = new_freq; > + devfreq->cur_freq = new_freq; > > if (devfreq->suspend_freq) > devfreq->resume_freq = cur_freq; > @@ -450,7 +454,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) > return; > } > > - devfreq_update_stats(devfreq, devfreq->previous_freq); > + devfreq_update_stats(devfreq, devfreq->cur_freq); > devfreq->stop_polling = true; > mutex_unlock(&devfreq->lock); > cancel_delayed_work_sync(&devfreq->work); > @@ -483,7 +487,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > > if (devfreq->profile->get_cur_freq && > !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) > - devfreq->previous_freq = freq; > + devfreq->cur_freq = freq; > > out: > mutex_unlock(&devfreq->lock); > @@ -644,7 +648,7 @@ struct devfreq *devfreq_add_device(struct device *dev, > devfreq->dev.release = devfreq_dev_release; > devfreq->profile = profile; > strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); > - devfreq->previous_freq = profile->initial_freq; > + devfreq->cur_freq = profile->initial_freq; > devfreq->last_status.current_frequency = profile->initial_freq; > devfreq->data = data; > devfreq->nb.notifier_call = devfreq_notifier_call; > @@ -1235,14 +1239,14 @@ static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr, > !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) > return sprintf(buf, "%lu\n", freq); > > - return sprintf(buf, "%lu\n", devfreq->previous_freq); > + return sprintf(buf, "%lu\n", devfreq->cur_freq); > } > static DEVICE_ATTR_RO(cur_freq); > > static ssize_t target_freq_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq); > + return sprintf(buf, "%lu\n", to_devfreq(dev)->cur_freq); > } > static DEVICE_ATTR_RO(target_freq); > > @@ -1398,7 +1402,7 @@ static ssize_t trans_stat_show(struct device *dev, > unsigned int max_state = devfreq->profile->max_state; > > if (!devfreq->stop_polling && > - devfreq_update_stats(devfreq, devfreq->previous_freq)) > + devfreq_update_stats(devfreq, devfreq->cur_freq)) > return 0; > if (max_state == 0) > return sprintf(buf, "Not Supported.\n"); > @@ -1413,7 +1417,7 @@ static ssize_t trans_stat_show(struct device *dev, > > for (i = 0; i < max_state; i++) { > if (devfreq->profile->freq_table[i] > - == devfreq->previous_freq) { > + == devfreq->cur_freq) { > len += sprintf(buf + len, "*"); > } else { > len += sprintf(buf + len, " "); > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c > index 1c746b96d3db..2d818eaceb39 100644 > --- a/drivers/devfreq/governor_passive.c > +++ b/drivers/devfreq/governor_passive.c > @@ -114,7 +114,7 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq) > dev_err(&devfreq->dev, > "Couldn't update frequency transition information.\n"); > > - devfreq->previous_freq = freq; > + devfreq->cur_freq = freq; > > out: > mutex_unlock(&devfreq->lock); > @@ -134,11 +134,11 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb, > > switch (event) { > case DEVFREQ_PRECHANGE: > - if (parent->previous_freq > freq) > + if (parent->cur_freq > freq) > update_devfreq_passive(devfreq, freq); > break; > case DEVFREQ_POSTCHANGE: > - if (parent->previous_freq < freq) > + if (parent->cur_freq < freq) > update_devfreq_passive(devfreq, freq); > break; > } > diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c > index af94942fcf95..566b8d1f0c17 100644 > --- a/drivers/devfreq/governor_userspace.c > +++ b/drivers/devfreq/governor_userspace.c > @@ -26,7 +26,7 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq) > if (data->valid) > *freq = data->user_frequency; > else > - *freq = df->previous_freq; /* No user freq specified yet */ > + *freq = df->cur_freq; /* No user freq specified yet */ > > return 0; > } > diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c > index ff82bac9ee4e..f99bd6557df5 100644 > --- a/drivers/devfreq/tegra20-devfreq.c > +++ b/drivers/devfreq/tegra20-devfreq.c > @@ -61,7 +61,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > return 0; > > restore_min_rate: > - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq); > + clk_set_min_rate(tegra->emc_clock, devfreq->cur_freq); > > return err; > } > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c > index 536273a811fe..7b3bf7a0b15f 100644 > --- a/drivers/devfreq/tegra30-devfreq.c > +++ b/drivers/devfreq/tegra30-devfreq.c > @@ -474,7 +474,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > return 0; > > restore_min_rate: > - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq); > + clk_set_min_rate(tegra->emc_clock, devfreq->cur_freq); > > return err; > } > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index 2bae9ed3c783..21d0108df4c5 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -120,7 +120,7 @@ struct devfreq_dev_profile { > * reevaluate operable frequencies. Devfreq users may use > * devfreq.nb to the corresponding register notifier call chain. > * @work: delayed work for load monitoring. > - * @previous_freq: previously configured frequency value. > + * @cur_freq: the current frequency. > * @data: Private data of the governor. The devfreq framework does not > * touch this. > * @min_freq: Limit minimum frequency requested by user (0: none) > @@ -156,7 +156,7 @@ struct devfreq { > struct notifier_block nb; > struct delayed_work work; > > - unsigned long previous_freq; > + unsigned long cur_freq; > struct devfreq_dev_status last_status; > > void *data; /* private data for governors */ > diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h > index cf5b8772175d..916cfaed5489 100644 > --- a/include/trace/events/devfreq.h > +++ b/include/trace/events/devfreq.h > @@ -22,7 +22,7 @@ TRACE_EVENT(devfreq_monitor, > ), > > TP_fast_assign( > - __entry->freq = devfreq->previous_freq; > + __entry->freq = devfreq->cur_freq; > __entry->busy_time = devfreq->last_status.busy_time; > __entry->total_time = devfreq->last_status.total_time; > __entry->polling_ms = devfreq->profile->polling_ms; > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] devfreq: Rename df->previous_freq to df->cur_freq 2019-09-26 1:41 ` Chanwoo Choi @ 2019-09-26 17:35 ` Matthias Kaehlcke 0 siblings, 0 replies; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-26 17:35 UTC (permalink / raw) To: Chanwoo Choi Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar, linux-pm, linux-kernel, linux-tegra, Douglas Anderson On Thu, Sep 26, 2019 at 10:41:24AM +0900, Chanwoo Choi wrote: > Hi, > > I'm not objecting this patch. > But, as I commented on previous patch, > Actually, according to reference time of the 'df->previous_freq', > 'previous_freq' is proper or 'cur_freq is proper. > But, In the comment of 'struct devfreq', it means the configured time > as following: It was the intention of author (Myungjoo). > * @previous_freq: previously configured frequency value. The name made perfect sense in the context of the original commit a3c98b8b2ede ("PM: Introduce devfreq: generic DVFS framework with device-specific OPPs"), where the field was indeed only used to store the previous frequency. However the devfreq sub-system has evolved in the past 8 years, and nowadays the use is different. > I think that it's not problem ans better to keep the name if possible. IMO it's a strong signal that over 85% of the users of the variable use it in a way that doesn't match it's name, i.e. you read the code and wonder 'why is it using the previous frequency here???'. Code should be as self explanatory as possible, misleading variable don't help with that. > I leave the final decision of this patch to Myungjoo. > If he like this patch, I have no any objection. Myungjoo, what do you think about the patch? > On 19. 9. 26. 오전 3:43, Matthias Kaehlcke wrote: > > The vast majority of code uses df->previous_freq to get the current > > frequency of the devfreq device, not the previous one. Rename the > > struct field to reflect this. > > > > Add a comment to devfreq_update_stats() to make clear that df->cur_freq > > must only be updated after calling this function in the context of a > > frequency transition. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > drivers/devfreq/devfreq.c | 28 ++++++++++++++++------------ > > drivers/devfreq/governor_passive.c | 6 +++--- > > drivers/devfreq/governor_userspace.c | 2 +- > > drivers/devfreq/tegra20-devfreq.c | 2 +- > > drivers/devfreq/tegra30-devfreq.c | 2 +- > > include/linux/devfreq.h | 4 ++-- > > include/trace/events/devfreq.h | 2 +- > > 7 files changed, 25 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index fb4318d59aa9..bf42130bb4ec 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -154,6 +154,10 @@ static int set_freq_table(struct devfreq *devfreq) > > * devfreq_update_stats() - Update statistics of devfreq behavior > > * @devfreq: the devfreq instance > > * @freq: the update target frequency > > + * > > + * If the function is called in the context of a frequency transition > > + * it expects df->cur_freq to contain the value *before* the transition. > > + * The field must only be updated after calling devfreq_update_stats(). > > */ > > int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > > { > > @@ -162,11 +166,11 @@ int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > > > > cur_time = jiffies; > > > > - /* Immediately exit if previous_freq is not initialized yet. */ > > - if (!devfreq->previous_freq) > > + /* Immediately exit if cur_freq is not initialized yet. */ > > + if (!devfreq->cur_freq) > > goto out; > > > > - prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq); > > + prev_lev = devfreq_get_freq_level(devfreq, devfreq->cur_freq); > > if (prev_lev < 0) { > > ret = prev_lev; > > goto out; > > @@ -295,7 +299,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, > > if (devfreq->profile->get_cur_freq) > > devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); > > else > > - cur_freq = devfreq->previous_freq; > > + cur_freq = devfreq->cur_freq; > > > > freqs.old = cur_freq; > > freqs.new = new_freq; > > @@ -315,7 +319,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, > > dev_err(&devfreq->dev, > > "Couldn't update frequency transition information.\n"); > > > > - devfreq->previous_freq = new_freq; > > + devfreq->cur_freq = new_freq; > > > > if (devfreq->suspend_freq) > > devfreq->resume_freq = cur_freq; > > @@ -450,7 +454,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) > > return; > > } > > > > - devfreq_update_stats(devfreq, devfreq->previous_freq); > > + devfreq_update_stats(devfreq, devfreq->cur_freq); > > devfreq->stop_polling = true; > > mutex_unlock(&devfreq->lock); > > cancel_delayed_work_sync(&devfreq->work); > > @@ -483,7 +487,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > > > > if (devfreq->profile->get_cur_freq && > > !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) > > - devfreq->previous_freq = freq; > > + devfreq->cur_freq = freq; > > > > out: > > mutex_unlock(&devfreq->lock); > > @@ -644,7 +648,7 @@ struct devfreq *devfreq_add_device(struct device *dev, > > devfreq->dev.release = devfreq_dev_release; > > devfreq->profile = profile; > > strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); > > - devfreq->previous_freq = profile->initial_freq; > > + devfreq->cur_freq = profile->initial_freq; > > devfreq->last_status.current_frequency = profile->initial_freq; > > devfreq->data = data; > > devfreq->nb.notifier_call = devfreq_notifier_call; > > @@ -1235,14 +1239,14 @@ static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr, > > !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) > > return sprintf(buf, "%lu\n", freq); > > > > - return sprintf(buf, "%lu\n", devfreq->previous_freq); > > + return sprintf(buf, "%lu\n", devfreq->cur_freq); > > } > > static DEVICE_ATTR_RO(cur_freq); > > > > static ssize_t target_freq_show(struct device *dev, > > struct device_attribute *attr, char *buf) > > { > > - return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq); > > + return sprintf(buf, "%lu\n", to_devfreq(dev)->cur_freq); > > } > > static DEVICE_ATTR_RO(target_freq); > > > > @@ -1398,7 +1402,7 @@ static ssize_t trans_stat_show(struct device *dev, > > unsigned int max_state = devfreq->profile->max_state; > > > > if (!devfreq->stop_polling && > > - devfreq_update_stats(devfreq, devfreq->previous_freq)) > > + devfreq_update_stats(devfreq, devfreq->cur_freq)) > > return 0; > > if (max_state == 0) > > return sprintf(buf, "Not Supported.\n"); > > @@ -1413,7 +1417,7 @@ static ssize_t trans_stat_show(struct device *dev, > > > > for (i = 0; i < max_state; i++) { > > if (devfreq->profile->freq_table[i] > > - == devfreq->previous_freq) { > > + == devfreq->cur_freq) { > > len += sprintf(buf + len, "*"); > > } else { > > len += sprintf(buf + len, " "); > > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c > > index 1c746b96d3db..2d818eaceb39 100644 > > --- a/drivers/devfreq/governor_passive.c > > +++ b/drivers/devfreq/governor_passive.c > > @@ -114,7 +114,7 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq) > > dev_err(&devfreq->dev, > > "Couldn't update frequency transition information.\n"); > > > > - devfreq->previous_freq = freq; > > + devfreq->cur_freq = freq; > > > > out: > > mutex_unlock(&devfreq->lock); > > @@ -134,11 +134,11 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb, > > > > switch (event) { > > case DEVFREQ_PRECHANGE: > > - if (parent->previous_freq > freq) > > + if (parent->cur_freq > freq) > > update_devfreq_passive(devfreq, freq); > > break; > > case DEVFREQ_POSTCHANGE: > > - if (parent->previous_freq < freq) > > + if (parent->cur_freq < freq) > > update_devfreq_passive(devfreq, freq); > > break; > > } > > diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c > > index af94942fcf95..566b8d1f0c17 100644 > > --- a/drivers/devfreq/governor_userspace.c > > +++ b/drivers/devfreq/governor_userspace.c > > @@ -26,7 +26,7 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq) > > if (data->valid) > > *freq = data->user_frequency; > > else > > - *freq = df->previous_freq; /* No user freq specified yet */ > > + *freq = df->cur_freq; /* No user freq specified yet */ > > > > return 0; > > } > > diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c > > index ff82bac9ee4e..f99bd6557df5 100644 > > --- a/drivers/devfreq/tegra20-devfreq.c > > +++ b/drivers/devfreq/tegra20-devfreq.c > > @@ -61,7 +61,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > > return 0; > > > > restore_min_rate: > > - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq); > > + clk_set_min_rate(tegra->emc_clock, devfreq->cur_freq); > > > > return err; > > } > > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c > > index 536273a811fe..7b3bf7a0b15f 100644 > > --- a/drivers/devfreq/tegra30-devfreq.c > > +++ b/drivers/devfreq/tegra30-devfreq.c > > @@ -474,7 +474,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > > return 0; > > > > restore_min_rate: > > - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq); > > + clk_set_min_rate(tegra->emc_clock, devfreq->cur_freq); > > > > return err; > > } > > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > > index 2bae9ed3c783..21d0108df4c5 100644 > > --- a/include/linux/devfreq.h > > +++ b/include/linux/devfreq.h > > @@ -120,7 +120,7 @@ struct devfreq_dev_profile { > > * reevaluate operable frequencies. Devfreq users may use > > * devfreq.nb to the corresponding register notifier call chain. > > * @work: delayed work for load monitoring. > > - * @previous_freq: previously configured frequency value. > > + * @cur_freq: the current frequency. > > * @data: Private data of the governor. The devfreq framework does not > > * touch this. > > * @min_freq: Limit minimum frequency requested by user (0: none) > > @@ -156,7 +156,7 @@ struct devfreq { > > struct notifier_block nb; > > struct delayed_work work; > > > > - unsigned long previous_freq; > > + unsigned long cur_freq; > > struct devfreq_dev_status last_status; > > > > void *data; /* private data for governors */ > > diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h > > index cf5b8772175d..916cfaed5489 100644 > > --- a/include/trace/events/devfreq.h > > +++ b/include/trace/events/devfreq.h > > @@ -22,7 +22,7 @@ TRACE_EVENT(devfreq_monitor, > > ), > > > > TP_fast_assign( > > - __entry->freq = devfreq->previous_freq; > > + __entry->freq = devfreq->cur_freq; > > __entry->busy_time = devfreq->last_status.busy_time; > > __entry->total_time = devfreq->last_status.total_time; > > __entry->polling_ms = devfreq->profile->polling_ms; > > > > > -- > Best Regards, > Chanwoo Choi > Samsung Electronics ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa 2019-09-25 18:43 ` [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Matthias Kaehlcke 2019-09-25 18:43 ` [PATCH 2/2] devfreq: Rename df->previous_freq to df->cur_freq Matthias Kaehlcke @ 2019-09-26 1:36 ` Chanwoo Choi 2019-09-26 16:51 ` Matthias Kaehlcke 2019-09-26 7:21 ` Ben Dooks 2 siblings, 1 reply; 9+ messages in thread From: Chanwoo Choi @ 2019-09-26 1:36 UTC (permalink / raw) To: Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar Cc: linux-pm, linux-kernel, linux-tegra, Douglas Anderson Hi, I'm not sure that it is necessary. I think that it depends on personal opinions. There are no correct answer perfectly. Also, after this changes, there are no any beneficial. It touch the history rather than behavior improvement. On 19. 9. 26. 오전 3:43, Matthias Kaehlcke wrote: > devfreq has two functions with very similar names, devfreq_update_status() > and devfreq_update_stats(). _update_status() currently updates > frequency transitions statistics, while _update_stats() retrieves the > device 'status'. The function names are inversed with respect to what > the functions are actually doing, rename devfreq_update_status() to > devfreq_update_stats() and viceversa. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > We could also rename the current devfreq_update_stats() to > devfreq_refresh_status() to make it easier to distinguish it from > devfreq_update_stats(). > --- > drivers/devfreq/devfreq.c | 12 ++++++------ > drivers/devfreq/governor.h | 4 ++-- > drivers/devfreq/governor_passive.c | 2 +- > drivers/devfreq/governor_simpleondemand.c | 2 +- > drivers/devfreq/tegra30-devfreq.c | 2 +- > 5 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 446490c9d635..fb4318d59aa9 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -151,11 +151,11 @@ static int set_freq_table(struct devfreq *devfreq) > } > > /** > - * devfreq_update_status() - Update statistics of devfreq behavior > + * devfreq_update_stats() - Update statistics of devfreq behavior > * @devfreq: the devfreq instance > * @freq: the update target frequency > */ > -int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > +int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > { > int lev, prev_lev, ret = 0; > unsigned long cur_time; > @@ -191,7 +191,7 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > devfreq->last_stat_updated = cur_time; > return ret; > } > -EXPORT_SYMBOL(devfreq_update_status); > +EXPORT_SYMBOL(devfreq_update_stats); > > /** > * find_devfreq_governor() - find devfreq governor from name > @@ -311,7 +311,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, > freqs.new = new_freq; > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > > - if (devfreq_update_status(devfreq, new_freq)) > + if (devfreq_update_stats(devfreq, new_freq)) > dev_err(&devfreq->dev, > "Couldn't update frequency transition information.\n"); > > @@ -450,7 +450,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) > return; > } > > - devfreq_update_status(devfreq, devfreq->previous_freq); > + devfreq_update_stats(devfreq, devfreq->previous_freq); > devfreq->stop_polling = true; > mutex_unlock(&devfreq->lock); > cancel_delayed_work_sync(&devfreq->work); > @@ -1398,7 +1398,7 @@ static ssize_t trans_stat_show(struct device *dev, > unsigned int max_state = devfreq->profile->max_state; > > if (!devfreq->stop_polling && > - devfreq_update_status(devfreq, devfreq->previous_freq)) > + devfreq_update_stats(devfreq, devfreq->previous_freq)) > return 0; > if (max_state == 0) > return sprintf(buf, "Not Supported.\n"); > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > index bbe5ff9fcecf..e11f447be2b5 100644 > --- a/drivers/devfreq/governor.h > +++ b/drivers/devfreq/governor.h > @@ -64,9 +64,9 @@ extern void devfreq_interval_update(struct devfreq *devfreq, > extern int devfreq_add_governor(struct devfreq_governor *governor); > extern int devfreq_remove_governor(struct devfreq_governor *governor); > > -extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq); > +extern int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq); > > -static inline int devfreq_update_stats(struct devfreq *df) > +static inline int devfreq_update_status(struct devfreq *df) > { > return df->profile->get_dev_status(df->dev.parent, &df->last_status); > } > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c > index be6eeab9c814..1c746b96d3db 100644 > --- a/drivers/devfreq/governor_passive.c > +++ b/drivers/devfreq/governor_passive.c > @@ -110,7 +110,7 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq) > goto out; > > if (devfreq->profile->freq_table > - && (devfreq_update_status(devfreq, freq))) > + && (devfreq_update_stats(devfreq, freq))) > dev_err(&devfreq->dev, > "Couldn't update frequency transition information.\n"); > > diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c > index 3d809f228619..2cbf26bdcfd6 100644 > --- a/drivers/devfreq/governor_simpleondemand.c > +++ b/drivers/devfreq/governor_simpleondemand.c > @@ -25,7 +25,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, > unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; > struct devfreq_simple_ondemand_data *data = df->data; > > - err = devfreq_update_stats(df); > + err = devfreq_update_status(df); > if (err) > return err; > > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c > index a6ba75f4106d..536273a811fe 100644 > --- a/drivers/devfreq/tegra30-devfreq.c > +++ b/drivers/devfreq/tegra30-devfreq.c > @@ -526,7 +526,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq, > unsigned int i; > int err; > > - err = devfreq_update_stats(devfreq); > + err = devfreq_update_status(devfreq); > if (err) > return err; > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa 2019-09-26 1:36 ` [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Chanwoo Choi @ 2019-09-26 16:51 ` Matthias Kaehlcke 0 siblings, 0 replies; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-26 16:51 UTC (permalink / raw) To: Chanwoo Choi Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar, linux-pm, linux-kernel, linux-tegra, Douglas Anderson On Thu, Sep 26, 2019 at 10:36:04AM +0900, Chanwoo Choi wrote: > Hi, > > I'm not sure that it is necessary. I think that it depends on > personal opinions. I disagree that this is about personal opinions. A function name should give clues about what a function is doing and not be misleading. Once these criteria are met personal opinions can come into play. devfreq_update_status() updates statistics, not any status, hence the name neither indicates what the function is doing, and it is misleading, especially since there is another function dealing with the 'status'. devfreq_update_stats() updates the 'status' of the device, not any statistics/stats, as in the other case, the name doesn't indicate that and is misleading. If you still don't agree that the names are poor, could you please make an argument about why the current function names are good/ok, besides "that's how it's currently done"? > There are no correct answer perfectly. I agree that there is no one answer, but the current naming is clearly poor, and we should aim to improve it. Whether that involves swapping the function names or come up with new ones is a different question. > Also, after this changes, there are no any beneficial. > It touch the history rather than behavior improvement. You are focussing exclusively on functional improvements, and you are right that this patch doesn't change anything in functional terms. However I disagree that there are no benefits. Naming is important, ideally code should be self explanatory, which requires using proper function and variable names, misleading names are particularly bad. Keeping poor names for the sake of history sounds like a bad idea, please avoid getting attached to them. Fortunately internal kernel APIs can be changed, and the ones in question aren't widely used. > On 19. 9. 26. 오전 3:43, Matthias Kaehlcke wrote: > > devfreq has two functions with very similar names, devfreq_update_status() > > and devfreq_update_stats(). _update_status() currently updates > > frequency transitions statistics, while _update_stats() retrieves the > > device 'status'. The function names are inversed with respect to what > > the functions are actually doing, rename devfreq_update_status() to > > devfreq_update_stats() and viceversa. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > We could also rename the current devfreq_update_stats() to > > devfreq_refresh_status() to make it easier to distinguish it from > > devfreq_update_stats(). > > --- > > drivers/devfreq/devfreq.c | 12 ++++++------ > > drivers/devfreq/governor.h | 4 ++-- > > drivers/devfreq/governor_passive.c | 2 +- > > drivers/devfreq/governor_simpleondemand.c | 2 +- > > drivers/devfreq/tegra30-devfreq.c | 2 +- > > 5 files changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index 446490c9d635..fb4318d59aa9 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -151,11 +151,11 @@ static int set_freq_table(struct devfreq *devfreq) > > } > > > > /** > > - * devfreq_update_status() - Update statistics of devfreq behavior > > + * devfreq_update_stats() - Update statistics of devfreq behavior > > * @devfreq: the devfreq instance > > * @freq: the update target frequency > > */ > > -int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > > +int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > > { > > int lev, prev_lev, ret = 0; > > unsigned long cur_time; > > @@ -191,7 +191,7 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > > devfreq->last_stat_updated = cur_time; > > return ret; > > } > > -EXPORT_SYMBOL(devfreq_update_status); > > +EXPORT_SYMBOL(devfreq_update_stats); > > > > /** > > * find_devfreq_governor() - find devfreq governor from name > > @@ -311,7 +311,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, > > freqs.new = new_freq; > > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > > > > - if (devfreq_update_status(devfreq, new_freq)) > > + if (devfreq_update_stats(devfreq, new_freq)) > > dev_err(&devfreq->dev, > > "Couldn't update frequency transition information.\n"); > > > > @@ -450,7 +450,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) > > return; > > } > > > > - devfreq_update_status(devfreq, devfreq->previous_freq); > > + devfreq_update_stats(devfreq, devfreq->previous_freq); > > devfreq->stop_polling = true; > > mutex_unlock(&devfreq->lock); > > cancel_delayed_work_sync(&devfreq->work); > > @@ -1398,7 +1398,7 @@ static ssize_t trans_stat_show(struct device *dev, > > unsigned int max_state = devfreq->profile->max_state; > > > > if (!devfreq->stop_polling && > > - devfreq_update_status(devfreq, devfreq->previous_freq)) > > + devfreq_update_stats(devfreq, devfreq->previous_freq)) > > return 0; > > if (max_state == 0) > > return sprintf(buf, "Not Supported.\n"); > > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > > index bbe5ff9fcecf..e11f447be2b5 100644 > > --- a/drivers/devfreq/governor.h > > +++ b/drivers/devfreq/governor.h > > @@ -64,9 +64,9 @@ extern void devfreq_interval_update(struct devfreq *devfreq, > > extern int devfreq_add_governor(struct devfreq_governor *governor); > > extern int devfreq_remove_governor(struct devfreq_governor *governor); > > > > -extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq); > > +extern int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq); > > > > -static inline int devfreq_update_stats(struct devfreq *df) > > +static inline int devfreq_update_status(struct devfreq *df) > > { > > return df->profile->get_dev_status(df->dev.parent, &df->last_status); > > } > > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c > > index be6eeab9c814..1c746b96d3db 100644 > > --- a/drivers/devfreq/governor_passive.c > > +++ b/drivers/devfreq/governor_passive.c > > @@ -110,7 +110,7 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq) > > goto out; > > > > if (devfreq->profile->freq_table > > - && (devfreq_update_status(devfreq, freq))) > > + && (devfreq_update_stats(devfreq, freq))) > > dev_err(&devfreq->dev, > > "Couldn't update frequency transition information.\n"); > > > > diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c > > index 3d809f228619..2cbf26bdcfd6 100644 > > --- a/drivers/devfreq/governor_simpleondemand.c > > +++ b/drivers/devfreq/governor_simpleondemand.c > > @@ -25,7 +25,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, > > unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; > > struct devfreq_simple_ondemand_data *data = df->data; > > > > - err = devfreq_update_stats(df); > > + err = devfreq_update_status(df); > > if (err) > > return err; > > > > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c > > index a6ba75f4106d..536273a811fe 100644 > > --- a/drivers/devfreq/tegra30-devfreq.c > > +++ b/drivers/devfreq/tegra30-devfreq.c > > @@ -526,7 +526,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq, > > unsigned int i; > > int err; > > > > - err = devfreq_update_stats(devfreq); > > + err = devfreq_update_status(devfreq); > > if (err) > > return err; > > > > > > > -- > Best Regards, > Chanwoo Choi > Samsung Electronics ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa 2019-09-25 18:43 ` [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Matthias Kaehlcke 2019-09-25 18:43 ` [PATCH 2/2] devfreq: Rename df->previous_freq to df->cur_freq Matthias Kaehlcke 2019-09-26 1:36 ` [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Chanwoo Choi @ 2019-09-26 7:21 ` Ben Dooks 2019-09-26 17:11 ` Matthias Kaehlcke 2 siblings, 1 reply; 9+ messages in thread From: Ben Dooks @ 2019-09-26 7:21 UTC (permalink / raw) To: Matthias Kaehlcke Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar, linux-pm, linux-kernel, linux-tegra, Douglas Anderson, linux-tegra-owner / On 2019-09-25 19:43, Matthias Kaehlcke wrote: > devfreq has two functions with very similar names, > devfreq_update_status() > and devfreq_update_stats(). _update_status() currently updates > frequency transitions statistics, while _update_stats() retrieves the > device 'status'. The function names are inversed with respect to what > the functions are actually doing, rename devfreq_update_status() to > devfreq_update_stats() and viceversa. Wouldn't having devfreq_get_stats() be a better name for this if it is retrieving the stats? > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > We could also rename the current devfreq_update_stats() to > devfreq_refresh_status() to make it easier to distinguish it from > devfreq_update_stats(). > --- > drivers/devfreq/devfreq.c | 12 ++++++------ > drivers/devfreq/governor.h | 4 ++-- > drivers/devfreq/governor_passive.c | 2 +- > drivers/devfreq/governor_simpleondemand.c | 2 +- > drivers/devfreq/tegra30-devfreq.c | 2 +- > 5 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 446490c9d635..fb4318d59aa9 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -151,11 +151,11 @@ static int set_freq_table(struct devfreq > *devfreq) > } > > /** > - * devfreq_update_status() - Update statistics of devfreq behavior > + * devfreq_update_stats() - Update statistics of devfreq behavior > * @devfreq: the devfreq instance > * @freq: the update target frequency > */ > -int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > +int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > { > int lev, prev_lev, ret = 0; > unsigned long cur_time; > @@ -191,7 +191,7 @@ int devfreq_update_status(struct devfreq *devfreq, > unsigned long freq) > devfreq->last_stat_updated = cur_time; > return ret; > } > -EXPORT_SYMBOL(devfreq_update_status); > +EXPORT_SYMBOL(devfreq_update_stats); > > /** > * find_devfreq_governor() - find devfreq governor from name > @@ -311,7 +311,7 @@ static int devfreq_set_target(struct devfreq > *devfreq, unsigned long new_freq, > freqs.new = new_freq; > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > > - if (devfreq_update_status(devfreq, new_freq)) > + if (devfreq_update_stats(devfreq, new_freq)) > dev_err(&devfreq->dev, > "Couldn't update frequency transition information.\n"); > > @@ -450,7 +450,7 @@ void devfreq_monitor_suspend(struct devfreq > *devfreq) > return; > } > > - devfreq_update_status(devfreq, devfreq->previous_freq); > + devfreq_update_stats(devfreq, devfreq->previous_freq); > devfreq->stop_polling = true; > mutex_unlock(&devfreq->lock); > cancel_delayed_work_sync(&devfreq->work); > @@ -1398,7 +1398,7 @@ static ssize_t trans_stat_show(struct device > *dev, > unsigned int max_state = devfreq->profile->max_state; > > if (!devfreq->stop_polling && > - devfreq_update_status(devfreq, devfreq->previous_freq)) > + devfreq_update_stats(devfreq, devfreq->previous_freq)) > return 0; > if (max_state == 0) > return sprintf(buf, "Not Supported.\n"); > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > index bbe5ff9fcecf..e11f447be2b5 100644 > --- a/drivers/devfreq/governor.h > +++ b/drivers/devfreq/governor.h > @@ -64,9 +64,9 @@ extern void devfreq_interval_update(struct devfreq > *devfreq, > extern int devfreq_add_governor(struct devfreq_governor *governor); > extern int devfreq_remove_governor(struct devfreq_governor *governor); > > -extern int devfreq_update_status(struct devfreq *devfreq, unsigned > long freq); > +extern int devfreq_update_stats(struct devfreq *devfreq, unsigned long > freq); > > -static inline int devfreq_update_stats(struct devfreq *df) > +static inline int devfreq_update_status(struct devfreq *df) > { > return df->profile->get_dev_status(df->dev.parent, &df->last_status); > } > diff --git a/drivers/devfreq/governor_passive.c > b/drivers/devfreq/governor_passive.c > index be6eeab9c814..1c746b96d3db 100644 > --- a/drivers/devfreq/governor_passive.c > +++ b/drivers/devfreq/governor_passive.c > @@ -110,7 +110,7 @@ static int update_devfreq_passive(struct devfreq > *devfreq, unsigned long freq) > goto out; > > if (devfreq->profile->freq_table > - && (devfreq_update_status(devfreq, freq))) > + && (devfreq_update_stats(devfreq, freq))) > dev_err(&devfreq->dev, > "Couldn't update frequency transition information.\n"); > > diff --git a/drivers/devfreq/governor_simpleondemand.c > b/drivers/devfreq/governor_simpleondemand.c > index 3d809f228619..2cbf26bdcfd6 100644 > --- a/drivers/devfreq/governor_simpleondemand.c > +++ b/drivers/devfreq/governor_simpleondemand.c > @@ -25,7 +25,7 @@ static int devfreq_simple_ondemand_func(struct > devfreq *df, > unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; > struct devfreq_simple_ondemand_data *data = df->data; > > - err = devfreq_update_stats(df); > + err = devfreq_update_status(df); > if (err) > return err; > > diff --git a/drivers/devfreq/tegra30-devfreq.c > b/drivers/devfreq/tegra30-devfreq.c > index a6ba75f4106d..536273a811fe 100644 > --- a/drivers/devfreq/tegra30-devfreq.c > +++ b/drivers/devfreq/tegra30-devfreq.c > @@ -526,7 +526,7 @@ static int tegra_governor_get_target(struct > devfreq *devfreq, > unsigned int i; > int err; > > - err = devfreq_update_stats(devfreq); > + err = devfreq_update_status(devfreq); > if (err) > return err; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa 2019-09-26 7:21 ` Ben Dooks @ 2019-09-26 17:11 ` Matthias Kaehlcke 2019-09-26 17:14 ` Matthias Kaehlcke 0 siblings, 1 reply; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-26 17:11 UTC (permalink / raw) To: Ben Dooks Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar, linux-pm, linux-kernel, linux-tegra, Douglas Anderson, linux-tegra-owner On Thu, Sep 26, 2019 at 08:21:51AM +0100, Ben Dooks wrote: > / > > On 2019-09-25 19:43, Matthias Kaehlcke wrote: > > devfreq has two functions with very similar names, > > devfreq_update_status() > > and devfreq_update_stats(). _update_status() currently updates > > frequency transitions statistics, while _update_stats() retrieves the > > device 'status'. The function names are inversed with respect to what > > the functions are actually doing, rename devfreq_update_status() to > > devfreq_update_stats() and viceversa. > > Wouldn't having devfreq_get_stats() be a better name for this if it > is retrieving the stats? struct devfreq_dev_status is a bit ambiguous. It contains 'stat' fields like 'total_time' and 'busy_time', but also 'current_frequency' which is more a 'status'. Given the name of the struct and the name of the hook profile->get_dev_status I'm inclined to refer to it as 'status', also to disambiguate it from the transition stats. That said I'd welcome a name that's easier to differantiate from the other devfreq_update_stat* function, like devfreq_update_status() or devfreq_refresh_status(). > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > We could also rename the current devfreq_update_stats() to > > devfreq_refresh_status() to make it easier to distinguish it from > > devfreq_update_stats(). > > --- > > drivers/devfreq/devfreq.c | 12 ++++++------ > > drivers/devfreq/governor.h | 4 ++-- > > drivers/devfreq/governor_passive.c | 2 +- > > drivers/devfreq/governor_simpleondemand.c | 2 +- > > drivers/devfreq/tegra30-devfreq.c | 2 +- > > 5 files changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index 446490c9d635..fb4318d59aa9 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -151,11 +151,11 @@ static int set_freq_table(struct devfreq *devfreq) > > } > > > > /** > > - * devfreq_update_status() - Update statistics of devfreq behavior > > + * devfreq_update_stats() - Update statistics of devfreq behavior > > * @devfreq: the devfreq instance > > * @freq: the update target frequency > > */ > > -int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > > +int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > > { > > int lev, prev_lev, ret = 0; > > unsigned long cur_time; > > @@ -191,7 +191,7 @@ int devfreq_update_status(struct devfreq *devfreq, > > unsigned long freq) > > devfreq->last_stat_updated = cur_time; > > return ret; > > } > > -EXPORT_SYMBOL(devfreq_update_status); > > +EXPORT_SYMBOL(devfreq_update_stats); > > > > /** > > * find_devfreq_governor() - find devfreq governor from name > > @@ -311,7 +311,7 @@ static int devfreq_set_target(struct devfreq > > *devfreq, unsigned long new_freq, > > freqs.new = new_freq; > > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > > > > - if (devfreq_update_status(devfreq, new_freq)) > > + if (devfreq_update_stats(devfreq, new_freq)) > > dev_err(&devfreq->dev, > > "Couldn't update frequency transition information.\n"); > > > > @@ -450,7 +450,7 @@ void devfreq_monitor_suspend(struct devfreq > > *devfreq) > > return; > > } > > > > - devfreq_update_status(devfreq, devfreq->previous_freq); > > + devfreq_update_stats(devfreq, devfreq->previous_freq); > > devfreq->stop_polling = true; > > mutex_unlock(&devfreq->lock); > > cancel_delayed_work_sync(&devfreq->work); > > @@ -1398,7 +1398,7 @@ static ssize_t trans_stat_show(struct device *dev, > > unsigned int max_state = devfreq->profile->max_state; > > > > if (!devfreq->stop_polling && > > - devfreq_update_status(devfreq, devfreq->previous_freq)) > > + devfreq_update_stats(devfreq, devfreq->previous_freq)) > > return 0; > > if (max_state == 0) > > return sprintf(buf, "Not Supported.\n"); > > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > > index bbe5ff9fcecf..e11f447be2b5 100644 > > --- a/drivers/devfreq/governor.h > > +++ b/drivers/devfreq/governor.h > > @@ -64,9 +64,9 @@ extern void devfreq_interval_update(struct devfreq > > *devfreq, > > extern int devfreq_add_governor(struct devfreq_governor *governor); > > extern int devfreq_remove_governor(struct devfreq_governor *governor); > > > > -extern int devfreq_update_status(struct devfreq *devfreq, unsigned long > > freq); > > +extern int devfreq_update_stats(struct devfreq *devfreq, unsigned long > > freq); > > > > -static inline int devfreq_update_stats(struct devfreq *df) > > +static inline int devfreq_update_status(struct devfreq *df) > > { > > return df->profile->get_dev_status(df->dev.parent, &df->last_status); > > } > > diff --git a/drivers/devfreq/governor_passive.c > > b/drivers/devfreq/governor_passive.c > > index be6eeab9c814..1c746b96d3db 100644 > > --- a/drivers/devfreq/governor_passive.c > > +++ b/drivers/devfreq/governor_passive.c > > @@ -110,7 +110,7 @@ static int update_devfreq_passive(struct devfreq > > *devfreq, unsigned long freq) > > goto out; > > > > if (devfreq->profile->freq_table > > - && (devfreq_update_status(devfreq, freq))) > > + && (devfreq_update_stats(devfreq, freq))) > > dev_err(&devfreq->dev, > > "Couldn't update frequency transition information.\n"); > > > > diff --git a/drivers/devfreq/governor_simpleondemand.c > > b/drivers/devfreq/governor_simpleondemand.c > > index 3d809f228619..2cbf26bdcfd6 100644 > > --- a/drivers/devfreq/governor_simpleondemand.c > > +++ b/drivers/devfreq/governor_simpleondemand.c > > @@ -25,7 +25,7 @@ static int devfreq_simple_ondemand_func(struct devfreq > > *df, > > unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; > > struct devfreq_simple_ondemand_data *data = df->data; > > > > - err = devfreq_update_stats(df); > > + err = devfreq_update_status(df); > > if (err) > > return err; > > > > diff --git a/drivers/devfreq/tegra30-devfreq.c > > b/drivers/devfreq/tegra30-devfreq.c > > index a6ba75f4106d..536273a811fe 100644 > > --- a/drivers/devfreq/tegra30-devfreq.c > > +++ b/drivers/devfreq/tegra30-devfreq.c > > @@ -526,7 +526,7 @@ static int tegra_governor_get_target(struct > > devfreq *devfreq, > > unsigned int i; > > int err; > > > > - err = devfreq_update_stats(devfreq); > > + err = devfreq_update_status(devfreq); > > if (err) > > return err; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa 2019-09-26 17:11 ` Matthias Kaehlcke @ 2019-09-26 17:14 ` Matthias Kaehlcke 0 siblings, 0 replies; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-26 17:14 UTC (permalink / raw) To: Ben Dooks Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, Steven Rostedt, Ingo Molnar, linux-pm, linux-kernel, linux-tegra, Douglas Anderson, linux-tegra-owner On Thu, Sep 26, 2019 at 10:11:00AM -0700, Matthias Kaehlcke wrote: > On Thu, Sep 26, 2019 at 08:21:51AM +0100, Ben Dooks wrote: > > / > > > > On 2019-09-25 19:43, Matthias Kaehlcke wrote: > > > devfreq has two functions with very similar names, > > > devfreq_update_status() > > > and devfreq_update_stats(). _update_status() currently updates > > > frequency transitions statistics, while _update_stats() retrieves the > > > device 'status'. The function names are inversed with respect to what > > > the functions are actually doing, rename devfreq_update_status() to > > > devfreq_update_stats() and viceversa. > > > > Wouldn't having devfreq_get_stats() be a better name for this if it > > is retrieving the stats? > > struct devfreq_dev_status is a bit ambiguous. It contains 'stat' fields > like 'total_time' and 'busy_time', but also 'current_frequency' which is > more a 'status'. Given the name of the struct and the name of the hook > profile->get_dev_status I'm inclined to refer to it as 'status', also to > disambiguate it from the transition stats. > > That said I'd welcome a name that's easier to differantiate from the other > devfreq_update_stat* function, like devfreq_update_status() or ~~~~~~~~~~~~~~~~~~~~~~~ I meant devfreq_get_status() > devfreq_refresh_status(). > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > We could also rename the current devfreq_update_stats() to > > > devfreq_refresh_status() to make it easier to distinguish it from > > > devfreq_update_stats(). > > > --- > > > drivers/devfreq/devfreq.c | 12 ++++++------ > > > drivers/devfreq/governor.h | 4 ++-- > > > drivers/devfreq/governor_passive.c | 2 +- > > > drivers/devfreq/governor_simpleondemand.c | 2 +- > > > drivers/devfreq/tegra30-devfreq.c | 2 +- > > > 5 files changed, 11 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > > index 446490c9d635..fb4318d59aa9 100644 > > > --- a/drivers/devfreq/devfreq.c > > > +++ b/drivers/devfreq/devfreq.c > > > @@ -151,11 +151,11 @@ static int set_freq_table(struct devfreq *devfreq) > > > } > > > > > > /** > > > - * devfreq_update_status() - Update statistics of devfreq behavior > > > + * devfreq_update_stats() - Update statistics of devfreq behavior > > > * @devfreq: the devfreq instance > > > * @freq: the update target frequency > > > */ > > > -int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) > > > +int devfreq_update_stats(struct devfreq *devfreq, unsigned long freq) > > > { > > > int lev, prev_lev, ret = 0; > > > unsigned long cur_time; > > > @@ -191,7 +191,7 @@ int devfreq_update_status(struct devfreq *devfreq, > > > unsigned long freq) > > > devfreq->last_stat_updated = cur_time; > > > return ret; > > > } > > > -EXPORT_SYMBOL(devfreq_update_status); > > > +EXPORT_SYMBOL(devfreq_update_stats); > > > > > > /** > > > * find_devfreq_governor() - find devfreq governor from name > > > @@ -311,7 +311,7 @@ static int devfreq_set_target(struct devfreq > > > *devfreq, unsigned long new_freq, > > > freqs.new = new_freq; > > > devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > > > > > > - if (devfreq_update_status(devfreq, new_freq)) > > > + if (devfreq_update_stats(devfreq, new_freq)) > > > dev_err(&devfreq->dev, > > > "Couldn't update frequency transition information.\n"); > > > > > > @@ -450,7 +450,7 @@ void devfreq_monitor_suspend(struct devfreq > > > *devfreq) > > > return; > > > } > > > > > > - devfreq_update_status(devfreq, devfreq->previous_freq); > > > + devfreq_update_stats(devfreq, devfreq->previous_freq); > > > devfreq->stop_polling = true; > > > mutex_unlock(&devfreq->lock); > > > cancel_delayed_work_sync(&devfreq->work); > > > @@ -1398,7 +1398,7 @@ static ssize_t trans_stat_show(struct device *dev, > > > unsigned int max_state = devfreq->profile->max_state; > > > > > > if (!devfreq->stop_polling && > > > - devfreq_update_status(devfreq, devfreq->previous_freq)) > > > + devfreq_update_stats(devfreq, devfreq->previous_freq)) > > > return 0; > > > if (max_state == 0) > > > return sprintf(buf, "Not Supported.\n"); > > > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > > > index bbe5ff9fcecf..e11f447be2b5 100644 > > > --- a/drivers/devfreq/governor.h > > > +++ b/drivers/devfreq/governor.h > > > @@ -64,9 +64,9 @@ extern void devfreq_interval_update(struct devfreq > > > *devfreq, > > > extern int devfreq_add_governor(struct devfreq_governor *governor); > > > extern int devfreq_remove_governor(struct devfreq_governor *governor); > > > > > > -extern int devfreq_update_status(struct devfreq *devfreq, unsigned long > > > freq); > > > +extern int devfreq_update_stats(struct devfreq *devfreq, unsigned long > > > freq); > > > > > > -static inline int devfreq_update_stats(struct devfreq *df) > > > +static inline int devfreq_update_status(struct devfreq *df) > > > { > > > return df->profile->get_dev_status(df->dev.parent, &df->last_status); > > > } > > > diff --git a/drivers/devfreq/governor_passive.c > > > b/drivers/devfreq/governor_passive.c > > > index be6eeab9c814..1c746b96d3db 100644 > > > --- a/drivers/devfreq/governor_passive.c > > > +++ b/drivers/devfreq/governor_passive.c > > > @@ -110,7 +110,7 @@ static int update_devfreq_passive(struct devfreq > > > *devfreq, unsigned long freq) > > > goto out; > > > > > > if (devfreq->profile->freq_table > > > - && (devfreq_update_status(devfreq, freq))) > > > + && (devfreq_update_stats(devfreq, freq))) > > > dev_err(&devfreq->dev, > > > "Couldn't update frequency transition information.\n"); > > > > > > diff --git a/drivers/devfreq/governor_simpleondemand.c > > > b/drivers/devfreq/governor_simpleondemand.c > > > index 3d809f228619..2cbf26bdcfd6 100644 > > > --- a/drivers/devfreq/governor_simpleondemand.c > > > +++ b/drivers/devfreq/governor_simpleondemand.c > > > @@ -25,7 +25,7 @@ static int devfreq_simple_ondemand_func(struct devfreq > > > *df, > > > unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; > > > struct devfreq_simple_ondemand_data *data = df->data; > > > > > > - err = devfreq_update_stats(df); > > > + err = devfreq_update_status(df); > > > if (err) > > > return err; > > > > > > diff --git a/drivers/devfreq/tegra30-devfreq.c > > > b/drivers/devfreq/tegra30-devfreq.c > > > index a6ba75f4106d..536273a811fe 100644 > > > --- a/drivers/devfreq/tegra30-devfreq.c > > > +++ b/drivers/devfreq/tegra30-devfreq.c > > > @@ -526,7 +526,7 @@ static int tegra_governor_get_target(struct > > > devfreq *devfreq, > > > unsigned int i; > > > int err; > > > > > > - err = devfreq_update_stats(devfreq); > > > + err = devfreq_update_status(devfreq); > > > if (err) > > > return err; ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-09-26 17:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20190925184505epcas2p3f8f4395f37df4b1fe33309393e8af4df@epcas2p3.samsung.com>
2019-09-25 18:43 ` [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Matthias Kaehlcke
2019-09-25 18:43 ` [PATCH 2/2] devfreq: Rename df->previous_freq to df->cur_freq Matthias Kaehlcke
2019-09-26 1:41 ` Chanwoo Choi
2019-09-26 17:35 ` Matthias Kaehlcke
2019-09-26 1:36 ` [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Chanwoo Choi
2019-09-26 16:51 ` Matthias Kaehlcke
2019-09-26 7:21 ` Ben Dooks
2019-09-26 17:11 ` Matthias Kaehlcke
2019-09-26 17:14 ` Matthias Kaehlcke
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).