Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753315AbdFGXi7 (ORCPT ); Wed, 7 Jun 2017 19:38:59 -0400 Received: from mail-qt0-f193.google.com ([209.85.216.193]:35031 "EHLO mail-qt0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752648AbdFGXi4 (ORCPT ); Wed, 7 Jun 2017 19:38:56 -0400 MIME-Version: 1.0 In-Reply-To: <20170608010928.3effdf38@crub> References: <1494777082-13375-1-git-send-email-agust@denx.de> <20170608010928.3effdf38@crub> From: Andy Shevchenko Date: Thu, 8 Jun 2017 02:38:55 +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: 2125 Lines: 76 On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin wrote: > On Fri, 2 Jun 2017 20:43:21 +0300 > Andy Shevchenko andy.shevchenko@gmail.com wrote: Besides below comments, please, do s/VSEC_/VSE_/g for entire file. We are following PCI and Thunderbolt pattern for use of Vendor Specific Extended Capability. >>> + 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). > > yes, this first attempt is above, please see original patch for full > context. Ah, it means you don't correctly use do {} while approach. Remove everything above do { and move usleep after check for the status inside the loop. >>> + /* clock to data ratio for uncompressed and unencrypted images */ >>> + conf->numclks = 1; >> >>To else branch of below? >> >>> + if (info) { >> >>> + } > > then, the default numclks value would have to be set in the if branch > as well. It is easier to set it before. See the magic below: u32 iflags = info ? info->flags : 0; ... if (iflags & FPGA_MGR_PARTIAL_RECONFIG) { dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); return -EINVAL; } if (iflags & FPGA_MGR_COMPRESSED_BITSTREAM) conf->numclks = 8; /* ratio for all compressed images */ else if (iflags & FPGA_MGR_ENCRYPTED_BITSTREAM) conf->numclks = 4; /* ratio for encrypted images */ else conf->numclks = 1; /* clock to data ratio for uncompressed and unencrypted images */ I would really recommend to double check the code every time you are about to send. A little thought can make a beauteful result! >>> +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. > > no, it doesn't have dedicated class. Hmm... It means any device of this vendor will jump into this driver... Not good. -- With Best Regards, Andy Shevchenko