lkml.org 
[lkml]   [2019]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] devfreq: Add tracepoint for frequency changes
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 <mka@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.

\
 
 \ /
  Last update: 2019-09-25 19:46    [W:0.076 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site