> > +#include <linux/pci.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/stddef.h>
> > +#include <linux/errno.h>
> > +#include <linux/aer.h>
> > +
> > +#define DRV_VERSION "EXPERIMENTAL VERSION"
>
> Is that a leftover? :)
Sorry, will fix this.
> > +#define DRV_NAME "intel-fpga-pci"
> > +
> > +/* PCI Device ID */
> > +#define PCIe_DEVICE_ID_PF_INT_5_X 0xBCBD
> > +#define PCIe_DEVICE_ID_PF_INT_6_X 0xBCC0
> > +#define PCIe_DEVICE_ID_PF_DSC_1_X 0x09C4
> > +/* VF Device */
> > +#define PCIe_DEVICE_ID_VF_INT_5_X 0xBCBF
> > +#define PCIe_DEVICE_ID_VF_INT_6_X 0xBCC1
> > +#define PCIe_DEVICE_ID_VF_DSC_1_X 0x09C5
> > +
> > +static struct pci_device_id cci_pcie_id_tbl[] = {
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_INT_5_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_INT_5_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_INT_6_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_INT_6_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_DSC_1_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_DSC_1_X),},
> > + {0,}
> > +};
> > +MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
> > +
> > +static
> > +int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id
> > *pcidevid)
> > +{
> > + int ret;
> > +
> > + ret = pci_enable_device(pcidev);
> > + if (ret < 0) {
> > + dev_err(&pcidev->dev, "Failed to enable device %d.\n", ret);
> > + goto exit;
> Why not 'return ret' here ?
Yes, you are right, will fix this.
> > + }
> > +
> > + ret = pci_enable_pcie_error_reporting(pcidev);
> > + if (ret && ret != -EINVAL)
> > + dev_info(&pcidev->dev, "PCIE AER unavailable %d.\n", ret);
>
> What if it is EINVAL?
pci_enable_pcie_error_reporting is always return -EINVAL when CONFIG_PCIEAER is
not selected.
Then we don't need this boring message. : )
>
> > +
> > + ret = pci_request_regions(pcidev, DRV_NAME);
> > + if (ret) {
> > + dev_err(&pcidev->dev, "Failed to request regions.\n");
> > + goto disable_error_report_exit;
> > + }
> > +
> > + pci_set_master(pcidev);
> > + pci_save_state(pcidev);
> > +
> > + if (!dma_set_mask(&pcidev->dev, DMA_BIT_MASK(64))) {
> > + dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(64));
> > + } else if (!dma_set_mask(&pcidev->dev, DMA_BIT_MASK(32))) {
> > + dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(32));
> > + } else {
> > + ret = -EIO;
> > + dev_err(&pcidev->dev, "No suitable DMA support available.\n");
> > + goto release_region_exit;
> > + }
> > +
> > + /* TODO: create and add the platform device per feature list */
> > + return 0;
> > +
> > +release_region_exit:
> > + pci_release_regions(pcidev);
> > +disable_error_report_exit:
> > + pci_disable_pcie_error_reporting(pcidev);
> > + pci_disable_device(pcidev);
> > +exit:
> > + return ret;
> If you return as suggested above, this can go away.
Yes, you are right. Will fix this in next version.
Thanks a lot for your review and comments. : )
Hao
