On Jan 12, 2008 12:20 PM, Peter Stuge <[EMAIL PROTECTED]> wrote:
> 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.
On the wiki page it looked like there were links to several mailing
list posts -- those emails probably contained the dumps for each chip.
Should that information be included in the data section of each vendor
file? If so, should there be a new field in the superio_registers
STRUCT to handle that?
> > Oh, and how should I go about getting an account on Trac?
>
> Stefan should get you hooked up. :)
Great! (hopefully he's reading this thread...)
>
>
> > +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.
Good point. (I haven't been programming in C for quite some time...)
>
> 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.
Yes, I noticed that as well (as well as a couple of other things that
could stand to be refactored). But as you pointed out, one thing at a
time.
>
>
> > +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?
I was just borrowing code? :-)
But yes, that's a good point.
Actually, here's another question for you:
Why is all of the data for superiotool stored in arrays terminated
with EOT? Can't we just use ARRAY_SIZE (or some similar construct) to
figure out the length of a given array before we iterate over it?
>
> > + 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?
yep.
> 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.
I don't believe that the vendor name is included in the reg_table --
just the chip id, chip name, and registers, right?
Here's the updated patch. Let me know how the dump URLs should be
stored, and I'll add support for that as well.
Signed-off-by: Robinson P. Tryon <[EMAIL PROTECTED]>
--R
Index: fintek.c
===================================================================
--- fintek.c (revision 3047)
+++ fintek.c (working copy)
@@ -3,6 +3,7 @@
*
* Copyright (C) 2006 coresystems GmbH <[EMAIL PROTECTED]>
* Copyright (C) 2007 Uwe Hermann <[EMAIL PROTECTED]>
+ * Copyright (C) 2008 Robinson P. Tryon <[EMAIL PROTECTED]>
*
* 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
@@ -99,3 +100,7 @@
exit_conf_mode_winbond_fintek_ite_8787(port);
}
+
+void print_fintek_chips(void) {
+ print_vendor_chips("Fintek", reg_table);
+}
Index: winbond.c
===================================================================
--- winbond.c (revision 3047)
+++ winbond.c (working copy)
@@ -2,6 +2,7 @@
* This file is part of the superiotool project.
*
* Copyright (C) 2007 Uwe Hermann <[EMAIL PROTECTED]>
+ * Copyright (C) 2008 Robinson P. Tryon <[EMAIL PROTECTED]>
*
* 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
@@ -445,3 +446,7 @@
probe_idregs_winbond_helper("(init=0x87,0x87) ", port);
exit_conf_mode_winbond_fintek_ite_8787(port);
}
+
+void print_winbond_chips(void) {
+ print_vendor_chips("Winbond", reg_table);
+}
Index: ite.c
===================================================================
--- ite.c (revision 3047)
+++ ite.c (working copy)
@@ -3,6 +3,7 @@
*
* Copyright (C) 2007 Carl-Daniel Hailfinger
* Copyright (C) 2007 Uwe Hermann <[EMAIL PROTECTED]>
+ * Copyright (C) 2008 Robinson P. Tryon <[EMAIL PROTECTED]>
*
* 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
@@ -380,3 +381,7 @@
probe_idregs_ite_helper("(init=0x87,0x87) ", port);
exit_conf_mode_winbond_fintek_ite_8787(port);
}
+
+void print_ite_chips(void) {
+ print_vendor_chips("ITE", reg_table);
+}
Index: nsc.c
===================================================================
--- nsc.c (revision 3047)
+++ nsc.c (working copy)
@@ -3,6 +3,7 @@
*
* Copyright (C) 2006 Ronald Minnich <[EMAIL PROTECTED]>
* Copyright (C) 2007 Uwe Hermann <[EMAIL PROTECTED]>
+ * Copyright (C) 2008 Robinson P. Tryon <[EMAIL PROTECTED]>
*
* 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
@@ -449,3 +450,7 @@
dump_superio("NSC", reg_table, port, id);
}
+
+void print_nsc_chips(void) {
+ print_vendor_chips("NSC", reg_table);
+}
Index: superiotool.c
===================================================================
--- superiotool.c (revision 3047)
+++ superiotool.c (working copy)
@@ -4,6 +4,7 @@
* Copyright (C) 2006 Ronald Minnich <[EMAIL PROTECTED]>
* Copyright (C) 2007 Uwe Hermann <[EMAIL PROTECTED]>
* Copyright (C) 2007 Carl-Daniel Hailfinger
+ * Copyright (C) 2008 Robinson P. Tryon <[EMAIL PROTECTED]>
*
* 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
@@ -161,6 +162,47 @@
vendor, info, port);
}
+/* Print information about a specific chip. */
+void print_chip(const struct superio_registers reg) {
+ printf(" %s", reg.name);
+ /* Unless the ldn is empty, assume this chip has a dump. */
+ if(reg.ldn[0].ldn != EOT)
+ printf(" (dump)");
+ printf("\n");
+}
+
+/* Print a list of all supported chips from the given vendor. */
+void print_vendor_chips(const char *vendor,
+ const struct superio_registers reg_table[]) {
+ int i;
+
+ printf(" Chips from %s:\n", vendor);
+
+ for (i = 0; reg_table[i].superio_id != EOT; i++) {
+ print_chip(reg_table[i]);
+ }
+
+ if (i == 0)
+ printf(" None.\n");
+}
+
+/* Print a list of all chips supported by superiotool. */
+void print_list_of_supported_chips(void)
+{
+ int i;
+
+ printf("Supported Super I/O Chips:\n");
+ printf(" Notes:\n");
+ printf(" - See http://linuxbios.org/Superiotool for more information.\n");
+ printf(" - Chips with (dump) after them have support for dumping registers.\n");
+ printf("\n");
+
+ for (i = 0; i < ARRAY_SIZE(vendor_print_functions); i++) {
+ vendor_print_functions[i].print_list();
+ printf("\n");
+ }
+}
+
static void print_version(void)
{
printf("superiotool r%s\n", SUPERIOTOOL_VERSION);
@@ -175,10 +217,11 @@
{"verbose", no_argument, NULL, 'V'},
{"version", no_argument, NULL, 'v'},
{"help", no_argument, NULL, 'h'},
+ {"list-supported", no_argument, NULL, 'l'},
{0, 0, 0, 0}
};
- while ((opt = getopt_long(argc, argv, "dVvh",
+ while ((opt = getopt_long(argc, argv, "dVvhl",
long_options, &option_index)) != EOF) {
switch (opt) {
case 'd':
@@ -195,6 +238,10 @@
printf(USAGE);
exit(0);
break;
+ case 'l':
+ print_list_of_supported_chips();
+ exit(0);
+ break;
default:
/* Unknown option. */
exit(1);
Index: ali.c
===================================================================
--- ali.c (revision 3047)
+++ ali.c (working copy)
@@ -2,6 +2,7 @@
* This file is part of the superiotool project.
*
* Copyright (C) 2007 Uwe Hermann <[EMAIL PROTECTED]>
+ * Copyright (C) 2008 Robinson P. Tryon <[EMAIL PROTECTED]>
*
* 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
@@ -99,3 +100,7 @@
exit_conf_mode_ali(port);
}
+
+void print_ali_chips(void) {
+ print_vendor_chips("ALi", reg_table);
+}
Index: smsc.c
===================================================================
--- smsc.c (revision 3047)
+++ smsc.c (working copy)
@@ -2,6 +2,7 @@
* This file is part of the superiotool project.
*
* Copyright (C) 2007 Uwe Hermann <[EMAIL PROTECTED]>
+ * Copyright (C) 2008 Robinson P. Tryon <[EMAIL PROTECTED]>
*
* 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
@@ -546,3 +547,7 @@
probe_idregs_smsc_helper(port, DEVICE_ID_REG, DEVICE_REV_REG);
probe_idregs_smsc_helper(port, DEVICE_ID_REG_OLD, DEVICE_REV_REG_OLD);
}
+
+void print_smsc_chips(void) {
+ print_vendor_chips("SMSC", reg_table);
+}
Index: superiotool.h
===================================================================
--- superiotool.h (revision 3047)
+++ superiotool.h (working copy)
@@ -3,6 +3,7 @@
*
* Copyright (C) 2007 Carl-Daniel Hailfinger
* Copyright (C) 2007 Uwe Hermann <[EMAIL PROTECTED]>
+ * Copyright (C) 2008 Robinson P. Tryon <[EMAIL PROTECTED]>
*
* 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
@@ -81,23 +82,34 @@
uint16_t port, uint16_t id);
void probing_for(const char *vendor, const char *info, uint16_t port);
+void print_chip(const struct superio_registers reg);
+void print_vendor_chips(const char *vendor,
+ const struct superio_registers reg_table[]);
+void print_list_of_supported_chips(void);
+
/* ali.c */
void probe_idregs_ali(uint16_t port);
+void print_ali_chips(void);
/* fintek.c */
void probe_idregs_fintek(uint16_t port);
+void print_fintek_chips(void);
/* ite.c */
void probe_idregs_ite(uint16_t port);
+void print_ite_chips(void);
/* nsc.c */
void probe_idregs_nsc(uint16_t port);
+void print_nsc_chips(void);
/* smsc.c */
void probe_idregs_smsc(uint16_t port);
+void print_smsc_chips(void);
/* winbond.c */
void probe_idregs_winbond(uint16_t port);
+void print_winbond_chips(void);
/** Table of which config ports to probe for each Super I/O family. */
static const struct {
@@ -112,4 +124,16 @@
{probe_idregs_winbond, {0x2e, 0x4e, 0x3f0, 0x370, 0x250, EOT}},
};
+
+/** Table of functions to print out supported Super I/O chips for each vendor */
+static const struct {
+ void (*print_list) (void);
+} vendor_print_functions [] = {
+ {print_ali_chips},
+ {print_fintek_chips},
+ {print_ite_chips},
+ {print_nsc_chips},
+ {print_smsc_chips},
+ {print_winbond_chips}};
+
#endif
--
coreboot mailing list
[email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
