[lm-sensors] [PATCH v1 01/01] hwmon: (w83627ehf) Add fan debounce

[lm-sensors] [PATCH v1 01/01] hwmon: (w83627ehf) Add fan debounce

[Ian Dobson]

Hello,

The nct6776f and nct6775f support debouncing the fan RPM signal, this can 
help improve the stability of th RPM signal for some fans (Arctic cooling 
fans for example).
This patch adds a module parameter fan_debounce, which when set to 1 enables 
debounce for all fans that the chip supports.
I've based this diff on the standalone w83627ehf driver from 7.3.2011 09:54, 
using diff -uN standardversion  myversion

Regards
Ian Dobson


--- w83627ehf.c.std	2011-03-04 17:01:18.630954100 +0100
+++ w83627ehf.c	2011-03-07 18:22:33.878852000 +0100
@@ -79,6 +79,10 @@
 module_param(force_id, ushort, 0);
 MODULE_PARM_DESC(force_id, "Override the detected device ID");

+static unsigned short fan_debounce;
+module_param(fan_debounce, ushort, 0);
+MODULE_PARM_DESC(fan_debounce, "Enable de-bouncing for fan RPM signal");
+
 #define DRVNAME "w83627ehf"

 /*
@@ -236,6 +240,7 @@
 static const u16 NCT6775_REG_FAN_STEP_OUTPUT[] = { 0x10b, 0x20b, 0x30b };
 static const u16 NCT6775_REG_FAN[] = { 0x630, 0x632, 0x634, 0x636, 0x638 };
 static const u16 NCT6776_REG_FAN_MIN[] = { 0x63a, 0x63c, 0x63e, 0x640, 
0x642};
+static const u8 NCT6776_REG_FAN_DEBOUNCE = 0xf0;

 static const u16 NCT6775_REG_TEMP[]
 	= { 0x27, 0x150, 0x250, 0x73, 0x75, 0x77, 0x62b, 0x62c, 0x62d };
@@ -2297,6 +2302,7 @@
 	static const char __initdata sio_name_NCT6776[] = "NCT6776F";

 	u16 val;
+	u8 tmp;
 	const char *sio_name;

 	superio_enter(sioaddr);
@@ -2369,6 +2375,22 @@
 	pr_info("Found %s chip at %#x\n", sio_name, *addr);
 	sio_data->sioreg = sioaddr;

+	if ( fan_debounce = 1 && (sio_data->kind = nct6775
+			|| sio_data->kind = nct6776 )) {
+		superio_enter(sio_data->sioreg);
+		superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
+		tmp = superio_inb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE );
+		if ( sio_data->kind = nct6776 ) {
+			superio_outb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE,
+				0x3e | tmp );
+		} else {
+			superio_outb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE,
+				0x1e | tmp );
+		}
+		superio_exit(sio_data->sioreg);
+		pr_info("Enabling fan debounce for chip %s\n", sio_name);
+	}
+
 	return 0;
 }



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v1 01/01] hwmon: (w83627ehf) Add fan

[Guenter Roeck]

Hi Ian,

On Mon, Mar 07, 2011 at 01:55:56PM -0500, Ian Dobson wrote:
> Hello,
> 
> The nct6776f and nct6775f support debouncing the fan RPM signal, this can 
> help improve the stability of th RPM signal for some fans (Arctic cooling 
> fans for example).
> This patch adds a module parameter fan_debounce, which when set to 1 enables 
> debounce for all fans that the chip supports.
> I've based this diff on the standalone w83627ehf driver from 7.3.2011 09:54, 
> using diff -uN standardversion  myversion
> 
> Regards
> Ian Dobson
> 
> 
> --- w83627ehf.c.std	2011-03-04 17:01:18.630954100 +0100
> +++ w83627ehf.c	2011-03-07 18:22:33.878852000 +0100
> @@ -79,6 +79,10 @@
>  module_param(force_id, ushort, 0);
>  MODULE_PARM_DESC(force_id, "Override the detected device ID");
> 
> +static unsigned short fan_debounce;
> +module_param(fan_debounce, ushort, 0);
> +MODULE_PARM_DESC(fan_debounce, "Enable de-bouncing for fan RPM signal");
> +

I wanted to ask you to document the module parameter in Documentation/hwmon/w83627ehf,
but turns out force_id isn't documented there either. So I'll leave it up to you
to decide if you want to spend the effort.

>  #define DRVNAME "w83627ehf"
> 
>  /*
> @@ -236,6 +240,7 @@
>  static const u16 NCT6775_REG_FAN_STEP_OUTPUT[] = { 0x10b, 0x20b, 0x30b };
>  static const u16 NCT6775_REG_FAN[] = { 0x630, 0x632, 0x634, 0x636, 0x638 };
>  static const u16 NCT6776_REG_FAN_MIN[] = { 0x63a, 0x63c, 0x63e, 0x640, 
> 0x642};
> +static const u8 NCT6776_REG_FAN_DEBOUNCE = 0xf0;
> 
You don't need a variable here; a define is good enough. Variables are only necessary
for arrays. I would suggest to use

#define NCT6775_REG_FAN_DEBOUNCE	0xf0

May seem odd, but the convention I used is to name registers after the first chip
supporting it. In this case, that would be NCT6775.

>  static const u16 NCT6775_REG_TEMP[]
>  	= { 0x27, 0x150, 0x250, 0x73, 0x75, 0x77, 0x62b, 0x62c, 0x62d };
> @@ -2297,6 +2302,7 @@
>  	static const char __initdata sio_name_NCT6776[] = "NCT6776F";
> 
>  	u16 val;
> +	u8 tmp;

Declare tmp where you need it, ie after the if() below.

>  	const char *sio_name;
> 
>  	superio_enter(sioaddr);
> @@ -2369,6 +2375,22 @@
>  	pr_info("Found %s chip at %#x\n", sio_name, *addr);
>  	sio_data->sioreg = sioaddr;
> 
Can you move the code below a little further up, just ahead of the previous
superio_exit() ?
This way you don't have to call superio_enter() and superio_exit() again.

> +	if ( fan_debounce = 1 && (sio_data->kind = nct6775
> +			|| sio_data->kind = nct6776 )) {

	if (fan_debounce &&
	    (sio_data->kind = nct6775 || sio_data->kind = nct6776)) {

should be good enough.

> +		superio_enter(sio_data->sioreg);
> +		superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
> +		tmp = superio_inb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE );
> +		if ( sio_data->kind = nct6776 ) {

Blanks after ( and before ) will cause checkpatch warnings. Please run the patch
through scripts/checkpatch.pl to make sure it is clean.

> +			superio_outb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE,
> +				0x3e | tmp );
> +		} else {
> +			superio_outb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE,
> +				0x1e | tmp );
> +		}

	The { } are not needed here.

> +		superio_exit(sio_data->sioreg);
> +		pr_info("Enabling fan debounce for chip %s\n", sio_name);

		Strictly speaking, that would be "Enabled" ;)
 
> +	}
> +
The semantics is that you can turn debounce on, but you can not turn it off. 
Is this intentional ? [ ok with me, and I think it makes sense - just asking ]

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v1 01/01] hwmon: (w83627ehf) Add fan

[Ian Dobson]

Hi Guenter,

--------------------------------------------------
From: "Guenter Roeck" <guenter.roeck@ericsson.com>
Sent: Monday, March 07, 2011 8:24 PM
To: "Ian Dobson" <i.dobson@planet-ian.com>
Cc: <lm-sensors@lm-sensors.org>
Subject: Re: [PATCH v1 01/01] hwmon: (w83627ehf) Add fan debounce support 
for NCT6775F and NCT6776F

> Hi Ian,
>
> On Mon, Mar 07, 2011 at 01:55:56PM -0500, Ian Dobson wrote:
>> Hello,
>>
>> The nct6776f and nct6775f support debouncing the fan RPM signal, this can
>> help improve the stability of th RPM signal for some fans (Arctic cooling
>> fans for example).
>> This patch adds a module parameter fan_debounce, which when set to 1 
>> enables
>> debounce for all fans that the chip supports.
>> I've based this diff on the standalone w83627ehf driver from 7.3.2011 
>> 09:54,
>> using diff -uN standardversion  myversion
>>
>> Regards
>> Ian Dobson
>>
>>
>> --- w83627ehf.c.std 2011-03-04 17:01:18.630954100 +0100
>> +++ w83627ehf.c 2011-03-07 18:22:33.878852000 +0100
>> @@ -79,6 +79,10 @@
>>  module_param(force_id, ushort, 0);
>>  MODULE_PARM_DESC(force_id, "Override the detected device ID");
>>
>> +static unsigned short fan_debounce;
>> +module_param(fan_debounce, ushort, 0);
>> +MODULE_PARM_DESC(fan_debounce, "Enable de-bouncing for fan RPM signal");
>> +
>
> I wanted to ask you to document the module parameter in 
> Documentation/hwmon/w83627ehf,
> but turns out force_id isn't documented there either. So I'll leave it up 
> to you
> to decide if you want to spend the effort.

If force_id isn't documented then maybe not.
>
>>  #define DRVNAME "w83627ehf"
>>
>>  /*
>> @@ -236,6 +240,7 @@
>>  static const u16 NCT6775_REG_FAN_STEP_OUTPUT[] = { 0x10b, 0x20b, 
>> 0x30b };
>>  static const u16 NCT6775_REG_FAN[] = { 0x630, 0x632, 0x634, 0x636, 
>> 0x638 };
>>  static const u16 NCT6776_REG_FAN_MIN[] = { 0x63a, 0x63c, 0x63e, 0x640,
>> 0x642};
>> +static const u8 NCT6776_REG_FAN_DEBOUNCE = 0xf0;
>>
> You don't need a variable here; a define is good enough. Variables are 
> only necessary
> for arrays. I would suggest to use
>
> #define NCT6775_REG_FAN_DEBOUNCE 0xf0
>
> May seem odd, but the convention I used is to name registers after the 
> first chip
> supporting it. In this case, that would be NCT6775.
OK I'll change that, I didn't realise use use the first chip name.
>
>>  static const u16 NCT6775_REG_TEMP[]
>>  = { 0x27, 0x150, 0x250, 0x73, 0x75, 0x77, 0x62b, 0x62c, 0x62d };
>> @@ -2297,6 +2302,7 @@
>>  static const char __initdata sio_name_NCT6776[] = "NCT6776F";
>>
>>  u16 val;
>> + u8 tmp;
>
> Declare tmp where you need it, ie after the if() below.
OK, I can change that, but most of the kernel code I've looked at defines 
all variables used in a function at the start of the function.

>
>>  const char *sio_name;
>>
>>  superio_enter(sioaddr);
>> @@ -2369,6 +2375,22 @@
>>  pr_info("Found %s chip at %#x\n", sio_name, *addr);
>>  sio_data->sioreg = sioaddr;
>>
> Can you move the code below a little further up, just ahead of the 
> previous
> superio_exit() ?
> This way you don't have to call superio_enter() and superio_exit() again.
I wanted to do it like this so that you get the dmesg

[194080.788004] w83627ehf: Found NCT6776F chip at 0x290
[194080.788024] w83627ehf: Enabling fan debounce for chip NCT6776F

and I didn't want to change too much code, but I'll have a look at it.

>
>> + if ( fan_debounce = 1 && (sio_data->kind = nct6775
>> + || sio_data->kind = nct6776 )) {
>
> if (fan_debounce &&
>     (sio_data->kind = nct6775 || sio_data->kind = nct6776)) {
>
> should be good enough.
I though about that but someone with limited coding experience trying to 
enable the debounce should be able to understand the code abit better, but 
I'll change that.

>
>> + superio_enter(sio_data->sioreg);
>> + superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
>> + tmp = superio_inb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE );
>> + if ( sio_data->kind = nct6776 ) {
>
> Blanks after ( and before ) will cause checkpatch warnings. Please run the 
> patch
> through scripts/checkpatch.pl to make sure it is clean.
OK will do.
>
>> + superio_outb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE,
>> + 0x3e | tmp );
>> + } else {
>> + superio_outb(sio_data->sioreg, NCT6776_REG_FAN_DEBOUNCE,
>> + 0x1e | tmp );
>> + }
>
> The { } are not needed here.
OK I'll fix that
>
>> + superio_exit(sio_data->sioreg);
>> + pr_info("Enabling fan debounce for chip %s\n", sio_name);
>
> Strictly speaking, that would be "Enabled" ;)
OK your right.
>
>> + }
>> +
> The semantics is that you can turn debounce on, but you can not turn it 
> off.
> Is this intentional ? [ ok with me, and I think it makes sense - just 
> asking ]
This is intentional, I only touch the debounce register if it is explicitly 
enabled by fan_debounce=1. The only way to disable it is to shutdown the 
box.

>
> Thanks,
> Guenter
Thanks for the review, I'll fix the points you have and send v2
Ian Dobson
 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v1 01/01] hwmon: (w83627ehf) Add fan

[Guenter Roeck]

Hi Ian,

On Mon, Mar 07, 2011 at 03:07:29PM -0500, Ian Dobson wrote:
> Hi Guenter,
> 
> --------------------------------------------------
> From: "Guenter Roeck" <guenter.roeck@ericsson.com>
> Sent: Monday, March 07, 2011 8:24 PM
> To: "Ian Dobson" <i.dobson@planet-ian.com>
> Cc: <lm-sensors@lm-sensors.org>
> Subject: Re: [PATCH v1 01/01] hwmon: (w83627ehf) Add fan debounce support 
> for NCT6775F and NCT6776F
> 
> > Hi Ian,
> >
> > On Mon, Mar 07, 2011 at 01:55:56PM -0500, Ian Dobson wrote:
> >> Hello,
> >>
> >> The nct6776f and nct6775f support debouncing the fan RPM signal, this can
> >> help improve the stability of th RPM signal for some fans (Arctic cooling
> >> fans for example).
> >> This patch adds a module parameter fan_debounce, which when set to 1 
> >> enables
> >> debounce for all fans that the chip supports.
> >> I've based this diff on the standalone w83627ehf driver from 7.3.2011 
> >> 09:54,
> >> using diff -uN standardversion  myversion
> >>
> >> Regards
> >> Ian Dobson
> >>
> >>
> >> --- w83627ehf.c.std 2011-03-04 17:01:18.630954100 +0100
> >> +++ w83627ehf.c 2011-03-07 18:22:33.878852000 +0100
> >> @@ -79,6 +79,10 @@
> >>  module_param(force_id, ushort, 0);
> >>  MODULE_PARM_DESC(force_id, "Override the detected device ID");
> >>
> >> +static unsigned short fan_debounce;
> >> +module_param(fan_debounce, ushort, 0);
> >> +MODULE_PARM_DESC(fan_debounce, "Enable de-bouncing for fan RPM signal");
> >> +
> >
> > I wanted to ask you to document the module parameter in 
> > Documentation/hwmon/w83627ehf,
> > but turns out force_id isn't documented there either. So I'll leave it up 
> > to you
> > to decide if you want to spend the effort.
> 
> If force_id isn't documented then maybe not.
> >
> >>  #define DRVNAME "w83627ehf"
> >>
> >>  /*
> >> @@ -236,6 +240,7 @@
> >>  static const u16 NCT6775_REG_FAN_STEP_OUTPUT[] = { 0x10b, 0x20b, 
> >> 0x30b };
> >>  static const u16 NCT6775_REG_FAN[] = { 0x630, 0x632, 0x634, 0x636, 
> >> 0x638 };
> >>  static const u16 NCT6776_REG_FAN_MIN[] = { 0x63a, 0x63c, 0x63e, 0x640,
> >> 0x642};
> >> +static const u8 NCT6776_REG_FAN_DEBOUNCE = 0xf0;
> >>
> > You don't need a variable here; a define is good enough. Variables are 
> > only necessary
> > for arrays. I would suggest to use
> >
> > #define NCT6775_REG_FAN_DEBOUNCE 0xf0
> >
> > May seem odd, but the convention I used is to name registers after the 
> > first chip
> > supporting it. In this case, that would be NCT6775.
> OK I'll change that, I didn't realise use use the first chip name.
> >
> >>  static const u16 NCT6775_REG_TEMP[]
> >>  = { 0x27, 0x150, 0x250, 0x73, 0x75, 0x77, 0x62b, 0x62c, 0x62d };
> >> @@ -2297,6 +2302,7 @@
> >>  static const char __initdata sio_name_NCT6776[] = "NCT6776F";
> >>
> >>  u16 val;
> >> + u8 tmp;
> >
> > Declare tmp where you need it, ie after the if() below.
> OK, I can change that, but most of the kernel code I've looked at defines 
> all variables used in a function at the start of the function.
> 
> >
> >>  const char *sio_name;
> >>
> >>  superio_enter(sioaddr);
> >> @@ -2369,6 +2375,22 @@
> >>  pr_info("Found %s chip at %#x\n", sio_name, *addr);
> >>  sio_data->sioreg = sioaddr;
> >>
> > Can you move the code below a little further up, just ahead of the 
> > previous
> > superio_exit() ?
> > This way you don't have to call superio_enter() and superio_exit() again.
> I wanted to do it like this so that you get the dmesg
> 
> [194080.788004] w83627ehf: Found NCT6776F chip at 0x290
> [194080.788024] w83627ehf: Enabling fan debounce for chip NCT6776F
> 
> and I didn't want to change too much code, but I'll have a look at it.
> 
Looking further into it, the code to enable debounce should actually be in the _probe 
function. There you also have a sequence with superio enabled. Maybe put your code
just before the superio_exit() there.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors