Re: [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