[coreboot] [LinuxBIOS] adding "--list-supported" feature to superiotool #91

[Peter Stuge]

Thanks for the patch! Some comments.

On Sat, Jan 12, 2008 at 06:09:21AM -0500, Robinson Tryon wrote:
> I wasn't very sure about what the "wiki links" part of this feature
> entailed.  I included the primary url for superiotool, but perhaps
> there's something more that needs to be done?

I think there's a page for each supported sio chip with the output
from previous superiotool runs. Or there should be. Would be handy
to have that address available as well.


> Oh, and how should I go about getting an account on Trac?

Stefan should get you hooked up. :)


> +void print_fintek_chips(void) {
> +  char name [] = "Fintek";
> +  print_vendor_chips(name, reg_table);
> +}

Why the extra char[]? The function parameter is const so the name
could just be a string constant in the call.

And thinking further I saw that the vendor name is given as a string
constant in more places in the files. Perhaps make it a #define near
the top of the file? That's a separate patch though, and just a
thought.


> +void print_vendor_chips(const char *vendor,
> +			const struct superio_registers reg_table[]) {	
> +	int i, any_supported_chips = 0;
> +	
> +	printf("  Chips from %s:\n", vendor);
> +	
> +	for (i = 0; /* Nothing */; i++) {
> +		if (reg_table[i].superio_id == EOT)
> +			break;

So why not write the break condition in the for statement?


> +		any_supported_chips = 1;
> +		print_chip(reg_table[i]);
> +	}
> +
> +	if (!any_supported_chips)
> +		printf("    None.\n");

any_supported_chips could just check if 0==i, right?


Also - isn't the vendor inclued in the reg_table? Why is it needed at
all in this function? Sorry if I'm being slow.


//Peter