[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