RE: [PATCH 03/16] fpga: intel: add FPGA PCIe device driver

["Wu, Hao" <hao.wu@intel.com>]

> > +#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