From:   Thomas Gleixner <tglx@linutronix.de>
To:     zhenwei pi <pizhenwei@bytedance.com>, pbonzini@redhat.com
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org,
        zhenwei pi <pizhenwei@bytedance.com>
Subject: Re: [PATCH 1/2] x86/cpu: introduce x86_get_freq
In-Reply-To: <20211129052038.43758-1-pizhenwei@bytedance.com>
References: <20211129052038.43758-1-pizhenwei@bytedance.com>
Date:   Tue, 30 Nov 2021 22:36:43 +0100
Message-ID: <87lf15ba1g.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Mon, Nov 29 2021 at 13:20, zhenwei pi wrote:

> Wrapper function x86_get_freq to get freq on a x86 platform, hide
> detailed implementation for proc routine.

Please rename this to x86_get_cpufreq_khz() simply because
x86_get_freq() is way too unspecific.

Please mention function names with 'function()' which makes it obvious
what it is. Here and in the Subject.

Also spell out frequency all over the place in the change log. There is
no reason for using abbreviations in text.

> +
> +unsigned int x86_get_freq(unsigned int cpu)
> +{
> +	unsigned int freq = 0;
> +
> +	if (cpu_has(&cpu_data(cpu), X86_FEATURE_TSC)) {

Please use cpu_feature_enabled(X86....) and make this condition
negated:

     if (!cpu_feature_enabled(X86_FEATURE_TSC))
            return 0;

which spares an indentation level and the 0 initialization of the variable.

Thanks,

        tglx