On Thu, Apr 10, 2008 at 09:51:07AM -0600, Jordan Crouse wrote:
I'd drop this, and put in into LICENSING (same for other external files).
I'm worried about this - I'm sure people will do to us what we did to HelenOS, i.e. borrow code. I think we need to make sure the entire legacy of the code is in each of the files so that if somebody comes across it on Google code then the chain of ownership is maintained.
Full ACK, I only meant to move the URL into LICENSING. The sentence 'It has originally been taken from the HelenOS project.' should stay, of course. We also say "taken from OpenBSD" without mentioning the URL every time, LICENSING already has the exact URL to the svn file etc.
+#define FONT_8X16_H_
We should decide for an include guard format and use it consistently everywhere. I propose FONT_8X16_H, as per v3 conventions (i.e. no prefix/postfix underscores).
I prefer the kernel format of _NAME_H - though as you can see, I use both prefix and postfix underscores sort of interchangeably. I would be happy to standardize in a later patch, but I think we need either the prefix or the postfix to avoid potentially contaminating the namespace.
Ok, let's use _FOO_H consistently then.
I would rather keep them separate. libpayload.h is the main external API for the library. PCI in particular defines static functions and other bits that nearly all payload writers will not care about. Rather then have this implicitly brought in by libpayload.h, I would prefer that the developer be asked to explicitly include it.
OK, if it has implications for code size let's keep pci.h extra.
+static unsigned long dcaddr; +static unsigned long vgaddr; +static unsigned long gpaddr; +static unsigned long fbaddr;
Could all be in one line.
They could be, but why?
Just cosmetics, one line vs. four. Nevermind, either is fine.
- if (!pci_find_device(0x1022, 0x2081, &dev))
Maybe add #defines for these in pci.h? No need for an extra pci_id.h though...
Nah - we'll only use these values once. I can define them locally in the file, but pci.h doesn't need to know.
OK, having them locally is fine too.
Is the inline needed or useful? I guess gcc can figure that out by itself.
Yes. We need to remove all inlines.
OK.
Uwe.