Re: Fwd: Patch for Fintek F71869 watchdoh

Re: Fwd: Patch for Fintek F71869 watchdoh

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

On Mon, Dec 06, 2010 at 11:14:05 +0100, Hans de Goede wrote:
>> -------- Original Message --------
>> Subject: Patch for Fintek F71869 watchdoh
>> Date: Mon, 6 Dec 2010 00:40:43 +0100
>> From: Michel Arboi <michel@arboi.fr.eu.org>
>> Organization: Compilo ergo sum
>> To: Hans de Goede <hdegoede@redhat.com>
>> 
>> I'm not sure that you are the right person, but actually I do not know
>> who I should contact. I saw that you were working on the support of
>> the Fintek F71869 sensors.

These are the e-mail addresses to send patches for that driver to:
> ~/git/linux-2.6 $ scripts/get_maintainer.pl -f drivers/watchdog/f71808e_wdt.c
> Wim Van Sebroeck <wim@iguana.be>
> Giel van Schijndel <me@mortis.eu>
> linux-watchdog@vger.kernel.org
> linux-kernel@vger.kernel.org

>> I have a Jetway NC9C-550-LF mobo, I quickly hacked this to support the
>> watchdog. It seems to work.
> 
> Here is a patch send to me to add support for the F71869 to the Fintek
> superio watchdog driver.
> 
> Michel, I've forwarded you're patch to Giel, who is the author and
> maintainer of the Fintek superio watchdog driver.

> --- ./drivers/watchdog/f71808e_wdt.c	2010-10-20 22:30:22.000000000 +0200
> +++ /tmp/f71808e_wdt.c	2010-12-05 01:19:48.819567802 +0100
> @@ -52,6 +52,8 @@
>  #define SIO_F71882_ID		0x0541	/* Chipset ID */
>  #define SIO_F71889_ID		0x0723	/* Chipset ID */
>  
> +#define SIO_F71869_ID		0x0814
> +
>  #define	F71882FG_REG_START		0x01
>  
>  #define F71808FG_REG_WDO_CONF		0xf0

Please insert that constant in the list of other Chipset ID's
alphabetically sorted (to keep that list consistent).

> @@ -98,7 +100,7 @@
>  MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
>  	" given initial timeout. Zero (default) disables this feature.");
>  
> -enum chips { f71808fg, f71858fg, f71862fg, f71882fg, f71889fg };
> +enum chips { f71808fg, f71858fg, f71862fg, f71882fg, f71889fg, f71869 };
>  
>  static const char *f71808e_names[] = {
>  	"f71808fg",
> @@ -106,6 +108,7 @@
>  	"f71862fg",
>  	"f71882fg",
>  	"f71889fg",
> +	"f71869",
>  };
>  
>  /* Super-I/O Function prototypes */
> @@ -308,6 +311,10 @@
>  		superio_set_bit(watchdog.sioaddr, 0x29, 1);
>  		break;
>  
> +	case f71869:
> +		/* GPIO14 --> WDTRST# */
> +		superio_clear_bit(watchdog.sioaddr, 0x29, 4);
> +		break;
>  	default:
>  		/*
>  		 * 'default' label to shut up the compiler and catch
> @@ -708,6 +715,9 @@
>  	case SIO_F71882_ID:
>  		watchdog.type = f71882fg;
>  		break;
> +	case SIO_F71869_ID:
> +		watchdog.type = f71869;
> +		break;
>  	case SIO_F71862_ID:
>  	case SIO_F71889_ID:
>  		/* These have a watchdog, though it isn't implemented (yet). */

The rest of that patch looks fine.  So if you perform the above
modification you'll get my Ack.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Measuring programming progress by lines of code is like measuring
 aircraft building progress by weight."
  -- Bill Gates

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Patch for Fintek F71869 watchdoh

[Michel Arboi]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On Mon, 6 Dec 2010 20:19:30 +0100
Giel van Schijndel <me@mortis.eu> wrote:

> Please insert that constant in the list of other Chipset ID's
> alphabetically sorted (to keep that list consistent).

Like this?

> The rest of that patch looks fine.  So if you perform the above
> modification you'll get my Ack.

