Re: [Xen-devel] [PATCH] ACPI: Add fixups for AMD P-state figures.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.13 at 22:33, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> +static void amd_fixup_frequency(struct xen_processor_px *px)
> +{
> +     u32 hi, lo, fid, did;
> +     int index = px->control & 0x00000007;
> +
> +     if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +             return;

This is pointless, the driver as a whole is already AMD specific.

> +
> +     if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10)
> +         || boot_cpu_data.x86 == 0x11) {

Instead I wonder whether this could (properly inverted) serve as
an early return condition, reducing indentation on the subsequent
block.

> +             rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi);
> +        /*
> +         * MSR C001_0064+:
> +         * Bit 63: PstateEn. Read-write. If set, the P-state is valid.
> +         */
> +        if (!(hi & (1UL << 31)))
> +            return;
> +
> +        fid = lo & 0x3f;
> +        did = (lo >> 6) & 7;
> +        if (boot_cpu_data.x86 == 0x10)
> +            px->core_frequency = (100 * (fid + 0x10)) >> did;
> +        else
> +            px->core_frequency = (100 * (fid + 8)) >> did;

0x10 vs 8? Please settle on decimal (preferred) or hex numbers in
a calculation like this.

> +     }
> +}

And as Boris already pointed out - indentation here should be
consistent in itself _and_ with the rest of the file.

> +
> +static void amd_fixup_freq(struct processor_performance *perf)
> +{
> +
> +    int i;

unsigned int

Jan

> +
> +    for (i = 0; i < perf->state_count; i++)
> +        amd_fixup_frequency(&perf->states[i]);
> +
> +}
>  static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
>  {
>      struct acpi_cpufreq_data *data;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel