On 10/04/08 02:48 +0200, Uwe Hermann wrote:
/* Clear the screen and kill the cursor */Should end with full stop.
We need to have a serious discussion about this. I realize that you prefer a period after all statements - but many of us don't. Thats okay - the style guide doesn't care one way or the other. You shouldn't point them out, and especially not as a reason to reject a patch (I know that wasn't your intent for this patch, since there were other flaws, but still - it was almost the first thing you mentioned).
Its okay to be strict about coding style - there is nothing worse then a poorly styled project. But its another thing to be too aggressive, especially with personal quirks that half the developers will do one way and the other half will do the other way.
Don't get me wrong, I really appreciate your comments, but it can be frustrating when you send out a patch of somewhat complex code and all you get back are comments about the coding style. Did you not have any concerns at all about how the code worked?
All of us have to be better about critically analyzing the code that goes by us. Ron and Stefan found a bug in the PCI system that has been there for nearly 8 years, and had been ported forward into v3. Yes, bugs can get through, but we need to be as diligent as our Acked-By lines say we are. Well formatted source code is nothing if it doesn't work.
Index: libpayload/drivers/pci.c
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ libpayload/drivers/pci.c 2008-04-01 17:11:41.000000000 -0600 @@ -0,0 +1,90 @@
[...]
+#include <libpayload.h> +#include <pci.h>
+static int find_on_bus(int bus, unsigned short vid, unsigned short did,
Why static? Might be useful elsewhere?
Its an internal recursive function used by pci_find_device. pci_find_device is the real useful function, and it is part of the external API.
pcidev_t *dev)+{
- int devfn;
- unsigned int val;
- unsigned char hdr;
I'd prefer to change all of these to u16 et. al., but not required for the patch to go in, this can be done later.
Okay - we can debate this. The value of unsigned int and unsigned char are guaranteed, and my editor nicely highlights them for me, but if we prefer to use typedefs, then thats fine.
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.
+#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.
+#ifdef HAVE_CONSOLE_FONT +#error "You have already defined a console font!"
Shouldn't kconfig take care that invalid configs can happen?
We'll see - for now, the developer can chose which fontset they want to use (even though we only have one). I'm not sure if we are going to eventually get to the point where the builder would chose a fontset through the Kconfig. Lets just leave this for now.
+#include <libpayload.h> +#include <pci.h>
I propose to #include <pci.h> in libpayload.h, there's not much use in complicating things everywhere in the code. IMHO just including libpayload.h only is perfectly fine.
I'd also add <curses.h> in libpayload.h so a payload writer doesn't have to add it explicitly. The _only_ header a payload writer should ever have to worry about is libpayload.h IMO.
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.
+#include <video_console.h> +#include <arch/io.h> +#include <arch/msr.h> +#include "font8x16.h"
Drop? Should all be in libpayload.h.
Again, same problem. I might be okay with <arch/io.h>, though I don't really buy into the idea of automatically including architecture specific code. <arch/msr.h> is only x86 specific, and so of course should not be included automatically.
Like I said, libpayload.h is the external API for the library. video_console.h and the font8x16.h are internal header files, they would never be used by the payload writer, so they should remain separate.
+/* Addresses for the various components */
+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?
- 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.
+#include <libpayload.h>
+#include <video_console.h>
Merge in libpayload.h.
See above.
+static inline uint8_t crtc_read(uint8_t index)
u8
Is the inline needed or useful? I guess gcc can figure that out by itself.
Yes. We need to remove all inlines.
All in all the patch looks quite nice. Please repost, but splitted in multiple patches. I'd say one which only moves files around and one which only adds files (while keeping libpayload in a compiling state). The final patch should do all other random modifications.
I can realistically divide it into two patches, one for the vga / video console infrastructure, and one for Geode. I will do that.
Jordan