[flashrom] [PATCH 08/12] simplify ich_set_bbar

[Stefan Tauner]

On Mon, 13 Jun 2011 22:09:35 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 13.06.2011 20:31 schrieb Stefan Tauner:
> > On Mon, 13 Jun 2011 19:24:55 +0200
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >
> >   
> >> Am 08.06.2011 04:55 schrieb Stefan Tauner:
> >>     
> >>> ~10 lines less, documenting better what the differences are (i.e. offset of BBAR only).
> >>>
> >>> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> >>>   
> >>>       
> >> Nice cleanup!
> >> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> >>     
> > ahem! :)
> >
> > because of idwer's remark that "msg_pdbg("Reserved bits in BBAR not
> > zero: 0x%04x"..." should get a \n (thx!), i reviewed that patch again
> > and noticed that it is quite broken.
> >
> > in the original code the real bbar is read from the register and
> > checked for reserved bits before we try to set the requested min
> > address. i somehow missed that, sorry. :/
> >   
> 
> Ouch. I overlooked that during review. The patch looked so simple.
famous last words ;)

> > the new, attached patch has following changes:
> >  - check for reserved bits
> >  - output 8 hex characters instead of 4 (i.e. 32b instead of 16b)
> >  - add \n to the message above
> >   
> 
> Since you're already changing the function prototype of ich_set_bbar,
> can you make it static?
> 
> And another point about the coding style:
> Can you use int bbar_offs instead of void *bbar_addr?
> That way, it is easier to check directly which physmap region the
> accesses correspond to.

like so?

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-simplify-ich_set_bbar.patch
Type: text/x-patch
Size: 2799 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110613/bb370508/attachment.patch>