On Thu, Mar 23, 2023 at 04:30:01PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 22, 2023 at 02:28:04PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 20, 2023 at 03:16:30PM +0200, Andy Shevchenko wrote:
> ...
>
> > > + pci_dev_for_each_resource_p(dev, r) {
> > > /* zap the 2nd function of the winbond chip */
> > > - if (dev->resource[i].flags & IORESOURCE_IO
> > > - && dev->bus->number == 0 && dev->devfn == 0x81)
> > > - dev->resource[i].flags &= ~IORESOURCE_IO;
> > > - if (dev->resource[i].start == 0 && dev->resource[i].end) {
> > > - dev->resource[i].flags = 0;
> > > - dev->resource[i].end = 0;
> > > + if (dev->bus->number == 0 && dev->devfn == 0x81 &&
> > > + r->flags & IORESOURCE_IO)
> >
> > This is a nice literal conversion, but it's kind of lame to test
> > bus->number and devfn *inside* the loop here, since they can't change
> > inside the loop.
>
> Hmm... why are you asking me, even if I may agree on that? It's
> in the original code and out of scope of this series.
Yeah, I don't think it would be *unreasonable* to clean this up at the
same time so the maintainers can look at both at the same time (this
is arch/powerpc/platforms/pseries/pci.c, so Michael, et al), but no
need for you to do anything, certainly. I can post a follow-up patch.
> > but
> > since we're converging on the "(dev, res)" style, I think we should
> > reverse the names so we have something like:
> >
> > pci_dev_for_each_resource(dev, res)
> > pci_dev_for_each_resource_idx(dev, res, i)
>
> Wouldn't it be more churn, including pci_bus_for_each_resource() correction?
Yes, it definitely is a little more churn because we already have
pci_bus_for_each_resource() that would have to be changed.
I poked around looking for similar patterns elsewhere with:
git grep "#define.*for_each_.*_p("
git grep "#define.*for_each_.*_idx("
I didn't find any other "_p" iterators and just a few "_idx" ones, so
my hope is to follow what little precedent there is, as well as
converge on the basic "*_for_each_resource()" iterators and remove the
"_idx()" versions over time by doing things like the
pci_claim_resource() change.
What do you think? If it seems like excessive churn, we can do it
as-is and still try to reduce the use of the index variable over time.
Bjorn
