On Thu, Nov 13, 2008 at 08:05:37PM +0200, Vesa J??skel?inen wrote:
>
> Why not hide the legacy comports if they are not there and we can auto
> detect that?
I went back and forth on this. I finally decided that COM1 to COM4 are
so firmly a part of the PC design I should leave them (note that if
GRUB_MACHINE_PCBIOS is not defined then these 4 I/O ports will be defined
whether or not they are there and I'll have to list them.). If there's
a strong preference I can drop them if the BIOS doesn't define an I/O
port for them.
>
> That actually brings us to other issue. Should be have some kind of HW
> dependant device path support that would be universal in grub?. Now if
> user unplugs in example USB serial converter then the order will change
> and can cause some problems.
Maybe. Seems like a little bit of over engineering right now.
>
> > --- include/grub/pci_serial_ids.h (revision 0)
> > +++ include/grub/pci_serial_ids.h (revision 0)
> > @@ -0,0 +1,642 @@
> > +/*
> > + * GRUB -- GRand Unified Bootloader
> > + * Copyright (C) 2000,2001,2002,2003,2004,2005,2007 Free Software
> > Foundation, Inc.
>
> This seems to be new file. Please change copyright years to start from 2008.
>
> Where did you get that device table?. Generated by yourself from pci
> database?
>
> Anyway. I think those vendor and device ID's should be in global file as
> they are "universal" to any PCI device.
It is now. My understanding is that a data table is not copyrightable
(no expressive content) but, just to make sure there is no issue, I've
replaced this with a table of just the devices I know about. I'll
create (a very small) generic PCI ID table, we can add on as time goes
by.
>
> > --- term/i386/pc/serial.c (revision 1911)
> > +++ term/i386/pc/serial.c (working copy)
>
> > +#include <grub/pci.h>
> >
> > +void grub_serial_add(int type, unsigned int id, unsigned int base,
> > unsigned int port);
> > +#include <grub/pci_serial_ids.h>
> > +
>
> Please keep includes at same place.
>
> Why is this prototype even here?. And should it be static?
Since the mapping table code calls this function the prototype
needs to be defined before the include of `pci_serial_ids.h'.
I'll move the prototype to `serial.h' and clean these two up.
>
> > +char *serial_types[] = {
> > + "legacy",
> > + " pci",
> > + " usb"
> > +};
>
> static const
>
> And please leave output formatting to actual printing.
sure.
>
> > + grub_printf("%c%2d: %s ", (p == serial_dev) ? '*' : ' ',
> > + i, serial_types[p->type]);
>
> What is p->type is not within range of serial_types?
Then we have a more serious problem since `p->type' is only set
by the code to a value between 0 - 2. Given that this could only
be wrong because of a code bug it seems silly to value check it.
>
> > +char parity[] = {
>
> static const
>
> > +static int
> > +serial_pci_scan (int bus, int dev, int func, grub_pci_id_t pciid)
>
> > + vid = pciid & 0xffff;
> > + did = pciid >> 16;
> > + addr = grub_pci_make_address (bus, dev, func, 2);
> > + w = grub_pci_read (addr);
> > + class = (w >> 16) | ((w >> 8) & 0xff);
> > + addr = grub_pci_make_address (bus, dev, func, 11);
> > + w = grub_pci_read (addr);
> > + ss_vid = w & 0xffff;
> > + ss_did = w >> 16;
>
> This smells. Perhaps helper function or macro.
>
> As a generic note. If this is generic information that is available on
> every PCI device. Why not change PCI to iterate with structure.
>
> Well... I didn't look too deep there this time. :)
I was trying not to re-write the PCI code too much, it is way to
hard coded right now but my goal was to get the serial support in
first. I can certainly look into regularizing the PCI code a little
on the way.
Thanks for the comments, I'll get an updated patch out soon.
--
Don Dugger
"Censeo Toto nos in Kansa esse decisse." - D. Gale
[EMAIL PROTECTED]
Ph: 303/443-3786
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
