Re: [lm-sensors] [PATCH] hwmon: New driver for SMSC SCH5627 hwmon

[Hans de Goede <hdegoede@redhat.com>]

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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors