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

[Robinson Tryon]

Here's a first shot at bug #91
(http://tracker.linuxbios.org/trac/LinuxBIOS/ticket/91), adding a
--list-supported switch to superiotool.

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?

In terms of formatting, I just grouped the chips by vendor.  For chips
that have dump support I added "(dump)" after their name.

Signed-off-by: Robinson P. Tryon

Oh, and how should I go about getting an account on Trac?  I'd be
happy to take a stab at adding --list-supported to flashrom (#90) once
this patch passes muster.

--R

[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

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

[Robinson Tryon]

On Jan 12, 2008 12:20 PM, Peter Stuge  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

--R

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

[Uwe Hermann]

On Sat, Jan 12, 2008 at 05:46:38PM -0500, Robinson Tryon wrote:
> On the wiki page it looked like there were links to several mailing
> list posts -- those emails probably contained the dumps for each chip.

Yes.
> 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?

Not sure I understand what you mean, but I think the answer is no.
_One_ link to http://coreboot.org/Superiotool#Supported_devices
is more than enough. I don't think it makes sense to have an extra
wiki page for each chip or an extra link for each chip in the
--list-supported output, if that's what you mean.
> 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?

Not all entries have the same length. Using the EOT method is nice, IMO.
> > 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?

Yep.
> Here's the updated patch.  Let me know how the dump URLs should be
> stored, and I'll add support for that as well.

Not needed. One link to http://coreboot.org/Superiotool#Supported_devices
at the bottom of the output or so should be enough.
> +/* Print information about a specific chip. */
> +void print_chip(const struct superio_registers reg) {
> printf("    %s", reg.name);

I'd make this function take a "const char *vendor" as argument and
print the vendor in each line, without indentation. I find that format
looks more readable (and is probably easier to parse/grep/sed if we ever
want that). Example output from a quick test I did:

Fintek F71862FG
Fintek F71872F/FG / F71806F/FG
Fintek F71882FG/F71883FG
Fintek F71805F/FG (dump)

ITE IT8661F (dump)
ITE IT8702F
ITE IT8705F/AF / IT8700F (dump)
ITE IT8706R
ITE IT8708F (dump)
ITE IT8710F
ITE IT8712F (dump)
ITE IT8716F (dump)
ITE IT8718F (dump)
ITE IT8726F

NSC PC97307 (dump)
NSC PC87317 (dump)
NSC PC97317 (dump)
NSC PC87309 (dump)
NSC PC87360 (dump)
NSC PC87351 (dump)
NSC PC87364
NSC PC87365
NSC PC87363
NSC PC87366 (dump)
NSC PC8739x
NSC PC87591x
NSC PC8741x (dump)
NSC PC87372
NSC PC8374L (dump)
NSC PC87427
NSC PC87373

SMSC FDC37C93xFR
SMSC FDC37N971
SMSC FDC37N972
SMSC LPC47N252
[...]
> /* Unless the ldn is empty, assume this chip has a dump. */
> if(reg.ldn[0].ldn != EOT)
> printf(" (dump)");

"dump available" is probably a bit clearer.
> 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");

I'd drop these (4) lines here (maybe add them in the manpage).

Uwe.

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

[Robinson Tryon]

On Jan 12, 2008 8:57 PM, Uwe Hermann  wrote:
>

...
> > 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?
>
> Not all entries have the same length. Using the EOT method is nice, IMO.

Okay, cool.
Is it common for C programmers to terminate arrays like this?  (I
haven't written much C code, so I'm just wondering for my own
edification... :-)
>

Here's a new patch.  I believe that I addressed everything that Uwe
brought up, including adding an entry in the superiotool manpage.  Let
me know how it looks.
(I noticed that there are a few references to "LinuxBIOS" in the
manpage, etc... that need to be changed to "coreboot"; I'll do that in
a separate patch)

Signed-off-by: Robinson P. Tryon

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

[Robinson Tryon]

On Jan 13, 2008 6:46 AM, Robinson Tryon  wrote:
>

...
> Here's a new patch.  I believe that I addressed everything that Uwe
> brought up, including adding an entry in the superiotool manpage.  Let
> me know how it looks.
> (I noticed that there are a few references to "LinuxBIOS" in the
> manpage, etc... that need to be changed to "coreboot"; I'll do that in
> a separate patch)
>
> Signed-off-by: Robinson P. Tryon

Hi,
Are there any more comments on my patch to implement the
--list-supported feature for superiotool?  If not, could someone go
ahead and commit the patch?

Thanks,
-- Robinson

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

[Uwe Hermann]

On Tue, Jan 15, 2008 at 02:43:52PM -0500, Robinson Tryon wrote:
> Are there any more comments on my patch to implement the
> --list-supported feature for superiotool?  If not, could someone go
> ahead and commit the patch?

Yeah, looks good mostly. Comitted in r91 with some minor changes:

- Coding style fixes and cosmetics.
 - Only add copyright line in superiotool.[ch], the other changes are
   one-liners and likely too trivial.
 - Minor code simplifications (e.g. drop print_chip()).
 - A bit less verbose manpage entry.
 - Missing update of USAGE #define.

Thanks, Uwe.

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

[Uwe Hermann]

On Tue, Jan 15, 2008 at 11:33:45PM +0100, Uwe Hermann wrote:
> On Tue, Jan 15, 2008 at 02:43:52PM -0500, Robinson Tryon wrote:
> > Are there any more comments on my patch to implement the
> > --list-supported feature for superiotool?  If not, could someone go
> > ahead and commit the patch?
>
> Yeah, looks good mostly. Comitted in r91 with some minor changes:

That's r3050 of course, not r91.

Uwe.