[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240613184514.GA1071919@bhelgaas>
Date: Thu, 13 Jun 2024 13:45:14 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Philipp Stanner <pstanner@...hat.com>,
Hans de Goede <hdegoede@...hat.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Sam Ravnborg <sam@...nborg.org>, dakr@...hat.com,
dri-devel@...ts.freedesktop.org,
LKML <linux-kernel@...r.kernel.org>, linux-pci@...r.kernel.org
Subject: Re: [PATCH v7 03/13] PCI: Reimplement plural devres functions
On Thu, Jun 13, 2024 at 07:54:38PM +0300, Ilpo Järvinen wrote:
> On Wed, 5 Jun 2024, Philipp Stanner wrote:
>
> > When the original PCI devres API was implemented, priority was given to
> > the creation of a set of "plural functions" such as
> > pcim_request_regions(). These functions have bit masks as parameters to
> > specify which BARs shall get mapped. Most users, however, only use those
> > to map 1-3 BARs.
> > +static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
> > + unsigned long offset, unsigned long maxlen,
> > + const char *name, int req_flags)
> > +{
> > + resource_size_t start = pci_resource_start(pdev, bar);
> > + resource_size_t len = pci_resource_len(pdev, bar);
> > + unsigned long dev_flags = pci_resource_flags(pdev, bar);
> > +
> > + if (start == 0 || len == 0) /* That's an unused BAR. */
> > + return 0;
> > + if (len <= offset)
> > + return -EINVAL;
>
> Extra space.
Thanks for reviewing this. I dropped the space locally in the v9
series.
> > void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
> > {
> > - void __iomem * const *iomap;
> > - int i;
> > -
> > - iomap = pcim_iomap_table(pdev);
> > - if (!iomap)
> > - return;
> > + short bar;
>
> The current best practice is to use unsigned for loop vars that will never
> be negative.
>
> I don't entirely follow what is reasoning behind making it short instead
> of unsigned int?
Existing interfaces like pcim_iomap() take "int bar". I locally
changed all the BAR indices to "int". We can make everything unsigned
later if worthwhile.
> > - for (i = 0; i < PCIM_IOMAP_MAX; i++) {
> > - if (!mask_contains_bar(mask, i))
> > + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
>
> Is this change minimal if it contains variable renames like this?
> Was "i" not "bar" even if it was given as a parameter to
> mask_contains_bar()?
Replaced locally with "i".
Bjorn
Powered by blists - more mailing lists