Re: [flashrom] Improving pcidev.c

[Stefan Tauner]

On Sat, 05 Jan 2013 18:56:33 +0100
Carl-Daniel Hailfinger  wrote:
> Am 05.01.2013 03:46 schrieb Carl-Daniel Hailfinger:
> > Am 01.01.2013 19:46 schrieb Carl-Daniel Hailfinger:
> > > > Am 04.09.2012 12:14 schrieb Stefan Tauner:
> > > > > > On Tue, 31 Jul 2012 09:27:47 +0200
> > > > > > Carl-Daniel Hailfinger  wrote:
> > > > > >
> > > > > > >> Ping?
> > > > > > >>
> > > > > > >> IMHO this patch fixes a few structural problems, and although it
> > > > > > >> probably isn't the final desired result, it is a step in the right
> > > > > > >> direction.
> > > > > > After my short adventure into trying to understand the PCI code while
> > > > > > refining the atavia patch by Jonathan, i have to agree 100% with
> > > > > > Michael's first mail regarding the old code base.
> > > > > >
> > > > > > Something that hasn't been mentioned (or i have missed) is that there
> > > > > > are some drivers that don't need a BAR at all because they work by
> > > > > > accessing the configuration space only. atavia is one case (at least i
> > > > > > think so). and satasii is even more special as it needs different BARs
> > > > > > for different PCI IDs, but you are aware of that afaics.
> >
> > Yes, there is still quite a bit of work ahead.
> >
> > That said, your PCI init cleanup patch had a tremendous effect on code
> > readability. Thanks for creating that patch.
> > > > > > I still don't have a complete overview on how all pcidev_init
> > > > > > callers work, but the current patch seems to improve things and hence
> > > > > > should go in ASAP (you can read that as an ACK if you think it is safe).
> >
> > This patch has changed quite a bit since then... it would be great if
> > you could take another look.
> > > > > > Since you touch all pcidev_init calls in this patch, it would be great
> > > > > > if you could switch the parameters though. The PCI dev table should be
> > > > > > first since it is the most important argument and the bar should IMHO
> > > > > > even be optional in the future or some kind of flag/mask as you
> > > > > > discussed... that argument apparently confused not only me but also
> > > > > > Jonathan and Idwer in the past.
> >
> > Very good point. I have changed the argument order, and it really looks
> > nicer and more consistent
> > > > > > A comment explaining the parameters would certainly improve the
> > > > > > situation too (e.g. mention that the bar parameter is not the number of
> > > > > > the BAR!).
> >
> > Hm yes... to be honest, I want to get rid of the bar parameter in a
> > followup patch, and supply a validator function instead. That one can
> > handle everything we might ever need in that direction.
>
> I have a followup patch which adds a validator function and should
> address all remaining comments raised during the various reviews.
>
> Updated patch: Fix a compile failure with disabled internal programmer.
> We were using a struct before declaring it first, so comapred to the
> previous patch this is only some code rearranging in programmer.h.
>
> Signed-off-by: Carl-Daniel Hailfinger

looks good enough to me to commit it ;)
Acked-by: Stefan Tauner