On Saturday, September 27, 2008 1:27 am Zhao, Yu wrote:
> Centralize capability related functions into several new functions and put
> PCI resource definitions into an enum.
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index f99160d..f2feebc 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
The sysfs changes look fine, they should be submitted separately.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 259eaff..400d3b3 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -356,25 +356,10 @@ pci_find_parent_resource(const struct pci_dev *dev,
> struct resource *res) static void
> pci_restore_bars(struct pci_dev *dev)
> {
> - int i, numres;
> -
> - switch (dev->hdr_type) {
> - case PCI_HEADER_TYPE_NORMAL:
> - numres = 6;
> - break;
> - case PCI_HEADER_TYPE_BRIDGE:
> - numres = 2;
> - break;
> - case PCI_HEADER_TYPE_CARDBUS:
> - numres = 1;
> - break;
> - default:
> - /* Should never get here, but just in case... */
> - return;
> - }
> + int i;
>
> - for (i = 0; i < numres; i ++)
> - pci_update_resource(dev, &dev->resource[i], i);
> + for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
> + pci_update_resource(dev, i);
> }
This confused me for a minute until I saw that the new pci_update_resource
ignores invalid BAR numbers. Not sure if that's clearer than the current
code...
> +/**
> + * pci_resource_bar - get position of the BAR associated with a resource
> + * @dev: the PCI device
> + * @resno: the resource number
> + * @type: the BAR type to be filled in
> + *
> + * Returns BAR position in config space, or 0 if the BAR is invalid.
> + */
> +int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type
> *type) +{
> + if (resno < PCI_ROM_RESOURCE) {
> + *type = pci_bar_unknown;
> + return PCI_BASE_ADDRESS_0 + 4 * resno;
> + } else if (resno == PCI_ROM_RESOURCE) {
> + *type = pci_bar_rom;
> + return dev->rom_base_reg;
> + }
> +
> + dev_err(&dev->dev, "BAR: invalid resource #%d\n", resno);
> + return 0;
> +}
It looks like this will spew an error even under normal circumstances since
pci_restore_bars gets called at resume time, right? You could make this into
a debug message or just get rid of it. Also now that I look at this, I don't
think it'll provide equivalent functionality to the old restore_bars code,
won't a cardbus bridge end up getting pci_update_resource called on invalid
BARs?
> +static void pci_init_capabilities(struct pci_dev *dev)
> +{
> + /* MSI/MSI-X list */
> + pci_msi_init_pci_dev(dev);
> +
> + /* Power Management */
> + pci_pm_init(dev);
> +
> + /* Vital Product Data */
> + pci_vpd_pci22_init(dev);
> +}
> +
These capabilities changes look good, care to separate them out?
Let's see if we can whittle down this patchset by extracting and applying all
the various cleanups; that should make the core bits easier to review.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
