[LinuxBIOS] [PATCH] flashrom: board specific enables based on matching pci-ids.
uwe at hermann-uwe.de
Mon Apr 2 00:36:28 CEST 2007
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
> * 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>
> - 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).
http://www.hermann-uwe.de | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 189 bytes
Desc: Digital signature
More information about the coreboot