On Fri, Apr 04, 2008 at 04:03:40PM -0600, Jordan Crouse wrote:
+config VGA_VIDEO_CONSOLE
- bool "VGA video console driver"
- depends VIDEO_CONSOLE
Please use "depends on". This may no be required yet, but newer versions of kconfig killed 'depends' and only allow 'depends on', I think.
Index: libpayload/curses/tinycurses.c
--- libpayload.orig/curses/tinycurses.c 2008-04-01 16:32:52.000000000 -0600 +++ libpayload/curses/tinycurses.c 2008-04-01 17:11:41.000000000 -0600 @@ -219,9 +219,11 @@ // def_prog_mode();
if (curses_flags & F_ENABLE_CONSOLE) {
/* Clear the screen and kill the cursor. */vga_clear();vga_cursor_enable(0);
/* Clear the screen and kill the cursor */
Should end with full stop.
video_console_clear();video_console_cursor_enable(0);
Indentation is broken.
Index: libpayload/drivers/Makefile.inc
[...]
+TARGETS-$(CONFIG_VIDEO_CONSOLE) += drivers/video/video.o +TARGETS-$(CONFIG_VGA_VIDEO_CONSOLE) += drivers/video/vga.o
+TARGETS-$(CONFIG_GEODE_VIDEO_CONSOLE) += drivers/video/geode.o drivers/video/font8x16.o
Longer than 80 chars per line.
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?
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.
- for(devfn = 0; devfn < 0x100; devfn++) {
pci_read_dword(bus, devfn, REG_VENDOR_ID, &val);if (val == 0xffffffff || val == 0x00000000 ||val == 0x0000ffff || val == 0xffff0000)continue;if (val == ((did << 16) | vid)) {*dev = PCIDEV(bus, devfn); return 1;}
Run this file through indent please, various parts have broken coding style.
Index: libpayload/drivers/video/font8x16.c
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ libpayload/drivers/video/font8x16.c 2008-04-04 15:53:02.000000000 -0600 @@ -0,0 +1,4646 @@ +/*
- This file is part of libpayload
Missing full stop.
- It has originally been taken from the HelenOS project
Ditto.
I'd drop this, and put in into LICENSING (same for other external files).
Index: libpayload/drivers/video/font8x16.h
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ libpayload/drivers/video/font8x16.h 2008-04-01 17:11:41.000000000 -0600 @@ -0,0 +1,45 @@
[...]
+#ifndef FONT_8X16_H_
s/TAB/space/ here.
+#define FONT_8X16_H_
We should decide for an include guard format and use it consistently everwhere. I propose FONT_8X16_H, as per v3 conventions (i.e. no prefix/postfix underscores).
+#ifdef HAVE_CONSOLE_FONT +#error "You have already defined a console font!"
Shouldn't kconfig take care that invalid configs can happen?
Index: libpayload/drivers/video/geode.c
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ libpayload/drivers/video/geode.c 2008-04-01 17:11:41.000000000 -0600
[...]
+#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.
+#include <video_console.h> +#include <arch/io.h> +#include <arch/msr.h> +#include "font8x16.h"
Drop? Should all be in libpayload.h.
+/* This is the video mode that we're going to use for our VGA screen */
+static const struct mode {
Drop "mode"? It's not used anywhere AFAICS.
+/* 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.
+static void geode_putc(uint8_t row, uint8_t col, unsigned int ch)
uint8_t -> u8. Let's only use one form consistently, the short one.
+static int geode_init(void) +{
- pcidev_t dev;
- int i;
- if (!pci_find_device(0x1022, 0x2081, &dev))
Maybe add #defines for these in pci.h? No need for an extra pci_id.h though...
return -1;- fbaddr = pci_read_resource(dev, 0);
- gpaddr = pci_read_resource(dev, 1);
- dcaddr = pci_read_resource(dev, 2);
- vgaddr = pci_read_resource(dev, 3);
- init_video_mode();
- /* Set up the palette */
- for(i = 0; i < 16; i++) {
16 -> ARRAY_SIZE(vga_colors). Just for readability.
geode_set_palette(i, vga_colors[i]);- }
- return 0;
+}
+struct video_console_t geode_video_console = {
video_console_t -> video_console, the _t makes it look like it's a typedef, which it isn't (and shouldn't be).
- .init = geode_init,
- .putc = geode_putc,
- .clear = geode_clear,
- .scroll_up = geode_scroll_up
+}; Index: libpayload/drivers/video/vga.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ libpayload/drivers/video/vga.c 2008-04-01 17:11:41.000000000 -0600 @@ -0,0 +1,141 @@
[...]
+#include <arch/types.h>
Drop, we alread have <libpayload.h>.
+#include <libpayload.h>
+#include <video_console.h>
Merge in libpayload.h.
+#define VGA_COLOR_WHITE 7
+#define CRTC_INDEX 0x3d4 +#define CRTC_DATA 0x3d5
+#define VIDEO(_r, _c)\
- ((uint16_t *) (0xB8000 + ((_r) * (VIDEO_COLS * 2)) + ((_c) * 2)))
u16
+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.
Index: libpayload/i386/timer.c
--- libpayload.orig/i386/timer.c 2008-04-01 16:32:53.000000000 -0600 +++ libpayload/i386/timer.c 2008-04-01 17:11:41.000000000 -0600 @@ -74,7 +74,12 @@
void ndelay(unsigned int n) {
- _delay(n * cpu_khz / 1000000);
- _delay(n * cpu_khz / 1000000);
Whitespace breakage.
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.
Cheers, Uwe.