[lm-sensors] Andigilog asc7621 driver: Temperature scaling

[Jean Delvare]

Hi Ken,

On Sun, 07 Mar 2010 02:13:07 +0000, Ken Milmore wrote:
> Hi George et al,
> 
> Its great to see the aSC7621 driver being resurrected, but I notice in 
> the patches flying about recently (see LKML 22 Feb 2010 in particular) 
> that the voltage calculation is a bit off, as I already pointed out in a 
> previous review.  To get the correct results, it is much easier to use 
> the 3/4 scale values given in the data sheet, and also be careful to do 
> the multiplication before the division to avoid loss of precision (Hint: 
> The BSD version of the driver does it correctly!).

The patch is already committed upstream:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=d58de038728221f780e11d50b32aa40d420c1150
Any further change must be provided as an incremental patch on top of
that.

> 
> See below:
> 
> 
> static int asc7621_in_scaling[] = {
>      2500, 2250, 3300, 5000, 12000
> };
> 
> 
> For the 10-bit voltage registers, do something like this:
> 
> long regval =
>      (data->reg[param->msb[0]] << 8) |
>      (data->reg[param->lsb[0]]);
> regval = (regval >> 6) * asc7621_in_scaling[index] / (192 << 2);
> 
> 
> And for the 8-bit registers:
> 
> long regval = data->reg[param->msb[0]];
> regval = regval * asc7621_in_scaling[index] / 192;

Looking at the current code, show_in8() looks OK to me. Multiplication
happens before division. It uses a 256 scaling factor instead of 192
but that shouldn't make a difference because the per-channel scale
values have been adjusted meanwhile.

show_in10(), OTOH, indeed suffers from the problem you describe: the
divisions happens before the multiplication, leading to loss of
accuracy.

Care to send a patch fixing this issue?

> I've not yet checked the code thoroughly to look for any of the other 
> fixes which were put in during our previous discussions, but I suspect 
> that some of the other minor fixes which were discussed on the mailing 
> list may have been left out.

Feel free to send patches fixing them.

> This was a very ambitious driver in that it included all the fancy 
> thermal management features of the aSC7621, and a complete review and 
> testing of everything in the driver was a lot more than I had the 
> stomach for.  I'm glad to see others have now picked it up though!

-- 
Jean Delvare