From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D323EC352AA for ; Thu, 26 Sep 2019 16:51:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 934B721A4A for ; Thu, 26 Sep 2019 16:51:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="dS/DPqHy" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727572AbfIZQvQ (ORCPT ); Thu, 26 Sep 2019 12:51:16 -0400 Received: from mail-pf1-f193.google.com ([209.85.210.193]:35833 "EHLO mail-pf1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726029AbfIZQvQ (ORCPT ); Thu, 26 Sep 2019 12:51:16 -0400 Received: by mail-pf1-f193.google.com with SMTP id 205so2180388pfw.2 for ; Thu, 26 Sep 2019 09:51:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=WzmNz7XRd5OiN6BFcznbPwBICwFGdE61Elyfk5RsfWY=; b=dS/DPqHyZLMaS2lJxibjNycVbnf0mJgFgGg10uqrOf//mwnDXXhdArCuzNugEf8ShP bGmmuAsYMCxyiI1B4NERevJu7nNWjjlPP6nqbCb74lo3iguhjhmY6M1btERTzmLHjyIj 57F+JI1l825F4d3JwlGCqUnReWmRX5rpGuWKo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=WzmNz7XRd5OiN6BFcznbPwBICwFGdE61Elyfk5RsfWY=; b=Uc4PAlq31mkrXjixAUALsUQ8rEiZM9eFp68gmqU87V3/wdRgAcWaCUnJ5VyRfW8awG F/jJOCVPT8TJPtcj1g8ysMabLZouGUZQeSfLVpCF514ft7IEt8saJRG2EDJC0W851/0K rTCxP6vbX3XrsaUeQ5p4HNdG4wywfX/UA1lPnAYv/RCBUy3m6YiroF6cbdEf9gqgo2o9 cDtmSw075oJ1dMZ+NkE4oY8JUfpFGfd2t92wSZpcUUn8wVU5Az49Uap8JWt/REPDnK82 9o4xi+5E/P8TZYVCXPSlwdtqzbnUqy1NZgXbMEw/tHR0bn4vxWxIv6DP4EGZ2QrcjfDN V5Hg== X-Gm-Message-State: APjAAAUL+mq699P2P6hRJw2bbRwF3EIvZCEJr/JOph3t6k090Ij5VOeN K1lURFfPjJIGnnYLtpS35EWlig== X-Google-Smtp-Source: APXvYqzaMUF2LR6+lvtMW6Moq1c0qAzEitFFg2b2QE97EIYjBkRBo0Q7IIO5yCKcjUfwoT5ECrS8rg== X-Received: by 2002:a62:c141:: with SMTP id i62mr4807427pfg.64.1569516675649; Thu, 26 Sep 2019 09:51:15 -0700 (PDT) Received: from localhost ([2620:15c:202:1:75a:3f6e:21d:9374]) by smtp.gmail.com with ESMTPSA id n6sm2380789pgp.12.2019.09.26.09.51.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Sep 2019 09:51:15 -0700 (PDT) Date: Thu, 26 Sep 2019 09:51:13 -0700 From: Matthias Kaehlcke To: Chanwoo Choi Cc: MyungJoo Ham , Kyungmin Park , Thierry Reding , Jonathan Hunter , Steven Rostedt , Ingo Molnar , linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org, Douglas Anderson Subject: Re: [PATCH 1/2] devfreq: Rename devfreq_update_status() to devfreq_update_stats() and viceversa Message-ID: <20190926165113.GP133864@google.com> References: <20190925184314.30251-1-mka@chromium.org> <3fb18474-b31e-bc7e-8b6a-660904a19443@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <3fb18474-b31e-bc7e-8b6a-660904a19443@samsung.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > > 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