[LinuxBIOS] [PATCH] probe_superio: rewrite, add ITE support, sophisticated dumping

[Carl-Daniel Hailfinger]

This patch rewrites probe_superio almost completely.
Common code sequences have been factored out, the code has been made
more generic, has better handling of corner cases and is actually much
shorter.

It also adds probing for almost all recent (since 1999) ITE Super I/O
chips to probe_superio. I did verify against all ITE datasheets
(including those not available any more) that the probing was
non-destructive.

For the ITE IT8712F, the complete configuration is dumped and as
comparison the default value from the data sheet is printed.
More information can be extracted easily, however this needs loads of
datasheet surfing.

This code has been tested extensively, dumping for other ITE chips will
follow as a separate patch.

Signed-off-by: Carl-Daniel Hailfinger

--- LinuxBIOSv2/util/probe_superio/probe_superio.c	(Revision 2748)
+++ LinuxBIOSv2/util/probe_superio/probe_superio.c	(Arbeitskopie)
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2006 Ronald Minnich 
  * Copyright (C) 2006 coresystems GmbH 
+ * Copyright (C) 2007 Carl-Daniel Hailfinger
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -32,11 +33,16 @@
   * if we need this. 
   */

-unsigned char regval(unsigned short port, unsigned short reg) {
+unsigned char regval(unsigned short port, unsigned char reg) {
    outb(reg, port);
    return inb(port+1);
 }

+void regwrite(unsigned short port, unsigned char reg, unsigned char val) {
+	outb(reg, port);
+	outb(val, port+1);
+}
+
 void
 dump_ns8374(unsigned short port) {
    printf("Enables: 21=%02x, 22=%02x, 23=%02x, 24=%02x, 26=%02x\n", 
@@ -46,15 +52,13 @@
    /* check COM1. This is all we care about at present. */
    printf("COM 1 is Globally %s\n", regval(port,0x26)&8 ? "disabled" : "enabled");
    /* select com1 */
-	outb(0x7, port);
-	outb(3, port+1);
+	regwrite(port, 0x07, 0x03);
    printf("COM 1 is locally %s\n", regval(port, 0x30) & 1 ? "enabled" : "disabled");
    printf("COM1 60=%02x, 61=%02x, 70=%02x, 71=%02x, 74=%02x, 75=%02x, f0=%02x\n", 
    	regval(port, 0x60), regval(port, 0x61), regval(port, 0x70), regval(port, 0x71),
    	regval(port, 0x74), regval(port, 0x75), regval(port, 0xf0));
    /* select gpio */
-	outb(0x7, port);
-	outb(7, port+1);
+	regwrite(port, 0x07, 0x07);
    printf("GPIO is %s\n", regval(port, 0x30) & 1 ? "enabled" : "disabled");
    printf("GPIO 60=%02x, 61=%02x, 70=%02x, 71=%02x, 74=%02x, 75=%02x, f0=%02x\n", 
    	regval(port, 0x60), regval(port, 0x61), regval(port, 0x70), regval(port, 0x71),
@@ -85,38 +89,33 @@
    printf("2b=%02x\n", regval(port, 0x2b));

/* select UART 1 */
-	outb(0x07, port);
-	outb(0x01, port+1);
+	regwrite(port, 0x07, 0x01);
    printf("UART1 is %s\n", regval(port, 0x30) & 1 ? "enabled" : "disabled");
    printf("UART1 base=%02x%02x, irq=%02x, mode=%s\n", 
    	regval(port, 0x60), regval(port, 0x61), regval(port, 0x70)&0x0f, 
    	regval(port, 0xf0)&0x10 ? "RS485":"RS232");

/* select UART 2 */
-	outb(0x07, port);
-	outb(0x02, port+1);
+	regwrite(port, 0x07, 0x02);
    printf("UART2 is %s\n", regval(port, 0x30) & 1 ? "enabled" : "disabled");
    printf("UART2 base=%02x%02x, irq=%02x, mode=%s\n", 
    	regval(port, 0x60), regval(port, 0x61), regval(port, 0x70)&0x0f, 
    	regval(port, 0xf0)&0x10 ? "RS485":"RS232");

/* select Parport */
-	outb(0x07, port);
-	outb(0x03, port+1);
+	regwrite(port, 0x07, 0x03);
    printf("PARPORT is %s\n", regval(port, 0x30) & 1 ? "enabled" : "disabled");
    printf("PARPORT base=%02x%02x, irq=%02x\n", 
    	regval(port, 0x60), regval(port, 0x61), regval(port, 0x70)&0x0f);

/* select hw monitor */
-	outb(0x07, port);
-	outb(0x04, port+1);
+	regwrite(port, 0x07, 0x04);
    printf("HW monitor is %s\n", regval(port, 0x30) & 1 ? "enabled" : "disabled");
    printf("HW monitor base=%02x%02x, irq=%02x\n", 
    	regval(port, 0x60), regval(port, 0x61), regval(port, 0x70)&0x0f);

/* select gpio */
-	outb(0x07, port);
-	outb(0x06, port+1);
+	regwrite(port, 0x07, 0x05);
    printf("GPIO is %s\n", regval(port, 0x30) & 1 ? "enabled" : "disabled");
    printf("GPIO 70=%02x, e0=%02x, e1=%02x, e2=%02x, e3=%02x, e4=%02x, e5=%02x\n", 
    	regval(port, 0x70), regval(port, 0xe0), regval(port, 0xe1), regval(port, 0xe2),
@@ -130,8 +129,152 @@

}

+//End Of Table
+#define EOT -1
+//NO LDN needed
+#define NOLDN -2
+//Not Available
+#define NANA -3
+//Biggest LDN
+#define MAXLDN 0xa
+//MAXimum NUMber of Indexes
+#define MAXNUMIDX 70
+const static struct ite_registers {
+	signed short superio_id; //yes, it should be unsigned, but then EOT has to be positive
+	struct ite_ldnidx {
+		signed short ldn;
+		signed short idx[MAXNUMIDX+1];
+		signed short def[MAXNUMIDX+1];
+	} ldn[MAXLDN+3]; //biggestLDN+0+NOLDN+EOT
+} ite_reg_table[] = {
+	{0x8712,{
+		{NOLDN,
+			{0x07,0x20,0x21,0x22,0x23,0x24,0x2b,EOT},
+			{NANA,0x87,0x12,0x08,0x00,0x00,0x00,EOT}},
+		{0x0,
+			{0x30,0x60,0x61,0x70,0x74,0xf0,0xf1,EOT},
+			{0x00,0x03,0xf0,0x06,0x02,0x00,0x00,EOT}},
+		{0x1,
+			{0x30,0x60,0x61,0x70,0xf0,0xf1,0xf2,0xf3,EOT},
+			{0x00,0x03,0xf8,0x04,0x00,0x50,0x00,0x7f,EOT}},
+		{0x2,
+			{0x30,0x60,0x61,0x70,0xf0,0xf1,0xf2,0xf3,EOT},
+			{0x00,0x02,0xf8,0x03,0x00,0x50,0x00,0x7f,EOT}},
+		{0x3,
+			{0x30,0x60,0x61,0x62,0x63,0x70,0x74,0xf0,EOT},
+			{0x00,0x03,0x78,0x07,0x78,0x07,0x03,0x03,EOT}},
+		{0x4,
+			{0x30,0x60,0x61,0x62,0x63,0x70,0xf0,0xf1,0xf2,0xf3,0xf4,0xf5,0xf6,EOT},
+			{0x00,0x02,0x90,0x02,0x30,0x09,0x00,0x00,0x00,0x00,0x00,NANA,NANA,EOT}},
+		{0x5,
+			{0x30,0x60,0x61,0x62,0x63,0x70,0x71,0xf0,EOT},
+			{0x01,0x00,0x60,0x00,0x64,0x01,0x02,0x00,EOT}},
+		{0x6,
+			{0x30,0x70,0x71,0xf0,EOT},
+			{0x00,0x0c,0x02,0x00,EOT}},
+		{0x7,
+			{0x25,0x26,0x27,0x28,0x29,0x2a,0x2c,0x60,0x61,0x62,0x63,0x64,0x65,0x70,0x71,0x72,0x73,0x74,0xb0,0xb1,0xb2,0xb3,0xb4,0xb5,0xb8,0xb9,0xba,0xbb,0xbc,0xbd,0xc0,0xc1,0xc2,0xc3,0xc4,0xc8,0xc9,0xca,0xcb,0xcc,0xe0,0xe1,0xe2,0xe3,0xe4,0xf0,0xf1,0xf2,0xf3,0xf4,0xf5,0xf6,0xf7,0xf8,0xf9,0xfa,0xfb,0xfc,0xfd,EOT},
+			{0x01,0x00,0x00,0x40,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x30,0x38,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x01,0x00,0x00,0x40,0x00,0x01,0x00,0x00,0x40,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,NANA,0x00,EOT}},
+		{0x8,
+			{0x30,0x60,0x61,0x70,0xf0,EOT},
+			{0x00,0x03,0x00,0x0a,0x00,EOT}},
+		{0x9,
+			{0x30,0x60,0x61,EOT},
+			{0x00,0x02,0x01,EOT}},
+		{0xa,
+			{0x30,0x60,0x61,0x70,0xf0,EOT},
+			{0x00,0x03,0x10,0x0b,0x00,EOT}},
+		{EOT}}},
+	{EOT}
+};

void
+dump_ite(unsigned short port, unsigned short id) 
+{
+	int i, j, k;
+	signed short *idx;
+	printf ("ITE ");
+
+
+	/* ID Mapping Table
+	   unknown -> IT8711 (no datasheet)
+	   unknown -> IT8722 (no datasheet)
+	   0x8702 -> IT8702
+	   0x8705 -> IT8700 or IT8705
+	   0x8708 -> IT8708
+	   0x8710 -> IT8710
+	   0x8712 -> IT8712
+	   0x8716 -> IT8716 or IT8726 (identical except CPU voltage control)
+	   0x8718 -> IT8718
+	   0x8726 does not exist according to datasheet, but experience differs
+	*/
+	switch(id) {
+		case 0x8702:
+			printf ("IT8702\n");
+			break;
+		case 0x8705:
+			printf ("IT8700 or IT8705\n");
+			break;
+		case 0x8710:
+			printf ("IT8710\n");
+			break;
+		case 0x8712:
+			printf ("IT8712\n");
+			for (i=0;; i++) {
+				if (ite_reg_table[i].superio_id == EOT)
+					break;
+				if ((unsigned short)ite_reg_table[i].superio_id != id)
+					continue;
+				for (j=0;; j++) {
+					if (ite_reg_table[i].ldn[j].ldn == EOT)
+						break;
+					if (ite_reg_table[i].ldn[j].ldn != NOLDN) {
+						printf("switching to LDN 0x%01x\n", ite_reg_table[i].ldn[j].ldn);
+						regwrite(port, 0x07, ite_reg_table[i].ldn[j].ldn);
+					}
+					idx = ite_reg_table[i].ldn[j].idx;
+					printf("idx ");
+					for (k=0;; k++) {
+						if (idx[k] == EOT)
+							break;
+						printf("%02x ", idx[k]);
+					}
+					printf("\nval ");
+					for (k=0;; k++) {
+						if (idx[k] == EOT)
+							break;
+						printf("%02x ", regval(port, idx[k]));
+					}
+					printf("\ndef ");
+					idx = ite_reg_table[i].ldn[j].def;
+					for (k=0;; k++) {
+						if (idx[k] == EOT)
+							break;
+						if (idx[k] == NANA)
+							printf("NA ");
+						else
+							printf("%02x ", idx[k]);
+					}
+					printf("\n");
+				}
+					
+			}
+			break;
+		case 0x8716:
+			printf ("IT8716 or IT8726\n");
+			break;
+		case 0x8718:
+			printf ("IT8718\n");
+			break;
+		default:
+			printf ("unknown ITE chip, id=%04x\n", id);
+			for (i=0x20; i<=0x24; i++)
+				printf("index %02x=%02x\n", i, regval(port, i));
+			break;
+	}
+}
+
+void
 probe_idregs_simple(unsigned short port){
    unsigned char id;
    outb(0x20, port);
@@ -167,9 +310,10 @@

void
 probe_idregs_fintek(unsigned short port){
-	unsigned int vid, did;
+	unsigned int vid, did, success = 0;

// Enable configuration sequence (Fintek uses this for example)
+	// Older ITE chips have the same enable sequence
    outb(0x87, port);
    outb(0x87, port);

@@ -185,13 +329,10 @@
    }
    did = inb(port+1);

-	outb(0x21, port);
-	did = did|(inb(port+1)<<8);
+	did = did|(regval(port, 0x21)<<8);

-	outb(0x23, port);
-	vid = inb(port+1);
-	outb(0x24, port);
-	vid = vid|(inb(port+1)<<8);
+	vid = regval(port, 0x23);
+	vid = vid|(regval(port, 0x24)<<8);

printf("SuperIO found at 0x%02x: vid=0x%04x/did=0x%04x\n", port, vid, did);

@@ -199,23 +340,104 @@
    	return;

// printf("%s\n", familyid[id]);
+	switch(did) {
+		case 0x0887: // reversed for ITE8708
+		case 0x1087: // reversed for ITE8710
+			success = 1;
+			dump_ite(port, ((did & 0xff) << 8) | ((did & 0xff00) >> 8));
+			// disable configuration
+			regwrite(port, 0x02, 0x02);
+			break;
+		default:
+			break;
+	}
    switch(vid) {
    	case 0x3419:
+			success = 1;
    		dump_fintek(port, did);
    		break;
    	default:
-			printf("no dump for 0x%04x/0x%04x\n", vid, did);
    		break;
    }
+	if (!success)
+		printf("no dump for vid 0x%04x, did 0x%04x\n", vid, did);

-	// disable configuration
+	// disable configuration (for Fintek, doesn't hurt ITE)
    outb(0xaa, port);
 }

void
+probe_idregs_ite(unsigned short port){
+	unsigned int id, chipver;
+
+	// Enable configuration sequence (ITE uses this for newer IT87[012]x)
+	// IT871[01] uses 0x87, 0x87 -> fintek detection should handle it
+	// IT8761 uses 0x87, 0x61, 0x55, 0x55/0xaa
+	// IT86xx series uses different ports
+	// IT8661 uses 0x86, 0x61, 0x55/0xaa, 0x55/0xaa and 32 more writes
+	// IT8673 uses 0x86, 0x80, 0x55/0xaa, 0x55/0xaa and 32 more writes
+	outb(0x87, port);
+	outb(0x01, port);
+	outb(0x55, port);
+	if (port == 0x2e)
+		outb(0x55, port);
+	else
+		outb(0xAA, port);
+
+	// Read Chip ID Byte 1
+	id = regval(port, 0x20);
+	if (id != 0x87)  {
+		if (inb(port) == 0xff )
+			printf ("No SuperIO chip found at 0x%04x\n", port);
+		else
+			printf("probing 0x%04x, failed (0x%02x), data returns 0x%02x\n", 
+					port, inb(port), inb(port+1));
+		return;
+	}
+
+	id <<= 8;
+
+	// Read Chip ID Byte 2
+	id |= regval(port, 0x21);
+
+	// Read Chip Version, only bit 3..0 for all IT87xx
+	chipver = regval(port, 0x22) & 0x0f;
+
+	/* ID Mapping Table
+	   unknown -> IT8711 (no datasheet)
+	   unknown -> IT8722 (no datasheet)
+	   0x8702 -> IT8702
+	   0x8705 -> IT8700 or IT8705
+	   0x8710 -> IT8710
+	   0x8712 -> IT8712
+	   0x8716 -> IT8716 or IT8726 (identical except CPU voltage control)
+	   0x8718 -> IT8718
+	*/
+	printf("ITE? SuperIO found at 0x%02x: id=0x%04x, chipver=0x%01x\n",
+	       port, id, chipver);
+
+	switch(id) {
+		case 0x8702:
+		case 0x8705:
+		case 0x8710: //pointless, IT8710 has different enable sequence
+		case 0x8712:
+		case 0x8716:
+		case 0x8718:
+			dump_ite(port, id);
+			break;
+		default:
+			printf("no dump for id 0x%04x\n", id);
+			break;
+	}
+	// disable configuration
+	regwrite(port, 0x02, 0x02);
+}
+
+void
 probe_superio(unsigned short port) {
    probe_idregs_simple(port);
    probe_idregs_fintek(port);
+	probe_idregs_ite(port);
 }

int

[Uwe Hermann]

On Fri, Aug 24, 2007 at 03:36:11AM +0200, Carl-Daniel Hailfinger wrote:
> --- LinuxBIOSv2/util/probe_superio/probe_superio.c	(Revision 2748)
> +++ LinuxBIOSv2/util/probe_superio/probe_superio.c	(Arbeitskopie)
> @@ -3,6 +3,7 @@
> Copyright (C) 2006 Ronald Minnich
> Copyright (C) 2006 coresystems GmbH
> Copyright (C) 2007 Carl-Daniel Hailfinger

Add email address? Or is this on purpose?
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
>
> @@ -32,11 +33,16 @@
> if we need this.
>
> */
>
> -unsigned char regval(unsigned short port, unsigned short reg) {
> +unsigned char regval(unsigned short port, unsigned char reg) {

Can we make the whole code use uint8_t etc. where appropriate?
> outb(reg, port);
>    return inb(port+1);
>  }
>
> +void regwrite(unsigned short port, unsigned char reg, unsigned char val) {
> outb(reg, port);
> outb(val, port+1);

Some coding style changes are needed, e.g. "port + 1" (and other changes
in various other places; please use indent and/or fix this manually).
> +//End Of Table

Use C-style comments everywhere please (yes, it's only a minor issue).
> {0x4,
> 	{0x30,0x60,0x61,0x62,0x63,0x70,0xf0,0xf1,0xf2,0xf3,0xf4,0xf5,0xf6,EOT},
> 	{0x00,0x02,0x90,0x02,0x30,0x09,0x00,0x00,0x00,0x00,0x00,NANA,NANA,EOT}},
> {0x5,
> 	{0x30,0x60,0x61,0x62,0x63,0x70,0x71,0xf0,EOT},
> 	{0x01,0x00,0x60,0x00,0x64,0x01,0x02,0x00,EOT}},
> {0x6,
> 	{0x30,0x70,0x71,0xf0,EOT},
> 	{0x00,0x0c,0x02,0x00,EOT}},
> {0x7,
> 	{0x25,0x26,0x27,0x28,0x29,0x2a,0x2c,0x60,0x61,0x62,0x63,0x64,0x65,0x70,0x71,0x72,0x73,0x74,0xb0,0xb1,0xb2,0xb3,0xb4,0xb5,0xb8,0xb9,0xba,0xbb,0xbc,0xbd,0xc0,0xc1,0xc2,0xc3,0xc4,0xc8,0xc9,0xca,0xcb,0xcc,0xe0,0xe1,0xe2,0xe3,0xe4,0xf0,0xf1,0xf2,0xf3,0xf4,0xf5,0xf6,0xf7,0xf8,0xf9,0xfa,0xfb,0xfc,0xfd,EOT},
> 	{0x01,0x00,0x00,0x40,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x30,0x38,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x01,0x00,0x00,0x40,0x00,0x01,0x00,0x00,0x40,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,NANA,0x00,EOT}},

Don't make these lines longer than 79 characters, please.
> case 0x8712:
> 	printf ("IT8712\n");
> 	for (i=0;; i++) {
> 		if (ite_reg_table[i].superio_id == EOT)
> 			break;
> 		if ((unsigned short)ite_reg_table[i].superio_id != id)
> 			continue;
> 		for (j=0;; j++) {
> 			if (ite_reg_table[i].ldn[j].ldn == EOT)
> 				break;
> 			if (ite_reg_table[i].ldn[j].ldn != NOLDN) {
> 				printf("switching to LDN 0x%01x\n", ite_reg_table[i].ldn[j].ldn);
> 				regwrite(port, 0x07, ite_reg_table[i].ldn[j].ldn);
> 			}
> 			idx = ite_reg_table[i].ldn[j].idx;
> 			printf("idx ");
> 			for (k=0;; k++) {
> 				if (idx[k] == EOT)
> 					break;
> 				printf("%02x ", idx[k]);
> 			}
> 			printf("\nval ");
> 			for (k=0;; k++) {
> 				if (idx[k] == EOT)
> 					break;
> 				printf("%02x ", regval(port, idx[k]));
> 			}
> 			printf("\ndef ");
> 			idx = ite_reg_table[i].ldn[j].def;
> 			for (k=0;; k++) {
> 				if (idx[k] == EOT)
> 					break;
> 				if (idx[k] == NANA)
> 					printf("NA ");
> 				else
> 					printf("%02x ", idx[k]);
> 			}
> 			printf("\n");
> 		}
> 	}

The indentation level gets pretty high here, maybe this can rewritten
with less nesting and/or using a helper function?
> outb(0x21, port);
> did = did|(inb(port+1)<<8);
> did = did|(regval(port, 0x21)<<8);

did |= regval(port, 0x21) << 8;
> outb(0x23, port);
> vid = inb(port+1);
> outb(0x24, port);
> vid = vid|(inb(port+1)<<8);
> vid = regval(port, 0x23);
> vid = vid|(regval(port, 0x24)<<8);

vid |= regval(port, 0x24) << 8;
> printf("SuperIO found at 0x%02x: vid=0x%04x/did=0x%04x\n", port, vid, did);

"Super I/O" to be consistent, we use that spelling everywhere else.
> /* ID Mapping Table
>   unknown -> IT8711 (no datasheet)
>   unknown -> IT8722 (no datasheet)
>   0x8702 -> IT8702
>   0x8705 -> IT8700 or IT8705
>   0x8710 -> IT8710
>   0x8712 -> IT8712
>   0x8716 -> IT8716 or IT8726 (identical except CPU voltage control)
>   0x8718 -> IT8718
> */
> printf("ITE? SuperIO found at 0x%02x: id=0x%04x, chipver=0x%01x\n",

s/ITE?/ITE/

This is a really nice patch, on the long run we can make probe_superio
(though I would rename it to 'superio-detect' personally) a
general-purpose tool for detecting and investigating Super I/Os.

Yes, there's sensorѕ-detect (from the lm-sensors project) which has an
overlapping functionality, but I'm not sure how that works internally
or how elaborate the detection and info is.

We should check if it makes sense to merge our code into sensors-detect
(or vice versa?) or not.

Uwe.

[LinuxBIOS] [PATCH] probe_superio: rewrite, add ITE support, sophisticated dumping

[Carl-Daniel Hailfinger]

On 24.08.2007 12:22, Uwe Hermann wrote:
> On Fri, Aug 24, 2007 at 03:36:11AM +0200, Carl-Daniel Hailfinger wrote:
> > --- LinuxBIOSv2/util/probe_superio/probe_superio.c	(Revision 2748)
> > +++ LinuxBIOSv2/util/probe_superio/probe_superio.c	(Arbeitskopie)
> > @@ -3,6 +3,7 @@
> > Copyright (C) 2006 Ronald Minnich
> > Copyright (C) 2006 coresystems GmbH
> > Copyright (C) 2007 Carl-Daniel Hailfinger
>
> Add email address? Or is this on purpose?

Purpose. My mail address will change soon, besides that it would violate
the 72 char limit.
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License as published by
> >
> > @@ -32,11 +33,16 @@
> > if we need this.
> >
> > */
> >
> > -unsigned char regval(unsigned short port, unsigned short reg) {
> > +unsigned char regval(unsigned short port, unsigned char reg) {
>
> Can we make the whole code use uint8_t etc. where appropriate?

I hate uint*_t, but yes, I can do that (or switch to u*).
> > outb(reg, port);
> >    return inb(port+1);
> >  }
> >
> > +void regwrite(unsigned short port, unsigned char reg, unsigned char val) {
> > outb(reg, port);
> > outb(val, port+1);
>
> Some coding style changes are needed, e.g. "port + 1" (and other changes
> in various other places; please use indent and/or fix this manually).

OK
> > +//End Of Table
>
> Use C-style comments everywhere please (yes, it's only a minor issue).

OK
> > {0x4,
> > 	{0x30,0x60,0x61,0x62,0x63,0x70,0xf0,0xf1,0xf2,0xf3,0xf4,0xf5,0xf6,EOT},
> > 	{0x00,0x02,0x90,0x02,0x30,0x09,0x00,0x00,0x00,0x00,0x00,NANA,NANA,EOT}},
> > {0x5,
> > 	{0x30,0x60,0x61,0x62,0x63,0x70,0x71,0xf0,EOT},
> > 	{0x01,0x00,0x60,0x00,0x64,0x01,0x02,0x00,EOT}},
> > {0x6,
> > 	{0x30,0x70,0x71,0xf0,EOT},
> > 	{0x00,0x0c,0x02,0x00,EOT}},
> > {0x7,
> > 	{0x25,0x26,0x27,0x28,0x29,0x2a,0x2c,0x60,0x61,0x62,0x63,0x64,0x65,0x70,0x71,0x72,0x73,0x74,0xb0,0xb1,0xb2,0xb3,0xb4,0xb5,0xb8,0xb9,0xba,0xbb,0xbc,0xbd,0xc0,0xc1,0xc2,0xc3,0xc4,0xc8,0xc9,0xca,0xcb,0xcc,0xe0,0xe1,0xe2,0xe3,0xe4,0xf0,0xf1,0xf2,0xf3,0xf4,0xf5,0xf6,0xf7,0xf8,0xf9,0xfa,0xfb,0xfc,0xfd,EOT},
> > 	{0x01,0x00,0x00,0x40,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x30,0x38,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x01,0x00,0x00,0x40,0x00,0x01,0x00,0x00,0x40,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,NANA,0x00,EOT}},
>
> Don't make these lines longer than 79 characters, please.

Will fixup.
> > case 0x8712:
> > 	printf ("IT8712\n");
> > 	for (i=0;; i++) {
> > 		if (ite_reg_table[i].superio_id == EOT)
> > 			break;
> > 		if ((unsigned short)ite_reg_table[i].superio_id != id)
> > 			continue;
> > 		for (j=0;; j++) {
> > 			if (ite_reg_table[i].ldn[j].ldn == EOT)
> > 				break;
> > 			if (ite_reg_table[i].ldn[j].ldn != NOLDN) {
> > 				printf("switching to LDN 0x%01x\n", ite_reg_table[i].ldn[j].ldn);
> > 				regwrite(port, 0x07, ite_reg_table[i].ldn[j].ldn);
> > 			}
> > 			idx = ite_reg_table[i].ldn[j].idx;
> > 			printf("idx ");
> > 			for (k=0;; k++) {
> > 				if (idx[k] == EOT)
> > 					break;
> > 				printf("%02x ", idx[k]);
> > 			}
> > 			printf("\nval ");
> > 			for (k=0;; k++) {
> > 				if (idx[k] == EOT)
> > 					break;
> > 				printf("%02x ", regval(port, idx[k]));
> > 			}
> > 			printf("\ndef ");
> > 			idx = ite_reg_table[i].ldn[j].def;
> > 			for (k=0;; k++) {
> > 				if (idx[k] == EOT)
> > 					break;
> > 				if (idx[k] == NANA)
> > 					printf("NA ");
> > 				else
> > 					printf("%02x ", idx[k]);
> > 			}
> > 			printf("\n");
> > 		}
> > 	}
>
> The indentation level gets pretty high here, maybe this can rewritten
> with less nesting and/or using a helper function?

Will look into that.
> > outb(0x21, port);
> > did = did|(inb(port+1)<<8);
> > did = did|(regval(port, 0x21)<<8);
>
> did |= regval(port, 0x21) << 8;
> > outb(0x23, port);
> > vid = inb(port+1);
> > outb(0x24, port);
> > vid = vid|(inb(port+1)<<8);
> > vid = regval(port, 0x23);
> > vid = vid|(regval(port, 0x24)<<8);
>
> vid |= regval(port, 0x24) << 8;

This was mostly an automatic conversion, but yes.
> > printf("SuperIO found at 0x%02x: vid=0x%04x/did=0x%04x\n", port, vid, did);
>
> "Super I/O" to be consistent, we use that spelling everywhere else.
> > /* ID Mapping Table
> >   unknown -> IT8711 (no datasheet)
> >   unknown -> IT8722 (no datasheet)
> >   0x8702 -> IT8702
> >   0x8705 -> IT8700 or IT8705
> >   0x8710 -> IT8710
> >   0x8712 -> IT8712
> >   0x8716 -> IT8716 or IT8726 (identical except CPU voltage control)
> >   0x8718 -> IT8718
> > */
> > printf("ITE? SuperIO found at 0x%02x: id=0x%04x, chipver=0x%01x\n",
>
> s/ITE?/ITE/

Intentional, since some other manufacturers have the same enable sequence.
> This is a really nice patch, on the long run we can make probe_superio
> (though I would rename it to 'superio-detect' personally) a
> general-purpose tool for detecting and investigating Super I/Os.

Can you commit? I'll send a followup patch to fix the issues you
mentioned. My harddisk is flaky and I'd like to make sure no code I work
on gets lost.

Besides that, the renaming should be done eventually.
* superio_detect?
* superio_dump?
* superio_analyze?

And then we want to have it in v3 via svn:externals.

Regards,
Carl-Daniel

[LinuxBIOS] [PATCH] probe_superio: rewrite, add ITE support, sophisticated dumping

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> > This is a really nice patch, on the long run we can make probe_superio
> > (though I would rename it to 'superio-detect' personally) a
> > general-purpose tool for detecting and investigating Super I/Os.
>
> Can you commit? I'll send a followup patch to fix the issues you
> mentioned. My harddisk is flaky and I'd like to make sure no code I work
> on gets lost.

Committed revision 2749. Please send updates ;-)
> Besides that, the renaming should be done eventually.
> superio_detect?
> superio_dump?
> superio_analyze?

Sounds good.
> And then we want to have it in v3 via svn:externals.

Sounds good, too.

[Uwe Hermann]

On Mon, Aug 27, 2007 at 09:30:15AM +0200, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > > This is a really nice patch, on the long run we can make probe_superio
> > > (though I would rename it to 'superio-detect' personally) a
> > > general-purpose tool for detecting and investigating Super I/Os.
> >
> > Can you commit? I'll send a followup patch to fix the issues you
> > mentioned. My harddisk is flaky and I'd like to make sure no code I work
> > on gets lost.
>
> Committed revision 2749. Please send updates ;-)
> > Besides that, the renaming should be done eventually.
> > superio_detect?
> > superio_dump?
> > superio_analyze?
>
> Sounds good.

I'd prefer superio-detect, I don't like executable names with
underscores...
> > And then we want to have it in v3 via svn:externals.
>
> Sounds good, too.

Not sure; do we want _all_ utilities replicated in all versions of
LinuxBIOS? I'd say we move all common utilities into util/
(will send a proposal later), and only replicate very few, often-needed
utilities in the respective v2/v3 trees? Comments?

Uwe.

[Stefan Reinauer]

* Uwe Hermann  [070827 10:05]:
> Not sure; do we want _all_ utilities replicated in all versions of
> LinuxBIOS? I'd say we move all common utilities into util/
> (will send a proposal later), and only replicate very few, often-needed
> utilities in the respective v2/v3 trees? Comments?

Definitely not all of them. I think we should let all unused utilities
die in their respective versions of LinuxBIOS.

I think the way we have been doing it was a good one: 
* We move the utilities as they are being worked on. 
* Stuff that is specific to a certain version (like lar) goes into 
  that version. 
* All generic stuff goes to the global util.

Stefan