[lm-sensors] RFC: nct6776f support in the w83627ehf and fan RPM signal de-bounce

Ian Dobson i.dobson at planet-ian.com
Sun Mar 6 20:40:30 CET 2011


Hi Guenter,

--------------------------------------------------
From: "Guenter Roeck" <guenter.roeck at ericsson.com>
Sent: Sunday, March 06, 2011 7:47 PM
To: "Ian Dobson" <i.dobson at planet-ian.com>
Cc: <lm-sensors at lm-sensors.org>
Subject: Re: RFC: nct6776f support in the w83627ehf and fan RPM signal 
de-bounce

> Hi Ian,
>
> On Sun, Mar 06, 2011 at 12:21:48PM -0500, Ian Dobson wrote:
>>
>> Hi Guenter,
>> --------------------------------------------------
>> From: "Guenter Roeck" <guenter.roeck at ericsson.com>
>> Sent: Sunday, March 06, 2011 6:07 PM
>> To: "Ian Dobson" <i.dobson at planet-ian.com>
>> Cc: <lm-sensors at lm-sensors.org>
>> Subject: Re: RFC: nct6776f support in the w83627ehf and fan RPM signal
>> de-bounce
>>
>> > Hi Ian,
>> >
>> > On Sun, Mar 06, 2011 at 07:38:24AM -0500, Ian Dobson wrote:
>> >> Hello All,
>> >>
>> >> Firstly I'd like to thank Guenter for all his work adding support for 
>> >> the
>> >> nct6776f superIO chip.
>> >>
>> >> During my testing of the driver on a Asus p8p67 pro motherboard and a
>> >> Arctic
>> >> cooling fan I've come across a small problem that the RPM reading is
>> >> sometimes incorrect, usually the incorrect reading is almost double 
>> >> the
>> >> expected value and is only incorrect for 2 seconds (Next actual read 
>> >> from
>> >> the chip). Looking at the tacho signal from the fan with an 
>> >> oscilloscope
>> >> I
>> >> can see short pulses/dropouts which cause the incorrect reading. 
>> >> Looking
>> >> through the data sheet I see that the nct6776f and nct6775f both 
>> >> support
>> >> de-bouncing the fan rpm signal (logical device b address 0xf0 bit 1-5 
>> >> for
>> >> nct6776f or 1-4 for nct6775f). After enabling the rpm de-bounce for 
>> >> the
>> >> CPU
>> >> fan I've not seen any more miss readings. I tested for 24hours and
>> >> usually I
>> >> see a incorrect reading within 10-30 minutes.
>> >>
>> >> The first question is, should we offer the possibility to enable rpm
>> >> de-bounce for chips that support it? If yes then what interface should 
>> >> we
>> >> use? I can see 3 possibilities:-
>> >> 1) Through sysfs fanX_debounce (0/1). I already have a patch for this 
>> >> and
>> >> the diff is about 60 lines of code
>> >> 2) Through a module parameter (fan_debounce=X) where X could be 0/1 
>> >> for
>> >> disable/enable for all fans or a decimal value which is the bit 
>> >> pattern
>> >> for
>> >> the de-bounce to enable.
>> >> 3) Always enable the fan de-bounce if the chip supports it.
>> >>
>> >> Note enabling de-bounce does not appear to have any negative effects.
>> >>
>> > Thanks a lot for the feedback and testing.
>> >
>> > Since there will always be just one of those devices in a system, 
>> > option
>> > 2) or 3)
>> > seem to be better choices; I like to avoid adding new attributes if 
>> > there
>> > is another
>> > option and if the attribute would only be used by a single driver.
>> >
>> > We could implement 3) for simplicity, but question is if this might 
>> > have
>> > any
>> > undesired side effects on other boards. So 2) might be the safer 
>> > approach.
>> >
>> > Thoughts, anyone ?
>> >
>> > Thanks,
>> > Guenter
>>
>> I already have a patch for 1 or 2, just let me know what you want.
>>
> Why don't you send the patch for 2) to the list ? If it is ok, I'll
> add it to the series.
>
>> I can't imagine that enabling de-bounce should have any undesired side
>> effects on other boards. I have afew contacts that have various Asus 
>> socket
>> 1155 boards with nct6776f chips, so maybe I can get them to test my
>> de-bounce patch to see if there are any side effects.
>>
> Me not either. Just playing safe.
>
> As it is, I am really much more concerned that I am unable to get an
> Acked-by: for the patch series, much less a Reviewed-by:, nor a Tested-by:
> from anyone but you. That might result in the series not making it into 
> 2.6.39,
> which I think would be unfortunate.
>
> Here are the requirements for Acked-by:
>
> "If a person was not directly involved in the preparation or handling of a
> patch but wishes to signify and record their approval of it then they can
> arrange to have an Acked-by: line added to the patch's changelog.
> ...
> Acked-by: is not as formal as Signed-off-by:.  It is a record that the 
> acker
> has at least reviewed the patch and has indicated acceptance.  Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by:."
>
> So, for anyone interested in seeing support for NCT6775F and NCT6776F in 
> 2.6.39,
> consider the above. If you think the patch series and your review of it 
> meets
> above criteria, please consider sending me an Acked-by: for the series, 
> or,
> if you only reviewed individual patches, for the patches you reviewed.
>
> Thanks,
> Guenter

I've put out a request to the other people I know of who are using your 
standalone driver (with my de-bounce patch) to test it and let me know how 
they get along.

I'd rather wait abit with my de-bounce patch, I'd rather get the driver into 
.39 and then look at adding changes. You've done alot of work on this driver 
and it would be a shame if it doesn't make it into the next merge window.

I'll do a full code review tomorrow but as you've already covered the 
comments I've had I don't think I'll find anything new, you already have my 
tested-by:

Regards and Thanks
Ian Dobson
 





More information about the lm-sensors mailing list