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

[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.