[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 <rminnich at gmail.com>
>>   * Copyright (C) 2006 coresystems GmbH <info at coresystems.de>
>> + * 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