Date: Thu, 8 Jun 2017 01:09:28 +0200
From: Anatolij Gustschin <agust@denx.de>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-fpga@vger.kernel.org, Alan Tull <atull@kernel.org>,
        Moritz Fischer <moritz.fischer@ettus.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        matthew.gerlach@linux.intel.com, yi1.li@linux.intel.com
Subject: Re: [PATCH v5] fpga manager: Add Altera CvP driver
Message-ID: <20170608010928.3effdf38@crub>
In-Reply-To: <CAHp75VfxGOw-G1+Cqk75eVK6EoLeRDemRc2zif1aJc2o+1fzMQ@mail.gmail.com>
References: <1494777082-13375-1-git-send-email-agust@denx.de>
        <CAHp75VfxGOw-G1+Cqk75eVK6EoLeRDemRc2zif1aJc2o+1fzMQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3198
Lines: 157

On Fri, 2 Jun 2017 20:43:21 +0300
Andy Shevchenko andy.shevchenko@gmail.com wrote:
...
>
>> +       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).

I changed to single line, (*write_data)(struct altera_cvp_conf *, u32);

...
>> +static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
>> +{
>> +       unsigned int i;
>> +       u32 val32;  
>
>Drop the useless suffix.

ok, done.

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

...
>> +       /* 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.

...
>> +               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?

return value > 0 is never used, here it can be if (ret).

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

ok, done.

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

done.

...
>> +       return snprintf(buf, 3, "%d\n", altera_cvp_chkcfg ? 1 : 0);  
>
>Just altera_cvp_chkcfg.

ok, done.

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

...
>> +       u16 cmd, val16;  
>
>Drop the suffix.

>> +        * space access works without enabling the pci device, memory  
>
>pci -> PCI

ok, done.

...
>> +        * have additional big iomem resources (e.g. 4GiB BARs) on 32-bit  
>
>iomem -> IOMEM

done.

...
>> +        * at all. Thus, enable the device via pci config space command.  
>
>pci -> PCI

ok.

...
>> +       ret = pci_request_region(pdev, CVP_BAR, "CVP");
>> +       if (ret < 0) {  
>
>if (ret)

ok, done.

...
>> +       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() ?

ok.

>> +err_unmap:
>> +       pci_iounmap(pdev, conf->map);
>> +       pci_release_region(pdev, CVP_BAR);  
>
>> +err:  
>
>err_disable:

done.

Thanks,
Anatolij