Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751213AbdFBRnZ (ORCPT ); Fri, 2 Jun 2017 13:43:25 -0400 Received: from mail-qk0-f195.google.com ([209.85.220.195]:33015 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751161AbdFBRnX (ORCPT ); Fri, 2 Jun 2017 13:43:23 -0400 MIME-Version: 1.0 In-Reply-To: <1494777082-13375-1-git-send-email-agust@denx.de> References: <1494777082-13375-1-git-send-email-agust@denx.de> From: Andy Shevchenko Date: Fri, 2 Jun 2017 20:43:21 +0300 Message-ID: Subject: Re: [PATCH v5] fpga manager: Add Altera CvP driver To: Anatolij Gustschin Cc: linux-fpga@vger.kernel.org, Alan Tull , Moritz Fischer , "linux-kernel@vger.kernel.org" , matthew.gerlach@linux.intel.com, yi1.li@linux.intel.com Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5286 Lines: 218 On Sun, May 14, 2017 at 6:51 PM, Anatolij Gustschin wrote: > Add FPGA manager driver for loading Arria-V/Cyclone-V/Stratix-V > and Arria-10 FPGAs via CvP. Few comments from me. > +struct altera_cvp_conf { > + struct fpga_manager *mgr; > + struct pci_dev *pci_dev; > + void __iomem *map; > + void (*write_data)(struct altera_cvp_conf *conf, > + u32 val); Is it too far beyond 80 characters? I would leave it in one line (~83 characters are okay). > + char mgr_name[64]; > + u8 numclks; > +}; > +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf) > +{ > + unsigned int i; > + u32 val32; Drop the useless suffix. > +} > +static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_msk, > + u32 status_val, int timeout_us) > +{ > + u32 val32; Ditto. > + if (!timeout_us) > + return -ETIMEDOUT; Hmm... What as a user I would expect here is at least one attempt (0 -- no timeout, but try once). > + > + do { > + /* use small usleep value to re-check and break early */ > + usleep_range(10, 11); > + > + pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32); > + if ((val32 & status_msk) == status_val) > + return 0; > + > + timeout_us -= 10; > + } while (timeout_us > 0); > + > + return -ETIMEDOUT; > +} > +static int altera_cvp_teardown(struct fpga_manager *mgr, > + struct fpga_image_info *info) > +{ > + u32 val32; Drop the suffix. > + return ret; > +} > +static int altera_cvp_write_init(struct fpga_manager *mgr, > + struct fpga_image_info *info, > + const char *buf, size_t count) > +{ > + u32 val32; Ditto. > + int ret; > + > + /* clock to data ratio for uncompressed and unencrypted images */ > + conf->numclks = 1; To else branch of below? > + if (info) { > + } > + ret = altera_cvp_teardown(mgr, info); > + if (ret < 0) > + return ret; What is the meaning of ret > 0? Can't it be just if (ret) here? > + ret = altera_cvp_wait_status(conf, VSEC_CVP_STATUS_CFG_RDY, > + VSEC_CVP_STATUS_CFG_RDY, 10); > + if (ret < 0) { > + dev_warn(&mgr->dev, "CFG_RDY == 1 timeout\n"); > + return ret; > + } Ditto. Also check another code like this above and below. > + return 0; > +} > +static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes) > +{ > + struct altera_cvp_conf *conf = mgr->priv; > + u32 val32; Suffix. > +} > +static int altera_cvp_write_complete(struct fpga_manager *mgr, > + struct fpga_image_info *info) > +{ > + u32 status_msk; status_mask > + u32 val32; Drop the suffix. > +} > +static ssize_t show_chkcfg(struct device_driver *dev, char *buf) > +{ > + return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0); Just altera_cvp_chkcfg. > +} > +static struct pci_device_id altera_cvp_id_tbl[] = { > + { PCI_VDEVICE(ALTERA, PCI_ANY_ID) }, Does it have dedicated PCI class? PCI_ANY_ID usually is too broad. > +static int altera_cvp_probe(struct pci_dev *pdev, > + const struct pci_device_id *dev_id) > +{ > + struct altera_cvp_conf *conf; > + u16 cmd, val16; Drop the suffix. > + /* > + * First check if this is the expected FPGA device. PCI config > + * space access works without enabling the pci device, memory pci -> PCI > + * space access is enabled further down. > + */ > + /* > + * Enable memory BAR access. We cannot use pci_enable_device() here > + * because it will make the driver unusable with FPGA devices that > + * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit iomem -> IOMEM > + * platform. Such BARs will not have an assigned address range and > + * pci_enable_device() will fail, complaining about not claimed BAR, > + * even if the concerned BAR is not needed for FPGA configuration > + * at all. Thus, enable the device via pci config space command. pci -> PCI > + */ > + ret = pci_request_region(pdev, CVP_BAR, "CVP"); > + if (ret < 0) { if (ret) > + dev_err(&pdev->dev, "Requesting CVP BAR region failed\n"); > + goto err; > + } > + snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02x:%02x.%d", > + ALTERA_CVP_MGR_NAME, pdev->bus->number, > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); pci_name() ? > +err_unmap: > + pci_iounmap(pdev, conf->map); > + pci_release_region(pdev, CVP_BAR); > +err: err_disable: > + cmd &= ~PCI_COMMAND_MEMORY; > + pci_write_config_word(pdev, PCI_COMMAND, cmd); > + return ret; > +} -- With Best Regards, Andy Shevchenko