lkml.org 
[lkml]   [2017]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5] fpga manager: Add Altera CvP driver
On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <agust@denx.de> 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

\
 
 \ /
  Last update: 2017-06-12 01:17    [W:0.059 / U:2.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site