[coreboot] libpayload: Geode video console support

[Uwe Hermann]

On Thu, Apr 10, 2008 at 09:51:07AM -0600, Jordan Crouse wrote:
> > > + * (http://www.helenos.eu)
> > 
> > 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.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org