[flashrom] Improving pcidev.c

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Sat Jan 5 23:45:26 CET 2013


On Sat, 05 Jan 2013 18:56:33 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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 <c-d.hailfinger.devel.2006 at gmx.net> 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 <c-d.hailfinger.devel.2006 at gmx.net>

looks good enough to me to commit it ;)
Acked-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner




More information about the flashrom mailing list