On Wed, Sep 25, 2019 at 10:56:15AM +0900, Chanwoo Choi wrote: > On 19. 9. 25. 오전 4:37, Matthias Kaehlcke wrote: > > On Fri, Sep 20, 2019 at 10:15:57AM +0900, Chanwoo Choi wrote: > >> Hi, > > > > sorry for the delayed response, you message got buried in my > > mailbox. > > > >> On 19. 9. 20. 오전 2:44, Matthias Kaehlcke wrote: > >>> Add a tracepoint for frequency changes of devfreq devices and > >>> use it. > >>> > >>> Signed-off-by: Matthias Kaehlcke <m...@chromium.org> > >>> --- > >>> (sending v2 without much delay wrt v1, since the change in devfreq > >>> probably isn't controversial, and I'll be offline a few days) > >>> > >>> Changes in v2: > >>> - included trace_devfreq_frequency_enabled() in the condition > >>> to avoid unnecessary evaluation when the trace point is > >>> disabled > >>> --- > >>> drivers/devfreq/devfreq.c | 3 +++ > >>> include/trace/events/devfreq.h | 18 ++++++++++++++++++ > >>> 2 files changed, 21 insertions(+) > >>> > >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > >>> index ab22bf8a12d6..e9f04dcafb01 100644 > >>> --- a/drivers/devfreq/devfreq.c > >>> +++ b/drivers/devfreq/devfreq.c > >>> @@ -317,6 +317,9 @@ static int devfreq_set_target(struct devfreq > >>> *devfreq, unsigned long new_freq, > >>> > >>> devfreq->previous_freq = new_freq; > >>> > >>> + if (trace_devfreq_frequency_enabled() && new_freq != cur_freq) > >>> + trace_devfreq_frequency(devfreq, new_freq); > >> > >> You can change as following without 'new_freq' variable > >> because devfreq->previous_freq is the new frequency. > >> trace_devfreq_frequency(devfreq); > > > > In general that sounds good. > > > > devfreq essentially uses df->previous_freq as df->cur_freq, I think > > most code using it would be clearer if we renamed it accordingly. > > I'll send a separate patch for this. > > 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: > * @previous_freq: previously configured frequency value.
sure, I wouldn't expect the comment of a variable/field called 'previous_freq' say that it's the current frequency. > I think that it it not big problem to keep the name. It's indeed not a big problem, because the code works either way, something like 'cur_freq' would just be less confusing. These are the functions that use 'previous_freq' and how they use it: devfreq_set_target devfreq_monitor_suspend cur_freq_show target_freq_show trans_stat_show devfreq_passive_notifier_call devfreq_userspace_func cur freq devfreq_update_status prev freq More than 85% use the variable as current frequency, which seems like a good argument to give it the proper name, instead of having folks wonder why the previous frequency is used.
