Messages in this thread | | | From | "Wu, Hao" <> | Subject | RE: [PATCH 03/16] fpga: intel: add FPGA PCIe device driver | Date | Wed, 5 Apr 2017 13:14:46 +0000 |
| |
> > +#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
| |