Hi,
On 03/13/2011 03:00 PM, Jean Delvare wrote:
Hi Hans,
On Sun, 13 Mar 2011 13:39:34 +0100, Hans de Goede wrote:
Hi,
On 03/12/2011 12:09 PM, Jean Delvare wrote:
Thanks for the review! I've prepared a new version of the
patch addressing all your comments, below are some remarks
from me wrt those comments which warrant a reply.
Thanks for the quick update. Below are my comments to some of your
comments:
+static int sch5627_read_virtual_reg12(struct sch5627_data *data, u16 msb_reg,
+ u16 lsn_reg, int high_nibble)
+{
+ int msb, lsn = -1;
Useless initialization, unless I'm blind.
+
+ /* Read MSB first, this will cause the matching LSN to be latched */
I.e. reverse from 16-bit values? How weird :(
Yep, this also means we cannot read the lsn register once for 2 12 bit reads,
I had code for that (which the useless initialization you spotted was a left
over of).
Why not? Can't you read MSB1, MSB2 and then LSN? If latching is done
per-nibble, that should work.
True, that will likely work, but would lead to really ugly / convoluted code.
Given that quite a few of the lsn's live in their own register anyways (iow
we will only save a few reads by combining lsn reads) I don't think this is
worth the trouble.
+static int reg_to_rpm(u16 reg)
+{
+ if (reg == 0)
+ reg = 1;
This arbitrary decision is discussable. I presume that reg == 0 means an
error condition, either an internal one or a problem with the fan. This
would rather be reported to user-space as 0 (0 RPM) or a negative error
code (libsensors does handle -EIO gracefully) rather than an impossible
sped of 5400540 RPM.
Agreed, since the app note says 0xFFFF is the fan fault value, we should
really never see 0, so I've changed the code to return -EIO in this case.
And value 0xFFFF should probably be reported as 0 RPM, rather than 82
RPM as your driver does today.
Agreed, I'm actually seeing the 82 value on my test machine (as fanX_min
value), so this is definitely worth fixing.
I'll do a v4 of the patch with this fixed tonight.
Regards,
Hans
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
