[lm-sensors] [PATCH] hwmon: New driver for SMSC SCH5627 hwmon (v2)
Hans de Goede
hdegoede at redhat.com
Sun Mar 13 16:12:08 CET 2011
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:
>> 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
>>>> +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.
More information about the lm-sensors