On Tue, 2012-01-31 at 16:12 -0500, Jean Delvare wrote:
> On Wed, 25 Jan 2012 04:43:32 -0800, Guenter Roeck wrote:
> > The VRM model table was missing several Intel CPUs, resulting in wrong VRM table
> > entries to be used for many recent CPUs. Update it. Also, use values from
> > struct cpuinfo_x86 to retrieve CPU model information instead of re-calculating
> > it locally.
> >
> > Cc: Rudolf Marek <r.marek@xxxxxxxxxxxx>
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> > drivers/hwmon/hwmon-vid.c | 103 ++++++++++++++++++++++-----------------------
> > 1 files changed, 50 insertions(+), 53 deletions(-)
>
> Very nice cleanup, I like it. Two comments:
>
> >
> > diff --git a/drivers/hwmon/hwmon-vid.c b/drivers/hwmon/hwmon-vid.c
> > index 4029ac6..5ec5e37 100644
> > --- a/drivers/hwmon/hwmon-vid.c
> > +++ b/drivers/hwmon/hwmon-vid.c
> > @@ -166,9 +166,10 @@ int vid_from_reg(int val, u8 vrm)
> >
> > struct vrm_model {
> > u8 vendor;
> > - u8 eff_family;
> > - u8 eff_model;
> > - u8 eff_stepping;
> > + u8 family;
> > + u8 model_from;
> > + u8 model_to;
> > + u8 stepping;
>
> It might make sense to name that one stepping_to for consistency.
>
> > u8 vrm_type;
> > };
> > (...)
> > @@ -272,21 +278,12 @@ static u8 find_vrm(u8 eff_family, u8 eff_model, u8 eff_stepping, u8 vendor)
> > u8 vid_which_vrm(void)
> > {
> > struct cpuinfo_x86 *c = &cpu_data(0);
> > - u32 eax;
> > - u8 eff_family, eff_model, eff_stepping, vrm_ret;
> > + u8 vrm_ret;
> >
> > if (c->x86 < 6) /* Any CPU with family lower than 6 */
> > return 0; /* doesn't have VID and/or CPUID */
>
> The reference to CPUID could be removed as you no longer call
> cpuid_eax().
>
> >
> > - eax = cpuid_eax(1);
> > - eff_family = ((eax & 0x00000F00)>>8);
> > - eff_model = ((eax & 0x000000F0)>>4);
> > - eff_stepping = eax & 0xF;
> > - if (eff_family == 0xF) { /* use extended model & family */
> > - eff_family += ((eax & 0x00F00000)>>20);
> > - eff_model += ((eax & 0x000F0000)>>16)<<4;
> > - }
> > - vrm_ret = find_vrm(eff_family, eff_model, eff_stepping, c->x86_vendor);
> > + vrm_ret = find_vrm(c->x86, c->x86_model, c->x86_mask, c->x86_vendor);
> > if (vrm_ret == 134)
> > vrm_ret = get_via_model_d_vrm();
> > if (vrm_ret == 0)
>
> Assuming you'll fix the above:
>
> Acked-by: Jean Delvare <khali@xxxxxxxxxxxx>
>
Hi Jean,
Thanks a lot for the review.
I made the above changes and applied the result to -next.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
