Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933137AbdDENPD (ORCPT ); Wed, 5 Apr 2017 09:15:03 -0400 Received: from mga14.intel.com ([192.55.52.115]:32967 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755218AbdDENOu (ORCPT ); Wed, 5 Apr 2017 09:14:50 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,278,1486454400"; d="scan'208";a="83455045" From: "Wu, Hao" To: Moritz Fischer CC: "atull@kernel.org" , "moritz.fischer@ettus.com" , "linux-fpga@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Kang, Luwei" , "Zhang, Yi Z" , "Whisonant, Tim" , "Luebbers, Enno" , "Rao, Shiva" , "Rauer, Christopher" , Xiao Guangrong , "Wu, Hao" Subject: RE: [PATCH 03/16] fpga: intel: add FPGA PCIe device driver Thread-Topic: [PATCH 03/16] fpga: intel: add FPGA PCIe device driver Thread-Index: AQHSrg6YUkLBziTG3kuovr0QxksDzg== Date: Wed, 5 Apr 2017 13:14:46 +0000 Message-ID: References: <1490875696-15145-1-git-send-email-hao.wu@intel.com> <1490875696-15145-4-git-send-email-hao.wu@intel.com> <20170404021028.GA8866@tyrael.amer.corp.natinst.com> In-Reply-To: <20170404021028.GA8866@tyrael.amer.corp.natinst.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v35DF7Zo006103 Content-Length: 3013 Lines: 97 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#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