[LinuxBIOS] [PATCH] flashrom: board specific enables based on matching pci-ids.
Luc Verhaegen
libv at skynet.be
Mon Apr 2 00:43:19 CEST 2007
On Mon, Apr 02, 2007 at 12:36:28AM +0200, Uwe Hermann wrote:
> Here's a quick review:
>
> On Mon, Mar 26, 2007 at 04:59:18PM +0200, Luc Verhaegen wrote:
> > been relegated to flash_rom.c. Then there's only some extraneous
> > whitespace removal and replacing // with /* */. I'm not sure how svn
> > handles moving of files, but that's usually a good point to do such
> > function-less changes.
>
> Nope, please don't. I suggest to split this up in multiple
> patches/steps. First, split flash_enable.c into board_enable.c and
> chipset_enable.c without _any_ changes in content (only Makefile fixes
> and related adaptions).
>
> Then do the other code changes (with no random cosmetics such as changing
> comment style in the patch), then as a last step change any cosmetics
> you don't like...
>
> This way all patches only contain related changes and are easily
> readable an reviewable.
>
>
> > * Fixes GPIO15 being raised on every VT8235 southbridge, regardless of what
> > that line actually controls; rom on EPIA-M, backlight on mitac 8999 laptop.
>
> Please sumbit an extra patch for this please, it's not related to the
> rest AFAICS.
>
>
> > * Adds flashrom support for Asus A7V400-MX (KM400 + VT8235)
>
> Ditto. In general, please send rather multiple smaller patches than one
> huge patch, that's a lot easier to review and merge. Don't mix unrelated
> changes into one huge patch.
>
>
> > signed-off-by: Luc Verhaegen <libv at skynet.be>
>
> Capital 'S'.
>
>
> > - rm -f *.o *~
> > + rm -f *.o *~ flashrom
>
> Why? 'make distclean' delete?? the binary already.
>
>
> > distclean: clean
> > rm -f $(PROGRAM) .dependencies
>
>
>
> > +struct pci_dev *
> > +pci_dev_find(uint16_t vendor, uint16_t device)
> > +{
> > + struct pci_dev *temp;
> > + struct pci_filter filter;
> > +
> > + pci_filter_init(NULL, &filter);
> > + filter.vendor = vendor;
> > + filter.device = device;
> > +
> > + for (temp = pacc->devices; temp; temp = temp->next)
> > + if (pci_filter_match(&filter, temp))
> > + return temp;
> > +
> > + return NULL;
> > +}
>
> Please use tabs for indenting (yes, much of the current LinuxBIOS code
> needs fixing too, but we should properly indent _new_ code at least).
>
> See also:
> http://linuxbios.org/Development_Guidelines#Coding_Style
>
>
> Uwe.
Ok, will do when i have some time again.
Luc Verhaegen.
More information about the coreboot
mailing list