Please note that this was just a quick hack. My basic tests showed that
the board did not reboot by itself, and it rebooted when I stopped the
watchdog process.
I downloaded the chip datasheet from Fintek's site but could not make
much sense out of it. It is rather terse.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: f71808e_wdt.patch --]
[-- Type: text/x-patch, Size: 1415 bytes --]

--- ./drivers/watchdog/f71808e_wdt.c	2010-10-20 22:30:22.000000000 +0200
+++ /tmp/f71808e_wdt.c	2010-12-06 20:48:00.257989527 +0100
@@ -49,6 +49,7 @@
 #define SIO_F71808_ID		0x0901  /* Chipset ID */
 #define SIO_F71858_ID		0x0507  /* Chipset ID */
 #define SIO_F71862_ID		0x0601	/* Chipset ID */
+#define SIO_F71869_ID		0x0814
 #define SIO_F71882_ID		0x0541	/* Chipset ID */
 #define SIO_F71889_ID		0x0723	/* Chipset ID */
 
@@ -98,12 +99,13 @@
 MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
 	" given initial timeout. Zero (default) disables this feature.");
 
-enum chips { f71808fg, f71858fg, f71862fg, f71882fg, f71889fg };
+enum chips { f71808fg, f71858fg, f71862fg, f71869, f71882fg, f71889fg };
 
 static const char *f71808e_names[] = {
 	"f71808fg",
 	"f71858fg",
 	"f71862fg",
+	"f71869",
 	"f71882fg",
 	"f71889fg",
 };
@@ -308,6 +310,10 @@
 		superio_set_bit(watchdog.sioaddr, 0x29, 1);
 		break;
 
+	case f71869:
+		/* GPIO14 --> WDTRST# */
+		superio_clear_bit(watchdog.sioaddr, 0x29, 4);
+		break;
 	default:
 		/*
 		 * 'default' label to shut up the compiler and catch
@@ -708,6 +714,9 @@
 	case SIO_F71882_ID:
 		watchdog.type = f71882fg;
 		break;
+	case SIO_F71869_ID:
+		watchdog.type = f71869;
+		break;
 	case SIO_F71862_ID:
 	case SIO_F71889_ID:
 		/* These have a watchdog, though it isn't implemented (yet). */

Re: Patch for Fintek F71869 watchdoh

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On Mon, Dec 06, 2010 at 20:53:45 +0100, Michel Arboi wrote:
> On Mon, 6 Dec 2010 20:19:30 +0100 Giel van Schijndel <me@mortis.eu> wrote:
>> Please insert that constant in the list of other Chipset ID's
>> alphabetically sorted (to keep that list consistent).
> 
> Like this?

Aye, looks good.

Acked-By: Giel van Schijndel <me@mortis.eu>

>> The rest of that patch looks fine.  So if you perform the above
>> modification you'll get my Ack.
> 
> Please note that this was just a quick hack. My basic tests showed
> that the board did not reboot by itself, and it rebooted when I
> stopped the watchdog process.

That's intended behaviour and probably sufficient for testing.  Most of
the behaviour isn't chip-specific, only the initialisation and
configuration of the chip is specific to that chip AFAIK.

> I downloaded the chip datasheet from Fintek's site but could not make
> much sense out of it. It is rather terse.

Yes, those datasheets can easily give you a headache when you're trying
to make sense of them.

PS You might want to include patches inline (i.e. as part of the message
   text) as that's standard procedure for Linux MLs.  That allows easy
   reviewing and commenting on patches.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Always code as if the guy who ends up maintaining your code will be a
 violent psychopath who knows where you live."
  -- Rick Osborne

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Patch for Fintek F71869 watchdoh

[Michel Arboi]

On Mon, 6 Dec 2010 21:21:55 +0100
Giel van Schijndel <me@mortis.eu> wrote:

> That's intended behaviour and probably sufficient for testing.  Most
> of the behaviour isn't chip-specific, only the initialisation and
> configuration of the chip is specific to that chip AFAIK.

OK. Hans, if you need some tests for the sensors part, you can send me a
patch and I'll try to help.
[I suspect that the current Linux code works, sensors reports 62 °C /
59 °C for the CPU]

> PS You might want to include patches inline (i.e. as part of the
> message text) as that's standard procedure for Linux MLs.  That
> allows easy reviewing and commenting on patches.

OK. In fact I did not know how to submit a kernel patch.