[LinuxBIOS] [PATCH] flashrom: board specific enables based on matching pci-ids.

Uwe Hermann 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
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.
-- 
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...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070402/378a3893/attachment.sig>


More information about the coreboot mailing list